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

Fix fungible unbalanced trait #12569

Merged
merged 9 commits into from
Nov 2, 2022
Merged

Fix fungible unbalanced trait #12569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e751aff
Fix fungible unbalanced trait
zjb0807 Oct 27, 2022
7691522
Add simple decrease_balance test
ggwpez Oct 29, 2022
0accb2f
Fix decrease_balance_at_most
zjb0807 Oct 30, 2022
f54544f
Merge branch 'fix-unbalanced-trait' of github.com:zjb0807/substrate i…
zjb0807 Oct 30, 2022
45b48de
Fix decrease_balance_at_most in fungibles
zjb0807 Oct 30, 2022
e959b42
Rename free_balanceto balance_on_free
zjb0807 Oct 30, 2022
ceeb256
Merge remote-tracking branch 'origin/master' into fix-unbalanced-trait
zjb0807 Oct 30, 2022
3550a84
Use reducible_balance instead of balance_on_free
zjb0807 Oct 30, 2022
94d9e39
Merge remote-tracking branch 'origin/master' into fix-unbalanced-trait
zjb0807 Oct 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions frame/balances/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1149,15 +1149,19 @@ impl<T: Config<I>, I: 'static> fungible::Transfer<T::AccountId> for Pallet<T, I>

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::mutate_account(who, |account| -> DispatchResult {
// fungibles::Unbalanced::decrease_balance didn't check account.reserved
// free = new_balance - reserved
account.free =
amount.checked_sub(&account.reserved).ok_or(ArithmeticError::Underflow)?;
Comment on lines -1152 to +1156
Copy link

@PoisonPhang PoisonPhang Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@PoisonPhang PoisonPhang Oct 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Self::deposit_event(Event::BalanceSet {
who: who.clone(),
free: account.free,
reserved: account.reserved,
});
})?;
Ok(())

Ok(())
})?
}

fn set_total_issuance(amount: Self::Balance) {
Expand Down
160 changes: 159 additions & 1 deletion frame/balances/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ macro_rules! decl_tests {
($test:ty, $ext_builder:ty, $existential_deposit:expr) => {

use crate::*;
use sp_runtime::{ArithmeticError, FixedPointNumber, traits::{SignedExtension, BadOrigin}};
use sp_runtime::{ArithmeticError, TokenError, FixedPointNumber, traits::{SignedExtension, BadOrigin}};
use frame_support::{
assert_noop, assert_storage_noop, assert_ok, assert_err,
traits::{
Expand Down Expand Up @@ -1298,5 +1298,163 @@ macro_rules! decl_tests {
assert_eq!(Balances::reserved_balance(&1337), 42);
});
}

#[test]
fn fungible_unbalanced_trait_set_balance_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 0);
assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 100);

assert_ok!(Balances::reserve(&1337, 60));
assert_eq!(Balances::free_balance(1337) , 40);
assert_eq!(Balances::reserved_balance(1337), 60);

assert_noop!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 0), ArithmeticError::Underflow);

assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 60));
assert_eq!(Balances::free_balance(1337) , 0);
assert_eq!(Balances::reserved_balance(1337), 60);
});
}

#[test]
fn fungible_unbalanced_trait_set_total_issuance_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_eq!(<Balances as fungible::Inspect<_>>::total_issuance(), 0);
<Balances as fungible::Unbalanced<_>>::set_total_issuance(100);
assert_eq!(<Balances as fungible::Inspect<_>>::total_issuance(), 100);
});
}

#[test]
fn fungible_unbalanced_trait_decrease_balance_simple_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 end up at 80.
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 80);
});
}

#[test]
fn fungible_unbalanced_trait_decrease_balance_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 100);

assert_noop!(
<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 101),
TokenError::NoFunds
);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 100),
Ok(100)
);
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 0);

// free: 40, reserved: 60
assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_ok!(Balances::reserve(&1337, 60));
assert_eq!(Balances::free_balance(1337) , 40);
assert_eq!(Balances::reserved_balance(1337), 60);
assert_noop!(
<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 41),
TokenError::NoFunds
);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance(&1337, 40),
Ok(40)
);
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 60);
assert_eq!(Balances::free_balance(1337), 0);
assert_eq!(Balances::reserved_balance(1337), 60);
});
}

