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 20 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
45 changes: 44 additions & 1 deletion frame/nomination-pools/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,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 @@ -268,6 +269,26 @@ frame_benchmarking::benchmarks! {
);
}

bond_extra_pending_rewards_other {
let origin_weight = Pools::<T>::depositor_min_bond() * 2u32.into();
let scenario = ListScenario::<T>::new(origin_weight, true)?;
let scenario_creator_lookup = T::Lookup::unlookup(scenario.creator1.clone());
let extra = (scenario.dest_weight - origin_weight).max(CurrencyOf::<T>::minimum_balance());

let reward_account = Pools::<T>::create_reward_account(1);
assert!(extra >= CurrencyOf::<T>::minimum_balance());
CurrencyOf::<T>::deposit_creating(&reward_account, extra);
Pools::<T>::set_reward_claim(RuntimeOrigin::Signed(scenario.creator1.clone()).into(), RewardClaim::Permissionless)
.unwrap();

}:_(RuntimeOrigin::Signed(scenario.creator1.clone()), scenario_creator_lookup)
verify {
assert!(
T::Staking::active_stake(&scenario.origin1).unwrap() >=
scenario.dest_weight
);
}

claim_payout {
let origin_weight = Pools::<T>::depositor_min_bond() * 2u32.into();
let ed = CurrencyOf::<T>::minimum_balance();
Expand Down Expand Up @@ -652,6 +673,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
97 changes: 96 additions & 1 deletion 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
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
#[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 @@ -1718,6 +1738,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 +2138,80 @@ pub mod pallet {
ensure!(bonded_pool.can_nominate(&who), Error::<T>::NotNominator);
T::Staking::chill(&bonded_pool.bonded_account())
}

/// Bond pending rewards of `member_account` into the pool they already belong.
///
/// Note: `member_account` must pass `RewardClaim::Permissionless` to `set_reward_claim`,
/// making this call permissionless.
#[pallet::call_index(14)]
#[pallet::weight(
T::WeightInfo::bond_extra_transfer()
.max(T::WeightInfo::bond_extra_reward())
)]
pub fn bond_extra_pending_rewards_other(
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have asked this before, but can't this be one transaction with the existing bond_extra? and even if we want to keep them separate, shan't they share most of the internal code?

Copy link
Contributor Author

@Doordashcon Doordashcon Jan 31, 2023

Choose a reason for hiding this comment

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

it does share most of the internal code of bond_extra, it excludes transferring pending rewards to the member from the do_reward_payout helper function and instead transfers the pending reward directly to the bounding account from try_bound_fund and increment the points accordingly.

Copy link
Contributor Author

@Doordashcon Doordashcon Feb 1, 2023

Choose a reason for hiding this comment

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

@kianenigma Yes an extra parameter other: Option<AccountId> can be added to bond_extra then the claimed rewards will be bonded for the account specified in other.

origin: OriginFor<T>,
member_account: AccountIdLookupOf<T>,
) -> DispatchResult {
ensure_signed(origin)?;
let member_account = T::Lookup::lookup(member_account)?;
let (mut member, mut bonded_pool, mut reward_pool) =
Self::get_member_with_pools(&member_account)?;

ensure!(
RewardClaimPermission::<T>::get(&member_account) == RewardClaim::Permissionless,
Error::<T>::DoesNotHavePermission
);

reward_pool.update_records(bonded_pool.id, bonded_pool.points)?;
ensure!(!member.active_points().is_zero(), Error::<T>::FullyUnbonding);

let current_reward_counter =
reward_pool.current_reward_counter(bonded_pool.id, bonded_pool.points)?;
let pending_rewards = member.pending_rewards(current_reward_counter)?;

if !pending_rewards.is_zero() {
member.last_recorded_reward_counter = current_reward_counter;
reward_pool.register_claimed_reward(pending_rewards);
}

let points_issued = bonded_pool.try_bond_funds(
&bonded_pool.reward_account(),
pending_rewards,
BondType::Later,
)?;

bonded_pool.ok_to_be_open()?;
member.points = member.points.saturating_add(points_issued);

Self::deposit_event(Event::<T>::Bonded {
member: member_account.clone(),
pool_id: member.pool_id,
bonded: pending_rewards,
joined: false,
});

Self::put_member_with_pools(&member_account, member, bonded_pool, reward_pool);

Ok(())
}

/// Set reward claim permission.
///
/// # Arguments
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
///
/// * `Origin` - Member of a pool.
/// * `actor` - Account to claim reward.
#[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 @@ -2303,7 +2398,7 @@ impl<T: Config> Pallet<T> {
// We check for zero above
.div(current_points)
}

// TODO: Investigate logic here to add conditional event emission on different account types.
/// If the member has some rewards, transfer a payout from the reward pool to the member.
// Emits events and potentially modifies pool state if any arithmetic saturates, but does
// not persist any of the mutable inputs to storage.
Expand Down
95 changes: 95 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,59 @@ mod bond_extra {
);
})
}

#[test]
fn bond_extra_pending_rewards_other_works() {
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);

// Make permissionless and bond pending rewards of member
assert_noop!(
Pools::bond_extra_pending_rewards_other(RuntimeOrigin::signed(80), 10),
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_pending_rewards_other(RuntimeOrigin::signed(50), 10));
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_pending_rewards_other(RuntimeOrigin::signed(40), 20),
Error::<Runtime>::DoesNotHavePermission
);
assert_ok!(Pools::set_reward_claim(
RuntimeOrigin::signed(20),
RewardClaim::Permissionless
));
assert_ok!(Pools::bond_extra_pending_rewards_other(RuntimeOrigin::signed(20), 20));

// 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 + 2);
assert_eq!(BondedPools::<Runtime>::get(1).unwrap().points, 30 + 3);
})
}
}

mod update_roles {
Expand Down