feat(utxorpc): implement read_genesis query#964
Merged
scarmuega merged 1 commit intotxpipe:mainfrom Apr 24, 2026
Merged
Conversation
📝 WalkthroughWalkthroughImplements Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPC_Server as Server
participant DomainStore as DomainGenesis/Eras
participant Mapper
participant FieldMasker as MaskApplier
Client->>Server: ReadGenesis / ReadEraSummary request (optional field_mask)
Server->>DomainStore: load genesis and era summaries (and protocol params)
DomainStore-->>Server: domain genesis + eras + params
Server->>Mapper: map domain structures -> Cardano protobuf types
Mapper-->>Server: populated Cardano structures
Server->>FieldMasker: apply optional field_mask
FieldMasker-->>Server: masked response
Server-->>Client: ReadGenesisResponse / ReadEraSummaryResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds the missing read_genesis() gRPC query implementation for the UTxO RPC QueryService so downstream consumers (e.g., konduit) can fetch genesis metadata/config via the service.
Changes:
- Added mapping helpers to translate Dolos
Genesis(Byron/Shelley/Alonzo/Conway) intou5c::cardano::Genesis(+ protocol params, cost models, prices, voting thresholds, etc.). - Implemented
QueryService::read_genesiswith optional field-mask support. - Added unit/integration-style tests covering CAIP-2 mapping, representative mapping, field masks, and the read_genesis response shape.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AndrewWestberg
force-pushed
the
amw/grpc_genesis
branch
from
105b4db to
7f5b176
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/serve/grpc/query.rs (1)
220-225: Potential truncation when castingu64toi32/u32.The casts from
u64toi32andu32will silently truncate if values exceed their respective maximums. While current genesis values likely fit, this could cause subtle bugs if larger values are ever used.Consider adding overflow checks or using
try_fromwith error handling:♻️ Optional: Add overflow-safe conversion
-fn rational_to_u5c(numerator: u64, denominator: u64) -> u5c::cardano::RationalNumber { - u5c::cardano::RationalNumber { - numerator: numerator as i32, - denominator: denominator as u32, - } +fn rational_to_u5c(numerator: u64, denominator: u64) -> Result<u5c::cardano::RationalNumber, Status> { + Ok(u5c::cardano::RationalNumber { + numerator: i32::try_from(numerator) + .map_err(|_| Status::internal("rational numerator overflow"))?, + denominator: u32::try_from(denominator) + .map_err(|_| Status::internal("rational denominator overflow"))?, + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serve/grpc/query.rs` around lines 220 - 225, The cast in rational_to_u5c silently truncates u64 -> i32/u32; change rational_to_u5c to perform overflow-safe conversions (use i32::try_from(numerator) and u32::try_from(denominator)) and return a Result<u5c::cardano::RationalNumber, YourErrorType> (or map the TryFrom errors into an existing error type) so callers can handle out-of-range values; update callers of rational_to_u5c to propagate or handle the Result and keep the same u5c::cardano::RationalNumber construction only after successful conversions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/serve/grpc/query.rs`:
- Around line 220-225: The cast in rational_to_u5c silently truncates u64 ->
i32/u32; change rational_to_u5c to perform overflow-safe conversions (use
i32::try_from(numerator) and u32::try_from(denominator)) and return a
Result<u5c::cardano::RationalNumber, YourErrorType> (or map the TryFrom errors
into an existing error type) so callers can handle out-of-range values; update
callers of rational_to_u5c to propagate or handle the Result and keep the same
u5c::cardano::RationalNumber construction only after successful conversions.
7f5b176 to
5250f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/serve/grpc/query.rs`:
- Around line 413-423: The current decoding of
genesis.conway.constitution.anchor.data_hash and the optional
constitution.script uses hex::decode but doesn’t enforce a fixed byte length,
allowing short/long hashes; update the creation of constitution_anchor_hash and
constitution_hash to decode and then validate the resulting Vec<u8> has the
expected fixed length (e.g., 32 bytes or your project's CONTENT_HASH_LEN
constant), returning a Status::internal (or appropriate gRPC Status) with a
clear message if the length is wrong; apply the same validation path for the
optional script (constitution.script) after the map/transpose so the
unwrap_or_default path stays correct but invalid-length hex returns an error.
- Around line 401-408: caip2_from_genesis currently returns Status::internal
when genesis.shelley.network_magic is None; instead treat None as 0 (consistent
with Genesis::network_magic normalization and map_cardano_genesis) so
read_genesis can succeed. Modify caip2_from_genesis to map None to "cardano:0"
(or handle None like Some(0)) and return Ok(format!("cardano:{x}")) accordingly;
update the match arms in the caip2_from_genesis function so missing
network_magic is serialized the same way as map_cardano_genesis.
- Around line 220-224: The helper rational_to_u5c must not silently cast u64 to
i32/u32; change rational_to_u5c to return a Result<u5c::cardano::RationalNumber,
E> and validate that numerator fits in i32 and denominator fits in u32 (return
Err if out of range), then update float_to_u5c_rational,
map_genesis_protocol_params, and map_cardano_genesis to propagate and handle
that Result (adjust their return types to Result where needed and bubble up the
error) so no unchecked truncation occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+220
to
+224
| fn rational_to_u5c(numerator: u64, denominator: u64) -> u5c::cardano::RationalNumber { | ||
| u5c::cardano::RationalNumber { | ||
| numerator: numerator as i32, | ||
| denominator: denominator as u32, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\nUnchecked casts in src/serve/grpc/query.rs:\n'
rg -n -C2 'fn rational_to_u5c|numerator: numerator as i32|denominator: denominator as u32' src/serve/grpc/query.rs
printf '\nRationalNumber definition in the repo:\n'
rg -n -C2 'struct RationalNumber|message RationalNumber|numerator: .*i32|denominator: .*u32' .Repository: txpipe/dolos
Length of output: 771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if cargo clippy warns about these unchecked casts
printf '\n=== Running cargo clippy on src/serve/grpc/query.rs ===\n'
cargo clippy --all-targets --all-features -- -W clippy::all 2>&1 | grep -A3 -B3 "rational_to_u5c\|query\.rs:22" || echo "No clippy warnings found for these lines"
# Find the protobuf definition of RationalNumber
printf '\n=== RationalNumber protobuf definition ===\n'
find . -name "*.proto" -o -name "*.rs" | xargs grep -l "RationalNumber" | head -5
# Look for RationalNumber definition in proto files or rust definitions
printf '\n=== Searching for RationalNumber message/struct definition ===\n'
rg -n "message RationalNumber|struct RationalNumber" --type proto --type rust -A5 2>/dev/null || rg -n "RationalNumber" --type proto -A5 2>/dev/null || true
# Check call sites of rational_to_u5c to understand usage
printf '\n=== Call sites of rational_to_u5c ===\n'
rg -n "rational_to_u5c" src/serve/grpc/query.rs
# Check what values are passed to this function
printf '\n=== Context around rational_to_u5c calls ===\n'
rg -n -B5 -A1 "rational_to_u5c" src/serve/grpc/query.rs | head -60Repository: txpipe/dolos
Length of output: 3459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for proto files or u5c definitions
printf '\n=== Searching for u5c_proto or .proto files ===\n'
find . -name "*.proto" | head -20
find . -type d -name "*u5c*" -o -name "*proto*" | head -20
# Check if there's a generated rust file from protobuf
printf '\n=== Looking for generated protobuf rust code ===\n'
find . -type f -name "*.rs" | xargs grep -l "u5c::cardano::RationalNumber" | head -5
# Find where u5c is defined or imported
printf '\n=== u5c crate definition ===\n'
rg "^u5c = " --type toml
# Check Cargo.toml for u5c dependency
printf '\n=== u5c in Cargo.toml ===\n'
rg "u5c" Cargo.toml
# Look for the actual definition in the u5c module/crate
printf '\n=== Examining u5c::cardano module ===\n'
find . -path "*u5c*cardano*" -name "*.rs" | head -10
# Try to find prost definitions or generated files
printf '\n=== Looking for prost-generated or u5c generated code ===\n'
find . -name "*.rs" | xargs grep -l "pub struct RationalNumber" | head -5Repository: txpipe/dolos
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find u5c in dependency files
printf '\n=== u5c dependency info ===\n'
grep -r "u5c" Cargo.lock | head -20
# Look at actual usage of the result to see if types matter
printf '\n=== Type signatures and results of rational_to_u5c ===\n'
rg -B2 "Some\(rational_to_u5c" src/serve/grpc/query.rs | head -30
# Check if float_to_rational produces constrained values
printf '\n=== float_to_rational implementation ===\n'
rg -n "fn float_to_rational" -A10
# Look at the pallas types being converted
printf '\n=== Pallas RationalNumber/ratio types ===\n'
rg -n "numerator.*denominator|struct.*Price|ExecutionPrices" --type rust | grep -E "numerator|denominator" | head -20Repository: txpipe/dolos
Length of output: 84
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
# Check workspace crates
printf '\n=== Workspace crates ===\n'
find . -maxdepth 2 -name "Cargo.toml" -type f | xargs grep -l "\[package\]" | head -20
# Check if u5c is a local workspace crate
printf '\n=== Looking for u5c as local crate ===\n'
find . -maxdepth 3 -path "*/u5c*" -type d
# List what crates exist
printf '\n=== Crates directory ===\n'
ls -la crates/ 2>/dev/null || echo "No crates directory"
# Check the main Cargo.toml for u5c
printf '\n=== Main Cargo.toml ===\n'
head -50 Cargo.toml
# Look at the input types: pallas numerator/denominator field types
printf '\n=== Pallas field types for numerator/denominator ===\n'
rg "numerator.*:" -t rust | grep -E "(u64|u32|i32|i64)" | head -20
# Check if inputs could realistically exceed i32::MAX or u32::MAX
printf '\n=== Genesis shelley a0/rho/tau types ===\n'
rg "a0:|rho:|tau:" crates/ -t rust -A1 | head -30Repository: txpipe/dolos
Length of output: 3681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where u5c is imported from
printf '\n=== u5c imports ===\n'
rg "use u5c" -t rust | head -20
# Check dolos-core dependencies for u5c
printf '\n=== dolos-core Cargo.toml ===\n'
cat crates/core/Cargo.toml | grep -A20 "\[dependencies\]" | head -30
# Find Ratio type definition
printf '\n=== Ratio type ===\n'
rg "type Ratio|struct Ratio|use.*Ratio" -t rust | grep -v "test\|RationalNumber" | head -20
# Check where Ratio comes from in the codebase
printf '\n=== Imports of Ratio ===\n'
rg "use.*Ratio|from.*Ratio" crates/cardano/ -t rust | head -10
# Check actual field types in pallas conversion
printf '\n=== Pallas ExUnitPrices step_price/mem_price types ===\n'
rg "step_price|mem_price" -A1 -B1 -t rust | head -40Repository: txpipe/dolos
Length of output: 76
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for u5c references to see how it's used
printf '\n=== All u5c references in query.rs ===\n'
rg "u5c::" src/serve/grpc/query.rs | head -10
# Check imports at top of query.rs
printf '\n=== Imports in query.rs (first 50 lines) ===\n'
head -50 src/serve/grpc/query.rs
# Look for u5c definition or re-export
printf '\n=== u5c in all Cargo.toml files ===\n'
find . -name "Cargo.toml" -type f -exec grep -l "u5c" {} \;
# Check if u5c is defined in workspace
printf '\n=== Looking for u5c package definition ===\n'
find . -name "Cargo.toml" -type f -exec grep -H "name.*=.*\"u5c\"" {} \;
# Look at actual field types from the conversions
printf '\n=== Actual input sources and their types ===\n'
rg "value\.step_price|value\.mem_price|shelley\.a0|genesis\.shelley" src/serve/grpc/query.rs -B5 | head -50Repository: txpipe/dolos
Length of output: 4038
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check actual field types in dolos-cardano where Ratio/numerator/denominator are defined
printf '\n=== Ratio definition and usage ===\n'
rg "type Ratio|impl.*Ratio|pub struct.*Ratio" crates/cardano/ -A3 | head -40
# Check if rational_to_u5c has tests
printf '\n=== Tests for rational_to_u5c ===\n'
rg -B5 -A10 "rational_to_u5c|ExPrices|PParams" src/serve/grpc/query.rs | grep -A10 "#\[test\]" | head -40
# Look at actual test file for query.rs
printf '\n=== Test sections in query.rs ===\n'
tail -200 src/serve/grpc/query.rs | head -100
# Check pallas dependencies to understand field constraints
printf '\n=== Pallas version and relevant types ===\n'
rg "pallas = " Cargo.toml workspace_Cargo.toml 2>/dev/null || rg "pallas" Cargo.toml | head -5
# Search for any bounds/validation on ratio values
printf '\n=== Validation or constraints on ratio values ===\n'
rg "numerator|denominator" crates/cardano/ -B2 -A2 | grep -E "validate|check|assert|min|max" | head -20Repository: txpipe/dolos
Length of output: 662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# BigRational is used - these CAN have arbitrary size integers
# Now check if there's test coverage and if overflow is realistic
printf '\n=== num_rational BigRational type ===\n'
rg "num_rational|BigRational" crates/cardano/src/pots.rs crates/cardano/src/rewards/formulas.rs | head -10
# Check if rational_to_u5c is tested anywhere
printf '\n=== Search for tests involving rational_to_u5c or protocol params conversion ===\n'
rg -B3 -A15 "mod test|#\[test\]|#\[cfg.*test" src/serve/grpc/query.rs | grep -A15 "rational\|PParams\|ExPrices" | head -50
# Look at actual test section at end of query.rs
printf '\n=== Last 300 lines of query.rs (test section) ===\n'
tail -300 src/serve/grpc/query.rs | head -200
# Check if there are realistic constraints on BigRational numerator/denominator
printf '\n=== Genesis parameter validation ===\n'
rg "a0\.|rho\.|tau\." crates/cardano/src/model.rs -B2 -A2 | head -50
# Check if pallas primitives have constraints
printf '\n=== Pallas RationalNumber constraints ===\n'
rg "step_price|mem_price|ExUnitPrices" crates/cardano/src/ -A3 | head -40Repository: txpipe/dolos
Length of output: 455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the test section from end of query.rs
printf '\n=== Test section (last 400 lines) of query.rs ===\n'
tail -400 src/serve/grpc/query.rs
# Also check how BigRational values are converted to u64
printf '\n=== How BigRational is converted to u64 in the codebase ===\n'
rg "BigRational|Ratio.*numerator|Ratio.*denominator" crates/cardano/src/ -B2 -A2 | head -60
# Look for actual test or conversion of protocol params
printf '\n=== Tests or conversions of genesis or protocol params ===\n'
find . -name "*.rs" -path "*/test*" -o -name "*test*.rs" | xargs rg "rational\|PParams\|protocol.*param" 2>/dev/null | head -30Repository: txpipe/dolos
Length of output: 15912
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check all call sites of rational_to_u5c to see if they expect Result or direct return
printf '\n=== All call sites of rational_to_u5c - full context ===\n'
rg -B3 -A3 "rational_to_u5c\(" src/serve/grpc/query.rs
# Check if the callers already handle Result types (map_cardano_genesis, etc)
printf '\n=== Return types of functions that call rational_to_u5c ===\n'
rg "fn (map_cardano_genesis|float_to_u5c_rational|map_.*genesis)" -A1 src/serve/grpc/query.rs
# Check actual input field types from pallas
printf '\n=== Pallas RationalNumber numerator/denominator actual types ===\n'
rg "step_price|mem_price" crates/cardano/src/ -B5 -A5 | head -80
# Run cargo build to ensure no actual compilation errors with current code
printf '\n=== Cargo build check ===\n'
cargo build --workspace --all-targets --all-features 2>&1 | grep -E "error|warning.*rational|warning.*cast" || echo "Build successful, no relevant warnings"
# Run cargo test to verify tests pass
printf '\n=== Cargo test check ===\n'
cargo test --workspace --all-features 2>&1 | tail -50Repository: txpipe/dolos
Length of output: 4928
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what types Pallas RationalNumber and BigRational numerator/denominator actually are
printf '\n=== Checking pallas and num-rational crate types ===\n'
rg "pub struct RationalNumber|pub type.*Rational|pub.*numerator.*:.*denominator" -t rust | head -30
# Search in workspace for actual usage with custom/large values
printf '\n=== Genesis parameter bounds and constraints ===\n'
rg "a0|rho|tau|min_fee.*cost.*byte" crates/cardano/src/model.rs -B1 -A1 | head -50
# Check if there's any documented constraint on BigRational values
printf '\n=== BigRational field documentation ===\n'
rg -B5 "numerator|denominator" crates/cardano/src/model.rs | grep -E "pub|struct|///|//|numerator|denominator" | head -40
# Check test genesis files to see realistic value ranges
printf '\n=== Looking at genesis parameter values in tests ===\n'
find . -name "*.json" -path "*test*" -o -name "*genesis*" | head -10
# Most directly: check if BigRational methods are used to extract numerator/denominator
printf '\n=== How BigRational numerator/denominator are accessed ===\n'
rg "\.numerator|\.denominator" crates/cardano/src/ | grep -v "protocol_params\|shelley" | head -20Repository: txpipe/dolos
Length of output: 2233
Address unchecked u64-to-i32/u32 casts in rational_to_u5c.
This helper casts u64 inputs directly to protobuf i32/u32 fields without bounds checking. While realistic Cardano genesis parameters (a0, rho, tau, execution prices) are typically small fractions that fit within these ranges, a custom genesis with larger numerators/denominators could silently truncate. The function is called from three places that have different return type signatures—some return values directly, others return Results—so fixing this requires coordinating return type changes across float_to_u5c_rational, map_genesis_protocol_params, and map_cardano_genesis.
🛠️ Suggested fix
-fn rational_to_u5c(numerator: u64, denominator: u64) -> u5c::cardano::RationalNumber {
- u5c::cardano::RationalNumber {
- numerator: numerator as i32,
- denominator: denominator as u32,
- }
+fn rational_to_u5c(
+ numerator: u64,
+ denominator: u64,
+) -> Result<u5c::cardano::RationalNumber, Status> {
+ Ok(u5c::cardano::RationalNumber {
+ numerator: numerator
+ .try_into()
+ .map_err(|_| Status::internal("rational numerator exceeds i32"))?,
+ denominator: denominator
+ .try_into()
+ .map_err(|_| Status::internal("rational denominator exceeds u32"))?,
+ })
}Update all call sites in float_to_u5c_rational, map_genesis_protocol_params, and map_cardano_genesis to propagate the Result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/serve/grpc/query.rs` around lines 220 - 224, The helper rational_to_u5c
must not silently cast u64 to i32/u32; change rational_to_u5c to return a
Result<u5c::cardano::RationalNumber, E> and validate that numerator fits in i32
and denominator fits in u32 (return Err if out of range), then update
float_to_u5c_rational, map_genesis_protocol_params, and map_cardano_genesis to
propagate and handle that Result (adjust their return types to Result where
needed and bubble up the error) so no unchecked truncation occurs.
Comment on lines
+401
to
+408
| fn caip2_from_genesis(genesis: &Genesis) -> Result<String, Status> { | ||
| match genesis.shelley.network_magic { | ||
| Some(764824073) => Ok("cardano:mainnet".into()), | ||
| Some(1) => Ok("cardano:preprod".into()), | ||
| Some(2) => Ok("cardano:preview".into()), | ||
| Some(x) => Ok(format!("cardano:{x}")), | ||
| None => Err(Status::internal("missing Cardano network magic")), | ||
| } |
There was a problem hiding this comment.
Don't turn missing network_magic into a 500.
Genesis::network_magic() already normalizes an absent Shelley network magic to 0, and map_cardano_genesis serializes that case the same way on Line 581. Returning Status::internal here makes read_genesis fail for inputs the rest of the code can still describe.
🛠️ Suggested fix
fn caip2_from_genesis(genesis: &Genesis) -> Result<String, Status> {
- match genesis.shelley.network_magic {
- Some(764824073) => Ok("cardano:mainnet".into()),
- Some(1) => Ok("cardano:preprod".into()),
- Some(2) => Ok("cardano:preview".into()),
- Some(x) => Ok(format!("cardano:{x}")),
- None => Err(Status::internal("missing Cardano network magic")),
+ match genesis.network_magic() {
+ 764824073 => Ok("cardano:mainnet".into()),
+ 1 => Ok("cardano:preprod".into()),
+ 2 => Ok("cardano:preview".into()),
+ x => Ok(format!("cardano:{x}")),
}
}📝 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.
Suggested change
| fn caip2_from_genesis(genesis: &Genesis) -> Result<String, Status> { | |
| match genesis.shelley.network_magic { | |
| Some(764824073) => Ok("cardano:mainnet".into()), | |
| Some(1) => Ok("cardano:preprod".into()), | |
| Some(2) => Ok("cardano:preview".into()), | |
| Some(x) => Ok(format!("cardano:{x}")), | |
| None => Err(Status::internal("missing Cardano network magic")), | |
| } | |
| fn caip2_from_genesis(genesis: &Genesis) -> Result<String, Status> { | |
| match genesis.network_magic() { | |
| 764824073 => Ok("cardano:mainnet".into()), | |
| 1 => Ok("cardano:preprod".into()), | |
| 2 => Ok("cardano:preview".into()), | |
| x => Ok(format!("cardano:{x}")), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/serve/grpc/query.rs` around lines 401 - 408, caip2_from_genesis currently
returns Status::internal when genesis.shelley.network_magic is None; instead
treat None as 0 (consistent with Genesis::network_magic normalization and
map_cardano_genesis) so read_genesis can succeed. Modify caip2_from_genesis to
map None to "cardano:0" (or handle None like Some(0)) and return
Ok(format!("cardano:{x}")) accordingly; update the match arms in the
caip2_from_genesis function so missing network_magic is serialized the same way
as map_cardano_genesis.
Comment on lines
+413
to
+423
| let constitution_anchor_hash = hex::decode(&genesis.conway.constitution.anchor.data_hash) | ||
| .map_err(|e| Status::internal(format!("invalid constitution anchor hash: {e}")))?; | ||
| let constitution_hash = genesis | ||
| .conway | ||
| .constitution | ||
| .script | ||
| .as_deref() | ||
| .map(hex::decode) | ||
| .transpose() | ||
| .map_err(|e| Status::internal(format!("invalid constitution script hash: {e}")))? | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Validate constitution hashes as fixed-length hashes.
hex::decode only checks that the strings are valid hex. A short or overlong value will still be emitted as content_hash / hash, so this endpoint can return malformed hash data instead of rejecting it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/serve/grpc/query.rs` around lines 413 - 423, The current decoding of
genesis.conway.constitution.anchor.data_hash and the optional
constitution.script uses hex::decode but doesn’t enforce a fixed byte length,
allowing short/long hashes; update the creation of constitution_anchor_hash and
constitution_hash to decode and then validate the resulting Vec<u8> has the
expected fixed length (e.g., 32 bytes or your project's CONTENT_HASH_LEN
constant), returning a Status::internal (or appropriate gRPC Status) with a
clear message if the length is wrong; apply the same validation path for the
optional script (constitution.script) after the map/transpose so the
unwrap_or_default path stays correct but invalid-length hex returns an error.
AndrewWestberg
force-pushed
the
amw/grpc_genesis
branch
from
5250f53 to
9780f79
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/serve/grpc/query.rs (2)
1060-1077: Consider adding a test forNonenetwork_magic.The tests cover known networks and custom magic numbers, but there's no test for the
Nonecase. Once thecaip2_from_genesisfunction is fixed to handleNone(per the existing review comment), a test should verify that behavior.🧪 Suggested test addition
#[test] fn handles_missing_network_magic_gracefully() { let mut genesis = dolos_cardano::include::preview::load(); genesis.shelley.network_magic = None; // After fixing caip2_from_genesis, this should succeed with "cardano:0" assert_eq!(caip2_from_genesis(&genesis).unwrap(), "cardano:0"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serve/grpc/query.rs` around lines 1060 - 1077, Add a unit test named handles_missing_network_magic_gracefully that loads a preview genesis via dolos_cardano::include::preview::load(), sets genesis.shelley.network_magic = None, and asserts that caip2_from_genesis(&genesis).unwrap() returns "cardano:0" to verify the function handles a None network_magic correctly.
574-576: Theexpect()will never panic, but the message is misleading.
u64_to_bigintalways returnsSome(...), neverNone. Theexpect()call is safe but unnecessary. Consider usingunwrap()with a comment or removing the Option wrapper fromu64_to_bigintif it always succeeds.♻️ Suggested simplification
Since
u64_to_bigintalways returnsSome, you could either:
- Change
u64_to_bigintto returnu5c::cardano::BigIntdirectly (notOption)- Or use
unwrap()here since the invariant is guaranteed by the function implementation- u64_to_bigint(v).expect("u64 genesis funds must map to bigint"), + u64_to_bigint(v).unwrap(), // u64_to_bigint always returns Some🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/serve/grpc/query.rs` around lines 574 - 576, The call to u64_to_bigint(...).expect("u64 genesis funds must map to bigint") uses an expect message that is misleading because u64_to_bigint always returns Some; either change u64_to_bigint to return a plain u5c::cardano::BigInt (removing the Option) and update all callers, or replace the expect here with unwrap() and add a short comment noting the invariant (e.g., "unwrap safe: u64_to_bigint always returns Some"), and update any other call sites of u64_to_bigint to match the chosen signature change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/serve/grpc/query.rs`:
- Around line 1060-1077: Add a unit test named
handles_missing_network_magic_gracefully that loads a preview genesis via
dolos_cardano::include::preview::load(), sets genesis.shelley.network_magic =
None, and asserts that caip2_from_genesis(&genesis).unwrap() returns "cardano:0"
to verify the function handles a None network_magic correctly.
- Around line 574-576: The call to u64_to_bigint(...).expect("u64 genesis funds
must map to bigint") uses an expect message that is misleading because
u64_to_bigint always returns Some; either change u64_to_bigint to return a plain
u5c::cardano::BigInt (removing the Option) and update all callers, or replace
the expect here with unwrap() and add a short comment noting the invariant
(e.g., "unwrap safe: u64_to_bigint always returns Some"), and update any other
call sites of u64_to_bigint to match the chosen signature change.
scarmuega
approved these changes
Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Needed to implement read_genesis() because downstream updates to konduit require it.
Summary by CodeRabbit