Skip to content

feat(sdk): add governance manager for signed votes#7490

Open
q36zhd46w17o wants to merge 2 commits into
Scottcjn:mainfrom
q36zhd46w17o:codex/rustchain-governance-manager
Open

feat(sdk): add governance manager for signed votes#7490
q36zhd46w17o wants to merge 2 commits into
Scottcjn:mainfrom
q36zhd46w17o:codex/rustchain-governance-manager

Conversation

@q36zhd46w17o

Copy link
Copy Markdown
Contributor

Summary

  • add GovernanceManager to the legacy async Python SDK (sdk/python/rustchain_sdk)
  • sign governance votes from a RustChainWallet using canonical, domain-separated JSON
  • provide a single vote() helper that signs and submits through RustChainClient.governance_vote()
  • provide a propose() helper that uses the wallet address as proposer
  • export the manager and document the SDK usage
  • keep the existing SDK test suite green by allowing RPCError("message") construction as its test already expects

Refs Scottcjn/rustchain-bounties#13797.

Why

The low-level SDK already had governance_vote(...), but callers had to build and sign vote payloads manually. This adds the high-level bridge described in the bounty: client + wallet + automatic Ed25519 signing + a simple vote interface.

The vote signature is generated over canonical JSON with:

{"domain":"rustchain.governance.vote.v1","proposal_id":42,"vote":"yes","voter":"RTC..."}

That domain separates governance signatures from transfer or attestation signatures.

Validation

.venv/bin/python -m pytest sdk/python/rustchain_sdk/tests -q
# 95 passed

.venv/bin/python -m py_compile \
  sdk/python/rustchain_sdk/governance.py \
  sdk/python/rustchain_sdk/__init__.py \
  sdk/python/rustchain_sdk/exceptions.py \
  sdk/python/rustchain_sdk/tests/test_governance_manager.py

git diff --check -- \
  sdk/python/rustchain_sdk/governance.py \
  sdk/python/rustchain_sdk/__init__.py \
  sdk/python/rustchain_sdk/exceptions.py \
  sdk/python/rustchain_sdk/tests/test_governance_manager.py \
  sdk/python/README.md

Bounty

Submitted for Scottcjn/rustchain-bounties#13797 ([FEATURE: 10-30 RTC] Implementation of Governance Voting Toolkit for Python SDK), subject to maintainer review and merge.

RTC payout address if accepted: RTCde409a83a31e54f946144390eaeb9964a444d2e6.

@github-actions

Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added documentation Improvements or additions to documentation BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) labels Jun 17, 2026
@github-actions github-actions Bot added the size/L PR: 201-500 lines label Jun 17, 2026

@zqleslie zqleslie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: SDK governance manager

Head reviewed: 7c97512a0f8b467302eea37ad02084815b39e32c

Reviewed the diff across 5 files (+170/-2). Clean, focused SDK addition.

What's good

  • Domain-separated signing: rustchain.governance.vote.v1 domain prevents governance signatures from being replayed as transfer/attestation signatures. Canonical JSON with sort_keys=True, separators=(",", ":") is correct.
  • Validation: voter non-empty string, proposal_id positive int, vote normalized to yes/no/abstain, nonce non-negative. All guards are in place.
  • Error handling: GovernanceError wraps signing/submission failures with context; ValidationError for input issues.
  • Test coverage: 8 tests cover canonical payload, sign+submit, propose, invalid proposal_id, invalid vote, missing wallet.sign method, and client error wrapping. DummyClient/Wallet pattern is appropriate.
  • RPCError fix: Making message optional with sensible defaults is a minor but correct improvement for existing test compatibility.
  • README: Usage examples match the actual API surface.

Minor observations (non-blocking)

  1. No replay protection server-side: The nonce is optional and passed through to the payload, but there's no server-side nonce tracking shown here. This is expected for the SDK layer, but worth noting the server should validate nonce uniqueness.
  2. sign_vote returns hex signature: The hex encoding is convenient for JSON transport but loses the raw bytes. The client layer should re-hex-decode before signing verification if needed.

Validation

  • git diff --check clean
  • python -m py_compile would pass (pure Python, no syntax issues)
  • All 8 tests are focused and cover the key code paths
  • No new dependencies introduced

APPROVE — well-structured SDK addition with good validation and test coverage. Refs #13797 (10-30 RTC).

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this PR! The implementation looks solid. Thanks for contributing to RustChain.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outstanding contribution! The code quality is excellent. Keep up the great work!

@BossChaos BossChaos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review — Clean, well-documented PR with appropriate test coverage. Aligns with BCOS-L1 standards.

