Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reap pool member whose balance goes below ED #5769

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions substrate/frame/nomination-pools/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,13 @@ pub trait StakeStrategy {
num_slashing_spans: u32,
) -> DispatchResult;

/// Immediately withdraw the given amount from the pool account.
fn force_withdraw(
who: Member<Self::AccountId>,
pool_account: Pool<Self::AccountId>,
amount: Self::Balance,
) -> DispatchResult;

/// Dissolve the pool account.
fn dissolve(pool_account: Pool<Self::AccountId>) -> DispatchResult;

Expand Down Expand Up @@ -308,6 +315,14 @@ impl<T: Config, Staking: StakingInterface<Balance = BalanceOf<T>, AccountId = T:
Ok(())
}

fn force_withdraw(
_who: Member<Self::AccountId>,
pool_account: Pool<Self::AccountId>,
amount: Self::Balance,
) -> DispatchResult {
Staking::force_withdraw(&pool_account.0, amount)
}

fn dissolve(pool_account: Pool<Self::AccountId>) -> DispatchResult {
defensive_assert!(
T::Currency::total_balance(&pool_account.clone().get()).is_zero(),
Expand Down Expand Up @@ -458,6 +473,15 @@ impl<
Delegation::migrate_delegation(pool.into(), delegator.into(), value)
}

fn force_withdraw(
who: Member<Self::AccountId>,
pool: Pool<Self::AccountId>,
amount: Self::Balance,
) -> DispatchResult {
Staking::force_withdraw(&pool.0, amount)?;
Delegation::withdraw_delegation(who.into(), pool.clone().into(), amount, 0)
}

#[cfg(feature = "runtime-benchmarks")]
fn remove_as_agent(pool: Pool<Self::AccountId>) {
Delegation::migrate_to_direct_staker(pool.into())
Expand Down
76 changes: 70 additions & 6 deletions substrate/frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2991,6 +2991,13 @@ pub mod pallet {
let member_account = T::Lookup::lookup(member_account)?;
Self::do_apply_slash(&member_account, Some(who))?;

let member =
PoolMembers::<T>::get(&member_account).ok_or(Error::<T>::PoolMemberNotFound)?;
let contribution = member.total_balance();
if contribution < T::Currency::minimum_balance() {
Self::reap_member(&member_account, contribution)?;
}

// If successful, refund the fees.
Ok(Pays::No.into())
}
Expand Down Expand Up @@ -3030,12 +3037,10 @@ pub mod pallet {
);

let pool_contribution = member.total_balance();
// ensure the pool contribution is greater than the existential deposit otherwise we
// cannot transfer funds to member account.
ensure!(
pool_contribution >= T::Currency::minimum_balance(),
Error::<T>::MinimumBondNotMet
);
if pool_contribution < T::Currency::minimum_balance() {
Self::reap_member(&member_account, pool_contribution)?;
return Err(Error::<T>::MinimumBondNotMet.into());
}

let delegation =
T::StakeAdapter::member_delegation_balance(Member::from(member_account.clone()));
Expand Down Expand Up @@ -3591,6 +3596,65 @@ impl<T: Config> Pallet<T> {
)
}

/// Reap member whose contribution to the pool is below ED.
fn reap_member(
member: &T::AccountId,
contribution: BalanceOf<T>,
) -> DispatchResult {
let (mut pool_member, mut bonded_pool, mut reward_pool) =
Self::get_member_with_pools(&member)?;
let pool_id = pool_member.pool_id;

reward_pool.update_records(
pool_id,
bonded_pool.points,
bonded_pool.commission.current(),
)?;

let _ =
Self::do_reward_payout(&member, &mut pool_member, &mut bonded_pool, &mut reward_pool);

if contribution > Zero::zero() {
T::StakeAdapter::force_withdraw(
Member::from(member.clone()),
Pool::from(bonded_pool.bonded_account()),
contribution,
)?;

// FIXME eagr update points and stuff
}

ClaimPermissions::<T>::remove(&member);
PoolMembers::<T>::remove(&member);

let released_dangling =
match T::StakeAdapter::member_delegation_balance(Member::from(member.clone())) {
Some(dangling_amount) => {
T::StakeAdapter::member_withdraw(
Member::from(member.clone()),
Pool::from(bonded_pool.bonded_account()),
dangling_amount,
0,
)?;
dangling_amount
},
None => Zero::zero(),
};

Self::deposit_event(Event::<T>::MemberRemoved {
pool_id,
member: member.clone(),
released_balance: released_dangling,
});

bonded_pool.dec_members().put();
// FIXME eagr update sub pools
let sub_pools = SubPoolsStorage::<T>::get(pool_id).unwrap_or_default();
SubPoolsStorage::<T>::insert(pool_id, sub_pools);

Ok(())
}

/// Pending slash for a member.
///
/// Takes the pool_member object corresponding to the `member_account`.
Expand Down
124 changes: 123 additions & 1 deletion substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use frame_support::{
use mock::*;
use pallet_nomination_pools::{
BondExtra, BondedPools, CommissionChangeRate, ConfigOp, Error as PoolsError,
Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, PoolState,
Event as PoolsEvent, LastPoolId, PoolMember, PoolMembers, PoolState, TotalValueLocked,
};
use pallet_staking::{
CurrentEra, Error as StakingError, Event as StakingEvent, Payee, RewardDestination,
Expand Down Expand Up @@ -1674,3 +1674,125 @@ fn pool_no_dangling_delegation() {
assert_eq!(Balances::total_balance_on_hold(&charlie), 0);
});
}

#[test]
fn reap_member_delegate() {
new_test_ext().execute_with(|| {
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);

assert_ok!(Pools::create(RuntimeOrigin::signed(10), 15, 10, 10, 10));
assert_eq!(LastPoolId::<Runtime>::get(), 1);

assert_eq!(
delegated_staking_events_since_last_call(),
vec![DelegatedStakingEvent::Delegated {
agent: POOL1_BONDED,
delegator: 10,
amount: 15
}],
);
assert_eq!(
staking_events_since_last_call(),
vec![StakingEvent::Bonded { stash: POOL1_BONDED, amount: 15 }]
);
assert_eq!(
pool_events_since_last_call(),
vec![
PoolsEvent::Created { depositor: 10, pool_id: 1 },
PoolsEvent::Bonded { member: 10, pool_id: 1, bonded: 15, joined: true },
]
);

assert_eq!(
Payee::<Runtime>::get(POOL1_BONDED),
Some(RewardDestination::Account(POOL1_REWARD))
);

assert_ok!(Pools::join(RuntimeOrigin::signed(20), 10, 1));
assert_ok!(Pools::join(RuntimeOrigin::signed(21), 5, 1));

assert_eq!(
delegated_staking_events_since_last_call(),
vec![
DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, delegator: 20, amount: 10 },
DelegatedStakingEvent::Delegated { agent: POOL1_BONDED, delegator: 21, amount: 5 },
],
);
assert_eq!(
staking_events_since_last_call(),
vec![
StakingEvent::Bonded { stash: POOL1_BONDED, amount: 10 },
StakingEvent::Bonded { stash: POOL1_BONDED, amount: 5 },
]
);
assert_eq!(
pool_events_since_last_call(),
vec![
PoolsEvent::Bonded { member: 20, pool_id: 1, bonded: 10, joined: true },
PoolsEvent::Bonded { member: 21, pool_id: 1, bonded: 5, joined: true },
]
);

assert_eq!(TotalValueLocked::<Runtime>::get(), 30);

CurrentEra::<Runtime>::set(Some(1));

// slash era 1
pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
15,
&mut Default::default(),
&mut Default::default(),
1,
);

assert_eq!(
staking_events_since_last_call(),
vec![StakingEvent::Slashed { staker: POOL1_BONDED, amount: 15 }],
);
assert_eq!(
pool_events_since_last_call(),
vec![PoolsEvent::PoolSlashed { pool_id: 1, balance: 15 }],
);

CurrentEra::<Runtime>::set(Some(5));

let tvl = TotalValueLocked::<Runtime>::get();

// >ED after slash
assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().total_balance(), 7);
// =ED after slash
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().total_balance(), 5);
// <ED after slash, will be reaped
assert_eq!(PoolMembers::<Runtime>::get(21).unwrap().total_balance(), 2);

assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 10));
assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 20));
assert_ok!(Pools::apply_slash(RuntimeOrigin::signed(21), 21));

// balance of reaped member is subtracted from TVL
assert_eq!(tvl - TotalValueLocked::<Runtime>::get(), 2);

assert_eq!(
staking_events_since_last_call(),
vec![
StakingEvent::Withdrawn { stash: POOL1_BONDED, amount: 2 },
],
);
assert_eq!(
delegated_staking_events_since_last_call(),
vec![DelegatedStakingEvent::Released { agent: POOL1_BONDED, delegator: 21, amount: 2 }],
);
assert_eq!(
pool_events_since_last_call(),
vec![PoolsEvent::MemberRemoved { pool_id: 1, member: 21, released_balance: 0 }],
);

// member reaped
assert_noop!(
Pools::apply_slash(RuntimeOrigin::signed(21), 21),
PoolsError::<Runtime>::PoolMemberNotFound
);
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,14 @@ impl pallet_nomination_pools::adapter::StakeStrategy for MockAdapter {
DelegateStake::member_withdraw(who, pool_account, amount, num_slashing_spans)
}

fn force_withdraw(
who: Member<Self::AccountId>,
pool: Pool<Self::AccountId>,
amount: Self::Balance,
) -> DispatchResult {
DelegateStake::force_withdraw(who, pool, amount)
}

