fix(ledger): certificate deposit validation #1254
Conversation
📝 WalkthroughWalkthroughThe diff introduces a new exported CertificateType (uint) and retypes certificate constants and uses to that type. Many test fixtures across eras were updated to cast certificate Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (7)ledger/alonzo/rules_test.go (1)
ledger/conway/rules_test.go (1)
ledger/allegra/rules_test.go (1)
ledger/shelley/rules_test.go (1)
ledger/common/certs.go (1)
ledger/common/common_test.go (1)
ledger/mary/rules_test.go (1)
⏰ 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)
🔇 Additional comments (10)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wolf31o2
force-pushed
the
fix/deposit-validation
branch
2 times, most recently
from
6072656 to
1e1dadb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ledger/shelley/errors.go (1)
147-158: New InvalidCertificateDepositError type is sound; consider a stronger CertificateType type aliasThe struct and
Error()implementation are correct and consistent with the existing error patterns in this file, and the formatted message is clear. If you already have a dedicated certificate type enum/alias elsewhere in the ledger (rather than plainuint), wiring that in here forCertificateTypewould tighten compile-time checks and avoid accidental mismatches; otherwise this is fine to keep as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ledger/allegra/rules_test.go(1 hunks)ledger/alonzo/rules_test.go(1 hunks)ledger/babbage/rules_test.go(1 hunks)ledger/conway/rules.go(2 hunks)ledger/conway/rules_test.go(1 hunks)ledger/mary/rules_test.go(1 hunks)ledger/shelley/errors.go(1 hunks)ledger/shelley/rules_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ledger/conway/rules_test.go
- ledger/mary/rules_test.go
- ledger/allegra/rules_test.go
- ledger/babbage/rules_test.go
- ledger/conway/rules.go
- ledger/alonzo/rules_test.go
- ledger/shelley/rules_test.go
wolf31o2
force-pushed
the
fix/deposit-validation
branch
from
1e1dadb to
501fde2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ledger/common/certs.go (1)
61-68: Resolve G115 int→uint conversion warning in UnmarshalCBOR
gosecis flagging the implicitint -> uintconversion inswitch CertificateType(certType)(line 66). Even if CBOR IDs are guaranteed non‑negative, the unchecked cast is what’s breaking the pipeline.You can both harden the code and quiet G115 by validating the sign once and converting to a
uintvariable that you then reuse:func (c *CertificateWrapper) UnmarshalCBOR(data []byte) error { // Determine cert type - certType, err := cbor.DecodeIdFromList(data) + certType, err := cbor.DecodeIdFromList(data) if err != nil { return err } - var tmpCert Certificate - switch CertificateType(certType) { + if certType < 0 { + return fmt.Errorf("negative certificate type: %d", certType) + } + certTypeUint := uint(certType) // #nosec G115 -- validated as non-negative + var tmpCert Certificate + switch CertificateType(certTypeUint) { @@ - // certType is known within uint range - c.Type = uint(certType) // #nosec G115 + // certType validated and cached as uint + c.Type = certTypeUint c.Certificate = tmpCert return nil }This preserves behavior, adds a defensive check for malformed CBOR, and should keep
gosechappy by ensuring the only int→uint cast is guarded and explicitly annotated.Also applies to: 115-116
🧹 Nitpick comments (3)
ledger/common/certs.go (1)
29-51: CertificateType alias and typed constants look good; consider propagating type usage laterDefining
type CertificateType uintand explicitly typing the certificate constants improves clarity and enables stronger typing for things likeInvalidCertificateDepositError. Right now, most struct fields (e.g.,CertificateWrapper.Type,CertTypeon individual certs) remain plainuint, so you don't fully benefit from the new alias yet. Not urgent, but over time it may be worth migrating those fields toCertificateTypefor additional type safety.ledger/conway/rules.go (1)
255-270: Conway deposit/refund handling looks solid overall; double‑check DeregistrationCertificate and zero‑amount assumptionsThe updated
UtxoValidateValueNotConservedUtxologic now:
Treats deposits correctly on the produced side for:
*common.PoolRegistrationCertificate(usingPoolDepositwhen not already registered),*common.RegistrationCertificate,*common.RegistrationDrepCertificate,*common.StakeRegistrationCertificate(usingKeyDeposit),*common.StakeRegistrationDelegationCertificate,*common.StakeVoteRegistrationDelegationCertificate,*common.VoteRegistrationDelegationCertificate,
withInvalidCertificateDepositErrorreturned on non‑positiveAmountfor the Amount‑based types.Treats refunds on the consumed side for:
*common.StakeDeregistrationCertificate(KeyDeposit),*common.DeregistrationCertificate(KeyDeposit),*common.DeregistrationDrepCertificate(Amount, with the same non‑positive guard).The pattern of checking
Amount <= 0before casting touint64is good: it both enforces a sane ledger invariant and avoids negative→unsigned wraparound.Two things worth explicitly confirming against the Conway ledger spec:
DeregistrationCertificate refund amount
common.DeregistrationCertificatehas anAmount int64field, but here you still refund usingtmpPparams.KeyDepositand ignoretmpCert.Amount, while the correspondingRegistrationCertificatedeposit is purely Amount‑based. If registrations can have deposit Amounts that differ fromKeyDeposit, this asymmetry could still skew the consumed/produced balance. If, conversely, the spec guaranteesAmountis either unused or always equal toKeyDepositfor this cert, the current logic is fine—but it would be good to confirm.Zero‑deposit certificates
For all the Amount‑driven certs (registration and DRep/delegation variants), you treatAmount <= 0as invalid. That matches the intent described in the PR, but it does bake in the assumption that “zero deposit” variants are always illegal. If the spec ever allows anAmount == 0case (e.g., for future parameter changes), these transactions would now be rejected up‑front byInvalidCertificateDepositError.Functionally, this change is a big step toward correct accounting of certificate deposits/refunds in Conway; I’d just recommend double‑checking those two assumptions to avoid subtle future mismatches.
Also applies to: 279-330
ledger/common/common_test.go (1)
756-875: Certificate type casting and expectations look correctThe updated cases consistently set
CertType: uint(CertificateTypeX)and assertexpected: uint(CertificateTypeX), which matches the structs’CertType uintfields and theCertificate.Type() uintcontract. The mapping across all the defined certificate types looks complete and correct.If you want to future‑proof this, you could add a small meta‑test that asserts this table covers every
CertificateType*constant so new types can’t be added without updating this test, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ledger/allegra/rules_test.go(2 hunks)ledger/alonzo/rules_test.go(2 hunks)ledger/babbage/rules_test.go(2 hunks)ledger/common/certs.go(3 hunks)ledger/common/common_test.go(1 hunks)ledger/conway/rules.go(2 hunks)ledger/conway/rules_test.go(2 hunks)ledger/mary/rules_test.go(2 hunks)ledger/shelley/errors.go(1 hunks)ledger/shelley/rules_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ledger/conway/rules_test.go
- ledger/shelley/rules_test.go
- ledger/allegra/rules_test.go
- ledger/mary/rules_test.go
🧰 Additional context used
🧬 Code graph analysis (5)
ledger/alonzo/rules_test.go (1)
ledger/common/certs.go (2)
CertificateTypeStakeRegistration(32-32)CertificateTypeStakeDeregistration(33-33)
ledger/conway/rules.go (2)
ledger/common/certs.go (7)
DeregistrationCertificate(825-831)DeregistrationDrepCertificate(1259-1265)CertificateType(29-29)RegistrationDrepCertificate(1208-1215)StakeRegistrationDelegationCertificate(961-968)StakeVoteRegistrationDelegationCertificate(1052-1060)VoteRegistrationDelegationCertificate(1004-1011)ledger/shelley/errors.go (1)
InvalidCertificateDepositError(147-150)
ledger/common/common_test.go (1)
ledger/common/certs.go (25)
CertificateTypeStakeRegistration(32-32)StakeDeregistrationCertificate(253-258)CertificateTypeStakeDeregistration(33-33)CertificateTypeStakeDelegation(34-34)PoolRegistrationCertificate(432-445)CertificateTypePoolRegistration(35-35)PoolRetirementCertificate(614-620)CertificateTypePoolRetirement(36-36)CertificateTypeGenesisKeyDelegation(37-37)CertificateTypeMoveInstantaneousRewards(38-38)RegistrationCertificate(784-790)CertificateTypeRegistration(39-39)DeregistrationCertificate(825-831)CertificateTypeDeregistration(40-40)CertificateTypeVoteDelegation(41-41)CertificateTypeStakeVoteDelegation(42-42)CertificateTypeStakeRegistrationDelegation(43-43)CertificateTypeVoteRegistrationDelegation(44-44)CertificateTypeStakeVoteRegistrationDelegation(45-45)CertificateTypeAuthCommitteeHot(46-46)CertificateTypeResignCommitteeCold(47-47)CertificateTypeRegistrationDrep(48-48)CertificateTypeDeregistrationDrep(49-49)UpdateDrepCertificate(1300-1306)CertificateTypeUpdateDrep(50-50)
ledger/shelley/errors.go (1)
ledger/common/certs.go (1)
CertificateType(29-29)
ledger/babbage/rules_test.go (1)
ledger/common/certs.go (2)
CertificateTypeStakeRegistration(32-32)CertificateTypeStakeDeregistration(33-33)
🪛 GitHub Actions: golangci-lint
ledger/common/certs.go
[error] 66-66: golangci-lint (gosec) G115: integer overflow conversion int -> uint detected at line 66. Command: golangci-lint run
🪛 GitHub Check: lint
ledger/common/certs.go
[failure] 66-66:
G115: integer overflow conversion int -> uint (gosec)
⏰ 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 (4)
ledger/common/certs.go (1)
1379-1381: LeiosEbCertificate.Type now correctly uses the typed constantReturning
uint(CertificateTypeLeiosEb)keeps the publicType() uintsignature while tying the value back to the strongly‑typed constant. This is a clean way to keep compatibility with existing callers.ledger/alonzo/rules_test.go (1)
561-595: Test updates correctly follow the new CertificateType representationCasting
common.CertificateTypeStakeRegistration/CertificateTypeStakeDeregistrationtouintforCertificateWrapper.Typekeeps the tests consistent with the newCertificateTypealias and theuint-typed field, without changing semantics.ledger/shelley/errors.go (1)
146-158: New InvalidCertificateDepositError cleanly surfaces bad certificate depositsThe
InvalidCertificateDepositErrortype is well‑shaped for the new Conway validation: it carries both the typed certificate kind and the offendingAmount, and the Error() message is consistent with the other error types in this file. This should make diagnosing malformed certificate deposits straightforward.ledger/babbage/rules_test.go (1)
561-595: Babbage value‑conservation tests correctly updated for CertificateTypeUsing
uint(common.CertificateTypeStakeRegistration)/StakeDeregistrationforCertificateWrapper.Typebrings these tests in line with the newCertificateTypealias and the underlyinguintfield type, matching the pattern used in other era tests.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
fix/deposit-validation
branch
from
501fde2 to
e86696b
Compare
Closes #1250
Summary by CodeRabbit
New Features
Bug Fixes
Refactor