refactor(rpc): compose api router from per-feature route modules by MegaRedHand · Pull Request #454 · lambdaclass/ethlambda

MegaRedHand · 2026-06-24T17:52:47Z

Behavior-preserving refactor of the RPC crate's router setup. Instead of a single monolithic router, routes are now composed from per-feature modules, each responsible for registering its own handlers.

This scaffolding is a prerequisite for the stacked endpoint PRs that follow: each new endpoint is added as its own module without touching unrelated code.

Passes clippy with -D warnings and all existing tests.

Split the monolithic lib.rs API router into focused modules: core.rs holds the finalized-state/finalized-block/justified-checkpoint handlers and shared response helpers; blocks.rs, fork_choice.rs, and admin.rs each expose pub(crate) routes() -> Router<Store>. build_api_router merges them with .with_state(store). No behavior change.

github-actions · 2026-06-24T17:53:56Z

🤖 Kimi Code Review

Overall Assessment: Good refactoring that improves code organization by decentralizing route definitions into submodule routes() functions. The structure follows Axum best practices. However, there are critical safety concerns in the new core.rs module regarding error handling and potential state mutation.

Critical Issues

Panic Risk in Finalized State Handler (crates/net/rpc/src/core.rs:26)
```
.expect("finalized state exists")
```
Using .expect() in an HTTP handler will panic the entire RPC server task if the database is corrupted or there's a pruning race condition. Return StatusCode::INTERNAL_SERVER_ERROR instead.
Potential Cache Corruption (crates/net/rpc/src/core.rs:30-32)
```
state.latest_block_header.state_root = H256::ZERO;
```
If Store::get_state returns a reference to cached internal data (e.g., Arc<BeaconState> or a database cache entry), this mutation corrupts the node's view of the finalized state. Verify that get_state returns an owned value or clone before mutating:
```
let mut state = store.get_state(&finalized.root)?.clone();
```

Moderate Issues

Misleading Function Name (crates/net/rpc/src/core.rs:47-50)
get_latest_justified_state returns a Checkpoint, not a state. Rename to get_latest_justified_checkpoint to match the route (/checkpoints/justified) and actual behavior.
Missing Module Documentation (crates/net/rpc/src/core.rs:1)
Add module-level documentation explaining this module contains canonical checkpoint sync endpoints per the Ethereum consensus specification.

Minor Observations

Unused State in Admin Router (crates/net/rpc/src/admin.rs:28)
The admin::routes() router accepts Store as state type but handlers only extract Extension<AggregatorController>. While harmless, consider using Router<()> if Store is never accessed, or document why the state type is preserved for composition purposes.
Genesis Block 404 (crates/net/rpc/src/core.rs:41-43)
The 404 response for genesis block is noted in the comment. Ensure this aligns with the checkpoint sync specification—some clients may expect the genesis block to be available even without signatures.

Security Note
The SSZ response helpers properly set application/octet-stream content type, which is correct for binary SSZ data. Ensure these endpoints have appropriate rate limiting in production to prevent DoS via large state serialization.

Consensus Correctness
The state_root zeroing logic (core.rs:30-32) correctly implements the canonical post-state representation required for checkpoint sync interoperability per the Ethereum spec. However, confirm this doesn't break if the client implements future tree-state or state diff optimizations where the retrieved state might be a delta rather than full copy.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

github-actions · 2026-06-24T17:54:23Z

🤖 Codex Code Review

Findings

Low: crates/net/rpc/src/core.rs:43 has a stale behavioral comment. It says /lean/v0/blocks/finalized returns 404 for genesis, but storage explicitly synthesizes an empty proof for the genesis anchor so that get_signed_block can serve it, including this RPC path (crates/storage/src/store.rs:2517). The moved code still returns 200 for genesis, so the implementation is fine; the risk is that a future cleanup may “fix” the code to match the comment and break checkpoint-sync/genesis retrieval semantics.

I didn’t find other correctness, security, performance, or consensus-path regressions in this PR. The change is effectively a route/module refactor; fork choice, attestation validation, justification/finalization, STF, XMSS, and SSZ logic are untouched.

I couldn’t run cargo test in this environment because the toolchain and cargo homes under /home/runner are read-only and dependency fetches are blocked, so this review is based on code inspection rather than a local compile/test pass.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

greptile-apps · 2026-06-24T17:55:56Z

Greptile Summary

This PR splits the RPC crate's monolithic router in lib.rs into four per-feature modules (core, blocks, fork_choice, admin), each exposing a routes() factory that is composed with Router::merge() in build_api_router. Visibility of handlers is narrowed to pub(crate), and json_response is centralised in core.rs and re-exported through lib.rs.

