Fix region nonfungible implementation (paritytech#5067)

The problem with the current implementation is that minting will cause the region coremask to be set to `Coremask::complete` regardless of the actual coremask. This PR fixes that. More details about the nonfungible implementation can be found here: paritytech#3455 --------- Co-authored-by: Dónal Murray <[email protected]> Co-authored-by: Branislav Kontur <[email protected]> Co-authored-by: Francisco Aguirre <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
moonbeam-foundation · Aug 2, 2024 · 1a31140 · 1a31140
1 parent 2af89bb
commit 1a31140
Show file tree

Hide file tree

Showing 7 changed files with 82 additions and 6 deletions.
diff --git a/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-rococo/src/lib.rs
@@ -832,7 +832,7 @@ impl_runtime_apis! {
 					let begin = 0;
 					let end = 42;
 
-					let region_id = pallet_broker::Pallet::<Runtime>::issue(core, begin, end, None, None);
+					let region_id = pallet_broker::Pallet::<Runtime>::issue(core, begin, pallet_broker::CoreMask::complete(), end, None, None);
 					Some((
 						Asset {
 							fun: NonFungible(Index(region_id.into())),

diff --git a/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs b/cumulus/parachains/runtimes/coretime/coretime-westend/src/lib.rs
@@ -823,7 +823,7 @@ impl_runtime_apis! {
 					let begin = 0;
 					let end = 42;
 
-					let region_id = pallet_broker::Pallet::<Runtime>::issue(core, begin, end, None, None);
+					let region_id = pallet_broker::Pallet::<Runtime>::issue(core, begin, pallet_broker::CoreMask::complete(), end, None, None);
 					Some((
 						Asset {
 							fun: NonFungible(Index(region_id.into())),

diff --git a/prdoc/pr_5067.prdoc b/prdoc/pr_5067.prdoc
@@ -0,0 +1,19 @@
+# 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: Fix region nonfungible implementation
+
+doc:
+  - audience: Runtime User
+    description: |
+      PR fixes the issue with the current implementation where minting causes
+      the region coremask to be set to `Coremask::complete` regardless of the
+      actual coremask of the region.
+
+crates: 
+- name: pallet-broker
+  bump: major
+- name: coretime-rococo-runtime
+  bump: patch
+- name: coretime-westend-runtime
+  bump: patch
diff --git a/substrate/frame/broker/src/dispatchable_impls.rs b/substrate/frame/broker/src/dispatchable_impls.rs
@@ -128,8 +128,14 @@ impl<T: Config> Pallet<T> {
 		let core = Self::purchase_core(&who, price, &mut sale)?;
 
 		SaleInfo::<T>::put(&sale);
-		let id =
-			Self::issue(core, sale.region_begin, sale.region_end, Some(who.clone()), Some(price));
+		let id = Self::issue(
+			core,
+			sale.region_begin,
+			CoreMask::complete(),
+			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)

diff --git a/substrate/frame/broker/src/nonfungible_impl.rs b/substrate/frame/broker/src/nonfungible_impl.rs
@@ -74,7 +74,14 @@ impl<T: Config> Mutate<T::AccountId> for Pallet<T> {
 		// '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);
+		Self::issue(
+			region_id.core,
+			region_id.begin,
+			region_id.mask,
+			record.end,
+			Some(who.clone()),
+			record.paid,
+		);
 
 		Ok(())
 	}

diff --git a/substrate/frame/broker/src/tests.rs b/substrate/frame/broker/src/tests.rs
@@ -240,6 +240,49 @@ fn mutate_operations_work() {
 	});
 }
 
+#[test]
+fn mutate_operations_work_with_partitioned_region() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(100, 1));
+		advance_to(2);
+		let region = Broker::do_purchase(1, u64::max_value()).unwrap();
+		let (region1, _region2) = Broker::do_partition(region, None, 2).unwrap();
+		let record_1 = Regions::<Test>::get(region1).unwrap();
+
+		// 'withdraw' the region from user 1:
+		assert_ok!(<Broker as Mutate<_>>::burn(&region1.into(), Some(&1)));
+		assert_eq!(Regions::<Test>::get(region1).unwrap().owner, None);
+
+		// `mint_into` works after burning:
+		assert_ok!(<Broker as Mutate<_>>::mint_into(&region1.into(), &1));
+
+		// Ensure the region minted is the same as the one we burned previously:
+		assert_eq!(Regions::<Test>::get(region1).unwrap(), record_1);
+	});
+}
+
+#[test]
+fn mutate_operations_work_with_interlaced_region() {
+	TestExt::new().endow(1, 1000).execute_with(|| {
+		assert_ok!(Broker::do_start_sales(100, 1));
+		advance_to(2);
+		let region = Broker::do_purchase(1, u64::max_value()).unwrap();
+		let (region1, _region2) =
+			Broker::do_interlace(region, None, CoreMask::from_chunk(0, 40)).unwrap();
+		let record_1 = Regions::<Test>::get(region1).unwrap();
+
+		// 'withdraw' the region from user 1:
+		assert_ok!(<Broker as Mutate<_>>::burn(&region1.into(), Some(&1)));
+		assert_eq!(Regions::<Test>::get(region1).unwrap().owner, None);
+
+		// `mint_into` works after burning:
+		assert_ok!(<Broker as Mutate<_>>::mint_into(&region1.into(), &1));
+
+		// Ensure the region minted is the same as the one we burned previously:
+		assert_eq!(Regions::<Test>::get(region1).unwrap(), record_1);
+	});
+}
+
 #[test]
 fn permanent_is_not_reassignable() {
 	TestExt::new().endow(1, 1000).execute_with(|| {

diff --git a/substrate/frame/broker/src/utility_impls.rs b/substrate/frame/broker/src/utility_impls.rs
@@ -94,11 +94,12 @@ impl<T: Config> Pallet<T> {
 	pub fn issue(
 		core: CoreIndex,
 		begin: Timeslice,
+		mask: CoreMask,
 		end: Timeslice,
 		owner: Option<T::AccountId>,
 		paid: Option<BalanceOf<T>>,
 	) -> RegionId {
-		let id = RegionId { begin, core, mask: CoreMask::complete() };
+		let id = RegionId { begin, core, mask };
 		let record = RegionRecord { end, owner, paid };
 		Regions::<T>::insert(&id, &record);
 		id