fix(platform-wallet): report the exact send_payment fee (inputs − outputs)#4049
fix(platform-wallet): report the exact send_payment fee (inputs − outputs)#4049QuantumExplorer wants to merge 5 commits into
Conversation
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
✅ Review complete (commit d45de7f) |
📝 WalkthroughWalkthroughAdds a broadcast transaction fee (duffs) return value to the DashPay payment send path, propagated from the Rust wallet's ChangesDashPay fee propagation
TestnetFaucet public API
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SendDashPayPaymentSheet
participant ManagedPlatformWallet
participant FFI
participant RustWallet
SendDashPayPaymentSheet->>ManagedPlatformWallet: sendDashPayPayment(...)
ManagedPlatformWallet->>FFI: platform_wallet_send_dashpay_payment(out_txid, out_fee_duffs)
FFI->>RustWallet: send_payment(...)
RustWallet-->>FFI: (txid, entry, fee)
FFI-->>ManagedPlatformWallet: writes out_txid, out_fee_duffs
ManagedPlatformWallet-->>SendDashPayPaymentSheet: (txid, 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 |
![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 (2)
packages/rs-platform-wallet/src/wallet/identity/network/payments.rs (1)
539-542: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
# Returnsdoc comment to mention the fee.The return type now includes
u64(network fee in duffs), but the doc comment still describes only theTxidandPaymentEntry. Callers reading the docs won't know about the third element.📝 Proposed doc update
/// # Returns /// -/// The `Txid` of the broadcast transaction and the newly created -/// [`PaymentEntry`] recording the outgoing payment. +/// A tuple of the `Txid` of the broadcast transaction, the newly created +/// [`PaymentEntry`] recording the outgoing payment, and the exact network +/// fee in duffs (inputs − outputs) of 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, The `# Returns` doc comment for the payment/broadcast function is out of date and does not mention the `u64` network fee in duffs. Update the return documentation near the function that returns the `Txid`, `PaymentEntry`, and fee so it clearly describes all three tuple elements, including the fee amount and its units.packages/rs-platform-wallet-ffi/src/dashpay.rs (1)
538-547: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDocument
out_fee_duffsin# Safety
Add the nullable-out-pointer contract forout_fee_duffs: callers may pass NULL to ignore the fee, and a non-null pointer must reference validu64storage.🤖 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-ffi/src/dashpay.rs` around lines 538 - 547, Update the safety contract for platform_wallet_send_dashpay_payment to explicitly document out_fee_duffs as a nullable output pointer: callers may pass NULL to ignore the fee, and any non-null value must point to valid writable u64 storage. Make sure the function’s # Safety section matches the existing nullable-output conventions used by the other FFI parameters in dashpay.rs.
🤖 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-ffi/src/dashpay.rs`:
- Around line 538-547: Update the safety contract for
platform_wallet_send_dashpay_payment to explicitly document out_fee_duffs as a
nullable output pointer: callers may pass NULL to ignore the fee, and any
non-null value must point to valid writable u64 storage. Make sure the
function’s # Safety section matches the existing nullable-output conventions
used by the other FFI parameters in dashpay.rs.
In `@packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- Around line 539-542: The `# Returns` doc comment for the payment/broadcast
function is out of date and does not mention the `u64` network fee in duffs.
Update the return documentation near the function that returns the `Txid`,
`PaymentEntry`, and fee so it clearly describes all three tuple elements,
including the fee amount and its units.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7123145-bd8d-4051-b361-f70822ba32bb
📒 Files selected for processing (5)
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/Sources/SwiftDashSDK/Utils/TestnetFaucet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/DashPay/SendDashPayPaymentSheet.swift
…fs contract CodeRabbit review nits on #4049: send_payment's # Returns now covers the third (fee) tuple element, and the FFI # Safety documents out_fee_duffs as a nullable out-pointer written only on success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both CodeRabbit findings fixed in 9ca4736: 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
Code Review
Source: reviewers: claude opus general (failed quota), codex gpt-5.5 general, claude opus security-auditor (failed quota), codex gpt-5.5 security-auditor, claude opus ffi-engineer (failed quota), codex gpt-5.5 ffi-engineer. Verifier: codex gpt-5.5 (Claude verifier failed quota).
Prior run at 57003a9 had no verified output, so there are no carried-forward findings. In the latest delta, the fee documentation/out-param contract is new and blocking because the returned value is still the builder's size-based fee, not the transaction's inputs-minus-outputs fee. Cumulatively, the promoted SDK faucet client has one in-scope hardening suggestion; the unrelated data-contract parser issue is real but outside PR #4049.
🔴 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 `packages/rs-platform-wallet/src/wallet/identity/network/payments.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:670-677: Return the actual transaction fee, not the builder's size fee
`send_payment` now exposes this value through Rust, FFI, and Swift as the exact network fee of the broadcast transaction, but it forwards the second value from `TransactionBuilder::build_signed`. In the pinned `key-wallet` source, `build_signed` computes that value as `fee_rate.calculate_fee(encoded_size(tx))`; it does not compute `sum(selected inputs) - sum(tx outputs)`. These diverge when the builder omits a dust change output: the dust remainder is also paid to miners, while the returned value remains only the size-based fee. That makes the new public API under-report the fee in real sends and breaks the PR's stated contract.
In `packages/swift-sdk/Sources/SwiftDashSDK/Utils/TestnetFaucet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Utils/TestnetFaucet.swift:264-266: Cap faucet proof-of-work by total expected work
This PR promotes `TestnetFaucet.requestCoreDash` into the SDK as reusable public API, but the captcha bounds still allow a same-domain or compromised faucet endpoint to return `c=256,d=6`. That is roughly 4.3 billion expected SHA-256 attempts before the 30-second timeout cancels the solver, pinning device CPU for each user-initiated faucet attempt. Bound the aggregate challenge cost, not just each field independently.
…e size fee Review finding on #4049: build_signed's returned fee is size-based (fee_rate x encoded_size) and under-reports whenever the builder drops a dust change remainder (<= 546 duffs) to miners instead of emitting the change output. send_payment now computes the fee it exposes as sum(selected input values) - sum(output values), reading input values from the account UTXO map (build reserves selected UTXOs but does not remove them). A selected input missing from the map is a hard error, never a fabricated fee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review hardening on #4049: the per-field bounds still admitted c=256, d=6 (~4.3B expected SHA-256 attempts) from a compromised or misbehaving faucet endpoint — enough to pin the device CPU until the 30 s timeout on every faucet tap. Bound the aggregate expected work (c x 16^d) at 64M attempts (~10x the live faucet's soft challenge); costlier challenges throw and route to the web-faucet fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both findings from the changes-requested review are addressed:
🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-reviewed current head eafbfda. Both prior findings are fixed: send_payment now reports inputs minus outputs, including dropped dust change, and the faucet caps aggregate expected proof-of-work at 64M attempts. Wallet and Swift CI pass. No additional issues found.
…fs contract CodeRabbit review nits on #4049: send_payment's # Returns now covers the third (fee) tuple element, and the FFI # Safety documents out_fee_duffs as a nullable out-pointer written only on success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e size fee Review finding on #4049: build_signed's returned fee is size-based (fee_rate x encoded_size) and under-reports whenever the builder drops a dust change remainder (<= 546 duffs) to miners instead of emitting the change output. send_payment now computes the fee it exposes as sum(selected input values) - sum(output values), reading input values from the account UTXO map (build reserves selected UTXOs but does not remove them). A selected input missing from the map is a hard error, never a fabricated fee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review hardening on #4049: the per-field bounds still admitted c=256, d=6 (~4.3B expected SHA-256 attempts) from a compromised or misbehaving faucet endpoint — enough to pin the device CPU until the 30 s timeout on every faucet tap. Bound the aggregate expected work (c x 16^d) at 64M attempts (~10x the live faucet's soft challenge); costlier challenges throw and route to the web-faucet fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
feat/dashpay-payment-fee-and-faucet-client
branch
from
eafbfda to
942b51d
Compare
|
Rebased onto current 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: Sol gpt-5.6-sol general, Sonnet claude-sonnet-5 general, Opus claude-opus-4-8 general. Verifier: Sonnet claude-sonnet-5. Orchestrator: openai/gpt-5.6-sol (orchestration-only, not reviewer evidence).
Prior reconciliation: both prior findings are FIXED at 942b51d. The payment path now computes the exact fee as inputs minus outputs, and the faucet caps aggregate expected PoW at 64,000,000 attempts. Carried-forward prior findings: none.
New findings in the latest cumulative head: add a successful send_payment regression test that exercises the dust/no-change case and asserts the exact returned fee. Also update dashpay.rs:599-601: the comment still says the fee comes straight from the builder, but this PR intentionally replaces the builder size-fee with an inputs-minus-outputs computation.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/network/payments.rs:677-709: No test exercises a successful send_payment fee computation
This block is the fix for the bug this PR addresses (returning the real inputs-minus-outputs fee instead of the builder's size-based estimate), but neither existing test reaches it successfully. `send_payment_runs_pending_contact_crypto_drain` (line 3259) pays a contact with no external account and fails before `build_signed`; `send_payment_passes_external_lookup_once_account_built` (line 3353) registers the external account but still asserts an `err` result at line 3419, so it also never reaches this code. Add a test with a funded test wallet that completes a real `send_payment` call, ideally hitting the dust-change-omission case called out in the comments at lines 677-681, and assert the returned fee equals `Σinputs − Σoutputs` and can exceed the builder's size-based fee. Without this, the exact behavior just fixed here can regress silently.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: Sol gpt-5.6-sol general, Sonnet claude-sonnet-5 general. Verifier: Sonnet claude-sonnet-5. Orchestrator: openai/gpt-5.6-sol (orchestration-only, not reviewer evidence).
Prior reconciliation: both prior findings are FIXED. The new tests complete a real build+sign+broadcast path and assert Σinputs−Σoutputs for both dropped-dust and emitted-change cases; the FFI comment now correctly describes that computation instead of the builder estimate. Carried-forward prior findings: none.
New findings in the latest delta: none.
New cumulative finding: ManagedPlatformWallet.swift:2105, introduced earlier in this PR, still documents the returned fee as coming “straight from the builder.” That is stale because send_payment deliberately discards the builder’s size-based estimate and returns selected inputs minus outputs, including dropped dust. Full platform-wallet + platform-wallet-ffi lib suites pass (140 tests), the two targeted fee tests pass, and clippy is clean.
💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
|
Nitpick fixed in 6210d2d — 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: Sol gpt-5.6-sol general/security-auditor/ffi-engineer; Sonnet claude-sonnet-5 general/security-auditor/ffi-engineer. Verifier: Sonnet claude-sonnet-5. Orchestrator: openai/gpt-5.6-sol (orchestration-only, not reviewer evidence).
Prior reconciliation: prior-swift-builder-doc is STILL VALID. PR #4049's own fee-computation commit changes the returned value away from the builder estimate to selected inputs minus outputs, so it directly makes the unchanged public Swift sentence false.
Carried-forward prior findings: one documentation nitpick:
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformWallet.swift:2105— Public Swift API doc still attributes the exact fee to the builder.sendDashPayPaymentsaysfeeDuffscomes “straight from the builder,” but #4049 deliberately discards that size-based estimate and computes the fee as selected inputs minus transaction outputs, including dropped dust change. The Rust FFI comment was corrected in this PR; the public Swift comment should likewise read:(duffs) computed as inputs minus outputs, including any dropped dust change.
New findings in the latest delta: none. The delta is rustfmt-only reformatting of two fee tests, and cargo fmt -p platform-wallet -- --check passes.
Additional cumulative findings: none.
💬 1 nitpick
|
This carried-forward nitpick is already fixed on the branch tip — this review round ran against 🤖 Addressed by Claude Code |
…fs contract CodeRabbit review nits on #4049: send_payment's # Returns now covers the third (fee) tuple element, and the FFI # Safety documents out_fee_duffs as a nullable out-pointer written only on success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e size fee Review finding on #4049: build_signed's returned fee is size-based (fee_rate x encoded_size) and under-reports whenever the builder drops a dust change remainder (<= 546 duffs) to miners instead of emitting the change output. send_payment now computes the fee it exposes as sum(selected input values) - sum(output values), reading input values from the account UTXO map (build reserves selected UTXOs but does not remove them). A selected input missing from the map is a hard error, never a fabricated fee. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review hardening on #4049: the per-field bounds still admitted c=256, d=6 (~4.3B expected SHA-256 attempts) from a compromised or misbehaving faucet endpoint — enough to pin the device CPU until the 30 s timeout on every faucet tap. Bound the aggregate expected work (c x 16^d) at 64M attempts (~10x the live faucet's soft challenge); costlier challenges throw and route to the web-faucet fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
feat/dashpay-payment-fee-and-faucet-client
branch
from
6210d2d to
9505cbd
Compare
|
Rebased onto current 🤖 Addressed by Claude Code |
…op case Two full end-to-end `send_payment` tests reach the fee block for the first time (no existing test did): they fund BIP-44 account 0 with a known UTXO, register the external contact account, sign with the seed signer, and broadcast through an accepting stub broadcaster injected via the `IdentityWallet<B>` broadcaster generic (production `identity()` pins `SpvBroadcaster`, whose runtime is never started in tests). Build, sign, and the Σin − Σout fee computation all run for real. - dust case: change ≤ 546 is dropped, so the tx has a single output and the reported fee is the full V − A, folding the dropped dust into the fee (the bug this PR fixed). - control: change > 546 is emitted, so Σin − Σout equals the plain size-based fee, proving the computation isn't blindly inflating sends. Also reword the stale rs-platform-wallet-ffi/dashpay.rs comment that claimed the fee comes "straight from the builder" — it is now computed in send_payment as Σ(inputs) − Σ(outputs). Co-Authored-By: Claude Opus <noreply@anthropic.com>
The Wallet-tests CI job runs cargo fmt --check; the hand-written test fixtures in 6618881 weren't rustfmt-clean. No logic change. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…t from the builder') sendDashPayPayment's doc still said the fee comes straight from the builder; send_payment now returns Σinputs − Σoutputs (dropped dust folded in). Comment-only. Co-Authored-By: Claude Opus <noreply@anthropic.com>
QuantumExplorer
force-pushed
the
feat/dashpay-payment-fee-and-faucet-client
branch
from
9505cbd to
d45de7f
Compare
QuantumExplorer
changed the title
|
Narrowed to the fee-correctness fix only. Per reviewer discussion, dropped the faucet captcha PoW-cap commit — the The captcha PoW cap is a real (minor) hardening on the now-upstream faucet — it'll come back separately alongside the faucet's placement review, not here. 🤖 Addressed by Claude Code |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: Sol gpt-5.6-sol general; Sonnet claude-sonnet-5 general. Verifier: Sonnet claude-sonnet-5. Orchestrator: openai/gpt-5.6-sol (orchestration-only, not reviewer evidence).
Prior reconciliation: the immediately prior review at 9505cbdd had zero verified findings.
Carried-forward prior findings: none.
New findings in the latest delta: one suggestion. The force-push dropped the aggregate captcha proof-of-work cap added as review hardening for this PR; the remaining independent bounds admit roughly 4.3 billion expected SHA-256 attempts and can saturate device CPUs until the 30-second timeout.
Additional cumulative findings: none. The exact-fee implementation, FFI/Swift threading, documentation, and dust-change/control tests are consistent. cargo test -p platform-wallet --lib send_payment_reports, cargo fmt -p platform-wallet -- --check, and cargo clippy -p platform-wallet -p platform-wallet-ffi --all-targets all passed at this head.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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/swift-sdk/Sources/SwiftDashSDK/Utils/TestnetFaucet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Utils/TestnetFaucet.swift:266-267: Restore the aggregate captcha proof-of-work cap dropped in the latest push
This exact cap existed at the immediately-prior-reviewed head of this PR (9505cbdd) and was added there specifically as 'Review hardening on #4049' (commit 942b51d0fc/eafbfda4ab). The latest force-push dropped it, and it is not merged anywhere else in the tree (confirmed absent from origin/v4.1-dev). The remaining per-field bounds alone still admit c=256, d=6 (~4.3B expected SHA-256 attempts), so any misbehaving/compromised same-domain faucet endpoint pins the device's cores across 256 parallel solver tasks until the 30-second timeout cancels them — an avoidable CPU/battery spike the file's own surrounding comments say should route to the web-faucet fallback instead. Re-add the aggregate bound so only the documented soft-challenge workload (~6.6M expected attempts) reaches the solver.
|
Re the dropped PoW cap: that was a deliberate scope decision by the repo owner, not force-push loss — this PR was narrowed to the 🤖 Addressed by Claude Code |
3 participants
Two commits used by dashwallet-ios (the DashPay contacts arc, dashwallet-ios #787), cherry-picked clean onto
v4.1-dev:Fee threading —
send_paymentreturns the exact network fee of the broadcast transaction;platform_wallet_send_dashpay_paymentgains a nullableout_fee_duffsout-param; SwiftsendDashPayPaymentreturns(txid: Data, feeDuffs: UInt64). Lets wallet UIs show the real fee on the payment success screen instead of an estimate.Testnet faucet client promoted into the SDK package —
TestnetFaucet+ the cap.js proof-of-work solver move from SwiftExampleApp intoSources/SwiftDashSDK/Utilsso dashwallet-ios can drive the faucet in-app without carrying a drifting copy (public API unchanged; the example app picks it up through its existing import). Includes the example-app call-site fix for the newsendDashPayPaymenttuple return.Verified:
cargo check -p platform-wallet -p platform-wallet-fficlean on this branch; both SwiftExampleApp and dashwallet-ios build against these commits (on the pre-rebase base); the fee display and the faucet request were exercised live on testnet (faucet txid395eeb08…).Not included (owned elsewhere): the tx-decode commit (PR #3981) and the
RawKeySignerextraction (47a83cf6d0, Row #27's branch).🤖 Generated with Claude Code
Summary by CodeRabbit