diff --git a/polkadot/xcm/src/v5/mod.rs b/polkadot/xcm/src/v5/mod.rs
index 924e514618dd942f054d463884844f32d7ad4867..e8c07549d90ebe56a3a3759fb4122f74c7306412 100644
--- a/polkadot/xcm/src/v5/mod.rs
+++ b/polkadot/xcm/src/v5/mod.rs
@@ -1055,10 +1055,11 @@ pub enum Instruction<Call> {
/// Errors: If the given origin is `Some` and not equal to the current Origin register.
UnpaidExecution { weight_limit: WeightLimit, check_origin: Option<Location> },
- /// Pay Fees.
+ /// Takes an asset, uses it to pay for execution and puts the rest in the fees register.
///
/// Successor to `BuyExecution`.
- /// Defined in fellowship RFC 105.
+ /// Defined in [Fellowship RFC 105](https://github.com/polkadot-fellows/RFCs/pull/105).
+ /// Subsequent `PayFees` after the first one are noops.
#[builder(pays_fees)]
PayFees { asset: Asset },
diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs
index 19a6a645083f2925aa4d4d228808a2c1a12827cd..794b3cc93ee7985952982912c83903b151d2be76 100644
--- a/polkadot/xcm/xcm-executor/src/lib.rs
+++ b/polkadot/xcm/xcm-executor/src/lib.rs
@@ -99,6 +99,7 @@ pub struct XcmExecutor<Config: config::Config> {
/// Stores the current message's weight.
message_weight: Weight,
asset_claimer: Option<Location>,
+ already_paid_fees: bool,
_config: PhantomData<Config>,
}
@@ -206,6 +207,9 @@ impl<Config: config::Config> XcmExecutor<Config> {
pub fn set_message_weight(&mut self, weight: Weight) {
self.message_weight = weight;
}
+ pub fn already_paid_fees(&self) -> bool {
+ self.already_paid_fees
+ }
}
pub struct WeighedMessage<Call>(Weight, Xcm<Call>);
@@ -362,6 +366,7 @@ impl<Config: config::Config> XcmExecutor<Config> {
asset_used_in_buy_execution: None,
message_weight: Weight::zero(),
asset_claimer: None,
+ already_paid_fees: false,
_config: PhantomData,
}
}
@@ -1361,14 +1366,12 @@ impl<Config: config::Config> XcmExecutor<Config> {
result
},
PayFees { asset } => {
- // Message was not weighed, there is nothing to pay.
- if self.message_weight == Weight::zero() {
- tracing::warn!(
- target: "xcm::executor::PayFees",
- "Message was not weighed or weight was 0. Nothing will be charged.",
- );
+ // If we've already paid for fees, do nothing.
+ if self.already_paid_fees {
return Ok(());
}
+ // Make sure `PayFees` won't be processed again.
+ self.already_paid_fees = true;
// Record old holding in case we need to rollback.
let old_holding = self.holding.clone();
// The max we're willing to pay for fees is decided by the `asset` operand.
@@ -1377,19 +1380,21 @@ impl<Config: config::Config> XcmExecutor<Config> {
asset_for_fees = ?asset,
message_weight = ?self.message_weight,
);
- let max_fee =
- self.holding.try_take(asset.into()).map_err(|_| XcmError::NotHoldingFees)?;
// Pay for execution fees.
let result = Config::TransactionalProcessor::process(|| {
+ let max_fee =
+ self.holding.try_take(asset.into()).map_err(|_| XcmError::NotHoldingFees)?;
let unspent =
self.trader.buy_weight(self.message_weight, max_fee, &self.context)?;
- // Move unspent to the `fees` register.
+ // Move unspent to the `fees` register, it can later be moved to holding
+ // by calling `RefundSurplus`.
self.fees.subsume_assets(unspent);
Ok(())
});
if Config::TransactionalProcessor::IS_TRANSACTIONAL && result.is_err() {
- // Rollback.
+ // Rollback on error.
self.holding = old_holding;
+ self.already_paid_fees = false;
}
result
},
diff --git a/polkadot/xcm/xcm-executor/src/tests/mock.rs b/polkadot/xcm/xcm-executor/src/tests/mock.rs
index c0bcfe88d2baa8aff42142c368b52f035a30b552..f94f289ff563c932260636920b38abc575b919be 100644
--- a/polkadot/xcm/xcm-executor/src/tests/mock.rs
+++ b/polkadot/xcm/xcm-executor/src/tests/mock.rs
@@ -30,8 +30,8 @@ use xcm::prelude::*;
use crate::{
traits::{
- DropAssets, FeeManager, Properties, ShouldExecute, TransactAsset, WeightBounds,
- WeightTrader,
+ DropAssets, FeeManager, ProcessTransaction, Properties, ShouldExecute, TransactAsset,
+ WeightBounds, WeightTrader,
},
AssetsInHolding, Config, FeeReason, XcmExecutor,
};
@@ -267,6 +267,20 @@ impl FeeManager for TestFeeManager {
fn handle_fee(_: Assets, _: Option<&XcmContext>, _: FeeReason) {}
}
+/// Dummy transactional processor that doesn't rollback storage changes, just
+/// aims to rollback executor state.
+pub struct TestTransactionalProcessor;
+impl ProcessTransaction for TestTransactionalProcessor {
+ const IS_TRANSACTIONAL: bool = true;
+
+ fn process<F>(f: F) -> Result<(), XcmError>
+ where
+ F: FnOnce() -> Result<(), XcmError>,
+ {
+ f()
+ }
+}
+
/// Test XcmConfig that uses all the test implementations in this file.
pub struct XcmConfig;
impl Config for XcmConfig {
@@ -294,7 +308,7 @@ impl Config for XcmConfig {
type CallDispatcher = Self::RuntimeCall;
type SafeCallFilter = Everything;
type Aliasers = Nothing;
- type TransactionalProcessor = ();
+ type TransactionalProcessor = TestTransactionalProcessor;
type HrmpNewChannelOpenRequestHandler = ();
type HrmpChannelAcceptedHandler = ();
type HrmpChannelClosingHandler = ();
diff --git a/polkadot/xcm/xcm-executor/src/tests/pay_fees.rs b/polkadot/xcm/xcm-executor/src/tests/pay_fees.rs
index 4c196831e6a46d6d4099bc9564ef31e3ce04d01d..d5b21d3bc601ff5f62ce7544215f2193b54db727 100644
--- a/polkadot/xcm/xcm-executor/src/tests/pay_fees.rs
+++ b/polkadot/xcm/xcm-executor/src/tests/pay_fees.rs
@@ -78,6 +78,7 @@ fn works_for_delivery_fees() {
// Build xcm.
let xcm = Xcm::<TestCall>::builder()
.withdraw_asset((Here, 100u128))
+ // We load `10` plancks to pay for fees.
.pay_fees((Here, 10u128))
// Send a bunch of messages, each charging delivery fees.
.report_error(query_response_info.clone())
@@ -212,6 +213,36 @@ fn putting_all_assets_in_pay_fees() {
assert_eq!(asset_list(RECIPIENT), []);
}
+#[test]
+fn putting_more_than_available_fails() {
+ // Make sure the sender has enough funds to withdraw.
+ add_asset(SENDER, (Here, 100u128));
+
+ // Build xcm.
+ let xcm = Xcm::<TestCall>::builder()
+ .withdraw_asset((Here, 100u128))
+ .pay_fees((Here, 200u128)) // 200% destined for fees, there's not even that much!
+ .deposit_asset(All, RECIPIENT)
+ .build();
+
+ let (mut vm, weight) = instantiate_executor(SENDER, xcm.clone());
+
+ // Program fails.
+ assert!(vm.bench_process(xcm).is_err());
+
+ // Everything is left in the `holding` register.
+ assert_eq!(get_first_fungible(vm.holding()).unwrap(), (Here, 100u128).into());
+ // Nothing in the `fees` register.
+ assert_eq!(get_first_fungible(vm.fees()), None);
+
+ // The recipient received no assets since message failed.
+ assert_eq!(asset_list(RECIPIENT), []);
+
+ // Leftover assets get trapped.
+ assert!(vm.bench_post_process(weight).ensure_complete().is_ok());
+ assert_eq!(asset_list(TRAPPED_ASSETS), [(Here, 100u128).into()]);
+}
+
#[test]
fn refunding_too_early() {
// Make sure the sender has enough funds to withdraw.
@@ -255,3 +286,58 @@ fn refunding_too_early() {
// No messages were "sent".
assert_eq!(sent_xcm(), Vec::new());
}
+
+#[test]
+fn pay_fees_is_processed_only_once() {
+ // Make sure the sender has enough funds to withdraw.
+ add_asset(SENDER, (Here, 100u128));
+
+ let xcm = Xcm::<TestCall>::builder()
+ .withdraw_asset((Here, 100u128))
+ .pay_fees((Here, 10u128))
+ // Will both be a noop.
+ .pay_fees((Parent, 10u128))
+ .pay_fees((Here, 200u128))
+ .deposit_asset(All, RECIPIENT)
+ .build();
+
+ let (mut vm, weight) = instantiate_executor(SENDER, xcm.clone());
+
+ // Program runs successfully.
+ assert!(vm.bench_process(xcm).is_ok());
+
+ // Nothing left in the `holding` register.
+ assert_eq!(get_first_fungible(vm.holding()), None);
+ // Only the first `PayFees` was executed and execution fees are 4,
+ // so there are 6 fees left in the `fees` register.
+ assert_eq!(get_first_fungible(vm.fees()).unwrap(), (Here, 6u128).into());
+
+ // Leftover fees get trapped.
+ assert!(vm.bench_post_process(weight).ensure_complete().is_ok());
+ assert_eq!(asset_list(TRAPPED_ASSETS), [(Here, 6u128).into()]);
+}
+
+#[test]
+fn already_paid_fees_rolls_back_on_error() {
+ // Make sure the sender has enough funds to withdraw.
+ add_asset(SENDER, (Here, 100u128));
+
+ let xcm = Xcm::<TestCall>::builder()
+ .withdraw_asset((Here, 100u128))
+ .pay_fees((Here, 200u128))
+ .deposit_asset(All, RECIPIENT)
+ .build();
+
+ let (mut vm, _) = instantiate_executor(SENDER, xcm.clone());
+
+ // Program fails.
+ assert!(vm.bench_process(xcm).is_err());
+
+ // Everything left in the `holding` register.
+ assert_eq!(get_first_fungible(vm.holding()).unwrap(), (Here, 100u128).into());
+ // Nothing in the `fees` register.
+ assert_eq!(get_first_fungible(vm.fees()), None);
+
+ // Already paid fees is false.
+ assert_eq!(vm.already_paid_fees(), false);
+}
diff --git a/prdoc/pr_7641.prdoc b/prdoc/pr_7641.prdoc
new file mode 100644
index 0000000000000000000000000000000000000000..2c0f4fb6441fe016b1f8c0ea6bee5e85e60d059e
--- /dev/null
+++ b/prdoc/pr_7641.prdoc
@@ -0,0 +1,10 @@
+title: 'XCM: Process PayFees only once'
+doc:
+- audience: Runtime Dev
+ description: "The `PayFees` instruction should only ever be used once. If it's used\
+ \ more than once, it's just a noop.\r\n"
+crates:
+- name: staging-xcm-executor
+ bump: patch
+- name: staging-xcm
+ bump: patch