XCM coretime region transfers (#3455)

This PR introduces changes enabling the transfer of coretime regions via
XCM.

TL;DR: There are two primary issues that are resolved in this PR:

1. The `mint` and `burn` functions were not implemented for coretime
regions. These operations are essential for moving assets to and from
the XCM holding register.
2. The transfer of non-fungible assets through XCM was previously
disallowed. This was due to incorrectly benchmarking non-fungible asset
transfers via XCM, which led to assigning it a weight of `Weight::Max`,
effectively preventing its execution.

### `mint_into` and `burn` implementation

This PR addresses the issue with cross-chain transferring regions back
to the Coretime chain. Remote reserve transfers are performed by
withdrawing and depositing the asset to and from the holding registry.
This requires the asset to support burning and minting functionality.

This PR adds burning and minting; however, they work a bit differently
than usual so that the associated region record is not lost when
burning. Instead of removing all the data, burning will set the owner of
the region to `None`, and when minting it back, it will set it to an
actual value. So, when cross-chain transferring, withdrawing into the
registry will remove the region from its original owner, and when
depositing it from the registry, it will set its owner to another
account

This was originally implemented in this PR: #3455, however we decided to
move all of it to this single PR
(https://github.com/paritytech/polkadot-sdk/pull/3455#discussion_r1547324892)

### Fixes made in this PR

- Update the `XcmReserveTransferFilter` on coretime chain since it is
meant as a reserve chain for coretime regions.
- Update the XCM benchmark to use `AssetTransactor` instead of assuming
`pallet-balances` for fungible transfers.
- Update the XCM benchmark to properly measure weight consumption for
nonfungible reserve asset transfers. ATM reserve transfers via the
extrinsic do not work since the weight for it is set to `Weight::max()`.

Closes: https://github.com/paritytech/polkadot-sdk/issues/865

---------

Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>
Co-authored-by: Dónal Murray <donalm@seadanda.dev>

substrate/frame/broker/Cargo.toml

@@ -15,6 +15,7 @@ workspace = true
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
+log = { workspace = true }
 codec = { package = "parity-scale-codec", version = "3.0.0", default-features = false, features = ["derive"] }
 scale-info = { version = "2.11.1", default-features = false, features = ["derive"] }
 bitvec = { version = "1.0.0", default-features = false }
@@ -40,6 +41,7 @@ std = [
 	"frame-benchmarking?/std",
 	"frame-support/std",
 	"frame-system/std",
+	"log/std",
 	"scale-info/std",
 	"sp-api/std",
 	"sp-arithmetic/std",

substrate/frame/broker/src/benchmarking.rs

@@ -313,8 +313,8 @@ mod benches {
 		assert_last_event::<T>(
 			Event::Transferred {
 				region_id: region,
-				old_owner: caller,
-				owner: recipient,
+				old_owner: Some(caller),
+				owner: Some(recipient),
 				duration: 3u32.into(),
 			}
 			.into(),

substrate/frame/broker/src/dispatchable_impls.rs

@@ -120,7 +120,8 @@ impl<T: Config> Pallet<T> {
 			sale.sellout_price = Some(price);
 		}
 		SaleInfo::<T>::put(&sale);
-		let id = Self::issue(core, sale.region_begin, sale.region_end, who.clone(), Some(price));
+		let id =
+			Self::issue(core, sale.region_begin, sale.region_end, Some(who.clone()), Some(price));
 		let duration = sale.region_end.saturating_sub(sale.region_begin);
 		Self::deposit_event(Event::Purchased { who, region_id: id, price, duration });
 		Ok(id)
@@ -178,11 +179,11 @@ impl<T: Config> Pallet<T> {
 		let mut region = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
 
 		if let Some(check_owner) = maybe_check_owner {
-			ensure!(check_owner == region.owner, Error::<T>::NotOwner);
+			ensure!(Some(check_owner) == region.owner, Error::<T>::NotOwner);
 		}
 
 		let old_owner = region.owner;
-		region.owner = new_owner;
+		region.owner = Some(new_owner);
 		Regions::<T>::insert(&region_id, &region);
 		let duration = region.end.saturating_sub(region_id.begin);
 		Self::deposit_event(Event::Transferred {
@@ -203,7 +204,7 @@ impl<T: Config> Pallet<T> {
 		let mut region = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
 
 		if let Some(check_owner) = maybe_check_owner {
-			ensure!(check_owner == region.owner, Error::<T>::NotOwner);
+			ensure!(Some(check_owner) == region.owner, Error::<T>::NotOwner);
 		}
 		let pivot = region_id.begin.saturating_add(pivot_offset);
 		ensure!(pivot < region.end, Error::<T>::PivotTooLate);
@@ -227,7 +228,7 @@ impl<T: Config> Pallet<T> {
 		let region = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
 
 		if let Some(check_owner) = maybe_check_owner {
-			ensure!(check_owner == region.owner, Error::<T>::NotOwner);
+			ensure!(Some(check_owner) == region.owner, Error::<T>::NotOwner);
 		}
 
 		ensure!((pivot & !region_id.mask).is_void(), Error::<T>::ExteriorPivot);

substrate/frame/broker/src/lib.rs

@@ -36,6 +36,7 @@ mod tick_impls;
 mod types;
 mod utility_impls;
 
+pub mod migration;
 pub mod runtime_api;
 
 pub mod weights;
@@ -46,6 +47,9 @@ pub use core_mask::*;
 pub use coretime_interface::*;
 pub use types::*;
 
+/// The log target for this pallet.
+const LOG_TARGET: &str = "runtime::broker";
+
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
@@ -61,7 +65,10 @@ pub mod pallet {
 	use sp_runtime::traits::{Convert, ConvertBack};
 	use sp_std::vec::Vec;
 
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
 
 	#[pallet::config]
@@ -216,9 +223,9 @@ pub mod pallet {
 			/// The duration of the Region.
 			duration: Timeslice,
 			/// The old owner of the Region.
-			old_owner: T::AccountId,
+			old_owner: Option<T::AccountId>,
 			/// The new owner of the Region.
-			owner: T::AccountId,
+			owner: Option<T::AccountId>,
 		},
 		/// A Region has been split into two non-overlapping Regions.
 		Partitioned {

substrate/frame/broker/src/migration.rs

@@ -0,0 +1,87 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use super::*;
+use crate::types::RegionRecord;
+use codec::{Decode, Encode};
+use core::marker::PhantomData;
+use frame_support::traits::{Get, UncheckedOnRuntimeUpgrade};
+use sp_runtime::Saturating;
+
+#[cfg(feature = "try-runtime")]
+use frame_support::ensure;
+#[cfg(feature = "try-runtime")]
+use sp_std::vec::Vec;
+
+mod v1 {
+	use super::*;
+
+	/// V0 region record.
+	#[derive(Encode, Decode)]
+	struct RegionRecordV0<AccountId, Balance> {
+		/// The end of the Region.
+		pub end: Timeslice,
+		/// The owner of the Region.
+		pub owner: AccountId,
+		/// The amount paid to Polkadot for this Region, or `None` if renewal is not allowed.
+		pub paid: Option<Balance>,
+	}
+
+	pub struct MigrateToV1Impl<T>(PhantomData<T>);
+
+	impl<T: Config> UncheckedOnRuntimeUpgrade for MigrateToV1Impl<T> {
+		fn on_runtime_upgrade() -> frame_support::weights::Weight {
+			let mut count: u64 = 0;
+
+			<Regions<T>>::translate::<RegionRecordV0<T::AccountId, BalanceOf<T>>, _>(|_, v0| {
+				count.saturating_inc();
+				Some(RegionRecord { end: v0.end, owner: Some(v0.owner), paid: v0.paid })
+			});
+
+			log::info!(
+				target: LOG_TARGET,
+				"Storage migration v1 for pallet-broker finished.",
+			);
+
+			// calculate and return migration weights
+			T::DbWeight::get().reads_writes(count as u64 + 1, count as u64 + 1)
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<Vec<u8>, sp_runtime::TryRuntimeError> {
+			Ok((Regions::<T>::iter_keys().count() as u32).encode())
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade(state: Vec<u8>) -> Result<(), sp_runtime::TryRuntimeError> {
+			let old_count = u32::decode(&mut &state[..]).expect("Known good");
+			let new_count = Regions::<T>::iter_values().count() as u32;
+
+			ensure!(old_count == new_count, "Regions count should not change");
+			Ok(())
+		}
+	}
+}
+
+/// Migrate the pallet storage from `0` to `1`.
+pub type MigrateV0ToV1<T> = frame_support::migrations::VersionedMigration<
+	0,
+	1,
+	v1::MigrateToV1Impl<T>,
+	Pallet<T>,
+	<T as frame_system::Config>::DbWeight,
+>;

substrate/frame/broker/src/nonfungible_impl.rs

@@ -25,12 +25,13 @@ use sp_std::vec::Vec;
 impl<T: Config> Inspect<T::AccountId> for Pallet<T> {
 	type ItemId = u128;
 
-	fn owner(index: &Self::ItemId) -> Option<T::AccountId> {
-		Regions::<T>::get(RegionId::from(*index)).map(|r| r.owner)
+	fn owner(item: &Self::ItemId) -> Option<T::AccountId> {
+		let record = Regions::<T>::get(RegionId::from(*item))?;
+		record.owner
 	}
 
-	fn attribute(index: &Self::ItemId, key: &[u8]) -> Option<Vec<u8>> {
-		let id = RegionId::from(*index);
+	fn attribute(item: &Self::ItemId, key: &[u8]) -> Option<Vec<u8>> {
+		let id = RegionId::from(*item);
 		let item = Regions::<T>::get(id)?;
 		match key {
 			b"begin" => Some(id.begin.encode()),
@@ -46,11 +47,49 @@ impl<T: Config> Inspect<T::AccountId> for Pallet<T> {
 }
 
 impl<T: Config> Transfer<T::AccountId> for Pallet<T> {
-	fn transfer(index: &Self::ItemId, dest: &T::AccountId) -> DispatchResult {
-		Self::do_transfer((*index).into(), None, dest.clone()).map_err(Into::into)
+	fn transfer(item: &Self::ItemId, dest: &T::AccountId) -> DispatchResult {
+		Self::do_transfer((*item).into(), None, dest.clone()).map_err(Into::into)
 	}
 }
 
-// We don't allow any of the mutate operations, so the default implementation is used, which will
-// return `TokenError::Unsupported` in case any of the operations is called.
-impl<T: Config> Mutate<T::AccountId> for Pallet<T> {}
+/// We don't really support burning and minting.
+///
+/// We only need this to allow the region to be reserve transferable.
+///
+/// For reserve transfers that are not 'local', the asset must first be withdrawn to the holding
+/// register and then deposited into the designated account. This process necessitates that the
+/// asset is capable of being 'burned' and 'minted'.
+///
+/// Since each region is associated with specific record data, we will not actually burn the asset.
+/// If we did, we wouldn't know what record to assign to the newly minted region. Therefore, instead
+/// of burning, we set the asset's owner to `None`. In essence, 'burning' a region involves setting
+/// its owner to `None`, whereas 'minting' the region assigns its owner to an actual account. This
+/// way we never lose track of the associated record data.
+impl<T: Config> Mutate<T::AccountId> for Pallet<T> {
+	/// Deposit a region into an account.
+	fn mint_into(item: &Self::ItemId, who: &T::AccountId) -> DispatchResult {
+		let region_id: RegionId = (*item).into();
+		let record = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
+
+		// 'Minting' can only occur if the asset has previously been burned (i.e. moved to the
+		// holding register)
+		ensure!(record.owner.is_none(), Error::<T>::NotAllowed);
+		Self::issue(region_id.core, region_id.begin, record.end, Some(who.clone()), record.paid);
+
+		Ok(())
+	}
+
+	/// Withdraw a region from account.
+	fn burn(item: &Self::ItemId, maybe_check_owner: Option<&T::AccountId>) -> DispatchResult {
+		let region_id: RegionId = (*item).into();
+		let mut record = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
+		if let Some(owner) = maybe_check_owner {
+			ensure!(Some(owner.clone()) == record.owner, Error::<T>::NotOwner);
+		}
+
+		record.owner = None;
+		Regions::<T>::insert(region_id, record);
+
+		Ok(())
+	}
+}

substrate/frame/broker/src/tests.rs

@@ -200,14 +200,35 @@ fn transfer_works() {
 }
 
 #[test]
-fn mutate_operations_unsupported_for_regions() {
-	TestExt::new().execute_with(|| {
+fn mutate_operations_work() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
 		let region_id = RegionId { begin: 0, core: 0, mask: CoreMask::complete() };
 		assert_noop!(
 			<Broker as Mutate<_>>::mint_into(&region_id.into(), &2),
-			TokenError::Unsupported
+			Error::<Test>::UnknownRegion
 		);
-		assert_noop!(<Broker as Mutate<_>>::burn(&region_id.into(), None), TokenError::Unsupported);
+
+		assert_ok!(Broker::do_start_sales(100, 1));
+		advance_to(2);
+		let region_id = Broker::do_purchase(1, u64::max_value()).unwrap();
+		assert_noop!(
+			<Broker as Mutate<_>>::mint_into(&region_id.into(), &2),
+			Error::<Test>::NotAllowed
+		);
+
+		assert_noop!(
+			<Broker as Mutate<_>>::burn(&region_id.into(), Some(&2)),
+			Error::<Test>::NotOwner
+		);
+		// 'withdraw' the region from user 1:
+		assert_ok!(<Broker as Mutate<_>>::burn(&region_id.into(), Some(&1)));
+		assert_eq!(Regions::<Test>::get(region_id).unwrap().owner, None);
+
+		// `mint_into` works after burning:
+		assert_ok!(<Broker as Mutate<_>>::mint_into(&region_id.into(), &2));
+		assert_eq!(Regions::<Test>::get(region_id).unwrap().owner, Some(2));
+
+		// Unsupported operations:
 		assert_noop!(
 			<Broker as Mutate<_>>::set_attribute(&region_id.into(), &[], &[]),
 			TokenError::Unsupported
@@ -285,7 +306,7 @@ fn nft_metadata_works() {
 		assert_eq!(attribute::<Timeslice>(region, b"begin"), 4);
 		assert_eq!(attribute::<Timeslice>(region, b"length"), 3);
 		assert_eq!(attribute::<Timeslice>(region, b"end"), 7);
-		assert_eq!(attribute::<u64>(region, b"owner"), 1);
+		assert_eq!(attribute::<Option<u64>>(region, b"owner"), Some(1));
 		assert_eq!(attribute::<CoreMask>(region, b"part"), 0xfffff_fffff_fffff_fffff.into());
 		assert_eq!(attribute::<CoreIndex>(region, b"core"), 0);
 		assert_eq!(attribute::<Option<u64>>(region, b"paid"), Some(100));
@@ -297,7 +318,7 @@ fn nft_metadata_works() {
 		assert_eq!(attribute::<Timeslice>(region, b"begin"), 6);
 		assert_eq!(attribute::<Timeslice>(region, b"length"), 1);
 		assert_eq!(attribute::<Timeslice>(region, b"end"), 7);
-		assert_eq!(attribute::<u64>(region, b"owner"), 42);
+		assert_eq!(attribute::<Option<u64>>(region, b"owner"), Some(42));
 		assert_eq!(attribute::<CoreMask>(region, b"part"), 0x00000_fffff_fffff_00000.into());
 		assert_eq!(attribute::<CoreIndex>(region, b"core"), 0);
 		assert_eq!(attribute::<Option<u64>>(region, b"paid"), None);

substrate/frame/broker/src/types.rs

@@ -84,7 +84,7 @@ pub struct RegionRecord<AccountId, Balance> {
 	/// The end of the Region.
 	pub end: Timeslice,
 	/// The owner of the Region.
-	pub owner: AccountId,
+	pub owner: Option<AccountId>,
 	/// The amount paid to Polkadot for this Region, or `None` if renewal is not allowed.
 	pub paid: Option<Balance>,
 }

substrate/frame/broker/src/utility_impls.rs

@@ -72,11 +72,11 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	pub(crate) fn issue(
+	pub fn issue(
 		core: CoreIndex,
 		begin: Timeslice,
 		end: Timeslice,
-		owner: T::AccountId,
+		owner: Option<T::AccountId>,
 		paid: Option<BalanceOf<T>>,
 	) -> RegionId {
 		let id = RegionId { begin, core, mask: CoreMask::complete() };
@@ -94,7 +94,7 @@ impl<T: Config> Pallet<T> {
 		let region = Regions::<T>::get(&region_id).ok_or(Error::<T>::UnknownRegion)?;
 
 		if let Some(check_owner) = maybe_check_owner {
-			ensure!(check_owner == region.owner, Error::<T>::NotOwner);
+			ensure!(Some(check_owner) == region.owner, Error::<T>::NotOwner);
 		}
 
 		Regions::<T>::remove(&region_id);