xcm-executor: DepositReserveAsset charges delivery fees from inner assets (#3142)

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 <adrian@parity.io>
Co-authored-by: Francisco Aguirre <franciscoaguirreperez@gmail.com>

Author: acatangiu

Cargo.lock

@@ -24,8 +24,12 @@ xcm = { package = "staging-xcm", path = "../../../../../../../polkadot/xcm", def
 pallet-xcm = { path = "../../../../../../../polkadot/xcm/pallet-xcm", default-features = false, version = "6.0.0" }
 xcm-executor = { package = "staging-xcm-executor", path = "../../../../../../../polkadot/xcm/xcm-executor", default-features = false, version = "6.0.0" }
 rococo-runtime = { version = "6.0.0", path = "../../../../../../../polkadot/runtime/rococo" }
+
+# Cumulus
 asset-test-utils = { version = "6.0.0", path = "../../../../../runtimes/assets/test-utils" }
-parachains-common = { version = "6.0.0", path = "../../../../../common" }
+parachains-common = { version = "6.0.0", path = "../../../../../../parachains/common" }
+cumulus-pallet-parachain-system = { path = "../../../../../../pallets/parachain-system", default-features = false }
+testnet-parachains-constants = { path = "../../../../../runtimes/constants", features = ["rococo"] }
 asset-hub-rococo-runtime = { version = "0.11.0", path = "../../../../../runtimes/assets/asset-hub-rococo" }
 emulated-integration-tests-common = { path = "../../../common", default-features = false, version = "2.0.0" }
 rococo-system-emulated-network = { version = "0.1.0", path = "../../../networks/rococo-system" }

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/Cargo.toml

@@ -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<Rococo, PenpalA>;
 pub type SystemParaToRelayTest = Test<AssetHubRococo, Rococo>;
 pub type SystemParaToParaTest = Test<AssetHubRococo, PenpalA>;
 pub type ParaToSystemParaTest = Test<PenpalA, AssetHubRococo>;
+pub type ParaToParaTest = Test<PenpalA, PenpalB, Rococo>;
 
 /// Returns a `TestArgs` instance to be used for the Relay Chain across integration tests
 pub fn relay_test_args(

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/lib.rs

@@ -152,6 +152,69 @@ fn system_para_to_para_assets_receiver_assertions<Test>(_: Test) {
 	);
 }
 
+fn para_to_para_sender_assertions(t: ParaToParaTest) {
+	type RuntimeEvent = <PenpalA as Chain>::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 = <Rococo as Chain>::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 = <PenpalB as Chain>::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 {
 	<Rococo as RococoPallet>::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 {
+	<PenpalA as PenpalAPallet>::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::<PenpalA>(para_to_para_sender_assertions);
+	test.set_assertion::<Rococo>(para_to_para_relay_hop_assertions);
+	test.set_assertion::<PenpalB>(para_to_para_receiver_assertions);
+	test.set_dispatchable::<PenpalA>(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::<
+			<PenpalRococoXcmConfig as xcm_executor::Config>::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);
+}

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/reserve_transfer.rs

@@ -51,7 +51,9 @@ pub use westend_system_emulated_network::{
 	westend_emulated_chain::{genesis::ED as WESTEND_ED, WestendRelayPallet as WestendPallet},
 	AssetHubWestendPara as AssetHubWestend, AssetHubWestendParaReceiver as AssetHubWestendReceiver,
 	AssetHubWestendParaSender as AssetHubWestendSender, BridgeHubWestendPara as BridgeHubWestend,
-	BridgeHubWestendParaReceiver as BridgeHubWestendReceiver, PenpalBPara as PenpalB,
+	BridgeHubWestendParaReceiver as BridgeHubWestendReceiver,
+	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,
@@ -67,6 +69,7 @@ pub type RelayToParaTest = Test<Westend, PenpalB>;
 pub type SystemParaToRelayTest = Test<AssetHubWestend, Westend>;
 pub type SystemParaToParaTest = Test<AssetHubWestend, PenpalB>;
 pub type ParaToSystemParaTest = Test<PenpalB, AssetHubWestend>;
+pub type ParaToParaTest = Test<PenpalB, PenpalA, Westend>;
 
 /// Returns a `TestArgs` instance to be used for the Relay Chain across integration tests
 pub fn relay_test_args(

cumulus/parachains/integration-tests/emulated/tests/assets/asset-hub-westend/src/lib.rs

@@ -162,6 +162,69 @@ fn system_para_to_para_assets_receiver_assertions<Test>(_: Test) {
 	);
 }
 
+fn para_to_para_sender_assertions(t: ParaToParaTest) {
+	type RuntimeEvent = <PenpalB as Chain>::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 = <Westend as Chain>::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 = <PenpalA as Chain>::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 {
 	<Westend as WestendPallet>::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 {
+	<PenpalB as PenpalBPallet>::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::<PenpalB>(para_to_para_sender_assertions);
+	test.set_assertion::<Westend>(para_to_para_relay_hop_assertions);
+	test.set_assertion::<PenpalA>(para_to_para_receiver_assertions);
+	test.set_dispatchable::<PenpalB>(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::<
+			<PenpalWestendXcmConfig as xcm_executor::Config>::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);
+}