feat(grpc): implement Read Era Summary and Read Genesis#648
feat(grpc): implement Read Era Summary and Read Genesis#648hrmtm23 wants to merge 2 commits intotxpipe:mainfrom
Conversation
hrmtm23
changed the title
…mestamp calculations
hrmtm23
force-pushed
the
feat/implement-utxorpc-spec-v17
branch
from
e1a0702 to
0ab83df
Compare
WalkthroughRefactors gRPC services to use domain-aware methods for chain-point/block-reference conversion and era-aware timestamp computation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncService
participant Domain
participant Era
Note over SyncService: read_tip flow (new)
Client->>SyncService: read_tip()
SyncService->>SyncService: point_to_blockref(chain_point)
alt Origin
SyncService->>SyncService: timestamp = 0
else Slot/Specific
SyncService->>Domain: get_pparams(up to slot)
Domain-->>SyncService: protocol params
SyncService->>Era: fold pparams -> chain summary
Era-->>SyncService: canonical slot_time
SyncService->>SyncService: timestamp = slot_time as u64
end
SyncService->>Client: ReadTipResponse(tip, timestamp)
sequenceDiagram
participant Client
participant WatchService
participant Domain
participant BlockProcessor
Note over WatchService: watch_tx stream (new)
Client->>WatchService: watch_tx(request)
loop for each TipEvent
WatchService->>WatchService: roll_to_watch_response(domain, mapper, log, request)
WatchService->>BlockProcessor: block_to_txs(block, domain, mapper, request)
BlockProcessor->>Domain: get protocol params & era summary
Domain-->>BlockProcessor: params + era context
BlockProcessor->>BlockProcessor: compute block timestamp (era-aware)
BlockProcessor-->>WatchService: AnyChainTx[] (header + timestamp)
WatchService-->>Client: WatchTxResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/serve/grpc/sync.rs (3)
43-57: Minor: double-decode to get height; consider single source of truth.You decode MultiEraBlock solely to read number() after already mapping the block header. If the mapped header exposes the height, use it; otherwise, consider decoding only when header is present to avoid redundant work. Not a blocker.
112-143: Two BlockRef helpers diverge; unify to avoid inconsistent height.You now have:
- free fn point_to_blockref (Line 60) → height defaulted via Default::default()
- method self.point_to_blockref(...) (Lines 112-143) → best-effort real height
Tip reset events still call the free fn, yielding height=0 while other paths provide real height. Unify by removing the free fn and routing all call sites through the method (e.g., make tip_event_to_response a method or pass a closure).
261-285: Timestamp logic OK; extract shared helper to de-duplicate.Same era-aware flow appears in query.rs and watch.rs. Extract a small helper, e.g., fn slot_timestamp<D: Domain + LedgerContext>(domain: &D, slot: u64) -> u64, and reuse.
src/serve/grpc/query.rs (1)
480-507: De-duplicate era-aware pparams folding and timestamp/era derivation.The update→fold_with_hacks→era_for_slot flow is duplicated here and elsewhere. Extract a shared utility and reuse to reduce complexity and drift.
Also applies to: 546-566
src/serve/grpc/watch.rs (1)
164-186: LGTM: era‑aware timestamp derivation. Consider centralizing helper.Same logic appears in sync/query; extract a shared slot_timestamp helper to keep behavior consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/serve/grpc/query.rs(7 hunks)src/serve/grpc/sync.rs(3 hunks)src/serve/grpc/watch.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/serve/grpc/sync.rs (2)
crates/core/src/point.rs (1)
slot(16-22)crates/cardano/src/eras.rs (2)
slot_time(19-23)slot_time(68-71)
src/serve/grpc/watch.rs (3)
src/serve/grpc/query.rs (4)
dolos_cardano(302-302)updates(75-77)updates(496-499)updates(554-557)src/serve/grpc/sync.rs (2)
updates(272-274)stream(237-237)crates/cardano/src/eras.rs (2)
slot_time(19-23)slot_time(68-71)
src/serve/grpc/query.rs (4)
crates/core/src/lib.rs (5)
network_magic(256-261)point(501-505)slot(497-497)genesis(710-710)hash(498-498)crates/core/src/point.rs (2)
slot(16-22)hash(24-29)src/serve/grpc/sync.rs (1)
updates(272-274)crates/cardano/src/eras.rs (2)
slot_time(19-23)slot_time(68-71)
🔇 Additional comments (7)
src/serve/grpc/sync.rs (1)
287-290: LGTM: tip now includes domain-aware height via self.point_to_blockref.src/serve/grpc/query.rs (4)
315-315: LGTM: ledger_tip via self.point_to_u5c for read_params.
365-366: LGTM: ledger_tip via self.point_to_u5c for read_utxos.
453-454: LGTM: ledger_tip via self.point_to_u5c for read_tx.
570-575: LGTM: era summaries mapped through mapper with current params.src/serve/grpc/watch.rs (2)
212-220: LGTM: domain-aware plumbing for roll_to_watch_response.
294-295: LGTM: stream composition with flat_map and cloned domain.
| /// Get the CAIP-2 blockchain identifier for the given network magic | ||
| fn get_caip2_identifier(network_magic: u32) -> String { | ||
| let network_magic = network_magic as u64; | ||
| match network_magic { | ||
| MAINNET_MAGIC => format!("cardano-mainnet:{}", network_magic), | ||
| TESTNET_MAGIC => format!("cardano-testnet:{}", network_magic), | ||
| PREVIEW_MAGIC => format!("cardano-preview:{}", network_magic), | ||
| PRE_PRODUCTION_MAGIC => format!("cardano-preprod:{}", network_magic), | ||
| _ => format!("cardano:{}", network_magic), // fallback for unknown networks | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm CAIP‑2 identifier format.
Mapping to strings like "cardano-mainnet:{magic}" may not match CAIP‑2 conventions or UTxO‑RPC v1.7 expectations. Please verify the exact namespace/reference format required by the spec and adjust accordingly.
🌐 Web query:
What CAIP-2 chain ID format should Cardano networks (mainnet, preprod, preview, testnet) use in UTxO-RPC v1.7’s ReadGenesisResponse.caip2 field? Provide examples and authoritative source.
💡 Result:
Use a CAIP‑2 chain id of the form : (CAIP‑2). For Cardano networks the canonical namespace is "cardano" and the human-readable references are the network names. Examples for ReadGenesisResponse.caip2 (UTxO‑RPC v1.7) are:
- cardano:mainnet
- cardano:preprod
- cardano:preview
- cardano:testnet
Authoritative reference: the CAIP‑2 specification (chain id syntax: namespace:reference). [1]
Sources
- CAIP‑2: Blockchain ID Specification (ChainAgnostic). [1]
Fix CAIP-2 identifier format to match UTxO-RPC v1.7 spec.
The current format includes the network magic number and incorrect namespace structure. CAIP-2 specifies the format as <namespace>:<reference>, with Cardano using namespace "cardano" and references as network names. Update the format strings to:
MAINNET_MAGIC => "cardano:mainnet",
TESTNET_MAGIC => "cardano:testnet",
PREVIEW_MAGIC => "cardano:preview",
PRE_PRODUCTION_MAGIC => "cardano:preprod",
_ => format!("cardano:{}", network_magic), // fallback using magic as reference
Remove the network magic from mainnet/testnet/preview/preprod identifiers; only include it in the fallback for unknown networks.
🤖 Prompt for AI Agents
In src/serve/grpc/query.rs around lines 13 to 22, the CAIP-2 identifiers are
currently formatted with the network magic number and incorrect namespace
structure; update the match arms so known networks return the canonical CAIP-2
references (use "cardano:mainnet", "cardano:testnet", "cardano:preview",
"cardano:preprod") and only use the network_magic value in the fallback (e.g.,
format!("cardano:{}", network_magic)) so the mainnet/testnet/preview/preprod
cases do not include the magic number.
src/serve/grpc/query.rs
Outdated
| fn point_to_u5c(&self, point: &ChainPoint) -> u5c::query::ChainPoint { | ||
| match point { | ||
| ChainPoint::Origin => u5c::query::ChainPoint { | ||
| slot: 0, | ||
| hash: vec![].into(), | ||
| height: 0, | ||
| timestamp: 0, | ||
| }, | ||
| ChainPoint::Slot(slot) | ChainPoint::Specific(slot, _) => { | ||
| // Calculate height by looking up block from slot | ||
| let height = self.domain | ||
| .archive() | ||
| .get_block_by_slot(slot) | ||
| .map(|block| { | ||
| block.map(|body| { | ||
| // Parse the block to get the height | ||
| use pallas::ledger::traverse::MultiEraBlock; | ||
| if let Ok(parsed_block) = MultiEraBlock::decode(&body) { | ||
| parsed_block.number() | ||
| } else { | ||
| *slot // Fallback to slot | ||
| } | ||
| }).unwrap_or(*slot) | ||
| }) | ||
| .unwrap_or(*slot); | ||
|
|
||
| // Calculate timestamp from slot using proper era handling | ||
| // Get protocol parameter updates up to this slot | ||
| let updates = self.domain.state() | ||
| .get_pparams(*slot) | ||
| .ok() | ||
| .and_then(|updates| { | ||
| updates.into_iter() | ||
| .map(TryInto::try_into) | ||
| .collect::<Result<Vec<_>, _>>() | ||
| .ok() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| // Get chain summary with proper era handling | ||
| let summary = pparams::fold_with_hacks(self.domain.genesis(), &updates, *slot); | ||
|
|
||
| // Calculate timestamp using the canonical function | ||
| let timestamp = dolos_cardano::slot_time(*slot, &summary) as u64; | ||
|
|
||
| u5c::query::ChainPoint { | ||
| slot: *slot, | ||
| hash: point.hash().map(|h| h.to_vec()).unwrap_or_default().into(), | ||
| height, | ||
| timestamp, | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Build break: pparams used without import.
pparams::fold_with_hacks is referenced but dolos_cardano::pparams isn’t imported in this file, causing a compile error. Add the import once near the top.
Apply this diff:
@@
use super::masking::apply_mask;
use crate::prelude::*;
+use dolos_cardano::pparams;
use pallas::ledger::traverse::wellknown::{MAINNET_MAGIC, TESTNET_MAGIC, PREVIEW_MAGIC, PRE_PRODUCTION_MAGIC};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/serve/grpc/query.rs around lines 43 to 96, the code references
pparams::fold_with_hacks but never imports dolos_cardano::pparams, causing a
compile error; add a single use import near the top of the file (e.g., use
dolos_cardano::pparams;) so pparams::fold_with_hacks resolves, then run cargo
build to verify the file compiles.
| let parsed_block = MultiEraBlock::decode(block).unwrap(); | ||
| let txs = parsed_block.txs(); | ||
|
|
There was a problem hiding this comment.
Panic risk: unwrap on block decode in hot path.
MultiEraBlock::decode(block).unwrap() can panic on malformed data and take down the stream/server. Handle the error and skip/propagate safely.
Apply this diff:
- let parsed_block = MultiEraBlock::decode(block).unwrap();
+ let parsed_block = match MultiEraBlock::decode(block) {
+ Ok(b) => b,
+ Err(e) => {
+ tracing::warn!(error = %e, "failed to decode block in watch; skipping txs");
+ return Vec::new();
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let parsed_block = MultiEraBlock::decode(block).unwrap(); | |
| let txs = parsed_block.txs(); | |
| let parsed_block = match MultiEraBlock::decode(block) { | |
| Ok(b) => b, | |
| Err(e) => { | |
| tracing::warn!(error = %e, "failed to decode block in watch; skipping txs"); | |
| return Vec::new(); | |
| } | |
| }; | |
| let txs = parsed_block.txs(); | |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/serve/grpc/query.rs (2)
13-22: CAIP-2 format remains incorrect—remove magic from known networks.This critical issue was flagged in a previous review but not yet addressed. The format should be
cardano:mainnet,cardano:testnet, etc., without the magic number suffix for known networks. Only the fallback for unknown networks should include the magic.Apply this diff:
fn get_caip2_identifier(network_magic: u32) -> String { let network_magic = network_magic as u64; match network_magic { - MAINNET_MAGIC => format!("cardano-mainnet:{}", network_magic), - TESTNET_MAGIC => format!("cardano-testnet:{}", network_magic), - PREVIEW_MAGIC => format!("cardano-preview:{}", network_magic), - PRE_PRODUCTION_MAGIC => format!("cardano-preprod:{}", network_magic), + MAINNET_MAGIC => "cardano:mainnet".to_string(), + TESTNET_MAGIC => "cardano:testnet".to_string(), + PREVIEW_MAGIC => "cardano:preview".to_string(), + PRE_PRODUCTION_MAGIC => "cardano:preprod".to_string(), _ => format!("cardano:{}", network_magic), // fallback for unknown networks } }
43-94: Build break: pparams module not imported.This critical issue was flagged in a previous review but remains unaddressed. Line 81 references
pparams::fold_with_hacksbutdolos_cardano::pparamsis not imported, causing a compilation error.Add the import near the top of the file:
use super::masking::apply_mask; use crate::prelude::*; +use dolos_cardano::pparams; use pallas::ledger::traverse::wellknown::{MAINNET_MAGIC, TESTNET_MAGIC, PREVIEW_MAGIC, PRE_PRODUCTION_MAGIC};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/serve/grpc/query.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/serve/grpc/query.rs (5)
crates/core/src/lib.rs (4)
network_magic(256-261)point(501-505)slot(497-497)genesis(710-710)crates/core/src/point.rs (1)
slot(16-22)src/serve/grpc/sync.rs (1)
updates(272-274)src/adapters.rs (1)
genesis(61-63)crates/cardano/src/eras.rs (2)
slot_time(19-23)slot_time(68-71)
🔇 Additional comments (3)
src/serve/grpc/query.rs (3)
312-312: LGTM—consistent use of the new method.The migration from the old standalone function to
self.point_to_u5cis correctly implemented across all query handlers, with proper error propagation.Also applies to: 363-364, 411-412, 453-454
519-519: Genesis hash correctly sourced.Line 519 now properly uses
genesis.shelley_hash.to_vec()instead of deriving from the cursor, matching the canonical genesis identifier for Cardano.
490-508: Consider failing when tip is unavailable.When no tip exists,
current_paramssilently falls back toNone, potentially masking initialization issues. Per a previous review comment, consider returningStatus::internalorStatus::unavailableinstead of proceeding with incomplete data, especially since genesis configuration should be static and always available.Run the following to check if a missing tip represents a valid state:
| let current_params = if let Some(tip_point) = tip.as_ref() { | ||
| let updates = self | ||
| .domain | ||
| .state() | ||
| .get_pparams(tip_point.slot()) | ||
| .map_err(into_status)?; | ||
|
|
||
| let updates: Vec<_> = updates | ||
| .into_iter() | ||
| .map(TryInto::try_into) | ||
| .try_collect::<_, _, pallas::codec::minicbor::decode::Error>() | ||
| .map_err(|e| Status::internal(e.to_string()))?; | ||
|
|
||
| let summary = pparams::fold_with_hacks(genesis, &updates, tip_point.slot()); | ||
| let era = summary.era_for_slot(tip_point.slot()); | ||
| Some(era.pparams.clone()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
Consider failing when tip is unavailable.
Similar to read_genesis, this handler silently sets current_params to None when the tip is unavailable. Era summaries are structural chain metadata that should always be available. Consider returning an error status instead of proceeding with incomplete parameters.
🤖 Prompt for AI Agents
In src/serve/grpc/query.rs around lines 540 to 558, the code currently sets
current_params to None when tip is unavailable; change this to return an
Err(Status::internal(...)) (or Status::not_found(...) per project's convention)
so the RPC fails instead of proceeding with incomplete era parameters.
Concretely, check if tip.is_none() and return an appropriate Status error with a
clear message (e.g., "tip/era summary unavailable") before attempting to derive
pparams; keep the rest of the existing pparams extraction logic unchanged.
scarmuega
force-pushed
the
main
branch
2 times, most recently
from
295b558 to
09d0b81
Compare
gonzalezzfelipe
left a comment
gonzalezzfelipe
left a comment
There was a problem hiding this comment.
@hrmtm23 its looking good, sory for the delayed response. Some of the APIs changed a bit but the functionality you are using is still available. Could you rebase and handle the conflict resolution?
2 participants
Summary by CodeRabbit
New Features
Bug Fixes