Skip to content

Conversation

@teddyknox
Copy link
Contributor

@teddyknox teddyknox commented Aug 11, 2025

Description

EngDeriver is only superficially separate from EngController, in that all of its event handling logic delegates to calls in EngController. In order to disintermediate events that EngDeriver handles, it would be nice to be able to have event emitters store a reference only to EngController, but call into the handlers in EngDeriver. Thus, we merge the two so that this is possible.

Separately, we MUST refactor EngController to be less monolithic.

Tests

EngDeriver is not well tested right now. This is something we will fix when refactoring EngController.

Additional context

Metadata

@teddyknox teddyknox requested review from a team as code owners August 11, 2025 20:17
@teddyknox teddyknox changed the base branch from develop to teddyknox/refactor-onForkchoiceUpdate August 11, 2025 20:17
@teddyknox teddyknox changed the title Combine EngDeriver and EngController Fold EngDeriver into EngController Aug 11, 2025
@codecov
Copy link

codecov bot commented Aug 11, 2025

Codecov Report

❌ Patch coverage is 73.66255% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.58%. Comparing base (7bb754f) to head (95a21ab).
⚠️ Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
op-node/rollup/engine/engine_controller.go 72.82% 48 Missing and 5 partials ⚠️
op-node/rollup/driver/driver.go 0.00% 3 Missing ⚠️
op-node/rollup/attributes/attributes.go 81.81% 1 Missing and 1 partial ⚠️
op-node/rollup/engine/payload_success.go 60.00% 1 Missing and 1 partial ⚠️
op-program/client/driver/driver.go 0.00% 2 Missing ⚠️
op-node/rollup/engine/payload_invalid.go 0.00% 1 Missing ⚠️
op-node/rollup/engine/payload_process.go 66.66% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (7bb754f) and HEAD (95a21ab). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (7bb754f) HEAD (95a21ab)
cannon-go-tests-64 3 0
contracts-bedrock-tests 2 1
Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #17015       +/-   ##
============================================
- Coverage    81.89%   44.58%   -37.31%     
============================================
  Files          161     1405     +1244     
  Lines         9253   114347   +105094     
============================================
+ Hits          7578    50985    +43407     
- Misses        1528    59707    +58179     
- Partials       147     3655     +3508     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 96.29% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
op-node/rollup/attributes/testutils.go 100.00% <100.00%> (ø)
op-node/rollup/driver/state.go 0.00% <ø> (ø)
op-node/rollup/engine/build_cancel.go 61.11% <100.00%> (ø)
op-node/rollup/engine/build_invalid.go 82.05% <100.00%> (ø)
op-node/rollup/engine/build_seal.go 64.17% <100.00%> (ø)
op-node/rollup/engine/build_sealed.go 100.00% <100.00%> (ø)
op-node/rollup/engine/build_start.go 83.05% <100.00%> (ø)
op-node/rollup/engine/build_started.go 100.00% <100.00%> (ø)
op-node/rollup/engine/events.go 59.13% <ø> (ø)
op-node/rollup/engine/payload_invalid.go 40.00% <0.00%> (ø)
... and 6 more

... and 1338 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@teddyknox teddyknox force-pushed the teddyknox/refactor-onForkchoiceUpdate branch from 7cca719 to b010029 Compare August 11, 2025 21:47
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from b737eb5 to d681555 Compare August 11, 2025 21:48
@teddyknox teddyknox force-pushed the teddyknox/refactor-onForkchoiceUpdate branch from b010029 to 28b1be5 Compare August 11, 2025 22:40
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from d681555 to 11a24ed Compare August 11, 2025 22:40
@teddyknox teddyknox force-pushed the teddyknox/refactor-onForkchoiceUpdate branch from 28b1be5 to 97a3827 Compare August 12, 2025 12:54
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch 2 times, most recently from 2327e1b to 787ddc0 Compare August 12, 2025 13:03
@teddyknox teddyknox force-pushed the teddyknox/refactor-onForkchoiceUpdate branch from 97a3827 to adf7653 Compare August 12, 2025 13:03
Base automatically changed from teddyknox/refactor-onForkchoiceUpdate to develop August 12, 2025 13:43
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from 787ddc0 to e11ac1a Compare August 12, 2025 13:53
@teddyknox teddyknox requested a review from nonsense August 12, 2025 13:54
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch 5 times, most recently from e2b8a59 to eb0be25 Compare August 12, 2025 14:34
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch 3 times, most recently from 49e1416 to 7c5bcf3 Compare August 12, 2025 15:26
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from 7c5bcf3 to 394812a Compare August 13, 2025 17:05
@teddyknox teddyknox requested a review from pcw109550 August 13, 2025 17:05
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from 394812a to 03e2cb8 Compare August 13, 2025 17:06
@teddyknox teddyknox enabled auto-merge August 13, 2025 17:06
@teddyknox teddyknox force-pushed the teddyknox/combine-eng-deriver-and-eng-controller branch from 03e2cb8 to 95a21ab Compare August 13, 2025 17:14
@teddyknox teddyknox added this pull request to the merge queue Aug 13, 2025
Merged via the queue into develop with commit 962c806 Aug 13, 2025
66 checks passed
@teddyknox teddyknox deleted the teddyknox/combine-eng-deriver-and-eng-controller branch August 13, 2025 17:45
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.

5 participants