Asset Pallet: Support repeated destroys to safely destroy large assets

bin/node/runtime/src/lib.rs

@@ -1410,6 +1410,7 @@ parameter_types! {
 	pub const StringLimit: u32 = 50;
 	pub const MetadataDepositBase: Balance = 10 * DOLLARS;
 	pub const MetadataDepositPerByte: Balance = 1 * DOLLARS;
+	pub const RemoveKeysLimit: u32 = 1000;
 }
 
 impl pallet_assets::Config for Runtime {
@@ -1427,6 +1428,8 @@ impl pallet_assets::Config for Runtime {
 	type Freezer = ();
 	type Extra = ();
 	type WeightInfo = pallet_assets::weights::SubstrateWeight<Runtime>;
+	type RemoveAccountsLimit = RemoveAccountsLimit;
+	type RemoveApprovalsLimit = RemoveApprovalsLimit;
 }
 
 parameter_types! {

frame/assets/src/benchmarking.rs

@@ -168,11 +168,10 @@ benchmarks_instance_pallet! {
 
 	destroy {
 		let c in 0 .. 5_000;
-		let s in 0 .. 5_000;
 		let a in 0 .. 5_00;
 		let (caller, _) = create_default_asset::<T, I>(true);
 		add_consumers::<T, I>(caller.clone(), c);
-		add_sufficients::<T, I>(caller.clone(), s);
+		// add_sufficients::<T, I>(caller.clone(), s);
 		add_approvals::<T, I>(caller.clone(), a);
 		let witness = Asset::<T, I>::get(T::AssetId::default()).unwrap().destroy_witness();
 	}: _(SystemOrigin::Signed(caller), Default::default(), witness)

frame/assets/src/functions.rs

@@ -679,35 +679,70 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 				if let Some(check_owner) = maybe_check_owner {
 					ensure!(details.owner == check_owner, Error::<T, I>::NoPermission);
 				}
+
+				ensure!(details.is_frozen, Error::<T, I>::BadWitness);
 				ensure!(details.accounts <= witness.accounts, Error::<T, I>::BadWitness);
 				ensure!(details.sufficients <= witness.sufficients, Error::<T, I>::BadWitness);
 				ensure!(details.approvals <= witness.approvals, Error::<T, I>::BadWitness);
 
-				for (who, v) in Account::<T, I>::drain_prefix(id) {
+				let mut removed_accounts = 0u32;
+				let mut removed_approvals = 0u32;
+
+				let accounts_to_delete: Vec<(T::AccountId, _)> = Account::<T, I>::iter_prefix(id)
+					.take(T::RemoveAccountsLimit::get() as usize)
+					.collect();
+
+				for (who, v) in accounts_to_delete {
+					if removed_accounts >= T::RemoveAccountsLimit::get() {
+						break
+					}
+
+					Account::<T, I>::remove(id, &who);
+
 					// We have to force this as it's destroying the entire asset class.
 					// This could mean that some accounts now have irreversibly reserved
 					// funds.
 					let _ = Self::dead_account(&who, &mut details, &v.reason, true);
 					dead_accounts.push(who);
+					removed_accounts += 1;
 				}
-				debug_assert_eq!(details.accounts, 0);
-				debug_assert_eq!(details.sufficients, 0);
 
-				let metadata = Metadata::<T, I>::take(&id);
-				T::Currency::unreserve(
-					&details.owner,
-					details.deposit.saturating_add(metadata.deposit),
-				);
+				for ((owner, destination), approval) in Approvals::<T, I>::iter_prefix((id,)) {
+					if removed_approvals >= T::RemoveApprovalsLimit::get() {
+						break
+					}
 
-				for ((owner, _), approval) in Approvals::<T, I>::drain_prefix((&id,)) {
 					T::Currency::unreserve(&owner, approval.deposit);
+					Approvals::<T, I>::remove((id, owner, destination));
+					removed_approvals += 1;
+				}
+
+				let accounts_remaining = Account::<T, I>::iter_prefix(id).count();
+				if accounts_remaining > 0 {
+					// Reintroduce the details, so the asset is not deleted yet.
+					let _ = maybe_details.insert(details.clone());
+
+					Self::deposit_event(Event::PartiallyDestroyed {
+						asset_id: id,
+						accounts_destroyed: dead_accounts.len() as u32,
+						accounts_remaining: accounts_remaining as u32,
+					});
+				} else {
+					debug_assert_eq!(details.accounts, 0);
+					debug_assert_eq!(details.sufficients, 0);
+
+					let metadata = Metadata::<T, I>::take(&id);
+					T::Currency::unreserve(
+						&details.owner,
+						details.deposit.saturating_add(metadata.deposit),
+					);
+					Self::deposit_event(Event::Destroyed { asset_id: id });
 				}
-				Self::deposit_event(Event::Destroyed { asset_id: id });
 
 				Ok(DestroyWitness {
-					accounts: details.accounts,
-					sufficients: details.sufficients,
-					approvals: details.approvals,
+					accounts: removed_accounts,
+					sufficients: 0,
+					approvals: removed_approvals,
 				})
 			},
 		)?;

frame/assets/src/lib.rs

@@ -193,6 +193,14 @@ pub mod pallet {
 			+ MaxEncodedLen
 			+ TypeInfo;
 
+		/// Max number of accounts to destroy per extrinsic call.
+		#[pallet::constant]
+		type RemoveAccountsLimit: Get<u32>;
+
+		/// Max number of approvalss to destroy per extrinsic call.
+		#[pallet::constant]
+		type RemoveApprovalsLimit: Get<u32>;
+
 		/// Identifier for the class of asset.
 		type AssetId: Member
 			+ Parameter
@@ -409,6 +417,12 @@ pub mod pallet {
 		AssetThawed { asset_id: T::AssetId },
 		/// An asset class was destroyed.
 		Destroyed { asset_id: T::AssetId },
+		/// An asset class was only destroyed. The destroy action should be re-executed.
+		PartiallyDestroyed {
+			asset_id: T::AssetId,
+			accounts_destroyed: u32,
+			accounts_remaining: u32,
+		},
 		/// Some asset class was force-created.
 		ForceCreated { asset_id: T::AssetId, owner: T::AccountId },
 		/// New metadata has been set for an asset.
@@ -570,6 +584,7 @@ pub mod pallet {
 		}
 
 		/// Destroy a class of fungible assets.
+		/// Due to weight restrictions, this function may need to be called multiple
 		///
 		/// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the
 		/// owner of the asset `id`.
@@ -587,10 +602,10 @@ pub mod pallet {
 		/// - `c = (witness.accounts - witness.sufficients)`
 		/// - `s = witness.sufficients`
 		/// - `a = witness.approvals`
+		/// TODO: Change the weights to T::RemoveKeysLimit::get() like in cloudloads
 		#[pallet::weight(T::WeightInfo::destroy(
-			witness.accounts.saturating_sub(witness.sufficients),
- 			witness.sufficients,
- 			witness.approvals,
+			T::RemoveAccountsLimit::get(),
+ 			T::RemoveApprovalsLimit::get(),
  		))]
 		pub fn destroy(
 			origin: OriginFor<T>,
@@ -603,8 +618,7 @@ pub mod pallet {
 			};
 			let details = Self::do_destroy(id, witness, maybe_check_owner)?;
 			Ok(Some(T::WeightInfo::destroy(
-				details.accounts.saturating_sub(details.sufficients),
-				details.sufficients,
+				details.accounts,
 				details.approvals,
 			))
 			.into())

frame/assets/src/mock.rs

@@ -84,6 +84,11 @@ impl pallet_balances::Config for Test {
 	type ReserveIdentifier = [u8; 8];
 }
 
+parameter_types! {
+	pub const RemoveAccountsLimit: u32 = 5;
+	pub const RemoveApprovalsLimit: u32 = 5;
+}
+
 impl Config for Test {
 	type RuntimeEvent = RuntimeEvent;
 	type Balance = u64;
@@ -99,6 +104,8 @@ impl Config for Test {
 	type Freezer = TestFreezer;
 	type WeightInfo = ();
 	type Extra = ();
+	type RemoveAccountsLimit = RemoveAccountsLimit;
+	type RemoveApprovalsLimit = RemoveApprovalsLimit;
 }
 
 use std::collections::HashMap;

frame/assets/src/tests.rs

@@ -315,6 +315,7 @@ fn lifecycle_should_work() {
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 20, 100));
 		assert_eq!(Account::<Test>::iter_prefix(0).count(), 2);
 
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
 		let w = Asset::<Test>::get(0).unwrap().destroy_witness();
 		assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w));
 		assert_eq!(Balances::reserved_balance(&1), 0);
@@ -335,6 +336,7 @@ fn lifecycle_should_work() {
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 20, 100));
 		assert_eq!(Account::<Test>::iter_prefix(0).count(), 2);
 
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
 		let w = Asset::<Test>::get(0).unwrap().destroy_witness();
 		assert_ok!(Assets::destroy(RuntimeOrigin::root(), 0, w));
 		assert_eq!(Balances::reserved_balance(&1), 0);
@@ -353,6 +355,7 @@ fn destroy_with_bad_witness_should_not_work() {
 		let mut w = Asset::<Test>::get(0).unwrap().destroy_witness();
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 10, 100));
 		// witness too low
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
 		assert_noop!(Assets::destroy(RuntimeOrigin::signed(1), 0, w), Error::<Test>::BadWitness);
 		// witness too high is okay though
 		w.accounts += 2;
