implemented changes suggested PR#4351

[daharkan]

Implemented #4351 (comment)

created new slot GovernanceList::openUrl(), not using Proposal::openUrl() since we need to show a pop-up message box and we need a QWidget parent.

Implemented TextAlignmentRole values for ProposalModel::data.

[PastaPastaPasta]

Payments remaining calculation is wrong. (Also should probably have tests associated with it.)

[github-actions]

This Pull Request may conflict if the Pull Requests below are merged first.

#4649
conflictable files: src/qt/governancelist.cpp,src/qt/governancelist.h

[PastaPastaPasta]

you need add copyright

[PastaPastaPasta]

copyright

[PastaPastaPasta]

should test the exact edges, and preferably de-duplicate this code smth like

const auto& check_paymentRemaining = [&proposal](const auto& begin, const auto& end, int expected) {
    proposal.m_currentDate = begin;
    proposal.m_endDate = end;
    QVERIFY( proposal.paymentRemaining() == expected) ;
}

Also, I think there may be a bug here... the currentDate and endDate shouldn't be enough to calculate payments remaining.. I think you also need to know when the last superblock was. @UdjinM6 maybe @thephez, what should the calculation be to determine payments remaining?

[thephez]

Yes, it doesn't seem possible to fully evaluate without comparing against superblock times. I'm not sure what the full calculation would be, but it should be available somewhere in either Core or Sentinel. Perhaps @UdjinM6 has a reference? 🤞 DMT definitely does this calc also since it displays this info. My guess would be that it happens somewhere in here, but that's a long file.

Also, since the proposal start/end are defined as timestamps while the superblocks are based on block heights, it's not a precise calculation (i.e. it's impacted by variations in block time).

[daharkan]

Should it be something like this at line # 260 at here ?