All route registrations and handler implementations are mechanically moved; no endpoint paths, HTTP methods, or response shapes change.
The fork_choice.rs test helper is simplified to call routes() directly, removing the manual route duplication.
Two minor issues: the new module is named core, which shadows Rust's built-in core crate in lib.rs, and a stale inline comment on get_latest_finalized_block still says genesis returns 404 despite the test asserting 200.

Confidence Score: 4/5

Safe to merge — all endpoints, paths, and response shapes are unchanged; the refactor is purely structural.

The change is a clean mechanical extraction: routes move into dedicated modules, handlers become crate-private, and the merge composition preserves axum's static-before-dynamic matching priority. Two minor rough edges remain — the new module is named core, which shadows the built-in Rust core crate and could surprise future contributors, and a copied inline comment incorrectly describes the genesis 404 behaviour that the tests disprove.

No files require special attention beyond the two style notes in lib.rs (module naming) and core.rs (stale comment).

Important Files Changed

Filename	Overview
crates/net/rpc/src/lib.rs	Monolithic router replaced with .merge() of per-feature route modules; naming a local module `core` shadows Rust's built-in `core` crate.
crates/net/rpc/src/core.rs	New module housing finalized-state, finalized-block, and justified-checkpoint endpoints plus shared helpers; stale comment claims genesis returns 404 but tests assert 200.
crates/net/rpc/src/admin.rs	Added routes() factory function; handlers tightened to pub(crate); no behavioural change.
crates/net/rpc/src/blocks.rs	Added routes() factory function; handlers tightened to pub(crate); no behavioural change.
crates/net/rpc/src/fork_choice.rs	Added routes() factory function; test helper updated to reuse it; handlers tightened to pub(crate); no behavioural change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["start_rpc_server()"] --> B["build_api_router(store)"]
    B --> C["core::routes()"]
    B --> D["blocks::routes()"]
    B --> E["fork_choice::routes()"]
    B --> F["admin::routes()"]

    C --> C1["/lean/v0/health"]
    C --> C2["/lean/v0/states/finalized"]
    C --> C3["/lean/v0/blocks/finalized"]
    C --> C4["/lean/v0/checkpoints/justified"]

    D --> D1["/lean/v0/blocks/{block_id}"]
    D --> D2["/lean/v0/blocks/{block_id}/header"]

    E --> E1["/lean/v0/fork_choice"]
    E --> E2["/lean/v0/fork_choice/ui"]

    F --> F1["/lean/v0/admin/aggregator (GET+POST)"]

    A --> G["build_debug_router()"]
    G --> G1["/debug/pprof/allocs"]
    G --> G2["/debug/pprof/allocs/flamegraph"]

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["start_rpc_server()"] --> B["build_api_router(store)"]
    B --> C["core::routes()"]
    B --> D["blocks::routes()"]
    B --> E["fork_choice::routes()"]
    B --> F["admin::routes()"]

    C --> C1["/lean/v0/health"]
    C --> C2["/lean/v0/states/finalized"]
    C --> C3["/lean/v0/blocks/finalized"]
    C --> C4["/lean/v0/checkpoints/justified"]

    D --> D1["/lean/v0/blocks/{block_id}"]
    D --> D2["/lean/v0/blocks/{block_id}/header"]

    E --> E1["/lean/v0/fork_choice"]
    E --> E2["/lean/v0/fork_choice/ui"]

    F --> F1["/lean/v0/admin/aggregator (GET+POST)"]

    A --> G["build_debug_router()"]
    G --> G1["/debug/pprof/allocs"]
    G --> G2["/debug/pprof/allocs/flamegraph"]

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/net/rpc/src/core.rs:43
**Stale inline comment contradicts test behaviour**

The comment says "Returns 404 for genesis since it doesn't have a valid signature", but the integration test `test_get_latest_finalized_block_serves_genesis_with_placeholder_proof` (in `lib.rs`) explicitly documents and asserts that genesis returns 200 with a synthesised placeholder proof. The comment was copied from the original `lib.rs` without being updated, so it now misleads anyone reading this handler.

### Issue 2 of 2
crates/net/rpc/src/lib.rs:13
**Module name `core` shadows the standard library crate**

Declaring `mod core;` in a Rust crate root shadows the built-in `core` crate for all code in `lib.rs`. Any future use of `core::` within this file will resolve to the local module rather than the standard library, which can cause confusing compilation errors or subtle misbehaviour. A name like `endpoints_core`, `base`, or `common` would sidestep this shadowing entirely.

_{Reviews (1): Last reviewed commit: "refactor(rpc): compose api router from p..." | Re-trigger Greptile}

greptile-apps · 2026-06-24T17:55:59Z

