Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit a0ab42a

Browse files
tonyalaribejoepetrowskimuharemggwpez
authored
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 <[email protected]> * Update frame/assets/src/types.rs Co-authored-by: joe petrowski <[email protected]> * Update frame/assets/src/lib.rs Co-authored-by: joe petrowski <[email protected]> * 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 <[email protected]> * Update frame/assets/src/migration.rs Co-authored-by: Muharem Ismailov <[email protected]> * 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 <[email protected]> * Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * Update frame/assets/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * Update frame/assets/src/functions.rs Co-authored-by: Oliver Tale-Yazdi <[email protected]> * effect change suggested during code review * move limit to a single location * Update frame/assets/src/functions.rs Co-authored-by: joe petrowski <[email protected]> * 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 <[email protected]> * 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 <[email protected]> Co-authored-by: parity-processbot <> Co-authored-by: Muharem Ismailov <[email protected]> Co-authored-by: Oliver Tale-Yazdi <[email protected]>
1 parent c067438 commit a0ab42a

File tree

11 files changed

+626
-229
lines changed

11 files changed

+626
-229
lines changed

‎bin/node/runtime/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,6 +1451,7 @@ impl pallet_assets::Config for Runtime {
14511451
type Freezer = ();
14521452
type Extra = ();
14531453
type WeightInfo = pallet_assets::weights::SubstrateWeight<Runtime>;
1454+
type RemoveItemsLimit = ConstU32<1000>;
14541455
}
14551456

14561457
parameter_types! {

‎frame/assets/src/benchmarking.rs

Lines changed: 57 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -78,25 +78,6 @@ fn swap_is_sufficient<T: Config<I>, I: 'static>(s: &mut bool) {
7878
});
7979
}
8080

81-
fn add_consumers<T: Config<I>, I: 'static>(minter: T::AccountId, n: u32) {
82-
let origin = SystemOrigin::Signed(minter);
83-
let mut s = false;
84-
swap_is_sufficient::<T, I>(&mut s);
85-
for i in 0..n {
86-
let target = account("consumer", i, SEED);
87-
T::Currency::make_free_balance_be(&target, T::Currency::minimum_balance());
88-
let target_lookup = T::Lookup::unlookup(target);
89-
assert!(Assets::<T, I>::mint(
90-
origin.clone().into(),
91-
Default::default(),
92-
target_lookup,
93-
100u32.into()
94-
)
95-
.is_ok());
96-
}
97-
swap_is_sufficient::<T, I>(&mut s);
98-
}
99-
10081
fn add_sufficients<T: Config<I>, I: 'static>(minter: T::AccountId, n: u32) {
10182
let origin = SystemOrigin::Signed(minter);
10283
let mut s = true;
@@ -168,18 +149,66 @@ benchmarks_instance_pallet! {
168149
assert_last_event::<T, I>(Event::ForceCreated { asset_id: Default::default(), owner: caller }.into());
169150
}
170151

