diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 6f1defe043dff..6d6187c9dce6f 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -15,7 +15,7 @@ use pallet_nomination_pools::{ MaxDelegatorsPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, Pallet as Pools, PoolRoles, PoolState, RewardPools, SubPoolsStorage, }; -use sp_runtime::traits::{Bounded, StaticLookup, Zero}; +use sp_runtime::traits::{Bounded, Zero}; use sp_staking::{EraIndex, StakingInterface}; // `frame_benchmarking::benchmarks!` macro needs this use pallet_nomination_pools::Call; @@ -32,13 +32,6 @@ pub trait Config: pub struct Pallet(Pools); -fn clear_storage() { - pallet_nomination_pools::BondedPools::::remove_all(); - pallet_nomination_pools::RewardPools::::remove_all(); - pallet_nomination_pools::SubPoolsStorage::::remove_all(); - pallet_nomination_pools::Delegators::::remove_all(); -} - fn create_funded_user_with_balance( string: &'static str, n: u32, @@ -123,13 +116,13 @@ impl ListScenario { 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))], + vec![account("random_validator", 0, USER_SEED)], )?; 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(), + vec![account("random_validator", 0, USER_SEED)].clone(), )?; // Find a destination weight that will trigger the worst case scenario @@ -146,7 +139,7 @@ impl ListScenario { 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))], + vec![account("random_validator", 0, USER_SEED)], )?; let weight_of = pallet_staking::Pallet::::weight_of_fn(); @@ -197,8 +190,6 @@ impl ListScenario { frame_benchmarking::benchmarks! { join { - clear_storage::(); - let origin_weight = pallet_nomination_pools::MinCreateBond::::get() .max(CurrencyOf::::minimum_balance()) * 2u32.into(); @@ -227,8 +218,6 @@ frame_benchmarking::benchmarks! { } claim_payout { - clear_storage::(); - let origin_weight = pallet_nomination_pools::MinCreateBond::::get().max(CurrencyOf::::minimum_balance()) * 2u32.into(); let (depositor, pool_account) = create_pool_account::(0, origin_weight); @@ -257,8 +246,6 @@ frame_benchmarking::benchmarks! { } unbond_other { - clear_storage::(); - // the weight the nominator will start at. The value used here is expected to be // significantly higher than the first position in a list (e.g. the first bag threshold). let origin_weight = BalanceOf::::try_from(952_994_955_240_703u128) @@ -285,7 +272,6 @@ frame_benchmarking::benchmarks! { pool_withdraw_unbonded { let s in 0 .. MAX_SPANS; - clear_storage::(); let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) @@ -330,7 +316,6 @@ frame_benchmarking::benchmarks! { withdraw_unbonded_other_update { let s in 0 .. MAX_SPANS; - clear_storage::(); let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) @@ -378,7 +363,6 @@ frame_benchmarking::benchmarks! { withdraw_unbonded_other_kill { let s in 0 .. MAX_SPANS; - clear_storage::(); let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) @@ -447,8 +431,6 @@ frame_benchmarking::benchmarks! { } create { - clear_storage::(); - let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) .max(CurrencyOf::::minimum_balance()); @@ -488,14 +470,12 @@ frame_benchmarking::benchmarks! { } ); assert_eq!( - T::StakingInterface::active_balance(&Pools::::create_bonded_account(1)), + T::StakingInterface::active_stake(&Pools::::create_bonded_account(1)), Some(min_create_bond) ); } nominate { - clear_storage::(); - // Create a pool let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) @@ -505,9 +485,7 @@ frame_benchmarking::benchmarks! { // Create some accounts to nominate. For the sake of benchmarking they don't need to be // actual validators let validators: Vec<_> = (0..T::MaxNominations::get()) - .map(|i| - T::Lookup::unlookup(account("stash", USER_SEED, i)) - ) + .map(|i| account("stash", USER_SEED, i)) .collect(); whitelist_account!(depositor); @@ -555,8 +533,6 @@ frame_benchmarking::benchmarks! { } set_metadata { - clear_storage::(); - // Create a pool let min_create_bond = MinCreateBond::::get() .max(T::StakingInterface::minimum_bond()) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index ba4bb8692eea5..714920ae4693d 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -709,11 +709,20 @@ impl BondedPool { /// number saturating indicates the pool can no longer correctly keep track of state. fn bound_check(&mut self, n: U256) -> U256 { if n == U256::max_value() { - self.state = PoolState::Destroying + self.set_state(PoolState::Destroying) } n } + + // Set the state of `self`, and deposit an event if the state changed. State should never be set directly in + // in order to ensure a state change event is always correctly deposited. + fn set_state(&mut self, state: PoolState) { + if self.state != state { + self.state = state; + Pallet::::deposit_event(Event::::State { pool_id: self.id, new_state: state }); + }; + } } /// A reward pool. @@ -1462,19 +1471,14 @@ pub mod pallet { // The downside is that this seems like a misleading API if bonded_pool.can_toggle_state(&who) { - bonded_pool.state = state + bonded_pool.set_state(state); } else if bonded_pool.ok_to_be_open().is_err() && state == PoolState::Destroying { // If the pool has bad properties, then anyone can set it as destroying - bonded_pool.state = PoolState::Destroying; + bonded_pool.set_state(PoolState::Destroying); } else { Err(Error::::CanNotChangeState)?; } - Self::deposit_event(Event::::State { - pool_id, - new_state: bonded_pool.state.clone(), - }); - bonded_pool.put(); Ok(()) @@ -1563,12 +1567,13 @@ 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 { T::PalletId::get().into_sub_account((AccountType::Bonded, 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 { // NOTE: in order to have a distinction in the test account id type (u128), we put account_type first so // it does not get truncated out. T::PalletId::get().into_sub_account((AccountType::Reward, id)) @@ -1678,6 +1683,9 @@ impl Pallet { }; // Record updates + if reward_pool.total_earnings == BalanceOf::::max_value() { + bonded_pool.set_state(PoolState::Destroying); + }; delegator.reward_pool_total_earnings = reward_pool.total_earnings; reward_pool.points = current_points.saturating_sub(delegator_virtual_points); reward_pool.balance = reward_pool.balance.saturating_sub(delegator_payout); @@ -1802,7 +1810,7 @@ impl Pallet { }); assert!(MaxDelegators::::get().map_or(true, |max| all_delegators <= max)); - if level <= u8::MAX / 2 { + if level <= 1 { return Ok(()) } diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index d254eafb80020..08ed41e58369b 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -204,7 +204,6 @@ impl ExtBuilder { } pub(crate) fn build(self) -> sp_io::TestExternalities { - // sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap();