Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Commit 1a5ad4c

Browse files
kianenigmaParity Botggwpez
authored
fix a few more things with nomination pools (#11373)
* fix a few more things with nomination pools * add bench * use weight fn * cargo run --quiet --profile=production --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark pallet --chain=dev --steps=50 --repeat=20 --pallet=pallet_nomination_pools --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/nomination-pools/src/weights.rs --template=./.maintain/frame-weight-template.hbs * allow real root to also set roles * Update frame/nomination-pools/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * Update frame/nomination-pools/src/lib.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * move out of the closure * fix a few more things Co-authored-by: Parity Bot <admin@parity.io> Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
1 parent 7143e65 commit 1a5ad4c

File tree

5 files changed

+310
-55
lines changed

5 files changed

+310
-55
lines changed

‎frame/nomination-pools/benchmarking/src/lib.rs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,28 @@ frame_benchmarking::benchmarks! {
631631
assert_eq!(MaxPoolMembersPerPool::<T>::get(), Some(u32::MAX));
632632
}
633633

634+
update_roles {
635+
let first_id = pallet_nomination_pools::LastPoolId::<T>::get() + 1;
636+
let (root, _) = create_pool_account::<T>(0, CurrencyOf::<T>::minimum_balance() * 2u32.into());
637+
let random: T::AccountId = account("but is anything really random in computers..?", 0, USER_SEED);
638+
}:_(
639+
Origin::Signed(root.clone()),
640+
first_id,
641+
Some(random.clone()),
642+
Some(random.clone()),
643+
Some(random.clone())
644+
) verify {
645+
assert_eq!(
646+
pallet_nomination_pools::BondedPools::<T>::get(first_id).unwrap().roles,
647+
pallet_nomination_pools::PoolRoles {
648+
depositor: root,
649+
nominator: random.clone(),
650+
state_toggler: random.clone(),
651+
root: random,
652+
},
653+
)
654+
}
655+
634656
impl_benchmark_test_suite!(
635657
Pallet,
636658
crate::mock::new_test_ext(),

‎frame/nomination-pools/src/lib.rs‎

Lines changed: 73 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ pub const POINTS_TO_BALANCE_INIT_RATIO: u32 = 1;
348348

349349
/// Possible operations on the configuration values of this pallet.
350350
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq, Clone)]
351-
pub enum ConfigOp<T: Default + Codec + Debug> {
351+
pub enum ConfigOp<T: Codec + Debug> {
352352
/// Don't change.
353353
Noop,
354354
/// Set the given value.
@@ -505,11 +505,11 @@ pub enum PoolState {
505505
/// Pool adminstration roles.
506506
#[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Debug, PartialEq, Clone)]
507507
pub struct PoolRoles<AccountId> {
508-
/// Creates the pool and is the initial member. They can only leave the pool once all
509-
/// other members have left. Once they fully leave, the pool is destroyed.
508+
/// Creates the pool and is the initial member. They can only leave the pool once all other
509+
/// members have left. Once they fully leave, the pool is destroyed.
510510
pub depositor: AccountId,
511-
/// Can change the nominator, state-toggler, or itself and can perform any of the actions
512-
/// the nominator or state-toggler can.
511+
/// Can change the nominator, state-toggler, or itself and can perform any of the actions the
512+
/// nominator or state-toggler can.
513513
pub root: AccountId,
514514
/// Can select which validators the pool nominates.
515515
pub nominator: AccountId,
@@ -665,6 +665,10 @@ impl<T: Config> BondedPool<T> {
665665
.saturating_sub(T::StakingInterface::active_stake(&account).unwrap_or_default())
666666
}
667667

668+
fn can_update_roles(&self, who: &T::AccountId) -> bool {
669+
*who == self.roles.root
670+
}
671+
668672
fn can_nominate(&self, who: &T::AccountId) -> bool {
669673
*who == self.roles.root || *who == self.roles.nominator
670674
}
@@ -1141,9 +1145,14 @@ pub mod pallet {
11411145
pub type Metadata<T: Config> =
11421146
CountedStorageMap<_, Twox64Concat, PoolId, BoundedVec<u8, T::MaxMetadataLen>, ValueQuery>;
11431147

1148+
/// Ever increasing number of all pools created so far.
11441149
#[pallet::storage]
11451150
pub type LastPoolId<T: Config> = StorageValue<_, u32, ValueQuery>;
11461151

1152+
/// A reverse lookup from the pool's account id to its id.
1153+
///
1154+
/// This is only used for slashing. In all other instances, the pool id is used, and the
1155+
/// accounts are deterministically derived from it.
11471156
#[pallet::storage]
11481157
pub type ReversePoolIdLookup<T: Config> =
11491158
CountedStorageMap<_, Twox64Concat, T::AccountId, PoolId, OptionQuery>;
@@ -1209,6 +1218,8 @@ pub mod pallet {
12091218
///
12101219
/// The removal can be voluntary (withdrawn all unbonded funds) or involuntary (kicked).
12111220
MemberRemoved { pool_id: PoolId, member: T::AccountId },
1221+
/// The roles of a pool have been updated to the given new roles.
1222+
RolesUpdated { root: T::AccountId, state_toggler: T::AccountId, nominator: T::AccountId },
12121223
}
12131224

12141225
#[pallet::error]
@@ -1436,9 +1447,9 @@ pub mod pallet {
14361447

14371448
bonded_pool.ok_to_unbond_with(&caller, &member_account, &member, unbonding_points)?;
14381449

1439-
// Claim the the payout prior to unbonding. Once the user is unbonding their points
1440-
// no longer exist in the bonded pool and thus they can no longer claim their payouts.
1441-
// It is not strictly necessary to claim the rewards, but we do it here for UX.
1450+
// Claim the the payout prior to unbonding. Once the user is unbonding their points no
1451+
// longer exist in the bonded pool and thus they can no longer claim their payouts. It
1452+
// is not strictly necessary to claim the rewards, but we do it here for UX.
14421453
Self::do_reward_payout(
14431454
&member_account,
14441455
&mut member,
@@ -1827,6 +1838,60 @@ pub mod pallet {
18271838

18281839
Ok(())
18291840
}
1841+
1842+
/// Update the roles of the pool.
1843+
///
1844+
/// The root is the only entity that can change any of the roles, including itself,
1845+
/// excluding the depositor, who can never change.
1846+
///
1847+
/// It emits an event, notifying UIs of the role change. This event is quite relevant to
1848+
/// most pool members and they should be informed of changes to pool roles.
1849+
#[pallet::weight(T::WeightInfo::update_roles())]
1850+
pub fn update_roles(
1851+
origin: OriginFor<T>,
1852+
pool_id: PoolId,
1853+
root: Option<T::AccountId>,
1854+
nominator: Option<T::AccountId>,
1855+
state_toggler: Option<T::AccountId>,
1856+
) -> DispatchResult {
1857+
let o1 = origin;
1858+
let o2 = o1.clone();
1859+
1860+
let mut bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
1861+
let is_pool_root = || -> Result<(), sp_runtime::DispatchError> {
1862+
let who = ensure_signed(o1)?;
1863+
ensure!(bonded_pool.can_update_roles(&who), Error::<T>::DoesNotHavePermission);
1864+
Ok(())
1865+
};
1866+
let is_root = || -> Result<(), sp_runtime::DispatchError> {
1867+
ensure_root(o2)?;
1868+
Ok(())
1869+
};
1870+
1871+
let _ = is_root().or_else(|_| is_pool_root())?;
1872+
1873+
match root {
1874+
None => (),
1875+
Some(v) => bonded_pool.roles.root = v,
1876+
};
1877+
match nominator {
1878+
None => (),
1879+
Some(v) => bonded_pool.roles.nominator = v,
1880+
};
1881+
match state_toggler {
1882+
None => (),
1883+
Some(v) => bonded_pool.roles.state_toggler = v,
1884+
};
1885+
1886+
Self::deposit_event(Event::<T>::RolesUpdated {
1887+
root: bonded_pool.roles.root.clone(),
1888+
nominator: bonded_pool.roles.nominator.clone(),
1889+
state_toggler: bonded_pool.roles.state_toggler.clone(),
1890+
});
1891+
1892+
bonded_pool.put();
1893+
Ok(())
1894+
}
18301895
}
18311896

18321897
#[pallet::hooks]

‎frame/nomination-pools/src/tests.rs‎

Lines changed: 160 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,20 @@ fn test_setup_works() {
6969
assert_eq!(
7070
PoolMembers::<Runtime>::get(10).unwrap(),
7171
PoolMember::<Runtime> { pool_id: last_pool, points: 10, ..Default::default() }
72-
)
72+
);
73+
74+
let bonded_account = Pools::create_bonded_account(last_pool);
75+
let reward_account = Pools::create_reward_account(last_pool);
76+
77+
// the bonded_account should be bonded by the depositor's funds.
78+
assert_eq!(StakingMock::active_stake(&bonded_account).unwrap(), 10);
79+
assert_eq!(StakingMock::total_stake(&bonded_account).unwrap(), 10);
80+
81+
// but not nominating yet.
82+
assert!(Nominations::get().is_empty());
83+
84+
// reward account should have an initial ED in it.
85+
assert_eq!(Balances::free_balance(&reward_account), Balances::minimum_balance());
7386
})
7487
}
7588

@@ -2082,7 +2095,7 @@ mod unbond {
20822095
// depositor can unbond inly up to `MinCreateBond`.
20832096
#[test]
20842097
fn depositor_permissioned_partial_unbond() {
2085-
ExtBuilder::default().ed(1).add_members(vec![(100, 100)]).build_and_execute(|| {
2098+
ExtBuilder::default().ed(1).build_and_execute(|| {
20862099
// given
20872100
assert_eq!(MinCreateBond::<Runtime>::get(), 2);
20882101
assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().active_points(), 10);
@@ -2098,12 +2111,12 @@ mod unbond {
20982111
Pools::unbond(Origin::signed(10), 10, 6),
20992112
Error::<Runtime>::NotOnlyPoolMember
21002113
);
2114+
21012115
assert_eq!(
21022116
pool_events_since_last_call(),
21032117
vec![
21042118
Event::Created { depositor: 10, pool_id: 1 },
21052119
Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
2106-
Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true },
21072120
Event::Unbonded { member: 10, pool_id: 1, amount: 3 }
21082121
]
21092122
);
@@ -2113,7 +2126,7 @@ mod unbond {
21132126
// same as above, but the pool is slashed and therefore the depositor cannot partially unbond.
21142127
#[test]
21152128
fn depositor_permissioned_partial_unbond_slashed() {
2116-
ExtBuilder::default().ed(1).add_members(vec![(100, 100)]).build_and_execute(|| {
2129+
ExtBuilder::default().ed(1).build_and_execute(|| {
21172130
// given
21182131
assert_eq!(MinCreateBond::<Runtime>::get(), 2);
21192132
assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().active_points(), 10);
@@ -2132,11 +2145,75 @@ mod unbond {
21322145
vec![
21332146
Event::Created { depositor: 10, pool_id: 1 },
21342147
Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
2135-
Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }
21362148
]
21372149
);
21382150
});
21392151
}
2152+
2153+
#[test]
2154+
fn every_unbonding_triggers_payout() {
2155+
ExtBuilder::default().build_and_execute(|| {
2156+
let initial_reward_account = Balances::free_balance(Pools::create_reward_account(1));
2157+
assert_eq!(initial_reward_account, Balances::minimum_balance());
2158+
assert_eq!(initial_reward_account, 5);
2159+
2160+
// set the pool to destroying so that depositor can leave.
2161+
unsafe_set_state(1, PoolState::Destroying).unwrap();
2162+
2163+
Balances::make_free_balance_be(
2164+
&Pools::create_reward_account(1),
2165+
2 * Balances::minimum_balance(),
2166+
);
2167+
2168+
assert_ok!(Pools::unbond(Origin::signed(10), 10, 2));
2169+
assert_eq!(
2170+
pool_events_since_last_call(),
2171+
vec![
2172+
Event::Created { depositor: 10, pool_id: 1 },
2173+
Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
2174+
// exactly equal to ed, all that can be claimed.
2175+
Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
2176+
Event::Unbonded { member: 10, pool_id: 1, amount: 2 }
2177+
]
2178+
);
2179+
2180+
CurrentEra::set(1);
2181+
Balances::make_free_balance_be(
2182+
&Pools::create_reward_account(1),
2183+
2 * Balances::minimum_balance(),
2184+
);
2185+
2186+
assert_ok!(Pools::unbond(Origin::signed(10), 10, 3));
2187+
assert_eq!(
2188+
pool_events_since_last_call(),
2189+
vec![
2190+
// exactly equal to ed, all that can be claimed.
2191+
Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
2192+
Event::Unbonded { member: 10, pool_id: 1, amount: 3 }
2193+
]
2194+
);
2195+
2196+
CurrentEra::set(2);
2197+
Balances::make_free_balance_be(
2198+
&Pools::create_reward_account(1),
2199+
2 * Balances::minimum_balance(),
2200+
);
2201+
2202+
assert_ok!(Pools::unbond(Origin::signed(10), 10, 5));
2203+
assert_eq!(
2204+
pool_events_since_last_call(),
2205+
vec![
2206+
Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
2207+
Event::Unbonded { member: 10, pool_id: 1, amount: 5 }
2208+
]
2209+
);
2210+
2211+
assert_eq!(
2212+
PoolMembers::<Runtime>::get(10).unwrap().unbonding_eras,
2213+
member_unbonding_eras!(3 => 2, 4 => 3, 5 => 5)
2214+
);
2215+
});
2216+
}
21402217
}
21412218

21422219
mod pool_withdraw_unbonded {
@@ -3504,3 +3581,81 @@ mod bond_extra {
35043581
})
35053582
}
35063583
}
3584+
3585+
mod update_roles {
3586+
use super::*;
3587+
3588+
#[test]
3589+
fn update_roles_works() {
3590+
ExtBuilder::default().build_and_execute(|| {
3591+
assert_eq!(
3592+
BondedPools::<Runtime>::get(1).unwrap().roles,
3593+
PoolRoles { depositor: 10, root: 900, nominator: 901, state_toggler: 902 },
3594+
);
3595+
3596+
// non-existent pools
3597+
assert_noop!(
3598+
Pools::update_roles(Origin::signed(1), 2, Some(5), Some(6), Some(7)),
3599+
Error::<Runtime>::PoolNotFound,
3600+
);
3601+
3602+
// depositor cannot change roles.
3603+
assert_noop!(
3604+
Pools::update_roles(Origin::signed(1), 1, Some(5), Some(6), Some(7)),
3605+
Error::<Runtime>::DoesNotHavePermission,
3606+
);
3607+
3608+
// nominator cannot change roles.
3609+
assert_noop!(
3610+
Pools::update_roles(Origin::signed(901), 1, Some(5), Some(6), Some(7)),
3611+
Error::<Runtime>::DoesNotHavePermission,
3612+
);
3613+
// state-toggler
3614+
assert_noop!(
3615+
Pools::update_roles(Origin::signed(902), 1, Some(5), Some(6), Some(7)),
3616+
Error::<Runtime>::DoesNotHavePermission,
3617+
);
3618+
3619+
// but root can
3620+
assert_ok!(Pools::update_roles(Origin::signed(900), 1, Some(5), Some(6), Some(7)));
3621+
3622+
assert_eq!(
3623+
pool_events_since_last_call(),
3624+
vec![
3625+
Event::Created { depositor: 10, pool_id: 1 },
3626+
Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
3627+
Event::RolesUpdated { root: 5, state_toggler: 7, nominator: 6 }
3628+
]
3629+
);
3630+
assert_eq!(
3631+
BondedPools::<Runtime>::get(1).unwrap().roles,
3632+
PoolRoles { depositor: 10, root: 5, nominator: 6, state_toggler: 7 },
3633+
);
3634+
3635+
// also root origin can
3636+
assert_ok!(Pools::update_roles(Origin::root(), 1, Some(1), Some(2), Some(3)));
3637+
3638+
assert_eq!(
3639+
pool_events_since_last_call(),
3640+
vec![Event::RolesUpdated { root: 1, state_toggler: 3, nominator: 2 }]
3641+
);
3642+
assert_eq!(
3643+
BondedPools::<Runtime>::get(1).unwrap().roles,
3644+
PoolRoles { depositor: 10, root: 1, nominator: 2, state_toggler: 3 },
3645+
);
3646+
3647+
// None is a noop
3648+
assert_ok!(Pools::update_roles(Origin::root(), 1, Some(11), None, None));
3649+
3650+
assert_eq!(
3651+
pool_events_since_last_call(),
3652+
vec![Event::RolesUpdated { root: 11, state_toggler: 3, nominator: 2 }]
3653+
);
3654+
3655+
assert_eq!(
3656+
BondedPools::<Runtime>::get(1).unwrap().roles,
3657+
PoolRoles { depositor: 10, root: 11, nominator: 2, state_toggler: 3 },
3658+
);
3659+
})
3660+
}
3661+
}

0 commit comments

Comments
 (0)