snowbridge: improve destination fee handling to avoid trapping fees d…

…ust (#5563)

On messages Ethereum -> Polkadot Asset Hub: whether they are a token
transfer or a `Transact` for registering new token, make sure to handle
unspent fees, rather than trapping them.

This PR deposits them to Snowbridge's sovereign account on Asset Hub.

---------

Co-authored-by: command-bot <>
(cherry picked from commit 62534e5)

bridges/snowbridge/pallets/inbound-queue/src/test.rs

@@ -40,8 +40,8 @@ fn test_submit_happy_path() {
 				.into(),
 			nonce: 1,
 			message_id: [
-				57, 61, 232, 3, 66, 61, 25, 190, 234, 188, 193, 174, 13, 186, 1, 64, 237, 94, 73,
-				83, 14, 18, 209, 213, 78, 121, 43, 108, 251, 245, 107, 67,
+				255, 125, 48, 71, 174, 185, 100, 26, 159, 43, 108, 6, 116, 218, 55, 155, 223, 143,
+				141, 22, 124, 110, 241, 18, 122, 217, 130, 29, 139, 76, 97, 201,
 			],
 			fee_burned: 110000000000,
 		}

bridges/snowbridge/primitives/router/src/inbound/mod.rs

@@ -249,7 +249,7 @@ impl<
 		let total_amount = fee + CreateAssetDeposit::get();
 		let total: Asset = (Location::parent(), total_amount).into();
 
-		let bridge_location: Location = (Parent, Parent, GlobalConsensus(network)).into();
+		let bridge_location = Location::new(2, GlobalConsensus(network));
 
 		let owner = GlobalConsensusEthereumConvertsFor::<[u8; 32]>::from_chain_id(&chain_id);
 		let asset_id = Self::convert_token_address(network, token);
@@ -262,8 +262,15 @@ impl<
 			// Pay for execution.
 			BuyExecution { fees: xcm_fee, weight_limit: Unlimited },
 			// Fund the snowbridge sovereign with the required deposit for creation.
-			DepositAsset { assets: Definite(deposit.into()), beneficiary: bridge_location },
-			// Only our inbound-queue pallet is allowed to invoke `UniversalOrigin`
+			DepositAsset { assets: Definite(deposit.into()), beneficiary: bridge_location.clone() },
+			// This `SetAppendix` ensures that `xcm_fee` not spent by `Transact` will be
+			// deposited to snowbridge sovereign, instead of being trapped, regardless of
+			// `Transact` success or not.
+			SetAppendix(Xcm(vec![
+				RefundSurplus,
+				DepositAsset { assets: AllCounted(1).into(), beneficiary: bridge_location },
+			])),
+			// Only our inbound-queue pallet is allowed to invoke `UniversalOrigin`.
 			DescendOrigin(PalletInstance(inbound_queue_pallet_index).into()),
 			// Change origin to the bridge.
 			UniversalOrigin(GlobalConsensus(network)),
@@ -280,12 +287,10 @@ impl<
 					.encode()
 					.into(),
 			},
-			RefundSurplus,
-			// Clear the origin so that remaining assets in holding
-			// are claimable by the physical origin (BridgeHub)
-			ClearOrigin,
 			// Forward message id to Asset Hub
 			SetTopic(message_id.into()),
+			// Once the program ends here, appendix program will run, which will deposit any
+			// leftover fee to snowbridge sovereign.
 		]
 		.into();
 
