From 9fe882667d4afeba0e06c001ff119bf20d08ef5c Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 3 Sep 2024 15:02:16 +0300 Subject: [PATCH 1/9] snowbridge: improve destination fee handling to avoid trapping fees dust --- .../primitives/router/src/inbound/mod.rs | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index 54e47a7a8b6a..345531392ad0 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -128,7 +128,8 @@ impl where + > +where CreateAssetCall: Get, CreateAssetDeposit: Get, InboundQueuePalletInstance: Get, @@ -167,7 +168,7 @@ where 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); @@ -180,8 +181,15 @@ where // 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![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)), @@ -199,9 +207,8 @@ where .into(), }, RefundSurplus, - // Clear the origin so that remaining assets in holding - // are claimable by the physical origin (BridgeHub) - ClearOrigin, + // Once the program ends here, appendix program will run, which will deposit any + // leftover fee to snowbridge sovereign. ] .into(); @@ -255,20 +262,23 @@ where 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![ // 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 }, ] .into(), }, + // Deposit any leftover unspent AH fees to the snowbridge sovereign. + DepositAsset { assets: Wild(AllCounted(1)), beneficiary: bridge_location }, ]); }, None => { @@ -281,6 +291,8 @@ where }, } + // The `instructions` to forward to AssetHub, and the `total_fees` to locally burn (since + // they are teleported within `instructions`). (instructions.into(), total_fees.into()) } From 2ea8842f3816540d4c6051c26c3e95096f343d9a Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 3 Sep 2024 15:11:15 +0300 Subject: [PATCH 2/9] add prdoc --- prdoc/pr_5563.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_5563.prdoc diff --git a/prdoc/pr_5563.prdoc b/prdoc/pr_5563.prdoc new file mode 100644 index 000000000000..54cac562d2d5 --- /dev/null +++ b/prdoc/pr_5563.prdoc @@ -0,0 +1,12 @@ +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 From 721614f52116fb75d5701c24bf202fd6374c21b6 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 12 Sep 2024 20:45:18 +0300 Subject: [PATCH 3/9] fix test --- .../tests/bridges/bridge-hub-rococo/src/lib.rs | 14 ++++++++------ .../bridge-hub-rococo/src/tests/snowbridge.rs | 18 +++++++++++++----- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs index ac08e48ded68..9d54d431e839 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/lib.rs +++ b/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; diff --git a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs b/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs index 84328fb7c6d2..d91a0c6895f9 100644 --- a/cumulus/parachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs +++ b/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( + ::RuntimeOrigin::signed(PenpalAssetOwner::get()), + native_id, + receiver, + penpal_runtime::EXISTENTIAL_DEPOSIT, + ); PenpalA::execute_with(|| { assert_ok!(::System::set_storage( From 4d2bf07227383f75bc7c03a127ef32c22cd173a6 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Thu, 12 Sep 2024 21:23:03 +0300 Subject: [PATCH 4/9] deposit unspent on errors too --- bridges/snowbridge/primitives/router/src/inbound/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index 345531392ad0..b9308ee598e1 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -265,6 +265,12 @@ where 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].into()), @@ -277,8 +283,6 @@ where ] .into(), }, - // Deposit any leftover unspent AH fees to the snowbridge sovereign. - DepositAsset { assets: Wild(AllCounted(1)), beneficiary: bridge_location }, ]); }, None => { From ad0a0f92a31a31feda24ecccee56dfd3983a2a39 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Fri, 13 Sep 2024 16:06:43 +0300 Subject: [PATCH 5/9] fix test --- bridges/snowbridge/pallets/inbound-queue/src/test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bridges/snowbridge/pallets/inbound-queue/src/test.rs b/bridges/snowbridge/pallets/inbound-queue/src/test.rs index bd993c968df7..536bafc847aa 100644 --- a/bridges/snowbridge/pallets/inbound-queue/src/test.rs +++ b/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, + 110, 77, 136, 239, 46, 53, 75, 198, 213, 193, 26, 111, 158, 230, 163, 212, 182, 42, + 192, 83, 31, 80, 19, 57, 192, 115, 122, 124, 65, 233, 32, 218, ], fee_burned: 110000000000, } From dbd8302d646dbaa26e1813c7e54a48fe8c3ca8af Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Fri, 13 Sep 2024 14:49:57 +0000 Subject: [PATCH 6/9] ".git/.scripts/commands/fmt/fmt.sh" --- bridges/snowbridge/primitives/router/src/inbound/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index 6ecb293acb5c..c74cb78e2079 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -168,8 +168,7 @@ impl< ConvertAssetId, EthereumUniversalLocation, GlobalAssetHubLocation, - > -where + > where CreateAssetCall: Get, CreateAssetDeposit: Get, InboundQueuePalletInstance: Get, @@ -227,8 +226,7 @@ impl< ConvertAssetId, EthereumUniversalLocation, GlobalAssetHubLocation, - > -where + > where CreateAssetCall: Get, CreateAssetDeposit: Get, InboundQueuePalletInstance: Get, From 58db7c2bd3bc4e590414b6e6e13eebb0f08b7356 Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Fri, 13 Sep 2024 19:15:26 +0300 Subject: [PATCH 7/9] fix prdoc --- prdoc/pr_5563.prdoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/prdoc/pr_5563.prdoc b/prdoc/pr_5563.prdoc index 54cac562d2d5..cbf436125bb5 100644 --- a/prdoc/pr_5563.prdoc +++ b/prdoc/pr_5563.prdoc @@ -10,3 +10,5 @@ doc: crates: - name: snowbridge-router-primitives bump: patch + - name: snowbridge-pallet-inbound-queue + bump: patch From f67d5e36aeb1d86b940814f6cc49d4bb625a85fd Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Mon, 23 Sep 2024 18:29:46 +0300 Subject: [PATCH 8/9] address comments --- bridges/snowbridge/primitives/router/src/inbound/mod.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/bridges/snowbridge/primitives/router/src/inbound/mod.rs b/bridges/snowbridge/primitives/router/src/inbound/mod.rs index c74cb78e2079..a10884b45531 100644 --- a/bridges/snowbridge/primitives/router/src/inbound/mod.rs +++ b/bridges/snowbridge/primitives/router/src/inbound/mod.rs @@ -266,10 +266,10 @@ impl< // 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![DepositAsset { - assets: AllCounted(1).into(), - beneficiary: bridge_location, - }])), + 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. @@ -287,7 +287,6 @@ impl< .encode() .into(), }, - RefundSurplus, // Forward message id to Asset Hub SetTopic(message_id.into()), // Once the program ends here, appendix program will run, which will deposit any From 883718508cc68e40c1ff5cdbf92d24d8ee60c4df Mon Sep 17 00:00:00 2001 From: Adrian Catangiu Date: Tue, 24 Sep 2024 10:46:55 +0300 Subject: [PATCH 9/9] fix test --- bridges/snowbridge/pallets/inbound-queue/src/test.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bridges/snowbridge/pallets/inbound-queue/src/test.rs b/bridges/snowbridge/pallets/inbound-queue/src/test.rs index 536bafc847aa..41c38460aabf 100644 --- a/bridges/snowbridge/pallets/inbound-queue/src/test.rs +++ b/bridges/snowbridge/pallets/inbound-queue/src/test.rs @@ -40,8 +40,8 @@ fn test_submit_happy_path() { .into(), nonce: 1, message_id: [ - 110, 77, 136, 239, 46, 53, 75, 198, 213, 193, 26, 111, 158, 230, 163, 212, 182, 42, - 192, 83, 31, 80, 19, 57, 192, 115, 122, 124, 65, 233, 32, 218, + 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, }