@@ -372,6 +375,7 @@ fn destroy_should_refund_approvals() {
 		assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 4, 50));
 		assert_eq!(Balances::reserved_balance(&1), 3);
 
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
 		let w = Asset::<Test>::get(0).unwrap().destroy_witness();
 		assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w));
 		assert_eq!(Balances::reserved_balance(&1), 0);
@@ -381,6 +385,38 @@ fn destroy_should_refund_approvals() {
 	});
 }
 
+#[test]
+fn partial_destroy_should_work() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 10));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 10));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 3, 10));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 4, 10));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 5, 10));
+		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 6, 10));
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
+
+		let w = Asset::<Test>::get(0).unwrap().destroy_witness();
+		assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w));
+		System::assert_has_event(RuntimeEvent::Assets(crate::Event::PartiallyDestroyed {
+			asset_id: 0,
+			accounts_destroyed: 5,
+			accounts_remaining: 1,
+		}));
+		// PartiallyDestroyed Asset should continue to exist
+		assert!(Asset::<Test>::contains_key(0));
+
+		// Second call to destroy on PartiallyDestroyed asset
+		let w2 = Asset::<Test>::get(0).unwrap().destroy_witness();
+		assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w2));
+		System::assert_has_event(RuntimeEvent::Assets(crate::Event::Destroyed { asset_id: 0 }));
+
+		// Destroyed Asset should not exist
+		assert!(!Asset::<Test>::contains_key(0));
+	})
+}
+
 #[test]
 fn non_providing_should_work() {
 	new_test_ext().execute_with(|| {
@@ -790,6 +826,7 @@ fn destroy_calls_died_hooks() {
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
 		assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 2, 100));
 		// Destroy the asset.
+		assert_ok!(Assets::freeze_asset(RuntimeOrigin::signed(1), 0));
 		let w = Asset::<Test>::get(0).unwrap().destroy_witness();
 		assert_ok!(Assets::destroy(RuntimeOrigin::signed(1), 0, w));
 

frame/assets/src/weights.rs

@@ -46,7 +46,7 @@ use sp_std::marker::PhantomData;
 pub trait WeightInfo {
 	fn create() -> Weight;
 	fn force_create() -> Weight;
-	fn destroy(c: u32, s: u32, a: u32, ) -> Weight;
+	fn destroy(c: u32, a: u32, ) -> Weight;
 	fn mint() -> Weight;
 	fn burn() -> Weight;
 	fn transfer() -> Weight;
@@ -89,21 +89,17 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 	// Storage: System Account (r:5000 w:5000)
 	// Storage: Assets Metadata (r:1 w:0)
 	// Storage: Assets Approvals (r:501 w:500)
-	fn destroy(c: u32, s: u32, a: u32, ) -> Weight {
+	fn destroy(c: u32, a: u32, ) -> Weight {
 		Weight::from_ref_time(0 as u64)
 			// Standard Error: 37_000
 			.saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64))
-			// Standard Error: 37_000
-			.saturating_add(Weight::from_ref_time(19_333_000 as u64).saturating_mul(s as u64))
 			// Standard Error: 375_000
 			.saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64))
 			.saturating_add(T::DbWeight::get().reads(5 as u64))
 			.saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(c as u64)))
