feat(platform-wallet): return the exact network fee from send_payment#4095
Conversation
The builder computed the fee for every DashPay payment and discarded it. Thread it through: send_payment returns (txid, entry, fee); platform_wallet_send_dashpay_payment gains a nullable out_fee_duffs; Swift sendDashPayPayment returns (txid, feeDuffs) so wallets can show the exact fee of the broadcast transaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
📝 WalkthroughWalkthroughDashPay payment sending now propagates the exact network fee from transaction construction through the Rust FFI and Swift SDK. The example app adapts to the tuple return while continuing to use the transaction ID. ChangesDashPay payment fee propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SwiftSDK as ManagedPlatformWallet
participant FFI as platform_wallet_send_dashpay_payment
participant Rust as DashPayView
participant Builder as TransactionBuilder
SwiftSDK->>FFI: Provide txid and fee output pointers
FFI->>Rust: Call send_payment
Rust->>Builder: Build signed transaction
Builder-->>Rust: Return transaction and fee
Rust-->>FFI: Return txid, entry, and fee
FFI-->>SwiftSDK: Populate txid and feeDuffs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
✅ Review complete (commit 5f4bb14) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (1)
539-542: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
# Returnsdoc to mention the network fee.The return type now includes
u64for the fee (line 555), but the# Returnsdoc still describes only theTxidandPaymentEntry. The Swift counterpart atManagedPlatformWallet.swift:2104-2105was updated; the Rust doc should be too.📝 Proposed doc update
/// # Returns /// -/// The `Txid` of the broadcast transaction and the newly created -/// [`PaymentEntry`] recording the outgoing payment. +/// The `Txid` of the broadcast transaction, the newly created +/// [`PaymentEntry`] recording the outgoing payment, and the +/// network fee in duffs from the broadcast transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs` around lines 539 - 542, Update the # Returns documentation for the payment-broadcasting function near PaymentEntry to describe all returned values, including the u64 network fee alongside the Txid and PaymentEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 539-542: Update the # Returns documentation for the
payment-broadcasting function near PaymentEntry to describe all returned values,
including the u64 network fee alongside the Txid and PaymentEntry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ff2754d6-85ee-44b5-8f7f-d32d5feafbb3
📒 Files selected for processing (4)
packages/rs-platform-wallet-ffi/src/dashpay.rspackages/rs-platform-wallet/src/wallet/identity/network/payments.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/SendDashPayPaymentSheet.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small plumbing change that threads an already-computed fee value from TransactionBuilder::build_signed through send_payment -> FFI out-param -> Swift tuple. The Swift/FFI marshalling is correct and safe (nullable out-param, exhaustive tuple matching, proper lifetime handling of the resolver signer). However, tracing the fee value back to its source in the vendored key-wallet crate (TransactionBuilder::build_signed, pinned rev 1ee1c94) shows it is a size*rate recomputation from the final signed tx, not the actual inputs-minus-outputs amount baked into the transaction during coin selection/change calculation — these two values provably diverge whenever a dust remainder is folded into the fee (no change output created) and can diverge more generally from encoded-size estimation differences. This directly undermines the PR's stated goal of exposing the "exact network fee."
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2): reviewers codex/general=gpt-5.6-sol(completed_parseable_output_with_nonzero_wrapper_exit); sonnet5/general=claude-sonnet-5(completed); verifier=verifier-sonnet5-4095-1783808164=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🔴 1 blocking | 💬 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 `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:668-675: Piped-through fee is a size-based recomputation, not the actual inputs-minus-outputs fee
I traced this through to `TransactionBuilder::build_signed` in the pinned `key-wallet` crate (rev `1ee1c94`, `key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`). The value now surfaced by this PR is `fee_rate.calculate_fee(encoded_size(&signed_tx))` — a fee-rate × final-serialized-size recomputation done *after* signing.
That is provably not the same quantity as the fee the broadcast transaction actually pays. During `assemble_unsigned`, the change output (if any) is sized as `total_input - total_output - selection.estimated_fee`, so the true on-chain fee (inputs minus outputs) always equals `selection.estimated_fee` — a value computed *before* signing from an estimated per-input size (148 bytes) and the coin-selector's dust-folding rules, not from the final signed size. The two numbers only match by coincidence:
- Dust case: when `change_amount` falls below the dust threshold, `accumulate_coins_with_size` folds the dust into `estimated_fee` (`total_value - target_amount`) so the true paid fee is inflated by up to 546 duffs. `build_signed`'s returned value is oblivious to this — it just does size × rate — so it undercounts the actual fee by exactly the folded-in dust amount whenever no change output is created.
- General case: the 148-byte/input estimate used for coin selection and the real encoded size of a signed P2PKH input (variable-length DER signatures) aren't guaranteed to match, so the two fee numbers can diverge even when a change output exists.
This PR's whole purpose is to let wallets show the *exact* fee instead of re-estimating after the fact — but the plumbed-through value inherits an estimate that's decoupled from what the change output / dust folding actually paid. Since `send_payment` previously discarded this value entirely (`_fee`), this inaccuracy had no consumer before; this PR is what turns it into an externally-relied-upon contract, which is why it's in scope here even though the root computation lives in the external `key-wallet` crate.
| let (tx, fee) = builder | ||
| .build_signed(signer, |addr| { | ||
| managed_account.address_derivation_path(&addr) | ||
| }) | ||
| .await | ||
| .map_err(|e| PlatformWalletError::TransactionBuild(e.to_string()))?; | ||
|
|
||
| (payment_address, tx) | ||
| (payment_address, tx, fee) |
There was a problem hiding this comment.
🔴 Blocking: Piped-through fee is a size-based recomputation, not the actual inputs-minus-outputs fee
I traced this through to TransactionBuilder::build_signed in the pinned key-wallet crate (rev 1ee1c94, key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs). The value now surfaced by this PR is fee_rate.calculate_fee(encoded_size(&signed_tx)) — a fee-rate × final-serialized-size recomputation done after signing.
That is provably not the same quantity as the fee the broadcast transaction actually pays. During assemble_unsigned, the change output (if any) is sized as total_input - total_output - selection.estimated_fee, so the true on-chain fee (inputs minus outputs) always equals selection.estimated_fee — a value computed before signing from an estimated per-input size (148 bytes) and the coin-selector's dust-folding rules, not from the final signed size. The two numbers only match by coincidence:
- Dust case: when
change_amountfalls below the dust threshold,accumulate_coins_with_sizefolds the dust intoestimated_fee(total_value - target_amount) so the true paid fee is inflated by up to 546 duffs.build_signed's returned value is oblivious to this — it just does size × rate — so it undercounts the actual fee by exactly the folded-in dust amount whenever no change output is created. - General case: the 148-byte/input estimate used for coin selection and the real encoded size of a signed P2PKH input (variable-length DER signatures) aren't guaranteed to match, so the two fee numbers can diverge even when a change output exists.
This PR's whole purpose is to let wallets show the exact fee instead of re-estimating after the fact — but the plumbed-through value inherits an estimate that's decoupled from what the change output / dust folding actually paid. Since send_payment previously discarded this value entirely (_fee), this inaccuracy had no consumer before; this PR is what turns it into an externally-relied-upon contract, which is why it's in scope here even though the root computation lives in the external key-wallet crate.
source: ['codex']
| // `PaymentEntry.memo` slot stays available for | ||
| // future local-note wiring. | ||
| let txid = try await wallet.sendDashPayPayment( | ||
| let (txid, _) = try await wallet.sendDashPayPayment( |
There was a problem hiding this comment.
💬 Nitpick: In-repo reference implementation discards the newly-exposed fee
This PR's stated motivation is that wallets could previously only re-estimate the fee after the fact. SendDashPayPaymentSheet is the in-tree DashPay send flow and the natural place to demonstrate the new capability, but it discards feeDuffs (let (txid, _) = try await wallet.sendDashPayPayment(...)) instead of surfacing it in the confirmation UI. Not a defect — the real consumer is dashwallet-ios — but the example app doesn't showcase the feature this PR adds.
source: ['claude']
…nd-example-app Conflict: packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (test region) — kept BOTH sides: the branch's DIP-15 used-flip persistence test (send_payment_persists_external_pool_used_flip, inline setup) and base #4049/#4095's AcceptingBroadcaster / fund_bip44_account_0 / register_sender_and_external_account helpers plus the two exact-fee tests. Production code auto-merged (used-flip persist block + exact-fee return both verified present). Adapted the JNI sendDashPayPayment wrapper to the new out_fee_duffs FFI out-param (captured; Kotlin still reads the fee from the persisted payment entry, as iOS does). Verified: cargo fmt, JNI check (shielded), workspace clippy --all-features -D warnings, the 3 payments tests (3 passed), gradle :sdk+:app unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Issue being fixed or feature implemented
DashPay payment callers (dashwallet-ios in particular) need to show the exact network fee of the broadcast transaction. The transaction builder already computes this fee for every DashPay payment, but
send_paymentdiscarded it, so wallets could only re-estimate it after the fact.This has been carried as a local patch on the dashwallet-ios platform pin branch and is flagged in that repo's DASHSYNC_MIGRATION.md as needing upstreaming — landing it here shrinks the next pin bump.
What was done?
Thread the fee the builder already computes through the full stack:
PlatformWallet::send_payment(rust): returns(txid, entry, fee)instead of dropping the fee.platform_wallet_send_dashpay_payment(FFI): gains a nullableout_fee_duffsout-param.ManagedPlatformWallet.sendDashPayPayment(Swift): returns(txid: Data, feeDuffs: UInt64).SendDashPayPaymentSheet(SwiftExampleApp): adjusted to the tuple return.How Has This Been Tested?
cargo check -p platform-wallet -p platform-wallet-ffi --all-targetspasses.cargo fmt --all --checkclean.DashSDKFFI.xcframeworkcontaining the updated header.Breaking Changes
None for consensus or the platform protocol. API-wise,
send_payment's return type and the FFI/Swift signatures change; the FFI out-param is nullable so existing C callers can pass NULL.Checklist:
For repository code-owners and collaborators only
🤖 Generated with Claude Code
Summary by CodeRabbit