Skip to content

Commit

Permalink
Pools: Make PermissionlessWithdraw the default claim permission (#3438
Browse files Browse the repository at this point in the history
)

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.
  • Loading branch information
Ross Bulat authored and pgherveou committed Apr 2, 2024
1 parent bc18f9f commit 3442711
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 42 deletions.
13 changes: 13 additions & 0 deletions prdoc/pr_3438.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
title: "Pools: Make PermissionlessWithdraw the default claim permission"

doc:
- audience: Runtime User
description: |
Makes permissionless withdrawing the default claim permission, giving any network participant
access to claim pool rewards on member's behalf, by default.

crates:
- name: pallet-nomination-pools
bump: minor
- name: pallet-nomination-pools-benchmarking
bump: minor
4 changes: 2 additions & 2 deletions substrate/frame/nomination-pools/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -795,9 +795,9 @@ frame_benchmarking::benchmarks! {
T::Staking::active_stake(&pool_account).unwrap(),
min_create_bond + min_join_bond
);
}:_(RuntimeOrigin::Signed(joiner.clone()), ClaimPermission::PermissionlessAll)
}:_(RuntimeOrigin::Signed(joiner.clone()), ClaimPermission::Permissioned)
verify {
assert_eq!(ClaimPermissions::<T>::get(joiner), ClaimPermission::PermissionlessAll);
assert_eq!(ClaimPermissions::<T>::get(joiner), ClaimPermission::Permissioned);
}

claim_commission {
Expand Down
31 changes: 16 additions & 15 deletions substrate/frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,22 +461,26 @@ pub enum ClaimPermission {
PermissionlessAll,
}

impl Default for ClaimPermission {
fn default() -> Self {
Self::PermissionlessWithdraw
}
}

impl ClaimPermission {
/// Permissionless compounding of pool rewards is allowed if the current permission is
/// `PermissionlessCompound`, or permissionless.
fn can_bond_extra(&self) -> bool {
matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessCompound)
}

/// Permissionless payout claiming is allowed if the current permission is
/// `PermissionlessWithdraw`, or permissionless.
fn can_claim_payout(&self) -> bool {
matches!(self, ClaimPermission::PermissionlessAll | ClaimPermission::PermissionlessWithdraw)
}
}

impl Default for ClaimPermission {
fn default() -> Self {
Self::Permissioned
}
}

/// A member in a pool.
#[derive(
Encode,
Expand Down Expand Up @@ -2630,7 +2634,7 @@ pub mod pallet {
///
/// In the case of `origin != other`, `origin` can only bond extra pending rewards of
/// `other` members assuming set_claim_permission for the given member is
/// `PermissionlessAll` or `PermissionlessCompound`.
/// `PermissionlessCompound` or `PermissionlessAll`.
#[pallet::call_index(14)]
#[pallet::weight(
T::WeightInfo::bond_extra_transfer()
Expand All @@ -2648,15 +2652,10 @@ pub mod pallet {
/// Allows a pool member to set a claim permission to allow or disallow permissionless
/// bonding and withdrawing.
///
/// By default, this is `Permissioned`, which implies only the pool member themselves can
/// claim their pending rewards. If a pool member wishes so, they can set this to
/// `PermissionlessAll` to allow any account to claim their rewards and bond extra to the
/// pool.
///
/// # Arguments
///
/// * `origin` - Member of a pool.
/// * `actor` - Account to claim reward. // improve this
/// * `permission` - The permission to be applied.
#[pallet::call_index(15)]
#[pallet::weight(T::DbWeight::get().reads_writes(1, 1))]
pub fn set_claim_permission(
Expand All @@ -2666,16 +2665,18 @@ pub mod pallet {
let who = ensure_signed(origin)?;

ensure!(PoolMembers::<T>::contains_key(&who), Error::<T>::PoolMemberNotFound);

ClaimPermissions::<T>::mutate(who, |source| {
*source = permission;
});

Ok(())
}

/// `origin` can claim payouts on some pool member `other`'s behalf.
///
/// Pool member `other` must have a `PermissionlessAll` or `PermissionlessWithdraw` in order
/// for this call to be successful.
/// Pool member `other` must have a `PermissionlessWithdraw` or `PermissionlessAll` claim
/// permission for this call to be successful.
#[pallet::call_index(16)]
#[pallet::weight(T::WeightInfo::claim_payout())]
pub fn claim_payout_other(origin: OriginFor<T>, other: T::AccountId) -> DispatchResult {
Expand Down
39 changes: 14 additions & 25 deletions substrate/frame/nomination-pools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2441,16 +2441,10 @@ mod claim_payout {
// given
assert_eq!(Currency::free_balance(&10), 35);

// Permissioned by default
assert_noop!(
Pools::claim_payout_other(RuntimeOrigin::signed(80), 10),
Error::<Runtime>::DoesNotHavePermission
);
// when

assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(10),
ClaimPermission::PermissionlessWithdraw
));
// NOTE: Claim permission of `PermissionlessWithdraw` allows payout claiming as default,
// so a claim permission does not need to be set for non-pool members prior to claiming.
assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10));

// then
Expand Down Expand Up @@ -2489,7 +2483,6 @@ mod unbond {
);

// Make permissionless
assert_eq!(ClaimPermissions::<Runtime>::get(10), ClaimPermission::Permissioned);
assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(20),
ClaimPermission::PermissionlessAll
Expand Down Expand Up @@ -4563,12 +4556,11 @@ mod withdraw_unbonded {
CurrentEra::set(1);
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().points, 20);

assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(20),
ClaimPermission::PermissionlessAll
));
assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 20));
assert_eq!(ClaimPermissions::<Runtime>::get(20), ClaimPermission::PermissionlessAll);
assert_eq!(
ClaimPermissions::<Runtime>::get(20),
ClaimPermission::PermissionlessWithdraw
);

assert_eq!(
pool_events_since_last_call(),
Expand Down Expand Up @@ -4792,7 +4784,7 @@ mod create {
}

#[test]
fn set_claimable_actor_works() {
fn set_claim_permission_works() {
ExtBuilder::default().build_and_execute(|| {
// Given
Currency::set_balance(&11, ExistentialDeposit::get() + 2);
Expand All @@ -4811,22 +4803,19 @@ fn set_claimable_actor_works() {
]
);

// Make permissionless
assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::Permissioned);
// Make permissioned
assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::PermissionlessWithdraw);
assert_noop!(
Pools::set_claim_permission(
RuntimeOrigin::signed(12),
ClaimPermission::PermissionlessAll
),
Pools::set_claim_permission(RuntimeOrigin::signed(12), ClaimPermission::Permissioned),
Error::<T>::PoolMemberNotFound
);
assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(11),
ClaimPermission::PermissionlessAll
ClaimPermission::Permissioned
));

// then
assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::PermissionlessAll);
assert_eq!(ClaimPermissions::<Runtime>::get(11), ClaimPermission::Permissioned);
});
}

Expand Down Expand Up @@ -5224,7 +5213,7 @@ mod bond_extra {

assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(10),
ClaimPermission::PermissionlessAll
ClaimPermission::PermissionlessCompound
));
assert_ok!(Pools::bond_extra_other(RuntimeOrigin::signed(50), 10, BondExtra::Rewards));
assert_eq!(Currency::free_balance(&default_reward_account()), 7);
Expand Down

0 comments on commit 3442711

Please sign in to comment.