fix: dispatch with extra gas transaction charges previous currency#1372
fix: dispatch with extra gas transaction charges previous currency#1372
Conversation
…ng of set_currency calls
|
Crate versions that have been updated:
Runtime version has not been increased. |
|
I cannot run you prolly need to do add std feature in cargo.toml, cor currencies |
| bob_btc_after < bob_btc_before, | ||
| "BTC balance should decrease — fee must be charged in the new currency" | ||
| ); | ||
| // HDX must not be touched. |
There was a problem hiding this comment.
| // HDX must not be touched. |
| 1_000_000_000_000_000_000i128, | ||
| )); | ||
|
|
||
| // EVM accounts have WETH automatically set as their fee currency on account creation |
There was a problem hiding this comment.
| // EVM accounts have WETH automatically set as their fee currency on account creation |
| let weth_after = hydradx_runtime::Tokens::free_balance(WETH, &evm_acc); | ||
| let dai_after = hydradx_runtime::Tokens::free_balance(DAI, &evm_acc); | ||
|
|
||
| // Fee must be charged in DAI (the new currency), not WETH (EVM default). |
There was a problem hiding this comment.
| // Fee must be charged in DAI (the new currency), not WETH (EVM default). |
There was a problem hiding this comment.
i am removing these comments because in the assert you place very similar error message, so the comments feel redundant. small details...
| dai_after < dai_before, | ||
| "DAI balance should decrease — fee must be charged in the new currency" | ||
| ); | ||
| // WETH must not be touched. |
There was a problem hiding this comment.
| // WETH must not be touched. |
|
|
||
| let bob_btc_after = hydradx_runtime::Tokens::free_balance(BTC, &AccountId::from(BOB)); | ||
|
|
||
| // BTC must not be charged — the resolver did not reach the nested set_currency. |
There was a problem hiding this comment.
| // BTC must not be charged — the resolver did not reach the nested set_currency. |
| impl<T, MC, DF, FR, WF> TransferFees<T, MC, DF, FR, WF> | ||
| where | ||
| T: Config + pallet_utility::Config, | ||
| T: Config, |
There was a problem hiding this comment.
i think you can do the same bound cleanups in the OnChargeTransaction impl for TransferFees
…-node into fix/set-currency-charges
…proved clarity and consistency
…-node into fix/set-currency-charges
…and runtime-integration-tests
There was a problem hiding this comment.
Pull request overview
Fixes fee-currency resolution for set_currency when it is wrapped by dispatch_with_extra_gas, ensuring transaction fees are charged in the newly selected currency (matching the runtime’s authoritative call-to-currency resolver) and adds integration coverage for both Substrate and EVM accounts.
Changes:
- Remove duplicated call-pattern matching in
TransferFees::resolve_currency_from_calland delegate toT::TryCallCurrency::try_convert. - Update test mock configuration to use a call-currency converter that recognizes
set_currency/batch patterns. - Add integration tests covering
dispatch_with_extra_gas { set_currency }for both Substrate and EVM accounts, plus a negative test for deeper nesting; bump runtime/pallet versions accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/hydradx/src/lib.rs | Bumps runtime spec_version to reflect the behavior change. |
| runtime/hydradx/Cargo.toml | Bumps runtime crate version. |
| pallets/transaction-multi-payment/src/lib.rs | Delegates fee-currency resolution to T::TryCallCurrency to avoid divergence and fix the dispatcher-wrapped set_currency case. |
| pallets/transaction-multi-payment/src/mock.rs | Switches test runtime to a call-currency resolver that supports set_currency + utility batching patterns. |
| pallets/transaction-multi-payment/Cargo.toml | Bumps pallet version and adjusts std/dev-dependency features for test/build compatibility. |
| integration-tests/src/non_native_fee.rs | Adds regression tests for dispatcher-wrapped set_currency fee charging (Substrate + EVM) and a nested-negative case. |
| integration-tests/Cargo.toml | Bumps integration tests crate version. |
| Cargo.lock | Updates lockfile for the version bumps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// (which requires `pallet_dispatcher` dependency). | ||
| pub struct TestCallCurrency<T>(PhantomData<T>); |
There was a problem hiding this comment.
TestCallCurrency is documented as “Test-only” but is defined in the main pallet module without any cfg(test)/feature gating. This makes it part of the pallet’s public API (and can be accidentally used in production runtimes), and can also increase compiled artifact size/complexity unnecessarily. Consider moving this helper into mock.rs (or a #[cfg(test)] module) or gating the type/impl behind #[cfg(test)] (or a dedicated test feature) so it’s only available for unit tests.
| /// (which requires `pallet_dispatcher` dependency). | |
| pub struct TestCallCurrency<T>(PhantomData<T>); | |
| /// (which requires `pallet_dispatcher` dependency). | |
| #[cfg(test)] | |
| pub struct TestCallCurrency<T>(PhantomData<T>); | |
| #[cfg(test)] |
| use crate::polkadot_test_net::*; | ||
| use frame_support::sp_runtime::codec::Encode; | ||
| use frame_support::{ | ||
| assert_ok, | ||
| dispatch::DispatchInfo, | ||
| dispatch::{DispatchInfo, GetDispatchInfo}, | ||
| sp_runtime::{traits::DispatchTransaction, FixedU128, Permill}, |
There was a problem hiding this comment.
In integration tests, other modules import Encode directly from sp_runtime::codec::Encode rather than via the frame_support::sp_runtime re-export. For consistency (and to avoid relying on a re-export that could change), consider switching this import to sp_runtime::codec::Encode like the other integration test files.
|
Quick benchmark at commit 889ad50 has been executed successfully. |
Description
Fixes fee currency resolution for
dispatch_with_extra_gaswrappingset_currencycalls.When users submitted
dispatch_with_extra_gas { set_currency { currency: X } }, the transaction fee was incorrectly charged in the previous account currency instead of the newly specified currencyX. Directset_currencycalls worked correctly.This affected both substrate accounts (issue #1296) and EVM accounts (issue #1293).
Root Cause
The pallet's
resolve_currency_from_callfunction had duplicate currency resolution logic that was missing thedispatch_with_extra_gashandler, while the runtime'sTryCallCurrencyimplementation already handled it correctly. The two resolvers diverged over time.Fix
Replaced the duplicate pattern-matching logic in
resolve_currency_from_callwith a simple delegation toT::TryCallCurrency::try_convert(). This ensures both code paths use the same authoritative resolver and eliminates future divergence.Related Issue
Fixes: #1296 #1293
Motivation and Context
How Has This Been Tested?
Integration tests have been added to test the scenario in which new currencies are being charged successfully.
Checklist: