Contracts: seal0::balance should return the free balance

[xermicus]

In seal0, the balance API function is supposed to return the "free" balance. I noticed that this was indeed the case with substrate contracts node < v0.25.0. However, with current versions, the free balance + the ED is returned instead. I don't think this is the wanted behavior, and if it was I think it'd constitute a breaking change requiring a new version for balance.

[pgherveou]

good catch, looks like this is a regression from paritytech/substrate#14020

 	fn balance(&self) -> BalanceOf<T> {
-		T::Currency::free_balance(&self.top_frame().account_id)
+		T::Currency::balance(&self.top_frame().account_id)
 	}

[athei]

Those are different traits. On the Currency trait (before) we had free_balance. But on the new trait Inspect it is just balance which also doesn't include holds or freezes.

[pgherveou]

isn't it simpler not to modify the fixture but to add ED to the transferred balance?

[xermicus]

Hmm, not sure what you mean, this does add the ED to the transferred balance. How would I do that without modifying the fixture?

[xermicus]

I mean the fixture is broken, given seal_balance should only return free balance: It tries to send away all free balance but expects the transfer to fail. Which should never fail. Hence we need to modify it anyways.

[juangirini]

The definition of balance used in substrate contracts node < v0.25.0 was this

	fn balance(&self) -> BalanceOf<T> {
		T::Currency::free_balance(&self.top_frame().account_id)
	}

https://github.com/paritytech/substrate/blob/6fa7fe1326ecaab9921c2c3888530ad679cfbb87/frame/contracts/src/exec.rs#L1290-L1292

This was the free_balance definition:

	/// The 'free' balance of a given account.
	///
	/// This is the only balance that matters in terms of most operations on tokens. It alone
	/// is used to determine the balance when in the contract execution environment. When this
	/// balance falls below the value of `ExistentialDeposit`, then the 'current account' is
	/// deleted: specifically `FreeBalance`.
	///
	/// `system::AccountNonce` is also deleted if `ReservedBalance` is also zero (it also gets
	/// collapsed to zero if it ever becomes less than `ExistentialDeposit`.
	fn free_balance(who: &AccountId) -> Self::Balance;

https://github.com/paritytech/substrate/blob/6fa7fe1326ecaab9921c2c3888530ad679cfbb87/frame/support/src/traits/tokens/currency.rs#L101-L110
This implies that free_balance sums up the ed.

In fact its implementation is just to return the free balance

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

https://github.com/paritytech/substrate/blob/6fa7fe1326ecaab9921c2c3888530ad679cfbb87/frame/balances/src/lib.rs#L1479-L1481

Which does never mention to not include ed

	/// Non-reserved part of the balance. There may still be restrictions on this, but it is the
	/// total pool what may in principle be transferred, reserved and used for tipping.
	///
	/// This is the only balance that matters in terms of most operations on tokens. It
	/// alone is used to determine the balance when in the contract execution environment.
	pub free: Balance,

https://github.com/paritytech/substrate/blob/6fa7fe1326ecaab9921c2c3888530ad679cfbb87/frame/balances/src/lib.rs#L691-L712

How is possible that the ed was not summed up in the 'old' balance calculation?

[xermicus]

You are right, that comment you found is confusing.

[... ] When this
/// balance falls below the value of ExistentialDeposit

Implies that ED would be included in "free" balance. However, that doesn't make sense to me based just the notions of "free" balance and "existential deposit": How can "free" balance include something called "existential deposit", which destroys your account if sent away? Which is not exactly what I'd understand of "free" balance. I think that might just have been a documentation issue?

[juangirini]

I don't think it's a documentation issue

polkadot-sdk/substrate/frame/balances/src/impl_currency.rs

Lines 448 to 480 in 3f0d28c

	/// Force the new free balance of a target account `who` to some new value `balance`.
	fn make_free_balance_be(
	who: &T::AccountId,
	value: Self::Balance,
	) -> SignedImbalance<Self::Balance, Self::PositiveImbalance> {
	Self::try_mutate_account_handling_dust(
	who,
	|account,
	is_new|
	-> Result<SignedImbalance<Self::Balance, Self::PositiveImbalance>, DispatchError> {
	let ed = T::ExistentialDeposit::get();
	// If we're attempting to set an existing account to less than ED, then
	// bypass the entire operation. It's a no-op if you follow it through, but
	// since this is an instance where we might account for a negative imbalance
	// (in the dust cleaner of set_account) before we account for its actual
	// equal and opposite cause (returned as an Imbalance), then in the
	// instance that there's no other accounts on the system at all, we might
	// underflow the issuance and our arithmetic will be off.
	ensure!(value >= ed || !is_new, Error::<T, I>::ExistentialDeposit);
	let imbalance = if account.free <= value {
	SignedImbalance::Positive(PositiveImbalance::new(value - account.free))
	} else {
	SignedImbalance::Negative(NegativeImbalance::new(account.free - value))
	};
	account.free = value;
	Self::deposit_event(Event::BalanceSet { who: who.clone(), free: account.free });
	Ok(imbalance)
	},
	)
	.unwrap_or_else(|_| SignedImbalance::Positive(Self::PositiveImbalance::zero()))
	}
	}

[athei]

The regression was not in paritytech/substrate#14020 as @juangirini pointed out. It happened when we had to adapt to the fact that reserved balance can no longer contribute to the ed.

We always included the ed in the free balance. However, before the ed was part of reserved balance and you could send away everything returned by balance. And we should go back to this behaviour. Good catch.

[xermicus]

bot fmt

[command-bot]

@xermicus https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3526923 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 10-4bf6d9ac-8685-43b7-b819-986e4350e5f2 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@xermicus Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3526923 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3526923/artifacts/download.

[xermicus]

Ah now I see. Thanks @athei

[xermicus]

bot merge

[command-bot]

@xermicus Unknown command "merge". Refer to help docs and/or source code.