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
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
63 commits
Select commit Hold shift + click to select a range
c5cde02
create enum
Doordashcon Nov 3, 2022
3ab9cd7
logic check
Doordashcon Nov 9, 2022
59958d5
add benchmarks
Doordashcon Nov 14, 2022
9d54b8a
-enum
Doordashcon Nov 18, 2022
8efecfe
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Nov 19, 2022
f7b0a2d
update
Doordashcon Nov 20, 2022
8931433
bond extra other
Doordashcon Dec 3, 2022
9f83103
update
Doordashcon Dec 4, 2022
9d63fdb
update
Doordashcon Dec 4, 2022
a3718f6
update
Doordashcon Dec 5, 2022
4c6a4e7
cargo fmt
Doordashcon Dec 5, 2022
8207d51
Permissioned
Doordashcon Dec 7, 2022
0401174
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Dec 7, 2022
a4b6ec2
update
Doordashcon Dec 15, 2022
db9c1b8
cargo fmt
Doordashcon Dec 15, 2022
4d8bc6f
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Dec 15, 2022
b5eda08
update
Doordashcon Dec 18, 2022
58173e0
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Jan 2, 2023
b110526
update index
Doordashcon Jan 2, 2023
126bc8d
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Jan 10, 2023
a45d615
doc update
Doordashcon Jan 26, 2023
ab1e7bb
doc update
Doordashcon Jan 30, 2023
ab064f7
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Feb 1, 2023
5f25374
cargo fmt
Doordashcon Feb 1, 2023
619e9b9
bond_extra auto compound
Doordashcon Feb 2, 2023
15c286f
bond_extra_other
Doordashcon Feb 7, 2023
1a1479d
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Feb 7, 2023
b21097b
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Feb 8, 2023
4ffe8f5
Merge branch 'paritytech:master' into ddc-MCF-T1
Doordashcon Feb 9, 2023
fcd63b2
Apply suggestions from code review
kianenigma Feb 13, 2023
f6254fa
Fixes from kian
kianenigma Feb 13, 2023
ce1ebc9
updates docs & test
Doordashcon Feb 14, 2023
62edda4
Merge remote-tracking branch 'origin/master' into ddc-MCF-T1
Feb 19, 2023
1211229
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
6e4f2b4
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
832281d
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
e242799
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
011d733
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
d9590b1
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
04d6b13
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
b8be98c
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
d8fac0e
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
79f9b7e
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
184d6f2
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
0e0cc6f
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
be3cc5e
Update frame/nomination-pools/src/lib.rs
Feb 20, 2023
768c63d
Update frame/nomination-pools/src/tests.rs
Feb 20, 2023
549a743
fixes + fmt
Feb 20, 2023
e823b43
Merge remote-tracking branch 'origin/master' into ddc-MCF-T1
Feb 21, 2023
1c17bcc
expand ClaimPermissions + add benchmarks
Feb 21, 2023
9b1eb00
".git/.scripts/commands/bench/bench.sh" pallet dev pallet_nomination_…
Feb 21, 2023
9996341
tidy up claim payout benches
Feb 21, 2023
6fa711c
fix
Feb 21, 2023
0f9f730
+ test: claim_payout_other_works
Feb 21, 2023
4bdf22a
comments, rename to set_claim_permission
Feb 21, 2023
2965fd9
fix comment
Feb 21, 2023
7c16c8e
remove ClaimPermission on leave pool
Feb 21, 2023
ef63fdf
fix test
Feb 21, 2023
f8cedba
".git/.scripts/commands/fmt/fmt.sh"
Feb 21, 2023
4ac673b
Merge remote-tracking branch 'origin/master' into ddc-MCF-T1
Feb 22, 2023
0ea2309
+ test for ClaimPermissions::remove()
Feb 22, 2023
a39573c
impl can_bond_extra & can_claim_payout
Feb 22, 2023
f7a6568
Merge remote-tracking branch 'origin/master' into ddc-MCF-T1
Feb 22, 2023
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
25 changes: 24 additions & 1 deletion frame/nomination-pools/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ use frame_system::RawOrigin as RuntimeOrigin;
use pallet_nomination_pools::{
BalanceOf, BondExtra, BondedPoolInner, BondedPools, ConfigOp, MaxPoolMembers,
MaxPoolMembersPerPool, MaxPools, Metadata, MinCreateBond, MinJoinBond, Pallet as Pools,
PoolMembers, PoolRoles, PoolState, RewardPools, SubPoolsStorage,
PoolMembers, PoolRoles, PoolState, RewardClaim, RewardClaimPermission, RewardPools,
SubPoolsStorage,
};
use sp_runtime::traits::{Bounded, StaticLookup, Zero};
use sp_staking::{EraIndex, StakingInterface};
Expand Down Expand Up @@ -654,6 +655,28 @@ frame_benchmarking::benchmarks! {
assert!(T::Staking::nominations(Pools::<T>::create_bonded_account(1)).is_none());
}

