chore(node/actor): Remove .expect(...) usage#3097
chore(node/actor): Remove .expect(...) usage#3097varun-doshi wants to merge 5 commits intoop-rs:mainfrom
Conversation
varun-doshi
requested review from
BioMark3r,
einar-oplabs,
emhane,
op-will and
theochap
as code owners
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. |
| if self.mode().is_sequencer() { | ||
| validate_sequencer_fields(&build_request_tx, &seal_request_tx, &unsafe_head_rx) | ||
| .ok()?; | ||
| } |
There was a problem hiding this comment.
We know self.mode().is_sequencer() is true, right? We're in the branch where sequencer_actor_builder is Some(_).
| // Validate sequencer fields if node is in sequencer mode. | ||
| if self.mode().is_sequencer() { | ||
| validate_sequencer_fields(&build_request_tx, &seal_request_tx, &unsafe_head_rx) | ||
| .ok()?; | ||
| } | ||
| let block_building_client = QueuedBlockBuildingClient { | ||
| build_request_tx: build_request_tx.expect( | ||
| "build_request_tx is None in sequencer mode. This should never happen.", | ||
| ), | ||
| build_request_tx: build_request_tx.unwrap(), | ||
| reset_request_tx: reset_request_tx.clone(), | ||
| seal_request_tx: seal_request_tx.expect( | ||
| "seal_request_tx is None in sequencer mode. This should never happen.", | ||
| ), | ||
| unsafe_head_rx: unsafe_head_rx.expect( | ||
| "unsafe_head_rx is None in sequencer mode. This should never happen.", | ||
| ), | ||
| seal_request_tx: seal_request_tx.unwrap(), | ||
| unsafe_head_rx: unsafe_head_rx.unwrap(), |
There was a problem hiding this comment.
.unwrap() panics if not Some(_), right?
Perhaps it's best to change these lines to
unsafe_head_rx: unsafe_head_rx?,
and return a well-formed error below if sequencer_actor is None when we expect it to be Some(_)?
There was a problem hiding this comment.
Please also note the .expect on line 250 below
| .ok_or(SequencerBuilderError::MissingField("admin_api_rx".to_string()))?, | ||
| attributes_builder: self | ||
| .attributes_builder | ||
| .ok_or(SequencerBuilderError::MissingField("attributes_builder".to_string()))?, |
There was a problem hiding this comment.
Here we should change the builder to prevent it from being instantiated if the fields are not specified instead of returning an error. This involves replacing the Option by the underlying types
| } | ||
| if unsafe_head_rx.is_none() { | ||
| return Err("unsafe_head_rx is None in sequencer mode. This should never happen".to_string()); | ||
| } |
There was a problem hiding this comment.
We shouldn't need validation for those fields. We should ensure that those fields are always set whenever we build the sequencer actor using the type system
|
Changes made:
My only concern now is that the builder approach is not exactly as it should be since most of the fields are being initialized at constructor level. |
|
checking CI failiures |
| reset_request_tx: reset_request_tx.clone(), | ||
| seal_request_tx: seal_request_tx.unwrap(), | ||
| unsafe_head_rx: unsafe_head_rx.unwrap(), | ||
| }; |
There was a problem hiding this comment.
In a follow-up we could try to tie up the is_sequencer mode to these fields so that we never need to call unwrap here
theochap
left a comment
theochap
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for taking care of it
|
@varun-doshi can you take care of the conflicts with main? |
|
#3152 Handles this by removing the unnecessary builder |
|
This pull request has been automatically marked as stale because it has been inactive for 3 weeks. |
|
This pull request has been automatically closed due to inactivity (4 weeks total). |
3 participants
Fixes #3090