-
Notifications
You must be signed in to change notification settings - Fork 97
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
Fix CI + one integration test for bridge-hub-kusama
#397
Fix CI + one integration test for bridge-hub-kusama
#397
Conversation
Co-authored-by: Javier Bullrich <javier@bullrich.dev>
Co-authored-by: Javier Bullrich <javier@bullrich.dev>
…ridgeHub, simply just do only dry-run
…er migration to xcm:v5)
bridge-hub-kusama
// Sender's balance is reduced | ||
assert_eq!(sender_ksms_after, sender_ksms_before - amount - delivery_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.
Is this no longer the case or are you just making the test less brittle?
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.
It was not correct, at least, it missed the subtraction of execution_fees
. I tried to add them as well, but let result = Runtime::dry_run_call(origin, call).unwrap();
did not return actual_weight
, so it is not possible to calculate WeightToFee
. Additionally, there was some calculation of intermediate_delivery_fees_amount
from BHK to BHP, which is not relevant. So yes, keep it simple. At least I added asserts to check that the dry-run succeeded and the message is ready to be relayed over bridge to the BHP.
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.
This was added by @franciscoaguirre, fee estimation doesn't yet work with BridgeHub exporters: #380 (comment)
The check will be added back when that is fixed as well.
/merge |
Enabled Available commands
For more information see the documentation |
Failed to update PR ❌There was an error while trying to keep this PR You may have conflicts More info in the logs 📋 |
c61e836
into
polkadot-fellows:main
Closes: #396
This PR:
bridge-hub-kusama-integration-tests
that was not caught due to a previous CI bug.Versioned*::V4
toVersioned*::from
to use the latest version (for easier migration toxcm:v5
).TODO