feat: decode support for additional Handshake covenants #450
Conversation
📝 WalkthroughWalkthroughThis PR adds explicit concrete covenant types and conversion constructors for many Handshake covenant variants. It introduces NoneCovenant, ClaimCovenant, OpenCovenant, BidCovenant, RevealCovenant, RedeemCovenant, RenewCovenant, TransferCovenant, FinalizeCovenant, and RevokeCovenant, each with an isCovenant() marker and NewFromGeneric(gc *GenericCovenant) constructors. Each constructor validates covenant type and item counts, enforces per-item length checks, decodes little-endian numeric fields (uint32/uint8), deep-copies hash fields, and populates RawName/NameHash where applicable. Covenant() dispatching was extended to route to these constructors and panics on conversion errors or unknown types. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Files/areas needing extra attention:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
internal/handshake/covenant.go (2)
65-142: Consider returning errors instead of panicking in theCovenant()method.Panicking in library code can crash caller applications unexpectedly. While the current pattern is consistent with the existing Register/Update handling, consider changing the signature to
Covenant() (Covenant, error)to give callers control over error handling.
172-196: Consider extracting a helper for safe uint32 decoding.The pattern of decoding
uint32from items is repeated across many constructors. A small helper could centralize the bounds check and reduce duplication.func decodeUint32(data []byte) (uint32, error) { if len(data) < 4 { return 0, errors.New("insufficient bytes for uint32") } return binary.LittleEndian.Uint32(data), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/handshake/covenant.go(3 hunks)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/handshake/covenant.go (2)
144-159: LGTM!The
NoneCovenantimplementation correctly validates that items are empty and returns an appropriate error otherwise.
198-223: LGTM!The
OpenCovenantimplementation safely copies hashes and converts the raw name. The comment on line 219 about ignoring index 1 is helpful documentation.
Fixes #449 Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
agaffney
force-pushed
the
feat/handshake-additional-covenants
branch
from
8363e66 to
7daedf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/handshake/covenant.go (2)
65-142: Consider returning errors instead of panicking.The method panics on conversion errors and unknown covenant types, which will crash the application. If this code processes untrusted or externally-sourced data, consider returning
(Covenant, error)to allow graceful error handling upstream.
211-229: Consider validating that item[1] is zero.The comment on line 225 notes that index 1 "always contains 0". For additional robustness against malformed input, you could add a check to verify this assumption.
// Copy hash copy(ret.NameHash, gc.Items[0]) - // NOTE: purposely ignoring index 1, which always contains 0 + // Validate that item[1] is zero as expected + if len(gc.Items[1]) != 1 || gc.Items[1][0] != 0 { + return nil, errors.New("item[1] must be zero") + } // Raw name ret.RawName = string(gc.Items[2])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/handshake/covenant.go(5 hunks)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
internal/handshake/covenant.go (11)
144-159: LGTM!The NoneCovenant implementation correctly validates the covenant type and ensures no items are present.
172-202: LGTM! Bounds checks properly added.The constructor now includes proper length validation for all items before accessing them (lines 181-186), addressing the previous review concern about missing bounds checks.
240-264: LGTM! Bounds check properly added.The constructor now validates the height item length (line 249-251) before decoding, addressing the previous review concern.
274-296: LGTM! Bounds check properly added.The constructor now validates the height item length (line 283-285) before decoding, addressing the previous review concern.
305-325: LGTM! Bounds check properly added.The constructor now validates the height item length (line 314-316) before decoding, addressing the previous review concern.
336-364: LGTM! Bounds check added.The constructor properly validates the height item length (line 345-347) before decoding, following the same safe pattern as other covenant constructors.
374-400: LGTM! Bounds check added.The constructor properly validates the height item length (line 383-385) before decoding, following the same safe pattern as other covenant constructors.
410-432: LGTM! Bounds check properly added.The constructor now validates the height item length (line 419-421) before decoding, addressing the previous review concern.
443-470: LGTM! Bounds checks properly added.The constructor now validates both the height item length (line 452-454) and address version item length (line 455-457) before accessing them, addressing the previous review concern.
484-515: LGTM! Bounds checks properly added.The constructor now validates all item lengths before accessing them (lines 493-498), including all uint32 fields and the uint8 flags field, addressing the previous review concern.
524-544: LGTM! Bounds check properly added.The constructor now validates the height item length (line 533-535) before decoding, addressing the previous review concern.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Fixes #449
Summary by cubic
Added decoding support for additional Handshake covenant types to fully parse transactions across the name lifecycle. Fixes #449.
Written for commit 7daedf1. Summary will update automatically on new commits.
Summary by CodeRabbit
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.