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

nomination-pools: add permissionless condition to chill #3453

Merged
merged 22 commits into from
Mar 7, 2024

Conversation

dastansam
Copy link
Contributor

@dastansam dastansam commented Feb 23, 2024

Currently, pool member funds cannot be unbonded if the depositor's stake is less than MinNominatorBond. This usually happens after T::MinNominatorBond is increased.

To fix this, the above mentioned condition is added as a case for permissionless dispatch of chill. After pool is chilled, pool members can unbond their funds since pool won't be nominating anymore.

Consequently, same check is added to nominate call, i.e pool can not start nominating if it's depositor does not have MinNominatorBond

cc @Ank4n @kianenigma

closes #2350

Polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

@dastansam dastansam marked this pull request as ready for review February 23, 2024 00:31
@Ank4n Ank4n added the T2-pallets This PR/Issue is related to a particular pallet. label Feb 23, 2024
Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and it is looking pretty good.

Great that you added the integration test (test-staking) as well. I would advise though to instead add a separate integration test and simulate also what happens after the pool is chilled. An open question that I have, should we put the pool in blocked state when depositor bond is below min nominator bond?
Wdyt @rossbulat?

substrate/frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/test-staking/src/lib.rs Outdated Show resolved Hide resolved
@dastansam dastansam changed the title Add permissionless condition to chill nomination-pools: Add permissionless condition to chill Feb 23, 2024
@dastansam dastansam changed the title nomination-pools: Add permissionless condition to chill nomination-pools: add permissionless condition to chill Feb 23, 2024
@dastansam
Copy link
Contributor Author

dastansam commented Feb 23, 2024

Thanks for the PR and it is looking pretty good.

Great that you added the integration test (test-staking) as well. I would advise though to instead add a separate integration test and simulate also what happens after the pool is chilled. An open question that I have, should we put the pool in blocked state when depositor bond is below min nominator bond? Wdyt @rossbulat?

I think it's not necessary to put it into blocked state, since the permissioned version of chill does not do it. And I don't see that other extrinsics have different behaviour depending on the nature (permissioned/less) of the call, or maybe I am missing it?

@dastansam
Copy link
Contributor Author

Thanks for the PR and it is looking pretty good.

Great that you added the integration test (test-staking) as well. I would advise though to instead add a separate integration test and simulate also what happens after the pool is chilled. An open question that I have, should we put the pool in blocked state when depositor bond is below min nominator bond? Wdyt @rossbulat?

I isolated the chill lifecycle into a separate test. Upon testing the behaviour, I found out that nominate should also enforce depositor to have the MinNominatorBond. Otherwise, if any member joins after pool is chilled, and his stake makes the total stake of the bonded_account more than MinNominatorBond, root/nominator of the pool can simply resume the nomination, even if the depositor does not have MinNominatorBond.

@dastansam dastansam requested a review from Ank4n February 23, 2024 18:50
@dastansam
Copy link
Contributor Author

hey @Ank4n,
I resolved your comments

@Ank4n
Copy link
Contributor

Ank4n commented Feb 28, 2024

I think it's not necessary to put it into blocked state, since the permissioned version of chill does not do it. And I don't see that other extrinsics have different behaviour depending on the nature (permissioned/less) of the call, or maybe I am missing it?

Yeah we should not do it in the chill but just conceptually if we do not block it, another user could end up joining this pool and not receive any rewards. But this is a problem frontends can solve better. For the current issue, I believe this solution is good enough.

@Ank4n
Copy link
Contributor

Ank4n commented Feb 28, 2024

I isolated the chill lifecycle into a separate test. Upon testing the behaviour, I found out that nominate should also enforce depositor to have the MinNominatorBond. Otherwise, if any member joins after pool is chilled, and his stake makes the total stake of the bonded_account more than MinNominatorBond, root/nominator of the pool can simply resume the nomination, even if the depositor does not have MinNominatorBond.

If the pool has more than MinNominatorBond it could be allowed to nominate since it will be eligible for reward. As long as the member can get out if they want by chilling pool, its probably okay. But we also want to punish the pool operator if they have not matched up the MinNominatorBond so keeping it chilled is also a decent option. I am okay with either option in favour of not lingering this too long.

Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Nice extra tests. Thanks

substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
@command-bot
Copy link

command-bot bot commented Feb 29, 2024

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5392171 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_nomination_pools. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-e4c983ee-5f92-4691-a28e-e47462995236 to cancel this command or bot cancel to cancel all commands in this pull request.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thank you!

/tip medium

@kianenigma
Copy link
Contributor

/tip medium

Copy link

@dastansam Invalid network: "DOT". Please select one of: polkadot, kusama, rococo, westend.

@command-bot
Copy link

command-bot bot commented Feb 29, 2024

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_nomination_pools has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5392171 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5392171/artifacts/download.

@liamaharon
Copy link
Contributor

/tip medium

Copy link

@liamaharon You are not allowed to request a tip. Only members of paritytech/tip-bot-approvers are allowed.

@mordamax
Copy link
Contributor

/tip medium

Copy link

@mordamax A referendum for a medium (80 DOT) tip was successfully submitted for @dastansam (16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/referenda tip

@Ank4n Ank4n added this pull request to the merge queue Mar 1, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 1, 2024
@Ank4n
Copy link
Contributor

Ank4n commented Mar 7, 2024

bot bench substrate-pallet --pallet=pallet_nomination_pools

@command-bot
Copy link

command-bot bot commented Mar 7, 2024

@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5460998 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_nomination_pools. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-0119392a-8502-4774-95f6-2d85e9bdeff6 to cancel this command or bot cancel to cancel all commands in this pull request.

…=dev --target_dir=substrate --pallet=pallet_nomination_pools
@command-bot
Copy link

command-bot bot commented Mar 7, 2024

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=dev --target_dir=substrate --pallet=pallet_nomination_pools has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5460998 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5460998/artifacts/download.

@Ank4n Ank4n added this pull request to the merge queue Mar 7, 2024
Merged via the queue into paritytech:master with commit 11831df Mar 7, 2024
129 of 130 checks passed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…ch#3453)

Currently, pool member funds cannot be unbonded if the depositor's stake
is less than `MinNominatorBond`. This usually happens after
`T::MinNominatorBond` is increased.

To fix this, the above mentioned condition is added as a case for
permissionless dispatch of `chill`. After pool is chilled, pool members
can unbond their funds since pool won't be nominating anymore.

Consequently, same check is added to `nominate` call, i.e pool can not
start nominating if it's depositor does not have `MinNominatorBond`

cc @Ank4n @kianenigma 

closes paritytech#2350

Polkadot address: 16FqwPZ8GRC5U5D4Fu7W33nA55ZXzXGWHwmbnE1eT6pxuqcT

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Nomination pools cannot chill if depositor bond is less than MinNominatorBond
6 participants