set_reward_claim {
// Create a pool
let min_create_bond = Pools::<T>::depositor_min_bond();
let (depositor, pool_account) = create_pool_account::<T>(0, min_create_bond);

// Join pool
let min_join_bond = MinJoinBond::<T>::get().max(CurrencyOf::<T>::minimum_balance());
let joiner = create_funded_user_with_balance::<T>("joiner", 0, min_join_bond * 4u32.into());
let joiner_lookup = T::Lookup::unlookup(joiner.clone());
Pools::<T>::join(RuntimeOrigin::Signed(joiner.clone()).into(), min_join_bond, 1)
.unwrap();

// Sanity check join worked
assert_eq!(
T::Staking::active_stake(&pool_account).unwrap(),
min_create_bond + min_join_bond
);
}:_(RuntimeOrigin::Signed(joiner.clone()), RewardClaim::Permissionless)
verify {
assert_eq!(RewardClaimPermission::<T>::get(joiner), RewardClaim::Permissionless);
}

impl_benchmark_test_suite!(
Pallet,
crate::mock::new_test_ext(),
Expand Down
144 changes: 117 additions & 27 deletions frame/nomination-pools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,21 @@ enum AccountType {
Reward,
}

/// Account to bond pending reward of a member.
#[derive(Encode, Decode, MaxEncodedLen, Clone, Copy, Debug, PartialEq, Eq, TypeInfo)]
pub enum RewardClaim {
/// Only the pool member themself can claim their reward.
Permissioned,
/// Anyone can claim the reward of this member.
Permissionless,
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
}

impl Default for RewardClaim {
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
fn default() -> Self {
Self::Permissioned
}
}

/// A member in a pool.
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, CloneNoBound)]
#[cfg_attr(feature = "std", derive(frame_support::PartialEqNoBound, DefaultNoBound))]
Expand Down Expand Up @@ -1316,6 +1331,11 @@ pub mod pallet {
pub type ReversePoolIdLookup<T: Config> =
CountedStorageMap<_, Twox64Concat, T::AccountId, PoolId, OptionQuery>;

/// Map from a pool member account to their preference regarding reward payout.
#[pallet::storage]
pub type RewardClaimPermission<T: Config> =
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
StorageMap<_, Twox64Concat, T::AccountId, RewardClaim, ValueQuery>;
Ank4n marked this conversation as resolved.
Show resolved Hide resolved

#[pallet::genesis_config]
pub struct GenesisConfig<T: Config> {
pub min_join_bond: BalanceOf<T>,
Expand Down Expand Up @@ -1473,6 +1493,8 @@ pub mod pallet {
PoolIdInUse,
/// Pool id provided is not correct/usable.
InvalidPoolId,
/// Restricted to pending rewards
CannotBondFreeBalanceOther,
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
}

#[derive(Encode, Decode, PartialEq, TypeInfo, frame_support::PalletError, RuntimeDebug)]
Expand Down Expand Up @@ -1563,6 +1585,7 @@ pub mod pallet {
/// accumulated rewards, see [`BondExtra`].
///
/// Bonding extra funds implies an automatic payout of all pending rewards as well.
/// See `bond_extra_other` to bond pending rewards of `other` members.
// NOTE: this transaction is implemented with the sole purpose of readability and
// correctness, not optimization. We read/write several storage items multiple times instead
// of just once, in the spirit reusing code.
Expand All @@ -1573,34 +1596,8 @@ pub mod pallet {
)]
pub fn bond_extra(origin: OriginFor<T>, extra: BondExtra<BalanceOf<T>>) -> DispatchResult {
let who = ensure_signed(origin)?;
let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&who)?;

// payout related stuff: we must claim the payouts, and updated recorded payout data
// before updating the bonded pool points, similar to that of `join` transaction.
reward_pool.update_records(bonded_pool.id, bonded_pool.points)?;
let claimed =
Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;

let (points_issued, bonded) = match extra {
BondExtra::FreeBalance(amount) =>
(bonded_pool.try_bond_funds(&who, amount, BondType::Later)?, amount),
BondExtra::Rewards =>
(bonded_pool.try_bond_funds(&who, claimed, BondType::Later)?, claimed),
};

bonded_pool.ok_to_be_open()?;
member.points =
member.points.checked_add(&points_issued).ok_or(Error::<T>::OverflowRisk)?;

Self::deposit_event(Event::<T>::Bonded {
member: who.clone(),
pool_id: member.pool_id,
bonded,
joined: false,
});
Self::put_member_with_pools(&who, member, bonded_pool, reward_pool);

Ok(())
Self::do_bond_extra(who, None, extra)
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
}

/// A bonded member can use this to claim their payout based on the rewards that the pool
Expand Down Expand Up @@ -1718,6 +1715,7 @@ pub mod pallet {
// Now that we know everything has worked write the items to storage.
SubPoolsStorage::insert(&member.pool_id, sub_pools);
Self::put_member_with_pools(&member_account, member, bonded_pool, reward_pool);
<RewardClaimPermission<T>>::remove(member_account);
rossbulat marked this conversation as resolved.
Show resolved Hide resolved

Ok(())
}
Expand Down Expand Up @@ -2117,6 +2115,52 @@ pub mod pallet {
ensure!(bonded_pool.can_nominate(&who), Error::<T>::NotNominator);
T::Staking::chill(&bonded_pool.bonded_account())
}

/// Bond `extra` more funds from `origin` and
/// pending rewards of `other` into their respective pools.
///
/// Origin can bond extra more funds from free balance and pending rewards
/// when `other` is `None`.
///
/// Origin can bond extra pending rewards of `other` given `Some` is a member and
/// set reward claim to Permissionless.
#[pallet::call_index(14)]
#[pallet::weight(
T::WeightInfo::bond_extra_transfer()
.max(T::WeightInfo::bond_extra_reward())
)]
pub fn bond_extra_other(
origin: OriginFor<T>,
other: Option<AccountIdLookupOf<T>>,
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
extra: BondExtra<BalanceOf<T>>,
) -> DispatchResult {
let who = ensure_signed(origin)?;

Self::do_bond_extra(who, other, extra)
}

