-
Notifications
You must be signed in to change notification settings - Fork 29
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(starknet_l1_provider): add start_block
#3226
base: main
Are you sure you want to change the base?
Conversation
4d2d043
to
806a96e
Compare
2ecd1a5
to
82a1090
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.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase)
crates/starknet_l1_provider/src/lib.rs
line 0 at r2 (raw file):
In separate PRs can you move stuff out of lib.rs?
crates/starknet_l1_provider/src/lib.rs
line 51 at r2 (raw file):
Err(L1ProviderError::unexpected_transition(self, new_state)) } }
Might be worth a docstring explaining this.
Code quote:
match new_state {
ProviderState::Propose | ProviderState::Validate => Ok(new_state),
ProviderState::Uninitialized | ProviderState::Pending => {
Err(L1ProviderError::unexpected_transition(self, new_state))
}
}
806a96e
to
295fa55
Compare
82a1090
to
9f9d7f7
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: 3 of 9 files reviewed, 2 unresolved discussions (waiting on @matan-starkware)
crates/starknet_l1_provider/src/lib.rs
line 51 at r2 (raw file):
Previously, matan-starkware wrote…
Might be worth a docstring explaining this.
Good point.
How about strong-typing it? i added a new "external" type for start_block, which will be the one sent through clients.
This has the added benefit of hiding the internal Pending/Uninitialized states from the users, which probably should have been the case to begin with 😅
crates/starknet_l1_provider/src/lib.rs
line at r2 (raw file):
Previously, matan-starkware wrote…
In separate PRs can you move stuff out of lib.rs?
Yep i remember you asked, i have a commit ready after this is merged, state is moved into l1_provider
9f9d7f7
to
2054a50
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.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @giladchase)
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 103 at r3 (raw file):
#[test] #[should_panic(expected = "Uninitialized L1 provider")]
Given that this (and get_txs) are public functions which already return a result type, should we just return an error from them instead?
Code quote:
#[should_panic(expected = "Uninitialized L1 provider")]
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 125 at r3 (raw file):
l1_provider.start_block(BlockNumber(1), ValidateSession).unwrap(); l1_provider.start_block(BlockNumber(1), ValidateSession).unwrap();
IDK if this is actually a good idea but would be kinda fun to replace all of this with cartesian_product - https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.cartesian_product
Code quote:
l1_provider.start_block(BlockNumber(1), ProposeSession).unwrap();
l1_provider.start_block(BlockNumber(1), ValidateSession).unwrap();
l1_provider.start_block(BlockNumber(1), ProposeSession).unwrap();
l1_provider.start_block(BlockNumber(1), ProposeSession).unwrap();
l1_provider.start_block(BlockNumber(1), ValidateSession).unwrap();
l1_provider.start_block(BlockNumber(1), ValidateSession).unwrap();
2054a50
to
269d382
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: 8 of 11 files reviewed, 2 unresolved discussions (waiting on @matan-starkware)
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 103 at r3 (raw file):
Previously, matan-starkware wrote…
Given that this (and get_txs) are public functions which already return a result type, should we just return an error from them instead?
Yeo, doing it in the commit_block stack.
The test is removed there because this flow is becomes a special case of the "get/validate called without start_block".
crates/starknet_l1_provider/src/l1_provider_tests.rs
line 125 at r3 (raw file):
Previously, matan-starkware wrote…
IDK if this is actually a good idea but would be kinda fun to replace all of this with cartesian_product - https://docs.rs/itertools/latest/itertools/trait.Itertools.html#method.cartesian_product
NOICE!
I can't we're using cartez in code in way that actually improves readability.
Artifacts upload workflows: |
- replace preivous API with the agreed upon `start_block`. - added state setter for use of start_block; commit_block will have a different setter.
269d382
to
0c97f87
Compare
start_block
.