From 4d26f385876db9e8827949a8a87660516665ad78 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 30 Jan 2024 19:04:23 +0200 Subject: [PATCH] xcm-executor: DepositReserveAsset charges delivery fees from inner assets This fix aims to solve an issue in Kusama that resulted in failed reserve asset transfers. During multi-hop XCMs, like reserve asset transfers where the reserve is not the sender nor the destination, but a third remote chain, the origin is not available to pay for delivery fees out of their account directly, so delivery fees should be paid out of transferred assets. This commit also adds an xcm-emulator regression test that validates this scenario is now working. Signed-off-by: Adrian Catangiu --- Cargo.lock | 1 + .../tests/assets/asset-hub-rococo/Cargo.toml | 1 + .../tests/assets/asset-hub-rococo/src/lib.rs | 5 +- .../src/tests/reserve_transfer.rs | 122 ++++++++++++++++++ .../tests/assets/asset-hub-westend/src/lib.rs | 4 +- .../src/tests/reserve_transfer.rs | 122 ++++++++++++++++++ .../runtimes/testing/penpal/src/xcm_config.rs | 57 ++++---- polkadot/xcm/xcm-executor/src/lib.rs | 16 +++ 8 files changed, 294 insertions(+), 34 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e7e59ac1174..4613101b946c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -822,6 +822,7 @@ dependencies = [ "assert_matches", "asset-hub-rococo-runtime", "asset-test-utils", + "cumulus-pallet-parachain-system", "emulated-integration-tests-common", "frame-support", "pallet-asset-conversion", diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/Cargo.toml b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/Cargo.toml index 445395fc7830..beeab1b837ea 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/Cargo.toml +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/Cargo.toml @@ -31,6 +31,7 @@ rococo-runtime = { path = "../../../../../../../polkadot/runtime/rococo" } # Cumulus asset-test-utils = { path = "../../../../../runtimes/assets/test-utils" } parachains-common = { path = "../../../../../../parachains/common" } +cumulus-pallet-parachain-system = { path = "../../../../../../pallets/parachain-system", default-features = false } asset-hub-rococo-runtime = { path = "../../../../../runtimes/assets/asset-hub-rococo" } emulated-integration-tests-common = { path = "../../../common", default-features = false } rococo-system-emulated-network = { path = "../../../networks/rococo-system" } diff --git a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs index 155c952327aa..1cc25cb54a14 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs +++ b/cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs @@ -51,8 +51,8 @@ pub use rococo_system_emulated_network::{ AssetHubRococoParaSender as AssetHubRococoSender, BridgeHubRococoPara as BridgeHubRococo, BridgeHubRococoParaReceiver as BridgeHubRococoReceiver, PenpalAPara as PenpalA, PenpalAParaReceiver as PenpalAReceiver, PenpalAParaSender as PenpalASender, - RococoRelay as Rococo, RococoRelayReceiver as RococoReceiver, - RococoRelaySender as RococoSender, + PenpalBPara as PenpalB, PenpalBParaReceiver as PenpalBReceiver, RococoRelay as Rococo, + RococoRelayReceiver as RococoReceiver, RococoRelaySender as RococoSender, }; pub const ASSET_ID: u32 = 1; @@ -65,6 +65,7 @@ pub type RelayToParaTest = Test; pub type SystemParaToRelayTest = Test; pub type SystemParaToParaTest = Test; pub type ParaToSystemParaTest = Test; +pub type ParaToParaTest = Test; #[cfg(test)] mod tests; 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 dfc0bc6ff392..50f47ab970ba 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 @@ -152,6 +152,69 @@ fn system_para_to_para_assets_receiver_assertions(_: Test) { ); } +fn para_to_para_sender_assertions(t: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + PenpalA::assert_xcm_pallet_attempted_complete(None); + assert_expected_events!( + PenpalA, + vec![ + // Amount to reserve transfer is transferred to Parachain's Sovereign account + RuntimeEvent::Balances( + pallet_balances::Event::Withdraw { who, amount } + ) => { + who: *who == t.sender.account_id, + amount: *amount == t.args.amount, + }, + // XCM sent to relay reserve + RuntimeEvent::ParachainSystem( + cumulus_pallet_parachain_system::Event::UpwardMessageSent { .. } + ) => {}, + ] + ); +} + +fn para_to_para_relay_hop_assertions(t: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + let sov_penpal_a_on_rococo = + Rococo::sovereign_account_id_of(Rococo::child_location_of(PenpalA::para_id())); + let sov_penpal_b_on_rococo = + Rococo::sovereign_account_id_of(Rococo::child_location_of(PenpalB::para_id())); + assert_expected_events!( + Rococo, + vec![ + // Withdrawn from sender parachain SA + RuntimeEvent::Balances( + pallet_balances::Event::Withdraw { who, amount } + ) => { + who: *who == sov_penpal_a_on_rococo, + amount: *amount == t.args.amount, + }, + // Deposited to receiver parachain SA + RuntimeEvent::Balances( + pallet_balances::Event::Deposit { who, .. } + ) => { + who: *who == sov_penpal_b_on_rococo, + }, + RuntimeEvent::MessageQueue( + pallet_message_queue::Event::Processed { success: true, .. } + ) => {}, + ] + ); +} + +fn para_to_para_receiver_assertions(_: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + assert_expected_events!( + PenpalB, + vec![ + RuntimeEvent::Balances(pallet_balances::Event::Deposit { .. }) => {}, + RuntimeEvent::MessageQueue( + pallet_message_queue::Event::Processed { success: true, .. } + ) => {}, + ] + ); +} + fn relay_to_para_reserve_transfer_assets(t: RelayToParaTest) -> DispatchResult { ::XcmPallet::limited_reserve_transfer_assets( t.signed_origin, @@ -185,6 +248,17 @@ fn para_to_system_para_reserve_transfer_assets(t: ParaToSystemParaTest) -> Dispa ) } +fn para_to_para_limited_reserve_transfer_assets(t: ParaToParaTest) -> DispatchResult { + ::PolkadotXcm::limited_reserve_transfer_assets( + t.signed_origin, + bx!(t.args.dest.into()), + bx!(t.args.beneficiary.into()), + bx!(t.args.assets.into()), + t.args.fee_asset_item, + t.args.weight_limit, + ) +} + /// Reserve Transfers of native asset from Relay Chain to the System Parachain shouldn't work #[test] fn reserve_transfer_native_asset_from_relay_to_system_para_fails() { @@ -493,3 +567,51 @@ fn reserve_transfer_assets_from_system_para_to_para() { // Receiver's balance is increased by exact amount assert_eq!(receiver_assets_after, receiver_assets_before + asset_amount_to_send); } + +/// Reserve Transfers of native asset from Parachain to Parachain (through Relay reserve) should +/// work +#[test] +fn reserve_transfer_native_asset_from_para_to_para() { + // Init values for Penpal Parachain + let destination = PenpalA::sibling_location_of(PenpalB::para_id()); + let beneficiary_id = PenpalBReceiver::get(); + let amount_to_send: Balance = ASSET_HUB_ROCOCO_ED * 10000; + let assets = (Parent, amount_to_send).into(); + + let test_args = TestContext { + sender: PenpalASender::get(), + receiver: PenpalBReceiver::get(), + args: TestArgs::new_para(destination, beneficiary_id, amount_to_send, assets, None, 0), + }; + + let mut test = ParaToParaTest::new(test_args); + + let sender_balance_before = test.sender.balance; + let receiver_balance_before = test.receiver.balance; + + let sender_as_seen_by_relay = Rococo::child_location_of(PenpalA::para_id()); + let sov_of_sender_on_relay = Rococo::sovereign_account_id_of(sender_as_seen_by_relay); + + // fund the PenpalA's SA on Rococo with the native tokens held in reserve + Rococo::fund_accounts(vec![(sov_of_sender_on_relay.into(), amount_to_send * 2)]); + + test.set_assertion::(para_to_para_sender_assertions); + test.set_assertion::(para_to_para_relay_hop_assertions); + test.set_assertion::(para_to_para_receiver_assertions); + test.set_dispatchable::(para_to_para_limited_reserve_transfer_assets); + test.assert(); + + let sender_balance_after = test.sender.balance; + let receiver_balance_after = test.receiver.balance; + + let delivery_fees = PenpalA::execute_with(|| { + xcm_helpers::transfer_assets_delivery_fees::< + ::XcmSender, + >(test.args.assets.clone(), 0, test.args.weight_limit, test.args.beneficiary, test.args.dest) + }); + + // Sender's balance is reduced + assert_eq!(sender_balance_before - amount_to_send - delivery_fees, sender_balance_after); + // Receiver's balance is increased + assert!(receiver_balance_after > receiver_balance_before); +} 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 018e948e5d76..409369df7bb0 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 @@ -56,7 +56,8 @@ pub use westend_system_emulated_network::{ AssetHubWestendPara as AssetHubWestend, AssetHubWestendParaReceiver as AssetHubWestendReceiver, AssetHubWestendParaSender as AssetHubWestendSender, BridgeHubWestendPara as BridgeHubWestend, BridgeHubWestendParaReceiver as BridgeHubWestendReceiver, - CollectivesWestendPara as CollectivesWestend, PenpalBPara as PenpalB, + CollectivesWestendPara as CollectivesWestend, PenpalAPara as PenpalA, + PenpalAParaReceiver as PenpalAReceiver, PenpalBPara as PenpalB, PenpalBParaReceiver as PenpalBReceiver, PenpalBParaSender as PenpalBSender, WestendRelay as Westend, WestendRelayReceiver as WestendReceiver, WestendRelaySender as WestendSender, @@ -72,6 +73,7 @@ pub type RelayToParaTest = Test; pub type SystemParaToRelayTest = Test; pub type SystemParaToParaTest = Test; pub type ParaToSystemParaTest = Test; +pub type ParaToParaTest = Test; #[cfg(test)] mod tests; 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 a2934be97580..851d7be6ec9a 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 @@ -162,6 +162,69 @@ fn system_para_to_para_assets_receiver_assertions(_: Test) { ); } +fn para_to_para_sender_assertions(t: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + PenpalB::assert_xcm_pallet_attempted_complete(None); + assert_expected_events!( + PenpalB, + vec![ + // Amount to reserve transfer is transferred to Parachain's Sovereign account + RuntimeEvent::Balances( + pallet_balances::Event::Withdraw { who, amount } + ) => { + who: *who == t.sender.account_id, + amount: *amount == t.args.amount, + }, + // XCM sent to relay reserve + RuntimeEvent::ParachainSystem( + cumulus_pallet_parachain_system::Event::UpwardMessageSent { .. } + ) => {}, + ] + ); +} + +fn para_to_para_relay_hop_assertions(t: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + let sov_penpal_b_on_westend = + Westend::sovereign_account_id_of(Westend::child_location_of(PenpalB::para_id())); + let sov_penpal_a_on_westend = + Westend::sovereign_account_id_of(Westend::child_location_of(PenpalA::para_id())); + assert_expected_events!( + Westend, + vec![ + // Withdrawn from sender parachain SA + RuntimeEvent::Balances( + pallet_balances::Event::Withdraw { who, amount } + ) => { + who: *who == sov_penpal_b_on_westend, + amount: *amount == t.args.amount, + }, + // Deposited to receiver parachain SA + RuntimeEvent::Balances( + pallet_balances::Event::Deposit { who, .. } + ) => { + who: *who == sov_penpal_a_on_westend, + }, + RuntimeEvent::MessageQueue( + pallet_message_queue::Event::Processed { success: true, .. } + ) => {}, + ] + ); +} + +fn para_to_para_receiver_assertions(_: ParaToParaTest) { + type RuntimeEvent = ::RuntimeEvent; + assert_expected_events!( + PenpalA, + vec![ + RuntimeEvent::Balances(pallet_balances::Event::Deposit { .. }) => {}, + RuntimeEvent::MessageQueue( + pallet_message_queue::Event::Processed { success: true, .. } + ) => {}, + ] + ); +} + fn relay_to_para_reserve_transfer_assets(t: RelayToParaTest) -> DispatchResult { ::XcmPallet::limited_reserve_transfer_assets( t.signed_origin, @@ -195,6 +258,17 @@ fn para_to_system_para_reserve_transfer_assets(t: ParaToSystemParaTest) -> Dispa ) } +fn para_to_para_limited_reserve_transfer_assets(t: ParaToParaTest) -> DispatchResult { + ::PolkadotXcm::limited_reserve_transfer_assets( + t.signed_origin, + bx!(t.args.dest.into()), + bx!(t.args.beneficiary.into()), + bx!(t.args.assets.into()), + t.args.fee_asset_item, + t.args.weight_limit, + ) +} + /// Reserve Transfers of native asset from Relay Chain to the System Parachain shouldn't work #[test] fn reserve_transfer_native_asset_from_relay_to_system_para_fails() { @@ -503,3 +577,51 @@ fn reserve_transfer_assets_from_system_para_to_para() { // Receiver's balance is increased by exact amount assert_eq!(receiver_assets_after, receiver_assets_before + asset_amount_to_send); } + +/// Reserve Transfers of native asset from Parachain to Parachain (through Relay reserve) should +/// work +#[test] +fn reserve_transfer_native_asset_from_para_to_para() { + // Init values for Penpal Parachain + let destination = PenpalB::sibling_location_of(PenpalA::para_id()); + let beneficiary_id = PenpalAReceiver::get(); + let amount_to_send: Balance = ASSET_HUB_WESTEND_ED * 1000; + let assets = (Parent, amount_to_send).into(); + + let test_args = TestContext { + sender: PenpalBSender::get(), + receiver: PenpalAReceiver::get(), + args: TestArgs::new_para(destination, beneficiary_id, amount_to_send, assets, None, 0), + }; + + let mut test = ParaToParaTest::new(test_args); + + let sender_balance_before = test.sender.balance; + let receiver_balance_before = test.receiver.balance; + + let sender_as_seen_by_relay = Westend::child_location_of(PenpalB::para_id()); + let sov_of_sender_on_relay = Westend::sovereign_account_id_of(sender_as_seen_by_relay); + + // fund the PenpalB's SA on Westend with the native tokens held in reserve + Westend::fund_accounts(vec![(sov_of_sender_on_relay.into(), amount_to_send * 2)]); + + test.set_assertion::(para_to_para_sender_assertions); + test.set_assertion::(para_to_para_relay_hop_assertions); + test.set_assertion::(para_to_para_receiver_assertions); + test.set_dispatchable::(para_to_para_limited_reserve_transfer_assets); + test.assert(); + + let sender_balance_after = test.sender.balance; + let receiver_balance_after = test.receiver.balance; + + let delivery_fees = PenpalB::execute_with(|| { + xcm_helpers::transfer_assets_delivery_fees::< + ::XcmSender, + >(test.args.assets.clone(), 0, test.args.weight_limit, test.args.beneficiary, test.args.dest) + }); + + // Sender's balance is reduced + assert_eq!(sender_balance_before - amount_to_send - delivery_fees, sender_balance_after); + // Receiver's balance is increased + assert!(receiver_balance_after > receiver_balance_before); +} diff --git a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs index 25a437f3fc6a..a32d186e91ef 100644 --- a/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs +++ b/cumulus/parachains/runtimes/testing/penpal/src/xcm_config.rs @@ -49,12 +49,12 @@ use xcm::latest::prelude::*; use xcm_builder::{ AccountId32Aliases, AllowExplicitUnpaidExecutionFrom, AllowKnownQueryResponses, AllowSubscriptionsFrom, AllowTopLevelPaidExecutionFrom, AsPrefixedGeneralIndex, - ConvertedConcreteId, CurrencyAdapter, DenyReserveTransferToRelayChain, DenyThenTry, - EnsureXcmOrigin, FixedWeightBounds, FrameTransactionalProcessor, FungiblesAdapter, IsConcrete, - LocalMint, NativeAsset, NoChecking, ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, - SiblingParachainAsNative, SiblingParachainConvertsVia, SignedAccountId32AsNative, - SignedToAccountId32, SovereignSignedViaLocation, StartsWith, TakeWeightCredit, - TrailingSetTopicAsId, UsingComponents, WithComputedOrigin, WithUniqueTopic, + ConvertedConcreteId, CurrencyAdapter, EnsureXcmOrigin, FixedWeightBounds, + FrameTransactionalProcessor, FungiblesAdapter, IsConcrete, LocalMint, NativeAsset, NoChecking, + ParentAsSuperuser, ParentIsPreset, RelayChainAsNative, SiblingParachainAsNative, + SiblingParachainConvertsVia, SignedAccountId32AsNative, SignedToAccountId32, + SovereignSignedViaLocation, StartsWith, TakeWeightCredit, TrailingSetTopicAsId, + UsingComponents, WithComputedOrigin, WithUniqueTopic, }; use xcm_executor::{traits::JustTry, XcmExecutor}; @@ -194,34 +194,29 @@ impl Contains for CommonGoodAssetsParachain { } } -pub type Barrier = TrailingSetTopicAsId< - DenyThenTry< - DenyReserveTransferToRelayChain, +pub type Barrier = TrailingSetTopicAsId<( + TakeWeightCredit, + // Expected responses are OK. + AllowKnownQueryResponses, + // Allow XCMs with some computed origins to pass through. + WithComputedOrigin< ( - TakeWeightCredit, - // Expected responses are OK. - AllowKnownQueryResponses, - // Allow XCMs with some computed origins to pass through. - WithComputedOrigin< - ( - // If the message is one that immediately attempts to pay for execution, then - // allow it. - AllowTopLevelPaidExecutionFrom, - // System Assets parachain, parent and its exec plurality get free - // execution - AllowExplicitUnpaidExecutionFrom<( - CommonGoodAssetsParachain, - ParentOrParentsExecutivePlurality, - )>, - // Subscriptions for version tracking are OK. - AllowSubscriptionsFrom, - ), - UniversalLocation, - ConstU32<8>, - >, + // If the message is one that immediately attempts to pay for execution, then + // allow it. + AllowTopLevelPaidExecutionFrom, + // System Assets parachain, parent and its exec plurality get free + // execution + AllowExplicitUnpaidExecutionFrom<( + CommonGoodAssetsParachain, + ParentOrParentsExecutivePlurality, + )>, + // Subscriptions for version tracking are OK. + AllowSubscriptionsFrom, ), + UniversalLocation, + ConstU32<8>, >, ->; +)>; /// Type alias to conveniently refer to `frame_system`'s `Config::AccountId`. pub type AccountIdOf = ::AccountId; diff --git a/polkadot/xcm/xcm-executor/src/lib.rs b/polkadot/xcm/xcm-executor/src/lib.rs index 86304052fbf9..b26779f3ae9d 100644 --- a/polkadot/xcm/xcm-executor/src/lib.rs +++ b/polkadot/xcm/xcm-executor/src/lib.rs @@ -822,6 +822,20 @@ impl XcmExecutor { DepositReserveAsset { assets, dest, xcm } => { let old_holding = self.holding.clone(); let result = Config::TransactionalProcessor::process(|| { + // 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 mut message_to_weigh = + vec![ReserveAssetDeposited(to_weigh.into()), ClearOrigin]; + message_to_weigh.extend(xcm.0.clone().into_iter()); + let (_, fee) = + validate_send::(dest.clone(), Xcm(message_to_weigh))?; + // set aside fee to be charged by XcmSender + let transport_fee = self.holding.saturating_take(fee.into()); + + // now take assets to deposit (excluding transport_fee) let deposited = self.holding.saturating_take(assets); for asset in deposited.assets_iter() { Config::AssetTransactor::deposit_asset(&asset, &dest, Some(&self.context))?; @@ -832,6 +846,8 @@ impl XcmExecutor { let assets = Self::reanchored(deposited, &dest, None); let mut message = vec![ReserveAssetDeposited(assets), ClearOrigin]; message.extend(xcm.0.into_iter()); + // put back transport_fee in holding register to be charged by XcmSender + self.holding.subsume_assets(transport_fee); self.send(dest, Xcm(message), FeeReason::DepositReserveAsset)?; Ok(()) });