feat(swift-example-app): wallet-signed Transfer & Withdraw for platform addresses (ADDR-02/04)#3923
Open
QuantumExplorer wants to merge 17 commits into
Open
feat(swift-example-app): wallet-signed Transfer & Withdraw for platform addresses (ADDR-02/04)#3923QuantumExplorer wants to merge 17 commits into
QuantumExplorer wants to merge 17 commits into
Conversation
…rm addresses (ADDR-02/04) Promote two SwiftExampleApp platform-address (DIP-17) actions from raw private-key demo forms to first-class, wallet-signed production UI, so users never paste a 64-char private key. Key derivation and signing stay in the platform-wallet crate behind rs-platform-wallet-ffi, driven by a KeychainSigner; Swift only picks source account / amount / destination and calls a single entry point. ADDR-02 (transfer) - UI only; the wallet-signed stack already existed: - New TransferPlatformAddressView: DIP-17 account picker (accountType 14) with credit balances, destination via own-wallet picker or pasted 20-byte P2PKH hash, amount field. Auto-selects an unused change address from the SwiftData pool internally, passes Auto nonce selection (not hardcoded 0), gates submit on amount + fee buffer <= balance and recipient != change, single-flight guard, resyncs balances on success. ADDR-04 (withdraw) - small Rust/FFI add + Swift wrapper + UI: - New FFI sibling platform_address_wallet_withdraw_to_address that accepts a base58 Core address, network-checks it Rust-side via require_network(wallet.network()), builds the script_pubkey, and delegates to the existing wallet.withdraw(...). Includes a unit test for the network check. - Thin ManagedPlatformAddressWallet.withdraw(accountIndex:coreAddress: coreFeePerByte:signer:) wrapper using INPUT_SELECTION_TYPE_AUTO + DeductFromInput(0) (full balance, no change output). - New WithdrawPlatformAddressView: source picker, Core L1 destination toggle (my-wallet via core_wallet_next_receive_address vs external paste), coreFeePerByte (default 1), full-balance total display, gated on Core wallet initialized with a clear "Core not ready" state, single-flight guard, resync on success. Wiring & docs: - WalletDetailView: Platform Balance row gains a Top Up / Transfer / Withdraw menu presenting both sheets. - TransitionCategoryView: legacy raw private-key Transfer/Withdraw links moved into a debug-only section. - TEST_PLAN.md: ADDR-02/ADDR-04 flipped to production status. Not a consensus change. Verified: cargo fmt clean, cargo check and the new FFI unit test pass, build_ios.sh regenerates the cbindgen header with the new symbol, and a clean SwiftExampleApp simulator build succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
QuantumExplorer
requested review from
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds production wallet-signed Platform→Core L1 withdrawal and refactors Platform→Platform transfer to use Rust AUTO input/fee selection. A new Rust FFI function handles network-validated Core address withdrawal. Swift SDK adds ChangesPlatform Address Withdrawal and Transfer
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletDetailView
participant WithdrawPlatformAddressView
participant ManagedPlatformAddressWallet
participant FFI as platform_address_wallet_withdraw_to_address
participant RustWallet as wallet.withdraw (Rust)
participant SwiftData
User->>WalletDetailView: tap ⋯ → Withdraw to Core
WalletDetailView->>WithdrawPlatformAddressView: present sheet
WithdrawPlatformAddressView->>WithdrawPlatformAddressView: checkCoreReady, autoSelectDefaults
User->>WithdrawPlatformAddressView: select account, destination, fee
User->>WithdrawPlatformAddressView: tap Submit
WithdrawPlatformAddressView->>ManagedPlatformAddressWallet: withdraw(accountIndex, coreAddress, fee, signer)
ManagedPlatformAddressWallet->>FFI: platform_address_wallet_withdraw_to_address
FFI->>FFI: parse base58 address, require_network
FFI->>RustWallet: wallet.withdraw(AUTO, CoreScript, fee)
RustWallet->>RustWallet: select_withdrawable_inputs, reserve_withdrawal_fee_on_largest_input
RustWallet-->>FFI: PlatformAddressChangeSet
FFI-->>ManagedPlatformAddressWallet: out_changeset
ManagedPlatformAddressWallet-->>WithdrawPlatformAddressView: [UpdatedBalance]
WithdrawPlatformAddressView->>SwiftData: persistUpdatedBalances
WithdrawPlatformAddressView-->>User: show success state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
|
✅ Review complete (commit 91e7c4b) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- Around line 57-58: The creditsPerDash constant uses Double which introduces
floating-point rounding errors when converting DASH amounts to credits in
value-transfer operations. Instead of using Double arithmetic, parse DASH
decimal amounts directly from their string representation into integer credits.
This needs to be fixed in multiple locations: where creditsPerDash is defined
(lines 57-58), and also at the other affected sites (lines 354-360 and 460-462).
For each location, replace the Double-based conversion with direct
string-to-integer parsing that multiplies the decimal DASH string value by 1e11
credits per DASH using integer arithmetic to avoid any floating-point precision
loss.
- Around line 89-97: The sheet can be dismissed via swipe gesture while a
transfer is in flight, even though the Cancel button is disabled, which causes
users to lose feedback and potentially resubmit the same transfer. Prevent
interactive dismissal of the sheet by adding the
`.interactiveDismissalDisabled()` modifier (or equivalent presentation binding
control) and configure it to block dismissal when a transfer submission is in
progress. Apply this protection at all locations where the transfer sheet is
presented, including both the primary location shown in the diff around line
89-97 and the secondary location at lines 438-455, ensuring the dismissal is
only disabled while the transfer request is actively being processed.
- Around line 295-306: The ownWalletRecipientCandidates property filters out the
auto-selected change address but does not exclude addresses that are
source-account inputs with balance, allowing the UI to list funded source
addresses as recipients. This causes the button to enable but then fail in the
wrapper. Modify the filter chain in ownWalletRecipientCandidates to also exclude
any address hashes that correspond to funded source inputs by collecting those
source-account input hashes and adding an additional filter condition to reject
them. Additionally, at the other affected locations (lines 370-376 and 405-410),
add validation to reject pasted external hashes that match any of these funded
source-input address hashes to prevent invalid recipient selection at those
sites as well.
- Around line 211-214: The footer text is performing unsafe arithmetic on
`credits + Self.feeBuffer` without overflow protection, while the `canSubmit`
logic already uses `addingReportingOverflow` for safe arithmetic. Apply the same
overflow-safe pattern to the insufficient balance check in the Text display by
using `addingReportingOverflow` instead of direct addition, ensuring that
potential overflow from very large typed amounts is handled safely when
rendering the error message.
- Around line 340-347: The resolvedDestination computed property's .ownWallet
case searches all platform addresses by hash alone, which in a multi-wallet
environment could incorrectly match a hash from another wallet. Modify the
lookup in the .ownWallet case to scope the search to only the current wallet's
platform addresses before searching by hash. Instead of searching
allPlatformAddresses directly for a matching addressHash, first filter or
constrain the search to addresses belonging to the current wallet row, then find
the address type from that scoped subset.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- Around line 316-320: The parsedFeePerByte property in
WithdrawPlatformAddressView accepts any positive UInt32 without validating
against unreasonably high fee rates, which could lead to accidental extreme fees
being deducted from a full-balance withdrawal due to a typo. Modify the
parsedFeePerByte property to either enforce a product-approved maximum
fee-per-byte threshold and return nil if exceeded, or add logic to require
explicit user confirmation when the parsed value exceeds a reasonable threshold
before returning it. This validation should occur in the guard statement or
immediately after successful UInt32 parsing to catch unusually high values
early.
- Around line 418-425: The withdraw() method at line 418 in
WithdrawPlatformAddressView returns an [UpdatedBalance] array that is currently
being discarded with the underscore pattern. Instead of discarding this return
value and relying solely on the performSync() call afterward, capture the
returned [UpdatedBalance] changeset and persist those balance changes directly
to the matching PersistentPlatformAddress rows immediately (before or alongside
the performSync() call) to prevent SwiftData from displaying incorrect balances
until the sync completes.
- Around line 84-96: The WithdrawPlatformAddressView sheet can be interactively
dismissed by swiping down even while isSubmitting is true and the Cancel button
is disabled, which allows the withdrawal task to continue in the background
unnoticed. Add the `.interactiveDismissDisabled(isSubmitting)` modifier to the
view to prevent interactive dismissal during the submission process, ensuring
the sheet remains visible until the withdrawal operation completes or fails.
Apply this modifier at the appropriate level to the view hierarchy containing
the toolbar and alert definitions.
In `@packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md`:
- Line 102: The statement on line 102 claims that document create operations are
reachable only through the builder interface, but this conflicts with DOC-02
elsewhere in the test plan which documents a production Contracts flow for
document creation. Locate the DOC-02 row documentation to understand what
production entry point it describes, then revise the line 102 statement to
accurately reflect that document creation (and any other operations mentioned)
may have multiple entry points including both the builder interface and
production flows. Ensure the revised text clearly communicates all available
routing paths so QA has consistent guidance on how to access these operations
throughout the app.
🪄 Autofix (Beta)
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
Run ID: bac36ed2-509d-409e-a471-4bf6054bf034
📒 Files selected for processing (7)
packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/WalletDetailView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionCategoryView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
thepastaclaw
suggested changes
Jun 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
PR adds the new platform_address_wallet_withdraw_to_address FFI plus production wallet-signed Transfer/Withdraw sheets. Three blocking issues in the new UI/wrapper path: the existing transfer wrapper newly exposed by this PR uses _ = signer instead of the optimizer-safe withExtendedLifetime(signer) that the new withdraw wrapper documents as required; the source-account picker is not actually honored by input selection because addressesWithBalances() reads from first_platform_payment_managed_account() regardless of accountIndex; and the footer's trapping credits + feeBuffer arithmetic can overflow on legal parsedCredits values and crash the sheet mid-render. One smaller logic issue (own-wallet recipient picker can default to a funded source-account address that the wrapper then excludes from inputs) plus a typed-error-mapping miss for the new network check round out the suggestions.
🔴 3 blocking | 🟡 2 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/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:284-309: Pin the KeychainSigner across the transfer FFI call
The new `TransferPlatformAddressView` routes a `KeychainSigner` (line 425) through `ManagedPlatformAddressWallet.transfer(...)` (line 442), which only does `_ = signer` inside its `Task.detached` block before calling `platform_address_wallet_transfer`. `KeychainSigner` registers its vtable ctx with `Unmanaged.passUnretained`, and the new `withdraw(...)` wrapper directly above (lines 442–467) explicitly documents that a bare discard reference can be elided by `-O` and drop the signer mid-call, causing a use-after-free in the synchronous vtable callback Rust invokes from `block_on(wallet.transfer(...))`. Wrap the FFI cascade in `withExtendedLifetime(signer) { ... }` so this production-UI path uses the same guaranteed lifetime boundary as `withdraw`, `fundFromAssetLock`, and `resumeFundFromAssetLock`.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:442-446: Selected source account is not honored by transfer input selection
The new sheet's source-account picker (lines 118–144) lets the user pick among every `accountType == 14` account on the wallet, and `submit()` forwards that choice to `ManagedPlatformAddressWallet.transfer(accountIndex:...)`. But that wrapper builds explicit inputs from `addressesWithBalances()` (line 205) without account scoping, and the underlying FFI/Rust path reads only `first_platform_payment_managed_account()` (`packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs:305`). With more than one PlatformPayment account on the wallet, picking account #1 can build explicit inputs from account #0's addresses while telling Rust `account_index = 1`, leading to wrong-account spending or post-spend persistence drift. Either scope input selection (and `addressesWithBalances`) by the chosen `accountIndex` end-to-end, or document and enforce a single-account precondition in the picker so the picker's choice cannot diverge from the wrapper's actual source.
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:211-218: Amount footer can overflow UInt64 and crash the transfer sheet
`parsedCredits` (line 354) accepts any value up to `Double(UInt64.max)`, so a user typing a very large but parseable amount produces `credits` near `UInt64.max`. The footer then evaluates `credits + Self.feeBuffer` with trapping `UInt64` arithmetic during every SwiftUI render — this overflows and traps long before `canSubmit`'s `addingReportingOverflow` (line 374) runs, crashing the sheet while the user is still typing. Use `addingReportingOverflow` (or an equivalent saturating check) in the footer so the overflow is surfaced as an insufficient-balance message instead of a process trap.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:300-306: Own-wallet recipient picker can default to a funded source-account address the wrapper then excludes from inputs
`ownWalletRecipientCandidates` only filters out the auto-selected change address. `autoSelectRecipient()` (line 390) then picks the first remaining candidate, which on a freshly funded source account is typically that account's lowest-index *funded* address. `ManagedPlatformAddressWallet.transfer` excludes recipient hashes from the input set (line 207), so the only funded source-account address can be removed from the spendable set, leaving `canSubmit` enabled (it checks total balance, not spendable balance) but the FFI call failing with insufficient inputs. Exclude funded addresses on the selected source account from the own-wallet recipient list, or at least from the auto-selected default, so the sheet can't auto-build a known-unsubmittable transfer.
In `packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/platform_addresses/withdrawal.rs:143-167: Wrong-network Core addresses cross FFI as ErrorUnknown
On `require_network` failure, line 147 wraps the error in `PlatformWalletError::AddressOperation`, and line 167's `unwrap_result_or_return!` then routes it through the blanket `From<PlatformWalletError>` impl, whose catch-all arm at `error.rs:240` maps `AddressOperation` to `ErrorUnknown`. Swift already has a dedicated `ErrorInvalidNetwork`/`PlatformWalletError.invalidNetwork` code (`PlatformWalletResult.swift`), so callers can no longer distinguish the precise wrong-network condition this new FFI function was specifically added to enforce. Detect the `AddressOperation` variant produced here (or thread the error through as a typed `InvalidNetwork` variant) and map it explicitly to `ErrorInvalidNetwork` before the blanket conversion runs.
…ransfer/withdraw UI - Parse DASH→credits exactly via integer arithmetic (multipliedReportingOverflow + addingReportingOverflow), dropping Double from the value-transfer path; Double is now display-only in formatCredits. - Add .interactiveDismissDisabled(isSubmitting) to both sheets so they can't be swiped away mid-submit. - Use overflow-safe fee arithmetic in the transfer amount footer (match canSubmit). - Exclude funded source-account inputs from transfer recipients (own-wallet picker, canSubmit, and submit guard) to avoid an enabled button that fails in the wrapper. - Scope resolvedDestination own-wallet lookup to the current wallet. - Bound withdraw coreFeePerByte to a sane max (10_000 duffs/byte) with validation. - Persist the returned [UpdatedBalance] changeset to matching rows immediately (both transfer and withdraw) before performSync(), mirroring the persistAddressBalances upsert, so SwiftData doesn't show drained inputs as spendable until sync. - TEST_PLAN: drop "document create" from the builder-only list (DOC-02 has a production Contracts flow). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at 20761a2. Latest delta (926eebb..20761a2) is clean Swift UI/wrapper hardening — no NEW blocking issues introduced by this delta. Prior reconciliation: prior-3 (UInt64 overflow in amount footer) and prior-4 (own-wallet recipient leaking funded source inputs) are FIXED; prior-1, prior-2, prior-5, prior-6 remain STILL VALID at this head. Two carried-forward blocking issues (transfer KeychainSigner lifetime, transfer ignoring selected source account) keep the PR not-ready. Codex also surfaces two additional smaller Swift-wrapper concerns (public-API overflow trap on fee-buffer add, and withdrawal DeductFromInput(0) edge case) that are worth fixing while in the same wrapper.
🔴 2 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:284-309: Carried forward (STILL VALID): pin KeychainSigner across the transfer FFI call
The transfer wrapper still relies on a bare `_ = signer` inside `Task.detached` (line 285) before invoking `platform_address_wallet_transfer`. The withdraw wrapper added in this same PR (lines 442–467) was patched to use `withExtendedLifetime(signer) { ... }` with a comment that spells out exactly why this matters: a bare `_ = signer` can be elided by the -O optimizer and drop the signer mid-call, causing a use-after-free in the vtable callback. The transfer path uses the same `signerHandle = signer.handle` → Rust-vtable pattern (Rust dereferences `signer_address_handle as *const VTableSigner` on the callback path), so the identical UAF hazard applies. KeychainSigner registers FFI callbacks against `Unmanaged.passUnretained(self)`; if ARC drops the signer before the synchronous FFI returns, the Rust callback trampoline will dereference a freed Swift object. Apply the same `withExtendedLifetime(signer) { ... }` wrapper here and drop the dead `_ = signer` stub — leaving transfer on the unsafe pattern while withdraw uses the safe one in the same file ships the exact bug the new comment warns about.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:205-230: Carried forward (STILL VALID): transfer wrapper ignores caller's accountIndex when selecting inputs
`transfer(accountIndex:outputs:changeAddress:signer:)` forwards `accountIndex` to `platform_address_wallet_transfer`, but the explicit inputs it builds come from `addressesWithBalances()` (line 205), which calls FFI `platform_address_wallet_addresses_with_balances`. That FFI has no `account_index` parameter and dispatches in Rust to `wallet.addresses_with_balances()`, which reads `info.core_wallet.first_platform_payment_managed_account()` — i.e. only the first DIP-17 account, regardless of the caller's selection. Returned entries also stamp `account_index: 0`. The new `TransferPlatformAddressView` now exposes a real picker over every `accountType == 14` account on the wallet, so a user with multiple platform-payment accounts selecting 'Account #N' will silently have inputs drawn from account #0's funded addresses while Rust signs/spends them and persists the changeset under the selected index. The result is either a misleading 'insufficient balance' error or a cross-account spend that violates the source-account boundary the UI presents. The right fix per `packages/swift-sdk/CLAUDE.md` is to add a Rust-side `addresses_with_balances_for_account(account_index)` entry point in `platform-wallet` plus a matching FFI export, then call it here. Until that is wired up, the picker must be locked to the first DIP-17 account so the wrapper's invariant matches the UI's claim.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:223-228: Public transfer API can trap on `totalRecipientCredits + Self.feeBuffer` overflow
The wrapper carefully uses `addingReportingOverflow` while summing the recipient outputs (lines 177-183), but then evaluates `totalRecipientCredits + Self.feeBuffer` with checked-`+` UInt64 arithmetic at lines 223 and 225. The SwiftUI sheet now rejects amounts that would overflow this sum, but `ManagedPlatformAddressWallet.transfer` is a public SDK API — a direct caller passing `credits > UInt64.max - feeBuffer` will hit a Swift integer-overflow trap instead of a typed `PlatformWalletError`. Compute the threshold once with `addingReportingOverflow(Self.feeBuffer)` before the loop and throw `invalidParameter` if it overflows, so the wrapper's contract matches the validation it already applies to recipient sums.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:438-440: Withdrawal fee strategy `DeductFromInput(0)` may fail when input 0 cannot cover the fee
The withdraw wrapper unconditionally sends a single `DeductFromInput(0)` fee step (line 439). Auto-selection on the Rust side will return every funded address on `accountIndex`, but the deduction step indexes only the lexicographically-first input. If that one input's balance is smaller than the estimated fee — even when other selected inputs would cover it — the transition can fail fee validation despite the account having enough aggregate balance. This is a Codex-only finding and is reachable from the new production ADDR-04 sheet. Worth confirming against the DPP fee-deduction logic and either widening the fee strategy to spread across inputs, or auto-selecting the largest-balance input as the fee source.
… withdrawal error Address second-reviewer findings on the platform-address transfer/withdraw UI: - Transfer source-account divergence (blocking): the transfer wrapper always spends first_platform_payment_managed_account() (account 0/key_class 0) via addresses_with_balances(), which is not account-scoped, while it forwards the picked accountIndex to Rust only for persistence. With >1 platform-payment account this spends the wrong account. Constrain the transfer source picker to accountType==14 && accountIndex==0 && keyClass==0 so the picker's choice provably matches the wrapper's real source. Withdraw's INPUT_SELECTION_TYPE_AUTO path is account-scoped (platform_payment_managed_account_at_index), so its multi-account picker is correct and left unchanged; both behaviors documented. - Wrong-network Core address now returns ErrorInvalidNetwork (maps to Swift .invalidNetwork) from platform_address_wallet_withdraw_to_address instead of falling through the blanket PlatformWalletError conversion to ErrorUnknown. - Drop the unnecessary unchecked_address.clone() in the FFI body (kept in the test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Verified at 6b65c44. The two-file delta from 20761a2 fixes the Rust-side withdrawal error mapping (prior-5) by routing wrong-network errors through a typed ErrorInvalidNetwork PlatformWalletFFIResult and drops the dead unchecked_address.clone() (prior-6). The Swift-side UI change limits the transfer source picker to (accountIndex 0, keyClass 0) as a workaround for prior-2. Prior-1 (transfer wrapper signer pinning), prior-2 (public transfer wrapper still account-unaware), prior-3 (fee-buffer overflow), and prior-4 (DeductFromInput(0)) all remain STILL VALID at the SDK boundary because the Swift wrapper at packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift is unchanged in this delta. No new in-scope defects were introduced by the latest delta.
🔴 2 blocking | 🟡 1 suggestion(s)
3 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:284-309: Carried forward (STILL VALID): pin KeychainSigner across the transfer FFI call
Verified at HEAD: line 285 still does `_ = signer` inside `Task.detached` and the entire `platform_address_wallet_transfer` invocation runs without a strong reference holding the Swift `KeychainSigner` alive. The sibling withdraw wrapper at line 447 uses `withExtendedLifetime(signer) { ... }` and the inline comment at lines 442-446 explicitly explains why: `_ = signer` is a no-op assignment the `-O` optimizer is free to elide, after which the signer can be deallocated mid-call while Rust is still invoking the vtable callbacks registered via `Unmanaged.passUnretained` — a use-after-free in the signing path. The transfer path performs the same callback-driven signing pattern but is missing the same fix. Mirror the withdraw wrapper and wrap the whole `withUnsafeBufferPointer` chain in `withExtendedLifetime(signer)`.
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:142-230: Carried forward (STILL VALID): public transfer API still ignores caller's accountIndex when selecting inputs
Verified at HEAD: `transfer(accountIndex:outputs:changeAddress:signer:)` forwards `accountIndex` to `platform_address_wallet_transfer` at line 293 to tell Rust which account to persist nonces/balances against, but the explicit input set is still built from `addressesWithBalances()` (line 205) whose FFI is resolved by `first_platform_payment_managed_account()` on the Rust side — always account (0, 0) regardless of `accountIndex`. The latest delta only constrains `TransferPlatformAddressView.platformAccountOptions` to account 0 / key class 0 as a workaround, which fixes the in-tree UI but does not fix the public SDK API. Any other consumer of `ManagedPlatformAddressWallet.transfer` (future SwiftExampleApp screens, third-party callers, tests) can still pass `accountIndex != 0` and silently spend account 0 balances while Rust persists the result under another account, drifting persistence from the sufficiency gate at lines 225-229. Either reject non-zero `accountIndex` in the wrapper with a typed `invalidParameter` (cheapest fix), drop the parameter entirely until an account-scoped FFI exists, or — per swift-sdk/CLAUDE.md — add an account-scoped `addressesWithBalances(accountIndex:)` on the Rust side so input selection stays in Rust where it belongs.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:223-228: Carried forward (STILL VALID): `totalRecipientCredits + Self.feeBuffer` can trap on UInt64 overflow
Verified at HEAD: lines 223 and 225 use the unchecked `+` operator on `totalRecipientCredits + Self.feeBuffer`. The recipient-sum loop at lines 177-183 already uses `addingReportingOverflow` to reject overflow with a typed `PlatformWalletError.invalidParameter`, proving the author is willing to validate this; the fee-buffer addition skips that treatment. A caller supplying outputs whose checked sum lands within `Self.feeBuffer` (100M credits) of `UInt64.max` will hit Swift's arithmetic overflow trap and abort the process rather than receive a typed error. This is a fail-stop denial-of-service on the wallet process, not a memory issue, but the wrapper is a public API and the fix is mechanical.
The platform-address withdrawal auto-selector inserted each selected input's full balance as its withdraw amount, leaving zero remaining balance on every input. Because the chain deducts the fee from each input's *remaining* balance (on_chain_balance - withdraw_amount), cascading through the fee_strategy steps, a single DeductFromInput(0) step could never be covered and the transition was rejected with fee_fully_covered = false for any input configuration (asserted by test_exact_balance_withdrawal_fails_insufficient_remaining_for_fees). The fee target also resolves to the BTreeMap index-0 entry, i.e. the lexicographically smallest address hash (PlatformAddress: Ord), independent of balance. Add reserve_withdrawal_fee_on_fee_source, which reduces the withdraw amount on the fee-source input (the index the first DeductFromInput step resolves to) by the estimated fee, leaving >= estimated_fee of headroom there while withdrawing the other inputs in full. Typed errors when no input can absorb the fee above min_input_amount or the net withdrawal falls below min_withdrawal_amount. This mirrors the transfer path's select_inputs_deduct_from_input invariant. Adds 4 unit tests (single-input headroom, small-fee-source-with-larger-peer, fee-source-too-small, total-below-fee). The buggy auto-selector predates this work (#3602); PR #3923 is the first to ship it to users via the ADDR-04 withdraw path, so it is fixed here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, largest-input withdrawal fee Address second-pass review findings on the platform-address transfer/withdraw SDK boundary (all newly shipped to users by this PR): - Pin the KeychainSigner across the transfer FFI call: transfer(...) now wraps the whole platform_address_wallet_transfer cascade in withExtendedLifetime(signer) and drops the elision-prone `_ = signer`, matching withdraw/fundFromAssetLock. Prevents a use-after-free of the Unmanaged.passUnretained signer ctx under -O. - Honor the caller's accountIndex in transfer input selection. Added Rust PlatformAddressWallet::addresses_with_balances_for_account(account_index) (using platform_payment_managed_account_at_index) + FFI platform_address_wallet_addresses_with_balances_for_account; transfer(...) now builds explicit inputs from the chosen account instead of always first_platform_payment_managed_account(). The existing account-agnostic method is untouched. The TransferPlatformAddressView picker offers all platform-payment accounts again (workaround pin removed). - Overflow-safe public transfer API: compute totalRecipientCredits + feeBuffer via addingReportingOverflow once and throw invalidParameter on overflow, instead of a trapping checked-add during the sufficiency gate. - Withdrawal fee source = largest-balance selected input. The auto-withdrawal path now reserves the fee on the largest selected input and emits DeductFromInput(<that input's index>) rather than trusting the wrapper's hardcoded index 0 (which resolves to the lexicographically-smallest address hash). A small lex-first input no longer blocks an otherwise-fundable withdrawal; genuine insufficiency still returns a typed error. platform-wallet lib tests 207 passed, platform-wallet-ffi 97 passed; build_ios.sh BUILD SUCCEEDED; SwiftExampleApp simulator build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward: all four prior findings (prior-1 transfer signer lifetime, prior-2 account-honored input selection, prior-3 fee-buffer overflow, prior-4 largest-input withdrawal fee reservation) are FIXED at 1bcedc7; both Claude and Codex agree, evidence verified against the worktree. New in latest delta: one blocking bug — the AUTO withdrawal path selects every address with balance > 0 without filtering by min_input_amount, so any dust address makes DPP reject the otherwise-valid transition. Plus four suggestions: a stale wrapper doc comment that still describes the old lex-smallest fee-source behavior, two UI/Rust scoping mismatches in the Withdraw and Transfer sheets when non-zero PlatformPayment key classes exist, and a defensive embedded-NUL check on the new Core-address C-string boundary.
🔴 1 blocking | 🟡 4 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/platform_addresses/withdrawal.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs:226-248: AUTO withdrawal selects dust inputs that DPP rejects with min_input_amount validation
`auto_select_inputs_for_withdrawal` selects every address with `balance > 0` (lines 230-239) and the resulting map values are the withdraw amounts handed to `sdk.withdraw_address_funds(...)`. DPP's `AddressCreditWithdrawalTransition` v0 validator (rs-dpp/.../address_credit_withdrawal_transition/v0/state_transition_validation.rs:148-154) rejects any input whose amount is `< platform_version.dpp.state_transitions.address_funds.min_input_amount` (100_000 credits in v1/v2/v3). A single sub-min address — easy to produce after partial spends or split deposits — makes the entire transition fail validation even when the account has more than enough withdrawable funds on other addresses.
The transfer auto-selector already handles this correctly: `transfer.rs:678` filters candidates with `*balance >= min_input_amount`. Withdrawal needs the same guard before constructing the input map. `reserve_withdrawal_fee_on_largest_input` only checks `min_input_amount` against the fee-source input (withdrawal.rs:328), so dust on any non-fee input slips through.
Fix by filtering at selection time (and either erroring or silently skipping; the transfer path errors via a typed `OnlyDustInputs`-style message).
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:446-506: Withdraw wrapper doc + inline comment still describe the old lex-smallest fee-source behavior; the Rust AUTO path now picks the largest-balance input
The wrapper doc comment at lines 454-461 says the Rust auto-selector reserves the estimated fee on `DeductFromInput(0)`, the lexicographically-smallest address — and the inline comment at 500-503 repeats that the index-0 input is the fee source. This describes the prior behavior, which was exactly what the previous review flagged as broken.
The new Rust path (withdrawal.rs:90-115 + `reserve_withdrawal_fee_on_largest_input` at withdrawal.rs:278-348) picks the LARGEST-balance selected input as the fee source, reduces *its* withdraw amount by the estimated fee, and emits `DeductFromInput(<that input's BTreeMap index>)` — intentionally overriding whatever fee strategy the Swift wrapper passes. The `feeRows: [DeductFromInput(0)]` constructed at lines 504-506 is silently discarded inside the AUTO branch.
This is non-functional (Rust does the right thing regardless), but the comments will mislead the next person debugging a withdrawal — including anyone wondering why `DeductFromInput(0)` is constructed at all. Either drop the Swift-side `feeRows` (the FFI accepts an empty strategy for AUTO) or update the doc + inline comments to state that the Rust AUTO path owns its own fee strategy and overrides the caller's.
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swift:488-491: Reject embedded NULs in coreAddress before crossing the C ABI
The wrapper trims and rejects empty strings, then passes `address.withCString` to `platform_address_wallet_withdraw_to_address`, where Rust reads it with `CStr::from_ptr(core_address).to_str()` (rs-platform-wallet-ffi/.../withdrawal.rs:116). Swift `String` can contain embedded NULs that survive into the `withCString` UTF-8 buffer; `CStr` stops at the first NUL. A caller/UI that supplies `validAddress\0suffix` would have Rust parse, network-check, sign, and withdraw to `validAddress`, while the Swift-side value is not the same string.
In practice base58check decoding would reject many truncations, and the iOS text fields don't easily produce embedded NULs, so this is defensive — but this is the new production wallet-signed withdrawal path, and rejecting NULs at the wrapper costs nothing. Either reject at the wrapper or move the FFI to a pointer + length signature.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:314-327: Withdraw source picker is not scoped to PlatformPayment key class 0 (Rust only spends key class 0)
`platformAccountOptions` filters source accounts by `accountType == 14` only, with no `keyClass == 0` filter (line 317). The Rust withdraw path receives only `account_index` and resolves the account via `platform_payment_managed_account_at_index(account_index)`, which is documented as key class 0 (wallet.rs:325-326). If a wallet ever stores PlatformPayment accounts at a non-zero key class for the same accountIndex, the picker would list duplicate rows for the same accountIndex and submit would either silently spend key-class-0 funds while the UI labeled it differently, or fail with insufficient funds when the user picks a non-zero-class row.
The sibling Transfer sheet was updated in this PR to filter `accountType == 14 && keyClass == 0` (TransferPlatformAddressView.swift:302). Apply the same filter here for consistency with what the Rust API actually spends. Also scope the balance aggregation (lines 320-324) through `acct.keyClass == 0` if and when PersistentAccount-joined queries are added.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:299-312: Transfer source-account balance is summed across all PlatformPayment key classes; only key class 0 is actually spendable
The source-account filter correctly restricts to `accountType == 14 && keyClass == 0` (line 302). However, the displayed balance at lines 304-309 sums every `PersistentPlatformAddress` matching `walletId + accountIndex` without joining through the parent account's `keyClass`. `PersistentPlatformAddress` does not carry `keyClass` directly (only via `account: PersistentAccount?`), so if a non-zero key-class PlatformPayment account at the same `accountIndex` ever holds funds, they'll be counted toward the displayed total of the key-class-0 row.
Result: with mixed-key-class storage, `selectedSourceAccountCredits` can be inflated, `canSubmit` can enable submission for a total Rust will never spend, and the actual `addressesWithBalances(forAccount:)` call (which Rust resolves to key class 0) returns less than the UI promised. Symptoms surface only when non-zero key class PlatformPayment accounts hold balances — a configuration that may be uncommon today but should be consistent.
Fix by joining the address rows through their parent account and filtering on `account?.keyClass == 0` (or denormalizing `keyClass` onto `PersistentPlatformAddress`).
…key class 0 Address third-pass review findings on the platform-address transfer/withdraw path: - AUTO withdrawal now filters inputs to balance >= min_input_amount before building the input map (new select_withdrawable_inputs helper), so a single sub-min "dust" address no longer makes DPP reject the whole transition. Returns typed OnlyDustInputs when only dust exists, the generic no-funds error otherwise; composes with reserve_withdrawal_fee_on_largest_input (largest input guaranteed >= min, post-reservation floor still enforced). Withdrawal therefore takes the full withdrawable (>=min) balance. Adds 3 unit tests. - Drop the dead [DeductFromInput(0)] the withdraw wrapper passed (the Rust AUTO branch owns its own largest-input fee strategy and ignores the caller's) and correct the now-stale doc/inline comments to describe the actual behavior. - Scope the withdraw source picker to accountType == 14 && keyClass == 0, matching the transfer sheet and what Rust (platform_payment_managed_account_at_index, key class 0) actually spends. - Filter the displayed source-account balance sum through account?.keyClass == 0 in both sheets so the total can't be inflated by non-zero-key-class addresses at the same accountIndex. - Reject embedded NULs in coreAddress at the withdraw wrapper before withCString, so a "valid\0suffix" string can't withdraw to a different address than the Swift value (CStr stops at the first NUL). platform-wallet lib tests 210 passed; clippy clean; build_ios.sh BUILD SUCCEEDED; SwiftExampleApp simulator build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
suggested changes
Jun 16, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Delta from 1bcedc7 to 5382a74 resolves all five prior findings: AUTO withdrawal now filters sub-min_input_amount dust with unit tests, the Swift withdraw wrapper passes empty fee strategy to match the Rust AUTO largest-balance selector, rejects embedded NULs in coreAddress, and Transfer/Withdraw pickers + displayed balances are scoped to accountType == 14 && keyClass == 0. Two new blocking issues introduced by the latest delta: the transfer autoChangeAddress picker is still missing the same keyClass == 0 scoping that was applied to the source picker, so change can be routed to a non-spendable sibling key-class row; and the successful-withdrawal path returns its PlatformAddressChangeSet without persisting it through self.persister, unlike every sibling platform-address flow (transfer, sync, fund_from_asset_lock).
🔴 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/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:364-375: Transfer `autoChangeAddress` misses the key-class-0 scoping just applied to the source picker
`platformAccountOptions` and `selectedSourceAccountCredits` were correctly tightened in this PR to `accountType == 14 && keyClass == 0`, matching the Rust transfer path's `platform_payment_managed_account_at_index(account_index)` (key class 0). `autoChangeAddress` (lines 364-375) still filters only by `walletId`, `accountIndex`, `!isUsed`, and `balance == 0` — no key-class filter. `PersistentAccount`'s unique constraint explicitly includes `keyClass`, so a sibling PlatformPayment account at the same `accountIndex` with `keyClass != 0` can legitimately exist; if such a sibling owns an unused, zero-balance row with a lower `addressIndex`, this view picks it as the change destination. Rust then spends from key-class-0 only and the change funds land on an address the production Transfer/Withdraw UI no longer surfaces in either balance or source picker — effectively stranding them outside the first-class wallet-signed flow. The fix mirrors what was already applied to the source picker.
In `packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs:171-174: Successful withdrawals do not persist the changeset like sibling platform-address flows
After updating the in-memory `ManagedPlatformAccount` and assembling a `PlatformAddressChangeSet`, this method returns `cs` directly without calling `self.persister.store(cs.clone().into())`. Every sibling platform-address flow does persist its post-submit changeset (`transfer.rs:314`, `fund_from_asset_lock.rs:300`, `sync.rs:159`) specifically so a restart does not hydrate spend paths from stale storage rows. Non-Swift callers — or any host where the SwiftData write side-channel is absent or fails — will restart with pre-withdrawal balances and may then construct invalid follow-up spends from credits that have already been withdrawn. Treat this as a missing persistence step on parity with the sibling flows, not as a Swift-only concern.
…ist withdrawal changeset - autoChangeAddress in the transfer sheet now filters account?.keyClass == 0, matching the source picker / balance sum tightened earlier in this PR. Without it, a sibling PlatformPayment account at the same accountIndex with keyClass != 0 could be picked as the change destination, but Rust routes change only through key class 0, stranding the change on an address the production UI no longer surfaces. - PlatformAddressWallet::withdraw now persists its changeset before returning, mirroring the sibling flows (transfer.rs:308-317, fund_from_asset_lock.rs:299-308, sync.rs:158-162): drop(wm) → if !cs.is_empty() → persister.store(cs.clone().into()) with tracing::error! log-on-error (the on-chain transition already succeeded). Without it, non-Swift callers or hosts without the SwiftData write side-channel would restart with pre-withdrawal balances and could build invalid follow-up spends. platform-wallet lib tests 210 passed; clippy clean; build_ios.sh BUILD SUCCEEDED; SwiftExampleApp simulator build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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 `@packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs`:
- Around line 429-431: The `fee_source_index as u16` cast in the DeductFromInput
construction can silently wrap when fee_source_index exceeds u16::MAX, causing
the wrong fee-source input to be selected. Add a guard check before constructing
the fee_strategy vector to ensure fee_source_index fits within the u16 range (0
to u16::MAX), and either return an error or handle the invalid case
appropriately if the value is out of bounds. This validation must occur before
the cast is performed in the AddressFundsFeeStrategyStep::DeductFromInput
construction.
🪄 Autofix (Beta)
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
Run ID: 7a5e032d-7979-4e62-88cc-9e6455b55cdf
📒 Files selected for processing (6)
packages/rs-platform-wallet-ffi/src/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift
- packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift
fee_source_index as u16 could silently wrap if a withdrawal ever selected more than u16::MAX inputs, targeting the wrong fee-source input. Use try_into and return a typed AddressOperation error instead. Defensive only (an account with >65535 funded addresses is not reachable in practice). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Prior findings: both prior blocking issues (autoChangeAddress keyClass scoping in TransferPlatformAddressView.swift, and post-broadcast persistence of the withdrawal changeset in withdrawal.rs) are FIXED at c1e291b — evidence verified directly. New findings: two cumulative API/UX correctness issues remain in the broader PR — the WithdrawPlatformAddressView accepts non-Fibonacci core_fee_per_byte values that the protocol's validate_structure deterministically rejects, and the new PlatformAddressFFI TryFrom path rejects address_type=1 even though the FFI struct and the new Swift TransferOutput API both advertise P2SH as a documented value. No new blocking issues. CodeRabbit produced no inline findings and its summary indicates processing in progress, so it is not treated as approval.
🟡 2 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/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:347-350: Withdraw sheet accepts fee rates the protocol always rejects
`parsedFeePerByte` accepts any integer 1–10,000 and `canSubmit` only requires that parse to succeed, but `AddressCreditWithdrawalTransitionV0::validate_structure` (packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_credit_withdrawal_transition/v0/state_transition_validation.rs:183) requires `core_fee_per_byte` to be a non-zero Fibonacci number — the sibling tests at lines 517-546 confirm `4` is rejected and `[1, 2, 3, 5, 8, 13, 21]` are accepted. Common manual rates like 4, 6, 10, or 100 enable the production Withdraw button and then deterministically fail structure validation at submit, so the new ADDR-04 sheet advertises invalid choices as valid (the help text even says "any whole number between 1 and 10,000"). Funds aren't at risk because the protocol rejects, but the example app misrepresents the protocol surface it's demonstrating.
In `packages/rs-platform-wallet-ffi/src/platform_address_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45: PlatformAddressFFI advertises P2SH but TryFrom rejects it
The FFI struct documents `address_type` as `0 = P2pkh, 1 = P2sh` (line 16), and `From<PlatformAddress>` emits `1` for P2sh (line 29). The new Swift `ManagedPlatformAddressWallet.TransferOutput.addressType` mirrors that contract (ManagedPlatformAddressWallet.swift:107 — "`0 = P2PKH`, `1 = P2SH`"). However the shared `TryFrom<PlatformAddressFFI>` used by `platform_address_wallet_transfer` (via `parse_outputs`, `parse_explicit_inputs`, `parse_explicit_inputs_with_nonces`) only accepts `0` and returns `"Unsupported address type"` for `1`. Sibling FFI surfaces in `identity_transfer.rs` and `identity_registration_with_signer.rs` accept both `0` and `1`. Not a memory-safety issue, but an ABI contract mismatch on this PR's newly-added transfer surface: a caller following the documented type byte hits an error long before the transition is built. Either accept `1` here, or narrow the doc and Swift API to `0` only.
…alid Fibonacci values DPP's AddressCreditWithdrawalTransitionV0::validate_structure requires core_fee_per_byte to be a non-zero Fibonacci number, but the Withdraw sheet accepted any 1–10,000 free-text value, so common entries (4, 6, 10, 100) enabled the button and then deterministically failed structure validation at submit. Replace the free-text fee field with an accessible picker offering only the valid non-zero Fibonacci rates <= the 10,000 app ceiling (1, 2, 3, 5, 8, 13, 21, 34, 55, 89, 144, 233, 377, 610, 987, 1597, 2584, 4181, 6765), default 1, so invalid rates are unselectable. Extract the rate set into a pure, testable WithdrawalCoreFeeRates helper (mirrors the validator's Fibonacci definition) with WithdrawalCoreFeeRatesTests (6 cases). Help text updated; single-flight guard, Core-not-ready gate, and the maxFeePerByte ceiling intact. Simulator build exit 0; WithdrawalCoreFeeRatesTests 6 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental verification at 8ab7c00. The latest delta (c1e291b..8ab7c00) is a single correct hardening — replaces an unchecked usize-to-u16 narrowing of the fee-source input index in reserve_withdrawal_fee_on_largest_input with a checked conversion — and introduces no new defects. Both carried-forward prior findings are STILL VALID: (1) the Swift Withdraw sheet enables submit for any UInt32 in 1..=10_000 while DPP requires core_fee_per_byte to be a non-zero Fibonacci number, and (2) PlatformAddressFFI advertises 1 = P2sh in its docstring and From emits 1, but TryFrom only accepts 0 and returns the generic 'Unsupported address type' for everything else. Both remain suggestions, not blockers. CodeRabbit is rate-limited; no inline CR findings to validate.
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/src/platform_address_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45: PlatformAddressFFI advertises P2SH (address_type=1) but TryFrom rejects it
Carried forward — still valid at 8ab7c009. The FFI contract is asymmetric:
- The struct docstring (line 16) documents `0 = P2pkh, 1 = P2sh`.
- `From<PlatformAddress>` (lines 22-35) emits `address_type: 1` for `PlatformAddress::P2sh(hash)`.
- The Swift surface mirrors that contract: `ManagedPlatformAddressWallet.TransferOutput.addressType` and `ChangeAddress.addressType` both document `0 = P2PKH, 1 = P2SH`, and `TransferPlatformAddressView` forwards the row's `addressType` verbatim.
- But `TryFrom<PlatformAddressFFI>` (lines 40-43) matches only `0` and returns `Err("Unsupported address type")` for any other discriminant.
Consequence: a Swift caller that constructs a transfer output, explicit input, or change address with `addressType = 1` — exactly what the published Rust/Swift contract invites — will fail inside `parse_outputs` / `parse_explicit_inputs` / `parse_explicit_inputs_with_nonces` with an opaque error after `Task.detached` + signer pinning. Today no production code path generates a P2SH platform address, so this is latent, but the new wallet-signed transfer/change surface area widens exposure to the mismatch, and `fundFromAssetLockPreflight` already has an explicit P2PKH-only preflight check that transfer/change do not.
Pick a direction and make the docs, types, and runtime agree:
1. Accept `1 => PlatformAddress::P2sh(ffi.hash)` so TryFrom honors the advertised contract and round-trips From's output, or
2. Narrow the Rust docstring + Swift addressType docs to P2PKH-only and reject `1` with a typed error code (and mirror the funding-path preflight on transfer outputs / change addresses).
…curate (not P2SH)
PlatformAddressFFI documented address_type 1 = P2SH and From<PlatformAddress>
emits it, but the shared TryFrom used by the transfer/withdraw parse paths
(parse_outputs / parse_explicit_inputs[_with_nonces]) rejects 1 — and rightly so:
the FFI signer returns an error for P2SH platform-address witnesses and the
KeychainSigner holds P2PKH key material only, so P2SH can't be signed/spent on
this surface, and the UI never produces P2SH outputs. The contract advertised a
value the surface can't honor.
Narrow the contract to match reality rather than relocate the failure:
- TryFrom<PlatformAddressFFI> now returns a specific, accurate message for P2SH
("...support P2PKH (address_type 0) only; P2SH cannot be signed or spent on this
surface") vs a generic message for other values; doc explains the
input-signing/output-practice rationale and the identity-sibling contrast (those
accept 1 because the address is a pure recipient signed by the identity key).
- PlatformAddressFFI + Swift TransferOutput/ChangeAddress/FundFromAssetLockRecipient
addressType docs changed from "0 = P2PKH, 1 = P2SH" to "Must be 0 (P2PKH)".
- Refresh stale error-string references in SendViewModel / fundFromAssetLockPreflight.
Runtime accept/reject behavior unchanged (P2SH was already rejected); only docs +
message text became accurate. Adds 2 FFI unit tests. platform-wallet-ffi 99 passed;
build_ios.sh BUILD SUCCEEDED; SwiftExampleApp simulator build exit 0.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Carried-forward prior findings: prior-1 (PlatformAddressFFI advertises P2SH via address_type=1 but TryFrom rejects it) is STILL VALID at 15cd3d2 — the file is unchanged. Resolved: prior-2 (free-form Core fee rate) is FIXED — WithdrawPlatformAddressView now binds a Picker to WithdrawalCoreFeeRates.rates(upTo:), which generates the non-zero Fibonacci sequence DPP's is_non_zero_fibonacci_number accepts, with unit tests pinning the offered set. No new in-scope defects in the 8ab7c00..15cd3d2 delta; new FFI surfaces (platform_address_wallet_withdraw_to_address, addresses_with_balances_for_account) follow the established handle/changeset/signer-vtable conventions and add a network-checked address parse on the Rust side.
🟡 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/rs-platform-wallet-ffi/src/platform_address_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/platform_address_types.rs:37-45: PlatformAddressFFI advertises P2SH (address_type=1) but TryFrom rejects it
Carried forward from the prior review — still valid at 15cd3d24; the file is untouched. The C-visible struct documents `0 = P2pkh, 1 = P2sh` (line 16) and `From<PlatformAddress> for PlatformAddressFFI` emits `address_type=1` for `PlatformAddress::P2sh` (lines 29-32). The Swift side mirrors that contract in `TransferOutput`/`ChangeAddress`/`FundFromAssetLockRecipient` doc-comments. But `TryFrom<PlatformAddressFFI> for PlatformAddress` only matches `0` and returns `"Unsupported address type"` for everything else (lines 40-42), so a P2SH value that round-trips out of Rust cannot be marshalled back in. Inbound boundary impact: `parse_outputs`, `parse_explicit_inputs`, and `parse_explicit_inputs_with_nonces` all funnel callers through `PlatformAddress::try_from(entry.address)`, so any future caller that follows the published `1 = P2sh` contract gets an opaque FFI error rather than a P2SH `PlatformAddress`. The PR's new `withdraw_to_address` does not go through this conversion (it builds the script_pubkey directly from `dashcore::Address`), so this is a latent ABI contract drift rather than a defect this PR introduces. Fix the asymmetry by either accepting `1 => P2sh` in `TryFrom` or narrowing the docstring + `From` impl to P2PKH only.
…nto Rust The Swift transfer wrapper was doing coin selection itself — fetching per-account balances, choosing which/how many inputs, building a separate change output to a fresh change address, and computing the fee index — then calling the FFI with INPUT_SELECTION_TYPE_EXPLICIT. That's exactly the "deciding which/how many/which index/which order" that swift-sdk/CLAUDE.md says must live in Rust. In the platform credit-balance model, InputSelection::Auto partial-spends inputs (the surplus simply stays on the source addresses), so no change output or change address is needed — the explicit-input + change-address ceremony was Bitcoin-UTXO habit. The transfer Auto path already filters dust (balance >= min_input_amount) and reserves fee headroom; it is the reference the withdrawal auto-selector mirrors. - Swift transfer(...) now calls platform_address_wallet_transfer with INPUT_SELECTION_TYPE_AUTO + accountIndex + recipient outputs + default fee strategy. Removes the Swift coin-selection loop, the changeAddress parameter and ChangeAddress struct, feeBuffer, and buildSortedFFIOutputs. This makes the fee-index misrouting bug class structurally impossible in Swift. - Deletes the Rust addresses_with_balances_for_account method and its FFI platform_address_wallet_addresses_with_balances_for_account (added only to feed the Swift selector). The no-account addresses_with_balances() stays (used by PlatformBalanceSyncService). - TransferPlatformAddressView drops autoChangeAddress + change-collision checks; keeps keyClass==0 source scoping, funded-source-input recipient exclusion, exact decimal parsing + overflow guards, single-flight, and persist+resync. - SendViewModel/SendTransactionView drop the changeAddress wiring. Deletes the now-dead ManagedPlatformAddressWalletTests (only covered buildSortedFFIOutputs; the fee-index regression is covered Rust-side in auto_select_tests). platform-wallet lib 210 passed, platform-wallet-ffi 99 passed; clippy clean; build_ios.sh BUILD SUCCEEDED (deleted symbol gone from header); SwiftExampleApp clean build + build-for-testing exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Verified HEAD 51f0c44. The 51f0c44 delta resolves prior-1 by deliberate design narrowing: the From/TryFrom asymmetry is preserved but now thoroughly documented at the struct, impl, Swift, and test level, with a P2SH-specific error message — classify INTENTIONALLY DEFERRED. One in-scope suggestion remains: the generic SendTransactionView's platform→platform branch picks senderAccountIndex and the change-address row without the keyClass==0 scoping that the new TransferPlatformAddressView/WithdrawPlatformAddressView sheets enforce and that the Rust platform_payment_managed_account_at_index path requires. Real but low-likelihood (only manifests for wallets with sibling PlatformPayment accounts sharing accountIndex across different key classes), and the dedicated sheets are the recommended production path — flagged as suggestion, not blocker.
Reviewed commit: 51f0c44
🟡 1 suggestion(s)
Verified findings
suggestion: Generic Send platform→platform picker doesn't scope source/change to key class 0
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift (line 235)
The platform→platform branch picks senderAccountIndex from the first PersistentPlatformAddress row with balance > 0 and selects the change row by accountIndex == senderAccountIndex only — neither filter requires account?.keyClass == 0. The Rust transfer path called by addressWallet.transfer(accountIndex:…) (SendViewModel.swift:524) resolves balances via platform_payment_managed_account_at_index (rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:265, 509), which is the key-class-0 account. The new dedicated sheets in this PR (TransferPlatformAddressView.swift:302, 314, 380; WithdrawPlatformAddressView mirrors it) already enforce accountType == 14 && keyClass == 0 for the source picker, source balance sum, and change picker for exactly this reason — PersistentAccount's unique constraint includes keyClass, so a sibling PlatformPayment account at the same accountIndex with a non-zero key class can legitimately own balance-bearing or unused rows. On such a wallet this screen would either enable a Platform send against a balance Rust will not spend, or route change to a non-key-class-0 address that Rust treats as external — stranding the change. Add the same account?.keyClass == 0 scoping here, or have this screen delegate the platform-address transfer to the dedicated Transfer sheet so there is one production picker, not two.
let senderAccountIndex = addressBalances
.filter { $0.account?.keyClass == 0 }
.first(where: { $0.balance > 0 })?
.accountIndex ?? 0
// Mirror ReceiveAddressView's selection:
// the lowest-indexed HD address that has
// never been used. Used as the change
// destination so the transition doesn't
// collide with any input address. Scoped
// to `senderAccountIndex` and `keyClass == 0`
// so multi-account wallets don't land change
// on a different platform-payment account /
// key class than the one Rust spends from
// (`platform_payment_managed_account_at_index`).
let changeAddressRow = addressBalances
.filter {
$0.accountIndex == senderAccountIndex
&& $0.account?.keyClass == 0
&& !$0.isUsed
&& $0.balance == 0
}
.min(by: { $0.addressIndex < $1.addressIndex })
_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3923 51f0c44
STDOUT:
STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 857, in
result = post_review(
File "/Users/cl), so I posted the same verified findings as a top-level review body._
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3923 +/- ##
============================================
+ Coverage 72.19% 72.48% +0.28%
============================================
Files 21 22 +1
Lines 2946 3020 +74
============================================
+ Hits 2127 2189 +62
- Misses 819 831 +12
🚀 New features to boost your workflow:
|
… key class 0 SendTransactionView's platform->platform branch picked senderAccountIndex from the first balance-bearing PersistentPlatformAddress without filtering on the parent account's key class. The Rust transfer path resolves the source via platform_payment_managed_account_at_index (key class 0), so on a wallet with a sibling non-key-class-0 PlatformPayment account at the same accountIndex this could derive an index whose key-class-0 account Rust spends from is empty. Add account?.keyClass == 0 to the selection, matching the dedicated Transfer/Withdraw sheets. (The change-address half of this finding is already gone — the prior refactor removed change-address selection entirely, since Auto leaves surplus on the source addresses.) SwiftExampleApp simulator clean build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift (1)
470-505:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t swallow SwiftData save failures after applying Rust balance updates.
Line 504 uses
try? modelContext.save(), which can hide persistence failures and still show a successful withdrawal. Bubble this error through the existingsubmit()catchso the UI doesn’t silently keep stale balances.Proposed fix
- persistUpdatedBalances(updated) + try persistUpdatedBalances(updated) await platformBalanceSyncService.performSync() didSucceed = true @@ - private func persistUpdatedBalances( + private func persistUpdatedBalances( _ updated: [ManagedPlatformAddressWallet.UpdatedBalance] - ) { + ) throws { @@ - try? modelContext.save() + try modelContext.save() }🤖 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/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift` around lines 470 - 505, The persistUpdatedBalances method is currently swallowing SwiftData save failures by using try? on the modelContext.save() call at the end of the function. To fix this, make persistUpdatedBalances a throwing function by adding throws to its method signature, then replace the try? modelContext.save() with try modelContext.save() so errors propagate. In the submit method where persistUpdatedBalances is called, update the call from persistUpdatedBalances(updated) to try persistUpdatedBalances(updated) so that any save errors bubble up to the existing catch block and properly set submitError instead of silently failing.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift (1)
539-557:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t report a sent transfer as failed when only cache persistence fails.
addressWallet.transfer(...)has already succeeded before this block. IfmodelContext.save()fails, showingerrormakes the user see a retryable failure after credits may already have moved; also scope the fetch towallet.walletIdlike the dedicated transfer sheet does.Proposed fix
+ let walletId = wallet.walletId for entry in updated { let entryHash = entry.hash let descriptor = FetchDescriptor<PersistentPlatformAddress>( - predicate: `#Predicate` { $0.addressHash == entryHash } + predicate: `#Predicate` { + $0.walletId == walletId && $0.addressHash == entryHash + } ) guard let row = try? modelContext.fetch(descriptor).first else { continue @@ do { try modelContext.save() } catch { - self.error = "Couldn't persist post-transfer balances: \(error.localizedDescription)" + successMessage = "Platform transfer submitted, but local balances could not be updated. Refresh before sending again: \(error.localizedDescription)" return }🤖 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/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift` around lines 539 - 557, The issue is that when modelContext.save() fails, the code sets self.error, which incorrectly reports the transfer as failed even though the actual transfer has already succeeded. Remove the error assignment in the catch block that handles the modelContext.save() failure in the loop over the updated entries. Additionally, scope the FetchDescriptor<PersistentPlatformAddress> predicate to include wallet.walletId alongside the addressHash comparison, consistent with how the dedicated transfer sheet performs similar queries.packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift (1)
353-397:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep own-wallet transfer recipients P2PKH-only.
External hashes are forced to
addressType = 0, but own-wallet rows forwardrow.addressType. A persisted P2SH row can therefore be selected and only fail after submit because the transfer FFI rejectsaddress_type = 1.Proposed fix
return allPlatformAddresses .filter { $0.walletId == wallet.walletId } + .filter { $0.addressType == 0 } .filter { !inputs.contains($0.addressHash) } .sorted { ($0.accountIndex, $0.addressIndex) < ($1.accountIndex, $1.addressIndex) } @@ guard let hash = selectedRecipientHash, let row = allPlatformAddresses.first(where: { $0.walletId == wallet.walletId && $0.addressHash == hash }) else { return nil } + guard row.addressType == 0 else { return nil } return (row.addressType, row.addressHash)🤖 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/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift` around lines 353 - 397, The `ownWalletRecipientCandidates` computed property and the `resolvedDestination` property's `.ownWallet` case do not enforce that only P2PKH addresses (addressType = 0) can be selected for own-wallet transfers. Add a filter to the `ownWalletRecipientCandidates` property to exclude any addresses where `addressType` is not 0 (P2PKH), ensuring that only P2PKH-type addresses are available as own-wallet transfer recipients and preventing the selection of invalid address types that would fail during the transfer FFI call.
🤖 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
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- Around line 231-245: The senderAccountIndex selection currently picks the
first account with any positive balance, but doesn't verify it has enough
balance to cover the requested transfer amount. When multiple key-class-0
PlatformPayment accounts exist, this can select an underfunded account while
another account has sufficient funds, causing the Rust layer to reject a valid
transfer. Modify the filter condition in the addressBalances chain where
senderAccountIndex is assigned to check that the selected account's balance is
greater than or equal to the requested transfer amount, not just greater than
zero. This ensures the selected account actually has sufficient funds to
complete the transaction.
---
Outside diff comments:
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swift`:
- Around line 539-557: The issue is that when modelContext.save() fails, the
code sets self.error, which incorrectly reports the transfer as failed even
though the actual transfer has already succeeded. Remove the error assignment in
the catch block that handles the modelContext.save() failure in the loop over
the updated entries. Additionally, scope the
FetchDescriptor<PersistentPlatformAddress> predicate to include wallet.walletId
alongside the addressHash comparison, consistent with how the dedicated transfer
sheet performs similar queries.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- Around line 353-397: The `ownWalletRecipientCandidates` computed property and
the `resolvedDestination` property's `.ownWallet` case do not enforce that only
P2PKH addresses (addressType = 0) can be selected for own-wallet transfers. Add
a filter to the `ownWalletRecipientCandidates` property to exclude any addresses
where `addressType` is not 0 (P2PKH), ensuring that only P2PKH-type addresses
are available as own-wallet transfer recipients and preventing the selection of
invalid address types that would fail during the transfer FFI call.
In
`@packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- Around line 470-505: The persistUpdatedBalances method is currently swallowing
SwiftData save failures by using try? on the modelContext.save() call at the end
of the function. To fix this, make persistUpdatedBalances a throwing function by
adding throws to its method signature, then replace the try? modelContext.save()
with try modelContext.save() so errors propagate. In the submit method where
persistUpdatedBalances is called, update the call from
persistUpdatedBalances(updated) to try persistUpdatedBalances(updated) so that
any save errors bubble up to the existing catch block and properly set
submitError instead of silently failing.
🪄 Autofix (Beta)
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
Run ID: c42b1e8d-d1fc-494a-92b0-bb4cee40552b
📒 Files selected for processing (12)
packages/rs-platform-wallet-ffi/src/platform_address_types.rspackages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rspackages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/ManagedPlatformAddressWallet.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/ViewModels/SendViewModel.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawalCoreFeeRates.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/ManagedPlatformAddressWalletTests.swiftpackages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/WithdrawalCoreFeeRatesTests.swiftpackages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
💤 Files with no reviewable changes (1)
- packages/swift-sdk/SwiftExampleApp/SwiftExampleAppTests/ManagedPlatformAddressWalletTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/swift-sdk/SwiftExampleApp/TEST_PLAN.md
- packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The latest delta (51f0c44..a62d1fa) cleanly delegates platform-address transfer input selection to the Rust Auto path: the Swift-side change-address picker, the changeAddressRow parameter, and the dead addresses_with_balances_for_account FFI helper are all removed in a way that matches the swift-sdk CLAUDE.md rule. Network validation on the new platform_address_wallet_withdraw_to_address entry point is Rust-side via require_network, the P2PKH/P2SH narrowing in TryFrom is pinned by tests, the signer is pinned across the FFI callback with withExtendedLifetime, and the auto-select fee-source picker reserves headroom on the largest input. One carried-forward suggestion remains on the legacy generic Send screen (source-account pick is not scoped to key class 0); the prior change-address half of that finding is fully resolved. No new latest-delta findings.
🟡 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:235-237: Carried forward (partial): generic Send source-account pick is not scoped to keyClass == 0
Carried forward from the prior PastaClaw finding. The change-address half was fully resolved in this delta (the Swift-side `changeAddressRow` picker and the `transfer(..., changeAddress:, ...)` overload were removed; surplus handling now lives entirely in the Rust Auto path). The source-account half is still present:
let senderAccountIndex = addressBalances
.first(where: { $0.balance > 0 })?
.accountIndex ?? 0
`addressBalances` is a `@Query<PersistentPlatformAddress>` filtered only by `walletId` (line 39-41), with no key-class filter. Across the FFI boundary Swift passes only `accountIndex`, and the Rust Auto path resolves the spend source via `platform_payment_managed_account_at_index(account_index)`, which is hardcoded to key class 0. If the first balance row belongs to a non-key-class-0 sibling sub-account whose key-class-0 sibling at the same `accountIndex` has zero balance, Rust will land on an account it considers empty and the user will see an opaque "insufficient" error after the Task detach + signer pin.
This is bounded — the new primary user-facing surfaces (`TransferPlatformAddressView.swift:305-325`, `WithdrawPlatformAddressView`) already filter on `accountType == 14 && keyClass == 0`, so this only affects the legacy generic Send screen — but the lines are part of this PR's net diff against master (introduced earlier in the PR stack), and the inconsistency with the two correct sister views is the right place to fix it.
…tal cache-save handling, P2PKH-only recipients
Address CodeRabbit review on the platform-address send/transfer/withdraw flows:
- SendTransactionView platform->platform now selects a key-class-0 PlatformPayment
account whose own balance covers amount + fee (Rust AUTO selects within one
account, not across), via a pure, unit-tested PlatformPaymentAccountSelection
helper; if no single account covers it, it aborts with a clear error instead of
silently handing Rust an underfunded account_index. (10 new tests.)
- Local cache-save failures after a SUCCESSFUL on-chain transfer/withdrawal are no
longer either silently swallowed (try?) or misreported as the op failing. The
op stays marked successful and a distinct non-fatal caveat is surfaced
("Submitted successfully, but local balances couldn't be updated — they'll
refresh on the next sync"), since performSync() runs immediately after and
reconciles. Applied consistently to SendViewModel, WithdrawPlatformAddressView,
and TransferPlatformAddressView. SendViewModel's post-transfer fetch is now
scoped to walletId + addressHash.
- TransferPlatformAddressView own-wallet recipient candidates and resolvedDestination
are now filtered to P2PKH (addressType == 0), matching the transfer FFI's
P2PKH-only contract, so a persisted P2SH row can't be selected and fail at submit.
SwiftExampleApp simulator clean build exit 0; new PlatformPaymentAccountSelectionTests 10/10.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Verified PR #3923 at head 10a3da7. The latest delta is a one-line fix (SendTransactionView.swift +9/-1) that addresses the prior carried-forward finding by scoping the generic Send source-account picker to account?.keyClass == 0 — prior-1 is FIXED. Cumulative review surfaces two in-scope suggestions: (1) an asymmetric account-scope filter in TransferPlatformAddressView's collision guard that can block legitimate destinations, and (2) the legacy raw-private-key forms remain compiled into normal navigation in TransitionCategoryView despite being labeled debug-only, which undercuts this PR's stated goal of moving users onto wallet-signed flows. No blocking issues.
🟡 2 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:340-351: Scope sourceInputHashes to keyClass == 0 to match the Rust spend account
`platformAccountOptions` (line 320) correctly filters source balance rows to `account?.keyClass == 0` because the Rust transfer path resolves the source account via `platform_payment_managed_account_at_index(accountIndex)` (key class 0). `sourceInputHashes` filters by `walletId` and `accountIndex` only, so a sibling non-key-class-0 row sharing the same `accountIndex` is treated as a source input and excluded as a destination — even though Rust would never select that row for this transfer. In wallets that have funded sibling rows at the same index across key classes, this blocks legitimate own-wallet/pasted-recipient destinations. Add the `account?.keyClass == 0` filter to align the UI collision guard with the spend account Rust actually uses.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionCategoryView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransitionCategoryView.swift:104-140: Gate raw private-key transfer/withdraw forms behind #if DEBUG
This PR's stated goal is to move ADDR-02/ADDR-04 users onto wallet-signed flows so they never paste raw platform-address private keys. The legacy `TransferAddressFundsView` and `WithdrawAddressFundsView` are still reachable from the normal address transition list with only UI text labeling them debug-only — in TestFlight or production builds a user can still be directed to paste a 64-character private key, putting spend key material into clipboard/text-field/process memory and bypassing the KeychainSigner account-scoped signing boundary the new flows enforce. Wrapping this `Section` in `#if DEBUG` keeps the raw-key escape hatch available for engineering while removing the key-material exposure surface from non-debug builds.
…, gate raw key forms to DEBUG - TransferPlatformAddressView.sourceInputHashes now also filters account?.keyClass == 0, matching platformAccountOptions and the Rust spend account (platform_payment_managed_account_at_index, key class 0). Without it a sibling non-key-class-0 row at the same accountIndex was treated as a source input and wrongly excluded as a destination on multi-key-class wallets. - Wrap the legacy raw private-key TransferAddressFundsView / WithdrawAddressFundsView NavigationLinks (TransitionCategoryView) in #if DEBUG so the 64-char-private-key paste surface is excluded from Release/TestFlight builds entirely, not just labeled debug-only. The production wallet-signed sheets remain off the WalletDetailView Platform Balance row. View definitions stay in AddressQueriesView; verified both Debug and Release simulator builds compile. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Two prior findings are carried forward as STILL VALID at c07e77a: prior-1 (sourceInputHashes scope mismatch in TransferPlatformAddressView) and prior-2 (raw private-key transfer/withdraw forms reachable in release builds — only label-demoted, not #if DEBUG-gated). One new latest-delta finding: the new resolvePlatformSenderAccountIndex helper in SendTransactionView counts the recipient row's balance when the recipient is an own-wallet address in the same key-class-0 PlatformPayment account, but the Rust Auto selector excludes recipient-collision rows from the input set, so the picker can choose an underfunded account Rust will reject. No blocking issues. Recommending COMMENT.
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:467-507: resolvePlatformSenderAccountIndex doesn't exclude the recipient address from per-account balance
`resolvePlatformSenderAccountIndex()` aggregates every key-class-0 PlatformPayment row's balance per `accountIndex`, then asks `PlatformPaymentAccountSelection.choose` for an account that covers `amount + fee`. It does not subtract any row whose `addressHash` matches the recipient.
The Rust Auto selector for platform→platform transfers excludes recipient addresses from the input set (DPP forbids an address being both input and output — the same invariant the dedicated `TransferPlatformAddressView` enforces via `sourceInputHashes`). When the user sends from a wallet whose recipient is an own-wallet address in the same key-class-0 account, this helper can pick an account whose remaining (recipient-excluded) balance falls short of `amount + fee`, and Rust then rejects the transfer the Send UI just enabled.
Fix: resolve the recipient's `addressHash` (when the recipient is on-wallet and at `accountType == 14`/`keyClass == 0`) and subtract any matching row's balance from that account's total before constructing `candidates` — mirroring the `sourceInputHashes` collision gate in `TransferPlatformAddressView`.
…heck; full accountType/keyClass parity - resolvePlatformSenderAccountIndex (generic Send screen) now excludes the recipient row from each key-class-0/type-14 PlatformPayment account's aggregate before PlatformPaymentAccountSelection.choose, mirroring the Rust Auto selector (which forbids an address being both input and output) and the dedicated sheet's sourceInputHashes gate. The recipient's 20-byte hash comes from a new read-only SendViewModel.platformRecipientHash that reuses the already-decoded detectedAddressType payload (no address decoding duplicated in the view). .platformToPlatform only; other flows unchanged. - sourceInputHashes (TransferPlatformAddressView) now also filters account?.accountType == 14, for full parity with platformAccountOptions and selectedSourceAccountCredits. SwiftExampleApp simulator clean build exit 0; PlatformPaymentAccountSelectionTests 10/10 and SendViewModelCoreRecipientsTests 9/9 still green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at e0f106b. Reconciliation of three prior findings: prior-1 (raw private-key entry points in TransitionCategoryView) is FIXED by the new #if DEBUG gate at lines 119-149; prior-3 (sourceInputHashes scope mismatch) is FIXED at TransferPlatformAddressView.swift:369 with the account?.keyClass == 0 predicate. Carried-forward: prior-2 (resolvePlatformSenderAccountIndex does not subtract recipient address from per-account totals) is STILL VALID and remains the only open finding — UX/availability impact on the generic Send screen, not a fund-safety or consensus issue. No new defects introduced in the latest delta.
🟡 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:467-507: Exclude recipient row before choosing platform sender account
`resolvePlatformSenderAccountIndex()` aggregates every key-class-0 / accountType==14 `addressBalances` row into per-account totals and then asks `PlatformPaymentAccountSelection.choose` for an account that covers `amount + fee`. When the pasted recipient is one of this wallet's funded PlatformPayment addresses on the chosen account, the Rust Auto selector (`platform_payment_managed_account_at_index(account_index)` → exclude-recipient input set, per DPP's `AddressFundsTransferTransition` forbidding the same address as both input and output) drops that input from the candidate set. The UI then enables submit on an account whose remaining spendable balance is below `amount + fee`, and the FFI call returns a typed insufficient-balance error post-signing.
Impact is bounded: no funds at risk, no consensus impact, no signing leak — the wallet-signed boundary still rejects correctly. But it is a regression-prone availability defect for the production Send screen wired up by this PR, and the dedicated `TransferPlatformAddressView` already gates this correctly via `sourceInputHashes` / `ownWalletRecipientCandidates`. The generic Send screen should follow the same pattern: if `viewModel.detectedAddressType` is a 21-byte platform address, derive its 20-byte hash and subtract any matching row from the corresponding account's total before calling `PlatformPaymentAccountSelection.choose`.
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Prior finding (resolvePlatformSenderAccountIndex recipient-row exclusion) is resolved at f808adf via the new SendViewModel.platformRecipientHash and the continue in per-account aggregation, matching the Rust Auto selector's input/output exclusion rule. New in the latest cumulative range: the new Transfer/Withdraw platform-address sheets show and gate on raw per-account credit totals that include sub-min_input_amount dust rows, while the Rust Auto/withdraw selectors drop those rows and return OnlyDustInputs — so canSubmit can be true for accounts Rust will refuse to spend. Two suggestions, no blockers.
🟡 2 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:318-343: Transfer source-account totals include sub-min_input_amount dust the Rust Auto selector will drop
`platformAccountOptions.totalCredits` (line 329-335) sums every key-class-0 / accountType-14 row's `balance`, and `canSubmit` gates on `selectedSourceAccountCredits` covering `amount + feeBuffer`. But the Rust Auto selector that this view delegates to filters out rows whose `balance < min_input_amount` (100,000 credits, see `packages/rs-platform-wallet/src/wallet/platform_addresses/transfer.rs:676-679` and `rs-platform-version/.../dpp_state_transition_versions/v*.rs:41`) and, when every candidate is sub-minimum, returns `PlatformWalletError::OnlyDustInputs`. When a source account's covering balance is composed of one or more dust rows plus a just-barely-non-dust row, this view enables Transfer for an amount the Rust path will then refuse with a dust/insufficient error. Mirror the Rust `balance >= min_input_amount` filter (and the recipient exclusion already applied in `sourceInputHashes`) inside `platformAccountOptions` so the displayed total and submit gate match the input set Rust will actually use. Pulling `min_input_amount` from `PlatformVersion` over FFI keeps the gate version-locked rather than hardcoded.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:346-397: Withdraw source-account totals and canSubmit ignore the Rust dust filter
`platformAccountOptions` (line 346-361) totals every key-class-0 row's `balance`, and `canSubmit` only requires `selectedSourceAccountCredits > 0`. The Rust withdraw Auto path (`packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs:294-333`) drops every row with `balance < min_input_amount` (100,000 credits) and returns `PlatformWalletError::OnlyDustInputs` when none survive — so an account whose only credits are dust shows a non-zero 'Total to Withdraw', enables Withdraw, and then fails Rust-side immediately. Apply the same `balance >= min_input_amount` filter to the per-account total used by the picker and `canSubmit`, so the button is only enabled when Rust will actually have a withdrawable input set. The same FFI-exposed `min_input_amount` the transfer view should read is enough; no need to duplicate the fee/min-withdrawal logic here — those keep returning typed Rust errors.
…ount (version-locked via FFI) The Transfer/Withdraw platform-address sheets summed every key-class-0 row into the per-account total and gated submit on it, but the Rust Auto/withdraw selectors drop rows with balance < min_input_amount (100,000 credits) and return OnlyDustInputs when none survive — so the UI could enable an op Rust then refuses. Expose the constant from Rust rather than hardcoding it in Swift (per swift-sdk/CLAUDE.md): new PlatformAddressWallet::min_input_amount reads sdk.version().dpp.state_transitions.address_funds.min_input_amount (the wallet's network-floored PlatformVersion), exposed via FFI platform_address_wallet_min_input_amount + a ManagedPlatformAddressWallet wrapper. Both sheets' platformAccountOptions now sum only rows with balance >= minInputAmount (resolved once per view on appear), so selectedSourceAccountCredits/canSubmit and the displayed totals match the input set Rust actually selects. Fallback when the constant can't be resolved is nil -> threshold UInt64.max (counts nothing) plus an explicit canSubmit guard, i.e. strictly non-under-gating; the view still renders. cargo: platform-wallet-ffi 99 passed, platform-wallet getter test passed, clippy clean; build_ios.sh BUILD SUCCEEDED (new symbol in header); SwiftExampleApp simulator clean build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Latest delta (9b5b7d7) fully resolves both prior findings (Transfer/Withdraw dust-filter divergence from Rust's min_input_amount selector) by exposing a version-locked FFI getter and gating Swift totals on it. No carried-forward prior findings. Three new suggestion-level findings remain in the same UI gating logic touched by this delta: a Core receive-address probe that mutates the address pool every time WithdrawPlatformAddressView opens, and two adjacent submit-gate gaps (transfer amount below DPP's min_output_amount, withdraw balance below the platform fee reservation) where Rust catches the error but the UI offers a deterministically invalid action.
🟡 3 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:462-476: checkCoreReady() probe mutates the Core receive-address pool
`checkCoreReady()` discards the result of `core.nextReceiveAddress(accountIndex: 0)`, treating it as a harmless readiness check, but that Swift call crosses FFI into `core_wallet_next_receive_address` → `CoreWallet::next_receive_address_for_account`, which calls `account.next_receive_address(Some(&xpub), true)` at `packages/rs-platform-wallet/src/wallet/core/wallet.rs:103`. The `true` flag advances the BIP-44 external address pool, so every time this sheet opens, one receive address is burned without ever being shown to the user. `resolveMyWalletAddress()` then calls the same FFI again when `myWalletAddress == nil`, so the first sheet open consumes two addresses. Repeated open/cancel cycles keep skipping forward. Either reuse the probed value as the destination (cache it in `myWalletAddress`), or replace the probe with a non-mutating Core-readiness check.
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:423-443: Withdraw submit gate doesn't reserve the platform withdrawal fee
`canSubmit` enables withdraw whenever the dust-filtered account total is > 0, but the Rust AUTO withdrawal path runs `reserve_withdrawal_fee_on_largest_input` and rejects with `InsufficientBalanceForFee`/`OnlyDustInputs` when the largest input can't cover the base fee (≈400,000,000 credits) plus per-input cost (≈500,000) and still leave the minimum withdrawal amount. So an account with a single 100,000-credit input clears the dust gate but fails deterministically at signing. Either expose the relevant fee/minimum constants via FFI (mirroring the `min_input_amount` getter) and gate on `largestInput >= baseFee + perInputFee + minOutputAmount`, or add a Rust preflight that returns whether the account can fund a withdrawal at the current fee schedule. The current gate offers an action Rust can already prove impossible.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/TransferPlatformAddressView.swift:520-547: Transfer submit gate doesn't check the per-output minimum
`canSubmit` enables submit on `parsedCredits > 0`, but address-funds transfers send exactly one output and DPP rejects any output below `platform_version.dpp.state_transitions.address_funds.min_output_amount` (currently 500,000 credits in v1/v2/v3). A user can type a small positive amount (e.g. 100 credits), the button enables, and the protocol returns a typed rejection only after submission. Since the FFI getter pattern for `min_input_amount` is already in place, exposing `min_output_amount` the same way (and adding `credits >= minOutputAmount` to the gate) would close this UX hole using the same version-locked source the validator uses.
Comment on lines
+423
to
+443
| private var canSubmit: Bool { | ||
| guard | ||
| !isSubmitting, | ||
| coreReady == true, | ||
| // The per-input minimum must be known before we can promise the | ||
| // account has anything withdrawable: `selectedSourceAccountCredits` | ||
| // sums only balances ≥ this floor, and an unresolved floor makes | ||
| // that figure 0. The `> 0` check below already closes the gate in | ||
| // that case; this makes the dependency explicit. | ||
| minInputAmount != nil, | ||
| sourceAccountIndex != nil, | ||
| // Require the dust-FILTERED (withdrawable) total > 0, not the raw | ||
| // total: the Rust selector returns `OnlyDustInputs` when no | ||
| // address clears `min_input_amount`, so a purely-dust account | ||
| // (raw balance > 0) must not enable the button. | ||
| selectedSourceAccountCredits > 0, | ||
| parsedFeePerByte != nil, | ||
| let addr = resolvedCoreAddress, !addr.isEmpty | ||
| else { return false } | ||
| return true | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Withdraw submit gate doesn't reserve the platform withdrawal fee
canSubmit enables withdraw whenever the dust-filtered account total is > 0, but the Rust AUTO withdrawal path runs reserve_withdrawal_fee_on_largest_input and rejects with InsufficientBalanceForFee/OnlyDustInputs when the largest input can't cover the base fee (≈400,000,000 credits) plus per-input cost (≈500,000) and still leave the minimum withdrawal amount. So an account with a single 100,000-credit input clears the dust gate but fails deterministically at signing. Either expose the relevant fee/minimum constants via FFI (mirroring the min_input_amount getter) and gate on largestInput >= baseFee + perInputFee + minOutputAmount, or add a Rust preflight that returns whether the account can fund a withdrawal at the current fee schedule. The current gate offers an action Rust can already prove impossible.
source: ['codex']
…ate transfer on min_output_amount - WithdrawPlatformAddressView.checkCoreReady() no longer probes nextReceiveAddress(advance: true) and discards it (which advanced the BIP-44 external pool and burned a receive address on every sheet open — two on first open via resolveMyWalletAddress). Readiness now uses non-mutating coreWallet() + network(); the My-Wallet receive address is resolved at most once and cached, so open/cancel/toggle never churns the pool. (No non-mutating account-present FFI exists; documented as a possible follow-up.) - Transfer submit gate now enforces DPP's min_output_amount (500,000 credits): new version-locked PlatformAddressWallet::min_output_amount + FFI platform_address_wallet_min_output_amount + ManagedPlatformAddressWallet .minOutputAmount(), mirroring the min_input_amount getter. canSubmit requires credits >= minOutputAmount with a clear footer; nil keeps the gate closed. platform-wallet lib 212 passed (+ min_output getter test), platform-wallet-ffi 99 passed; clippy clean; build_ios.sh BUILD SUCCEEDED (new symbol in header); SwiftExampleApp simulator clean build exit 0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Verified at head 91e7c4b. Latest delta cleanly resolves prior-1 (checkCoreReady now uses non-mutating core.network() instead of nextReceiveAddress) and prior-2 (TransferPlatformAddressView.canSubmit now requires credits >= minOutputAmount, sourced from a new version-locked FFI getter platform_address_wallet_min_output_amount that mirrors the min_input_amount pattern). Prior-3 (Withdraw submit gate doesn't preflight the platform withdrawal fee/min output) is STILL VALID and carried forward — canSubmit still enables submit whenever the dust-filtered total is > 0, so an account with a small-but-non-dust input that cannot cover base fee + per-input fee + min withdrawal will still light the button and fail deterministically in Rust's reserve_withdrawal_fee_on_largest_input. No new in-scope defects in the latest delta; the generic Send platform-to-platform gap raised by Codex is real but pre-existing and adjacent to this PR's scope.
🟡 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/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift`:
- [SUGGESTION] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Views/WithdrawPlatformAddressView.swift:423-443: Withdraw submit gate doesn't preflight the Rust fee-reservation path
`canSubmit` enables withdrawal whenever `selectedSourceAccountCredits > 0` (the dust-filtered total) and `minInputAmount != nil`. The Rust AUTO withdrawal path in `packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs` then runs `reserve_withdrawal_fee_on_largest_input`, which rejects when (a) the largest input minus the estimated address-credit-withdrawal fee falls below `min_input_amount`, or (b) the aggregate minus fee falls below `system_limits.min_withdrawal_amount`. An account with a single input that clears `min_input_amount` but is below `min_input_amount + estimated_fee`, or whose aggregate cannot clear `min_withdrawal_amount` after fee, will therefore light the Withdraw button and then fail deterministically after sign. The clean fix mirrors the `min_output_amount` getter pattern just introduced in this PR: expose either the version-locked withdrawal fee/min constants over FFI, or a single Rust preflight (`can_withdraw(account_index) -> Result<...>`) that runs the same input-selection + fee-reservation logic dry. Per packages/swift-sdk/CLAUDE.md, prefer the preflight FFI so the decision stays Rust-owned and doesn't need a Swift mirror that would drift across protocol versions.
Comment on lines
+423
to
+443
| private var canSubmit: Bool { | ||
| guard | ||
| !isSubmitting, | ||
| coreReady == true, | ||
| // The per-input minimum must be known before we can promise the | ||
| // account has anything withdrawable: `selectedSourceAccountCredits` | ||
| // sums only balances ≥ this floor, and an unresolved floor makes | ||
| // that figure 0. The `> 0` check below already closes the gate in | ||
| // that case; this makes the dependency explicit. | ||
| minInputAmount != nil, | ||
| sourceAccountIndex != nil, | ||
| // Require the dust-FILTERED (withdrawable) total > 0, not the raw | ||
| // total: the Rust selector returns `OnlyDustInputs` when no | ||
| // address clears `min_input_amount`, so a purely-dust account | ||
| // (raw balance > 0) must not enable the button. | ||
| selectedSourceAccountCredits > 0, | ||
| parsedFeePerByte != nil, | ||
| let addr = resolvedCoreAddress, !addr.isEmpty | ||
| else { return false } | ||
| return true | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Withdraw submit gate doesn't preflight the Rust fee-reservation path
canSubmit enables withdrawal whenever selectedSourceAccountCredits > 0 (the dust-filtered total) and minInputAmount != nil. The Rust AUTO withdrawal path in packages/rs-platform-wallet/src/wallet/platform_addresses/withdrawal.rs then runs reserve_withdrawal_fee_on_largest_input, which rejects when (a) the largest input minus the estimated address-credit-withdrawal fee falls below min_input_amount, or (b) the aggregate minus fee falls below system_limits.min_withdrawal_amount. An account with a single input that clears min_input_amount but is below min_input_amount + estimated_fee, or whose aggregate cannot clear min_withdrawal_amount after fee, will therefore light the Withdraw button and then fail deterministically after sign. The clean fix mirrors the min_output_amount getter pattern just introduced in this PR: expose either the version-locked withdrawal fee/min constants over FFI, or a single Rust preflight (can_withdraw(account_index) -> Result<...>) that runs the same input-selection + fee-reservation logic dry. Per packages/swift-sdk/CLAUDE.md, prefer the preflight FFI so the decision stays Rust-owned and doesn't need a Swift mirror that would drift across protocol versions.
source: ['claude', 'codex']
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
2 participants
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.
Issue being fixed or feature implemented
Two SwiftExampleApp platform-address (DIP-17) actions still required pasting a raw 64-char private key into demo forms:
This promotes both to first-class, wallet-signed production UI (the same pattern as ADDR-03 / ID-04 / ID-10 / ID-12), so a user never handles key material. Per
packages/swift-sdk/CLAUDE.md, key derivation and signing stay in theplatform-walletcrate behindrs-platform-wallet-ffi, driven by aKeychainSigner; Swift only selects source account / amount / destination and calls one entry point.What was done?
ADDR-02 (transfer) — UI only. The wallet-signed stack (
PlatformAddressWallet::transfer→ FFIplatform_address_wallet_transfer→ManagedPlatformAddressWallet.transfer) already existed.TransferPlatformAddressView: DIP-17 account picker (accountType == 14) showing credit balances; destination via own-wallet picker or pasted 20-byte P2PKH hash; amount field.nonce: 0of the raw form).amount + feeBuffer ≤ balanceand recipient ≠ change address; single-flight guard against double-submit; triggers a DIP-17 resync on success.ADDR-04 (withdraw) — small Rust/FFI add + Swift wrapper + UI.
platform_address_wallet_withdraw_to_address(...)that accepts a base58 Core address, network-checks it Rust-side viarequire_network(wallet.network())(precedent:core_wallet/broadcast.rs), builds thescript_pubkey, and delegates to the existingwallet.withdraw(...). Includes a#[cfg(test)]unit test for the network check.ManagedPlatformAddressWallet.withdraw(accountIndex:coreAddress:coreFeePerByte:signer:)wrapper usingINPUT_SELECTION_TYPE_AUTO+DeductFromInput(0)(withdrawals consume the full funded balance — no change output), reusing theTask.detached+withExtendedLifetime(signer)pattern fromfundFromAssetLock.WithdrawPlatformAddressView: source picker; Core L1 destination toggle (my-wallet viacore_wallet_next_receive_addressvs external paste);coreFeePerByte(default 1); shows computed full-balance total; gated on the Core wallet being initialized with a clear "Core not ready" state; single-flight guard; resync on success.Wiring & docs.
WalletDetailView: the Platform Balance row gains a Top Up / Transfer / Withdraw menu presenting both new sheets.TransitionCategoryView: the legacy raw private-keyTransferAddressFundsView/WithdrawAddressFundsViewlinks moved into a 🧪 debug-only section.TEST_PLAN.md: ADDR-02 / ADDR-04 flipped to production status.New FFI signature
How Has This Been Tested?
cargo fmt --all -- --checkclean.cargo check -p platform-wallet-ffipasses; new unit testwithdraw_address_network_check_rejects_wrong_networkpasses (testnet address accepted byrequire_network(Testnet), rejected byrequire_network(Mainnet)).packages/swift-sdk/build_ios.sh→ BUILD SUCCEEDED; the new symbol is present in the regenerated cbindgen header.xcodebuild -project SwiftExampleApp/SwiftExampleApp.xcodeproj -scheme SwiftExampleApp -sdk iphonesimulator -destination 'platform=iOS Simulator,name=iPhone 16,arch=arm64' -quiet clean build→ exit 0, no errors.Not yet runtime-verified: no funded testnet platform account was available in this session, so the sheets were not driven end-to-end on a booted simulator. Manual check remaining: Platform Balance ⋯ → Transfer / Withdraw → pickers populate from a synced DIP-17 account → submit → confirm balances + unused-address pool catch up after the auto-resync.
Breaking Changes
None. This is not a consensus change and introduces no breaking API changes (additive FFI symbol + new Swift wrapper/views).
Checklist:
Summary by CodeRabbit
minInputAmountto support consistent client-side validation.address_typeerrors.