nit(node/service): move all the l1 arguments into a single struct#3144
nit(node/service): move all the l1 arguments into a single struct#3144
Conversation
theochap
requested review from
BioMark3r,
einar-oplabs,
emhane and
op-will
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the L1-related configuration by consolidating scattered L1 configuration fields into two cohesive structs: L1Config (runtime configuration) and L1ConfigBuilder (builder pattern). This improves code organization by grouping related configuration parameters together.
Key changes:
- Introduced
L1Configstruct containingchain,trust_rpc,beacon, andengine_providerfields - Introduced
L1ConfigBuilderfor constructingL1Configinstances withconfig,trust_rpc,beacon, andrpc_urlfields - Updated
RollupNodeto use the newL1Configstruct instead of individual fields - Removed redundant
l1_beaconfield fromEngineConfigsince it's now part ofL1Config
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/node/service/src/service/node.rs | Adds L1Config struct and updates RollupNode to use it; all field accesses updated to reference nested fields |
| crates/node/service/src/service/mod.rs | Exports new L1Config and L1ConfigBuilder types |
| crates/node/service/src/service/builder.rs | Adds L1ConfigBuilder and updates RollupNodeBuilder to construct L1Config instances |
| crates/node/service/src/lib.rs | Re-exports new L1Config and L1ConfigBuilder types publicly |
| crates/node/service/src/actors/engine/actor.rs | Removes redundant l1_beacon field from EngineConfig |
| bin/node/src/commands/node.rs | Updates to construct L1ConfigBuilder and passes it to RollupNodeBuilder |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct RollupNodeBuilder { | ||
| /// The rollup configuration. | ||
| pub config: RollupConfig, | ||
| /// The L1 chain configuration. |
There was a problem hiding this comment.
The documentation comment is misleading. The field l1_config_builder is not just "The L1 chain configuration" but is a complete builder that includes the chain configuration, trust RPC flag, beacon URL, and RPC URL. Consider updating to: /// The L1 configuration builder.
| /// The L1 chain configuration. | |
| /// The L1 configuration builder. |
| pub chain: Arc<L1ChainConfig>, | ||
| /// Whether to trust the L1 RPC. | ||
| pub trust_rpc: bool, | ||
| /// The L1 beacon client. | ||
| pub beacon: OnlineBeaconClient, |
There was a problem hiding this comment.
nit: Doesn't hurt to be slightly more specific about these.
| pub chain: Arc<L1ChainConfig>, | |
| /// Whether to trust the L1 RPC. | |
| pub trust_rpc: bool, | |
| /// The L1 beacon client. | |
| pub beacon: OnlineBeaconClient, | |
| pub chain_config: Arc<L1ChainConfig>, | |
| /// Whether to trust the L1 RPC. | |
| pub trust_rpc: bool, | |
| /// The L1 beacon client. | |
| pub beacon_client: OnlineBeaconClient, |
theochap
force-pushed
the
theo/refactor-l1-args-into-config
branch
from
b1b31f8 to
4339379
Compare
…-rs/kona#3144) ## Description Really simple PR that moves all the l1 related config args into the same struct.
…-rs/kona#3144) ## Description Really simple PR that moves all the l1 related config args into the same struct.
Description
Really simple PR that moves all the l1 related config args into the same struct.