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
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
ac22eaf
add v9 migration
Feb 22, 2024
a3c99b2
Revert "add v9 migration"
Feb 22, 2024
aef4328
v9 migration
Feb 22, 2024
ee7d671
use OldClaimPermission
Feb 22, 2024
2022bb6
make `PermissionlessWithdraw` default
Feb 22, 2024
d767c17
don't store default
Feb 22, 2024
5bc0356
fix typo
Feb 22, 2024
36c6c97
fix comment
Feb 22, 2024
cf38a3f
add migration
Feb 22, 2024
c305dda
add prdoc
Feb 22, 2024
4077e3f
amend post_upgrade
Feb 22, 2024
af6a347
fix
Feb 22, 2024
d08910f
Merge branch 'master' into rb-claim-permission-default-change
Feb 22, 2024
ab4e5a3
Merge branch 'master' into rb-claim-permission-default-change
Feb 28, 2024
a8d72d4
action feedback
Feb 28, 2024
2e32747
bump storage version
Feb 28, 2024
b51c28d
amend post_upgrade
Feb 28, 2024
425e725
post_upgrade checks count
Feb 28, 2024
32c8ed3
u64
Feb 28, 2024
0c89e34
cast
Feb 28, 2024
d1d9119
Merge branch 'master' into rb-claim-permission-default-change
Feb 28, 2024
bdfe4ea
Merge branch 'master' into rb-claim-permission-default-change
Feb 28, 2024
43de3cc
Merge branch 'master' into rb-claim-permission-default-change
Feb 29, 2024
f5a75b1
audence update
Feb 29, 2024
a001fe7
Merge branch 'rb-claim-permission-default-change' of https://github.c…
Feb 29, 2024
b0afaa4
Merge branch 'master' into rb-claim-permission-default-change
Feb 29, 2024
b80571b
Merge branch 'master' into rb-claim-permission-default-change
Feb 29, 2024
8c6a654
Merge branch 'master' into rb-claim-permission-default-change
Feb 29, 2024
8bf1c2a
Merge branch 'master' into rb-claim-permission-default-change
Mar 1, 2024
b50b31a
Merge branch 'master' into rb-claim-permission-default-change
Mar 2, 2024
62d8979
Merge branch 'master' into rb-claim-permission-default-change
Mar 3, 2024
350c009
Merge branch 'master' into rb-claim-permission-default-change
Mar 5, 2024
0b6f0ad
Merge branch 'master' into rb-claim-permission-default-change
Mar 5, 2024
5493232
Merge branch 'master' into rb-claim-permission-default-change
Mar 5, 2024
387e9bd
Merge branch 'master' into rb-claim-permission-default-change
Mar 6, 2024
69d35b9
Merge branch 'master' into rb-claim-permission-default-change
Mar 7, 2024
56a24b9
Merge branch 'master' into rb-claim-permission-default-change
Mar 8, 2024
34d4807
remove migration
Mar 13, 2024
7c7cc7a
Merge branch 'rb-claim-permission-default-change' of https://github.c…
Mar 13, 2024
c354aca
Merge branch 'master' into rb-claim-permission-default-change
Mar 13, 2024
e0891a0
roll back storage version
Mar 13, 2024
65db056
Merge branch 'rb-claim-permission-default-change' of https://github.c…
Mar 13, 2024
e39539a
rm
Mar 14, 2024
682b334
fmt
Mar 14, 2024
8d6871d
Merge branch 'master' into rb-claim-permission-default-change
Mar 14, 2024
d800fcc
amend prdoc
Mar 15, 2024
767e539
Merge branch 'rb-claim-permission-default-change' of https://github.c…
Mar 15, 2024
2d1eec6
Merge branch 'master' into rb-claim-permission-default-change
Mar 21, 2024
35505c6
bring back PermissionlessAll
Mar 21, 2024
892e7ca
add comment
Mar 21, 2024
6c77e10
Merge branch 'master' into rb-claim-permission-default-change
Mar 21, 2024
83caea6
Merge branch 'master' into rb-claim-permission-default-change
Mar 22, 2024
a8041d1
Merge branch 'master' into rb-claim-permission-default-change
Mar 24, 2024
e10332a
Merge branch 'master' into rb-claim-permission-default-change
Mar 27, 2024
05e124f
Merge branch 'master' into rb-claim-permission-default-change
Mar 28, 2024
04b2d50
Merge branch 'master' into rb-claim-permission-default-change
Mar 31, 2024
31ad8a4
Merge branch 'master' into rb-claim-permission-default-change
Apr 1, 2024
80499a6
add bump
Apr 1, 2024
95ae56f
Merge branch 'rb-claim-permission-default-change' of https://github.c…
Apr 1, 2024
abe04ca
prdoc
Apr 1, 2024
0b1d09e
fix
Apr 1, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions prdoc/pr_3438.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
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
12 changes: 6 additions & 6 deletions substrate/frame/nomination-pools/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@ frame_benchmarking::benchmarks! {
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let extra = (scenario.dest_weight - origin_weight).max(CurrencyOf::<T>::minimum_balance());

// set claim preferences to `PermissionlessAll` to any account to bond extra on member's behalf.
let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessAll);
// set claim preferences to `PermissionlessCompound` to any account to bond extra on member's behalf.
let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), ClaimPermission::PermissionlessCompound);

