-
Notifications
You must be signed in to change notification settings - Fork 57
apollo_consensus_orchestrator: remove some panics from context #8047
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
base: main-v0.14.0
Are you sure you want to change the base?
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
eth to fri rate: discussed solution on slack, needed due to historic blocks. client errors: if a remote component disconnects this shouldn't cause us to panic
36e46e8
to
8e5a64b
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 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @matan-starkware)
crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs
line 753 at r1 (raw file):
Ok(response) => break response, Err(BatcherClientError::BatcherError(e)) => { panic!("Failed to add decision due to batcher error: {e:?}");
Beyond the scope of this PR but should this cause a panic? maybe add a TODO if it shouldn't
(same for the other methods that wrap client calls)
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 @matan-starkware)
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 all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AlonLStarkWare)
crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs
line 753 at r1 (raw file):
Previously, AlonLStarkWare (Alon-Lukatch-Starkware) wrote…
Beyond the scope of this PR but should this cause a panic? maybe add a TODO if it shouldn't
(same for the other methods that wrap client calls)
For now yes
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 @AlonLStarkWare)
eth to fri rate: discussed solution on slack, needed due to historic blocks.
client errors: if a remote component disconnects this shouldn't cause us to panic