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

Pools: Make PermissionlessWithdraw the default claim permission #3438

Merged
merged 61 commits into from
Apr 1, 2024

Conversation

rossbulat
Copy link

@rossbulat rossbulat commented Feb 22, 2024

Related Issue #3398

This PR makes permissionless withdrawing the default option, giving any network participant access to claim pool rewards on member's behalf. Of course, members can still opt out of this by setting a Permissioned claim permission.

Permissionless claiming has been a part of the nomination pool pallet for around 9 months now, with very limited uptake (~4% of total pool members). 1.6% of pool members are using PermissionlessAll, strongly suggesting it is not wanted - it is too ambiguous and doesn't provide guidance to claimers.

Stakers expect rewards to be claimed on their behalf by default - I have expanded upon this in detail within the accompanying issue's discussion. Other protocols have this behaviour, whereby staking rewards are received without the staker having to take any action. From this perspective, permissionless claiming is not intuitive for pool members. As evidence of this, over 150,000 DOT is currently unclaimed on Polkadot, and is growing at a non-linear rate.

@rossbulat rossbulat added I5-enhancement An additional feature request. T2-pallets This PR/Issue is related to a particular pallet. labels Feb 22, 2024
@rossbulat rossbulat changed the title Pools Make PermissionlessWithdraw the default claim permission Pools: Make PermissionlessWithdraw the default claim permission Feb 22, 2024
@rossbulat rossbulat marked this pull request as ready for review February 22, 2024 08:07
@rossbulat rossbulat requested review from Ank4n and gpestana and removed request for Ank4n February 22, 2024 09:04
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.

Had a quick look and added some nits but do not see anything wrong functionally.

While personally I feel the defaults are okay, would be good to get more eyeballs on this issue/change.

substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 462 to 466
impl Default for ClaimPermission {
fn default() -> Self {
Self::PermissionlessWithdraw
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it is not an obvious default, my opinion would be to remove the default impl and explicitly use the variant where needed.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I understand the ambiguity - the code is explicitly stating this is the default value so I think it looks quite obvious to the keen pallet observer. I think coming from nominators also it is intuitively the same behaviour, e.g. anyone can trigger a validator payout in the same manner as anyone can claim an unclaimed reward in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO obvious defaults are like false, zero but anything else should ideally not have defaults. Since its a value query it will default to this value if the key is missing and I generally think its better to be explicit as much as we can.

Though its just my opinion and if you disagree and no one else objects, I am fine with this.

@gupnik
Copy link
Contributor

gupnik commented Feb 26, 2024

Changing the default makes sense to me, but I am not completely sure if the removal of PermissionlessAll variant achieves the objective here.

I think it's discussed a bit in the linked issue around potential ambiguity but shouldn't we try to clarify that, instead of removing it altogether?

@rossbulat
Copy link
Author

Changing the default makes sense to me, but I am not completely sure if the removal of PermissionlessAll variant achieves the objective here.

I think it's discussed a bit in the linked issue around potential ambiguity but shouldn't we try to clarify that, instead of removing it altogether?

The problem is that there is no real way to clarify what pool members actually want to do - the only way is to change to PermissionlessWithdraw or PermissionlessCompound, which then defeats the purpose of PermissionlessAll. The uptake of 1.6% of pool members is worth stressing again - we don't want to consume pallet logic and storage with unused features. I think with the default switch PermissionlessAll is further rendered obsolete - why would it be chosen if members are already receiving rewards as free balance.

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.

Code still needs updating right? please re-request review once done.

FWIW, I am in favor of also changing join() to require the claim permission to be specified upon joining.

For all existing members, we reside to PermissionlessWithdraw as a middle ground.

@rossbulat
Copy link
Author

Code still needs updating right? please re-request review once done.

FWIW, I am in favor of also changing join() to require the claim permission to be specified upon joining.

For all existing members, we reside to PermissionlessWithdraw as a middle ground.

Thanks @kianenigma, It looks like we are good to go now. I'm in agreement that join should also require a claim permission. Perhaps we could explore an additional call for this in another PR.

@rossbulat rossbulat enabled auto-merge April 1, 2024 07:22
@rossbulat rossbulat added this pull request to the merge queue Apr 1, 2024
Merged via the queue into master with commit b772cb5 Apr 1, 2024
129 of 132 checks passed
@rossbulat rossbulat deleted the rb-claim-permission-default-change branch April 1, 2024 09:58
pgherveou pushed a commit that referenced this pull request Apr 2, 2024
)

Related Issue #3398

This PR makes permissionless withdrawing the default option, giving any
network participant access to claim pool rewards on member's behalf. Of
course, members can still opt out of this by setting a `Permissioned`
claim permission.

Permissionless claiming has been a part of the nomination pool pallet
for around 9 months now, with very limited uptake (~4% of total pool
members). 1.6% of pool members are using `PermissionlessAll`, strongly
suggesting it is not wanted - it is too ambiguous and doesn't provide
guidance to claimers.

Stakers expect rewards to be claimed on their behalf by default - I have
expanded upon this in detail within the [accompanying issue's
discussion](#3398).
Other protocols have this behaviour, whereby staking rewards are
received without the staker having to take any action. From this
perspective, permissionless claiming is not intuitive for pool members.
As evidence of this, over 150,000 DOT is currently unclaimed on
Polkadot, and is growing at a non-linear rate.
Ank4n pushed a commit that referenced this pull request Apr 9, 2024
)

Related Issue #3398

This PR makes permissionless withdrawing the default option, giving any
network participant access to claim pool rewards on member's behalf. Of
course, members can still opt out of this by setting a `Permissioned`
claim permission.

Permissionless claiming has been a part of the nomination pool pallet
for around 9 months now, with very limited uptake (~4% of total pool
members). 1.6% of pool members are using `PermissionlessAll`, strongly
suggesting it is not wanted - it is too ambiguous and doesn't provide
guidance to claimers.

Stakers expect rewards to be claimed on their behalf by default - I have
expanded upon this in detail within the [accompanying issue's
discussion](#3398).
Other protocols have this behaviour, whereby staking rewards are
received without the staker having to take any action. From this
perspective, permissionless claiming is not intuitive for pool members.
As evidence of this, over 150,000 DOT is currently unclaimed on
Polkadot, and is growing at a non-linear rate.
dharjeezy pushed a commit to dharjeezy/polkadot-sdk that referenced this pull request Apr 9, 2024
…ritytech#3438)

Related Issue paritytech#3398

This PR makes permissionless withdrawing the default option, giving any
network participant access to claim pool rewards on member's behalf. Of
course, members can still opt out of this by setting a `Permissioned`
claim permission.

Permissionless claiming has been a part of the nomination pool pallet
for around 9 months now, with very limited uptake (~4% of total pool
members). 1.6% of pool members are using `PermissionlessAll`, strongly
suggesting it is not wanted - it is too ambiguous and doesn't provide
guidance to claimers.

Stakers expect rewards to be claimed on their behalf by default - I have
expanded upon this in detail within the [accompanying issue's
discussion](paritytech#3398).
Other protocols have this behaviour, whereby staking rewards are
received without the staker having to take any action. From this
perspective, permissionless claiming is not intuitive for pool members.
As evidence of this, over 150,000 DOT is currently unclaimed on
Polkadot, and is growing at a non-linear rate.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants