Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1089 +/- ##
===========================================
- Coverage 80.69% 58.12% -22.58%
===========================================
Files 55 55
Lines 2274 2173 -101
Branches 71 71
===========================================
- Hits 1835 1263 -572
- Misses 422 893 +471
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. |
claravanstaden
left a comment
claravanstaden
left a comment
There was a problem hiding this comment.
Looks great @yrong, thanks!
| ] | ||
| beacon-spec-mainnet = [ | ||
| "snowbridge-ethereum-beacon-client/beacon-spec-mainnet", | ||
| fast-runtime = [ |
There was a problem hiding this comment.
Lets use beacon-spec-minimal here so that the flag sounds like its to do with beacon chain as opposed to any of the runtimes.
There was a problem hiding this comment.
Then the prod_or_fast macro will not work.
https://github.com/Snowfork/polkadot-sdk/pull/89/files#r1434875211
Btw, it's already used in Polkadot codebase everywhere(e.g. for a fast epoch time) and I would prefer to be in consistent with them.
There was a problem hiding this comment.
I don't see that problem. The BridgeHub runtime will continue to have a fast-runtime feature, that will enable the beacon-spec-mainnet feature within the beacon client spec
| "sp-runtime/try-runtime", | ||
| ] | ||
| beacon-spec-mainnet = [] | ||
| fast-runtime = [] |
There was a problem hiding this comment.
Within the context of this crate, I prefer the name beacon-spec-minimal
There was a problem hiding this comment.
#1089 (comment) and IMO mininal-spec is just some kind of the fast-runtime, it's more abstract not only apply to the context of beacon but other contexts we may need later.
There was a problem hiding this comment.
As Vincent suggests, Stick to use fast-runtime feature in BH runtime and only enables the beacon-spec-minimal feature in that case.
4 participants
Address the comments in paritytech/polkadot-sdk#2777 (comment)
Requires: Snowfork/polkadot-sdk#89