feat(wallet): fund sends from every spendable account - #928
Merged
Conversation
Adopts `.allSpendable` (platform #4329) on both send paths, replacing the explicit `.bip44` pin. The home balance has always been wallet-wide — `refreshBalanceBridge` publishes `coreWallet().balance()` and the initial seed uses `getWalletBalance(walletId:)`, neither of which is per-account — while sends could only spend BIP44. A user holding funds on BIP32 or a DashPay contact-receiving account saw them in the total and got "insufficient funds" trying to spend them. `maxSendable`, derived from that same balance, was over-optimistic for the same reason; it is now correct. `.allSpendable` resolves to SEND_FUNDING_SOURCES — BIP44 + BIP32 + AllDashpayReceivingFunds. CoinJoin is excluded by construction: spending mixed outputs beside transparent ones links them and undoes the mixing. A contact's watch-only external coins are excluded too — the selector takes only the receiving side the local seed can sign. Change returns to BIP44, the first pooled source. The MAYA swap deposit takes the same funding. `changeToFirstInput` is unaffected by pooling: it routes to whichever input BIP-69 puts at VIN0 regardless of source account, and the builder already sizes the change output for the largest eligible routing script. Its shape assertion still holds because every pooled source is P2PKH; the comment claiming BIP44 is the only possible source is corrected, and now records that VIN0 decides the MAYA refund address, which under pooling can be a DashPay receiving address — still this wallet's own seed-signable address, still counted in its balance. Two call sites keep their explicit pins: the CoinJoin sweep must drain exactly that account, and the CrowdNode selected-input send must keep inputs and change on the address CrowdNode identifies the user by. Requires a DashSDKFFI.xcframework rebuilt from platform v4.2-dev at or past #4329, which adds CORE_ACCOUNT_TYPE_FFI_ALL_SPENDABLE. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughStandard payments and swap deposits now use ChangesTransaction Funding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKTransactionSender.swift`:
- Around line 110-117: Update maxSendFeeReserveDuffs() to estimate fees against
the same .allSpendable pool used by finalizeAtomic, including BIP44, BIP32, and
eligible DashPay receiving UTXOs while excluding CoinJoin and watch-only
outputs. Use the SDK’s pooled-selection fee estimate if available, otherwise
enumerate that exact pool before calculating the reserve so
feeAwareMaxSendable() cannot exceed what finalizeAtomic can build.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f369d2a5-0ce5-40d1-b7c8-118b87361c4e
📒 Files selected for processing (1)
DashWallet/Sources/Infrastructure/SwiftDashSDK/SwiftDashSDKTransactionSender.swift
Review follow-up, and a regression this PR introduced. maxSendFeeReserveDuffs() enumerated BIP44 account 0 only. That matched the old BIP44-pinned send exactly, but once finalizeAtomic pools BIP44 + BIP32 + DashPay receiving, a real send can carry inputs the reserve never counted — more inputs, larger transaction, higher fee than reserved. feeAwareMaxSendable() subtracts that reserve from a wallet-wide balance, so Max could price itself above what the builder can fund and fail to build. The +50 % margin absorbs jitter, not a systematic undercount: BIP44 with 2 UTXOs and a DashPay account with 30 reserves for 2 inputs and spends 32. The reserve now enumerates the same set `.allSpendable` spends — BIP44 and BIP32 at the funding index plus every DashPay receiving account — and still excludes CoinJoin and a contact's watch-only external coins (tag 13), which the builder cannot sign. The flat-reserve fallback and the safety margin are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopts
.allSpendablefrom dashpay/platform#4329 on both send paths, replacing the explicit.bip44pin.Why this is a bug fix, not just an opt-in
The home balance has always been wallet-wide:
refreshBalanceBridgepublishescoreWallet().balance()and the initial seed usesgetWalletBalance(walletId:)— neither is per-account. But sends could only spend BIP44.So a user holding funds on BIP32 or a DashPay contact-receiving account saw those funds in their total and got "insufficient funds" trying to spend them.
maxSendablederives from the same wallet-wide balance, so it was over-optimistic for exactly the same reason; it becomes correct here.CoinJoin is not pooled
.allSpendableresolves toSEND_FUNDING_SOURCES= BIP44 + BIP32 +AllDashpayReceivingFunds. CoinJoin is excluded by construction in the Rust const, not merely by convention — spending mixed outputs beside transparent ones links them and undoes the mixing. A contact's watch-onlyDashpayExternalAccountcoins are excluded too: the selector takes only the receiving side the local seed can sign. Change returns to BIP44, the first pooled source.The mixed balance stays reachable only through the paths that name it explicitly — the CoinJoin sweep and the CoinJoin-drain asset lock — which are the two deliberate "move the mixed coins" flows.
MAYA swap path — checked
The swap deposit takes the same pooled funding. Verified rather than assumed:
changeToFirstInputis unaffected. It routes change toselected_inputs.first()post-BIP-69-sort, whatever account that input came from, and the builder already reserves "the largest eligible routing script so every possible winner is covered" when sizing the change output. Account-agnostic by construction.assertSwapDepositShapestill holds, because every pooled source is P2PKH. Its comment claimed BIP44 was the only possible source; corrected.If a non-P2PKH input ever reached VIN0 the assertion refuses the build: fail-closed, before broadcast.
Left pinned deliberately
.coinJoin.bip44+addInputsRequires an SDK rebuild
Needs
DashSDKFFI.xcframeworkrebuilt from platformv4.2-devat or past #4329 (addsCORE_ACCOUNT_TYPE_FFI_ALL_SPENDABLE):cd ../platform/packages/swift-sdk && ./build_ios.sh --target ios --target sim.Max-send reserve
maxSendFeeReserveDuffs()enumerated BIP44 account 0 only — exactly right for the old BIP44-pinned send, but an under-reserve once funding pools BIP32 and DashPay receiving inputs. SincefeeAwareMaxSendable()subtracts that reserve from a wallet-wide balance, Max could have priced itself above what the builder can fund. Fixed in f8c03da: the reserve now enumerates the same set.allSpendablespends, still excluding CoinJoin and watch-only external coins.Verification
dashpayscheme builds clean (arm64 simulator) against a freshly rebuilt xcframework, on a branch merged up to currentdevelop(#925 and #926 are in).Not exercised on-device: spending pooled funds needs a wallet holding balance on BIP32 or a DashPay receiving account, and the simulator is PIN-gated.
Summary by CodeRabbit