chore(deps): utxorpc/go-codegen 0.18.1 #1223
Conversation
📝 WalkthroughWalkthroughThe patch migrates many cardano codegen usages to the utxorpc v1alpha schema: go.mod upgrades github.com/utxorpc/go-codegen and protobuf indirect deps; ledger/common adds ToUtxorpcBigInt(uint64); protocol-parameter converters (shelley, mary, allegra, alonzo, babbage, conway) now return *utxorpc.PParams and map numeric/rational fields to utxorpc.BigInt, utxorpc.RationalNumber, utxorpc.ExUnits/ExPrices, and utxorpc.ProtocolVersion with additional nil/bounds checks; transaction outputs, per-asset quantities, fees, and certificate amounts are encoded via utxorpc.BigInt wrappers; tests updated to use utxorpc types and accessors. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go.mod(2 hunks)
🔇 Additional comments (2)
go.mod (2)
33-33: No security fixes found in protobuf v1.36.10 — upgrade contains only bug fixes and maintenance updates.The upgrade from v1.36.6 to v1.36.10 contains no security fixes; the intervening releases only list bug fixes and maintenance updates. Snyk's vulnerability page shows no direct vulnerabilities for google.golang.org/protobuf. This is a routine patch version bump with no security concerns.
20-35: No issues found - go-cmp removal is intentional and safe.The removal of
github.com/google/go-cmp v0.6.0from indirect dependencies was intentional, occurring as part of the utxorpc/go-codegen upgrade from v0.17.0 to v0.18.1. The new version no longer requires go-cmp as a transitive dependency. No direct imports of go-cmp exist in the codebase, and go-cmp still appears in go.sum for other transitive dependencies.
| github.com/jinzhu/copier v0.4.0 | ||
| github.com/stretchr/testify v1.11.1 | ||
| github.com/utxorpc/go-codegen v0.17.0 | ||
| github.com/utxorpc/go-codegen v0.18.1 |
There was a problem hiding this comment.
🧩 Analysis chain
Verify utxorpc/go-codegen v0.18.1 doesn't introduce breaking changes.
This is a minor version bump (v0.17.0 → v0.18.1). While generally safe under semantic versioning, code generation tools can have subtle breaking changes.
Please confirm:
- Release notes or changelog for v0.18.1 and any intermediate versions don't indicate breaking changes
- Generated code (if any) has been regenerated with the new version and is correct
- All tests pass with the updated code generator
🌐 Web query:
utxorpc/go-codegen v0.18.1 release notes breaking changes
💡 Result:
I couldn't find a release named v0.18.1 for utxorpc/go-codegen. The repository's Releases list shows v0.17.0 as the latest (tagged Jul 7, 2025). [1][2]
If you meant a different tag/branch or a pre-release, tell me the exact tag or paste the release link and I’ll inspect it for breaking-change notes.
Sources:
- utxorpc/go-codegen GitHub — Releases (shows v0.17.0 latest). [1]
- Libraries.io entry for github.com/utxorpc/go-codegen. [2]
Dependency version v0.18.1 does not exist in utxorpc/go-codegen repository.
The latest release in the utxorpc/go-codegen repository is v0.17.0, but the PR updates go.mod to v0.18.1. This version does not appear to exist. Verify the intended target version and correct go.mod accordingly—either revert to v0.17.0 or use the correct version number if this is a typo.
🤖 Prompt for AI Agents
In go.mod around line 15, the dependency is set to github.com/utxorpc/go-codegen
v0.18.1 which does not exist; confirm the intended release (likely v0.17.0) by
checking the repository tags/releases, then update go.mod to the correct
existing version (e.g., v0.17.0) and run go get
github.com/utxorpc/go-codegen@<correct-version> followed by go mod tidy to
update go.sum and ensure the dependency resolves.
There was a problem hiding this comment.
Incorrect. That version was released this morning.
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
utxorpc go-codegen v0.18.1 November 2025 release
💡 Result:
Yes — utxorpc/go-codegen v0.18.1 was released on November 3, 2025. Changes are primarily dependency bumps (google.golang.org/protobuf and connectrpc/connect via dependabot). [1][2]
Sources:
- GitHub Releases — utxorpc/go-codegen v0.18.1 (Nov 3, 2025). [1]
- Libraries.io package page for github.com/utxorpc/go-codegen. [2]
@wolf31o2, you're absolutely right—I apologize for the incorrect information. v0.18.1 was indeed released on November 3, 2025, and the changes are primarily dependency bumps for google.golang.org/protobuf and connectrpc/connect.
The version bump looks good, and since it's a maintenance release with dependency updates, there shouldn't be any breaking changes to worry about. Thank you for the correction!
There was a problem hiding this comment.
The tests are failing. Some changes may be necessary in the ledger/common package.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the ledger/common package files
echo "=== Files in ledger/common package ==="
fd -t f . ledger/common
echo -e "\n=== Searching for utxorpc imports in the codebase ==="
rg -n --type=go "github.com/utxorpc/go-codegen" -A 2
echo -e "\n=== Examining ledger/common package code for utxorpc usage ==="
fd -t f -e go . ledger/common --exec cat {}
echo -e "\n=== Checking for any test files in ledger/common ==="
fd -t f -e go . ledger/common | grep -i test
Length of output: 230971
🏁 Script executed:
#!/bin/bash
# Check if there's a go.sum to see the actual dependency changes
echo "=== Checking go.sum for utxorpc changes ==="
git diff HEAD go.sum | grep utxorpc || echo "No utxorpc changes visible in go.sum diff"
echo -e "\n=== Looking for utxorpc changelog or recent changes ==="
# Try to get release notes from GitHub
gh api repos/utxorpc/go-codegen/releases --jq '.[] | select(.tag_name == "v0.18.1" or .tag_name == "v0.18.0" or .tag_name == "v0.17.0") | {tag: .tag_name, date: .published_at, body: .body}' 2>/dev/null || echo "Could not fetch release notes"
echo -e "\n=== Checking for test failures by running tests ==="
# Run tests in ledger/common package to see actual errors
go test -v ./ledger/common/... 2>&1 | head -100
Length of output: 3911
🏁 Script executed:
#!/bin/bash
echo "=== Examining error locations in certs.go ==="
sed -n '585,600p' ledger/common/certs.go
echo ""
sed -n '755,770p' ledger/common/certs.go
echo -e "\n=== Examining error location in tx.go ==="
sed -n '235,245p' ledger/common/tx.go
echo -e "\n=== Searching for BigInt type definition in utxorpc ==="
# Look for BigInt in the downloaded utxorpc package
fd -t f -e go . $(go env GOPATH)/pkg/mod/github.com/utxorpc/go-codegen* 2>/dev/null | head -5 | xargs grep -l "type BigInt" 2>/dev/null | head -1 | xargs cat 2>/dev/null | grep -A 10 "type BigInt"
echo -e "\n=== Checking if there's a BigInt conversion helper ==="
rg -n "func.*BigInt" $(go env GOPATH)/pkg/mod/github.com/utxorpc/go-codegen* 2>/dev/null | head -10
Length of output: 3780
🏁 Script executed:
#!/bin/bash
echo "=== Looking for BigInt constructor patterns ==="
rg -A 5 "BigInt{" $(go env GOPATH)/pkg/mod/github.com/utxorpc/[email protected] 2>/dev/null | head -30
echo -e "\n=== Checking existing usage in other ledger eras ==="
# Check if other eras already handle this conversion
rg -n "BigInt" ledger/*/pparams.go ledger/*/babbage.go ledger/*/shelley.go 2>/dev/null | head -10
echo -e "\n=== Examining the full PoolRegistrationCert definition ==="
rg -A 20 "type PoolRegistrationCert struct" $(go env GOPATH)/pkg/mod/github.com/utxorpc/[email protected]/utxorpc/v1alpha/cardano/cardano.pb.go 2>/dev/null | head -30
echo -e "\n=== Examining the Tx Fee field definition ==="
rg -B 5 -A 5 "Fee.*BigInt" $(go env GOPATH)/pkg/mod/github.com/utxorpc/[email protected]/utxorpc/v1alpha/cardano/cardano.pb.go 2>/dev/null | head -20
Length of output: 4301
@wolf31o2, I've identified the test failures. The utxorpc/go-codegen v0.18.1 update changed several numeric fields from uint64/int64 to *cardano.BigInt pointers. Here are the specific changes needed in the ledger/common package:
Required Changes
File: ledger/common/certs.go
Lines 592-593 (PoolRegistrationCert):
// Change from:
Pledge: c.Pledge,
Cost: c.Cost,
// To:
Pledge: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(c.Pledge)}},
Cost: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(c.Cost)}},Line 763 (MirCert):
// Change from:
DeltaCoin: int64(deltaCoin),
// To:
DeltaCoin: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(deltaCoin)}},File: ledger/common/tx.go
Line 239 (Tx):
// Change from:
Fee: tx.Fee(),
// To:
Fee: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(tx.Fee())}},Optional: Helper Function
For cleaner code, you could add a helper function at the top of ledger/common/certs.go:
// uint64ToBigInt converts a uint64 to a cardano.BigInt pointer
func uint64ToBigInt(val uint64) *utxorpc.BigInt {
return &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(val)}}
}Then use it like:
Pledge: uint64ToBigInt(c.Pledge),
Cost: uint64ToBigInt(c.Cost),The changes are straightforward - the protobuf schema now uses BigInt to support arbitrarily large numbers, but for Cardano's typical values, they fit comfortably in int64, so we use the BigInt_Int variant.
There was a problem hiding this comment.
After making the requested changes, the tests are still failing.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Running tests to see current failures ==="
go test -v ./ledger/common/... 2>&1 | head -150
echo -e "\n=== Checking current code at the error locations ==="
echo "--- certs.go lines 585-600 ---"
sed -n '585,600p' ledger/common/certs.go
echo -e "\n--- certs.go lines 755-770 ---"
sed -n '755,770p' ledger/common/certs.go
echo -e "\n--- tx.go lines 235-245 ---"
sed -n '235,245p' ledger/common/tx.go
echo -e "\n=== Searching for all utxorpc.BigInt usage patterns in codebase ==="
rg -n "utxorpc\.BigInt" --type=go -A 2 -B 2
echo -e "\n=== Checking the BigInt protobuf definition ==="
rg -A 15 "message BigInt" $(go env GOPATH)/pkg/mod/github.com/utxorpc/[email protected] 2>/dev/null | head -30
echo -e "\n=== Looking for all PoolRegistrationCert instantiations ==="
rg -n "PoolRegistrationCert\{" ledger/common/ -A 10
Length of output: 21312
🏁 Script executed:
#!/bin/bash
echo "=== Running complete test suite for ledger/common ==="
go test ./ledger/common/... 2>&1
echo -e "\n=== Test exit code: $? ==="
echo -e "\n=== Also checking if there are any test failures in other packages ==="
go test ./... 2>&1 | grep -E "(FAIL|PASS:|^ok |^FAIL)" | tail -50
Length of output: 1674
🏁 Script executed:
#!/bin/bash
echo "=== Checking build errors in ledger era packages ==="
echo "--- allegra ---"
go test -c ./ledger/allegra 2>&1 | head -20
echo -e "\n--- alonzo ---"
go test -c ./ledger/alonzo 2>&1 | head -20
echo -e "\n--- babbage ---"
go test -c ./ledger/babbage 2>&1 | head -20
echo -e "\n--- byron ---"
go test -c ./ledger/byron 2>&1 | head -20
echo -e "\n--- conway ---"
go test -c ./ledger/conway 2>&1 | head -20
echo -e "\n--- mary ---"
go test -c ./ledger/mary 2>&1 | head -20
echo -e "\n--- shelley ---"
go test -c ./ledger/shelley 2>&1 | head -20
echo -e "\n=== Checking localstatequery test failure ==="
go test -v ./protocol/localstatequery 2>&1
Length of output: 8595
@wolf31o2, I see the issue now. The ledger/common package tests pass, but the test files in all the ledger era packages (allegra, alonzo, babbage, byron, conway, mary, shelley) need to be updated to work with the new *cardano.BigInt type. Here's what needs to be done:
Required Test File Updates
The errors fall into two categories:
1. Creating BigInt values in test structs
When creating test PParams or Tx structs, integer literals need to be wrapped:
Pattern to find and replace:
// OLD:
MinFeeConstant: 500,
StakeKeyDeposit: 2000,
Fee: 100,
Coin: amount,
// NEW:
MinFeeConstant: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: 500}},
StakeKeyDeposit: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: 2000}},
Fee: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: 100}},
Coin: &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: int64(amount)}},2. Comparing BigInt values in assertions
Pattern to find and replace:
// OLD:
if got.Fee != 100 { ... }
if got.Outputs[0].Coin != 5000 { ... }
if actual.Fee != tx.Fee() { ... }
// NEW:
if got.Fee.BigInt.(*utxorpc.BigInt_Int).Int != 100 { ... }
if got.Outputs[0].Coin.BigInt.(*utxorpc.BigInt_Int).Int != 5000 { ... }
if got.Fee.BigInt.(*utxorpc.BigInt_Int).Int != int64(tx.Fee()) { ... }3. Helper function for tests (recommended)
Add this helper function at the top of each *_test.go file that needs it:
// Helper function to create BigInt for tests
func testBigInt(val int64) *utxorpc.BigInt {
return &utxorpc.BigInt{BigInt: &utxorpc.BigInt_Int{Int: val}}
}
// Helper function to extract int64 from BigInt for comparisons
func bigIntToInt64(bi *utxorpc.BigInt) int64 {
if bi == nil {
return 0
}
if i, ok := bi.BigInt.(*utxorpc.BigInt_Int); ok {
return i.Int
}
return 0
}Then use them like:
// Creating values:
MinFeeConstant: testBigInt(500),
Fee: testBigInt(100),
Coin: testBigInt(int64(amount)),
// Comparing values:
if bigIntToInt64(got.Fee) != 100 { ... }
if bigIntToInt64(got.Outputs[0].Coin) != 5000 { ... }Affected Files
Based on the errors, you need to update these test files:
ledger/allegra/pparams_test.go(lines 96, 97, 101, 102, 174, 236)ledger/alonzo/pparams_test.go(lines 377, 379, 380, 383, 384, 399, 479, 531, 547, 594+)ledger/babbage/pparams_test.go(lines 484, 486, 487, 490, 491, 506, 603, 645, 654, 690+)ledger/byron/byron_test.go(lines 66, 103)ledger/conway/pparams_test.go(lines 529, 531, 532, 535, 536, 551, 665, 674, 742, 751+)ledger/mary/pparams_test.go(lines 99, 100, 104, 105, 181, 223, 232, 271, 280, 480+)ledger/shelley/pparams_test.go(lines 131, 132, 135, 136, 223, 281, 349)
Would you like me to generate the specific fixes for these files, or would you prefer to apply the patterns systematically yourself?
Note: The protocol/localstatequery test failure is unrelated to this PR—it's missing a test fixture file and appears to be a pre-existing issue.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
2 times, most recently
from
210e958 to
e47e971
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ledger/conway/pparams.go (1)
64-156: Prevent nil dereference in Utxorpc() sanity checks.
p.A0,p.Rho,p.Tau, and the Plutus price fields can still be nil on a freshly decoded struct (e.g., before UpdateFromGenesis populates them). As soon as we call.Num()on any nil rat we panic the caller. We should reject missing values up front instead of crashing.Apply this diff to guard the dereferences:
func (p *ConwayProtocolParameters) Utxorpc() (*utxorpc.PParams, error) { - // sanity check + if p.A0 == nil || p.Rho == nil || p.Tau == nil { + return nil, errors.New("missing required rational parameters (A0/Rho/Tau)") + } + if p.ExecutionCosts.MemPrice == nil || p.ExecutionCosts.StepPrice == nil { + return nil, errors.New("missing Plutus execution prices") + } + // sanity check if p.A0.Num().Int64() > math.MaxInt32 || p.A0.Denom().Int64() < 0 || p.A0.Denom().Int64() > math.MaxUint32 {ledger/alonzo/pparams.go (1)
277-369: Add nil guards before touching rational and price fields.Just like Conway,
p.A0,p.Rho,p.Tau, or the execution price rats can be nil on a zero-value/newly decoded struct. Calling.Num()on nil will SIGSEGV the caller. Please validate the pointers and return an error instead of panicking.Apply this diff to keep the method safe:
func (p *AlonzoProtocolParameters) Utxorpc() (*utxorpc.PParams, error) { - // sanity check + if p.A0 == nil || p.Rho == nil || p.Tau == nil { + return nil, errors.New("missing required rational parameters (A0/Rho/Tau)") + } + if p.ExecutionCosts.MemPrice == nil || p.ExecutionCosts.StepPrice == nil { + return nil, errors.New("missing Plutus execution price parameters") + } + // sanity check if p.A0.Num().Int64() > math.MaxInt32 || p.A0.Denom().Int64() < 0 || p.A0.Denom().Int64() > math.MaxUint32 {
🧹 Nitpick comments (1)
ledger/babbage/pparams_test.go (1)
552-557: Use proto.Equal for utxorpc comparisonsThe utxorpc values are protobuf messages, so
reflect.DeepEqualcan produce spurious mismatches once the message state (unknown fields, caches, etc.) diverges from the struct literal. Switching toproto.Equalkeeps the tests stable as we marshal/unmarshal or evolve the code.import ( "encoding/hex" "math/big" "reflect" "testing" "github.com/blinklabs-io/gouroboros/cbor" "github.com/blinklabs-io/gouroboros/ledger/babbage" "github.com/blinklabs-io/gouroboros/ledger/common" "github.com/blinklabs-io/gouroboros/ledger/mary" "github.com/blinklabs-io/gouroboros/ledger/shelley" "github.com/stretchr/testify/assert" utxorpc "github.com/utxorpc/go-codegen/utxorpc/v1alpha/cardano" + "google.golang.org/protobuf/proto" ) @@ - if !reflect.DeepEqual(result, testDef.expectedUtxorpc) { + if !proto.Equal(result, testDef.expectedUtxorpc) { @@ - if !reflect.DeepEqual(got, want) { + if !proto.Equal(got, want) { @@ - if !reflect.DeepEqual(got, want) { + if !proto.Equal(got, want) {Also applies to: 578-584, 609-615
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
go.mod(2 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(4 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(7 hunks)ledger/byron/block_test.go(1 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(4 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- ledger/shelley/shelley.go
- ledger/alonzo/alonzo.go
- ledger/common/tx.go
- ledger/byron/byron.go
- go.mod
- ledger/mary/mary.go
- ledger/mary/pparams.go
- ledger/babbage/pparams.go
- ledger/common/certs.go
🧰 Additional context used
🧬 Code graph analysis (11)
ledger/byron/block_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/babbage.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams.go (1)
ledger/shelley.go (1)
ShelleyProtocolParameters(31-31)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/alonzo/pparams_test.go (5)
protocol/versions.go (1)
ProtocolVersion(26-42)ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/babbage_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams_test.go (4)
protocol/versions.go (1)
ProtocolVersion(26-42)ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(485-489)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams_test.go (1)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/conway/pparams.go (2)
ledger/conway.go (1)
ConwayProtocolParameters(29-29)ledger/common/common.go (1)
ExUnits(485-489)
🪛 GitHub Actions: go-test
ledger/mary/pparams_test.go
[error] 132-132: TestMaryUtxorpc: Utxorpc() test failed for Mary: Expected: &cardano.PParams{...}, Got: &cardano.PParams{...} (MinFeeCoefficient/MinFeeConstant mismatch).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
ledger/allegra/block_test.go (1)
155-156: LGTM! Fee access correctly updated for utxorpc.BigInt wrapper.The change at lines 155-156 correctly uses the
GetInt()accessor pattern, which aligns with the migration toutxorpc.BigIntwrappers in v0.18.1. This pattern is consistently applied across the entire codebase (20+ uses in conway, alonzo, mary, shelley, byron, babbage, and other ledger packages), confirming the library guarantees proper initialization of the Fee field.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
e47e971 to
57a27da
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ledger/byron/block_test.go
- ledger/mary/mary.go
- ledger/allegra/block_test.go
- ledger/babbage/babbage_test.go
- ledger/shelley/shelley.go
- ledger/byron/byron.go
- ledger/alonzo/alonzo.go
- ledger/babbage/pparams_test.go
🧰 Additional context used
🧬 Code graph analysis (9)
ledger/babbage/babbage.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams.go (1)
ledger/shelley.go (1)
ShelleyProtocolParameters(31-31)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams_test.go (3)
ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(485-489)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/babbage/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/alonzo/pparams_test.go (3)
ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams.go (2)
ledger/alonzo.go (1)
AlonzoProtocolParameters(30-30)ledger/common/common.go (1)
ExUnits(485-489)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
🪛 GitHub Actions: go-test
go.mod
[error] 1-1: Go test failed due to compilation error in ledger/alonzo/pparams_test.go
ledger/alonzo/pparams_test.go
[error] 563-563: invalid operation: got.Outputs[0].Coin != 1000 (mismatched types *cardano.BigInt and untyped int)
🪛 GitHub Actions: nilaway
ledger/alonzo/pparams_test.go
[error] 563-563: nilaway: invalid operation: got.Outputs[0].Coin != 1000 (mismatched types *cardano.BigInt and untyped int)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
ledger/byron/byron_test.go (4)
18-18: LGTM! Import changes align with schema migration.The addition of
math/bigand the updated utxorpc import path correctly support the migration to the v1alpha schema.Also applies to: 25-25
39-42: LGTM! Type migration correctly applied.The change from
cardano.TxInputtoutxorpc.TxInputis consistent with the v1alpha schema migration.
65-72: LGTM! BigInt wrapper construction looks correct.The migration to
utxorpc.TxOutputwith theBigIntwrapper forCoinfollows the expected protobuf pattern. The nestedBigInt_BigUIntstructure containing the big-endian byte representation is the correct way to represent positive integers in the new schema.
97-97: LGTM! Fee access pattern correctly updated.The change to use
result.Fee.GetInt()properly handles the newBigIntwrapper type, and the error message update maintains consistency with the schema migration.Also applies to: 108-109
go.mod (1)
15-18: LGTM! Dependency updates are correct.The bump to utxorpc/go-codegen v0.18.1 and related protobuf dependency updates align with the PR objectives for migrating to utxorpc v1alpha types.
ledger/allegra/pparams_test.go (2)
95-129: LGTM! Test expectations correctly updated for utxorpc types.The test properly constructs utxorpc.PParams with BigInt wrappers for numeric fields and RationalNumber for ratios, consistent with the migration pattern.
182-188: LGTM! Correct use of GetInt() accessor.The fee assertion properly uses
GetInt()to extract the int64 value from the BigInt wrapper for comparison.ledger/common/tx.go (1)
239-241: LGTM! Fee correctly wrapped in BigInt.The fee value is properly wrapped in a utxorpc.BigInt using the BigInt_Int variant, consistent with other fee representations throughout the codebase.
ledger/babbage/babbage.go (2)
640-648: LGTM! Correct BigInt_BigUInt usage for asset quantities.The implementation properly uses
BigInt_BigUIntwithbig.Intconversion to handle the full uint64 range for asset quantities, avoiding the overflow issues that would occur with int64 casts.
664-668: LGTM! Correct BigInt_BigUInt usage for coin amounts.The Coin field correctly uses
BigInt_BigUIntto support the full uint64 range, consistent with the asset quantity encoding.ledger/common/certs.go (2)
593-598: LGTM! Certificate fields correctly wrapped in BigInt.The Pledge and Cost fields are properly wrapped using
BigInt_Int, which is appropriate for these Cardano values that fit within int64 range.
767-769: LGTM! MIR delta coin correctly wrapped.The DeltaCoin field in MirTarget is properly wrapped using
BigInt_Int, consistent with other certificate encodings.ledger/mary/pparams.go (1)
143-205: LGTM! Mary protocol parameters correctly migrated to utxorpc types.The Utxorpc method properly constructs a utxorpc.PParams with all numeric fields wrapped in BigInt structures and rational numbers using RationalNumber, consistent with the migration pattern across other eras.
ledger/alonzo/pparams_test.go (1)
376-448: LGTM! Test expectations correctly updated for utxorpc types.The expected PParams structure properly uses BigInt wrappers for numeric fields, RationalNumber for ratios, and the correct utxorpc types for cost models and execution units.
ledger/babbage/pparams.go (1)
166-259: LGTM! Babbage protocol parameters correctly migrated to utxorpc types.The Utxorpc method properly constructs a utxorpc.PParams with all fields wrapped in appropriate types (BigInt, RationalNumber, ExUnits, ExPrices), maintaining consistency with other era implementations.
ledger/alonzo/pparams.go (1)
279-308: Verify: Inconsistent rational number validation compared to Shelley era.The validation logic for A0, Rho, Tau, MemPrice, and StepPrice only checks the upper bound (MaxInt32) but omits the lower bound (MinInt32) check. This is inconsistent with the Shelley implementation (lines 174-193 in
ledger/shelley/pparams.go).Without the lower bound validation, numerator values less than MinInt32 could pass through and cause overflow when cast to
int32at lines 332, 336, 340, 358, and 362.Consider adding the
< math.MinInt32checks for consistency with Shelley, or document why this validation is intentionally different in the Alonzo era.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
57a27da to
e3c2496
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/conway/pparams_test.go (1)
605-608: Fix StepPrice validation bounds in Utxorpc() method.The StepPrice validation check is incorrect. It uses
< math.MaxInt32 || > math.MaxInt32when it should use< math.MinInt32 || > math.MaxInt32to match the validation pattern for A0, Rho, and Tau. This causes the method to reject virtually all inputs (except the exact value MaxInt32), resulting in the test failure.Fix required in
ledger/conway/pparams.go: Change the StepPrice validation condition from< math.MaxInt32to< math.MinInt32.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- ledger/common/tx.go
- ledger/babbage/babbage_test.go
- go.mod
- ledger/byron/byron.go
- ledger/byron/block_test.go
- ledger/byron/byron_test.go
- ledger/common/certs.go
- ledger/mary/pparams_test.go
- ledger/mary/pparams.go
🧰 Additional context used
🧬 Code graph analysis (11)
ledger/mary/mary.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/shelley.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/babbage.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams_test.go (3)
ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(485-489)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams.go (2)
ledger/alonzo.go (1)
AlonzoProtocolParameters(30-30)ledger/common/common.go (1)
ExUnits(485-489)
ledger/conway/pparams.go (2)
ledger/conway.go (1)
ConwayProtocolParameters(29-29)ledger/common/common.go (1)
ExUnits(485-489)
ledger/alonzo/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/babbage/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/babbage/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/alonzo.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
🪛 GitHub Actions: go-test
ledger/conway/pparams_test.go
[error] 607-607: TestUtxorpc() conversion failed.
🔇 Additional comments (22)
ledger/allegra/block_test.go (1)
155-156: LGTM!The fee assertion correctly adapts to the utxorpc BigInt wrapper using
.GetInt()and compares against the appropriateint64(0)type. The test logic is preserved and the change aligns with the PR's migration to utxorpc v1alpha schema.ledger/conway/pparams_test.go (2)
30-30: LGTM: Import alias updated correctly.The import alias change from
cardanotoutxorpcaligns with the repository-wide migration to the utxorpc v1alpha schema.
677-678: Verify BigInt accessor usage for GetBigUInt().The tests access coin values via
GetBigUInt()and convert to uint64 usingnew(big.Int).SetBytes(). However,GetBigUInt()returns[]byte, not a direct numeric value. Ensure this pattern correctly handles:
- Empty byte slices (zero values)
- Big-endian byte order (which
big.Int.SetBytesexpects)- Potential nil returns
Run the following test to verify the accessor behavior:
Also applies to: 686-693, 769-776
ledger/babbage/pparams_test.go (3)
29-31: LGTM: Imports updated for utxorpc migration.The addition of
google.golang.org/protobuf/protoand the utxorpc import alias correctly support the migration to protobuf-based types.
565-565: LGTM: proto.Equal used for protobuf message comparison.Using
proto.Equalinstead ofreflect.DeepEqualis the correct approach for comparing protobuf messages, as it handles protobuf-specific semantics properly.
671-678: Verify consistency in BigInt conversion pattern.The pattern
new(big.Int).SetBytes(got.Outputs[0].Coin.GetBigUInt()).Uint64()is used to extract uint64 values from BigInt wrappers. This is consistent with the utxorpc migration but assumes:
- GetBigUInt() never returns nil
- The value fits in uint64 range
Consider adding explicit nil checks and range validation in production code:
if got.Outputs[0].Coin.GetBigUInt() != nil { coinValue := new(big.Int).SetBytes(got.Outputs[0].Coin.GetBigUInt()).Uint64() if coinValue != uint64(5000) { t.Errorf("Output coin mismatch: got %d, want 5000", coinValue) } }Also applies to: 722-729
ledger/mary/mary.go (1)
539-543: LGTM: Coin encoding migrated to utxorpc BigInt wrapper.The Coin field now correctly uses
utxorpc.BigIntwithBigInt_BigUIntto encode the amount as bytes. This change is consistent with the broader utxorpc v1alpha migration across all ledger modules.ledger/shelley/shelley.go (1)
470-474: LGTM: Coin encoding migrated to utxorpc BigInt wrapper.The Coin field encoding is consistent with other ledger modules (Mary, Babbage, Alonzo), using
utxorpc.BigIntwithBigInt_BigUIntfor the amount bytes representation.ledger/babbage/babbage.go (1)
640-647: LGTM: Asset quantities and Coin now use BigInt_BigUInt encoding.The changes correctly address the overflow concerns raised in previous reviews by using
BigInt_BigUIntwith byte encoding vianew(big.Int).SetUint64(...).Bytes(). This preserves the full uint64 range (0 to 2^64-1) without the truncation issues that would occur with int64 casts.Also applies to: 664-668
ledger/babbage/pparams.go (2)
166-192: LGTM: Proper validation before utxorpc conversion.The sanity checks for rational numbers (A0, Rho, Tau, MemPrice, StepPrice) ensure values fit within int32/uint32 bounds before constructing utxorpc types. This prevents runtime errors and data loss.
194-258: LGTM: PParams construction migrated to utxorpc types.The construction correctly uses:
utxorpc.BigIntfor numeric fields (CoinsPerUtxoByte, MinFeeCoefficient, MinFeeConstant, StakeKeyDeposit, PoolDeposit, MinPoolCost)utxorpc.RationalNumberfor rational fields (PoolInfluence, MonetaryExpansion, TreasuryExpansion)utxorpc.ProtocolVersionfor protocol versionutxorpc.ExPriceswith nestedRationalNumberfor execution costsutxorpc.ExUnitsfor execution unitsThis aligns with the broader utxorpc v1alpha migration across all ledger modules.
ledger/alonzo/alonzo.go (1)
457-464: LGTM: Asset quantities and Coin migrated to BigInt_BigUInt encoding.The encoding pattern is consistent with Babbage and other ledger modules, correctly using
BigInt_BigUIntwith byte representation to preserve the full uint64 range without overflow.Also applies to: 485-489
ledger/shelley/pparams_test.go (4)
1-1: LGTM: Copyright year updated.The copyright year update to 2025 is appropriate.
27-27: LGTM: Test expectations migrated to utxorpc types.The test now correctly expects
*utxorpc.PParamswith proper BigInt and RationalNumber wrappers for all numeric and rational fields, consistent with the utxorpc v1alpha migration.Also applies to: 129-163
231-233: LGTM: Coin field uses BigInt_BigUInt encoding.The Coin field correctly uses
BigInt_BigUIntwith byte representation vianew(big.Int).SetUint64(1000).Bytes(), consistent with the pattern across all ledger modules.
291-295: LGTM: Fee accessor using GetInt() correctly.The tests correctly use
GetInt()to access fee values. Fees use theBigInt_Intvariant (notBigInt_BigUInt), soGetInt()is the appropriate accessor.Also applies to: 359-363
ledger/shelley/pparams.go (2)
24-24: Public API migration to utxorpc types looks good.The import alias and method signature changes properly reflect the migration from cardano to utxorpc v1alpha schema as intended by this PR.
Also applies to: 172-172
173-231: Validation and type wrapping implemented correctly.The validation checks for A0, Rho, and Tau include both lower (
< math.MinInt32) and upper (> math.MaxInt32) bounds, preventing overflow when casting toint32. All numeric fields are properly wrapped with utxorpc types (BigInt, RationalNumber, ProtocolVersion).ledger/conway/pparams.go (1)
102-166: PParams construction uses proper utxorpc type wrappers.The PParams construction correctly wraps all numeric fields with utxorpc.BigInt, rational fields with utxorpc.RationalNumber, and execution-related fields with utxorpc.ExPrices and utxorpc.ExUnits. Once the validation issue on line 95 is fixed, this construction is safe from overflow.
ledger/allegra/pparams_test.go (1)
76-142: Test expectations correctly updated to utxorpc types.The test properly constructs expected utxorpc.PParams with BigInt wrappers for numeric fields, RationalNumber for rationals, and ProtocolVersion for version info. The GetInt() accessor usage (lines 182, 185, 244, 247) is appropriate for comparing BigInt_Int values.
ledger/alonzo/pparams_test.go (2)
563-571: Coin value comparisons correctly handle BigInt_BigUInt representation.The tests properly use
GetBigUInt()to retrieve byte representation, convert tobig.Int, and then touint64for comparison. This correctly addresses the storage format where coin values useBigInt_BigUInt(bytes) rather thanBigInt_Int. The fee comparisons at lines 547-548 and 616-617 correctly useGetInt()for theBigInt_Intvariant.Also applies to: 625-633
376-448: Test expectations properly migrated to utxorpc types.The test correctly constructs expected utxorpc.PParams with proper type wrappers: BigInt for numeric fields, RationalNumber for rationals, ExPrices/ExUnits for execution parameters, and CostModels with PlutusV1/V2/V3 variants.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
e3c2496 to
9d5308f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/mary/pparams_test.go (1)
98-132: Add missing MinPoolCost field to fix pipeline failure.The test is failing because the expected structure is missing the
MinPoolCostfield. WheninputParamsdoesn't explicitly setMinPoolCost, it defaults to 0, and theUtxorpc()method includes this field in the returned structure. TheDeepEqualcomparison at line 139 fails due to this mismatch.Apply this diff to add the missing field:
ProtocolVersion: &utxorpc.ProtocolVersion{ Major: 8, Minor: 0, }, + MinPoolCost: &utxorpc.BigInt{ + BigInt: &utxorpc.BigInt_Int{Int: 0}, + }, }
♻️ Duplicate comments (1)
ledger/conway/pparams.go (1)
103-137: Keep full uint64 precision in Conway PParamsAll of these fields are unsigned consensus coins, yet we cast them to
int64and rely on theBigInt_Intarm. As soon as we see something abovemath.MaxInt64(e.g.MinPoolCost—seeTestAlonzoUpgradePParams, which already exercisesmath.MaxUint64), we will encode-1into the utxorpc message. That is data corruption. Please promote large values into theBigUIntarm instead of overflowing.@@ - // #nosec G115 - return &utxorpc.PParams{ - CoinsPerUtxoByte: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.AdaPerUtxoByte)}, - }, + // #nosec G115 + toBigInt := func(v uint64) *utxorpc.BigInt { + if v <= math.MaxInt64 { + return &utxorpc.BigInt{ + BigInt: &utxorpc.BigInt_Int{Int: int64(v)}, + } + } + return &utxorpc.BigInt{ + BigInt: &utxorpc.BigInt_BigUInt{ + BigUInt: new(big.Int).SetUint64(v).Bytes(), + }, + } + } + return &utxorpc.PParams{ + CoinsPerUtxoByte: toBigInt(p.AdaPerUtxoByte), MaxTxSize: uint64(p.MaxTxSize), - MinFeeCoefficient: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.MinFeeA)}, - }, - MinFeeConstant: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.MinFeeB)}, - }, + MinFeeCoefficient: toBigInt(uint64(p.MinFeeA)), + MinFeeConstant: toBigInt(uint64(p.MinFeeB)), MaxBlockBodySize: uint64(p.MaxBlockBodySize), MaxBlockHeaderSize: uint64(p.MaxBlockHeaderSize), - StakeKeyDeposit: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.KeyDeposit)}, - }, - PoolDeposit: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.PoolDeposit)}, - }, + StakeKeyDeposit: toBigInt(uint64(p.KeyDeposit)), + PoolDeposit: toBigInt(uint64(p.PoolDeposit)), @@ - MinPoolCost: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_Int{Int: int64(p.MinPoolCost)}, - }, + MinPoolCost: toBigInt(p.MinPoolCost),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ledger/babbage/babbage.go
- ledger/byron/block_test.go
- ledger/shelley/shelley.go
- ledger/common/tx.go
- ledger/babbage/babbage_test.go
🧰 Additional context used
🧬 Code graph analysis (13)
ledger/mary/mary.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/byron.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/byron_test.go (1)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/shelley/pparams.go (1)
ledger/shelley.go (1)
ShelleyProtocolParameters(31-31)
ledger/conway/pparams.go (2)
ledger/conway.go (1)
ConwayProtocolParameters(29-29)ledger/common/common.go (1)
ExUnits(485-489)
ledger/babbage/pparams_test.go (3)
ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/data.go (1)
Datum(25-28)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(485-489)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/alonzo/pparams_test.go (4)
ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(485-489)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/alonzo.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (1)
ExUnits(485-489)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
🪛 GitHub Actions: go-test
ledger/mary/pparams_test.go
[error] 140-140: TestMaryUtxorpc failed: Utxorpc() test failed for Mary due to a PParams mismatch between expected and actual values.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
ledger/mary/pparams_test.go (5)
149-171: LGTM!The transaction input conversion test correctly uses the new
*utxorpc.TxInputtype and properly validates the conversion.
174-201: LGTM!The transaction output conversion correctly uses
*utxorpc.TxOutputwith*utxorpc.BigIntwrapping the coin value asBigInt_BigUInt. The conversion pattern is appropriate for uint64 amounts.
233-250: LGTM!The test correctly uses accessor methods
GetInt()for fee values andGetBigUInt()for coin amounts, which aligns with the utxorpc v1alpha type migration.
287-304: LGTM!The transaction conversion test follows the same correct pattern as the body test, using appropriate accessor methods for fee and coin values.
502-505: LGTM!The MinPoolCost field is correctly accessed via
GetInt()and compared asint64. This test pattern is consistent with the utxorpc type migration.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
9d5308f to
d35e81c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- ledger/common/tx.go
- ledger/shelley/shelley.go
- ledger/byron/block_test.go
- ledger/babbage/babbage.go
- go.mod
- ledger/babbage/babbage_test.go
- ledger/common/certs.go
- ledger/byron/byron.go
- ledger/mary/mary.go
🧰 Additional context used
🧬 Code graph analysis (10)
ledger/alonzo/pparams_test.go (4)
ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/data.go (1)
Datum(25-28)
ledger/alonzo/alonzo.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams.go (1)
ledger/common/common.go (1)
ExUnits(487-491)
ledger/babbage/pparams_test.go (3)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/data.go (1)
Datum(25-28)
ledger/conway/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams_test.go (1)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams.go (2)
ledger/shelley.go (1)
ShelleyProtocolParameters(31-31)ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
🪛 GitHub Actions: golangci-lint
ledger/alonzo/pparams.go
[error] 327-327: golangci-lint: G115: integer overflow conversion int64 -> int32 (gosec)
🪛 GitHub Check: lint
ledger/alonzo/pparams.go
[failure] 336-336:
G115: integer overflow conversion int64 -> uint32 (gosec)
[failure] 335-335:
G115: integer overflow conversion int64 -> int32 (gosec)
[failure] 332-332:
G115: integer overflow conversion int64 -> uint32 (gosec)
[failure] 331-331:
G115: integer overflow conversion int64 -> int32 (gosec)
[failure] 328-328:
G115: integer overflow conversion int64 -> uint32 (gosec)
[failure] 327-327:
G115: integer overflow conversion int64 -> int32 (gosec)
[failure] 351-351:
G115: integer overflow conversion int64 -> uint32 (gosec)
[failure] 350-350:
G115: integer overflow conversion int64 -> int32 (gosec)
🔇 Additional comments (16)
ledger/alonzo/alonzo.go (2)
457-465: LGTM! Asset quantity migration to BigInt encoding is correct.The conversion from
uint64to the nestedAsset_OutputCoin→BigInt→BigUIntstructure is implemented correctly usingnew(big.Int).SetUint64(amount).Bytes(). This properly encodes asset quantities as big-endian byte arrays, consistent with the utxorpc v1alpha migration.
485-489: LGTM! Coin amount migration to BigInt encoding is correct.The
Coinfield conversion to the nestedBigInt→BigUIntstructure is implemented correctly, consistent with the asset quantity encoding. The use ofnew(big.Int).SetUint64(o.Amount()).Bytes()properly encodes the coin amount for the utxorpc v1alpha schema.ledger/conway/pparams.go (3)
26-26: LGTM: Import alias and function signature updated correctly.The import alias change to
utxorpcand the updatedUtxorpc()signature returning*utxorpc.PParamsare consistent with the migration to utxorpc v1alpha schema across the codebase.Also applies to: 64-64
66-100: LGTM: Validation checks are complete and correct.The nil checks and bounds validation for rational numbers (A0, Rho, Tau, MemPrice, StepPrice) properly validate both lower (
MinInt32) and upper (MaxInt32) bounds before casting toint32. This prevents integer overflows and is consistent with the Shelley implementation.
101-153: LGTM: Field assignments correctly use utxorpc wrappers.The implementation properly:
- Uses
common.ToUtxorpcBigInthelper for numeric fields, which handles both int64 and BigUInt representations- Wraps rational numbers with
utxorpc.RationalNumber- Constructs
utxorpc.ProtocolVersion,utxorpc.ExPrices, andutxorpc.ExUnitstypes correctly- Maps all protocol parameter fields to their utxorpc equivalents
ledger/allegra/block_test.go (1)
155-156: LGTM: Fee assertion correctly updated for utxorpc BigInt.The assertion properly uses
GetInt()accessor to retrieve the fee value from the utxorpcBigIntwrapper and compares it withint64(0).ledger/conway/pparams_test.go (2)
485-600: LGTM: Test expectations correctly updated to utxorpc types.The test expectations properly use:
utxorpc.BigIntwrappers for numeric fields (CoinsPerUtxoByte, MinFeeCoefficient, etc.)utxorpc.RationalNumberfor rational fieldsutxorpc.ProtocolVersionfor protocol versionutxorpc.ExPricesandutxorpc.ExUnitsfor execution parametersutxorpc.CostModelsfor Plutus cost models
677-678: LGTM: Assertions correctly use utxorpc accessors.The test assertions properly:
- Use
GetInt()to retrieve fee values fromutxorpc.BigInt- Use
GetBigUInt()for coin values and convert viabig.Int.SetBytes()touint64- Handle both
BigInt_IntandBigInt_BigUIntrepresentations appropriatelyAlso applies to: 686-693, 760-761, 769-776
ledger/alonzo/pparams_test.go (2)
376-448: LGTM: Test expectations align with utxorpc migration.The Alonzo test expectations correctly use utxorpc wrapper types for all protocol parameter fields, consistent with the implementation and other test files in this PR.
547-548: LGTM: Assertions correctly handle utxorpc BigInt accessors.The test assertions properly use
GetInt()for fee comparisons andGetBigUInt()withbig.Intconversion for coin values, consistent with the utxorpc BigInt representation patterns used throughout the PR.Also applies to: 563-570, 616-617, 625-632
ledger/shelley/pparams_test.go (1)
129-163: LGTM: Shelley test updates are correct and consistent.The test changes properly:
- Update expected types to
*utxorpc.PParams,*utxorpc.TxInput, and*utxorpc.TxOutput- Use
utxorpc.BigIntwrappers for numeric fields- Use
GetInt()accessor for fee comparisons- Use
BigInt_BigUIntrepresentation for coin valuesAll changes align with the broader utxorpc migration pattern in this PR.
Also applies to: 194-194, 229-234, 291-295, 359-363
ledger/babbage/pparams_test.go (2)
30-30: LGTM: Using proto.Equal for utxorpc type comparison.The switch from
reflect.DeepEqualtoproto.Equalis the correct approach for comparing protobuf messages (utxorpc types). This provides more accurate comparison that understands protobuf semantics.Also applies to: 565-565, 591-591, 626-626
443-556: LGTM: Test expectations and assertions are correct.The Babbage test updates properly use utxorpc wrappers in expectations and correctly employ
GetInt()andGetBigUInt()accessors in assertions, consistent with the migration pattern throughout this PR.Also applies to: 662-663, 671-678, 713-714, 722-729
ledger/alonzo/pparams.go (2)
279-313: LGTM: Validation checks are comprehensive and correct.The nil checks and bounds validation for rational numbers properly validate both lower (
MinInt32) and upper (MaxInt32) bounds before casting toint32. This prevents integer overflows and is consistent across the codebase.
314-366: LGTM: Field assignments correctly use utxorpc wrappers.The implementation properly uses
common.ToUtxorpcBigInthelper for numeric fields and constructs all utxorpc types (RationalNumber, ProtocolVersion, ExPrices, ExUnits) correctly.Note: The G115 linter warnings for int64→int32/uint32 conversions at lines 327-351 are safe because the values are validated to be within the appropriate ranges (MinInt32/MaxInt32, 0/MaxUint32) before casting.
ledger/allegra/pparams_test.go (1)
95-129: LGTM: Allegra test updates are correct and consistent.The test changes properly:
- Update expected types to use
*utxorpc.PParamswith appropriate wrapper types- Use
GetInt()accessor for fee value comparisons- Maintain consistency with the utxorpc migration pattern used throughout this PR
Also applies to: 182-185, 244-247
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
d35e81c to
8e644bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- ledger/shelley/shelley.go
- ledger/common/tx.go
- ledger/alonzo/pparams.go
- ledger/common/certs.go
- ledger/byron/byron.go
- go.mod
- ledger/alonzo/alonzo.go
- ledger/allegra/block_test.go
- ledger/byron/byron_test.go
- ledger/babbage/babbage.go
🧰 Additional context used
🧬 Code graph analysis (11)
ledger/mary/mary.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/block_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/babbage/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/mary/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/babbage_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/shelley/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
🔇 Additional comments (11)
ledger/alonzo/pparams_test.go (1)
563-574: Past review comment correctly addressed!The coin value comparisons now properly handle the BigInt_BigUInt representation by converting bytes to uint64 before comparison. This is the correct approach, as noted in the past review discussion.
Also applies to: 628-639
ledger/babbage/pparams_test.go (2)
565-565: Good use of proto.Equal for protobuf comparison.Using
proto.Equalinstead ofreflect.DeepEqualis the preferred approach for comparing protobuf messages, as it understands protobuf semantics and handles default values correctly.
671-682: Correct handling of BigUInt coin values.The test properly handles coin values stored as BigUInt by converting the bytes to uint64 before comparison. This pattern correctly accommodates the BigInt wrapper representation.
Also applies to: 725-736
ledger/babbage/pparams.go (1)
195-203: Past review comment correctly addressed with ToUtxorpcBigInt helper.The use of
common.ToUtxorpcBigIntfor all coin-like fields prevents overflow issues that would occur when castinguint64values abovemath.MaxInt64toint64. The helper automatically selects the appropriate BigInt representation (Int or BigUInt) based on the value range.ledger/shelley/pparams_test.go (1)
129-163: Test expectations correctly migrated to utxorpc types.The expected PParams structure properly uses utxorpc wrappers (BigInt for numeric fields, RationalNumber for rationals, ProtocolVersion for version). This aligns with the utxorpc v1alpha schema.
ledger/shelley/pparams.go (1)
198-203: Past review comment resolved - overflow prevention implemented.The migration to
common.ToUtxorpcBigIntensures that unsigned protocol parameters exceedingmath.MaxInt64are correctly encoded as BigUInt rather than wrapping to negative values. This maintains correctness for the full uint64 range.ledger/conway/pparams.go (2)
66-100: Past review comments correctly addressed - validation now complete.The validation now properly checks both lower and upper bounds for all rational number numerators:
- A0, Rho, and Tau now validate
< math.MinInt32 && > math.MaxInt32- ExecutionCosts.MemPrice and StepPrice have the same complete validation
- Line 95 now correctly uses
math.MinInt32for the lower bound check (previously was incorrectly usingmath.MaxInt32)This prevents int32 overflow when casting the numerator values.
103-111: Correct use of ToUtxorpcBigInt for coin-like fields.Consistent with other eras, coin-like fields use the
common.ToUtxorpcBigInthelper to prevent overflow issues.ledger/allegra/pparams_test.go (1)
95-129: Clean migration to utxorpc types.The test expectations correctly use utxorpc.PParams with appropriate wrappers for all field types (BigInt, RationalNumber, ProtocolVersion). This is consistent with the migration pattern across other era tests.
ledger/mary/pparams_test.go (2)
514-529: Past review comment excellently addressed - both BigInt variants handled.The test now correctly handles MinPoolCost values in both representations:
- When encoded as BigUInt (large values like math.MaxUint64), it converts bytes to uint64
- When encoded as Int (small values), it uses GetInt()
This comprehensive approach ensures the test works correctly across the full uint64 range, including the maximum value test case at line 501.
99-136: Well-structured test expectations with proper utxorpc wrappers.The expected PParams structure correctly represents all fields using utxorpc types, including the MinPoolCost field that was the focus of the past review concern.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
8e644bb to
49670f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
ledger/byron/byron.go (1)
499-503: Consider usingcommon.ToUtxorpcBigIntfor consistency.While the current implementation is correct, using
common.ToUtxorpcBigInt(o.Amount())would provide consistency across the codebase and optimize for smaller values by using theIntvariant when possible.Apply this diff:
- Coin: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_BigUInt{ - BigUInt: new(big.Int).SetUint64(o.Amount()).Bytes(), - }, - }, + Coin: common.ToUtxorpcBigInt(o.Amount()),ledger/mary/mary.go (1)
539-543: Consider usingcommon.ToUtxorpcBigIntfor consistency.The current implementation is correct, but using
common.ToUtxorpcBigInt(o.Amount())would align with the codebase pattern and optimize for values withinint64range.Apply this diff:
- Coin: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_BigUInt{ - BigUInt: new(big.Int).SetUint64(o.Amount()).Bytes(), - }, - }, + Coin: common.ToUtxorpcBigInt(o.Amount()),ledger/shelley/shelley.go (1)
470-474: Consider usingcommon.ToUtxorpcBigIntfor consistency.Using
common.ToUtxorpcBigInt(o.Amount())would provide consistency and potential optimization for smaller coin values.Apply this diff:
- Coin: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_BigUInt{ - BigUInt: new(big.Int).SetUint64(o.Amount()).Bytes(), - }, - }, + Coin: common.ToUtxorpcBigInt(o.Amount()),ledger/alonzo/alonzo.go (2)
457-464: Consider usingcommon.ToUtxorpcBigIntfor asset quantities.For consistency with the codebase pattern, consider using the helper function for asset quantity encoding.
Apply this diff:
asset := &utxorpc.Asset{ Name: assetName, Quantity: &utxorpc.Asset_OutputCoin{ - OutputCoin: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_BigUInt{ - BigUInt: new(big.Int).SetUint64(amount).Bytes(), - }, - }, + OutputCoin: common.ToUtxorpcBigInt(amount), }, }
485-489: Consider usingcommon.ToUtxorpcBigIntfor output coin.Using the helper function would provide consistency with the codebase pattern.
Apply this diff:
- Coin: &utxorpc.BigInt{ - BigInt: &utxorpc.BigInt_BigUInt{ - BigUInt: new(big.Int).SetUint64(o.Amount()).Bytes(), - }, - }, + Coin: common.ToUtxorpcBigInt(o.Amount()),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- ledger/byron/block_test.go
- ledger/babbage/babbage.go
- ledger/babbage/babbage_test.go
- ledger/common/certs.go
- go.mod
- ledger/allegra/pparams_test.go
🧰 Additional context used
🧬 Code graph analysis (14)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/alonzo/alonzo.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/mary/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/conway/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/mary/mary.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams_test.go (3)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/byron.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/pparams_test.go (3)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/data.go (1)
Datum(25-28)
ledger/shelley/shelley.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
ledger/allegra/block_test.go (1)
153-158: The review comment is incorrect and based on a false premise.The Fee field is a
*utxorpc.BigInttype initialized withBigInt_Int{Int: int64(tx.Fee())}, notBigUInt. TheGetInt()method is the correct and only accessor for this signed BigInt type. The test code callingfirstRpcTx.Fee.GetInt()without a nil check is correct because the Fee field is always initialized as a non-nil BigInt during transaction conversion.Likely an incorrect or invalid review comment.
ledger/common/common.go (1)
496-508: LGTM! Well-designed helper for utxorpc BigInt conversion.The
ToUtxorpcBigInthelper correctly handles the boundary between signed and unsigned representations atmath.MaxInt64, using the more efficientIntvariant when possible and falling back toBigUIntfor larger values.ledger/mary/pparams.go (1)
167-194: LGTM! Protocol parameter encoding correctly uses utxorpc types.The implementation properly uses
common.ToUtxorpcBigIntfor all numeric coin/deposit fields, avoiding potential overflow issues. The rational number and protocol version encodings are also correctly structured.ledger/babbage/pparams_test.go (2)
443-573: LGTM! Test expectations correctly updated for utxorpc types.The test properly uses
utxorpc.PParamsstructure withBigIntwrappers for monetary fields,RationalNumberfor ratios, andproto.Equalfor comparisons. All field mappings align with the new schema.
662-682: LGTM! Proper usage of BigInt accessors in assertions.The tests correctly use
GetInt()for fee values andGetBigUInt()for coin values, with appropriate conversion from bytes to uint64 for the latter. This properly validates the utxorpc BigInt wrapper behavior.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
2 times, most recently
from
927e9d7 to
0d94b8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(4 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- ledger/common/common.go
- ledger/common/certs.go
- ledger/allegra/block_test.go
- ledger/shelley/shelley.go
- ledger/common/tx.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.651Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.651Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
go.mod
🧬 Code graph analysis (17)
ledger/mary/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/conway/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/byron/block_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/shelley/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/babbage/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/mary/mary.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/alonzo.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/babbage.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/byron_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/byron/byron.go (2)
ledger/common/script/wrappers.go (1)
Coin(112-112)ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/shelley/pparams_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/babbage_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams_test.go (1)
ledger/common/common.go (2)
ExUnits(487-491)ToUtxorpcBigInt(497-508)
🔇 Additional comments (7)
go.mod (1)
15-18: Dependency bump lines look consistent with the generated proto updates and match the API changes elsewhere in the PR.ledger/babbage/babbage_test.go (1)
2794-2798: Assertion now unpacks the BigUInt representation, which aligns with the updatedTxOutputschema—tests remain meaningful after the type change.ledger/byron/byron.go (1)
498-500: Switching tocommon.ToUtxorpcBigIntkeeps Byron outputs consistent with the new helper and handles both int and big-uint cases cleanly.ledger/mary/mary.go (1)
533-541: Mary outputs now reuse the shared BigInt helper, so encoding stays consistent across eras without duplicating conversion logic.ledger/byron/block_test.go (1)
147-163: The test now branches on the BigInt variant, so it keeps verifying coin positivity regardless of whether the helper returns int or big-uint.ledger/babbage/babbage.go (1)
640-669: Embedding the BigUInt conversion directly for assets matches the proto schema and prevents overflow truncation—looks good.ledger/alonzo/alonzo.go (1)
457-483: Alonzo outputs now share the utility helper, keeping the era in sync with the new BigInt proto expectations.
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
0d94b8a to
f090dcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ledger/babbage/babbage_test.go (1)
794-798: Fix coin assertion to check both BigInt representations.For value 1000 (≤ MaxInt64),
ToUtxorpcBigIntstores it asBigInt_Int{Int: 1000}, soGetBigUInt()returns nil and this assertion is skipped entirely. Please check both representations:- assert.Equal( - t, - uint64(1000), - new(big.Int).SetBytes(txOutput.Coin.GetBigUInt()).Uint64(), - ) + coin := txOutput.Coin + if bigUInt := coin.GetBigUInt(); bigUInt != nil { + coinValue := new(big.Int).SetBytes(bigUInt).Uint64() + assert.Equal(t, uint64(1000), coinValue) + } else { + assert.Equal(t, int64(1000), coin.GetInt()) + }ledger/conway/pparams.go (1)
107-158: Re-add Conway governance fields to utxorpc payload.
Utxorpc()currently stops at the shared Shelley/Babbage-era fields, so every Conway-only parameter (pool/drep voting thresholds, committee sizing, governance action timers & deposits, DRep deposits/inactivity, min fee ref script cost) gets dropped on the floor. Consumers rely on these values (see the CIP‑139 protocol-parameter schema and downstream SDK docs), and without them they fall back to zeros, which is a correctness break for on-chain governance flows.(cips.cardano.org)Please map the remaining Conway fields before returning the message. Concretely:
- Encode
PoolVotingThresholds/DRepVotingThresholdsas the respective utxorpc structs.- Marshal
MinCommitteeSize,CommitteeTermLimit,GovActionValidityPeriod,GovActionDeposit,DRepDeposit,DRepInactivityPeriod.- Convert
MinFeeRefScriptCostPerByteinto autxorpc.RationalNumber.Otherwise the Conway export deviates from the spec and downstream RPCs will misreport the live parameters.
Also applies to: 209-260
♻️ Duplicate comments (1)
ledger/alonzo/pparams_test.go (1)
624-635: Critical: Duplicate issue - coin validation is never executed.Same issue as lines 559-570: the validation only runs if
GetBigUInt() != nil, but the test value (2000) is stored asIntrepresentation byToUtxorpcBigInt, so this entire validation block is skipped.Apply the same fix as suggested for lines 559-570:
- if got.Outputs[0].Coin.GetBigUInt() != nil { - coinValue := new( - big.Int, - ).SetBytes(got.Outputs[0].Coin.GetBigUInt()). - Uint64() - if coinValue != uint64(2000) { - t.Errorf( - "Output coin mismatch: got %d, want 2000", - coinValue, - ) - } + var coinValue uint64 + if got.Outputs[0].Coin.GetBigUInt() != nil { + coinValue = new(big.Int).SetBytes(got.Outputs[0].Coin.GetBigUInt()).Uint64() + } else { + coinValue = uint64(got.Outputs[0].Coin.GetInt()) + } + if coinValue != uint64(2000) { + t.Errorf( + "Output coin mismatch: got %d, want 2000", + coinValue, + ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(3 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/allegra/block_test.go
- ledger/common/certs.go
- ledger/common/tx.go
- ledger/babbage/babbage.go
- ledger/byron/byron.go
- ledger/mary/mary.go
- ledger/byron/block_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T15:54:22.683Z
Learnt from: wolf31o2
Repo: blinklabs-io/gouroboros PR: 1093
File: ledger/mary/pparams.go:143-150
Timestamp: 2025-11-04T15:54:22.683Z
Learning: In the blinklabs-io/gouroboros repository, the design goal for CBOR round-trip tests is to achieve byte-identical encoding WITHOUT using stored CBOR (cbor.DecodeStoreCbor). Instead, the approach uses proper field types (pointers for optional fields) and relies on the cbor package's deterministic encoding (SortCoreDeterministic) to ensure reproducible output. The stored CBOR pattern should not be suggested as a solution for round-trip fidelity in this codebase.
Applied to files:
go.mod
🧬 Code graph analysis (14)
ledger/babbage/pparams_test.go (3)
ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(487-491)ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)
ledger/shelley/shelley.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/mary/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/mary/pparams_test.go (2)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/conway/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/babbage/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/shelley/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/alonzo.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/pparams_test.go (1)
ledger/common/common.go (2)
ExUnits(487-491)ToUtxorpcBigInt(497-508)
ledger/byron/byron_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/shelley/pparams_test.go (3)
ledger/babbage/babbage_test.go (1)
TxOutput(2771-2775)ledger/common/script/wrappers.go (1)
Coin(112-112)ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/babbage_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/conway/pparams_test.go (2)
ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
go.mod (1)
15-15: LGTM! Dependency updates align with utxorpc migration.The bump to
utxorpc/go-codegen v0.18.1and addition ofgoogle.golang.org/protobuf v1.36.10support the broader migration to utxorpc v1alpha types across ledger packages.Also applies to: 18-18
ledger/common/common.go (1)
496-508: LGTM! Well-designed BigInt conversion helper.The
ToUtxorpcBigInthelper correctly:
- Uses the compact
BigInt_Intrepresentation for typical values (≤ MaxInt64)- Falls back to
BigInt_BigUIntbyte encoding for edge cases exceeding MaxInt64This provides an efficient, reusable conversion path for the utxorpc migration.
ledger/shelley/shelley.go (1)
470-470: LGTM! Coin field correctly uses ToUtxorpcBigInt.The conversion properly wraps the amount in a utxorpc BigInt using the new helper.
ledger/mary/pparams.go (1)
169-175: LGTM! Fee and deposit fields correctly use ToUtxorpcBigInt.All monetary fields (MinFeeCoefficient, MinFeeConstant, StakeKeyDeposit, PoolDeposit, MinPoolCost) properly leverage the helper to avoid overflow issues.
ledger/alonzo/alonzo.go (1)
458-460: LGTM! Asset quantities and coin amounts correctly wrapped.Both per-asset quantities (lines 458-460) and the output coin (line 481) properly use
ToUtxorpcBigIntfor consistent utxorpc representation.Also applies to: 481-481
ledger/conway/pparams_test.go (1)
686-696: LGTM! Coin assertions correctly check both BigInt representations.The tests properly validate both the
BigInt_Intpath (typical values) and theBigInt_BigUIntpath (large values), ensuring complete coverage.Also applies to: 772-782
ledger/byron/byron_test.go (3)
24-24: LGTM! Import alias updated correctly.The import alias has been properly updated from
cardanotoutxorpcto align with the migration to utxorpc v1alpha types.
38-41: LGTM! Correct usage of utxorpc types and helper.The test correctly uses
utxorpc.TxInputandutxorpc.TxOutputtypes, and properly employscommon.ToUtxorpcBigIntfor coin encoding.Also applies to: 64-67
103-105: LGTM! Fee accessor usage is appropriate for zero-value check.Using
GetInt()to check for a zero fee in an empty transaction is acceptable, as both Int and BigUInt representations of zero would return 0 fromGetInt().ledger/shelley/pparams.go (1)
172-223: LGTM! Protocol parameters migration completed correctly.The
Utxorpc()method has been properly updated to return*utxorpc.PParamswith correct field mappings:
- Numeric coin fields use
common.ToUtxorpcBigIntto safely handle uint64 values (preventing overflow issues addressed in previous reviews)- Rational fields properly use
*utxorpc.RationalNumber- Protocol version correctly uses
*utxorpc.ProtocolVersionBased on learnings
ledger/allegra/pparams_test.go (3)
27-27: LGTM! Import alias updated correctly.
95-129: LGTM! Test expectations properly use utxorpc types.The expected
utxorpc.PParamsstructure correctly uses:
*utxorpc.BigIntwrappers for numeric coin fields*utxorpc.RationalNumberfor rational fields*utxorpc.ProtocolVersionfor protocol version
182-186: LGTM! Fee accessor usage is appropriate.Using
GetInt()for fee comparisons is correct here since the test values (200, 150) are well within the int64 range and will be stored using the Int representation byToUtxorpcBigInt.Also applies to: 244-249
ledger/alonzo/pparams_test.go (2)
33-33: LGTM! Migration to utxorpc types completed correctly.The test properly uses
utxorpc.PParamswith all field types correctly mapped to their utxorpc equivalents, including*utxorpc.BigInt,*utxorpc.RationalNumber,*utxorpc.ProtocolVersion,*utxorpc.CostModels,*utxorpc.ExPrices, and*utxorpc.ExUnits.Also applies to: 376-448, 489-496
543-544: LGTM! Fee comparisons use correct accessor.Using
GetInt()for fee comparisons is appropriate since the test values (50, 75) are well within the int64 range.Also applies to: 615-616
Signed-off-by: Chris Gianelloni <[email protected]>
wolf31o2
force-pushed
the
chore/deps-utxorpc-0.18.1
branch
from
f090dcf to
0f186c6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (24)
go.mod(1 hunks)ledger/allegra/block_test.go(1 hunks)ledger/allegra/pparams_test.go(5 hunks)ledger/alonzo/alonzo.go(2 hunks)ledger/alonzo/pparams.go(3 hunks)ledger/alonzo/pparams_test.go(6 hunks)ledger/babbage/babbage.go(2 hunks)ledger/babbage/babbage_test.go(1 hunks)ledger/babbage/pparams.go(3 hunks)ledger/babbage/pparams_test.go(8 hunks)ledger/byron/block_test.go(2 hunks)ledger/byron/byron.go(1 hunks)ledger/byron/byron_test.go(5 hunks)ledger/common/certs.go(2 hunks)ledger/common/common.go(3 hunks)ledger/common/tx.go(1 hunks)ledger/conway/pparams.go(3 hunks)ledger/conway/pparams_test.go(6 hunks)ledger/mary/mary.go(1 hunks)ledger/mary/pparams.go(3 hunks)ledger/mary/pparams_test.go(7 hunks)ledger/shelley/pparams.go(3 hunks)ledger/shelley/pparams_test.go(7 hunks)ledger/shelley/shelley.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/byron/byron.go
- ledger/shelley/shelley.go
- go.mod
- ledger/babbage/babbage.go
- ledger/mary/pparams_test.go
- ledger/byron/block_test.go
- ledger/conway/pparams.go
🧰 Additional context used
🧬 Code graph analysis (15)
ledger/mary/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/common/tx.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/shelley/pparams_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/common/certs.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/pparams_test.go (1)
ledger/common/common.go (2)
ExUnits(487-491)ToUtxorpcBigInt(497-508)
ledger/alonzo/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/conway/pparams_test.go (4)
protocol/versions.go (1)
ProtocolVersion(26-42)ledger/alonzo/genesis.go (1)
CostModel(39-39)ledger/common/common.go (1)
ExUnits(487-491)ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/alonzo/pparams_test.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/mary/mary.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/shelley/pparams.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/babbage/babbage_test.go (1)
ledger/common/script/wrappers.go (1)
Coin(112-112)
ledger/babbage/pparams.go (1)
ledger/common/common.go (2)
ToUtxorpcBigInt(497-508)ExUnits(487-491)
ledger/byron/byron_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/alonzo/alonzo.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
ledger/allegra/pparams_test.go (1)
ledger/common/common.go (1)
ToUtxorpcBigInt(497-508)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
ledger/common/common.go (1)
496-508: LGTM! Well-designed overflow-safe helper.The
ToUtxorpcBigIntfunction correctly handles the uint64-to-BigInt conversion by using the efficientBigInt_Intvariant for values that fit withinmath.MaxInt64, and falling back toBigInt_BigUIntwith byte encoding for larger values. This prevents overflow issues that would occur with naive int64 casts.ledger/alonzo/alonzo.go (1)
457-461: Correct application of overflow-safe conversion.The changes properly apply
common.ToUtxorpcBigIntto both per-asset quantities and the output coin amount, ensuring large values are handled correctly without overflow.Also applies to: 481-481
ledger/allegra/block_test.go (1)
155-157: Test correctly updated for BigInt accessor.The test properly adapts to the new
BigIntwrapper by using theGetInt()accessor method for the fee value comparison.ledger/mary/mary.go (1)
539-539: Consistent overflow-safe coin conversion.The Mary transaction output correctly uses
common.ToUtxorpcBigIntfor the coin amount, maintaining consistency with other era implementations.ledger/common/tx.go (1)
239-239: Fee overflow issue properly resolved.The transaction fee now uses
ToUtxorpcBigInt, correctly addressing the overflow risk from the previous naive int64 cast. This ensures large fee values are handled safely.ledger/common/certs.go (1)
593-594: Certificate amount conversions now overflow-safe.The
Pledge,Cost, andDeltaCoinfields correctly useToUtxorpcBigIntto prevent overflow when converting large uint64 values to BigInt representations. This addresses the data corruption risks identified in previous reviews.Also applies to: 761-761
ledger/babbage/babbage_test.go (1)
2794-2800: Test correctly handles both BigInt variants.The test properly accommodates both
BigInt_IntandBigInt_BigUIntrepresentations by checkingGetBigUInt()first and converting from bytes, then falling back toGetInt()for smaller values. This comprehensive approach ensures the test works regardless of which variant is used.ledger/mary/pparams.go (2)
169-175: Protocol parameter numeric fields now overflow-safe.All coin and fee-related parameters (
MinFeeA,MinFeeB,KeyDeposit,PoolDeposit,MinPoolCost) correctly usecommon.ToUtxorpcBigIntto prevent overflow issues. This addresses the concerns from previous reviews and ensures large parameter values are handled safely.
178-193: Correct utxorpc wrapper usage for rational and version fields.The rational number fields (
PoolInfluence,MonetaryExpansion,TreasuryExpansion) andProtocolVersionare properly wrapped in their respective utxorpc types, with appropriate bounds checking performed earlier in the function (lines 144-165).ledger/alonzo/pparams_test.go (1)
547-558: Robust coin validation across both BigInt encodingsNice touch covering both the BigUInt and Int variants; this keeps the test resilient regardless of which branch
ToUtxorpcBigIntchooses and saves us from silent regressions.
| coin := got.Outputs[0].Coin | ||
| if bigInt := coin.GetBigUInt(); bigInt != nil { | ||
| coinValue := new(big.Int).SetBytes(bigInt).Uint64() | ||
| if coinValue != uint64(5000) { | ||
| t.Errorf( | ||
| "Output coin mismatch: got %d, want 5000", | ||
| coinValue, | ||
| ) | ||
| } | ||
| } else if coin.GetInt() != 5000 { | ||
| t.Errorf("Output coin mismatch: got %d, want 5000", coin.GetInt()) | ||
| } |
There was a problem hiding this comment.
Fail fast before dereferencing got.Outputs[0].
Here too the length guard logs with t.Errorf, so the test keeps running and immediately reads got.Outputs[0], which will panic when the slice is empty. Keeping the test alive defeats the purpose of the check and hides the real failure behind an index-out-of-range panic. (staticcheck.dev) Switch to t.Fatalf (or return) so the test stops before indexing.
@@
- if len(got.Outputs) != 1 {
- t.Errorf("Expected 1 output, got %d", len(got.Outputs))
- }
+ if len(got.Outputs) != 1 {
+ t.Fatalf("Expected 1 output, got %d", len(got.Outputs))
+ }
@@
- if len(got.Outputs) != 1 {
- t.Errorf("Expected 1 output, got %d", len(got.Outputs))
- }
+ if len(got.Outputs) != 1 {
+ t.Fatalf("Expected 1 output, got %d", len(got.Outputs))
+ }Also applies to: 721-732
🤖 Prompt for AI Agents
In ledger/babbage/pparams_test.go around lines 667 to 678 (and also apply the
same change at lines 721-732), the test currently uses t.Errorf for the length
guard but then immediately indexes got.Outputs[0], which can cause an
index-out-of-range panic when the slice is empty; change the length-check error
calls to t.Fatalf (or return after logging) so the test fails immediately and
does not dereference got.Outputs[0], preserving the original guard and keeping
the rest of the test unchanged.
| if len(got.Outputs) != 1 { | ||
| t.Errorf("Expected 1 output, got %d", len(got.Outputs)) | ||
| } | ||
| if got.Outputs[0].Coin != 5000 { | ||
| t.Errorf("Output coin mismatch: got %d, want 5000", got.Outputs[0].Coin) | ||
| coin := got.Outputs[0].Coin | ||
| if bigInt := coin.GetBigUInt(); bigInt != nil { | ||
| coinValue := new(big.Int).SetBytes(bigInt).Uint64() | ||
| if coinValue != uint64(5000) { | ||
| t.Errorf( | ||
| "Output coin mismatch: got %d, want 5000", | ||
| coinValue, | ||
| ) | ||
| } | ||
| } else if coin.GetInt() != 5000 { | ||
| t.Errorf("Output coin mismatch: got %d, want 5000", coin.GetInt()) | ||
| } |
There was a problem hiding this comment.
Stop after the output-count check before indexing.
t.Errorf only marks the test as failed and continues, so when len(got.Outputs) is zero the next line still dereferences got.Outputs[0], which will panic with an index-out-of-range error instead of reporting a clear assertion failure. (staticcheck.dev) Accessing a slice element when the slice is empty triggers a runtime panic. (slingacademy.com) Use t.Fatalf (or return) so the test aborts before touching got.Outputs[0].
@@
- if len(got.Outputs) != 1 {
- t.Errorf("Expected 1 output, got %d", len(got.Outputs))
- }
+ if len(got.Outputs) != 1 {
+ t.Fatalf("Expected 1 output, got %d", len(got.Outputs))
+ }
@@
- if len(got.Outputs) != 1 {
- t.Errorf("Expected 1 output, got %d", len(got.Outputs))
- }
+ if len(got.Outputs) != 1 {
+ t.Fatalf("Expected 1 output, got %d", len(got.Outputs))
+ }Also applies to: 775-786
🤖 Prompt for AI Agents
In ledger/conway/pparams_test.go around lines 683 to 697 (and similarly
775-786), the test uses t.Errorf to report a wrong output count but then
unconditionally indexes got.Outputs[0], which can panic if the slice is empty;
change the flow to abort the test after the output-count check by replacing
t.Errorf with t.Fatalf (or return) so the test stops before any indexing, and
apply the same fix to the other occurrence at lines 775-786 to prevent
slice-out-of-range panics and produce a clear assertion failure.
Summary by CodeRabbit