fungible conformance tests: Unbalanced and Balanced

[liamaharon]

Partial paritytech/polkadot-sdk#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 == 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

--- a/frame/support/src/traits/tokens/fungible/regular.rs
+++ b/frame/support/src/traits/tokens/fungible/regular.rs
@@ -182,9 +183,14 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
        ) -> Result<Self::Balance, DispatchError> {
                let old_balance = Self::balance(who);
                let free = Self::reducible_balance(who, preservation, force);
                if let BestEffort = precision {
                        amount = amount.min(free);
                }
+               // Under no circumstances should the account go below free when preservation is Preserve.
+               // TODO BEFORE MERGING: Confirm with Gav this is correct behavior
+               if amount > free && preservation == Preservation::Preserve {
+                       return Err(TokenError::BelowMinimum.into())
+               }
                let new_balance = old_balance.checked_sub(&amount).ok_or(TokenError::FundsUnavailable)?;
                if let Some(dust) = Self::write_balance(who, new_balance)? {
                        Self::handle_dust(Dust(dust));

Test for this behavior:

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

Lines 935 to 961 in ae4a2c4

	/// Tests for [`Unbalanced::decrease_balance`] used with preservation
	/// [`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 reversed the creation order of Credit and Debt. Now Credit is produced first, ensuring the total_supply of the system will be >= the amount of Debt that will need to be created.

@@ -408,18 +414,18 @@ pub trait Balanced<AccountId>: Inspect<AccountId> + Unbalanced<AccountId> {
        ///
        /// This is just the same as burning and issuing the same amount and has no effect on the
        /// total issuance.
-       fn pair(amount: Self::Balance) -> (Debt<AccountId, Self>, Credit<AccountId, Self>) {
-               (Self::rescind(amount), Self::issue(amount))
+       fn pair(amount: Self::Balance) -> (Credit<AccountId, Self>, Debt<AccountId, Self>) {
+               (Self::issue(amount), Self::rescind(amount))
        }

Note: This change alters the Balanced::pair API, making it a breaking change. However the method is not used in Substrate (or likely any/many other places), and trivial to update, so I don't think the breakage is a concern.

Test for this behavior:

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

Lines 1348 to 1370 in 46db2ed

	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 calculated with 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:

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

Lines 1061 to 1095 in 177d55e

	/// Conformance tests for [`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 tweaked the deactivate function to ensure InactiveIssuance never surpasses TotalIssuance.

--- a/frame/balances/src/impl_fungible.rs
+++ b/frame/balances/src/impl_fungible.rs
@@ -174,7 +174,10 @@ impl<T: Config<I>, I: 'static> fungible::Unbalanced<T::AccountId> for Pallet<T,
        }

        fn deactivate(amount: Self::Balance) {
-               InactiveIssuance::<T, I>::mutate(|b| b.saturating_accrue(amount));
+               InactiveIssuance::<T, I>::mutate(|b| {
+                       // InactiveIssuance cannot be greater than TotalIssuance.
+                       *b = b.saturating_add(amount).min(TotalIssuance::<T, I>::get());
+               });
        }