Skip to content

Conversation

@arepala-uml
Copy link
Contributor

@arepala-uml arepala-uml commented Nov 30, 2025

  1. Used CIP-129 specifications to create the bech32 (string) representations of the Voter type in the ledger/common.
  2. Added constants to build the CIP-129 headers and encode bech32 safely.
  3. Added TestVoterType in gov_test.go with coverage for the new string encodings.

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.

  • New Features
    • Implemented Voter.String() for cc hot (key/script), drep (key/script), and staking pool (via PoolId).
    • Added CIP-129 header constants and safe bech32 encoding helpers.
    • Added tests covering expected encodings and panic on unknown voter type.

Written for commit 2d53bca. Summary will update automatically on new commits.

Summary by CodeRabbit

  • New Features

    • Added CIP-129 bech32 voter encoding support for proper voter identifier construction.
    • Voter objects now generate string representations for Constitutional Committee, DRep, and Staking Pool voter types.
  • Tests

    • Added comprehensive test coverage validating voter string representations across all supported voter types.

✏️ Tip: You can customize this high-level summary in your review settings.

…cations to generate bech32 encodings for each voter type

Signed-off-by: Akhil Repala <arepala@blinklabs.io>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 30, 2025

📝 Walkthrough

Walkthrough

This change implements a String() method on the Voter type following CIP-105 and CIP-129 specifications for bech32 encoding. The implementation adds CIP-129 constants and helper functions (encodeCip129Voter and encodeVoterBech32) to construct bech32 voter identifiers from various voter data types (Constitutional Committee hashes, DRep hashes, and staking pool identifiers). Existing ToPlutusData() paths remain unchanged. A comprehensive test suite validates string outputs across multiple voter type variants and confirms panic behavior for unknown types.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas requiring extra attention:

  • Bech32 encoding correctness: Verify that encodeCip129Voter and encodeVoterBech32 implementations comply with CIP-129 specification and correctly handle all voter type variants
  • Error handling strategy: Review the use of panics on bech32 conversion or encoding failures—confirm this aligns with codebase patterns and is the intended approach versus returning errors
  • Test case completeness: Validate that all five voter types (ConstitutionalCommitteeHotKeyHash, ConstitutionalCommitteeHotScriptHash, DRepKeyHash, DRepScriptHash, and StakingPoolKeyHash) have correct expected string representations
  • Hash encoding consistency: Ensure hash arrays are properly encoded in the bech32 format across different voter types

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding a String() method to the Voter type.
Linked Issues check ✅ Passed The PR implements Voter.String() using CIP-129 bech32 encoding as specified in issue #994, with proper helper functions and comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the Voter.String() method and CIP-129 bech32 encoding support as required by issue #994.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch voter_string

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@arepala-uml arepala-uml marked this pull request as ready for review November 30, 2025 21:26
@arepala-uml arepala-uml requested a review from a team as a code owner November 30, 2025 21:26
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 hashes

The 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 hardening

The 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, then bech32.Encode) is appropriate for these identifiers. Two small, optional hardening tweaks you might consider:

  • Mask keyType as well, e.g. byte(((keyType & 0x0f) << 4) | (credentialType & 0x0f)), to guard against future changes accidentally using values > 0x0f.
  • Have encodeVoterBech32 return (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

📥 Commits

Reviewing files that changed from the base of the PR and between 1808850 and 2d53bca.

📒 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 identifiers

The 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.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a 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
)
Copy link
Contributor

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))
Copy link
Contributor

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 {
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

String method for Voter

3 participants