Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

permissionless bond_extra in nomination pools #12608

Merged
merged 63 commits into from
Feb 22, 2023

Conversation

Doordashcon
Copy link
Contributor

@Doordashcon Doordashcon commented Nov 3, 2022

Resolves #11638

polkadot address: 12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h

polkadot companion: paritytech/polkadot#6753

@Doordashcon Doordashcon changed the title Allow pool operators to manage claimable fund Allow pool operators bond_extra Nov 7, 2022
@Doordashcon
Copy link
Contributor Author

Doordashcon commented Nov 9, 2022

Ideally this action should not be at any cost to pool operators?

Asked this because I had no idea incentives were not offered to pool operators.

@Doordashcon Doordashcon marked this pull request as ready for review November 13, 2022 12:04
// TODO: For when the pool member leaves or is kiked out, clear storage.
// Update weight info
#[pallet::weight(T::DbWeight::get().reads_writes(1, 1))]
pub fn set_claimable_action(origin: OriginFor<T>, action: bool) -> DispatchResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Action should be an enum with better names about what each variant mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

And then impl Default for Action should be whatever it is the current behavior, so that we won't need any migration.

Copy link
Contributor Author

@Doordashcon Doordashcon Nov 15, 2022

Choose a reason for hiding this comment

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

Making it an enum would be preferred because this makes it into a binary state and more actions can be added if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so that we won't need any migration.

I am curious as to why we should avoid making migrations in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a member unbond should that change action back to default?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious as to why we should avoid making migrations in this case?

If you have a map for user's desired behavior, then you won't need a migration.
If you want to add this as a new filed to the Member, which is arguably the more "neat" way of doing this, then you would need a migration.

For now, the map approach is fine. But if you are down for it, I am down to experiment with the alternative as well.

When a member unbond should that change action back to default?

Can you elaborate on the scenario? I don't think unbond would have anything to do with this. When the member fully unbonds and leaves the system, then their entry in this map should also be removed.

@kianenigma
Copy link
Contributor

kianenigma commented Nov 20, 2022

We discussed this internally and our interim opinion is to not add a new role for this, and not attach it to root either.

If a user allows this, ANYONE should be able to claim their rewards. WDYT @Doordashcon?

(we will soon post this in the polkadot forum as well to get more feedback from the existing pool operators.)

@Doordashcon
Copy link
Contributor Author

Doordashcon commented Nov 23, 2022

(we will soon post this in the polkadot forum as well to get more feedback from the existing pool operators.)

Sounds good @kianenigma i'll wait till it's up on the forum before implementing changes.

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/roles-in-nomination-pools/1169/5

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.

I think we can move forward with this. Needs a few renames and tweaks, but in general is looking good.

@kianenigma kianenigma added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Dec 4, 2022
@rossbulat
Copy link

bot fmt

@command-bot
Copy link

command-bot bot commented Feb 21, 2023

@rossbulat https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2423241 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/substrate/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 16-aecb558a-8df6-43ca-bd19-a38e048ce880 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Feb 21, 2023

@rossbulat Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2423241 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2423241/artifacts/download.

@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Feb 21, 2023
@rossbulat
Copy link

bot rebase

@paritytech-processbot
Copy link

Rebased

@rossbulat
Copy link

bot rebase

@paritytech-processbot
Copy link

Rebased

@rossbulat
Copy link

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 473b5d0 into paritytech:master Feb 22, 2023
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* create enum

* logic check

* add benchmarks

* -enum

* update

* bond extra other

* update

* update

* update

* cargo fmt

* Permissioned

* update

* cargo fmt

* update

* update index

* doc update

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

* doc update

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* cargo fmt

* bond_extra auto compound

* bond_extra_other

* Apply suggestions from code review

* Fixes from kian

* updates docs & test

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* fixes + fmt

* expand ClaimPermissions + add benchmarks

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nomination_pools

* tidy up claim payout benches

* fix

* + test: claim_payout_other_works

* comments, rename to set_claim_permission

* fix comment

* remove ClaimPermission on leave pool

* fix test

* ".git/.scripts/commands/fmt/fmt.sh"

* + test for ClaimPermissions::remove()

* impl can_bond_extra & can_claim_payout

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: parity-processbot <>
Co-authored-by: Ross Bulat <ross@parity.io>
Ank4n added a commit that referenced this pull request Feb 28, 2023
* create enum

* logic check

* add benchmarks

* -enum

* update

* bond extra other

* update

* update

* update

* cargo fmt

* Permissioned

* update

* cargo fmt

* update

* update index

* doc update

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

* doc update

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* cargo fmt

* bond_extra auto compound

* bond_extra_other

* Apply suggestions from code review

* Fixes from kian

* updates docs & test

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* fixes + fmt

* expand ClaimPermissions + add benchmarks

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nomination_pools

* tidy up claim payout benches

* fix

* + test: claim_payout_other_works

* comments, rename to set_claim_permission

* fix comment

* remove ClaimPermission on leave pool

* fix test

* ".git/.scripts/commands/fmt/fmt.sh"

* + test for ClaimPermissions::remove()

* impl can_bond_extra & can_claim_payout

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: parity-processbot <>
Co-authored-by: Ross Bulat <ross@parity.io>
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/discussion-on-global-maximum-commission-for-nomination-pools-on-polkadot/2325/1

ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* create enum

* logic check

* add benchmarks

* -enum

* update

* bond extra other

* update

* update

* update

* cargo fmt

* Permissioned

* update

* cargo fmt

* update

* update index

* doc update

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

* doc update

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* cargo fmt

* bond_extra auto compound

* bond_extra_other

* Apply suggestions from code review

* Fixes from kian

* updates docs & test

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* fixes + fmt

* expand ClaimPermissions + add benchmarks

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nomination_pools

* tidy up claim payout benches

* fix

* + test: claim_payout_other_works

* comments, rename to set_claim_permission

* fix comment

* remove ClaimPermission on leave pool

* fix test

* ".git/.scripts/commands/fmt/fmt.sh"

* + test for ClaimPermissions::remove()

* impl can_bond_extra & can_claim_payout

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: parity-processbot <>
Co-authored-by: Ross Bulat <ross@parity.io>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* create enum

* logic check

* add benchmarks

* -enum

* update

* bond extra other

* update

* update

* update

* cargo fmt

* Permissioned

* update

* cargo fmt

* update

* update index

* doc update

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>

* doc update

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* cargo fmt

* bond_extra auto compound

* bond_extra_other

* Apply suggestions from code review

* Fixes from kian

* updates docs & test

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/tests.rs

* Update frame/nomination-pools/src/lib.rs

* Update frame/nomination-pools/src/tests.rs

* fixes + fmt

* expand ClaimPermissions + add benchmarks

* ".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nomination_pools

* tidy up claim payout benches

* fix

* + test: claim_payout_other_works

* comments, rename to set_claim_permission

* fix comment

* remove ClaimPermission on leave pool

* fix test

* ".git/.scripts/commands/fmt/fmt.sh"

* + test for ClaimPermissions::remove()

* impl can_bond_extra & can_claim_payout

---------

Co-authored-by: Ankan <10196091+Ank4n@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>
Co-authored-by: parity-processbot <>
Co-authored-by: Ross Bulat <ross@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

nomination-pools: allow pool operators to manage claimable funds
10 participants