diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index cc3f6b314d8c6..e8c6ca85267d2 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -68,7 +68,7 @@ fn create_pool_account( let pool_account = pallet_nomination_pools::BondedPools::::iter() .find(|(_, bonded_pool)| bonded_pool.roles.depositor == pool_creator) - .map(|(pool_account, _)| pool_account) + .map(|(pool_id, _)| Pools::::create_bonded_account(pool_id)) .expect("pool_creator created a pool above"); (pool_creator, pool_account) @@ -117,15 +117,15 @@ impl ListScenario { let i = CurrencyOf::::burn(CurrencyOf::::total_issuance()); sp_std::mem::forget(i); - // create accounts with the origin weight - let (_, pool_origin1) = create_pool_account::(USER_SEED + 2, origin_weight); + // Create accounts with the origin weight + let (_, pool_origin1) = create_pool_account::(USER_SEED + 1, origin_weight); T::StakingInterface::nominate( pool_origin1.clone(), // NOTE: these don't really need to be validators. vec![T::Lookup::unlookup(account("random_validator", 0, USER_SEED))], )?; - let (_, pool_origin2) = create_pool_account::(USER_SEED + 3, origin_weight); + let (_, pool_origin2) = create_pool_account::(USER_SEED + 2, origin_weight); T::StakingInterface::nominate( pool_origin2.clone(), vec![T::Lookup::unlookup(account("random_validator", 0, USER_SEED))].clone(), @@ -142,7 +142,7 @@ impl ListScenario { dest_weight_as_vote.try_into().map_err(|_| "could not convert u64 to Balance")?; // Create an account with the worst case destination weight - let (_, pool_dest1) = create_pool_account::(USER_SEED + 1, dest_weight); + let (_, pool_dest1) = create_pool_account::(USER_SEED + 3, dest_weight); T::StakingInterface::nominate( pool_dest1.clone(), vec![T::Lookup::unlookup(account("random_validator", 0, USER_SEED))], @@ -175,12 +175,11 @@ impl ListScenario { .expect("the pool was created in `Self::new`."); // Account pool points for the unbonded balance. - BondedPools::::mutate(&self.origin1, |maybe_pool| { + BondedPools::::mutate(&1, |maybe_pool| { maybe_pool.as_mut().map(|pool| pool.points -= amount) }); - Pools::::join(Origin::Signed(joiner.clone()).into(), amount, self.origin1.clone()) - .unwrap(); + Pools::::join(Origin::Signed(joiner.clone()).into(), amount, 1).unwrap(); // Sanity check that the vote weight is still the same as the original bonded let weight_of = pallet_staking::Pallet::::weight_of_fn(); @@ -189,7 +188,7 @@ impl ListScenario { // Sanity check the delegator was added correctly let delegator = Delegators::::get(&joiner).unwrap(); assert_eq!(delegator.points, amount); - assert_eq!(delegator.bonded_pool_account, self.origin1); + assert_eq!(delegator.pool_id, 1); self } @@ -217,7 +216,7 @@ frame_benchmarking::benchmarks! { = create_funded_user_with_balance::("joiner", 0, joiner_free); whitelist_account!(joiner); - }: _(Origin::Signed(joiner.clone()), max_additional, scenario.origin1.clone()) + }: _(Origin::Signed(joiner.clone()), max_additional, 1) verify { assert_eq!(CurrencyOf::::free_balance(&joiner), joiner_free - max_additional); assert_eq!( @@ -232,9 +231,7 @@ frame_benchmarking::benchmarks! { let origin_weight = pallet_nomination_pools::MinCreateBond::::get().max(CurrencyOf::::minimum_balance()) * 2u32.into(); let (depositor, pool_account) = create_pool_account::(0, origin_weight); - let reward_account = pallet_nomination_pools::RewardPools::::get(pool_account) - .map(|r| r.account.clone()) - .expect("pool created above."); + let reward_account = Pools::::create_reward_account(1); // Send funds to the reward account of the pool CurrencyOf::::make_free_balance_be(&reward_account, origin_weight); @@ -297,7 +294,7 @@ frame_benchmarking::benchmarks! { // Add a new delegator let min_join_bond = MinJoinBond::::get().max(CurrencyOf::::minimum_balance()); let joiner = create_funded_user_with_balance::("joiner", 0, min_join_bond * 2u32.into()); - Pools::::join(Origin::Signed(joiner.clone()).into(), min_join_bond, pool_account.clone()) + Pools::::join(Origin::Signed(joiner.clone()).into(), min_join_bond, 1) .unwrap(); // Sanity check join worked @@ -322,7 +319,7 @@ frame_benchmarking::benchmarks! { // Add `s` count of slashing spans to storage. pallet_staking::benchmarking::add_slashing_spans::(&pool_account, s); whitelist_account!(pool_account); - }: _(Origin::Signed(pool_account.clone()), pool_account.clone(), s) + }: _(Origin::Signed(pool_account.clone()), 1, s) verify { // The joiners funds didn't change assert_eq!(CurrencyOf::::free_balance(&joiner), min_join_bond); @@ -342,7 +339,7 @@ frame_benchmarking::benchmarks! { // Add a new delegator let min_join_bond = MinJoinBond::::get().max(CurrencyOf::::minimum_balance()); let joiner = create_funded_user_with_balance::("joiner", 0, min_join_bond * 2u32.into()); - Pools::::join(Origin::Signed(joiner.clone()).into(), min_join_bond, pool_account.clone()) + Pools::::join(Origin::Signed(joiner.clone()).into(), min_join_bond, 1) .unwrap(); // Sanity check join worked @@ -389,7 +386,7 @@ frame_benchmarking::benchmarks! { let (depositor, pool_account) = create_pool_account::(0, min_create_bond); // We set the pool to the destroying state so the depositor can leave - BondedPools::::try_mutate(&pool_account, |maybe_bonded_pool| { + BondedPools::::try_mutate(&1, |maybe_bonded_pool| { maybe_bonded_pool.as_mut().ok_or(()).map(|bonded_pool| { bonded_pool.state = PoolState::Destroying; }) @@ -398,7 +395,15 @@ frame_benchmarking::benchmarks! { // Unbond the creator pallet_staking::CurrentEra::::put(0); + // Simulate some rewards so we can check if the rewards storage is cleaned up. We check this + // here to ensure the complete flow for destroying a pool works - the reward pool account + // should never exist by time the depositor withdraws so we test that it gets cleaned + // up when unbonding. + let reward_account = Pools::::create_reward_account(1); + CurrencyOf::::make_free_balance_be(&reward_account, CurrencyOf::::minimum_balance()); + assert!(frame_system::Account::::contains_key(&reward_account)); Pools::::unbond_other(Origin::Signed(depositor.clone()).into(), depositor.clone()).unwrap(); + assert!(!frame_system::Account::::contains_key(&reward_account)); // Sanity check that unbond worked assert_eq!( @@ -416,33 +421,27 @@ frame_benchmarking::benchmarks! { // Some last checks that storage items we expect to get cleaned up are present assert!(pallet_staking::Ledger::::contains_key(&pool_account)); - assert!(BondedPools::::contains_key(&pool_account)); - assert!(SubPoolsStorage::::contains_key(&pool_account)); - assert!(RewardPools::::contains_key(&pool_account)); + assert!(BondedPools::::contains_key(&1)); + assert!(SubPoolsStorage::::contains_key(&1)); + assert!(RewardPools::::contains_key(&1)); assert!(Delegators::::contains_key(&depositor)); - let reward_account = pallet_nomination_pools::RewardPools::::get(&pool_account) - .map(|r| r.account.clone()) - .expect("pool created above."); - // Simulate some rewards so we can check if the rewards storage is cleaned up - CurrencyOf::::make_free_balance_be(&reward_account, CurrencyOf::::minimum_balance()); - assert!(frame_system::Account::::contains_key(&reward_account)); whitelist_account!(depositor); }: withdraw_unbonded_other(Origin::Signed(depositor.clone()), depositor.clone(), s) verify { // Pool removal worked assert!(!pallet_staking::Ledger::::contains_key(&pool_account)); - assert!(!BondedPools::::contains_key(&pool_account)); - assert!(!SubPoolsStorage::::contains_key(&pool_account)); - assert!(!RewardPools::::contains_key(&pool_account)); + assert!(!BondedPools::::contains_key(&1)); + assert!(!SubPoolsStorage::::contains_key(&1)); + assert!(!RewardPools::::contains_key(&1)); assert!(!Delegators::::contains_key(&depositor)); assert!(!frame_system::Account::::contains_key(&pool_account)); - assert!(!frame_system::Account::::contains_key(&reward_account)); // Funds where transferred back correctly assert_eq!( CurrencyOf::::free_balance(&depositor), - min_create_bond * 2u32.into() + // gets bond back + rewards collecting when unbonding + min_create_bond * 2u32.into() + CurrencyOf::::minimum_balance() ); } @@ -472,7 +471,7 @@ frame_benchmarking::benchmarks! { verify { assert_eq!(RewardPools::::count(), 1); assert_eq!(BondedPools::::count(), 1); - let (pool_account, new_pool) = BondedPools::::iter().next().unwrap(); + let (_, new_pool) = BondedPools::::iter().next().unwrap(); assert_eq!( new_pool, BondedPoolInner { @@ -488,7 +487,7 @@ frame_benchmarking::benchmarks! { } ); assert_eq!( - T::StakingInterface::bonded_balance(&pool_account), + T::StakingInterface::bonded_balance(&Pools::::create_bonded_account(1)), Some(min_create_bond) ); } @@ -511,11 +510,11 @@ frame_benchmarking::benchmarks! { .collect(); whitelist_account!(depositor); - }:_(Origin::Signed(depositor.clone()), pool_account, validators) + }:_(Origin::Signed(depositor.clone()), 1, validators) verify { assert_eq!(RewardPools::::count(), 1); assert_eq!(BondedPools::::count(), 1); - let (pool_account, new_pool) = BondedPools::::iter().next().unwrap(); + let (_, new_pool) = BondedPools::::iter().next().unwrap(); assert_eq!( new_pool, BondedPoolInner { @@ -531,7 +530,7 @@ frame_benchmarking::benchmarks! { } ); assert_eq!( - T::StakingInterface::bonded_balance(&pool_account), + T::StakingInterface::bonded_balance(&Pools::::create_bonded_account(1)), Some(min_create_bond) ); } @@ -542,16 +541,16 @@ frame_benchmarking::benchmarks! { .max(T::StakingInterface::minimum_bond()) .max(CurrencyOf::::minimum_balance()); let (depositor, pool_account) = create_pool_account::(0, min_create_bond); - BondedPools::::mutate(&pool_account, |maybe_pool| { + BondedPools::::mutate(&1, |maybe_pool| { // Force the pool into an invalid state maybe_pool.as_mut().map(|mut pool| pool.points = min_create_bond * 10u32.into()); }); let caller = account("caller", 0, USER_SEED); whitelist_account!(caller); - }:_(Origin::Signed(caller), pool_account.clone(), PoolState::Destroying) + }:_(Origin::Signed(caller), 1, PoolState::Destroying) verify { - assert_eq!(BondedPools::::get(pool_account).unwrap().state, PoolState::Destroying); + assert_eq!(BondedPools::::get(1).unwrap().state, PoolState::Destroying); } set_metadata { @@ -567,9 +566,9 @@ frame_benchmarking::benchmarks! { let metadata: Vec = (0..::MaxMetadataLen::get()).map(|_| 42).collect(); whitelist_account!(depositor); - }:_(Origin::Signed(depositor), pool_account.clone(), metadata.clone()) + }:_(Origin::Signed(depositor), 1, metadata.clone()) verify { - assert_eq!(Metadata::::get(&pool_account), metadata); + assert_eq!(Metadata::::get(&1), metadata); } } diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index 895b9b672c96d..6dfc93f1a790b 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -19,7 +19,7 @@ use frame_election_provider_support::VoteWeight; use frame_support::{pallet_prelude::*, parameter_types, traits::ConstU64, PalletId}; use sp_runtime::traits::{Convert, IdentityLookup}; -type AccountId = u64; +type AccountId = u128; type AccountIndex = u32; type BlockNumber = u64; type Balance = u128; diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 2ac769b50a506..0f1127ed71178 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -621,8 +621,7 @@ impl BondedPool { // This is a depositor if !sub_pools.no_era.points.is_zero() { // Unbonded pool has some points, so if they are the last delegator they must be - // here - // Since the depositor is the last to unbond, this should never be possible + // here. Since the depositor is the last to unbond, this should never be possible. ensure!(sub_pools.with_era.len().is_zero(), Error::::NotOnlyDelegator); ensure!( sub_pools.no_era.points == target_delegator.points, @@ -1539,14 +1538,15 @@ pub mod pallet { impl Pallet { /// Create the main, bonded account of a pool with the given id. - fn create_bonded_account(id: PoolId) -> T::AccountId { + pub fn create_bonded_account(id: PoolId) -> T::AccountId { + // 4bytes ++ 8bytes ++ 1byte ++ 4bytes T::PalletId::get().into_sub_account((1u8, id)) } /// Create the reward account of a pool with the given id. - fn create_reward_account(id: PoolId) -> T::AccountId { + pub fn create_reward_account(id: PoolId) -> T::AccountId { // TODO: integrity check for what is the reasonable max number of pools based on this. - // 4 + 8 + 4 + 1 + // 4bytes ++ 8bytes ++ 1byte ++ 4bytes T::PalletId::get().into_sub_account((2u8, id)) }