From c65a4c2857bcee6b90f96a4cacc3f11c7313fa3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20Dorado=20Su=C3=A1rez?= Date: Fri, 6 Sep 2024 09:41:11 -0500 Subject: [PATCH] change(pallet-assets-holder): revert `died` to make it an infallible function --- .../frame/assets-holder/src/impl_fungibles.rs | 24 ++++++++++++------- substrate/frame/assets-holder/src/tests.rs | 12 +++++++++- substrate/frame/assets/src/functions.rs | 12 +++++----- substrate/frame/assets/src/mock.rs | 3 +-- substrate/frame/assets/src/types.rs | 11 +++++---- 5 files changed, 39 insertions(+), 23 deletions(-) diff --git a/substrate/frame/assets-holder/src/impl_fungibles.rs b/substrate/frame/assets-holder/src/impl_fungibles.rs index 0b6663647a49..ef277f8b8d06 100644 --- a/substrate/frame/assets-holder/src/impl_fungibles.rs +++ b/substrate/frame/assets-holder/src/impl_fungibles.rs @@ -39,16 +39,22 @@ impl, I: 'static> BalanceOnHold::get(asset, who) } - fn died(asset: T::AssetId, who: &T::AccountId) -> DispatchResult { - Holds::::try_mutate(asset.clone(), who, |holds| { - for l in holds.iter() { - Self::burn_all_held(asset.clone(), &l.id, who, Precision::Exact, Fortitude::Force)?; - } + fn died(asset: T::AssetId, who: &T::AccountId) { + if pallet_assets::Pallet::::can_withdraw(asset.clone(), who, Zero::zero()) == + WithdrawConsequence::Success + { + defensive_assert!( + Holds::::get(asset.clone(), who).is_empty(), + "The list of Holds should be empty before allowing an account to die" + ); + defensive_assert!( + BalancesOnHold::::get(asset.clone(), who).is_none(), + "The should not be a balance on hold before allowing to die" + ); + } - *holds = BoundedVec::new(); - BalancesOnHold::::remove(asset.clone(), who); - Ok(()) - }) + Holds::::remove(asset.clone(), who); + BalancesOnHold::::remove(asset, who); } } diff --git a/substrate/frame/assets-holder/src/tests.rs b/substrate/frame/assets-holder/src/tests.rs index 8676105a49dc..433ed664a144 100644 --- a/substrate/frame/assets-holder/src/tests.rs +++ b/substrate/frame/assets-holder/src/tests.rs @@ -76,11 +76,21 @@ mod impl_balance_on_hold { }); } + #[test] + #[should_panic = "The list of Holds should be empty before allowing an account to die"] + fn died_fails_if_holds_exist() { + new_test_ext(|| { + test_hold(DummyHoldReason::Governance, 1); + AssetsHolder::died(ASSET_ID, &WHO); + }); + } + #[test] fn died_works() { new_test_ext(|| { test_hold(DummyHoldReason::Governance, 1); - assert_ok!(AssetsHolder::died(ASSET_ID, &WHO)); + test_release(DummyHoldReason::Governance); + AssetsHolder::died(ASSET_ID, &WHO); assert!(BalancesOnHold::::get(ASSET_ID, WHO).is_none()); assert!(Holds::::get(ASSET_ID, WHO).is_empty()); }); diff --git a/substrate/frame/assets/src/functions.rs b/substrate/frame/assets/src/functions.rs index 54d544481cba..75ff0013ce9d 100644 --- a/substrate/frame/assets/src/functions.rs +++ b/substrate/frame/assets/src/functions.rs @@ -384,7 +384,7 @@ impl, I: 'static> Pallet { Asset::::insert(&id, details); // Executing a hook here is safe, since it is not in a `mutate`. T::Freezer::died(id.clone(), &who); - T::Holder::died(id, &who)?; + T::Holder::died(id, &who); Ok(()) } @@ -421,7 +421,7 @@ impl, I: 'static> Pallet { Asset::::insert(&id, details); // Executing a hook here is safe, since it is not in a `mutate`. T::Freezer::died(id.clone(), &who); - T::Holder::died(id, &who)?; + T::Holder::died(id, &who); return Ok(()) } @@ -590,7 +590,7 @@ impl, I: 'static> Pallet { // Execute hook outside of `mutate`. if let Some(Remove) = target_died { T::Freezer::died(id.clone(), target); - T::Holder::died(id, target)?; + T::Holder::died(id, target); } Ok(actual) } @@ -615,7 +615,7 @@ impl, I: 'static> Pallet { Self::transfer_and_die(id.clone(), source, dest, amount, maybe_need_admin, f)?; if let Some(Remove) = died { T::Freezer::died(id.clone(), source); - T::Holder::died(id, source)?; + T::Holder::died(id, source); } Ok(balance) } @@ -816,7 +816,7 @@ impl, I: 'static> Pallet { for who in &dead_accounts { T::Freezer::died(id.clone(), &who); - T::Holder::died(id.clone(), &who)?; + T::Holder::died(id.clone(), &who); } Self::deposit_event(Event::AccountsDestroyed { @@ -978,7 +978,7 @@ impl, I: 'static> Pallet { // Execute hook outside of `mutate`. if let Some(Remove) = owner_died { T::Freezer::died(id.clone(), owner); - T::Holder::died(id, owner)?; + T::Holder::died(id, owner); } Ok(()) } diff --git a/substrate/frame/assets/src/mock.rs b/substrate/frame/assets/src/mock.rs index 6f9db628f92f..0f81eea47f57 100644 --- a/substrate/frame/assets/src/mock.rs +++ b/substrate/frame/assets/src/mock.rs @@ -125,9 +125,8 @@ impl BalanceOnHold for TestHolder { Held::get().get(&(asset, *who)).cloned() } - fn died(_asset: u32, _who: &u64) -> DispatchResult { + fn died(_asset: u32, _who: &u64) { // TODO: Connect with existing hooks list - Ok(()) } } diff --git a/substrate/frame/assets/src/types.rs b/substrate/frame/assets/src/types.rs index 50e3fab91b6f..162e88f5dc0d 100644 --- a/substrate/frame/assets/src/types.rs +++ b/substrate/frame/assets/src/types.rs @@ -249,17 +249,18 @@ pub trait BalanceOnHold { /// Called after an account has been removed. /// - /// NOTE: It is possible that the asset does no longer exist when this hook is called. - fn died(asset: AssetId, who: &AccountId) -> DispatchResult; + /// It is expected that this method is called only when there is not balance + /// on hold. Otherwise, an account should not be removed. + /// + /// NOTE: It is possible that the asset no longer exists when this hook is called. + fn died(asset: AssetId, who: &AccountId); } impl BalanceOnHold for () { fn balance_on_hold(_: AssetId, _: &AccountId) -> Option { None } - fn died(_: AssetId, _: &AccountId) -> DispatchResult { - Ok(()) - } + fn died(_: AssetId, _: &AccountId) {} } #[derive(Copy, Clone, PartialEq, Eq)]