Unbalanced and Balanced fungible conformance tests, and fungible fixes (

paritytech#1296)

Original PR paritytech/substrate#14655

---

Partial paritytech#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` is `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`:

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

https://github.com/paritytech/polkadot-sdk/blob/e5c876dd6b59e2b7dbacaa4538cb42c802db3730/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L912-L937

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

https://github.com/paritytech/polkadot-sdk/blob/e5c876dd6b59e2b7dbacaa4538cb42c802db3730/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L1323-L1346

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

https://github.com/paritytech/polkadot-sdk/blob/e5c876dd6b59e2b7dbacaa4538cb42c802db3730/substrate/frame/support/src/traits/tokens/fungible/conformance_tests/regular.rs#L1036-L1071

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

---------

Co-authored-by: Muharem <[email protected]>
(cherry picked from commit 46090ff)

prdoc/pr_1296.prdoc

@@ -0,0 +1,16 @@
+title: fungible fixes and more conformance tests
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Adds conformance tests for the Balanced and Unbalanced fungible traits
+      Fixes Unbalanced::decrease_balance not respecting preservation
+      Fixes Balanced::pair possibly returning pairs of imbalances which do not cancel each other out. Method now returns a Result instead (breaking change).
+      Fixes Balances pallet active_issuance possible '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
+
+crates:
+  - name: pallet-balances
+  - name: frame-support

substrate/frame/assets/src/tests/sets.rs

@@ -153,7 +153,7 @@ fn pair_from_set_types_works() {
 		assert_eq!(First::<Assets>::total_issuance(()), 100);
 		assert_eq!(First::<Assets>::total_issuance(()), Assets::total_issuance(asset1));
 
-		let (debt, credit) = First::<Assets>::pair((), 100);
+		let (debt, credit) = First::<Assets>::pair((), 100).unwrap();
 		assert_eq!(First::<Assets>::total_issuance(()), 100);
 		assert_eq!(debt.peek(), 100);
 		assert_eq!(credit.peek(), 100);

substrate/frame/balances/src/impl_fungible.rs

@@ -177,7 +177,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());
+		});
 	}
 
 	fn reactivate(amount: Self::Balance) {

substrate/frame/balances/src/tests/fungible_conformance_tests.rs

@@ -19,28 +19,30 @@ use super::*;
 use frame_support::traits::fungible::{conformance_tests, Inspect, Mutate};
 use paste::paste;
 
-macro_rules! run_tests {
-    ($path:path, $ext_deposit:expr, $($name:ident),*) => {
+macro_rules! generate_tests {
+	// Handle a conformance test that requires special testing with and without a dust trap.
+    (dust_trap_variation, $base_path:path, $scope:expr, $trait:ident, $ext_deposit:expr, $($test_name:ident),*) => {
 		$(
 			paste! {
 				#[test]
-				fn [< $name _existential_deposit_ $ext_deposit _dust_trap_on >]() {
+				fn [<$trait _ $scope _ $test_name _existential_deposit_ $ext_deposit _dust_trap_on >]() {
+					// Some random trap account.
 					let trap_account = <Test as frame_system::Config>::AccountId::from(65174286u64);
 					let builder = ExtBuilder::default().existential_deposit($ext_deposit).dust_trap(trap_account);
 					builder.build_and_execute_with(|| {
 						Balances::set_balance(&trap_account, Balances::minimum_balance());
-						$path::$name::<
+						$base_path::$scope::$trait::$test_name::<
 							Balances,
 							<Test as frame_system::Config>::AccountId,
 						>(Some(trap_account));
 					});
 				}
 
 				#[test]
-				fn [< $name _existential_deposit_ $ext_deposit _dust_trap_off >]() {
+				fn [< $trait _ $scope _ $test_name _existential_deposit_ $ext_deposit _dust_trap_off >]() {
 					let builder = ExtBuilder::default().existential_deposit($ext_deposit);
 					builder.build_and_execute_with(|| {
-						$path::$name::<
+						$base_path::$scope::$trait::$test_name::<
 							Balances,
 							<Test as frame_system::Config>::AccountId,
 						>(None);
@@ -49,9 +51,37 @@ macro_rules! run_tests {
 			}
 		)*
 	};
-	($path:path, $ext_deposit:expr) => {
-		run_tests!(
-			$path,
+	// Regular conformance test
+    ($base_path:path, $scope:expr, $trait:ident, $ext_deposit:expr, $($test_name:ident),*) => {
+		$(
+			paste! {
+				#[test]
+				fn [< $trait _ $scope _ $test_name _existential_deposit_ $ext_deposit>]() {
+					let builder = ExtBuilder::default().existential_deposit($ext_deposit);
+					builder.build_and_execute_with(|| {
+						$base_path::$scope::$trait::$test_name::<
+							Balances,
+							<Test as frame_system::Config>::AccountId,
+						>();
+					});
+				}
+			}
+		)*
+	};
+	($base_path:path, $ext_deposit:expr) => {
+		// regular::mutate
+		generate_tests!(
+			dust_trap_variation,
+			$base_path,
+			regular,
+			mutate,
+			$ext_deposit,
+			transfer_expendable_dust
+		);
+		generate_tests!(
+			$base_path,
+			regular,
+			mutate,
 			$ext_deposit,
 			mint_into_success,
 			mint_into_overflow,
@@ -66,7 +96,6 @@ macro_rules! run_tests {
 			shelve_insufficient_funds,
 			transfer_success,
 			transfer_expendable_all,
-			transfer_expendable_dust,
 			transfer_protect_preserve,
 			set_balance_mint_success,
 			set_balance_burn_success,
@@ -79,10 +108,34 @@ macro_rules! run_tests {
 			reducible_balance_expendable,
 			reducible_balance_protect_preserve
 		);
+		// regular::unbalanced
+		generate_tests!(
+			$base_path,
+			regular,
+			unbalanced,
+			$ext_deposit,
+			write_balance,
+			decrease_balance_expendable,
+			decrease_balance_preserve,
+			increase_balance,
+			set_total_issuance,
+			deactivate_and_reactivate
+		);
+		// regular::balanced
+		generate_tests!(
+			$base_path,
+			regular,
+			balanced,
+			$ext_deposit,
+			issue_and_resolve_credit,
+			rescind_and_settle_debt,
+			deposit,
+			withdraw,
+			pair
+		);
 	};
 }
 
-run_tests!(conformance_tests::inspect_mutate, 1);
-run_tests!(conformance_tests::inspect_mutate, 2);
-run_tests!(conformance_tests::inspect_mutate, 5);
-run_tests!(conformance_tests::inspect_mutate, 1000);
+generate_tests!(conformance_tests, 1);
+generate_tests!(conformance_tests, 5);
+generate_tests!(conformance_tests, 1000);

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

@@ -16,3 +16,4 @@
 // limitations under the License.
 
 pub mod inspect_mutate;
+pub mod regular;