feat(rpc): add GET /lean/v0/attestations for validator participation#459
feat(rpc): add GET /lean/v0/attestations for validator participation#459MegaRedHand wants to merge 3 commits into
Conversation
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.
🤖 Kimi Code ReviewReview of PR #459 (Attestations RPC endpoint): Potential Compatibility Issue
Performance/DoS Concerns
Code Quality
Testing
Security
Consensus Correctness
Recommendation: Address the MSRV issue and add pagination before merging if this targets production networks. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
No consensus-critical logic, STF, fork choice, XMSS, or SSZ code changed in this PR beyond this observability endpoint. I couldn’t run the crate tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR adds
Confidence Score: 3/5The endpoint is additive and read-only, so it cannot corrupt state, but it ships with a documented filtering capability that simply does not work. The validator-index filter is called out in the PR description as a supported feature but is entirely absent from the implementation — any client relying on it today receives unfiltered data without any indication of the problem. This is a present gap in the delivered interface, not a speculative future concern. crates/net/rpc/src/attestations.rs — the query struct, response shape, and test coverage all need attention before this endpoint matches its stated contract.
|
| Filename | Overview |
|---|---|
| crates/net/rpc/src/attestations.rs | New endpoint GET /lean/v0/attestations; missing the advertised validator_index query filter, checkpoint roots are dropped from the response, and tests only cover the empty-store happy path. |
| crates/net/rpc/src/lib.rs | Wires the new attestations module into the API router via a single .merge(attestations::routes()) call; straightforward, no issues. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant Router as Axum Router
participant Handler as get_attestations
participant Store
Client->>Router: "GET /lean/v0/attestations[?slot=N]"
Router->>Handler: "Query(AttQuery { slot }), State(store)"
Handler->>Store: extract_latest_known_attestations()
Store-->>Handler: "HashMap<validator_index, AttestationData>"
Handler->>Handler: filter by slot (if present)
Handler->>Handler: "map to AttestationEntry { validator_index, slot, source_slot, target_slot }"
Handler->>Handler: sort by validator_index
Handler-->>Client: 200 JSON array
%%{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"}}}%%
sequenceDiagram
participant Client
participant Router as Axum Router
participant Handler as get_attestations
participant Store
Client->>Router: "GET /lean/v0/attestations[?slot=N]"
Router->>Handler: "Query(AttQuery { slot }), State(store)"
Handler->>Store: extract_latest_known_attestations()
Store-->>Handler: "HashMap<validator_index, AttestationData>"
Handler->>Handler: filter by slot (if present)
Handler->>Handler: "map to AttestationEntry { validator_index, slot, source_slot, target_slot }"
Handler->>Handler: sort by validator_index
Handler-->>Client: 200 JSON array
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/net/rpc/src/attestations.rs:12-14
**Validator-index filter is missing despite being advertised**
The PR description says the endpoint is "Optionally filterable by slot or validator index", and the handler docstring echoes this, but `AttQuery` only contains `slot`. Any client that sends `?validator_index=42` will silently receive the full unfiltered list instead of an error or a narrowed result. This is a concrete misalignment between the documented interface and the implementation. A `validator_index: Option<u64>` field should be added to `AttQuery` and applied as an additional `.filter` step alongside the slot check.
### Issue 2 of 3
crates/net/rpc/src/attestations.rs:18-24
**Checkpoint roots are dropped; only slots are returned**
`AttestationData` carries full `Checkpoint` values (both `slot` and `root`) for `source`, `target`, and `head`. `AttestationEntry` exposes only `source_slot` and `target_slot` and omits `head` entirely. Explorer tooling diagnosing finality stalls needs to correlate checkpoints against specific block roots, not just slot numbers. Consider including at least `source_root`, `target_root`, and the `head` checkpoint so consumers can cross-reference with `/lean/v0/blocks/{root}`.
### Issue 3 of 3
crates/net/rpc/src/attestations.rs:48-76
**Tests do not cover filtering or non-empty payloads**
The single test verifies only that the endpoint returns HTTP 200 with a JSON array against an empty store, so every meaningful code path — slot filtering, the sort, and the mapping of `source_slot`/`target_slot` — is exercised only in production. A test that inserts at least two attestations with different slots and then requests `?slot=<n>` would catch any filtering regression without requiring much extra setup.
Reviews (1): Last reviewed commit: "feat(rpc): add GET /lean/v0/attestations..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[derive(Deserialize)] | ||
| struct AttQuery { | ||
| slot: Option<u64>, |
There was a problem hiding this comment.
The PR description says the endpoint is "Optionally filterable by slot or validator index", and the handler docstring echoes this, but AttQuery only contains slot. Any client that sends ?validator_index=42 will silently receive the full unfiltered list instead of an error or a narrowed result. This is a concrete misalignment between the documented interface and the implementation. A validator_index: Option<u64> field should be added to AttQuery and applied as an additional .filter step alongside the slot check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/attestations.rs
Line: 12-14
Comment:
**Validator-index filter is missing despite being advertised**
The PR description says the endpoint is "Optionally filterable by slot or validator index", and the handler docstring echoes this, but `AttQuery` only contains `slot`. Any client that sends `?validator_index=42` will silently receive the full unfiltered list instead of an error or a narrowed result. This is a concrete misalignment between the documented interface and the implementation. A `validator_index: Option<u64>` field should be added to `AttQuery` and applied as an additional `.filter` step alongside the slot check.
How can I resolve this? If you propose a fix, please make it concise.| struct AttestationEntry { | ||
| validator_index: u64, | ||
| slot: u64, | ||
| source_slot: u64, | ||
| target_slot: u64, | ||
| } | ||
|
|
There was a problem hiding this comment.
AttestationData carries full Checkpoint values (both slot and root) for source, target, and head. AttestationEntry exposes only source_slot and target_slot and omits head entirely. Explorer tooling diagnosing finality stalls needs to correlate checkpoints against specific block roots, not just slot numbers. Consider including at least source_root, target_root, and the head checkpoint so consumers can cross-reference with /lean/v0/blocks/{root}.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/attestations.rs
Line: 18-24
Comment:
**Checkpoint roots are dropped; only slots are returned**
`AttestationData` carries full `Checkpoint` values (both `slot` and `root`) for `source`, `target`, and `head`. `AttestationEntry` exposes only `source_slot` and `target_slot` and omits `head` entirely. Explorer tooling diagnosing finality stalls needs to correlate checkpoints against specific block roots, not just slot numbers. Consider including at least `source_root`, `target_root`, and the `head` checkpoint so consumers can cross-reference with `/lean/v0/blocks/{root}`.
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!
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::test_utils::create_test_state; | ||
| use axum::{ | ||
| body::Body, | ||
| http::{Request, StatusCode}, | ||
| }; | ||
| use ethlambda_storage::{Store, backend::InMemoryBackend}; | ||
| use http_body_util::BodyExt; | ||
| use std::sync::Arc; | ||
| use tower::ServiceExt; | ||
|
|
||
| #[tokio::test] | ||
| async fn attestations_returns_array() { | ||
| let store = Store::from_anchor_state(Arc::new(InMemoryBackend::new()), create_test_state()); | ||
| let app = crate::build_api_router(store); | ||
| let resp = app | ||
| .oneshot( | ||
| Request::builder() | ||
| .uri("/lean/v0/attestations") | ||
| .body(Body::empty()) | ||
| .unwrap(), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status(), StatusCode::OK); | ||
| let body = resp.into_body().collect().await.unwrap().to_bytes(); | ||
| let json: serde_json::Value = serde_json::from_slice(&body).unwrap(); | ||
| assert!(json.is_array()); |
There was a problem hiding this comment.
The single test verifies only that the endpoint returns HTTP 200 with a JSON array against an empty store, so every meaningful code path — slot filtering, the sort, and the mapping of source_slot/target_slot — is exercised only in production. A test that inserts at least two attestations with different slots and then requests ?slot=<n> would catch any filtering regression without requiring much extra setup.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/net/rpc/src/attestations.rs
Line: 48-76
Comment:
**Tests do not cover filtering or non-empty payloads**
The single test verifies only that the endpoint returns HTTP 200 with a JSON array against an empty store, so every meaningful code path — slot filtering, the sort, and the mapping of `source_slot`/`target_slot` — is exercised only in production. A test that inserts at least two attestations with different slots and then requests `?slot=<n>` would catch any filtering regression without requiring much extra setup.
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!
🤖 Claude Code ReviewNow I have all the data I need. Let me write the final review. PR Review:
|
…lter, JSON errors, tests)
- Add `?validator_index=N` filter to GET /lean/v0/attestations (combinable with `?slot=`)
- Document that `?slot=` filters over latest-only attestations, not all historical ones
- Bad query params now return JSON {"error":"..."} + 400 instead of Axum's plain-text 422
- Add 5 real tests: seeded entries with field assertions, slot filter, validator_index
filter, combined filter, and bad-param JSON 400 contract
1 participant
Adds
GET /lean/v0/attestations, which returns the currently known (fork-choice-active) attestations. Optionally filterable by slot or validator index.Enables explorer tooling to surface validator participation rates and diagnose finality stalls. Has unit tests and passed clippy.
Stacked on #454.