-
Notifications
You must be signed in to change notification settings - Fork 107
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
Refactoring Inbound fixture #1125
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1125 +/- ##
==========================================
- Coverage 75.95% 75.77% -0.19%
==========================================
Files 58 60 +2
Lines 2458 2464 +6
Branches 72 72
==========================================
Hits 1867 1867
- Misses 574 580 +6
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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 would prefer a separate crate Ron, even if its just a single file. As we have seen, feature gating introduces a lot of complexity.
Over time we will need to add many fixtures for different messages coming from Ethereum. So could end up being more than a single file.
use sp_std::vec; | ||
|
||
#[derive(Clone, RuntimeDebug)] | ||
pub struct InboundQueueTest { |
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.
Lets rename this to the describe the message content, ie RegisterAsset, SendToken, etc.
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.
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.
Nice work, @yrong! 🥳
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.
🚢
use snowbridge_core::inbound::{Log, Message, Proof}; | ||
use sp_std::vec; | ||
|
||
pub type SendToken = InboundQueueFixture; |
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 think its a bit unnecessary to add this alias for each fixture.
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.
We should rather add a doc comment (only if necessary - the function name may be enough) saying that the returned fixture contains a send token message.
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.
Yeah, the function name should be enough. Alias removed in 7faa581
Requires: Snowfork/polkadot-sdk#106
Refactoring inbound fixtures to a separate crate and reuse it in emulated tests.