fn dissolve(pool_account: Pool<Self::AccountId>) -> DispatchResult {
if LegacyAdapter::get() {
return TransferStake::dissolve(pool_account)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use frame_support::{assert_noop, assert_ok, traits::Currency};
use mock::*;
use pallet_nomination_pools::{
BondExtra, BondedPools, Error as PoolsError, Event as PoolsEvent, LastPoolId, PoolMember,
PoolMembers, PoolState,
PoolMembers, PoolState, TotalValueLocked,
};
use pallet_staking::{
CurrentEra, Error as StakingError, Event as StakingEvent, Payee, RewardDestination,
Expand Down
25 changes: 25 additions & 0 deletions substrate/frame/staking/src/pallet/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1856,6 +1856,7 @@
validator == *who || exposure_page.others.iter().any(|i| i.who == *who)
})
}

fn status(
who: &Self::AccountId,
) -> Result<sp_staking::StakerStatus<Self::AccountId>, DispatchError> {
Expand All @@ -1880,6 +1881,30 @@
}
}

fn force_withdraw(stash: &Self::AccountId, amount: Self::Balance) -> DispatchResult {
ensure!(
amount > Zero::zero() && amount < T::Currency::minimum_balance(),

Check failure on line 1886 in substrate/frame/staking/src/pallet/impls.rs

View workflow job for this annotation

GitHub Actions / cargo-check-all-crate-macos

no function or associated item named `minimum_balance` found for associated type `<T as pallet::pallet::Config>::Currency` in the current scope
Error::<T>::InvalidWithdrawAmount,
);

let mut ledger = Self::ledger(Stash(stash.clone()))?;
ledger.active = ledger.active.saturating_sub(amount);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be you should look to withdraw first from ledger.unlocking chunks before trying to withdraw from active?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it affect other members who're unbonding funds? Let's say

ED = 100, MIN_BOND = ED
A deposits 500 and unbonds 100
B deposits 100
next era, slash happens (so the 100 unlocking fund isn't affected right?)

obviously, B will be reaped

assuming no one unbonds after the slash
if we withdraw first from unlocking, then when the fund is fully unlocked
what's left would be less than what A would expect to withdraw (100 - x) right?

Do you mean we withdraw from unlocking and unbond more to make up for the portion withdrawn from the unlocking? Then wouldn't it length the unlock period?

ensure!(ledger.active >= T::Currency::minimum_balance(), Error::<T>::InsufficientBond);

Check failure on line 1892 in substrate/frame/staking/src/pallet/impls.rs

View workflow job for this annotation

GitHub Actions / cargo-check-all-crate-macos

no function or associated item named `minimum_balance` found for associated type `<T as pallet::pallet::Config>::Currency` in the current scope
ledger.total = ledger.total.saturating_sub(amount);
ledger.update()?;

if T::VoterList::contains(stash) {
let _ = T::VoterList::on_update(stash, Self::weight_of(stash)).defensive();
}

// Delegation relies on the `on_withdraw` event to increase agent's unclaimed
// withdrawals. We do this to maintain the ledger integrity.
T::EventListeners::on_withdraw(stash, amount);
Self::deposit_event(Event::<T>::Withdrawn { stash: stash.clone(), amount });

Ok(())
}

/// Whether `who` is a virtual staker whose funds are managed by another pallet.
fn is_virtual_staker(who: &T::AccountId) -> bool {
VirtualStakers::<T>::contains_key(who)
Expand Down
2 changes: 2 additions & 0 deletions substrate/frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -886,6 +886,8 @@ pub mod pallet {
/// governance (see `MinValidatorBond` and `MinNominatorBond`). If unbonding is the
/// intention, `chill` first to remove one's role as validator/nominator.
InsufficientBond,
/// Invalid amount to withdraw.
InvalidWithdrawAmount,
/// Can not schedule more unlock chunks.
NoMoreChunks,
/// Can not rebond without unlocking chunks.
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7995,7 +7995,7 @@ mod ledger_recovery {
assert_eq!(Balances::balance_locked(crate::STAKING_ID, &333), lock_333_before); // OK
assert_eq!(Bonded::<Test>::get(&333), Some(444)); // OK
assert!(Payee::<Test>::get(&333).is_some()); // OK
// however, ledger associated with its controller was killed.
// however, ledger associated with its controller was killed.
assert!(Ledger::<Test>::get(&444).is_none()); // NOK

// side effects on 444 - ledger, bonded, payee, lock should be completely removed.
Expand Down
3 changes: 3 additions & 0 deletions substrate/primitives/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,9 @@ pub trait StakingInterface {
/// Returns the fraction of the slash to be rewarded to reporter.
fn slash_reward_fraction() -> Perbill;

/// Immediately withdraw and burn the given amount of bonded fund from `stash`.
fn force_withdraw(stash: &Self::AccountId, amount: Self::Balance) -> DispatchResult;

#[cfg(feature = "runtime-benchmarks")]
fn max_exposure_page_size() -> Page;

Expand Down
Loading