refactor engineClient for Unified xlayer binary : implement transport-agnostic engine-client#205
refactor engineClient for Unified xlayer binary : implement transport-agnostic engine-client#205
Conversation
Remove OpEngineApi<Optimism, Http<HyperAuthClient>> as a supertrait of EngineClient. The eight Engine API methods used by the task queue (new_payload_v2/v3/v4, fork_choice_updated_v2/v3, get_payload_v2/v3/v4) are now declared explicitly on EngineClient itself. OpEngineClient's EngineClient impl is updated to delegate to its existing HTTP providers and rollup-boost server — behaviour is unchanged. MockEngineClient in test_utils is consolidated: the redundant OpEngineApi impl block is removed and the same methods are served from the single EngineClient impl. Unused storage fields and builder helpers for OpEngineApi-only methods (get_payload_bodies, exchange_capabilities, etc.) are removed. A new test `test_engine_client_is_transport_agnostic` asserts at compile time that a struct with no HTTP types can implement EngineClient and be used as `dyn EngineClient`, which is the prerequisite for the xlayer-node in-process engine bridge (RethInProcessClient in op-reth). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add RollupNode::start_with_client<E: EngineClient> so callers can supply a pre-built engine client instead of having the node build an HTTP client from EngineConfig. The existing start() method is unchanged in behaviour: it builds OpEngineClient as before and delegates to the new shared body start_with_engine(). To support clients that have no RollupBoost server (e.g. in-process bridges), EngineRpcProcessor.rollup_boost_server is changed from Arc<RollupBoostServer> to Option<Arc<RollupBoostServer>>. When None: - Admin queries are logged and ignored. - Health queries respond with ServiceUnavailable. create_engine_actor is made generic over E: EngineClient and now receives a pre-built Arc<E> + Option<Arc<RollupBoostServer>> instead of building the client itself. This is the injection point required for RethInProcessClient (op-reth), which will implement EngineClient and route Engine API calls over Rust channels instead of HTTP. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| async fn new_payload_v2( | ||
| &self, | ||
| payload: ExecutionPayloadInputV2, | ||
| ) -> TransportResult<PayloadStatus> { | ||
| <L2Provider as OpEngineApi<Optimism, Http<HyperAuthClient>>>::new_payload_v2( | ||
| &self.engine, | ||
| payload, | ||
| ) | ||
| .await | ||
| } |
There was a problem hiding this comment.
delegates the function call to OpEngineAPI which will make http based
The delegation always existed — before it was implicit (compiler wired it via supertrait), now it's explicit (we wrote it by hand). Same machine code, different source structure.
Vui-Chee
left a comment
There was a problem hiding this comment.
Extensibility concern: trait will need modification for every new fork
The transport-agnostic refactor is the right direction. One follow-up concern worth addressing before RethInProcessClient is written.
The problem
versions.rs already has the right version-selection enums:
```rust
EngineNewPayloadVersion { V2, V3, V4 }
EngineForkchoiceVersion { V2, V3 }
EngineGetPayloadVersion { V2, V3, V4 }
```
BuildTask and SealTask already consult these enums via from_cfg(). But then the tasks switch on them to call individual versioned methods on the trait (fork_choice_updated_v2/v3, get_payload_v2/v3/v4, new_payload_v2/v3/v4).
This means every new OP Stack fork (V5, V6, ...) requires:
- New methods added to
EngineClienttrait - New impls on
OpEngineClient - New impls on
MockEngineClient - New impls on
RethInProcessClient(the upcoming in-process bridge) - New match arms in
InsertTask,SealTask,BuildTask
Steps 1–4 are pure boilerplate from a leaky abstraction. Step 5 is unavoidable and desirable (compiler-enforced exhaustiveness check is a safety net).
Suggested fix: enum-dispatched methods
Replace the 8 versioned methods with 3 stable methods. The version enum (or the payload variant itself) becomes the discriminant:
```rust
pub trait EngineClient: Send + Sync {
// OpExecutionPayload variant IS the version — no vN method needed
async fn new_payload(
&self,
envelope: OpExecutionPayloadEnvelope,
) -> TransportResult;
// Version enum replaces fork_choice_updated_v2 / _v3
async fn fork_choice_updated(
&self,
version: EngineForkchoiceVersion,
state: ForkchoiceState,
attrs: Option<OpPayloadAttributes>,
) -> TransportResult<ForkchoiceUpdated>;
// Version enum replaces get_payload_v2 / _v3 / _v4; returns unified envelope
async fn get_payload(
&self,
version: EngineGetPayloadVersion,
payload_id: PayloadId,
) -> TransportResult<OpExecutionPayloadEnvelope>;
// all non-versioned methods unchanged
}
```
Impact on call sites
InsertTask::execute() — the 30-line match that calls new_payload_vN collapses to:
```rust
let response = self.client.new_payload(self.envelope.clone()).await;
```
The OpExecutionPayload variant match (for constructing the OpBlock) stays — that's the compiler safety net we want.
SealTask::seal_payload() — the 30-line match that calls get_payload_vN and normalises three different return types into OpExecutionPayloadEnvelope collapses to:
```rust
let version = EngineGetPayloadVersion::from_cfg(cfg, payload_timestamp);
let payload_envelope = self.engine.get_payload(version, payload_id).await?;
```
The normalisation moves into OpEngineClient::get_payload() where it belongs.
BuildTask::start_build() — the match calling fork_choice_updated_v2/v3 collapses to:
```rust
let update = engine_client.fork_choice_updated(forkchoice_version, new_forkchoice, Some(...)).await?;
```
When V5 arrives
- Add
V5to the version enums inversions.rs✓ - Add
V5match arm inOpEngineClient::new_payload()/get_payload()— one place each ✓ - Compiler forces new match arms in the tasks — the safety check we want ✓
- No trait changes. No new impl blocks on Mock or RethInProcessClient. ✓
Recommendation
This is worth doing before RethInProcessClient is written. Once that impl exists, this refactor touches one more file. The trait surface is the contract between kona-node and the in-process bridge — better to get it right before it has two implementors.
@Vui-Chee this sounds nice. will add a new task to enhance this to support versioning. probably in to same PR CC @cliff0412 |
|
@Vui-Chee Planning a follow-up PR to collapse to 3 methods before RethInProcessClient is implemented. Will keep this PR scoped to the trait/injection changes as is. started working on this equivalent changes in |
…num-dispatched Replace new_payload_v1/v2/v3/v4, fork_choice_updated_v2/v3, and get_payload_v2/v3/v4 on the EngineClient trait with three unified methods: - new_payload(OpExecutionPayloadEnvelope) - fork_choice_updated(EngineForkchoiceVersion, ForkchoiceState, Option<OpPayloadAttributes>) - get_payload(EngineGetPayloadVersion, PayloadId) -> OpExecutionPayloadEnvelope Version selection logic moves into OpEngineClient's impl; all task files (insert, build, seal, synchronize) call one method per operation. MockEngineClient and its builder/setter API simplified to match. Compiles clean with zero warnings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR Description
Groundwork for running kona and op-reth in the same process. No in-process client yet — just makes the trait and service ready to accept one.
What changed
EngineClienttrait — HTTP supertrait removed, engine methods declared explicitly:OpEngineClient(HTTP) still delegates to the same providers — no behaviour change.RollupNode— injection point added:flowchart LR subgraph before["before"] S1["start()"] --> B1["build OpEngineClient\ninternally"] --> W1["wire actors"] end subgraph after["after"] S2["start()"] --> B2["build OpEngineClient"] --> SE["start_with_engine()"] --> W2["wire actors"] S3["start_with_client(Arc<E>)"] --> SE endrollup_boost_serveronEngineRpcProcessor:Arc<_>→Option<Arc<_>>—Noneon the in-process path.MockEngineClientconsolidated; addedtest_engine_client_is_transport_agnostic.Follow-up
okx/reth:RethInProcessClient implements EngineClient— routes calls ontoBeaconEngineMessagechannel into op-reth's engine treeokx/xlayer: binary wires both, callsrollup_node.start_with_client(Arc::new(client))