171-
destroy {
172-
let c in 0 .. 5_000;
173-
let s in 0 .. 5_000;
174-
let a in 0 .. 5_00;
152+
start_destroy {
153+
let (caller, caller_lookup) = create_default_minted_asset::<T, I>(true, 100u32.into());
154+
Assets::<T, I>::freeze_asset(
155+
SystemOrigin::Signed(caller.clone()).into(),
156+
Default::default(),
157+
)?;
158+
}:_(SystemOrigin::Signed(caller), Default::default())
159+
verify {
160+
assert_last_event::<T, I>(Event::DestructionStarted { asset_id: Default::default() }.into());
161+
}
162+
163+
destroy_accounts {
164+
let c in 0 .. T::RemoveItemsLimit::get();
175165
let (caller, _) = create_default_asset::<T, I>(true);
176-
add_consumers::<T, I>(caller.clone(), c);
177-
add_sufficients::<T, I>(caller.clone(), s);
166+
add_sufficients::<T, I>(caller.clone(), c);
167+
Assets::<T, I>::freeze_asset(
168+
SystemOrigin::Signed(caller.clone()).into(),
169+
Default::default(),
170+
)?;
171+
Assets::<T,I>::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?;
172+
}:_(SystemOrigin::Signed(caller), Default::default())
173+
verify {
174+
assert_last_event::<T, I>(Event::AccountsDestroyed {
175+
asset_id: Default::default() ,
176+
accounts_destroyed: c,
177+
accounts_remaining: 0,
178+
}.into());
179+
}
180+
181+
destroy_approvals {
182+
let a in 0 .. T::RemoveItemsLimit::get();
183+
let (caller, _) = create_default_minted_asset::<T, I>(true, 100u32.into());
178184
add_approvals::<T, I>(caller.clone(), a);
179-
let witness = Asset::<T, I>::get(T::AssetId::default()).unwrap().destroy_witness();
180-
}: _(SystemOrigin::Signed(caller), Default::default(), witness)
185+
Assets::<T, I>::freeze_asset(
186+
SystemOrigin::Signed(caller.clone()).into(),
187+
Default::default(),
188+
)?;
189+
Assets::<T,I>::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?;
190+
}:_(SystemOrigin::Signed(caller), Default::default())
181191
verify {
182-
assert_last_event::<T, I>(Event::Destroyed { asset_id: Default::default() }.into());
192+
assert_last_event::<T, I>(Event::ApprovalsDestroyed {
193+
asset_id: Default::default() ,
194+
approvals_destroyed: a,
195+
approvals_remaining: 0,
196+
}.into());
197+
}
198+
199+
finish_destroy {
200+
let (caller, caller_lookup) = create_default_asset::<T, I>(true);
201+
Assets::<T, I>::freeze_asset(
202+
SystemOrigin::Signed(caller.clone()).into(),
203+
Default::default(),
204+
)?;
205+
Assets::<T,I>::start_destroy(SystemOrigin::Signed(caller.clone()).into(), Default::default())?;
206+
}:_(SystemOrigin::Signed(caller), Default::default())
207+
verify {
208+
assert_last_event::<T, I>(Event::Destroyed {
209+
asset_id: Default::default() ,
210+
}.into()
211+
);
183212
}
184213

