fungible conformance tests: Unbalanced and Balanced

Partial https://github.com/paritytech/polkadot-sdk/issues/225

[x] Adds conformance tests for Unbalanced
[x] Adds conformance tests for Balanced
Several minor fixes to fungible default implementations and the Balances pallet
- [x] Unbalanced::decrease_balance can reap account when Preservation == Preserve
- [x] Balanced::pair can return pairs of imbalances which do not cancel each other out
- [x] Balances pallet active_issuance 'underflow'
[x] 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.
[x] Improve doc comments
[x] 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:

https://github.com/paritytech/substrate/blob/ae4a2c4906418478ab8965874f3ea73a99daf26b/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L935-L961

`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:

https://github.com/paritytech/substrate/blob/46db2ed91f5663beaccff7c79d419287b7180093/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L1348-L1370

`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:

https://github.com/paritytech/substrate/blob/177d55ed2231c16205449ac5cbf21ef796838843/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L1061-L1095

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());
+               });
        }

paritytech / substrate