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

pallet-asset: Fix transfer of a large amount of an asset #11241

Merged
merged 6 commits into from
Apr 25, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
14 changes: 11 additions & 3 deletions frame/assets/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,24 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Remove
}

/// Returns `true` when the balance of `account` can be increased by `amount`.
///
/// - `id`: The id of the asset that should be increased.
/// - `who`: The account of which the balance should be increased.
/// - `amount`: The amount by which the balance should be increased.
/// - `will_increase_supply`: Will the supply of the asset be increased by `amount` at the same
bkchr marked this conversation as resolved.
Show resolved Hide resolved
/// time as crediting the `account`.
pub(super) fn can_increase(
id: T::AssetId,
who: &T::AccountId,
amount: T::Balance,
increase_supply: bool,
) -> DepositConsequence {
let details = match Asset::<T, I>::get(id) {
Some(details) => details,
None => return DepositConsequence::UnknownAsset,
};
if details.supply.checked_add(&amount).is_none() {
if increase_supply && details.supply.checked_add(&amount).is_none() {
return DepositConsequence::Overflow
}
if let Some(balance) = Self::maybe_balance(id, who) {
Expand Down Expand Up @@ -283,7 +291,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
(true, Some(dust)) => (amount, Some(dust)),
_ => (debit, None),
};
Self::can_increase(id, &dest, credit).into_result()?;
Self::can_increase(id, &dest, credit, false).into_result()?;
Ok((credit, maybe_burn))
}

Expand Down Expand Up @@ -379,7 +387,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
return Ok(())
}

Self::can_increase(id, beneficiary, amount).into_result()?;
Self::can_increase(id, beneficiary, amount, true).into_result()?;
Asset::<T, I>::try_mutate(id, |maybe_details| -> DispatchResult {
let details = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;

Expand Down
3 changes: 2 additions & 1 deletion frame/assets/src/impl_fungibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ impl<T: Config<I>, I: 'static> fungibles::Inspect<<T as SystemConfig>::AccountId
asset: Self::AssetId,
who: &<T as SystemConfig>::AccountId,
amount: Self::Balance,
mint: bool,
) -> DepositConsequence {
Pallet::<T, I>::can_increase(asset, who, amount)
Pallet::<T, I>::can_increase(asset, who, amount, mint)
}

fn can_withdraw(
Expand Down
10 changes: 10 additions & 0 deletions frame/assets/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -967,3 +967,13 @@ fn querying_allowance_should_work() {
assert_eq!(Assets::allowance(0, &1, &2), 0);
});
}

