-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat(l1_provider): implement proposal_start
#2058
base: main
Are you sure you want to change the base?
Conversation
Artifacts upload triggered. View details here |
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.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)
crates/starknet_l1_provider/src/errors.rs
line 17 at r1 (raw file):
GetTransactionConsensusBug, #[error("Cannot transition from {from} to {to}")] UnexpectedProviderState { from: ProviderState, to: ProviderState },
If not too long IYO.
Suggestion:
UnexpectedProviderStateTransition
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 11 at r1 (raw file):
let mut l1_provider = L1Provider::default(); // Test
Suggestion:
// Test.
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 13 at r1 (raw file):
// Test l1_provider.proposal_start().unwrap(); assert_matches!(
Want to check all illegal transitions?
crates/starknet_l1_provider/src/lib.rs
line 3 at r1 (raw file):
pub mod errors; use std::fmt;
Fully-qualified, please.
Code quote:
use std::fmt;
crates/starknet_l1_provider/src/lib.rs
line 102 at r1 (raw file):
/// Current state of the provider, where pending means: idle, between proposal/validation cycles. #[derive(Debug, Default, Clone, Copy)]
Alphabetize.
Code quote:
derive
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2058 +/- ##
===========================================
+ Coverage 40.10% 77.16% +37.05%
===========================================
Files 26 376 +350
Lines 1895 39851 +37956
Branches 1895 39851 +37956
===========================================
+ Hits 760 30750 +29990
- Misses 1100 6819 +5719
- Partials 35 2282 +2247 ☔ 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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @giladchase and @MohammadNassar1)
6c0a505
to
d5692cd
Compare
d5692cd
to
90e39ef
Compare
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.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @elintul and @MohammadNassar1)
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 13 at r1 (raw file):
Previously, elintul (Elin) wrote…
Want to check all illegal transitions?
Thought about it, but i decided it might be easier to see what's being worked on by focusing on the get_txs flow, which I have cooking now. If I start implementing parts of other flows it might be hard to see what's implemented fully and what isn't.
WDYT?
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
No description provided.