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
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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']
thepastaclaw
force-pushed
the
fix/integer-max-amount
branch
from
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
thepastaclaw
force-pushed
the
fix/integer-max-amount
branch
from
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)
| let min_transfer_balance = self | ||
| .app_context | ||
| .fee_estimator() | ||
| .estimate_credit_transfer(); | ||
| let can_transfer = qualified_identity.identity.balance() > min_transfer_balance; |
There was a problem hiding this comment.
💬 Nitpick: Transfer-action gate fee diverges from PlatformAddress reservation in the opened screen
The action-menu gate at lines 659-663 uses fee_estimator().estimate_credit_transfer() (identity-destination fee). The opened TransferScreen (transfer_screen.rs:136-143) dispatches on destination_type and, for PlatformAddress, reserves estimate_credit_transfer_to_addresses(1), which is typically larger than credit_transfer (it adds address_funds_transfer_output_cost). An identity with balance in the band [estimate_credit_transfer, estimate_credit_transfer_to_addresses(1)) passes the gate, opens the screen, and if the user picks PlatformAddress, Max collapses to 0 with no UI cue why. Two viable fixes: (a) gate on the larger of the two estimates so the entry affordance can't promise more than it delivers, or (b) reflect the actual reserved fee in the screen's status when Max would be 0 for the chosen destination type. Confidence reduced because the divergent band is small in absolute terms and may not be reachable with typical balances.
source: ['claude']
3 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)