[xcm] Fix SovereignPaidRemoteExporter and DepositAsset handling (#3157)

This PR addresses two issues:
- It modifies `DepositAsset`'s asset filter from `All` to
`AllCounted(1)` to prevent potentially charging excessive weight/fees.
This adjustment avoids situations where fees could be calculated based
on the count of assets, as illustrated
[here](https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/xcm/mod.rs#L38-L46).
- It encapsulates `DepositAsset` with `SetAppendix` to ensure that
`fees` are not trapped in any case. For instance, this prevents issues
when `ExportXcm::validate` encounters an error during the processing of
`ExportMessage`.

cumulus/parachains/runtimes/assets/test-utils/src/test_cases_over_bridge.rs

@@ -244,6 +244,11 @@ pub fn limited_reserve_transfer_assets_for_native_asset_works<
 						_ => Err(ProcessMessageError::BadFormat),
 					})
 					.expect("contains BuyExecution")
+					.match_next_inst(|instr| match instr {
+						SetAppendix(_) => Ok(()),
+						_ => Err(ProcessMessageError::BadFormat),
+					})
+					.expect("contains SetAppendix")
 			} else {
 				xcm_sent
 					.0

cumulus/parachains/runtimes/bridge-hubs/test-utils/src/test_cases/mod.rs

@@ -578,6 +578,10 @@ where
 			fees: Asset { id: AssetId(Location::new(1, [])), fun: Fungible(34333299) },
 			weight_limit: Unlimited,
 		},
+		SetAppendix(Xcm(vec![DepositAsset {
+			assets: Wild(AllCounted(1)),
+			beneficiary: Location::new(1, [Parachain(1000)]),
+		}])),
 		ExportMessage {
 			network: Polkadot,
 			destination: [Parachain(1000)].into(),
@@ -614,7 +618,6 @@ where
 				]),
 			]),
 		},
