-
Notifications
You must be signed in to change notification settings - Fork 766
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
snowbridge: improve destination fee handling to avoid trapping fees dust #5563
Conversation
02e3ee6
to
2ea8842
Compare
The CI pipeline was cancelled due to failure one of the required jobs. |
In #4175 we only cover the trapping on AH, thanks for the fix. Btw:
failed with the error
I think we may need to increase the |
Yes, and once this change is deployed you also need to check this in UIs in production, to avoid failures for users. Check that beneficiary on final dest either:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for the improvement.
…bridge-return-surplus-fees
…bridge-return-surplus-fees
…bridge-return-surplus-fees
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments but they're not that big. Overall looks good
...arachains/integration-tests/emulated/tests/bridges/bridge-hub-rococo/src/tests/snowbridge.rs
Show resolved
Hide resolved
…bridge-return-surplus-fees
…bridge-return-surplus-fees
@@ -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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
62534e5
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-5563-to-stable2407
git worktree add --checkout .worktree/backport-5563-to-stable2407 backport-5563-to-stable2407
cd .worktree/backport-5563-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 62534e53eaeabe021de6fbe9ad51fa0e160a56c5
git push --force-with-lease |
…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)
Successfully created backport PR for |
Backport #5563 into `stable2409` from acatangiu. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Adrian Catangiu <adrian@parity.io>
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.