#[test]
fn transfer_large_asset() {
new_test_ext().execute_with(|| {
let amount = u64::pow(2, 63) + 2;
assert_ok!(Assets::force_create(Origin::root(), 0, 1, true, 1));
assert_ok!(Assets::mint(Origin::signed(1), 0, 1, amount));
assert_ok!(Assets::transfer(Origin::signed(1), 0, 2, amount - 1));
})
}
9 changes: 5 additions & 4 deletions frame/balances/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -810,12 +810,13 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
_who: &T::AccountId,
amount: T::Balance,
account: &AccountData<T::Balance>,
mint: bool,
) -> DepositConsequence {
if amount.is_zero() {
return DepositConsequence::Success
}

if TotalIssuance::<T, I>::get().checked_add(&amount).is_none() {
if mint && TotalIssuance::<T, I>::get().checked_add(&amount).is_none() {
return DepositConsequence::Overflow
}

Expand Down Expand Up @@ -1093,8 +1094,8 @@ impl<T: Config<I>, I: 'static> fungible::Inspect<T::AccountId> for Pallet<T, I>
liquid.saturating_sub(must_remain_to_exist)
}
}
fn can_deposit(who: &T::AccountId, amount: Self::Balance) -> DepositConsequence {
Self::deposit_consequence(who, amount, &Self::account(who))
fn can_deposit(who: &T::AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence {
Self::deposit_consequence(who, amount, &Self::account(who), mint)
}
fn can_withdraw(
who: &T::AccountId,
Expand All @@ -1110,7 +1111,7 @@ impl<T: Config<I>, I: 'static> fungible::Mutate<T::AccountId> for Pallet<T, I> {
return Ok(())
}
Self::try_mutate_account(who, |account, _is_new| -> DispatchResult {
Self::deposit_consequence(who, amount, &account).into_result()?;
Self::deposit_consequence(who, amount, &account, true).into_result()?;
account.free += amount;
Ok(())
})?;
Expand Down
14 changes: 10 additions & 4 deletions frame/support/src/traits/tokens/fungible.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,11 @@ pub trait Inspect<AccountId> {
fn reducible_balance(who: &AccountId, keep_alive: bool) -> Self::Balance;

/// Returns `true` if the balance of `who` may be increased by `amount`.
fn can_deposit(who: &AccountId, amount: Self::Balance) -> DepositConsequence;
///
/// - `who`: The account of which the balance should be increased by `amount`.
/// - `amount`: How much should the balance be increased?
/// - `mint`: Will `amount` be minted to deposit it into `account`?
fn can_deposit(who: &AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence;

/// Returns `Failed` if the balance of `who` may not be decreased by `amount`, otherwise
/// the consequence.
Expand Down Expand Up @@ -86,7 +90,9 @@ pub trait Mutate<AccountId>: Inspect<AccountId> {
amount: Self::Balance,
) -> Result<Self::Balance, DispatchError> {
let extra = Self::can_withdraw(&source, amount).into_result()?;
Self::can_deposit(&dest, amount.saturating_add(extra)).into_result()?;
// As we first burn and then mint, we don't need to check if `mint` fits into the supply.
// If we can withdraw/burn it, we can also mint it again.
Self::can_deposit(&dest, amount.saturating_add(extra), false).into_result()?;
let actual = Self::burn_from(source, amount)?;
debug_assert!(
actual == amount.saturating_add(extra),
Expand Down Expand Up @@ -216,8 +222,8 @@ impl<
fn reducible_balance(who: &AccountId, keep_alive: bool) -> Self::Balance {
<F as fungibles::Inspect<AccountId>>::reducible_balance(A::get(), who, keep_alive)
}
fn can_deposit(who: &AccountId, amount: Self::Balance) -> DepositConsequence {
<F as fungibles::Inspect<AccountId>>::can_deposit(A::get(), who, amount)
fn can_deposit(who: &AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence {
<F as fungibles::Inspect<AccountId>>::can_deposit(A::get(), who, amount, mint)
}
fn can_withdraw(who: &AccountId, amount: Self::Balance) -> WithdrawConsequence<Self::Balance> {
<F as fungibles::Inspect<AccountId>>::can_withdraw(A::get(), who, amount)
Expand Down
10 changes: 9 additions & 1 deletion frame/support/src/traits/tokens/fungibles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,16 @@ pub trait Inspect<AccountId> {
fn reducible_balance(asset: Self::AssetId, who: &AccountId, keep_alive: bool) -> Self::Balance;

/// Returns `true` if the `asset` balance of `who` may be increased by `amount`.
///
/// - `asset`: The asset that should be deposited.
/// - `who`: The account of which the balance should be increased by `amount`.
/// - `amount`: How much should the balance be increased?
/// - `mint`: Will `amount` be minted to deposit it into `account`?
fn can_deposit(
asset: Self::AssetId,
who: &AccountId,
amount: Self::Balance,
mint: bool,
) -> DepositConsequence;

/// Returns `Failed` if the `asset` balance of `who` may not be decreased by `amount`, otherwise
Expand Down Expand Up @@ -137,7 +143,9 @@ pub trait Mutate<AccountId>: Inspect<AccountId> {
amount: Self::Balance,
) -> Result<Self::Balance, DispatchError> {
let extra = Self::can_withdraw(asset, &source, amount).into_result()?;
Self::can_deposit(asset, &dest, amount.saturating_add(extra)).into_result()?;
// As we first burn and then mint, we don't need to check if `mint` fits into the supply.
// If we can withdraw/burn it, we can also mint it again.
Self::can_deposit(asset, &dest, amount.saturating_add(extra), false).into_result()?;
let actual = Self::burn_from(asset, source, amount)?;
debug_assert!(
actual == amount.saturating_add(extra),
Expand Down