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

Use can_hold instead of can_reserve in fn hold #12004

Closed
wants to merge 17 commits into from
Closed

Use can_hold instead of can_reserve in fn hold #12004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
408ede3
Use can_hold instead of can_reserve in fn hold
KiChjang Aug 10, 2022
462a9de
Add tests for fungible::hold
KiChjang Sep 27, 2022
8be79c2
Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…
KiChjang Sep 27, 2022
13d2261
cargo fmt
KiChjang Sep 27, 2022
7a6db84
Add keep_alive parameter to can_hold
KiChjang Oct 7, 2022
f41bd2a
Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…
KiChjang Nov 15, 2022
3730f24
Merge remote-tracking branch 'origin/master' into kckyeung/fix-fungib…
Nov 29, 2022
9420645
Revert "Add keep_alive parameter to can_hold"
ruseinov Nov 29, 2022
17e226b
Make sure `ReservableCurrency::can_reserve` takes `ExistentialDeposit…
ruseinov Nov 29, 2022
5153396
Enforce within .
ruseinov Nov 29, 2022
b41b36b
fix reserve
ruseinov Nov 29, 2022
7916a0a
fix tests
ruseinov Dec 1, 2022
b58674d
fix bench
ruseinov Dec 1, 2022
84e648c
fix assets tests
ruseinov Dec 1, 2022
8b50397
fix asset bench
ruseinov Dec 1, 2022
37ab667
add a clarification
ruseinov Dec 1, 2022
6b7da31
fix benches
ruseinov Dec 2, 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
3 changes: 2 additions & 1 deletion frame/assets/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ fn add_sufficients<T: Config<I>, I: 'static>(minter: T::AccountId, n: u32) {

fn add_approvals<T: Config<I>, I: 'static>(minter: T::AccountId, n: u32) {
let asset_id = default_asset_id::<T, I>();
T::Currency::deposit_creating(&minter, T::ApprovalDeposit::get() * n.into());
// taking into account the ED requirement enforced by `Balances::reserve` we do (n + 1).
T::Currency::deposit_creating(&minter, T::ApprovalDeposit::get() * (n + 1).into());
let minter_lookup = T::Lookup::unlookup(minter.clone());
let origin = SystemOrigin::Signed(minter);
Assets::<T, I>::mint(origin.clone().into(), asset_id, minter_lookup, (100 * (n + 1)).into())
Expand Down
23 changes: 8 additions & 15 deletions frame/assets/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ fn approval_lifecycle_works() {
// so we create it :)
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
assert_eq!(Asset::<Test>::get(0).unwrap().approvals, 1);
assert_eq!(Balances::reserved_balance(&1), 1);
Expand All @@ -189,7 +189,7 @@ fn transfer_approved_all_funds() {
// so we create it :)
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
assert_eq!(Asset::<Test>::get(0).unwrap().approvals, 1);
assert_eq!(Balances::reserved_balance(&1), 1);
Expand All @@ -211,7 +211,7 @@ fn approval_deposits_work() {
let e = BalancesError::<Test>::InsufficientBalance;
assert_noop!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50), e);

Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
assert_eq!(Balances::reserved_balance(&1), 1);

Expand All @@ -229,7 +229,7 @@ fn cannot_transfer_more_than_approved() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
let e = Error::<Test>::Unapproved;
assert_noop!(Assets::transfer_approved(RuntimeOrigin::signed(2), 0, 1, 3, 51), e);
Expand All @@ -241,7 +241,7 @@ fn cannot_transfer_more_than_exists() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 101));
let e = Error::<Test>::BalanceLow;
assert_noop!(Assets::transfer_approved(RuntimeOrigin::signed(2), 0, 1, 3, 101), e);
Expand All @@ -253,7 +253,7 @@ fn cancel_approval_works() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
assert_eq!(Asset::<Test>::get(0).unwrap().approvals, 1);
assert_noop!(
Expand Down Expand Up @@ -283,7 +283,7 @@ fn force_cancel_approval_works() {
new_test_ext().execute_with(|| {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 50));
assert_eq!(Asset::<Test>::get(0).unwrap().approvals, 1);
let e = Error::<Test>::NoPermission;
Expand Down Expand Up @@ -509,13 +509,6 @@ fn min_balance_should_work() {
assert!(Assets::maybe_balance(0, 1).is_none());
assert_eq!(Asset::<Test>::get(0).unwrap().accounts, 0);
assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]);

// Death by `transfer_approved`.
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
assert_ok!(Assets::approve_transfer(RuntimeOrigin::signed(1), 0, 2, 100));
assert_ok!(Assets::transfer_approved(RuntimeOrigin::signed(2), 0, 1, 3, 91));
assert_eq!(take_hooks(), vec![Hook::Died(0, 1)]);
});
}

Expand Down Expand Up @@ -1154,7 +1147,7 @@ fn querying_allowance_should_work() {
use frame_support::traits::tokens::fungibles::approvals::{Inspect, Mutate};
assert_ok!(Assets::force_create(RuntimeOrigin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(RuntimeOrigin::signed(1), 0, 1, 100));
Balances::make_free_balance_be(&1, 1);
Balances::make_free_balance_be(&1, 2);
assert_ok!(Assets::approve(0, &1, &2, 50));
assert_eq!(Assets::allowance(0, &1, &2), 50);
// Transfer asset 0, from owner 1 and delegate 2 to destination 3
Expand Down
6 changes: 3 additions & 3 deletions frame/balances/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,17 +201,17 @@ benchmarks_instance_pallet! {
// Give some multiple of the existential deposit
let existential_deposit = T::ExistentialDeposit::get();
let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);
let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance + existential_deposit);

// Reserve the balance
<Balances<T, I> as ReservableCurrency<_>>::reserve(&user, balance)?;
assert_eq!(Balances::<T, I>::reserved_balance(&user), balance);
assert!(Balances::<T, I>::free_balance(&user).is_zero());
assert_eq!(Balances::<T, I>::free_balance(&user), existential_deposit);

}: _(RawOrigin::Root, user_lookup, balance)
verify {
assert!(Balances::<T, I>::reserved_balance(&user).is_zero());
assert_eq!(Balances::<T, I>::free_balance(&user), balance);
assert_eq!(Balances::<T, I>::free_balance(&user), balance + existential_deposit);
}

impl_benchmark_test_suite!(
Expand Down
34 changes: 23 additions & 11 deletions frame/balances/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,12 +1193,15 @@ impl<T: Config<I>, I: 'static> fungible::MutateHold<T::AccountId> for Pallet<T,
if amount.is_zero() {
return Ok(())
}
ensure!(Self::can_reserve(who, amount), Error::<T, I>::InsufficientBalance);
Self::mutate_account(who, |a| {
a.free -= amount;
a.reserved += amount;
})?;
Ok(())
ensure!(
<Self as fungible::InspectHold<T::AccountId>>::can_hold(who, amount),
Copy link
Contributor Author

@KiChjang KiChjang Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, after writing unit tests, I found out that there is a difference between can_reserve and can_hold: the latter ensures that the reserved/held amount doesn't make the free balance go below the existential deposit. Should we still continue using can_hold here then? Or should we change can_hold to match the semantics of can_reserve?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we change can_hold what that mess up any existing code using can_hold / what are the tradeoffs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want people to be able to use fungible traits instead of Currency in the future. See #8285 and #8453. We currently use Currency traits everywhere, and we'd like to change that.

Error::<T, I>::InsufficientBalance,
);
Self::try_mutate_account(who, |a, _| -> DispatchResult {
a.free = a.free.checked_sub(&amount).ok_or(Error::<T, I>::InsufficientBalance)?;
a.reserved = a.reserved.checked_add(&amount).ok_or(ArithmeticError::Overflow)?;
Ok(())
})
}
fn release(
who: &T::AccountId,
Expand Down Expand Up @@ -1755,16 +1758,23 @@ impl<T: Config<I>, I: 'static> ReservableCurrency<T::AccountId> for Pallet<T, I>
where
T::Balance: MaybeSerializeDeserialize + Debug,
{
/// Check if `who` can reserve `value` from their free balance.
/// Check if `who` can reserve `value` from their free balance taking into account the
/// `ExistentialDeposit` making sure the account is not going to be destroyed in case of
/// overdraft.
///
/// Always `true` if value to be reserved is zero.
fn can_reserve(who: &T::AccountId, value: Self::Balance) -> bool {
if value.is_zero() {
return true
}
Self::account(who).free.checked_sub(&value).map_or(false, |new_balance| {
Self::ensure_can_withdraw(who, value, WithdrawReasons::RESERVE, new_balance).is_ok()
})

let account = Self::account(who);
let min_balance = T::ExistentialDeposit::get().max(account.frozen(Reasons::All));

Self::account(who)
.free
.checked_sub(&value)
.map_or(false, |new_balance| new_balance >= min_balance)
}

fn reserved_balance(who: &T::AccountId) -> Self::Balance {
Expand All @@ -1784,7 +1794,9 @@ where
account.free.checked_sub(&value).ok_or(Error::<T, I>::InsufficientBalance)?;
account.reserved =
account.reserved.checked_add(&value).ok_or(ArithmeticError::Overflow)?;
Self::ensure_can_withdraw(&who, value, WithdrawReasons::RESERVE, account.free)
Self::can_reserve(who, value)
.then(|| Ok::<(), sp_runtime::DispatchError>(()))
.ok_or(Error::<T, I>::InsufficientBalance)?
})?;

Self::deposit_event(Event::Reserved { who: who.clone(), amount: value });
Expand Down
Loading