test(tokens): mutation hooks (OnDeposit, OnSlash, OnTransfer)

Author: daniel-savu

tokens/src/lib.rs

@@ -901,11 +901,11 @@ impl<T: Config> Pallet<T> {
 		amount: T::Balance,
 		existence_requirement: ExistenceRequirement,
 	) -> DispatchResult {
-		T::OnTransfer::on_transfer(currency_id, from, to, amount)?;
 		if amount.is_zero() || from == to {
 			return Ok(());
 		}
 
+		T::OnTransfer::on_transfer(currency_id, from, to, amount)?;
 		Self::try_mutate_account(to, currency_id, |to_account, _existed| -> DispatchResult {
 			Self::try_mutate_account(from, currency_id, |from_account, _existed| -> DispatchResult {
 				from_account.free = from_account
@@ -1027,11 +1027,11 @@ impl<T: Config> Pallet<T> {
 		require_existed: bool,
 		change_total_issuance: bool,
 	) -> DispatchResult {
-		T::OnDeposit::on_deposit(currency_id, who, amount)?;
 		if amount.is_zero() {
 			return Ok(());
 		}
 
+		T::OnDeposit::on_deposit(currency_id, who, amount)?;
 		Self::try_mutate_account(who, currency_id, |account, existed| -> DispatchResult {
 			if require_existed {
 				ensure!(existed, Error::<T>::DeadAccount);
@@ -1123,11 +1123,11 @@ impl<T: Config> MultiCurrency<T::AccountId> for Pallet<T> {
 	/// reserved funds, however we err on the side of punishment if things
 	/// are inconsistent or `can_slash` wasn't used appropriately.
 	fn slash(currency_id: Self::CurrencyId, who: &T::AccountId, amount: Self::Balance) -> Self::Balance {
-		T::OnSlash::on_slash(currency_id, who, amount);
 		if amount.is_zero() {
 			return amount;
 		}
 
+		T::OnSlash::on_slash(currency_id, who, amount);
 		let account = Self::accounts(who, currency_id);
 		let free_slashed_amount = account.free.min(amount);
 		// Cannot underflow because free_slashed_amount can never be greater than amount
@@ -1290,11 +1290,11 @@ impl<T: Config> MultiReservableCurrency<T::AccountId> for Pallet<T> {
 	///
 	/// Is a no-op if the value to be slashed is zero.
 	fn slash_reserved(currency_id: Self::CurrencyId, who: &T::AccountId, value: Self::Balance) -> Self::Balance {
-		T::OnSlash::on_slash(currency_id, who, value);
 		if value.is_zero() {
 			return value;
 		}
 
+		T::OnSlash::on_slash(currency_id, who, value);
 		let reserved_balance = Self::reserved_balance(currency_id, who);
 		let actual = reserved_balance.min(value);
 		Self::mutate_account(who, currency_id, |account, _| {

tokens/src/mock.rs

@@ -268,6 +268,55 @@ impl Happened<(AccountId, CurrencyId)> for TrackKilledAccounts {
 	}
 }
 
+thread_local! {
+	pub static ON_SLASH_CALLS: RefCell<u32> = RefCell::new(0);
+	pub static ON_DEPOSIT_CALLS: RefCell<u32> = RefCell::new(0);
+	pub static ON_TRANSFER_CALLS: RefCell<u32> = RefCell::new(0);
+}
+
+pub struct OnSlashHook<T>(marker::PhantomData<T>);
+impl<T: Config> OnSlash<T::AccountId, CurrencyId, Balance> for OnSlashHook<T> {
+	fn on_slash(_currency_id: CurrencyId, _account_id: &T::AccountId, _amount: Balance) {
+		ON_SLASH_CALLS.with(|cell| *cell.borrow_mut() += 1);
+	}
+}
+impl<T: Config> OnSlashHook<T> {
+	pub fn calls() -> u32 {
+		ON_SLASH_CALLS.with(|accounts| accounts.borrow().clone())
+	}
+}
+
+pub struct OnDepositHook<T>(marker::PhantomData<T>);
+impl<T: Config> OnDeposit<T::AccountId, CurrencyId, Balance> for OnDepositHook<T> {
+	fn on_deposit(_currency_id: CurrencyId, _account_id: &T::AccountId, _amount: Balance) -> DispatchResult {
+		ON_DEPOSIT_CALLS.with(|cell| *cell.borrow_mut() += 1);
+		Ok(())
+	}
+}
+impl<T: Config> OnDepositHook<T> {
+	pub fn calls() -> u32 {
+		ON_DEPOSIT_CALLS.with(|accounts| accounts.borrow().clone())
+	}
+}
+
+pub struct OnTransferHook<T>(marker::PhantomData<T>);
+impl<T: Config> OnTransfer<T::AccountId, CurrencyId, Balance> for OnTransferHook<T> {
+	fn on_transfer(
+		_currency_id: CurrencyId,
+		_from: &T::AccountId,
+		_to: &T::AccountId,
+		_amount: Balance,
+	) -> DispatchResult {
+		ON_TRANSFER_CALLS.with(|cell| *cell.borrow_mut() += 1);
+		Ok(())
+	}
+}
+impl<T: Config> OnTransferHook<T> {
+	pub fn calls() -> u32 {
+		ON_TRANSFER_CALLS.with(|accounts| accounts.borrow().clone())
+	}
+}
+
 parameter_types! {
 	pub DustReceiver: AccountId = PalletId(*b"orml/dst").into_account_truncating();
 }
@@ -280,9 +329,9 @@ impl Config for Runtime {
 	type WeightInfo = ();
 	type ExistentialDeposits = ExistentialDeposits;
 	type OnDust = TransferDust<Runtime, DustReceiver>;
-	type OnSlash = ();
-	type OnDeposit = ();
-	type OnTransfer = ();
+	type OnSlash = OnSlashHook<Runtime>;
+	type OnDeposit = OnDepositHook<Runtime>;
+	type OnTransfer = OnTransferHook<Runtime>;
 	type OnNewTokenAccount = TrackCreatedAccounts;
 	type OnKilledTokenAccount = TrackKilledAccounts;
 	type MaxLocks = ConstU32<2>;

tokens/src/tests.rs

@@ -1167,3 +1167,61 @@ fn lifecycle_callbacks_are_activated() {
 		assert_eq!(TrackKilledAccounts::accounts(), vec![(ALICE, BTC)]);
 	})
 }
+
+// *************************************************
+// tests for mutation hooks (OnDeposit, OnTransfer)
+// (tests for the OnSlash hook can be found in `./tests_multicurrency.rs`)
+// *************************************************
+
+#[test]
+fn deposit_hook_works() {
+	ExtBuilder::default().build().execute_with(|| {
+		let initial_hook_calls = OnDepositHook::<Runtime>::calls();
+		assert_ok!(Tokens::do_deposit(DOT, &CHARLIE, 0, false, true),);
+		assert_eq!(OnDepositHook::<Runtime>::calls(), initial_hook_calls);
+
+		assert_ok!(Tokens::do_deposit(DOT, &CHARLIE, 100, false, true),);
+		assert_eq!(OnDepositHook::<Runtime>::calls(), initial_hook_calls + 1);
+
+		// The hook must be called even if the actual deposit ends up failing
+		assert_noop!(
+			Tokens::do_deposit(DOT, &BOB, 1, false, true),
+			Error::<Runtime>::ExistentialDeposit
+		);
+		assert_eq!(OnDepositHook::<Runtime>::calls(), initial_hook_calls + 2);
+	});
+}
+
+#[test]
+fn transfer_hook_works() {
+	ExtBuilder::default()
+		.balances(vec![(ALICE, DOT, 100)])
+		.build()
+		.execute_with(|| {
+			let initial_hook_calls = OnTransferHook::<Runtime>::calls();
+			assert_ok!(Tokens::do_transfer(
+				DOT,
+				&ALICE,
+				&CHARLIE,
+				0,
+				ExistenceRequirement::AllowDeath
+			),);
+			assert_eq!(OnTransferHook::<Runtime>::calls(), initial_hook_calls);
+
+			assert_ok!(Tokens::do_transfer(
+				DOT,
+				&ALICE,
+				&CHARLIE,
+				10,
+				ExistenceRequirement::AllowDeath
+			));
+			assert_eq!(OnTransferHook::<Runtime>::calls(), initial_hook_calls + 1);
+
+			// The hook must be called even if the actual transfer ends up failing
+			assert_noop!(
+				Tokens::do_transfer(DOT, &ALICE, &BOB, 1, ExistenceRequirement::AllowDeath),
+				Error::<Runtime>::ExistentialDeposit
+			);
+			assert_eq!(OnTransferHook::<Runtime>::calls(), initial_hook_calls + 2);
+		});
+}

tokens/src/tests_multicurrency.rs

@@ -731,3 +731,72 @@ fn named_multi_reservable_repatriate_all_reserved_named_works() {
 			}));
 		});
 }
+
+#[test]
+fn slash_hook_works() {
+	ExtBuilder::default()
+		.balances(vec![(ALICE, DOT, 100)])
+		.build()
+		.execute_with(|| {
+			let initial_hook_calls = OnSlashHook::<Runtime>::calls();
+
+			// slashing zero tokens is a no-op
+			assert_eq!(Tokens::slash(DOT, &ALICE, 0), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_hook_calls);
+
+			assert_eq!(Tokens::slash(DOT, &ALICE, 50), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_hook_calls + 1);
+
+			// `slash` calls the hook even if no amount was slashed
+			assert_eq!(Tokens::slash(DOT, &ALICE, 100), 50);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_hook_calls + 2);
+		});
+}
+
+#[test]
+fn slash_hook_works_for_reserved() {
+	ExtBuilder::default()
+		.balances(vec![(ALICE, DOT, 100)])
+		.build()
+		.execute_with(|| {
+			let initial_slash_hook_calls = OnSlashHook::<Runtime>::calls();
+
+			assert_ok!(Tokens::reserve(DOT, &ALICE, 50));
+			// slashing zero tokens is a no-op
+			assert_eq!(Tokens::slash_reserved(DOT, &ALICE, 0), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls);
+
+			assert_eq!(Tokens::slash_reserved(DOT, &ALICE, 50), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls + 1);
+
+			// `slash_reserved` calls the hook even if no amount was slashed
+			assert_eq!(Tokens::slash_reserved(DOT, &ALICE, 50), 50);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls + 2);
+		});
+}
+
+#[test]
+fn slash_hook_works_for_reserved_named() {
+	ExtBuilder::default()
+		.balances(vec![(ALICE, DOT, 100)])
+		.build()
+		.execute_with(|| {
+			let initial_slash_hook_calls = OnSlashHook::<Runtime>::calls();
+
+			assert_ok!(Tokens::reserve_named(&RID_1, DOT, &ALICE, 10));
+			// slashing zero tokens is a no-op
+			assert_eq!(Tokens::slash_reserved_named(&RID_1, DOT, &ALICE, 0), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls);
+
+			assert_eq!(Tokens::slash_reserved_named(&RID_1, DOT, &ALICE, 10), 0);
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls + 1);
+
+			// `slash_reserved_named` calls `slash_reserved` under-the-hood with a
+			// value to slash based on the account's balance. Because the account's
+			// balance is currently zero, `slash_reserved` will be a no-op and
+			// the OnSlash hook will not be called.
+			assert_eq!(Tokens::slash_reserved_named(&RID_1, DOT, &ALICE, 50), 50);
+			// Same value as previously because of the no-op
+			assert_eq!(OnSlashHook::<Runtime>::calls(), initial_slash_hook_calls + 1);
+		});
+}