feat(platform-wallet): exact network fee from send_payment + shared testnet faucet client#4049
feat(platform-wallet): exact network fee from send_payment + shared testnet faucet client#4049QuantumExplorer wants to merge 5 commits into
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>
…ackage Moves TestnetFaucet + CapSolver from SwiftExampleApp into Sources/SwiftDashSDK/Utils so dashwallet-ios can drive the faucet in-app without carrying a drifting copy. Public API unchanged (TestnetFaucet().requestCoreDash, TestnetFaucet.webURL); the example app picks it up through its existing SwiftDashSDK import. Also folds in the example-app fallout from the fee-threading change: sendDashPayPayment returns (txid, feeDuffs) now, so SendDashPayPaymentSheet destructures the tuple. Both apps build; live-verified against faucet.thepasta.org (challenge -> solve -> redeem -> /api/core-faucet 200, txid 395eeb08...). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
Warning Review limit reached
Next review available in: 11 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 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 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.
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