/// Sets permission to claim reward.
///
/// Lets a pool member to choose who can claim pending rewards on their behalf. 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 `Permissionless` 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
#[pallet::call_index(15)]
#[pallet::weight(T::DbWeight::get().reads_writes(1, 1))]
pub fn set_reward_claim(origin: OriginFor<T>, actor: RewardClaim) -> DispatchResult {
let who = ensure_signed(origin)?;

ensure!(PoolMembers::<T>::contains_key(&who), Error::<T>::PoolMemberNotFound);
<RewardClaimPermission<T>>::mutate(who, |source| {
*source = actor;
});
Ok(())
}
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
}

#[pallet::hooks]
Expand Down Expand Up @@ -2419,6 +2463,52 @@ impl<T: Config> Pallet<T> {
Ok(())
}

fn do_bond_extra(
who: T::AccountId,
other: Option<AccountIdLookupOf<T>>,
extra: BondExtra<BalanceOf<T>>,
) -> DispatchResult {
let other = other.map(T::Lookup::lookup).transpose()?;
let is_bonding_for = other.is_some();
let who = other.unwrap_or(who);
if is_bonding_for {
ensure!(
RewardClaimPermission::<T>::get(&who) == RewardClaim::Permissionless,
Error::<T>::DoesNotHavePermission
);
ensure!(extra == BondExtra::Rewards, Error::<T>::CannotBondFreeBalanceOther);
}
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

let (mut member, mut bonded_pool, mut reward_pool) = Self::get_member_with_pools(&who)?;

// payout related stuff: we must claim the payouts, and updated recorded payout data
// before updating the bonded pool points, similar to that of `join` transaction.
reward_pool.update_records(bonded_pool.id, bonded_pool.points)?;
let claimed =
Self::do_reward_payout(&who, &mut member, &mut bonded_pool, &mut reward_pool)?;

let (points_issued, bonded) = match extra {
BondExtra::FreeBalance(amount) =>
(bonded_pool.try_bond_funds(&who, amount, BondType::Later)?, amount),
BondExtra::Rewards =>
(bonded_pool.try_bond_funds(&who, claimed, BondType::Later)?, claimed),
};

bonded_pool.ok_to_be_open()?;
member.points =
member.points.checked_add(&points_issued).ok_or(Error::<T>::OverflowRisk)?;

Self::deposit_event(Event::<T>::Bonded {
member: who.clone(),
pool_id: member.pool_id,
bonded,
joined: false,
});
Self::put_member_with_pools(&who, member, bonded_pool, reward_pool);

Ok(())
}

/// Ensure the correctness of the state of this pallet.
///
/// This should be valid before or after each state transition of this pallet.
Expand Down
107 changes: 107 additions & 0 deletions frame/nomination-pools/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2090,10 +2090,18 @@ mod unbond {
Error::<T>::MinimumBondNotMet
);

