[AHM] Poke deposits: Indices pallet

prdoc/pr_7587.prdoc

@@ -0,0 +1,20 @@
+title: Poke deposits
+doc:
+- audience: Runtime Dev
+  description: |-
+    # Description
+
+    * This PR adds a new extrinsic `reconsider` to `pallet-indices`. This extrinsic will be used to re-adjust the deposits made in the pallet.
+    * Part of #5591
+
+    ## Review Notes
+
+    * Added a new extrinsic `reconsider` in `pallet-indices`.
+    * Added a new event `DepositReconsidered` to be emitted upon a successful call of the extrinsic.
+    * Although the immediate use of the extrinsic will be to give back some of the deposit after the AH-migration, the extrinsic is written such that it can work if the deposit decreases or increases (both).
+    * The call to the extrinsic would be `free` if an actual adjustment is made to the deposit and `paid` otherwise.
+    * Added tests to test all scenarios.
+    * Added a benchmark to test the "worst case" (maximum compute) flow of the extrinsic which is when the deposit amount is updated to a new value.
+crates:
+- name: pallet-indices
+  bump: major

substrate/frame/indices/src/benchmarking.rs

@@ -112,6 +112,32 @@ mod benchmarks {
 		Ok(())
 	}
 
+	#[benchmark]
+	fn reconsider() -> Result<(), BenchmarkError> {
+		let account_index = T::AccountIndex::from(SEED);
+		// Setup accounts
+		let caller: T::AccountId = whitelisted_caller();
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		// Set initial deposit to 10
+		mock::IndexDeposit::set(10);
+		assert_eq!(T::Deposit::get(), 10u64.into());
+		// Claim the index
+		Pallet::<T>::claim(RawOrigin::Signed(caller.clone()).into(), account_index)?;
+		// Verify the initial deposit amount in storage
+		assert_eq!(Accounts::<T>::get(account_index).unwrap().1, 10u64.into());
+		// Set new deposit value to 1
+		mock::IndexDeposit::set(1);
+		assert_eq!(T::Deposit::get(), 1u64.into());
+
+		#[extrinsic_call]
+		_(RawOrigin::Signed(caller.clone()), account_index);
+
+		assert!(Accounts::<T>::contains_key(account_index));
+		assert_eq!(Accounts::<T>::get(account_index).unwrap().0, caller);
+		assert_eq!(Accounts::<T>::get(account_index).unwrap().1, 1u64.into());
+		Ok(())
+	}
+
 	// TODO in another PR: lookup and unlookup trait weights (not critical)
 
 	impl_benchmark_test_suite!(Pallet, mock::new_test_ext(), mock::Test);

substrate/frame/indices/src/lib.rs

@@ -232,6 +232,57 @@ pub mod pallet {
 			Self::deposit_event(Event::IndexFrozen { index, who });
 			Ok(())
 		}
+
+		/// Reconsider the deposit reserved for an index.
+		/// 
+		/// The dispatch origin for this call must be _Signed_ and the signing account must have a
+		/// non-frozen account `index`.
+		/// 
+		/// The transaction fees is waived if the deposit is changed after reconsideration.
+		/// 
+		/// - `index`: the index whose deposit is to be reconsidered.
+		/// 
+		/// Emits `DepositReconsidered` if successful.
+		/// 
+		/// ## Complexity
+		/// - `O(1)`.
+		#[pallet::call_index(5)]
+		#[pallet::weight(T::WeightInfo::reconsider())]
+		pub fn reconsider(origin: OriginFor<T>, index: T::AccountIndex) -> DispatchResultWithPostInfo {
+			let who = ensure_signed(origin)?;
+
+			Accounts::<T>::try_mutate(index, |maybe_value| -> DispatchResultWithPostInfo {
+				let (account, old_amount, perm) = maybe_value.take().ok_or(Error::<T>::NotAssigned)?;
+				ensure!(!perm, Error::<T>::Permanent);
+				ensure!(account == who, Error::<T>::NotOwner);
+
+				let new_amount = T::Deposit::get();
+
+				if new_amount > old_amount {
+					// Need to reserve more
+					let extra = new_amount.saturating_sub(old_amount);
+					T::Currency::reserve(&who, extra)?;
+				} else if new_amount < old_amount {
+					// Need to unreserve some
+					let excess = old_amount.saturating_sub(new_amount);
+					T::Currency::unreserve(&who, excess);
+				}
+
+				*maybe_value = Some((account, new_amount, perm));
+
+				if old_amount != new_amount {
+					Self::deposit_event(Event::DepositReconsidered {
+						who,
+						index,
+						old_deposit: old_amount,
+						new_deposit: new_amount,
+					});
+					Ok(Pays::No.into())
+				} else {
+					Ok(Pays::Yes.into())
+				}
+			})
+		}
 	}
 
 	#[pallet::event]
@@ -243,6 +294,8 @@ pub mod pallet {
 		IndexFreed { index: T::AccountIndex },
 		/// A account index has been frozen to its current account ID.
 		IndexFrozen { index: T::AccountIndex, who: T::AccountId },
+		/// A deposit to reserve an index has been reconsidered.
+		DepositReconsidered { who: T::AccountId, index: T::AccountIndex, old_deposit: BalanceOf<T>, new_deposit: BalanceOf<T> },
 	}
 
 	#[pallet::error]

substrate/frame/indices/src/mock.rs

@@ -20,11 +20,15 @@
 #![cfg(test)]
 
 use crate::{self as pallet_indices, Config};
-use frame_support::{derive_impl, traits::ConstU64};
+use frame_support::{derive_impl, parameter_types};
 use sp_runtime::BuildStorage;
 
 type Block = frame_system::mocking::MockBlock<Test>;
 
+parameter_types! {
+	pub static IndexDeposit: u64 = 1;
+}
+
 frame_support::construct_runtime!(
 	pub enum Test
 	{
@@ -50,7 +54,7 @@ impl pallet_balances::Config for Test {
 impl Config for Test {
 	type AccountIndex = u64;
 	type Currency = Balances;
-	type Deposit = ConstU64<1>;
+	type Deposit = IndexDeposit;
 	type RuntimeEvent = RuntimeEvent;
 	type WeightInfo = ();
 }
@@ -63,5 +67,8 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
 	}
 	.assimilate_storage(&mut t)
 	.unwrap();
-	t.into()
+	let mut ext: sp_io::TestExternalities = t.into();
+	// Initialize the block number to 1 for event registration
+	ext.execute_with(|| System::set_block_number(1));
+	ext
 }

substrate/frame/indices/src/tests.rs

@@ -20,7 +20,7 @@
 #![cfg(test)]
 
 use super::{mock::*, *};
-use frame_support::{assert_noop, assert_ok};
+use frame_support::{assert_noop, assert_ok, pallet_prelude::Pays};
 use pallet_balances::Error as BalancesError;
 use sp_runtime::MultiAddress::Id;
 
@@ -119,3 +119,128 @@ fn force_transfer_index_on_free_should_work() {
 		assert_eq!(Indices::lookup_index(0), Some(3));
 	});
 }
+
+#[test]
+fn reconsider_should_fail_for_unassigned_index() {
+	new_test_ext().execute_with(|| {
+		assert_noop!(
+			Indices::reconsider(Some(1).into(), 0),
+			Error::<Test>::NotAssigned
+		);
+	});
+}
+
+#[test]
+fn reconsider_should_fail_for_wrong_owner() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+		assert_noop!(
+			Indices::reconsider(Some(2).into(), 0),
+			Error::<Test>::NotOwner
+		);
+	});
+}
+
+#[test]
+fn reconsider_should_fail_for_permanent_index() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+		assert_ok!(Indices::freeze(Some(1).into(), 0));
+		assert_noop!(
+			Indices::reconsider(Some(1).into(), 0),
+			Error::<Test>::Permanent
+		);
+	});
+}
+
+#[test]
+fn reconsider_should_fail_for_insufficient_balance() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+
+		// Set deposit higher than available balance
+		IndexDeposit::set(1000);
+
+		assert_noop!(
+			Indices::reconsider(Some(1).into(), 0),
+			BalancesError::<Test, _>::InsufficientBalance
+		);
+	});
+}
+
+#[test]
+fn reconsider_should_work_when_deposit_increases() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+		assert_eq!(Balances::reserved_balance(1), 1);
+
+		// Change deposit to 2
+		IndexDeposit::set(2);
+
+		// Reconsider should work and be free
+		let initial_balance = Balances::free_balance(1);
+		let result = Indices::reconsider(Some(1).into(), 0);
+		assert_ok!(result.as_ref());
+		let post_info = result.unwrap();
+		assert_eq!(post_info.pays_fee, Pays::No);
+		assert_eq!(Balances::reserved_balance(1), 2);
+
+		// Balance should only reduce by the deposit difference
+		assert_eq!(Balances::free_balance(1), initial_balance - 1);
+
+		System::assert_has_event(Event::DepositReconsidered { 
+			who: 1, 
+			index: 0, 
+			old_deposit: 1, 
+			new_deposit: 2 
+		}.into());
+	});
+}
+
+#[test]
+fn reconsider_should_work_when_deposit_decreases() {
+	new_test_ext().execute_with(|| {
+		// Set initial deposit to 2
+		IndexDeposit::set(2);
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+		assert_eq!(Balances::reserved_balance(1), 2);
+
+		// Change deposit to 1
+		IndexDeposit::set(1);
+
+		let initial_balance = Balances::free_balance(1);
+		let result = Indices::reconsider(Some(1).into(), 0);
+		assert_ok!(result.as_ref());
+		let post_info = result.unwrap();
+		assert_eq!(post_info.pays_fee, Pays::No);
+		assert_eq!(Balances::reserved_balance(1), 1);
+
+		// Balance should increase by the unreserved amount
+		assert_eq!(Balances::free_balance(1), initial_balance + 1);
+
+		System::assert_has_event(Event::DepositReconsidered {
+			who: 1,
+			index: 0,
+			old_deposit: 2,
+			new_deposit: 1
+		}.into());
+	});
+}
+
+#[test]
+fn reconsider_should_charge_fee_when_deposit_unchanged() {
+	new_test_ext().execute_with(|| {
+		assert_ok!(Indices::claim(Some(1).into(), 0));
+		assert_eq!(Balances::reserved_balance(1), 1);
+
+		// Reconsider with same deposit amount
+		let result = Indices::reconsider(Some(1).into(), 0);
+		assert_ok!(result.as_ref());
+		// Verify fee payment
+		let post_info = result.unwrap();
+		assert_eq!(post_info.pays_fee, Pays::Yes);
+
+		// Reserved balance should remain the same
+		assert_eq!(Balances::reserved_balance(1), 1);
+	});
+}