Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snowbridge: improve destination fee handling to avoid trapping fees dust #5563

Merged
merged 15 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions bridges/snowbridge/primitives/router/src/inbound/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ impl<CreateAssetCall, CreateAssetDeposit, InboundQueuePalletInstance, AccountId,
InboundQueuePalletInstance,
AccountId,
Balance,
> where
>
where
CreateAssetCall: Get<CallIndex>,
CreateAssetDeposit: Get<u128>,
InboundQueuePalletInstance: Get<u8>,
Expand Down Expand Up @@ -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);
Expand All @@ -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 {
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
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)),
Expand All @@ -199,9 +207,8 @@ where
.into(),
},
RefundSurplus,
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
// 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();

Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is just a hypothetical problem if dest_para_fee and/or asset do not cover the ED on BH. Do we need to prefund this SA?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this has already been prefunded on live chains and is guaranteed to exist (satisfy ED)


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 },
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
]
.into(),
},
// Deposit any leftover unspent AH fees to the snowbridge sovereign.
DepositAsset { assets: Wild(AllCounted(1)), beneficiary: bridge_location },
acatangiu marked this conversation as resolved.
Show resolved Hide resolved
]);
},
None => {
Expand All @@ -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())
}

Expand Down
12 changes: 12 additions & 0 deletions prdoc/pr_5563.prdoc
Original file line number Diff line number Diff line change
@@ -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
Loading