update_roles for nomination pools

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,
+		ConfigOp::Set(random.clone()),
+		ConfigOp::Set(random.clone()),
+		ConfigOp::Set(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,
@@ -1141,9 +1141,14 @@ pub mod pallet {
 	pub type Metadata<T: Config> =
 		CountedStorageMap<_, Twox64Concat, PoolId, BoundedVec<u8, T::MaxMetadataLen>, ValueQuery>;
 
+	/// The last pool id.
 	#[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 +1214,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]
@@ -1269,6 +1276,8 @@ pub mod pallet {
 		NotEnoughPointsToUnbond,
 		/// Partial unbonding now allowed permissionlessly.
 		PartialUnbondNotAllowedPermissionlessly,
+		/// Cannot remove any of the roles, they can only be set ot left unchanged.
+		CannotRemoveRole,
 	}
 
 	#[pallet::call]
@@ -1436,9 +1445,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 +1836,50 @@ 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: ConfigOp<T::AccountId>,
+			nominator: ConfigOp<T::AccountId>,
+			state_toggler: ConfigOp<T::AccountId>,
+		) -> DispatchResult {
+			let who = ensure_signed(origin)?;
+			let mut bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
+			ensure!(bonded_pool.roles.root == who, Error::<T>::DoesNotHavePermission);
+
+			match root {
+				ConfigOp::Noop => (),
+				ConfigOp::Set(v) => bonded_pool.roles.root = v,
+				ConfigOp::Remove => Err(Error::<T>::CannotRemoveRole)?,
+			};
+			match nominator {
+				ConfigOp::Noop => (),
+				ConfigOp::Set(v) => bonded_pool.roles.nominator = v,
+				ConfigOp::Remove => Err(Error::<T>::CannotRemoveRole)?,
+			};
+			match state_toggler {
+				ConfigOp::Noop => (),
+				ConfigOp::Set(v) => bonded_pool.roles.state_toggler = v,
+				ConfigOp::Remove => Err(Error::<T>::CannotRemoveRole)?,
+			};
+
+			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,
+					ConfigOp::Set(5),
+					ConfigOp::Set(6),
+					ConfigOp::Set(7)
+				),
+				Error::<Runtime>::PoolNotFound,
+			);
+
+			// depositor cannot change roles.
+			assert_noop!(
+				Pools::update_roles(
+					Origin::signed(1),
+					1,
+					ConfigOp::Set(5),
+					ConfigOp::Set(6),
+					ConfigOp::Set(7)
+				),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+
+			// nominator cannot change roles.
+			assert_noop!(
+				Pools::update_roles(
+					Origin::signed(901),
+					1,
+					ConfigOp::Set(5),
+					ConfigOp::Set(6),
+					ConfigOp::Set(7)
+				),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+			// state-toggler
+			assert_noop!(
+				Pools::update_roles(
+					Origin::signed(902),
+					1,
+					ConfigOp::Set(5),
+					ConfigOp::Set(6),
+					ConfigOp::Set(7)
+				),
+				Error::<Runtime>::DoesNotHavePermission,
+			);
+
+			// but root can
+			assert_ok!(Pools::update_roles(
+				Origin::signed(900),
+				1,
+				ConfigOp::Set(5),
+				ConfigOp::Set(6),
+				ConfigOp::Set(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 }
+				]
+			);
+		})
+	}
+}

frame/nomination-pools/src/weights.rs

@@ -58,6 +58,7 @@ pub trait WeightInfo {
 	fn set_state() -> Weight;
 	fn set_metadata(n: u32, ) -> Weight;
 	fn set_configs() -> Weight;
+	fn update_roles() -> Weight;
 }
 
 /// Weights for pallet_nomination_pools using the Substrate node and recommended hardware.
@@ -269,6 +270,9 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
 		(2_793_000 as Weight)
 			.saturating_add(T::DbWeight::get().writes(5 as Weight))
 	}
+	fn update_roles() -> Weight {
+		0
+	}
 }
 
 // For backwards compatibility and tests
@@ -479,4 +483,7 @@ impl WeightInfo for () {
 		(2_793_000 as Weight)
 			.saturating_add(RocksDbWeight::get().writes(5 as Weight))
 	}
+	fn update_roles() -> Weight {
+		0
+	}
 }