diff --git a/prdoc/pr_4998.prdoc b/prdoc/pr_4998.prdoc new file mode 100644 index 000000000000..41e3886405cc --- /dev/null +++ b/prdoc/pr_4998.prdoc @@ -0,0 +1,20 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: Ensure members can always exit the pool gracefully + +doc: + - audience: Runtime Dev + description: | + Ensures when a member wants to withdraw all funds but the pool is not able to provide all their funds, the member + can receive as much as possible and exit pool. Also handles cases where some extra funds held in member's account + is released when they are removed. + +crates: + - name: pallet-delegated-staking + bump: patch + - name: pallet-nomination-pools + bump: major + - name: sp-staking + bump: major + diff --git a/substrate/frame/delegated-staking/src/impls.rs b/substrate/frame/delegated-staking/src/impls.rs index f8df9dfe7b46..8c05b0bcfc22 100644 --- a/substrate/frame/delegated-staking/src/impls.rs +++ b/substrate/frame/delegated-staking/src/impls.rs @@ -32,28 +32,34 @@ impl DelegationInterface for Pallet { .ok() } + fn agent_transferable_balance(agent: Agent) -> Option { + AgentLedgerOuter::::get(&agent.get()) + .map(|a| a.ledger.unclaimed_withdrawals) + .ok() + } + fn delegator_balance(delegator: Delegator) -> Option { Delegation::::get(&delegator.get()).map(|d| d.amount) } /// Delegate funds to an `Agent`. - fn delegate( - who: Delegator, + fn register_agent( agent: Agent, reward_account: &Self::AccountId, - amount: Self::Balance, ) -> DispatchResult { Pallet::::register_agent( RawOrigin::Signed(agent.clone().get()).into(), reward_account.clone(), - )?; + ) + } - // Delegate the funds from who to the `Agent` account. - Pallet::::delegate_to_agent(RawOrigin::Signed(who.get()).into(), agent.get(), amount) + /// Remove `Agent` registration. + fn remove_agent(agent: Agent) -> DispatchResult { + Pallet::::remove_agent(RawOrigin::Signed(agent.clone().get()).into()) } /// Add more delegation to the `Agent` account. - fn delegate_extra( + fn delegate( who: Delegator, agent: Agent, amount: Self::Balance, @@ -118,7 +124,7 @@ impl DelegationMigrator for Pallet { /// Only used for testing. #[cfg(feature = "runtime-benchmarks")] - fn drop_agent(agent: Agent) { + fn migrate_to_direct_staker(agent: Agent) { >::remove(agent.clone().get()); >::iter() .filter(|(_, delegation)| delegation.agent == agent.clone().get()) diff --git a/substrate/frame/delegated-staking/src/lib.rs b/substrate/frame/delegated-staking/src/lib.rs index 8203f7513305..1882989cfa7d 100644 --- a/substrate/frame/delegated-staking/src/lib.rs +++ b/substrate/frame/delegated-staking/src/lib.rs @@ -304,6 +304,27 @@ pub mod pallet { Ok(()) } + /// Remove an account from being an `Agent`. + /// + /// This can only be called if the agent has no delegated funds, no pending slashes and no + /// unclaimed withdrawals. + pub fn remove_agent(origin: OriginFor) -> DispatchResult { + let who = ensure_signed(origin)?; + let ledger = AgentLedger::::get(&who).ok_or(Error::::NotAgent)?; + + ensure!( + ledger.total_delegated == Zero::zero() && + ledger.pending_slash == Zero::zero() && + ledger.unclaimed_withdrawals == Zero::zero(), + Error::::NotAllowed + ); + + // remove provider reference + let _ = frame_system::Pallet::::dec_providers(&who)?; + >::remove(who); + Ok(()) + } + /// Migrate from a `Nominator` account to `Agent` account. /// /// The origin needs to @@ -599,44 +620,19 @@ impl Pallet { ensure!(delegation.agent == agent, Error::::NotAgent); ensure!(delegation.amount >= amount, Error::::NotEnoughFunds); - // if we do not already have enough funds to be claimed, try withdraw some more. - // keep track if we killed the staker in the process. - let stash_killed = if agent_ledger.ledger.unclaimed_withdrawals < amount { + // if we do not already have enough funds to be claimed, try to withdraw some more. + if agent_ledger.ledger.unclaimed_withdrawals < amount { // withdraw account. - let killed = T::CoreStaking::withdraw_unbonded(agent.clone(), num_slashing_spans) + let _ = T::CoreStaking::withdraw_unbonded(agent.clone(), num_slashing_spans) .map_err(|_| Error::::WithdrawFailed)?; // reload agent from storage since withdrawal might have changed the state. agent_ledger = agent_ledger.reload()?; - Some(killed) - } else { - None - }; + } // if we still do not have enough funds to release, abort. ensure!(agent_ledger.ledger.unclaimed_withdrawals >= amount, Error::::NotEnoughFunds); + agent_ledger.remove_unclaimed_withdraw(amount)?.update(); - // Claim withdraw from agent. Kill agent if no delegation left. - // TODO: Ideally if there is a register, there should be an unregister that should - // clean up the agent. Can be improved in future. - if agent_ledger.remove_unclaimed_withdraw(amount)?.update_or_kill()? { - match stash_killed { - Some(killed) => { - // this implies we did a `CoreStaking::withdraw` before release. Ensure - // we killed the staker as well. - ensure!(killed, Error::::BadState); - }, - None => { - // We did not do a `CoreStaking::withdraw` before release. Ensure staker is - // already killed in `CoreStaking`. - ensure!(T::CoreStaking::status(&agent).is_err(), Error::::BadState); - }, - } - - // Remove provider reference for `who`. - let _ = frame_system::Pallet::::dec_providers(&agent).defensive(); - } - - // book keep delegation delegation.amount = delegation .amount .checked_sub(&amount) diff --git a/substrate/frame/delegated-staking/src/tests.rs b/substrate/frame/delegated-staking/src/tests.rs index ade0872dd390..bc8bc2dccc3c 100644 --- a/substrate/frame/delegated-staking/src/tests.rs +++ b/substrate/frame/delegated-staking/src/tests.rs @@ -943,7 +943,7 @@ mod pool_integration { vec![ PoolsEvent::Withdrawn { member: 302, pool_id, balance: 100, points: 100 }, PoolsEvent::Withdrawn { member: 303, pool_id, balance: 200, points: 200 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 303 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 303, released_balance: 0 }, ] ); @@ -1055,7 +1055,7 @@ mod pool_integration { balance: creator_stake, points: creator_stake, }, - PoolsEvent::MemberRemoved { pool_id, member: creator }, + PoolsEvent::MemberRemoved { pool_id, member: creator, released_balance: 0 }, PoolsEvent::Destroyed { pool_id }, ] ); diff --git a/substrate/frame/delegated-staking/src/types.rs b/substrate/frame/delegated-staking/src/types.rs index 24b457356544..aff282774c3c 100644 --- a/substrate/frame/delegated-staking/src/types.rs +++ b/substrate/frame/delegated-staking/src/types.rs @@ -251,25 +251,10 @@ impl AgentLedgerOuter { self.ledger.update(&key) } - /// Save self and remove if no delegation left. - /// - /// Returns: - /// - true if agent killed. - /// - error if the delegate is in an unexpected state. - pub(crate) fn update_or_kill(self) -> Result { + /// Update agent ledger. + pub(crate) fn update(self) { let key = self.key; - // see if delegate can be killed - if self.ledger.total_delegated == Zero::zero() { - ensure!( - self.ledger.unclaimed_withdrawals == Zero::zero() && - self.ledger.pending_slash == Zero::zero(), - Error::::BadState - ); - >::remove(key); - return Ok(true) - } self.ledger.update(&key); - Ok(false) } /// Reloads self from storage. diff --git a/substrate/frame/nomination-pools/src/adapter.rs b/substrate/frame/nomination-pools/src/adapter.rs index 4d571855e4fe..272b3b60612b 100644 --- a/substrate/frame/nomination-pools/src/adapter.rs +++ b/substrate/frame/nomination-pools/src/adapter.rs @@ -106,9 +106,11 @@ pub trait StakeStrategy { /// Balance that can be transferred from pool account to member. /// - /// This is part of the pool balance that is not actively staked. That is, tokens that are - /// in unbonding period or unbonded. - fn transferable_balance(pool_account: Pool) -> Self::Balance; + /// This is part of the pool balance that can be withdrawn. + fn transferable_balance( + pool_account: Pool, + member_account: Member, + ) -> Self::Balance; /// Total balance of the pool including amount that is actively staked. fn total_balance(pool_account: Pool) -> Option; @@ -181,6 +183,9 @@ pub trait StakeStrategy { num_slashing_spans: u32, ) -> DispatchResult; + /// Dissolve the pool account. + fn dissolve(pool_account: Pool) -> DispatchResult; + /// Check if there is any pending slash for the pool. fn pending_slash(pool_account: Pool) -> Self::Balance; @@ -253,12 +258,15 @@ impl, AccountId = T: StakeStrategyType::Transfer } - fn transferable_balance(pool_account: Pool) -> BalanceOf { + fn transferable_balance( + pool_account: Pool, + _: Member, + ) -> BalanceOf { T::Currency::balance(&pool_account.0).saturating_sub(Self::active_stake(pool_account)) } fn total_balance(pool_account: Pool) -> Option> { - Some(T::Currency::total_balance(&pool_account.0)) + Some(T::Currency::total_balance(&pool_account.get())) } fn member_delegation_balance( @@ -300,6 +308,17 @@ impl, AccountId = T: Ok(()) } + fn dissolve(pool_account: Pool) -> DispatchResult { + defensive_assert!( + T::Currency::total_balance(&pool_account.clone().get()).is_zero(), + "dissolving pool should not have any balance" + ); + + // Defensively force set balance to zero. + T::Currency::set_balance(&pool_account.get(), Zero::zero()); + Ok(()) + } + fn pending_slash(_: Pool) -> Self::Balance { // for transfer stake strategy, slashing is greedy and never deferred. Zero::zero() @@ -360,11 +379,14 @@ impl< StakeStrategyType::Delegate } - fn transferable_balance(pool_account: Pool) -> BalanceOf { - Delegation::agent_balance(pool_account.clone().into()) + fn transferable_balance( + pool_account: Pool, + member_account: Member, + ) -> BalanceOf { + Delegation::agent_transferable_balance(pool_account.clone().into()) // pool should always be an agent. .defensive_unwrap_or_default() - .saturating_sub(Self::active_stake(pool_account)) + .min(Delegation::delegator_balance(member_account.into()).unwrap_or_default()) } fn total_balance(pool_account: Pool) -> Option> { @@ -384,12 +406,13 @@ impl< ) -> DispatchResult { match bond_type { BondType::Create => { - // first delegation - Delegation::delegate(who.into(), pool_account.into(), reward_account, amount) + // first delegation. Register agent first. + Delegation::register_agent(pool_account.clone().into(), reward_account)?; + Delegation::delegate(who.into(), pool_account.into(), amount) }, BondType::Extra => { // additional delegation - Delegation::delegate_extra(who.into(), pool_account.into(), amount) + Delegation::delegate(who.into(), pool_account.into(), amount) }, } } @@ -403,6 +426,10 @@ impl< Delegation::withdraw_delegation(who.into(), pool_account.into(), amount, num_slashing_spans) } + fn dissolve(pool_account: Pool) -> DispatchResult { + Delegation::remove_agent(pool_account.into()) + } + fn pending_slash(pool_account: Pool) -> Self::Balance { Delegation::pending_slash(pool_account.into()).defensive_unwrap_or_default() } @@ -433,6 +460,6 @@ impl< #[cfg(feature = "runtime-benchmarks")] fn remove_as_agent(pool: Pool) { - Delegation::drop_agent(pool.into()) + Delegation::migrate_to_direct_staker(pool.into()) } } diff --git a/substrate/frame/nomination-pools/src/lib.rs b/substrate/frame/nomination-pools/src/lib.rs index 70ad06e2a4da..9108060ccb2a 100644 --- a/substrate/frame/nomination-pools/src/lib.rs +++ b/substrate/frame/nomination-pools/src/lib.rs @@ -1831,7 +1831,9 @@ pub mod pallet { /// 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 }, + /// Any funds that are still delegated (i.e. dangling delegation) are released and are + /// represented by `released_balance`. + MemberRemoved { pool_id: PoolId, member: T::AccountId, released_balance: BalanceOf }, /// The roles of a pool have been updated to the given new roles. Note that the depositor /// can never change. RolesUpdated { @@ -2342,9 +2344,10 @@ pub mod pallet { // 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 members can leave the pool and it can be destroyed. - .min(T::StakeAdapter::transferable_balance(Pool::from( - bonded_pool.bonded_account(), - ))); + .min(T::StakeAdapter::transferable_balance( + Pool::from(bonded_pool.bonded_account()), + Member::from(member_account.clone()), + )); // this can fail if the pool uses `DelegateStake` strategy and the member delegation // is not claimed yet. See `Call::migrate_delegation()`. @@ -2368,9 +2371,27 @@ pub mod pallet { // member being reaped. PoolMembers::::remove(&member_account); + + // Ensure any dangling delegation is withdrawn. + let dangling_withdrawal = match T::StakeAdapter::member_delegation_balance( + Member::from(member_account.clone()), + ) { + Some(dangling_delegation) => { + T::StakeAdapter::member_withdraw( + Member::from(member_account.clone()), + Pool::from(bonded_pool.bonded_account()), + dangling_delegation, + num_slashing_spans, + )?; + dangling_delegation + }, + None => Zero::zero(), + }; + Self::deposit_event(Event::::MemberRemoved { pool_id: member.pool_id, member: member_account.clone(), + released_balance: dangling_withdrawal, }); if member_account == bonded_pool.roles.depositor { @@ -3078,16 +3099,11 @@ impl Pallet { T::Currency::total_balance(&reward_account) == Zero::zero(), "could not transfer all amount to depositor while dissolving pool" ); - defensive_assert!( - T::StakeAdapter::total_balance(Pool::from(bonded_pool.bonded_account())) - .unwrap_or_default() == - Zero::zero(), - "dissolving pool should not have any balance" - ); // NOTE: Defensively force set balance to zero. T::Currency::set_balance(&reward_account, Zero::zero()); - // NOTE: With `DelegateStake` strategy, this won't do anything. - T::Currency::set_balance(&bonded_pool.bonded_account(), Zero::zero()); + + // dissolve pool account. + let _ = T::StakeAdapter::dissolve(Pool::from(bonded_account)).defensive(); Self::deposit_event(Event::::Destroyed { pool_id: bonded_pool.id }); // Remove bonded pool metadata. @@ -3525,13 +3541,8 @@ impl Pallet { // this is their balance in the pool let expected_balance = pool_member.total_balance(); - defensive_assert!( - actual_balance >= expected_balance, - "actual balance should always be greater or equal to the expected" - ); - // return the amount to be slashed. - Ok(actual_balance.defensive_saturating_sub(expected_balance)) + Ok(actual_balance.saturating_sub(expected_balance)) } /// Apply freeze on reward account to restrict it from going below ED. diff --git a/substrate/frame/nomination-pools/src/tests.rs b/substrate/frame/nomination-pools/src/tests.rs index 28063c2ecaec..06261699a5b2 100644 --- a/substrate/frame/nomination-pools/src/tests.rs +++ b/substrate/frame/nomination-pools/src/tests.rs @@ -2364,10 +2364,10 @@ mod claim_payout { Event::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, Event::Unbonded { member: 20, pool_id: 1, balance: 20, points: 20, era: 3 }, Event::Withdrawn { member: 20, pool_id: 1, balance: 20, points: 20 }, - Event::MemberRemoved { pool_id: 1, member: 20 }, + Event::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 }, Event::Unbonded { member: 10, pool_id: 1, balance: 10, points: 10, era: 6 }, Event::Withdrawn { member: 10, pool_id: 1, balance: 10, points: 10 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 } ] ); @@ -2939,9 +2939,9 @@ mod unbond { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 40, pool_id: 1, points: 6, balance: 6 }, - Event::MemberRemoved { pool_id: 1, member: 40 }, + Event::MemberRemoved { pool_id: 1, member: 40, released_balance: 0 }, Event::Withdrawn { member: 550, pool_id: 1, points: 92, balance: 92 }, - Event::MemberRemoved { pool_id: 1, member: 550 }, + Event::MemberRemoved { pool_id: 1, member: 550, released_balance: 0 }, Event::PaidOut { member: 10, pool_id: 1, payout: 10 }, Event::Unbonded { member: 10, pool_id: 1, points: 2, balance: 2, era: 6 } ] @@ -3688,7 +3688,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 550, pool_id: 1, balance: 275, points: 550 }, - Event::MemberRemoved { pool_id: 1, member: 550 } + Event::MemberRemoved { pool_id: 1, member: 550, released_balance: 0 } ] ); assert_eq!( @@ -3709,7 +3709,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 40, pool_id: 1, balance: 20, points: 40 }, - Event::MemberRemoved { pool_id: 1, member: 40 } + Event::MemberRemoved { pool_id: 1, member: 40, released_balance: 0 } ] ); assert_eq!( @@ -3731,7 +3731,7 @@ mod withdraw_unbonded { vec![ Event::Unbonded { member: 10, pool_id: 1, balance: 5, points: 5, era: 9 }, Event::Withdrawn { member: 10, pool_id: 1, balance: 5, points: 5 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 } ] ); @@ -3806,7 +3806,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 40, pool_id: 1, balance: 20, points: 20 }, - Event::MemberRemoved { pool_id: 1, member: 40 } + Event::MemberRemoved { pool_id: 1, member: 40, released_balance: 0 } ] ); @@ -3827,7 +3827,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 550, pool_id: 1, balance: 275, points: 275 }, - Event::MemberRemoved { pool_id: 1, member: 550 } + Event::MemberRemoved { pool_id: 1, member: 550, released_balance: 0 } ] ); assert!(SubPoolsStorage::::get(1).unwrap().with_era.is_empty()); @@ -3862,7 +3862,7 @@ mod withdraw_unbonded { vec![ Event::Unbonded { member: 10, pool_id: 1, points: 5, balance: 5, era: 6 }, Event::Withdrawn { member: 10, pool_id: 1, points: 5, balance: 5 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 } ] ); @@ -4018,9 +4018,9 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 100, pool_id: 1, points: 100, balance: 100 }, - Event::MemberRemoved { pool_id: 1, member: 100 }, + Event::MemberRemoved { pool_id: 1, member: 100, released_balance: 0 }, Event::Withdrawn { member: 200, pool_id: 1, points: 200, balance: 200 }, - Event::MemberRemoved { pool_id: 1, member: 200 } + Event::MemberRemoved { pool_id: 1, member: 200, released_balance: 0 } ] ); }); @@ -4070,7 +4070,7 @@ mod withdraw_unbonded { Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }, Event::Unbonded { member: 100, pool_id: 1, points: 100, balance: 100, era: 3 }, Event::Withdrawn { member: 100, pool_id: 1, points: 100, balance: 100 }, - Event::MemberRemoved { pool_id: 1, member: 100 } + Event::MemberRemoved { pool_id: 1, member: 100, released_balance: 0 } ] ); }); @@ -4310,7 +4310,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 100, pool_id: 1, points: 25, balance: 25 }, - Event::MemberRemoved { pool_id: 1, member: 100 } + Event::MemberRemoved { pool_id: 1, member: 100, released_balance: 0 } ] ); }) @@ -4548,7 +4548,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 10, pool_id: 1, points: 13, balance: 13 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 }, ] ); @@ -4588,7 +4588,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 20, pool_id: 1, balance: 20, points: 20 }, - Event::MemberRemoved { pool_id: 1, member: 20 } + Event::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 } ] ); @@ -4626,7 +4626,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 10, pool_id: 1, points: 10, balance: 10 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 }, ] ); @@ -4672,7 +4672,7 @@ mod withdraw_unbonded { pool_events_since_last_call(), vec![ Event::Withdrawn { member: 10, pool_id: 1, points: 10, balance: 10 }, - Event::MemberRemoved { pool_id: 1, member: 10 }, + Event::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, Event::Destroyed { pool_id: 1 }, ] ); diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs index 51f6470f90d0..a50a6b73f5fc 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs @@ -152,9 +152,9 @@ fn pool_lifecycle_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 20, pool_id: 1, points: 10, balance: 10 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 20 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 }, PoolsEvent::Withdrawn { member: 21, pool_id: 1, points: 10, balance: 10 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 21 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 21, released_balance: 0 }, ] ); @@ -193,7 +193,7 @@ fn pool_lifecycle_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 10, pool_id: 1, points: 50, balance: 50 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 10 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, PoolsEvent::Destroyed { pool_id: 1 } ] ); @@ -476,10 +476,10 @@ fn pool_slash_e2e() { vec![ // 20 had unbonded 10 safely, and 10 got slashed by half. PoolsEvent::Withdrawn { member: 20, pool_id: 1, balance: 10 + 5, points: 20 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 20 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 }, // 21 unbonded all of it after the slash PoolsEvent::Withdrawn { member: 21, pool_id: 1, balance: 5 + 5, points: 15 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 21 } + PoolsEvent::MemberRemoved { pool_id: 1, member: 21, released_balance: 0 } ] ); assert_eq!( @@ -525,7 +525,7 @@ fn pool_slash_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 10, pool_id: 1, balance: 10 + 15, points: 30 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 10 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, PoolsEvent::Destroyed { pool_id: 1 } ] ); @@ -1160,7 +1160,7 @@ fn pool_migration_e2e() { vec![ PoolsEvent::Withdrawn { member: 21, pool_id: 1, balance: 10, points: 10 }, // 21 was fully unbonding and removed from pool. - PoolsEvent::MemberRemoved { member: 21, pool_id: 1 }, + PoolsEvent::MemberRemoved { member: 21, pool_id: 1, released_balance: 0 }, PoolsEvent::Withdrawn { member: 22, pool_id: 1, balance: 5, points: 5 }, ] ); @@ -1183,3 +1183,313 @@ fn pool_migration_e2e() { ); }) } + +#[test] +fn pool_no_dangling_delegation() { + new_test_ext().execute_with(|| { + ExistentialDeposit::set(1); + assert_eq!(Balances::minimum_balance(), 1); + assert_eq!(Staking::current_era(), None); + // pool creator + let alice = 10; + let bob = 20; + let charlie = 21; + + // create the pool, we know this has id 1. + assert_ok!(Pools::create(RuntimeOrigin::signed(alice), 40, alice, alice, alice)); + assert_eq!(LastPoolId::::get(), 1); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 40 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Created { depositor: alice, pool_id: 1 }, + PoolsEvent::Bonded { member: alice, pool_id: 1, bonded: 40, joined: true }, + ] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: alice, + amount: 40 + },] + ); + + assert_eq!( + Payee::::get(POOL1_BONDED), + Some(RewardDestination::Account(POOL1_REWARD)) + ); + + // have two members join + assert_ok!(Pools::join(RuntimeOrigin::signed(bob), 20, 1)); + assert_ok!(Pools::join(RuntimeOrigin::signed(charlie), 20, 1)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Bonded { stash: POOL1_BONDED, amount: 20 }, + StakingEvent::Bonded { stash: POOL1_BONDED, amount: 20 } + ] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Bonded { member: bob, pool_id: 1, bonded: 20, joined: true }, + PoolsEvent::Bonded { member: charlie, pool_id: 1, bonded: 20, joined: true }, + ] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: bob, + amount: 20 + }, + DelegatedStakingEvent::Delegated { + agent: POOL1_BONDED, + delegator: charlie, + amount: 20 + }, + ] + ); + + // now let's progress a bit. + CurrentEra::::set(Some(1)); + + // bob is completely unbonding + assert_ok!(Pools::unbond(RuntimeOrigin::signed(bob), 20, 20)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 20 },] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { member: bob, pool_id: 1, balance: 20, points: 20, era: 4 }] + ); + + // this era will get slashed + CurrentEra::::set(Some(2)); + + assert_ok!(Pools::unbond(RuntimeOrigin::signed(alice), 10, 10)); + assert_ok!(Pools::unbond(RuntimeOrigin::signed(charlie), 21, 10)); + + assert_eq!( + staking_events_since_last_call(), + vec![ + StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 10 }, + StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 10 }, + ] + ); + + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Unbonded { member: alice, pool_id: 1, balance: 10, points: 10, era: 5 }, + PoolsEvent::Unbonded { + member: charlie, + pool_id: 1, + balance: 10, + points: 10, + era: 5 + }, + ] + ); + + // At this point, bob's 20 that is unlocking is safe from slash, 10 (alice) + 10 (charlie) + // are also unlocking but vulnerable to slash, and another 40 are active and vulnerable to + // slash. Let's slash half of them. + pallet_staking::slashing::do_slash::( + &POOL1_BONDED, + 30, + &mut Default::default(), + &mut Default::default(), + 2, // slash era 2, affects chunks at era 5 onwards. + ); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Slashed { staker: POOL1_BONDED, amount: 30 }] + ); + + assert_eq!( + pool_events_since_last_call(), + vec![ + // unbonding pool of 20 for era 5 has been slashed to 10 + PoolsEvent::UnbondingPoolSlashed { pool_id: 1, era: 5, balance: 10 }, + // active stake of 40 has been slashed to half + PoolsEvent::PoolSlashed { pool_id: 1, balance: 20 } /* no slash to era 4 + * unbonding pool */ + ] + ); + + // alice's initial stake of 40 is reduced to half + assert_eq!(PoolMembers::::get(alice).unwrap().total_balance(), 20); + // bob unbonded in era 1 and is safe from slash + assert_eq!(PoolMembers::::get(bob).unwrap().total_balance(), 20); + // charlie's initial stake of 20 is slashed to half + assert_eq!(PoolMembers::::get(charlie).unwrap().total_balance(), 10); + + // apply pending slash to alice. + assert_eq!(Pools::api_member_pending_slash(alice), 20); + assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(10), alice)); + // apply pending slash to charlie. + assert_eq!(Pools::api_member_pending_slash(charlie), 10); + assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(10), charlie)); + // no pending slash for bob + assert_eq!(Pools::api_member_pending_slash(bob), 0); + + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Slashed { + agent: POOL1_BONDED, + delegator: alice, + amount: 20 + }, + DelegatedStakingEvent::Slashed { + agent: POOL1_BONDED, + delegator: charlie, + amount: 10 + }, + ] + ); + + // go forward to an era after PostUnbondingPoolsWindow = 10 ends for era 5. + CurrentEra::::set(Some(15)); + // At this point subpools will all be merged in no-era causing Bob to lose some value while + // Alice and Charlie will gain some value. + assert_ok!(Pools::unbond(RuntimeOrigin::signed(charlie), charlie, 10)); + + // Now alice and charlie has less balance locked than their contribution. + assert_eq!(Balances::total_balance_on_hold(&alice), 20); + assert_eq!(PoolMembers::::get(alice).unwrap().total_balance(), 22); + assert_eq!(Balances::total_balance_on_hold(&charlie), 10); + assert_eq!(PoolMembers::::get(charlie).unwrap().total_balance(), 12); + + // and bob has more balance locked than his contribution. + assert_eq!(Balances::total_balance_on_hold(&bob), 20); + assert_eq!(PoolMembers::::get(bob).unwrap().total_balance(), 15); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 5 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![PoolsEvent::Unbonded { + member: charlie, + pool_id: 1, + balance: 5, + points: 5, + era: 18 + }] + ); + + // When bob withdraws all, he gets all his locked funds back. + let bob_pre_withdraw_balance = Balances::free_balance(&bob); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(bob), bob, 0)); + assert_eq!(Balances::free_balance(&bob), bob_pre_withdraw_balance + 20); + assert_eq!(Balances::total_balance_on_hold(&bob), 0); + assert!(!PoolMembers::::contains_key(bob)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 30 },] + ); + + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Withdrawn { pool_id: 1, member: bob, balance: 15, points: 20 }, + // dangling delegation of 5 is released + PoolsEvent::MemberRemoved { pool_id: 1, member: bob, released_balance: 5 }, + ] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![ + DelegatedStakingEvent::Released { agent: POOL1_BONDED, delegator: bob, amount: 15 }, + // the second release is the dangling delegation when member is removed. + DelegatedStakingEvent::Released { agent: POOL1_BONDED, delegator: bob, amount: 5 }, + ] + ); + + // Charlie can withdraw as much as he has locked. + CurrentEra::::set(Some(18)); + let charlie_pre_withdraw_balance = Balances::free_balance(&charlie); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(charlie), charlie, 0)); + // Charlie's total balance was 12, but we don't have enough funds to unlock. We try the best + // effort and unlock 10. + assert_eq!(Balances::free_balance(&charlie), charlie_pre_withdraw_balance + 10); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 5 },] + ); + + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Withdrawn { pool_id: 1, member: charlie, balance: 10, points: 15 }, + PoolsEvent::MemberRemoved { member: charlie, pool_id: 1, released_balance: 0 } + ] + ); + assert_eq!( + delegated_staking_events_since_last_call(), + vec![DelegatedStakingEvent::Released { + agent: POOL1_BONDED, + delegator: charlie, + amount: 10 + },] + ); + + // Set pools to destroying so alice can withdraw + assert_ok!(Pools::set_state(RuntimeOrigin::signed(alice), 1, PoolState::Destroying)); + assert_ok!(Pools::unbond(RuntimeOrigin::signed(alice), alice, 30)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Unbonded { stash: POOL1_BONDED, amount: 15 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::StateChanged { pool_id: 1, new_state: PoolState::Destroying }, + PoolsEvent::Unbonded { + member: alice, + pool_id: 1, + points: 15, + balance: 15, + era: 21 + } + ] + ); + + CurrentEra::::set(Some(21)); + assert_ok!(Pools::withdraw_unbonded(RuntimeOrigin::signed(alice), alice, 0)); + + assert_eq!( + staking_events_since_last_call(), + vec![StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 15 }] + ); + assert_eq!( + pool_events_since_last_call(), + vec![ + PoolsEvent::Withdrawn { member: alice, pool_id: 1, balance: 20, points: 25 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: alice, released_balance: 0 }, + PoolsEvent::Destroyed { pool_id: 1 } + ] + ); + + // holds for all members are released. + assert_eq!(Balances::total_balance_on_hold(&alice), 0); + assert_eq!(Balances::total_balance_on_hold(&bob), 0); + assert_eq!(Balances::total_balance_on_hold(&charlie), 0); + }); +} diff --git a/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs b/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs index ed47932a323b..d1bc4ef8ff28 100644 --- a/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs +++ b/substrate/frame/nomination-pools/test-delegate-stake/src/mock.rs @@ -154,11 +154,14 @@ impl pallet_nomination_pools::adapter::StakeStrategy for MockAdapter { } DelegateStake::strategy_type() } - fn transferable_balance(pool_account: Pool) -> Self::Balance { + fn transferable_balance( + pool_account: Pool, + member_account: Member, + ) -> Self::Balance { if LegacyAdapter::get() { - return TransferStake::transferable_balance(pool_account) + return TransferStake::transferable_balance(pool_account, member_account) } - DelegateStake::transferable_balance(pool_account) + DelegateStake::transferable_balance(pool_account, member_account) } fn total_balance(pool_account: Pool) -> Option { @@ -200,6 +203,13 @@ impl pallet_nomination_pools::adapter::StakeStrategy for MockAdapter { DelegateStake::member_withdraw(who, pool_account, amount, num_slashing_spans) } + fn dissolve(pool_account: Pool) -> DispatchResult { + if LegacyAdapter::get() { + return TransferStake::dissolve(pool_account) + } + DelegateStake::dissolve(pool_account) + } + fn pending_slash(pool_account: Pool) -> Self::Balance { if LegacyAdapter::get() { return TransferStake::pending_slash(pool_account) diff --git a/substrate/frame/nomination-pools/test-transfer-stake/src/lib.rs b/substrate/frame/nomination-pools/test-transfer-stake/src/lib.rs index aa9135025900..28e978bba0e5 100644 --- a/substrate/frame/nomination-pools/test-transfer-stake/src/lib.rs +++ b/substrate/frame/nomination-pools/test-transfer-stake/src/lib.rs @@ -145,9 +145,9 @@ fn pool_lifecycle_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 20, pool_id: 1, points: 10, balance: 10 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 20 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 }, PoolsEvent::Withdrawn { member: 21, pool_id: 1, points: 10, balance: 10 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 21 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 21, released_balance: 0 }, ] ); @@ -186,7 +186,7 @@ fn pool_lifecycle_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 10, pool_id: 1, points: 50, balance: 50 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 10 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, PoolsEvent::Destroyed { pool_id: 1 } ] ); @@ -275,7 +275,7 @@ fn destroy_pool_with_erroneous_consumer() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 10, pool_id: 1, points: 50, balance: 50 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 10 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, PoolsEvent::Destroyed { pool_id: 1 } ] ); @@ -558,10 +558,10 @@ fn pool_slash_e2e() { vec![ // 20 had unbonded 10 safely, and 10 got slashed by half. PoolsEvent::Withdrawn { member: 20, pool_id: 1, balance: 10 + 5, points: 20 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 20 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 20, released_balance: 0 }, // 21 unbonded all of it after the slash PoolsEvent::Withdrawn { member: 21, pool_id: 1, balance: 5 + 5, points: 15 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 21 } + PoolsEvent::MemberRemoved { pool_id: 1, member: 21, released_balance: 0 } ] ); assert_eq!( @@ -607,7 +607,7 @@ fn pool_slash_e2e() { pool_events_since_last_call(), vec![ PoolsEvent::Withdrawn { member: 10, pool_id: 1, balance: 10 + 15, points: 30 }, - PoolsEvent::MemberRemoved { pool_id: 1, member: 10 }, + PoolsEvent::MemberRemoved { pool_id: 1, member: 10, released_balance: 0 }, PoolsEvent::Destroyed { pool_id: 1 } ] ); diff --git a/substrate/primitives/staking/src/lib.rs b/substrate/primitives/staking/src/lib.rs index 03c1af50240b..5e94524816a0 100644 --- a/substrate/primitives/staking/src/lib.rs +++ b/substrate/primitives/staking/src/lib.rs @@ -525,23 +525,26 @@ pub trait DelegationInterface { /// This takes into account any pending slashes to `Agent` against the delegated balance. fn agent_balance(agent: Agent) -> Option; + /// Returns the total amount of funds that is unbonded and can be withdrawn from the `Agent` + /// account. `None` if not an `Agent`. + fn agent_transferable_balance(agent: Agent) -> Option; + /// Returns the total amount of funds delegated. `None` if not a `Delegator`. fn delegator_balance(delegator: Delegator) -> Option; - /// Delegate funds to `Agent`. - /// - /// Only used for the initial delegation. Use [`Self::delegate_extra`] to add more delegation. - fn delegate( - delegator: Delegator, + /// Register `Agent` such that it can accept delegation. + fn register_agent( agent: Agent, reward_account: &Self::AccountId, - amount: Self::Balance, ) -> DispatchResult; - /// Add more delegation to the `Agent`. + /// Removes `Agent` registration. /// - /// If this is the first delegation, use [`Self::delegate`] instead. - fn delegate_extra( + /// This should only be allowed if the agent has no staked funds. + fn remove_agent(agent: Agent) -> DispatchResult; + + /// Add delegation to the `Agent`. + fn delegate( delegator: Delegator, agent: Agent, amount: Self::Balance, @@ -616,7 +619,7 @@ pub trait DelegationMigrator { /// /// Also removed from [`StakingUnchecked`] as a Virtual Staker. Useful for testing. #[cfg(feature = "runtime-benchmarks")] - fn drop_agent(agent: Agent); + fn migrate_to_direct_staker(agent: Agent); } sp_core::generate_feature_enabled_macro!(runtime_benchmarks_enabled, feature = "runtime-benchmarks", $);