[Fix] Make sure pool metadata is removed on pool dissolve

frame/nomination-pools/src/lib.rs

@@ -1141,7 +1141,7 @@ pub mod pallet {
 	use sp_runtime::traits::CheckedAdd;
 
 	/// The current storage version.
-	const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
+	const STORAGE_VERSION: StorageVersion = StorageVersion::new(3);
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(crate) trait Store)]
@@ -2186,8 +2186,8 @@ impl<T: Config> Pallet<T> {
 	}
 	/// Remove everything related to the given bonded pool.
 	///
-	/// All sub-pools are also deleted. All accounts are dusted and the leftover of the reward
-	/// account is returned to the depositor.
+	/// Metadata and all of the sub-pools are also deleted. All accounts are dusted and the leftover
+	/// of the reward account is returned to the depositor.
 	pub fn dissolve_pool(bonded_pool: BondedPool<T>) {
 		let reward_account = bonded_pool.reward_account();
 		let bonded_account = bonded_pool.bonded_account();
@@ -2224,9 +2224,15 @@ impl<T: Config> Pallet<T> {
 		T::Currency::make_free_balance_be(&bonded_pool.bonded_account(), Zero::zero());
 
 		Self::deposit_event(Event::<T>::Destroyed { pool_id: bonded_pool.id });
+		Self::remove_metadata(bonded_pool.id);
 		bonded_pool.remove();
 	}
 
+	/// Remove bonded pool metadata.
+	pub fn remove_metadata(bonded_pool_id: PoolId) {
+		Metadata::<T>::remove(bonded_pool_id);
+	}
+
 	/// Create the main, bonded account of a pool with the given id.
 	pub fn create_bonded_account(id: PoolId) -> T::AccountId {
 		T::PalletId::get().into_sub_account_truncating((AccountType::Bonded, id))

frame/nomination-pools/src/migration.rs

@@ -384,3 +384,73 @@ pub mod v2 {
 		}
 	}
 }
+
+pub mod v3 {
+	use super::*;
+
+	const CURRENT: u16 = 3;
+	const ONCHAIN: u16 = 2;
+
+	fn check_version(current: StorageVersion, onchain: StorageVersion) -> bool {
+		current == CURRENT && onchain == ONCHAIN
+	}
+
+	pub struct MigrateToV3<T>(sp_std::marker::PhantomData<T>);
+	impl<T: Config> OnRuntimeUpgrade for MigrateToV3<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 check_version(current, onchain) {
+				let mut metadata_iterated = 0u64;
+				let mut metadata_removed = 0u64;
+				Metadata::<T>::iter_keys()
+					.filter(|id| {
+						metadata_iterated += 1;
+						!BondedPools::<T>::contains_key(&id)
+					})
+					.for_each(|id| {
+						metadata_removed += 1;
+						Metadata::<T>::remove(&id);
+					});
+				// metadata iterated + bonded pools read + a storage version read
+				let total_reads = metadata_iterated * 2 + 1;
+				T::DbWeight::get().reads_writes(total_reads, metadata_removed)
+			} else {
+				log!(info, "MigrateToV3 should be removed");
+				T::DbWeight::get().reads(1)
+			}
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn pre_upgrade() -> Result<(), &'static str> {
+			ensure!(
+				!check_version(
+					Pallet::<T>::current_storage_version(),
+					Pallet::<T>::on_chain_storage_version()
+				),
+				"must upgrade linearly"
+			);
+			Ok(())
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn post_upgrade() -> Result<(), &'static str> {
+			ensure!(
+				Metadata::<T>::iter_keys()
+					.filter(|id| !BondedPools::<T>::contains_key(&id))
+					.count() == 0,
+				"not all of the stale metadata has been removed"
+			);
+			ensure!(Pallet::<T>::current_storage_version() == 3, "wrong storage version");
+			Ok(())
+		}
+	}
+}

frame/nomination-pools/src/tests.rs

@@ -1911,6 +1911,10 @@ mod claim_payout {
 			assert_ok!(Pools::withdraw_unbonded(Origin::signed(20), 20, 0));
 			assert_ok!(fully_unbond_permissioned(10));
 
+			// set metadata to check that it's being removed on dissolve
+			assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1]));
+			assert!(Metadata::<T>::contains_key(1));
+
 			CurrentEra::set(6);
 			assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0));
 
@@ -1928,6 +1932,7 @@ mod claim_payout {
 				]
 			);
 
+			assert!(!Metadata::<T>::contains_key(1));
 			// original ed + ed put into reward account + reward + bond + dust.
 			assert_eq!(Balances::free_balance(&10), 35 + 5 + 13 + 10 + 1);
 		})
@@ -3148,6 +3153,10 @@ mod withdraw_unbonded {
 				current_era += 3;
 				CurrentEra::set(current_era);
 
+				// set metadata to check that it's being removed on dissolve
+				assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1]));
+				assert!(Metadata::<T>::contains_key(1));
+
 				// when
 				assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0));
 				assert_eq!(
@@ -3159,6 +3168,7 @@ mod withdraw_unbonded {
 						Event::Destroyed { pool_id: 1 }
 					]
 				);
+				assert!(!Metadata::<T>::contains_key(1));
 				assert_eq!(
 					balances_events_since_last_call(),
 					vec![
@@ -3269,6 +3279,10 @@ mod withdraw_unbonded {
 
 				CurrentEra::set(CurrentEra::get() + 3);
 
+				// set metadata to check that it's being removed on dissolve
+				assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1]));
+				assert!(Metadata::<T>::contains_key(1));
+
 				// when
 				assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0));
 
@@ -3287,6 +3301,7 @@ mod withdraw_unbonded {
 						Event::Destroyed { pool_id: 1 }
 					]
 				);
+				assert!(!Metadata::<T>::contains_key(1));
 				assert_eq!(
 					balances_events_since_last_call(),
 					vec![
@@ -3764,6 +3779,10 @@ mod withdraw_unbonded {
 			);
 			assert_ok!(Pools::unbond(Origin::signed(10), 10, 10));
 
+			// set metadata to check that it's being removed on dissolve
+			assert_ok!(Pools::set_metadata(Origin::signed(900), 1, vec![1, 1]));
+			assert!(Metadata::<T>::contains_key(1));
+
 			// now the 7 should be free.
 			CurrentEra::set(3);
 			assert_ok!(Pools::withdraw_unbonded(Origin::signed(10), 10, 0));
@@ -3797,6 +3816,7 @@ mod withdraw_unbonded {
 					Event::Destroyed { pool_id: 1 },
 				]
 			);
+			assert!(!Metadata::<T>::contains_key(1));
 		})
 	}
 }
@@ -4039,7 +4059,7 @@ mod set_state {
 			// Then
 			assert_eq!(BondedPool::<Runtime>::get(1).unwrap().state, PoolState::Destroying);
 
-			// If the pool is not ok to be open, it cannot be permissionleslly set to a state that
+			// If the pool is not ok to be open, it cannot be permissionlessly set to a state that
 			// isn't destroying
 			unsafe_set_state(1, PoolState::Open);
 			assert_noop!(