diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 53e1c48a5e..514267e8bf 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -469,7 +469,7 @@ impl PoolMember { self.points = new_points; Ok(()) } else { - Err(Error::::NotEnoughPointsToUnbond) + Err(Error::::MinimumBondNotMet) } } @@ -781,45 +781,60 @@ impl BondedPool { let is_depositor = *target_account == self.roles.depositor; let is_full_unbond = unbonding_points == target_member.active_points(); + let balance_after_unbond = { + let new_depositor_points = + target_member.active_points().saturating_sub(unbonding_points); + let mut target_member_after_unbond = (*target_member).clone(); + target_member_after_unbond.points = new_depositor_points; + target_member_after_unbond.active_balance() + }; + // any partial unbonding is only ever allowed if this unbond is permissioned. ensure!( is_permissioned || is_full_unbond, Error::::PartialUnbondNotAllowedPermissionlessly ); + // any unbond must comply with the balance condition: + ensure!( + is_full_unbond || + balance_after_unbond >= + if is_depositor { + Pallet::::depositor_min_bond() + } else { + MinJoinBond::::get() + }, + Error::::MinimumBondNotMet + ); + + // additional checks: match (is_permissioned, is_depositor) { - // If the pool is blocked, then an admin with kicking permissions can remove a - // member. If the pool is being destroyed, anyone can remove a member + (true, false) => (), + (true, true) => { + // permission depositor unbond: if destroying and pool is empty, always allowed, + // with no additional limits. + if self.is_destroying_and_only_depositor(target_member.active_points()) { + // everything good, let them unbond anything. + } else { + // depositor cannot fully unbond yet. + ensure!(!is_full_unbond, Error::::MinimumBondNotMet); + } + }, (false, false) => { + // If the pool is blocked, then an admin with kicking permissions can remove a + // member. If the pool is being destroyed, anyone can remove a member + debug_assert!(is_full_unbond); ensure!( self.can_kick(caller) || self.is_destroying(), Error::::NotKickerOrDestroying ) }, - // Any member who is not the depositor can always unbond themselves - (true, false) => (), - (_, true) => { - if self.is_destroying_and_only_depositor(target_member.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 balance_after_unbond = { - let new_depositor_points = - target_member.active_points().saturating_sub(unbonding_points); - let mut depositor_after_unbond = (*target_member).clone(); - depositor_after_unbond.points = new_depositor_points; - depositor_after_unbond.active_balance() - }; - ensure!( - balance_after_unbond >= MinCreateBond::::get(), - Error::::NotOnlyPoolMember - ); - } + (false, true) => { + // the depositor can simply not be unbonded permissionlessly, period. + return Err(Error::::DoesNotHavePermission.into()) }, }; + Ok(()) } @@ -830,25 +845,14 @@ impl BondedPool { &self, caller: &T::AccountId, target_account: &T::AccountId, - target_member: &PoolMember, - sub_pools: &SubPools, ) -> Result<(), DispatchError> { - if *target_account == self.roles.depositor { - ensure!( - sub_pools.sum_unbonding_points() == target_member.unbonding_points(), - Error::::NotOnlyPoolMember - ); - debug_assert_eq!(self.member_counter, 1, "only member must exist at this point"); - Ok(()) - } else { - // This isn't a depositor - let is_permissioned = caller == target_account; - ensure!( - is_permissioned || self.can_kick(caller) || self.is_destroying(), - Error::::NotKickerOrDestroying - ); - Ok(()) - } + // This isn't a depositor + let is_permissioned = caller == target_account; + ensure!( + is_permissioned || self.can_kick(caller) || self.is_destroying(), + Error::::NotKickerOrDestroying + ); + Ok(()) } /// Bond exactly `amount` from `who`'s funds into this pool. @@ -1100,15 +1104,6 @@ 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 { @@ -1419,15 +1414,16 @@ pub mod pallet { /// 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. + /// + /// The depositor can never unbond to a value less than + /// `Pallet::depositor_min_bond`. The caller does not have nominating + /// permissions for the pool. Members can never unbond to a value below `MinJoinBond`. MinimumBondNotMet, /// The transaction could not be executed due to overflow risk for the pool. OverflowRisk, /// A pool must be in [`PoolState::Destroying`] in order for the depositor to unbond or for /// other members to be permissionlessly unbonded. NotDestroying, - /// The depositor must be the only member in the bonded pool in order to unbond. And the - /// depositor must be the only member in the sub pools in order to withdraw unbonded. - NotOnlyPoolMember, /// The caller does not have nominating permissions for the pool. NotNominator, /// Either a) the caller cannot make a valid kick or b) the pool is not destroying. @@ -1447,8 +1443,6 @@ pub mod pallet { /// Some error occurred that should never happen. This should be reported to the /// maintainers. Defensive(DefensiveError), - /// Not enough points. Ty unbonding less. - NotEnoughPointsToUnbond, /// Partial unbonding now allowed permissionlessly. PartialUnbondNotAllowedPermissionlessly, } @@ -1758,12 +1752,7 @@ pub mod pallet { let mut sub_pools = SubPoolsStorage::::get(member.pool_id) .defensive_ok_or::>(DefensiveError::SubPoolsNotFound.into())?; - bonded_pool.ok_to_withdraw_unbonded_with( - &caller, - &member_account, - &member, - &sub_pools, - )?; + bonded_pool.ok_to_withdraw_unbonded_with(&caller, &member_account)?; // NOTE: must do this after we have done the `ok_to_withdraw_unbonded_other_with` check. let withdrawn_points = member.withdraw_unlocked(current_era); @@ -1878,13 +1867,7 @@ pub mod pallet { ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!( - amount >= - T::StakingInterface::minimum_bond() - .max(MinCreateBond::::get()) - .max(MinJoinBond::::get()), - Error::::MinimumBondNotMet - ); + ensure!(amount >= Pallet::::depositor_min_bond(), Error::::MinimumBondNotMet); ensure!( MaxPools::::get() .map_or(true, |max_pools| BondedPools::::count() < max_pools), @@ -2162,6 +2145,18 @@ pub mod pallet { } impl Pallet { + /// The amount of bond that MUST REMAIN IN BONDED in ALL POOLS. + /// + /// It is the responsibility of the depositor to put these funds into the pool initially. Upon + /// unbond, they can never unbond to a value below this amount. + /// + /// It is essentially `max { MinNominatorBond, MinCreateBond, MinJoinBond }`, where the former + /// is coming from the staking pallet and the latter two are configured in this pallet. + fn depositor_min_bond() -> BalanceOf { + T::StakingInterface::minimum_bond() + .max(MinCreateBond::::get()) + .max(MinJoinBond::::get()) + } /// Remove everything related to the given bonded pool. /// /// All sub-pools are also deleted. All accounts are dusted and the leftover of the reward diff --git a/substrate/frame/nomination-pools/src/mock.rs b/substrate/frame/nomination-pools/src/mock.rs index 042a0b666e..5138c55afc 100644 --- a/substrate/frame/nomination-pools/src/mock.rs +++ b/substrate/frame/nomination-pools/src/mock.rs @@ -22,6 +22,7 @@ pub fn default_reward_account() -> AccountId { } parameter_types! { + pub static MinJoinBondConfig: Balance = 2; pub static CurrentEra: EraIndex = 0; pub static BondingDuration: EraIndex = 3; pub storage BondedBalanceMap: BTreeMap = Default::default(); @@ -245,6 +246,11 @@ impl ExtBuilder { self } + pub(crate) fn min_join_bond(self, min: Balance) -> Self { + MinJoinBondConfig::set(min); + self + } + pub(crate) fn with_check(self, level: u8) -> Self { CheckLevel::set(level); self @@ -261,11 +267,12 @@ impl ExtBuilder { } pub(crate) fn build(self) -> sp_io::TestExternalities { + sp_tracing::try_init_simple(); let mut storage = frame_system::GenesisConfig::default().build_storage::().unwrap(); let _ = crate::GenesisConfig:: { - min_join_bond: 2, + min_join_bond: MinJoinBondConfig::get(), min_create_bond: 2, max_pools: Some(2), max_members_per_pool: self.max_members_per_pool, @@ -280,8 +287,8 @@ impl ExtBuilder { frame_system::Pallet::::set_block_number(1); // make a pool - let amount_to_bond = ::StakingInterface::minimum_bond(); - Balances::make_free_balance_be(&10, amount_to_bond * 2); + let amount_to_bond = Pools::depositor_min_bond(); + Balances::make_free_balance_be(&10, amount_to_bond * 5); assert_ok!(Pools::create(RawOrigin::Signed(10).into(), amount_to_bond, 900, 901, 902)); let last_pool = LastPoolId::::get(); @@ -302,12 +309,13 @@ impl ExtBuilder { } } -pub(crate) fn unsafe_set_state(pool_id: PoolId, state: PoolState) -> Result<(), ()> { +pub(crate) fn unsafe_set_state(pool_id: PoolId, state: PoolState) { BondedPools::::try_mutate(pool_id, |maybe_bonded_pool| { maybe_bonded_pool.as_mut().ok_or(()).map(|bonded_pool| { bonded_pool.state = state; }) }) + .unwrap() } parameter_types! { diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 9989893d86..c971239bef 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -538,13 +538,13 @@ mod join { StakingMock::set_bonded_balance(Pools::create_bonded_account(1), max_points_to_balance); // Cannot join a pool that isn't open - unsafe_set_state(123, PoolState::Blocked).unwrap(); + unsafe_set_state(123, PoolState::Blocked); assert_noop!( Pools::join(Origin::signed(11), max_points_to_balance, 123), Error::::NotOpen ); - unsafe_set_state(123, PoolState::Destroying).unwrap(); + unsafe_set_state(123, PoolState::Destroying); assert_noop!( Pools::join(Origin::signed(11), max_points_to_balance, 123), Error::::NotOpen @@ -1824,7 +1824,8 @@ mod claim_payout { fn rewards_are_rounded_down_depositor_collects_them() { ExtBuilder::default().add_members(vec![(20, 20)]).build_and_execute(|| { // initial balance of 10. - assert_eq!(Balances::free_balance(&10), 5); + + assert_eq!(Balances::free_balance(&10), 35); assert_eq!( Balances::free_balance(&default_reward_account()), Balances::minimum_balance() @@ -1875,7 +1876,7 @@ mod claim_payout { ); // original ed + ed put into reward account + reward + bond + dust. - assert_eq!(Balances::free_balance(&10), 5 + 5 + 13 + 10 + 1); + assert_eq!(Balances::free_balance(&10), 35 + 5 + 13 + 10 + 1); }) } @@ -1942,10 +1943,269 @@ mod claim_payout { mod unbond { use super::*; + #[test] + fn member_unbond_open() { + // depositor in pool, pool state open + // - member unbond above limit + // - member unbonds to 0 + // - member cannot unbond between within limit and 0 + ExtBuilder::default() + .min_join_bond(10) + .add_members(vec![(20, 20)]) + .build_and_execute(|| { + // can unbond to above limit + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 15); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 5); + + // cannot go to below 10: + assert_noop!( + Pools::unbond(Origin::signed(20), 20, 10), + Error::::MinimumBondNotMet + ); + + // but can go to 0 + assert_ok!(Pools::unbond(Origin::signed(20), 20, 15)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 0); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 20); + }) + } + + #[test] + fn member_kicked() { + // depositor in pool, pool state blocked + // - member cannot be kicked to above limit + // - member cannot be kicked between within limit and 0 + // - member kicked to 0 + ExtBuilder::default() + .min_join_bond(10) + .add_members(vec![(20, 20)]) + .build_and_execute(|| { + unsafe_set_state(1, PoolState::Blocked); + let kicker = DEFAULT_ROLES.state_toggler.unwrap(); + + // cannot be kicked to above the limit. + assert_noop!( + Pools::unbond(Origin::signed(kicker), 20, 5), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // cannot go to below 10: + assert_noop!( + Pools::unbond(Origin::signed(kicker), 20, 15), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // but they themselves can do an unbond + assert_ok!(Pools::unbond(Origin::signed(20), 20, 2)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 18); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 2); + + // can be kicked to 0. + assert_ok!(Pools::unbond(Origin::signed(kicker), 20, 18)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 0); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 20); + }) + } + + #[test] + fn member_unbond_destroying() { + // depositor in pool, pool state destroying + // - member cannot be permissionlessly unbonded to above limit + // - member cannot be permissionlessly unbonded between within limit and 0 + // - member permissionlessly unbonded to 0 + ExtBuilder::default() + .min_join_bond(10) + .add_members(vec![(20, 20)]) + .build_and_execute(|| { + unsafe_set_state(1, PoolState::Destroying); + let random = 123; + + // cannot be kicked to above the limit. + assert_noop!( + Pools::unbond(Origin::signed(random), 20, 5), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // cannot go to below 10: + assert_noop!( + Pools::unbond(Origin::signed(random), 20, 15), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // but they themselves can do an unbond + assert_ok!(Pools::unbond(Origin::signed(20), 20, 2)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 18); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 2); + + // but can go to 0 + assert_ok!(Pools::unbond(Origin::signed(random), 20, 18)); + assert_eq!(PoolMembers::::get(20).unwrap().active_points(), 0); + assert_eq!(PoolMembers::::get(20).unwrap().unbonding_points(), 20); + }) + } + + #[test] + fn depositor_unbond_open() { + // depositor in pool, pool state open + // - depositor unbonds to above limit + // - depositor cannot unbond to below limit or 0 + ExtBuilder::default().min_join_bond(10).build_and_execute(|| { + // give the depositor some extra funds. + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + assert_eq!(PoolMembers::::get(10).unwrap().points, 20); + + // can unbond to above the limit. + assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 15); + assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 5); + + // cannot go to below 10: + assert_noop!(Pools::unbond(Origin::signed(10), 10, 10), Error::::MinimumBondNotMet); + + // cannot go to 0 either. + assert_noop!(Pools::unbond(Origin::signed(10), 10, 15), Error::::MinimumBondNotMet); + }) + } + + #[test] + fn depositor_kick() { + // depositor in pool, pool state blocked + // - depositor can never be kicked. + ExtBuilder::default().min_join_bond(10).build_and_execute(|| { + // give the depositor some extra funds. + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + assert_eq!(PoolMembers::::get(10).unwrap().points, 20); + + // set the stage + unsafe_set_state(1, PoolState::Blocked); + let kicker = DEFAULT_ROLES.state_toggler.unwrap(); + + // cannot be kicked to above limit. + assert_noop!( + Pools::unbond(Origin::signed(kicker), 10, 5), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // or below the limit + assert_noop!( + Pools::unbond(Origin::signed(kicker), 10, 15), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // or 0. + assert_noop!( + Pools::unbond(Origin::signed(kicker), 10, 20), + Error::::DoesNotHavePermission + ); + + // they themselves cannot do it either + assert_noop!(Pools::unbond(Origin::signed(10), 10, 20), Error::::MinimumBondNotMet); + }) + } + + #[test] + fn depositor_unbond_destroying_permissionless() { + // depositor can never be permissionlessly unbonded. + ExtBuilder::default().min_join_bond(10).build_and_execute(|| { + // give the depositor some extra funds. + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + assert_eq!(PoolMembers::::get(10).unwrap().points, 20); + + // set the stage + unsafe_set_state(1, PoolState::Destroying); + let random = 123; + + // cannot be kicked to above limit. + assert_noop!( + Pools::unbond(Origin::signed(random), 10, 5), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // or below the limit + assert_noop!( + Pools::unbond(Origin::signed(random), 10, 15), + Error::::PartialUnbondNotAllowedPermissionlessly + ); + + // or 0. + assert_noop!( + Pools::unbond(Origin::signed(random), 10, 20), + Error::::DoesNotHavePermission + ); + + // they themselves can do it in this case though. + assert_ok!(Pools::unbond(Origin::signed(10), 10, 20)); + }) + } + + #[test] + fn depositor_unbond_destroying_not_last_member() { + // deposit in pool, pool state destroying + // - depositor can never leave if there is another member in the pool. + ExtBuilder::default() + .min_join_bond(10) + .add_members(vec![(20, 20)]) + .build_and_execute(|| { + // give the depositor some extra funds. + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + assert_eq!(PoolMembers::::get(10).unwrap().points, 20); + + // set the stage + unsafe_set_state(1, PoolState::Destroying); + + // can go above the limit + assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 15); + assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 5); + + // but not below the limit + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 10), + Error::::MinimumBondNotMet + ); + + // and certainly not zero + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 15), + Error::::MinimumBondNotMet + ); + }) + } + + #[test] + fn depositor_unbond_destroying_last_member() { + // deposit in pool, pool state destroying + // - depositor can unbond to above limit always. + // - depositor cannot unbond to below limit if last. + // - depositor can unbond to 0 if last and destroying. + ExtBuilder::default().min_join_bond(10).build_and_execute(|| { + // give the depositor some extra funds. + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + assert_eq!(PoolMembers::::get(10).unwrap().points, 20); + + // set the stage + unsafe_set_state(1, PoolState::Destroying); + + // can unbond to above the limit. + assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 15); + assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 5); + + // still cannot go to below limit + assert_noop!(Pools::unbond(Origin::signed(10), 10, 10), Error::::MinimumBondNotMet); + + // can go to 0 too. + assert_ok!(Pools::unbond(Origin::signed(10), 10, 15)); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 0); + assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 20); + }) + } + #[test] fn unbond_of_1_works() { ExtBuilder::default().build_and_execute(|| { - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); assert_ok!(fully_unbond_permissioned(10)); assert_eq!( @@ -2021,7 +2281,7 @@ mod unbond { assert_eq!(Balances::free_balance(&40), 40 + 40); // We claim rewards when unbonding // When - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); assert_ok!(fully_unbond_permissioned(550)); // Then @@ -2111,7 +2371,7 @@ mod unbond { }, }, ); - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); // When let current_era = 1 + TotalUnbondingPools::::get(); @@ -2148,7 +2408,7 @@ mod unbond { .add_members(vec![(100, 100), (200, 200)]) .build_and_execute(|| { // Given - unsafe_set_state(1, PoolState::Blocked).unwrap(); + unsafe_set_state(1, PoolState::Blocked); let bonded_pool = BondedPool::::get(1).unwrap(); assert_eq!(bonded_pool.roles.root.unwrap(), 900); assert_eq!(bonded_pool.roles.nominator.unwrap(), 901); @@ -2216,7 +2476,7 @@ mod unbond { // Scenarios where non-admin accounts can unbond others ExtBuilder::default().add_members(vec![(100, 100)]).build_and_execute(|| { // Given the pool is blocked - unsafe_set_state(1, PoolState::Blocked).unwrap(); + unsafe_set_state(1, PoolState::Blocked); // A permissionless unbond attempt errors assert_noop!( @@ -2231,16 +2491,17 @@ mod unbond { ); // Given the pool is destroying - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); // The depositor cannot be fully unbonded until they are the last member assert_noop!( Pools::fully_unbond(Origin::signed(10), 10), - Error::::NotOnlyPoolMember + Error::::MinimumBondNotMet, ); // Any account can unbond a member that is not the depositor assert_ok!(Pools::fully_unbond(Origin::signed(420), 100)); + assert_eq!( pool_events_since_last_call(), vec![ @@ -2258,7 +2519,7 @@ mod unbond { ); // Given the pool is blocked - unsafe_set_state(1, PoolState::Blocked).unwrap(); + unsafe_set_state(1, PoolState::Blocked); // The depositor cannot be unbonded assert_noop!( @@ -2267,7 +2528,7 @@ mod unbond { ); // Given the pools is destroying - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); // The depositor cannot be unbonded yet. assert_noop!( @@ -2285,8 +2546,13 @@ mod unbond { Error::::PartialUnbondNotAllowedPermissionlessly, ); - // but full unbond works. - assert_ok!(Pools::fully_unbond(Origin::signed(420), 10)); + // depositor can never be unbonded permissionlessly . + assert_noop!( + Pools::fully_unbond(Origin::signed(420), 10), + Error::::DoesNotHavePermission + ); + // but depositor itself can do it. + assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); assert_eq!(BondedPools::::get(1).unwrap().points, 0); assert_eq!( @@ -2346,6 +2612,12 @@ mod unbond { #[test] fn partial_unbond_era_tracking() { ExtBuilder::default().build_and_execute(|| { + // to make the depositor capable of withdrawing. + StakingMinBond::set(1); + MinCreateBond::::set(1); + MinJoinBond::::set(1); + assert_eq!(Pools::depositor_min_bond(), 1); + // given assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 10); assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 0); @@ -2360,7 +2632,7 @@ mod unbond { assert_eq!(BondingDuration::get(), 3); // so the depositor can leave, just keeps the test simpler. - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); // when: casual unbond assert_ok!(Pools::unbond(Origin::signed(10), 10, 1)); @@ -2444,13 +2716,13 @@ mod unbond { ); // when: unbonding more than our active: error - assert_err!( + assert_noop!( frame_support::storage::in_storage_layer(|| Pools::unbond( Origin::signed(10), 10, 5 )), - Error::::NotEnoughPointsToUnbond + Error::::MinimumBondNotMet ); // instead: assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); @@ -2482,26 +2754,24 @@ mod unbond { #[test] 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(); + ExtBuilder::default().add_members(vec![(20, 20)]).ed(1).build_and_execute(|| { MaxUnbonding::set(2); // given - assert_ok!(Pools::unbond(Origin::signed(10), 10, 2)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 2)); CurrentEra::set(1); - assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 3)); assert_eq!( - PoolMembers::::get(10).unwrap().unbonding_eras, + PoolMembers::::get(20).unwrap().unbonding_eras, member_unbonding_eras!(3 => 2, 4 => 3) ); // when CurrentEra::set(2); - assert_err!( + assert_noop!( frame_support::storage::in_storage_layer(|| Pools::unbond( - Origin::signed(10), - 10, + Origin::signed(20), + 20, 4 )), Error::::MaxUnbondingLimit @@ -2509,30 +2779,35 @@ mod unbond { // when MaxUnbonding::set(3); - assert_ok!(Pools::unbond(Origin::signed(10), 10, 1)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 1)); + assert_eq!( - PoolMembers::::get(10).unwrap().unbonding_eras, + PoolMembers::::get(20).unwrap().unbonding_eras, member_unbonding_eras!(3 => 2, 4 => 3, 5 => 1) ); + assert_eq!( pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - Event::Unbonded { member: 10, pool_id: 1, points: 2, balance: 2 }, - Event::Unbonded { member: 10, pool_id: 1, points: 3, balance: 3 }, - Event::Unbonded { member: 10, pool_id: 1, points: 1, balance: 1 } + Event::Bonded { member: 20, pool_id: 1, bonded: 20, joined: true }, + Event::Unbonded { member: 20, pool_id: 1, balance: 2, points: 2 }, + Event::Unbonded { member: 20, pool_id: 1, balance: 3, points: 3 }, + Event::Unbonded { member: 20, pool_id: 1, balance: 1, points: 1 } ] ); }) } - // depositor can unbond inly up to `MinCreateBond`. + // depositor can unbond only up to `MinCreateBond`. #[test] fn depositor_permissioned_partial_unbond() { ExtBuilder::default().ed(1).build_and_execute(|| { // given - assert_eq!(MinCreateBond::::get(), 2); + StakingMinBond::set(5); + assert_eq!(Pools::depositor_min_bond(), 5); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 10); assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 0); @@ -2544,7 +2819,7 @@ mod unbond { // but not less than 2 assert_noop!( Pools::unbond(Origin::signed(10), 10, 6), - Error::::NotOnlyPoolMember + Error::::MinimumBondNotMet ); assert_eq!( @@ -2558,7 +2833,6 @@ mod unbond { }); } - // same as above, but the pool is slashed and therefore the depositor cannot partially unbond. #[test] fn depositor_permissioned_partial_unbond_slashed() { ExtBuilder::default().ed(1).build_and_execute(|| { @@ -2573,78 +2847,69 @@ mod unbond { // cannot unbond even 7, because the value of shares is now less. assert_noop!( Pools::unbond(Origin::signed(10), 10, 7), - Error::::NotOnlyPoolMember - ); - assert_eq!( - pool_events_since_last_call(), - vec![ - Event::Created { depositor: 10, pool_id: 1 }, - Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - ] + Error::::MinimumBondNotMet ); }); } #[test] fn every_unbonding_triggers_payout() { - ExtBuilder::default().build_and_execute(|| { - let initial_reward_account = Balances::free_balance(Pools::create_reward_account(1)); + ExtBuilder::default().add_members(vec![(20, 20)]).build_and_execute(|| { + let initial_reward_account = Balances::free_balance(default_reward_account()); assert_eq!(initial_reward_account, Balances::minimum_balance()); assert_eq!(initial_reward_account, 5); - // set the pool to destroying so that depositor can leave. - unsafe_set_state(1, PoolState::Destroying).unwrap(); - Balances::make_free_balance_be( - &Pools::create_reward_account(1), - 2 * Balances::minimum_balance(), + &default_reward_account(), + 4 * Balances::minimum_balance(), ); - assert_ok!(Pools::unbond(Origin::signed(10), 10, 2)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 2)); assert_eq!( pool_events_since_last_call(), vec![ + // 2/3 of ed, which is 20's share. Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - // exactly equal to ed, all that can be claimed. - Event::PaidOut { member: 10, pool_id: 1, payout: 5 }, - Event::Unbonded { member: 10, pool_id: 1, points: 2, balance: 2 } + Event::Bonded { member: 20, pool_id: 1, bonded: 20, joined: true }, + Event::PaidOut { member: 20, pool_id: 1, payout: 10 }, + Event::Unbonded { member: 20, pool_id: 1, balance: 2, points: 2 } ] ); CurrentEra::set(1); Balances::make_free_balance_be( - &Pools::create_reward_account(1), - 2 * Balances::minimum_balance(), + &default_reward_account(), + 4 * Balances::minimum_balance(), ); - assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 3)); assert_eq!( pool_events_since_last_call(), vec![ - // exactly equal to ed, all that can be claimed. - Event::PaidOut { member: 10, pool_id: 1, payout: 5 }, - Event::Unbonded { member: 10, pool_id: 1, points: 3, balance: 3 } + // 2/3 of ed, which is 20's share. + Event::PaidOut { member: 20, pool_id: 1, payout: 6 }, + Event::Unbonded { member: 20, pool_id: 1, points: 3, balance: 3 } ] ); CurrentEra::set(2); Balances::make_free_balance_be( - &Pools::create_reward_account(1), - 2 * Balances::minimum_balance(), + &default_reward_account(), + 4 * Balances::minimum_balance(), ); - assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); + assert_ok!(Pools::unbond(Origin::signed(20), 20, 5)); assert_eq!( pool_events_since_last_call(), vec![ - Event::PaidOut { member: 10, pool_id: 1, payout: 5 }, - Event::Unbonded { member: 10, pool_id: 1, points: 5, balance: 5 } + Event::PaidOut { member: 20, pool_id: 1, payout: 3 }, + Event::Unbonded { member: 20, pool_id: 1, points: 5, balance: 5 } ] ); assert_eq!( - PoolMembers::::get(10).unwrap().unbonding_eras, + PoolMembers::::get(20).unwrap().unbonding_eras, member_unbonding_eras!(3 => 2, 4 => 3, 5 => 5) ); }); @@ -2801,7 +3066,7 @@ mod withdraw_unbonded { ); // now, finally, the depositor can take out its share. - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); assert_ok!(fully_unbond_permissioned(10)); current_era += 3; @@ -2911,7 +3176,7 @@ mod withdraw_unbonded { assert!(SubPoolsStorage::::get(&1).unwrap().with_era.is_empty()); // now, finally, the depositor can take out its share. - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); assert_ok!(fully_unbond_permissioned(10)); // because everyone else has left, the points @@ -2926,7 +3191,7 @@ mod withdraw_unbonded { assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0)); // then - assert_eq!(Balances::free_balance(&10), 10 + 5); + assert_eq!(Balances::free_balance(&10), 10 + 35); assert_eq!(Balances::free_balance(&default_bonded_account()), 0); // in this test 10 also gets a fair share of the slash, because the slash was @@ -2955,9 +3220,9 @@ mod withdraw_unbonded { ExtBuilder::default().build_and_execute(|| { // Given assert_eq!(Balances::minimum_balance(), 5); - assert_eq!(Balances::free_balance(&10), 5); + assert_eq!(Balances::free_balance(&10), 35); assert_eq!(Balances::free_balance(&default_bonded_account()), 10); - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); assert_ok!(Pools::fully_unbond(Origin::signed(10), 10)); // Simulate a slash that is not accounted for in the sub pools. @@ -2974,7 +3239,7 @@ mod withdraw_unbonded { assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0)); // Then - assert_eq!(Balances::free_balance(10), 10 + 5); + assert_eq!(Balances::free_balance(10), 10 + 35); assert_eq!(Balances::free_balance(&default_bonded_account()), 0); }); } @@ -3054,7 +3319,7 @@ mod withdraw_unbonded { ); // Given - unsafe_set_state(1, PoolState::Blocked).unwrap(); + unsafe_set_state(1, PoolState::Blocked); // Cannot kick as a nominator assert_noop!( @@ -3112,7 +3377,7 @@ mod withdraw_unbonded { ); // Given - unsafe_set_state(1, PoolState::Destroying).unwrap(); + unsafe_set_state(1, PoolState::Destroying); // Can permissionlesly withdraw a member that is not the depositor assert_ok!(Pools::withdraw_unbonded(Origin::signed(420), 100, 0)); @@ -3137,8 +3402,8 @@ mod withdraw_unbonded { #[test] fn partial_withdraw_unbonded_depositor() { ExtBuilder::default().ed(1).build_and_execute(|| { - // so the depositor can leave, just keeps the test simpler. - unsafe_set_state(1, PoolState::Destroying).unwrap(); + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + unsafe_set_state(1, PoolState::Destroying); // given assert_ok!(Pools::unbond(Origin::signed(10), 10, 6)); @@ -3158,13 +3423,14 @@ mod withdraw_unbonded { } } ); - assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 3); + assert_eq!(PoolMembers::::get(10).unwrap().active_points(), 13); assert_eq!(PoolMembers::::get(10).unwrap().unbonding_points(), 7); assert_eq!( pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: false }, Event::Unbonded { member: 10, pool_id: 1, points: 6, balance: 6 }, Event::Unbonded { member: 10, pool_id: 1, points: 1, balance: 1 } ] @@ -3368,50 +3634,72 @@ mod withdraw_unbonded { #[test] fn full_multi_step_withdrawing_depositor() { ExtBuilder::default().ed(1).build_and_execute(|| { - // given + // depositor now has 20, they can unbond to 10. + assert_eq!(Pools::depositor_min_bond(), 10); + assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::FreeBalance(10))); + + // now they can. assert_ok!(Pools::unbond(Origin::signed(10), 10, 7)); // progress one era and unbond the leftover. CurrentEra::set(1); - unsafe_set_state(1, PoolState::Destroying).unwrap(); assert_ok!(Pools::unbond(Origin::signed(10), 10, 3)); + assert_eq!( PoolMembers::::get(10).unwrap().unbonding_eras, member_unbonding_eras!(3 => 7, 4 => 3) ); + // they can't unbond to a value below 10 other than 0.. assert_noop!( - Pools::withdraw_unbonded(Origin::signed(10), 10, 0), - Error::::CannotWithdrawAny + Pools::unbond(Origin::signed(10), 10, 5), + Error::::MinimumBondNotMet ); + // but not even full, because they pool is not yet destroying. + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 10), + Error::::MinimumBondNotMet + ); + + // but now they can. + unsafe_set_state(1, PoolState::Destroying); + assert_noop!( + Pools::unbond(Origin::signed(10), 10, 5), + Error::::MinimumBondNotMet + ); + assert_ok!(Pools::unbond(Origin::signed(10), 10, 10)); + // now the 7 should be free. CurrentEra::set(3); assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0)); + assert_eq!( pool_events_since_last_call(), vec![ Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - Event::Unbonded { member: 10, pool_id: 1, points: 7, balance: 7 }, - Event::Unbonded { member: 10, pool_id: 1, points: 3, balance: 3 }, - Event::Withdrawn { member: 10, pool_id: 1, points: 7, balance: 7 } + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: false }, + Event::Unbonded { member: 10, pool_id: 1, balance: 7, points: 7 }, + Event::Unbonded { member: 10, pool_id: 1, balance: 3, points: 3 }, + Event::Unbonded { member: 10, pool_id: 1, balance: 10, points: 10 }, + Event::Withdrawn { member: 10, pool_id: 1, balance: 7, points: 7 } ] ); assert_eq!( PoolMembers::::get(10).unwrap().unbonding_eras, - member_unbonding_eras!(4 => 3) + member_unbonding_eras!(4 => 13) ); - // the 25 should be free now, and the member removed. + // the 13 should be free now, and the member removed. CurrentEra::set(4); assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0)); + assert_eq!( pool_events_since_last_call(), vec![ - Event::Withdrawn { member: 10, pool_id: 1, points: 3, balance: 3 }, + Event::Withdrawn { member: 10, pool_id: 1, points: 13, balance: 13 }, Event::MemberRemoved { pool_id: 1, member: 10 }, - // the pool is also destroyed now. Event::Destroyed { pool_id: 1 }, ] ); @@ -3640,7 +3928,7 @@ mod set_state { // If the pool is not ok to be open, then anyone can set it to destroying // Given - unsafe_set_state(1, PoolState::Open).unwrap(); + unsafe_set_state(1, PoolState::Open); let mut bonded_pool = BondedPool::::get(1).unwrap(); bonded_pool.points = 100; bonded_pool.put(); @@ -3651,7 +3939,7 @@ mod set_state { // Given Balances::make_free_balance_be(&default_bonded_account(), Balance::max_value() / 10); - unsafe_set_state(1, PoolState::Open).unwrap(); + unsafe_set_state(1, PoolState::Open); // When assert_ok!(Pools::set_state(Origin::signed(11), 1, PoolState::Destroying)); // Then @@ -3659,7 +3947,7 @@ mod set_state { // If the pool is not ok to be open, it cannot be permissionleslly set to a state that // isn't destroying - unsafe_set_state(1, PoolState::Open).unwrap(); + unsafe_set_state(1, PoolState::Open); assert_noop!( Pools::set_state(Origin::signed(11), 1, PoolState::Blocked), Error::::CanNotChangeState @@ -3820,13 +4108,13 @@ mod bond_extra { // given assert_eq!(PoolMembers::::get(10).unwrap().points, 10); assert_eq!(BondedPools::::get(1).unwrap().points, 10); - assert_eq!(Balances::free_balance(10), 5); + assert_eq!(Balances::free_balance(10), 35); // when assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::Rewards)); // then - assert_eq!(Balances::free_balance(10), 5); + assert_eq!(Balances::free_balance(10), 35); assert_eq!(PoolMembers::::get(10).unwrap().points, 10 + claimable_reward); assert_eq!(BondedPools::::get(1).unwrap().points, 10 + claimable_reward); @@ -3862,14 +4150,14 @@ mod bond_extra { assert_eq!(PoolMembers::::get(10).unwrap().points, 10); assert_eq!(PoolMembers::::get(20).unwrap().points, 20); assert_eq!(BondedPools::::get(1).unwrap().points, 30); - assert_eq!(Balances::free_balance(10), 5); + assert_eq!(Balances::free_balance(10), 35); assert_eq!(Balances::free_balance(20), 20); // when assert_ok!(Pools::bond_extra(Origin::signed(10), BondExtra::Rewards)); // then - assert_eq!(Balances::free_balance(10), 5); + assert_eq!(Balances::free_balance(10), 35); // 10's share of the reward is 1/3, since they gave 10/30 of the total shares. assert_eq!(PoolMembers::::get(10).unwrap().points, 10 + 1); assert_eq!(BondedPools::::get(1).unwrap().points, 30 + 1); diff --git a/substrate/frame/nomination-pools/test-staking/src/lib.rs b/substrate/frame/nomination-pools/test-staking/src/lib.rs index 3b9350f250..eed8e5fd39 100644 --- a/substrate/frame/nomination-pools/test-staking/src/lib.rs +++ b/substrate/frame/nomination-pools/test-staking/src/lib.rs @@ -72,7 +72,7 @@ fn pool_lifecycle_e2e() { // depositor cannot unbond yet. assert_noop!( Pools::unbond(Origin::signed(10), 10, 50), - PoolsError::::NotOnlyPoolMember, + PoolsError::::MinimumBondNotMet, ); // now the members want to unbond. @@ -103,7 +103,7 @@ fn pool_lifecycle_e2e() { // depositor cannot still unbond assert_noop!( Pools::unbond(Origin::signed(10), 10, 50), - PoolsError::::NotOnlyPoolMember, + PoolsError::::MinimumBondNotMet, ); for e in 1..BondingDuration::get() { @@ -120,7 +120,7 @@ fn pool_lifecycle_e2e() { // depositor cannot still unbond assert_noop!( Pools::unbond(Origin::signed(10), 10, 50), - PoolsError::::NotOnlyPoolMember, + PoolsError::::MinimumBondNotMet, ); // but members can now withdraw.