Allow nomination pools to chill + fix dismantle scenario

Cargo.toml

@@ -114,6 +114,7 @@ members = [
 	"frame/proxy",
 	"frame/nomination-pools",
 	"frame/nomination-pools/benchmarking",
+	"frame/nomination-pools/test-staking",
 	"frame/randomness-collective-flip",
 	"frame/recovery",
 	"frame/referenda",

frame/nomination-pools/src/lib.rs

@@ -452,16 +452,18 @@ impl<T: Config> PoolMember<T> {
 	/// Returns `Ok(())` and updates `unbonding_eras` and `points` if success, `Err(_)` otherwise.
 	fn try_unbond(
 		&mut self,
-		points: BalanceOf<T>,
+		points_dissolved: BalanceOf<T>,
+		points_issued: BalanceOf<T>,
 		unbonding_era: EraIndex,
 	) -> Result<(), Error<T>> {
-		if let Some(new_points) = self.points.checked_sub(&points) {
+		if let Some(new_points) = self.points.checked_sub(&points_dissolved) {
 			match self.unbonding_eras.get_mut(&unbonding_era) {
 				Some(already_unbonding_points) =>
-					*already_unbonding_points = already_unbonding_points.saturating_add(points),
+					*already_unbonding_points =
+						already_unbonding_points.saturating_add(points_issued),
 				None => self
 					.unbonding_eras
-					.try_insert(unbonding_era, points)
+					.try_insert(unbonding_era, points_issued)
 					.map(|old| {
 						if old.is_some() {
 							defensive!("value checked to not exist in the map; qed");
@@ -721,9 +723,13 @@ impl<T: Config> BondedPool<T> {
 	}
 
 	fn is_destroying_and_only_depositor(&self, alleged_depositor_points: BalanceOf<T>) -> bool {
-		// NOTE: if we add `&& self.member_counter == 1`, then this becomes even more strict and
-		// ensures that there are no unbonding members hanging around either.
-		self.is_destroying() && self.points == alleged_depositor_points
+		// we need to ensure that `self.member_counter == 1` as well, because the depositor's
+		// initial `MinCreateBond` (or more) is what guarantees that the ledger of the pool does not
+		// get killed in the staking system, and that it does not fall below `MinimumNominatorBond`,
+		// which could prevent other non-depositor members from fully leaving. Thus, all members
+		// must withdraw, then depositor can unbond, and finally withdraw after waiting another
+		// cycle.
+		self.is_destroying() && self.points == alleged_depositor_points && self.member_counter == 1
 	}
 
 	/// Whether or not the pool is ok to be in `PoolSate::Open`. If this returns an `Err`, then the
@@ -979,9 +985,13 @@ impl<T: Config> UnbondPool<T> {
 	}
 
 	/// Issue points and update the balance given `new_balance`.
-	fn issue(&mut self, new_funds: BalanceOf<T>) {
-		self.points = self.points.saturating_add(self.balance_to_point(new_funds));
+	///
+	/// Returns the actual amounts of points issued.
+	fn issue(&mut self, new_funds: BalanceOf<T>) -> BalanceOf<T> {
+		let new_points = self.balance_to_point(new_funds);
+		self.points = self.points.saturating_add(new_points);
 		self.balance = self.balance.saturating_add(new_funds);
+		new_points
 	}
 
 	/// Dissolve some points from the unbonding pool, reducing the balance of the pool
@@ -1136,6 +1146,15 @@ pub mod pallet {
 	///
 	/// This is the amount that the depositor must put as their initial stake in the pool, as an
 	/// indication of "skin in the game".
+	///
+	/// This is the value that will always exist in the staking ledger of the pool bonded account
+	/// while all other accounts leave.
+	///
+	/// A sane configuration should thus ensure that this value is large enough such that it is more
+	/// than `MinimumNominatorBond` and `ExistentialDeposit`. The pool's bonded account's stash
+	/// falling behind `MinimumNominatorBond` it cannot call unbond, and the pool bonded account's
+	/// stash falling below `ExistentialDeposit` means the ledger can be killed prematurely in a
+	/// call to `T::StakingInterface::withdraw_unbonded`.
 	#[pallet::storage]
 	pub type MinCreateBond<T: Config> = StorageValue<_, BalanceOf<T>, ValueQuery>;
 
@@ -1242,9 +1261,33 @@ pub mod pallet {
 		/// A payout has been made to a member.
 		PaidOut { member: T::AccountId, pool_id: PoolId, payout: BalanceOf<T> },
 		/// A member has unbonded from their pool.
-		Unbonded { member: T::AccountId, pool_id: PoolId, amount: BalanceOf<T> },
+		///
+		/// - `balance` is the corresponding balance of the number of points that have been
+		///   requested to be unbonded (the argument of the `unbond` transaction) from the bonded
+		///   pool.
+		/// - `points` is the number of points that are issued as a result of
+		/// `balance` into the corresponding unbonding pool.
+		///
+		/// In the absence of slashing, these values will match. In the presence of slashing, the
+		/// number of points that are issued in the unbonding pool will be less than the amount
+		/// requested to be unbonded.
+		Unbonded {
+			member: T::AccountId,
+			pool_id: PoolId,
+			balance: BalanceOf<T>,
+			points: BalanceOf<T>,
+		},
 		/// A member has withdrawn from their pool.
-		Withdrawn { member: T::AccountId, pool_id: PoolId, amount: BalanceOf<T> },
+		///
+		/// The given number of `points` have been dissolved in return of `balance`.
+		///
+		/// Similar to `Unbonded` event, in the absence of slashing, these values will match.
+		Withdrawn {
+			member: T::AccountId,
+			pool_id: PoolId,
+			balance: BalanceOf<T>,
+			points: BalanceOf<T>,
+		},
 		/// A pool has been destroyed.
 		Destroyed { pool_id: PoolId },
 		/// The state of a pool has changed
@@ -1260,6 +1303,10 @@ pub mod pallet {
 			state_toggler: Option<T::AccountId>,
 			nominator: Option<T::AccountId>,
 		},
+		/// The active balance of pool `pool_id` has been slashed to `balance`.
+		PoolSlashed { pool_id: PoolId, balance: BalanceOf<T> },
+		/// The unbond pool at `era` of pool `pool_id` has been slashed to `balance`.
+		UnbondingPoolSlashed { pool_id: PoolId, era: EraIndex, balance: BalanceOf<T> },
 	}
 
 	#[pallet::error]
@@ -1276,8 +1323,6 @@ pub mod pallet {
 		/// An account is already delegating in another pool. An account may only belong to one
 		/// pool at a time.
 		AccountBelongsToOtherPool,
-		/// The pool has insufficient balance to bond as a nominator.
-		InsufficientBond,
 		/// The member is already unbonding in this era.
 		AlreadyUnbonding,
 		/// The member is fully unbonded (and thus cannot access the bonded and reward pool
@@ -1500,9 +1545,6 @@ pub mod pallet {
 			let current_era = T::StakingInterface::current_era();
 			let unbond_era = T::StakingInterface::bonding_duration().saturating_add(current_era);
 
-			// Try and unbond in the member map.
-			member.try_unbond(unbonding_points, unbond_era)?;
-
 			// Unbond in the actual underlying nominator.
 			let unbonding_balance = bonded_pool.dissolve(unbonding_points);
 			T::StakingInterface::unbond(bonded_pool.bonded_account(), unbonding_balance)?;
@@ -1523,17 +1565,21 @@ pub mod pallet {
 					.defensive_map_err(|_| Error::<T>::DefensiveError)?;
 			}
 
-			sub_pools
+			let actual_unbonding_points = sub_pools
 				.with_era
 				.get_mut(&unbond_era)
 				// The above check ensures the pool exists.
 				.defensive_ok_or_else(|| Error::<T>::DefensiveError)?
 				.issue(unbonding_balance);
 
+			// Try and unbond in the member map.
+			member.try_unbond(unbonding_points, actual_unbonding_points, unbond_era)?;
+
 			Self::deposit_event(Event::<T>::Unbonded {
 				member: member_account.clone(),
 				pool_id: member.pool_id,
-				amount: unbonding_balance,
+				points: actual_unbonding_points,
+				balance: unbonding_balance,
 			});
 
 			// Now that we know everything has worked write the items to storage.
@@ -1616,14 +1662,16 @@ pub mod pallet {
 
 			// Before calculate the `balance_to_unbond`, with call withdraw unbonded to ensure the
 			// `transferrable_balance` is correct.
-			T::StakingInterface::withdraw_unbonded(
+			let destroyed = T::StakingInterface::withdraw_unbonded(
 				bonded_pool.bonded_account(),
 				num_slashing_spans,
 			)?;
 
+			let mut sum_unlocked_points: BalanceOf<T> = Zero::zero();
 			let balance_to_unbond = withdrawn_points
 				.iter()
 				.fold(BalanceOf::<T>::zero(), |accumulator, (era, unlocked_points)| {
+					sum_unlocked_points = sum_unlocked_points.saturating_add(*unlocked_points);
 					if let Some(era_pool) = sub_pools.with_era.get_mut(&era) {
 						let balance_to_unbond = era_pool.dissolve(*unlocked_points);
 						if era_pool.points.is_zero() {
@@ -1656,7 +1704,8 @@ pub mod pallet {
 			Self::deposit_event(Event::<T>::Withdrawn {
 				member: member_account.clone(),
 				pool_id: member.pool_id,
-				amount: balance_to_unbond,
+				points: sum_unlocked_points,
+				balance: balance_to_unbond,
 			});
 
 			let post_info_weight = if member.total_points().is_zero() {
@@ -1784,6 +1833,13 @@ pub mod pallet {
 			Ok(())
 		}
 
+		/// Nominate on behalf of the pool.
+		///
+		/// The dispatch origin of this call must be signed by the pool nominator or the the pool
+		/// root role.
+		///
+		/// This directly forward the call to the staking pallet, on behalf of the pool bonded
+		/// account.
 		#[pallet::weight(T::WeightInfo::nominate(validators.len() as u32))]
 		pub fn nominate(
 			origin: OriginFor<T>,
@@ -1793,10 +1849,28 @@ pub mod pallet {
 			let who = ensure_signed(origin)?;
 			let bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
 			ensure!(bonded_pool.can_nominate(&who), Error::<T>::NotNominator);
-			T::StakingInterface::nominate(bonded_pool.bonded_account(), validators)?;
-			Ok(())
+			T::StakingInterface::nominate(bonded_pool.bonded_account(), validators)
+		}
+
+		/// Nominate on behalf of the pool.
+		///
+		/// The dispatch origin of this call must be signed by the pool nominator or the the pool
+		/// root role, same as [`Pallet::nominate`].
+		///
+		/// This directly forward the call to the staking pallet, on behalf of the pool bonded
+		/// account.
+		#[pallet::weight(T::WeightInfo::chill())]
+		pub fn chill(origin: OriginFor<T>, pool_id: PoolId) -> DispatchResult {
+			let who = ensure_signed(origin)?;
+			let bonded_pool = BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?;
+			ensure!(bonded_pool.can_nominate(&who), Error::<T>::NotNominator);
+			T::StakingInterface::chill(bonded_pool.bonded_account())
 		}
 
+		/// Set a new state for the pool.
+		///
+		/// The dispatch origin of this call must be signed by the state toggler, or the root role
+		/// of the pool.
 		#[pallet::weight(T::WeightInfo::set_state())]
 		pub fn set_state(
 			origin: OriginFor<T>,
@@ -1823,6 +1897,10 @@ pub mod pallet {
 			Ok(())
 		}
 
+		/// Set a new metadata for the pool.
+		///
+		/// The dispatch origin of this call must be signed by the state toggler, or the root role
+		/// of the pool.
 		#[pallet::weight(T::WeightInfo::set_metadata(metadata.len() as u32))]
 		pub fn set_metadata(
 			origin: OriginFor<T>,
@@ -2237,6 +2315,7 @@ impl<T: Config> Pallet<T> {
 		if level.is_zero() {
 			return Ok(())
 		}
+
 		// note: while a bit wacky, since they have the same key, even collecting to vec should
 		// result in the same set of keys, in the same order.
 		let bonded_pools = BondedPools::<T>::iter_keys().collect::<Vec<_>>();
@@ -2337,9 +2416,16 @@ impl<T: Config> OnStakerSlash<T::AccountId, BalanceOf<T>> for Pallet<T> {
 			};
 			for (era, slashed_balance) in slashed_unlocking.iter() {
 				if let Some(pool) = sub_pools.with_era.get_mut(era) {
-					pool.balance = *slashed_balance
+					pool.balance = *slashed_balance;
+					Self::deposit_event(Event::<T>::UnbondingPoolSlashed {
+						era: *era,
+						pool_id,
+						balance: *slashed_balance,
+					});
 				}
 			}
+
+			Self::deposit_event(Event::<T>::PoolSlashed { pool_id, balance: _slashed_bonded });
 			SubPoolsStorage::<T>::insert(pool_id, sub_pools);
 		}
 	}

frame/nomination-pools/src/mock.rs

@@ -2,7 +2,6 @@ use super::*;
 use crate::{self as pools};
 use frame_support::{assert_ok, parameter_types, PalletId};
 use frame_system::RawOrigin;
-use std::collections::HashMap;
 
 pub type AccountId = u128;
 pub type Balance = u128;
@@ -20,17 +19,19 @@ pub fn default_reward_account() -> AccountId {
 parameter_types! {
 	pub static CurrentEra: EraIndex = 0;
 	pub static BondingDuration: EraIndex = 3;
-	static BondedBalanceMap: HashMap<AccountId, Balance> = Default::default();
-	static UnbondingBalanceMap: HashMap<AccountId, Balance> = Default::default();
+	pub storage BondedBalanceMap: BTreeMap<AccountId, Balance> = Default::default();
+	pub storage UnbondingBalanceMap: BTreeMap<AccountId, Balance> = Default::default();
 	#[derive(Clone, PartialEq)]
 	pub static MaxUnbonding: u32 = 8;
-	pub static Nominations: Vec<AccountId> = vec![];
+	pub storage Nominations: Vec<AccountId> = vec![];
 }
 
 pub struct StakingMock;
 impl StakingMock {
 	pub(crate) fn set_bonded_balance(who: AccountId, bonded: Balance) {
-		BONDED_BALANCE_MAP.with(|m| m.borrow_mut().insert(who, bonded));
+		let mut x = BondedBalanceMap::get();
+		x.insert(who, bonded);
+		BondedBalanceMap::set(&x)
 	}
 }
 
@@ -66,22 +67,33 @@ impl sp_staking::StakingInterface for StakingMock {
 	}
 
 	fn bond_extra(who: Self::AccountId, extra: Self::Balance) -> DispatchResult {
-		BONDED_BALANCE_MAP.with(|m| *m.borrow_mut().get_mut(&who).unwrap() += extra);
+		let mut x = BondedBalanceMap::get();
+		x.get_mut(&who).map(|v| *v += extra);
+		BondedBalanceMap::set(&x);
 		Ok(())
 	}
 
 	fn unbond(who: Self::AccountId, amount: Self::Balance) -> DispatchResult {
-		BONDED_BALANCE_MAP.with(|m| *m.borrow_mut().get_mut(&who).unwrap() -= amount);
-		UNBONDING_BALANCE_MAP
-			.with(|m| *m.borrow_mut().entry(who).or_insert(Self::Balance::zero()) += amount);
+		let mut x = BondedBalanceMap::get();
+		*x.get_mut(&who).unwrap() = x.get_mut(&who).unwrap().saturating_sub(amount);
+		BondedBalanceMap::set(&x);
+		let mut y = UnbondingBalanceMap::get();
+		*y.entry(who).or_insert(Self::Balance::zero()) += amount;
+		UnbondingBalanceMap::set(&y);
 		Ok(())
 	}
 
-	fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result<u64, DispatchError> {
+	fn chill(_: Self::AccountId) -> sp_runtime::DispatchResult {
+		Ok(())
+	}
+
+	fn withdraw_unbonded(who: Self::AccountId, _: u32) -> Result<bool, DispatchError> {
 		// Simulates removing unlocking chunks and only having the bonded balance locked
-		let _maybe_new_free = UNBONDING_BALANCE_MAP.with(|m| m.borrow_mut().remove(&who));
+		let mut x = UnbondingBalanceMap::get();
+		x.remove(&who);
+		UnbondingBalanceMap::set(&x);
 
-		Ok(100)
+		Ok(UnbondingBalanceMap::get().is_empty())
 	}
 
 	fn bond(
@@ -95,7 +107,7 @@ impl sp_staking::StakingInterface for StakingMock {
 	}
 
 	fn nominate(_: Self::AccountId, nominations: Vec<Self::AccountId>) -> DispatchResult {
-		Nominations::set(nominations);
+		Nominations::set(&nominations);
 		Ok(())
 	}
 }
@@ -263,7 +275,8 @@ pub(crate) fn unsafe_set_state(pool_id: PoolId, state: PoolState) -> Result<(),
 }
 
 parameter_types! {
-	static ObservedEvents: usize = 0;
+	static PoolsEvents: usize = 0;
+	static BalancesEvents: usize = 0;
 }
 
 /// All events of this pallet.
@@ -273,8 +286,20 @@ pub(crate) fn pool_events_since_last_call() -> Vec<super::Event<Runtime>> {
 		.map(|r| r.event)
 		.filter_map(|e| if let Event::Pools(inner) = e { Some(inner) } else { None })
 		.collect::<Vec<_>>();
-	let already_seen = ObservedEvents::get();
-	ObservedEvents::set(events.len());
+	let already_seen = PoolsEvents::get();
+	PoolsEvents::set(events.len());
+	events.into_iter().skip(already_seen).collect()
+}
+
+/// All events of this pallet.
+pub(crate) fn balances_events_since_last_call() -> Vec<pallet_balances::Event<Runtime>> {
+	let events = System::events()
+		.into_iter()
+		.map(|r| r.event)
+		.filter_map(|e| if let Event::Balances(inner) = e { Some(inner) } else { None })
+		.collect::<Vec<_>>();
+	let already_seen = BalancesEvents::get();
+	BalancesEvents::set(events.len());
 	events.into_iter().skip(already_seen).collect()
 }