diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index 1452b8250f116..6f1defe043dff 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -168,7 +168,7 @@ impl ListScenario { self.origin1_delegator = Some(joiner.clone()); CurrencyOf::::make_free_balance_be(&joiner, amount * 2u32.into()); - let original_bonded = T::StakingInterface::bonded_balance(&self.origin1).unwrap(); + let original_bonded = T::StakingInterface::active_stake(&self.origin1).unwrap(); // Unbond `amount` from the underlying pool account so when the delegator joins // we will maintain `current_bonded`. @@ -206,7 +206,7 @@ frame_benchmarking::benchmarks! { // setup the worst case list scenario. let scenario = ListScenario::::new(origin_weight, true)?; assert_eq!( - T::StakingInterface::bonded_balance(&scenario.origin1).unwrap(), + T::StakingInterface::active_stake(&scenario.origin1).unwrap(), origin_weight ); @@ -221,7 +221,7 @@ frame_benchmarking::benchmarks! { verify { assert_eq!(CurrencyOf::::free_balance(&joiner), joiner_free - max_additional); assert_eq!( - T::StakingInterface::bonded_balance(&scenario.origin1).unwrap(), + T::StakingInterface::active_stake(&scenario.origin1).unwrap(), scenario.dest_weight ); } @@ -273,7 +273,7 @@ frame_benchmarking::benchmarks! { whitelist_account!(delegator_id); }: _(Origin::Signed(delegator_id.clone()), delegator_id.clone()) verify { - let bonded_after = T::StakingInterface::bonded_balance(&scenario.origin1).unwrap(); + let bonded_after = T::StakingInterface::active_stake(&scenario.origin1).unwrap(); // We at least went down to the destination bag, (if not an even lower bag) assert!(bonded_after <= scenario.dest_weight.clone()); let delegator = Delegators::::get( @@ -300,7 +300,7 @@ frame_benchmarking::benchmarks! { // Sanity check join worked assert_eq!( - T::StakingInterface::bonded_balance(&pool_account).unwrap(), + T::StakingInterface::active_stake(&pool_account).unwrap(), min_create_bond + min_join_bond ); assert_eq!(CurrencyOf::::free_balance(&joiner), min_join_bond); @@ -310,7 +310,7 @@ frame_benchmarking::benchmarks! { // Sanity check that unbond worked assert_eq!( - T::StakingInterface::bonded_balance(&pool_account).unwrap(), + T::StakingInterface::active_stake(&pool_account).unwrap(), min_create_bond ); assert_eq!(pallet_staking::Ledger::::get(&pool_account).unwrap().unlocking.len(), 1); @@ -345,7 +345,7 @@ frame_benchmarking::benchmarks! { // Sanity check join worked assert_eq!( - T::StakingInterface::bonded_balance(&pool_account).unwrap(), + T::StakingInterface::active_stake(&pool_account).unwrap(), min_create_bond + min_join_bond ); assert_eq!(CurrencyOf::::free_balance(&joiner), min_join_bond); @@ -356,7 +356,7 @@ frame_benchmarking::benchmarks! { // Sanity check that unbond worked assert_eq!( - T::StakingInterface::bonded_balance(&pool_account).unwrap(), + T::StakingInterface::active_stake(&pool_account).unwrap(), min_create_bond ); assert_eq!(pallet_staking::Ledger::::get(&pool_account).unwrap().unlocking.len(), 1); @@ -408,7 +408,7 @@ frame_benchmarking::benchmarks! { // Sanity check that unbond worked assert_eq!( - T::StakingInterface::bonded_balance(&pool_account).unwrap(), + T::StakingInterface::active_stake(&pool_account).unwrap(), Zero::zero() ); assert_eq!( @@ -488,7 +488,7 @@ frame_benchmarking::benchmarks! { } ); assert_eq!( - T::StakingInterface::bonded_balance(&Pools::::create_bonded_account(1)), + T::StakingInterface::active_balance(&Pools::::create_bonded_account(1)), Some(min_create_bond) ); } @@ -531,7 +531,7 @@ frame_benchmarking::benchmarks! { } ); assert_eq!( - T::StakingInterface::bonded_balance(&Pools::::create_bonded_account(1)), + T::StakingInterface::active_stake(&Pools::::create_bonded_account(1)), Some(min_create_bond) ); } diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index a17407f0a64a3..ba4bb8692eea5 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -284,19 +284,6 @@ //! * Watch out for overflow of [`RewardPoints`] and [`BalanceOf`] types. Consider things like the //! chains total issuance, staking reward rate, and burn rate. -// Invariants -// * A `delegator.bonded_pool_account` must always be a valid entry in `RewardPools`, and -// `BondedPoolInner`. -// * Every entry in `BondedPoolInner` must have a corresponding entry in `RewardPools` -// * If a delegator unbonds, the sub pools should always correctly track slashses such that the -// calculated amount when withdrawing unbonded is a lower bound of the pools free balance. -// * If the depositor is actively unbonding, the pool is in destroying state. To achieve this, once -// a pool is flipped to a destroying state it cannot change its state. -// * The sum of each pools delegator counter equals the `Delegators::count()`. -// * A pool's `delegator_counter` should always be gt 0. -// * TODO: metadata should only exist if the pool exist. - -// Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] use codec::Codec; @@ -312,15 +299,13 @@ use frame_support::{ }; use scale_info::TypeInfo; use sp_core::U256; -use sp_runtime::traits::{AccountIdConversion, Bounded, Convert, Saturating, StaticLookup, Zero}; +use sp_runtime::traits::{AccountIdConversion, Bounded, Convert, Saturating, Zero}; use sp_staking::{EraIndex, OnStakerSlash, StakingInterface}; use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, ops::Div, vec::Vec}; #[cfg(test)] mod mock; #[cfg(test)] -mod sanity; -#[cfg(test)] mod tests; pub mod weights; @@ -331,7 +316,6 @@ pub use weights::WeightInfo; pub type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; pub type SubPoolsWithEra = BoundedBTreeMap, TotalUnbondingPools>; -// NOTE: this assumes the balance type u128 or smaller. TODO: integrity-check pub type RewardPoints = U256; pub type PoolId = u32; @@ -354,6 +338,13 @@ enum PoolBond { Join, } +/// The type of account being created. +#[derive(Encode, Decode)] +enum AccountType { + Bonded, + Reward, +} + /// A delegator in a pool. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound)] #[cfg_attr(feature = "std", derive(Clone, PartialEq))] @@ -480,14 +471,14 @@ impl BondedPool { /// Get the amount of points to issue for some new funds that will be bonded in the pool. fn points_to_issue(&self, new_funds: BalanceOf) -> BalanceOf { let bonded_balance = - T::StakingInterface::bonded_balance(&self.bonded_account()).unwrap_or(Zero::zero()); + T::StakingInterface::active_stake(&self.bonded_account()).unwrap_or(Zero::zero()); Pallet::::points_to_issue(bonded_balance, self.points, new_funds) } /// Get the amount of balance to unbond from the pool based on a delegator's points of the pool. fn balance_to_unbond(&self, delegator_points: BalanceOf) -> BalanceOf { let bonded_balance = - T::StakingInterface::bonded_balance(&self.bonded_account()).unwrap_or(Zero::zero()); + T::StakingInterface::active_stake(&self.bonded_account()).unwrap_or(Zero::zero()); Pallet::::balance_to_unbond(bonded_balance, self.points, delegator_points) } @@ -521,11 +512,11 @@ impl BondedPool { self } - /// The pools balance that is not locked. This assumes the staking system is the only - fn non_locked_balance(&self) -> BalanceOf { + /// The pools balance that is transferrable. + fn transferrable_balance(&self) -> BalanceOf { let account = self.bonded_account(); T::Currency::free_balance(&account) - .saturating_sub(T::StakingInterface::locked_balance(&account).unwrap_or_default()) + .saturating_sub(T::StakingInterface::active_stake(&account).unwrap_or_default()) } fn can_nominate(&self, who: &T::AccountId) -> bool { @@ -555,7 +546,7 @@ impl BondedPool { ensure!(!self.is_destroying(), Error::::CanNotChangeState); let bonded_balance = - T::StakingInterface::bonded_balance(&self.bonded_account()).unwrap_or(Zero::zero()); + T::StakingInterface::active_stake(&self.bonded_account()).unwrap_or(Zero::zero()); ensure!(!bonded_balance.is_zero(), Error::::OverflowRisk); let points_to_balance_ratio_floor = self @@ -747,7 +738,6 @@ pub struct RewardPool { impl RewardPool { /// Mutate the reward pool by updating the total earnings and current free balance. fn update_total_earnings_and_balance(&mut self, id: PoolId) { - // TODO: not happy with this. let current_balance = T::Currency::free_balance(&Pallet::::create_reward_account(id)); // The earnings since the last time it was updated let new_earnings = current_balance.saturating_sub(self.balance); @@ -883,8 +873,6 @@ pub mod pallet { /// Weight information for extrinsics in this pallet. type WeightInfo: weights::WeightInfo; - // TODO: Should this just be part of the StakingInterface trait? We want the currencies to - // be the same anyways. /// The nominating balance. type Currency: Currency; @@ -902,7 +890,6 @@ pub mod pallet { type StakingInterface: StakingInterface< Balance = BalanceOf, AccountId = Self::AccountId, - LookupSource = ::Source, >; /// The amount of eras a `SubPools::with_era` pool can exist before it gets merged into the @@ -1150,8 +1137,10 @@ pub mod pallet { Ok(()) } - /// Unbond _all_ of the `target`'s funds from the pool. Under certain conditions, this call - /// can be dispatched permissionlessly (i.e. by any account). + /// Unbond _all_ of the `delegator_account`'s funds from the pool. + /// + /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any + /// account). /// /// # Conditions for a permissionless dispatch /// @@ -1170,6 +1159,7 @@ pub mod pallet { /// Note: If there are too many unlocking chunks to unbond with the pool account, /// [`Self::withdraw_unbonded_pool`] can be called to try and minimize unlocking chunks. #[pallet::weight(T::WeightInfo::unbond_other())] + #[frame_support::transactional] pub fn unbond_other( origin: OriginFor, delegator_account: T::AccountId, @@ -1181,19 +1171,11 @@ pub mod pallet { .defensive_ok_or_else(|| Error::::PoolNotFound)?; bonded_pool.ok_to_unbond_other_with(&caller, &delegator_account, &delegator)?; - // alternative: do_reward_payout can report if it changed states. - let was_destroying = bonded_pool.is_destroying(); - // Claim the the payout prior to unbonding. Once the user is unbonding their points // no longer exist in the bonded pool and thus they can no longer claim their payouts. // It is not strictly necessary to claim the rewards, but we do it here for UX. Self::do_reward_payout(delegator_account.clone(), &mut delegator, &mut bonded_pool)?; - // Note that we lazily create the unbonding pools here if they don't already exist - let sub_pools = SubPoolsStorage::::get(delegator.pool_id).unwrap_or_default(); - let current_era = T::StakingInterface::current_era(); - let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era); - let balance_to_unbond = bonded_pool.balance_to_unbond(delegator.points); // Update the bonded pool. Note that we must do this *after* calculating the balance @@ -1201,16 +1183,19 @@ pub mod pallet { bonded_pool.points = bonded_pool.points.saturating_sub(delegator.points); // Unbond in the actual underlying pool - // TODO: can fail after write: better make it transactional. T::StakingInterface::unbond(bonded_pool.bonded_account(), balance_to_unbond)?; + // Note that we lazily create the unbonding pools here if they don't already exist + let sub_pools = SubPoolsStorage::::get(delegator.pool_id).unwrap_or_default(); + let current_era = T::StakingInterface::current_era(); + let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era); + // Merge any older pools into the general, era agnostic unbond pool. Note that we do // this before inserting to ensure we don't go over the max unbonding pools. let mut sub_pools = sub_pools.maybe_merge_pools(unbond_era); - // Update the unbond pool associated with the current era with the - // unbonded funds. Note that we lazily create the unbond pool if it - // does not yet exist. + // Update the unbond pool associated with the current era with the unbonded funds. Note + // that we lazily create the unbond pool if it does not yet exist. sub_pools.unchecked_with_era_get_or_make(unbond_era).issue(balance_to_unbond); delegator.unbonding_era = Some(unbond_era); @@ -1221,14 +1206,6 @@ pub mod pallet { amount: balance_to_unbond, }); - if bonded_pool.is_destroying() && !was_destroying { - // Persist the pool to storage iff the state was updated - Self::deposit_event(Event::::State { - pool_id: delegator.pool_id, - new_state: PoolState::Destroying, - }); - } - // Now that we know everything has worked write the items to storage. bonded_pool.put(); SubPoolsStorage::insert(&delegator.pool_id, sub_pools); @@ -1334,7 +1311,7 @@ pub mod pallet { // This check is also defensive in cases where the unbond pool does not update its // balance (e.g. a bug in the slashing hook.) We gracefully proceed in // order to ensure delegators can leave the pool and it can be destroyed. - .min(bonded_pool.non_locked_balance()); + .min(bonded_pool.transferrable_balance()); T::Currency::transfer( &bonded_pool.bonded_account(), @@ -1368,7 +1345,7 @@ pub mod pallet { Zero::zero() ); debug_assert_eq!( - T::StakingInterface::locked_balance(&bonded_pool.bonded_account()) + T::StakingInterface::total_stake(&bonded_pool.bonded_account()) .unwrap_or_default(), Zero::zero() ); @@ -1462,7 +1439,7 @@ pub mod pallet { pub fn nominate( origin: OriginFor, pool_id: PoolId, - validators: Vec<::Source>, + validators: Vec, ) -> DispatchResult { let who = ensure_signed(origin)?; let bonded_pool = BondedPool::::get(pool_id).ok_or(Error::::PoolNotFound)?; @@ -1568,6 +1545,12 @@ pub mod pallet { #[pallet::hooks] impl Hooks> for Pallet { fn integrity_test() { + assert!( + sp_std::mem::size_of::() >= + 2 * sp_std::mem::size_of::>(), + "bit-length of the reward points must be at least twice as much as balance" + ); + assert!( T::StakingInterface::bonding_duration() < TotalUnbondingPools::::get(), "There must be more unbonding pools then the bonding duration / @@ -1580,16 +1563,15 @@ pub mod pallet { impl Pallet { /// Create the main, bonded account of a pool with the given id. - pub fn create_bonded_account(id: PoolId) -> T::AccountId { - // 4bytes ++ 8bytes ++ 1byte ++ 4bytes - T::PalletId::get().into_sub_account((1u8, id)) + 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. - pub fn create_reward_account(id: PoolId) -> T::AccountId { - // TODO: integrity check for what is the reasonable max number of pools based on this. - // 4bytes ++ 8bytes ++ 1byte ++ 4bytes - T::PalletId::get().into_sub_account((2u8, id)) + 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)) } /// Calculate the number of points to issue from a pool as `(current_points / current_balance) * @@ -1637,6 +1619,9 @@ impl Pallet { } /// Calculate the rewards for `delegator`. + /// + /// Returns the payout amount, and whether the pool state has been switched to destroying during + /// this call. fn calculate_delegator_payout( bonded_pool: &mut BondedPool, reward_pool: &mut RewardPool, @@ -1693,9 +1678,6 @@ impl Pallet { }; // Record updates - if reward_pool.total_earnings == BalanceOf::::max_value() { - bonded_pool.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); @@ -1706,7 +1688,7 @@ impl Pallet { /// If the delegator has some rewards, transfer a payout from the reward pool to the delegator. /// /// # Note - /// + // TODO: revise this. /// This will persist updates for the reward pool to storage. But it will *not* persist updates /// to the `delegator` or `bonded_pool` to storage, that is the responsibility of the caller. fn do_reward_payout( @@ -1729,6 +1711,16 @@ impl Pallet { ExistenceRequirement::AllowDeath, )?; + if reward_pool.total_earnings == BalanceOf::::max_value() && + bonded_pool.state != PoolState::Destroying + { + bonded_pool.state = PoolState::Destroying; + Self::deposit_event(Event::::State { + pool_id: delegator.pool_id, + new_state: PoolState::Destroying, + }); + } + Self::deposit_event(Event::::PaidOut { delegator: delegator_account, pool_id: delegator.pool_id, @@ -1740,6 +1732,106 @@ impl Pallet { Ok(()) } + + /// Ensure the correctness of the state of this pallet. + /// + /// This should be valid before or after each state transition of this pallet. + /// + /// ## Invariants: + /// + /// First, let's consider pools: + /// + /// * `BondedPools` and `RewardPools` must all have the EXACT SAME key-set. + /// * `SubPoolsStorage` must be a subset of the above superset. + /// * `Metadata` keys must be a subset of the above superset. + /// * the count of the above set must be less than `MaxPools`. + /// + /// Then, considering delegators as well: + /// + /// * each `BondedPool.delegator_counter` must be: + /// - correct (compared to actual count of delegator who have `.pool_id` this pool) + /// - less than `MaxDelegatorsPerPool`. + /// * each `delegator.pool_id` must correspond to an existing `BondedPool.id` (which implies the + /// existence of the reward pool as well). + /// * count of all delegators must be less than `MaxDelegators`. + /// + /// Then, considering unbonding delegators: + /// + /// for each pool: + /// * sum of the balance that's tracked in all unbonding pools must be the same as the + /// unbonded balance of the main account, as reported by the staking interface. + /// * sum of the balance that's tracked in all unbonding pools, plus the bonded balance of the + /// main account should be less than or qual to the total balance of the main account. + /// + /// ## Sanity check level + /// + /// To cater for tests that want to escape parts of these checks, this function is split into + /// multiple `level`s, where the higher the level, the more checks we performs. So, + /// `sanity_check(255)` is the strongest sanity check, and `0` performs no checks. + #[cfg(any(test, debug_assertions))] + pub fn sanity_checks(level: u8) -> Result<(), &'static str> { + if level.is_zero() { + return Ok(()) + } + // note: while a bit wacky, since they have the same key, even collecting to vec should + // result in the same set of keys, in the same order. + let bonded_pools = BondedPools::::iter_keys().collect::>(); + let reward_pools = RewardPools::::iter_keys().collect::>(); + assert_eq!(bonded_pools, reward_pools); + + assert!(Metadata::::iter_keys().all(|k| bonded_pools.contains(&k))); + assert!(SubPoolsStorage::::iter_keys().all(|k| bonded_pools.contains(&k))); + + assert!(MaxPools::::get().map_or(true, |max| bonded_pools.len() <= (max as usize))); + + let mut pools_delegators = BTreeMap::::new(); + let mut all_delegators = 0u32; + Delegators::::iter().for_each(|(_, d)| { + assert!(BondedPools::::contains_key(d.pool_id)); + *pools_delegators.entry(d.pool_id).or_default() += 1; + all_delegators += 1; + }); + + BondedPools::::iter().for_each(|(id, bonded_pool)| { + assert_eq!( + pools_delegators.get(&id).map(|x| *x).unwrap_or_default(), + bonded_pool.delegator_counter + ); + assert!(MaxDelegatorsPerPool::::get() + .map_or(true, |max| bonded_pool.delegator_counter <= max)); + }); + assert!(MaxDelegators::::get().map_or(true, |max| all_delegators <= max)); + + if level <= u8::MAX / 2 { + return Ok(()) + } + + for (pool_id, _) in BondedPools::::iter() { + let pool_account = Pallet::::create_bonded_account(pool_id); + let subs = SubPoolsStorage::::get(pool_id).unwrap_or_default(); + let sum_unbonding_balance = subs + .with_era + .into_iter() + .map(|(_, v)| v) + .chain(sp_std::iter::once(subs.no_era)) + .map(|unbond_pool| unbond_pool.balance) + .fold(Zero::zero(), |a, b| a + b); + + let bonded_balance = + T::StakingInterface::active_stake(&pool_account).unwrap_or_default(); + let total_balance = T::Currency::total_balance(&pool_account); + + assert!( + total_balance >= bonded_balance + sum_unbonding_balance, + "total_balance {:?} >= bonded_balance {:?} + sum_unbonding_balance {:?}", + total_balance, + bonded_balance, + sum_unbonding_balance + ); + } + + Ok(()) + } } impl OnStakerSlash> for Pallet { diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 0216df5a01faf..d254eafb80020 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -34,7 +34,6 @@ impl StakingMock { impl sp_staking::StakingInterface for StakingMock { type Balance = Balance; type AccountId = AccountId; - type LookupSource = Self::AccountId; fn minimum_bond() -> Self::Balance { 10 @@ -48,11 +47,11 @@ impl sp_staking::StakingInterface for StakingMock { BondingDuration::get() } - fn bonded_balance(who: &Self::AccountId) -> Option { + fn active_stake(who: &Self::AccountId) -> Option { BondedBalanceMap::get().get(who).map(|v| *v) } - fn locked_balance(who: &Self::AccountId) -> Option { + fn total_stake(who: &Self::AccountId) -> Option { match ( UnbondingBalanceMap::get().get(who).map(|v| *v), BondedBalanceMap::get().get(who).map(|v| *v), @@ -92,7 +91,7 @@ impl sp_staking::StakingInterface for StakingMock { Ok(()) } - fn nominate(_: Self::AccountId, nominations: Vec) -> DispatchResult { + fn nominate(_: Self::AccountId, nominations: Vec) -> DispatchResult { Nominations::set(nominations); Ok(()) } @@ -158,6 +157,7 @@ impl Convert for U256ToBalance { parameter_types! { pub static PostUnbondingPoolsWindow: u32 = 2; pub static MaxMetadataLen: u32 = 2; + pub static CheckLevel: u8 = 255; pub const PoolsPalletId: PalletId = PalletId(*b"py/nopls"); } impl pools::Config for Runtime { @@ -198,6 +198,11 @@ impl ExtBuilder { self } + pub(crate) fn with_check(self, level: u8) -> Self { + CheckLevel::set(level); + self + } + pub(crate) fn build(self) -> sp_io::TestExternalities { // sp_tracing::try_init_simple(); let mut storage = @@ -233,13 +238,7 @@ impl ExtBuilder { pub fn build_and_execute(self, test: impl FnOnce() -> ()) { self.build().execute_with(|| { test(); - crate::sanity::checks::(); - }) - } - - pub fn build_and_execute_no_checks(self, test: impl FnOnce() -> ()) { - self.build().execute_with(|| { - test(); + Pools::sanity_checks(CheckLevel::get()).unwrap(); }) } } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 0a024a298623c..11aa5bd0442c7 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -407,7 +407,7 @@ mod join { #[test] fn join_errors_correctly() { - ExtBuilder::default().build_and_execute_no_checks(|| { + ExtBuilder::default().with_check(0).build_and_execute(|| { // 10 is already part of the default pool created. assert_eq!(Delegators::::get(&10).unwrap().pool_id, 1); @@ -1322,7 +1322,7 @@ mod unbond { } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 0); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); }); } @@ -1357,7 +1357,7 @@ mod unbond { } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 94); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 94); assert_eq!(Delegators::::get(40).unwrap().unbonding_era, Some(0 + 3)); assert_eq!(Balances::free_balance(&40), 40 + 40); // We claim rewards when unbonding @@ -1382,7 +1382,7 @@ mod unbond { } } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 2); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 2); assert_eq!(Delegators::::get(550).unwrap().unbonding_era, Some(0 + 3)); assert_eq!(Balances::free_balance(&550), 550 + 550); @@ -1406,7 +1406,7 @@ mod unbond { } } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 0); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); assert_eq!(Delegators::::get(550).unwrap().unbonding_era, Some(0 + 3)); assert_eq!(Balances::free_balance(&550), 550 + 550); }); @@ -1414,7 +1414,7 @@ mod unbond { #[test] fn unbond_other_merges_older_pools() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().with_check(1).build_and_execute(|| { // Given assert_eq!(StakingMock::bonding_duration(), 3); SubPoolsStorage::::insert( @@ -1487,7 +1487,7 @@ mod unbond { } } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 10); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 10); assert_eq!( SubPoolsStorage::::get(1).unwrap(), SubPools { @@ -1555,7 +1555,7 @@ mod unbond { } } ); - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()).unwrap(), 0); + assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); assert_eq!( UNBONDING_BALANCE_MAP .with(|m| *m.borrow_mut().get(&default_bonded_account()).unwrap()), @@ -1622,16 +1622,16 @@ mod pool_withdraw_unbonded { // Given 10 unbond'ed directly against the pool account assert_ok!(StakingMock::unbond(default_bonded_account(), 5)); // and the pool account only has 10 balance - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()), Some(5)); - assert_eq!(StakingMock::locked_balance(&default_bonded_account()), Some(10)); + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Some(5)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Some(10)); assert_eq!(Balances::free_balance(&default_bonded_account()), 10); // When assert_ok!(Pools::pool_withdraw_unbonded(Origin::signed(10), 1, 0)); // Then there unbonding balance is no longer locked - assert_eq!(StakingMock::bonded_balance(&default_bonded_account()), Some(5)); - assert_eq!(StakingMock::locked_balance(&default_bonded_account()), Some(5)); + assert_eq!(StakingMock::active_stake(&default_bonded_account()), Some(5)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Some(5)); assert_eq!(Balances::free_balance(&default_bonded_account()), 10); }); } @@ -1739,7 +1739,7 @@ mod withdraw_unbonded_other { // Given StakingMock::set_bonded_balance(default_bonded_account(), 100); // slash bonded balance Balances::make_free_balance_be(&default_bonded_account(), 100); - assert_eq!(StakingMock::locked_balance(&default_bonded_account()), Some(100)); + assert_eq!(StakingMock::total_stake(&default_bonded_account()), Some(100)); assert_ok!(Pools::unbond_other(Origin::signed(40), 40)); assert_ok!(Pools::unbond_other(Origin::signed(550), 550)); @@ -1825,7 +1825,7 @@ mod withdraw_unbonded_other { #[test] fn withdraw_unbonded_other_errors_correctly() { - ExtBuilder::default().build_and_execute_no_checks(|| { + ExtBuilder::default().with_check(0).build_and_execute(|| { // Insert the sub-pool let sub_pools = SubPools { no_era: Default::default(), @@ -2099,7 +2099,7 @@ mod create { assert!(!BondedPools::::contains_key(2)); assert!(!RewardPools::::contains_key(2)); assert!(!Delegators::::contains_key(11)); - assert_eq!(StakingMock::bonded_balance(&next_pool_stash), None); + assert_eq!(StakingMock::active_stake(&next_pool_stash), None); Balances::make_free_balance_be(&11, StakingMock::minimum_bond()); assert_ok!(Pools::create( @@ -2138,7 +2138,7 @@ mod create { } ); assert_eq!( - StakingMock::bonded_balance(&next_pool_stash).unwrap(), + StakingMock::active_stake(&next_pool_stash).unwrap(), StakingMock::minimum_bond() ); assert_eq!( @@ -2154,7 +2154,7 @@ mod create { #[test] fn create_errors_correctly() { - ExtBuilder::default().build_and_execute_no_checks(|| { + ExtBuilder::default().with_check(0).build_and_execute(|| { assert_noop!( Pools::create(Origin::signed(10), 420, 123, 456, 789), Error::::AccountBelongsToOtherPool diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index 593a362f8d9a1..686b023bc91d3 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -1318,7 +1318,6 @@ impl SortedListProvider for UseNominatorsMap { impl StakingInterface for Pallet { type AccountId = T::AccountId; type Balance = BalanceOf; - type LookupSource = ::Source; fn minimum_bond() -> Self::Balance { MinNominatorBond::::get() @@ -1332,11 +1331,11 @@ impl StakingInterface for Pallet { Self::current_era().unwrap_or(Zero::zero()) } - fn bonded_balance(controller: &Self::AccountId) -> Option { + fn active_stake(controller: &Self::AccountId) -> Option { Self::ledger(controller).map(|l| l.active) } - fn locked_balance(controller: &Self::AccountId) -> Option { + fn total_stake(controller: &Self::AccountId) -> Option { Self::ledger(controller).map(|l| l.total) } @@ -1375,7 +1374,8 @@ impl StakingInterface for Pallet { ) } - fn nominate(controller: Self::AccountId, targets: Vec) -> DispatchResult { + fn nominate(controller: Self::AccountId, targets: Vec) -> DispatchResult { + let targets = targets.into_iter().map(T::Lookup::unlookup).collect::>(); Self::nominate(RawOrigin::Signed(controller).into(), targets) } } diff --git a/primitives/staking/src/lib.rs b/primitives/staking/src/lib.rs index a49bfa8d41548..24cf794ee8436 100644 --- a/primitives/staking/src/lib.rs +++ b/primitives/staking/src/lib.rs @@ -61,9 +61,6 @@ pub trait StakingInterface { /// AccountId type used by the staking system type AccountId; - /// The type for the `validators` argument to `Self::nominate`. - type LookupSource; - /// The minimum amount required to bond in order to be a nominator. This does not necessarily /// mean the nomination will be counted in an election, but instead just enough to be stored as /// a nominator. In other words, this is the minimum amount to register the intention to @@ -82,17 +79,17 @@ pub trait StakingInterface { /// This should be the latest planned era that the staking system knows about. fn current_era() -> EraIndex; - /// Balance `controller` has bonded for nominating. - fn bonded_balance(controller: &Self::AccountId) -> Option; + /// The amount of active stake that `controller` has in the staking system. + fn active_stake(controller: &Self::AccountId) -> Option; - /// Balance the _Stash_ linked to `controller` has locked by the staking system. This should - /// include both the users bonded funds and their unlocking funds. + /// The total stake that `controller` has in the staking system. This includes the + /// [`active_stake`], and any funds currently in the process of unbonding via [`unbond`]. /// /// # Note /// /// This is only guaranteed to reflect the amount locked by the staking system. If there are /// non-staking locks on the bonded pair's balance this may not be accurate. - fn locked_balance(controller: &Self::AccountId) -> Option; + fn total_stake(controller: &Self::AccountId) -> Option; /// Bond (lock) `value` of `stash`'s balance. `controller` will be set as the account /// controlling `stash`. This creates what is referred to as "bonded pair". @@ -106,7 +103,7 @@ pub trait StakingInterface { /// Have `controller` nominate `validators`. fn nominate( controller: Self::AccountId, - validators: sp_std::vec::Vec, + validators: sp_std::vec::Vec, ) -> DispatchResult; /// Bond some extra amount in the _Stash_'s free balance against the active bonded balance of