Asset Pallet: Support repeated destroys to safely destroy large assets (#12310)

* Support repeated destroys to safely destroy large assets * require freezing accounts before destroying * support only deleting asset as final stage when there's no assets left * pre: introduce the RemoveKeyLimit config parameter * debug_ensure empty account in the right if block * update to having separate max values for accounts and approvals * add tests and use RemoveKeyLimit constant * add useful comments to the extrinsics, and calculate returned weight * add benchmarking for start_destroy and finish destroy * push failing benchmark logic * add benchmark tests for new functions * update weights via local benchmarks * remove extra weight file * Update frame/assets/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/assets/src/types.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * Update frame/assets/src/lib.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * effect some changes from codereview * use NotFrozen error * remove origin checks, as anyone can complete destruction after owner has begun the process; Add live check for other extrinsics * fix comments about Origin behaviour * add AssetStatus docs * modularize logic to allow calling logic in on_idle and on_initialize hooks * introduce simple migration for assets details * reintroduce logging in the migrations * move deposit_Event out of the mutate block * Update frame/assets/src/functions.rs Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com> * Update frame/assets/src/migration.rs Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com> * move AssetNotLive checkout out of the mutate blocks * rename RemoveKeysLimit to RemoveItemsLimit * update docs * fix event name in benchmark * fix cargo fmt. * fix lint in benchmarking * Empty commit to trigger CI * Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * effect change suggested during code review * move limit to a single location * Update frame/assets/src/functions.rs Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> * rename events * fix weight typo, using rocksdb instead of T::DbWeight. Pending generating weights * switch to using dead_account.len() * rename event in the benchmarks * empty to retrigger CI * trigger CI to check cumulus dependency * trigger CI for dependent cumulus * Update frame/assets/src/migration.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * move is-frozen to the assetStatus enum (#12547) * add pre and post migration hooks * update do_transfer logic to add new assert for more correct error messages * trigger CI * switch checking AssetStatus from checking Destroying state to checking live state * fix error type in tests from Frozen to AssetNotLive * trigger CI * change ensure check for fn reducible_balance() * change the error type to Error:<T,I>::IncorrectStatus to be clearer * Trigger CI Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Co-authored-by: parity-processbot <> Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
2026-04-26 13:27:57 +00:00 · 2022-11-15 11:59:47 +02:00
parent 8fef631a95
commit 679d2dcd25
11 changed files with 630 additions and 233 deletions
@@ -121,11 +121,15 @@
 //! * [`System`](../frame_system/index.html)
 //! * [`Support`](../frame_support/index.html)

+// This recursion limit is needed because we have too many benchmarks and benchmarking will fail if
+// we add more without this limit.
+#![recursion_limit = "1024"]
 // Ensure we're `no_std` when compiling for Wasm.
 #![cfg_attr(not(feature = "std"), no_std)]

 #[cfg(feature = "runtime-benchmarks")]
 mod benchmarking;
+pub mod migration;
 #[cfg(test)]
 pub mod mock;
 #[cfg(test)]
@@ -174,8 +178,12 @@ pub mod pallet {
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;

+	/// The current storage version.
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T, I = ()>(_);

 	#[pallet::config]
@@ -195,6 +203,12 @@ pub mod pallet {
 			+ MaxEncodedLen
 			+ TypeInfo;

+		/// Max number of items to destroy per `destroy_accounts` and `destroy_approvals` call.
+		///
+		/// Must be configured to result in a weight that makes each call fit in a block.
+		#[pallet::constant]
+		type RemoveItemsLimit: Get<u32>;
+
 		/// Identifier for the class of asset.
 		type AssetId: Member
 			+ Parameter
@@ -342,7 +356,7 @@ pub mod pallet {
 						accounts: 0,
 						sufficients: 0,
 						approvals: 0,
-						is_frozen: false,
+						status: AssetStatus::Live,
 					},
 				);
 			}
@@ -417,6 +431,16 @@ pub mod pallet {
 		AssetFrozen { asset_id: T::AssetId },
 		/// Some asset `asset_id` was thawed.
 		AssetThawed { asset_id: T::AssetId },
+		/// Accounts were destroyed for given asset.
+		AccountsDestroyed { asset_id: T::AssetId, accounts_destroyed: u32, accounts_remaining: u32 },
+		/// Approvals were destroyed for given asset.
+		ApprovalsDestroyed {
+			asset_id: T::AssetId,
+			approvals_destroyed: u32,
+			approvals_remaining: u32,
+		},
+		/// An asset class is in the process of being destroyed.
+		DestructionStarted { asset_id: T::AssetId },
 		/// An asset class was destroyed.
 		Destroyed { asset_id: T::AssetId },
 		/// Some asset class was force-created.
@@ -487,6 +511,15 @@ pub mod pallet {
 		NoDeposit,
 		/// The operation would result in funds being burned.
 		WouldBurn,
+		/// The asset is a live asset and is actively being used. Usually emit for operations such
+		/// as `start_destroy` which require the asset to be in a destroying state.
+		LiveAsset,
+		/// The asset is not live, and likely being destroyed.
+		AssetNotLive,
+		/// The asset status is not the expected status.
+		IncorrectStatus,
+		/// The asset should be frozen before the given operation.
+		NotFrozen,
 	}

 	#[pallet::call]
@@ -540,7 +573,7 @@ pub mod pallet {
 					accounts: 0,
 					sufficients: 0,
 					approvals: 0,
-					is_frozen: false,
+					status: AssetStatus::Live,
 				},
 			);
 			Self::deposit_event(Event::Created { asset_id: id, creator: owner, owner: admin });
@@ -579,45 +612,89 @@ pub mod pallet {
 			Self::do_force_create(id, owner, is_sufficient, min_balance)
 		}

-		/// Destroy a class of fungible assets.
+		/// Start the process of destroying a class of fungible asset
+		/// start_destroy is the first in a series of extrinsics that should be called, to allow
+		/// destroying an asset.
 		///
 		/// The origin must conform to `ForceOrigin` or must be Signed and the sender must be the
 		/// owner of the asset `id`.
 		///
 		/// - `id`: The identifier of the asset to be destroyed. This must identify an existing
-		/// asset.
+		///   asset.
 		///
-		/// Emits `Destroyed` event when successful.
-		///
-		/// NOTE: It can be helpful to first freeze an asset before destroying it so that you
-		/// can provide accurate witness information and prevent users from manipulating state
-		/// in a way that can make it harder to destroy.
-		///
-		/// Weight: `O(c + p + a)` where:
-		/// - `c = (witness.accounts - witness.sufficients)`
-		/// - `s = witness.sufficients`
-		/// - `a = witness.approvals`
-		#[pallet::weight(T::WeightInfo::destroy(
-			witness.accounts.saturating_sub(witness.sufficients),
- 			witness.sufficients,
- 			witness.approvals,
- 		))]
-		pub fn destroy(
+		/// Assets must be freezed before calling start_destroy.
+		#[pallet::weight(T::WeightInfo::start_destroy())]
+		pub fn start_destroy(
 			origin: OriginFor<T>,
 			#[pallet::compact] id: T::AssetId,
-			witness: DestroyWitness,
-		) -> DispatchResultWithPostInfo {
+		) -> DispatchResult {
 			let maybe_check_owner = match T::ForceOrigin::try_origin(origin) {
 				Ok(_) => None,
 				Err(origin) => Some(ensure_signed(origin)?),
 			};
-			let details = Self::do_destroy(id, witness, maybe_check_owner)?;
-			Ok(Some(T::WeightInfo::destroy(
-				details.accounts.saturating_sub(details.sufficients),
-				details.sufficients,
-				details.approvals,
-			))
-			.into())
+			Self::do_start_destroy(id, maybe_check_owner)
+		}
+
+		/// Destroy all accounts associated with a given asset.
+		/// `destroy_accounts` should only be called after `start_destroy` has been called, and the
+		/// asset is in a `Destroying` state
+		///
+		/// Due to weight restrictions, this function may need to be called multiple
+		/// times to fully destroy all accounts. It will destroy `RemoveItemsLimit` accounts at a
+		/// time.
+		///
+		/// - `id`: The identifier of the asset to be destroyed. This must identify an existing
+		///   asset.
+		///
+		/// Each call Emits the `Event::DestroyedAccounts` event.
+		#[pallet::weight(T::WeightInfo::destroy_accounts(T::RemoveItemsLimit::get()))]
+		pub fn destroy_accounts(
+			origin: OriginFor<T>,
+			#[pallet::compact] id: T::AssetId,
+		) -> DispatchResultWithPostInfo {
+			let _ = ensure_signed(origin)?;
+			let removed_accounts = Self::do_destroy_accounts(id, T::RemoveItemsLimit::get())?;
+			Ok(Some(T::WeightInfo::destroy_accounts(removed_accounts)).into())
+		}
+
+		/// Destroy all approvals associated with a given asset up to the max (T::RemoveItemsLimit),
+		/// `destroy_approvals` should only be called after `start_destroy` has been called, and the
+		/// asset is in a `Destroying` state
+		///
+		/// Due to weight restrictions, this function may need to be called multiple
+		/// times to fully destroy all approvals. It will destroy `RemoveItemsLimit` approvals at a
+		/// time.
+		///
+		/// - `id`: The identifier of the asset to be destroyed. This must identify an existing
+		///   asset.
+		///
+		/// Each call Emits the `Event::DestroyedApprovals` event.
+		#[pallet::weight(T::WeightInfo::destroy_approvals(T::RemoveItemsLimit::get()))]
+		pub fn destroy_approvals(
+			origin: OriginFor<T>,
+			#[pallet::compact] id: T::AssetId,
+		) -> DispatchResultWithPostInfo {
+			let _ = ensure_signed(origin)?;
+			let removed_approvals = Self::do_destroy_approvals(id, T::RemoveItemsLimit::get())?;
+			Ok(Some(T::WeightInfo::destroy_approvals(removed_approvals)).into())
+		}
+
+		/// Complete destroying asset and unreserve currency.
+		/// `finish_destroy` should only be called after `start_destroy` has been called, and the
+		/// asset is in a `Destroying` state. All accounts or approvals should be destroyed before
+		/// hand.
+		///
+		/// - `id`: The identifier of the asset to be destroyed. This must identify an existing
+		///   asset.
+		///
+		/// Each successful call Emits the `Event::Destroyed` event.
+		#[pallet::weight(T::WeightInfo::finish_destroy())]
+		pub fn finish_destroy(
+			origin: OriginFor<T>,
+			#[pallet::compact] id: T::AssetId,
+		) -> DispatchResult {
+			let _ = ensure_signed(origin)?;
+			Self::do_finish_destroy(id)
 		}

 		/// Mint assets of a particular class.
@@ -793,6 +870,10 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;

 			let d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
+			ensure!(
+				d.status == AssetStatus::Live || d.status == AssetStatus::Frozen,
+				Error::<T, I>::AssetNotLive
+			);
 			ensure!(origin == d.freezer, Error::<T, I>::NoPermission);
 			let who = T::Lookup::lookup(who)?;

@@ -824,6 +905,10 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;

 			let details = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
+			ensure!(
+				details.status == AssetStatus::Live || details.status == AssetStatus::Frozen,
+				Error::<T, I>::AssetNotLive
+			);
 			ensure!(origin == details.admin, Error::<T, I>::NoPermission);
 			let who = T::Lookup::lookup(who)?;

@@ -854,9 +939,10 @@ pub mod pallet {

 			Asset::<T, I>::try_mutate(id, |maybe_details| {
 				let d = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
+				ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
 				ensure!(origin == d.freezer, Error::<T, I>::NoPermission);

-				d.is_frozen = true;
+				d.status = AssetStatus::Frozen;

 				Self::deposit_event(Event::<T, I>::AssetFrozen { asset_id: id });
 				Ok(())
@@ -882,8 +968,9 @@ pub mod pallet {
 			Asset::<T, I>::try_mutate(id, |maybe_details| {
 				let d = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
 				ensure!(origin == d.admin, Error::<T, I>::NoPermission);
+				ensure!(d.status == AssetStatus::Frozen, Error::<T, I>::NotFrozen);

-				d.is_frozen = false;
+				d.status = AssetStatus::Live;

 				Self::deposit_event(Event::<T, I>::AssetThawed { asset_id: id });
 				Ok(())
@@ -911,6 +998,7 @@ pub mod pallet {

 			Asset::<T, I>::try_mutate(id, |maybe_details| {
 				let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
+				ensure!(details.status == AssetStatus::Live, Error::<T, I>::LiveAsset);
 				ensure!(origin == details.owner, Error::<T, I>::NoPermission);
 				if details.owner == owner {
 					return Ok(())
@@ -956,6 +1044,7 @@ pub mod pallet {

 			Asset::<T, I>::try_mutate(id, |maybe_details| {
 				let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
+				ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
 				ensure!(origin == details.owner, Error::<T, I>::NoPermission);

 				details.issuer = issuer.clone();
@@ -1014,6 +1103,7 @@ pub mod pallet {
 			let origin = ensure_signed(origin)?;

 			let d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
+			ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
 			ensure!(origin == d.owner, Error::<T, I>::NoPermission);

 			Metadata::<T, I>::try_mutate_exists(id, |metadata| {
@@ -1142,13 +1232,18 @@ pub mod pallet {

 			Asset::<T, I>::try_mutate(id, |maybe_asset| {
 				let mut asset = maybe_asset.take().ok_or(Error::<T, I>::Unknown)?;
+				ensure!(asset.status != AssetStatus::Destroying, Error::<T, I>::AssetNotLive);
 				asset.owner = T::Lookup::lookup(owner)?;
 				asset.issuer = T::Lookup::lookup(issuer)?;
 				asset.admin = T::Lookup::lookup(admin)?;
 				asset.freezer = T::Lookup::lookup(freezer)?;
 				asset.min_balance = min_balance;
 				asset.is_sufficient = is_sufficient;
-				asset.is_frozen = is_frozen;
+				if is_frozen {
+					asset.status = AssetStatus::Frozen;
+				} else {
+					asset.status = AssetStatus::Live;
+				}
 				*maybe_asset = Some(asset);

 				Self::deposit_event(Event::AssetStatusChanged { asset_id: id });
@@ -1210,6 +1305,7 @@ pub mod pallet {
 			let owner = ensure_signed(origin)?;
 			let delegate = T::Lookup::lookup(delegate)?;
 			let mut d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
+			ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
 			let approval =
 				Approvals::<T, I>::take((id, &owner, &delegate)).ok_or(Error::<T, I>::Unknown)?;
 			T::Currency::unreserve(&owner, approval.deposit);
@@ -1242,6 +1338,7 @@ pub mod pallet {
 			delegate: AccountIdLookupOf<T>,
 		) -> DispatchResult {
 			let mut d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
+			ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
 			T::ForceOrigin::try_origin(origin)
 				.map(|_| ())
 				.or_else(|origin| -> DispatchResult {