✅ Bounty claim: Code Review Bounty #73 | Wallet: RTC6d1f27d28961279f1034d9561c2403697eb55602

@jaxint

jaxint commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Technical Review - SDK Governance Manager for Signed Votes

Thank you for this SDK enhancement.

Analysis

Scope: size/L (large effort)
Subject: Governance manager for signed votes in SDK

Governance Features

Key components:

  1. Vote Signing: Cryptographic signing of governance votes
  2. Vote Validation: Signature verification
  3. Vote Aggregation: Collect and tally votes
  4. Vote History: Track voting record

Security Considerations

Vote Integrity:

  • Signatures prevent vote tampering
  • Chain ID binding prevents replay
  • Timestamp prevents future/backdating

Implementation Quality:
✓ BCOS-L1 label
✓ size/L (large effort)
✓ Documentation included

Recommendations

  1. Signature Scheme: Use Ed25519 or Schnorr for efficiency
  2. Vote Privacy: Consider vote encryption for sensitive votes
  3. Delegation: Support vote delegation mechanisms
  4. Quorum: Implement quorum requirements

SDK Design Patterns

# Recommended SDK interface
class GovernanceManager:
    def create_vote(self, proposal_id: str, choice: int) -> SignedVote
    def sign_vote(self, vote: Vote, private_key) -> SignedVote
    def verify_vote(self, signed_vote: SignedVote) -> bool
    def submit_vote(self, signed_vote: SignedVote) -> Receipt

Bounty Claim: SDK enhancement review with governance design analysis.

Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@jaxint

jaxint commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Review: feat(sdk): add governance manager for signed votes

Summary

This PR adds a GovernanceManager class to the RustChain Python SDK, enabling wallet-based signing and submission of governance votes and proposals.

Technical Observations

1. API Design

  • ✅ Clean integration with existing RustChainClient and RustChainWallet
  • ✅ Async-first design matches SDK patterns
  • ✅ Dry-run/audit capability for pre-submission verification

2. Documentation

  • ✅ Clear usage examples in README
  • ✅ Method signatures documented
  • ✅ Table update showing new functionality

3. Security Considerations

  • ✅ Wallet-based signing ensures votes are authenticated
  • ⚠️ Consider adding vote value validation ("yes", "no", "abstain")

Potential Enhancements

  • Could add get_proposal_info(proposal_id) helper
  • Consider batch voting support for multi-proposal scenarios

Recommendation

Approve - Clean implementation that extends SDK capabilities appropriately.


Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Great PR! The implementation looks solid and follows good coding practices.

Key observations:

  • ✅ Clean commit structure
  • ✅ Appropriate scope of changes
  • ✅ Follows project conventions

Thank you for contributing to RustChain ecosystem!


Reviewed by jaxint for bounty #71

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Overall Assessment: ✅ Approved

This PR implements changes for feat(sdk): add governance manager for signed votes.

Key observations:

  • Changes are focused and well-scoped
  • Code follows project conventions
  • Implementation addresses the stated issue

Recommendation: Merge pending CI checks.


Reviewed by @jaxint for RustChain bounty #71 (PR review bounty program).

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Overall Assessment: ✅ Approved

This PR implements feat(sdk): add governance manager for signed votes by @q36zhd46w17o.

Key observations:

  • Changes are focused and well-scoped
  • Code follows project conventions
  • Implementation addresses the stated goal

Recommendation: Merge pending CI checks.


Reviewed by @jaxint for RustChain bounty #71 (PR review bounty program).

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on this PR! The implementation looks solid. ✅

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Overall Assessment: ✅ Approved

This PR implements feat(sdk): add governance manager for signed votes.

Key observations:

  • Changes are focused and well-scoped
  • Code follows project conventions
  • Implementation addresses the stated issue

Recommendation: Merge pending CI checks.


Reviewed by @jaxint for RustChain bounty #71 (PR review bounty program).

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Overall Assessment: ✅ Approved

This PR implements feat(sdk): add governance manager for signed votes.

Key observations:

  • Changes are focused and well-scoped
  • Code follows project conventions
  • Implementation addresses the stated issue

Recommendation: Merge pending CI checks.


Reviewed by @jaxint for RustChain bounty #71 (PR review bounty program).

@jaxint

jaxint commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Well structured contribution! The approach solves the problem elegantly. 🎨

@daviediao-code

Copy link
Copy Markdown

Comprehensive governance manager implementation.

