Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Question: Is pallet asset trait FrozenBalance correctly implemented ? #10391

Closed
gui1117 opened this issue Nov 30, 2021 · 1 comment · Fixed by #10431
Closed

Question: Is pallet asset trait FrozenBalance correctly implemented ? #10391

gui1117 opened this issue Nov 30, 2021 · 1 comment · Fixed by #10431
Labels
Z7-question Issue is a question. Closer should answer.

Comments

@gui1117
Copy link
Contributor

gui1117 commented Nov 30, 2021

here the trait:

/// Trait for allowing a minimum balance on the account to be specified, beyond the
/// `minimum_balance` of the asset. This is additive - the `minimum_balance` of the asset must be
/// met *and then* anything here in addition.
pub trait FrozenBalance<AssetId, AccountId, Balance> {
	/// Return the frozen balance. Under normal behaviour, this amount should always be
	/// withdrawable.
	///
	/// In reality, the balance of every account must be at least the sum of this (if `Some`) and
	/// the asset's minimum_balance, since there may be complications to destroying an asset's
	/// account completely.
	///
	/// If `None` is returned, then nothing special is enforced.
	///
	/// If any operation ever breaks this requirement (which will only happen through some sort of
	/// privileged intervention), then `melted` is called to do any cleanup.
	fn frozen_balance(asset: AssetId, who: &AccountId) -> Option<Balance>;

	/// Called when an account has been removed.
	fn died(asset: AssetId, who: &AccountId);
}

So I expected that an account should always have more than min+frozen, except for priviledge operation like some root stuff.

but minting less than this amount is allowed, and transferring less than this amount is also allowed and result with an account for which the property doesn't hold.

#[test]
fn foo() {
	let asset_id = 0;
	set_frozen_balance(asset_id, 1, 100);
	set_frozen_balance(asset_id, 2, 100);
	new_test_ext().execute_with(|| {
		assert_ok!(Assets::force_create(Origin::root(), asset_id, 1, true, 1));
		assert_ok!(Assets::mint(Origin::signed(1), asset_id, 1, 100)); // Should fail because min of 1 is 101
		assert_eq!(Assets::balance(asset_id, 1), 100); // Should be 0 as its min is 101
		assert_ok!(Assets::mint(Origin::signed(1), asset_id, 3, 100));
		assert_ok!(Assets::transfer(Origin::signed(3), asset_id, 2, 100));
		assert_eq!(Assets::balance(asset_id, 2), 100); // Should be 0 because min of 2 is 101
	});
}

Either we should add some more check in the code.
Or we should update the doc saying that an account can have less than asset_min_balance + frozen_amount_for_the_account but just cannot go below it in case it is was above before.
Or do I miss something ?

Also other note: there is a mention of melted but I can't find it in the code.

@gui1117 gui1117 changed the title Question: Is pallet asset FrozenBalance correctly implemented ? Question: Is pallet asset trait FrozenBalance correctly implemented ? Nov 30, 2021
@gavofyork
Copy link
Member

That's right - it should really say "the balance may not be reduced to become less than minimum_balance + frozen_balance".

@kianenigma kianenigma added the Z7-question Issue is a question. Closer should answer. label Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z7-question Issue is a question. Closer should answer.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants