chore(node): Make Actors Testable by op-will · Pull Request #3045 · op-rs/kona

op-will · 2025-11-14T21:57:37Z

Summary

This PR makes a number of actor-framework-level changes to allow dependency injection and ultimately unit tests within all of the actors. The actor that is used to demonstrate the updates is SequencerActor. If this approach looks good, issues can be created to update other actors and add test coverage.

This also starts to create client traits for different actor functionality in an effort to remove channel dependencies at the interface level. This will help streamline unit tests and allow the interfaces to evolve more seamlessly.

Key Changes

Loosen NodeActor trait requirements (remove build() fn and builder type)
Remove RollupNodeService trait since it had one implementation and logic split between trait and impl
Create traits for most SequencerActor dependencies
- This includes making facade client traits for actor-actor communication, loosening the interface-level dependence on channels and increasing unit testing ergonomics
- The remaining dependencies will be wrapped in traits as a follow-up PR to this one to avoid further bulking this PR
Make SequencerActor generic over dependency traits
Inject concrete instances of dependencies into SequencerActor
Separate logic in SequencerActor into helper functions for better readability and self-documentation
Pull metrics, admin api server, error, and builder utilities into separate files to self-document and reduce size of main SequencerActor file

Ref: #3025, #3021, #2623

wiz-b4c72f16a4 · 2025-11-14T21:57:59Z

Wiz Scan Summary

Scanner	Findings
Vulnerabilities	-
Sensitive Data	-
Secrets	-
IaC Misconfigurations	-

Total	-

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

crates/node/engine/src/task_queue/tasks/build/error.rs

crates/node/engine/src/task_queue/tasks/seal/error.rs

crates/node/engine/src/task_queue/tasks/build/error.rs

crates/node/engine/src/task_queue/tasks/synchronize/error.rs

Also - Creates a facade for SequencerAdminAPIClient, moving away from explicit dependence on channels - Starts to inject dependencies into SequencerActor. We have to change NodeActor in unfavorable ways to do so, though.

codecov · 2025-11-20T03:38:51Z

Codecov Report

❌ Patch coverage is 65.92179% with 183 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.9%. Comparing base (55152ee) to head (9bc7515).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ode/service/src/actors/sequencer/admin_api_impl.rs	13.0%	60 Missing ⚠️
...e/service/src/actors/sequencer/admin_api_client.rs	4.3%	44 Missing ⚠️
crates/node/service/src/actors/sequencer/actor.rs	60.0%	42 Missing ⚠️
crates/node/service/src/actors/engine/api.rs	37.5%	10 Missing ⚠️
...tes/node/service/src/actors/sequencer/conductor.rs	0.0%	6 Missing ⚠️
...de/service/src/actors/sequencer/origin_selector.rs	33.3%	6 Missing ⚠️
...rates/node/service/src/actors/sequencer/builder.rs	95.1%	4 Missing ⚠️
crates/node/service/src/service/node.rs	97.4%	4 Missing ⚠️
crates/node/rpc/src/admin.rs	40.0%	3 Missing ⚠️
...rates/node/service/src/actors/sequencer/metrics.rs	87.5%	3 Missing ⚠️
... and 1 more

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

- Removed RollupNodeService trait. There was only one implementation, and building out the start function to support actor creation requires access to its state. - Made SequencerActor generic - Consolidated SequencerActor (removing SequencerActorState) - Simplified SequencerActor methods - Pulled SequencerActor admin_api_impl into its own file - Pulled sequencer errors and metrics out to their own files - Pulling reset_engine_forkchoice into BlockEngineClient - Changed EngineAPIClient to use task errors, giving the sequencer much more granular control over its error handling

einar-oplabs

Awesome work!

This looks in line with what we discussed.

Remember using conventional commit in title.

crates/node/engine/src/task_queue/tasks/seal/task.rs

crates/node/service/src/actors/engine/actor.rs

crates/node/service/src/actors/engine/api.rs

crates/node/service/src/actors/sequencer/actor.rs

crates/node/rpc/src/admin.rs

crates/node/service/src/actors/engine/api.rs

crates/node/service/src/actors/engine/actor.rs

crates/node/service/src/service/node.rs

crates/node/service/src/actors/sequencer/actor.rs

crates/node/service/src/actors/sequencer/conductor.rs

crates/node/service/src/actors/engine/api.rs

crates/node/service/src/actors/sequencer/actor.rs

theochap

Looks good overall. A few last nits to fix. Should be good to merge once those are fixed

That is now handled within the sequencer's update_metrics fn. Also: - Simplifies access modifiers in sequencer/metrics.rs - Fixes typo in sequencer/admin_api_client.rs - Moves metrics update inside of conductor presence check in admin_api_impl.rs `override_leader` since if there is no conductor, nothing will change.

This is a remnant from #3045 that should have been removed at that time, as all of its logic was pulled into service/node.rs.

@einar-oplabs

This is a remnant from #3045 that should have been removed at that time, as all of its logic was pulled into service/node.rs. Thanks to @einar-oplabs for pointing this out!

# Summary This PR makes a number of actor-framework-level changes to allow dependency injection and ultimately unit tests within all of the actors. The actor that is used to demonstrate the updates is `SequencerActor`. If this approach looks good, issues can be created to update other actors and add test coverage. This also starts to create client traits for different actor functionality in an effort to remove channel dependencies at the interface level. This will help streamline unit tests and allow the interfaces to evolve more seamlessly. ## Key Changes * Loosen `NodeActor` trait requirements (remove `build()` fn and `builder` type) * Remove `RollupNodeService` trait since it had one implementation and logic split between trait and impl * Create traits for _most_ `SequencerActor` dependencies * This includes making facade client traits for actor-actor communication, loosening the interface-level dependence on channels and increasing unit testing ergonomics * The remaining dependencies will be wrapped in traits as a follow-up PR to this one to avoid further bulking this PR * Make `SequencerActor` generic over dependency traits * Inject concrete instances of dependencies into `SequencerActor` * Separate logic in `SequencerActor` into helper functions for better readability and self-documentation * Pull metrics, admin api server, error, and builder utilities into separate files to self-document and reduce size of main `SequencerActor` file --- Ref: op-rs/kona#3025, op-rs/kona#3021, op-rs/kona#2623

@einar-oplabs

…kona#3129) This is a remnant from op-rs/kona#3045 that should have been removed at that time, as all of its logic was pulled into service/node.rs. Thanks to @einar-oplabs for pointing this out!

# Summary This PR makes a number of actor-framework-level changes to allow dependency injection and ultimately unit tests within all of the actors. The actor that is used to demonstrate the updates is `SequencerActor`. If this approach looks good, issues can be created to update other actors and add test coverage. This also starts to create client traits for different actor functionality in an effort to remove channel dependencies at the interface level. This will help streamline unit tests and allow the interfaces to evolve more seamlessly. ## Key Changes * Loosen `NodeActor` trait requirements (remove `build()` fn and `builder` type) * Remove `RollupNodeService` trait since it had one implementation and logic split between trait and impl * Create traits for _most_ `SequencerActor` dependencies * This includes making facade client traits for actor-actor communication, loosening the interface-level dependence on channels and increasing unit testing ergonomics * The remaining dependencies will be wrapped in traits as a follow-up PR to this one to avoid further bulking this PR * Make `SequencerActor` generic over dependency traits * Inject concrete instances of dependencies into `SequencerActor` * Separate logic in `SequencerActor` into helper functions for better readability and self-documentation * Pull metrics, admin api server, error, and builder utilities into separate files to self-document and reduce size of main `SequencerActor` file --- Ref: #3025, #3021, #2623

theochap reviewed Nov 17, 2025

View reviewed changes

crates/node/engine/src/task_queue/tasks/build/error.rs Outdated Show resolved Hide resolved

theochap reviewed Nov 17, 2025

View reviewed changes

crates/node/engine/src/task_queue/tasks/seal/error.rs Outdated Show resolved Hide resolved

theochap reviewed Nov 17, 2025

View reviewed changes

crates/node/engine/src/task_queue/tasks/seal/error.rs Outdated Show resolved Hide resolved

theochap reviewed Nov 17, 2025

View reviewed changes

crates/node/engine/src/task_queue/tasks/build/error.rs Outdated Show resolved Hide resolved

theochap reviewed Nov 17, 2025

View reviewed changes

crates/node/engine/src/task_queue/tasks/synchronize/error.rs Outdated Show resolved Hide resolved

op-will force-pushed the op-will/spike/refactorNodeForTestability branch from d76717a to fb89f13 Compare November 20, 2025 02:24

Updating SequencerActor to use BuildEngine

9246600

Also - Creates a facade for SequencerAdminAPIClient, moving away from explicit dependence on channels - Starts to inject dependencies into SequencerActor. We have to change NodeActor in unfavorable ways to do so, though.

op-will force-pushed the op-will/spike/refactorNodeForTestability branch from fb89f13 to 6cf140e Compare November 20, 2025 03:33

op-will force-pushed the op-will/spike/refactorNodeForTestability branch from 6cf140e to 8f9d1ee Compare November 20, 2025 03:41

op-will changed the title ~~[Draft] Spike: Make Actors Testable~~ [Draft] Make Actors Testable Nov 20, 2025

einar-oplabs approved these changes Nov 20, 2025

View reviewed changes

op-will changed the title ~~[Draft] Make Actors Testable~~ chore(node): Make Actors Testable Nov 20, 2025

op-will marked this pull request as ready for review November 20, 2025 15:14

op-will requested review from BioMark3r and emhane as code owners November 20, 2025 15:14