From ee94903f15cc3d631d9003ea82991c7bdd8c3673 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 11:17:06 +0000 Subject: [PATCH 1/4] Update benches to new account format --- .../nomination-pools/benchmarking/src/lib.rs | 67 +++++++++---------- .../nomination-pools/benchmarking/src/mock.rs | 2 +- frame/nomination-pools/src/lib.rs | 14 ++-- 3 files changed, 37 insertions(+), 46 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index cc3f6b314d8c6..6e28fe6772bae 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 + 5, 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 + 6, 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 + 7, 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; }) @@ -416,13 +413,11 @@ 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."); + let reward_account = Pools::::create_reward_account(1); // 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)); @@ -432,9 +427,9 @@ frame_benchmarking::benchmarks! { 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)); @@ -472,7 +467,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 +483,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 +506,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 +526,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 +537,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 +562,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..0082c887d2912 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, @@ -1348,10 +1347,6 @@ pub mod pallet { // this point. // TODO: in correct scenario, these two accounts should be zero when we reach there // anyway. - debug_assert_eq!( - T::Currency::free_balance(&bonded_pool.reward_account()), - Zero::zero() - ); debug_assert_eq!( T::Currency::free_balance(&bonded_pool.bonded_account()), Zero::zero() @@ -1539,14 +1534,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)) } From 14d948886a3af786173408820afa194806a0024b Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 11:24:45 +0000 Subject: [PATCH 2/4] More sensible seeds --- frame/nomination-pools/benchmarking/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 6e28fe6772bae..247d69e09e18b 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -118,14 +118,14 @@ impl ListScenario { sp_std::mem::forget(i); // Create accounts with the origin weight - let (_, pool_origin1) = create_pool_account::(USER_SEED + 5, 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 + 6, 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 + 7, 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))], From fba8de9466eb170016b9c13214a52bab9128a1db Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 11:51:55 +0000 Subject: [PATCH 3/4] bring back rward account sanity check --- frame/nomination-pools/benchmarking/src/lib.rs | 16 ++++++++++------ frame/nomination-pools/src/lib.rs | 4 ++++ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 247d69e09e18b..7df6f6bde9c24 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -395,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 must test that it gets cleaned + // up here. + 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!( @@ -417,10 +425,6 @@ frame_benchmarking::benchmarks! { assert!(SubPoolsStorage::::contains_key(&1)); assert!(RewardPools::::contains_key(&1)); assert!(Delegators::::contains_key(&depositor)); - let reward_account = Pools::::create_reward_account(1); - // 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) @@ -432,12 +436,12 @@ frame_benchmarking::benchmarks! { 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() ); } diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 0082c887d2912..0f1127ed71178 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1347,6 +1347,10 @@ pub mod pallet { // this point. // TODO: in correct scenario, these two accounts should be zero when we reach there // anyway. + debug_assert_eq!( + T::Currency::free_balance(&bonded_pool.reward_account()), + Zero::zero() + ); debug_assert_eq!( T::Currency::free_balance(&bonded_pool.bonded_account()), Zero::zero() From 609c54a093d652e1d86f2ffaf798d3622478cecc Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Tue, 15 Mar 2022 11:54:06 +0000 Subject: [PATCH 4/4] Comment --- frame/nomination-pools/benchmarking/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 7df6f6bde9c24..e8c6ca85267d2 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -397,8 +397,8 @@ frame_benchmarking::benchmarks! { 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 must test that it gets cleaned - // up here. + // 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));