Fix: Region reserve transfers

prdoc/pr_3553.prdoc

@@ -0,0 +1,28 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Region reserve transfers fix
+
+doc:
+  - audience: Runtime User
+    description: |
+      This PR addresses the issue with cross-chain transferring regions back to the
+      Coretime chain. TL;DR: 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
+
+migrations: 
+  db: []
+  runtime: 
+    - reference: pallet-broker
+      description: |
+        The region owner is optional.
+
+crates: 
+  - name: pallet-broker

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.10.0", default-features = false, features = ["derive"] }
 bitvec = { version = "1.0.0", default-features = false }
@@ -38,6 +39,7 @@ std = [
 	"frame-benchmarking?/std",
 	"frame-support/std",
 	"frame-system/std",
+	"log/std",
 	"scale-info/std",
 	"sp-arithmetic/std",
 	"sp-core/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

@@ -178,11 +178,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 +203,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 +227,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 weights;
 pub use weights::WeightInfo;
 
@@ -44,6 +45,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::*;
@@ -59,7 +63,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]
@@ -214,9 +221,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, OnRuntimeUpgrade};
+use sp_runtime::Saturating;
+
+#[cfg(feature = "try-runtime")]
+use frame_support::ensure;
+#[cfg(feature = "try-runtime")]
+use sp_std::vec::Vec;
+
+/// V0 region record.
+#[derive(Encode, Decode)]
+pub 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>,
+}
+
+mod v1 {
+	use super::*;
+
+	pub struct MigrateToV1Impl<T>(PhantomData<T>);
+
+	impl<T: Config> OnRuntimeUpgrade 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 registrar)
+		ensure!(record.owner.is_none(), Error::<T>::NotAllowed);
+		Self::issue(region_id.core, region_id.begin, record.end, 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

@@ -198,14 +198,32 @@ 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
@@ -283,7 +301,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));
@@ -295,7 +313,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

@@ -80,7 +80,7 @@ impl<T: Config> Pallet<T> {
 		paid: Option<BalanceOf<T>>,
 	) -> RegionId {
 		let id = RegionId { begin, core, mask: CoreMask::complete() };
-		let record = RegionRecord { end, owner, paid };
+		let record = RegionRecord { end, owner: Some(owner), paid };
 		Regions::<T>::insert(&id, &record);
 		id
 	}
@@ -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);