From f02e12d3a67e0977f2dbb7c06b996d0827187117 Mon Sep 17 00:00:00 2001 From: Raphael Date: Mon, 3 Mar 2025 15:57:38 +0100 Subject: [PATCH 1/4] fix: only destroy funds on refund if collateral is BelowMinimum --- pallets/pallet-bonded-coins/src/lib.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/lib.rs b/pallets/pallet-bonded-coins/src/lib.rs index 469d02089..d4592b4e7 100644 --- a/pallets/pallet-bonded-coins/src/lib.rs +++ b/pallets/pallet-bonded-coins/src/lib.rs @@ -64,7 +64,7 @@ pub mod pallet { Create as CreateFungibles, Destroy as DestroyFungibles, Inspect as InspectFungibles, Mutate as MutateFungibles, }, - tokens::{Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance}, + tokens::{DepositConsequence, Fortitude, Precision as WithdrawalPrecision, Preservation, Provenance}, AccountTouch, }, Hashable, Parameter, @@ -1119,8 +1119,7 @@ pub mod pallet { if amount.is_zero() || T::Collaterals::can_deposit(pool_details.collateral.clone(), &who, amount, Provenance::Extant) - .into_result() - .is_err() + == DepositConsequence::BelowMinimum { // Funds are burnt but the collateral received is not sufficient to be deposited // to the account. This is tolerated as otherwise we could have edge cases where From a8e1a83605ce2fa16c964e8d5c0bc908db7fcbd0 Mon Sep 17 00:00:00 2001 From: Raphael Date: Tue, 4 Mar 2025 14:30:08 +0100 Subject: [PATCH 2/4] test: add negative test to check that funds are not burnt if account is locked --- .../src/tests/transactions/refund_account.rs | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs index d94d04446..af60a319f 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs @@ -17,7 +17,7 @@ // If you feel like getting in touch with us, you can do so at info@botlabs.org use frame_support::{ assert_err, assert_ok, - traits::fungibles::{Create, Inspect, Mutate}, + traits::fungibles::{roles::Inspect as InspectRole, Create, Inspect, Mutate}, }; use frame_system::{pallet_prelude::OriginFor, RawOrigin}; use sp_runtime::TokenError; @@ -266,6 +266,47 @@ fn refund_below_min_balance() { }); } +#[test] +fn refund_account_fails_when_account_blocked() { + let pool_details = generate_pool_details( + vec![DEFAULT_BONDED_CURRENCY_ID], + get_linear_bonding_curve(), + false, + Some(PoolStatus::Refunding), + Some(ACCOUNT_00), + None, + None, + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]); + + ExtBuilder::default() + .with_native_balances(vec![(ACCOUNT_01, ONE_HUNDRED_KILT)]) + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .with_bonded_balance(vec![ + (DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), ONE_HUNDRED_KILT), + (DEFAULT_COLLATERAL_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + (DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + ]) + .build_and_execute_with_sanity_tests(|| { + let origin: OriginFor = RawOrigin::Signed(ACCOUNT_01).into(); + + Assets::block( + RawOrigin::Signed(Assets::owner(DEFAULT_COLLATERAL_CURRENCY_ID).unwrap()).into(), + DEFAULT_COLLATERAL_CURRENCY_ID, + ACCOUNT_01, + ) + .expect("Failed to block account for test"); + + // Ensure the refund_account call fails due to pool not being in refunding state + assert_err!( + BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1), + TokenError::Blocked + ); + }); +} + #[test] fn refund_account_fails_when_pool_not_refunding() { let pool_details = generate_pool_details( From 4e315967d15219f13cb2d3496eee769d42ca987f Mon Sep 17 00:00:00 2001 From: Raphael Date: Wed, 5 Mar 2025 10:08:05 +0100 Subject: [PATCH 3/4] fixup! test: add negative test to check that funds are not burnt if account is locked --- .../src/tests/transactions/refund_account.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs index af60a319f..83156b413 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs @@ -299,7 +299,7 @@ fn refund_account_fails_when_account_blocked() { ) .expect("Failed to block account for test"); - // Ensure the refund_account call fails due to pool not being in refunding state + // Ensure the refund_account call fails due to failing can_deposit check assert_err!( BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1), TokenError::Blocked From 87cb2e8496d58a43c4ada089477bb22882e1a4d8 Mon Sep 17 00:00:00 2001 From: Raphael Date: Wed, 5 Mar 2025 14:42:42 +0100 Subject: [PATCH 4/4] test: MaxConsumers limit does not lead to burn of funds --- pallets/pallet-bonded-coins/src/mock.rs | 37 +++++++++- .../src/tests/transactions/refund_account.rs | 67 ++++++++++++++++++- 2 files changed, 101 insertions(+), 3 deletions(-) diff --git a/pallets/pallet-bonded-coins/src/mock.rs b/pallets/pallet-bonded-coins/src/mock.rs index 21d1887fb..a78b10ae1 100644 --- a/pallets/pallet-bonded-coins/src/mock.rs +++ b/pallets/pallet-bonded-coins/src/mock.rs @@ -58,6 +58,7 @@ pub mod runtime { weights::constants::RocksDbWeight, }; use frame_system::{EnsureRoot, EnsureSigned}; + use pallet_assets::FrozenBalance; use sp_core::U256; use sp_runtime::{ traits::{BlakeTwo256, IdentifyAccount, IdentityLookup, Verify}, @@ -69,7 +70,7 @@ pub mod runtime { self as pallet_bonded_coins, traits::NextAssetIds, types::{Locks, PoolStatus}, - Config, DepositBalanceOf, FungiblesAssetIdOf, PoolDetailsOf, + AccountIdOf, Config, DepositBalanceOf, FungiblesAssetIdOf, FungiblesBalanceOf, PoolDetailsOf, }; pub type Hash = sp_core::H256; @@ -190,6 +191,28 @@ pub mod runtime { } } + /// Store freezes for the assets pallet. + #[storage_alias] + pub type Freezes = StorageDoubleMap< + Assets, + Blake2_128Concat, + FungiblesAssetIdOf, + Blake2_128Concat, + AccountIdOf, + FungiblesBalanceOf, + OptionQuery, + >; + + pub struct FreezesHook; + + impl FrozenBalance for FreezesHook { + fn died(_asset: AssetId, _who: &AccountId) {} + + fn frozen_balance(asset: AssetId, who: &AccountId) -> Option { + Freezes::::get(asset, who) + } + } + frame_support::construct_runtime!( pub enum Test { @@ -279,7 +302,7 @@ pub mod runtime { type Currency = Balances; type Extra = (); type ForceOrigin = EnsureRoot; - type Freezer = (); + type Freezer = FreezesHook; type MetadataDepositBase = ConstU128<0>; type MetadataDepositPerByte = ConstU128<0>; type RemoveItemsLimit = ConstU32<5>; @@ -355,6 +378,7 @@ pub mod runtime { // pool_id, PoolDetails pools: Vec<(AccountId, PoolDetailsOf)>, collaterals: Vec, + freezes: Vec<(AssetId, AccountId, Balance)>, } impl ExtBuilder { @@ -378,6 +402,11 @@ pub mod runtime { self } + pub(crate) fn with_freezes(mut self, freezes: Vec<(AssetId, AccountId, Balance)>) -> Self { + self.freezes = freezes; + self + } + pub(crate) fn build(self) -> sp_io::TestExternalities { let mut storage = frame_system::GenesisConfig::::default().build_storage().unwrap(); @@ -448,6 +477,10 @@ pub mod runtime { }); NextAssetId::::set(next_asset_id); + + self.freezes.iter().for_each(|(asset_id, account, amount)| { + Freezes::::set(asset_id, account, Some(*amount)); + }); }); ext diff --git a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs index 83156b413..427e1e010 100644 --- a/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs +++ b/pallets/pallet-bonded-coins/src/tests/transactions/refund_account.rs @@ -19,7 +19,7 @@ use frame_support::{ assert_err, assert_ok, traits::fungibles::{roles::Inspect as InspectRole, Create, Inspect, Mutate}, }; -use frame_system::{pallet_prelude::OriginFor, RawOrigin}; +use frame_system::{pallet_prelude::OriginFor, ConsumerLimits, RawOrigin}; use sp_runtime::TokenError; use crate::{ @@ -307,6 +307,71 @@ fn refund_account_fails_when_account_blocked() { }); } +#[test] +fn refund_account_fails_if_account_cannot_be_created() { + let pool_details = generate_pool_details( + vec![DEFAULT_BONDED_CURRENCY_ID], + get_linear_bonding_curve(), + false, + Some(PoolStatus::Refunding), + Some(ACCOUNT_00), + None, + None, + None, + ); + let pool_id: AccountIdOf = calculate_pool_id(&[DEFAULT_BONDED_CURRENCY_ID]); + + // get MaxConsumers value + let max_consumers: usize = ::MaxConsumers::max_consumers() + .try_into() + .expect(""); + // Collateral must be non-sufficient for these tests to work, so we'll create a + // new collateral asset in the test + let collateral_id = DEFAULT_BONDED_CURRENCY_ID + 1; + + ExtBuilder::default() + .with_native_balances(vec![ + (ACCOUNT_01, ONE_HUNDRED_KILT), + (pool_id.clone(), ONE_HUNDRED_KILT), + ]) + .with_pools(vec![(pool_id.clone(), pool_details)]) + .with_collaterals(vec![DEFAULT_COLLATERAL_CURRENCY_ID]) + .with_bonded_balance(vec![ + (DEFAULT_COLLATERAL_CURRENCY_ID, pool_id.clone(), ONE_HUNDRED_KILT), + (DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, ONE_HUNDRED_KILT), + ]) + // Freeze some funds for ACCOUNT_01 so refund can only be partial + .with_freezes(vec![(DEFAULT_BONDED_CURRENCY_ID, ACCOUNT_01, 100_000)]) + .build_and_execute_with_sanity_tests(|| { + // switch pool's collateral to a non-sufficient one + assert_ok!(>::create(collateral_id, ACCOUNT_00, false, 1)); + Pools::::mutate(&pool_id, |details| details.as_mut().unwrap().collateral = collateral_id); + // make sure pool account holds collateral - this should work because it also + // holds the (sufficient) original collateral + assert_ok!(Assets::mint_into(collateral_id, &pool_id, ONE_HUNDRED_KILT)); + // create non-sufficient assets to increase ACCOUNT_01 consumers count + // the assets pallet requires at least two references to be available for + // creating an account, but creates only one, meaning we create max_consumers - + // 2 currencies (resulting in max_consumers - 1 consumers because we created one + // previously) + for i in 1..max_consumers - 1 { + let i_u32: u32 = i.try_into().expect("Failed to convert to u32"); + let asset_id = collateral_id + i_u32; + assert_ok!(>::create(asset_id, ACCOUNT_00, false, 1)); + assert_ok!(Assets::mint_into(asset_id, &ACCOUNT_01, ONE_HUNDRED_KILT)); + } + + let origin: OriginFor = RawOrigin::Signed(ACCOUNT_01).into(); + + // Collateral is non-sufficient and thus would create additional consumer + // references on ACCOUNT_01, which is too many + assert_err!( + BondingPallet::refund_account(origin, pool_id.clone(), ACCOUNT_01, 0, 1), + TokenError::CannotCreate + ); + }); +} + #[test] fn refund_account_fails_when_pool_not_refunding() { let pool_details = generate_pool_details(