-
Notifications
You must be signed in to change notification settings - Fork 21
feat(ledger): String method for Voter #1287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…cations to generate bech32 encodings for each voter type Signed-off-by: Akhil Repala <arepala@blinklabs.io>
📝 WalkthroughWalkthroughThis change implements a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ledger/common/gov_test.go (1)
171-231: Voter.String test coverage is solid; optional extension for non-zero CIP-129 hashesThe table-driven tests nicely cover all VoterType variants and the panic on unknown types, with clear vectors for zero hashes and a realistic pool ID via
sequentialHash. If you want to harden this further, consider adding at least one non-zero hash case for a CIP-129 voter (e.g., DRep key/script) to ensure the header+payload encoding behaves as expected beyond the all-zero pattern, but this is purely optional given the current coverage.ledger/common/gov.go (1)
43-74: CIP-129 header construction and Bech32 usage look correct; consider minor hardeningThe header composition
(keyType << 4) | (credentialType & 0x0f)and payload layout[header | hash...]match the intended “high nibble = key type, low nibble = credential type” scheme, and the Bech32 path (8→5 bit conversion with padding, thenbech32.Encode) is appropriate for these identifiers. Two small, optional hardening tweaks you might consider:
- Mask
keyTypeas well, e.g.byte(((keyType & 0x0f) << 4) | (credentialType & 0x0f)), to guard against future changes accidentally using values > 0x0f.- Have
encodeVoterBech32return(string, error)and let callers decide how to handle unexpected encoding failures rather than panicking, if you anticipate this ever being called on untrusted/externally-sourced values. Right now the panics are effectively “should never happen” paths, which is also reasonable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ledger/common/gov.go(2 hunks)ledger/common/gov_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ledger/common/gov_test.go (1)
ledger/common/gov.go (6)
Voter(37-41)VoterTypeConstitutionalCommitteeHotKeyHash(30-30)VoterTypeConstitutionalCommitteeHotScriptHash(31-31)VoterTypeDRepKeyHash(32-32)VoterTypeDRepScriptHash(33-33)VoterTypeStakingPoolKeyHash(34-34)
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (1)
ledger/common/gov.go (1)
76-113: Voter.String switch cleanly maps voter types to CIP-129 / pool identifiersThe
String()implementation cleanly separates concerns:
- CC hot and DRep voters are encoded via CIP-129 using appropriate HRPs (
"cc_hot","drep") and key/script credential types.- Staking pool voters correctly delegate to
PoolId(v.Hash).String(), reusing the established pool Bech32 format.- The default case panics on unknown voter types, which, together with the test that asserts this behavior, makes it a good “fail fast on impossible state” guard for newly added enum values.
This looks consistent with the tests and the intended standards usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
| cip129KeyTypeDRep uint8 = 2 | ||
| cip129CredentialTypeKeyHash uint8 = 0x02 | ||
| cip129CredentialTypeScriptHash uint8 = 0x03 | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values are the same as the existing constants at the top of this file, and the existing ones should be used instead
| credentialType uint8, | ||
| hash []byte, | ||
| ) string { | ||
| header := byte((keyType << 4) | (credentialType & 0x0f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a comment about what's actually happening here
| return encodeVoterBech32(prefix, data) | ||
| } | ||
|
|
||
| func encodeVoterBech32(prefix string, data []byte) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is only ever called by the function above and could easily be merged into it.
Closes #994
Summary by cubic
Add a String() method for Voter that outputs CIP-129 bech32 identifiers for all voter types. This makes voter credentials human-readable and consistent with Cardano standards.
Written for commit 2d53bca. Summary will update automatically on new commits.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.