fix(wallets): show the fee on outgoing transaction history - #900
Conversation
The Transaction History "Fee" column was already wired to `tx.fee`, but upstream always created discovered `TransactionRecord`s with `fee: None`, so the column rendered "-" unconditionally. For ordinary wallet-created outgoing transactions the record already carries every input and output value, so the exact fee is derived as sum(known inputs) - sum(outputs) — no new persistence needed. The helper preserves an already-recorded fee, requires complete and correctly indexed input details, uses checked sums and checked subtraction, and returns None on incomplete data, overflow, or underflow. Also generalize `estimate_platform_fee` to take an output count. The advanced Platform-to-Platform preview already computed that count but had no way to pass it, so multi-output transfers were underestimated; simple callers pass 1. Reapplied onto the post-#894 base, which independently landed a newer SND-005 pre-send estimate. That base version is kept: this change drops the older duplicate send-screen renderer and the fixed-action shielded-route preview, which would have misreported the fee because backend note selection determines the real action count. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
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 |
|
⛔ Blockers found — Sonnet deferred (commit ca8d708) |
24307e5
into
docs/platform-wallet-migration-design
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The outgoing transaction-history fee derivation is conservative and uses checked arithmetic, but its fail-closed behavior lacks regression coverage beyond the happy path. The Platform-to-Platform preview counts populated form rows instead of the distinct positive-credit outputs submitted to the backend, so valid duplicate or zero-credit rows produce an incorrect fee estimate.
Validated blockers were found in the Codex precheck. Sonnet 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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 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/ui/wallets/send_screen.rs`:
- [BLOCKING] src/ui/wallets/send_screen.rs:2823-2835: Count the Platform outputs actually submitted
This branch prices every row with a non-empty address, but `send_advanced_platform_to_platform` skips zero-credit rows and inserts outputs into a `BTreeMap`, which coalesces duplicate destinations. Two rows for the same Platform address therefore submit one transition output but are priced as two, and a populated zero-credit row is priced despite being omitted. Because the newly generalized fee calculation charges per output, derive the count from the same distinct, positive-credit destinations used by the send path.
In `src/wallet_backend/snapshot.rs`:
- [SUGGESTION] src/wallet_backend/snapshot.rs:882-916: Cover the fee derivation's fail-closed behavior
The new test covers only a single-input happy path. The helper's user-visible correctness also depends on preserving an existing upstream fee and returning `None` for incomplete or misindexed input metadata and when outputs exceed inputs. Add regression cases for those branches so a future simplification cannot display a fabricated transaction fee.
| Some(estimate_platform_fee( | ||
| &self.app_context.fee_estimator(), | ||
| num_inputs, | ||
| num_outputs, | ||
| )) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Count the Platform outputs actually submitted
This branch prices every row with a non-empty address, but send_advanced_platform_to_platform skips zero-credit rows and inserts outputs into a BTreeMap, which coalesces duplicate destinations. Two rows for the same Platform address therefore submit one transition output but are priced as two, and a populated zero-credit row is priced despite being omitted. Because the newly generalized fee calculation charges per output, derive the count from the same distinct, positive-credit destinations used by the send path.
| Some(estimate_platform_fee( | |
| &self.app_context.fee_estimator(), | |
| num_inputs, | |
| num_outputs, | |
| )) | |
| } | |
| AdvancedSourceType::Platform if has_platform_out && !has_core_out => { | |
| let num_inputs = self | |
| .platform_inputs | |
| .iter() | |
| .filter(|i| !i.amount.trim().is_empty()) | |
| .count() | |
| .max(1); | |
| let num_outputs = self | |
| .advanced_outputs | |
| .iter() | |
| .filter_map(|output| { | |
| let destination = | |
| PlatformAddress::from_bech32m_string(output.address.trim()).ok()?; | |
| let credits = Self::parse_amount_to_credits(&output.amount).ok()?; | |
| (credits > 0).then_some(destination) | |
| }) | |
| .collect::<std::collections::BTreeSet<_>>() | |
| .len(); | |
| if num_outputs == 0 { | |
| return None; | |
| } | |
| Some(estimate_platform_fee( | |
| &self.app_context.fee_estimator(), | |
| num_inputs, | |
| num_outputs, | |
| )) | |
| } |
source: ['codex']
| fn outgoing_transaction_fee_is_derived_from_known_inputs() { | ||
| use dash_sdk::dpp::dashcore::TxIn; | ||
|
|
||
| let source = addr(10); | ||
| let destination = addr(11); | ||
| let mut tx = tx_with(10); | ||
| tx.input.push(TxIn::default()); | ||
| tx.output.push(TxOut { | ||
| value: 9_700, | ||
| script_pubkey: destination.script_pubkey(), | ||
| }); | ||
| let record = TransactionRecord::new( | ||
| tx, | ||
| AccountType::Standard { | ||
| index: 0, | ||
| standard_account_type: StandardAccountType::BIP44Account, | ||
| }, | ||
| TransactionContext::Mempool, | ||
| TransactionType::Standard, | ||
| TransactionDirection::Outgoing, | ||
| vec![InputDetail { | ||
| index: 0, | ||
| value: 10_000, | ||
| address: source, | ||
| }], | ||
| vec![OutputDetail { | ||
| index: 0, | ||
| role: OutputRole::Sent, | ||
| address: Some(destination), | ||
| value: 9_700, | ||
| }], | ||
| -10_000, | ||
| ); | ||
|
|
||
| assert_eq!(map_transaction_record(&record).fee, Some(300)); |
There was a problem hiding this comment.
🟡 Suggestion: Cover the fee derivation's fail-closed behavior
The new test covers only a single-input happy path. The helper's user-visible correctness also depends on preserving an existing upstream fee and returning None for incomplete or misindexed input metadata and when outputs exceed inputs. Add regression cases for those branches so a future simplification cannot display a fabricated transaction fee.
source: ['codex']
2 participants
Why this PR exists
Problem: The Transaction History "Fee" column renders
-for every transaction, always. The column is correctly wired totx.fee, but discoveredTransactionRecords are always built withfee: None, so there is never a value to show. Separately,estimate_platform_feetakes no output count, so the advanced Platform→Platform preview underestimates multi-output transfers.What breaks without it:
-. It shows-for every row, forever — a user cannot see what any transaction cost them, and the column looks broken rather than empty-by-design.estimate_platform_fee, which prices a single output. A transfer with several outputs is quoted below its real cost.Blocking relationship: None — targets feat: rewrite Dash Evo Tool onto the new platform-wallet #860's branch. This commit was orphaned: it sat on an abandoned branch while fix: rebase SND-003 onto PR893 + QA-campaign fix batches #894 was squash-merged, and the squash absorbed everything around it but not this. It looked merged because the base does contain an SND-005 commit — but that one is the pre-send fee estimate, a different defect. The history-column fix was never merged.
What was done
wallet_backend/snapshot.rs).transaction_fee(record)computessum(known inputs) - sum(outputs)for ordinary wallet-created outgoing transactions, which already carry every input and output value — so no new persistence is needed. It preserves an already-recorded fee, requires complete and correctly indexed input details, uses checked sums and checked subtraction, and returnsNoneon incomplete data, overflow, or underflow, leaving the column at-rather than showing a wrong number.estimate_platform_feeto take an output count (model/fee_estimation.rs). Simple callers pass1; the advanced Platform→Platform path passes the count it already had.Reapplied onto the post-#894 base, which independently landed a newer SND-005 pre-send estimate. That newer base version is kept. This change drops the orphaned commit's older duplicate send-screen renderer, and its fixed-action shielded-route preview — the latter deliberately, because a fixed two-action estimate would misreport the fee when backend note selection determines the real action count.
Testing
outgoing_transaction_fee_is_derived_from_known_inputs— new regression test; asserts the derived fee isSome(300). Confirmed by name in the run log, not inferred from a green exit.platform_fee_accounts_for_every_outputcovering the output-count fix.cargo clippy --all-features --all-targets -- -D warnings— clean, which is also the gate proving no dead code was left behind by the dropped shielded-route path.cargo +nightly fmt --all— clean.Net diff is 3 files, +91/−8.
Breaking changes
None. The fee column populates where it previously showed
-; a transaction whose inputs are not fully known still shows-.estimate_platform_fee's new output-count argument is internal.🤖 Co-authored by Claudius the Magnificent AI Agent