diff --git a/substrate/frame/nomination-pools/Cargo.toml b/substrate/frame/nomination-pools/Cargo.toml index 882a6f363a..8c2f9daf27 100644 --- a/substrate/frame/nomination-pools/Cargo.toml +++ b/substrate/frame/nomination-pools/Cargo.toml @@ -31,6 +31,7 @@ sp-tracing = { version = "5.0.0", path = "../../primitives/tracing" } [features] runtime-benchmarks = [] +try-runtime = [] default = ["std"] std = [ "codec/std", diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index bafed9fc2f..8b26bad6c7 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -403,7 +403,6 @@ pub struct PoolMember { } impl PoolMember { - #[cfg(any(test, debug_assertions))] fn total_points(&self) -> BalanceOf { self.active_points().saturating_add(self.unbonding_points()) } @@ -739,6 +738,14 @@ impl BondedPool { ) -> Result<(), DispatchError> { let is_permissioned = caller == target_account; let is_depositor = *target_account == self.roles.depositor; + let is_full_unbond = unbonding_points == target_member.active_points(); + + // any partial unbonding is only ever allowed if this unbond is permissioned. + ensure!( + is_permissioned || is_full_unbond, + Error::::PartialUnbondNotAllowedPermissionlessly + ); + 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 @@ -1198,6 +1205,10 @@ pub mod pallet { Destroyed { pool_id: PoolId }, /// The state of a pool has changed StateChanged { pool_id: PoolId, new_state: PoolState }, + /// A member has been removed from a pool. + /// + /// The removal can be voluntary (withdrawn all unbonded funds) or involuntary (kicked). + MemberRemoved { pool_id: PoolId, member: T::AccountId }, } #[pallet::error] @@ -1256,6 +1267,8 @@ pub mod pallet { DefensiveError, /// Not enough points. Ty unbonding less. NotEnoughPointsToUnbond, + /// Partial unbonding now allowed permissionlessly. + PartialUnbondNotAllowedPermissionlessly, } #[pallet::call] @@ -1595,9 +1608,13 @@ pub mod pallet { amount: balance_to_unbond, }); - let post_info_weight = if member.active_points().is_zero() { + let post_info_weight = if member.total_points().is_zero() { // member being reaped. PoolMembers::::remove(&member_account); + Self::deposit_event(Event::::MemberRemoved { + pool_id: member.pool_id, + member: member_account.clone(), + }); if member_account == bonded_pool.roles.depositor { Pallet::::dissolve_pool(bonded_pool); @@ -2044,6 +2061,10 @@ impl Pallet { let member_payout = Self::calculate_member_payout(member, bonded_pool, reward_pool)?; + if member_payout.is_zero() { + return Ok(member_payout) + } + // Transfer payout to the member. T::Currency::transfer( &bonded_pool.reward_account(), diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 7df9222808..cd66c3d774 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -1386,6 +1386,17 @@ mod unbond { } } ); + 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: 40, pool_id: 1, bonded: 40, joined: true }, + Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::PaidOut { member: 40, pool_id: 1, payout: 40 }, + Event::Unbonded { member: 40, pool_id: 1, amount: 6 } + ] + ); assert_eq!(StakingMock::active_stake(&default_bonded_account()).unwrap(), 94); assert_eq!( @@ -1421,6 +1432,13 @@ mod unbond { member_unbonding_eras!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::PaidOut { member: 550, pool_id: 1, payout: 550 }, + Event::Unbonded { member: 550, pool_id: 1, amount: 92 } + ] + ); // When assert_ok!(fully_unbond_permissioned(10)); @@ -1448,6 +1466,13 @@ mod unbond { member_unbonding_eras!(0 + 3 => 550) ); assert_eq!(Balances::free_balance(&550), 550 + 550); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::PaidOut { member: 10, pool_id: 1, payout: 10 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 2 } + ] + ); }); } @@ -1485,7 +1510,15 @@ mod unbond { current_era + 3 => UnbondPool { balance: 10, points: 10 }, }, }, - ) + ); + 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, amount: 10 } + ] + ); }); } @@ -1502,7 +1535,7 @@ mod unbond { assert_eq!(bonded_pool.roles.nominator, 901); assert_eq!(bonded_pool.roles.state_toggler, 902); - // When the nominator trys to kick, then its a noop + // When the nominator tries to kick, then its a noop assert_noop!( Pools::fully_unbond(Origin::signed(901), 100), Error::::NotKickerOrDestroying @@ -1511,9 +1544,25 @@ mod unbond { // When the root kicks then its ok assert_ok!(Pools::fully_unbond(Origin::signed(900), 100)); + 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: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Bonded { member: 200, pool_id: 1, bonded: 200, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 100 }, + ] + ); + // When the state toggler kicks then its ok assert_ok!(Pools::fully_unbond(Origin::signed(902), 200)); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 200, pool_id: 1, amount: 200 }] + ); + assert_eq!( BondedPool::::get(1).unwrap(), BondedPool { @@ -1557,6 +1606,12 @@ mod unbond { Error::::NotKickerOrDestroying ); + // permissionless unbond must be full + assert_noop!( + Pools::unbond(Origin::signed(420), 100, 80), + Error::::PartialUnbondNotAllowedPermissionlessly, + ); + // Given the pool is destroying unsafe_set_state(1, PoolState::Destroying).unwrap(); @@ -1568,6 +1623,21 @@ mod unbond { // 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![ + Event::Created { depositor: 10, pool_id: 1 }, + Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, + Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 100 } + ] + ); + + // still permissionless unbond must be full + assert_noop!( + Pools::unbond(Origin::signed(420), 100, 80), + Error::::PartialUnbondNotAllowedPermissionlessly, + ); // Given the pool is blocked unsafe_set_state(1, PoolState::Blocked).unwrap(); @@ -1584,6 +1654,17 @@ mod unbond { // The depositor can be unbonded by anyone. assert_ok!(Pools::fully_unbond(Origin::signed(420), 10)); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 10, pool_id: 1, amount: 10 }] + ); + + // still permissionless unbond must be full. + assert_noop!( + Pools::unbond(Origin::signed(420), 10, 5), + Error::::PartialUnbondNotAllowedPermissionlessly, + ); + assert_eq!(BondedPools::::get(1).unwrap().points, 0); assert_eq!( SubPoolsStorage::::get(1).unwrap(), @@ -1682,6 +1763,14 @@ mod unbond { } } ); + 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, amount: 1 } + ] + ); // when: casual further unbond, same era. assert_ok!(Pools::unbond(Origin::signed(10), 10, 5)); @@ -1703,6 +1792,10 @@ mod unbond { } } ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 10, pool_id: 1, amount: 5 }] + ); // when: casual further unbond, next era. CurrentEra::set(1); @@ -1726,6 +1819,10 @@ mod unbond { } } ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 10, pool_id: 1, amount: 1 }] + ); // when: unbonding more than our active: error assert_noop!( @@ -1753,6 +1850,10 @@ mod unbond { } } ); + assert_eq!( + pool_events_since_last_call(), + vec![Event::Unbonded { member: 10, pool_id: 1, amount: 3 }] + ); }); } @@ -1786,6 +1887,16 @@ mod unbond { PoolMembers::::get(10).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, amount: 2 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 3 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 1 } + ] + ); }) } @@ -1808,6 +1919,15 @@ mod unbond { Pools::unbond(Origin::signed(10), 10, 6), 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 }, + Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Unbonded { member: 10, pool_id: 1, amount: 3 } + ] + ); }); } @@ -1828,6 +1948,14 @@ mod unbond { 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 }, + Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true } + ] + ); }); } } @@ -1944,6 +2072,26 @@ mod withdraw_unbonded { assert!(!SubPoolsStorage::::contains_key(1),); assert!(!RewardPools::::contains_key(1),); assert!(!BondedPools::::contains_key(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::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, + Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::Unbonded { member: 550, pool_id: 1, amount: 550 }, + Event::Unbonded { member: 40, pool_id: 1, amount: 40 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 10 }, + Event::Withdrawn { member: 550, pool_id: 1, amount: 545 }, + Event::MemberRemoved { pool_id: 1, member: 550 }, + Event::Withdrawn { member: 40, pool_id: 1, amount: 40 }, + Event::MemberRemoved { pool_id: 1, member: 40 }, + Event::Withdrawn { member: 10, pool_id: 1, amount: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::Destroyed { pool_id: 1 } + ] + ); }); } @@ -2006,9 +2154,29 @@ mod withdraw_unbonded { assert_eq!(Balances::free_balance(&default_bonded_account()), 0); assert!(!PoolMembers::::contains_key(10)); // Pools are removed from storage because the depositor left - assert!(!SubPoolsStorage::::contains_key(1),); - assert!(!RewardPools::::contains_key(1),); - assert!(!BondedPools::::contains_key(1),); + assert!(!SubPoolsStorage::::contains_key(1)); + assert!(!RewardPools::::contains_key(1)); + assert!(!BondedPools::::contains_key(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::Bonded { member: 40, pool_id: 1, bonded: 40, joined: true }, + Event::Bonded { member: 550, pool_id: 1, bonded: 550, joined: true }, + Event::Unbonded { member: 40, pool_id: 1, amount: 6 }, + Event::Unbonded { member: 550, pool_id: 1, amount: 92 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 2 }, + Event::Withdrawn { member: 40, pool_id: 1, amount: 6 }, + Event::MemberRemoved { pool_id: 1, member: 40 }, + Event::Withdrawn { member: 550, pool_id: 1, amount: 92 }, + Event::MemberRemoved { pool_id: 1, member: 550 }, + Event::Withdrawn { member: 10, pool_id: 1, amount: 0 }, + Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::Destroyed { pool_id: 1 } + ] + ); }); } @@ -2103,6 +2271,17 @@ mod withdraw_unbonded { Pools::withdraw_unbonded(Origin::signed(902), 100, 0), Error::::NotKickerOrDestroying ); + 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: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Bonded { member: 200, pool_id: 1, bonded: 200, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 100 }, + Event::Unbonded { member: 200, pool_id: 1, amount: 200 } + ] + ); // Given unsafe_set_state(1, PoolState::Blocked).unwrap(); @@ -2124,6 +2303,15 @@ mod withdraw_unbonded { assert!(!PoolMembers::::contains_key(100)); assert!(!PoolMembers::::contains_key(200)); assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default()); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Withdrawn { member: 100, pool_id: 1, amount: 100 }, + Event::MemberRemoved { pool_id: 1, member: 100 }, + Event::Withdrawn { member: 200, pool_id: 1, amount: 200 }, + Event::MemberRemoved { pool_id: 1, member: 200 } + ] + ); }); } @@ -2162,6 +2350,17 @@ mod withdraw_unbonded { assert_eq!(SubPoolsStorage::::get(1).unwrap(), Default::default(),); assert_eq!(Balances::free_balance(100), 100 + 100); assert!(!PoolMembers::::contains_key(100)); + 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: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 100 }, + Event::Withdrawn { member: 100, pool_id: 1, amount: 100 }, + Event::MemberRemoved { pool_id: 1, member: 100 } + ] + ); }); } @@ -2240,6 +2439,26 @@ mod withdraw_unbonded { assert!(!SubPoolsStorage::::contains_key(1)); assert!(!RewardPools::::contains_key(1)); assert!(!BondedPools::::contains_key(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::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Bonded { member: 200, pool_id: 1, bonded: 200, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 100 }, + Event::Unbonded { member: 200, pool_id: 1, amount: 200 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 10 }, + Event::Withdrawn { member: 100, pool_id: 1, amount: 100 }, + Event::MemberRemoved { pool_id: 1, member: 100 }, + Event::Withdrawn { member: 200, pool_id: 1, amount: 200 }, + Event::MemberRemoved { pool_id: 1, member: 200 }, + Event::Withdrawn { member: 10, pool_id: 1, amount: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::Destroyed { pool_id: 1 } + ] + ); }); } @@ -2325,9 +2544,7 @@ mod withdraw_unbonded { vec![ Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, - Event::PaidOut { member: 10, pool_id: 1, payout: 0 }, Event::Unbonded { member: 10, pool_id: 1, amount: 6 }, - Event::PaidOut { member: 10, pool_id: 1, payout: 0 }, Event::Unbonded { member: 10, pool_id: 1, amount: 1 } ] ); @@ -2414,9 +2631,7 @@ mod withdraw_unbonded { Event::Created { depositor: 10, pool_id: 1 }, Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true }, Event::Bonded { member: 11, pool_id: 1, bonded: 10, joined: true }, - Event::PaidOut { member: 11, pool_id: 1, payout: 0 }, Event::Unbonded { member: 11, pool_id: 1, amount: 6 }, - Event::PaidOut { member: 11, pool_id: 1, payout: 0 }, Event::Unbonded { member: 11, pool_id: 1, amount: 1 } ] ); @@ -2473,6 +2688,114 @@ mod withdraw_unbonded { ); }); } + + #[test] + fn full_multi_step_withdrawing_non_depositor() { + ExtBuilder::default().add_members(vec![(100, 100)]).build_and_execute(|| { + // given + assert_ok!(Pools::unbond(Origin::signed(100), 100, 75)); + assert_eq!( + PoolMembers::::get(100).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 75) + ); + + // progress one era and unbond the leftover. + CurrentEra::set(1); + assert_ok!(Pools::unbond(Origin::signed(100), 100, 25)); + assert_eq!( + PoolMembers::::get(100).unwrap().unbonding_eras, + member_unbonding_eras!(3 => 75, 4 => 25) + ); + + assert_noop!( + Pools::withdraw_unbonded(Origin::signed(100), 100, 0), + Error::::CannotWithdrawAny + ); + + // now the 75 should be free. + CurrentEra::set(3); + assert_ok!(Pools::withdraw_unbonded(Origin::signed(100), 100, 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::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }, + Event::Unbonded { member: 100, pool_id: 1, amount: 75 }, + Event::Unbonded { member: 100, pool_id: 1, amount: 25 }, + Event::Withdrawn { member: 100, pool_id: 1, amount: 75 }, + ] + ); + assert_eq!( + PoolMembers::::get(100).unwrap().unbonding_eras, + member_unbonding_eras!(4 => 25) + ); + + // the 25 should be free now, and the member removed. + CurrentEra::set(4); + assert_ok!(Pools::withdraw_unbonded(Origin::signed(100), 100, 0)); + assert_eq!( + pool_events_since_last_call(), + vec![ + Event::Withdrawn { member: 100, pool_id: 1, amount: 25 }, + Event::MemberRemoved { pool_id: 1, member: 100 } + ] + ); + }) + } + + #[test] + fn full_multi_step_withdrawing_depositor() { + ExtBuilder::default().ed(1).build_and_execute(|| { + // given + 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) + ); + + assert_noop!( + Pools::withdraw_unbonded(Origin::signed(10), 10, 0), + Error::::CannotWithdrawAny + ); + + // 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, amount: 7 }, + Event::Unbonded { member: 10, pool_id: 1, amount: 3 }, + Event::Withdrawn { member: 10, pool_id: 1, amount: 7 } + ] + ); + assert_eq!( + PoolMembers::::get(10).unwrap().unbonding_eras, + member_unbonding_eras!(4 => 3) + ); + + // the 25 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, amount: 3 }, + Event::MemberRemoved { pool_id: 1, member: 10 }, + // the pool is also destroyed now. + Event::Destroyed { pool_id: 1 }, + ] + ); + }) + } } mod create {