Refactor session pallet to replace NamedReservableCurrency with hold functionality for key management. Update related types and events to reflect changes from reservation to holding of funds.

Author: Krayt78

substrate/frame/session/src/lib.rs

@@ -130,8 +130,8 @@ use frame_support::{
 	ensure,
 	traits::{
 		Defensive, EstimateNextNewSession, EstimateNextSessionRotation, FindAuthor, Get,
-		OneSessionHandler, ValidatorRegistration, ValidatorSet, Currency, NamedReservableCurrency,
-		ReservableCurrency,
+		OneSessionHandler, ValidatorRegistration, ValidatorSet,
+		fungible::{hold::Mutate as HoldMutate, hold::Inspect as HoldInspect, Inspect}, 
 	},
 	weights::Weight,
 	Parameter,
@@ -398,9 +398,9 @@ pub mod pallet {
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
-	/// A simple identifier for session keys.
+	/// A simple identifier for session keys hold reason.
 	#[derive(codec::Encode, codec::Decode, codec::MaxEncodedLen, scale_info::TypeInfo, Debug, PartialEq, Eq, Clone)]
-	pub struct SessionKeysReserveId;
+	pub struct SessionKeysHoldReason;
 
 	#[pallet::config]
 	pub trait Config: frame_system::Config {
@@ -442,19 +442,19 @@ pub mod pallet {
 		/// Weight information for extrinsics in this pallet.
 		type WeightInfo: WeightInfo;
 
-		/// The currency type for reserving when setting keys.
-		type Currency: NamedReservableCurrency<Self::AccountId>;
+		/// The currency type for placing holds when setting keys.
+		type Currency: HoldMutate<Self::AccountId> + Inspect<Self::AccountId>;
 
-		/// The reserve identifier type.
-		type ReserveIdentifier: Get<<Self::Currency as NamedReservableCurrency<Self::AccountId>>::ReserveIdentifier> + TypeInfo + 'static;
+		/// The hold reason type.
+		type HoldReason: Get<<Self::Currency as HoldInspect<Self::AccountId>>::Reason> + TypeInfo + 'static;
 
-		/// The amount to be reserved when setting keys.
+		/// The amount to be held when setting keys.
 		#[pallet::constant]
 		type KeyDeposit: Get<BalanceOf<Self>>;
 	}
 
 	// Add a type alias for the balance
-	type BalanceOf<T> = <<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
+	type BalanceOf<T> = <<T as Config>::Currency as Inspect<<T as frame_system::Config>::AccountId>>::Balance;
 
 	#[pallet::genesis_config]
 	#[derive(frame_support::DefaultNoBound)]
@@ -576,10 +576,10 @@ pub mod pallet {
 		ValidatorDisabled { validator: T::ValidatorId },
 		/// Validator has been re-enabled.
 		ValidatorReenabled { validator: T::ValidatorId },
-		/// Funds have been reserved for setting keys.
-		KeysFundsReserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance },
-		/// Funds have been unreserved when purging keys.
-		KeysFundsUnreserved { account: T::AccountId, amount: <<T as Config>::Currency as Currency<T::AccountId>>::Balance },
+		/// Funds have been held for setting keys.
+		KeysFundsHeld { account: T::AccountId, amount: BalanceOf<T> },
+		/// Funds have been released when purging keys.
+		KeysFundsReleased { account: T::AccountId, amount: BalanceOf<T> },
 	}
 
 	/// Error for the session pallet.
@@ -868,11 +868,9 @@ impl<T: Config> Pallet<T> {
 		// For new registrations, ensure the account has enough funds for the deposit
 		if is_new_registration {
 			let deposit = T::KeyDeposit::get();
-			let _= T::ReserveIdentifier::get();
-			// Since NamedReservableCurrency doesn't directly expose can_reserve,
-			// we need to use the normal reserve method for checking
+			let reason = T::HoldReason::get();
 			ensure!(
-				<T::Currency as ReservableCurrency<_>>::can_reserve(account, deposit),
+				<T::Currency as HoldInspect<_>>::can_hold(&reason, account, deposit),
 				Error::<T>::InsufficientFunds
 			);
 		}
@@ -881,17 +879,17 @@ impl<T: Config> Pallet<T> {
 		// Pass old_keys to avoid another storage read
 		Self::inner_set_keys(&who, keys, old_keys)?;
 
-		// Reserve deposit if this is a new registration
+		// Place deposit on hold if this is a new registration
 		if is_new_registration {
 			let deposit = T::KeyDeposit::get();
-			let id = T::ReserveIdentifier::get();
-			T::Currency::reserve_named(&id, account, deposit)?;
+			let reason = T::HoldReason::get();
+			T::Currency::hold(&reason, account, deposit)?;
 
 			let assertion = frame_system::Pallet::<T>::inc_consumers(account).is_ok();
 			debug_assert!(assertion, "can_inc_consumer() returned true; no change since; qed");
 
-			// Emit an event for the reserved funds
-			Self::deposit_event(Event::<T>::KeysFundsReserved { 
+			// Emit an event for the held funds
+			Self::deposit_event(Event::<T>::KeysFundsHeld { 
 				account: account.clone(),
 				amount: deposit,
 			});
@@ -952,16 +950,17 @@ impl<T: Config> Pallet<T> {
 			Self::clear_key_owner(*id, key_data);
 		}
 
-		// Unreserve the deposit using the named reserve
-		let deposit = T::KeyDeposit::get();
-		let id = T::ReserveIdentifier::get();
-		let _ = T::Currency::unreserve_named(&id, account, deposit);
+		// Release the deposit from hold
+		let reason = T::HoldReason::get();
 
-		// Emit an event for the unreserved funds
-		Self::deposit_event(Event::<T>::KeysFundsUnreserved { 
-			account: account.clone(),
-			amount: deposit,
-		});
+		// Use release_all to handle the case where the exact amount might not be available
+		if let Ok(released) = T::Currency::release_all(&reason, account, frame_support::traits::tokens::Precision::BestEffort) {
+			// Emit an event for the released funds
+			Self::deposit_event(Event::<T>::KeysFundsReleased { 
+				account: account.clone(),
+				amount: released,
+			});
+		}
 
 		frame_system::Pallet::<T>::dec_consumers(account);
 

substrate/frame/session/src/mock.rs

@@ -31,7 +31,10 @@ use sp_state_machine::BasicExternalities;
 
 use frame_support::{
 	derive_impl, parameter_types, traits::{ConstU64, WithdrawReasons, Currency, ReservableCurrency, 
-	SignedImbalance, NamedReservableCurrency, BalanceStatus}
+	SignedImbalance, tokens::{fungible::{
+		hold::{Mutate as HoldMutate, Inspect as HoldInspect, Unbalanced as UnbalancedHold},
+		Inspect as FungibleInspect, Unbalanced as FungibleUnbalanced, Dust
+	}, Preservation, Fortitude}}, 
 };
 
 impl_opaque_keys! {
@@ -263,10 +266,10 @@ pub(crate) const DISABLING_LIMIT_FACTOR: usize = 3;
 pub type SessionKeysId = [u8; 12];
 pub const TEST_SESSION_KEYS_ID: SessionKeysId = *b"session_keys";
 
-// Define TestReserveIdentifier with the necessary traits
+// Define TestHoldReason with the necessary traits
 #[derive(Debug, Clone, PartialEq, Eq, codec::Encode, codec::Decode, scale_info::TypeInfo)]
-pub struct TestReserveIdentifier;
-impl Get<SessionKeysId> for TestReserveIdentifier {
+pub struct TestHoldReason;
+impl Get<SessionKeysId> for TestHoldReason {
 	fn get() -> SessionKeysId {
 		TEST_SESSION_KEYS_ID
 	}
@@ -288,7 +291,7 @@ impl Config for Test {
 		disabling::UpToLimitWithReEnablingDisablingStrategy<DISABLING_LIMIT_FACTOR>;
 	type WeightInfo = ();
 	type Currency = pallet_balances::Pallet<Test>;
-	type ReserveIdentifier = TestReserveIdentifier;
+	type HoldReason = TestHoldReason;
 	type KeyDeposit = KeyDeposit;
 }
 
@@ -301,6 +304,10 @@ impl crate::historical::Config for Test {
 pub mod pallet_balances {
 	use super::*;
 	use frame_support::pallet_prelude::*;
+	use frame_support::traits::{
+		tokens::{WithdrawConsequence, Provenance, DepositConsequence},
+		fungible::Dust,
+	};
 
 	pub struct Pallet<T>(core::marker::PhantomData<T>);
 
@@ -466,12 +473,76 @@ pub mod pallet_balances {
 		}
 	}
 
-	impl<T> NamedReservableCurrency<u64> for Pallet<T> {
-		type ReserveIdentifier = [u8; 12];
+	impl<T> FungibleInspect<u64> for Pallet<T> {
+		type Balance = u64;
+		
+		fn total_issuance() -> Self::Balance {
+			CurrencyBalance::get()
+		}
+		
+		fn minimum_balance() -> Self::Balance {
+			0
+		}
+		
+		fn balance(_who: &u64) -> Self::Balance {
+			CurrencyBalance::get()
+		}
+		
+		fn total_balance(_who: &u64) -> Self::Balance {
+			CurrencyBalance::get()
+		}
+		
+		fn reducible_balance(
+			_who: &u64, 
+			_keep_alive: Preservation, 
+			_force: Fortitude,
+		) -> Self::Balance {
+			CurrencyBalance::get()
+		}
+		
+		fn can_deposit(
+			_who: &u64,
+			_amount: Self::Balance,
+			_provenance: Provenance,
+		) -> DepositConsequence {
+			DepositConsequence::Success
+		}
+		
+		fn can_withdraw(
+			who: &u64,
+			amount: Self::Balance,
+		) -> WithdrawConsequence<Self::Balance> {
+			if *who == 999 || amount > CurrencyBalance::get() {
+				WithdrawConsequence::BalanceLow
+			} else {
+				WithdrawConsequence::Success
+			}
+		}
+	}
+	
+	impl<T> FungibleUnbalanced<u64> for Pallet<T> {
+		fn handle_dust(_dust: Dust<u64, Self>) {
+			// No-op in mock
+		}
+		
+		fn write_balance(
+			_who: &u64, 
+			_amount: Self::Balance
+		) -> Result<Option<Self::Balance>, DispatchError> {
+			Ok(None)
+		}
+		
+		fn set_total_issuance(_amount: Self::Balance) {
+			// No-op in mock
+		}
+	}
+	
+	impl<T> HoldInspect<u64> for Pallet<T> {
+		type Reason = SessionKeysId;
 
-		fn reserved_balance_named(id: &Self::ReserveIdentifier, who: &u64) -> Self::Balance {
+		fn balance_on_hold(reason: &Self::Reason, who: &u64) -> Self::Balance {
 			// Convert fixed array to Vec for lookup
-			let id_vec = id.to_vec();
+			let id_vec = reason.to_vec();
 
 			ReservedBalances::get()
 				.get(who)
@@ -480,81 +551,42 @@ pub mod pallet_balances {
 				.unwrap_or(0)
 		}
 
-		fn slash_reserved_named(
-			_id: &Self::ReserveIdentifier,
-			_who: &u64,
-			_amount: Self::Balance,
-		) -> (Self::NegativeImbalance, Self::Balance) {
-			((), 0)
+		fn total_balance_on_hold(who: &u64) -> Self::Balance {
+			// Sum up all held balances for the account
+			ReservedBalances::get()
+				.get(who)
+				.map(|holds| holds.values().sum())
+				.unwrap_or(0)
 		}
 
-		fn reserve_named(
-			id: &Self::ReserveIdentifier,
+		fn can_hold(_reason: &Self::Reason, who: &u64, amount: Self::Balance) -> bool {
+			// Account 999 is special and always has insufficient funds for testing
+			if *who == 999 {
+				return false
+			}
+			CurrencyBalance::get() >= amount
+		}
+	}
+	
+	impl<T> UnbalancedHold<u64> for Pallet<T> {
+		fn set_balance_on_hold(
+			reason: &Self::Reason,
 			who: &u64,
 			amount: Self::Balance,
 		) -> DispatchResult {
-			if !Self::can_reserve(who, amount) {
-				return Err(DispatchError::Other("InsufficientBalance"))
-			}
-			
 			// Convert fixed array to Vec for storage
-			let id_vec = id.to_vec();
+			let id_vec = reason.to_vec();
 
-			// Update the reserved balance
+			// Update the held balance
 			ReservedBalances::mutate(|balances| {
-				let account_reserves = balances.entry(*who).or_insert_with(BTreeMap::new);
-				let reserved = account_reserves.entry(id_vec).or_insert(0);
-				*reserved += amount;
+				let account_holds = balances.entry(*who).or_insert_with(BTreeMap::new);
+				account_holds.insert(id_vec, amount);
 			});
 
 			Ok(())
 		}
-		
-		fn unreserve_named(
-			id: &Self::ReserveIdentifier,
-			who: &u64,
-			amount: Self::Balance,
-		) -> Self::Balance {
-			// Convert fixed array to Vec for lookup/storage
-			let id_vec = id.to_vec();
-			
-			// Get the current reserved amount
-			let mut remaining = amount;
-			ReservedBalances::mutate(|balances| {
-				if let Some(account_reserves) = balances.get_mut(who) {
-					if let Some(reserved) = account_reserves.get_mut(&id_vec) {
-						if *reserved >= amount {
-							*reserved -= amount;
-							remaining = 0;
-						} else {
-							remaining = amount - *reserved;
-							*reserved = 0;
-						}
-						
-						// Clean up empty reserves
-						if *reserved == 0 {
-							account_reserves.remove(&id_vec);
-						}
-					}
-					
-					// Clean up empty accounts
-					if account_reserves.is_empty() {
-						balances.remove(who);
-					}
-				}
-			});
-			
-			remaining
-		}
-		
-		fn repatriate_reserved_named(
-			_id: &Self::ReserveIdentifier,
-			_slashed: &u64,
-			_beneficiary: &u64,
-			_amount: Self::Balance,
-			_status: BalanceStatus,
-		) -> Result<Self::Balance, DispatchError> {
-			Ok(0)
-		}
 	}
+	
+	impl<T> HoldMutate<u64> for Pallet<T> {}
 }
+