make pool roles optional (#11411)

* make pool roles optional

* undo lock file changes?

* add migration

* Fix

* fix review comments

substrate/Cargo.lock

@@ -5892,6 +5892,7 @@ version = "1.0.0"
 dependencies = [
  "frame-support",
  "frame-system",
+ "log",
  "pallet-balances",
  "parity-scale-codec",
  "scale-info",

substrate/frame/nomination-pools/Cargo.toml

@@ -23,6 +23,7 @@ sp-runtime = { version = "6.0.0", default-features = false, path = "../../primit
 sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" }
 sp-staking = { version = "4.0.0-dev", default-features = false, path = "../../primitives/staking" }
 sp-core = { version = "6.0.0", default-features = false, path = "../../primitives/core" }
+log = { version = "0.4.0", default-features = false }
 
 [dev-dependencies]
 pallet-balances = { version = "4.0.0-dev", path = "../balances" }
@@ -42,4 +43,5 @@ std = [
 	"sp-std/std",
 	"sp-staking/std",
 	"sp-core/std",
+	"log/std",
 ]

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

@@ -526,9 +526,9 @@ frame_benchmarking::benchmarks! {
 				member_counter: 1,
 				roles: PoolRoles {
 					depositor: depositor.clone(),
-					root: depositor.clone(),
-					nominator: depositor.clone(),
-					state_toggler: depositor.clone(),
+					root: Some(depositor.clone()),
+					nominator: Some(depositor.clone()),
+					state_toggler: Some(depositor.clone()),
 				},
 			}
 		);
@@ -567,9 +567,9 @@ frame_benchmarking::benchmarks! {
 				member_counter: 1,
 				roles: PoolRoles {
 					depositor: depositor.clone(),
-					root: depositor.clone(),
-					nominator: depositor.clone(),
-					state_toggler: depositor.clone(),
+					root: Some(depositor.clone()),
+					nominator: Some(depositor.clone()),
+					state_toggler: Some(depositor.clone()),
 				}
 			}
 		);
@@ -638,17 +638,17 @@ frame_benchmarking::benchmarks! {
 	}:_(
 		Origin::Signed(root.clone()),
 		first_id,
-		Some(random.clone()),
-		Some(random.clone()),
-		Some(random.clone())
+		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,
+				nominator: Some(random.clone()),
+				state_toggler: Some(random.clone()),
+				root: Some(random),
 			},
 		)
 	}

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