185214
mint {

‎frame/assets/src/functions.rs

Lines changed: 118 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
157157
if details.supply.checked_sub(&amount).is_none() {
158158
return Underflow
159159
}
160-
if details.is_frozen {
160+
if details.status == AssetStatus::Frozen {
161161
return Frozen
162162
}
163163
if amount.is_zero() {
@@ -205,7 +205,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
205205
keep_alive: bool,
206206
) -> Result<T::Balance, DispatchError> {
207207
let details = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
208-
ensure!(!details.is_frozen, Error::<T, I>::Frozen);
208+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
209209

210210
let account = Account::<T, I>::get(id, who).ok_or(Error::<T, I>::NoAccount)?;
211211
ensure!(!account.is_frozen, Error::<T, I>::Frozen);
@@ -300,6 +300,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
300300
ensure!(!Account::<T, I>::contains_key(id, &who), Error::<T, I>::AlreadyExists);
301301
let deposit = T::AssetAccountDeposit::get();
302302
let mut details = Asset::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
303+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
303304
let reason = Self::new_account(&who, &mut details, Some(deposit))?;
304305
T::Currency::reserve(&who, deposit)?;
305306
Asset::<T, I>::insert(&id, details);
@@ -321,9 +322,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
321322
let mut account = Account::<T, I>::get(id, &who).ok_or(Error::<T, I>::NoDeposit)?;
322323
let deposit = account.reason.take_deposit().ok_or(Error::<T, I>::NoDeposit)?;
323324
let mut details = Asset::<T, I>::get(&id).ok_or(Error::<T, I>::Unknown)?;
324-
325+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
325326
ensure!(account.balance.is_zero() || allow_burn, Error::<T, I>::WouldBurn);
326-
ensure!(!details.is_frozen, Error::<T, I>::Frozen);
327327
ensure!(!account.is_frozen, Error::<T, I>::Frozen);
328328

329329
T::Currency::unreserve(&who, deposit);
@@ -390,7 +390,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
390390
Self::can_increase(id, beneficiary, amount, true).into_result()?;
391391
Asset::<T, I>::try_mutate(id, |maybe_details| -> DispatchResult {
392392
let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
393-
393+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
394394
check(details)?;
395395

396396
Account::<T, I>::try_mutate(id, beneficiary, |maybe_account| -> DispatchResult {
@@ -430,6 +430,12 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
430430
maybe_check_admin: Option<T::AccountId>,
431431
f: DebitFlags,
432432
) -> Result<T::Balance, DispatchError> {
433+
let d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
434+
ensure!(
435+
d.status == AssetStatus::Live || d.status == AssetStatus::Frozen,
436+
Error::<T, I>::AssetNotLive
437+
);
438+
433439
let actual = Self::decrease_balance(id, target, amount, f, |actual, details| {
434440
// Check admin rights.
435441
if let Some(check_admin) = maybe_check_admin {
@@ -467,12 +473,14 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
467473
return Ok(amount)
468474
}
469475

476+
let details = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
477+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
478+
470479
let actual = Self::prep_debit(id, target, amount, f)?;
471480
let mut target_died: Option<DeadConsequence> = None;
472481

473482
Asset::<T, I>::try_mutate(id, |maybe_details| -> DispatchResult {
474483
let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
475-
476484
check(actual, details)?;
477485

478486
Account::<T, I>::try_mutate(id, target, |maybe_account| -> DispatchResult {
@@ -540,6 +548,8 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
540548
if amount.is_zero() {
541549
return Ok((amount, None))
542550
}
551+
let details = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
552+
ensure!(details.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
543553

544554
// Figure out the debit and credit, together with side-effects.
545555
let debit = Self::prep_debit(id, source, amount, f.into())?;
@@ -651,72 +661,123 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
651661
accounts: 0,
652662
sufficients: 0,
653663
approvals: 0,
654-
is_frozen: false,
664+
status: AssetStatus::Live,
655665
},
656666
);
657667
Self::deposit_event(Event::ForceCreated { asset_id: id, owner });
658668
Ok(())
659669
}
660670

661-
/// Destroy an existing asset.
662-
///
663-
/// * `id`: The asset you want to destroy.
664-
/// * `witness`: Witness data needed about the current state of the asset, used to confirm
665-
/// complexity of the operation.
666-
/// * `maybe_check_owner`: An optional check before destroying the asset, if the provided
667-
/// account is the owner of that asset. Can be used for authorization checks.
668-
pub(super) fn do_destroy(
671+
/// Start the process of destroying an asset, by setting the asset status to `Destroying`, and
672+
/// emitting the `DestructionStarted` event.
673+
pub(super) fn do_start_destroy(
669674
id: T::AssetId,
670-
witness: DestroyWitness,
671675
maybe_check_owner: Option<T::AccountId>,
672-
) -> Result<DestroyWitness, DispatchError> {
673-
let mut dead_accounts: Vec<T::AccountId> = vec![];
676+
) -> DispatchResult {
677+
Asset::<T, I>::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> {
678+
let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
679+
if let Some(check_owner) = maybe_check_owner {
680+
ensure!(details.owner == check_owner, Error::<T, I>::NoPermission);
681+
}
682+
details.status = AssetStatus::Destroying;
674683

675-
let result_witness: DestroyWitness = Asset::<T, I>::try_mutate_exists(
676-
id,
677-
|maybe_details| -> Result<DestroyWitness, DispatchError> {
678-
let mut details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?;
679-
if let Some(check_owner) = maybe_check_owner {
680-
ensure!(details.owner == check_owner, Error::<T, I>::NoPermission);
681-
}
682-
ensure!(details.accounts <= witness.accounts, Error::<T, I>::BadWitness);
683-
ensure!(details.sufficients <= witness.sufficients, Error::<T, I>::BadWitness);
684-
ensure!(details.approvals <= witness.approvals, Error::<T, I>::BadWitness);
684+
Self::deposit_event(Event::DestructionStarted { asset_id: id });
685+
Ok(())
686+
})
687+
}
688+
689+
/// Destroy accounts associated with a given asset up to the max (T::RemoveItemsLimit).
690+
///
691+
/// Each call emits the `Event::DestroyedAccounts` event.
692+
/// Returns the number of destroyed accounts.
693+
pub(super) fn do_destroy_accounts(
694+
id: T::AssetId,
695+
max_items: u32,
696+
) -> Result<u32, DispatchError> {
697+
let mut dead_accounts: Vec<T::AccountId> = vec![];
698+
let mut remaining_accounts = 0;
699+
let _ =
700+
Asset::<T, I>::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> {
701+
let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
702+
// Should only destroy accounts while the asset is in a destroying state
703+
ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
685704

686705
for (who, v) in Account::<T, I>::drain_prefix(id) {
687-
// We have to force this as it's destroying the entire asset class.
688-
// This could mean that some accounts now have irreversibly reserved
689-
// funds.
690706
let _ = Self::dead_account(&who, &mut details, &v.reason, true);
691707
dead_accounts.push(who);
708+
if dead_accounts.len() >= (max_items as usize) {
709+
break
710+
}
692711
}
693-
debug_assert_eq!(details.accounts, 0);
694-
debug_assert_eq!(details.sufficients, 0);
712+
remaining_accounts = details.accounts;
713+
Ok(())
714+
})?;
715+
716+
for who in &dead_accounts {
717+
T::Freezer::died(id, &who);
718+
}
695719

696-
let metadata = Metadata::<T, I>::take(&id);
697-
T::Currency::unreserve(
698-
&details.owner,
699-
details.deposit.saturating_add(metadata.deposit),
700-
);
720+
Self::deposit_event(Event::AccountsDestroyed {
721+
asset_id: id,
722+
accounts_destroyed: dead_accounts.len() as u32,
723+
accounts_remaining: remaining_accounts as u32,
724+
});
725+
Ok(dead_accounts.len() as u32)
726+
}
701727

702-
for ((owner, _), approval) in Approvals::<T, I>::drain_prefix((&id,)) {
728+
/// Destroy approvals associated with a given asset up to the max (T::RemoveItemsLimit).
729+
///
730+
/// Each call emits the `Event::DestroyedApprovals` event
731+
/// Returns the number of destroyed approvals.
732+
pub(super) fn do_destroy_approvals(
733+
id: T::AssetId,
734+
max_items: u32,
735+
) -> Result<u32, DispatchError> {
736+
let mut removed_approvals = 0;
737+
let _ =
738+
Asset::<T, I>::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> {
739+
let mut details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
740+
741+
// Should only destroy accounts while the asset is in a destroying state.
742+
ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
743+
744+
for ((owner, _), approval) in Approvals::<T, I>::drain_prefix((id,)) {
703745
T::Currency::unreserve(&owner, approval.deposit);
746+
removed_approvals = removed_approvals.saturating_add(1);
747+
details.approvals = details.approvals.saturating_sub(1);
748+
if removed_approvals >= max_items {
749+
break
750+
}
704751
}
705-
Self::deposit_event(Event::Destroyed { asset_id: id });
752+
Self::deposit_event(Event::ApprovalsDestroyed {
753+
asset_id: id,
754+
approvals_destroyed: removed_approvals as u32,
755+
approvals_remaining: details.approvals as u32,
756+
});
757+
Ok(())
758+
})?;
759+
Ok(removed_approvals)
760+
}
706761

707-
Ok(DestroyWitness {
708-
accounts: details.accounts,
709-
sufficients: details.sufficients,
710-
approvals: details.approvals,
711-
})
712-
},
713-
)?;
762+
/// Complete destroying an asset and unreserve the deposit.
763+
///
764+
/// On success, the `Event::Destroyed` event is emitted.
765+
pub(super) fn do_finish_destroy(id: T::AssetId) -> DispatchResult {
766+
Asset::<T, I>::try_mutate_exists(id, |maybe_details| -> Result<(), DispatchError> {
767+
let details = maybe_details.take().ok_or(Error::<T, I>::Unknown)?;
768+
ensure!(details.status == AssetStatus::Destroying, Error::<T, I>::IncorrectStatus);
769+
ensure!(details.accounts == 0, Error::<T, I>::InUse);
770+
ensure!(details.approvals == 0, Error::<T, I>::InUse);
771+
772+
let metadata = Metadata::<T, I>::take(&id);
773+
T::Currency::unreserve(
774+
&details.owner,
775+
details.deposit.saturating_add(metadata.deposit),
776+
);
777+
Self::deposit_event(Event::Destroyed { asset_id: id });
714778

715-
// Execute hooks outside of `mutate`.
716-
for who in dead_accounts {
717-
T::Freezer::died(id, &who);
718-
}
719-
Ok(result_witness)
779+
Ok(())
780+
})
720781
}
721782

722783
/// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate'
@@ -730,7 +791,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
730791
amount: T::Balance,
731792
) -> DispatchResult {
732793
let mut d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
733-
ensure!(!d.is_frozen, Error::<T, I>::Frozen);
794+
ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
734795
Approvals::<T, I>::try_mutate(
735796
(id, &owner, &delegate),
736797
|maybe_approved| -> DispatchResult {
@@ -780,6 +841,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
780841
) -> DispatchResult {
781842
let mut owner_died: Option<DeadConsequence> = None;
782843

844+
let d = Asset::<T, I>::get(id).ok_or(Error::<T, I>::Unknown)?;
845+
ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
846+
783847
Approvals::<T, I>::try_mutate_exists(
784848
(id, &owner, delegate),
785849
|maybe_approved| -> DispatchResult {
@@ -826,6 +890,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
826890
symbol.clone().try_into().map_err(|_| Error::<T, I>::BadMetadata)?;
827891

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

831896
Metadata::<T, I>::try_mutate_exists(id, |metadata| {

0 commit comments

Comments
 (0)