fix a few more things with nomination pools (paritytech#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>

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

@@ -631,6 +631,28 @@ frame_benchmarking::benchmarks! {
 		assert_eq!(MaxPoolMembersPerPool::<T>::get(), Some(u32::MAX));
 	}
 
+	update_roles {
+		let first_id = pallet_nomination_pools::LastPoolId::<T>::get() + 1;
+		let (root, _) = create_pool_account::<T>(0, CurrencyOf::<T>::minimum_balance() * 2u32.into());
+		let random: T::AccountId = account("but is anything really random in computers..?", 0, USER_SEED);
+	}:_(
+		Origin::Signed(root.clone()),
+		first_id,
+		Some(random.clone()),
+		Some(random.clone()),
+		Some(random.clone())
+	) verify {
+		assert_eq!(
+			pallet_nomination_pools::BondedPools::<T>::get(first_id).unwrap().roles,
+			pallet_nomination_pools::PoolRoles {
+				depositor: root,
+				nominator: random.clone(),
+				state_toggler: random.clone(),
+				root: random,
+			},
+		)
+	}
+
 	impl_benchmark_test_suite!(
 		Pallet,
 		crate::mock::new_test_ext(),

frame/nomination-pools/src/lib.rs

@@ -348,7 +348,7 @@ pub const POINTS_TO_BALANCE_INIT_RATIO: u32 = 1;
 
 /// Possible operations on the configuration values of this pallet.
 #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, RuntimeDebugNoBound, PartialEq, Clone)]
-pub enum ConfigOp<T: Default + Codec + Debug> {
+pub enum ConfigOp<T: Codec + Debug> {
 	/// Don't change.
 	Noop,
 	/// Set the given value.
@@ -505,11 +505,11 @@ pub enum PoolState {
 /// Pool adminstration roles.
 #[derive(Encode, Decode, MaxEncodedLen, TypeInfo, Debug, PartialEq, Clone)]
 pub struct PoolRoles<AccountId> {
-	/// Creates the pool and is the initial member. They can only leave the pool once all
-	/// other members have left. Once they fully leave, the pool is destroyed.
+	/// Creates the pool and is the initial member. They can only leave the pool once all other
+	/// members have left. Once they fully leave, the pool is destroyed.
 	pub depositor: AccountId,
-	/// Can change the nominator, state-toggler, or itself and can perform any of the actions
-	/// the nominator or state-toggler can.
+	/// Can change the nominator, state-toggler, or itself and can perform any of the actions the
+	/// nominator or state-toggler can.
 	pub root: AccountId,
 	/// Can select which validators the pool nominates.
 	pub nominator: AccountId,
@@ -665,6 +665,10 @@ impl<T: Config> BondedPool<T> {
 			.saturating_sub(T::StakingInterface::active_stake(&account).unwrap_or_default())
 	}
 
+	fn can_update_roles(&self, who: &T::AccountId) -> bool {
+		*who == self.roles.root
+	}
+
 	fn can_nominate(&self, who: &T::AccountId) -> bool {
 		*who == self.roles.root || *who == self.roles.nominator
 	}
@@ -1141,9 +1145,14 @@ pub mod pallet {
 	pub type Metadata<T: Config> =
 		CountedStorageMap<_, Twox64Concat, PoolId, BoundedVec<u8, T::MaxMetadataLen>, ValueQuery>;
 
+	/// Ever increasing number of all pools created so far.
 	#[pallet::storage]
 	pub type LastPoolId<T: Config> = StorageValue<_, u32, ValueQuery>;
 
+	/// A reverse lookup from the pool's account id to its id.
+	///
+	/// This is only used for slashing. In all other instances, the pool id is used, and the
+	/// accounts are deterministically derived from it.
 	#[pallet::storage]
 	pub type ReversePoolIdLookup<T: Config> =
 		CountedStorageMap<_, Twox64Concat, T::AccountId, PoolId, OptionQuery>;
@@ -1209,6 +1218,8 @@ pub mod pallet {
 		///
 		/// The removal can be voluntary (withdrawn all unbonded funds) or involuntary (kicked).
 		MemberRemoved { pool_id: PoolId, member: T::AccountId },
+		/// The roles of a pool have been updated to the given new roles.
+		RolesUpdated { root: T::AccountId, state_toggler: T::AccountId, nominator: T::AccountId },
 	}
 
 	#[pallet::error]
@@ -1436,9 +1447,9 @@ pub mod pallet {
 
 			bonded_pool.ok_to_unbond_with(&caller, &member_account, &member, unbonding_points)?;
 
-			// Claim the the payout prior to unbonding. Once the user is unbonding their points
-			// no longer exist in the bonded pool and thus they can no longer claim their payouts.
-			// It is not strictly necessary to claim the rewards, but we do it here for UX.
+			// Claim the the payout prior to unbonding. Once the user is unbonding their points no
+			// longer exist in the bonded pool and thus they can no longer claim their payouts. It
+			// is not strictly necessary to claim the rewards, but we do it here for UX.
 			Self::do_reward_payout(
 				&member_account,
 				&mut member,
@@ -1827,6 +1838,60 @@ pub mod pallet {
 
 			Ok(())
 		}
+
+		/// Update the roles of the pool.
+		///
+		/// The root is the only entity that can change any of the roles, including itself,
+		/// excluding the depositor, who can never change.
+		///
+		/// It emits an event, notifying UIs of the role change. This event is quite relevant to
+		/// most pool members and they should be informed of changes to pool roles.
+		#[pallet::weight(T::WeightInfo::update_roles())]
+		pub fn update_roles(
+			origin: OriginFor<T>,
+			pool_id: PoolId,
+			root: Option<T::AccountId>,
+			nominator: Option<T::AccountId>,
+			state_toggler: Option<T::AccountId>,
+		) -> DispatchResult {
+			let o1 = origin;
+			let o2 = o1.clone();
+
+			let mut bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
+			let is_pool_root = || -> Result<(), sp_runtime::DispatchError> {
+				let who = ensure_signed(o1)?;
+				ensure!(bonded_pool.can_update_roles(&who), Error::<T>::DoesNotHavePermission);
+				Ok(())
+			};
+			let is_root = || -> Result<(), sp_runtime::DispatchError> {
+				ensure_root(o2)?;
+				Ok(())
+			};
+
+			let _ = is_root().or_else(|_| is_pool_root())?;
+
+			match root {
+				None => (),
+				Some(v) => bonded_pool.roles.root = v,
+			};
+			match nominator {
+				None => (),
+				Some(v) => bonded_pool.roles.nominator = v,
+			};
+			match state_toggler {
+				None => (),
+				Some(v) => bonded_pool.roles.state_toggler = v,
+			};
+
+			Self::deposit_event(Event::<T>::RolesUpdated {
+				root: bonded_pool.roles.root.clone(),
+				nominator: bonded_pool.roles.nominator.clone(),
+				state_toggler: bonded_pool.roles.state_toggler.clone(),
+			});
+
+			bonded_pool.put();
+			Ok(())
+		}
 	}
 
 	#[pallet::hooks]

frame/nomination-pools/src/tests.rs

@@ -69,7 +69,20 @@ fn test_setup_works() {
 		assert_eq!(
 			PoolMembers::<Runtime>::get(10).unwrap(),
 			PoolMember::<Runtime> { pool_id: last_pool, points: 10, ..Default::default() }
-		)
+		);
+
+		let bonded_account = Pools::create_bonded_account(last_pool);
+		let reward_account = Pools::create_reward_account(last_pool);
+
+		// the bonded_account should be bonded by the depositor's funds.
+		assert_eq!(StakingMock::active_stake(&bonded_account).unwrap(), 10);
+		assert_eq!(StakingMock::total_stake(&bonded_account).unwrap(), 10);
+
+		// but not nominating yet.
+		assert!(Nominations::get().is_empty());
+
+		// reward account should have an initial ED in it.
+		assert_eq!(Balances::free_balance(&reward_account), Balances::minimum_balance());
 	})
 }
 
@@ -2082,7 +2095,7 @@ mod unbond {
 	// depositor can unbond inly up to `MinCreateBond`.
 	#[test]
 	fn depositor_permissioned_partial_unbond() {
-		ExtBuilder::default().ed(1).add_members(vec![(100, 100)]).build_and_execute(|| {
+		ExtBuilder::default().ed(1).build_and_execute(|| {
 			// given
 			assert_eq!(MinCreateBond::<Runtime>::get(), 2);
 			assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().active_points(), 10);
@@ -2098,12 +2111,12 @@ mod unbond {
 				Pools::unbond(Origin::signed(10), 10, 6),
 				Error::<Runtime>::NotOnlyPoolMember
 			);
+
 			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: 100, pool_id: 1, bonded: 100, joined: true },
 					Event::Unbonded { member: 10, pool_id: 1, amount: 3 }
 				]
 			);
@@ -2113,7 +2126,7 @@ mod unbond {
 	// same as above, but the pool is slashed and therefore the depositor cannot partially unbond.
 	#[test]
 	fn depositor_permissioned_partial_unbond_slashed() {
-		ExtBuilder::default().ed(1).add_members(vec![(100, 100)]).build_and_execute(|| {
+		ExtBuilder::default().ed(1).build_and_execute(|| {
 			// given
 			assert_eq!(MinCreateBond::<Runtime>::get(), 2);
 			assert_eq!(PoolMembers::<Runtime>::get(10).unwrap().active_points(), 10);
@@ -2132,11 +2145,75 @@ mod unbond {
 				vec![
 					Event::Created { depositor: 10, pool_id: 1 },
 					Event::Bonded { member: 10, pool_id: 1, bonded: 10, joined: true },
-					Event::Bonded { member: 100, pool_id: 1, bonded: 100, joined: true }
 				]
 			);
 		});
 	}
+
+	#[test]
+	fn every_unbonding_triggers_payout() {
+		ExtBuilder::default().build_and_execute(|| {
+			let initial_reward_account = Balances::free_balance(Pools::create_reward_account(1));
+			assert_eq!(initial_reward_account, Balances::minimum_balance());
+			assert_eq!(initial_reward_account, 5);
+
+			// set the pool to destroying so that depositor can leave.
+			unsafe_set_state(1, PoolState::Destroying).unwrap();
+
+			Balances::make_free_balance_be(
+				&Pools::create_reward_account(1),
+				2 * Balances::minimum_balance(),
+			);
+
+			assert_ok!(Pools::unbond(Origin::signed(10), 10, 2));
+			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 },
+					// exactly equal to ed, all that can be claimed.
+					Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
+					Event::Unbonded { member: 10, pool_id: 1, amount: 2 }
+				]
+			);
+
+			CurrentEra::set(1);
+			Balances::make_free_balance_be(
+				&Pools::create_reward_account(1),
+				2 * Balances::minimum_balance(),
+			);
+
+			assert_ok!(Pools::unbond(Origin::signed(10), 10, 3));
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					// exactly equal to ed, all that can be claimed.
+					Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
+					Event::Unbonded { member: 10, pool_id: 1, amount: 3 }
+				]
+			);
+
+			CurrentEra::set(2);
+			Balances::make_free_balance_be(
+				&Pools::create_reward_account(1),
+				2 * Balances::minimum_balance(),
+			);
+
+			assert_ok!(Pools::unbond(Origin::signed(10), 10, 5));
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![
+					Event::PaidOut { member: 10, pool_id: 1, payout: 5 },
+					Event::Unbonded { member: 10, pool_id: 1, amount: 5 }
+				]
+			);
+
+			assert_eq!(
+				PoolMembers::<Runtime>::get(10).unwrap().unbonding_eras,
+				member_unbonding_eras!(3 => 2, 4 => 3, 5 => 5)
+			);
+		});
+	}
 }
 
 mod pool_withdraw_unbonded {
@@ -3504,3 +3581,81 @@ mod bond_extra {
 		})
 	}
 }
+
+mod update_roles {
+	use super::*;
+
+	#[test]
+	fn update_roles_works() {
+		ExtBuilder::default().build_and_execute(|| {
+			assert_eq!(
+				BondedPools::<Runtime>::get(1).unwrap().roles,
+				PoolRoles { depositor: 10, root: 900, nominator: 901, state_toggler: 902 },
+			);
+
+			// non-existent pools
+			assert_noop!(
+				Pools::update_roles(Origin::signed(1), 2, Some(5), Some(6), Some(7)),
+				Error::<Runtime>::PoolNotFound,
+			);
+
+			// depositor cannot change roles.
+			assert_noop!(
+				Pools::update_roles(Origin::signed(1), 1, Some(5), Some(6), Some(7)),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+
+			// nominator cannot change roles.
+			assert_noop!(
+				Pools::update_roles(Origin::signed(901), 1, Some(5), Some(6), Some(7)),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+			// state-toggler
+			assert_noop!(
+				Pools::update_roles(Origin::signed(902), 1, Some(5), Some(6), Some(7)),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+
+			// but root can
+			assert_ok!(Pools::update_roles(Origin::signed(900), 1, Some(5), Some(6), Some(7)));
+
+			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::RolesUpdated { root: 5, state_toggler: 7, nominator: 6 }
+				]
+			);
+			assert_eq!(
+				BondedPools::<Runtime>::get(1).unwrap().roles,
+				PoolRoles { depositor: 10, root: 5, nominator: 6, state_toggler: 7 },
+			);
+
+			// also root origin can
+			assert_ok!(Pools::update_roles(Origin::root(), 1, Some(1), Some(2), Some(3)));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![Event::RolesUpdated { root: 1, state_toggler: 3, nominator: 2 }]
+			);
+			assert_eq!(
+				BondedPools::<Runtime>::get(1).unwrap().roles,
+				PoolRoles { depositor: 10, root: 1, nominator: 2, state_toggler: 3 },
+			);
+
+			// None is a noop
+			assert_ok!(Pools::update_roles(Origin::root(), 1, Some(11), None, None));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![Event::RolesUpdated { root: 11, state_toggler: 3, nominator: 2 }]
+			);
+
+			assert_eq!(
+				BondedPools::<Runtime>::get(1).unwrap().roles,
+				PoolRoles { depositor: 10, root: 11, nominator: 2, state_toggler: 3 },
+			);
+		})
+	}
+}