-		DepositAsset { assets: Wild(All), beneficiary: Location::new(1, [Parachain(1000)]) },
 		SetTopic([
 			36, 224, 250, 165, 82, 195, 67, 110, 160, 170, 140, 87, 217, 62, 201, 164, 42, 98, 219,
 			157, 124, 105, 248, 25, 131, 218, 199, 36, 109, 173, 100, 122,

polkadot/xcm/xcm-builder/src/tests/bridging/mod.rs

@@ -33,6 +33,7 @@ mod paid_remote_relay_relay;
 mod remote_para_para;
 mod remote_para_para_via_relay;
 mod remote_relay_relay;
+mod universal_exports;
 
 parameter_types! {
 	pub Local: NetworkId = ByGenesis([0; 32]);

polkadot/xcm/xcm-builder/src/tests/bridging/paid_remote_relay_relay.rs

@@ -24,9 +24,9 @@
 use super::*;
 
 parameter_types! {
-	// 100 to use the bridge (export) and 80 for the remote execution weight (4 instructions x (10 +
+	// 100 to use the bridge (export) and 80 for the remote execution weight (5 instructions x (10 +
 	// 10) weight each).
-	pub SendOverBridgePrice: u128 = 180u128 + if UsingTopic::get() { 20 } else { 0 };
+	pub SendOverBridgePrice: u128 = 200u128 + if UsingTopic::get() { 20 } else { 0 };
 	pub UniversalLocation: Junctions = [GlobalConsensus(Local::get()), Parachain(100)].into();
 	pub RelayUniversalLocation: Junctions = [GlobalConsensus(Local::get())].into();
 	pub RemoteUniversalLocation: Junctions = [GlobalConsensus(Remote::get())].into();
@@ -101,15 +101,18 @@ fn sending_to_bridged_chain_works() {
 				vec![
 					WithdrawAsset(Asset::from((Here, price)).into()),
 					BuyExecution { fees: (Here, price).into(), weight_limit: Unlimited },
+					SetAppendix(Xcm(vec![DepositAsset {
+						assets: Wild(AllCounted(1)),
+						beneficiary: Parachain(100).into(),
+					}])),
 					ExportMessage {
 						network: ByGenesis([1; 32]),
 						destination: Here,
 						xcm: xcm_with_topic([0; 32], vec![Trap(1)]),
 					},
-					DepositAsset { assets: Wild(All), beneficiary: Parachain(100).into() },
 				],
 			),
-			outcome: Outcome::Complete { used: test_weight(4) },
+			outcome: Outcome::Complete { used: test_weight(5) },
 			paid: true,
 		};
 		assert_eq!(RoutingLog::take(), vec![entry]);
@@ -175,15 +178,18 @@ fn sending_to_parachain_of_bridged_chain_works() {
 				vec![
 					WithdrawAsset(Asset::from((Here, price)).into()),
 					BuyExecution { fees: (Here, price).into(), weight_limit: Unlimited },
+					SetAppendix(Xcm(vec![DepositAsset {
+						assets: Wild(AllCounted(1)),
+						beneficiary: Parachain(100).into(),
+					}])),
 					ExportMessage {
 						network: ByGenesis([1; 32]),
 						destination: Parachain(100).into(),
 						xcm: xcm_with_topic([0; 32], vec![Trap(1)]),
 					},
-					DepositAsset { assets: Wild(All), beneficiary: Parachain(100).into() },
 				],
 			),
-			outcome: Outcome::Complete { used: test_weight(4) },
+			outcome: Outcome::Complete { used: test_weight(5) },
 			paid: true,
 		};
 		assert_eq!(RoutingLog::take(), vec![entry]);

polkadot/xcm/xcm-builder/src/tests/bridging/universal_exports.rs

@@ -0,0 +1,108 @@
+// Copyright (C) Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.
+
+// Polkadot is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Polkadot is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+use super::*;
+use crate::test_utils::TrappedAssets;
+
+#[test]
+fn sovereign_paid_remote_exporter_produces_xcm_which_does_not_trap_assets() {
+	frame_support::parameter_types! {
+		pub BridgeFeeAsset: Location = Parent.into();
+		pub LocalNetwork: NetworkId = ExecutorUniversalLocation::get().global_consensus().expect("valid `NetworkId`");
+		pub LocalBridgeLocation: Location = match &ExecutorUniversalLocation::get().split_global() {
+			Ok((_, junctions)) => Location::new(1, junctions.clone()),
+			_ => panic!("unexpected location format")
+		};
+		pub RemoteNetwork: NetworkId = ByGenesis([1; 32]);
+		pub SendOverBridgePrice: u128 = 333;
+		pub BridgeTable: Vec<NetworkExportTableItem> = vec![
+			NetworkExportTableItem::new(
+				RemoteNetwork::get(),
+				None,
+				LocalBridgeLocation::get(),
+				Some((BridgeFeeAsset::get(), SendOverBridgePrice::get()).into())
+			)
+		];
+		pub static SenderUniversalLocation: InteriorLocation = (LocalNetwork::get(), Parachain(50)).into();
+	}
+
+	// `SovereignPaidRemoteExporter` e.g. used on sibling of `ExecutorUniversalLocation`
+	type Exporter = SovereignPaidRemoteExporter<
+		NetworkExportTable<BridgeTable>,
+		TestMessageSender,
+		SenderUniversalLocation,
+	>;
+
+	// prepare message on sending chain with tested `Exporter` and translate it to the executor
+	// message type
+	let message = Exporter::validate(
+		&mut Some(Location::new(2, [GlobalConsensus(RemoteNetwork::get())])),
+		&mut Some(Xcm(vec![])),
+	)
+	.expect("valid message");
+	let message = Xcm::<TestCall>::from(message.0 .1);
+	let mut message_id = message.using_encoded(sp_io::hashing::blake2_256);
+
+	// allow origin to pass barrier
+	let origin = Location::new(1, Parachain(50));
+	AllowPaidFrom::set(vec![origin.clone()]);
+
+	// fund origin
+	add_asset(origin.clone(), (AssetId(BridgeFeeAsset::get()), SendOverBridgePrice::get() * 2));
+	WeightPrice::set((BridgeFeeAsset::get().into(), 1_000_000_000_000, 1024 * 1024));
+
+	// check before
+	assert!(TrappedAssets::get().is_empty());
+	assert_eq!(exported_xcm(), vec![]);
+
+	// execute XCM with overrides for `MessageExporter` behavior to return `Unroutable` error on
+	// validate
+	set_exporter_override(
+		|_, _, _, _, _| Err(SendError::Unroutable),
+		|_, _, _, _, _| Err(SendError::Transport("not allowed to call here")),
+	);
+	let r = XcmExecutor::<TestConfig>::prepare_and_execute(
+		origin.clone(),
+		message.clone(),
+		&mut message_id,
+		Weight::from_parts(2_000_000_000_000, 2_000_000_000_000),
+		Weight::zero(),
+	);
+	assert_eq!(
+		r,
+		Outcome::Incomplete { used: Weight::from_parts(50, 50), error: XcmError::Unroutable }
+	);
+	// check empty trapped assets
+	assert!(TrappedAssets::get().is_empty());
+	// no xcm exported
+	assert_eq!(exported_xcm(), vec![]);
+
+	// execute XCM again with clear `MessageExporter` overrides behavior to expect delivery
+	clear_exporter_override();
+	let r = XcmExecutor::<TestConfig>::prepare_and_execute(
+		origin.clone(),
+		message,
+		&mut message_id,
+		Weight::from_parts(2_000_000_000_000, 2_000_000_000_000),
+		Weight::zero(),
+	);
+	assert_eq!(r, Outcome::Complete { used: Weight::from_parts(50, 50) });
+
+	// check empty trapped assets
+	assert!(TrappedAssets::get().is_empty());
+	// xcm exported
+	assert_eq!(exported_xcm().len(), 1);
+}

polkadot/xcm/xcm-builder/src/universal_exports.rs

@@ -305,8 +305,14 @@ impl<Bridges: ExporterFor, Router: SendXcm, UniversalLocation: Get<InteriorLocat
 			vec![
 				WithdrawAsset(fees.clone().into()),
 				BuyExecution { fees, weight_limit: Unlimited },
+				// `SetAppendix` ensures that `fees` are not trapped in any case, for example, when
+				// `ExportXcm::validate` encounters an error during the processing of
+				// `ExportMessage`.
+				SetAppendix(Xcm(vec![DepositAsset {
+					assets: AllCounted(1).into(),
+					beneficiary: local_from_bridge,
+				}])),
 				export_instruction,
-				DepositAsset { assets: All.into(), beneficiary: local_from_bridge },
 			]
 		} else {
 			vec![export_instruction]