feat(sdk): add governance manager for signed votes#7490
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
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! |
zqleslie
left a comment
There was a problem hiding this comment.
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.v1domain prevents governance signatures from being replayed as transfer/attestation signatures. Canonical JSON withsort_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:
GovernanceErrorwraps signing/submission failures with context;ValidationErrorfor 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
messageoptional 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)
- 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.
sign_votereturns 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 --checkcleanpython -m py_compilewould 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
left a comment
There was a problem hiding this comment.
Great work on this PR! The implementation looks solid. Thanks for contributing to RustChain.
jaxint
left a comment
There was a problem hiding this comment.
Outstanding contribution! The code quality is excellent. Keep up the great work!
Technical Review - SDK Governance Manager for Signed VotesThank you for this SDK enhancement. AnalysisScope: size/L (large effort) Governance FeaturesKey components:
Security ConsiderationsVote Integrity:
Implementation Quality: Recommendations
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) -> ReceiptBounty Claim: SDK enhancement review with governance design analysis. Wallet: |
PR Review: feat(sdk): add governance manager for signed votesSummaryThis PR adds a Technical Observations1. API Design
2. Documentation
3. Security Considerations
Potential Enhancements
RecommendationApprove - Clean implementation that extends SDK capabilities appropriately. Wallet: |
jaxint
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Nice work on this PR! The implementation looks solid. ✅
jaxint
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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).
|
Well structured contribution! The approach solves the problem elegantly. 🎨 |
|
Comprehensive governance manager implementation. Review:
Note:
Approved ✅ |
jaxint
left a comment
There was a problem hiding this comment.
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
|
Review Comment: Good work on this optimization! It reduces complexity. Automated review submitted for bounty #71 |
|
Thanks for the submission! Code is clear and maintainable. |
jaxint
left a comment
There was a problem hiding this comment.
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.
PR ReviewThank you for this contribution! I've reviewed the changes and here's my assessment: Code Quality
Testing
Documentation
Overall: This looks good to merge. Nice work! 🎉 Reviewed as part of RustChain bounty program (#71) |
jaxint
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Well implemented! The error handling is comprehensive.
jaxint
left a comment
There was a problem hiding this comment.
Thanks for this PR! The changes look good. Please ensure all tests pass before merging.
jaxint
left a comment
There was a problem hiding this comment.
Looks good to me! Ready for merge.
Summary
GovernanceManagerto the legacy async Python SDK (sdk/python/rustchain_sdk)RustChainWalletusing canonical, domain-separated JSONvote()helper that signs and submits throughRustChainClient.governance_vote()propose()helper that uses the wallet address as proposerRPCError("message")construction as its test already expectsRefs 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.mdBounty
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.