-			.saturating_add(T::DbWeight::get().reads((2 as u64).saturating_mul(s as u64)))
 			.saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(a as u64)))
 			.saturating_add(T::DbWeight::get().writes(2 as u64))
 			.saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(c as u64)))
-			.saturating_add(T::DbWeight::get().writes((2 as u64).saturating_mul(s as u64)))
 			.saturating_add(T::DbWeight::get().writes((1 as u64).saturating_mul(a as u64)))
 	}
 	// Storage: Assets Asset (r:1 w:1)
@@ -274,21 +270,17 @@ impl WeightInfo for () {
 	// Storage: System Account (r:5000 w:5000)
 	// Storage: Assets Metadata (r:1 w:0)
 	// Storage: Assets Approvals (r:501 w:500)
-	fn destroy(c: u32, s: u32, a: u32, ) -> Weight {
+	fn destroy(c: u32, a: u32, ) -> Weight {
 		Weight::from_ref_time(0 as u64)
 			// Standard Error: 37_000
 			.saturating_add(Weight::from_ref_time(17_145_000 as u64).saturating_mul(c as u64))
-			// Standard Error: 37_000
-			.saturating_add(Weight::from_ref_time(19_333_000 as u64).saturating_mul(s as u64))
 			// Standard Error: 375_000
 			.saturating_add(Weight::from_ref_time(17_046_000 as u64).saturating_mul(a as u64))
 			.saturating_add(RocksDbWeight::get().reads(5 as u64))
 			.saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(c as u64)))
-			.saturating_add(RocksDbWeight::get().reads((2 as u64).saturating_mul(s as u64)))
 			.saturating_add(RocksDbWeight::get().reads((1 as u64).saturating_mul(a as u64)))
 			.saturating_add(RocksDbWeight::get().writes(2 as u64))
 			.saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(c as u64)))
-			.saturating_add(RocksDbWeight::get().writes((2 as u64).saturating_mul(s as u64)))
 			.saturating_add(RocksDbWeight::get().writes((1 as u64).saturating_mul(a as u64)))
 	}
 	// Storage: Assets Asset (r:1 w:1)