-
Notifications
You must be signed in to change notification settings - Fork 707
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
[NPoS] When pool contribution of a pool member goes below ED, it should be reaped. #5009
Comments
## Context Pool members using the old `TransferStake` strategy were able to transfer all their funds to the pool. With `DelegateStake` changes, we want to ensure similar behaviour by allowing members to delegate all their stake to the pool. ## Changes - Ensure all the balance including ED of an account can be delegated (and used in the pool) by adding a provider for delegators. - Gates calls that mutates the pool or pool member if they are in unmigrated state. Closes paritytech-secops/srlabs_findings#409. - Adds remote test to migrate all pools and members to `DelegateStake` which can be used with `Kusama` and `Polkadot` runtime state. closes #4629. - Add new runtime apis to read pool and member balance. ## Addressing possible migration errors Pool members migrating can run into two types of errors: - Already Staking: If the pool member is already staking, we cannot migrate them to `DelegateStake` since this may mean they are able to use the same staked funds in the pool. Users would need to withdraw all their funds from staking, in order to migrate their pool funds. - Pool contribution below ED: For these cases transfer from pool account to member account would fail. The affected users can top up their accounts and redo migration. Another error that was earlier possible was when member's free balance is below ED. This PR adds a provider to delegator allowing all user balance including ED can be contributed towards the pool. This helps `1095` accounts in Polkadot and `41` accounts in Kusama to migrate now which would have earlier failed. ## Results from RemoteExternalities Tests. ### Kusama `Migration stats: success: 3017, direct_stakers: 361, unexpected_errors: 0` ### Polkadot `Migration stats: success: 42859, direct_stakers: 643, unexpected_errors: 0` ## TODO - [x] Add runtime api for member total balance. - [x] New [issue](#5009) to reap pool members with contribution below ED. - [x] Add provider for delegators so whole balance including ED can be held while contributing to pools. - [x] Gate all pool extrinsics if pool/member is in non-migrated state. --------- Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
I’ll try to paraphrase your words, please correct me if misunderstand anything, coz some of the terms are not as straightforward to outsider like me. :)) There should be a way such that if the contribution (in terms of stake) of a member somehow goes below the existential deposit (however much on that network), they’ll be reaped, meaning to be removed from the pool. Correct? |
We need to add a permissionless call to Nomination Pools to reap a pool member. Reaping should be only possible if member contribution is below ED (can happen in case of slashing). When you reap a pool member, it should do some additional cleanup,
Don't hesitate to ask any more clarifying questions if you end up taking this issue. :) |
Hi @Ank4n , is this still open? I will have a go at it then. |
sorry I kinda forgot about this. I just have something drafted up. not sure if it's on the right direction though |
@eagr ahh okay didn't realise you have started working on this. I will have a look shortly. |
@eagr I have put some additional context in the description. I am also realising it has some subtle logic that requires knowledge of pool and delegation system. The issue is probably not as straight forward anymore but happy to help you if you are still keen to fix it. :) |
@Ank4n Sure, sounds like a great opportunity to learn about staking/delegation! I'm going through the docs and codebase and accumulating questions. Will get back to you soon. Btw, have a nice weekend. :) |
If When one joins a pool,
I suppose we kinda need to do the opposite in the reversed order. I could see the pallets in question are loosely coupled via the composition of the traits below. Below is a very rough idea for the extrinsic to trigger actions in those staking pallets. impl StakingInterface for pallet_staking::Pallet {
// with `dust_bonded()` added to the trait
fn dust_bonded() {
// update account ledger
// burn (drop) balance
}
}
impl DelegationInterface for pallet_nomination_pools::Pallet {
fn dust_delegation() {
}
}
impl StakeStrategy for TransferStake {
// with `member_dust()` added to the trait
// going along with other names in the trait:
// member_withdraw, member_slash
fn member_dust() -> DispatchResult {
// ...
Staking::dust_bonded();
// ...
}
}
impl StakeStrategy for DelegateStake {
fn member_dust() -> DispatchResult {
// ...
Staking::dust_bonded();
Delegation::dust_delegation();
// ...
}
}
// the extrinsic
fn dust() {
// ..., if member active balance < ED
T::StakeAdapter::member_dust();
// ...
} Any suggestions? |
Hey @eagr ,
absolutely correct. this will only happen in the event of slashing. This shall not be included in the regular slashing operation itself because we want to minimize the amount of computation it needs to do in one block. We basically clean up after the slash. We want to do it this way to make sure that we don't exceed the limits of computation that can be done within one block in case a slashing event occurs. |
You raise a good point. The extrinsic was probably needed for members with the legacy
Not sure about the trait definition you shared. From |
Updated the PR to achieve what you described here to the best of my knowledge. Still not quite sure if I'm on the right track tough. I left a few questions in the code prefixed by In the PR, for legacy pools with Can the pool creator be reaped if their active stake goes below ED? If so, what should happen to the pool? |
We should try to keep the same behaviour for both adapters. I can think of two choices (it can be configurable as well)
Either of the options are fine, with a little higher preference for
If pool is not in |
There are cases where because of slashing, nomination pool members have contribution less than ED. When this happens, we should have a way to reap these accounts. This could be exposed via a permissionless extrinsic.
Things it should do
pallet-nomination-pools
.pallet-delegated-staking
pallet-staking
. This should never exceed ED.How I see this to be implemented
reap member
. It should only execute if pool contribution of member is below ED.pallet-staking
. Force withdraw that amount from pallet-staking and burn it. This should be similar to how balance pallet handles dust (seefungible::Unbalanced::handle_dust
). This should also be removed fromStakingLedger.active
.delegator
(in pallet-delegated-staking) and pool member. This logic would be somewhat similar to what happens inpool::withdraw_unbonded
when all member funds are withdrawn.Additionally, this should also ensure when the pool is destroyed, the pool_account is reaped from
pallet-staking
. If the pool is already destroyed, but it exists inpallet-staking
, it should get reaped as well.The text was updated successfully, but these errors were encountered: