-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(gui): add governance voting functionality to Qt wallet #6690
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: develop
Are you sure you want to change the base?
feat(gui): add governance voting functionality to Qt wallet #6690
Conversation
- Add right-click context menu to governance proposals with Vote Yes/No/Abstain options - Implement voting capability detection based on masternode voting keys in wallet - Add vote signing and submission logic matching RPC implementation - Handle testnet-specific signing (SignSpecialTxPayload) vs mainnet (SignMessage) - Add processVoteAndRelay interface method between Qt and core - Update wallet interfaces to support special transaction payload signing - Show voting results in dialog with success/failure counts and error details Users can now vote on governance proposals directly from the GUI if they have masternode voting keys in their wallet, eliminating the need to use CLI commands.
- Display number of masternodes wallet can vote with - Position masternode count next to proposal count on the right - Update count when masternode list changes - Helps users understand their voting capability at a glance
WalkthroughThis change set introduces direct governance proposal voting capabilities to the Qt GUI. Users with masternode voting keys can now cast "Yes", "No", or "Abstain" votes from the governance tab via a right-click context menu. The UI is updated to display the count of votable masternodes in the wallet. Core and interface changes add new methods for signing and submitting governance votes, including wallet support for signing votes using either a special payload or standard message signing, depending on the network. Supporting code updates integrate wallet and masternode data, error handling, and UI feedback for the voting process. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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: 1
🔭 Outside diff range comments (1)
src/qt/governancelist.cpp (1)
1-639
:⚠️ Potential issueFix code formatting to match project standards.
The CI pipeline indicates formatting issues. Please run clang-format on this file to ensure consistent code style.
#!/bin/bash # Check if clang-format is available and show the formatting differences if command -v clang-format &> /dev/null; then echo "Checking formatting differences for src/qt/governancelist.cpp..." clang-format --style=file src/qt/governancelist.cpp | diff -u src/qt/governancelist.cpp - else echo "clang-format not found. Please install it to check formatting." fi🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1000: Clang format differences found. The file does not conform to the required code style. Please run clang-format to fix formatting issues.
🧹 Nitpick comments (2)
doc/release-notes/release-notes-6690.md (1)
4-5
: Minor grammatical improvements for better readability.The release notes accurately document the new governance voting features. Consider these minor improvements for clarity:
-- Added governance proposal voting functionality to the Qt interface. Users with masternode voting keys can now vote on governance proposals directly from the governance tab via right-click context menu (#6690). -- Added masternode count display to governance tab showing how many masternodes the wallet can vote with (#6690). +- Added governance proposal voting functionality to the Qt interface. Users with masternode voting keys can now vote on governance proposals directly from the governance tab via the right-click context menu (#6690). +- Added masternode count display to the governance tab, showing how many masternodes the wallet can vote with (#6690).🧰 Tools
🪛 LanguageTool
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...ls directly from the governance tab via right-click context menu (#6690). - Added mas...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ... masternode count display to governance tab showing how many masternodes the wallet...(AI_HYDRA_LEO_MISSING_COMMA)
src/qt/governancelist.cpp (1)
511-512
: Consider validating the vote outcome parameter.While the function is currently only called with valid outcomes, adding validation would make it more robust.
void GovernanceList::voteForProposal(vote_outcome_enum_t outcome) { + // Validate outcome + if (outcome != VOTE_OUTCOME_YES && outcome != VOTE_OUTCOME_NO && outcome != VOTE_OUTCOME_ABSTAIN) { + QMessageBox::warning(this, tr("Voting Failed"), + tr("Invalid vote outcome.")); + return; + } + if (!walletModel) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
doc/release-notes/release-notes-6690.md
(1 hunks)src/interfaces/node.h
(2 hunks)src/interfaces/wallet.h
(1 hunks)src/node/interfaces.cpp
(2 hunks)src/qt/forms/governancelist.ui
(1 hunks)src/qt/governancelist.cpp
(6 hunks)src/qt/governancelist.h
(5 hunks)src/qt/walletview.cpp
(1 hunks)src/wallet/interfaces.cpp
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/interfaces/node.h (1)
src/node/interfaces.cpp (2)
vote
(145-157)vote
(145-145)
src/qt/walletview.cpp (1)
src/qt/governancelist.h (1)
walletModel
(44-44)
src/node/interfaces.cpp (1)
src/interfaces/node.h (11)
vote
(71-71)context
(59-59)context
(59-59)context
(72-72)context
(72-72)context
(81-81)context
(81-81)context
(94-94)context
(94-94)context
(363-363)context
(363-363)
src/wallet/interfaces.cpp (2)
src/interfaces/wallet.h (1)
hash
(118-118)src/wallet/wallet.h (7)
hash
(501-501)hash
(551-551)hash
(554-554)hash
(557-557)hash
(559-559)hash
(618-618)hash
(658-658)
🪛 LanguageTool
doc/release-notes/release-notes-6690.md
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...ls directly from the governance tab via right-click context menu (#6690). - Added mas...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~5-~5: Possible missing comma found.
Context: ... masternode count display to governance tab showing how many masternodes the wallet...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 GitHub Actions: Clang Diff Format Check
src/qt/governancelist.cpp
[error] 1-1000: Clang format differences found. The file does not conform to the required code style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (18)
src/interfaces/wallet.h (1)
117-118
: LGTM! Clean interface extension for governance voting support.The new
signSpecialTxPayload
method follows established interface patterns and provides the necessary signing capability for governance vote transactions. The method signature is well-designed with appropriate parameter types and return semantics.src/interfaces/node.h (2)
31-31
: LGTM! Proper forward declaration placement.The forward declaration for
CGovernanceVote
is correctly positioned with other class declarations and follows the established pattern in the file.
71-71
: LGTM! Well-designed interface method for vote processing.The
processVoteAndRelay
method follows established interface patterns with appropriate parameter types and error handling. The implementation insrc/node/interfaces.cpp
lines 145-157 shows proper validation of required managers and comprehensive error reporting.src/qt/walletview.cpp (1)
111-111
: LGTM! Proper wallet model initialization for governance voting.The addition of
setWalletModel(walletModel)
for the governance list page follows the established pattern used by other components (likemasternodeListPage
on line 106). This ensures the governance list has proper access to wallet functionality needed for vote signing and submission.src/wallet/interfaces.cpp (1)
192-195
: LGTM! Clean interface implementation.The
signSpecialTxPayload
method correctly implements the pure virtual interface method by delegating to the underlying wallet'sSignSpecialTxPayload
method. The implementation follows the established pattern used by other methods in theWalletImpl
class.src/qt/forms/governancelist.ui (1)
76-115
: LGTM! Well-structured UI layout changes.The addition of masternode count display elements is properly implemented:
- The expanding spacer provides flexible layout adjustment
- The new label pair ("Masternode Count:" and count display) follows the existing pattern used for "Proposal Count:"
- Spacing adjustments accommodate the new elements appropriately
src/node/interfaces.cpp (3)
14-14
: LGTM! Necessary include for governance exception handling.The include for
governance/exceptions.h
is required for theCGovernanceException
type used in theprocessVoteAndRelay
method implementation.
17-17
: LGTM! Required include for governance vote types.The include for
governance/vote.h
is necessary for theCGovernanceVote
parameter type in theprocessVoteAndRelay
method.
145-157
: LGTM! Robust implementation with proper error handling.The
processVoteAndRelay
method implementation is well-structured:
- Dependency validation: Properly checks for required components (governance manager, connection manager, peer manager) before proceeding
- Exception handling: Uses
CGovernanceException
to capture and extract error messages from the underlying governance system- Error reporting: Provides clear error messages both for processing failures and missing dependencies
- Return semantics: Correctly returns boolean success/failure status
The implementation follows the established pattern used by other methods in the
GOVImpl
class.src/qt/governancelist.h (8)
9-9
: LGTM! Required include for voting functionality.The include for
governance/vote.h
is necessary for thevote_outcome_enum_t
type used in thevoteForProposal
method signature.
22-22
: LGTM! Standard include for container usage.The
<map>
include is required for thestd::map<uint256, CKeyID> votableMasternodes
member variable.
33-33
: LGTM! Proper forward declaration.The forward declaration for
WalletModel
is appropriate since the class only uses a pointer toWalletModel
in the interface, avoiding unnecessary header dependencies.
44-44
: LGTM! Clean public interface addition.The
setWalletModel
method provides a clean way to inject the wallet model dependency into the governance list widget, following the same pattern assetClientModel
.
48-48
: LGTM! Properly initialized member.The
walletModel
member is appropriately declared as a pointer with nullptr initialization, consistent with the existingclientModel
member.
57-62
: LGTM! Well-designed voting interface.The voting-related additions are well-structured:
- Data member:
votableMasternodes
map efficiently tracks eligible masternodes by their proTxHash to voting key mapping- Helper methods:
updateVotingCapability()
provides centralized logic for determining voting eligibilitycanVote()
offers a simple inline check using the votable masternodes statevoteForProposal()
encapsulates the core voting logic with parameterized outcomeThe design promotes clean separation of concerns and reusability.
68-68
: LGTM! Consistent with existing update pattern.The
updateMasternodeCount()
slot follows the same naming and signature pattern as the existingupdateProposalCount()
method.
71-75
: LGTM! Complete voting action interface.The voting slots provide a complete set of actions for all possible vote outcomes:
voteYes()
- for supporting proposalsvoteNo()
- for opposing proposalsvoteAbstain()
- for neutral stanceThis covers all standard governance voting options and will integrate well with context menu actions.
src/qt/governancelist.cpp (1)
588-599
:⚠️ Potential issueFix incorrect PKHash usage in vote signing.
The code uses
PKHash(votingKeyID)
butvotingKeyID
is already aCKeyID
(which is a hash). This double-hashing would cause signing to fail on non-testnet networks.// Other networks use SignMessage std::string strMessage = vote.GetSignatureString(); std::string signature; - SigningResult result = walletModel->wallet().signMessage(strMessage, PKHash(votingKeyID), signature); + SigningResult result = walletModel->wallet().signMessage(strMessage, votingKeyID, signature); if (result == SigningResult::OK) {Likely an incorrect or invalid review comment.
<item> | ||
<widget class="QLabel" name="label_mn_count"> | ||
<property name="text"> | ||
<string>Masternode Count:</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.
maybe add hint with some explanation of what this count for?
src/qt/governancelist.cpp
Outdated
bool signSuccess = false; | ||
|
||
// Special implementation for testnet (same as RPC SignVote) | ||
if (Params().NetworkIDString() == CBaseChainParams::TESTNET) { |
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.
let's re-use helper from rpc code here?
static bool SignVote(const CWallet& wallet, const CKeyID& keyID, CGovernanceVote& vote)
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.
Hmm, I didn't think it made sense to directly use RPC code in qt; seems to violate separation concepts
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.
yes, that's right; reference to RPC headers is not good. but you can move this helper to src/wallet/ or src/governance/.
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.
see 20effe0, moving to src/governance causes circular dependency
It would be nice to show voting status for wallets with masternodes if it could be easily done - like "x yes votes, x no votes, x abstain votes" or something. |
Please further describe where / how this would / should be displayed |
With an additional column in the table that would only be displayed if the wallet contained masternode voting keys? |
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
CI failed |
src/governance/vote.cpp
Outdated
@@ -266,3 +266,4 @@ bool operator<(const CGovernanceVote& vote1, const CGovernanceVote& vote2) | |||
|
|||
return fResult; | |||
} | |||
|
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.
nit: non-related changes
src/governance/vote.h
Outdated
@@ -15,6 +15,7 @@ class CGovernanceVote; | |||
class CMasternodeSync; | |||
class CKey; | |||
class CKeyID; | |||
class CWallet; |
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.
nit:; should revert this changes too
LogPrintf("SignGovernanceVote -- SignHash() failed\n"); | ||
return false; | ||
} | ||
vote.SetSignature(signature); |
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.
wallet/wallet.cpp:2050:39: error: member access into incomplete type 'CGovernanceVote'
2050 | if (!SignSpecialTxPayload(vote.GetSignatureHash(), keyID, signature)) {
| ^
./validationinterface.h:22:7: note: forward declaration of 'CGovernanceVote'
22 | class CGovernanceVote;
| ^
missing vote.h include
Issue being fixed or feature implemented
What was done?
This PR adds complete governance voting functionality to the Qt wallet interface:
How Has This Been Tested?
Testing environment: macOS, testnet, wallet with 3 masternode voting keys
Screen.Recording.2025-05-26.at.13.51.49.mov
Breaking Changes
None - this PR only adds new functionality to the GUI without modifying existing behavior.
Checklist: