[MOON-3323] Refactor XCM Transactor to Delegate Fee Pricing to XCM Weight Trader #3569
Conversation
librelois
added
B7-runtimenoteworthy
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
librelois
changed the title
librelois
changed the title
Moonbase Weight Difference Report
Moonriver Weight Difference Report
Moonbeam Weight Difference Report
|
WASM runtime size check:Compared to target branchMoonbase runtime: 2124 KB (no changes) ✅ Moonbeam runtime: 2236 KB (-4 KB) ✅ Moonriver runtime: 2236 KB (-4 KB) ✅ Compared to latest release (runtime-4100)Moonbase runtime: 2124 KB (+188 KB compared to latest release) Moonbeam runtime: 2236 KB (+204 KB compared to latest release) Moonriver runtime: 2236 KB (+204 KB compared to latest release) |
Coverage Report@@ Coverage Diff @@
## master elois-xcm-transactor-trader +/- ##
===============================================================
- Coverage 76.68% 76.65% -0.03%
+ Files 389 390 +1
+ Lines 76615 76634 +19
===============================================================
- Hits 58745 58739 -6
+ Misses 17870 17895 +25
|
There was a problem hiding this comment.
@librelois we may need a migration. Can you confirm that every asset in DestinationAssetFeePerSecond in the WeightTrader pallet?
|
|
||
| // We have used 1000 units to pay for the fees in the relay (plus 1 transact_extra_weight), | ||
| // so balance and supply should have changed | ||
| const expectedBalance = 100000000000000n - 1000n - 1n; |
There was a problem hiding this comment.
This could be a constant and re-used in registerAndFundAsset
| type HrmpManipulatorOrigin = GeneralAdminOrRoot; | ||
| type HrmpOpenOrigin = FastGeneralAdminOrRoot; | ||
| type MaxHrmpFee = xcm_builder::Case<MaxHrmpRelayFee>; | ||
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; |
There was a problem hiding this comment.
Could be replaced with:
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; | |
| type FeeTrader = XcmWeightTrader; |
| type HrmpManipulatorOrigin = GeneralAdminOrRoot; | ||
| type HrmpOpenOrigin = FastGeneralAdminOrRoot; | ||
| type MaxHrmpFee = xcm_builder::Case<MaxHrmpRelayFee>; | ||
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; |
There was a problem hiding this comment.
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; | |
| type FeeTrader = XcmWeightTrader; |
| type HrmpManipulatorOrigin = GeneralAdminOrRoot; | ||
| type HrmpOpenOrigin = FastGeneralAdminOrRoot; | ||
| type MaxHrmpFee = xcm_builder::Case<MaxHrmpRelayFee>; | ||
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; |
There was a problem hiding this comment.
| type FeeTrader = pallet_xcm_weight_trader::Pallet<Runtime>; | |
| type FeeTrader = XcmWeightTrader; |
| @@ -0,0 +1,79 @@ | |||
| // Copyright 2019-2025 PureStake Inc. | |||
There was a problem hiding this comment.
| // Copyright 2019-2025 PureStake Inc. | |
| // Copyright 2025 Moonbeam foundation |
| // along with Moonbeam. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| //! Test primitives for Moonbeam, including mock implementations for testing. | ||
There was a problem hiding this comment.
Lets enforce that this can only be used in tests.
| #![cfg(test)] |
| id: 1n, | ||
| location: RELAY_SOURCE_LOCATION, | ||
| metadata: relayAssetMetadata, | ||
| relativePrice: 1n, |
Brief summary of changes that might impact apps and tools that interact with the chain:
pallet-xcm-transactorextrinsicsset_fee_per_secondandremove_fee_per_secondare removed from the callable API (their call indices remain reserved as commented holes for SCALE compatibility).DestinationAssetFeePerSecondstorage map inpallet-xcm-transactoris removed.pallet-xcm-weight-trader::SupportedAssetsinstead of a dedicated map inpallet-xcm-transactor.DestFeePerSecondChangedandDestFeePerSecondRemovedevents are removed (kept as commented placeholders with their original codec indices).Error::FeePerSecondNotSetis removed; callers now seeUnableToWithdrawAsset/AssetHasNoReserve/AssetIsNotReserveInDestinationdepending on the failure mode. All error and event variants after the removed ones are explicitly annotated with#[codec(index = N)]to preserve their original SCALE indices.What does it do?
This PR refactors XCM fee handling so that:
pallet-xcm-transactorno longer stores or manages fee-per-second data; instead, it delegates fee computation to a new traitxcm_primitives::XcmFeeTrader.pallet-xcm-weight-traderimplementsXcmFeeTraderand becomes the single source of truth for XCM execution fee pricing.pallet-xcm-transactor’s internalcalculate_fee:T::FeeTrader::compute_feeto obtain the fee amount for a givenweightandfee_location.T::ReserveProvider, returningAssetHasNoReserveorAssetIsNotReserveInDestinationwhen invalid.FeeTraderSetteris scoped topallet-xcm-transactortests/benchmarks only and is no longer implemented in the runtimes; runtime governance should now use the xcm-weight-trader pallet extrinsics directly for fee configuration.What important points should reviewers know?
#[pallet::call_index]positions as commented-out stubs, leaving intentional “holes” so existing call indices remain stable.ErrorandEventenums are carefully annotated with#[codec(index = N)]for all variants after the removed ones, preserving the historical SCALE indices used on-chain.XcmFeeTradernow lives inprimitives/xcm/src/fee_trader.rsand is re-exported fromprimitives/xcm/src/lib.rs.XcmFeeTrader::compute_fee; it is explicitly done inpallet-xcm-transactor::calculate_fee, which usesT::ReserveProviderand maps failures to pallet errors.pallet-xcm-transactorunit tests and benchmarks are updated to:UnableToWithdrawAsset, reserve-related errors) instead of the previousFeePerSecondNotSet.runtime/common/src/migrations.rsadds a newDestinationAssetFeePerSecondStorageNameconstant and aResetStorageentry forpallet_xcm_transactor::Pallet<Runtime>’sDestinationAssetFeePerSecondmap inMultiBlockMigrations, ensuring the old map is wiped clean on all runtimes.Is there something left for follow-up PRs?
No
What alternative implementations were considered?
N/A
Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
No
What value does it bring to the blockchain users?
pallet-xcm-weight-trader, reducing configuration drift and simplifying audits.What's solved in this change and what features are modified?
Brief summary of issue that should be resolved (if any)
pallet-xcm-transactorandpallet-xcm-weight-trader.DestinationAssetFeePerSecondmap and the need for transactor-specific fee extrinsics.pallet-xcm-weight-traderdirectly intopallet-xcm-transactorvia theXcmFeeTradertrait.High-level summary of feature changes or specifications of new feature
XcmFeeTraderis promoted to a shared primitive trait inprimitives/xcm, implemented bypallet-xcm-weight-trader.pallet-xcm-transactorcalculates fees exclusively viaXcmFeeTrader, then enforces reserve validity throughReserveProvider.FeeTraderSetteris limited to tests/benchmarks and is not part of the runtime API surface.What changes to storage structures, processes or high-level assumptions have been made?
pallet-xcm-weight-trader’sSupportedAssetsstorage, not bypallet-xcm-transactor.Low-level summary of any processes or storage structures that are changed.
pallet-xcm-transactor::DestinationAssetFeePerSecondmap is removed, and aResetStoragemulti-block migration clears any existing keys on-chain.pallet-xcm-weight-trader::SupportedAssetsbecomes the authoritative store for per-asset pricing and is used by theXcmFeeTraderimplementation.pallet-xcm-transactorcomputes the weight for an XCM call.calculate_feecallsT::FeeTrader::compute_fee(weight, &fee_location, explicit_amount)to get the fee amount.T::ReserveProviderand builds the feeAsset.Are there additional mechanisms or storage structures indirectly affected by these changes?
Describe any known side-effects of this change (not directly visible in diff).
XcmTransactor::DestinationAssetFeePerSecondwill need to be updated to querypallet-xcm-weight-traderinstead.XcmTransactor::set_fee_per_second/remove_fee_per_secondmust now usepallet-xcm-weight-traderextrinsics.Describe any relationships, high-level constraints, or assumptions that this aims to change.
pallet-xcm-weight-trader: defines pricing.pallet-xcm-transactor: computes required weight and enforces reserve correctness, but does not own pricing data.pallet-xcm-weight-traderis present and correctly configured in all runtimes usingpallet-xcm-transactor.What risks have already been internally considered or handled?
Describe any internal concerns related to these changes.
ResetStoragemigration forDestinationAssetFeePerSecondis misconfigured (wrong storage name or pallet), stale data could remain or, worse, unrelated keys could be cleared.SupportedAssetsis not configured for commonly used fee assets, transactor calls will fail withUnableToWithdrawAssetor reserve-related errors.Describe how these risks were handled (e.g tests, design-decisions, rationale).
ResetStoragemigration for theDestinationAssetFeePerSecondmap, reusing the existing multi-block migration framework to avoid block-weight issues on chains with many entries.What runtime is this intended for?
Runtime 4200