@@ -324,11 +324,26 @@ use sp_runtime::traits::{AccountIdConversion, Bounded, CheckedSub, Convert, Satu
 use sp_staking::{EraIndex, OnStakerSlash, StakingInterface};
 use sp_std::{collections::btree_map::BTreeMap, fmt::Debug, ops::Div, vec::Vec};
 
+/// The log target of this pallet.
+pub const LOG_TARGET: &'static str = "runtime::nomination-pools";
+
+// syntactic sugar for logging.
+#[macro_export]
+macro_rules! log {
+	($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
+		log::$level!(
+			target: crate::LOG_TARGET,
+			concat!("[{:?}] 🏊‍♂️ ", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
+		)
+	};
+}
+
 #[cfg(test)]
 mod mock;
 #[cfg(test)]
 mod tests;
 
+pub mod migration;
 pub mod weights;
 
 pub use pallet::*;
@@ -502,7 +517,11 @@ pub enum PoolState {
 	Destroying,
 }
 
-/// Pool adminstration roles.
+/// Pool administration roles.
+///
+/// Any pool has a depositor, which can never change. But, all the other roles are optional, and
+/// cannot exist. Note that if `root` is set to `None`, it basically means that the roles of this
+/// pool can never change again (except via governance).
 #[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
@@ -510,11 +529,11 @@ pub struct PoolRoles<AccountId> {
 	pub depositor: AccountId,
 	/// Can change the nominator, state-toggler, or itself and can perform any of the actions the
 	/// nominator or state-toggler can.
-	pub root: AccountId,
+	pub root: Option<AccountId>,
 	/// Can select which validators the pool nominates.
-	pub nominator: AccountId,
+	pub nominator: Option<AccountId>,
 	/// Can change the pools state and kick members if the pool is blocked.
-	pub state_toggler: AccountId,
+	pub state_toggler: Option<AccountId>,
 }
 
 /// Pool permissions and state
@@ -665,25 +684,36 @@ impl<T: Config> BondedPool<T> {
 			.saturating_sub(T::StakingInterface::active_stake(&account).unwrap_or_default())
 	}
 
+	fn is_root(&self, who: &T::AccountId) -> bool {
+		self.roles.root.as_ref().map_or(false, |root| root == who)
+	}
+
+	fn is_state_toggler(&self, who: &T::AccountId) -> bool {
+		self.roles
+			.state_toggler
+			.as_ref()
+			.map_or(false, |state_toggler| state_toggler == who)
+	}
+
 	fn can_update_roles(&self, who: &T::AccountId) -> bool {
-		*who == self.roles.root
+		self.is_root(who)
 	}
 
 	fn can_nominate(&self, who: &T::AccountId) -> bool {
-		*who == self.roles.root || *who == self.roles.nominator
+		self.is_root(who) ||
+			self.roles.nominator.as_ref().map_or(false, |nominator| nominator == who)
 	}
 
 	fn can_kick(&self, who: &T::AccountId) -> bool {
-		(*who == self.roles.root || *who == self.roles.state_toggler) &&
-			self.state == PoolState::Blocked
+		self.state == PoolState::Blocked && (self.is_root(who) || self.is_state_toggler(who))
 	}
 
 	fn can_toggle_state(&self, who: &T::AccountId) -> bool {
-		(*who == self.roles.root || *who == self.roles.state_toggler) && !self.is_destroying()
+		(self.is_root(who) || self.is_state_toggler(who)) && !self.is_destroying()
 	}
 
 	fn can_set_metadata(&self, who: &T::AccountId) -> bool {
-		*who == self.roles.root || *who == self.roles.state_toggler
+		self.is_root(who) || self.is_state_toggler(who)
 	}
 
 	fn is_destroying(&self) -> bool {
@@ -987,11 +1017,12 @@ impl<T: Config> SubPools<T> {
 	///
 	/// This is often used whilst getting the sub-pool from storage, thus it consumes and returns
 	/// `Self` for ergonomic purposes.
-	fn maybe_merge_pools(mut self, unbond_era: EraIndex) -> Self {
+	fn maybe_merge_pools(mut self, current_era: EraIndex) -> Self {
 		// Ex: if `TotalUnbondingPools` is 5 and current era is 10, we only want to retain pools
 		// 6..=10. Note that in the first few eras where `checked_sub` is `None`, we don't remove
 		// anything.
-		if let Some(newest_era_to_remove) = unbond_era.checked_sub(TotalUnbondingPools::<T>::get())
+		if let Some(newest_era_to_remove) =
+			current_era.checked_sub(T::PostUnbondingPoolsWindow::get())
 		{
 			self.with_era.retain(|k, v| {
 				if *k > newest_era_to_remove {
@@ -1045,11 +1076,15 @@ impl<T: Config> Get<u32> for TotalUnbondingPools<T> {
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
-	use frame_support::transactional;
+	use frame_support::{traits::StorageVersion, transactional};
 	use frame_system::{ensure_signed, pallet_prelude::*};
 
+	/// The current storage version.
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(crate) trait Store)]
+	#[pallet::storage_version(STORAGE_VERSION)]
 	pub struct Pallet<T>(_);
 
 	#[pallet::config]
@@ -1218,8 +1253,13 @@ 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 },
+		/// The roles of a pool have been updated to the given new roles. Note that the depositor
+		/// can never change.
+		RolesUpdated {
+			root: Option<T::AccountId>,
+			state_toggler: Option<T::AccountId>,
+			nominator: Option<T::AccountId>,
+		},
 	}
 
 	#[pallet::error]
@@ -1470,7 +1510,7 @@ pub mod pallet {
 			// Note that we lazily create the unbonding pools here if they don't already exist
 			let mut sub_pools = SubPoolsStorage::<T>::get(member.pool_id)
 				.unwrap_or_default()
-				.maybe_merge_pools(unbond_era);
+				.maybe_merge_pools(current_era);
 
 			// Update the unbond pool associated with the current era with the unbonded funds. Note
 			// that we lazily create the unbond pool if it does not yet exist.
@@ -1693,7 +1733,12 @@ pub mod pallet {
 			});
 			let mut bonded_pool = BondedPool::<T>::new(
 				pool_id,
-				PoolRoles { root, nominator, state_toggler, depositor: who.clone() },
+				PoolRoles {
+					root: Some(root),
+					nominator: Some(nominator),
+					state_toggler: Some(state_toggler),
+					depositor: who.clone(),
+				},
 			);
 
 			bonded_pool.try_inc_members()?;
@@ -1850,9 +1895,9 @@ pub mod pallet {
 		pub fn update_roles(
 			origin: OriginFor<T>,
 			pool_id: PoolId,
-			root: Option<T::AccountId>,
-			nominator: Option<T::AccountId>,
-			state_toggler: Option<T::AccountId>,
+			new_root: ConfigOp<T::AccountId>,
+			new_nominator: ConfigOp<T::AccountId>,
+			new_state_toggler: ConfigOp<T::AccountId>,
 		) -> DispatchResult {
 			let mut bonded_pool = match ensure_root(origin.clone()) {
 				Ok(()) => BondedPool::<T>::get(pool_id).ok_or(Error::<T>::PoolNotFound)?,
@@ -1865,17 +1910,20 @@ pub mod pallet {
 				},
 			};
 
-			match root {
-				None => (),
-				Some(v) => bonded_pool.roles.root = v,
+			match new_root {
+				ConfigOp::Noop => (),
+				ConfigOp::Remove => bonded_pool.roles.root = None,
+				ConfigOp::Set(v) => bonded_pool.roles.root = Some(v),
 			};
-			match nominator {
-				None => (),
-				Some(v) => bonded_pool.roles.nominator = v,
+			match new_nominator {
+				ConfigOp::Noop => (),
+				ConfigOp::Remove => bonded_pool.roles.nominator = None,
+				ConfigOp::Set(v) => bonded_pool.roles.nominator = Some(v),
 			};
-			match state_toggler {
-				None => (),
-				Some(v) => bonded_pool.roles.state_toggler = v,
+			match new_state_toggler {
+				ConfigOp::Noop => (),
+				ConfigOp::Remove => bonded_pool.roles.state_toggler = None,
+				ConfigOp::Set(v) => bonded_pool.roles.state_toggler = Some(v),
 			};
 
 			Self::deposit_event(Event::<T>::RolesUpdated {
@@ -2282,7 +2330,7 @@ impl<T: Config> OnStakerSlash<T::AccountId, BalanceOf<T>> for Pallet<T> {
 		_slashed_bonded: BalanceOf<T>,
 		slashed_unlocking: &BTreeMap<EraIndex, BalanceOf<T>>,
 	) {
-		if let Some(pool_id) = ReversePoolIdLookup::<T>::get(pool_account) {
+		if let Some(pool_id) = ReversePoolIdLookup::<T>::get(pool_account).defensive() {
 			let mut sub_pools = match SubPoolsStorage::<T>::get(pool_id).defensive() {
 				Some(sub_pools) => sub_pools,
 				None => return,

substrate/frame/nomination-pools/src/migration.rs

@@ -0,0 +1,105 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2022 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use super::*;
+
+pub mod v1 {
+	use super::*;
+	use crate::log;
+	use frame_support::traits::OnRuntimeUpgrade;
+
+	#[derive(Decode)]
+	pub struct OldPoolRoles<AccountId> {
+		pub depositor: AccountId,
+		pub root: AccountId,
+		pub nominator: AccountId,
+		pub state_toggler: AccountId,
+	}
+
+	impl<AccountId> OldPoolRoles<AccountId> {
+		fn migrate_to_v1(self) -> PoolRoles<AccountId> {
+			PoolRoles {
+				depositor: self.depositor,
+				root: Some(self.root),
+				nominator: Some(self.nominator),
+				state_toggler: Some(self.state_toggler),
+			}
+		}
+	}
+
+	#[derive(Decode)]
+	pub struct OldBondedPoolInner<T: Config> {
+		pub points: BalanceOf<T>,
+		pub state: PoolState,
+		pub member_counter: u32,
+		pub roles: OldPoolRoles<T::AccountId>,
+	}
+
+	impl<T: Config> OldBondedPoolInner<T> {
+		fn migrate_to_v1(self) -> BondedPoolInner<T> {
+			BondedPoolInner {
+				member_counter: self.member_counter,
+				points: self.points,
+				state: self.state,
+				roles: self.roles.migrate_to_v1(),
+			}
+		}
+	}
+
+	/// Trivial migration which makes the roles of each pool optional.
+	///
+	/// Note: The depositor is not optional since he can never change.
+	pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
+		fn on_runtime_upgrade() -> Weight {
+			let current = Pallet::<T>::current_storage_version();
+			let onchain = Pallet::<T>::on_chain_storage_version();
+
+			log!(
+				info,
+				"Running migration with current storage version {:?} / onchain {:?}",
+				current,
+				onchain
+			);
+
+			if current == 1 && onchain == 0 {
+				// this is safe to execute on any runtime that has a bounded number of pools.
+				let mut translated = 0u64;
+				BondedPools::<T>::translate::<OldBondedPoolInner<T>, _>(|_key, old_value| {
+					translated.saturating_inc();
+					Some(old_value.migrate_to_v1())
+				});
+
+				current.put::<Pallet<T>>();
+
+				log!(info, "Upgraded {} pools, storage to version {:?}", translated, current);
+
+				T::DbWeight::get().reads_writes(translated + 1, translated + 1)
+			} else {
+				log!(info, "Migration did not executed. This probably should be removed");
+				T::DbWeight::get().reads(1)
+			}
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade() -> Result<(), &'static str> {
+			// new version must be set.
+			assert_eq!(Pallet::<T>::on_chain_storage_version(), 1);
+			Ok(())
+		}
+	}
+}

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

@@ -38,7 +38,7 @@ macro_rules! member_unbonding_eras {
 }
 
 pub const DEFAULT_ROLES: PoolRoles<AccountId> =
-	PoolRoles { depositor: 10, root: 900, nominator: 901, state_toggler: 902 };
+	PoolRoles { depositor: 10, root: Some(900), nominator: Some(901), state_toggler: Some(902) };
 
 #[test]
 fn test_setup_works() {
@@ -333,6 +333,8 @@ mod sub_pools {
 	fn maybe_merge_pools_works() {
 		ExtBuilder::default().build_and_execute(|| {
 			assert_eq!(TotalUnbondingPools::<Runtime>::get(), 5);
+			assert_eq!(BondingDuration::get(), 3);
+			assert_eq!(PostUnbondingPoolsWindow::get(), 2);
 
 			// Given
 			let mut sub_pool_0 = SubPools::<Runtime> {
@@ -347,19 +349,19 @@ mod sub_pools {
 			};
 
 			// When `current_era < TotalUnbondingPools`,
-			let sub_pool_1 = sub_pool_0.clone().maybe_merge_pools(3);
+			let sub_pool_1 = sub_pool_0.clone().maybe_merge_pools(0);
 
 			// Then it exits early without modifications
 			assert_eq!(sub_pool_1, sub_pool_0);
 
 			// When `current_era == TotalUnbondingPools`,
-			let sub_pool_1 = sub_pool_1.maybe_merge_pools(4);
+			let sub_pool_1 = sub_pool_1.maybe_merge_pools(1);
 
 			// Then it exits early without modifications
 			assert_eq!(sub_pool_1, sub_pool_0);
 
 			// When  `current_era - TotalUnbondingPools == 0`,
-			let mut sub_pool_1 = sub_pool_1.maybe_merge_pools(5);
+			let mut sub_pool_1 = sub_pool_1.maybe_merge_pools(2);
 
 			// Then era 0 is merged into the `no_era` pool
 			sub_pool_0.no_era = sub_pool_0.with_era.remove(&0).unwrap();
@@ -376,7 +378,7 @@ mod sub_pools {
 				.unwrap();
 
 			// When `current_era - TotalUnbondingPools == 1`
-			let sub_pool_2 = sub_pool_1.maybe_merge_pools(6);
+			let sub_pool_2 = sub_pool_1.maybe_merge_pools(3);
 			let era_1_pool = sub_pool_0.with_era.remove(&1).unwrap();
 
 			// Then era 1 is merged into the `no_era` pool
@@ -385,7 +387,7 @@ mod sub_pools {
 			assert_eq!(sub_pool_2, sub_pool_0);
 
 			// When `current_era - TotalUnbondingPools == 5`, so all pools with era <= 4 are removed
-			let sub_pool_3 = sub_pool_2.maybe_merge_pools(10);
+			let sub_pool_3 = sub_pool_2.maybe_merge_pools(7);
 
 			// Then all eras <= 5 are merged into the `no_era` pool
 			for era in 2..=5 {
@@ -1723,9 +1725,9 @@ mod unbond {
 				// Given
 				unsafe_set_state(1, PoolState::Blocked).unwrap();
 				let bonded_pool = BondedPool::<Runtime>::get(1).unwrap();
-				assert_eq!(bonded_pool.roles.root, 900);
-				assert_eq!(bonded_pool.roles.nominator, 901);
-				assert_eq!(bonded_pool.roles.state_toggler, 902);
+				assert_eq!(bonded_pool.roles.root.unwrap(), 900);
+				assert_eq!(bonded_pool.roles.nominator.unwrap(), 901);
+				assert_eq!(bonded_pool.roles.state_toggler.unwrap(), 902);
 
 				// When the nominator tries to kick, then its a noop
 				assert_noop!(
@@ -3143,9 +3145,9 @@ mod create {
 						state: PoolState::Open,
 						roles: PoolRoles {
 							depositor: 11,
-							root: 123,
-							nominator: 456,
-							state_toggler: 789
+							root: Some(123),
+							nominator: Some(456),
+							state_toggler: Some(789)
 						}
 					}
 				}
@@ -3590,71 +3592,164 @@ mod update_roles {
 		ExtBuilder::default().build_and_execute(|| {
 			assert_eq!(
 				BondedPools::<Runtime>::get(1).unwrap().roles,
-				PoolRoles { depositor: 10, root: 900, nominator: 901, state_toggler: 902 },
+				PoolRoles {
+					depositor: 10,
+					root: Some(900),
+					nominator: Some(901),
+					state_toggler: Some(902)
+				},
 			);
 
 			// non-existent pools
 			assert_noop!(
-				Pools::update_roles(Origin::signed(1), 2, Some(5), Some(6), Some(7)),
+				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, Some(5), Some(6), Some(7)),
+				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, Some(5), Some(6), Some(7)),
+				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, Some(5), Some(6), Some(7)),
+				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, Some(5), Some(6), Some(7)));
+			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 }
+					Event::RolesUpdated {
+						root: Some(5),
+						state_toggler: Some(7),
+						nominator: Some(6)
+					}
 				]
 			);
 			assert_eq!(
 				BondedPools::<Runtime>::get(1).unwrap().roles,
-				PoolRoles { depositor: 10, root: 5, nominator: 6, state_toggler: 7 },
+				PoolRoles {
+					depositor: 10,
+					root: Some(5),
+					nominator: Some(6),
+					state_toggler: Some(7)
+				},
 			);
 
 			// also root origin can
-			assert_ok!(Pools::update_roles(Origin::root(), 1, Some(1), Some(2), Some(3)));
+			assert_ok!(Pools::update_roles(
+				Origin::root(),
+				1,
+				ConfigOp::Set(1),
+				ConfigOp::Set(2),
+				ConfigOp::Set(3)
+			));
 
 			assert_eq!(
 				pool_events_since_last_call(),
-				vec![Event::RolesUpdated { root: 1, state_toggler: 3, nominator: 2 }]
+				vec![Event::RolesUpdated {
+					root: Some(1),
+					state_toggler: Some(3),
+					nominator: Some(2)
+				}]
 			);
 			assert_eq!(
 				BondedPools::<Runtime>::get(1).unwrap().roles,
-				PoolRoles { depositor: 10, root: 1, nominator: 2, state_toggler: 3 },
+				PoolRoles {
+					depositor: 10,
+					root: Some(1),
+					nominator: Some(2),
+					state_toggler: Some(3)
+				},
 			);
 
-			// None is a noop
-			assert_ok!(Pools::update_roles(Origin::root(), 1, Some(11), None, None));
+			// Noop works
+			assert_ok!(Pools::update_roles(
+				Origin::root(),
+				1,
+				ConfigOp::Set(11),
+				ConfigOp::Noop,
+				ConfigOp::Noop
+			));
 
 			assert_eq!(
 				pool_events_since_last_call(),
-				vec![Event::RolesUpdated { root: 11, state_toggler: 3, nominator: 2 }]
+				vec![Event::RolesUpdated {
+					root: Some(11),
+					state_toggler: Some(3),
+					nominator: Some(2)
+				}]
 			);
 
 			assert_eq!(
 				BondedPools::<Runtime>::get(1).unwrap().roles,
-				PoolRoles { depositor: 10, root: 11, nominator: 2, state_toggler: 3 },
+				PoolRoles {
+					depositor: 10,
+					root: Some(11),
+					nominator: Some(2),
+					state_toggler: Some(3)
+				},
+			);
+
+			// Remove works
+			assert_ok!(Pools::update_roles(
+				Origin::root(),
+				1,
+				ConfigOp::Set(69),
+				ConfigOp::Remove,
+				ConfigOp::Remove
+			));
+
+			assert_eq!(
+				pool_events_since_last_call(),
+				vec![Event::RolesUpdated { root: Some(69), state_toggler: None, nominator: None }]
+			);
+
+			assert_eq!(
+				BondedPools::<Runtime>::get(1).unwrap().roles,
+				PoolRoles { depositor: 10, root: Some(69), nominator: None, state_toggler: None },
 			);
 		})
 	}

substrate/frame/staking/src/lib.rs

@@ -533,10 +533,12 @@ impl<T: Config> StakingLedger<T> {
 	/// case that either the active bonded or some unlocking chunks become dust after slashing.
 	/// Returns the amount of funds actually slashed.
 	///
+	/// `slash_era` is the era in which the slash (which is being enacted now) actually happened.
+	///
 	/// # Note
 	///
-	/// This calls `Config::OnStakerSlash::on_slash` with information as to how the slash
-	/// was applied.
+	/// This calls `Config::OnStakerSlash::on_slash` with information as to how the slash was
+	/// applied.
 	fn slash(
 		&mut self,
 		slash_amount: BalanceOf<T>,
@@ -615,6 +617,7 @@ impl<T: Config> StakingLedger<T> {
 				break
 			}
 		}
+
 		self.unlocking.retain(|c| !c.value.is_zero());
 		T::OnStakerSlash::on_slash(&self.stash, self.active, &slashed_unlocking);
 		pre_slash_total.saturating_sub(self.total)

substrate/frame/staking/src/tests.rs

@@ -2787,6 +2787,80 @@ fn deferred_slashes_are_deferred() {
 	})
 }
 
+#[test]
+fn staker_cannot_bail_deferred_slash() {
+	// as long as SlashDeferDuration is less than BondingDuration, this should not be possible.
+	ExtBuilder::default().slash_defer_duration(2).build_and_execute(|| {
+		mock::start_active_era(1);
+
+		assert_eq!(Balances::free_balance(11), 1000);
+		assert_eq!(Balances::free_balance(101), 2000);
+
+		let exposure = Staking::eras_stakers(active_era(), 11);
+		let nominated_value = exposure.others.iter().find(|o| o.who == 101).unwrap().value;
+
+		on_offence_now(
+			&[OffenceDetails {
+				offender: (11, Staking::eras_stakers(active_era(), 11)),
+				reporters: vec![],
+			}],
+			&[Perbill::from_percent(10)],
+		);
+
+		// now we chill
+		assert_ok!(Staking::chill(Origin::signed(100)));
+		assert_ok!(Staking::unbond(Origin::signed(100), 500));
+
+		assert_eq!(Staking::current_era().unwrap(), 1);
+		assert_eq!(active_era(), 1);
+
+		assert_eq!(
+			Ledger::<Test>::get(100).unwrap(),
+			StakingLedger {
+				active: 0,
+				total: 500,
+				stash: 101,
+				claimed_rewards: Default::default(),
+				unlocking: bounded_vec![UnlockChunk { era: 4u32, value: 500 }],
+			}
+		);
+
+		// no slash yet.
+		assert_eq!(Balances::free_balance(11), 1000);
+		assert_eq!(Balances::free_balance(101), 2000);
+
+		// no slash yet.
+		mock::start_active_era(2);
+		assert_eq!(Balances::free_balance(11), 1000);
+		assert_eq!(Balances::free_balance(101), 2000);
+		assert_eq!(Staking::current_era().unwrap(), 2);
+		assert_eq!(active_era(), 2);
+
+		// no slash yet.
+		mock::start_active_era(3);
+		assert_eq!(Balances::free_balance(11), 1000);
+		assert_eq!(Balances::free_balance(101), 2000);
+		assert_eq!(Staking::current_era().unwrap(), 3);
+		assert_eq!(active_era(), 3);
+
+		// and cannot yet unbond:
+		assert_storage_noop!(assert!(Staking::withdraw_unbonded(Origin::signed(100), 0).is_ok()));
+		assert_eq!(
+			Ledger::<Test>::get(100).unwrap().unlocking.into_inner(),
+			vec![UnlockChunk { era: 4u32, value: 500 as Balance }],
+		);
+
+		// at the start of era 4, slashes from era 1 are processed,
+		// after being deferred for at least 2 full eras.
+		mock::start_active_era(4);
+
+		assert_eq!(Balances::free_balance(11), 900);
+		assert_eq!(Balances::free_balance(101), 2000 - (nominated_value / 10));
+
+		// and the leftover of the funds can now be unbonded.
+	})
+}
+
 #[test]
 fn remove_deferred() {
 	ExtBuilder::default().slash_defer_duration(2).build_and_execute(|| {
@@ -4856,7 +4930,7 @@ fn force_apply_min_commission_works() {
 }
 
 #[test]
-fn ledger_slash_works() {
+fn proportional_ledger_slash_works() {
 	let c = |era, value| UnlockChunk::<Balance> { era, value };
 	// Given
 	let mut ledger = StakingLedger::<Test> {