-
Notifications
You must be signed in to change notification settings - Fork 26
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
refactor(starknet_batcher): move around some types and utils to a common utils file #2622
refactor(starknet_batcher): move around some types and utils to a common utils file #2622
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 12-04-refactor_starknet_batcher_validate_flow_refactor #2622 +/- ##
=========================================================================================
+ Coverage 6.41% 6.45% +0.03%
=========================================================================================
Files 145 146 +1
Lines 17384 17386 +2
Branches 17384 17386 +2
=========================================================================================
+ Hits 1116 1122 +6
+ Misses 16191 16188 -3
+ Partials 77 76 -1 ☔ 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.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)
crates/starknet_batcher/src/utils.rs
line 24 at r1 (raw file):
pub(crate) enum ProposalError { #[error(transparent)] BlockBuilderError(Arc<BlockBuilderError>),
BlockBuilderError has an Aborted variant. Can we use it and remove the ProposalError completely?
fe25e29
to
9fff59e
Compare
09f7de9
to
08ae4fc
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: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)
crates/starknet_batcher/src/utils.rs
line 24 at r1 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
BlockBuilderError has an Aborted variant. Can we use it and remove the ProposalError completely?
Done.
8a1dcfd
to
b3026d5
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 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs
line 341 at r3 (raw file):
.await .expect("Proposal should exist.") .map_err(|_| BatcherError::InternalError)?;
Why is this an internal error? The proposal ID comes from the input
Same below
crates/starknet_batcher/src/utils.rs
line 114 at r3 (raw file):
// transactions), or couldn't finish in time. BlockBuilderError::FailOnError(_) => Ok(ProposalStatus::InvalidProposal), BlockBuilderError::Aborted => Err(BatcherError::ProposalAborted),
When can this be returned? Does it make sense to return this error in all those cases?
Code quote:
BlockBuilderError::Aborted => Err(BatcherError::ProposalAborted),
b3026d5
to
9e4709d
Compare
9fff59e
to
fe2c664
Compare
9e4709d
to
dfa16fa
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: all files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs
line 341 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Why is this an internal error? The proposal ID comes from the input
Same below
Note that we expect
the proposal to exist. The InternalError
is returned in case the proposal finished with an error.
By the way, I didn't change any logic here, the same thing happened before but was hidden behind the ?
, and the From
implementation:
impl From<ProposalError> for BatcherError {
fn from(err: ProposalError) -> Self {
match err {
ProposalError::BlockBuilderError(..) => BatcherError::InternalError,
ProposalError::Aborted => BatcherError::ProposalAborted,
}
}
}
crates/starknet_batcher/src/utils.rs
line 114 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
When can this be returned? Does it make sense to return this error in all those cases?
It can only happen if the consensus sends a get_proposal_content
request about an aborted proposal (which shouldn't really happen).
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, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs
line 341 at r3 (raw file):
Previously, dafnamatsry wrote…
Note that we
expect
the proposal to exist. TheInternalError
is returned in case the proposal finished with an error.By the way, I didn't change any logic here, the same thing happened before but was hidden behind the
?
, and theFrom
implementation:impl From<ProposalError> for BatcherError { fn from(err: ProposalError) -> Self { match err { ProposalError::BlockBuilderError(..) => BatcherError::InternalError, ProposalError::Aborted => BatcherError::ProposalAborted, } } }
Right. expect
ing it is even worse, no? A wrong input can make the batcher panic. Do we want an error there?
crates/starknet_batcher/src/utils.rs
line 114 at r3 (raw file):
Previously, dafnamatsry wrote…
It can only happen if the consensus sends a
get_proposal_content
request about an aborted proposal (which shouldn't really happen).
Great, thanks.
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, 1 unresolved discussion (waiting on @alonh5 and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs
line 341 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Right.
expect
ing it is even worse, no? A wrong input can make the batcher panic. Do we want an error there?
I think the expect
here assumes we already check the proposal exists above:
let tx_stream = &mut self
.propose_tx_streams
.get_mut(&proposal_id)
.ok_or(BatcherError::ProposalNotFound { proposal_id })?;
I can change it to return an error instead, but it shouldn't happen.
dfa16fa
to
5fda731
Compare
5fda731
to
cbee779
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: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs
line 341 at r3 (raw file):
Previously, dafnamatsry wrote…
I think the
expect
here assumes we already check the proposal exists above:let tx_stream = &mut self .propose_tx_streams .get_mut(&proposal_id) .ok_or(BatcherError::ProposalNotFound { proposal_id })?;
I can change it to return an error instead, but it shouldn't happen.
Oh I see. No that's fine.
No description provided.