// transfer exactly `extra` to the depositor of the src pool (1),
let reward_account1 = Pools::<T>::create_reward_account(1);
Expand All @@ -313,9 +313,9 @@ frame_benchmarking::benchmarks! {
// Send funds to the reward account of the pool
CurrencyOf::<T>::set_balance(&reward_account, ed + origin_weight);

// set claim preferences to `PermissionlessAll` so any account can claim rewards on member's
// set claim preferences to `PermissionlessWithdraw` so any account can claim rewards on member's
// behalf.
let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessAll);
let _ = Pools::<T>::set_claim_permission(RuntimeOrigin::Signed(depositor.clone()).into(), ClaimPermission::PermissionlessWithdraw);

// Sanity check
assert_eq!(
Expand Down 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
37 changes: 18 additions & 19 deletions substrate/frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,23 +457,25 @@ pub enum ClaimPermission {
PermissionlessCompound,
/// Anyone can withdraw rewards on a pool member's behalf.
PermissionlessWithdraw,
/// Anyone can withdraw and compound rewards on a pool member's behalf.
PermissionlessAll,
}

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.


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

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

impl Default for ClaimPermission {
fn default() -> Self {
Self::Permissioned
matches!(self, ClaimPermission::PermissionlessWithdraw)
}
}

Expand Down Expand Up @@ -2630,7 +2632,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`.
#[pallet::call_index(14)]
#[pallet::weight(
T::WeightInfo::bond_extra_transfer()
Expand All @@ -2648,15 +2650,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 +2663,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` 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
48 changes: 13 additions & 35 deletions substrate/frame/nomination-pools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2441,16 +2441,9 @@ 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
));
// Claim permission of `PermissionlessWithdraw` allows payout claiming as default.
assert_ok!(Pools::claim_payout_other(RuntimeOrigin::signed(80), 10));

// then
Expand Down Expand Up @@ -2488,21 +2481,10 @@ mod unbond {
Error::<T>::MinimumBondNotMet
);

// Make permissionless
assert_eq!(ClaimPermissions::<Runtime>::get(10), ClaimPermission::Permissioned);
assert_ok!(Pools::set_claim_permission(
RuntimeOrigin::signed(20),
ClaimPermission::PermissionlessAll
));

// but can go to 0
assert_ok!(Pools::unbond(RuntimeOrigin::signed(20), 20, 15));
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().active_points(), 0);
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().unbonding_points(), 20);
assert_eq!(
ClaimPermissions::<Runtime>::get(20),
ClaimPermission::PermissionlessAll
);
})
}

Expand Down Expand Up @@ -4563,12 +4545,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 +4773,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 +4792,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 +5202,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
Loading