From e6127ffa45fffe9c0ee8a0547c55368980ef09a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Tue, 19 Apr 2022 23:08:25 +0200 Subject: [PATCH 1/5] pallet-asset: Fix transfer of a large amount of an asset Before this pr transferring a large amount of an asset would check that transferring the asset would not overflow the supply of the asset. However, it doesn't make sense to check for asset supply overflow when we just transfer from one account to another account and don't increase the supply in any way. It also required to extend the `can_deposit` method of `fungible` and `fungibles` with a `mint` parameter. If this parameter is set to `true`, it means we want to mint the amount of an asset before transferring it into an account. For `can_withdraw` we don't need to add an extra parameter, because withdrawing should never be able to underflow the supply. If that would happen, it would mean that somewhere the supply wasn't increased while increasing the balance of an account. --- frame/assets/src/functions.rs | 18 +++++++++++++----- frame/assets/src/impl_fungibles.rs | 3 ++- frame/assets/src/tests.rs | 10 ++++++++++ frame/balances/src/lib.rs | 9 +++++---- frame/support/src/traits/tokens/fungible.rs | 14 ++++++++++---- frame/support/src/traits/tokens/fungibles.rs | 10 +++++++++- 6 files changed, 49 insertions(+), 15 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 0be79619e0967..2c5e5e4bc44dc 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -103,16 +103,24 @@ impl, I: 'static> Pallet { 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 + /// 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::::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) { @@ -283,7 +291,7 @@ impl, I: 'static> Pallet { (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)) } @@ -379,7 +387,7 @@ impl, I: 'static> Pallet { return Ok(()) } - Self::can_increase(id, beneficiary, amount).into_result()?; + Self::can_increase(id, beneficiary, amount, true).into_result()?; Asset::::try_mutate(id, |maybe_details| -> DispatchResult { let details = maybe_details.as_mut().ok_or(Error::::Unknown)?; @@ -535,8 +543,8 @@ impl, I: 'static> Pallet { } // Figure out the debit and credit, together with side-effects. - let debit = Self::prep_debit(id, &source, amount, f.into())?; - let (credit, maybe_burn) = Self::prep_credit(id, &dest, amount, debit, f.burn_dust)?; + let debit = dbg!(Self::prep_debit(id, &source, amount, f.into()))?; + let (credit, maybe_burn) = dbg!(Self::prep_credit(id, &dest, amount, debit, f.burn_dust))?; let mut source_account = Account::::get(id, &source).ok_or(Error::::NoAccount)?; diff --git a/frame/assets/src/impl_fungibles.rs b/frame/assets/src/impl_fungibles.rs index 49caac83f4c4a..6b263bc0c7bef 100644 --- a/frame/assets/src/impl_fungibles.rs +++ b/frame/assets/src/impl_fungibles.rs @@ -47,8 +47,9 @@ impl, I: 'static> fungibles::Inspect<::AccountId asset: Self::AssetId, who: &::AccountId, amount: Self::Balance, + mint: bool, ) -> DepositConsequence { - Pallet::::can_increase(asset, who, amount) + Pallet::::can_increase(asset, who, amount, mint) } fn can_withdraw( diff --git a/frame/assets/src/tests.rs b/frame/assets/src/tests.rs index db0d6a5f212f9..50ab04111edff 100644 --- a/frame/assets/src/tests.rs +++ b/frame/assets/src/tests.rs @@ -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)); + }) +} diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 6bf37dfda037b..0eab933f7757d 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -810,12 +810,13 @@ impl, I: 'static> Pallet { _who: &T::AccountId, amount: T::Balance, account: &AccountData, + mint: bool, ) -> DepositConsequence { if amount.is_zero() { return DepositConsequence::Success } - if TotalIssuance::::get().checked_add(&amount).is_none() { + if mint && TotalIssuance::::get().checked_add(&amount).is_none() { return DepositConsequence::Overflow } @@ -1093,8 +1094,8 @@ impl, I: 'static> fungible::Inspect for Pallet 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, @@ -1110,7 +1111,7 @@ impl, I: 'static> fungible::Mutate for Pallet { 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(()) })?; diff --git a/frame/support/src/traits/tokens/fungible.rs b/frame/support/src/traits/tokens/fungible.rs index 712103a1e8837..7422a9d651874 100644 --- a/frame/support/src/traits/tokens/fungible.rs +++ b/frame/support/src/traits/tokens/fungible.rs @@ -50,7 +50,11 @@ pub trait Inspect { 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. @@ -86,7 +90,9 @@ pub trait Mutate: Inspect { amount: Self::Balance, ) -> Result { 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), @@ -216,8 +222,8 @@ impl< fn reducible_balance(who: &AccountId, keep_alive: bool) -> Self::Balance { >::reducible_balance(A::get(), who, keep_alive) } - fn can_deposit(who: &AccountId, amount: Self::Balance) -> DepositConsequence { - >::can_deposit(A::get(), who, amount) + fn can_deposit(who: &AccountId, amount: Self::Balance, mint: bool) -> DepositConsequence { + >::can_deposit(A::get(), who, amount, mint) } fn can_withdraw(who: &AccountId, amount: Self::Balance) -> WithdrawConsequence { >::can_withdraw(A::get(), who, amount) diff --git a/frame/support/src/traits/tokens/fungibles.rs b/frame/support/src/traits/tokens/fungibles.rs index 8e68b36d60c7a..2abadf037687d 100644 --- a/frame/support/src/traits/tokens/fungibles.rs +++ b/frame/support/src/traits/tokens/fungibles.rs @@ -53,10 +53,16 @@ pub trait Inspect { 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 @@ -137,7 +143,9 @@ pub trait Mutate: Inspect { amount: Self::Balance, ) -> Result { 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), From fa59fb71845033734706e677380e652ef6363343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 20 Apr 2022 00:17:11 +0200 Subject: [PATCH 2/5] Update frame/assets/src/functions.rs --- frame/assets/src/functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 2c5e5e4bc44dc..7ed8fd485921d 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -543,7 +543,7 @@ impl, I: 'static> Pallet { } // Figure out the debit and credit, together with side-effects. - let debit = dbg!(Self::prep_debit(id, &source, amount, f.into()))?; + let debit = Self::prep_debit(id, &source, amount, f.into())?; let (credit, maybe_burn) = dbg!(Self::prep_credit(id, &dest, amount, debit, f.burn_dust))?; let mut source_account = From efa358276f351b53b7bf7fb820be08b1bf04ab2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 20 Apr 2022 06:25:55 +0200 Subject: [PATCH 3/5] Update frame/assets/src/functions.rs --- frame/assets/src/functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 7ed8fd485921d..ba9fe1a0925a3 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -544,7 +544,7 @@ impl, I: 'static> Pallet { // Figure out the debit and credit, together with side-effects. let debit = Self::prep_debit(id, &source, amount, f.into())?; - let (credit, maybe_burn) = dbg!(Self::prep_credit(id, &dest, amount, debit, f.burn_dust))?; + let (credit, maybe_burn) = Self::prep_credit(id, &dest, amount, debit, f.burn_dust)?; let mut source_account = Account::::get(id, &source).ok_or(Error::::NoAccount)?; From 07cd75c607d3f3b8b109e1bd78f8a74db81edd4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Wed, 20 Apr 2022 17:33:39 +0200 Subject: [PATCH 4/5] Update frame/assets/src/functions.rs Co-authored-by: Shawn Tabrizi --- frame/assets/src/functions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index ba9fe1a0925a3..263c1550164df 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -108,7 +108,7 @@ impl, I: 'static> Pallet { /// - `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 + /// - `increase_supply`: Will the supply of the asset be increased by `amount` at the same /// time as crediting the `account`. pub(super) fn can_increase( id: T::AssetId, From 9d382519566f2065c9ba173bdb9a82b65fb1b8e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 25 Apr 2022 12:32:32 +0200 Subject: [PATCH 5/5] FMT --- frame/assets/src/functions.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/assets/src/functions.rs b/frame/assets/src/functions.rs index 263c1550164df..a6abfd9e0409c 100644 --- a/frame/assets/src/functions.rs +++ b/frame/assets/src/functions.rs @@ -108,8 +108,8 @@ impl, I: 'static> Pallet { /// - `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. - /// - `increase_supply`: Will the supply of the asset be increased by `amount` at the same - /// time as crediting the `account`. + /// - `increase_supply`: Will the supply of the asset be increased by `amount` at the same time + /// as crediting the `account`. pub(super) fn can_increase( id: T::AssetId, who: &T::AccountId,