nomination-pools: Fix state event regression + benches

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<T: Config>(Pools<T>);
 
-fn clear_storage<T: pallet_nomination_pools::Config>() {
-	pallet_nomination_pools::BondedPools::<T>::remove_all();
-	pallet_nomination_pools::RewardPools::<T>::remove_all();
-	pallet_nomination_pools::SubPoolsStorage::<T>::remove_all();
-	pallet_nomination_pools::Delegators::<T>::remove_all();
-}
-
 fn create_funded_user_with_balance<T: pallet_nomination_pools::Config>(
 	string: &'static str,
 	n: u32,
@@ -123,13 +116,13 @@ impl<T: Config> ListScenario<T> {
 		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::<T>(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<T: Config> ListScenario<T> {
 		let (_, pool_dest1) = create_pool_account::<T>(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::<T>::weight_of_fn();
@@ -197,8 +190,6 @@ impl<T: Config> ListScenario<T> {
 
 frame_benchmarking::benchmarks! {
 	join {
-		clear_storage::<T>();
-
 		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get()
 			.max(CurrencyOf::<T>::minimum_balance())
 			* 2u32.into();
@@ -227,8 +218,6 @@ frame_benchmarking::benchmarks! {
 	}
 
 	claim_payout {
-		clear_storage::<T>();
-
 		let origin_weight = pallet_nomination_pools::MinCreateBond::<T>::get().max(CurrencyOf::<T>::minimum_balance()) * 2u32.into();
 		let (depositor, pool_account) = create_pool_account::<T>(0, origin_weight);
 
@@ -257,8 +246,6 @@ frame_benchmarking::benchmarks! {
 	}
 
 	unbond_other {
-		clear_storage::<T>();
-
 		// 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::<T>::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::<T>();
 
 		let min_create_bond = MinCreateBond::<T>::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::<T>();
 
 		let min_create_bond = MinCreateBond::<T>::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::<T>();
 
 		let min_create_bond = MinCreateBond::<T>::get()
 			.max(T::StakingInterface::minimum_bond())
@@ -447,8 +431,6 @@ frame_benchmarking::benchmarks! {
 	}
 
 	create {
-		clear_storage::<T>();
-
 		let min_create_bond = MinCreateBond::<T>::get()
 			.max(T::StakingInterface::minimum_bond())
 			.max(CurrencyOf::<T>::minimum_balance());
@@ -488,14 +470,12 @@ frame_benchmarking::benchmarks! {
 			}
 		);
 		assert_eq!(
-			T::StakingInterface::active_balance(&Pools::<T>::create_bonded_account(1)),
+			T::StakingInterface::active_stake(&Pools::<T>::create_bonded_account(1)),
 			Some(min_create_bond)
 		);
 	}
 
 	nominate {
-		clear_storage::<T>();
-
 		// Create a pool
 		let min_create_bond = MinCreateBond::<T>::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::<T>();
-
 		// Create a pool
 		let min_create_bond = MinCreateBond::<T>::get()
 			.max(T::StakingInterface::minimum_bond())

frame/nomination-pools/src/lib.rs

@@ -709,11 +709,20 @@ impl<T: Config> BondedPool<T> {
 	/// 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::<T>::deposit_event(Event::<T>::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::<T>::CanNotChangeState)?;
 			}
 
-			Self::deposit_event(Event::<T>::State {
-				pool_id,
-				new_state: bonded_pool.state.clone(),
-			});
-
 			bonded_pool.put();
 
 			Ok(())
@@ -1563,12 +1567,13 @@ pub mod pallet {
 
 impl<T: Config> Pallet<T> {
 	/// 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<T: Config> Pallet<T> {
 		};
 
 		// Record updates
+		if reward_pool.total_earnings == BalanceOf::<T>::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<T: Config> Pallet<T> {
 		});
 		assert!(MaxDelegators::<T>::get().map_or(true, |max| all_delegators <= max));
 
-		if level <= u8::MAX / 2 {
+		if level <= 1 {
 			return Ok(())
 		}
 

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::<Runtime>().unwrap();