feat(ledger): String method for Voter #1287
Conversation
…cations to generate bech32 encodings for each voter type Signed-off-by: Akhil Repala <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis change adds 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)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)ledger/common/gov.go (3)
⏰ 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 (3)
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: 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.
ledger/common/gov.go
Outdated
| cip129KeyTypeDRep uint8 = 2 | ||
| cip129CredentialTypeKeyHash uint8 = 0x02 | ||
| cip129CredentialTypeScriptHash uint8 = 0x03 | ||
| ) |
There was a problem hiding this comment.
These values are the same as the existing constants at the top of this file, and the existing ones should be used instead
There was a problem hiding this comment.
I removed these new constants and used existing constants only. please review it.
ledger/common/gov.go
Outdated
| credentialType uint8, | ||
| hash []byte, | ||
| ) string { | ||
| header := byte((keyType << 4) | (credentialType & 0x0f)) |
There was a problem hiding this comment.
This could use a comment about what's actually happening here
There was a problem hiding this comment.
I added a comment here for what we are doing here. please review it.
ledger/common/gov.go
Outdated
| return encodeVoterBech32(prefix, data) | ||
| } | ||
|
|
||
| func encodeVoterBech32(prefix string, data []byte) string { |
There was a problem hiding this comment.
This function is only ever called by the function above and could easily be merged into it.
There was a problem hiding this comment.
I merged this function into the encodeCip129Voter function and please review it.
Signed-off-by: Akhil Repala <[email protected]>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="ledger/common/gov.go">
<violation number="1" location="ledger/common/gov.go:50">
P3: Minor typo: missing space after comma in comment (`1,we` should be `1, we`).</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> Signed-off-by: Chris Gianelloni <[email protected]>
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 232ddab. 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.