+    axum::extract::State(store): axum::extract::State<Store>,
+) -> impl IntoResponse {
+    let finalized = store.latest_finalized();
+    // Returns 404 for genesis since it doesn't have a valid signature


Stale inline comment contradicts test behaviour

The comment says "Returns 404 for genesis since it doesn't have a valid signature", but the integration test test_get_latest_finalized_block_serves_genesis_with_placeholder_proof (in lib.rs) explicitly documents and asserts that genesis returns 200 with a synthesised placeholder proof. The comment was copied from the original lib.rs without being updated, so it now misleads anyone reading this handler.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/net/rpc/src/core.rs Line: 43 Comment: **Stale inline comment contradicts test behaviour** The comment says "Returns 404 for genesis since it doesn't have a valid signature", but the integration test `test_get_latest_finalized_block_serves_genesis_with_placeholder_proof` (in `lib.rs`) explicitly documents and asserts that genesis returns 200 with a synthesised placeholder proof. The comment was copied from the original `lib.rs` without being updated, so it now misleads anyone reading this handler. How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps · 2026-06-24T17:56:00Z


 mod admin;
 mod blocks;
+mod core;


Module name core shadows the standard library crate

Declaring mod core; in a Rust crate root shadows the built-in core crate for all code in lib.rs. Any future use of core:: within this file will resolve to the local module rather than the standard library, which can cause confusing compilation errors or subtle misbehaviour. A name like endpoints_core, base, or common would sidestep this shadowing entirely.

Prompt To Fix With AI

This is a comment left during a code review. Path: crates/net/rpc/src/lib.rs Line: 13 Comment: **Module name `core` shadows the standard library crate** Declaring `mod core;` in a Rust crate root shadows the built-in `core` crate for all code in `lib.rs`. Any future use of `core::` within this file will resolve to the local module rather than the standard library, which can cause confusing compilation errors or subtle misbehaviour. A name like `endpoints_core`, `base`, or `common` would sidestep this shadowing entirely. How can I resolve this? If you propose a fix, please make it concise.

github-actions · 2026-06-24T17:58:56Z

🤖 Claude Code Review

I have enough to write the review. Here are my findings:

PR Review: `refactor(rpc): compose api router from per-feature route modules`

This is a behavior-preserving refactor that decomposes the monolithic build_api_router in lib.rs into per-module routes() functions and introduces a new core.rs module for the catch-all "foundational" endpoints. The routing logic is functionally equivalent, and the move to Router::merge() is the right axum pattern. One correctness note and a few lower-severity findings below.

Findings

crates/net/rpc/src/core.rs:43 — Stale comment contradicts actual endpoint behavior

// Returns 404 for genesis since it doesn't have a valid signature
match store.get_signed_block(&finalized.root) {
    Some(block) => ssz_response(block.to_ssz()),
    None => axum::http::StatusCode::NOT_FOUND.into_response(),
}

store.get_signed_block now synthesizes a blank proof for slot-0 genesis anchors and always returns Some(...). The None branch (404) is therefore dead code for genesis. The existing test test_get_latest_finalized_block_serves_genesis_with_placeholder_proof (lib.rs:484) explicitly asserts 200, directly contradicting this comment. The comment was already stale in the deleted lib.rs code, but propagating it into the new file makes the mismatch harder to spot. The comment and the dead None arm should reflect the current invariant — or the None arm should be replaced with a clear unreachable!() guarded by a comment explaining the synthesize-always behavior.

crates/net/rpc/src/core.rs:50 — Handler named get_latest_justified_state but returns a Checkpoint

pub(crate) async fn get_latest_justified_state(...) -> impl IntoResponse {
    let checkpoint = store.latest_justified();
    json_response(checkpoint)
}

The function name says "state" but returns a Checkpoint. The endpoint is GET /lean/v0/checkpoints/justified and the behavior is correct, but the handler name is misleading — a maintainer adding a "justified state SSZ" endpoint would naturally collide with this name. Pre-existing, but worth fixing while the file is new. get_latest_justified_checkpoint or get_justified_checkpoint would be accurate.

crates/net/rpc/src/core.rs:15 — GET /lean/v0/blocks/finalized lives in core.rs rather than blocks.rs

// core.rs
.route("/lean/v0/blocks/finalized", get(get_latest_finalized_block))
// blocks.rs
.route("/lean/v0/blocks/{block_id}", get(get_block))
.route("/lean/v0/blocks/{block_id}/header", get(get_block_header))

The /lean/v0/blocks/ namespace is split across two modules. Axum's matchit router correctly prefers the literal /blocks/finalized over the parameterized {block_id} regardless of merge order, so there is no current routing bug. However, the correct merge ordering (core before blocks in lib.rs:99-102) is now a latent invariant that nothing enforces — a future reordering of the .merge() calls would still be safe at the router level but would look suspicious to a reviewer who doesn't know about matchit's guarantee. Moving get_latest_finalized_block and its route registration into blocks.rs collocates all block endpoints and makes the dependency on ordering disappear entirely.

crates/net/rpc/src/core.rs:23,40,50 — Fully-qualified axum::extract::State in handler signatures

pub(crate) async fn get_latest_finalized_state(
    axum::extract::State(store): axum::extract::State<Store>,

The other modules in this PR (blocks.rs, admin.rs) import State at the top and use the short form. Using the full qualified path in core.rs is inconsistent and makes the signatures harder to scan. A use axum::extract::State; import at the top of core.rs would align with the codebase style. The same pattern appears in fork_choice.rs:37 (pre-existing there).

crates/net/rpc/src/core.rs:66 — ssz_response is private to core.rs while json_response is re-exported

// core.rs — private
fn ssz_response(bytes: Vec<u8>) -> axum::response::Response { ... }

// lib.rs — re-exported
pub(crate) use core::json_response;

Both helpers serve the same purpose (building typed HTTP responses). If a future module needs to serve SSZ (e.g. a states-by-root or blocks-by-root endpoint), it will either have to duplicate ssz_response or access crate::core::ssz_response via a private path. Either exporting ssz_response alongside json_response in lib.rs, or keeping both helpers in lib.rs directly, removes this asymmetry.

Automated review by Claude (Anthropic) · sonnet · custom prompt

…eckpoint handler name - Rename `mod core` → `mod base` to avoid shadowing Rust's built-in `core` crate - Fix stale comment on `get_latest_finalized_block`: genesis returns 200 with a synthesized blank proof, not 404 - Rename handler `get_latest_justified_state` → `get_latest_justified_checkpoint` to reflect that it returns a Checkpoint, not a State

Adds a plain `GET /health` liveness probe on the metrics port (5054), alongside `/metrics`. The handler reuses the existing `get_health` function, so the response body and content-type are identical to the API server's `/lean/v0/health`. This lets k8s/ops probe the metrics port independently of the API server. - `GET /health` → 200 `{"status":"healthy","service":"lean-rpc-api"}` with `application/json` - `GET /metrics` unchanged Two unit tests cover the new route and regression-check `/metrics`. Stacked on #454.

Adds the `GET /lean/v0/genesis` endpoint, which returns the genesis time and genesis validator set from the node's configuration. Useful for clients that need to bootstrap their slot clock or verify they are on the correct network. Has unit tests and passed clippy. Stacked on #454.

Adds the `GET /lean/v0/config/spec` endpoint, which exposes chain configuration parameters (slot duration, committee sizes, finality constants, etc.) as JSON. Allows external tools and cross-client integrations to discover runtime config without out-of-band coordination. Has unit tests and passed clippy. Stacked on #454.

Adds two node-info endpoints: - `GET /lean/v0/node/identity` — returns peer ID and ENR. - `GET /lean/v0/node/syncing` — returns current sync status (head slot, finalized slot, sync lag). Both are useful for health dashboards and orchestration tooling. Has unit tests and passed clippy. Stacked on #454.

MegaRedHand mentioned this pull request Jun 24, 2026

feat(rpc): add GET /lean/v0/events SSE stream (head/block/finalized_checkpoint) #460

Draft

greptile-apps Bot reviewed Jun 24, 2026

View reviewed changes

MegaRedHand mentioned this pull request Jun 24, 2026

feat(rpc): add /health endpoint on the metrics server #462

Merged

Merge branch 'main' into feat/lean-api-router-scaffold

6188392

pablodeymo approved these changes Jun 24, 2026

View reviewed changes

pablodeymo merged commit e4c5c74 into main Jun 24, 2026
2 checks passed

pablodeymo deleted the feat/lean-api-router-scaffold branch June 24, 2026 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(rpc): compose api router from per-feature route modules#454

refactor(rpc): compose api router from per-feature route modules#454
pablodeymo merged 3 commits into
mainfrom
feat/lean-api-router-scaffold

MegaRedHand commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

greptile-apps Bot commented Jun 24, 2026

Important Files Changed

Uh oh!

greptile-apps Bot Jun 24, 2026

Uh oh!

greptile-apps Bot Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

MegaRedHand commented Jun 24, 2026

Uh oh!

github-actions Bot commented Jun 24, 2026

🤖 Kimi Code Review

Uh oh!

github-actions Bot commented Jun 24, 2026

🤖 Codex Code Review

Uh oh!

greptile-apps Bot commented Jun 24, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 24, 2026

🤖 Claude Code Review

PR Review: refactor(rpc): compose api router from per-feature route modules

Findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PR Review: `refactor(rpc): compose api router from per-feature route modules`