feat(swift-sdk): partial-amount platform address withdrawal wrapper#4139
Conversation
Add a withdraw(accountIndex:coreAddress:inputs:signer:) overload to ManagedPlatformAddressWallet using INPUT_SELECTION_TYPE_EXPLICIT: each WithdrawalInput names an address and the exact credits to withdraw, so the payout equals the requested amount (the transition fee is deducted from the fee-source input's remaining balance, per the default DeductFromInput(0) strategy). Complements the existing AUTO overload, which always withdraws the full non-dust balance. Swift-only — the FFI entry point already ships in DashSDKFFI. Used by dashwallet-ios for partial Platform → Transparent internal transfers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
⛔ Blockers found — Sonnet deferred (commit 5043b86) |
📝 WalkthroughWalkthroughChangesExplicit withdrawal flow
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Sequence Diagram(s)sequenceDiagram
participant ManagedPlatformAddressWallet
participant KeychainSigner
participant platform_address_wallet_withdraw_to_address
ManagedPlatformAddressWallet->>ManagedPlatformAddressWallet: Validate coreAddress and WithdrawalInput values
ManagedPlatformAddressWallet->>KeychainSigner: Pin signer for FFI call
ManagedPlatformAddressWallet->>platform_address_wallet_withdraw_to_address: Submit ExplicitInputFFI rows and fee rate
platform_address_wallet_withdraw_to_address-->>ManagedPlatformAddressWallet: Return changeset
ManagedPlatformAddressWallet->>ManagedPlatformAddressWallet: Check, free, and decode UpdatedBalance entries
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.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift (1)
557-573: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReject duplicate input addresses and validate the credits sum.
Similarly to how outputs are pre-checked in
transfer, duplicate input addresses should be rejected up-front to prevent silent overwrites when the Rust side parses them into a map (e.g.,BTreeMap). Additionally, checking forUInt64overflow when summing the input credits ensures the total is safely bounded before offloading the work to the detached task.♻️ Proposed validations
- for input in inputs { + let inputHashes = Set(inputs.map { $0.hash }) + guard inputHashes.count == inputs.count else { + throw PlatformWalletError.invalidParameter( + "Duplicate source addresses in inputs" + ) + } + + var totalInputCredits: UInt64 = 0 + for input in inputs { guard input.addressType == 0 else { throw PlatformWalletError.invalidParameter( "only P2PKH (addressType 0) withdrawal inputs are supported" ) } guard input.hash.count == 20 else { throw PlatformWalletError.invalidParameter( "withdrawal input hash must be 20 bytes, got \(input.hash.count)" ) } guard input.credits > 0 else { throw PlatformWalletError.invalidParameter( "withdrawal input credits must be > 0" ) } + + let sum = totalInputCredits.addingReportingOverflow(input.credits) + if sum.overflow { + throw PlatformWalletError.invalidParameter( + "Input credits sum overflowed UInt64" + ) + } + totalInputCredits = sum.partialValue }🤖 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift` around lines 557 - 573, Update the input pre-validation loop in the withdrawal flow to reject duplicate input addresses before creating the detached task, using the same address uniqueness approach as transfer output validation. While validating inputs, accumulate credits with checked UInt64 addition and throw PlatformWalletError.invalidParameter on overflow; preserve the existing addressType, hash-length, and positive-credit checks.
🤖 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.
Nitpick comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- Around line 557-573: Update the input pre-validation loop in the withdrawal
flow to reject duplicate input addresses before creating the detached task,
using the same address uniqueness approach as transfer output validation. While
validating inputs, accumulate credits with checked UInt64 addition and throw
PlatformWalletError.invalidParameter on overflow; preserve the existing
addressType, hash-length, and positive-credit checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a581e95-12bc-4f5c-b3f0-cd3e33beadaa
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The new partial-withdrawal API has two in-scope blockers. Caller-supplied explicit inputs are not authorized against the managed wallet/account before the shared signer resolves keys, and the documented first-input fee source is reinterpreted using Rust's canonical address order.
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 (failed),default— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed),default— security-auditor (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— ffi-engineer (failed),default— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:583-613: Explicit withdrawal inputs are not bound to the managed wallet or account
This managed-wallet overload accepts arbitrary address hashes and forwards them as `InputSelection::Explicit` without checking that they belong to `self` or `accountIndex`. The Rust explicit branch does not use `wallet_id` or `account_index` for authorization; the SDK only fetches the supplied addresses' balances and then asks the signer to sign every map key. `KeychainSigner` resolves a platform key by `addressHash` across the shared `ModelContainer`, takes the matched row's `walletId` and derivation path, and loads that wallet's mnemonic. Because one manager/container holds multiple wallets, code using wallet A's handle can supply a persisted wallet B hash and have the normal signer authorize a withdrawal to an arbitrary Core address. Post-broadcast reconciliation merely skips the foreign address and cannot prevent the spend. Validate every explicit input against this wallet and account before signing, preferably in the Rust wallet/FFI layer, and bind signer resolution to the expected wallet ID.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:593-610: Default fee strategy targets the canonical first address, not inputs[0]
The API tells callers to leave fee headroom on the first Swift input, but passing `nil, 0` becomes `DeductFromInput(0)` only after `parse_explicit_inputs` has inserted the array into a `BTreeMap<PlatformAddress, Credits>`. DPP resolves index 0 against that map's derived `Ord` order, so it means the lexicographically smallest address hash rather than `inputs[0]`. If a later, smaller-sorting input is withdrawn in full, fee validation fails because that unintended target has no remainder; if it has headroom, the fee is charged to the wrong address. This is the same insertion-order-versus-canonical-index class previously fixed for transfer outputs. Compute the canonical rank of the caller's intended fee source and pass an explicit `FeeStrategyStepFFI`, with a regression test whose Swift order differs from hash order.
| let ffiInputs = inputRows.map { input -> ExplicitInputFFI in | ||
| ExplicitInputFFI( | ||
| address: PlatformAddressFFI( | ||
| address_type: input.addressType, | ||
| hash: Self.hashTuple(from: input.hash) | ||
| ), | ||
| balance: input.credits | ||
| ) | ||
| } | ||
| var changeset = PlatformAddressChangeSetFFI(updated: nil, updated_count: 0) | ||
| // Fee strategy nil/0 → the Rust default `DeductFromInput(0)`: | ||
| // the first input is the fee source. Callers order inputs so | ||
| // index 0 carries the fee headroom. | ||
| let result = withExtendedLifetime(signer) { | ||
| address.withCString { addrCStr in | ||
| ffiInputs.withUnsafeBufferPointer { inputBp in | ||
| platform_address_wallet_withdraw_to_address( | ||
| handle, | ||
| accountIndex, | ||
| INPUT_SELECTION_TYPE_EXPLICIT, | ||
| inputBp.baseAddress, | ||
| UInt(inputBp.count), | ||
| nil, | ||
| 0, | ||
| addrCStr, | ||
| feePerByte, | ||
| nil, | ||
| 0, | ||
| signerHandle, | ||
| &changeset | ||
| ) |
There was a problem hiding this comment.
🔴 Blocking: Explicit withdrawal inputs are not bound to the managed wallet or account
This managed-wallet overload accepts arbitrary address hashes and forwards them as InputSelection::Explicit without checking that they belong to self or accountIndex. The Rust explicit branch does not use wallet_id or account_index for authorization; the SDK only fetches the supplied addresses' balances and then asks the signer to sign every map key. KeychainSigner resolves a platform key by addressHash across the shared ModelContainer, takes the matched row's walletId and derivation path, and loads that wallet's mnemonic. Because one manager/container holds multiple wallets, code using wallet A's handle can supply a persisted wallet B hash and have the normal signer authorize a withdrawal to an arbitrary Core address. Post-broadcast reconciliation merely skips the foreign address and cannot prevent the spend. Validate every explicit input against this wallet and account before signing, preferably in the Rust wallet/FFI layer, and bind signer resolution to the expected wallet ID.
source: ['codex']
| // Fee strategy nil/0 → the Rust default `DeductFromInput(0)`: | ||
| // the first input is the fee source. Callers order inputs so | ||
| // index 0 carries the fee headroom. | ||
| let result = withExtendedLifetime(signer) { | ||
| address.withCString { addrCStr in | ||
| ffiInputs.withUnsafeBufferPointer { inputBp in | ||
| platform_address_wallet_withdraw_to_address( | ||
| handle, | ||
| accountIndex, | ||
| INPUT_SELECTION_TYPE_EXPLICIT, | ||
| inputBp.baseAddress, | ||
| UInt(inputBp.count), | ||
| nil, | ||
| 0, | ||
| addrCStr, | ||
| feePerByte, | ||
| nil, | ||
| 0, |
There was a problem hiding this comment.
🔴 Blocking: Default fee strategy targets the canonical first address, not inputs[0]
The API tells callers to leave fee headroom on the first Swift input, but passing nil, 0 becomes DeductFromInput(0) only after parse_explicit_inputs has inserted the array into a BTreeMap<PlatformAddress, Credits>. DPP resolves index 0 against that map's derived Ord order, so it means the lexicographically smallest address hash rather than inputs[0]. If a later, smaller-sorting input is withdrawn in full, fee validation fails because that unintended target has no remainder; if it has headroom, the fee is charged to the wrong address. This is the same insertion-order-versus-canonical-index class previously fixed for transfer outputs. Compute the canonical rank of the caller's intended fee source and pass an explicit FeeStrategyStepFFI, with a regression test whose Swift order differs from hash order.
source: ['codex']
2 participants
Summary
Adds a
withdraw(accountIndex:coreAddress:inputs:signer:)overload toManagedPlatformAddressWalletusingINPUT_SELECTION_TYPE_EXPLICIT: eachWithdrawalInputnames an address and the exact credits to withdraw, so the payout equals the requested amount (the transition fee is deducted from the fee-source input's remaining balance, per the defaultDeductFromInput(0)strategy).Complements the existing AUTO overload, which always withdraws the full non-dust balance. Swift-only — the FFI entry point already ships in
DashSDKFFI.Why now
dashwallet-ios has required this API since its partial Platform → Transparent transfer landed (dashwallet-ios #817, and the Send redesign #819 builds on it) — building the app's
swift-sdk-integrationbranch againstv4.1-devwithout this commit fails with missing-member errors onwithdraw(…inputs:)/WithdrawalInput. This upstreams the local commit so plainv4.1-devbuilds the app again.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit