Unbalanced and Balanced fungible conformance tests, and fungible fixes

[liamaharon]

Original PR paritytech/substrate#14655

---

Partial #225

• Adds conformance tests for Unbalanced
• Adds conformance tests for Balanced
• Several minor fixes to fungible default implementations and the Balances pallet
  • Unbalanced::decrease_balance can reap account when Preservation is Preserve
  • Balanced::pair can return pairs of imbalances which do not cancel each other out
  • Balances pallet active_issuance 'underflow'
  • Refactors the conformance test file structure to match the fungible file structure: tests for traits in regular.rs go into a test file named regular.rs, tests for traits in freezes.rs go into a test file named freezes.rs, etc.
• Improve doc comments
• Simplify macros

Fixes

Unbalanced::decrease_balance can reap account when called with Preservation::Preserve

There is a potential issue in the default implementation of Unbalanced::decrease_balance. The implementation can delete an account even when it is called with preservation: Preservation::Preserve. This seems to contradict the documentation of Preservation::Preserve:

	/// The account may not be killed and our provider reference must remain (in the context of
	/// tokens, this means that the account may not be dusted).
	Preserve,

I updated Unbalanced::decrease_balance to return Err(TokenError::BelowMinimum) when a withdrawal would cause the account to be reaped and preservation: Preservation::Preserve.

• TODO Confirm with @gavofyork that this is correct behavior

Test for this behavior:

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs

Lines 912 to 937 in e5c876d

	/// Tests [`Unbalanced::decrease_balance`] called with [`Preservation::Preserve`].
	pub fn decrease_balance_preserve<T, AccountId>()
	where
	T: Unbalanced<AccountId>,
	<T as Inspect<AccountId>>::Balance: AtLeast8BitUnsigned + Debug,
	AccountId: AtLeast8BitUnsigned,
	{
	// Setup account with some balance
	let account_0 = AccountId::from(0);
	let account_0_initial_balance = T::minimum_balance() + 10.into();
	T::increase_balance(&account_0, account_0_initial_balance, Precision::Exact).unwrap();
	// Decreasing the balance below the minimum when Precision::Exact should fail.
	let amount = 11.into();
	assert_eq!(
	T::decrease_balance(
	&account_0,
	amount,
	Precision::Exact,
	Preservation::Preserve,
	Fortitude::Polite,
	),
	Err(TokenError::BelowMinimum.into()),
	);
	// Balance should not have changed.
	assert_eq!(T::balance(&account_0), account_0_initial_balance);

Balanced::pair returning non-canceling pairs

Balanced::pair is supposed to create a pair of imbalances that cancel each other out. However this is not the case when the method is called with an amount greater than the total supply.

In the existing default implementation, Balanced::pair creates a pair by first rescinding the balance, creating Debt, and then issuing the balance, creating Credit.

When creating Debt, if the amount to create exceeds the total_supply, total_supply units of Debt are created instead of amount units of Debt. This can lead to non-canceling amount of Credit and Debt being created.

To address this, I create the credit and debt directly in the method instead of calling issue and rescind.

Test for this behavior:

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs

Lines 1323 to 1346 in e5c876d

	/// Tests [`Balanced::pair`].
	pub fn pair<T, AccountId>()
	where
	T: Balanced<AccountId>,
	<T as Inspect<AccountId>>::Balance: AtLeast8BitUnsigned + Debug,
	AccountId: AtLeast8BitUnsigned,
	{
	// Pair zero balance works
	let (credit, debt) = T::pair(0.into());
	assert_eq!(debt.peek(), 0.into());
	assert_eq!(credit.peek(), 0.into());
	// Pair with non-zero balance: the credit and debt cancel each other out
	let balance = 10.into();
	let (credit, debt) = T::pair(balance);
	assert_eq!(credit.peek(), balance);
	assert_eq!(debt.peek(), balance);
	// Pair with max balance: the credit and debt still cancel each other out
	let balance = T::Balance::max_value() - 1.into();
	let (debt, credit) = T::pair(balance);
	assert_eq!(debt.peek(), balance);
	assert_eq!(credit.peek(), balance);
	}

Balances pallet active_issuance 'underflow'

This PR resolves an issue in the Balances pallet that can lead to odd behavior of active_issuance.

Currently, the Balances pallet doesn't check if InactiveIssuance remains less than or equal to TotalIssuance when supply is deactivated. This allows InactiveIssuance to be greater than TotalIssuance, which can result in unexpected behavior from the perspective of the fungible API.

active_issuance is derived from TotalIssuance.saturating_sub(InactiveIssuance).

If an amount is deactivated that causes InactiveIssuance to become greater TotalIssuance, active_issuance will return 0. However once in that state, reactivating an amount will not increase active_issuance by the reactivated amount as expected.

Consider this test where the last assertion would fail due to this issue:

polkadot-sdk/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs

Lines 1036 to 1071 in e5c876d

	/// Tests [`Unbalanced::deactivate`] and [`Unbalanced::reactivate`].
	pub fn deactivate_and_reactivate<T, AccountId>()
	where
	T: Unbalanced<AccountId>,
	<T as Inspect<AccountId>>::Balance: AtLeast8BitUnsigned + Debug,
	AccountId: AtLeast8BitUnsigned,
	{
	T::set_total_issuance(10.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 10.into());
	T::deactivate(2.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 8.into());
	// Saturates at total_issuance
	T::reactivate(4.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 10.into());
	// Decrements correctly after saturating at total_issuance
	T::deactivate(1.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 9.into());
	// Saturates at zero
	T::deactivate(15.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 0.into());
	// Increments correctly after saturating at zero
	T::reactivate(1.into());
	assert_eq!(T::total_issuance(), 10.into());
	assert_eq!(T::active_issuance(), 1.into());
	}
	}

To address this, I've modified the deactivate function to ensure InactiveIssuance never surpasses TotalIssuance.

[liamaharon]

@muharem finally got around to addressing your comments. Thanks.