// Make permissionless
assert_eq!(RewardClaimPermission::<Runtime>::get(10), RewardClaim::Permissioned);
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
assert_ok!(Pools::set_reward_claim(
RuntimeOrigin::signed(20),
RewardClaim::Permissionless
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
));

// 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!(RewardClaimPermission::<Runtime>::get(20), RewardClaim::Permissioned);
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down Expand Up @@ -4240,6 +4248,39 @@ mod create {
}
}

#[test]
fn set_claimable_actor_works() {
ExtBuilder::default().build_and_execute(|| {
// Given
Balances::make_free_balance_be(&11, ExistentialDeposit::get() + 2);
assert!(!PoolMembers::<Runtime>::contains_key(&11));

// When
assert_ok!(Pools::join(RuntimeOrigin::signed(11), 2, 1));

// Then
assert_eq!(
pool_events_since_last_call(),
vec![
Event::Created { depositor: 10, pool_id: 1 },
Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
Event::Bonded { member: 11, pool_id: 1, bonded: 2, joined: true },
]
);

// Make permissionless
assert_eq!(RewardClaimPermission::<Runtime>::get(11), RewardClaim::Permissioned);
assert_noop!(
Pools::set_reward_claim(RuntimeOrigin::signed(12), RewardClaim::Permissionless),
Error::<T>::PoolMemberNotFound
);
assert_ok!(Pools::set_reward_claim(RuntimeOrigin::signed(11), RewardClaim::Permissionless));

// then
assert_eq!(RewardClaimPermission::<Runtime>::get(11), RewardClaim::Permissionless);
});
}
rossbulat marked this conversation as resolved.
Show resolved Hide resolved

mod nominate {
use super::*;

Expand Down Expand Up @@ -4552,6 +4593,7 @@ mod bond_extra {

// when
assert_ok!(Pools::bond_extra(RuntimeOrigin::signed(10), BondExtra::Rewards));
assert_eq!(Balances::free_balance(&default_reward_account()), 7);

// then
assert_eq!(Balances::free_balance(10), 35);
Expand Down Expand Up @@ -4583,6 +4625,71 @@ mod bond_extra {
);
})
}

#[test]
fn bond_extra_other() {
ExtBuilder::default().add_members(vec![(20, 20)]).build_and_execute(|| {
Balances::make_free_balance_be(&default_reward_account(), 8);
// ... if which only 3 is claimable to make sure the reward account does not die.
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
let claimable_reward = 8 - ExistentialDeposit::get();
// NOTE: easier to read of we use 3, so let's use the number instead of variable.
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(claimable_reward, 3, "test is correct if rewards are divisible by 3");

// given
assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().points, 10);
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().points, 20);
assert_eq!(BondedPools::<Runtime>::get(1).unwrap().points, 30);
assert_eq!(Balances::free_balance(10), 35);
assert_eq!(Balances::free_balance(20), 20);

// Permissioned by default
assert_noop!(
Pools::bond_extra_other(RuntimeOrigin::signed(80), Some(20), BondExtra::Rewards),
Error::<Runtime>::DoesNotHavePermission
);

assert_ok!(Pools::set_reward_claim(
RuntimeOrigin::signed(10),
RewardClaim::Permissionless
rossbulat marked this conversation as resolved.
Show resolved Hide resolved
));
assert_ok!(Pools::bond_extra_other(
RuntimeOrigin::signed(50),
Some(10),
BondExtra::Rewards
));
assert_eq!(Balances::free_balance(&default_reward_account()), 7);

// then
assert_eq!(Balances::free_balance(10), 35);
assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().points, 10 + 1);
assert_eq!(BondedPools::<Runtime>::get(1).unwrap().points, 30 + 1);

// when
assert_noop!(
Pools::bond_extra_other(RuntimeOrigin::signed(40), None, BondExtra::Rewards),
Error::<Runtime>::PoolMemberNotFound
);
assert_ok!(Pools::set_reward_claim(
RuntimeOrigin::signed(20),
RewardClaim::Permissionless
));
assert_noop!(
Pools::bond_extra_other(
RuntimeOrigin::signed(20),
Some(20),
BondExtra::FreeBalance(10)
),
Error::<Runtime>::CannotBondFreeBalanceOther
);

// then
assert_eq!(Balances::free_balance(20), 20);
// 20's share of the rewards is the other 2/3 of the rewards, since they have 20/30 of
// the shares
assert_eq!(PoolMembers::<Runtime>::get(20).unwrap().points, 20);
assert_eq!(BondedPools::<Runtime>::get(1).unwrap().points, 31);
})
}
}

mod update_roles {
Expand Down