@@ -340,17 +345,24 @@ impl<
 		match dest_para_id {
 			Some(dest_para_id) => {
 				let dest_para_fee_asset: Asset = (Location::parent(), dest_para_fee).into();
+				let bridge_location = Location::new(2, GlobalConsensus(network));
 
 				instructions.extend(vec![
+					// After program finishes deposit any leftover assets to the snowbridge
+					// sovereign.
+					SetAppendix(Xcm(vec![DepositAsset {
+						assets: Wild(AllCounted(2)),
+						beneficiary: bridge_location,
+					}])),
 					// Perform a deposit reserve to send to destination chain.
 					DepositReserveAsset {
-						assets: Definite(vec![dest_para_fee_asset.clone(), asset.clone()].into()),
+						assets: Definite(vec![dest_para_fee_asset.clone(), asset].into()),
 						dest: Location::new(1, [Parachain(dest_para_id)]),
 						xcm: vec![
 							// Buy execution on target.
 							BuyExecution { fees: dest_para_fee_asset, weight_limit: Unlimited },
-							// Deposit asset to beneficiary.
-							DepositAsset { assets: Definite(asset.into()), beneficiary },
+							// Deposit assets to beneficiary.
+							DepositAsset { assets: Wild(AllCounted(2)), beneficiary },
 							// Forward message id to destination parachain.
 							SetTopic(message_id.into()),
 						]
@@ -371,6 +383,8 @@ impl<
 		// Forward message id to Asset Hub.
 		instructions.push(SetTopic(message_id.into()));
 
+		// The `instructions` to forward to AssetHub, and the `total_fees` to locally burn (since
+		// they are teleported within `instructions`).
 		(instructions.into(), total_fees.into())
 	}
 

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs

@@ -55,9 +55,12 @@ mod imports {
 			BridgeHubRococoXcmConfig, EthereumBeaconClient, EthereumInboundQueue,
 		},
 		penpal_emulated_chain::{
-			penpal_runtime::xcm_config::{
-				CustomizableAssetFromSystemAssetHub as PenpalCustomizableAssetFromSystemAssetHub,
-				UniversalLocation as PenpalUniversalLocation,
+			penpal_runtime::{
+				self,
+				xcm_config::{
+					CustomizableAssetFromSystemAssetHub as PenpalCustomizableAssetFromSystemAssetHub,
+					UniversalLocation as PenpalUniversalLocation,
+				},
 			},
 			PenpalAParaPallet as PenpalAPallet, PenpalAssetOwner,
 		},
@@ -72,9 +75,8 @@ mod imports {
 		BridgeHubRococoParaReceiver as BridgeHubRococoReceiver,
 		BridgeHubRococoParaSender as BridgeHubRococoSender,
 		BridgeHubWestendPara as BridgeHubWestend, PenpalAPara as PenpalA,
-		PenpalAParaReceiver as PenpalAReceiver, PenpalAParaSender as PenpalASender,
-		RococoRelay as Rococo, RococoRelayReceiver as RococoReceiver,
-		RococoRelaySender as RococoSender,
+		PenpalAParaSender as PenpalASender, RococoRelay as Rococo,
+		RococoRelayReceiver as RococoReceiver, RococoRelaySender as RococoSender,
 	};
 
 	pub const ASSET_MIN_BALANCE: u128 = 1000;

cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs

@@ -286,11 +286,19 @@ fn send_token_from_ethereum_to_penpal() {
 	// Fund AssetHub sovereign account so it can pay execution fees for the asset transfer
 	BridgeHubRococo::fund_accounts(vec![(asset_hub_sovereign.clone(), INITIAL_FUND)]);
 
-	// Fund PenPal sender and receiver
-	PenpalA::fund_accounts(vec![
-		(PenpalAReceiver::get(), INITIAL_FUND),
-		(PenpalASender::get(), INITIAL_FUND),
-	]);
+	// Fund PenPal receiver (covering ED)
+	let native_id: Location = Parent.into();
+	let receiver: AccountId = [
+		28, 189, 45, 67, 83, 10, 68, 112, 90, 208, 136, 175, 49, 62, 24, 248, 11, 83, 239, 22, 179,
+		97, 119, 205, 75, 119, 184, 70, 242, 165, 240, 124,
+	]
+	.into();
+	PenpalA::mint_foreign_asset(
+		<PenpalA as Chain>::RuntimeOrigin::signed(PenpalAssetOwner::get()),
+		native_id,
+		receiver,
+		penpal_runtime::EXISTENTIAL_DEPOSIT,
+	);
 
 	PenpalA::execute_with(|| {
 		assert_ok!(<PenpalA as Chain>::System::set_storage(

prdoc/pr_5563.prdoc

@@ -0,0 +1,14 @@
+title: "snowbridge: improve destination fee handling to avoid trapping fees dust"
+
+doc:
+  - audience: Runtime User
+    description: |
+      On Ethereum -> Polkadot Asset Hub messages, whether they are a token transfer
+      or a `Transact` for registering a new token, any unspent fees are deposited to
+      Snowbridge's sovereign account on Asset Hub, rather than trapped in AH's asset trap.
+
+crates:
+  - name: snowbridge-router-primitives
+    bump: patch
+  - name: snowbridge-pallet-inbound-queue
+    bump: patch