From a783e822ed509b9b54977d06f2beed54885925f0 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 9 Dec 2024 21:58:10 +0200 Subject: [PATCH] xcm-executor: take transport fee from transferred assets if necessary (#4834) # Description Sending XCM messages to other chains requires paying a "transport fee". This can be paid either: - from `origin` local account if `jit_withdraw = true`, - taken from Holding register otherwise. This currently works for following hops/scenarios: 1. On destination no transport fee needed (only sending costs, not receiving), 2. Local/originating chain: just set JIT=true and fee will be paid from signed account, 3. Intermediary hops - only if intermediary is acting as reserve between two untrusted chains (aka only for `DepositReserveAsset` instruction) - this was fixed in https://github.com/paritytech/polkadot-sdk/pull/3142 But now we're seeing more complex asset transfers that are mixing reserve transfers with teleports depending on the involved chains. # Example E.g. transferring DOT between Relay and parachain, but through AH (using AH instead of the Relay chain as parachain's DOT reserve). In the `Parachain --1--> AssetHub --2--> Relay` scenario, DOT has to be reserve-withdrawn in leg `1`, then teleported in leg `2`. On the intermediary hop (AssetHub), `InitiateTeleport` fails to send onward message because of missing transport fees. We also can't rely on `jit_withdraw` because the original origin is lost on the way, and even if it weren't we can't rely on the user having funded accounts on each hop along the way. # Solution/Changes - Charge the transport fee in the executor from the transferred assets (if available), - Only charge from transferred assets if JIT_WITHDRAW was not set, - Only charge from transferred assets if unless using XCMv5 `PayFees` where we do not have this problem. # Testing Added regression tests in emulated transfers. Fixes https://github.com/paritytech/polkadot-sdk/issues/4832 Fixes https://github.com/paritytech/polkadot-sdk/issues/6637 --------- Signed-off-by: Adrian Catangiu Co-authored-by: Francisco Aguirre --- .../primitives/router/src/inbound/mod.rs | 4 +- .../src/tests/reserve_transfer.rs | 6 +- .../tests/assets/asset-hub-westend/src/lib.rs | 1 + .../src/tests/hybrid_transfers.rs | 141 +++++++++++++++++- .../src/tests/reserve_transfer.rs | 6 +- .../asset-hub-westend/src/tests/transact.rs | 2 +- .../src/tests/asset_transfers.rs | 2 +- .../src/tests/register_bridged_assets.rs | 2 +- .../bridge-hub-rococo/src/tests/send_xcm.rs | 2 +- .../src/tests/asset_transfers.rs | 4 +- .../src/tests/register_bridged_assets.rs | 2 +- .../bridge-hub-westend/src/tests/send_xcm.rs | 2 +- .../bridge-hub-westend/src/tests/transact.rs | 2 +- polkadot/xcm/xcm-executor/src/lib.rs | 83 ++++++++--- prdoc/pr_4834.prdoc | 15 ++ 15 files changed, 230 insertions(+), 44 deletions(-) create mode 100644 prdoc/pr_4834.prdoc diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index 54a578b988a4..a7bd0dfc1234 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -358,7 +358,9 @@ where }])), // Perform a deposit reserve to send to destination chain. DepositReserveAsset { - assets: Definite(vec![dest_para_fee_asset.clone(), asset].into()), + // Send over assets and unspent fees, XCM delivery fee will be charged from + // here. + assets: Wild(AllCounted(2)), dest: Location::new(1, [Parachain(dest_para_id)]), xcm: vec![ // Buy execution on target. diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs index 698ef2c9e792..d642e877f002 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs @@ -115,7 +115,7 @@ pub fn system_para_to_para_sender_assertions(t: SystemParaToParaTest) { assert_expected_events!( AssetHubRococo, vec![ - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm(pallet_xcm::Event::FeesPaid { .. }) => {}, ] ); @@ -274,7 +274,7 @@ fn system_para_to_para_assets_sender_assertions(t: SystemParaToParaTest) { t.args.dest.clone() ), }, - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm( pallet_xcm::Event::FeesPaid { .. } ) => {}, @@ -305,7 +305,7 @@ fn para_to_system_para_assets_sender_assertions(t: ParaToSystemParaTest) { owner: *owner == t.sender.account_id, balance: *balance == t.args.amount, }, - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm( pallet_xcm::Event::FeesPaid { .. } ) => {}, diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs index 3cca99fbfe5c..36630e2d2221 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs @@ -106,6 +106,7 @@ mod imports { pub type ParaToParaThroughRelayTest = Test; pub type ParaToParaThroughAHTest = Test; pub type RelayToParaThroughAHTest = Test; + pub type PenpalToRelayThroughAHTest = Test; } #[cfg(test)] diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs index a0fc82fba6ef..0686bd71d085 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs @@ -658,13 +658,13 @@ fn bidirectional_teleport_foreign_asset_between_para_and_asset_hub_using_explici } // =============================================================== -// ===== Transfer - Native Asset - Relay->AssetHub->Parachain ==== +// ====== Transfer - Native Asset - Relay->AssetHub->Penpal ====== // =============================================================== -/// Transfers of native asset Relay to Parachain (using AssetHub reserve). Parachains want to avoid +/// Transfers of native asset Relay to Penpal (using AssetHub reserve). Parachains want to avoid /// managing SAs on all system chains, thus want all their DOT-in-reserve to be held in their /// Sovereign Account on Asset Hub. #[test] -fn transfer_native_asset_from_relay_to_para_through_asset_hub() { +fn transfer_native_asset_from_relay_to_penpal_through_asset_hub() { // Init values for Relay let destination = Westend::child_location_of(PenpalA::para_id()); let sender = WestendSender::get(); @@ -820,6 +820,137 @@ fn transfer_native_asset_from_relay_to_para_through_asset_hub() { assert!(receiver_assets_after < receiver_assets_before + amount_to_send); } +// =============================================================== +// ===== Transfer - Native Asset - Penpal->AssetHub->Relay ======= +// =============================================================== +/// Transfers of native asset Penpal to Relay (using AssetHub reserve). Parachains want to avoid +/// managing SAs on all system chains, thus want all their DOT-in-reserve to be held in their +/// Sovereign Account on Asset Hub. +#[test] +fn transfer_native_asset_from_penpal_to_relay_through_asset_hub() { + // Init values for Penpal + let destination = RelayLocation::get(); + let sender = PenpalASender::get(); + let amount_to_send: Balance = WESTEND_ED * 100; + + // Init values for Penpal + let relay_native_asset_location = RelayLocation::get(); + let receiver = WestendReceiver::get(); + + // Init Test + let test_args = TestContext { + sender: sender.clone(), + receiver: receiver.clone(), + args: TestArgs::new_para( + destination.clone(), + receiver.clone(), + amount_to_send, + (Parent, amount_to_send).into(), + None, + 0, + ), + }; + let mut test = PenpalToRelayThroughAHTest::new(test_args); + + let sov_penpal_on_ah = AssetHubWestend::sovereign_account_id_of( + AssetHubWestend::sibling_location_of(PenpalA::para_id()), + ); + // fund Penpal's sender account + PenpalA::mint_foreign_asset( + ::RuntimeOrigin::signed(PenpalAssetOwner::get()), + relay_native_asset_location.clone(), + sender.clone(), + amount_to_send * 2, + ); + // fund Penpal's SA on AssetHub with the assets held in reserve + AssetHubWestend::fund_accounts(vec![(sov_penpal_on_ah.clone().into(), amount_to_send * 2)]); + + // prefund Relay checking account so we accept teleport "back" from AssetHub + let check_account = + Westend::execute_with(|| ::XcmPallet::check_account()); + Westend::fund_accounts(vec![(check_account, amount_to_send)]); + + // Query initial balances + let sender_balance_before = PenpalA::execute_with(|| { + type ForeignAssets = ::ForeignAssets; + >::balance(relay_native_asset_location.clone(), &sender) + }); + let sov_penpal_on_ah_before = AssetHubWestend::execute_with(|| { + ::Balances::free_balance(sov_penpal_on_ah.clone()) + }); + let receiver_balance_before = Westend::execute_with(|| { + ::Balances::free_balance(receiver.clone()) + }); + + fn transfer_assets_dispatchable(t: PenpalToRelayThroughAHTest) -> DispatchResult { + let fee_idx = t.args.fee_asset_item as usize; + let fee: Asset = t.args.assets.inner().get(fee_idx).cloned().unwrap(); + let asset_hub_location = PenpalA::sibling_location_of(AssetHubWestend::para_id()); + let context = PenpalUniversalLocation::get(); + + // reanchor fees to the view of destination (Westend Relay) + let mut remote_fees = fee.clone().reanchored(&t.args.dest, &context).unwrap(); + if let Fungible(ref mut amount) = remote_fees.fun { + // we already spent some fees along the way, just use half of what we started with + *amount = *amount / 2; + } + let xcm_on_final_dest = Xcm::<()>(vec![ + BuyExecution { fees: remote_fees, weight_limit: t.args.weight_limit.clone() }, + DepositAsset { + assets: Wild(AllCounted(t.args.assets.len() as u32)), + beneficiary: t.args.beneficiary, + }, + ]); + + // reanchor final dest (Westend Relay) to the view of hop (Asset Hub) + let mut dest = t.args.dest.clone(); + dest.reanchor(&asset_hub_location, &context).unwrap(); + // on Asset Hub + let xcm_on_hop = Xcm::<()>(vec![InitiateTeleport { + assets: Wild(AllCounted(t.args.assets.len() as u32)), + dest, + xcm: xcm_on_final_dest, + }]); + + // First leg is a reserve-withdraw, from there a teleport to final dest + ::PolkadotXcm::transfer_assets_using_type_and_then( + t.signed_origin, + bx!(asset_hub_location.into()), + bx!(t.args.assets.into()), + bx!(TransferType::DestinationReserve), + bx!(fee.id.into()), + bx!(TransferType::DestinationReserve), + bx!(VersionedXcm::from(xcm_on_hop)), + t.args.weight_limit, + ) + } + test.set_dispatchable::(transfer_assets_dispatchable); + test.assert(); + + // Query final balances + let sender_balance_after = PenpalA::execute_with(|| { + type ForeignAssets = ::ForeignAssets; + >::balance(relay_native_asset_location.clone(), &sender) + }); + let sov_penpal_on_ah_after = AssetHubWestend::execute_with(|| { + ::Balances::free_balance(sov_penpal_on_ah.clone()) + }); + let receiver_balance_after = Westend::execute_with(|| { + ::Balances::free_balance(receiver.clone()) + }); + + // Sender's asset balance is reduced by amount sent plus delivery fees + assert!(sender_balance_after < sender_balance_before - amount_to_send); + // SA on AH balance is decreased by `amount_to_send` + assert_eq!(sov_penpal_on_ah_after, sov_penpal_on_ah_before - amount_to_send); + // Receiver's balance is increased + assert!(receiver_balance_after > receiver_balance_before); + // Receiver's balance increased by `amount_to_send - delivery_fees - bought_execution`; + // `delivery_fees` might be paid from transfer or JIT, also `bought_execution` is unknown but + // should be non-zero + assert!(receiver_balance_after < receiver_balance_before + amount_to_send); +} + // ============================================================================================== // ==== Bidirectional Transfer - Native + Teleportable Foreign Assets - Parachain<->AssetHub ==== // ============================================================================================== @@ -839,7 +970,7 @@ fn bidirectional_transfer_multiple_assets_between_penpal_and_asset_hub() { // xcm to be executed at dest let xcm_on_dest = Xcm(vec![ // since this is the last hop, we don't need to further use any assets previously - // reserved for fees (there are no further hops to cover transport fees for); we + // reserved for fees (there are no further hops to cover delivery fees for); we // RefundSurplus to get back any unspent fees RefundSurplus, DepositAsset { assets: Wild(All), beneficiary: t.args.beneficiary }, @@ -875,7 +1006,7 @@ fn bidirectional_transfer_multiple_assets_between_penpal_and_asset_hub() { // xcm to be executed at dest let xcm_on_dest = Xcm(vec![ // since this is the last hop, we don't need to further use any assets previously - // reserved for fees (there are no further hops to cover transport fees for); we + // reserved for fees (there are no further hops to cover delivery fees for); we // RefundSurplus to get back any unspent fees RefundSurplus, DepositAsset { assets: Wild(All), beneficiary: t.args.beneficiary }, diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs index 558eab13e5c7..707e8adc8a56 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/reserve_transfer.rs @@ -115,7 +115,7 @@ pub fn system_para_to_para_sender_assertions(t: SystemParaToParaTest) { assert_expected_events!( AssetHubWestend, vec![ - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm(pallet_xcm::Event::FeesPaid { .. }) => {}, ] ); @@ -274,7 +274,7 @@ fn system_para_to_para_assets_sender_assertions(t: SystemParaToParaTest) { t.args.dest.clone() ), }, - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm( pallet_xcm::Event::FeesPaid { .. } ) => {}, @@ -305,7 +305,7 @@ fn para_to_system_para_assets_sender_assertions(t: ParaToSystemParaTest) { owner: *owner == t.sender.account_id, balance: *balance == t.args.amount, }, - // Transport fees are paid + // Delivery fees are paid RuntimeEvent::PolkadotXcm( pallet_xcm::Event::FeesPaid { .. } ) => {}, diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/transact.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/transact.rs index 592c2845255c..7e881a332a53 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/transact.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/transact.rs @@ -46,7 +46,7 @@ fn transfer_and_transact_in_same_xcm( Transact { origin_kind: OriginKind::Xcm, call, fallback_max_weight: None }, ExpectTransactStatus(MaybeErrorCode::Success), // since this is the last hop, we don't need to further use any assets previously - // reserved for fees (there are no further hops to cover transport fees for); we + // reserved for fees (there are no further hops to cover delivery fees for); we // RefundSurplus to get back any unspent fees RefundSurplus, DepositAsset { assets: Wild(All), beneficiary }, diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs index 33ab1e70b97b..a2a61660afff 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/asset_transfers.rs @@ -16,7 +16,7 @@ use crate::tests::*; fn send_assets_over_bridge(send_fn: F) { - // fund the AHR's SA on BHR for paying bridge transport fees + // fund the AHR's SA on BHR for paying bridge delivery fees BridgeHubRococo::fund_para_sovereign(AssetHubRococo::para_id(), 10_000_000_000_000u128); // set XCM versions diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/register_bridged_assets.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/register_bridged_assets.rs index 1ae3a1b15805..70e7a7a3ddd3 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/register_bridged_assets.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/register_bridged_assets.rs @@ -58,7 +58,7 @@ fn register_rococo_asset_on_wah_from_rah() { let destination = asset_hub_westend_location(); - // fund the RAH's SA on RBH for paying bridge transport fees + // fund the RAH's SA on RBH for paying bridge delivery fees BridgeHubRococo::fund_para_sovereign(AssetHubRococo::para_id(), 10_000_000_000_000u128); // set XCM versions diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/send_xcm.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/send_xcm.rs index 931a3128f826..116ec4dc0e55 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/send_xcm.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/send_xcm.rs @@ -65,7 +65,7 @@ fn send_xcm_through_opened_lane_with_different_xcm_version_on_hops_works() { let native_token = Location::parent(); let amount = ASSET_HUB_ROCOCO_ED * 1_000; - // fund the AHR's SA on BHR for paying bridge transport fees + // fund the AHR's SA on BHR for paying bridge delivery fees BridgeHubRococo::fund_para_sovereign(AssetHubRococo::para_id(), 10_000_000_000_000u128); // fund sender AssetHubRococo::fund_accounts(vec![(AssetHubRococoSender::get().into(), amount * 10)]); diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs index ab09517339db..cc90c10b54bc 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/asset_transfers.rs @@ -17,7 +17,7 @@ use crate::{create_pool_with_native_on, tests::*}; use xcm::latest::AssetTransferFilter; fn send_assets_over_bridge(send_fn: F) { - // fund the AHW's SA on BHW for paying bridge transport fees + // fund the AHW's SA on BHW for paying bridge delivery fees BridgeHubWestend::fund_para_sovereign(AssetHubWestend::para_id(), 10_000_000_000_000u128); // set XCM versions @@ -592,7 +592,7 @@ fn do_send_pens_and_wnds_from_penpal_westend_via_ahw_to_asset_hub_rococo( // XCM to be executed at dest (Rococo Asset Hub) let xcm_on_dest = Xcm(vec![ // since this is the last hop, we don't need to further use any assets previously - // reserved for fees (there are no further hops to cover transport fees for); we + // reserved for fees (there are no further hops to cover delivery fees for); we // RefundSurplus to get back any unspent fees RefundSurplus, // deposit everything to final beneficiary diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/register_bridged_assets.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/register_bridged_assets.rs index 424f1e55956b..952fc35e6703 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/register_bridged_assets.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/register_bridged_assets.rs @@ -82,7 +82,7 @@ fn register_asset_on_rah_from_wah(bridged_asset_at_rah: Location) { let destination = asset_hub_rococo_location(); - // fund the WAH's SA on WBH for paying bridge transport fees + // fund the WAH's SA on WBH for paying bridge delivery fees BridgeHubWestend::fund_para_sovereign(AssetHubWestend::para_id(), 10_000_000_000_000u128); // set XCM versions diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/send_xcm.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/send_xcm.rs index 787d7dc842cb..acce60b4fa76 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/send_xcm.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/send_xcm.rs @@ -65,7 +65,7 @@ fn send_xcm_through_opened_lane_with_different_xcm_version_on_hops_works() { let native_token = Location::parent(); let amount = ASSET_HUB_WESTEND_ED * 1_000; - // fund the AHR's SA on BHR for paying bridge transport fees + // fund the AHR's SA on BHR for paying bridge delivery fees BridgeHubWestend::fund_para_sovereign(AssetHubWestend::para_id(), 10_000_000_000_000u128); // fund sender AssetHubWestend::fund_accounts(vec![(AssetHubWestendSender::get().into(), amount * 10)]); diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/transact.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/transact.rs index 7831c8d66357..f6a3c53c4bf5 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/transact.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-westend/src/tests/transact.rs @@ -52,7 +52,7 @@ fn transfer_and_transact_in_same_xcm( Transact { origin_kind: OriginKind::Xcm, call, fallback_max_weight: None }, ExpectTransactStatus(MaybeErrorCode::Success), // since this is the last hop, we don't need to further use any assets previously - // reserved for fees (there are no further hops to cover transport fees for); we + // reserved for fees (there are no further hops to cover delivery fees for); we // RefundSurplus to get back any unspent fees RefundSurplus, DepositAsset { assets: Wild(All), beneficiary }, diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 64a32f488420..d0f18aea1ab3 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1087,18 +1087,19 @@ impl XcmExecutor { DepositReserveAsset { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { - let maybe_delivery_fee_from_holding = if self.fees.is_empty() { - self.get_delivery_fee_from_holding(&assets, &dest, &xcm)? + let mut assets = self.holding.saturating_take(assets); + // When not using `PayFees`, nor `JIT_WITHDRAW`, delivery fees are paid from + // transferred assets. + let maybe_delivery_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + // Deduct and return the part of `assets` that shall be used for delivery fees. + self.take_delivery_fee_from_assets(&mut assets, &dest, FeeReason::DepositReserveAsset, &xcm)? } else { None }; - let mut message = Vec::with_capacity(xcm.len() + 2); - // now take assets to deposit (after having taken delivery fees) - let deposited = self.holding.saturating_take(assets); - tracing::trace!(target: "xcm::DepositReserveAsset", ?deposited, "Assets except delivery fee"); + tracing::trace!(target: "xcm::DepositReserveAsset", ?assets, "Assets except delivery fee"); Self::do_reserve_deposit_assets( - deposited, + assets, &dest, &mut message, Some(&self.context), @@ -1107,7 +1108,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_delivery_fee_from_holding { + if let Some(delivery_fee) = maybe_delivery_fee_from_assets { // Put back delivery_fee in holding register to be charged by XcmSender. self.holding.subsume_assets(delivery_fee); } @@ -1122,7 +1123,15 @@ impl XcmExecutor { InitiateReserveWithdraw { assets, reserve, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { - let assets = self.holding.saturating_take(assets); + let mut assets = self.holding.saturating_take(assets); + // When not using `PayFees`, nor `JIT_WITHDRAW`, delivery fees are paid from + // transferred assets. + let maybe_delivery_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + // Deduct and return the part of `assets` that shall be used for delivery fees. + self.take_delivery_fee_from_assets(&mut assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)? + } else { + None + }; let mut message = Vec::with_capacity(xcm.len() + 2); Self::do_reserve_withdraw_assets( assets, @@ -1134,6 +1143,10 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); + if let Some(delivery_fee) = maybe_delivery_fee_from_assets { + // Put back delivery_fee in holding register to be charged by XcmSender. + self.holding.subsume_assets(delivery_fee); + } self.send(reserve, Xcm(message), FeeReason::InitiateReserveWithdraw)?; Ok(()) }); @@ -1145,13 +1158,25 @@ impl XcmExecutor { InitiateTeleport { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { - let assets = self.holding.saturating_take(assets); + let mut assets = self.holding.saturating_take(assets); + // When not using `PayFees`, nor `JIT_WITHDRAW`, delivery fees are paid from + // transferred assets. + let maybe_delivery_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + // Deduct and return the part of `assets` that shall be used for delivery fees. + self.take_delivery_fee_from_assets(&mut assets, &dest, FeeReason::InitiateTeleport, &xcm)? + } else { + None + }; let mut message = Vec::with_capacity(xcm.len() + 2); Self::do_teleport_assets(assets, &dest, &mut message, &self.context)?; // clear origin for subsequent custom instructions message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); + if let Some(delivery_fee) = maybe_delivery_fee_from_assets { + // Put back delivery_fee in holding register to be charged by XcmSender. + self.holding.subsume_assets(delivery_fee); + } self.send(dest.clone(), Xcm(message), FeeReason::InitiateTeleport)?; Ok(()) }); @@ -1714,36 +1739,48 @@ impl XcmExecutor { Ok(()) } - /// Gets the necessary delivery fee to send a reserve transfer message to `destination` from - /// holding. + /// Take from transferred `assets` the delivery fee required to send an onward transfer message + /// to `destination`. /// /// Will be removed once the transition from `BuyExecution` to `PayFees` is complete. - fn get_delivery_fee_from_holding( - &mut self, - assets: &AssetFilter, + fn take_delivery_fee_from_assets( + &self, + assets: &mut AssetsInHolding, destination: &Location, + reason: FeeReason, xcm: &Xcm<()>, ) -> Result, XcmError> { - // we need to do this take/put cycle to solve wildcards and get exact assets to - // be weighed - let to_weigh = self.holding.saturating_take(assets.clone()); - self.holding.subsume_assets(to_weigh.clone()); + let to_weigh = assets.clone(); let to_weigh_reanchored = Self::reanchored(to_weigh, &destination, None); - let mut message_to_weigh = vec![ReserveAssetDeposited(to_weigh_reanchored), ClearOrigin]; + let remote_instruction = match reason { + FeeReason::DepositReserveAsset => ReserveAssetDeposited(to_weigh_reanchored), + FeeReason::InitiateReserveWithdraw => WithdrawAsset(to_weigh_reanchored), + FeeReason::InitiateTeleport => ReceiveTeleportedAsset(to_weigh_reanchored), + _ => { + tracing::debug!( + target: "xcm::take_delivery_fee_from_assets", + "Unexpected delivery fee reason", + ); + return Err(XcmError::NotHoldingFees); + }, + }; + let mut message_to_weigh = Vec::with_capacity(xcm.len() + 2); + message_to_weigh.push(remote_instruction); + message_to_weigh.push(ClearOrigin); message_to_weigh.extend(xcm.0.clone().into_iter()); let (_, fee) = validate_send::(destination.clone(), Xcm(message_to_weigh))?; let maybe_delivery_fee = fee.get(0).map(|asset_needed_for_fees| { tracing::trace!( - target: "xcm::fees::DepositReserveAsset", + target: "xcm::fees::take_delivery_fee_from_assets", "Asset provided to pay for fees {:?}, asset required for delivery fees: {:?}", self.asset_used_in_buy_execution, asset_needed_for_fees, ); let asset_to_pay_for_fees = self.calculate_asset_for_delivery_fees(asset_needed_for_fees.clone()); // set aside fee to be charged by XcmSender - let delivery_fee = self.holding.saturating_take(asset_to_pay_for_fees.into()); - tracing::trace!(target: "xcm::fees::DepositReserveAsset", ?delivery_fee); + let delivery_fee = assets.saturating_take(asset_to_pay_for_fees.into()); + tracing::trace!(target: "xcm::fees::take_delivery_fee_from_assets", ?delivery_fee); delivery_fee }); Ok(maybe_delivery_fee) diff --git a/prdoc/pr_4834.prdoc b/prdoc/pr_4834.prdoc new file mode 100644 index 000000000000..b7c8b15cb073 --- /dev/null +++ b/prdoc/pr_4834.prdoc @@ -0,0 +1,15 @@ +title: "xcm-executor: take delivery fee from transferred assets if necessary" + +doc: + - audience: Runtime Dev + description: | + In asset transfers, as a last resort, XCM delivery fees are taken from + transferred assets rather than failing the transfer. + +crates: + - name: staging-xcm-executor + bump: patch + - name: snowbridge-router-primitives + bump: patch + - name: snowbridge-pallet-inbound-queue + bump: patch