diff --git a/substrate/frame/asset-conversion/ops/src/tests.rs b/substrate/frame/asset-conversion/ops/src/tests.rs index 84bbe63367473..79d1049b92e13 100644 --- a/substrate/frame/asset-conversion/ops/src/tests.rs +++ b/substrate/frame/asset-conversion/ops/src/tests.rs @@ -63,13 +63,15 @@ fn migrate_pool_account_id_with_native() { )); // assert user's balance. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000 + ed); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000 + ed); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // record total issuances before migration. - let total_issuance_token1 = NativeAndAssets::total_issuance(token_1.clone()); - let total_issuance_token2 = NativeAndAssets::total_issuance(token_2.clone()); + let total_issuance_token1 = + >::total_issuance(token_1.clone()); + let total_issuance_token2 = + >::total_issuance(token_2.clone()); let total_issuance_lp_token = PoolAssets::total_issuance(lp_token); let pool_account = PoolLocator::address(&pool_id).unwrap(); @@ -78,8 +80,14 @@ fn migrate_pool_account_id_with_native() { assert_eq!(pool_account, prior_pool_account); // assert pool's balances before migration. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 100); // migrate. @@ -90,23 +98,41 @@ fn migrate_pool_account_id_with_native() { )); // assert user's balance has not changed. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000 + ed); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000 + ed); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // assert pool's balance on new account id is same as on prior account id. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &new_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &new_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &new_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &new_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &new_pool_account), 100); // assert pool's balance on prior account id is zero. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 0); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 0); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 0 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 0 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 0); // assert total issuance has not changed. - assert_eq!(total_issuance_token1, NativeAndAssets::total_issuance(token_1)); - assert_eq!(total_issuance_token2, NativeAndAssets::total_issuance(token_2)); + assert_eq!( + total_issuance_token1, + >::total_issuance(token_1) + ); + assert_eq!( + total_issuance_token2, + >::total_issuance(token_2) + ); assert_eq!(total_issuance_lp_token, PoolAssets::total_issuance(lp_token)); }); } @@ -147,13 +173,15 @@ fn migrate_pool_account_id_with_insufficient_assets() { )); // assert user's balance. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // record total issuances before migration. - let total_issuance_token1 = NativeAndAssets::total_issuance(token_1.clone()); - let total_issuance_token2 = NativeAndAssets::total_issuance(token_2.clone()); + let total_issuance_token1 = + >::total_issuance(token_1.clone()); + let total_issuance_token2 = + >::total_issuance(token_2.clone()); let total_issuance_lp_token = PoolAssets::total_issuance(lp_token); let pool_account = PoolLocator::address(&pool_id).unwrap(); @@ -162,8 +190,14 @@ fn migrate_pool_account_id_with_insufficient_assets() { assert_eq!(pool_account, prior_pool_account); // assert pool's balances before migration. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 100); // migrate. @@ -174,23 +208,41 @@ fn migrate_pool_account_id_with_insufficient_assets() { )); // assert user's balance has not changed. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // assert pool's balance on new account id is same as on prior account id. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &new_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &new_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &new_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &new_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &new_pool_account), 100); // assert pool's balance on prior account id is zero. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 0); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 0); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 0 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 0 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 0); // assert total issuance has not changed. - assert_eq!(total_issuance_token1, NativeAndAssets::total_issuance(token_1)); - assert_eq!(total_issuance_token2, NativeAndAssets::total_issuance(token_2)); + assert_eq!( + total_issuance_token1, + >::total_issuance(token_1) + ); + assert_eq!( + total_issuance_token2, + >::total_issuance(token_2) + ); assert_eq!(total_issuance_lp_token, PoolAssets::total_issuance(lp_token)); }); } @@ -231,13 +283,15 @@ fn migrate_pool_account_id_with_sufficient_assets() { )); // assert user's balance. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // record total issuances before migration. - let total_issuance_token1 = NativeAndAssets::total_issuance(token_1.clone()); - let total_issuance_token2 = NativeAndAssets::total_issuance(token_2.clone()); + let total_issuance_token1 = + >::total_issuance(token_1.clone()); + let total_issuance_token2 = + >::total_issuance(token_2.clone()); let total_issuance_lp_token = PoolAssets::total_issuance(lp_token); let pool_account = PoolLocator::address(&pool_id).unwrap(); @@ -246,8 +300,14 @@ fn migrate_pool_account_id_with_sufficient_assets() { assert_eq!(pool_account, prior_pool_account); // assert pool's balances before migration. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 100); // migrate. @@ -258,23 +318,41 @@ fn migrate_pool_account_id_with_sufficient_assets() { )); // assert user's balance has not changed. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &user), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &user), 1000 - 10); + assert_eq!(>::balance(token_1.clone(), &user), 10000); + assert_eq!(>::balance(token_2.clone(), &user), 1000 - 10); assert_eq!(PoolAssets::balance(lp_token, &user), 216); // assert pool's balance on new account id is same as on prior account id. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &new_pool_account), 10000); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &new_pool_account), 10); + assert_eq!( + >::balance(token_1.clone(), &new_pool_account), + 10000 + ); + assert_eq!( + >::balance(token_2.clone(), &new_pool_account), + 10 + ); assert_eq!(PoolAssets::balance(lp_token, &new_pool_account), 100); // assert pool's balance on prior account id is zero. - assert_eq!(NativeAndAssets::balance(token_1.clone(), &prior_pool_account), 0); - assert_eq!(NativeAndAssets::balance(token_2.clone(), &prior_pool_account), 0); + assert_eq!( + >::balance(token_1.clone(), &prior_pool_account), + 0 + ); + assert_eq!( + >::balance(token_2.clone(), &prior_pool_account), + 0 + ); assert_eq!(PoolAssets::balance(lp_token, &prior_pool_account), 0); // assert total issuance has not changed. - assert_eq!(total_issuance_token1, NativeAndAssets::total_issuance(token_1)); - assert_eq!(total_issuance_token2, NativeAndAssets::total_issuance(token_2)); + assert_eq!( + total_issuance_token1, + >::total_issuance(token_1) + ); + assert_eq!( + total_issuance_token2, + >::total_issuance(token_2) + ); assert_eq!(total_issuance_lp_token, PoolAssets::total_issuance(lp_token)); }); } diff --git a/substrate/frame/asset-conversion/src/tests.rs b/substrate/frame/asset-conversion/src/tests.rs index e69d14fcb3c40..e523ec038a693 100644 --- a/substrate/frame/asset-conversion/src/tests.rs +++ b/substrate/frame/asset-conversion/src/tests.rs @@ -81,7 +81,7 @@ fn create_tokens_with_ed(owner: u128, tokens: Vec>, ed: u128 } fn balance(owner: u128, token_id: NativeOrWithId) -> u128 { - <::Assets>::balance(token_id, &owner) + <::Assets as Inspect>::balance(token_id, &owner) } fn pool_balance(owner: u128, token_id: u32) -> u128 { @@ -89,7 +89,7 @@ fn pool_balance(owner: u128, token_id: u32) -> u128 { } fn get_native_ed() -> u128 { - <::Assets>::minimum_balance(NativeOrWithId::Native) + <::Assets as Inspect>::minimum_balance(NativeOrWithId::Native) } macro_rules! bvec { diff --git a/substrate/frame/support/src/traits/tokens/fungible/union_of.rs b/substrate/frame/support/src/traits/tokens/fungible/union_of.rs index f8d476d517fe4..fd0f2e4d28d79 100644 --- a/substrate/frame/support/src/traits/tokens/fungible/union_of.rs +++ b/substrate/frame/support/src/traits/tokens/fungible/union_of.rs @@ -195,6 +195,45 @@ impl< } } +impl, Right, Criterion, AssetKind, AccountId> + fungible::Inspect for UnionOf +{ + type Balance = Left::Balance; + + fn total_issuance() -> Self::Balance { + >::total_issuance() + } + fn active_issuance() -> Self::Balance { + >::active_issuance() + } + fn minimum_balance() -> Self::Balance { + >::minimum_balance() + } + fn balance(who: &AccountId) -> Self::Balance { + >::balance(who) + } + fn total_balance(who: &AccountId) -> Self::Balance { + >::total_balance(who) + } + fn reducible_balance( + who: &AccountId, + preservation: Preservation, + force: Fortitude, + ) -> Self::Balance { + >::reducible_balance(who, preservation, force) + } + fn can_deposit( + who: &AccountId, + amount: Self::Balance, + provenance: Provenance, + ) -> DepositConsequence { + >::can_deposit(who, amount, provenance) + } + fn can_withdraw(who: &AccountId, amount: Self::Balance) -> WithdrawConsequence { + >::can_withdraw(who, amount) + } +} + impl< Left: fungible::InspectHold, Right: fungibles::InspectHold, @@ -259,6 +298,28 @@ impl< } } +impl, Right, Criterion, AssetKind, AccountId> + fungible::InspectHold for UnionOf +{ + type Reason = Left::Reason; + + fn reducible_total_balance_on_hold(who: &AccountId, force: Fortitude) -> Self::Balance { + >::reducible_total_balance_on_hold(who, force) + } + fn hold_available(reason: &Self::Reason, who: &AccountId) -> bool { + >::hold_available(reason, who) + } + fn total_balance_on_hold(who: &AccountId) -> Self::Balance { + >::total_balance_on_hold(who) + } + fn balance_on_hold(reason: &Self::Reason, who: &AccountId) -> Self::Balance { + >::balance_on_hold(reason, who) + } + fn can_hold(reason: &Self::Reason, who: &AccountId, amount: Self::Balance) -> bool { + >::can_hold(reason, who, amount) + } +} + impl< Left: fungible::InspectFreeze, Right: fungibles::InspectFreeze, @@ -365,6 +426,48 @@ impl< } } +impl, Right, Criterion, AssetKind, AccountId> + fungible::Unbalanced for UnionOf +{ + fn handle_dust(dust: fungible::Dust) + where + Self: Sized, + { + >::handle_dust(fungible::Dust(dust.0)) + } + fn write_balance( + who: &AccountId, + amount: Self::Balance, + ) -> Result, DispatchError> { + >::write_balance(who, amount) + } + fn set_total_issuance(amount: Self::Balance) -> () { + >::set_total_issuance(amount) + } + fn decrease_balance( + who: &AccountId, + amount: Self::Balance, + precision: Precision, + preservation: Preservation, + force: Fortitude, + ) -> Result { + >::decrease_balance( + who, + amount, + precision, + preservation, + force, + ) + } + fn increase_balance( + who: &AccountId, + amount: Self::Balance, + precision: Precision, + ) -> Result { + >::increase_balance(who, amount, precision) + } +} + impl< Left: fungible::UnbalancedHold, Right: fungibles::UnbalancedHold, @@ -422,6 +525,38 @@ impl< } } +impl, Right, Criterion, AssetKind, AccountId> + fungible::UnbalancedHold for UnionOf +{ + fn set_balance_on_hold( + reason: &Self::Reason, + who: &AccountId, + amount: Self::Balance, + ) -> DispatchResult { + >::set_balance_on_hold(reason, who, amount) + } + fn decrease_balance_on_hold( + reason: &Self::Reason, + who: &AccountId, + amount: Self::Balance, + precision: Precision, + ) -> Result { + >::decrease_balance_on_hold( + reason, who, amount, precision, + ) + } + fn increase_balance_on_hold( + reason: &Self::Reason, + who: &AccountId, + amount: Self::Balance, + precision: Precision, + ) -> Result { + >::increase_balance_on_hold( + reason, who, amount, precision, + ) + } +} + impl< Left: fungible::Mutate, Right: fungibles::Mutate, @@ -618,6 +753,63 @@ impl< } } +impl, Right, Criterion, AssetKind, AccountId> + fungible::MutateHold for UnionOf +{ + fn hold(reason: &Self::Reason, who: &AccountId, amount: Self::Balance) -> DispatchResult { + >::hold(reason, who, amount) + } + fn release( + reason: &Self::Reason, + who: &AccountId, + amount: Self::Balance, + precision: Precision, + ) -> Result { + >::release(reason, who, amount, precision) + } + fn burn_held( + reason: &Self::Reason, + who: &AccountId, + amount: Self::Balance, + precision: Precision, + force: Fortitude, + ) -> Result { + >::burn_held(reason, who, amount, precision, force) + } + fn transfer_on_hold( + reason: &Self::Reason, + source: &AccountId, + dest: &AccountId, + amount: Self::Balance, + precision: Precision, + mode: Restriction, + force: Fortitude, + ) -> Result { + >::transfer_on_hold( + reason, source, dest, amount, precision, mode, force, + ) + } + fn transfer_and_hold( + reason: &Self::Reason, + source: &AccountId, + dest: &AccountId, + amount: Self::Balance, + precision: Precision, + preservation: Preservation, + force: Fortitude, + ) -> Result { + >::transfer_and_hold( + reason, + source, + dest, + amount, + precision, + preservation, + force, + ) + } +} + impl< Left: fungible::MutateFreeze, Right: fungibles::MutateFreeze, diff --git a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs index b1482b77fff57..70911ab0f82f6 100644 --- a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs +++ b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/mock.rs @@ -100,7 +100,7 @@ parameter_types! { #[derive_impl(pallet_balances::config_preludes::TestDefaultConfig)] impl pallet_balances::Config for Runtime { - type ExistentialDeposit = ConstU64<10>; + type ExistentialDeposit = ExistentialDeposit; type AccountStore = System; } diff --git a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs index 05182c3c9ee65..994b3f528dad1 100644 --- a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs +++ b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/payment.rs @@ -22,13 +22,16 @@ use core::marker::PhantomData; use frame_support::{ defensive, ensure, traits::{ + fungible::{self, MutateHold}, fungibles, tokens::{Balance, Fortitude, Precision, Preservation, WithdrawConsequence}, - Defensive, OnUnbalanced, SameOrOther, + OnUnbalanced, }, unsigned::TransactionValidityError, }; -use pallet_asset_conversion::{QuotePrice, SwapCredit}; +use frame_system::Pallet as System; +use pallet_asset_conversion::{QuotePrice, Swap, SwapCredit}; +use pallet_transaction_payment::HoldReason; use sp_runtime::{ traits::{DispatchInfoOf, Get, PostDispatchInfoOf, Zero}, transaction_validity::InvalidTransaction, @@ -99,8 +102,10 @@ pub struct SwapAssetAdapter(PhantomData<(A, F, S, OU)>); impl OnChargeAssetTransaction for SwapAssetAdapter where A: Get, - F: fungibles::Balanced, AssetId = T::AssetId>, - S: SwapCredit< + F: fungibles::Balanced, AssetId = T::AssetId> + + MutateHold, Reason = T::RuntimeHoldReason>, + S: Swap, AssetKind = T::AssetId> + + SwapCredit< T::AccountId, Balance = BalanceOf, AssetKind = T::AssetId, @@ -119,56 +124,65 @@ where _dispatch_info: &DispatchInfoOf<::RuntimeCall>, asset_id: Self::AssetId, fee: Self::Balance, - _tip: Self::Balance, + tip: Self::Balance, ) -> Result { - if asset_id == A::get() { - // The `asset_id` is the target asset, we do not need to swap. - let fee_credit = F::withdraw( - asset_id.clone(), - who, - fee, - Precision::Exact, - Preservation::Preserve, - Fortitude::Polite, - ) - .map_err(|_| InvalidTransaction::Payment)?; + // We need to have the account stay alive even when all the free balance is sent away. + // Otherwise the held balance is burned before we have a chance to recover it. + >::inc_providers(who); - return Ok((fee_credit, fee)); - } + // if we are not paying in native balance we need to convert first + let (asset_fee, fee) = if asset_id != A::get() { + // target account needs to be brought to live + let native_ed = >::minimum_balance(); + let fee = if >::balance(who).is_zero() { + fee.saturating_add(native_ed) + } else { + fee + }; - // Quote the amount of the `asset_id` needed to pay the fee in the asset `A`. - let asset_fee = - S::quote_price_tokens_for_exact_tokens(asset_id.clone(), A::get(), fee, true) - .ok_or(InvalidTransaction::Payment)?; + // Quote the amount of the `asset_id` needed to pay the fee in the asset `A`. + let asset_fee = + S::quote_price_tokens_for_exact_tokens(asset_id.clone(), A::get(), fee, true) + .ok_or(InvalidTransaction::Payment)?; - // Withdraw the `asset_id` credit for the swap. - let asset_fee_credit = F::withdraw( - asset_id.clone(), + match >::swap_exact_tokens_for_tokens( + who.clone(), + vec![asset_id.clone(), A::get()], + asset_fee, + Some(fee), + who.clone(), + true, + ) { + Ok(fee_used) => ensure!(fee_used == fee, InvalidTransaction::Payment), + Err(_) => { + defensive!("Fee swap should pass for the quoted amount"); + return Err(InvalidTransaction::Payment.into()) + }, + }; + + (asset_fee, fee) + } else { + (fee, fee) + }; + + // Pallets have no way of knowing the amount of tip. Hence they have no way + // of making sure that they don't consume the tip. This is why we exclude it + // from the hold. + let tip_credit = F::withdraw( + A::get(), who, - asset_fee, + tip, Precision::Exact, - Preservation::Preserve, + Preservation::Expendable, Fortitude::Polite, ) - .map_err(|_| InvalidTransaction::Payment)?; - - let (fee_credit, change) = match S::swap_tokens_for_exact_tokens( - vec![asset_id, A::get()], - asset_fee_credit, - fee, - ) { - Ok((fee_credit, change)) => (fee_credit, change), - Err((credit_in, _)) => { - defensive!("Fee swap should pass for the quoted amount"); - let _ = F::resolve(who, credit_in).defensive_proof("Should resolve the credit"); - return Err(InvalidTransaction::Payment.into()) - }, - }; + .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))?; - // Since the exact price for `fee` has been quoted, the change should be zero. - ensure!(change.peek().is_zero(), InvalidTransaction::Payment); + // Put on hold so that pallets can withdraw from it in order to pay for deposits. + F::hold(&HoldReason::Payment.into(), who, fee.saturating_sub(tip)) + .map_err(|_| InvalidTransaction::Payment)?; - Ok((fee_credit, asset_fee)) + Ok((tip_credit, asset_fee)) } /// Dry run of swap & withdraw the predicted fee from the transaction origin. @@ -183,7 +197,8 @@ where ) -> Result<(), TransactionValidityError> { if asset_id == A::get() { // The `asset_id` is the target asset, we do not need to swap. - match F::can_withdraw(asset_id.clone(), who, fee) { + match >::can_withdraw(asset_id.clone(), who, fee) + { WithdrawConsequence::BalanceLow | WithdrawConsequence::UnknownAsset | WithdrawConsequence::Underflow | @@ -196,12 +211,24 @@ where } } + // target account needs to be brought to live + let native_ed = >::minimum_balance(); + let fee = if >::balance(who).is_zero() { + fee.saturating_add(native_ed) + } else { + fee + }; + let asset_fee = S::quote_price_tokens_for_exact_tokens(asset_id.clone(), A::get(), fee, true) .ok_or(InvalidTransaction::Payment)?; // Ensure we can withdraw enough `asset_id` for the swap. - match F::can_withdraw(asset_id.clone(), who, asset_fee) { + match >::can_withdraw( + asset_id.clone(), + who, + asset_fee, + ) { WithdrawConsequence::BalanceLow | WithdrawConsequence::UnknownAsset | WithdrawConsequence::Underflow | @@ -223,101 +250,64 @@ where corrected_fee: Self::Balance, tip: Self::Balance, asset_id: Self::AssetId, - already_withdrawn: Self::LiquidityInfo, + already_paid: Self::LiquidityInfo, ) -> Result, TransactionValidityError> { - let (fee_paid, initial_asset_consumed) = already_withdrawn; - let refund_amount = fee_paid.peek().saturating_sub(corrected_fee); - let (fee_in_asset, adjusted_paid) = if refund_amount.is_zero() || - F::total_balance(asset_id.clone(), who).is_zero() - { - // Nothing to refund or the account was removed be the dispatched function. - (initial_asset_consumed, fee_paid) - } else if asset_id == A::get() { - // The `asset_id` is the target asset, we do not need to swap. - let (refund, fee_paid) = fee_paid.split(refund_amount); - if let Err(refund) = F::resolve(who, refund) { - let fee_paid = fee_paid.merge(refund).map_err(|_| { - defensive!("`fee_paid` and `refund` are credits of the same asset."); - InvalidTransaction::Payment - })?; - (initial_asset_consumed, fee_paid) - } else { - (fee_paid.peek().saturating_sub(refund_amount), fee_paid) - } - } else { - // Check if the refund amount can be swapped back into the asset used by `who` for fee - // payment. - let refund_asset_amount = S::quote_price_exact_tokens_for_tokens( + let (tip_credit, asset_fee) = already_paid; + let corrected_fee = corrected_fee.saturating_sub(tip); + let account_dead = >::reference_count(who) == 1; + let available_fee = F::balance_on_hold(&HoldReason::Payment.into(), who); + + // If pallets take away too much it makes the transaction invalid. They need to make + // sure that this does not happen. + if available_fee < corrected_fee { + defensive!("Not enough balance on hold to pay tx fees. This is a bug."); + Err(TransactionValidityError::Invalid(InvalidTransaction::Payment))?; + } + + F::release(&HoldReason::Payment.into(), who, available_fee, Precision::Exact) + .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))?; + + let (refund_credit, payable_credit) = { + // need to withdraw everything in one go to prevent a dusting in case the account + // was only kept alive by the transaction fee + let available_credit = F::withdraw( A::get(), - asset_id.clone(), - refund_amount, - true, + who, + available_fee, + Precision::Exact, + Preservation::Expendable, + Fortitude::Polite, ) - // No refund given if it cannot be swapped back. - .unwrap_or(Zero::zero()); - - let debt = if refund_asset_amount.is_zero() { - fungibles::Debt::::zero(asset_id.clone()) + .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))?; + let refund = if account_dead { + Zero::zero() } else { - // Deposit the refund before the swap to ensure it can be processed. - match F::deposit(asset_id.clone(), &who, refund_asset_amount, Precision::BestEffort) - { - Ok(debt) => debt, - // No refund given since it cannot be deposited. - Err(_) => fungibles::Debt::::zero(asset_id.clone()), - } + available_fee.saturating_sub(corrected_fee) }; + available_credit.split(refund) + }; - if debt.peek().is_zero() { - // No refund given. - (initial_asset_consumed, fee_paid) - } else { - let (refund, adjusted_paid) = fee_paid.split(refund_amount); - match S::swap_exact_tokens_for_tokens( - vec![A::get(), asset_id], - refund, - Some(refund_asset_amount), - ) { - Ok(refund_asset) => { - match refund_asset.offset(debt) { - Ok(SameOrOther::None) => {}, - // This arm should never be reached, as the amount of `debt` is - // expected to be exactly equal to the amount of `refund_asset` credit. - _ => { - defensive!("Debt should be equal to the refund credit"); - return Err(InvalidTransaction::Payment.into()) - }, - }; - ( - initial_asset_consumed.saturating_sub(refund_asset_amount.into()), - adjusted_paid, - ) - }, - // The error should not occur since swap was quoted before. - Err((refund, _)) => { - defensive!("Refund swap should pass for the quoted amount"); - match F::settle(who, debt, Preservation::Expendable) { - Ok(dust) => ensure!(dust.peek().is_zero(), InvalidTransaction::Payment), - // The error should not occur as the `debt` was just withdrawn above. - Err(_) => { - defensive!("Should settle the debt"); - return Err(InvalidTransaction::Payment.into()) - }, - }; - let adjusted_paid = adjusted_paid.merge(refund).map_err(|_| { - // The error should never occur since `adjusted_paid` and `refund` are - // credits of the same asset. - InvalidTransaction::Payment - })?; - (initial_asset_consumed, adjusted_paid) - }, - } - } + // only the refund needs to be swapped back + let refund_credit = if asset_id != A::get() && !refund_credit.peek().is_zero() { + >::swap_exact_tokens_for_tokens( + vec![A::get(), asset_id.clone()], + refund_credit, + None, + ) + .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))? + } else { + refund_credit }; - // Handle the imbalance (fee and tip separately). - let (tip, fee) = adjusted_paid.split(tip); - OU::on_unbalanceds(Some(fee).into_iter().chain(Some(tip))); - Ok(fee_in_asset) + let asset_refund_amount = refund_credit.peek(); + if !asset_refund_amount.is_zero() { + F::resolve(who, refund_credit) + .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))?; + } + + >::dec_providers(who).expect("We increased the provider in withdraw_fee. We assume all other providers are balanced. qed"); + + OU::on_unbalanceds(Some(payable_credit).into_iter().chain(Some(tip_credit))); + Ok(asset_fee.saturating_sub(asset_refund_amount).saturating_add(tip)) } } diff --git a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs index 76d46aa164713..79670bf7800d2 100644 --- a/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs +++ b/substrate/frame/transaction-payment/asset-conversion-tx-payment/src/tests.rs @@ -513,6 +513,96 @@ fn asset_transaction_payment_with_tip_and_refund() { }); } +/// The tx fee is below the ed and the caller is only kept alive by the sufficient asset +/// Make sure that we can swap into a below ed native asset for payment +#[test] +fn fee_below_native_ed() { + let base_weight = Weight::from_parts(0, 0); + let balance_factor = 100; + ExtBuilder::default() + .balance_factor(balance_factor) + .base_weight(base_weight) + .build() + .execute_with(|| { + // create the asset + let asset_id = 1; + let min_balance = 2; + assert_ok!(Assets::force_create( + RuntimeOrigin::root(), + asset_id.into(), + 42, /* owner */ + true, /* is_sufficient */ + min_balance, + )); + + setup_lp(asset_id, balance_factor); + + // mint into the caller account + let caller = 333; + let beneficiary = ::Lookup::unlookup(caller); + let balance = 1000; + + assert_ok!(Assets::mint_into(asset_id.into(), &beneficiary, balance)); + assert_eq!(Assets::balance(asset_id, caller), balance); + + // assert that native balance is not necessary + assert_eq!(Balances::free_balance(caller), 0); + let weight = 1; + let len = 0; + + let fee_in_native = base_weight.ref_time() + weight + len as u64; + let fee_in_native_pre = fee_in_native + ExistentialDeposit::get(); + let fee_in_asset = AssetConversion::quote_price_tokens_for_exact_tokens( + NativeOrWithId::WithId(asset_id), + NativeOrWithId::Native, + fee_in_native, + true, + ) + .unwrap(); + let fee_in_asset_pre = AssetConversion::quote_price_tokens_for_exact_tokens( + NativeOrWithId::WithId(asset_id), + NativeOrWithId::Native, + fee_in_native_pre, + true, + ) + .unwrap(); + assert_eq!(fee_in_native, 1); + assert_eq!(fee_in_native_pre, 11); + assert_eq!(fee_in_asset, 11); + assert_eq!(fee_in_asset_pre, 111); + assert!(fee_in_asset_pre > fee_in_asset); + + let (pre, _) = ChargeAssetTxPayment::::from(0, Some(asset_id.into())) + .validate_and_prepare( + Some(caller).into(), + CALL, + &info_from_weight(Weight::from_parts(weight, 0)), + len, + 0, + ) + .unwrap(); + // check that fee was charged in the given asset + assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset_pre); + + assert_ok!(ChargeAssetTxPayment::::post_dispatch_details( + pre, + &info_from_weight(Weight::from_parts(weight, 0)), + &default_post_info(), + len, + &Ok(()), + )); + + // for some reason the native refund is correct (9) but when swapping + // back to the asset it is 89 instead of 90. probably a price slippage? + // this is why -1. + assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset - 1); + assert_eq!(Balances::free_balance(caller), 0); + + assert_eq!(TipUnbalancedAmount::get(), 0); + assert_eq!(FeeUnbalancedAmount::get(), fee_in_native); + }); +} + #[test] fn payment_from_account_with_only_assets() { let base_weight = 5; @@ -549,6 +639,7 @@ fn payment_from_account_with_only_assets() { let len = 10; let fee_in_native = base_weight + weight + len as u64; + let fee_in_native_pre = fee_in_native + ExistentialDeposit::get(); let fee_in_asset = AssetConversion::quote_price_tokens_for_exact_tokens( NativeOrWithId::WithId(asset_id), NativeOrWithId::Native, @@ -556,7 +647,18 @@ fn payment_from_account_with_only_assets() { true, ) .unwrap(); + let fee_in_asset_pre = AssetConversion::quote_price_tokens_for_exact_tokens( + NativeOrWithId::WithId(asset_id), + NativeOrWithId::Native, + fee_in_native_pre, + true, + ) + .unwrap(); + assert_eq!(fee_in_native, 20); + assert_eq!(fee_in_native_pre, 30); assert_eq!(fee_in_asset, 201); + assert_eq!(fee_in_asset_pre, 301); + assert!(fee_in_asset_pre > fee_in_asset); let (pre, _) = ChargeAssetTxPayment::::from(0, Some(asset_id.into())) .validate_and_prepare( @@ -568,7 +670,7 @@ fn payment_from_account_with_only_assets() { ) .unwrap(); // check that fee was charged in the given asset - assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset); + assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset_pre); assert_ok!(ChargeAssetTxPayment::::post_dispatch_details( pre, @@ -577,7 +679,7 @@ fn payment_from_account_with_only_assets() { len, &Ok(()), )); - assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset); + assert_eq!(Assets::balance(asset_id, caller), balance - fee_in_asset - 1); assert_eq!(Balances::free_balance(caller), 0); assert_eq!(TipUnbalancedAmount::get(), 0); @@ -831,22 +933,34 @@ fn transfer_add_and_remove_account() { let len = 10; let fee_in_native = base_weight + call_weight + extension_weight.ref_time() + len as u64 + tip; - let input_quote = AssetConversion::quote_price_tokens_for_exact_tokens( + let fee_in_native_pre = fee_in_native + ExistentialDeposit::get(); + let fee_in_asset = AssetConversion::quote_price_tokens_for_exact_tokens( NativeOrWithId::WithId(asset_id), NativeOrWithId::Native, fee_in_native, true, - ); - assert!(!input_quote.unwrap().is_zero()); + ) + .unwrap(); + let fee_in_asset_pre = AssetConversion::quote_price_tokens_for_exact_tokens( + NativeOrWithId::WithId(asset_id), + NativeOrWithId::Native, + fee_in_native_pre, + true, + ) + .unwrap(); + assert_eq!(fee_in_native, 140); + assert_eq!(fee_in_native_pre, 150); + assert_eq!(fee_in_asset, 1407); + assert_eq!(fee_in_asset_pre, 1507); + assert!(fee_in_asset_pre > fee_in_asset); - let fee_in_asset = input_quote.unwrap(); let mut info = info_from_weight(WEIGHT_100); info.extension_weight = extension_weight; let (pre, _) = ChargeAssetTxPayment::::from(tip, Some(asset_id.into())) .validate_and_prepare(Some(caller).into(), CALL, &info, len, 0) .unwrap(); - assert_eq!(Assets::balance(asset_id, &caller), balance - fee_in_asset); + assert_eq!(Assets::balance(asset_id, &caller), balance - fee_in_asset_pre); // remove caller account. assert_ok!(Assets::burn_from( @@ -882,10 +996,11 @@ fn transfer_add_and_remove_account() { // fee paid with no refund. assert_eq!(TipUnbalancedAmount::get(), tip); - assert_eq!(FeeUnbalancedAmount::get(), fee_in_native - tip); + assert_eq!(FeeUnbalancedAmount::get(), fee_in_native_pre - tip); // caller account removed. assert_eq!(Assets::balance(asset_id, caller), 0); + assert!(!System::account_exists(&caller)); }); }