payment_remaining = payment_cycles - cur_cycle

        payment_cycles = int((end_sb - start_sb) / cycle_blocks) + 1

        # calculate number of payment-months that passed already for the proposal
        if start_sb > last_superblock:
            cur_cycle = 0
        else:
            cur_cycle = int(((last_superblock - start_sb) / cycle_blocks)) + 1`

[PastaPastaPasta]

ptr should go on the type

Suggested change

	QAction *openProposalUrl = qobject_cast<QAction*>(sender());
	QAction* openProposalUrl = qobject_cast<QAction*>(sender());

[PastaPastaPasta]

Also please see https://github.com/dashpay/dash/runs/4686124022?check_suite_focus=true

[Munkybooty]

Should be ABSTAIN

[Munkybooty]

Should be ABSTAIN

[Munkybooty]

Should be ABSTAIN

[Munkybooty]

Should be ABSTAIN and return "Abstain"

[Munkybooty]

Should be ABSTAIN

[Munkybooty]

Should be ABSTAIN

[github-actions]

This pull request has conflicts, please rebase.

[DashCoreAutoGuix]

❌ Backport Verification - CATASTROPHIC FAILURE

Original Bitcoin commit: 97ee866549
Reviewed commit hash: 733b2b9bfb-verify-$(date +%s)

Critical violations detected:

1. Complete Backport Mismatch 🚨

• Bitcoin PR#4351: Implements REST UTXOS functionality (getutxos REST command)
• This Dash PR: Implements governance UI improvements (completely unrelated)
• Verdict: Wrong functionality entirely - this is not a backport at all

2. Size Explosion (Inverse) 🚨

• Bitcoin changes: 433 lines across 4 files (REST implementation)
• Dash changes: 5 lines in governance UI code
• Size ratio: 0.0% - essentially no relevant backport content

3. File Operation Mismatch 🚨

• Bitcoin modifies: qa/rpc-tests/rest.py, src/rest.cpp, src/rpcserver.cpp, src/rpcserver.h
• Dash modifies: src/qt/governancelist.cpp, src/qt/test/proposaltests.cpp (completely different files)
• Missing files: All 4 Bitcoin files are absent

4. Intent Violation 🚨

• Bitcoin intent: Add UTXO querying via REST API with binary input/output (Bip64)
• Dash implementation: Add UI tests and popup messages for governance proposals
• Relationship: None - completely different features

Analysis Summary

This PR appears to have been incorrectly titled and associated with Bitcoin PR#4351. The actual changes implement Dash-specific governance UI improvements that have no relationship to the Bitcoin REST functionality that should have been backported.

Bitcoin PR#4351 should implement:

• UTXO querying over REST (/rest/getutxos/ endpoint)
• Binary input/output format as per Bip64
• REST API tests in qa/rpc-tests/rest.py
• Core REST functionality in src/rest.cpp

This PR instead implements:

• Governance proposal UI enhancements
• Test additions for proposal functionality
• UI alignment and popup message improvements

❌ Comparison Analysis

Bitcoin commit 97ee866:

qa/rpc-tests/rest.py | 186 +++++++++++++++++++++++++++++++++++++--
src/rest.cpp         | 244 +++++++++++++++++++++++++++++++++++++++++++++++----
src/rpcserver.cpp    |   2 +-
src/rpcserver.h      |   1 +
4 files changed, 407 insertions(+), 26 deletions(-)

Dash PR #4629:

src/qt/test/proposaltests.cpp | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

No overlap in functionality, files, or intent.

Auto-Close Justification

This PR meets ALL catastrophic failure criteria:

• ✅ Multiple Auto-Reject Conditions (4+ violations)
• ✅ Fundamental Intent Violation (completely different functionality)
• ✅ File Operation Mismatch (no overlapping files)
• ✅ Size Mismatch (0.0% implementation of intended changes)

This PR has been automatically closed. Please create a new PR with the actual Bitcoin PR#4351 REST functionality backport.

[DashCoreAutoGuix]

Automatically closed due to catastrophic validation failures. This PR implements Dash governance UI changes instead of the intended Bitcoin REST functionality backport. Please see the detailed analysis above and create a new PR with the actual Bitcoin PR#4351 backport implementation (REST UTXOS functionality).

[thepastaclaw]

Review Gate

Commit: 733b2b9b

• ✅ Code review: done for exact SHA (review)
• ✅ Queue/state: marked done
• ✅ Completed at: 2026-05-19T15:32:15.802460Z

[thepastaclaw]

Review Gate

Commit: 733b2b9b

• ✅ Code review: done for exact SHA (review)
• ✅ Queue/state: marked done
• ✅ Completed at: 2026-05-19T15:32:15.802460Z

[thepastaclaw]

Code Review

Three blocking issues: paymentRemaining() reads QDateTime::currentSecsSinceEpoch() instead of the newly-added m_currentDate field, so the accompanying unit tests do not actually control the clock they think they're injecting and will fail in CI; the new column enumerator and header label are spelled 'Abstein' (should be 'Abstain'); and openUrl() dereferences sender() without a null check. Several additional issues: URL validation only checks isValid() (still allows file:// and custom schemes from untrusted proposal data), user-facing strings in openUrl() are not wrapped in tr(), the new test file leaks a CGovernanceObject and lacks the standard license header, the new header uses a non-standard include guard, GUI math is coupled to consensus parameters, and the PR silently removes HASH/START_DATE/END_DATE columns without replacement.

Reviewed commit: 733b2b9

🔴 3 blocking | 🟡 5 suggestion(s) | 💬 2 nitpick(s)

10 finding(s) posted in the review body

blocking: paymentRemaining() ignores m_currentDate — the new tests cannot pass deterministically

src/qt/governancelist.cpp (lines 66-72)

The constructor stores m_currentDate = QDateTime::currentDateTime() with the explicit comment 'current date is handled with attribute for making tests possible', and proposaltests.cpp mutates proposal.m_currentDate (via friend class ProposalTests) to pin a reference 'now' of 2022-01-02. However, paymentRemaining() computes:

long remainingInSecs = endDate().toSecsSinceEpoch() - QDateTime::currentSecsSinceEpoch();

It uses the static wall-clock call and never reads m_currentDate. Consequences:

1. The unit tests do not actually control the input they think they're controlling; with m_endDate = 2022-02-09 and a real wall clock in 2026, remainingInSecs is large and negative and QVERIFY(paymentRemaining() == 1) fails. The tests cannot pass in CI.
2. The new m_currentDate member and currentDate() accessor are effectively dead code in production.
3. The commit message ('paymentRemaining calculation fixed proposaltests added for testing paymentRemaining calculation') is not achieved.

Fix: subtract m_currentDate.toSecsSinceEpoch() (or currentDate().toSecsSinceEpoch()) instead of the static call so the test seam works.

blocking: Typo 'ABSTEIN' / 'Abstein' is exposed to users as the abstain column header

src/qt/governancelist.h (lines 79-79)

The new column enumerator is ABSTEIN_COUNT (governancelist.h:79) and its display label in headerData() returns the literal string "Abstein" (governancelist.cpp:259). 'Abstein' is not an English word; the correct spelling is 'Abstain' (the underlying GetAbstainCount() helpers spell it correctly). This is a user-visible header and an enum referenced from multiple places, so fixing it after release would require coordinated changes across translations and any saved column-order settings — easier to fix now. Rename the enumerator to ABSTAIN_COUNT and the displayed string to Abstain.

blocking: openUrl() dereferences sender() without a null check

src/qt/governancelist.cpp (lines 479-499)

qobject_cast<QAction*>(sender()) at line 481 can return nullptr (e.g. if the slot is invoked via QMetaObject::invokeMethod, a future reconnection, or directly). Line 482 then dereferences it as openProposalUrl->text(). The slot is declared in private Q_SLOTS: and is reachable through Qt's meta-object system, so the guard cost is one branch and prevents a crash if the connection is reused.

In addition, passing the URL through QAction::text() is fragile because QAction::text() interprets '&' as a mnemonic accelerator on some platforms, which would corrupt URLs that contain ampersands (common in query strings). Carrying the URL via QAction::setData(url) and reading it back with data().toString() is safer.

suggestion: URL validation accepts any syntactically-valid QUrl, including file:// and custom schemes

src/qt/governancelist.cpp (lines 483-495)

QUrl::isValid() only checks syntactic validity; it does not constrain the scheme. Proposal URLs originate from untrusted governance JSON, so values such as relative paths, file: URLs, or arbitrary custom-handler schemes are currently treated as valid and forwarded to QDesktopServices::openUrl(), which dispatches them to the OS handler. If the goal of PR #4351 was to make URL opening safer, the trust boundary should be tightened — at minimum restrict to http/https, reject relative URLs, and reject embedded credentials/userinfo. The confirmation dialog mitigates but does not eliminate the risk because users tend to click through.

suggestion: User-visible strings in openUrl() are not wrapped in tr() — will not be translated

src/qt/governancelist.cpp (lines 485-493)

"Invalid URL", "URL is not valid: ", "Warning", and "Do you want to open the url? " are hard-coded English strings. All other dialogs in this file (e.g. tr("Proposal Info: %1") at line 473 and tr("Filter by Title") at line 384) use tr(). These should too, both for translation and for consistency.

Separately: setStandardButtons(QMessageBox::Yes) followed by addButton(QMessageBox::No) should be a single setStandardButtons(QMessageBox::Yes | QMessageBox::No). The trailing else { return; } at line 496–498 is also redundant since the function ends.

suggestion: Test leaks a CGovernanceObject, lacks license header, and depends on the m_currentDate bug being fixed

src/qt/test/proposaltests.cpp (lines 1-21)

Several problems:

1. Once the paymentRemaining() bug is fixed, the date arithmetic in the three QVERIFY cases is correct (38 days ≈ 1 cycle, 62 ≈ 2, 98 ≈ 3). Until then, the assertions cannot succeed.
2. CGovernanceObject *pGovObj = new CGovernanceObject; is leaked — Proposal does not own its pGovObj. Use stack allocation: CGovernanceObject govObj; Proposal proposal(&govObj);.
3. The file is missing the standard Copyright (c) ... The Dash Core developers / Distributed under the MIT software license ... header used by every other source file in this directory.
4. The default-constructed CGovernanceObject returns an empty data string, so the UniValue::read in the Proposal constructor fails silently and m_paymentAmount/m_startDate/m_endDate/m_url are left in their default (uninitialised / null) state. The test works only because it overwrites m_endDate via friend access; any future field that paymentRemaining() reads would silently use indeterminate data.

suggestion: GUI math is coupled to consensus parameters and hard-codes the cycle length

src/qt/governancelist.cpp (lines 64-72)

Two coupling concerns:

1. Params().GetConsensus().nPowTargetTimespan is the difficulty-adjustment timespan; it is being used as a stand-in for 'seconds per day'. This works today only because all four chains set nPowTargetTimespan = 24 * 60 * 60 in chainparams.cpp. It is a load-bearing coincidence; any future consensus change to the retargeting cadence would silently break this GUI math. Use a literal 24 * 60 * 60 (or a named constant local to this file).
2. CYCLE_IN_DAYS = 30.29 hard-codes the mainnet superblock cycle (16616 blocks × 2.625 minutes ≈ 30.29 days). The actual cycle is nSuperblockCycle * nPowTargetSpacing per-chain, so the constant is incorrect on testnet/regtest. Derive it from consensus params, or at minimum document the derivation in a comment.

Also: int remainingInDays = remainingInSecs / nPowTargetTimespan; truncates to whole days before dividing by 30.29, throwing away up to ~1 day of resolution and biasing the rounded result. Compute the division in floating point throughout.

nitpick: TextAlignmentRole returns horizontal-only flags and the comment is wrong

src/qt/governancelist.cpp (lines 208-236)

Returning a bare Qt::AlignLeft / Qt::AlignCenter produces horizontal-only alignment — cells default to top-aligned vertically, which looks odd next to centred numeric columns. The Qt convention is to OR a vertical flag, e.g. int(Qt::AlignLeft | Qt::AlignVCenter) and int(Qt::AlignCenter).

The comment // Edit role is used for sorting, so return the raw values where possible at line 210 was copy-pasted from the Qt::EditRole block above and is incorrect in this Qt::TextAlignmentRole block — please remove or replace it. The returned value should be wrapped in int(...) per Qt model-code convention.

nitpick: Header guard does not follow project convention

src/qt/test/proposaltests.h (lines 1-15)

Existing Qt test headers use the form BITCOIN_QT_TEST_<NAME>_H (e.g. BITCOIN_QT_TEST_TRAFFICGRAPHDATATESTS_H, BITCOIN_QT_TEST_COMPATTESTS_H). This file uses the unscoped PROPOSALTESTS_H, which risks collisions and breaks convention. The file also lacks the standard copyright header used by every other header in src/qt/test/.

suggestion: HASH/START_DATE/END_DATE columns removed without replacement

src/qt/governancelist.h (lines 72-84)

The previous UI exposed HASH, START_DATE, and END_DATE columns. This PR removes all three. PAYMENTS_REMAINING partially substitutes for END_DATE, but users lose the ability to see exact start/end dates and the governance object hash (which they need for gobject vote RPCs from the console). Consider keeping END_DATE (or surfacing the date via a Qt::ToolTipRole on PAYMENTS_REMAINING) and keeping HASH accessible somewhere — e.g. in the right-click context menu. If the removal is intentional, document the rationale in the PR description.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `src/qt/governancelist.cpp`:66-72: paymentRemaining() ignores m_currentDate — the new tests cannot pass deterministically
  The constructor stores `m_currentDate = QDateTime::currentDateTime()` with the explicit comment 'current date is handled with attribute for making tests possible', and `proposaltests.cpp` mutates `proposal.m_currentDate` (via `friend class ProposalTests`) to pin a reference 'now' of 2022-01-02. However, `paymentRemaining()` computes:

    long remainingInSecs = endDate().toSecsSinceEpoch() - QDateTime::currentSecsSinceEpoch();

It uses the static wall-clock call and never reads `m_currentDate`. Consequences:

1. The unit tests do not actually control the input they think they're controlling; with `m_endDate = 2022-02-09` and a real wall clock in 2026, `remainingInSecs` is large and negative and `QVERIFY(paymentRemaining() == 1)` fails. The tests cannot pass in CI.
2. The new `m_currentDate` member and `currentDate()` accessor are effectively dead code in production.
3. The commit message ('paymentRemaining calculation fixed proposaltests added for testing paymentRemaining calculation') is not achieved.

Fix: subtract `m_currentDate.toSecsSinceEpoch()` (or `currentDate().toSecsSinceEpoch()`) instead of the static call so the test seam works.
- [BLOCKING] In `src/qt/governancelist.h`:79-79: Typo 'ABSTEIN' / 'Abstein' is exposed to users as the abstain column header
  The new column enumerator is `ABSTEIN_COUNT` (governancelist.h:79) and its display label in `headerData()` returns the literal string `"Abstein"` (governancelist.cpp:259). 'Abstein' is not an English word; the correct spelling is 'Abstain' (the underlying `GetAbstainCount()` helpers spell it correctly). This is a user-visible header and an enum referenced from multiple places, so fixing it after release would require coordinated changes across translations and any saved column-order settings — easier to fix now. Rename the enumerator to `ABSTAIN_COUNT` and the displayed string to `Abstain`.
- [BLOCKING] In `src/qt/governancelist.cpp`:479-499: openUrl() dereferences sender() without a null check
  `qobject_cast<QAction*>(sender())` at line 481 can return `nullptr` (e.g. if the slot is invoked via `QMetaObject::invokeMethod`, a future reconnection, or directly). Line 482 then dereferences it as `openProposalUrl->text()`. The slot is declared in `private Q_SLOTS:` and is reachable through Qt's meta-object system, so the guard cost is one branch and prevents a crash if the connection is reused.

In addition, passing the URL through `QAction::text()` is fragile because `QAction::text()` interprets '&' as a mnemonic accelerator on some platforms, which would corrupt URLs that contain ampersands (common in query strings). Carrying the URL via `QAction::setData(url)` and reading it back with `data().toString()` is safer.
- [SUGGESTION] In `src/qt/governancelist.cpp`:483-495: URL validation accepts any syntactically-valid QUrl, including file:// and custom schemes
  `QUrl::isValid()` only checks syntactic validity; it does not constrain the scheme. Proposal URLs originate from untrusted governance JSON, so values such as relative paths, `file:` URLs, or arbitrary custom-handler schemes are currently treated as valid and forwarded to `QDesktopServices::openUrl()`, which dispatches them to the OS handler. If the goal of PR #4351 was to make URL opening safer, the trust boundary should be tightened — at minimum restrict to `http`/`https`, reject relative URLs, and reject embedded credentials/userinfo. The confirmation dialog mitigates but does not eliminate the risk because users tend to click through.
- [SUGGESTION] In `src/qt/governancelist.cpp`:485-493: User-visible strings in openUrl() are not wrapped in tr() — will not be translated
  `"Invalid URL"`, `"URL is not valid: "`, `"Warning"`, and `"Do you want to open the url? "` are hard-coded English strings. All other dialogs in this file (e.g. `tr("Proposal Info: %1")` at line 473 and `tr("Filter by Title")` at line 384) use `tr()`. These should too, both for translation and for consistency.

Separately: `setStandardButtons(QMessageBox::Yes)` followed by `addButton(QMessageBox::No)` should be a single `setStandardButtons(QMessageBox::Yes | QMessageBox::No)`. The trailing `else { return; }` at line 496–498 is also redundant since the function ends.
- [SUGGESTION] In `src/qt/test/proposaltests.cpp`:1-21: Test leaks a CGovernanceObject, lacks license header, and depends on the m_currentDate bug being fixed
  Several problems:

1. Once the `paymentRemaining()` bug is fixed, the date arithmetic in the three QVERIFY cases is correct (38 days ≈ 1 cycle, 62 ≈ 2, 98 ≈ 3). Until then, the assertions cannot succeed.
2. `CGovernanceObject *pGovObj = new CGovernanceObject;` is leaked — `Proposal` does not own its `pGovObj`. Use stack allocation: `CGovernanceObject govObj; Proposal proposal(&govObj);`.
3. The file is missing the standard `Copyright (c) ... The Dash Core developers / Distributed under the MIT software license ...` header used by every other source file in this directory.
4. The default-constructed `CGovernanceObject` returns an empty data string, so the `UniValue::read` in the `Proposal` constructor fails silently and `m_paymentAmount`/`m_startDate`/`m_endDate`/`m_url` are left in their default (uninitialised / null) state. The test works only because it overwrites `m_endDate` via friend access; any future field that `paymentRemaining()` reads would silently use indeterminate data.
- [SUGGESTION] In `src/qt/governancelist.cpp`:64-72: GUI math is coupled to consensus parameters and hard-codes the cycle length
  Two coupling concerns:

1. `Params().GetConsensus().nPowTargetTimespan` is the difficulty-adjustment timespan; it is being used as a stand-in for 'seconds per day'. This works today only because all four chains set `nPowTargetTimespan = 24 * 60 * 60` in `chainparams.cpp`. It is a load-bearing coincidence; any future consensus change to the retargeting cadence would silently break this GUI math. Use a literal `24 * 60 * 60` (or a named constant local to this file).
2. `CYCLE_IN_DAYS = 30.29` hard-codes the mainnet superblock cycle (16616 blocks × 2.625 minutes ≈ 30.29 days). The actual cycle is `nSuperblockCycle * nPowTargetSpacing` per-chain, so the constant is incorrect on testnet/regtest. Derive it from consensus params, or at minimum document the derivation in a comment.

Also: `int remainingInDays = remainingInSecs / nPowTargetTimespan;` truncates to whole days before dividing by 30.29, throwing away up to ~1 day of resolution and biasing the rounded result. Compute the division in floating point throughout.
- [NITPICK] In `src/qt/governancelist.cpp`:208-236: TextAlignmentRole returns horizontal-only flags and the comment is wrong
  Returning a bare `Qt::AlignLeft` / `Qt::AlignCenter` produces horizontal-only alignment — cells default to top-aligned vertically, which looks odd next to centred numeric columns. The Qt convention is to OR a vertical flag, e.g. `int(Qt::AlignLeft | Qt::AlignVCenter)` and `int(Qt::AlignCenter)`.

The comment `// Edit role is used for sorting, so return the raw values where possible` at line 210 was copy-pasted from the `Qt::EditRole` block above and is incorrect in this `Qt::TextAlignmentRole` block — please remove or replace it. The returned value should be wrapped in `int(...)` per Qt model-code convention.
- [NITPICK] In `src/qt/test/proposaltests.h`:1-15: Header guard does not follow project convention
  Existing Qt test headers use the form `BITCOIN_QT_TEST_<NAME>_H` (e.g. `BITCOIN_QT_TEST_TRAFFICGRAPHDATATESTS_H`, `BITCOIN_QT_TEST_COMPATTESTS_H`). This file uses the unscoped `PROPOSALTESTS_H`, which risks collisions and breaks convention. The file also lacks the standard copyright header used by every other header in `src/qt/test/`.
- [SUGGESTION] In `src/qt/governancelist.h`:72-84: HASH/START_DATE/END_DATE columns removed without replacement
  The previous UI exposed `HASH`, `START_DATE`, and `END_DATE` columns. This PR removes all three. `PAYMENTS_REMAINING` partially substitutes for `END_DATE`, but users lose the ability to see exact start/end dates and the governance object hash (which they need for `gobject vote` RPCs from the console). Consider keeping `END_DATE` (or surfacing the date via a `Qt::ToolTipRole` on `PAYMENTS_REMAINING`) and keeping `HASH` accessible somewhere — e.g. in the right-click context menu. If the removal is intentional, document the rationale in the PR description.

Inline posting failed; posted verified findings in the top-level review body.