#[test]
fn fungible_unbalanced_trait_decrease_balance_at_most_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 100);

assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance_at_most(&1337, 101),
100
);
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 0);

assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance_at_most(&1337, 100),
100
);
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 0);

// free: 40, reserved: 60
assert_ok!(<Balances as fungible::Unbalanced<_>>::set_balance(&1337, 100));
assert_ok!(Balances::reserve(&1337, 60));
assert_eq!(Balances::free_balance(1337) , 40);
assert_eq!(Balances::reserved_balance(1337), 60);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance_at_most(&1337, 0),
0
);
assert_eq!(Balances::free_balance(1337) , 40);
assert_eq!(Balances::reserved_balance(1337), 60);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance_at_most(&1337, 10),
10
);
assert_eq!(Balances::free_balance(1337), 30);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::decrease_balance_at_most(&1337, 200),
30
);
assert_eq!(<Balances as fungible::Inspect<_>>::balance(&1337), 60);
assert_eq!(Balances::free_balance(1337), 0);
assert_eq!(Balances::reserved_balance(1337), 60);
});
}

#[test]
fn fungible_unbalanced_trait_increase_balance_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_noop!(
<Balances as fungible::Unbalanced<_>>::increase_balance(&1337, 0),
TokenError::BelowMinimum
);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::increase_balance(&1337, 1),
Ok(1)
);
assert_noop!(
<Balances as fungible::Unbalanced<_>>::increase_balance(&1337, u64::MAX),
ArithmeticError::Overflow
);
});
}

#[test]
fn fungible_unbalanced_trait_increase_balance_at_most_works() {
<$ext_builder>::default().build().execute_with(|| {
assert_eq!(
<Balances as fungible::Unbalanced<_>>::increase_balance_at_most(&1337, 0),
0
);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::increase_balance_at_most(&1337, 1),
1
);
assert_eq!(
<Balances as fungible::Unbalanced<_>>::increase_balance_at_most(&1337, u64::MAX),
u64::MAX - 1
);
});
}
}
}
7 changes: 4 additions & 3 deletions frame/support/src/traits/tokens/fungible/balanced.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
amount: Self::Balance,
) -> Result<Self::Balance, DispatchError> {
let old_balance = Self::balance(who);
let (mut new_balance, mut amount) = if old_balance < amount {
let (mut new_balance, mut amount) = if Self::reducible_balance(who, false) < amount {
return Err(TokenError::NoFunds.into())
} else {
(old_balance - amount, amount)
Expand All @@ -186,8 +186,9 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
/// Return the imbalance by which the account was reduced.
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)
let old_free_balance = Self::reducible_balance(who, false);
let (mut new_balance, mut amount) = if old_free_balance < amount {
(old_balance.saturating_sub(old_free_balance), old_free_balance)
} else {
(old_balance - amount, amount)
};
Expand Down
7 changes: 4 additions & 3 deletions frame/support/src/traits/tokens/fungibles/balanced.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
amount: Self::Balance,
) -> Result<Self::Balance, DispatchError> {
let old_balance = Self::balance(asset, who);
let (mut new_balance, mut amount) = if old_balance < amount {
let (mut new_balance, mut amount) = if Self::reducible_balance(asset, who, false) < amount {
return Err(TokenError::NoFunds.into())
} else {
(old_balance - amount, amount)
Expand All @@ -211,8 +211,9 @@ pub trait Unbalanced<AccountId>: Inspect<AccountId> {
amount: Self::Balance,
) -> Self::Balance {
let old_balance = Self::balance(asset, who);
let (mut new_balance, mut amount) = if old_balance < amount {
(Zero::zero(), old_balance)
let old_free_balance = Self::reducible_balance(asset, who, false);
let (mut new_balance, mut amount) = if old_free_balance < amount {
(old_balance.saturating_sub(old_free_balance), old_free_balance)
} else {
(old_balance - amount, amount)
};
Expand Down