-
Notifications
You must be signed in to change notification settings - Fork 105
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
Disable system calls from xcm & Refactoring emulated tests #1115
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1115 +/- ##
==========================================
+ Coverage 49.74% 50.05% +0.31%
==========================================
Files 61 63 +2
Lines 3701 3736 +35
Branches 72 72
==========================================
+ Hits 1841 1870 +29
- Misses 1843 1849 +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.
+1
@@ -360,6 +358,17 @@ pub mod pallet { | |||
})?; | |||
Ok(()) | |||
} | |||
|
|||
pub fn refund_relayer( |
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 seems we have now have this pattern in the emulated tests where we call these three functions in order:
- refund_relayer
- do_convert
- send_xcm
However this is problematic as internal implementation details are now leaking into the tests. It makes the tests more brittle, and less useful. The tests should treat submit
as a black box.
I would rather refactor the submit
extrinsic, so that it calls the following function, where verification is optional.
fn process_message(message: &Message, verify: bool) -> DispatchResult
And then the simulated tests can call process_message(msg, false)
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.
Notice @claravanstaden is making InboundQueueFixture auto-generated in #1119, then I would prefer to reuse it for the emulated test here. So there is no need to add the process_message(msg, false)
function and just test the original submit
function with the verification included.
The problem is that InboundQueueTest
is for benchmark only and we want to use it also in unit/emulated tests. We did make #1013 for that purpose though closed it for some other high-priority PRs.
So maybe it's time to reopen it. Meanwhile In #1119 we also take it into consideration when generating InboundQueueFixture
.
@vgeddes @claravanstaden WDYT?
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 made a comment here: #1119 I would also like to use https://github.com/Snowfork/snowbridge/tree/main/parachain/pallets/ethereum-client/tests/fixtures in the runtime tests. Maybe worthwhile to split test fixtures out to a new crate.
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.
Cool, ok, this all makes sense - lets split the fixture out into a separate crate - something like pallets/inbound-queue/fixtures
.
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.
Addressed in #1125
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.
Ok now that we have the fixtures, its no longer necessary to have this refactor_relayer
function. @yrong can you revert it?
Also helps reduce the amount of code which needs to be re-audited.
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.
Revert in 4f9f56e
Requires: Snowfork/polkadot-sdk#108