Conversation
There was a problem hiding this comment.
Code Review
This pull request expands the Espresso API by implementing the Data and Consensus APIs for v2 and the Availability API for v1. These changes introduce endpoints for retrieving namespace proofs, incorrect encoding proofs, state certificates, and stake tables, along with their corresponding proto definitions and Axum routing logic. The review feedback identifies a critical recurring logic error across several new handlers in crates/espresso/node/src/api/state.rs, where the join! macro is used twice on the same set of futures. This pattern is invalid as the first join! resolves the futures, causing subsequent calls to with_timeout or a second join! to fail during compilation.
|
Unable to build without Cargo.lock file. This means that after this change 3rd party projects may have For the first failing build see: https://github.com/EspressoSystems/espresso-network/actions/runs/24975975015 To reproduce locally run This PR can still be merged. |
| .await | ||
| .map(Json) | ||
| .map_err(ApiError::Internal) | ||
| }; |
There was a problem hiding this comment.
this part here feels a bit verbose and could probably be condensed. exploring options in the next PR.
|
|
||
| // Fetch block and VID common data | ||
| let ds = &*self.data_source; | ||
| let timeout = std::time::Duration::from_millis(500); |
There was a problem hiding this comment.
this seems a bit weird, but the existing incorrect_encoding_proof path uses get_block_for_ns_proof which (I believe) does essentially the same thing
| })?; | ||
|
|
||
| // Store the fetched certificate | ||
| ds.insert_state_cert(epoch, cert.clone()).await?; |
There was a problem hiding this comment.
this mirrors the existing get_state_cert in espresso/node/src/api/endpoints/availability.rs, though maybe we should revisit the behaviour
| /// Get namespace proof by block height | ||
| /// Path: GET /v1/availability/namespace/{namespace}/block/{height} | ||
| pub const NAMESPACE_PROOF_BY_HEIGHT_ROUTE: &str = | ||
| "/v1/availability/namespace/{namespace}/block/{height}"; |
There was a problem hiding this comment.
Is the change of parameter order intentional?
espresso-network/crates/espresso/node/api/availability.toml
Lines 1 to 10 in 178357d
There was a problem hiding this comment.
no definitely not, very good catch!
There was a problem hiding this comment.
added tests to ensure this doesn't happen: 9d917b1
| .to_string(); | ||
|
|
||
| Ok(v2::Transaction { | ||
| namespace: tx.namespace.0 as u32, |
There was a problem hiding this comment.
It seems we have namespace ID as u64 and u32 in different places. Maybe a comment why this is fine here?
There was a problem hiding this comment.
I think u32 was how the old Tide API was casting it in some places. But u64 is the internal type, and I think this is actually better to just make a u64 for consistency (especially since it only eliminates conversions)
fixed: 6ad876c
| Ok(proofs) | ||
| } | ||
|
|
||
| async fn get_incorrect_encoding_proof( |
There was a problem hiding this comment.
I'm confused. This function always returns an error.
There was a problem hiding this comment.
oops, fixed: 9864160
the code is a bit weird, but I think this follows the existing behaviour around line 413 in crates/espresso/node/src/api/endpoints/availability.rs. unfortunately I think this specific endpoint is hard to add to the test, we don't have any existing tests for it and I'm not sure if anyone is using this endpoint anyway
|
This PR adds the axum migration for the espresso node availability API overlay but most of the availability API is from the hotshot query service. I think we can merge this first but just wanted to mention it, it kind of confused me at first. |
|
|
||
| // IP address (v4 or v6) with port | ||
| message InetAddr { | ||
| string ip = 1; // IP address string (e.g., "127.0.0.1" or "::1") |
There was a problem hiding this comment.
Are we expecting to be able to concatenate the port and the ip without inspecting the ip value itself?
If so, then we may actually want to store [::1] instead of just ::1 for convenience.
sveitser
left a comment
There was a problem hiding this comment.
I think this is fine but what about the query service availability API? The comment I left here: #4216 (comment)
Nextest failures (1) in this run
See the step summary for flaky tests and slowest tests. |
This PR migrates the
availabilityendpoint to Axum, with a copy of the old endpoint served atv1/availabilityand newer protobuf-specified endpoints served atv2/dataandv2/consensus.You can skip
test_api.rs(this is just to render an example UI without having to compileespresso-node) andespresso.api.v2.rs(this is automatically generated).