feat(bin/node): L2 Chain ID Abstraction#2394
Conversation
There was a problem hiding this comment.
Pull Request Overview
Abstracts the L2 chain ID to a Chain enum instead of raw u64 for improved type safety.
- Changed
l2_chain_idinRollupConfigfromu64toChain, updating defaults and conversions. - Replaced hardcoded chain IDs with
Chain::{optimism_sepolia, optimism_mainnet, base_sepolia, base_mainnet}in test utilities and module initializers. - Propagated
Chainusage through batch decoding, gossip, protocol builders, and executor environment; added thealloy-chainsdependency where needed.
Reviewed Changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/protocol/registry/src/test_utils/op_sepolia.rs | Use Chain::optimism_sepolia() for L2 chain ID |
| crates/protocol/registry/src/test_utils/op_mainnet.rs | Use Chain::optimism_mainnet() for L2 chain ID |
| crates/protocol/registry/src/test_utils/base_sepolia.rs | Use Chain::base_sepolia() for L2 chain ID |
| crates/protocol/registry/src/test_utils/base_mainnet.rs | Use Chain::base_mainnet() for L2 chain ID |
| crates/protocol/protocol/src/batch/core.rs | Converted .derive() call to accept Chain via .into() |
| crates/protocol/genesis/src/rollup.rs | Changed l2_chain_id type to Chain and updated defaults |
| crates/protocol/genesis/src/chain/config.rs | Converted builder to .into() on chain_id |
| crates/protocol/genesis/Cargo.toml | Added alloy-chains dependency |
| crates/proof/executor/src/builder/env.rs | Updated .with_chain_id() to use .into() |
| crates/node/service/src/actors/network/builder.rs | Updated discovery builder to use .id() on Chain |
| crates/node/service/Cargo.toml | Added alloy-chains workspace dependency |
| crates/node/p2p/tests/common/mod.rs | Use Chain::optimism_mainnet() in test setup |
| crates/node/p2p/src/gossip/handler.rs | Switched test handlers to use Chain enum |
| crates/node/p2p/src/gossip/driver.rs | Updated ENR validation to use .into() on Chain |
| crates/node/p2p/src/gossip/block_validity.rs | Updated signature and tests to use .into() on Chain |
| crates/node/p2p/src/gossip/behaviour.rs | Switched test behavior to use Chain enum |
| crates/node/p2p/Cargo.toml | Added alloy-chains workspace dependency |
crates/node/p2p/src/gossip/driver.rs
Outdated
| let validation = | ||
| EnrValidation::validate(&enr, self.handler.rollup_config.l2_chain_id.into()); | ||
| if validation.is_invalid() { | ||
| trace!(target: "gossip", "Invalid OP Stack ENR for chain id {}: {}", self.handler.rollup_config.l2_chain_id, validation); |
There was a problem hiding this comment.
The trace macro is printing a Chain enum directly with {}, which may not render the numeric ID as expected. Use self.handler.rollup_config.l2_chain_id.id() (or .into()) to log the actual u64 chain ID.
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
refcell
left a comment
There was a problem hiding this comment.
Few small changes. Otherwise, looks great
Fixes op-rs/kona#2316 --------- Co-authored-by: refcell <abigger87@gmail.com>
Fixes #2316 --------- Co-authored-by: refcell <abigger87@gmail.com>
Fixes #2316