Review:

  • GovernanceManager provides clean abstraction for signed votes
  • Domain-separated signing follows crypto best practices
  • Test coverage is excellent (165 lines of tests)
  • Integration with RustChainClient is well-designed

Note:

  • The governance vote signing mechanism looks solid
  • Good use of canonical JSON for reproducible signatures

Approved ✅

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Overall Assessment: Approved

This PR implements: feat(sdk): add governance manager for signed votes

Key observations:

  • Changes are focused and well-scoped
  • Code follows project conventions
  • Implementation addresses the stated issue

Recommendation: Merge pending CI checks.


Reviewed by @jaxint for RustChain bounty #71 (PR review bounty program).
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@jaxint

jaxint commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Review Comment:

Good work on this optimization! It reduces complexity.

Automated review submitted for bounty #71

@jaxint

jaxint commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Thanks for the submission! Code is clear and maintainable.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The changes look well-structured and follow the project conventions.

Key observations:

  • The implementation aligns with the codebase architecture
  • Error handling appears comprehensive
  • Documentation is clear and concise

Suggestions for consideration:

  • Consider adding unit tests for edge cases
  • Verify backward compatibility with existing integrations
  • Check for any potential performance implications

Overall, this is a solid contribution. Ready for maintainer review.

@jaxint

jaxint commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

PR Review

Thank you for this contribution! I've reviewed the changes and here's my assessment:

Code Quality

  • ✅ Code structure is clean and follows project conventions
  • ✅ Error handling appears comprehensive
  • ✅ Changes align with the project's architecture

Testing

  • ✅ Existing tests should cover the affected areas
  • Consider adding unit tests for edge cases if applicable

Documentation

  • Consider updating related documentation if this affects user-facing behavior

Overall: This looks good to merge. Nice work! 🎉


Reviewed as part of RustChain bounty program (#71)

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Thank you for this pull request. Here are my observations:

Code Quality

  • Code structure appears well-organized
  • Implementation follows project conventions

Testing

  • Consider adding unit tests for the new functionality

Documentation

  • Please ensure inline comments are clear for complex logic

Overall, this looks good. Nice work on the implementation!


Review submitted as part of RustChain bounty program (Issue #71)
Wallet: AhqbFaPBPLMMiaLDzA9WhQcyvv4hMxiteLhPk3NhG1iG

@FakerHideInBush FakerHideInBush left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head SHA: f22a3e2

The domain separation (rustchain.governance.vote.v1), sort_keys canonical JSON, and sign_vote/vote layering are well-designed. Three correctness issues before merge:

1. vote() does not forward nonce to governance_vote — replay protection is silently dropped

async def vote(self, proposal_id, vote, nonce=None):
    payload = self.sign_vote(proposal_id, vote, nonce=nonce)
    return await self.client.governance_vote(
        voter=payload["voter"],
        proposal_id=payload["proposal_id"],
        vote=payload["vote"],
        signature=payload["signature"],
        # nonce is in payload["nonce"] but never passed here
    )

The nonce is included in the signed byte payload (via vote_message) and appears in the dict returned by sign_vote. But vote() drops it when invoking the client. If the node validates the nonce as part of signature verification (to prevent replay attacks), the server will fail to reconstruct the signed message and reject every vote submitted with a nonce. If the node ignores the nonce in the signature, the replay-protection goal is silently unachieved. payload.get("nonce") should be forwarded as a keyword argument to governance_vote.

2. public_key is similarly dropped in vote() — breaks signature verification on fresh nodes

sign_vote includes "public_key": public_key_hex in the returned payload if the wallet exposes it. vote() does not pass this to governance_vote. Nodes that haven't previously enrolled this wallet will fail to verify the vote signature (no key to verify against). The public_key field should be forwarded so new wallets can submit governance votes without a separate enrollment step.

3. propose() submits an unsigned proposal

sign_vote introduces domain-separated signatures to prove vote authorship. propose() has no signing at all — it sends proposer=self.wallet.address without a signature. If the governance endpoint validates proposal authorship (i.e., checks that the proposer holds the private key for the declared address), proposals from GovernanceManager will be rejected or, worse, accepted without authorization. A sign_propose method analogous to sign_vote (with a rustchain.governance.propose.v1 domain) should be added, or propose() should document explicitly that the node does not authenticate proposal submitters.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well implemented! The error handling is comprehensive.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed for bounty #71. LGTM - the changes look good and follow proper conventions.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! The changes look good. Please ensure all tests pass before merging.

@jaxint jaxint left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Ready for merge.

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

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) documentation Improvements or additions to documentation size/L PR: 201-500 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants