diff --git a/Cargo.lock b/Cargo.lock index f9ecad6935ee4..01fa1e51c1480 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13918,6 +13918,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log", "pallet-balances", "parity-scale-codec", "scale-info", diff --git a/prdoc/pr_9780.prdoc b/prdoc/pr_9780.prdoc new file mode 100644 index 0000000000000..e2043dabd2cac --- /dev/null +++ b/prdoc/pr_9780.prdoc @@ -0,0 +1,20 @@ +title: '[pallet_transaction_payment]: Share withdrawn tx fee credit with other pallets' +doc: +- audience: Runtime Dev + description: |- + Replaces https://github.com/paritytech/polkadot-sdk/pull/9590. + + The audit of #9590 showed that holding the txfee as held balance and especially playing around with `providers` causes a lot of troubles. + + This PR is a much lighter change. It keeps the original withdraw/deposit pattern. It simply stores the withdrawn `Credit` and allows other pallets to withdraw from it. + + It is also better in terms of performance since all tx signers share a single storage item (instead of a named hold per account). +crates: +- name: pallet-origin-restriction + bump: major +- name: frame-support + bump: major +- name: pallet-transaction-payment + bump: major +- name: pallet-asset-tx-payment + bump: major diff --git a/substrate/frame/origin-restriction/src/mock.rs b/substrate/frame/origin-restriction/src/mock.rs index 0e006c029cae5..91842e263e629 100644 --- a/substrate/frame/origin-restriction/src/mock.rs +++ b/substrate/frame/origin-restriction/src/mock.rs @@ -183,6 +183,7 @@ frame_support::parameter_types! { } pub struct OnChargeTransaction; + impl pallet_transaction_payment::OnChargeTransaction for OnChargeTransaction { type Balance = u64; type LiquidityInfo = (); @@ -224,6 +225,10 @@ impl pallet_transaction_payment::OnChargeTransaction for OnChargeTransacti } } +impl pallet_transaction_payment::TxCreditHold for OnChargeTransaction { + type Credit = (); +} + impl pallet_transaction_payment::Config for Test { type WeightInfo = (); type RuntimeEvent = RuntimeEvent; diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index 481ac09a95412..c0e9b3837644e 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -98,8 +98,8 @@ mod storage; pub use storage::MaybeConsideration; pub use storage::{ Consideration, ConstantStoragePrice, Disabled, Footprint, Incrementable, Instance, - LinearStoragePrice, PartialStorageInfoTrait, StorageInfo, StorageInfoTrait, StorageInstance, - TrackedStorageKey, WhitelistedStorageKeys, + LinearStoragePrice, NoDrop, PartialStorageInfoTrait, StorageInfo, StorageInfoTrait, + StorageInstance, SuppressedDrop, TrackedStorageKey, WhitelistedStorageKeys, }; mod dispatch; diff --git a/substrate/frame/support/src/traits/storage.rs b/substrate/frame/support/src/traits/storage.rs index 4f0e8607a0239..8deac03e7998d 100644 --- a/substrate/frame/support/src/traits/storage.rs +++ b/substrate/frame/support/src/traits/storage.rs @@ -18,8 +18,8 @@ //! Traits for encoding data related to pallet's storage items. use alloc::{collections::btree_set::BTreeSet, vec, vec::Vec}; -use codec::{Decode, Encode, FullCodec, MaxEncodedLen}; -use core::marker::PhantomData; +use codec::{Decode, DecodeWithMemTracking, Encode, FullCodec, MaxEncodedLen}; +use core::{marker::PhantomData, mem, ops::Drop}; use frame_support::CloneNoBound; use impl_trait_for_tuples::impl_for_tuples; use scale_info::TypeInfo; @@ -342,6 +342,80 @@ where impl_incrementable!(u8, u16, u32, u64, u128, i8, i16, i32, i64, i128); +/// Wrap a type so that is `Drop` impl is never called. +/// +/// Useful when storing types like `Imbalance` which would trigger their `Drop` +/// implementation whenever they are written to storage as they are dropped after +/// being serialized. +#[derive(Default, Encode, Decode, DecodeWithMemTracking, MaxEncodedLen, TypeInfo)] +pub struct NoDrop(T); + +impl Drop for NoDrop { + fn drop(&mut self) { + mem::forget(mem::take(&mut self.0)) + } +} + +/// Sealed trait that marks a type with a suppressed Drop implementation. +/// +/// Useful for constraining your storage items types by this bound to make +/// sure they won't runD rop when stored. +pub trait SuppressedDrop: sealed::Sealed { + /// The wrapped whose drop function is ignored. + type Inner; + + fn new(inner: Self::Inner) -> Self; + fn as_ref(&self) -> &Self::Inner; + fn as_mut(&mut self) -> &mut Self::Inner; + fn into_inner(self) -> Self::Inner; +} + +impl SuppressedDrop for () { + type Inner = (); + + fn new(inner: Self::Inner) -> Self { + inner + } + + fn as_ref(&self) -> &Self::Inner { + self + } + + fn as_mut(&mut self) -> &mut Self::Inner { + self + } + + fn into_inner(self) -> Self::Inner { + self + } +} + +impl SuppressedDrop for NoDrop { + type Inner = T; + + fn as_ref(&self) -> &Self::Inner { + &self.0 + } + + fn as_mut(&mut self) -> &mut Self::Inner { + &mut self.0 + } + + fn into_inner(mut self) -> Self::Inner { + mem::take(&mut self.0) + } + + fn new(inner: Self::Inner) -> Self { + Self(inner) + } +} + +mod sealed { + pub trait Sealed {} + impl Sealed for () {} + impl Sealed for super::NoDrop {} +} + #[cfg(test)] mod tests { use super::*; diff --git a/substrate/frame/support/src/traits/tokens/fungible/imbalance.rs b/substrate/frame/support/src/traits/tokens/fungible/imbalance.rs index a9367d0f164f7..b6a686c3cd2bb 100644 --- a/substrate/frame/support/src/traits/tokens/fungible/imbalance.rs +++ b/substrate/frame/support/src/traits/tokens/fungible/imbalance.rs @@ -21,10 +21,13 @@ //! See the [`crate::traits::fungible`] doc for more information about fungible traits. use super::{super::Imbalance as ImbalanceT, Balanced, *}; -use crate::traits::{ - fungibles, - misc::{SameOrOther, TryDrop}, - tokens::{imbalance::TryMerge, AssetId, Balance}, +use crate::{ + pallet_prelude::{Decode, DecodeWithMemTracking, Encode, MaxEncodedLen, TypeInfo}, + traits::{ + fungibles, + misc::{SameOrOther, TryDrop}, + tokens::{imbalance::TryMerge, AssetId, Balance}, + }, }; use core::marker::PhantomData; use frame_support_procedural::{EqNoBound, PartialEqNoBound, RuntimeDebugNoBound}; @@ -47,7 +50,17 @@ impl HandleImbalanceDrop for () { /// /// Importantly, it has a special `Drop` impl, and cannot be created outside of this module. #[must_use] -#[derive(EqNoBound, PartialEqNoBound, RuntimeDebugNoBound)] +#[derive( + EqNoBound, + PartialEqNoBound, + RuntimeDebugNoBound, + Encode, + Decode, + DecodeWithMemTracking, + MaxEncodedLen, + TypeInfo, +)] +#[scale_info(skip_type_params(OnDrop, OppositeOnDrop))] pub struct Imbalance< B: Balance, OnDrop: HandleImbalanceDrop, diff --git a/substrate/frame/transaction-payment/Cargo.toml b/substrate/frame/transaction-payment/Cargo.toml index 3f22e59c86d7e..1848c0bd75e73 100644 --- a/substrate/frame/transaction-payment/Cargo.toml +++ b/substrate/frame/transaction-payment/Cargo.toml @@ -20,6 +20,7 @@ codec = { features = ["derive"], workspace = true } frame-benchmarking = { optional = true, workspace = true } frame-support = { workspace = true } frame-system = { workspace = true } +log = { workspace = true } scale-info = { features = ["derive"], workspace = true } serde = { optional = true, workspace = true, default-features = true } sp-io = { workspace = true } @@ -36,6 +37,7 @@ std = [ "frame-benchmarking?/std", "frame-support/std", "frame-system/std", + "log/std", "scale-info/std", "serde", "sp-io/std", diff --git a/substrate/frame/transaction-payment/asset-tx-payment/src/lib.rs b/substrate/frame/transaction-payment/asset-tx-payment/src/lib.rs index 6eeb634b9ccc5..214c61c3a6d6d 100644 --- a/substrate/frame/transaction-payment/asset-tx-payment/src/lib.rs +++ b/substrate/frame/transaction-payment/asset-tx-payment/src/lib.rs @@ -384,7 +384,7 @@ where }; match initial_payment { - InitialPayment::Native(already_withdrawn) => { + InitialPayment::Native(liquidity_info) => { // Take into account the weight used by this extension before calculating the // refund. let actual_ext_weight = ::WeightInfo::charge_asset_tx_payment_native(); @@ -392,11 +392,7 @@ where let mut actual_post_info = *post_info; actual_post_info.refund(unspent_weight); pallet_transaction_payment::ChargeTransactionPayment::::post_dispatch_details( - pallet_transaction_payment::Pre::Charge { - tip, - who, - imbalance: already_withdrawn, - }, + pallet_transaction_payment::Pre::Charge { tip, who, liquidity_info }, info, &actual_post_info, len, diff --git a/substrate/frame/transaction-payment/src/lib.rs b/substrate/frame/transaction-payment/src/lib.rs index 4424cbac0d615..4c20b0fdd298f 100644 --- a/substrate/frame/transaction-payment/src/lib.rs +++ b/substrate/frame/transaction-payment/src/lib.rs @@ -55,7 +55,7 @@ use frame_support::{ DispatchClass, DispatchInfo, DispatchResult, GetDispatchInfo, Pays, PostDispatchInfo, }, pallet_prelude::TransactionSource, - traits::{Defensive, EstimateCallFee, Get}, + traits::{Defensive, EstimateCallFee, Get, Imbalance, SuppressedDrop}, weights::{Weight, WeightToFee}, RuntimeDebugNoBound, }; @@ -88,6 +88,10 @@ pub mod weights; pub type Multiplier = FixedU128; type BalanceOf = <::OnChargeTransaction as OnChargeTransaction>::Balance; +type CreditOf = as SuppressedDrop>::Inner; +type StoredCreditOf = <::OnChargeTransaction as TxCreditHold>::Credit; + +const LOG_TARGET: &str = "runtime::txpayment"; /// A struct to update the weight multiplier per block. It implements `Convert`, meaning that it can convert the previous multiplier to the next one. This should @@ -411,6 +415,13 @@ pub mod pallet { #[pallet::storage] pub type StorageVersion = StorageValue<_, Releases, ValueQuery>; + /// The `OnChargeTransaction` stores the withdrawn tx fee here. + /// + /// Use `withdraw_txfee` and `remaining_txfee` to access from outside the crate. + #[pallet::storage] + #[pallet::whitelist_storage] + pub(crate) type TxPaymentCredit = StorageValue<_, StoredCreditOf>; + #[pallet::genesis_config] pub struct GenesisConfig { pub multiplier: Multiplier, @@ -446,6 +457,15 @@ pub mod pallet { NextFeeMultiplier::::mutate(|fm| { *fm = T::FeeMultiplierUpdate::convert(*fm); }); + + // We generally expect the `OnChargeTransaction` implementation to delete this value + // after each transaction. To make sure it is never stored between blocks we + // delete the value here just in case. + TxPaymentCredit::::take().map(|credit| { + log::error!(target: LOG_TARGET, "The `TxPaymentCredit` was stored between blocks. This is a bug."); + // Converting to inner makes sure that the drop implementation is called. + credit.into_inner() + }); } #[cfg(feature = "std")] @@ -579,7 +599,7 @@ impl Pallet { Self::compute_fee_details(len, &dispatch_info, tip) } - /// Compute the final fee value for a particular transaction. + /// Compute the final fee value (including tip) for a particular transaction. pub fn compute_fee( len: u32, info: &DispatchInfoOf, @@ -683,6 +703,66 @@ impl Pallet { pub fn deposit_fee_paid_event(who: T::AccountId, actual_fee: BalanceOf, tip: BalanceOf) { Self::deposit_event(Event::TransactionFeePaid { who, actual_fee, tip }); } + + /// Withdraw `amount` from the currents transaction's fees. + /// + /// If enough balance is available a credit of size `amount` is returned. + /// + /// # Warning + /// + /// Do **not** use this to pay for Weight fees. Use only to pay for storage fees + /// that can be rolled back by a storage transaction. + /// + /// # Note + /// + /// This is only useful if a pallet knows that the pre-dispatch weight was vastly + /// overestimated. Pallets need to make sure to leave enough balance to pay for the + /// transaction fees. They can do that by first drawing as much as they need and then + /// at the end of the transaction (when they know the post dispatch fee) return an error + /// in case not enough is left. The error will automatically roll back all the storage + /// changes done by the pallet including the balance drawn by calling this function. + pub fn withdraw_txfee(amount: Balance) -> Option> + where + CreditOf: Imbalance, + Balance: PartialOrd, + { + >::mutate(|credit| { + let credit = SuppressedDrop::as_mut(credit.as_mut()?); + if amount > credit.peek() { + return None + } + Some(credit.extract(amount)) + }) + } + + /// Deposit some additional balance. + pub fn deposit_txfee(deposit: CreditOf) + where + CreditOf: Imbalance, + { + >::mutate(|credit| { + if let Some(credit) = credit.as_mut().map(SuppressedDrop::as_mut) { + credit.subsume(deposit); + } else { + *credit = Some(SuppressedDrop::new(deposit)) + } + }); + } + + /// Return how much balance is currently available to pay for the transaction. + /// + /// Does **not** include the tip. + /// + /// If noone calls `charge_from_txfee` it is the same as the pre dispatch fee. + pub fn remaining_txfee() -> Balance + where + CreditOf: Imbalance, + Balance: Default, + { + >::get() + .map(|c| SuppressedDrop::as_ref(&c).peek()) + .unwrap_or_default() + } } impl Convert> for Pallet @@ -733,7 +813,7 @@ where who: &T::AccountId, call: &T::RuntimeCall, info: &DispatchInfoOf, - fee: BalanceOf, + fee_with_tip: BalanceOf, ) -> Result< ( BalanceOf, @@ -744,9 +824,13 @@ where let tip = self.0; <::OnChargeTransaction as OnChargeTransaction>::withdraw_fee( - who, call, info, fee, tip, + who, + call, + info, + fee_with_tip, + tip, ) - .map(|i| (fee, i)) + .map(|liquidity_info| (fee_with_tip, liquidity_info)) } fn can_withdraw_fee( @@ -757,12 +841,16 @@ where len: usize, ) -> Result, TransactionValidityError> { let tip = self.0; - let fee = Pallet::::compute_fee(len as u32, info, tip); + let fee_with_tip = Pallet::::compute_fee(len as u32, info, tip); <::OnChargeTransaction as OnChargeTransaction>::can_withdraw_fee( - who, call, info, fee, tip, + who, + call, + info, + fee_with_tip, + tip, )?; - Ok(fee) + Ok(fee_with_tip) } /// Get an appropriate priority for a transaction with the given `DispatchInfo`, encoded length @@ -782,7 +870,7 @@ where info: &DispatchInfoOf, len: usize, tip: BalanceOf, - final_fee: BalanceOf, + final_fee_with_tip: BalanceOf, ) -> TransactionPriority { // Calculate how many such extrinsics we could fit into an empty block and take the // limiting factor. @@ -831,7 +919,7 @@ where // enough to prevent a possible spam attack by sending invalid operational // extrinsics which push away regular transactions from the pool. let fee_multiplier = T::OperationalFeeMultiplier::get().saturated_into(); - let virtual_tip = final_fee.saturating_mul(fee_multiplier); + let virtual_tip = final_fee_with_tip.saturating_mul(fee_multiplier); let scaled_virtual_tip = max_reward(virtual_tip); scaled_tip.saturating_add(scaled_virtual_tip) @@ -860,7 +948,7 @@ pub enum Val { // who paid the fee who: T::AccountId, // transaction fee - fee: BalanceOf, + fee_with_tip: BalanceOf, }, NoCharge, } @@ -872,8 +960,9 @@ pub enum Pre { tip: BalanceOf, // who paid the fee who: T::AccountId, - // imbalance resulting from withdrawing the fee - imbalance: <::OnChargeTransaction as OnChargeTransaction>::LiquidityInfo, + // implementation defined type that is passed into the post charge function + liquidity_info: + <::OnChargeTransaction as OnChargeTransaction>::LiquidityInfo, }, NoCharge { // weight initially estimated by the extension, to be refunded @@ -885,7 +974,7 @@ impl core::fmt::Debug for Pre { #[cfg(feature = "std")] fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { match self { - Pre::Charge { tip, who, imbalance: _ } => { + Pre::Charge { tip, who, liquidity_info: _ } => { write!(f, "Charge {{ tip: {:?}, who: {:?}, imbalance: }}", tip, who) }, Pre::NoCharge { refund } => write!(f, "NoCharge {{ refund: {:?} }}", refund), @@ -927,14 +1016,14 @@ where let Ok(who) = frame_system::ensure_signed(origin.clone()) else { return Ok((ValidTransaction::default(), Val::NoCharge, origin)); }; - let final_fee = self.can_withdraw_fee(&who, call, info, len)?; + let fee_with_tip = self.can_withdraw_fee(&who, call, info, len)?; let tip = self.0; Ok(( ValidTransaction { - priority: Self::get_priority(info, len, tip, final_fee), + priority: Self::get_priority(info, len, tip, fee_with_tip), ..Default::default() }, - Val::Charge { tip: self.0, who, fee: final_fee }, + Val::Charge { tip: self.0, who, fee_with_tip }, origin, )) } @@ -948,10 +1037,11 @@ where _len: usize, ) -> Result { match val { - Val::Charge { tip, who, fee } => { + Val::Charge { tip, who, fee_with_tip } => { // Mutating call to `withdraw_fee` to actually charge for the transaction. - let (_final_fee, imbalance) = self.withdraw_fee(&who, call, info, fee)?; - Ok(Pre::Charge { tip, who, imbalance }) + let (_fee_with_tip, liquidity_info) = + self.withdraw_fee(&who, call, info, fee_with_tip)?; + Ok(Pre::Charge { tip, who, liquidity_info }) }, Val::NoCharge => Ok(Pre::NoCharge { refund: self.weight(call) }), } @@ -964,18 +1054,28 @@ where len: usize, _result: &DispatchResult, ) -> Result { - let (tip, who, imbalance) = match pre { - Pre::Charge { tip, who, imbalance } => (tip, who, imbalance), + let (tip, who, liquidity_info) = match pre { + Pre::Charge { tip, who, liquidity_info } => (tip, who, liquidity_info), Pre::NoCharge { refund } => { // No-op: Refund everything return Ok(refund) }, }; - let actual_fee = Pallet::::compute_actual_fee(len as u32, info, &post_info, tip); + let actual_fee_with_tip = + Pallet::::compute_actual_fee(len as u32, info, &post_info, tip); T::OnChargeTransaction::correct_and_deposit_fee( - &who, info, &post_info, actual_fee, tip, imbalance, + &who, + info, + &post_info, + actual_fee_with_tip, + tip, + liquidity_info, )?; - Pallet::::deposit_event(Event::::TransactionFeePaid { who, actual_fee, tip }); + Pallet::::deposit_event(Event::::TransactionFeePaid { + who, + actual_fee: actual_fee_with_tip, + tip, + }); Ok(Weight::zero()) } } diff --git a/substrate/frame/transaction-payment/src/payment.rs b/substrate/frame/transaction-payment/src/payment.rs index b8a047fee3e65..0164818fe9268 100644 --- a/substrate/frame/transaction-payment/src/payment.rs +++ b/substrate/frame/transaction-payment/src/payment.rs @@ -16,28 +16,30 @@ // limitations under the License. /// ! Traits and default implementation for paying transaction fees. -use crate::Config; +use crate::{Config, Pallet, TxPaymentCredit, LOG_TARGET}; +use codec::{DecodeWithMemTracking, FullCodec, MaxEncodedLen}; use core::marker::PhantomData; -use sp_runtime::{ - traits::{CheckedSub, DispatchInfoOf, PostDispatchInfoOf, Saturating, Zero}, - transaction_validity::InvalidTransaction, -}; - use frame_support::{ traits::{ - fungible::{Balanced, Credit, Debt, Inspect}, + fungible::{Balanced, Credit, Inspect}, tokens::{Precision, WithdrawConsequence}, - Currency, ExistenceRequirement, Imbalance, OnUnbalanced, WithdrawReasons, + Currency, ExistenceRequirement, Imbalance, NoDrop, OnUnbalanced, SuppressedDrop, + WithdrawReasons, }, unsigned::TransactionValidityError, }; +use scale_info::TypeInfo; +use sp_runtime::{ + traits::{CheckedSub, DispatchInfoOf, PostDispatchInfoOf, Saturating, Zero}, + transaction_validity::InvalidTransaction, +}; type NegativeImbalanceOf = ::AccountId>>::NegativeImbalance; /// Handle withdrawing, refunding and depositing of transaction fees. -pub trait OnChargeTransaction { +pub trait OnChargeTransaction: TxCreditHold { /// The underlying integer type in which fees are calculated. type Balance: frame_support::traits::tokens::Balance; @@ -46,23 +48,21 @@ pub trait OnChargeTransaction { /// Before the transaction is executed the payment of the transaction fees /// need to be secured. /// - /// Note: The `fee` already includes the `tip`. + /// Returns the tip credit fn withdraw_fee( who: &T::AccountId, call: &T::RuntimeCall, dispatch_info: &DispatchInfoOf, - fee: Self::Balance, + fee_with_tip: Self::Balance, tip: Self::Balance, ) -> Result; /// Check if the predicted fee from the transaction origin can be withdrawn. - /// - /// Note: The `fee` already includes the `tip`. fn can_withdraw_fee( who: &T::AccountId, call: &T::RuntimeCall, dispatch_info: &DispatchInfoOf, - fee: Self::Balance, + fee_with_tip: Self::Balance, tip: Self::Balance, ) -> Result<(), TransactionValidityError>; @@ -75,9 +75,9 @@ pub trait OnChargeTransaction { who: &T::AccountId, dispatch_info: &DispatchInfoOf, post_info: &PostDispatchInfoOf, - corrected_fee: Self::Balance, + corrected_fee_with_tip: Self::Balance, tip: Self::Balance, - already_withdrawn: Self::LiquidityInfo, + liquidity_info: Self::LiquidityInfo, ) -> Result<(), TransactionValidityError>; #[cfg(feature = "runtime-benchmarks")] @@ -87,6 +87,23 @@ pub trait OnChargeTransaction { fn minimum_balance() -> Self::Balance; } +/// Needs to be implemented for every [`OnChargeTransaction`]. +/// +/// Cannot be added to `OnChargeTransaction` directly as this would +/// cause cycles in trait resolution. +pub trait TxCreditHold { + /// The credit that is used to represent the withdrawn transaction fees. + /// + /// The pallet will put this into a temporary storage item in order to + /// make it available to other pallets during tx application. + /// + /// Is only used within a transaction. Hence changes to the encoding of this + /// type **won't** require a storage migration. + /// + /// Set to `()` if your `OnChargeTransaction` impl does not store the credit. + type Credit: FullCodec + DecodeWithMemTracking + MaxEncodedLen + TypeInfo + SuppressedDrop; +} + /// Implements transaction payment for a pallet implementing the [`frame_support::traits::fungible`] /// trait (eg. pallet_balances) using an unbalance handler (implementing /// [`OnUnbalanced`]). @@ -98,47 +115,52 @@ pub struct FungibleAdapter(PhantomData<(F, OU)>); impl OnChargeTransaction for FungibleAdapter where T: Config, - F: Balanced, - OU: OnUnbalanced>, + T::OnChargeTransaction: TxCreditHold>>, + F: Balanced + 'static, + OU: OnUnbalanced<::Inner>, { - type LiquidityInfo = Option>; + type LiquidityInfo = Option<::Inner>; type Balance = ::AccountId>>::Balance; fn withdraw_fee( who: &::AccountId, _call: &::RuntimeCall, _dispatch_info: &DispatchInfoOf<::RuntimeCall>, - fee: Self::Balance, - _tip: Self::Balance, + fee_with_tip: Self::Balance, + tip: Self::Balance, ) -> Result { - if fee.is_zero() { + if fee_with_tip.is_zero() { return Ok(None) } - match F::withdraw( + let credit = F::withdraw( who, - fee, + fee_with_tip, Precision::Exact, frame_support::traits::tokens::Preservation::Preserve, frame_support::traits::tokens::Fortitude::Polite, - ) { - Ok(imbalance) => Ok(Some(imbalance)), - Err(_) => Err(InvalidTransaction::Payment.into()), - } + ) + .map_err(|_| InvalidTransaction::Payment)?; + + let (tip_credit, inclusion_fee) = credit.split(tip); + + >::deposit_txfee(inclusion_fee); + + Ok(Some(tip_credit)) } fn can_withdraw_fee( who: &T::AccountId, _call: &T::RuntimeCall, _dispatch_info: &DispatchInfoOf, - fee: Self::Balance, + fee_with_tip: Self::Balance, _tip: Self::Balance, ) -> Result<(), TransactionValidityError> { - if fee.is_zero() { + if fee_with_tip.is_zero() { return Ok(()) } - match F::can_withdraw(who, fee) { + match F::can_withdraw(who, fee_with_tip) { WithdrawConsequence::Success => Ok(()), _ => Err(InvalidTransaction::Payment.into()), } @@ -148,32 +170,40 @@ where who: &::AccountId, _dispatch_info: &DispatchInfoOf<::RuntimeCall>, _post_info: &PostDispatchInfoOf<::RuntimeCall>, - corrected_fee: Self::Balance, + corrected_fee_with_tip: Self::Balance, tip: Self::Balance, - already_withdrawn: Self::LiquidityInfo, + tip_credit: Self::LiquidityInfo, ) -> Result<(), TransactionValidityError> { - if let Some(paid) = already_withdrawn { - // Calculate how much refund we should return - let refund_amount = paid.peek().saturating_sub(corrected_fee); - // Refund to the the account that paid the fees if it exists & refund is non-zero. - // Otherwise, don't refund anything. - let refund_imbalance = - if refund_amount > Zero::zero() && F::total_balance(who) > F::Balance::zero() { - F::deposit(who, refund_amount, Precision::BestEffort) - .unwrap_or_else(|_| Debt::::zero()) - } else { - Debt::::zero() - }; - // merge the imbalance caused by paying the fees and refunding parts of it again. - let adjusted_paid: Credit = paid - .offset(refund_imbalance) - .same() - .map_err(|_| TransactionValidityError::Invalid(InvalidTransaction::Payment))?; - // Call someone else to handle the imbalance (fee and tip separately) - let (tip, fee) = adjusted_paid.split(tip); - OU::on_unbalanceds(Some(fee).into_iter().chain(Some(tip))); + let corrected_fee = corrected_fee_with_tip.saturating_sub(tip); + + let remaining_credit = >::take() + .map(|stored_credit| stored_credit.into_inner()) + .unwrap_or_default(); + + // If pallets take away too much it makes the transaction invalid. They need to make + // sure that this does not happen. We do not invalide the transaction because we already + // executed it and we rather collect too little fees than none at all. + if remaining_credit.peek() < corrected_fee { + log::error!(target: LOG_TARGET, "Not enough balance on hold to pay tx fees. This is a bug."); } + // skip refund if account was killed by the tx + let fee_credit = if frame_system::Pallet::::account_exists(who) { + let (mut fee_credit, refund_credit) = remaining_credit.split(corrected_fee); + // resolve might fail if refund is below the ed and account + // is kept alive by other providers + if !refund_credit.peek().is_zero() { + if let Err(not_refunded) = F::resolve(who, refund_credit) { + fee_credit.subsume(not_refunded); + } + } + fee_credit + } else { + remaining_credit + }; + + OU::on_unbalanceds(Some(fee_credit).into_iter().chain(tip_credit)); + Ok(()) } @@ -188,6 +218,14 @@ where } } +impl TxCreditHold for FungibleAdapter +where + T: Config, + F: Balanced + 'static, +{ + type Credit = NoDrop::AccountId, F>>; +} + /// Implements the transaction payment for a pallet implementing the [`Currency`] /// trait (eg. the pallet_balances) using an unbalance handler (implementing /// [`OnUnbalanced`]). @@ -228,10 +266,10 @@ where who: &T::AccountId, _call: &T::RuntimeCall, _info: &DispatchInfoOf, - fee: Self::Balance, + fee_with_tip: Self::Balance, tip: Self::Balance, ) -> Result { - if fee.is_zero() { + if fee_with_tip.is_zero() { return Ok(None) } @@ -241,7 +279,7 @@ where WithdrawReasons::TRANSACTION_PAYMENT | WithdrawReasons::TIP }; - match C::withdraw(who, fee, withdraw_reason, ExistenceRequirement::KeepAlive) { + match C::withdraw(who, fee_with_tip, withdraw_reason, ExistenceRequirement::KeepAlive) { Ok(imbalance) => Ok(Some(imbalance)), Err(_) => Err(InvalidTransaction::Payment.into()), } @@ -254,10 +292,10 @@ where who: &T::AccountId, _call: &T::RuntimeCall, _info: &DispatchInfoOf, - fee: Self::Balance, + fee_with_tip: Self::Balance, tip: Self::Balance, ) -> Result<(), TransactionValidityError> { - if fee.is_zero() { + if fee_with_tip.is_zero() { return Ok(()) } @@ -267,9 +305,10 @@ where WithdrawReasons::TRANSACTION_PAYMENT | WithdrawReasons::TIP }; - let new_balance = - C::free_balance(who).checked_sub(&fee).ok_or(InvalidTransaction::Payment)?; - C::ensure_can_withdraw(who, fee, withdraw_reason, new_balance) + let new_balance = C::free_balance(who) + .checked_sub(&fee_with_tip) + .ok_or(InvalidTransaction::Payment)?; + C::ensure_can_withdraw(who, fee_with_tip, withdraw_reason, new_balance) .map(|_| ()) .map_err(|_| InvalidTransaction::Payment.into()) } @@ -283,13 +322,13 @@ where who: &T::AccountId, _dispatch_info: &DispatchInfoOf, _post_info: &PostDispatchInfoOf, - corrected_fee: Self::Balance, + corrected_fee_with_tip: Self::Balance, tip: Self::Balance, already_withdrawn: Self::LiquidityInfo, ) -> Result<(), TransactionValidityError> { if let Some(paid) = already_withdrawn { // Calculate how much refund we should return - let refund_amount = paid.peek().saturating_sub(corrected_fee); + let refund_amount = paid.peek().saturating_sub(corrected_fee_with_tip); // refund to the the account that paid the fees. If this fails, the // account might have dropped below the existential balance. In // that case we don't refund anything. @@ -317,3 +356,8 @@ where C::minimum_balance() } } + +#[allow(deprecated)] +impl TxCreditHold for CurrencyAdapter { + type Credit = (); +}