Fix fungible unbalanced trait

[zjb0807]

The fungibles::Inspect::balance() include free and reserved, but fungibles::Unbalanced::set_balance didn't check reserved in fungibles::Unbalanced::decrease_balance(), fungibles::Unbalanced::decrease_balance_at_most(), fungibles::Unbalanced::increase_balance_at_most(), fungibles::Unbalanced::increase_balance().

e.g. Alice's balance is 100(free: 50, reserved: 50), decrease_balance(20), the final balance is 130(free: 80, reserved: 50), but the expected balance should be 80(free: 30, reserved: 50).

A minimal reproducible example would be:

// Put in https://github.com/paritytech/substrate/blob/a47f200eebeb88a5bde6f1ed2be9728b82536dde/frame/balances/src/tests.rs#L1300
#[test]
fn fungible_unbalanced_trait_set_balance_works() {
	<$ext_builder>::default().build().execute_with(|| {
		// An Account that starts at 100
		assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
		// and reserves 50
		assert_ok!(Balances::reserve(&1337, 50));
		// and is decreased by 20
		assert_ok!(<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 20));
		// should and up at 80, but fails here with 130.
		assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 80);
	});
}

The related positions in the code are:

substrate/frame/balances/src/lib.rs

Lines 1073 to 1075 in 696bdc6

	fn balance(who: &T::AccountId) -> Self::Balance {
	Self::account(who).total()
	}

substrate/frame/balances/src/lib.rs

Lines 1150 to 1161 in 696bdc6

	impl<T: Config<I>, I: 'static> fungible::Unbalanced<T::AccountId> for Pallet<T, I> {
	fn set_balance(who: &T::AccountId, amount: Self::Balance) -> DispatchResult {
	Self::mutate_account(who, |account| {
	account.free = amount;
	Self::deposit_event(Event::BalanceSet {
	who: who.clone(),
	free: account.free,
	reserved: account.reserved,
	});
	})?;
	Ok(())
	}

substrate/frame/support/src/traits/tokens/fungible/balanced.rs

Lines 162 to 179 in 696bdc6

	fn decrease_balance(
	who: &AccountId,
	amount: Self::Balance,
	) -> Result<Self::Balance, DispatchError> {
	let old_balance = Self::balance(who);
	let (mut new_balance, mut amount) = if old_balance < amount {
	return Err(TokenError::NoFunds.into())
	} else {
	(old_balance - amount, amount)
	};
	if new_balance < Self::minimum_balance() {
	amount = amount.saturating_add(new_balance);
	new_balance = Zero::zero();
	}
	// Defensive only - this should not fail now.
	Self::set_balance(who, new_balance)?;
	Ok(amount)
	}

Relate issue: open-web3-stack/open-runtime-module-library#821

[PoisonPhang]

This assumes that Unbalanced::set_balance is always being used with an amount derived from free + reserved + amount. This is incorrect.
It would be more correct to separately get/set for both free and reserve within their respective traits. Then InspectMutate could have a function for getting free + reserve when it's needed for minimum_balance/ED requirements

[gavofyork]

The trait doesn't really concern itself with "free" and "reserved" - that's a distinction only relevant for the pallet and the *Hold traits. I would avoid polluting the base traits with the concern of hold/reserve, as not all that many uses care about them.

In short, I think this is a perfectly reasonable fix.

[PoisonPhang]

I'd argue that because of the *Hold traits, any actions that account for reserve should only belong to *Hold traits and that other fungible(s) traits should operate correctly without being concerned with reserve.
Even if this means having *Hold traits that overlap some of the functionalities from trait functions that aren't dependent on *Hold.

More plainly, *Hold functionality should be an extension of fungible(s) traits and not something at all implemented whiten the regular fungible(s) traits such as Mutate

[KiChjang]

If I'm not mistaken, the purpose of this function is to set the total balance of the supplied account, irrespective of whether the end result is made up of free balances, reserve balances or a mixture of both. The fact that an account previously had held funds should not automatically mean that they suddenly needed to call another function just to modify the total balance -- that would mean users need to keep track of the state of the account in order to call the correct function.

I get the part where it is functionally cleaner if only *Hold traits deal with reserve balances, but it is highly inconvenient and an unnecessary distinction from a usability perspective.

[ggwpez]

There indeed seems to be something wrong with the implementation of the traits.

But for the fix I have similar concerns to what @PoisonPhang said.
Another way would be to first deduce from free and then from reserved, but I am not experienced enough with the traits.

A minimal test for your issue description would be:

// Put in https://github.com/paritytech/substrate/blob/a47f200eebeb88a5bde6f1ed2be9728b82536dde/frame/balances/src/tests.rs#L1300
#[test]
fn fungible_unbalanced_trait_set_balance_works() {
	<$ext_builder>::default().build().execute_with(|| {
		// An Account that starts at 100
		assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
		// and reserves 50
		assert_ok!(Balances::reserve(&1337, 50));
		// and is decreased by 20
		assert_ok!(<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 20));
		// should and up at 80, but fails here with 130.
		assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 80);
	});
}

[KiChjang]

Would like to get the test case that @ggwpez mentioned in the PR prior to merging this.

[ggwpez]

Would like to get the test case that @ggwpez mentioned in the PR prior to merging this.

Added it for your convenience @KiChjang.

[KiChjang]

I don't see the test case included in the files?

[ggwpez]

I don't see the test case included in the files?

I thought you meant in the description 🤦‍♂️. Added it in 7691522

[zjb0807]

I found a bug, wait for me to fix it

[zjb0807]

substrate/frame/support/src/traits/tokens/fungible/balanced.rs

Lines 187 to 199 in 696bdc6

	fn decrease_balance_at_most(who: &AccountId, amount: Self::Balance) -> Self::Balance {
	let old_balance = Self::balance(who);
	let (mut new_balance, mut amount) = if old_balance < amount {
	(Zero::zero(), old_balance)
	} else {
	(old_balance - amount, amount)
	};
	let minimum_balance = Self::minimum_balance();
	if new_balance < minimum_balance {
	amount = amount.saturating_add(new_balance);
	new_balance = Zero::zero();
	}
	let mut r = Self::set_balance(who, new_balance);

If a balance is 100(free: 50, reserved: 50), decrease_balance_at_most(200), the new_balance will be 0 here, it will fail in set_balance(0)(amount.checked_sub(&account.reserved).

So I added the balance_on_free method to ensure that decrease_balance_at_most can succeed.

[KiChjang]

So I added the balance_on_free method to ensure that decrease_balance_at_most can succeed.

I'm not sure why a new method is necessary. Isn't this covered by reducible_balance?

[ggwpez]

Approving to unblock.

[ggwpez]

bot merge