Skip to content

Conversation

@einar-oplabs
Copy link
Collaborator

closes #3151

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 89.83051% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.8%. Comparing base (62657bf) to head (9507503).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/node/service/src/actors/sequencer/actor.rs 0.0% 3 Missing ⚠️
crates/node/service/src/service/node.rs 93.0% 3 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@op-will op-will left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

Only small nitpicks, so please address as you see fit and merge after you verify tests pass 👍

@op-will op-will marked this pull request as ready for review December 6, 2025 15:20
@einar-oplabs einar-oplabs force-pushed the einar/remove-SequencerActorBuilder branch 3 times, most recently from 77c3d92 to 86c67be Compare December 8, 2025 19:19
@einar-oplabs einar-oplabs force-pushed the einar/remove-SequencerActorBuilder branch from 86c67be to c9bd913 Compare December 9, 2025 14:25
)?,
unsafe_head_rx: unsafe_head_rx.ok_or(
"unsafe_head_rx is None in sequencer mode. This should never happen.".to_string(),
)?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an issue to add type-safety to ensure this actually never happens and we don't need to return an error

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be moved into the if self.mode().is_sequencer() block or else it will break kona-node when run as a validator.

Copy link
Collaborator

@op-will op-will Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add an issue...

I think the easiest approach here is to:

  1. always create these channels
  2. inject the other end into the EngineActor
  3. Only use them if self.mode().is_sequencer()

Since spawn_and_wait! hangs, the sequencer side won't be dropped if running as a validator, but we should probably handle that case gracefully (continuing on rather than erring) in EngineActor

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming tests pass, can we merge this PR as is please?

Moving forward, I am happy to create channels unconditionally or alternative (and not necessarily better) we can create Option channels.

I created this issue #3179. Was that what you had in mind, when you mentioned type-safety, @theochap?

Copy link
Member

@theochap theochap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the negative diff, a few comments

@einar-oplabs einar-oplabs force-pushed the einar/remove-SequencerActorBuilder branch from c9bd913 to 9507503 Compare December 9, 2025 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(node/service): remove SequencerActorBuilder

4 participants