fix(ui): replace f64 precision-lossy max amount calculation with integer arithmetic#622
fix(ui): replace f64 precision-lossy max amount calculation with integer arithmetic#622thepastaclaw wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughHardcoded fee constants used for max-amount calculations in the withdraw and transfer screens, and for button-enable guards in the identities screen, are replaced with dynamic calls to ChangesDynamic Fee Estimation for Identities UI
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean, correct fix. Replaces lossy f64 round-trips with exact integer arithmetic using saturating_sub. Fee constants (20M for transfer, 500M for withdrawal) are consistent with existing code. Handles underflow edge case correctly. Both agents confirm no issues.
Reviewed commit: a5ed125
|
✅ Review complete (commit 57a1d07) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's core change is correct: replacing the lossy u64→f64→u64 round-trip with saturating_sub fixes a real precision bug while preserving the prior .max(0.0) underflow-to-zero behavior. Verifier confirmed all flagged code locations exist. The notable design concern is that the Max-button reservation now hardcodes integer literals (20_000_000 / 500_000_000) while the same screens already use app_context.fee_estimator() for the displayed estimated fee, creating two divergent sources of truth for the same fee. Test coverage for the boundary values that motivated the fix is also missing.
Reviewed commit: a5ed125
🟡 3 suggestion(s) | 💬 1 nitpick(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/ui/identities/transfer_screen.rs`:
- [SUGGESTION] lines 137-138: Max-button fee reservation should use the fee estimator, not a hardcoded literal
The same screen already uses `self.app_context.fee_estimator()` to compute the displayed estimated fee at lines 681-688 (`estimate_credit_transfer()` for identity destinations, `estimate_credit_transfer_to_addresses(1)` for platform addresses). The Max button at line 138 instead reserves a literal `20_000_000` credits, so the two values can drift apart whenever the cached epoch fee multiplier changes or the destination type matters (platform-address transfers are estimated higher than identity transfers). Routing the Max calculation through the same estimator preserves a single authoritative fee path. This was previously masked by the f64 round-trip; the precision fix makes the divergence cleaner to address.
- [SUGGESTION] line 138: No regression test pins the boundary behavior the PR fixes
The bug being fixed — f64 round-trip producing ~1000 credits where 1 was correct — is exactly the kind of off-by-many-credits arithmetic that benefits from a small unit test pinning expected outputs at boundary balances: `0`, `< reserve`, `== reserve`, `reserve + 1`, and a large value. As-is, the egui rendering wraps the calculation, so a small extracted helper (e.g. `fn max_amount_after_fee(balance: u64, fee: u64) -> u64`) would make this trivially testable and prevent a future refactor from silently re-introducing the same precision loss. Verified that no existing test under the workspace exercises `TransferScreen` / `WithdrawalScreen` Max-amount behavior.
In `src/ui/identities/withdraw_screen.rs`:
- [SUGGESTION] lines 110-111: Withdrawal Max-button reservation should use the fee estimator
The same screen displays the estimated withdrawal fee via `self.app_context.fee_estimator().estimate_credit_withdrawal()` (line 543), but the Max button at line 111 reserves a separate literal `500_000_000` credits. This splits one business rule across two sources of truth: the literal won't reflect any future change in `min_fees.credit_withdrawal` or in the cached fee multiplier. Deriving the reservation from the estimator preserves a single authoritative calculation path.
| // 0.0002 DASH = 20_000_000 credits (1 DASH = 100_000_000_000 credits) | ||
| let max_amount_credits = self.max_amount.saturating_sub(20_000_000); |
There was a problem hiding this comment.
🟡 Suggestion: Max-button fee reservation should use the fee estimator, not a hardcoded literal
The same screen already uses self.app_context.fee_estimator() to compute the displayed estimated fee at lines 681-688 (estimate_credit_transfer() for identity destinations, estimate_credit_transfer_to_addresses(1) for platform addresses). The Max button at line 138 instead reserves a literal 20_000_000 credits, so the two values can drift apart whenever the cached epoch fee multiplier changes or the destination type matters (platform-address transfers are estimated higher than identity transfers). Routing the Max calculation through the same estimator preserves a single authoritative fee path. This was previously masked by the f64 round-trip; the precision fix makes the divergence cleaner to address.
💡 Suggested change
| // 0.0002 DASH = 20_000_000 credits (1 DASH = 100_000_000_000 credits) | |
| let max_amount_credits = self.max_amount.saturating_sub(20_000_000); | |
| // Reserve fee via the estimator so the Max button stays in sync with the displayed fee. | |
| let fee_estimator = self.app_context.fee_estimator(); | |
| let reserved_fee = match self.destination_type { | |
| TransferDestinationType::Identity => fee_estimator.estimate_credit_transfer(), | |
| TransferDestinationType::PlatformAddress => { | |
| fee_estimator.estimate_credit_transfer_to_addresses(1) | |
| } | |
| }; | |
| let max_amount_credits = self.max_amount.saturating_sub(reserved_fee); |
source: ['codex']
🤖 Fix this 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/ui/identities/transfer_screen.rs`:
- [SUGGESTION] lines 137-138: Max-button fee reservation should use the fee estimator, not a hardcoded literal
The same screen already uses `self.app_context.fee_estimator()` to compute the displayed estimated fee at lines 681-688 (`estimate_credit_transfer()` for identity destinations, `estimate_credit_transfer_to_addresses(1)` for platform addresses). The Max button at line 138 instead reserves a literal `20_000_000` credits, so the two values can drift apart whenever the cached epoch fee multiplier changes or the destination type matters (platform-address transfers are estimated higher than identity transfers). Routing the Max calculation through the same estimator preserves a single authoritative fee path. This was previously masked by the f64 round-trip; the precision fix makes the divergence cleaner to address.
| // 0.005 DASH = 500_000_000 credits (1 DASH = 100_000_000_000 credits) | ||
| let max_amount_credits = self.max_amount.saturating_sub(500_000_000); |
There was a problem hiding this comment.
🟡 Suggestion: Withdrawal Max-button reservation should use the fee estimator
The same screen displays the estimated withdrawal fee via self.app_context.fee_estimator().estimate_credit_withdrawal() (line 543), but the Max button at line 111 reserves a separate literal 500_000_000 credits. This splits one business rule across two sources of truth: the literal won't reflect any future change in min_fees.credit_withdrawal or in the cached fee multiplier. Deriving the reservation from the estimator preserves a single authoritative calculation path.
💡 Suggested change
| // 0.005 DASH = 500_000_000 credits (1 DASH = 100_000_000_000 credits) | |
| let max_amount_credits = self.max_amount.saturating_sub(500_000_000); | |
| let reserved_fee = self.app_context.fee_estimator().estimate_credit_withdrawal(); | |
| let max_amount_credits = self.max_amount.saturating_sub(reserved_fee); |
source: ['codex']
🤖 Fix this 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/ui/identities/withdraw_screen.rs`:
- [SUGGESTION] lines 110-111: Withdrawal Max-button reservation should use the fee estimator
The same screen displays the estimated withdrawal fee via `self.app_context.fee_estimator().estimate_credit_withdrawal()` (line 543), but the Max button at line 111 reserves a separate literal `500_000_000` credits. This splits one business rule across two sources of truth: the literal won't reflect any future change in `min_fees.credit_withdrawal` or in the cached fee multiplier. Deriving the reservation from the estimator preserves a single authoritative calculation path.
| let max_amount_minus_fee = (self.max_amount as f64 / 100_000_000_000.0 - 0.0002).max(0.0); | ||
| let max_amount_credits = (max_amount_minus_fee * 100_000_000_000.0) as u64; | ||
| // 0.0002 DASH = 20_000_000 credits (1 DASH = 100_000_000_000 credits) | ||
| let max_amount_credits = self.max_amount.saturating_sub(20_000_000); |
There was a problem hiding this comment.
🟡 Suggestion: No regression test pins the boundary behavior the PR fixes
The bug being fixed — f64 round-trip producing ~1000 credits where 1 was correct — is exactly the kind of off-by-many-credits arithmetic that benefits from a small unit test pinning expected outputs at boundary balances: 0, < reserve, == reserve, reserve + 1, and a large value. As-is, the egui rendering wraps the calculation, so a small extracted helper (e.g. fn max_amount_after_fee(balance: u64, fee: u64) -> u64) would make this trivially testable and prevent a future refactor from silently re-introducing the same precision loss. Verified that no existing test under the workspace exercises TransferScreen / WithdrawalScreen Max-amount behavior.
source: ['claude', 'codex']
🤖 Fix this 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/ui/identities/transfer_screen.rs`:
- [SUGGESTION] line 138: No regression test pins the boundary behavior the PR fixes
The bug being fixed — f64 round-trip producing ~1000 credits where 1 was correct — is exactly the kind of off-by-many-credits arithmetic that benefits from a small unit test pinning expected outputs at boundary balances: `0`, `< reserve`, `== reserve`, `reserve + 1`, and a large value. As-is, the egui rendering wraps the calculation, so a small extracted helper (e.g. `fn max_amount_after_fee(balance: u64, fee: u64) -> u64`) would make this trivially testable and prevent a future refactor from silently re-introducing the same precision loss. Verified that no existing test under the workspace exercises `TransferScreen` / `WithdrawalScreen` Max-amount behavior.
| // 0.0002 DASH = 20_000_000 credits (1 DASH = 100_000_000_000 credits) | ||
| let max_amount_credits = self.max_amount.saturating_sub(20_000_000); |
There was a problem hiding this comment.
💬 Nitpick: Fee-floor magic numbers duplicated across three files
Verified: 20_000_000 appears in transfer_screen.rs:138 and identities_screen.rs:658; 500_000_000 appears in withdraw_screen.rs:111 and identities_screen.rs:622. Each site re-derives the conversion in a comment (e.g. 0.0002 DASH = 20_000_000 credits). CREDITS_PER_DASH: u64 = 100_000_000_000 already exists in model/fee_estimation.rs:636. If the fee-estimator suggestions above are not adopted, at minimum hoist these into named constants in a shared module so the three call sites cannot silently drift. The Max-button reservation and the identities_screen.rs insufficient-balance gate encode the same fee assumption and must remain in sync.
source: ['claude']
a5ed125 to
12b9670
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's precision fix (replacing the u64→f64→u64 round-trip with saturating_sub via max_amount_after_fee) is correct and well-tested. Both Max-button paths in transfer_screen.rs and withdraw_screen.rs now correctly derive reserved fees from app_context.fee_estimator(). Two convergent suggestions remain: the action-menu balance gates in identities_screen.rs at lines 622 and 658 still use hardcoded literals (500_000_000 and 20_000_000), diverging from the estimator-based path the opened screens now use, so the entry gate can disagree with the screen behavior under a non-unit fee multiplier.
Reviewed commit: 12b9670
🟡 2 suggestion(s)
2 additional findings
🟡 suggestion: Withdraw action-menu gate still uses a hardcoded `500_000_000` literal; diverges from the estimator-driven withdrawal screen
src/ui/identities/identities_screen.rs (lines 621-629)
WithdrawalScreen::render_amount_input (withdraw_screen.rs:109-114) now reserves self.app_context.fee_estimator().estimate_credit_withdrawal(), which applies the cached fee_multiplier_permille. The action-menu gate here still uses a fixed 500_000_000 credit threshold with strict >. Two failure modes follow:
- Under
fee_multiplier_permille < 1000(or whenever the epoch base fee is below 0.005 DASH), identities with balances between the real estimate and500_000_000are blocked from opening the withdrawal flow even though the screen itself would let them act. - Under
fee_multiplier_permille > 1000, a user withbalance > 500_000_000but< estimated_feepasses the gate and lands on the screen withmax_amount_after_fee == 0.
Route this gate through the same fee_estimator().estimate_credit_withdrawal() call so the affordance matches the screen's actual policy.
💡 Suggested change
let min_withdrawal_balance = self
.app_context
.fee_estimator()
.estimate_credit_withdrawal();
let can_withdraw = qualified_identity.identity.balance() > min_withdrawal_balance;
let withdraw_hover = if can_withdraw {
"Withdraw credits from this identity to a Dash Core address".to_string()
} else {
format!(
"Insufficient balance for withdrawal (need at least {} for fees)",
crate::model::fee_estimation::format_credits_as_dash(min_withdrawal_balance)
)
};
🟡 suggestion: Transfer action-menu gate still uses a hardcoded `20_000_000` literal; diverges from the estimator-driven transfer screen
src/ui/identities/identities_screen.rs (lines 657-665)
TransferScreen::render_amount_input (transfer_screen.rs:136-143) now computes its reserved fee from fee_estimator.estimate_credit_transfer() for identity destinations or estimate_credit_transfer_to_addresses(1) for platform-address destinations. The action-menu gate here still requires a fixed 20_000_000 credits with strict >. That value sits above both estimator outputs at unit multiplier (identity transfer ≈ 100,000 credits; platform-address ≈ 6,500,000 credits at 1x), so identities with enough balance to actually transfer are blocked from opening the screen. Under a fee multiplier high enough to push the estimate above 20_000_000, the inverse failure also occurs: the gate enables but max_amount_after_fee collapses to 0. Reuse the same estimator call so the entry gate stays consistent with the screen it opens.
💡 Suggested change
let min_transfer_balance = self
.app_context
.fee_estimator()
.estimate_credit_transfer();
let can_transfer = qualified_identity.identity.balance() > min_transfer_balance;
let transfer_hover = if can_transfer {
"Transfer credits from this identity to another identity".to_string()
} else {
format!(
"Insufficient balance for transfer (need at least {} for fees)",
crate::model::fee_estimation::format_credits_as_dash(min_transfer_balance)
)
};
🤖 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/ui/identities/identities_screen.rs`:
- [SUGGESTION] lines 621-629: Withdraw action-menu gate still uses a hardcoded `500_000_000` literal; diverges from the estimator-driven withdrawal screen
`WithdrawalScreen::render_amount_input` (withdraw_screen.rs:109-114) now reserves `self.app_context.fee_estimator().estimate_credit_withdrawal()`, which applies the cached `fee_multiplier_permille`. The action-menu gate here still uses a fixed `500_000_000` credit threshold with strict `>`. Two failure modes follow:
1. Under `fee_multiplier_permille < 1000` (or whenever the epoch base fee is below 0.005 DASH), identities with balances between the real estimate and `500_000_000` are blocked from opening the withdrawal flow even though the screen itself would let them act.
2. Under `fee_multiplier_permille > 1000`, a user with `balance > 500_000_000` but `< estimated_fee` passes the gate and lands on the screen with `max_amount_after_fee == 0`.
Route this gate through the same `fee_estimator().estimate_credit_withdrawal()` call so the affordance matches the screen's actual policy.
- [SUGGESTION] lines 657-665: Transfer action-menu gate still uses a hardcoded `20_000_000` literal; diverges from the estimator-driven transfer screen
`TransferScreen::render_amount_input` (transfer_screen.rs:136-143) now computes its reserved fee from `fee_estimator.estimate_credit_transfer()` for identity destinations or `estimate_credit_transfer_to_addresses(1)` for platform-address destinations. The action-menu gate here still requires a fixed `20_000_000` credits with strict `>`. That value sits above both estimator outputs at unit multiplier (identity transfer ≈ 100,000 credits; platform-address ≈ 6,500,000 credits at 1x), so identities with enough balance to actually transfer are blocked from opening the screen. Under a fee multiplier high enough to push the estimate above `20_000_000`, the inverse failure also occurs: the gate enables but `max_amount_after_fee` collapses to 0. Reuse the same estimator call so the entry gate stays consistent with the screen it opens.
… calculations Replace floating-point round-trip (u64→f64→u64) with integer saturating_sub for fee reservation in Max button calculations. This eliminates precision loss that could cause incorrect max amounts in transfer and withdraw screens. - transfer_screen: 0.0002 DASH = 20_000_000 credits - withdraw_screen: 0.005 DASH = 500_000_000 credits
12b9670 to
57a1d07
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The PR's core fix (replacing the lossy u64→f64→u64 round-trip with integer saturating_sub via the extracted max_amount_after_fee helper) is correct, well-tested across boundary cases, and addresses all prior-review findings. Both Max-button reservation paths and both action-menu balance gates now derive their thresholds from app_context.fee_estimator(), eliminating the previously duplicated 20_000_000 and 500_000_000 literals. One residual low-confidence asymmetry remains: the transfer action-menu gate uses the identity-destination fee, but the opened screen reserves the (larger) addresses fee when PlatformAddress is selected.
Reviewed commit: 57a1d07
💬 1 nitpick(s)
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReviews resumed. |
|
Superseded by #927, which now carries this fix in full (transfer/withdraw "Max" amount calc — 🤖 Co-authored by Claudius the Magnificent AI Agent |
…nners (#927) * fix: centralize validation and fee hygiene - PROJ-001: enforce shared model validators in UI and backend paths - PROJ-002: reserve transfer and withdrawal fees with integer estimator results - PROJ-003: keep technical errors in banner details - PROJ-005: use named placeholders and complete translation units in scope - PROJ-006: route local identity removal through IdentityTask - RUST-001: replace fragile offsets and annotate upstream string matching Co-Authored-By: Codex Sol <noreply@openai.com> * fix(wallets): validate HD alias before writing the seed envelope register_wallet ran validate_wallet_alias inside write_wallet_meta, which fires AFTER write_seed_envelope. An overlong HD alias therefore failed the meta write only after the encrypted seed was already resident in the vault, leaving an orphaned seed with no meta row (never hydrated, no cleanup path). Hoist the alias validation to the top of register_wallet, before any secret-critical write, mirroring the single-key import path. Adds a regression test asserting no raw-seed entry survives a rejected HD registration (RED against the buggy write order). Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(tokens): name the duplicated language, not the plural form The duplicate-token-name check keys on name_with_language.2 (the TokenNameLanguage) but the error message interpolated name_with_language.1 (the plural-name String), so users saw a nonsensical value and could not tell which language was duplicated. Interpolate .2 to match the check and the sibling name-length messages. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(tools): surface concrete parse errors in the Transition Visualizer The Transition Visualizer is a Platform-Developer tool whose purpose is diagnosing why a state transition fails to parse. The unified error-message policy (aimed at the Everyday-User banner flows) had genericized these messages and routed the concrete error only to tracing::debug! (off by default) — and the inline parse_error label has no details panel, so the diagnostic was unreachable in the UI at default verbosity. Append the concrete error to each inline message (byte-list, hex/base64 decode, state-transition deserialize, JSON serialize) so the developer sees the actual failure without restarting under RUST_LOG=debug. GroveSTARK already preserves its error via .with_details() and is left unchanged. Co-Authored-By: Claude Opus <noreply@anthropic.com> * perf(identity): look up one identity directly in remove_identity remove_identity loaded and deserialized every local qualified identity just to find the one being removed and read its associated voter id. Use the direct get_local_qualified_identity(&id) lookup instead — same extraction, no full-collection scan. Co-Authored-By: Claude Opus <noreply@anthropic.com> * fix(ui): use fee estimator for identity-list withdraw/transfer thresholds Ports the remaining piece of #622 (fix/integer-max-amount by thepastaclaw) that this branch's PROJ-002 hadn't covered yet: the Withdraw/Transfer button enable/disable thresholds in the identities list's action popup still used hardcoded f64-derived credit constants. Switches them to self.app_context.fee_estimator(), matching the pattern already used in transfer_screen.rs and withdraw_screen.rs on this branch. Also drops the amount from the disabled-button tooltip text since it no longer matches a single hardcoded figure. With this, PR #927 fully supersedes #622, which is being closed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Codex Sol <noreply@openai.com> Co-authored-by: Claude Opus <noreply@anthropic.com>
4 participants
Replace floating-point round-trip (
u64→f64→u64) with integersaturating_subfor fee reservation in Max button calculations on transfer and withdraw screens.Problem
The original code converted credits to DASH as
f64, subtracted a fee constant, then converted back tou64:This loses precision in the floating-point round-trip. For example, a balance of exactly
20_000_001credits would produce0.00000000020000001in f64, subtract0.0002, multiply back — and the result may differ from the mathematically correct1due to IEEE 754 representation.Fix
Use integer arithmetic directly on the credit value:
Cherry-picked from
ralph/improvements(commit 1909415).Validation
What was tested:
cargo clippy --all-features --all-targets -- -D warnings— lint checkcargo test --all-features --workspace— full workspace test suiteResults:
ClippyCI check — pass (5m45s)Test SuiteCI check — pass (8m27s)Environment: Local macOS arm64; GitHub Actions CI (ubuntu-latest)
Summary by CodeRabbit
Bug Fixes
Tests