diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 73804fc832c99..48a86ca3cfa04 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -84,13 +84,11 @@ impl, I: 'static> Pallet { } pub(super) fn dead_account( - what: T::AssetId, who: &T::AccountId, d: &mut AssetDetails>, reason: &ExistenceReason>, force: bool, ) -> DeadConsequence { - let mut result = Remove; match *reason { ExistenceReason::Consumer => frame_system::Pallet::::dec_consumers(who), ExistenceReason::Sufficient => { @@ -99,11 +97,10 @@ impl, I: 'static> Pallet { }, ExistenceReason::DepositRefunded => {}, ExistenceReason::DepositHeld(_) if !force => return Keep, - ExistenceReason::DepositHeld(_) => result = Keep, + ExistenceReason::DepositHeld(_) => {}, } d.accounts = d.accounts.saturating_sub(1); - T::Freezer::died(what, who); - result + Remove } pub(super) fn can_increase( @@ -323,12 +320,14 @@ impl, I: 'static> Pallet { T::Currency::unreserve(&who, deposit); - if let Remove = Self::dead_account(id, &who, &mut details, &account.reason, false) { + if let Remove = Self::dead_account(&who, &mut details, &account.reason, false) { Account::::remove(id, &who); } else { debug_assert!(false, "refund did not result in dead account?!"); } Asset::::insert(&id, details); + // Executing a hook here is safe, since it is not in a `mutate`. + T::Freezer::died(id, &who); Ok(()) } @@ -461,6 +460,7 @@ impl, I: 'static> Pallet { } let actual = Self::prep_debit(id, target, amount, f)?; + let mut target_died: Option = None; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let mut details = maybe_details.as_mut().ok_or(Error::::Unknown)?; @@ -475,9 +475,9 @@ impl, I: 'static> Pallet { account.balance = account.balance.saturating_sub(actual); if account.balance < details.min_balance { debug_assert!(account.balance.is_zero(), "checked in prep; qed"); - if let Remove = - Self::dead_account(id, target, &mut details, &account.reason, false) - { + target_died = + Some(Self::dead_account(target, &mut details, &account.reason, false)); + if let Some(Remove) = target_died { return Ok(()) } }; @@ -488,6 +488,10 @@ impl, I: 'static> Pallet { Ok(()) })?; + // Execute hook outside of `mutate`. + if let Some(Remove) = target_died { + T::Freezer::died(id, target); + } Ok(actual) } @@ -507,6 +511,24 @@ impl, I: 'static> Pallet { maybe_need_admin: Option, f: TransferFlags, ) -> Result { + let (balance, died) = + Self::transfer_and_die(id, source, dest, amount, maybe_need_admin, f)?; + if let Some(Remove) = died { + T::Freezer::died(id, source); + } + Ok(balance) + } + + /// Same as `do_transfer` but it does not execute the `FrozenBalance::died` hook and + /// instead returns whether and how the `source` account died in this operation. + fn transfer_and_die( + id: T::AssetId, + source: &T::AccountId, + dest: &T::AccountId, + amount: T::Balance, + maybe_need_admin: Option, + f: TransferFlags, + ) -> Result<(T::Balance, Option), DispatchError> { // Early exist if no-op. if amount.is_zero() { Self::deposit_event(Event::Transferred { @@ -515,7 +537,7 @@ impl, I: 'static> Pallet { to: dest.clone(), amount, }); - return Ok(amount) + return Ok((amount, None)) } // Figure out the debit and credit, together with side-effects. @@ -524,6 +546,7 @@ impl, I: 'static> Pallet { let mut source_account = Account::::get(id, &source).ok_or(Error::::NoAccount)?; + let mut source_died: Option = None; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; @@ -576,9 +599,9 @@ impl, I: 'static> Pallet { // Remove source account if it's now dead. if source_account.balance < details.min_balance { debug_assert!(source_account.balance.is_zero(), "checked in prep; qed"); - if let Remove = - Self::dead_account(id, &source, details, &source_account.reason, false) - { + source_died = + Some(Self::dead_account(&source, details, &source_account.reason, false)); + if let Some(Remove) = source_died { Account::::remove(id, &source); return Ok(()) } @@ -593,7 +616,7 @@ impl, I: 'static> Pallet { to: dest.clone(), amount: credit, }); - Ok(credit) + Ok((credit, source_died)) } /// Create a new asset without taking a deposit. @@ -646,41 +669,53 @@ impl, I: 'static> Pallet { witness: DestroyWitness, maybe_check_owner: Option, ) -> Result { - Asset::::try_mutate_exists(id, |maybe_details| { - let mut details = maybe_details.take().ok_or(Error::::Unknown)?; - if let Some(check_owner) = maybe_check_owner { - ensure!(details.owner == check_owner, Error::::NoPermission); - } - ensure!(details.accounts <= witness.accounts, Error::::BadWitness); - ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); - ensure!(details.approvals <= witness.approvals, Error::::BadWitness); - - for (who, v) in Account::::drain_prefix(id) { - // 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(id, &who, &mut details, &v.reason, true); - } - debug_assert_eq!(details.accounts, 0); - debug_assert_eq!(details.sufficients, 0); + let mut dead_accounts: Vec = vec![]; - let metadata = Metadata::::take(&id); - T::Currency::unreserve( - &details.owner, - details.deposit.saturating_add(metadata.deposit), - ); + let result_witness: DestroyWitness = Asset::::try_mutate_exists( + id, + |maybe_details| -> Result { + let mut details = maybe_details.take().ok_or(Error::::Unknown)?; + if let Some(check_owner) = maybe_check_owner { + ensure!(details.owner == check_owner, Error::::NoPermission); + } + ensure!(details.accounts <= witness.accounts, Error::::BadWitness); + ensure!(details.sufficients <= witness.sufficients, Error::::BadWitness); + ensure!(details.approvals <= witness.approvals, Error::::BadWitness); + + for (who, v) in Account::::drain_prefix(id) { + // 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); + } + debug_assert_eq!(details.accounts, 0); + debug_assert_eq!(details.sufficients, 0); - for ((owner, _), approval) in Approvals::::drain_prefix((&id,)) { - T::Currency::unreserve(&owner, approval.deposit); - } - Self::deposit_event(Event::Destroyed { asset_id: id }); + let metadata = Metadata::::take(&id); + T::Currency::unreserve( + &details.owner, + details.deposit.saturating_add(metadata.deposit), + ); - Ok(DestroyWitness { - accounts: details.accounts, - sufficients: details.sufficients, - approvals: details.approvals, - }) - }) + for ((owner, _), approval) in Approvals::::drain_prefix((&id,)) { + T::Currency::unreserve(&owner, approval.deposit); + } + Self::deposit_event(Event::Destroyed { asset_id: id }); + + Ok(DestroyWitness { + accounts: details.accounts, + sufficients: details.sufficients, + approvals: details.approvals, + }) + }, + )?; + + // Execute hooks outside of `mutate`. + for who in dead_accounts { + T::Freezer::died(id, &who); + } + Ok(result_witness) } /// Creates an approval from `owner` to spend `amount` of asset `id` tokens by 'delegate' @@ -742,6 +777,8 @@ impl, I: 'static> Pallet { destination: &T::AccountId, amount: T::Balance, ) -> DispatchResult { + let mut owner_died: Option = None; + Approvals::::try_mutate_exists( (id, &owner, delegate), |maybe_approved| -> DispatchResult { @@ -750,7 +787,7 @@ impl, I: 'static> Pallet { approved.amount.checked_sub(&amount).ok_or(Error::::Unapproved)?; let f = TransferFlags { keep_alive: false, best_effort: false, burn_dust: false }; - Self::do_transfer(id, &owner, &destination, amount, None, f)?; + owner_died = Self::transfer_and_die(id, &owner, &destination, amount, None, f)?.1; if remaining.is_zero() { T::Currency::unreserve(&owner, approved.deposit); @@ -766,6 +803,11 @@ impl, I: 'static> Pallet { Ok(()) }, )?; + + // Execute hook outside of `mutate`. + if let Some(Remove) = owner_died { + T::Freezer::died(id, owner); + } Ok(()) } diff --git a/frame/assets/src/mock.rs b/frame/assets/src/mock.rs index 34a4cf9ef38f6..67690e2b28ec1 100644 --- a/frame/assets/src/mock.rs +++ b/frame/assets/src/mock.rs @@ -120,19 +120,27 @@ impl FrozenBalance for TestFreezer { fn died(asset: u32, who: &u64) { HOOKS.with(|h| h.borrow_mut().push(Hook::Died(asset, who.clone()))); + // Sanity check: dead accounts have no balance. + assert!(Assets::balance(asset, *who).is_zero()); } } pub(crate) fn set_frozen_balance(asset: u32, who: u64, amount: u64) { FROZEN.with(|f| f.borrow_mut().insert((asset, who), amount)); } + pub(crate) fn clear_frozen_balance(asset: u32, who: u64) { FROZEN.with(|f| f.borrow_mut().remove(&(asset, who))); } + pub(crate) fn hooks() -> Vec { HOOKS.with(|h| h.borrow().clone()) } +pub(crate) fn take_hooks() -> Vec { + HOOKS.with(|h| h.take()) +} + pub(crate) fn new_test_ext() -> sp_io::TestExternalities { let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap(); @@ -154,6 +162,8 @@ pub(crate) fn new_test_ext() -> sp_io::TestExternalities { config.assimilate_storage(&mut storage).unwrap(); let mut ext: sp_io::TestExternalities = storage.into(); + // Clear thread local vars for https://github.com/paritytech/substrate/issues/10479. + ext.execute_with(|| take_hooks()); ext.execute_with(|| System::set_block_number(1)); ext } diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index a9f4dafc910c0..7430b742e7d2a 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -125,6 +125,21 @@ fn refunding_asset_deposit_without_burn_should_work() { }); } +/// Refunding reaps an account and calls the `FrozenBalance::died` hook. +#[test] +fn refunding_calls_died_hook() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(Origin::root(), 0, 1, false, 1)); + Balances::make_free_balance_be(&1, 100); + assert_ok!(Assets::touch(Origin::signed(1), 0)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100)); + assert_ok!(Assets::refund(Origin::signed(1), 0, true)); + + assert_eq!(Asset::::get(0).unwrap().accounts, 0); + assert_eq!(hooks(), vec![Hook::Died(0, 1)]); + }); +} + #[test] fn approval_lifecycle_works() { new_test_ext().execute_with(|| { @@ -389,19 +404,32 @@ fn min_balance_should_work() { ); // When deducting from an account to below minimum, it should be reaped. + // Death by `transfer`. assert_ok!(Assets::transfer(Origin::signed(1), 0, 2, 91)); assert!(Assets::maybe_balance(0, 1).is_none()); assert_eq!(Assets::balance(0, 2), 100); assert_eq!(Asset::::get(0).unwrap().accounts, 1); + assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]); + // Death by `force_transfer`. assert_ok!(Assets::force_transfer(Origin::signed(1), 0, 2, 1, 91)); assert!(Assets::maybe_balance(0, 2).is_none()); assert_eq!(Assets::balance(0, 1), 100); assert_eq!(Asset::::get(0).unwrap().accounts, 1); + assert_eq!(take_hooks(), vec![Hook::Died(0, 2)]); + // Death by `burn`. assert_ok!(Assets::burn(Origin::signed(1), 0, 1, 91)); assert!(Assets::maybe_balance(0, 1).is_none()); assert_eq!(Asset::::get(0).unwrap().accounts, 0); + assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]); + + // Death by `transfer_approved`. + assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100)); + Balances::make_free_balance_be(&1, 1); + assert_ok!(Assets::approve_transfer(Origin::signed(1), 0, 2, 100)); + assert_ok!(Assets::transfer_approved(Origin::signed(2), 0, 1, 3, 91)); + assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]); }); } @@ -448,6 +476,7 @@ fn transferring_enough_to_kill_source_when_keep_alive_should_fail() { assert_ok!(Assets::transfer_keep_alive(Origin::signed(1), 0, 2, 90)); assert_eq!(Assets::balance(0, 1), 10); assert_eq!(Assets::balance(0, 2), 90); + assert!(hooks().is_empty()); }); } @@ -684,6 +713,24 @@ fn set_metadata_should_work() { }); } +/// Destroying an asset calls the `FrozenBalance::died` hooks of all accounts. +#[test] +fn destroy_calls_died_hooks() { + new_test_ext().execute_with(|| { + assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 50)); + // Create account 1 and 2. + assert_ok!(Assets::mint(Origin::signed(1), 0, 1, 100)); + assert_ok!(Assets::mint(Origin::signed(1), 0, 2, 100)); + // Destroy the asset. + let w = Asset::::get(0).unwrap().destroy_witness(); + assert_ok!(Assets::destroy(Origin::signed(1), 0, w)); + + // Asset is gone and accounts 1 and 2 died. + assert!(Asset::::get(0).is_none()); + assert_eq!(hooks(), vec![Hook::Died(0, 1), Hook::Died(0, 2)]); + }) +} + #[test] fn freezer_should_work() { new_test_ext().execute_with(|| { diff --git a/frame/assets/src/types.rs b/frame/assets/src/types.rs index f8172e0a4a51b..56034e59086b9 100644 --- a/frame/assets/src/types.rs +++ b/frame/assets/src/types.rs @@ -172,13 +172,9 @@ pub trait FrozenBalance { /// If `None` is returned, then nothing special is enforced. fn frozen_balance(asset: AssetId, who: &AccountId) -> Option; - /// Called when an account has been removed. + /// Called after an account has been removed. /// - /// # Warning - /// - /// This function must never access storage of pallet asset. This function is called while some - /// change are pending. Calling into the pallet asset in this function can result in unexpected - /// state. + /// NOTE: It is possible that the asset does no longer exist when this hook is called. fn died(asset: AssetId, who: &AccountId); }