From 616dc1add37b26b87e83accbd7715c246dbbe044 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 10 Apr 2022 09:11:59 +0200 Subject: [PATCH 01/33] first draft of partial unbonding for pools --- frame/nomination-pools/src/lib.rs | 313 ++++++++++++++++++++-------- frame/nomination-pools/src/mock.rs | 19 +- frame/nomination-pools/src/tests.rs | 161 +++++++------- frame/support/src/lib.rs | 20 ++ frame/support/src/traits/misc.rs | 2 +- 5 files changed, 348 insertions(+), 167 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index bcabda6586021..40a03e6796433 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -305,7 +305,7 @@ use codec::Codec; use frame_support::{ - ensure, + defensive, ensure, pallet_prelude::{MaxEncodedLen, *}, storage::bounded_btree_map::BoundedBTreeMap, traits::{ @@ -316,7 +316,7 @@ use frame_support::{ }; use scale_info::TypeInfo; use sp_core::U256; -use sp_runtime::traits::{AccountIdConversion, Bounded, Convert, Saturating, Zero}; +use sp_runtime::traits::{AccountIdConversion, Bounded, CheckedSub, Convert, Saturating, Zero}; use sp_staking::{EraIndex, OnStakerSlash, StakingInterface}; use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, ops::Div, vec::Vec}; @@ -393,8 +393,90 @@ pub struct Delegator { /// This value lines up with the [`RewardPool::total_earnings`] after a delegator claims a /// payout. pub reward_pool_total_earnings: BalanceOf, - /// The era this delegator started unbonding at. - pub unbonding_era: Option, + /// The eras as which this delegator is unbonding. + /// + /// `None` means to funds are unbonding. `Some(_)` means a set of eras at which the user's + /// funds will be unlocked (not the era at which the unbonding was initiated!), mapped to the + /// amount of points being unbonded. + pub unbonding_eras: Option, T::MaxUnbonding>>, +} + +impl Delegator { + /// Total points of the delegator, both active and unbonding. + pub(crate) fn total_points(&self) -> BalanceOf { + self.unbonding_points().saturating_add(self.active_points()) + } + + pub(crate) fn active_points(&self) -> BalanceOf { + self.points + } + + pub(crate) fn unbonding_points(&self) -> BalanceOf { + self.unbonding_eras + .as_ref() + .map(|inner| { + inner.iter().fold(BalanceOf::::zero(), |acc, (_, v)| acc.saturating_add(*v)) + }) + .unwrap_or_default() + } + + /// Try and unbond `points` from self, with the given target unbonding era. + /// + /// Returns `Ok(())` and updates `unbonding_eras` and `points` if success, `Err(_)` otherwise. + fn try_unbond( + &mut self, + points: BalanceOf, + unbonding_era: EraIndex, + ) -> Result<(), Error> { + let mut current_unbonding_eras = self.unbonding_eras.clone().unwrap_or_default(); + if let Some(new_points) = self.points.checked_sub(&points) { + ensure!( + !current_unbonding_eras.contains_key(&unbonding_era), + Error::::AlreadyUnbonding + ); + current_unbonding_eras.try_insert(unbonding_era, points).map_or( + Err(Error::::MaxUnbonding), + |maybe_old| { + if maybe_old.is_some() { + defensive!("map is checked to not contain this kye."); + } + self.points = new_points; + self.unbonding_eras = Some(current_unbonding_eras); + Ok(()) + }, + ) + } else { + Err(Error::::NotEnoughPointsToUnbond) + } + } + + /// Withdraw any funds in [`Self::unbonding_eras`] who's deadline in reached and is fully + /// unlocked. + /// + /// Returns a a subset of [`Self::unbonding_eras`] that got withdrawn. + /// + /// Infallible, noop if no unbonding eras exist. + fn withdraw_unlocked( + &mut self, + current_era: EraIndex, + ) -> BoundedBTreeMap, T::MaxUnbonding> { + // NOTE: if only drain-filter was stable.. + let mut removed_points = + BoundedBTreeMap::, T::MaxUnbonding>::default(); + if let Some(unbonding_eras) = self.unbonding_eras.as_mut() { + unbonding_eras.retain(|e, p| { + if *e > current_era { + true + } else { + removed_points + .try_insert(*e, p.clone()) + .expect("source map is bounded, this is a subset, will be bounded; qed"); + false + } + }) + } + removed_points + } } /// A pool's possible states. @@ -528,6 +610,20 @@ impl BondedPool { points_to_issue } + /// Dissolve i.e. unbond the given amount of points from this pool. This is the opposite of + /// issuing some funds into the pool. + /// + /// Mutates self in place, but does not write anything to storage. + /// + /// Returns the equivalent balance amount that actually needs to get unbonded. + fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { + // NOTE: do not optimize by removing `balance`. it must be computed before mutating + // `self.point`. + let balance = self.balance_to_unbond(points); + self.points = self.points.saturating_sub(points); + balance + } + /// Increment the delegator counter. Ensures that the pool and system delegator limits are /// respected. fn try_inc_delegators(&mut self) -> Result<(), DispatchError> { @@ -638,7 +734,10 @@ impl BondedPool { // destroying it cannot switch states, so by being in destroying we are guaranteed no // other delegators can possibly join. (_, true) => { - ensure!(target_delegator.points == self.points, Error::::NotOnlyDelegator); + ensure!( + target_delegator.active_points() == self.points, + Error::::NotOnlyDelegator + ); ensure!(self.is_destroying(), Error::::NotDestroying); }, }; @@ -661,7 +760,7 @@ impl BondedPool { // 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, + sub_pools.no_era.points == target_delegator.unbonding_points(), Error::::NotOnlyDelegator ); } else { @@ -675,7 +774,7 @@ impl BondedPool { .values() .next() .filter(|only_unbonding_pool| { - only_unbonding_pool.points == target_delegator.points + only_unbonding_pool.points == target_delegator.unbonding_points() }) .ok_or(Error::::NotOnlyDelegator)?; } @@ -804,12 +903,15 @@ impl RewardPool { } } +/// An unbonding pool. This is always mapped with an era. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DefaultNoBound, RuntimeDebugNoBound)] #[cfg_attr(feature = "std", derive(Clone, PartialEq))] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct UnbondPool { + /// The points in this pool. points: BalanceOf, + /// The funds in the pool. balance: BalanceOf, } @@ -827,6 +929,20 @@ impl UnbondPool { self.points = self.points.saturating_add(self.points_to_issue(new_funds)); self.balance = self.balance.saturating_add(new_funds); } + + /// Dissolve some points from the unbonding pool, reducing the balance of the pool + /// proportionally. + /// + /// This is the opposite of `issue`. + /// + /// Returns the actual amount of `Balance` that was removed from the pool. + fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { + let balance_to_unbond = self.balance_to_unbond(points); + self.points = self.points.saturating_sub(points); + self.balance = self.balance.saturating_sub(balance_to_unbond); + + balance_to_unbond + } } #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DefaultNoBound, RuntimeDebugNoBound)] @@ -929,6 +1045,9 @@ pub mod pallet { /// The maximum length, in bytes, that a pools metadata maybe. type MaxMetadataLen: Get; + + /// The maximum number of simultaneous unbonding chunks that can exist per delegator. + type MaxUnbonding: Get; } /// Minimum amount to bond to join a pool. @@ -1061,8 +1180,13 @@ pub mod pallet { AccountBelongsToOtherPool, /// The pool has insufficient balance to bond as a nominator. InsufficientBond, - /// The delegator is already unbonding. + /// The delegator is already unbonding in this era. AlreadyUnbonding, + /// The delegator is fully unbonded (and thus cannot access the bonded and reward pool + /// anymore to, for example, collect rewards). + FullyUnbonding, + /// The delegator cannot unbond further chunks due to reaching the limit. + MaxUnbonding, /// The delegator is not unbonding and thus cannot withdraw funds. NotUnbonding, /// Unbonded funds cannot be withdrawn yet because the bonding duration has not passed. @@ -1098,6 +1222,8 @@ pub mod pallet { DefensiveError, /// The caller has insufficient balance to create the pool. InsufficientBalanceToCreate, + /// Not enough points. Ty unbonding less. + NotEnoughPointsToUnbond, } #[pallet::call] @@ -1143,7 +1269,7 @@ pub mod pallet { // next 2 eras because their vote weight will not be counted until the // snapshot in active era + 1. reward_pool_total_earnings: reward_pool.total_earnings, - unbonding_era: None, + unbonding_eras: None, }, ); bonded_pool.put(); @@ -1220,12 +1346,14 @@ pub mod pallet { Ok(()) } - /// Unbond _all_ of the `delegator_account`'s funds from the pool. + /// Unbond up to `unbonding_points` of the `delegator_account`'s funds from the pool. It + /// implicitly collects the rewards one last time, since not doing so would mean some + /// rewards would go forfeited. /// /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any /// account). /// - /// # Conditions for a permissionless dispatch + /// ## Conditions for a permissionless dispatch. /// /// * The pool is blocked and the caller is either the root or state-toggler. This is /// refereed to as a kick. @@ -1233,7 +1361,7 @@ pub mod pallet { /// * The pool is destroying, the delegator is the depositor and no other delegators are in /// the pool. /// - /// # Conditions for permissioned dispatch (i.e. the caller is also the + /// ## Conditions for permissioned dispatch (i.e. the caller is also the /// `delegator_account`): /// /// * The caller is not the depositor. @@ -1251,16 +1379,15 @@ pub mod pallet { pub fn unbond_other( origin: OriginFor, delegator_account: T::AccountId, + mut unbonding_points: BalanceOf, ) -> DispatchResult { let caller = ensure_signed(origin)?; let (mut delegator, mut bonded_pool, mut reward_pool) = Self::get_delegator_with_pools(&delegator_account)?; - bonded_pool.ok_to_unbond_other_with(&caller, &delegator_account, &delegator)?; - // 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. + unbonding_points = unbonding_points.min(delegator.active_points()); + // This is only for UX. Self::do_reward_payout( &delegator_account, &mut delegator, @@ -1268,17 +1395,16 @@ pub mod pallet { &mut reward_pool, )?; - let balance_to_unbond = bonded_pool.balance_to_unbond(delegator.points); + let current_era = T::StakingInterface::current_era(); + let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era); - // Update the bonded pool. Note that we must do this *after* calculating the balance - // to unbond so we have the correct points for the balance:share ratio. - bonded_pool.points = bonded_pool.points.saturating_sub(delegator.points); + // Try and unbond in the delegator map. + delegator.try_unbond(unbonding_points, unbond_era)?; - // Unbond in the actual underlying pool - T::StakingInterface::unbond(bonded_pool.bonded_account(), balance_to_unbond)?; + // Unbond in the actual underlying nominator. + let unbonding_balance = bonded_pool.dissolve(unbonding_points); + T::StakingInterface::unbond(bonded_pool.bonded_account(), unbonding_balance)?; - let current_era = T::StakingInterface::current_era(); - let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era); // Note that we lazily create the unbonding pools here if they don't already exist let mut sub_pools = SubPoolsStorage::::get(delegator.pool_id) .unwrap_or_default() @@ -1294,19 +1420,18 @@ pub mod pallet { // always enough space to insert. .defensive_map_err(|_| Error::::DefensiveError)?; } + sub_pools .with_era .get_mut(&unbond_era) // The above check ensures the pool exists. .defensive_ok_or_else(|| Error::::DefensiveError)? - .issue(balance_to_unbond); - - delegator.unbonding_era = Some(unbond_era); + .issue(unbonding_balance); Self::deposit_event(Event::::Unbonded { delegator: delegator_account.clone(), pool_id: delegator.pool_id, - amount: balance_to_unbond, + amount: unbonding_balance, }); // Now that we know everything has worked write the items to storage. @@ -1338,8 +1463,11 @@ pub mod pallet { Ok(()) } - /// Withdraw unbonded funds for the `target` delegator. Under certain conditions, - /// this call can be dispatched permissionlessly (i.e. by any account). + /// Withdraw unbonded funds who's unbonding period has passed for the `target` delegator. If + /// no bond can be unbonded, this is a successful no-op. + /// + /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any + /// account). /// /// # Conditions for a permissionless dispatch /// @@ -1364,16 +1492,15 @@ pub mod pallet { num_slashing_spans: u32, ) -> DispatchResultWithPostInfo { let caller = ensure_signed(origin)?; - let delegator = + let mut delegator = Delegators::::get(&delegator_account).ok_or(Error::::DelegatorNotFound)?; - let unbonding_era = delegator.unbonding_era.ok_or(Error::::NotUnbonding)?; let current_era = T::StakingInterface::current_era(); - ensure!(current_era >= unbonding_era, Error::::NotUnbondedYet); - let mut sub_pools = SubPoolsStorage::::get(delegator.pool_id) - .defensive_ok_or_else(|| Error::::SubPoolsNotFound)?; let bonded_pool = BondedPool::::get(delegator.pool_id) .defensive_ok_or_else(|| Error::::PoolNotFound)?; + let mut sub_pools = SubPoolsStorage::::get(delegator.pool_id) + .defensive_ok_or_else(|| Error::::SubPoolsNotFound)?; + let should_remove_pool = bonded_pool.ok_to_withdraw_unbonded_other_with( &caller, &delegator_account, @@ -1381,6 +1508,9 @@ pub mod pallet { &sub_pools, )?; + // NOTE: must do this after we have done the `ok_to_withdraw_unbonded_other_with` check. + let withdrawn_points = delegator.withdraw_unlocked(current_era); + // Before calculate the `balance_to_unbond`, with call withdraw unbonded to ensure the // `non_locked_balance` is correct. T::StakingInterface::withdraw_unbonded( @@ -1388,35 +1518,28 @@ pub mod pallet { num_slashing_spans, )?; - let balance_to_unbond = - if let Some(pool) = sub_pools.with_era.get_mut(&unbonding_era) { - let balance_to_unbond = pool.balance_to_unbond(delegator.points); - pool.points = pool.points.saturating_sub(delegator.points); - pool.balance = pool.balance.saturating_sub(balance_to_unbond); - if pool.points.is_zero() { - // Clean up pool that is no longer used - sub_pools.with_era.remove(&unbonding_era); + let balance_to_unbond = withdrawn_points + .iter() + .fold(BalanceOf::::zero(), |accumulator, (era, unlocked_points)| { + if let Some(era_pool) = sub_pools.with_era.get_mut(&era) { + let balance_to_unbond = era_pool.dissolve(*unlocked_points); + if era_pool.points.is_zero() { + sub_pools.with_era.remove(&era); + } + accumulator.saturating_add(balance_to_unbond) + } else { + // A pool does not belong to this era, so it must have been merged to the + // era-less pool. + accumulator.saturating_add(sub_pools.no_era.dissolve(*unlocked_points)) } - - balance_to_unbond - } else { - // A pool does not belong to this era, so it must have been merged to the - // era-less pool. - let balance_to_unbond = sub_pools.no_era.balance_to_unbond(delegator.points); - sub_pools.no_era.points = - sub_pools.no_era.points.saturating_sub(delegator.points); - sub_pools.no_era.balance = - sub_pools.no_era.balance.saturating_sub(balance_to_unbond); - - balance_to_unbond - } + }) // A call to this function may cause the pool's stash to get dusted. If this happens // before the last delegator has withdrawn, then all subsequent withdraws will be 0. // However the unbond pools do no get updated to reflect this. In the aforementioned // scenario, this check ensures we don't try to withdraw funds that don't exist. // 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. + // 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.transferrable_balance()); T::Currency::transfer( @@ -1425,40 +1548,49 @@ pub mod pallet { balance_to_unbond, ExistenceRequirement::AllowDeath, ) - .defensive_map_err(|e| e)?; + .defensive()?; + Self::deposit_event(Event::::Withdrawn { delegator: delegator_account.clone(), pool_id: delegator.pool_id, amount: balance_to_unbond, }); - let post_info_weight = if should_remove_pool { - ReversePoolIdLookup::::remove(bonded_pool.bonded_account()); - RewardPools::::remove(delegator.pool_id); - Self::deposit_event(Event::::Destroyed { pool_id: delegator.pool_id }); - SubPoolsStorage::::remove(delegator.pool_id); - // Kill accounts from storage by making their balance go below ED. We assume that - // the accounts have no references that would prevent destruction once we get to - // this point. - debug_assert_eq!( - T::Currency::free_balance(&bonded_pool.bonded_account()), - Zero::zero() - ); - debug_assert_eq!( - T::StakingInterface::total_stake(&bonded_pool.bonded_account()) - .unwrap_or_default(), - Zero::zero() - ); - T::Currency::make_free_balance_be(&bonded_pool.reward_account(), Zero::zero()); - T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); - bonded_pool.remove(); - None + let post_info_weight = if delegator.active_points().is_zero() { + // delegator being reaped. + Delegators::::remove(&delegator_account); + if should_remove_pool { + ReversePoolIdLookup::::remove(bonded_pool.bonded_account()); + RewardPools::::remove(delegator.pool_id); + Self::deposit_event(Event::::Destroyed { pool_id: delegator.pool_id }); + SubPoolsStorage::::remove(delegator.pool_id); + // Kill accounts from storage by making their balance go below ED. We assume + // that the accounts have no references that would prevent destruction once we + // get to this point. + debug_assert_eq!( + T::Currency::free_balance(&bonded_pool.bonded_account()), + Zero::zero() + ); + debug_assert_eq!( + T::StakingInterface::total_stake(&bonded_pool.bonded_account()) + .unwrap_or_default(), + Zero::zero() + ); + T::Currency::make_free_balance_be(&bonded_pool.reward_account(), Zero::zero()); + T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); + bonded_pool.remove(); + None + } else { + bonded_pool.dec_delegators().put(); + SubPoolsStorage::::insert(&delegator.pool_id, sub_pools); + Some(T::WeightInfo::withdraw_unbonded_other_update(num_slashing_spans)) + } } else { - bonded_pool.dec_delegators().put(); + // we certainly don't need to delete any pools, because no one is being removed. SubPoolsStorage::::insert(&delegator.pool_id, sub_pools); - Some(T::WeightInfo::withdraw_unbonded_other_update(num_slashing_spans)) + Delegators::::insert(&delegator_account, delegator); + None }; - Delegators::::remove(&delegator_account); Ok(post_info_weight.into()) } @@ -1536,7 +1668,7 @@ pub mod pallet { pool_id, points, reward_pool_total_earnings: Zero::zero(), - unbonding_era: None, + unbonding_eras: None, }, ); RewardPools::::insert( @@ -1759,9 +1891,9 @@ impl Pallet { /// Returns the payout amount, and whether the pool state has been switched to destroying during /// this call. fn calculate_delegator_payout( + delegator: &mut Delegator, bonded_pool: &mut BondedPool, reward_pool: &mut RewardPool, - delegator: &mut Delegator, ) -> Result, DispatchError> { // Presentation Notes: // Reward pool points @@ -1776,9 +1908,6 @@ impl Pallet { let u256 = |x| T::BalanceToU256::convert(x); let balance = |x| T::U256ToBalance::convert(x); - // If the delegator is unbonding they cannot claim rewards. Note that when the delegator - // goes to unbond, the unbond function should claim rewards for the final time. - ensure!(delegator.unbonding_era.is_none(), Error::::AlreadyUnbonding); let last_total_earnings = reward_pool.total_earnings; reward_pool.update_total_earnings_and_balance(bonded_pool.id); @@ -1804,7 +1933,7 @@ impl Pallet { // The points of the reward pool that belong to the delegator. let delegator_virtual_points = // The delegators portion of the reward pool - u256(delegator.points) + u256(delegator.active_points()) // times the amount the pool has earned since the delegator last claimed. .saturating_mul(u256(new_earnings_since_last_claim)); @@ -1840,17 +1969,19 @@ impl Pallet { /// If the delegator has some rewards, transfer a payout from the reward pool to the delegator. // Emits events and potentially modifies pool state if any arithmetic saturates, but does // not persist any of the mutable inputs to storage. - fn do_reward_payout( + pub(crate) fn do_reward_payout( delegator_account: &T::AccountId, delegator: &mut Delegator, bonded_pool: &mut BondedPool, reward_pool: &mut RewardPool, ) -> Result, DispatchError> { debug_assert_eq!(delegator.pool_id, bonded_pool.id); + // a delegator who has no skin in the game anymore cannot claim any rewards. + ensure!(!delegator.active_points().is_zero(), Error::::FullyUnbonding); let was_destroying = bonded_pool.is_destroying(); let delegator_payout = - Self::calculate_delegator_payout(bonded_pool, reward_pool, delegator)?; + Self::calculate_delegator_payout(delegator, bonded_pool, reward_pool)?; // Transfer payout to the delegator. T::Currency::transfer( diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 28b00c3488678..d4a2b50b4997d 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use super::*; use crate::{self as pools}; use frame_support::{assert_ok, parameter_types, PalletId}; @@ -18,10 +20,13 @@ pub fn default_reward_account() -> AccountId { parameter_types! { pub static CurrentEra: EraIndex = 0; - static BondedBalanceMap: std::collections::HashMap = Default::default(); - static UnbondingBalanceMap: std::collections::HashMap = Default::default(); + static BondedBalanceMap: HashMap = Default::default(); + static UnbondingBalanceMap: HashMap = Default::default(); pub static BondingDuration: EraIndex = 3; + #[derive(Clone, PartialEq)] + pub const MaxUnbonding: u32 = 8; pub static Nominations: Vec = vec![]; + } pub struct StakingMock; @@ -170,6 +175,7 @@ impl pools::Config for Runtime { type PostUnbondingPoolsWindow = PostUnbondingPoolsWindow; type PalletId = PoolsPalletId; type MaxMetadataLen = MaxMetadataLen; + type MaxUnbonding = MaxUnbonding; } type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic; @@ -269,6 +275,15 @@ pub(crate) fn pool_events_since_last_call() -> Vec> { events.into_iter().skip(already_seen).collect() } +/// Fully unbond the shares of `delegator`, when executed from `origin`. +/// +/// This is useful for backwards compatibility with the majority of tests that only deal with full +/// unbonding, not partial unbonding. +pub fn fully_unbond_other(origin: Origin, delegator: AccountId) -> DispatchResult { + let points = Delegators::::get(&delegator).map(|d| d.points).unwrap_or_default(); + Pools::unbond_other(origin, delegator, points) +} + #[cfg(test)] mod test { use super::*; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 9dc35761fd7d7..38e7a05030317 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -22,6 +22,7 @@ use frame_support::{ storage::{with_transaction, TransactionOutcome}, }; +// TODO: replace this with the new bounded_btree_map. macro_rules! sub_pools_with_era { ($($k:expr => $v:expr),* $(,)?) => {{ use sp_std::iter::{Iterator, IntoIterator}; @@ -65,7 +66,7 @@ fn test_setup_works() { pool_id: last_pool, points: 10, reward_pool_total_earnings: 0, - unbonding_era: None + unbonding_eras: None } ) }) @@ -412,7 +413,7 @@ mod join { pool_id: 1, points: 2, reward_pool_total_earnings: 0, - unbonding_era: None + unbonding_eras: None } ); assert_eq!(BondedPool::::get(1).unwrap(), bonded(12, 2)); @@ -435,7 +436,7 @@ mod join { pool_id: 1, points: 24, reward_pool_total_earnings: 0, - unbonding_era: None + unbonding_eras: None } ); assert_eq!(BondedPool::::get(1).unwrap(), bonded(12 + 24, 3)); @@ -570,7 +571,7 @@ mod claim_payout { use super::*; fn del(points: Balance, reward_pool_total_earnings: Balance) -> Delegator { - Delegator { pool_id: 1, points, reward_pool_total_earnings, unbonding_era: None } + Delegator { pool_id: 1, points, reward_pool_total_earnings, unbonding_eras: None } } fn rew(balance: Balance, points: u32, total_earnings: Balance) -> RewardPool { @@ -762,7 +763,7 @@ mod claim_payout { #[test] fn do_reward_payout_correctly_sets_pool_state_to_destroying() { ExtBuilder::default().build_and_execute(|| { - with_transaction(|| -> TransactionOutcome { + let _ = with_transaction(|| -> TransactionOutcome { let mut bonded_pool = BondedPool::::get(1).unwrap(); let mut reward_pool = RewardPools::::get(1).unwrap(); let mut delegator = Delegators::::get(10).unwrap(); @@ -787,7 +788,7 @@ mod claim_payout { }); // -- current_points saturates (reward_pool.points + new_earnings * bonded_pool.points) - with_transaction(|| -> TransactionOutcome { + let _ = with_transaction(|| -> TransactionOutcome { // Given let mut bonded_pool = BondedPool::::get(1).unwrap(); let mut reward_pool = RewardPools::::get(1).unwrap(); @@ -816,20 +817,18 @@ mod claim_payout { } #[test] - fn calculate_delegator_payout_errors_if_a_delegator_is_unbonding() { - ExtBuilder::default().build_and_execute(|| { + fn reward_payout_errors_if_a_delegator_is_fully_unbonding() { + ExtBuilder::default().add_delegators(vec![(11, 11)]).build_and_execute(|| { + // fully unbond the delegator. + assert_ok!(fully_unbond_other(Origin::signed(11), 11)); + let mut bonded_pool = BondedPool::::get(1).unwrap(); let mut reward_pool = RewardPools::::get(1).unwrap(); - let mut delegator = Delegators::::get(10).unwrap(); - delegator.unbonding_era = Some(0 + 3); + let mut delegator = Delegators::::get(11).unwrap(); assert_noop!( - Pools::calculate_delegator_payout( - &mut bonded_pool, - &mut reward_pool, - &mut delegator - ), - Error::::AlreadyUnbonding + Pools::do_reward_payout(&11, &mut delegator, &mut bonded_pool, &mut reward_pool,), + Error::::FullyUnbonding ); }); } @@ -847,9 +846,9 @@ mod claim_payout { // Given no rewards have been earned // When let payout = Pools::calculate_delegator_payout( + &mut delegator, &mut bonded_pool, &mut reward_pool, - &mut delegator, ) .unwrap(); @@ -863,9 +862,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut delegator, &mut bonded_pool, &mut reward_pool, - &mut delegator, ) .unwrap(); @@ -879,9 +878,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut delegator, &mut bonded_pool, &mut reward_pool, - &mut delegator, ) .unwrap(); @@ -895,9 +894,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut delegator, &mut bonded_pool, &mut reward_pool, - &mut delegator, ) .unwrap(); @@ -931,9 +930,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_10, &mut bonded_pool, &mut reward_pool, - &mut del_10, ) .unwrap(); @@ -946,9 +945,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_40, &mut bonded_pool, &mut reward_pool, - &mut del_40, ) .unwrap(); @@ -969,9 +968,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_50, &mut bonded_pool, &mut reward_pool, - &mut del_50, ) .unwrap(); @@ -987,9 +986,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_10, &mut bonded_pool, &mut reward_pool, - &mut del_10, ) .unwrap(); @@ -1002,9 +1001,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_40, &mut bonded_pool, &mut reward_pool, - &mut del_40, ) .unwrap(); @@ -1020,9 +1019,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_50, &mut bonded_pool, &mut reward_pool, - &mut del_50, ) .unwrap(); @@ -1046,9 +1045,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_10, &mut bonded_pool, &mut reward_pool, - &mut del_10, ) .unwrap(); @@ -1065,9 +1064,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_10, &mut bonded_pool, &mut reward_pool, - &mut del_10, ) .unwrap(); @@ -1095,9 +1094,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_10, &mut bonded_pool, &mut reward_pool, - &mut del_10, ) .unwrap(); @@ -1110,9 +1109,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_40, &mut bonded_pool, &mut reward_pool, - &mut del_40, ) .unwrap(); @@ -1125,9 +1124,9 @@ mod claim_payout { // When let payout = Pools::calculate_delegator_payout( + &mut del_50, &mut bonded_pool, &mut reward_pool, - &mut del_50, ) .unwrap(); @@ -1372,12 +1371,13 @@ mod claim_payout { mod unbond { use super::*; + use frame_support::bounded_btree_map; #[test] fn unbond_other_of_1_works() { ExtBuilder::default().build_and_execute(|| { unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); assert_eq!( SubPoolsStorage::::get(1).unwrap().with_era, @@ -1413,7 +1413,7 @@ mod unbond { Balances::make_free_balance_be(&default_reward_account(), ed + 600); // When - assert_ok!(Pools::unbond_other(Origin::signed(40), 40)); + assert_ok!(fully_unbond_other(Origin::signed(40), 40)); // Then assert_eq!( @@ -1434,12 +1434,15 @@ mod unbond { ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 94); - assert_eq!(Delegators::::get(40).unwrap().unbonding_era, Some(0 + 3)); + assert_eq!( + Delegators::::get(40).unwrap().unbonding_eras, + Some(bounded_btree_map!(0 + 3 => 40)) + ); assert_eq!(Balances::free_balance(&40), 40 + 40); // We claim rewards when unbonding // When unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(550), 550)); + assert_ok!(fully_unbond_other(Origin::signed(550), 550)); // Then assert_eq!( @@ -1459,11 +1462,14 @@ mod unbond { } ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 2); - assert_eq!(Delegators::::get(550).unwrap().unbonding_era, Some(0 + 3)); + assert_eq!( + Delegators::::get(550).unwrap().unbonding_eras, + Some(bounded_btree_map!(0 + 3 => 550)) + ); assert_eq!(Balances::free_balance(&550), 550 + 550); // When - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); // Then assert_eq!( @@ -1483,7 +1489,10 @@ mod unbond { } ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); - assert_eq!(Delegators::::get(550).unwrap().unbonding_era, Some(0 + 3)); + assert_eq!( + Delegators::::get(550).unwrap().unbonding_eras, + Some(bounded_btree_map!(0 + 3 => 550)) + ); assert_eq!(Balances::free_balance(&550), 550 + 550); }); } @@ -1510,7 +1519,7 @@ mod unbond { let current_era = 1 + TotalUnbondingPools::::get(); CurrentEra::set(current_era); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); // Then assert_eq!( @@ -1541,15 +1550,15 @@ mod unbond { // When the nominator trys to kick, then its a noop assert_noop!( - Pools::unbond_other(Origin::signed(901), 100), + fully_unbond_other(Origin::signed(901), 100), Error::::NotKickerOrDestroying ); // When the root kicks then its ok - assert_ok!(Pools::unbond_other(Origin::signed(900), 100)); + assert_ok!(fully_unbond_other(Origin::signed(900), 100)); // When the state toggler kicks then its ok - assert_ok!(Pools::unbond_other(Origin::signed(902), 200)); + assert_ok!(fully_unbond_other(Origin::signed(902), 200)); assert_eq!( BondedPool::::get(1).unwrap(), @@ -1590,7 +1599,7 @@ mod unbond { // A permissionless unbond attempt errors assert_noop!( - Pools::unbond_other(Origin::signed(420), 100), + fully_unbond_other(Origin::signed(420), 100), Error::::NotKickerOrDestroying ); @@ -1599,19 +1608,19 @@ mod unbond { // The depositor cannot be unbonded until they are the last delegator assert_noop!( - Pools::unbond_other(Origin::signed(420), 10), + fully_unbond_other(Origin::signed(420), 10), Error::::NotOnlyDelegator ); // Any account can unbond a delegator that is not the depositor - assert_ok!(Pools::unbond_other(Origin::signed(420), 100)); + assert_ok!(fully_unbond_other(Origin::signed(420), 100)); // Given the pool is blocked unsafe_set_state(1, PoolState::Blocked).unwrap(); // The depositor cannot be unbonded assert_noop!( - Pools::unbond_other(Origin::signed(420), 10), + fully_unbond_other(Origin::signed(420), 10), Error::::NotDestroying ); @@ -1619,7 +1628,7 @@ mod unbond { unsafe_set_state(1, PoolState::Destroying).unwrap(); // The depositor can be unbonded - assert_ok!(Pools::unbond_other(Origin::signed(420), 10)); + assert_ok!(fully_unbond_other(Origin::signed(420), 10)); assert_eq!(BondedPools::::get(1).unwrap().points, 0); assert_eq!( @@ -1645,7 +1654,7 @@ mod unbond { fn unbond_errors_correctly() { ExtBuilder::default().build_and_execute(|| { assert_noop!( - Pools::unbond_other(Origin::signed(11), 11), + fully_unbond_other(Origin::signed(11), 11), Error::::DelegatorNotFound ); @@ -1654,11 +1663,11 @@ mod unbond { pool_id: 2, points: 10, reward_pool_total_earnings: 0, - unbonding_era: None, + unbonding_eras: None, }; Delegators::::insert(11, delegator); - let _ = Pools::unbond_other(Origin::signed(11), 11); + let _ = fully_unbond_other(Origin::signed(11), 11); }); } @@ -1670,7 +1679,7 @@ mod unbond { pool_id: 2, points: 10, reward_pool_total_earnings: 0, - unbonding_era: None, + unbonding_eras: None, }; Delegators::::insert(11, delegator); BondedPool:: { @@ -1684,7 +1693,7 @@ mod unbond { } .put(); - let _ = Pools::unbond_other(Origin::signed(11), 11); + let _ = fully_unbond_other(Origin::signed(11), 11); }); } } @@ -1714,6 +1723,8 @@ mod pool_withdraw_unbonded { } mod withdraw_unbonded_other { + use frame_support::bounded_btree_map; + use super::*; #[test] @@ -1723,15 +1734,15 @@ mod withdraw_unbonded_other { .build_and_execute(|| { // Given assert_eq!(StakingMock::bonding_duration(), 3); - assert_ok!(Pools::unbond_other(Origin::signed(550), 550)); - assert_ok!(Pools::unbond_other(Origin::signed(40), 40)); + assert_ok!(fully_unbond_other(Origin::signed(550), 550)); + assert_ok!(fully_unbond_other(Origin::signed(40), 40)); assert_eq!(Balances::free_balance(&default_bonded_account()), 600); let mut current_era = 1; CurrentEra::set(current_era); // In a new era, unbond the depositor unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); let mut sub_pools = SubPoolsStorage::::get(1).unwrap(); let unbond_pool = sub_pools.with_era.get_mut(&(current_era + 3)).unwrap(); @@ -1815,10 +1826,10 @@ mod withdraw_unbonded_other { Balances::make_free_balance_be(&default_bonded_account(), 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)); + assert_ok!(fully_unbond_other(Origin::signed(40), 40)); + assert_ok!(fully_unbond_other(Origin::signed(550), 550)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); SubPoolsStorage::::insert( 1, @@ -1876,7 +1887,7 @@ mod withdraw_unbonded_other { assert_eq!(Balances::free_balance(&10), 5); assert_eq!(Balances::free_balance(&default_bonded_account()), 10); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); // Simulate a slash that is not accounted for in the sub pools. Balances::make_free_balance_be(&default_bonded_account(), 5); @@ -1916,10 +1927,13 @@ mod withdraw_unbonded_other { pool_id: 1, points: 10, reward_pool_total_earnings: 0, - unbonding_era: None, + unbonding_eras: None, }; Delegators::::insert(11, delegator.clone()); + // TODO: this correctly triggers the defensive `SubPoolsNotFound` because a + // corresponding call to `unbond_other` has not been called to create the sub-pools. how + // on earth is it working on master? // The delegator has not called `unbond` assert_noop!( Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0), @@ -1927,7 +1941,7 @@ mod withdraw_unbonded_other { ); // Simulate calling `unbond` - delegator.unbonding_era = Some(0 + 3); + delegator.unbonding_eras = Some(bounded_btree_map!(3 + 0 => 10)); Delegators::::insert(11, delegator.clone()); // We are still in the bonding duration @@ -1949,8 +1963,8 @@ mod withdraw_unbonded_other { .add_delegators(vec![(100, 100), (200, 200)]) .build_and_execute(|| { // Given - assert_ok!(Pools::unbond_other(Origin::signed(100), 100)); - assert_ok!(Pools::unbond_other(Origin::signed(200), 200)); + assert_ok!(fully_unbond_other(Origin::signed(100), 100)); + assert_ok!(fully_unbond_other(Origin::signed(200), 200)); assert_eq!( BondedPool::::get(1).unwrap(), BondedPool { @@ -1998,7 +2012,7 @@ mod withdraw_unbonded_other { fn withdraw_unbonded_other_destroying_permissionless() { ExtBuilder::default().add_delegators(vec![(100, 100)]).build_and_execute(|| { // Given - assert_ok!(Pools::unbond_other(Origin::signed(100), 100)); + assert_ok!(fully_unbond_other(Origin::signed(100), 100)); assert_eq!( BondedPool::::get(1).unwrap(), BondedPool { @@ -2016,7 +2030,7 @@ mod withdraw_unbonded_other { // Cannot permissionlessly withdraw assert_noop!( - Pools::unbond_other(Origin::signed(420), 100), + fully_unbond_other(Origin::signed(420), 100), Error::::NotKickerOrDestroying ); @@ -2038,14 +2052,14 @@ mod withdraw_unbonded_other { .add_delegators(vec![(100, 100), (200, 200)]) .build_and_execute(|| { // Given - assert_ok!(Pools::unbond_other(Origin::signed(100), 100)); + assert_ok!(fully_unbond_other(Origin::signed(100), 100)); let mut current_era = 1; CurrentEra::set(current_era); - assert_ok!(Pools::unbond_other(Origin::signed(200), 200)); + assert_ok!(fully_unbond_other(Origin::signed(200), 200)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2057,6 +2071,7 @@ mod withdraw_unbonded_other { } } ); + // Skip ahead eras to where its valid for the delegators to withdraw current_era += StakingMock::bonding_duration(); CurrentEra::set(current_era); @@ -2080,9 +2095,9 @@ mod withdraw_unbonded_other { } ); - // Cannot withdraw if their is another delegator in the depositors `with_era` pool + // Cannot withdraw the depositor if their is a delegator in another `with_era` pool. assert_noop!( - Pools::unbond_other(Origin::signed(420), 10), + Pools::withdraw_unbonded_other(Origin::signed(420), 10, 0), Error::::NotOnlyDelegator ); @@ -2113,9 +2128,9 @@ mod withdraw_unbonded_other { fn withdraw_unbonded_other_depositor_no_era_pool() { ExtBuilder::default().add_delegators(vec![(100, 100)]).build_and_execute(|| { // Given - assert_ok!(Pools::unbond_other(Origin::signed(100), 100)); + assert_ok!(fully_unbond_other(Origin::signed(100), 100)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(Pools::unbond_other(Origin::signed(10), 10)); + assert_ok!(fully_unbond_other(Origin::signed(10), 10)); // Skip ahead to an era where the `with_era` pools can get merged into the `no_era` // pool. let current_era = TotalUnbondingPools::::get(); @@ -2192,7 +2207,7 @@ mod create { pool_id: 2, points: StakingMock::minimum_bond(), reward_pool_total_earnings: Zero::zero(), - unbonding_era: None + unbonding_eras: None } ); assert_eq!( diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 714449eec7847..ccf83afab6e80 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -86,6 +86,8 @@ pub use self::{ StorageHasher, Twox128, Twox256, Twox64Concat, }, storage::{ + bounded_btree_map::BoundedBTreeMap, + bounded_btree_set::BoundedBTreeSet, bounded_vec::{BoundedSlice, BoundedVec}, migration, weak_bounded_vec::WeakBoundedVec, @@ -137,6 +139,24 @@ macro_rules! bounded_vec { } } +/// Build a bounded btree-map from the given literals. +/// +/// The type of the outcome must be known. +/// +/// Will not handle any errors and just panic if the given literals cannot fit in the corresponding +/// bounded vec type. Thus, this is only suitable for testing and non-consensus code. +#[macro_export] +#[cfg(feature = "std")] +macro_rules! bounded_btree_map { + ($ ( $key:expr => $value:expr ),* $(,)?) => { + { + use $crate::traits::TryCollect as _; + $crate::sp_std::vec![$(($key, $value)),*].into_iter().try_collect().unwrap() + } + }; + +} + /// Generate a new type alias for [`storage::types::StorageValue`], /// [`storage::types::StorageMap`], [`storage::types::StorageDoubleMap`] /// and [`storage::types::StorageNMap`]. diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index 7575aa8c19dc6..cfbc7e90f8953 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -380,7 +380,7 @@ impl Get for GetDefault { macro_rules! impl_const_get { ($name:ident, $t:ty) => { - #[derive($crate::RuntimeDebug)] + #[derive($crate::RuntimeDebug, Clone)] pub struct $name; impl Get<$t> for $name { fn get() -> $t { From c045e06a385664f9241adc69390bf06aa4849647 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Sun, 10 Apr 2022 16:54:08 +0200 Subject: [PATCH 02/33] remove option --- frame/nomination-pools/src/lib.rs | 63 +++++++++++------------- frame/nomination-pools/src/tests.rs | 74 ++++++++++------------------- frame/support/src/lib.rs | 5 +- 3 files changed, 57 insertions(+), 85 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 40a03e6796433..9431072ad9c44 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -379,7 +379,14 @@ enum AccountType { /// A delegator in a pool. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound)] -#[cfg_attr(feature = "std", derive(Clone, PartialEq))] +#[cfg_attr( + feature = "std", + derive( + frame_support::CloneNoBound, + frame_support::PartialEqNoBound, + frame_support::DefaultNoBound + ) +)] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct Delegator { @@ -393,31 +400,23 @@ pub struct Delegator { /// This value lines up with the [`RewardPool::total_earnings`] after a delegator claims a /// payout. pub reward_pool_total_earnings: BalanceOf, - /// The eras as which this delegator is unbonding. - /// - /// `None` means to funds are unbonding. `Some(_)` means a set of eras at which the user's - /// funds will be unlocked (not the era at which the unbonding was initiated!), mapped to the - /// amount of points being unbonded. - pub unbonding_eras: Option, T::MaxUnbonding>>, + /// The eras as which this delegator is unbonding, mapped from era index to the number of + /// points scheduled to unbond in the given era. + pub unbonding_eras: BoundedBTreeMap, T::MaxUnbonding>, } impl Delegator { - /// Total points of the delegator, both active and unbonding. - pub(crate) fn total_points(&self) -> BalanceOf { - self.unbonding_points().saturating_add(self.active_points()) - } - + /// Active points of the delegator. pub(crate) fn active_points(&self) -> BalanceOf { self.points } + /// Inactive points of the delegator, waiting to be withdrawn. pub(crate) fn unbonding_points(&self) -> BalanceOf { self.unbonding_eras .as_ref() - .map(|inner| { - inner.iter().fold(BalanceOf::::zero(), |acc, (_, v)| acc.saturating_add(*v)) - }) - .unwrap_or_default() + .iter() + .fold(BalanceOf::::zero(), |acc, (_, v)| acc.saturating_add(*v)) } /// Try and unbond `points` from self, with the given target unbonding era. @@ -428,20 +427,18 @@ impl Delegator { points: BalanceOf, unbonding_era: EraIndex, ) -> Result<(), Error> { - let mut current_unbonding_eras = self.unbonding_eras.clone().unwrap_or_default(); if let Some(new_points) = self.points.checked_sub(&points) { ensure!( - !current_unbonding_eras.contains_key(&unbonding_era), + !self.unbonding_eras.contains_key(&unbonding_era), Error::::AlreadyUnbonding ); - current_unbonding_eras.try_insert(unbonding_era, points).map_or( + self.unbonding_eras.try_insert(unbonding_era, points).map_or( Err(Error::::MaxUnbonding), |maybe_old| { if maybe_old.is_some() { defensive!("map is checked to not contain this kye."); } self.points = new_points; - self.unbonding_eras = Some(current_unbonding_eras); Ok(()) }, ) @@ -463,18 +460,16 @@ impl Delegator { // NOTE: if only drain-filter was stable.. let mut removed_points = BoundedBTreeMap::, T::MaxUnbonding>::default(); - if let Some(unbonding_eras) = self.unbonding_eras.as_mut() { - unbonding_eras.retain(|e, p| { - if *e > current_era { - true - } else { - removed_points - .try_insert(*e, p.clone()) - .expect("source map is bounded, this is a subset, will be bounded; qed"); - false - } - }) - } + self.unbonding_eras.retain(|e, p| { + if *e > current_era { + true + } else { + removed_points + .try_insert(*e, p.clone()) + .expect("source map is bounded, this is a subset, will be bounded; qed"); + false + } + }); removed_points } } @@ -1269,7 +1264,7 @@ pub mod pallet { // next 2 eras because their vote weight will not be counted until the // snapshot in active era + 1. reward_pool_total_earnings: reward_pool.total_earnings, - unbonding_eras: None, + unbonding_eras: Default::default(), }, ); bonded_pool.put(); @@ -1668,7 +1663,7 @@ pub mod pallet { pool_id, points, reward_pool_total_earnings: Zero::zero(), - unbonding_eras: None, + unbonding_eras: Default::default(), }, ); RewardPools::::insert( diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 38e7a05030317..4221ae0b4c0f6 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -18,7 +18,7 @@ use super::*; use crate::mock::*; use frame_support::{ - assert_noop, assert_ok, assert_storage_noop, + assert_noop, assert_ok, assert_storage_noop, bounded_btree_map, storage::{with_transaction, TransactionOutcome}, }; @@ -31,6 +31,13 @@ macro_rules! sub_pools_with_era { }}; } +macro_rules! typed_bounded_btree_map { + ($( $any:tt )*) => {{ + let x: BoundedBTreeMap = bounded_btree_map!($( $any )*); + x + }}; +} + pub const DEFAULT_ROLES: PoolRoles = PoolRoles { depositor: 10, root: 900, nominator: 901, state_toggler: 902 }; @@ -62,12 +69,7 @@ fn test_setup_works() { ); assert_eq!( Delegators::::get(10).unwrap(), - Delegator:: { - pool_id: last_pool, - points: 10, - reward_pool_total_earnings: 0, - unbonding_eras: None - } + Delegator:: { pool_id: last_pool, points: 10, ..Default::default() } ) }) } @@ -409,12 +411,7 @@ mod join { // then assert_eq!( Delegators::::get(&11).unwrap(), - Delegator:: { - pool_id: 1, - points: 2, - reward_pool_total_earnings: 0, - unbonding_eras: None - } + Delegator:: { pool_id: 1, points: 2, ..Default::default() } ); assert_eq!(BondedPool::::get(1).unwrap(), bonded(12, 2)); @@ -432,12 +429,7 @@ mod join { // Then assert_eq!( Delegators::::get(&12).unwrap(), - Delegator:: { - pool_id: 1, - points: 24, - reward_pool_total_earnings: 0, - unbonding_eras: None - } + Delegator:: { pool_id: 1, points: 24, ..Default::default() } ); assert_eq!(BondedPool::::get(1).unwrap(), bonded(12 + 24, 3)); }); @@ -571,7 +563,12 @@ mod claim_payout { use super::*; fn del(points: Balance, reward_pool_total_earnings: Balance) -> Delegator { - Delegator { pool_id: 1, points, reward_pool_total_earnings, unbonding_eras: None } + Delegator { + pool_id: 1, + points, + reward_pool_total_earnings, + unbonding_eras: Default::default(), + } } fn rew(balance: Balance, points: u32, total_earnings: Balance) -> RewardPool { @@ -1371,7 +1368,6 @@ mod claim_payout { mod unbond { use super::*; - use frame_support::bounded_btree_map; #[test] fn unbond_other_of_1_works() { @@ -1436,7 +1432,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 94); assert_eq!( Delegators::::get(40).unwrap().unbonding_eras, - Some(bounded_btree_map!(0 + 3 => 40)) + typed_bounded_btree_map!(0 + 3 => 40) ); assert_eq!(Balances::free_balance(&40), 40 + 40); // We claim rewards when unbonding @@ -1464,7 +1460,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 2); assert_eq!( Delegators::::get(550).unwrap().unbonding_eras, - Some(bounded_btree_map!(0 + 3 => 550)) + typed_bounded_btree_map!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); @@ -1491,7 +1487,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); assert_eq!( Delegators::::get(550).unwrap().unbonding_eras, - Some(bounded_btree_map!(0 + 3 => 550)) + typed_bounded_btree_map!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); }); @@ -1659,12 +1655,7 @@ mod unbond { ); // Add the delegator - let delegator = Delegator { - pool_id: 2, - points: 10, - reward_pool_total_earnings: 0, - unbonding_eras: None, - }; + let delegator = Delegator { pool_id: 2, points: 10, ..Default::default() }; Delegators::::insert(11, delegator); let _ = fully_unbond_other(Origin::signed(11), 11); @@ -1675,12 +1666,7 @@ mod unbond { #[should_panic = "Defensive failure has been triggered!"] fn unbond_panics_when_reward_pool_not_found() { ExtBuilder::default().build_and_execute(|| { - let delegator = Delegator { - pool_id: 2, - points: 10, - reward_pool_total_earnings: 0, - unbonding_eras: None, - }; + let delegator = Delegator { pool_id: 2, points: 10, ..Default::default() }; Delegators::::insert(11, delegator); BondedPool:: { id: 1, @@ -1923,12 +1909,7 @@ mod withdraw_unbonded_other { Error::::DelegatorNotFound ); - let mut delegator = Delegator { - pool_id: 1, - points: 10, - reward_pool_total_earnings: 0, - unbonding_eras: None, - }; + let mut delegator = Delegator { pool_id: 1, points: 10, ..Default::default() }; Delegators::::insert(11, delegator.clone()); // TODO: this correctly triggers the defensive `SubPoolsNotFound` because a @@ -1941,7 +1922,7 @@ mod withdraw_unbonded_other { ); // Simulate calling `unbond` - delegator.unbonding_eras = Some(bounded_btree_map!(3 + 0 => 10)); + delegator.unbonding_eras = typed_bounded_btree_map!(3 + 0 => 10); Delegators::::insert(11, delegator.clone()); // We are still in the bonding duration @@ -2203,12 +2184,7 @@ mod create { assert_eq!(Balances::free_balance(&11), 0); assert_eq!( Delegators::::get(11).unwrap(), - Delegator { - pool_id: 2, - points: StakingMock::minimum_bond(), - reward_pool_total_earnings: Zero::zero(), - unbonding_eras: None - } + Delegator { pool_id: 2, points: StakingMock::minimum_bond(), ..Default::default() } ); assert_eq!( BondedPool::::get(2).unwrap(), diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index ccf83afab6e80..056abbad19b23 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -150,8 +150,9 @@ macro_rules! bounded_vec { macro_rules! bounded_btree_map { ($ ( $key:expr => $value:expr ),* $(,)?) => { { - use $crate::traits::TryCollect as _; - $crate::sp_std::vec![$(($key, $value)),*].into_iter().try_collect().unwrap() + $crate::traits::TryCollect::<$crate::BoundedBTreeMap<_, _, _>>::try_collect( + $crate::sp_std::vec![$(($key, $value)),*].into_iter() + ).unwrap() } }; From ad7f37ca17688286c976fb6d9a6e9c74ed269f8a Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 Apr 2022 08:12:52 +0200 Subject: [PATCH 03/33] Add some more tests and fix issues --- frame/nomination-pools/src/lib.rs | 102 ++++++----- frame/nomination-pools/src/mock.rs | 2 +- frame/nomination-pools/src/tests.rs | 263 ++++++++++++++++++++++++++-- 3 files changed, 301 insertions(+), 66 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 05d90e07f5262..8de04d850a899 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -428,20 +428,21 @@ impl Delegator { unbonding_era: EraIndex, ) -> Result<(), Error> { if let Some(new_points) = self.points.checked_sub(&points) { - ensure!( - !self.unbonding_eras.contains_key(&unbonding_era), - Error::::AlreadyUnbonding - ); - self.unbonding_eras.try_insert(unbonding_era, points).map_or( - Err(Error::::MaxUnbonding), - |maybe_old| { - if maybe_old.is_some() { - defensive!("map is checked to not contain this kye."); - } - self.points = new_points; - Ok(()) - }, - ) + match self.unbonding_eras.get_mut(&unbonding_era) { + Some(already_unbonding_points) => + *already_unbonding_points = already_unbonding_points.saturating_add(points), + None => self + .unbonding_eras + .try_insert(unbonding_era, points) + .map(|old| { + if old.is_some() { + defensive!("value checked to not exist in the map; qed"); + } + }) + .map_err(|_| Error::::MaxUnbonding)?, + } + self.points = new_points; + Ok(()) } else { Err(Error::::NotEnoughPointsToUnbond) } @@ -730,6 +731,7 @@ impl BondedPool { // destroying it cannot switch states, so by being in destroying we are guaranteed no // other delegators can possibly join. (_, true) => { + // TODO: a secondary invariant here is that no delegator can have 0 points. ensure!( target_delegator.active_points() == self.points, Error::::NotOnlyDelegator @@ -755,30 +757,10 @@ impl BondedPool { sub_pools: &SubPools, ) -> Result { if *target_account == self.roles.depositor { - // 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. - ensure!(sub_pools.with_era.len().is_zero(), Error::::NotOnlyDelegator); - ensure!( - sub_pools.no_era.points == target_delegator.unbonding_points(), - Error::::NotOnlyDelegator - ); - } else { - // No points in the `no_era` pool, so they must be in a `with_era` pool - // If there are no other delegators, this can be the only `with_era` pool since the - // depositor was the last to withdraw. This assumes with_era sub pools are destroyed - // whenever their points go to zero. - ensure!(sub_pools.with_era.len() == 1, Error::::NotOnlyDelegator); - sub_pools - .with_era - .values() - .next() - .filter(|only_unbonding_pool| { - only_unbonding_pool.points == target_delegator.unbonding_points() - }) - .ok_or(Error::::NotOnlyDelegator)?; - } + ensure!( + sub_pools.sum_unbonding_points() == target_delegator.unbonding_points(), + Error::::NotOnlyDelegator + ); Ok(true) } else { // This isn't a depositor @@ -906,7 +888,7 @@ impl RewardPool { /// An unbonding pool. This is always mapped with an era. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, DefaultNoBound, RuntimeDebugNoBound)] -#[cfg_attr(feature = "std", derive(Clone, PartialEq))] +#[cfg_attr(feature = "std", derive(Clone, PartialEq, Eq))] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct UnbondPool { @@ -985,6 +967,25 @@ impl SubPools { self } + + /// The sum of all unbonding points, regardless of whether they are actually unlocked or not. + fn sum_unbonding_points(&self) -> BalanceOf { + self.no_era.points.saturating_add( + self.with_era + .values() + .fold(BalanceOf::::zero(), |acc, pool| acc.saturating_add(pool.points)), + ) + } + + /// The sum of all unbonding balance, regardless of whether they are actually unlocked or not. + #[cfg(any(test, debug_assertions))] + fn sum_unbonding_balance(&self) -> BalanceOf { + self.no_era.balance.saturating_add( + self.with_era + .values() + .fold(BalanceOf::::zero(), |acc, pool| acc.saturating_add(pool.balance)), + ) + } } /// The maximum amount of eras an unbonding pool can exist prior to being merged with the @@ -1188,10 +1189,8 @@ pub mod pallet { FullyUnbonding, /// The delegator cannot unbond further chunks due to reaching the limit. MaxUnbonding, - /// The delegator is not unbonding and thus cannot withdraw funds. - NotUnbonding, - /// Unbonded funds cannot be withdrawn yet because the bonding duration has not passed. - NotUnbondedYet, + /// None of the funds cannot be withdrawn yet because the bonding duration has not passed. + CannotWithdrawAny, /// The amount does not meet the minimum bond to either join or create a pool. MinimumBondNotMet, /// The transaction could not be executed due to overflow risk for the pool. @@ -1466,7 +1465,7 @@ pub mod pallet { } /// Withdraw unbonded funds who's unbonding period has passed for the `target` delegator. If - /// no bond can be unbonded, this is a successful no-op. + /// no bond can be unbonded, an error is returned. /// /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any /// account). @@ -1512,6 +1511,7 @@ pub mod pallet { // NOTE: must do this after we have done the `ok_to_withdraw_unbonded_other_with` check. let withdrawn_points = delegator.withdraw_unlocked(current_era); + ensure!(!withdrawn_points.is_empty(), Error::::CannotWithdrawAny); // Before calculate the `balance_to_unbond`, with call withdraw unbonded to ensure the // `non_locked_balance` is correct. @@ -2072,24 +2072,20 @@ impl Pallet { return Ok(()) } - for (pool_id, _) in BondedPools::::iter() { + for (pool_id, _pool) 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 sum_unbonding_balance = subs.sum_unbonding_balance(); 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 {:?}", + "fault pool: {:?} / {:?}, total_balance {:?} >= bonded_balance {:?} + sum_unbonding_balance {:?}", + pool_id, + _pool, total_balance, bonded_balance, sum_unbonding_balance diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index d4a2b50b4997d..953bb39fbc91f 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -20,9 +20,9 @@ pub fn default_reward_account() -> AccountId { parameter_types! { pub static CurrentEra: EraIndex = 0; + pub static BondingDuration: EraIndex = 3; static BondedBalanceMap: HashMap = Default::default(); static UnbondingBalanceMap: HashMap = Default::default(); - pub static BondingDuration: EraIndex = 3; #[derive(Clone, PartialEq)] pub const MaxUnbonding: u32 = 8; pub static Nominations: Vec = vec![]; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index a10e38b13887e..bcc83d929a9d0 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1681,6 +1681,114 @@ mod unbond { let _ = fully_unbond_other(Origin::signed(11), 11); }); } + + #[test] + fn partial_unbond_era_tracking() { + ExtBuilder::default().build_and_execute(|| { + // given + assert_eq!(Delegators::::get(10).unwrap().active_points(), 10); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 0); + assert_eq!(Delegators::::get(10).unwrap().pool_id, 1); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!() + ); + assert_eq!(BondedPool::::get(1).unwrap().points, 10); + assert!(SubPoolsStorage::::get(1).is_none()); + assert_eq!(CurrentEra::get(), 0); + assert_eq!(BondingDuration::get(), 3); + + // so event the depositor can leave, just keeps the test simpler. + unsafe_set_state(1, PoolState::Destroying).unwrap(); + + // when: casual unbond + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); + + // then + assert_eq!(Delegators::::get(10).unwrap().active_points(), 9); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 1); + assert_eq!(BondedPool::::get(1).unwrap().points, 9); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 1, balance: 1 } + } + } + ); + + // when: casual further unbond, same era. + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 5)); + + // then + assert_eq!(Delegators::::get(10).unwrap().active_points(), 4); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 6); + assert_eq!(BondedPool::::get(1).unwrap().points, 4); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 6) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 6, balance: 6 } + } + } + ); + + // when: casual further unbond, next era. + CurrentEra::set(1); + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); + + // then + assert_eq!(Delegators::::get(10).unwrap().active_points(), 3); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 7); + assert_eq!(BondedPool::::get(1).unwrap().points, 3); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 6, 4 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 6, balance: 6 }, + 4 => UnbondPool { points: 1, balance: 1 } + } + } + ); + + // when: unbonding more than our active: capped + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 5)); + + // then + assert_eq!(Delegators::::get(10).unwrap().active_points(), 0); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 10); + assert_eq!(BondedPool::::get(1).unwrap().points, 0); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 6, 4 => 4) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 6, balance: 6 }, + 4 => UnbondPool { points: 4, balance: 4 } + } + } + ); + }); + } } mod pool_withdraw_unbonded { @@ -1708,9 +1816,8 @@ mod pool_withdraw_unbonded { } mod withdraw_unbonded_other { - use frame_support::bounded_btree_map; - use super::*; + use frame_support::bounded_btree_map; #[test] fn withdraw_unbonded_other_works_against_slashed_no_era_sub_pool() { @@ -1911,15 +2018,6 @@ mod withdraw_unbonded_other { let mut delegator = Delegator { pool_id: 1, points: 10, ..Default::default() }; Delegators::::insert(11, delegator.clone()); - // TODO: this correctly triggers the defensive `SubPoolsNotFound` because a - // corresponding call to `unbond_other` has not been called to create the sub-pools. how - // on earth is it working on master? - // The delegator has not called `unbond` - assert_noop!( - Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0), - Error::::NotUnbonding - ); - // Simulate calling `unbond` delegator.unbonding_eras = typed_bounded_btree_map!(3 + 0 => 10); Delegators::::insert(11, delegator.clone()); @@ -1927,7 +2025,7 @@ mod withdraw_unbonded_other { // We are still in the bonding duration assert_noop!( Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0), - Error::::NotUnbondedYet + Error::::CannotWithdrawAny ); // If we error the delegator does not get removed @@ -2154,6 +2252,147 @@ mod withdraw_unbonded_other { assert!(!BondedPools::::contains_key(1)); }); } + + #[test] + fn partial_withdraw_unbonded_depositor() { + ExtBuilder::default().build_and_execute(|| { + // so event the depositor can leave, just keeps the test simpler. + unsafe_set_state(1, PoolState::Destroying).unwrap(); + + // given + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 6)); + CurrentEra::set(1); + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 6, 4 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 6, balance: 6 }, + 4 => UnbondPool { points: 1, balance: 1 } + } + } + ); + assert_eq!(Delegators::::get(10).unwrap().active_points(), 3); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 7); + + // when + CurrentEra::set(2); + assert_noop!( + Pools::withdraw_unbonded_other(Origin::signed(10), 10, 0), + Error::::CannotWithdrawAny + ); + + // when + CurrentEra::set(3); + assert_ok!(Pools::withdraw_unbonded_other(Origin::signed(10), 10, 0)); + + // then + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(4 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 4 => UnbondPool { points: 1, balance: 1 } + } + } + ); + + // when + CurrentEra::set(4); + assert_ok!(Pools::withdraw_unbonded_other(Origin::signed(10), 10, 0)); + + // then + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!() + ); + assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); + + // when repeating: + assert_noop!( + Pools::withdraw_unbonded_other(Origin::signed(10), 10, 0), + Error::::CannotWithdrawAny + ); + }); + } + + #[test] + fn partial_withdraw_unbonded_non_depositor() { + ExtBuilder::default().add_delegators(vec![(11, 10)]).build_and_execute(|| { + // given + assert_ok!(Pools::unbond_other(Origin::signed(11), 11, 6)); + CurrentEra::set(1); + assert_ok!(Pools::unbond_other(Origin::signed(11), 11, 1)); + assert_eq!( + Delegators::::get(11).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 6, 4 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 3 => UnbondPool { points: 6, balance: 6 }, + 4 => UnbondPool { points: 1, balance: 1 } + } + } + ); + assert_eq!(Delegators::::get(11).unwrap().active_points(), 3); + assert_eq!(Delegators::::get(11).unwrap().unbonding_points(), 7); + + // when + CurrentEra::set(2); + assert_noop!( + Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0), + Error::::CannotWithdrawAny + ); + + // when + CurrentEra::set(3); + assert_ok!(Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0)); + + // then + assert_eq!( + Delegators::::get(11).unwrap().unbonding_eras, + typed_bounded_btree_map!(4 => 1) + ); + assert_eq!( + SubPoolsStorage::::get(1).unwrap(), + SubPools { + no_era: Default::default(), + with_era: unbonding_pools_with_era! { + 4 => UnbondPool { points: 1, balance: 1 } + } + } + ); + + // when + CurrentEra::set(4); + assert_ok!(Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0)); + + // then + assert_eq!( + Delegators::::get(11).unwrap().unbonding_eras, + typed_bounded_btree_map!() + ); + assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); + + // when repeating: + assert_noop!( + Pools::withdraw_unbonded_other(Origin::signed(11), 11, 0), + Error::::CannotWithdrawAny + ); + }); + } } mod create { From 764ae6fe26ada6933c3bb7a69fc412ca793dc609 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 Apr 2022 09:31:43 +0200 Subject: [PATCH 04/33] Fix all tests --- frame/nomination-pools/src/lib.rs | 12 +++------ frame/nomination-pools/src/mock.rs | 5 ++++ frame/nomination-pools/src/tests.rs | 41 ++++++++++++++++++++++++++--- 3 files changed, 46 insertions(+), 12 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 8de04d850a899..31d607674c7e9 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1273,14 +1273,8 @@ pub mod pallet { unbonding_eras: Default::default(), }, ); - bonded_pool.put(); - Self::deposit_event(Event::::Bonded { - delegator: who, - pool_id, - bonded: amount, - joined: true, - }); + bonded_pool.put(); Ok(()) } @@ -1678,7 +1672,7 @@ pub mod pallet { }, ); ReversePoolIdLookup::::insert(bonded_pool.bonded_account(), pool_id); - Self::deposit_event(Event::::Created { depositor: who, pool_id }); + Self::deposit_event(Event::::Created { depositor: who.clone(), pool_id }); bonded_pool.put(); Ok(()) @@ -2083,7 +2077,7 @@ impl Pallet { assert!( total_balance >= bonded_balance + sum_unbonding_balance, - "fault pool: {:?} / {:?}, total_balance {:?} >= bonded_balance {:?} + sum_unbonding_balance {:?}", + "faulty pool: {:?} / {:?}, total_balance {:?} >= bonded_balance {:?} + sum_unbonding_balance {:?}", pool_id, _pool, total_balance, diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 953bb39fbc91f..a5e0740045309 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -204,6 +204,11 @@ impl ExtBuilder { self } + pub(crate) fn ed(self, ed: Balance) -> Self { + ExistentialDeposit::set(ed); + self + } + pub(crate) fn with_check(self, level: u8) -> Self { CheckLevel::set(level); self diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index bcc83d929a9d0..a6042b071b429 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -16,7 +16,7 @@ // limitations under the License. use super::*; -use crate::mock::*; +use crate::{mock::*, Event as PoolsEvent}; use frame_support::{ assert_noop, assert_ok, assert_storage_noop, bounded_btree_map, storage::{with_transaction, TransactionOutcome}, @@ -2255,7 +2255,7 @@ mod withdraw_unbonded_other { #[test] fn partial_withdraw_unbonded_depositor() { - ExtBuilder::default().build_and_execute(|| { + ExtBuilder::default().ed(1).build_and_execute(|| { // so event the depositor can leave, just keeps the test simpler. unsafe_set_state(1, PoolState::Destroying).unwrap(); @@ -2279,6 +2279,16 @@ mod withdraw_unbonded_other { ); assert_eq!(Delegators::::get(10).unwrap().active_points(), 3); assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 7); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Created { depositor: 10, pool_id: 1 }, + PoolsEvent::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, + PoolsEvent::Unbonded { delegator: 10, pool_id: 1, amount: 6 }, + PoolsEvent::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, + PoolsEvent::Unbonded { delegator: 10, pool_id: 1, amount: 1 } + ] + ); // when CurrentEra::set(2); @@ -2305,6 +2315,10 @@ mod withdraw_unbonded_other { } } ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Withdrawn { delegator: 10, pool_id: 1, amount: 6 }] + ); // when CurrentEra::set(4); @@ -2316,6 +2330,10 @@ mod withdraw_unbonded_other { typed_bounded_btree_map!() ); assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Withdrawn { delegator: 10, pool_id: 1, amount: 1 },] + ); // when repeating: assert_noop!( @@ -2348,6 +2366,16 @@ mod withdraw_unbonded_other { ); assert_eq!(Delegators::::get(11).unwrap().active_points(), 3); assert_eq!(Delegators::::get(11).unwrap().unbonding_points(), 7); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Created { depositor: 10, pool_id: 1 }, + PoolsEvent::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, + PoolsEvent::Unbonded { delegator: 11, pool_id: 1, amount: 6 }, + PoolsEvent::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, + PoolsEvent::Unbonded { delegator: 11, pool_id: 1, amount: 1 } + ] + ); // when CurrentEra::set(2); @@ -2374,6 +2402,10 @@ mod withdraw_unbonded_other { } } ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Withdrawn { delegator: 11, pool_id: 1, amount: 6 }] + ); // when CurrentEra::set(4); @@ -2385,6 +2417,10 @@ mod withdraw_unbonded_other { typed_bounded_btree_map!() ); assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Withdrawn { delegator: 11, pool_id: 1, amount: 1 }] + ); // when repeating: assert_noop!( @@ -2838,7 +2874,6 @@ mod bond_extra { pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, - Event::Bonded { delegator: 20, pool_id: 1, bonded: 20, joined: true }, Event::PaidOut { delegator: 10, pool_id: 1, payout: 1 }, Event::Bonded { delegator: 10, pool_id: 1, bonded: 1, joined: false }, Event::PaidOut { delegator: 20, pool_id: 1, payout: 2 }, From 28844a2285cdd5ea323d453ee575bfdfb1c46bc9 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 13 Apr 2022 09:46:22 +0200 Subject: [PATCH 05/33] simplify some tests --- frame/nomination-pools/src/lib.rs | 11 ++++- frame/nomination-pools/src/mock.rs | 2 +- frame/nomination-pools/src/tests.rs | 65 ++++++++++++++++++++++------- 3 files changed, 60 insertions(+), 18 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 31d607674c7e9..adce81a3c5118 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -439,7 +439,7 @@ impl Delegator { defensive!("value checked to not exist in the map; qed"); } }) - .map_err(|_| Error::::MaxUnbonding)?, + .map_err(|_| Error::::MaxUnbondingLimit)?, } self.points = new_points; Ok(()) @@ -1188,7 +1188,7 @@ pub mod pallet { /// anymore to, for example, collect rewards). FullyUnbonding, /// The delegator cannot unbond further chunks due to reaching the limit. - MaxUnbonding, + MaxUnbondingLimit, /// None of the funds cannot be withdrawn yet because the bonding duration has not passed. CannotWithdrawAny, /// The amount does not meet the minimum bond to either join or create a pool. @@ -1274,7 +1274,14 @@ pub mod pallet { }, ); + Self::deposit_event(Event::::Bonded { + delegator: who, + pool_id, + bonded: amount, + joined: true, + }); bonded_pool.put(); + Ok(()) } diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index a5e0740045309..1a4cabadc5426 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -24,7 +24,7 @@ parameter_types! { static BondedBalanceMap: HashMap = Default::default(); static UnbondingBalanceMap: HashMap = Default::default(); #[derive(Clone, PartialEq)] - pub const MaxUnbonding: u32 = 8; + pub static MaxUnbonding: u32 = 8; pub static Nominations: Vec = vec![]; } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index a6042b071b429..6df3dc2f07117 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -16,7 +16,7 @@ // limitations under the License. use super::*; -use crate::{mock::*, Event as PoolsEvent}; +use crate::{mock::*, Event}; use frame_support::{ assert_noop, assert_ok, assert_storage_noop, bounded_btree_map, storage::{with_transaction, TransactionOutcome}, @@ -1789,6 +1789,39 @@ mod unbond { ); }); } + + #[test] + fn partial_unbond_mac_chunks() { + ExtBuilder::default().ed(1).build_and_execute(|| { + // so event the depositor can leave, just keeps the test simpler. + unsafe_set_state(1, PoolState::Destroying).unwrap(); + MaxUnbonding::set(2); + + // given + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 2)); + CurrentEra::set(1); + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 3)); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 2, 4 => 3) + ); + + // when + CurrentEra::set(2); + assert_noop!( + Pools::unbond_other(Origin::signed(10), 10, 4), + Error::::MaxUnbondingLimit + ); + + // when + MaxUnbonding::set(3); + assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); + assert_eq!( + Delegators::::get(10).unwrap().unbonding_eras, + typed_bounded_btree_map!(3 => 2, 4 => 3, 5 => 1) + ); + }) + } } mod pool_withdraw_unbonded { @@ -2282,11 +2315,11 @@ mod withdraw_unbonded_other { assert_eq!( pool_events_since_last_call(), vec![ - PoolsEvent::Created { depositor: 10, pool_id: 1 }, - PoolsEvent::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, - PoolsEvent::Unbonded { delegator: 10, pool_id: 1, amount: 6 }, - PoolsEvent::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, - PoolsEvent::Unbonded { delegator: 10, pool_id: 1, amount: 1 } + Event::Created { depositor: 10, pool_id: 1 }, + Event::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, + Event::Unbonded { delegator: 10, pool_id: 1, amount: 6 }, + Event::PaidOut { delegator: 10, pool_id: 1, payout: 0 }, + Event::Unbonded { delegator: 10, pool_id: 1, amount: 1 } ] ); @@ -2317,7 +2350,7 @@ mod withdraw_unbonded_other { ); assert_eq!( pool_events_since_last_call(), - vec![PoolsEvent::Withdrawn { delegator: 10, pool_id: 1, amount: 6 }] + vec![Event::Withdrawn { delegator: 10, pool_id: 1, amount: 6 }] ); // when @@ -2332,7 +2365,7 @@ mod withdraw_unbonded_other { assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); assert_eq!( pool_events_since_last_call(), - vec![PoolsEvent::Withdrawn { delegator: 10, pool_id: 1, amount: 1 },] + vec![Event::Withdrawn { delegator: 10, pool_id: 1, amount: 1 },] ); // when repeating: @@ -2369,11 +2402,12 @@ mod withdraw_unbonded_other { assert_eq!( pool_events_since_last_call(), vec![ - PoolsEvent::Created { depositor: 10, pool_id: 1 }, - PoolsEvent::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, - PoolsEvent::Unbonded { delegator: 11, pool_id: 1, amount: 6 }, - PoolsEvent::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, - PoolsEvent::Unbonded { delegator: 11, pool_id: 1, amount: 1 } + Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { delegator: 11, pool_id: 1, bonded: 10, joined: true }, + Event::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, + Event::Unbonded { delegator: 11, pool_id: 1, amount: 6 }, + Event::PaidOut { delegator: 11, pool_id: 1, payout: 0 }, + Event::Unbonded { delegator: 11, pool_id: 1, amount: 1 } ] ); @@ -2404,7 +2438,7 @@ mod withdraw_unbonded_other { ); assert_eq!( pool_events_since_last_call(), - vec![PoolsEvent::Withdrawn { delegator: 11, pool_id: 1, amount: 6 }] + vec![Event::Withdrawn { delegator: 11, pool_id: 1, amount: 6 }] ); // when @@ -2419,7 +2453,7 @@ mod withdraw_unbonded_other { assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); assert_eq!( pool_events_since_last_call(), - vec![PoolsEvent::Withdrawn { delegator: 11, pool_id: 1, amount: 1 }] + vec![Event::Withdrawn { delegator: 11, pool_id: 1, amount: 1 }] ); // when repeating: @@ -2874,6 +2908,7 @@ mod bond_extra { pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { delegator: 20, pool_id: 1, bonded: 20, joined: true }, Event::PaidOut { delegator: 10, pool_id: 1, payout: 1 }, Event::Bonded { delegator: 10, pool_id: 1, bonded: 1, joined: false }, Event::PaidOut { delegator: 20, pool_id: 1, payout: 2 }, From 9a2af205180692a7a0263c4eb49facd205594fc6 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Wed, 13 Apr 2022 16:32:57 +0200 Subject: [PATCH 06/33] Update frame/nomination-pools/src/mock.rs --- frame/nomination-pools/src/mock.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 1a4cabadc5426..203c8cabbfd28 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use super::*; use crate::{self as pools}; use frame_support::{assert_ok, parameter_types, PalletId}; From de80c7be8822669181e9a2a7c15d9c7117d4d28e Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:19:43 +0200 Subject: [PATCH 07/33] remove clone --- frame/support/src/traits/misc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/traits/misc.rs b/frame/support/src/traits/misc.rs index cfbc7e90f8953..7575aa8c19dc6 100644 --- a/frame/support/src/traits/misc.rs +++ b/frame/support/src/traits/misc.rs @@ -380,7 +380,7 @@ impl Get for GetDefault { macro_rules! impl_const_get { ($name:ident, $t:ty) => { - #[derive($crate::RuntimeDebug, Clone)] + #[derive($crate::RuntimeDebug)] pub struct $name; impl Get<$t> for $name { fn get() -> $t { From d0f0121c68d30df69f463c7c51b1a38c09e34080 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:21:10 +0200 Subject: [PATCH 08/33] rename to delegator_unbonding_eras --- frame/nomination-pools/src/tests.rs | 36 ++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 6df3dc2f07117..db269aabb5b6c 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -30,7 +30,7 @@ macro_rules! unbonding_pools_with_era { }}; } -macro_rules! typed_bounded_btree_map { +macro_rules! delegator_unbonding_eras { ($( $any:tt )*) => {{ let x: BoundedBTreeMap = bounded_btree_map!($( $any )*); x @@ -1431,7 +1431,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 94); assert_eq!( Delegators::::get(40).unwrap().unbonding_eras, - typed_bounded_btree_map!(0 + 3 => 40) + delegator_unbonding_eras!(0 + 3 => 40) ); assert_eq!(Balances::free_balance(&40), 40 + 40); // We claim rewards when unbonding @@ -1459,7 +1459,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 2); assert_eq!( Delegators::::get(550).unwrap().unbonding_eras, - typed_bounded_btree_map!(0 + 3 => 550) + delegator_unbonding_eras!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); @@ -1486,7 +1486,7 @@ mod unbond { assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 0); assert_eq!( Delegators::::get(550).unwrap().unbonding_eras, - typed_bounded_btree_map!(0 + 3 => 550) + delegator_unbonding_eras!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); }); @@ -1691,7 +1691,7 @@ mod unbond { assert_eq!(Delegators::::get(10).unwrap().pool_id, 1); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!() + delegator_unbonding_eras!() ); assert_eq!(BondedPool::::get(1).unwrap().points, 10); assert!(SubPoolsStorage::::get(1).is_none()); @@ -1710,7 +1710,7 @@ mod unbond { assert_eq!(BondedPool::::get(1).unwrap().points, 9); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 1) + delegator_unbonding_eras!(3 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -1731,7 +1731,7 @@ mod unbond { assert_eq!(BondedPool::::get(1).unwrap().points, 4); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 6) + delegator_unbonding_eras!(3 => 6) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -1753,7 +1753,7 @@ mod unbond { assert_eq!(BondedPool::::get(1).unwrap().points, 3); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 6, 4 => 1) + delegator_unbonding_eras!(3 => 6, 4 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -1775,7 +1775,7 @@ mod unbond { assert_eq!(BondedPool::::get(1).unwrap().points, 0); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 6, 4 => 4) + delegator_unbonding_eras!(3 => 6, 4 => 4) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -1803,7 +1803,7 @@ mod unbond { assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 3)); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 2, 4 => 3) + delegator_unbonding_eras!(3 => 2, 4 => 3) ); // when @@ -1818,7 +1818,7 @@ mod unbond { assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 2, 4 => 3, 5 => 1) + delegator_unbonding_eras!(3 => 2, 4 => 3, 5 => 1) ); }) } @@ -2052,7 +2052,7 @@ mod withdraw_unbonded_other { Delegators::::insert(11, delegator.clone()); // Simulate calling `unbond` - delegator.unbonding_eras = typed_bounded_btree_map!(3 + 0 => 10); + delegator.unbonding_eras = delegator_unbonding_eras!(3 + 0 => 10); Delegators::::insert(11, delegator.clone()); // We are still in the bonding duration @@ -2298,7 +2298,7 @@ mod withdraw_unbonded_other { assert_ok!(Pools::unbond_other(Origin::signed(10), 10, 1)); assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 6, 4 => 1) + delegator_unbonding_eras!(3 => 6, 4 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2337,7 +2337,7 @@ mod withdraw_unbonded_other { // then assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!(4 => 1) + delegator_unbonding_eras!(4 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2360,7 +2360,7 @@ mod withdraw_unbonded_other { // then assert_eq!( Delegators::::get(10).unwrap().unbonding_eras, - typed_bounded_btree_map!() + delegator_unbonding_eras!() ); assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); assert_eq!( @@ -2385,7 +2385,7 @@ mod withdraw_unbonded_other { assert_ok!(Pools::unbond_other(Origin::signed(11), 11, 1)); assert_eq!( Delegators::::get(11).unwrap().unbonding_eras, - typed_bounded_btree_map!(3 => 6, 4 => 1) + delegator_unbonding_eras!(3 => 6, 4 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2425,7 +2425,7 @@ mod withdraw_unbonded_other { // then assert_eq!( Delegators::::get(11).unwrap().unbonding_eras, - typed_bounded_btree_map!(4 => 1) + delegator_unbonding_eras!(4 => 1) ); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2448,7 +2448,7 @@ mod withdraw_unbonded_other { // then assert_eq!( Delegators::::get(11).unwrap().unbonding_eras, - typed_bounded_btree_map!() + delegator_unbonding_eras!() ); assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); assert_eq!( From 03dafcb638b5437302c6bd0efaaea760667e9371 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:21:37 +0200 Subject: [PATCH 09/33] Update frame/nomination-pools/src/tests.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index db269aabb5b6c..d1fba236ef95d 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1698,7 +1698,7 @@ mod unbond { assert_eq!(CurrentEra::get(), 0); assert_eq!(BondingDuration::get(), 3); - // so event the depositor can leave, just keeps the test simpler. + // so the depositor can leave, just keeps the test simpler. unsafe_set_state(1, PoolState::Destroying).unwrap(); // when: casual unbond From 257f48b01d3468f87f71b4046d0efae2528c518f Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:21:42 +0200 Subject: [PATCH 10/33] Update frame/nomination-pools/src/tests.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index d1fba236ef95d..c8f1519b42144 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1793,7 +1793,7 @@ mod unbond { #[test] fn partial_unbond_mac_chunks() { ExtBuilder::default().ed(1).build_and_execute(|| { - // so event the depositor can leave, just keeps the test simpler. + // so the depositor can leave, just keeps the test simpler. unsafe_set_state(1, PoolState::Destroying).unwrap(); MaxUnbonding::set(2); From 4e04c7938fa38b512a9c960e46b8095b8ce242e7 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:21:50 +0200 Subject: [PATCH 11/33] Update frame/nomination-pools/src/tests.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index c8f1519b42144..b5528f02219b6 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -2289,7 +2289,7 @@ mod withdraw_unbonded_other { #[test] fn partial_withdraw_unbonded_depositor() { ExtBuilder::default().ed(1).build_and_execute(|| { - // so event the depositor can leave, just keeps the test simpler. + // so the depositor can leave, just keeps the test simpler. unsafe_set_state(1, PoolState::Destroying).unwrap(); // given From 26197e383f6349f59da668dbd79b14a39b7890a0 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:22:57 +0200 Subject: [PATCH 12/33] remove pub --- frame/nomination-pools/src/lib.rs | 2 +- frame/nomination-pools/src/mock.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index adce81a3c5118..7dbd58e56512c 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1957,7 +1957,7 @@ impl Pallet { /// If the delegator has some rewards, transfer a payout from the reward pool to the delegator. // Emits events and potentially modifies pool state if any arithmetic saturates, but does // not persist any of the mutable inputs to storage. - pub(crate) fn do_reward_payout( + pub fn do_reward_payout( delegator_account: &T::AccountId, delegator: &mut Delegator, bonded_pool: &mut BondedPool, diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 203c8cabbfd28..eab3ad64a5585 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -2,6 +2,7 @@ use super::*; use crate::{self as pools}; use frame_support::{assert_ok, parameter_types, PalletId}; use frame_system::RawOrigin; +use std::collections::HashMap; pub type AccountId = u128; pub type Balance = u128; From a46943390b17974ffa1e471dbc5e1d55fe520bd6 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:23:32 +0200 Subject: [PATCH 13/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 7dbd58e56512c..bf94f8643ebc8 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1466,7 +1466,7 @@ pub mod pallet { } /// Withdraw unbonded funds who's unbonding period has passed for the `target` delegator. If - /// no bond can be unbonded, an error is returned. + /// no bonded funds can be unbonded, an error is returned. /// /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any /// account). From a1d5e3a0e8552b79da5330e07aa3c0a4b6f5cb96 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:23:47 +0200 Subject: [PATCH 14/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index bf94f8643ebc8..636c745116474 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1465,7 +1465,7 @@ pub mod pallet { Ok(()) } - /// Withdraw unbonded funds who's unbonding period has passed for the `target` delegator. If + /// Withdraw unbonded funds from `delegator_account`. If /// no bonded funds can be unbonded, an error is returned. /// /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any From b1ee72e83c859fa0539caa0cf0b249ca59b8282b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:25:29 +0200 Subject: [PATCH 15/33] undo --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 7dbd58e56512c..a3e4fc846dbfb 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1355,7 +1355,7 @@ pub mod pallet { /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any /// account). /// - /// ## Conditions for a permissionless dispatch. + /// # Conditions for a permissionless dispatch. /// /// * The pool is blocked and the caller is either the root or state-toggler. This is /// refereed to as a kick. From ef96a111528a338b0a29915eb5448d7cfd12349f Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:26:39 +0200 Subject: [PATCH 16/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index ff8c797705ffa..d164fae8ba596 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1389,7 +1389,9 @@ pub mod pallet { bonded_pool.ok_to_unbond_other_with(&caller, &delegator_account, &delegator)?; unbonding_points = unbonding_points.min(delegator.active_points()); - // This is only for UX. + // 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, &mut delegator, From 17296face6f0be2435ac13c2dde1978d78b5ded1 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Thu, 14 Apr 2022 08:26:49 +0200 Subject: [PATCH 17/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index d164fae8ba596..1dc77eaa70fce 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1590,7 +1590,7 @@ pub mod pallet { // we certainly don't need to delete any pools, because no one is being removed. SubPoolsStorage::::insert(&delegator.pool_id, sub_pools); Delegators::::insert(&delegator_account, delegator); - None + Some(T::WeightInfo::withdraw_unbonded_other_update(num_slashing_spans)) }; Ok(post_info_weight.into()) From 5c049160c4b3b13dae60b08c1106a6a24822a846 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:33:56 +0200 Subject: [PATCH 18/33] leftovers --- frame/nomination-pools/src/lib.rs | 3 +++ frame/nomination-pools/src/tests.rs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index ff8c797705ffa..18d9d5ffb8787 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1057,6 +1057,9 @@ pub mod pallet { pub type MinJoinBond = StorageValue<_, BalanceOf, ValueQuery>; /// Minimum bond required to create a pool. + /// + /// This is the amount that the depositor must put as their initial stake in the pool, as an + /// indication of "skin in the game". #[pallet::storage] pub type MinCreateBond = StorageValue<_, BalanceOf, ValueQuery>; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index b5528f02219b6..41d35b656f614 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1791,7 +1791,7 @@ mod unbond { } #[test] - fn partial_unbond_mac_chunks() { + fn partial_unbond_max_chunks() { ExtBuilder::default().ed(1).build_and_execute(|| { // so the depositor can leave, just keeps the test simpler. unsafe_set_state(1, PoolState::Destroying).unwrap(); From 2b52a2582bf8882527e093fd2dab0f14abec2d9e Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 14 Apr 2022 08:35:12 +0200 Subject: [PATCH 19/33] fix invariant --- frame/nomination-pools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 84b0d41e6e033..688e02c7fd748 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -731,7 +731,6 @@ impl BondedPool { // destroying it cannot switch states, so by being in destroying we are guaranteed no // other delegators can possibly join. (_, true) => { - // TODO: a secondary invariant here is that no delegator can have 0 points. ensure!( target_delegator.active_points() == self.points, Error::::NotOnlyDelegator @@ -1593,7 +1592,7 @@ pub mod pallet { // we certainly don't need to delete any pools, because no one is being removed. SubPoolsStorage::::insert(&delegator.pool_id, sub_pools); Delegators::::insert(&delegator_account, delegator); - Some(T::WeightInfo::withdraw_unbonded_other_update(num_slashing_spans)) + Some(T::WeightInfo::withdraw_unbonded_other_update(num_slashing_spans)) }; Ok(post_info_weight.into()) @@ -2060,6 +2059,7 @@ impl Pallet { let mut all_delegators = 0u32; Delegators::::iter().for_each(|(_, d)| { assert!(BondedPools::::contains_key(d.pool_id)); + assert!(!d.points.is_zero(), "no delegator should have zero points"); *pools_delegators.entry(d.pool_id).or_default() += 1; all_delegators += 1; }); From b2b547046f24638ee55bd462ffc174d5567631f1 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 Apr 2022 15:07:16 +0200 Subject: [PATCH 20/33] Fix the depositor assumption --- frame/nomination-pools/src/lib.rs | 56 ++++++++++++++++++++++------- frame/nomination-pools/src/mock.rs | 12 ++++++- frame/nomination-pools/src/tests.rs | 48 +++++++++++++++++++------ 3 files changed, 92 insertions(+), 24 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index d9a1afa25f197..d6ad91eedc499 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -680,6 +680,15 @@ impl BondedPool { matches!(self.state, PoolState::Destroying) } + fn is_destroying_and_only_bonded_delegator( + &self, + alleged_depositor_points: BalanceOf, + ) -> bool { + // NOTE: if we add `&& self.delegator_counter == 1`, then this becomes even more strict and + // ensures that there ano unbonding delegators hanging around either. + self.is_destroying() && self.points == alleged_depositor_points + } + /// Whether or not the pool is ok to be in `PoolSate::Open`. If this returns an `Err`, then the /// pool is unrecoverable and should be in the destroying state. fn ok_to_be_open(&self, new_funds: BalanceOf) -> Result<(), DispatchError> { @@ -722,6 +731,7 @@ impl BondedPool { caller: &T::AccountId, target_account: &T::AccountId, target_delegator: &Delegator, + unbonding_points: BalanceOf, ) -> Result<(), DispatchError> { let is_permissioned = caller == target_account; let is_depositor = *target_account == self.roles.depositor; @@ -736,16 +746,21 @@ impl BondedPool { }, // Any delegator who is not the depositor can always unbond themselves (true, false) => (), - // The depositor can only start unbonding if the pool is already being destroyed and - // they are the delegator in the pool. Note that an invariant is once the pool is - // destroying it cannot switch states, so by being in destroying we are guaranteed no - // other delegators can possibly join. (_, true) => { - ensure!( - target_delegator.active_points() == self.points, - Error::::NotOnlyDelegator - ); - ensure!(self.is_destroying(), Error::::NotDestroying); + if self.is_destroying_and_only_bonded_delegator(target_delegator.active_points()) { + // if the pool is about to be destroyed, anyone can unbond the depositor, and + // they can fully unbond. + } else { + // only the depositor can partially unbond, and they can only unbond up to the + // threshold. + ensure!(is_permissioned, Error::::DoesNotHavePermission); + let new_depositor_points = + target_delegator.active_points().saturating_sub(unbonding_points); + ensure!( + new_depositor_points >= MinCreateBond::::get(), + Error::::NotOnlyDelegator + ); + } }, }; Ok(()) @@ -1396,14 +1411,20 @@ pub mod pallet { pub fn unbond( origin: OriginFor, delegator_account: T::AccountId, - mut unbonding_points: BalanceOf, + #[pallet::compact] mut unbonding_points: BalanceOf, ) -> DispatchResult { let caller = ensure_signed(origin)?; let (mut delegator, mut bonded_pool, mut reward_pool) = Self::get_delegator_with_pools(&delegator_account)?; - bonded_pool.ok_to_unbond_with(&caller, &delegator_account, &delegator)?; - unbonding_points = unbonding_points.min(delegator.active_points()); + + bonded_pool.ok_to_unbond_with( + &caller, + &delegator_account, + &delegator, + unbonding_points, + )?; + // 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. @@ -2093,13 +2114,22 @@ impl Pallet { all_delegators += 1; }); - BondedPools::::iter().for_each(|(id, bonded_pool)| { + BondedPools::::iter().for_each(|(id, inner)| { + let bonded_pool = BondedPool { id, inner }; 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)); + + let depositor = Delegators::::get(&bonded_pool.roles.depositor).unwrap(); + assert!( + bonded_pool.is_destroying_and_only_bonded_delegator(depositor.active_points()) || + depositor.active_points() >= MinCreateBond::::get(), + "depositor must always have MinCreateBond stake in the pool, except for when the \ + pool is being destroyed and the depositor is the last member", + ); }); assert!(MaxDelegators::::get().map_or(true, |max| all_delegators <= max)); diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index 84ec8934628e0..f5d0cb2c2cc5e 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -284,10 +284,20 @@ pub(crate) fn pool_events_since_last_call() -> Vec> { /// This is useful for backwards compatibility with the majority of tests that only deal with full /// unbonding, not partial unbonding. pub fn fully_unbond(origin: Origin, delegator: AccountId) -> DispatchResult { - let points = Delegators::::get(&delegator).map(|d| d.points).unwrap_or_default(); + let points = Delegators::::get(&delegator) + .map(|d| d.active_points()) + .unwrap_or_default(); Pools::unbond(origin, delegator, points) } +/// Same as `fully_unbond`, in permissioned setting. +pub fn fully_unbond_permissioned(delegator: AccountId) -> DispatchResult { + let points = Delegators::::get(&delegator) + .map(|d| d.active_points()) + .unwrap_or_default(); + Pools::unbond(Origin::signed(delegator), delegator, points) +} + #[cfg(test)] mod test { use super::*; diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 6def033c4cfb7..16edb244317b8 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1372,7 +1372,7 @@ mod unbond { fn unbond_of_1_works() { ExtBuilder::default().build_and_execute(|| { unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(fully_unbond_permissioned(10)); assert_eq!( SubPoolsStorage::::get(1).unwrap().with_era, @@ -1408,7 +1408,7 @@ mod unbond { Balances::make_free_balance_be(&default_reward_account(), ed + 600); // When - assert_ok!(fully_unbond(Origin::signed(40), 40)); + assert_ok!(fully_unbond_permissioned(40)); // Then assert_eq!( @@ -1437,7 +1437,7 @@ mod unbond { // When unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(550), 550)); + assert_ok!(fully_unbond_permissioned(550)); // Then assert_eq!( @@ -1464,7 +1464,7 @@ mod unbond { assert_eq!(Balances::free_balance(&550), 550 + 550); // When - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(fully_unbond_permissioned(10)); // Then assert_eq!( @@ -1514,7 +1514,7 @@ mod unbond { let current_era = 1 + TotalUnbondingPools::::get(); CurrentEra::set(current_era); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(fully_unbond_permissioned(10)); // Then assert_eq!( @@ -1586,7 +1586,7 @@ mod unbond { } #[test] - fn unbond_with_non_admins_works() { + fn unbond_permissionless_works() { // Scenarios where non-admin accounts can unbond others ExtBuilder::default().add_delegators(vec![(100, 100)]).build_and_execute(|| { // Given the pool is blocked @@ -1601,8 +1601,8 @@ mod unbond { // Given the pool is destroying unsafe_set_state(1, PoolState::Destroying).unwrap(); - // The depositor cannot be unbonded until they are the last delegator - assert_noop!(fully_unbond(Origin::signed(420), 10), Error::::NotOnlyDelegator); + // The depositor cannot be fully unbonded until they are the last delegator + assert_noop!(fully_unbond(Origin::signed(10), 10), Error::::NotOnlyDelegator); // Any account can unbond a delegator that is not the depositor assert_ok!(fully_unbond(Origin::signed(420), 100)); @@ -1611,12 +1611,15 @@ mod unbond { unsafe_set_state(1, PoolState::Blocked).unwrap(); // The depositor cannot be unbonded - assert_noop!(fully_unbond(Origin::signed(420), 10), Error::::NotDestroying); + assert_noop!( + fully_unbond(Origin::signed(420), 10), + Error::::DoesNotHavePermission + ); // Given the pools is destroying unsafe_set_state(1, PoolState::Destroying).unwrap(); - // The depositor can be unbonded + // The depositor can be unbonded by anyone. assert_ok!(fully_unbond(Origin::signed(420), 10)); assert_eq!(BondedPools::::get(1).unwrap().points, 0); @@ -1813,6 +1816,31 @@ mod unbond { ); }) } + + #[test] + fn depositor_permissioned_partial_unbond() { + // Scenarios where non-admin accounts can unbond others + ExtBuilder::default() + .ed(1) + .add_delegators(vec![(100, 100)]) + .build_and_execute(|| { + // given + assert_eq!(MinCreateBond::::get(), 2); + assert_eq!(Delegators::::get(10).unwrap().active_points(), 10); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 0); + + // can unbond a bit.. + assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); + assert_eq!(Delegators::::get(10).unwrap().active_points(), 7); + assert_eq!(Delegators::::get(10).unwrap().unbonding_points(), 3); + + // but not less than 2 + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 6), + Error::::NotOnlyDelegator + ); + }); + } } mod pool_withdraw_unbonded { From 70f9f8fb7e7289f1066f1dce23a39954f2f02123 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 Apr 2022 15:49:24 +0200 Subject: [PATCH 21/33] round of self-review --- frame/nomination-pools/src/lib.rs | 95 ++++++++++++++--------------- frame/nomination-pools/src/tests.rs | 9 ++- 2 files changed, 54 insertions(+), 50 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index d6ad91eedc499..91812df4280a7 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -317,7 +317,7 @@ use frame_support::{ Currency, Defensive, DefensiveOption, DefensiveResult, DefensiveSaturating, ExistenceRequirement, Get, }, - DefaultNoBound, RuntimeDebugNoBound, + CloneNoBound, DefaultNoBound, PartialEqNoBound, RuntimeDebugNoBound, }; use scale_info::TypeInfo; use sp_core::U256; @@ -384,14 +384,7 @@ enum AccountType { /// A delegator in a pool. #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound)] -#[cfg_attr( - feature = "std", - derive( - frame_support::CloneNoBound, - frame_support::PartialEqNoBound, - frame_support::DefaultNoBound - ) -)] +#[cfg_attr(feature = "std", derive(CloneNoBound, PartialEqNoBound, DefaultNoBound))] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct Delegator { @@ -768,24 +761,21 @@ impl BondedPool { /// # Returns /// - /// * Ok(true) if [`Call::withdraw_unbonded`] can be called and the target account is the - /// depositor. - /// * Ok(false) if [`Call::withdraw_unbonded`] can be called and target account is *not* the - /// depositor. - /// * Err(DispatchError) if [`Call::withdraw_unbonded`] *cannot* be called. + /// * Ok(()) if [`Call::withdraw_unbonded`] can be called, `Err(DispatchError)` otherwise. fn ok_to_withdraw_unbonded_with( &self, caller: &T::AccountId, target_account: &T::AccountId, target_delegator: &Delegator, sub_pools: &SubPools, - ) -> Result { + ) -> Result<(), DispatchError> { if *target_account == self.roles.depositor { ensure!( sub_pools.sum_unbonding_points() == target_delegator.unbonding_points(), Error::::NotOnlyDelegator ); - Ok(true) + debug_assert_eq!(self.delegator_counter, 1, "only delegator must exist at this point"); + Ok(()) } else { // This isn't a depositor let is_permissioned = caller == target_account; @@ -793,7 +783,7 @@ impl BondedPool { is_permissioned || self.can_kick(caller) || self.is_destroying(), Error::::NotKickerOrDestroying ); - Ok(false) + Ok(()) } } @@ -1411,12 +1401,11 @@ pub mod pallet { pub fn unbond( origin: OriginFor, delegator_account: T::AccountId, - #[pallet::compact] mut unbonding_points: BalanceOf, + #[pallet::compact] unbonding_points: BalanceOf, ) -> DispatchResult { let caller = ensure_signed(origin)?; let (mut delegator, mut bonded_pool, mut reward_pool) = Self::get_delegator_with_pools(&delegator_account)?; - unbonding_points = unbonding_points.min(delegator.active_points()); bonded_pool.ok_to_unbond_with( &caller, @@ -1503,8 +1492,8 @@ pub mod pallet { Ok(()) } - /// Withdraw unbonded funds from `delegator_account`. If - /// no bonded funds can be unbonded, an error is returned. + /// Withdraw unbonded funds from `delegator_account`. If no bonded funds can be unbonded, an + /// error is returned. /// /// Under certain conditions, this call can be dispatched permissionlessly (i.e. by any /// account). @@ -1541,7 +1530,7 @@ pub mod pallet { let mut sub_pools = SubPoolsStorage::::get(delegator.pool_id) .defensive_ok_or_else(|| Error::::SubPoolsNotFound)?; - let should_remove_pool = bonded_pool.ok_to_withdraw_unbonded_with( + bonded_pool.ok_to_withdraw_unbonded_with( &caller, &delegator_account, &delegator, @@ -1600,31 +1589,9 @@ pub mod pallet { let post_info_weight = if delegator.active_points().is_zero() { // delegator being reaped. Delegators::::remove(&delegator_account); - if should_remove_pool { - let reward_account = bonded_pool.reward_account(); - ReversePoolIdLookup::::remove(bonded_pool.bonded_account()); - RewardPools::::remove(delegator.pool_id); - SubPoolsStorage::::remove(delegator.pool_id); - // Kill accounts from storage by making their balance go below ED. We assume - // that the accounts have no references that would prevent destruction once we - // get to this point. - debug_assert_eq!( - T::StakingInterface::total_stake(&bonded_pool.bonded_account()) - .unwrap_or_default(), - Zero::zero() - ); - let reward_pool_remaining = T::Currency::free_balance(&reward_account); - // This shouldn't fail, but if it does we don't really care - let _ = T::Currency::transfer( - &reward_account, - &delegator_account, - reward_pool_remaining, - ExistenceRequirement::AllowDeath, - ); - T::Currency::make_free_balance_be(&reward_account, Zero::zero()); - T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); - Self::deposit_event(Event::::Destroyed { pool_id: delegator.pool_id }); - bonded_pool.remove(); + + if delegator_account == bonded_pool.roles.depositor { + Pallet::::dissolve_pool(bonded_pool); None } else { bonded_pool.dec_delegators().put(); @@ -1856,6 +1823,38 @@ pub mod pallet { } impl Pallet { + /// Remove everything related to the given bonded pool. + /// + /// All sub-pools are also deleted. All accounts are dusted and the leftover of the reward + /// account is returned to the depositor. + pub fn dissolve_pool(bonded_pool: BondedPool) { + // .. and the pool being destroyed now. + let reward_account = bonded_pool.reward_account(); + let bonded_account = bonded_pool.bonded_account(); + ReversePoolIdLookup::::remove(&bonded_account); + RewardPools::::remove(bonded_pool.id); + SubPoolsStorage::::remove(bonded_pool.id); + // Kill accounts from storage by making their balance go below ED. We assume + // that the accounts have no references that would prevent destruction once we + // get to this point. + debug_assert_eq!( + T::StakingInterface::total_stake(&bonded_account).unwrap_or_default(), + Zero::zero() + ); + let reward_pool_remaining = T::Currency::free_balance(&reward_account); + // This shouldn't fail, but if it does we don't really care + let _ = T::Currency::transfer( + &reward_account, + &bonded_pool.roles.depositor, + reward_pool_remaining, + ExistenceRequirement::AllowDeath, + ); + T::Currency::make_free_balance_be(&reward_account, Zero::zero()); + T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); + Self::deposit_event(Event::::Destroyed { pool_id: bonded_pool.id }); + bonded_pool.remove(); + } + /// Create the main, bonded account of a pool with the given id. pub fn create_bonded_account(id: PoolId) -> T::AccountId { T::PalletId::get().into_sub_account((AccountType::Bonded, id)) @@ -2011,7 +2010,7 @@ impl Pallet { /// If the delegator has some rewards, transfer a payout from the reward pool to the delegator. // Emits events and potentially modifies pool state if any arithmetic saturates, but does // not persist any of the mutable inputs to storage. - pub fn do_reward_payout( + fn do_reward_payout( delegator_account: &T::AccountId, delegator: &mut Delegator, bonded_pool: &mut BondedPool, diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 16edb244317b8..33b50114d402d 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -1760,8 +1760,13 @@ mod unbond { } ); - // when: unbonding more than our active: capped - assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); + // when: unbonding more than our active: error + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 5), + Error::::NotEnoughPointsToUnbond + ); + // instead: + assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); // then assert_eq!(Delegators::::get(10).unwrap().active_points(), 0); From 4d389f557a67ecdbcee8b9bcd7d0753439924505 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 15 Apr 2022 15:56:29 +0200 Subject: [PATCH 22/33] little bit more cleanup --- frame/nomination-pools/src/lib.rs | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 91812df4280a7..7912e336f60de 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1828,29 +1828,39 @@ impl Pallet { /// All sub-pools are also deleted. All accounts are dusted and the leftover of the reward /// account is returned to the depositor. pub fn dissolve_pool(bonded_pool: BondedPool) { - // .. and the pool being destroyed now. let reward_account = bonded_pool.reward_account(); let bonded_account = bonded_pool.bonded_account(); + ReversePoolIdLookup::::remove(&bonded_account); RewardPools::::remove(bonded_pool.id); SubPoolsStorage::::remove(bonded_pool.id); - // Kill accounts from storage by making their balance go below ED. We assume - // that the accounts have no references that would prevent destruction once we - // get to this point. + + // Kill accounts from storage by making their balance go below ED. We assume that the + // accounts have no references that would prevent destruction once we get to this point. We + // don't work with the system pallet directly, but + // 1. we drain the reward account and kill it. This account should never have any extra + // consumers anyway. + // 2. the bonded account should become a 'killed stash' in the staking system, and all of + // its consumers removed. + debug_assert_eq!(frame_system::Pallet::::consumers(&reward_account), 0); + debug_assert_eq!(frame_system::Pallet::::consumers(&bonded_account), 0); debug_assert_eq!( T::StakingInterface::total_stake(&bonded_account).unwrap_or_default(), Zero::zero() ); - let reward_pool_remaining = T::Currency::free_balance(&reward_account); + // This shouldn't fail, but if it does we don't really care + let reward_pool_remaining = T::Currency::free_balance(&reward_account); let _ = T::Currency::transfer( &reward_account, &bonded_pool.roles.depositor, reward_pool_remaining, ExistenceRequirement::AllowDeath, ); + T::Currency::make_free_balance_be(&reward_account, Zero::zero()); T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); + Self::deposit_event(Event::::Destroyed { pool_id: bonded_pool.id }); bonded_pool.remove(); } From 31c522d1778029882d5bf7d9aeb581b7ed48624c Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Fri, 15 Apr 2022 16:52:31 -0700 Subject: [PATCH 23/33] Update frame/nomination-pools/src/mock.rs --- frame/nomination-pools/src/mock.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index f5d0cb2c2cc5e..f9bc757a97770 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -25,7 +25,6 @@ parameter_types! { #[derive(Clone, PartialEq)] pub static MaxUnbonding: u32 = 8; pub static Nominations: Vec = vec![]; - } pub struct StakingMock; From d7fabb291ee2f726693e4d3fab02a3a22648ca6d Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Fri, 15 Apr 2022 17:14:35 -0700 Subject: [PATCH 24/33] Apply suggestions from code review --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 7912e336f60de..879c5003dea98 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -609,7 +609,7 @@ impl BondedPool { points_to_issue } - /// Dissolve i.e. unbond the given amount of points from this pool. This is the opposite of + /// Dissolve some points from the pool i.e. unbond the given amount of points from this pool. This is the opposite of /// issuing some funds into the pool. /// /// Mutates self in place, but does not write anything to storage. From 40c21f54d083b9d40cbd493b8b3bc0e97083506a Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 18 Apr 2022 09:29:38 +0200 Subject: [PATCH 25/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 879c5003dea98..6bd2e6f1dbfd0 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -678,7 +678,7 @@ impl BondedPool { alleged_depositor_points: BalanceOf, ) -> bool { // NOTE: if we add `&& self.delegator_counter == 1`, then this becomes even more strict and - // ensures that there ano unbonding delegators hanging around either. + // ensures that there are no unbonding delegators hanging around either. self.is_destroying() && self.points == alleged_depositor_points } From cb56178606de72ec6649c6bee5cd2b7df2df28e3 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 19 Apr 2022 09:19:18 +0200 Subject: [PATCH 26/33] Fix interpretation of MinCreateBond --- frame/nomination-pools/src/lib.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 7912e336f60de..f4c8ba4b25ea5 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -409,6 +409,15 @@ impl Delegator { self.active_points().saturating_add(self.unbonding_points()) } + pub(crate) fn active_balance(&self) -> BalanceOf { + if let Some(pool) = BondedPool::::get(self.pool_id).defensive() { + // TODO: this function is questionable in both name and implementation. + pool.balance_to_unbond(self.points) + } else { + Zero::zero() + } + } + /// Active points of the delegator. pub(crate) fn active_points(&self) -> BalanceOf { self.points @@ -747,10 +756,15 @@ impl BondedPool { // only the depositor can partially unbond, and they can only unbond up to the // threshold. ensure!(is_permissioned, Error::::DoesNotHavePermission); - let new_depositor_points = - target_delegator.active_points().saturating_sub(unbonding_points); + let balance_after_unbond = { + let new_depositor_points = + target_delegator.active_points().saturating_sub(unbonding_points); + let mut delegator_after_unbond = target_delegator.clone(); + delegator_after_unbond.points = new_depositor_points; + delegator_after_unbond.active_balance() + }; ensure!( - new_depositor_points >= MinCreateBond::::get(), + balance_after_unbond >= MinCreateBond::::get(), Error::::NotOnlyDelegator ); } From 011e8bc50a5876c458356d345b6d11ee4c824295 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 19 Apr 2022 09:27:40 +0200 Subject: [PATCH 27/33] controvesial refactor --- frame/nomination-pools/src/lib.rs | 59 ++++++++++++------------ frame/nomination-pools/src/tests.rs | 70 ++++++++++++++--------------- 2 files changed, 66 insertions(+), 63 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index a8b90bb137cc0..ab42e9999208e 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -412,7 +412,7 @@ impl Delegator { pub(crate) fn active_balance(&self) -> BalanceOf { if let Some(pool) = BondedPool::::get(self.pool_id).defensive() { // TODO: this function is questionable in both name and implementation. - pool.balance_to_unbond(self.points) + pool.points_to_balance(self.points) } else { Zero::zero() } @@ -596,30 +596,33 @@ impl BondedPool { BondedPools::::remove(self.id); } - /// 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 { + /// Convert the given amount of balance to points given the current pool state. + /// + /// This is often used for bonding and issuing new funds into the pool. + fn balance_to_point(&self, new_funds: BalanceOf) -> BalanceOf { let bonded_balance = T::StakingInterface::active_stake(&self.bonded_account()).unwrap_or(Zero::zero()); - Pallet::::points_to_issue(bonded_balance, self.points, new_funds) + Pallet::::balance_to_point(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 { + /// Convert the given number of points to balance given the current pool state. + /// + /// This is often used for unbonding. + fn points_to_balance(&self, points: BalanceOf) -> BalanceOf { let bonded_balance = T::StakingInterface::active_stake(&self.bonded_account()).unwrap_or(Zero::zero()); - Pallet::::balance_to_unbond(bonded_balance, self.points, delegator_points) + Pallet::::point_to_balance(bonded_balance, self.points, points) } /// Issue points to [`Self`] for `new_funds`. fn issue(&mut self, new_funds: BalanceOf) -> BalanceOf { - let points_to_issue = self.points_to_issue(new_funds); + let points_to_issue = self.balance_to_point(new_funds); self.points = self.points.saturating_add(points_to_issue); - points_to_issue } - /// Dissolve some points from the pool i.e. unbond the given amount of points from this pool. This is the opposite of - /// issuing some funds into the pool. + /// Dissolve some points from the pool i.e. unbond the given amount of points from this pool. + /// This is the opposite of issuing some funds into the pool. /// /// Mutates self in place, but does not write anything to storage. /// @@ -627,7 +630,7 @@ impl BondedPool { fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { // NOTE: do not optimize by removing `balance`. it must be computed before mutating // `self.point`. - let balance = self.balance_to_unbond(points); + let balance = self.points_to_balance(points); self.points = self.points.saturating_sub(points); balance } @@ -927,17 +930,17 @@ pub struct UnbondPool { } impl UnbondPool { - fn points_to_issue(&self, new_funds: BalanceOf) -> BalanceOf { - Pallet::::points_to_issue(self.balance, self.points, new_funds) + fn balance_to_point(&self, new_funds: BalanceOf) -> BalanceOf { + Pallet::::balance_to_point(self.balance, self.points, new_funds) } - fn balance_to_unbond(&self, delegator_points: BalanceOf) -> BalanceOf { - Pallet::::balance_to_unbond(self.balance, self.points, delegator_points) + fn point_to_balance(&self, points: BalanceOf) -> BalanceOf { + Pallet::::point_to_balance(self.balance, self.points, points) } /// Issue points and update the balance given `new_balance`. fn issue(&mut self, new_funds: BalanceOf) { - self.points = self.points.saturating_add(self.points_to_issue(new_funds)); + self.points = self.points.saturating_add(self.balance_to_point(new_funds)); self.balance = self.balance.saturating_add(new_funds); } @@ -948,7 +951,7 @@ impl UnbondPool { /// /// Returns the actual amount of `Balance` that was removed from the pool. fn dissolve(&mut self, points: BalanceOf) -> BalanceOf { - let balance_to_unbond = self.balance_to_unbond(points); + let balance_to_unbond = self.point_to_balance(points); self.points = self.points.saturating_sub(points); self.balance = self.balance.saturating_sub(balance_to_unbond); @@ -1916,9 +1919,9 @@ impl Pallet { Delegators::::insert(delegator_account, delegator); } - /// Calculate the number of points to issue from a pool as `(current_points / current_balance) * - /// new_funds` except for some zero edge cases; see logic and tests for details. - fn points_to_issue( + /// Calculate the equivalent point of `new_funds` in a pool with `current_balance` and + /// `current_points`. + fn balance_to_point( current_balance: BalanceOf, current_points: BalanceOf, new_funds: BalanceOf, @@ -1944,21 +1947,21 @@ impl Pallet { } } - // Calculate the balance of a pool to unbond as `(current_balance / current_points) * - // delegator_points`. Returns zero if any of the inputs are zero. - fn balance_to_unbond( + /// Calculate the equivalent balance of `points` in a pool with `current_balance` and + /// `current_points`. + fn point_to_balance( current_balance: BalanceOf, current_points: BalanceOf, - delegator_points: BalanceOf, + points: BalanceOf, ) -> BalanceOf { - if current_balance.is_zero() || current_points.is_zero() || delegator_points.is_zero() { + if current_balance.is_zero() || current_points.is_zero() || points.is_zero() { // There is nothing to unbond return Zero::zero() } - // Equivalent of (current_balance / current_points) * delegator_points + // Equivalent of (current_balance / current_points) * points current_balance - .saturating_mul(delegator_points) + .saturating_mul(points) // We check for zero above .div(current_points) } diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index 33b50114d402d..bb5892ad8caaf 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -89,41 +89,41 @@ mod bonded_pool { // 1 points : 1 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 100); - assert_eq!(bonded_pool.points_to_issue(10), 10); - assert_eq!(bonded_pool.points_to_issue(0), 0); + assert_eq!(bonded_pool.balance_to_point(10), 10); + assert_eq!(bonded_pool.balance_to_point(0), 0); // 2 points : 1 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 50); - assert_eq!(bonded_pool.points_to_issue(10), 20); + assert_eq!(bonded_pool.balance_to_point(10), 20); // 1 points : 2 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 100); bonded_pool.points = 50; - assert_eq!(bonded_pool.points_to_issue(10), 5); + assert_eq!(bonded_pool.balance_to_point(10), 5); // 100 points : 0 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 0); bonded_pool.points = 100; - assert_eq!(bonded_pool.points_to_issue(10), 100 * 10); + assert_eq!(bonded_pool.balance_to_point(10), 100 * 10); // 0 points : 100 balance StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 100); bonded_pool.points = 100; - assert_eq!(bonded_pool.points_to_issue(10), 10); + assert_eq!(bonded_pool.balance_to_point(10), 10); // 10 points : 3 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 30); - assert_eq!(bonded_pool.points_to_issue(10), 33); + assert_eq!(bonded_pool.balance_to_point(10), 33); // 2 points : 3 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 300); bonded_pool.points = 200; - assert_eq!(bonded_pool.points_to_issue(10), 6); + assert_eq!(bonded_pool.balance_to_point(10), 6); // 4 points : 9 balance ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 900); bonded_pool.points = 400; - assert_eq!(bonded_pool.points_to_issue(90), 40); + assert_eq!(bonded_pool.balance_to_point(90), 40); } #[test] @@ -140,37 +140,37 @@ mod bonded_pool { }; StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 100); - assert_eq!(bonded_pool.balance_to_unbond(10), 10); - assert_eq!(bonded_pool.balance_to_unbond(0), 0); + assert_eq!(bonded_pool.points_to_balance(10), 10); + assert_eq!(bonded_pool.points_to_balance(0), 0); // 2 balance : 1 points ratio bonded_pool.points = 50; - assert_eq!(bonded_pool.balance_to_unbond(10), 20); + assert_eq!(bonded_pool.points_to_balance(10), 20); // 100 balance : 0 points ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 0); bonded_pool.points = 0; - assert_eq!(bonded_pool.balance_to_unbond(10), 0); + assert_eq!(bonded_pool.points_to_balance(10), 0); // 0 balance : 100 points ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 0); bonded_pool.points = 100; - assert_eq!(bonded_pool.balance_to_unbond(10), 0); + assert_eq!(bonded_pool.points_to_balance(10), 0); // 10 balance : 3 points ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 100); bonded_pool.points = 30; - assert_eq!(bonded_pool.balance_to_unbond(10), 33); + assert_eq!(bonded_pool.points_to_balance(10), 33); // 2 balance : 3 points ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 200); bonded_pool.points = 300; - assert_eq!(bonded_pool.balance_to_unbond(10), 6); + assert_eq!(bonded_pool.points_to_balance(10), 6); // 4 balance : 9 points ratio StakingMock::set_bonded_balance(bonded_pool.bonded_account(), 400); bonded_pool.points = 900; - assert_eq!(bonded_pool.balance_to_unbond(90), 40); + assert_eq!(bonded_pool.points_to_balance(90), 40); } #[test] @@ -244,72 +244,72 @@ mod unbond_pool { fn points_to_issue_works() { // 1 points : 1 balance ratio let unbond_pool = UnbondPool:: { points: 100, balance: 100 }; - assert_eq!(unbond_pool.points_to_issue(10), 10); - assert_eq!(unbond_pool.points_to_issue(0), 0); + assert_eq!(unbond_pool.balance_to_point(10), 10); + assert_eq!(unbond_pool.balance_to_point(0), 0); // 2 points : 1 balance ratio let unbond_pool = UnbondPool:: { points: 100, balance: 50 }; - assert_eq!(unbond_pool.points_to_issue(10), 20); + assert_eq!(unbond_pool.balance_to_point(10), 20); // 1 points : 2 balance ratio let unbond_pool = UnbondPool:: { points: 50, balance: 100 }; - assert_eq!(unbond_pool.points_to_issue(10), 5); + assert_eq!(unbond_pool.balance_to_point(10), 5); // 100 points : 0 balance ratio let unbond_pool = UnbondPool:: { points: 100, balance: 0 }; - assert_eq!(unbond_pool.points_to_issue(10), 100 * 10); + assert_eq!(unbond_pool.balance_to_point(10), 100 * 10); // 0 points : 100 balance let unbond_pool = UnbondPool:: { points: 0, balance: 100 }; - assert_eq!(unbond_pool.points_to_issue(10), 10); + assert_eq!(unbond_pool.balance_to_point(10), 10); // 10 points : 3 balance ratio let unbond_pool = UnbondPool:: { points: 100, balance: 30 }; - assert_eq!(unbond_pool.points_to_issue(10), 33); + assert_eq!(unbond_pool.balance_to_point(10), 33); // 2 points : 3 balance ratio let unbond_pool = UnbondPool:: { points: 200, balance: 300 }; - assert_eq!(unbond_pool.points_to_issue(10), 6); + assert_eq!(unbond_pool.balance_to_point(10), 6); // 4 points : 9 balance ratio let unbond_pool = UnbondPool:: { points: 400, balance: 900 }; - assert_eq!(unbond_pool.points_to_issue(90), 40); + assert_eq!(unbond_pool.balance_to_point(90), 40); } #[test] fn balance_to_unbond_works() { // 1 balance : 1 points ratio let unbond_pool = UnbondPool:: { points: 100, balance: 100 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 10); - assert_eq!(unbond_pool.balance_to_unbond(0), 0); + assert_eq!(unbond_pool.point_to_balance(10), 10); + assert_eq!(unbond_pool.point_to_balance(0), 0); // 1 balance : 2 points ratio let unbond_pool = UnbondPool:: { points: 100, balance: 50 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 5); + assert_eq!(unbond_pool.point_to_balance(10), 5); // 2 balance : 1 points ratio let unbond_pool = UnbondPool:: { points: 50, balance: 100 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 20); + assert_eq!(unbond_pool.point_to_balance(10), 20); // 100 balance : 0 points ratio let unbond_pool = UnbondPool:: { points: 0, balance: 100 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 0); + assert_eq!(unbond_pool.point_to_balance(10), 0); // 0 balance : 100 points ratio let unbond_pool = UnbondPool:: { points: 100, balance: 0 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 0); + assert_eq!(unbond_pool.point_to_balance(10), 0); // 10 balance : 3 points ratio let unbond_pool = UnbondPool:: { points: 30, balance: 100 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 33); + assert_eq!(unbond_pool.point_to_balance(10), 33); // 2 balance : 3 points ratio let unbond_pool = UnbondPool:: { points: 300, balance: 200 }; - assert_eq!(unbond_pool.balance_to_unbond(10), 6); + assert_eq!(unbond_pool.point_to_balance(10), 6); // 4 balance : 9 points ratio let unbond_pool = UnbondPool:: { points: 900, balance: 400 }; - assert_eq!(unbond_pool.balance_to_unbond(90), 40); + assert_eq!(unbond_pool.point_to_balance(90), 40); } } From 49409d7551bb9ba3f56d554339376ea96b2468d1 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 19 Apr 2022 09:30:34 +0200 Subject: [PATCH 28/33] rename --- frame/nomination-pools/src/lib.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index ab42e9999208e..43ba9aa124961 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -411,7 +411,6 @@ impl Delegator { pub(crate) fn active_balance(&self) -> BalanceOf { if let Some(pool) = BondedPool::::get(self.pool_id).defensive() { - // TODO: this function is questionable in both name and implementation. pool.points_to_balance(self.points) } else { Zero::zero() @@ -762,9 +761,9 @@ impl BondedPool { let balance_after_unbond = { let new_depositor_points = target_delegator.active_points().saturating_sub(unbonding_points); - let mut delegator_after_unbond = target_delegator.clone(); - delegator_after_unbond.points = new_depositor_points; - delegator_after_unbond.active_balance() + let mut depositor_after_unbond = target_delegator.clone(); + depositor_after_unbond.points = new_depositor_points; + depositor_after_unbond.active_balance() }; ensure!( balance_after_unbond >= MinCreateBond::::get(), From 8e8aa5adf8029d88fed5f43fe0697357b1d4f063 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 19 Apr 2022 12:29:55 +0200 Subject: [PATCH 29/33] make everything build --- bin/node/runtime/src/lib.rs | 1 + frame/nomination-pools/Cargo.toml | 1 + .../nomination-pools/benchmarking/Cargo.toml | 2 +- .../nomination-pools/benchmarking/src/lib.rs | 19 ++++-- .../nomination-pools/benchmarking/src/mock.rs | 1 + frame/nomination-pools/src/lib.rs | 21 +++++-- frame/nomination-pools/src/mock.rs | 11 ---- frame/nomination-pools/src/tests.rs | 62 ++++++++++--------- 8 files changed, 68 insertions(+), 50 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index b9b5c29f82694..30197ec718337 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -731,6 +731,7 @@ impl pallet_nomination_pools::Config for Runtime { type StakingInterface = pallet_staking::Pallet; type PostUnbondingPoolsWindow = PostUnbondPoolsWindow; type MaxMetadataLen = ConstU32<256>; + type MaxUnbonding = ConstU32<8>; type PalletId = NominationPoolsPalletId; } diff --git a/frame/nomination-pools/Cargo.toml b/frame/nomination-pools/Cargo.toml index 6565e695901e2..5e485e62f21d9 100644 --- a/frame/nomination-pools/Cargo.toml +++ b/frame/nomination-pools/Cargo.toml @@ -30,6 +30,7 @@ pallet-balances = { version = "4.0.0-dev", path = "../balances" } sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" } [features] +runtime-benchmarks = [] default = ["std"] std = [ "codec/std", diff --git a/frame/nomination-pools/benchmarking/Cargo.toml b/frame/nomination-pools/benchmarking/Cargo.toml index 700e626e79ae0..8416216d12e94 100644 --- a/frame/nomination-pools/benchmarking/Cargo.toml +++ b/frame/nomination-pools/benchmarking/Cargo.toml @@ -24,7 +24,7 @@ frame-support = { version = "4.0.0-dev", default-features = false, path = "../.. frame-system = { version = "4.0.0-dev", default-features = false, path = "../../system" } pallet-bags-list = { version = "4.0.0-dev", default-features = false, features = ["runtime-benchmarks"], path = "../../bags-list" } pallet-staking = { version = "4.0.0-dev", default-features = false, features = ["runtime-benchmarks"], path = "../../staking" } -pallet-nomination-pools = { version = "1.0.0", default-features = false, path = "../" } +pallet-nomination-pools = { version = "1.0.0", default-features = false, path = "../", features = ["runtime-benchmarks"] } # Substrate Primitives sp-runtime = { version = "6.0.0", default-features = false, path = "../../../primitives/runtime" } diff --git a/frame/nomination-pools/benchmarking/src/lib.rs b/frame/nomination-pools/benchmarking/src/lib.rs index c2d118113b60b..50dd97d2cd1a5 100644 --- a/frame/nomination-pools/benchmarking/src/lib.rs +++ b/frame/nomination-pools/benchmarking/src/lib.rs @@ -289,13 +289,13 @@ frame_benchmarking::benchmarks! { .map_err(|_| "balance expected to be a u128") .unwrap(); let scenario = ListScenario::::new(origin_weight, false)?; - let amount = origin_weight - scenario.dest_weight.clone(); let scenario = scenario.add_joiner(amount); let delegator_id = scenario.origin1_delegator.unwrap().clone(); + let all_points = Delegators::::get(&delegator_id).unwrap().points; whitelist_account!(delegator_id); - }: _(Origin::Signed(delegator_id.clone()), delegator_id.clone()) + }: _(Origin::Signed(delegator_id.clone()), delegator_id.clone(), all_points) verify { let bonded_after = T::StakingInterface::active_stake(&scenario.origin1).unwrap(); // We at least went down to the destination bag @@ -304,7 +304,14 @@ frame_benchmarking::benchmarks! { &delegator_id ) .unwrap(); - assert_eq!(delegator.unbonding_era, Some(0 + T::StakingInterface::bonding_duration())); + assert_eq!( + delegator.unbonding_eras.keys().cloned().collect::>(), + vec![0 + T::StakingInterface::bonding_duration()] + ); + assert_eq!( + delegator.unbonding_eras.values().cloned().collect::>(), + vec![all_points] + ); } pool_withdraw_unbonded { @@ -329,7 +336,7 @@ frame_benchmarking::benchmarks! { assert_eq!(CurrencyOf::::free_balance(&joiner), min_join_bond); // Unbond the new delegator - Pools::::unbond(Origin::Signed(joiner.clone()).into(), joiner.clone()).unwrap(); + Pools::::fully_unbond(Origin::Signed(joiner.clone()).into(), joiner.clone()).unwrap(); // Sanity check that unbond worked assert_eq!( @@ -374,7 +381,7 @@ frame_benchmarking::benchmarks! { // Unbond the new delegator pallet_staking::CurrentEra::::put(0); - Pools::::unbond(Origin::Signed(joiner.clone()).into(), joiner.clone()).unwrap(); + Pools::::fully_unbond(Origin::Signed(joiner.clone()).into(), joiner.clone()).unwrap(); // Sanity check that unbond worked assert_eq!( @@ -423,7 +430,7 @@ frame_benchmarking::benchmarks! { // up when unbonding. let reward_account = Pools::::create_reward_account(1); assert!(frame_system::Account::::contains_key(&reward_account)); - Pools::::unbond(Origin::Signed(depositor.clone()).into(), depositor.clone()).unwrap(); + Pools::::fully_unbond(Origin::Signed(depositor.clone()).into(), depositor.clone()).unwrap(); // Sanity check that unbond worked assert_eq!( diff --git a/frame/nomination-pools/benchmarking/src/mock.rs b/frame/nomination-pools/benchmarking/src/mock.rs index b6a43471b8355..94b7f300099a4 100644 --- a/frame/nomination-pools/benchmarking/src/mock.rs +++ b/frame/nomination-pools/benchmarking/src/mock.rs @@ -155,6 +155,7 @@ impl pallet_nomination_pools::Config for Runtime { type StakingInterface = Staking; type PostUnbondingPoolsWindow = PostUnbondingPoolsWindow; type MaxMetadataLen = ConstU32<256>; + type MaxUnbonding = ConstU32<8>; type PalletId = PoolsPalletId; } diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 7bbbf243a49a4..053fecea614e7 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -215,7 +215,6 @@ //! after the election snapshot is taken it will benefit from the rewards of the next _2_ eras //! because it's vote weight will not be counted until the election snapshot in active era + 1. //! Related: -//! // _Note to maintainers_: In order to ensure the reward account never falls below the existential // deposit, at creation the reward account must be endowed with the existential deposit. All logic // for calculating rewards then does not see that existential deposit as part of the free balance. @@ -383,8 +382,8 @@ enum AccountType { } /// A delegator in a pool. -#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound)] -#[cfg_attr(feature = "std", derive(CloneNoBound, PartialEqNoBound, DefaultNoBound))] +#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, CloneNoBound)] +#[cfg_attr(feature = "std", derive(PartialEqNoBound, DefaultNoBound))] #[codec(mel_bound(T: Config))] #[scale_info(skip_type_params(T))] pub struct Delegator { @@ -761,7 +760,7 @@ impl BondedPool { let balance_after_unbond = { let new_depositor_points = target_delegator.active_points().saturating_sub(unbonding_points); - let mut depositor_after_unbond = target_delegator.clone(); + let mut depositor_after_unbond = (*target_delegator).clone(); depositor_after_unbond.points = new_depositor_points; depositor_after_unbond.active_balance() }; @@ -2184,6 +2183,20 @@ impl Pallet { Ok(()) } + + /// Fully unbond the shares of `delegator`, when executed from `origin`. + /// + /// This is useful for backwards compatibility with the majority of tests that only deal with + /// full unbonding, not partial unbonding. + #[cfg(any(feature = "runtime-benchmarks", test))] + pub fn fully_unbond( + origin: frame_system::pallet_prelude::OriginFor, + delegator: T::AccountId, + ) -> DispatchResult { + let points = + Delegators::::get(&delegator).map(|d| d.active_points()).unwrap_or_default(); + Self::unbond(origin, delegator, points) + } } impl OnStakerSlash> for Pallet { diff --git a/frame/nomination-pools/src/mock.rs b/frame/nomination-pools/src/mock.rs index f9bc757a97770..a84c45398ff72 100644 --- a/frame/nomination-pools/src/mock.rs +++ b/frame/nomination-pools/src/mock.rs @@ -278,17 +278,6 @@ pub(crate) fn pool_events_since_last_call() -> Vec> { events.into_iter().skip(already_seen).collect() } -/// Fully unbond the shares of `delegator`, when executed from `origin`. -/// -/// This is useful for backwards compatibility with the majority of tests that only deal with full -/// unbonding, not partial unbonding. -pub fn fully_unbond(origin: Origin, delegator: AccountId) -> DispatchResult { - let points = Delegators::::get(&delegator) - .map(|d| d.active_points()) - .unwrap_or_default(); - Pools::unbond(origin, delegator, points) -} - /// Same as `fully_unbond`, in permissioned setting. pub fn fully_unbond_permissioned(delegator: AccountId) -> DispatchResult { let points = Delegators::::get(&delegator) diff --git a/frame/nomination-pools/src/tests.rs b/frame/nomination-pools/src/tests.rs index bb5892ad8caaf..350bc97295f23 100644 --- a/frame/nomination-pools/src/tests.rs +++ b/frame/nomination-pools/src/tests.rs @@ -816,7 +816,7 @@ mod claim_payout { fn reward_payout_errors_if_a_delegator_is_fully_unbonding() { ExtBuilder::default().add_delegators(vec![(11, 11)]).build_and_execute(|| { // fully unbond the delegator. - assert_ok!(fully_unbond(Origin::signed(11), 11)); + assert_ok!(Pools::fully_unbond(Origin::signed(11), 11)); let mut bonded_pool = BondedPool::::get(1).unwrap(); let mut reward_pool = RewardPools::::get(1).unwrap(); @@ -1545,15 +1545,15 @@ mod unbond { // When the nominator trys to kick, then its a noop assert_noop!( - fully_unbond(Origin::signed(901), 100), + Pools::fully_unbond(Origin::signed(901), 100), Error::::NotKickerOrDestroying ); // When the root kicks then its ok - assert_ok!(fully_unbond(Origin::signed(900), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(900), 100)); // When the state toggler kicks then its ok - assert_ok!(fully_unbond(Origin::signed(902), 200)); + assert_ok!(Pools::fully_unbond(Origin::signed(902), 200)); assert_eq!( BondedPool::::get(1).unwrap(), @@ -1594,7 +1594,7 @@ mod unbond { // A permissionless unbond attempt errors assert_noop!( - fully_unbond(Origin::signed(420), 100), + Pools::fully_unbond(Origin::signed(420), 100), Error::::NotKickerOrDestroying ); @@ -1602,17 +1602,20 @@ mod unbond { unsafe_set_state(1, PoolState::Destroying).unwrap(); // The depositor cannot be fully unbonded until they are the last delegator - assert_noop!(fully_unbond(Origin::signed(10), 10), Error::::NotOnlyDelegator); + assert_noop!( + Pools::fully_unbond(Origin::signed(10), 10), + Error::::NotOnlyDelegator + ); // Any account can unbond a delegator that is not the depositor - assert_ok!(fully_unbond(Origin::signed(420), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(420), 100)); // Given the pool is blocked unsafe_set_state(1, PoolState::Blocked).unwrap(); // The depositor cannot be unbonded assert_noop!( - fully_unbond(Origin::signed(420), 10), + Pools::fully_unbond(Origin::signed(420), 10), Error::::DoesNotHavePermission ); @@ -1620,7 +1623,7 @@ mod unbond { unsafe_set_state(1, PoolState::Destroying).unwrap(); // The depositor can be unbonded by anyone. - assert_ok!(fully_unbond(Origin::signed(420), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(420), 10)); assert_eq!(BondedPools::::get(1).unwrap().points, 0); assert_eq!( @@ -1645,13 +1648,16 @@ mod unbond { #[should_panic = "Defensive failure has been triggered!"] fn unbond_errors_correctly() { ExtBuilder::default().build_and_execute(|| { - assert_noop!(fully_unbond(Origin::signed(11), 11), Error::::DelegatorNotFound); + assert_noop!( + Pools::fully_unbond(Origin::signed(11), 11), + Error::::DelegatorNotFound + ); // Add the delegator let delegator = Delegator { pool_id: 2, points: 10, ..Default::default() }; Delegators::::insert(11, delegator); - let _ = fully_unbond(Origin::signed(11), 11); + let _ = Pools::fully_unbond(Origin::signed(11), 11); }); } @@ -1672,7 +1678,7 @@ mod unbond { } .put(); - let _ = fully_unbond(Origin::signed(11), 11); + let _ = Pools::fully_unbond(Origin::signed(11), 11); }); } @@ -1883,15 +1889,15 @@ mod withdraw_unbonded { .build_and_execute(|| { // Given assert_eq!(StakingMock::bonding_duration(), 3); - assert_ok!(fully_unbond(Origin::signed(550), 550)); - assert_ok!(fully_unbond(Origin::signed(40), 40)); + assert_ok!(Pools::fully_unbond(Origin::signed(550), 550)); + assert_ok!(Pools::fully_unbond(Origin::signed(40), 40)); assert_eq!(Balances::free_balance(&default_bonded_account()), 600); let mut current_era = 1; CurrentEra::set(current_era); // In a new era, unbond the depositor unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); let mut sub_pools = SubPoolsStorage::::get(1).unwrap(); let unbond_pool = sub_pools.with_era.get_mut(&(current_era + 3)).unwrap(); @@ -1975,10 +1981,10 @@ mod withdraw_unbonded { Balances::make_free_balance_be(&default_bonded_account(), 100); assert_eq!(StakingMock::total_stake(&default_bonded_account()), Some(100)); - assert_ok!(fully_unbond(Origin::signed(40), 40)); - assert_ok!(fully_unbond(Origin::signed(550), 550)); + assert_ok!(Pools::fully_unbond(Origin::signed(40), 40)); + assert_ok!(Pools::fully_unbond(Origin::signed(550), 550)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); SubPoolsStorage::::insert( 1, @@ -2036,7 +2042,7 @@ mod withdraw_unbonded { assert_eq!(Balances::free_balance(&10), 5); assert_eq!(Balances::free_balance(&default_bonded_account()), 10); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); // Simulate a slash that is not accounted for in the sub pools. Balances::make_free_balance_be(&default_bonded_account(), 5); @@ -2098,8 +2104,8 @@ mod withdraw_unbonded { .add_delegators(vec![(100, 100), (200, 200)]) .build_and_execute(|| { // Given - assert_ok!(fully_unbond(Origin::signed(100), 100)); - assert_ok!(fully_unbond(Origin::signed(200), 200)); + assert_ok!(Pools::fully_unbond(Origin::signed(100), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(200), 200)); assert_eq!( BondedPool::::get(1).unwrap(), BondedPool { @@ -2147,7 +2153,7 @@ mod withdraw_unbonded { fn withdraw_unbonded_destroying_permissionless() { ExtBuilder::default().add_delegators(vec![(100, 100)]).build_and_execute(|| { // Given - assert_ok!(fully_unbond(Origin::signed(100), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(100), 100)); assert_eq!( BondedPool::::get(1).unwrap(), BondedPool { @@ -2165,7 +2171,7 @@ mod withdraw_unbonded { // Cannot permissionlessly withdraw assert_noop!( - fully_unbond(Origin::signed(420), 100), + Pools::fully_unbond(Origin::signed(420), 100), Error::::NotKickerOrDestroying ); @@ -2187,14 +2193,14 @@ mod withdraw_unbonded { .add_delegators(vec![(100, 100), (200, 200)]) .build_and_execute(|| { // Given - assert_ok!(fully_unbond(Origin::signed(100), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(100), 100)); let mut current_era = 1; CurrentEra::set(current_era); - assert_ok!(fully_unbond(Origin::signed(200), 200)); + assert_ok!(Pools::fully_unbond(Origin::signed(200), 200)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -2263,9 +2269,9 @@ mod withdraw_unbonded { fn withdraw_unbonded_depositor_no_era_pool() { ExtBuilder::default().add_delegators(vec![(100, 100)]).build_and_execute(|| { // Given - assert_ok!(fully_unbond(Origin::signed(100), 100)); + assert_ok!(Pools::fully_unbond(Origin::signed(100), 100)); unsafe_set_state(1, PoolState::Destroying).unwrap(); - assert_ok!(fully_unbond(Origin::signed(10), 10)); + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); // Skip ahead to an era where the `with_era` pools can get merged into the `no_era` // pool. let current_era = TotalUnbondingPools::::get(); From 8985a84a70d8669aa36e07f4ab09d7a22d94181b Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 19 Apr 2022 12:31:15 +0200 Subject: [PATCH 30/33] add TODO about killing the reward account --- frame/nomination-pools/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 053fecea614e7..37ea7aef12033 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1873,6 +1873,7 @@ impl Pallet { ExistenceRequirement::AllowDeath, ); + // TODO: this is purely defensive. T::Currency::make_free_balance_be(&reward_account, Zero::zero()); T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero()); From 20846ce0f082aa5fd64b92ce82db6ff10fb39a42 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Tue, 19 Apr 2022 12:31:30 +0200 Subject: [PATCH 31/33] Update frame/nomination-pools/src/lib.rs Co-authored-by: Zeke Mostov --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 37ea7aef12033..57ad949d4b453 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -1221,7 +1221,7 @@ pub mod pallet { FullyUnbonding, /// The delegator cannot unbond further chunks due to reaching the limit. MaxUnbondingLimit, - /// None of the funds cannot be withdrawn yet because the bonding duration has not passed. + /// None of the funds can be withdrawn yet because the bonding duration has not passed. CannotWithdrawAny, /// The amount does not meet the minimum bond to either join or create a pool. MinimumBondNotMet, From 1e4f7fa718fe5ef087b12076effe5271bf2dd197 Mon Sep 17 00:00:00 2001 From: Zeke Mostov Date: Tue, 19 Apr 2022 11:58:57 -0700 Subject: [PATCH 32/33] Update frame/nomination-pools/src/lib.rs --- frame/nomination-pools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index 57ad949d4b453..b5e605e26f555 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -397,7 +397,7 @@ pub struct Delegator { /// This value lines up with the [`RewardPool::total_earnings`] after a delegator claims a /// payout. pub reward_pool_total_earnings: BalanceOf, - /// The eras as which this delegator is unbonding, mapped from era index to the number of + /// The eras in which this delegator is unbonding, mapped from era index to the number of /// points scheduled to unbond in the given era. pub unbonding_eras: BoundedBTreeMap, T::MaxUnbonding>, } From 8657efc5045b4df1b49dac28c456bedaa5872373 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Wed, 20 Apr 2022 10:17:28 +0200 Subject: [PATCH 33/33] last self-review --- frame/nomination-pools/src/lib.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/frame/nomination-pools/src/lib.rs b/frame/nomination-pools/src/lib.rs index e7f205f724a47..47655454c5fde 100644 --- a/frame/nomination-pools/src/lib.rs +++ b/frame/nomination-pools/src/lib.rs @@ -408,6 +408,10 @@ impl Delegator { self.active_points().saturating_add(self.unbonding_points()) } + /// Active balance of the delegator. + /// + /// This is derived from the ratio of points in the pool to which the delegator belongs to. + /// Might return different values based on the pool state for the same delegator and points. pub(crate) fn active_balance(&self) -> BalanceOf { if let Some(pool) = BondedPool::::get(self.pool_id).defensive() { pool.points_to_balance(self.points) @@ -683,10 +687,7 @@ impl BondedPool { matches!(self.state, PoolState::Destroying) } - fn is_destroying_and_only_bonded_delegator( - &self, - alleged_depositor_points: BalanceOf, - ) -> bool { + fn is_destroying_and_only_depositor(&self, alleged_depositor_points: BalanceOf) -> bool { // NOTE: if we add `&& self.delegator_counter == 1`, then this becomes even more strict and // ensures that there are no unbonding delegators hanging around either. self.is_destroying() && self.points == alleged_depositor_points @@ -750,7 +751,7 @@ impl BondedPool { // Any delegator who is not the depositor can always unbond themselves (true, false) => (), (_, true) => { - if self.is_destroying_and_only_bonded_delegator(target_delegator.active_points()) { + if self.is_destroying_and_only_depositor(target_delegator.active_points()) { // if the pool is about to be destroyed, anyone can unbond the depositor, and // they can fully unbond. } else { @@ -1557,7 +1558,7 @@ pub mod pallet { ensure!(!withdrawn_points.is_empty(), Error::::CannotWithdrawAny); // Before calculate the `balance_to_unbond`, with call withdraw unbonded to ensure the - // `non_locked_balance` is correct. + // `transferrable_balance` is correct. T::StakingInterface::withdraw_unbonded( bonded_pool.bonded_account(), num_slashing_spans, @@ -2151,7 +2152,7 @@ impl Pallet { let depositor = Delegators::::get(&bonded_pool.roles.depositor).unwrap(); assert!( - bonded_pool.is_destroying_and_only_bonded_delegator(depositor.active_points()) || + bonded_pool.is_destroying_and_only_depositor(depositor.active_points()) || depositor.active_points() >= MinCreateBond::::get(), "depositor must always have MinCreateBond stake in the pool, except for when the \ pool is being destroyed and the depositor is the last member",