From 2ea6aba8818cbbaac520753e585bcbc99fe03918 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 2 Dec 2024 16:03:28 +0200 Subject: [PATCH 1/6] 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. 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. - 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 Holding doesn't already contain enough (other) assets to pay for the transport fee. Added regression tests in emulated transfers. Fixes https://github.com/paritytech/polkadot-sdk/issues/4832 Signed-off-by: Adrian Catangiu --- .../tests/assets/asset-hub-westend/src/lib.rs | 1 + .../src/tests/hybrid_transfers.rs | 137 +++++++++++++++++- polkadot/xcm/xcm-executor/src/lib.rs | 56 +++++-- prdoc/pr_4834.prdoc | 11 ++ 4 files changed, 193 insertions(+), 12 deletions(-) create mode 100644 prdoc/pr_4834.prdoc 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..35af8e58f60a 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 ==== // ============================================================================================== diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 4e051f24050c..cab641a7b0fb 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1086,12 +1086,13 @@ 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)? + // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // transferred assets. + let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + self.get_delivery_fee_from_holding(&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); @@ -1106,7 +1107,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_transport_fee_from_holding { // Put back delivery_fee in holding register to be charged by XcmSender. self.holding.subsume_assets(delivery_fee); } @@ -1121,6 +1122,13 @@ impl XcmExecutor { InitiateReserveWithdraw { assets, reserve, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // transferred assets. + let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + self.get_delivery_fee_from_holding(&assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)? + } else { + None + }; let assets = self.holding.saturating_take(assets); let mut message = Vec::with_capacity(xcm.len() + 2); Self::do_reserve_withdraw_assets( @@ -1133,6 +1141,10 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); + if let Some(delivery_fee) = maybe_transport_fee_from_holding { + // 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(()) }); @@ -1144,6 +1156,13 @@ impl XcmExecutor { InitiateTeleport { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // transferred assets. + let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + self.get_delivery_fee_from_holding(&assets, &dest, FeeReason::InitiateTeleport, &xcm)? + } else { + None + }; let assets = self.holding.saturating_take(assets); let mut message = Vec::with_capacity(xcm.len() + 2); Self::do_teleport_assets(assets, &dest, &mut message, &self.context)?; @@ -1151,6 +1170,10 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); + if let Some(delivery_fee) = maybe_transport_fee_from_holding { + // 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(()) }); @@ -1707,14 +1730,15 @@ impl XcmExecutor { Ok(()) } - /// Gets the necessary delivery fee to send a reserve transfer message to `destination` from - /// holding. + /// Gets the necessary delivery fee from holding 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, destination: &Location, + reason: FeeReason, xcm: &Xcm<()>, ) -> Result, XcmError> { // we need to do this take/put cycle to solve wildcards and get exact assets to @@ -1722,13 +1746,27 @@ impl XcmExecutor { let to_weigh = self.holding.saturating_take(assets.clone()); self.holding.subsume_assets(to_weigh.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::get_delivery_fee_from_holding", + "Unexpected transport 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::get_delivery_fee_from_holding", "Asset provided to pay for fees {:?}, asset required for delivery fees: {:?}", self.asset_used_in_buy_execution, asset_needed_for_fees, ); @@ -1736,7 +1774,7 @@ impl XcmExecutor { 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); + tracing::trace!(target: "xcm::fees::get_delivery_fee_from_holding", ?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..248070d26392 --- /dev/null +++ b/prdoc/pr_4834.prdoc @@ -0,0 +1,11 @@ +title: "xcm-executor: take transport fee from transferred assets if necessary" + +doc: + - audience: Runtime Dev + description: | + In asset transfers, as a last resort, XCM transport fees are taken from + transferred assets rather than failing the transfer. + +crates: + - name: staging-xcm-executor + bump: patch From 275295f4622509927c99fd6bdfdff537cf2103c2 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 4 Dec 2024 16:36:07 +0200 Subject: [PATCH 2/6] accurately account for transport fee --- polkadot/xcm/xcm-executor/src/lib.rs | 55 ++++++++++++++-------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index cab641a7b0fb..112394902edc 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1086,19 +1086,19 @@ impl XcmExecutor { DepositReserveAsset { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { - self.get_delivery_fee_from_holding(&assets, &dest, FeeReason::DepositReserveAsset, &xcm)? + let maybe_transport_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 transport 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 +1107,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_transport_fee_from_holding { + if let Some(delivery_fee) = maybe_transport_fee_from_assets { // Put back delivery_fee in holding register to be charged by XcmSender. self.holding.subsume_assets(delivery_fee); } @@ -1122,14 +1122,15 @@ impl XcmExecutor { InitiateReserveWithdraw { assets, reserve, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { - self.get_delivery_fee_from_holding(&assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)? + let maybe_transport_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 transport fees. + self.take_delivery_fee_from_assets(&mut assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)? } else { None }; - let assets = self.holding.saturating_take(assets); let mut message = Vec::with_capacity(xcm.len() + 2); Self::do_reserve_withdraw_assets( assets, @@ -1141,7 +1142,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_transport_fee_from_holding { + if let Some(delivery_fee) = maybe_transport_fee_from_assets { // Put back delivery_fee in holding register to be charged by XcmSender. self.holding.subsume_assets(delivery_fee); } @@ -1156,21 +1157,22 @@ impl XcmExecutor { InitiateTeleport { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_holding = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { - self.get_delivery_fee_from_holding(&assets, &dest, FeeReason::InitiateTeleport, &xcm)? + let maybe_transport_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 transport fees. + self.take_delivery_fee_from_assets(&mut assets, &dest, FeeReason::InitiateTeleport, &xcm)? } else { None }; - let assets = self.holding.saturating_take(assets); 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_transport_fee_from_holding { + if let Some(delivery_fee) = maybe_transport_fee_from_assets { // Put back delivery_fee in holding register to be charged by XcmSender. self.holding.subsume_assets(delivery_fee); } @@ -1730,21 +1732,18 @@ impl XcmExecutor { Ok(()) } - /// Gets the necessary delivery fee from holding to send an onward transfer message to - /// `destination`. + /// 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 remote_instruction = match reason { FeeReason::DepositReserveAsset => ReserveAssetDeposited(to_weigh_reanchored), @@ -1752,7 +1751,7 @@ impl XcmExecutor { FeeReason::InitiateTeleport => ReceiveTeleportedAsset(to_weigh_reanchored), _ => { tracing::debug!( - target: "xcm::get_delivery_fee_from_holding", + target: "xcm::take_delivery_fee_from_assets", "Unexpected transport fee reason", ); return Err(XcmError::NotHoldingFees); @@ -1766,15 +1765,15 @@ impl XcmExecutor { 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::get_delivery_fee_from_holding", + 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::get_delivery_fee_from_holding", ?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) From e8e431af9295567b94fc243fda0ec2f19639d01c Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 4 Dec 2024 17:03:36 +0200 Subject: [PATCH 3/6] Apply suggestions from code review Co-authored-by: Francisco Aguirre --- polkadot/xcm/xcm-executor/src/lib.rs | 12 ++++++------ prdoc/pr_4834.prdoc | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 112394902edc..2bca19b13231 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1089,7 +1089,7 @@ impl XcmExecutor { let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + 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 transport fees. self.take_delivery_fee_from_assets(&mut assets, &dest, FeeReason::DepositReserveAsset, &xcm)? } else { @@ -1107,7 +1107,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_transport_fee_from_assets { + 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); } @@ -1125,7 +1125,7 @@ impl XcmExecutor { let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + 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 transport fees. self.take_delivery_fee_from_assets(&mut assets, &reserve, FeeReason::InitiateReserveWithdraw, &xcm)? } else { @@ -1142,7 +1142,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_transport_fee_from_assets { + 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); } @@ -1160,7 +1160,7 @@ impl XcmExecutor { let mut assets = self.holding.saturating_take(assets); // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from // transferred assets. - let maybe_transport_fee_from_assets = if self.fees.is_empty() && !self.fees_mode.jit_withdraw { + 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 transport fees. self.take_delivery_fee_from_assets(&mut assets, &dest, FeeReason::InitiateTeleport, &xcm)? } else { @@ -1172,7 +1172,7 @@ impl XcmExecutor { message.push(ClearOrigin); // append custom instructions message.extend(xcm.0.into_iter()); - if let Some(delivery_fee) = maybe_transport_fee_from_assets { + 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); } diff --git a/prdoc/pr_4834.prdoc b/prdoc/pr_4834.prdoc index 248070d26392..b6c10fd8b295 100644 --- a/prdoc/pr_4834.prdoc +++ b/prdoc/pr_4834.prdoc @@ -1,9 +1,9 @@ -title: "xcm-executor: take transport fee from transferred assets if necessary" +title: "xcm-executor: take delivery fee from transferred assets if necessary" doc: - audience: Runtime Dev description: | - In asset transfers, as a last resort, XCM transport fees are taken from + In asset transfers, as a last resort, XCM delivery fees are taken from transferred assets rather than failing the transfer. crates: From 7cf3cb87c10df4752915a14bdc6cf4a09e15730b Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Wed, 4 Dec 2024 17:07:26 +0200 Subject: [PATCH 4/6] use delivery fees instead of transport fees as term everywhere --- .../asset-hub-rococo/src/tests/reserve_transfer.rs | 6 +++--- .../src/tests/hybrid_transfers.rs | 4 ++-- .../src/tests/reserve_transfer.rs | 6 +++--- .../assets/asset-hub-westend/src/tests/transact.rs | 2 +- .../bridge-hub-rococo/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 | 14 +++++++------- 12 files changed, 24 insertions(+), 24 deletions(-) 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/tests/hybrid_transfers.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/hybrid_transfers.rs index 35af8e58f60a..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 @@ -970,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 }, @@ -1006,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 3c53cfb261be..a180cb85f780 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 }, 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 db42704dae61..0745cedcc44a 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 }, 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 2bca19b13231..802a03fa350f 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -1087,10 +1087,10 @@ impl XcmExecutor { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { let mut assets = self.holding.saturating_take(assets); - // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // 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 transport fees. + // 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 @@ -1123,10 +1123,10 @@ impl XcmExecutor { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { let mut assets = self.holding.saturating_take(assets); - // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // 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 transport fees. + // 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 @@ -1158,10 +1158,10 @@ impl XcmExecutor { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { let mut assets = self.holding.saturating_take(assets); - // When not using `PayFees`, nor `JIT_WITHDRAW`, transport fees are paid from + // 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 transport fees. + // 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 @@ -1752,7 +1752,7 @@ impl XcmExecutor { _ => { tracing::debug!( target: "xcm::take_delivery_fee_from_assets", - "Unexpected transport fee reason", + "Unexpected delivery fee reason", ); return Err(XcmError::NotHoldingFees); }, From 80f434de633c8b9c88a76cc401e60ae4a835d14f Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 5 Dec 2024 00:49:15 +0200 Subject: [PATCH 5/6] fix snowbridge transfer to parachain through asset hub --- bridges/snowbridge/primitives/router/src/inbound/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index e03560f66e24..3f3253b8eb6f 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -357,7 +357,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. From 41df153bcf95084f5d032e88e358c20a04904c1f Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 5 Dec 2024 00:57:21 +0200 Subject: [PATCH 6/6] fix prdoc --- prdoc/pr_4834.prdoc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/prdoc/pr_4834.prdoc b/prdoc/pr_4834.prdoc index b6c10fd8b295..b7c8b15cb073 100644 --- a/prdoc/pr_4834.prdoc +++ b/prdoc/pr_4834.prdoc @@ -9,3 +9,7 @@ doc: crates: - name: staging-xcm-executor bump: patch + - name: snowbridge-router-primitives + bump: patch + - name: snowbridge-pallet-inbound-queue + bump: patch