feat(qt): UI refresh (2/n, update "Governance" Tab layout, lifecycle icons, description dialogs, resumable proposal creation) #7110
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces comprehensive governance UI refactoring and extends the Node interface to support governance queries. Key changes include: new Node interface methods for vote counting and proposal funding queries; extraction of Proposal and ProposalModel classes from governancelist.cpp into standalone components; reworking of GovernanceList to support multiple proposal sources (Active/Local) with filtering and empty-state handling; introduction of new UI components including DescriptionDialog (replacing TransactionDescDialog), ProposalResume for broadcasting pending proposals, and ProposalWizard simplification; unconditional creation of masternodes and governance tabs with dynamic visibility management via OptionsModel signals; build system reconfiguration for new UI forms and moc files; font utility enhancements for QTextEdit support; and refactoring of OptionsModel to use signals instead of restart triggers for tab visibility changes. Sequence DiagramsequenceDiagram
participant User
participant ProposalResume as ProposalResume Dialog
participant WalletModel as WalletModel
participant Node as Node Interface
participant UI as UI Layer
User->>ProposalResume: Opens dialog with proposals
ProposalResume->>Node: Queries governance info<br/>(funded height, vote counts)
Node-->>ProposalResume: Returns governance state
ProposalResume->>UI: Displays proposal entries<br/>with broadcast buttons
ProposalResume->>WalletModel: Polls for collateral<br/>confirmations (timer loop)
WalletModel-->>ProposalResume: Returns confirmation count
alt Confirmations met
ProposalResume->>UI: Enable broadcast button
User->>ProposalResume: Clicks broadcast
ProposalResume->>Node: submitProposal()
Node-->>ProposalResume: Success/error
ProposalResume->>UI: Show result dialog<br/>Remove entry / update empty state
else Waiting for confirmations
ProposalResume->>UI: Show confirmation count
Note over ProposalResume: Continues polling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/qt/guiutil_font.cpp (1)
17-25: Run clang-format on this file (CI is failing).
The Clang Diff Format Check reports formatting differences in this file; please reformat before merge.src/interfaces/node.h (1)
20-26: Add missing<optional>include forstd::optional.
The file usesstd::optionalin the function signatures at lines 156–157 but doesn't include the required<optional>header, which will cause compilation failures.🛠️ Suggested fix
`#include` <functional> `#include` <memory> +#include <optional> `#include` <stddef.h> `#include` <stdint.h> `#include` <string>Also applies to: 156–157
src/qt/governancelist.cpp (1)
1-1: Address clang-format pipeline failure.The pipeline indicates clang-format differences in this file. Please run
clang-formatto fix formatting before merging.
🤖 Fix all issues with AI agents
In `@src/Makefile.qt.include`:
- Around line 34-36: The non-backported list is missing the new C++ proposal
sources: add entries for src/qt/proposaldesc.*, src/qt/proposaldescdialog.*, and
src/qt/proposalmodel.* to test/util/data/non-backported.txt (do not add any .ui
form files); note that src/qt/proposalwizard.* is already registered so no
change needed for it.
In `@src/node/interfaces.cpp`:
- Around line 310-316: The call to context().chainman->ActiveChain().Height()
inside getProposalFundedHeight is not protected by the cs_main lock; wrap the
ActiveChain().Height() access in LOCK(::cs_main) (as done in getNumBlocks() /
getLastBlockTime()) to prevent a race. Add a scoped LOCK(::cs_main) before
reading nTipHeight so the loop over context()->govman triggers uses a
thread-safe snapshot of the active chain height.
In `@src/qt/guiutil_font.h`:
- Around line 84-86: Run clang-format on src/qt/guiutil_font.h to fix CI
formatting failures; reformat the declarations/Getters (GetScaleSteps,
GetScaledFontSize, GetFont) and surrounding spacing/line breaks so they match
the project's clang-format rules, then stage the updated header (ensuring member
names m_scale_steps, m_font_scale, m_font remain unchanged) and push the
formatted file.
In `@src/qt/proposalmodel.cpp`:
- Around line 354-361: In ProposalModel::setVotingParams the dataChanged call
uses createIndex(rowCount(), Column::VOTING_STATUS) which can create an
out-of-range QModelIndex; fix by guarding for empty model and emitting
dataChanged only when rowCount() > 0, using createIndex(0,
Column::VOTING_STATUS) as the first index and createIndex(rowCount() - 1,
Column::VOTING_STATUS) as the last index so the range is valid; update the
conditional in setVotingParams to check rowCount() before calling Q_EMIT
dataChanged.
- Around line 364-366: The getProposalAt method can dereference m_data with an
invalid QModelIndex and crash; update ProposalModel::getProposalAt to validate
the index before accessing m_data by checking index.isValid() and that
index.row() is within the range 0 .. m_data.size()-1 (and optionally that
index.model() == this), and return nullptr for any invalid or out-of-range index
instead of indexing m_data directly.
- Around line 239-242: Replace the hard-coded English words with translatable
strings: use tr("Voting") and tr("Voted") for the status (from
proposal->status() / ProposalStatus::Voting) and use Qt's plural-aware tr() for
the block text by switching the message to a pluralized pattern such as tr("%1,
%n block(s) till superblock", "", blocks) (where blocks is from
proposal->blocksUntilSuperblock()) so translators can provide correct
singular/plural forms.
🧹 Nitpick comments (9)
src/qt/guiutil_font.cpp (1)
562-567: Preserve per‑widget base font size for registeredQTextEdits.
updateFonts()re-applies styling withDEFAULT_FONT_SIZEfor all registered text edits, which can override custom base sizes. If anyQTextEdituses a non-default base size, consider storing the base size at registration time and reusing it here.♻️ Possible approach
-std::map<QPointer<QTextEdit>, QString> mapTextEditStyleUpdates; +struct TextEditStyleUpdate { + QString html; + double base_point_size{0}; +}; +std::map<QPointer<QTextEdit>, TextEditStyleUpdate> mapTextEditStyleUpdates;- for (const auto& [widget, html] : mapTextEditStyleUpdates) { - setFontBodyHTML(widget, html, - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/false), - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/true)); + for (const auto& [widget, style] : mapTextEditStyleUpdates) { + const double base_size = style.base_point_size > 0 ? style.base_point_size + : FontRegistry::DEFAULT_FONT_SIZE; + setFontBodyHTML(widget, style.html, + getScaledFont(base_size, /*bold=*/false), + getScaledFont(base_size, /*bold=*/true)); }- mapTextEditStyleUpdates[widget] = html; + mapTextEditStyleUpdates[widget] = {html, widget->font().pointSizeF()};src/qt/proposaldescdialog.cpp (2)
5-13: Consider reordering includes for consistency.The include order could be improved for consistency with typical project conventions (own header first, then related headers, then Qt headers):
♻️ Suggested reordering
-#include <qt/forms/ui_proposaldescdialog.h> - -#include <qt/proposaldesc.h> `#include` <qt/proposaldescdialog.h> + +#include <qt/forms/ui_proposaldescdialog.h> +#include <qt/proposaldesc.h> `#include` <qt/proposalmodel.h> -#include <qt/guiutil_font.h> - `#include` <qt/guiutil.h> +#include <qt/guiutil_font.h>Also,
guiutil_font.happears unused since no direct font functions from that header are called in this file.
15-24: Consider adding a null check for the proposal pointer.The constructor dereferences
proposalwithout validating it. While callers may guarantee non-null, defensive programming would prevent potential crashes.🛡️ Suggested defensive check
ProposalDescDialog::ProposalDescDialog(const Proposal* proposal, BitcoinUnit unit, QWidget* parent) : QDialog(parent, GUIUtil::dialog_flags), ui(new Ui::ProposalDescDialog) { + assert(proposal != nullptr); ui->setupUi(this); setWindowTitle(tr("Details for %1").arg(proposal->title())); GUIUtil::registerWidget(ui->detailText, ProposalDesc::toHTML(proposal, unit)); GUIUtil::updateFonts(); GUIUtil::handleCloseWindowShortcut(this); }Based on learnings, fail-fast asserts are preferred for programmer errors that should be caught during development.
src/qt/proposaldesc.cpp (2)
14-34: Consider HTML-escaping hash values for consistency.While hash values are typically safe hex strings, applying
HtmlEscapetohash(),parentHash(), andcollateralHash()would provide defense-in-depth and maintain consistency with other fields.♻️ Suggested change
- strHTML += "<b>" + tr("Object Hash") + ":</b> " + proposal->hash() + "<br>"; - strHTML += "<b>" + tr("Parent Hash") + ":</b> " + proposal->parentHash() + "<br>"; + strHTML += "<b>" + tr("Object Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->hash()) + "<br>"; + strHTML += "<b>" + tr("Parent Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->parentHash()) + "<br>"; strHTML += "<b>" + tr("Collateral Date") + ":</b> " + GUIUtil::dateTimeStr(proposal->collateralDate()) + "<br>"; - strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + proposal->collateralHash() + "<br>"; + strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->collateralHash()) + "<br>";
18-18: Minor: HTML structure could be more complete.The HTML starts with
<html>but lacks<body>tags. While Qt's QTextEdit typically handles incomplete HTML gracefully, a proper structure would be more robust.src/qt/forms/governancelist.ui (1)
47-67: Add a buddy to the filter label for keyboard focus.This improves keyboard navigation (Alt+key focuses the filter input).
♿ Suggested UI tweak
<widget class="QLabel" name="label_filter_2"> <property name="text"> <string>Filter List:</string> </property> + <property name="buddy"> + <cstring>filterLineEdit</cstring> + </property> </widget>src/qt/governancelist.h (1)
33-36: Include<cstdint>foruint8_t.Avoid relying on transitive includes for fixed-width integer types.
♻️ Proposed include
`#include` <QWidget> +#include <cstdint> + `#include` <map> `#include` <memory>src/qt/bitcoingui.cpp (1)
1470-1500: Hardcoded toolbar action indices are fragile.Lines 1478 and 1494 use
appToolBar->actions()[5]andappToolBar->actions()[6]respectively. This mirrors the existing pattern at line 1460 for CoinJoin (actions()[4]), but hardcoded indices are error-prone — if buttons are reordered or added, these will silently break.Consider storing action references when buttons are added, or adding a comment documenting the expected order. This is a maintenance concern rather than a blocking issue since it follows the existing pattern.
src/qt/proposalmodel.h (1)
27-31: Q_OBJECT macro placement is unconventional.The
Q_OBJECTmacro is placed in aprivate:section. While this compiles, the Qt convention is to place it at the very beginning of the class body (before any access specifier). This is a minor style nit.Suggested adjustment
class Proposal : public QObject { -private: Q_OBJECT +private: ClientModel* clientModel;
src/Makefile.qt.include
Outdated
| qt/forms/proposaldescdialog.ui \ | ||
| qt/forms/proposalwizard.ui \ | ||
| qt/forms/psbtoperationsdialog.ui \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the Makefile.qt.include to see what proposal files are being added
head -60 src/Makefile.qt.include | tail -30Repository: dashpay/dash
Length of output: 168
🏁 Script executed:
# Check lines around 163-166 and 275-278 in src/Makefile.qt.include
sed -n '30,40p;160,170p;270,280p' src/Makefile.qt.includeRepository: dashpay/dash
Length of output: 971
🏁 Script executed:
# Check current non-backported.txt entries for proposal files
cat test/util/data/non-backported.txt | grep -i proposalRepository: dashpay/dash
Length of output: 79
🏁 Script executed:
# Get full current non-backported.txt to see all entries
cat test/util/data/non-backported.txtRepository: dashpay/dash
Length of output: 1383
Add missing proposal C++ files to non-backported list.
The following new C++ proposal files should be registered in test/util/data/non-backported.txt:
src/qt/proposaldesc.*src/qt/proposaldescdialog.*src/qt/proposalmodel.*
(Note: src/qt/proposalwizard.* is already registered. The .ui form files should not be added to this list as it only tracks C++ sources.)
🤖 Prompt for AI Agents
In `@src/Makefile.qt.include` around lines 34 - 36, The non-backported list is
missing the new C++ proposal sources: add entries for src/qt/proposaldesc.*,
src/qt/proposaldescdialog.*, and src/qt/proposalmodel.* to
test/util/data/non-backported.txt (do not add any .ui form files); note that
src/qt/proposalwizard.* is already registered so no change needed for it.
| std::optional<int32_t> getProposalFundedHeight(const uint256& proposal_hash) override | ||
| { | ||
| if (context().govman != nullptr && context().chainman != nullptr && context().dmnman != nullptr) { | ||
| const int32_t nTipHeight = context().chainman->ActiveChain().Height(); | ||
| for (const auto& trigger : context().govman->GetActiveTriggers()) { | ||
| if (!trigger || trigger->GetBlockHeight() > nTipHeight) continue; | ||
| for (const auto& hash : trigger->GetProposalHashes()) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find the file and understand its structure
find . -name "interfaces.cpp" -type f | head -20Repository: dashpay/dash
Length of output: 164
🏁 Script executed:
# Search for ActiveHeight method definition in the codebase
rg "ActiveHeight\(\)" -t cpp --max-count 20Repository: dashpay/dash
Length of output: 704
🏁 Script executed:
# Look for context() usage and how chain is typically accessed in interfaces
rg "context\(\)\.chainman->Active" -t cpp -A 2 -B 2 | head -100Repository: dashpay/dash
Length of output: 533
🏁 Script executed:
# Check the specific file and surrounding context
cat -n src/node/interfaces.cpp | sed -n '300,330p'Repository: dashpay/dash
Length of output: 1864
🏁 Script executed:
# Look for cs_main lock usage in this file
rg "cs_main" src/node/interfaces.cpp -B 2 -A 2Repository: dashpay/dash
Length of output: 7179
🏁 Script executed:
# Search for CChainState and understand the ActiveHeight pattern
rg "def.*ActiveHeight|ActiveHeight.*{" -t cpp -A 3 | head -50Repository: dashpay/dash
Length of output: 185
🏁 Script executed:
# Check what class this method belongs to and the full context
rg "getProposalFundedHeight" -B 20 src/node/interfaces.cpp | head -60Repository: dashpay/dash
Length of output: 1115
🏁 Script executed:
# Search for similar proposal-related methods to see their locking pattern
rg "std::optional.*Proposal" -A 10 src/node/interfaces.cppRepository: dashpay/dash
Length of output: 1345
🏁 Script executed:
# Look at what govman->GetActiveTriggers() might require
rg "GetActiveTriggers" -B 3 -A 5 --max-count 15Repository: dashpay/dash
Length of output: 5886
🏁 Script executed:
# Check if there are any lock constraints on this method or its class
rg "getProposalFundedHeight" -B 30 src/node/interfaces.cpp | head -50Repository: dashpay/dash
Length of output: 1375
🏁 Script executed:
# Check the actual governance method to see if cs_main is required there
rg "GetPaymentsLimit" -B 5 -A 10 src/governance/ | head -80Repository: dashpay/dash
Length of output: 5691
🏁 Script executed:
# See how cs_main and cs_store interact in the codebase
rg "LOCK2.*cs_main.*cs_store|LOCK2.*cs_store.*cs_main" -A 3Repository: dashpay/dash
Length of output: 776
🏁 Script executed:
# Check what the learning note says about CChain operator[] and verify if that applies here
cat src/validation.h | grep -A 5 "class CChain"Repository: dashpay/dash
Length of output: 793
Guard ActiveChain().Height() access with cs_main lock.
The race condition concern is valid—ActiveChain().Height() requires the cs_main lock for thread safety. Simply renaming to ActiveHeight() does not fix this, as it is only a wrapper that still calls ActiveChain().Height() without locking. Use LOCK(::cs_main) to guard the access, consistent with other methods in this file that access the active chain (e.g., getNumBlocks(), getLastBlockTime()).
🛠️ Suggested fix
- const int32_t nTipHeight = context().chainman->ActiveChain().Height();
+ int32_t nTipHeight;
+ {
+ LOCK(::cs_main);
+ nTipHeight = context().chainman->ActiveChain().Height();
+ }🤖 Prompt for AI Agents
In `@src/node/interfaces.cpp` around lines 310 - 316, The call to
context().chainman->ActiveChain().Height() inside getProposalFundedHeight is not
protected by the cs_main lock; wrap the ActiveChain().Height() access in
LOCK(::cs_main) (as done in getNumBlocks() / getLastBlockTime()) to prevent a
race. Add a scoped LOCK(::cs_main) before reading nTipHeight so the loop over
context()->govman triggers uses a thread-safe snapshot of the active chain
height.
| double GetScaleSteps() const { return m_scale_steps; } | ||
| double GetScaledFontSize(int size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| double GetScaledFontSize(double size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| QString GetFont() const { return m_font; } |
There was a problem hiding this comment.
Run clang-format on this header (CI is failing).
The Clang Diff Format Check reports formatting differences here; please reformat before merge.
🤖 Prompt for AI Agents
In `@src/qt/guiutil_font.h` around lines 84 - 86, Run clang-format on
src/qt/guiutil_font.h to fix CI formatting failures; reformat the
declarations/Getters (GetScaleSteps, GetScaledFontSize, GetFont) and surrounding
spacing/line breaks so they match the project's clang-format rules, then stage
the updated header (ensuring member names m_scale_steps, m_font_scale, m_font
remain unchanged) and push the formatted file.
| void ProposalModel::setVotingParams(int newAbsVoteReq) | ||
| { | ||
| if (this->nAbsVoteReq != newAbsVoteReq) { | ||
| this->nAbsVoteReq = newAbsVoteReq; | ||
| // Changing either of the voting params may change the voting status | ||
| // column. Emit signal to force recalculation. | ||
| Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), createIndex(rowCount(), Column::VOTING_STATUS)); | ||
| } |
There was a problem hiding this comment.
Fix dataChanged range to avoid an out-of-range index.
Using rowCount() as the last index can produce an invalid QModelIndex. Guard for empty models and use rowCount() - 1.
🐛 Proposed fix
- Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), createIndex(rowCount(), Column::VOTING_STATUS));
+ if (rowCount() > 0) {
+ Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS),
+ createIndex(rowCount() - 1, Column::VOTING_STATUS));
+ }🤖 Prompt for AI Agents
In `@src/qt/proposalmodel.cpp` around lines 354 - 361, In
ProposalModel::setVotingParams the dataChanged call uses createIndex(rowCount(),
Column::VOTING_STATUS) which can create an out-of-range QModelIndex; fix by
guarding for empty model and emitting dataChanged only when rowCount() > 0,
using createIndex(0, Column::VOTING_STATUS) as the first index and
createIndex(rowCount() - 1, Column::VOTING_STATUS) as the last index so the
range is valid; update the conditional in setVotingParams to check rowCount()
before calling Q_EMIT dataChanged.
src/qt/proposalmodel.cpp
Outdated
| const Proposal* ProposalModel::getProposalAt(const QModelIndex& index) const | ||
| { | ||
| return m_data[index.row()]; |
There was a problem hiding this comment.
Guard getProposalAt against invalid indices.
A bad index (e.g., empty selection) will crash here. Safer to return nullptr for invalid/out-of-range rows.
🐛 Proposed fix
- return m_data[index.row()];
+ if (!index.isValid() || index.row() < 0 || index.row() >= m_data.count()) return nullptr;
+ return m_data[index.row()];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const Proposal* ProposalModel::getProposalAt(const QModelIndex& index) const | |
| { | |
| return m_data[index.row()]; | |
| const Proposal* ProposalModel::getProposalAt(const QModelIndex& index) const | |
| { | |
| if (!index.isValid() || index.row() < 0 || index.row() >= m_data.count()) return nullptr; | |
| return m_data[index.row()]; | |
| } |
🤖 Prompt for AI Agents
In `@src/qt/proposalmodel.cpp` around lines 364 - 366, The getProposalAt method
can dereference m_data with an invalid QModelIndex and crash; update
ProposalModel::getProposalAt to validate the index before accessing m_data by
checking index.isValid() and that index.row() is within the range 0 ..
m_data.size()-1 (and optionally that index.model() == this), and return nullptr
for any invalid or out-of-range index instead of indexing m_data directly.
kwvg
force-pushed
the
gov_refresh
branch
3 times, most recently
from
fc8a95a to
4490d40
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/qt/proposalmodel.cpp`:
- Around line 30-51: The Proposal constructor leaves m_paymentAmount
uninitialized if JSON parsing fails or "payment_amount" is absent; initialize
m_paymentAmount to a safe default (e.g., 0.0) either in the Proposal member
initializer list or immediately before parsing in the
Proposal::Proposal(ClientModel*, const CGovernanceObject&, QObject*) constructor
so that m_paymentAmount always has a defined value for display/sorting; update
the constructor to set m_paymentAmount = 0.0 (or another appropriate default)
and keep the existing assignment from prop_data when payment_amount exists.
In `@src/qt/proposalmodel.h`:
- Around line 71-83: Add a destructor for the ProposalModel class to free owned
Proposal pointers in m_data: implement ProposalModel::~ProposalModel() to
iterate m_data, delete each Proposal*, and clear the list; this mirrors
ownership semantics already used in remove() and reconcile() and prevents leaks
by releasing remaining proposals when the model is destroyed.
In `@src/qt/proposalresume.cpp`:
- Around line 135-141: The broadcast button lambdas capture a stale numeric
index (size_t index = m_entries.size()) causing wrong or out-of-bounds lookups
when entries are removed; change the lambda to find the clicked button's current
position by searching m_entries for the sender button pointer
(entry.broadcast_btn) at runtime and call onBroadcast with that computed index,
and add `#include` <algorithm> at the top for std::find support; update the
connect call to use the button pointer via the lambda capture (e.g., capture the
QPushButton* or use sender()) and compute index =
std::distance(m_entries.begin(), std::find(...)) before calling onBroadcast.
♻️ Duplicate comments (5)
src/node/interfaces.cpp (2)
317-331: GuardActiveChain().Height()access withcs_mainlock.As previously noted,
context().chainman->ActiveChain().Height()requires thecs_mainlock for thread safety. This is consistent with other methods in this file that access the active chain (e.g.,getNumBlocks(),getLastBlockTime()).🔒 Suggested fix
std::optional<int32_t> getProposalFundedHeight(const uint256& proposal_hash) override { if (context().govman != nullptr && context().chainman != nullptr && context().dmnman != nullptr) { - const int32_t nTipHeight = context().chainman->ActiveChain().Height(); + int32_t nTipHeight; + { + LOCK(::cs_main); + nTipHeight = context().chainman->ActiveChain().Height(); + } for (const auto& trigger : context().govman->GetActiveTriggers()) {
301-302: GuardActiveHeight()access withcs_mainlock.
ctx.chainman->ActiveHeight()reads chain state that can change concurrently. This should be protected bycs_mainfor thread safety, consistent with other chain height accesses in this file (e.g.,getNumBlocks(),getLastBlockTime()).Note: Lines 309-314 have a similar pre-existing issue where
GetListAtChainTip()andActiveChain()are accessed outside a lock. Consider wrapping the entire block that accesses chain state.🔒 Suggested fix
if (ctx.chainman) { + LOCK(::cs_main); CSuperblock::GetNearestSuperblocksHeights(ctx.chainman->ActiveHeight(), info.lastsuperblock, info.nextsuperblock); }Or consolidate locking for all chain-dependent reads in this method.
src/qt/proposalmodel.cpp (2)
356-362: GuarddataChangedwhen the model is empty.
rowCount() - 1can be-1, producing an invalid QModelIndex range.🐛 Proposed fix
- Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), createIndex(rowCount() - 1, Column::VOTING_STATUS)); + if (rowCount() > 0) { + Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), + createIndex(rowCount() - 1, Column::VOTING_STATUS)); + }
366-370: Bounds check ingetProposalAtis off by one.
std::clamp(..., 0, m_data.count())allowsrow == count, which can access past the end.🐛 Proposed fix
- if (index.isValid() && index.row() == std::clamp(index.row(), 0, m_data.count())) { - return m_data[index.row()]; - } - return nullptr; + if (!index.isValid() || index.row() < 0 || index.row() >= m_data.count()) return nullptr; + return m_data[index.row()];src/Makefile.qt.include (1)
164-167: Verify non-backported.txt includes new proposal C++ files.Per previous review feedback, the following new C++ files should be added to
test/util/data/non-backported.txt:
src/qt/descriptiondialog.cppandsrc/qt/descriptiondialog.hsrc/qt/proposaldesc.cppandsrc/qt/proposaldesc.hsrc/qt/proposalmodel.cppandsrc/qt/proposalmodel.hsrc/qt/proposalresume.cppandsrc/qt/proposalresume.hNote:
src/qt/proposalwizard.*is already registered, and.uifiles should not be added per project guidelines.#!/bin/bash # Verify non-backported.txt includes the new proposal C++ files echo "=== Current entries in non-backported.txt ===" cat test/util/data/non-backported.txt | grep -E "(descriptiondialog|proposaldesc|proposalmodel|proposalresume)" || echo "No matching entries found" echo "" echo "=== Expected new entries ===" echo "src/qt/descriptiondialog.cpp" echo "src/qt/descriptiondialog.h" echo "src/qt/proposaldesc.cpp" echo "src/qt/proposaldesc.h" echo "src/qt/proposalmodel.cpp" echo "src/qt/proposalmodel.h" echo "src/qt/proposalresume.cpp" echo "src/qt/proposalresume.h"Also applies to: 268-279
🧹 Nitpick comments (6)
src/qt/proposalmodel.h (1)
42-49: Assert non‑null ClientModel inProposalconstruction.Most methods dereference
clientModel, so a fail‑fast assert makes the invariant explicit.♻️ Suggested assert
+#include <cassert> ... Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : QObject(parent), clientModel(_clientModel), govObj(_govObj) { + assert(clientModel); ... }Based on learnings, prefer a fail‑fast assert to enforce this invariant.
src/qt/descriptiondialog.cpp (1)
6-11: Consider reordering includes for consistency.The typical convention is to include the corresponding header file first (descriptiondialog.h), followed by other project headers, then third-party headers. Currently, the UI header is included first.
♻️ Suggested include order
-#include <qt/forms/ui_descriptiondialog.h> - `#include` <qt/descriptiondialog.h> + +#include <qt/forms/ui_descriptiondialog.h> `#include` <qt/guiutil_font.h> - `#include` <qt/guiutil.h>src/qt/optionsmodel.h (1)
163-164: Consider adding default initializers for safety.The new member variables
m_enable_masternodesandm_enable_governancelack default initializers. WhileInit()sets these values from QSettings before they're used, adding default initializers (e.g.,{false}) provides defense-in-depth against potential future code paths that might access these before initialization.Suggested change
- bool m_enable_masternodes; - bool m_enable_governance; + bool m_enable_masternodes{false}; + bool m_enable_governance{false};src/qt/bitcoingui.cpp (1)
1470-1499: Refactor to eliminate hard-coded toolbar action indices.The visibility methods use hard-coded indices to access toolbar actions:
- Line 1460:
appToolBar->actions()[4]for CoinJoin- Line 1478:
appToolBar->actions()[5]for Masternodes- Line 1494:
appToolBar->actions()[6]for GovernanceThis pattern is fragile. Store action pointers when buttons are added to the toolbar, or use a lookup method (e.g.,
QToolBar::widgetForAction()) instead of relying on index positions. If the toolbar layout changes, these indices will silently reference wrong actions.src/qt/guiutil_font.cpp (1)
212-243: Character-by-character iteration may be slow for large HTML documents.The
setFontBodyHTMLfunction iterates one character at a time through the entire document to apply formatting. For large documents, this could cause noticeable UI lag.Consider using
QTextBlockiteration combined withQTextBlock::iteratorfor fragment-level processing, which would be more efficient:QTextBlock block = widget->document()->begin(); while (block.isValid()) { for (auto it = block.begin(); !it.atEnd(); ++it) { QTextFragment fragment = it.fragment(); // Process fragment... } block = block.next(); }That said, for typical proposal descriptions this is likely acceptable.
src/qt/proposalwizard.cpp (1)
206-212: Consider more specific error handling for creation failures.The error handling shows a generic error message from
prepareProposal. It might be helpful to distinguish between different failure modes (e.g., insufficient funds, network issues, validation failures) for better user guidance.However, this is a minor UX improvement and the current implementation is functional.
src/qt/proposalmodel.cpp
Outdated
| Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : | ||
| QObject(parent), | ||
| clientModel(_clientModel), | ||
| govObj(_govObj) | ||
| { | ||
| UniValue prop_data; | ||
| if (prop_data.read(govObj.GetDataAsPlainString())) { | ||
| if (const UniValue& titleValue = prop_data.find_value("name"); titleValue.isStr()) { | ||
| m_title = QString::fromStdString(titleValue.get_str()); | ||
| } | ||
|
|
||
| if (const UniValue& paymentStartValue = prop_data.find_value("start_epoch"); paymentStartValue.isNum()) { | ||
| m_startDate = QDateTime::fromSecsSinceEpoch(paymentStartValue.getInt<int64_t>()); | ||
| } | ||
|
|
||
| if (const UniValue& paymentEndValue = prop_data.find_value("end_epoch"); paymentEndValue.isNum()) { | ||
| m_endDate = QDateTime::fromSecsSinceEpoch(paymentEndValue.getInt<int64_t>()); | ||
| } | ||
|
|
||
| if (const UniValue& amountValue = prop_data.find_value("payment_amount"); amountValue.isNum()) { | ||
| m_paymentAmount = amountValue.get_real(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and locate the class definition
fd proposalmodel '\.(cpp|h)$'Repository: dashpay/dash
Length of output: 168
🏁 Script executed:
# Read the header file to see the class declaration
cat src/qt/proposalmodel.hRepository: dashpay/dash
Length of output: 2848
🏁 Script executed:
# Read the full cpp file to see all usages of m_paymentAmount
cat src/qt/proposalmodel.cppRepository: dashpay/dash
Length of output: 13081
Initialize m_paymentAmount to avoid undefined behavior.
If JSON parsing fails or the payment_amount field is missing, this member remains uninitialized but is later used in display and sorting operations, leading to undefined behavior.
🐛 Proposed fix
Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) :
QObject(parent),
clientModel(_clientModel),
- govObj(_govObj)
+ govObj(_govObj),
+ m_paymentAmount(0.0)
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : | |
| QObject(parent), | |
| clientModel(_clientModel), | |
| govObj(_govObj) | |
| { | |
| UniValue prop_data; | |
| if (prop_data.read(govObj.GetDataAsPlainString())) { | |
| if (const UniValue& titleValue = prop_data.find_value("name"); titleValue.isStr()) { | |
| m_title = QString::fromStdString(titleValue.get_str()); | |
| } | |
| if (const UniValue& paymentStartValue = prop_data.find_value("start_epoch"); paymentStartValue.isNum()) { | |
| m_startDate = QDateTime::fromSecsSinceEpoch(paymentStartValue.getInt<int64_t>()); | |
| } | |
| if (const UniValue& paymentEndValue = prop_data.find_value("end_epoch"); paymentEndValue.isNum()) { | |
| m_endDate = QDateTime::fromSecsSinceEpoch(paymentEndValue.getInt<int64_t>()); | |
| } | |
| if (const UniValue& amountValue = prop_data.find_value("payment_amount"); amountValue.isNum()) { | |
| m_paymentAmount = amountValue.get_real(); | |
| } | |
| Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : | |
| QObject(parent), | |
| clientModel(_clientModel), | |
| govObj(_govObj), | |
| m_paymentAmount(0.0) | |
| { | |
| UniValue prop_data; | |
| if (prop_data.read(govObj.GetDataAsPlainString())) { | |
| if (const UniValue& titleValue = prop_data.find_value("name"); titleValue.isStr()) { | |
| m_title = QString::fromStdString(titleValue.get_str()); | |
| } | |
| if (const UniValue& paymentStartValue = prop_data.find_value("start_epoch"); paymentStartValue.isNum()) { | |
| m_startDate = QDateTime::fromSecsSinceEpoch(paymentStartValue.getInt<int64_t>()); | |
| } | |
| if (const UniValue& paymentEndValue = prop_data.find_value("end_epoch"); paymentEndValue.isNum()) { | |
| m_endDate = QDateTime::fromSecsSinceEpoch(paymentEndValue.getInt<int64_t>()); | |
| } | |
| if (const UniValue& amountValue = prop_data.find_value("payment_amount"); amountValue.isNum()) { | |
| m_paymentAmount = amountValue.get_real(); | |
| } |
🤖 Prompt for AI Agents
In `@src/qt/proposalmodel.cpp` around lines 30 - 51, The Proposal constructor
leaves m_paymentAmount uninitialized if JSON parsing fails or "payment_amount"
is absent; initialize m_paymentAmount to a safe default (e.g., 0.0) either in
the Proposal member initializer list or immediately before parsing in the
Proposal::Proposal(ClientModel*, const CGovernanceObject&, QObject*) constructor
so that m_paymentAmount always has a defined value for display/sorting; update
the constructor to set m_paymentAmount = 0.0 (or another appropriate default)
and keep the existing assignment from prop_data when payment_amount exists.
| class ProposalModel : public QAbstractTableModel | ||
| { | ||
| Q_OBJECT | ||
|
|
||
| private: | ||
| QList<const Proposal*> m_data; | ||
| int nAbsVoteReq = 0; | ||
| BitcoinUnit m_display_unit{BitcoinUnit::DASH}; | ||
|
|
||
| public: | ||
| explicit ProposalModel(QObject* parent = nullptr) : | ||
| QAbstractTableModel(parent){}; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the proposalmodel files
find . -name "proposalmodel.*" -type f | head -20Repository: dashpay/dash
Length of output: 168
🏁 Script executed:
# Read the header file to understand the full class structure
if [ -f "src/qt/proposalmodel.h" ]; then
wc -l src/qt/proposalmodel.h
fiRepository: dashpay/dash
Length of output: 82
🏁 Script executed:
# Read the implementation file if it exists
if [ -f "src/qt/proposalmodel.cpp" ]; then
wc -l src/qt/proposalmodel.cpp
fiRepository: dashpay/dash
Length of output: 84
🏁 Script executed:
cat -n src/qt/proposalmodel.hRepository: dashpay/dash
Length of output: 3611
🏁 Script executed:
cat -n src/qt/proposalmodel.cppRepository: dashpay/dash
Length of output: 15699
Add a destructor to ProposalModel to release owned proposals.
ProposalModel explicitly deletes proposals in remove() and reconcile(), indicating ownership. Without a destructor, remaining proposals leak when the model is destroyed.
Proposed fix
class ProposalModel : public QAbstractTableModel
{
Q_OBJECT
...
public:
explicit ProposalModel(QObject* parent = nullptr) :
QAbstractTableModel(parent){};
+ ~ProposalModel() override;// src/qt/proposalmodel.cpp
ProposalModel::~ProposalModel()
{
for (auto* p : m_data) delete p;
m_data.clear();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class ProposalModel : public QAbstractTableModel | |
| { | |
| Q_OBJECT | |
| private: | |
| QList<const Proposal*> m_data; | |
| int nAbsVoteReq = 0; | |
| BitcoinUnit m_display_unit{BitcoinUnit::DASH}; | |
| public: | |
| explicit ProposalModel(QObject* parent = nullptr) : | |
| QAbstractTableModel(parent){}; | |
| class ProposalModel : public QAbstractTableModel | |
| { | |
| Q_OBJECT | |
| private: | |
| QList<const Proposal*> m_data; | |
| int nAbsVoteReq = 0; | |
| BitcoinUnit m_display_unit{BitcoinUnit::DASH}; | |
| public: | |
| explicit ProposalModel(QObject* parent = nullptr) : | |
| QAbstractTableModel(parent){}; | |
| ~ProposalModel() override; |
🤖 Prompt for AI Agents
In `@src/qt/proposalmodel.h` around lines 71 - 83, Add a destructor for the
ProposalModel class to free owned Proposal pointers in m_data: implement
ProposalModel::~ProposalModel() to iterate m_data, delete each Proposal*, and
clear the list; this mirrors ownership semantics already used in remove() and
reconcile() and prevents leaks by releasing remaining proposals when the model
is destroyed.
src/qt/proposalresume.cpp
Outdated
| size_t index = m_entries.size(); | ||
| entry.broadcast_btn = new QPushButton(tr("Broadcast"), entry.container); | ||
| entry.broadcast_btn->setEnabled(false); | ||
| entry.broadcast_btn->setMinimumWidth(100); | ||
| connect(entry.broadcast_btn, &QPushButton::clicked, this, [this, index]() { | ||
| onBroadcast(index); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -e cpp -e h "proposalresume" src/Repository: dashpay/dash
Length of output: 105
🏁 Script executed:
wc -l src/qt/proposalresume.cpp src/qt/proposalresume.hRepository: dashpay/dash
Length of output: 129
🏁 Script executed:
cat -n src/qt/proposalresume.hRepository: dashpay/dash
Length of output: 2399
🏁 Script executed:
cat -n src/qt/proposalresume.cppRepository: dashpay/dash
Length of output: 11423
Broadcast buttons can break after an entry is removed.
Captured indices become stale when earlier entries are erased. After removing entry 0, remaining button 1 with captured index=1 will target the wrong entry (original entry 2), and button 2 may access out-of-bounds data.
🐛 Proposed fix (lookup by button pointer)
- size_t index = m_entries.size();
entry.broadcast_btn = new QPushButton(tr("Broadcast"), entry.container);
entry.broadcast_btn->setEnabled(false);
entry.broadcast_btn->setMinimumWidth(100);
- connect(entry.broadcast_btn, &QPushButton::clicked, this, [this, index]() {
- onBroadcast(index);
- });
+ connect(entry.broadcast_btn, &QPushButton::clicked, this, [this, btn = entry.broadcast_btn]() {
+ auto it = std::find_if(m_entries.begin(), m_entries.end(),
+ [btn](const ProposalEntry& e) { return e.broadcast_btn == btn; });
+ if (it != m_entries.end()) {
+ onBroadcast(static_cast<size_t>(std::distance(m_entries.begin(), it)));
+ }
+ });Add #include <algorithm> at the top of the file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| size_t index = m_entries.size(); | |
| entry.broadcast_btn = new QPushButton(tr("Broadcast"), entry.container); | |
| entry.broadcast_btn->setEnabled(false); | |
| entry.broadcast_btn->setMinimumWidth(100); | |
| connect(entry.broadcast_btn, &QPushButton::clicked, this, [this, index]() { | |
| onBroadcast(index); | |
| }); | |
| entry.broadcast_btn = new QPushButton(tr("Broadcast"), entry.container); | |
| entry.broadcast_btn->setEnabled(false); | |
| entry.broadcast_btn->setMinimumWidth(100); | |
| connect(entry.broadcast_btn, &QPushButton::clicked, this, [this, btn = entry.broadcast_btn]() { | |
| auto it = std::find_if(m_entries.begin(), m_entries.end(), | |
| [btn](const ProposalEntry& e) { return e.broadcast_btn == btn; }); | |
| if (it != m_entries.end()) { | |
| onBroadcast(static_cast<size_t>(std::distance(m_entries.begin(), it))); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@src/qt/proposalresume.cpp` around lines 135 - 141, The broadcast button
lambdas capture a stale numeric index (size_t index = m_entries.size()) causing
wrong or out-of-bounds lookups when entries are removed; change the lambda to
find the clicked button's current position by searching m_entries for the sender
button pointer (entry.broadcast_btn) at runtime and call onBroadcast with that
computed index, and add `#include` <algorithm> at the top for std::find support;
update the connect call to use the button pointer via the lambda capture (e.g.,
capture the QPushButton* or use sender()) and compute index =
std::distance(m_entries.begin(), std::find(...)) before calling onBroadcast.
Also `s/coinJoinEnabledChanged/showCoinJoinChanged/g` to establish a naming convention that will be used soon.
This should fix the misalignment of the `subFeeFromAmount` `QCheckBox` compared to the rest of the `QCheckBox`es Review with `git log -p -n1 --ignore-space-change --color-moved=dimmed_zebra`.
Review with `git log -p -n1 --color-moved=dimmed_zebra`.
kwvg
changed the title
QTextEdit styling, governance layout refresh)Review with `git log -p -n1 --color-moved=dimmed_zebra`.
Inheriting the whole structure is overkill just to use `tr` and `hash()` is used in hot loops so we should cache the value in the ctor. Also remove dead variable.
Review with `git log -p -n1 --ignore-space-change --color-moved=dimmed_zebra`.
Upcoming work will require multiple such containers and we might as well reuse what we've got.
This is closer to how we report details for transactions and more translation-friendly and easier to copy.
Visiting URLs could be dangerous, add a warning before we open a link in someone's browser.
Dash Governance Tools' budget generator is a much more visually friendly design and we can model our wizard after it.
The JSON and payload hex means little for the vast majority of users, what matters is the error messages, so we responsively validate contents and drop an extra panel in the process.
The visual language should match other coin-spending activities, also warn that that you don't get the fee back.
The proposal creation process can be interrupted for reasons beyond our control, since submitting the actual proposal is different from spending the collateral, it should have its own page. Right now this removes the old code but doesn't create a new layout, we will do that in the next commit.
This replaces the remaining pages of the proposal wizard that we removed in the last commit.
You cannot create a proposal without an adequate balance, it's a bad idea to allow any kind of proposal manipulation without being synced.
We are no longer using the wizard format so the name is a misnomer, the
logic is also relatively simplified (as now half of it is in
`qt/proposalresume.{cpp,h}`)
1 participant
Additional Information
Proposal Creation
Breaking Changes
None expected.
Checklist