Skip to content

Snowbridge: deposit extra fee to beneficiary on Asset Hub #4175

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.

Sign up for GitHub

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

Merged
serban300 merged 16 commits into from
Apr 26, 2024
Merged

Snowbridge: deposit extra fee to beneficiary on Asset Hub #4175

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
8f86f35
Deposit fee to beneficiary
yrong Apr 17, 2024
0a218c7
E2E tests
yrong Apr 18, 2024
3fd810c
Add SetErrorHandler
yrong Apr 18, 2024
db52103
Revert "Add SetErrorHandler"
yrong Apr 18, 2024
bf3856b
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 22, 2024
908442b
Add prdoc
yrong Apr 22, 2024
54d8d4e
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 22, 2024
dee6ecc
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 23, 2024
76755f9
Update cumulus/parachains/integration-tests/emulated/tests/bridges/br…
yrong Apr 23, 2024
3eed3aa
Update cumulus/parachains/integration-tests/emulated/tests/bridges/br…
yrong Apr 23, 2024
bd36b4d
Merge branch 'master' into deposit-fee-to-beneficiary
claravanstaden Apr 25, 2024
45481f9
Update prdoc/pr_4175.prdoc
yrong Apr 25, 2024
3c7a66d
Update prdoc/pr_4175.prdoc
yrong Apr 25, 2024
3c1ac1a
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 25, 2024
a194ca5
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 25, 2024
b79bc16
Merge branch 'master' into deposit-fee-to-beneficiary
yrong Apr 26, 2024
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
6 changes: 4 additions & 2 deletions bridges/snowbridge/primitives/router/src/inbound/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,8 +273,10 @@ where
},
None => {
instructions.extend(vec![
// Deposit asset to beneficiary.
DepositAsset { assets: Definite(asset.into()), beneficiary },
// Deposit both asset and fees to beneficiary so the fees will not get
// trapped. Another benefit is when fees left more than ED on AssetHub could be
// used to create the beneficiary account in case it does not exist.
DepositAsset { assets: Wild(All), beneficiary },
Copy link
Copy Markdown
Contributor

@acatangiu acatangiu Apr 17, 2024

Choose a reason for hiding this comment

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

I believe whole XCM fails if the beneficiary account does not exist and the leftover fees do not satisfy ED... :(

Maybe add an SetErrorHandler and fall back to deposit just the assets if the wildcard one fails.

I recommend building a test or more to validate both happy and corner cases.

Copy link
Copy Markdown
Contributor Author

@yrong yrong Apr 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for review it.

fall back to deposit just the assets if the wildcard one fails.

Currently WETH or any foreign asset from Ethereum is created as non_sufficient, so the fall back won't help in this case.

the leftover fees do not satisfy ED

As mentioned in Snowfork#137 (comment), we'll ensure this won't happen.

building a test or more to validate both happy and corner cases.

Sure, I'll do that.

Copy link
Copy Markdown
Contributor

@bkontur bkontur Apr 17, 2024

Choose a reason for hiding this comment

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

I would avoid using DepositAsset(Wild(All)) because it can be quite expensive, e.g on the AssetHub here: https://github.com/paritytech/polkadot-sdk/blob/master/cumulus/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/xcm/mod.rs#L33-L51. It's better to use either Wild(AllOfCounted(2)) to deposit both the asset and fees, or to use Definite([asset, fees]).

We encountered a very similar issue raised by the security audit team here: #3157, and there was also a recommendation to use SetAppendix to avoid potential trapping.

Copy link
Copy Markdown
Contributor

@bkontur bkontur Apr 17, 2024

Choose a reason for hiding this comment

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

I recommend building a test or more to validate both happy and corner cases.

👆 💯

Copy link
Copy Markdown
Contributor Author

@yrong yrong Apr 18, 2024

Choose a reason for hiding this comment

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

More tests added in
0a218c7

@bkontur I'm not sure SetAppendix will help in this case, the DepositAsset is the last instruction of the xcm and if any of the preceding instructions fails, we do want the xcm fail as a whole.

Copy link
Copy Markdown
Contributor

@bkontur bkontur Apr 18, 2024

Choose a reason for hiding this comment

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

SetAppendix

if you put something in the holding register (e.g. WithdrawAsset...) and another instruction fails, then the assets are trapped. But if you use SetAppendix(DepositAssets and place it before the failing instruction, then assets won't be trapped in the case of error, but instead Deposited to some account, so no asset trapping.

But it depends case-by-case, I'm not saying that you need to use it, I am just saying that there is this possibility.

Copy link
Copy Markdown
Contributor Author

@yrong yrong Apr 18, 2024

Choose a reason for hiding this comment

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

Thanks for the reminder. I'll let our team to double check though I do think DepositAsset should be the last instruction here.

Copy link
Copy Markdown
Contributor Author

@yrong yrong Apr 22, 2024

Choose a reason for hiding this comment

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

@bkontur Confirmed that we don't need the SetAppendix. We'll just leave the asset trapped when error happens.

]);
},
}
Expand Down