fix: trim trailing zeros in proposal amounts - #7644
Conversation
|
✅ Final review complete — no blockers (commit dd7fba2) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25da1b5b5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| namespace { | ||
| QString formatProposalAmount(const BitcoinUnit& unit, const CAmount& amount) | ||
| { | ||
| QString result = BitcoinUnits::format(unit, amount, false, BitcoinUnits::SeparatorStyle::ALWAYS); |
There was a problem hiding this comment.
When a user selects 2–7 “Decimal digits” in the display settings, this call bypasses floorWithUnit, which reads that setting, and always emits the unit's full precision before removing zeros. For example, a proposal for 15.2714286 DASH now displays all seven fractional digits even when the user configured two; apply the configured precision first and then trim its insignificant trailing zeros so this focused change does not also disable the existing preference.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in dd7fba2. formatProposalAmount() now starts from BitcoinUnits::floorWithUnit() (which reads the QSettings digits preference, default 2), strips the appended unit suffix, trims the insignificant trailing zeros and any bare trailing ., then re-appends the suffix. With the default setting 15.2714286 DASH renders as 15.27 DASH again, while 1.00000000 DASH still collapses to 1 DASH.
🤖 Posted autonomously by Claude on behalf of pasta.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe proposal model adds Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This change only trims insignificant trailing zeros from displayed proposal amounts while preserving configured precision and underlying integer values; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The trailing-zero cleanup preserves the underlying integer model values and sorting, but the new formatter bypasses the configured decimal-digit setting. Because the Governance amount column previously honored that user preference, this introduces an in-scope display regression that should be fixed before merge.
Source: reviewer backend gpt-5.6-sol (general and dash-core-commit-history); final verifier backend gpt-5.6-sol(high); orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
🤖 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.
In `src/qt/proposalmodel.cpp`:
- [BLOCKING] src/qt/proposalmodel.cpp:25: Preserve the configured decimal precision
`BitcoinUnits::format()` always emits the unit's full precision, while the replaced `floorWithUnit()` call reads the QSettings `digits` preference, whose default is two. Consequently, an amount such as `15.2714286 DASH` now displays all seven fractional digits even when the user selected two, changing the established behavior of the Governance amount column. Start with the configured-precision representation, remove its unit suffix, and then let the existing code trim insignificant zeros.
| namespace { | ||
| QString formatProposalAmount(const BitcoinUnit& unit, const CAmount& amount) | ||
| { | ||
| QString result = BitcoinUnits::format(unit, amount, false, BitcoinUnits::SeparatorStyle::ALWAYS); |
There was a problem hiding this comment.
🔴 Blocking: Preserve the configured decimal precision
BitcoinUnits::format() always emits the unit's full precision, while the replaced floorWithUnit() call reads the QSettings digits preference, whose default is two. Consequently, an amount such as 15.2714286 DASH now displays all seven fractional digits even when the user selected two, changing the established behavior of the Governance amount column. Start with the configured-precision representation, remove its unit suffix, and then let the existing code trim insignificant zeros.
| QString result = BitcoinUnits::format(unit, amount, false, BitcoinUnits::SeparatorStyle::ALWAYS); | |
| QString result = BitcoinUnits::floorWithUnit(unit, amount, false, BitcoinUnits::SeparatorStyle::ALWAYS); | |
| result.chop(BitcoinUnits::name(unit).size() + 1); |
source: ['codex']
There was a problem hiding this comment.
Confirmed and fixed in dd7fba2. formatProposalAmount() now starts from BitcoinUnits::floorWithUnit() (which reads the QSettings digits preference, default 2), strips the appended unit suffix, trims the insignificant trailing zeros and any bare trailing ., then re-appends the suffix. With the default setting 15.2714286 DASH renders as 15.27 DASH again, while 1.00000000 DASH still collapses to 1 DASH.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Preserve the configured decimal precision no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Governance proposal amounts were padded to the display unit's full precision ("1.00000000 DASH"), which adds noise without conveying information.
Format at the user's configured decimal precision, as before, then drop only insignificant trailing zeros and a bare trailing decimal point.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
25da1b5 to
dd7fba2
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — GLM Flash + Sol
The exact-head implementation fixes the prior precision regression by formatting through the QSettings-backed floorWithUnit() before removing insignificant zeros, while the edit role continues to expose the raw integer amount for sorting. No correctness defect remains, but the newly introduced formatting behavior still lacks automated regression coverage despite the earlier precision bug demonstrating its value.
Source: reviewer backends glm-5.3-flash, gpt-5.6-sol, and Claude Opus 5 (1M context); final verifier backend Claude Opus 5 (1M context).
Review provenance
- Phase 1 reviewers (GLM Flash):
glm-5.3-flash— general (completed),glm-5.3-flash— dash-core-commit-history (completed) - Fresh verifier (Sol):
gpt-5.6-sol— final-verifier - Phase 2 reviewers (Sol):
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 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.
In `src/qt/proposalmodel.cpp`:
- [SUGGESTION] src/qt/proposalmodel.cpp:24-40: Add automated coverage for proposal amount formatting
The new helper implements the PR's complete behavior—configured precision, suffix removal, trailing-zero trimming, and bare-decimal-point removal—but no automated test exercises those invariants. The initial revision already bypassed the configured `digits` preference and required a correction, so a regression test would protect behavior that has demonstrably been easy to break. Add focused Qt coverage that sets the `digits` preference and verifies whole and partially fractional amounts, retained nonzero digits, negative and grouped amounts, and a unit whose native precision is lower than the configured value.
| //! insignificant trailing zeros ("1.00 DASH" -> "1 DASH"). | ||
| QString formatProposalAmount(const BitcoinUnit& unit, const CAmount& amount) | ||
| { | ||
| const QString suffix{QLatin1Char(' ') + BitcoinUnits::name(unit)}; | ||
| QString result{BitcoinUnits::floorWithUnit(unit, amount, /*plussign=*/false, BitcoinUnits::SeparatorStyle::ALWAYS)}; | ||
| result.chop(suffix.size()); | ||
|
|
||
| if (result.contains('.')) { | ||
| while (result.endsWith('0')) { | ||
| result.chop(1); | ||
| } | ||
|
|
||
| if (result.endsWith('.')) { | ||
| result.chop(1); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: Add automated coverage for proposal amount formatting
The new helper implements the PR's complete behavior—configured precision, suffix removal, trailing-zero trimming, and bare-decimal-point removal—but no automated test exercises those invariants. The initial revision already bypassed the configured digits preference and required a correction, so a regression test would protect behavior that has demonstrably been easy to break. Add focused Qt coverage that sets the digits preference and verifies whole and partially fractional amounts, retained nonzero digits, negative and grouped amounts, and a unit whose native precision is lower than the configured value.
source: ['claude', 'codex']
2 participants
Issue being fixed or feature implemented
Governance proposal amounts were displayed with fixed-width trailing zeros (for example,
1.00000000 DASH). This adds unnecessary visual noise to the proposal list and makes scanning amounts harder.What was done?
The Governance payment-amount column now formats the amount at the user's configured decimal precision (the
digitsdisplay option, default 2 — the same precision the column used before), then removes only insignificant trailing zeros and any bare trailing decimal point.1.00000000 DASHrenders as1 DASH1.10 DASHrenders as1.1 DASH15.27 DASHis unchangedNo amount value is rounded beyond what the display-precision setting already did, and the model's sort/underlying values still use the integer amount.
How Has This Been Tested?
dash-qtsuccessfully on macOS ARM64.git diff --check.Breaking Changes
None. This is a GUI display-only change; sorting and model values continue to use the integer amount.
Checklist:
This pull request was created by Codex.