feat(platform-wallet): load watch-only wallet state from local storage at startup#3692
feat(platform-wallet): load watch-only wallet state from local storage at startup#3692Claudius-Maginificent wants to merge 80 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR introduces seedless watch-only wallet rehydration, returns per-row load outcomes with skip tracking, exposes skipped-wallet results through FFI/Swift, and updates related identity replay, core-bridge warnings, and signer test stubs. ChangesWatch-only rehydration and load outcomes
Core bridge UTXO warnings
Mnemonic signer and test stubs
Sequence Diagram(s)sequenceDiagram
participant FFIPersister
participant PlatformWalletManager
participant PlatformEventManager
participant SwiftPlatformWalletManager
FFIPersister->>PlatformWalletManager: load() returns ClientStartState
PlatformWalletManager->>PlatformEventManager: on_wallet_skipped_on_load(wallet_id, reason)
PlatformWalletManager->>SwiftPlatformWalletManager: return LoadOutcome / skipped wallets
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b573fca to
b7508a0
Compare
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-05-22T10:49:18.857Z |
🔁 Seedless-load rework landed —
|
0e92cb4 to
f57b117
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3692 +/- ##
=========================================
Coverage 87.21% 87.21%
=========================================
Files 2642 2642
Lines 328287 328287
=========================================
Hits 286330 286330
Misses 41957 41957
🚀 New features to boost your workflow:
|
|
@coderabbitai review all |
|
✅ Review complete (commit f8ee551) |
|
✅ Action performedFull review finished. |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
I verified the reported wallet rehydration findings against the PR head and surrounding call paths. Three correctness issues are in scope for this restore/storage work: the SQLite account-registration key is lossy, repeated loads collide with already-registered wallets, and the FFI load path still turns malformed account xpub rows into whole-load failures before the manager can skip them per wallet.
🔴 6 blocking | 🟡 1 suggestion(s)
Findings not posted inline (3)
These findings could not be anchored to the current diff, but they are still part of this review.
- [BLOCKING]
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants — The writer upsertsaccount_registrationson(wallet_id, account_type, account_index), and the migration declares the same primary key. That key is narrower thankey_wallet's account identity:PlatformPaymentis keyed by(account, key_class), and DashPay receiving/external accounts are k... - [BLOCKING]
packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager —load_from_persistoralways callswm.insert_wallet(wallet, platform_info)for every persisted wallet. The underlyingkey-wallet-managerimplementation returnsWalletExistswhen the wallet id is already registered, and this code maps that to a hardWalletCreationerror that aborts the loa... - [BLOCKING]
packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore — The FFI persister builds eachWalletRestoreEntryFFIwithbuild_wallet_start_state(entry)?before returningClientStartStatetoPlatformWalletManager::load_from_persistor. Inside that helper, malformedaccount_xpub_bytesreturnsPersistenceErrorwhile decoding the account xpub, so one...
🤖 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-storage/src/sqlite/schema/accounts.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants
The writer upserts `account_registrations` on `(wallet_id, account_type, account_index)`, and the migration declares the same primary key. That key is narrower than `key_wallet`'s account identity: `PlatformPayment` is keyed by `(account, key_class)`, and DashPay receiving/external accounts are keyed by `(index, user_identity_id, friend_identity_id)`. The blob stores the full `AccountRegistrationEntry`, but inserting two such variants with the same label and numeric index overwrites the first row before `load_state` can reconstruct the manifest. A restored watch-only wallet can therefore lose accounts and their address/platform state. Persist enough discriminator columns, or an unambiguous serialized account-type key, and include it in the conflict key/order.
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs:107-111: Account registration primary key collapses distinct account variants
The writer upserts `account_registrations` on `(wallet_id, account_type, account_index)`, and the migration declares the same primary key. That key is narrower than `key_wallet`'s account identity: `PlatformPayment` is keyed by `(account, key_class)`, and DashPay receiving/external accounts are keyed by `(index, user_identity_id, friend_identity_id)`. The blob stores the full `AccountRegistrationEntry`, but inserting two such variants with the same label and numeric index overwrites the first row before `load_state` can reconstruct the manifest. A restored watch-only wallet can therefore lose accounts and their address/platform state. Persist enough discriminator columns, or an unambiguous serialized account-type key, and include it in the conflict key/order.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager
`load_from_persistor` always calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet. The underlying `key-wallet-manager` implementation returns `WalletExists` when the wallet id is already registered, and this code maps that to a hard `WalletCreation` error that aborts the load batch. A second `load_from_persistor` call on the same manager, or a restore after the same wallet has already been registered in memory, fails instead of treating the persisted wallet as already satisfied. Handle `WalletExists` or pre-check the existing wallet id so repeated restore is idempotent and still proceeds to the remaining persisted wallets.
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the manager
`load_from_persistor` always calls `wm.insert_wallet(wallet, platform_info)` for every persisted wallet. The underlying `key-wallet-manager` implementation returns `WalletExists` when the wallet id is already registered, and this code maps that to a hard `WalletCreation` error that aborts the load batch. A second `load_from_persistor` call on the same manager, or a restore after the same wallet has already been registered in memory, fails instead of treating the persisted wallet as already satisfied. Handle `WalletExists` or pre-check the existing wallet id so repeated restore is idempotent and still proceeds to the remaining persisted wallets.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
The FFI persister builds each `WalletRestoreEntryFFI` with `build_wallet_start_state(entry)?` before returning `ClientStartState` to `PlatformWalletManager::load_from_persistor`. Inside that helper, malformed `account_xpub_bytes` returns `PersistenceError` while decoding the account xpub, so one corrupt SwiftData account row makes the entire load callback fail. That bypasses the new per-wallet skip contract documented on the manager and FFI surfaces, and prevents otherwise valid persisted wallets from loading. Convert per-entry account decode failures into a skipped wallet entry or an equivalent row-local corruption marker that the manager can report through `LoadOutcome::skipped`.
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1543-1545: Malformed FFI account xpub aborts the whole restore
The FFI persister builds each `WalletRestoreEntryFFI` with `build_wallet_start_state(entry)?` before returning `ClientStartState` to `PlatformWalletManager::load_from_persistor`. Inside that helper, malformed `account_xpub_bytes` returns `PersistenceError` while decoding the account xpub, so one corrupt SwiftData account row makes the entire load callback fail. That bypasses the new per-wallet skip contract documented on the manager and FFI surfaces, and prevents otherwise valid persisted wallets from loading. Convert per-entry account decode failures into a skipped wallet entry or an equivalent row-local corruption marker that the manager can report through `LoadOutcome::skipped`.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API
`pub mod rehydrate` exposes `manager::rehydrate::apply_persisted_core_state` to downstream crates even though it mutates `ManagedWalletInfo` according to load-path-specific assumptions such as first-funds-account UTXO routing and bounded address-pool probing. The only external uses are storage integration tests, so making this part of the public SDK API hardens internal restore details that should be free to change. Keep the module/function crate-private and move those cross-crate assertions behind an internal test feature or into `rs-platform-wallet` tests.
…ecurse nonce extractor QA-001 (HIGH): the AddressNonceMismatch variant added at the wallet layer had no FFI mapping, so it both regressed and under-delivered at the C boundary hosts actually consume. - Regression: a shield nonce-rejection that used to surface as `ErrorShieldedBroadcastFailed` (code 16 — definitively failed, notes released, safe to retry) fell through `map_spend_result`'s catch-all to `ErrorWalletOperation` (code 6), dropping the retry-safety contract. - Fix: add a dedicated `ErrorAddressNonceMismatch = 21` result code (an additive enum slot) carrying the SAME definitively-failed / notes- released / safe-to-retry contract, and map to it in BOTH `map_spend_result` (covers shield/unshield/transfer/withdraw) AND the blanket `From<PlatformWalletError>` (covers identity `top_up_from_addresses`, which propagates via `?`/`.into()`). Hosts can now recognize the self- healing nonce race and retry — the retry re-fetches the address nonce. Nonce values are NOT exposed as structured out-fields: the shared `PlatformWalletFFIResult` is returned by value and mirrored by every language binding, so adding fields (or send-function out-params) would be an ABI-breaking change for all consumers. The submitted/expected nonces still travel in the result `message` (typed Display), and an FFI retry re-fetches the nonce anyway, so the practical loss is nil. QA-002 (LOW): `as_address_invalid_nonce` now recurses through `dash_sdk::Error::NoAvailableAddressesToRetry(inner)`, matching its sibling predicate `broadcast_definitely_failed` so the two stay in lockstep (latent today — the retry envelope never wraps a ConsensusError yet). Tests: AddressNonceMismatch -> ErrorAddressNonceMismatch via both the blanket From and map_spend_result; extractor unwraps the retry envelope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… test assertions QA-R2 #1 (MEDIUM): the Swift host binding — the only in-repo host — stopped at result code 20, so the new ErrorAddressNonceMismatch (=21) collapsed to `.errorUnknown` → `.unknown`, and the typed nonce-race fix never reached the shipped host. Mirror it in PlatformWalletResult.swift: add `case errorAddressNonceMismatch = 21` to `PlatformWalletResultCode`, its `init(ffi:)` arm (matching the cbindgen constant `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_ADDRESS_NONCE_MISMATCH`), a dedicated `PlatformWalletError.addressNonceMismatch(String)` case, and the `init(result:)` mapping — mirroring `errorShieldedBroadcastFailed`'s definitively-failed / notes-released / safe-to-retry semantics. Swift compilation was NOT verified: the cbindgen-generated header + DashSDKFFI module are produced by build_ios.sh (iOS toolchain, unavailable here); the change is pattern-exact against the existing arms and the `prefix_with_name + ScreamingSnakeCase` cbindgen config. QA-R2 #2 (LOW): the two new nonce-value FFI tests asserted bare digits (`contains('1') && contains('2')`), which a provided/expected transposition would pass. Pin the exact rendered substrings instead (`submitted nonce 1` / `Platform expected 2`) so a transposition fails. Per the ABI decision (keep dedicated-code-only), document at the blanket `From` mapping arm that structured provided/expected nonce out-fields are intentionally out of scope — PlatformWalletFFIResult is by-value/ABI-frozen; the values travel in the message and an FFI retry re-fetches the nonce. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Source: reviewers: Codex gpt-5.5 general, rust-quality, and ffi-engineer completed; Claude/Opus general, rust-quality, and ffi-engineer failed at ACP startup due extra-usage exhaustion (reset Jul 10, 8am America/Chicago). Verifier: Codex gpt-5.5; Claude/Opus verifier failed for the same quota reason.
Carried-forward prior-1 is STILL_VALID at 069ee6a7935f44801d3b78d899ef07444d34205a: Rust preserves transient/fatal persister-load classification, but the FFI/Swift boundary still flattens that path to an unknown wallet error. I found no new in-scope findings in the latest delta from d7c684607e2a58fa67c24fbfcb80a5753b909037 to 069ee6a7935f44801d3b78d899ef07444d34205a.
Prior Finding Reconciliation:
- STILL VALID:
packages/rs-platform-wallet-ffi/src/error.rs:258-313still has noPersisterLoad(_)arm, so the typed Rust load-path retry classification falls through toErrorUnknownbefore Swift receives it.
Carried-Forward Prior Findings:
- suggestion: Preserve persister-load retry classification across FFI.
New Findings In Latest Delta:
- None.
1 suggestion
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/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:258-313: Preserve persister-load retry classification across FFI
`PlatformWalletError::PersisterLoad` now wraps the typed `PersistenceError`, and the Rust load tests explicitly require transient and fatal classifications to survive `load_from_persistor`. The FFI entry point calls `manager.load_from_persistor()` and returns hard load failures through `unwrap_result_or_return!(result)`, which uses this blanket `From<PlatformWalletError>` conversion. Because the match has no `PersisterLoad(_)` arm, both transient storage contention and permanent persisted-state failures become `ErrorUnknown` plus a display string; Swift then maps that to `.unknown(String)`, so launch callers cannot reliably decide whether to retry without parsing text. Add dedicated persister-load result codes, or another structured ABI contract that carries retryability, and mirror it in Swift.
There was a problem hiding this comment.
Code Review
Source: reviewers: Codex gpt-5.5 general, rust-quality, and ffi-engineer completed; Claude/Opus general, rust-quality, and ffi-engineer failed at ACP startup due extra-usage exhaustion (reset Jul 10, 8am America/Chicago). Verifier: Codex gpt-5.5; Claude/Opus verifier failed for the same quota reason. Specialists: rust-quality, ffi-engineer.
Incremental + cumulative review at 6103ab6b862e7ac336d88da09f1d425d4fdf9639, reconciling prior review 069ee6a7935f44801d3b78d899ef07444d34205a.
Prior Finding Reconciliation:
- STILL VALID:
packages/rs-platform-wallet-ffi/src/error.rs:258-313still has noPersisterLoad(_)arm, so the typed Rust load-path retry/fatal classification falls through toErrorUnknownbefore Swift receives it.
Carried-Forward Prior Findings:
- suggestion: Preserve persister-load retry classification across FFI.
New Findings In Latest Delta:
- blocking: Swift integration tests still call the removed
ManagedCoreWallet.sendToAddressesAPI after the split core send API landed. - blocking: The example app now passes a Platform Payment account index into BIP44 Core funding/signing for non-platform sends.
- suggestion: The split transaction-builder FFI path exposes same-account funding selection and reservation as separate calls, so callers can race and select the same UTXO before either builder reserves it.
2 blocking | 2 suggestions
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/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift`:
- [BLOCKING] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift:35-37: Update integration tests for the split core send API
`ManagedCoreWallet.sendToAddresses` was removed in this delta and replaced by `CoreTransactionBuilder` plus `broadcastTransaction(_:)`, but this SwiftPM integration test target still calls the old method here. The same stale call remains in `SpvRestartIntegrationTests.swift` and `PersisterRestartClassificationIntegrationTests.swift`, and `Package.swift` declares `SwiftDashSDKIntegrationTests` as a real test target, so `swift test`/Xcode test builds fail with `ManagedCoreWallet` having no member `sendToAddresses`. Update these tests to build/sign with `CoreTransactionBuilder` and broadcast the resulting transaction, or restore a compatibility wrapper.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282: Do not use Platform Payment account indexes for Core sends
The non-`platformToPlatform` fallback still derives `senderAccountIndex` from BLAST Platform Payment address rows, and the comment says those flows ignore it. That is no longer true: the `.coreToCore` path now passes this value into `CoreTransactionBuilder.setFunding(... accountType: .bip44 ...)` and `buildSigned(... accountType: .bip44 ...)`. A normal Core payment can therefore try to spend BIP44 account N where N came from an unrelated Platform Payment account, either failing with a missing BIP44 account or spending the wrong Core account if that BIP44 index exists. Keep the Platform Payment account picker scoped to `platformToPlatform`, and choose a BIP44 funding account from Core account state for `.coreToCore` or preserve the previous default of account 0.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:258-313: Preserve persister-load retry classification across FFI
`PlatformWalletError::PersisterLoad` wraps the typed `PersistenceError`, and the Rust load tests require transient and fatal classifications to survive `load_from_persistor`. The FFI entry point calls `manager.load_from_persistor()` and returns hard load failures through `unwrap_result_or_return!(result)`, which uses this blanket `From<PlatformWalletError>` conversion. Because the match has no `PersisterLoad(_)` arm, both transient storage contention and permanent persisted-state failures become `ErrorUnknown` plus a display string; Swift then maps that to `.unknown(String)`, so launch callers cannot reliably decide whether to retry without parsing text. Add dedicated persister-load result codes, or another structured ABI contract that carries retryability, and mirror it in Swift.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:354-400: Keep same-account funding selection and reservation atomic
`core_wallet_tx_builder_set_funding` selects and snapshots spendable UTXOs into a long-lived builder, but the reservation is not taken until `core_wallet_tx_builder_build_signed` later calls `build_signed`. The Rust comment above this function acknowledges the gap: because the wallet lock is released between the FFI calls, two concurrent builders for the same account can both select the same free UTXO before either reserves it. That makes the new public split-builder API rely on foreign callers to serialize same-account sends, while the Swift wrapper does not expose or enforce that contract. Move funding selection into the signed-build step under one wallet/account critical section, or add a per-account send mutex/structured API so selection and reservation remain atomic for FFI callers.
… branch Points the 8 rust-dashcore workspace deps at dashpay/rust-dashcore#850 (feat/key-wallet-asset-lock-explicit-utxo-override @ d42081e7), which adds an explicit-UTXO-override escape hatch for asset-lock funding. Updates the one build_asset_lock_with_signer call site in this branch for the new optional override parameter (passed as None — this branch doesn't use the override). TODO(pin) left in Cargo.toml: bump again once #850 merges to dev. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Source: reviewers: Codex gpt-5.5 general, rust-quality, and ffi-engineer completed; Claude/Opus general, rust-quality, and ffi-engineer failed at ACP startup due extra-usage exhaustion (reset Jul 10, 8am America/Chicago). Verifier: Codex gpt-5.5; Claude/Opus verifier failed for the same quota reason. Specialists: rust-quality, ffi-engineer.
Incremental + cumulative review at 062caeaad99a5ef24f4027ca16a447da41333557, reconciling prior review 6103ab6b862e7ac336d88da09f1d425d4fdf9639. The latest delta only bumps the rust-dashcore pin to the key-wallet asset-lock override branch and passes the new asset-lock builder argument, so I found no new latest-delta-specific defects. All four prior findings remain valid at the current head and are carried forward below.
Prior Finding Reconciliation:
- STILL VALID:
packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift:35-37still calls removedManagedCoreWallet.sendToAddresses; the same stale call remains in the other Swift integration tests. - STILL VALID:
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282still derives non-platform sender account indexes from Platform Payment rows even though.coreToCorenow uses the value for BIP44 funding. - STILL VALID:
packages/rs-platform-wallet-ffi/src/error.rs:258-313still has noPersisterLoad(_)arm, so typed Rust retry/fatal classification still flattens toErrorUnknownat the FFI boundary. - STILL VALID:
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348still documents and exposes split funding selection/reservation, leaving same-account concurrent FFI callers to serialize externally.
Carried-Forward Prior Findings:
- blocking: Update Swift integration tests for the split core send API.
- blocking: Do not use Platform Payment account indexes for Core sends.
- suggestion: Preserve persister-load retry classification across FFI.
- suggestion: Keep same-account funding selection and reservation atomic.
New Findings In Latest Delta:
- None.
2 blocking | 2 suggestions
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/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift`:
- [BLOCKING] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift:35-37: Update integration tests for the split core send API
`ManagedCoreWallet.sendToAddresses` is not present in the current Swift SDK API, but this integration test still calls it here. The same stale call is also present in `SpvRestartIntegrationTests.swift` and `PersisterRestartClassificationIntegrationTests.swift`, while `Package.swift` declares `SwiftDashSDKIntegrationTests` as a real SwiftPM test target. `swift test` or an Xcode test build will fail before these tests can run. Update the tests to build and sign with `CoreTransactionBuilder` and then call `ManagedCoreWallet.broadcastTransaction(_:)`, or restore a compatibility wrapper over the new split API.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282: Do not use Platform Payment account indexes for Core sends
For non-`platformToPlatform` flows this fallback derives `senderAccountIndex` from BLAST Platform Payment address rows, and the nearby comment says those flows ignore it. That is no longer true: the `.coreToCore` path passes this same value to `CoreTransactionBuilder.setFunding(... accountType: .bip44 ...)` and `buildSigned(... accountType: .bip44 ...)`. A normal Core payment can therefore try to spend BIP44 account N where N came from an unrelated Platform Payment account, either failing because that BIP44 account is missing or spending from the wrong Core account if it exists. Keep the Platform Payment account selection scoped to `platformToPlatform`, and choose the BIP44 funding account from Core account state for `.coreToCore` or preserve account 0.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:258-313: Preserve persister-load retry classification across FFI
`PlatformWalletError::PersisterLoad` wraps the typed `PersistenceError`, which carries transient versus fatal classification through `kind()` and `is_transient()`, and `platform_wallet_manager_load_from_persistor` still returns hard load failures through `unwrap_result_or_return!(result)`. This blanket `From<PlatformWalletError>` conversion has no `PersisterLoad(_)` arm, so retryable storage contention and permanent persisted-state failures both become `ErrorUnknown`; Swift maps that to `.unknown(String)`. Launch callers therefore cannot branch on retryability without parsing display text. Add dedicated persister-load FFI result codes, or another structured ABI contract that carries retryability, and mirror it in Swift.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348: Keep same-account funding selection and reservation atomic
`core_wallet_tx_builder_set_funding` selects spendable UTXOs into a long-lived builder, but the reservation is not taken until `core_wallet_tx_builder_build_signed` later calls `build_signed`. The comment above this function documents the gap: the wallet lock is released between the FFI calls, so two concurrent builders for the same account can both select the same free UTXO before either reserves it. That leaves the public split-builder API relying on foreign callers to serialize same-account sends, while the Swift wrapper does not enforce that contract. Move funding selection into the signed-build step under one wallet/account critical section, or add a per-account send mutex or structured API so selection and reservation remain atomic for FFI callers.
PR #850 (asset-lock explicit UTXO override) and PR #851 (key-wallet out-of-order UTXO spend fix, #649) are disjoint branches off `dev`. Re-point the pin at #851's head, which this crate's watch-only rehydration work actually needs. The only call site depending on #850's API (`build_asset_lock_with_signer`'s `override_utxo` param) was passing `None` — a no-op default — so dropping the argument to match #851's 5-arg signature is a pure mechanical revert, no behavior change. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Source: reviewers: Claude/Opus general, rust-quality, and ffi-engineer failed at ACP startup; Codex gpt-5.5 rust-quality and ffi-engineer completed, and Codex gpt-5.5 general emitted usable JSON despite nonzero exit. Verifier: Codex gpt-5.5. Specialists: rust-quality, ffi-engineer.
Incremental + cumulative review at a985d2359a3744194e9a5c074f7384ba8d1ecc95, reconciling prior review 062caeaa. The latest delta only changes Cargo.toml, Cargo.lock, and packages/rs-platform-wallet/src/wallet/asset_lock/build.rs to switch the rust-dashcore pin from PR #850 to PR #851 and remove the now-obsolete asset-lock builder argument. No new latest-delta-specific findings were verified. All four prior findings remain valid on the current head and are carried forward below.
Prior Finding Reconciliation:
- STILL VALID:
packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift:35-37still calls removedManagedCoreWallet.sendToAddresses; the same stale call remains in the other Swift integration tests. - STILL VALID:
packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282still derives non-platform sender account indexes from Platform Payment rows even though.coreToCorenow uses the value for BIP44 funding. - STILL VALID:
packages/rs-platform-wallet-ffi/src/error.rs:258-313still has noPersisterLoad(_)arm, so typed Rust retry/fatal classification still flattens toErrorUnknownat the FFI boundary. - STILL VALID:
packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348still documents and exposes split funding selection/reservation, leaving same-account concurrent FFI callers to serialize externally.
Carried-Forward Prior Findings:
- blocking: Update integration tests for the split core send API.
- blocking: Do not use Platform Payment account indexes for Core sends.
- suggestion: Preserve persister-load retry classification across FFI.
- suggestion: Keep same-account funding selection and reservation atomic.
New Findings In Latest Delta:
- None.
2 blocking | 2 suggestions
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/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift`:
- [BLOCKING] packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift:35-37: Update integration tests for the split core send API
`ManagedCoreWallet.sendToAddresses` is not present in the current Swift SDK API, but this integration test still calls it here. The same stale call is also present in `SpvRestartIntegrationTests.swift` and `PersisterRestartClassificationIntegrationTests.swift`, while `Package.swift` declares `SwiftDashSDKIntegrationTests` as a real SwiftPM test target. `swift test` or an Xcode test build will fail before these tests can run. Update the tests to build and sign with `CoreTransactionBuilder` and then call `ManagedCoreWallet.broadcastTransaction(_:)`, or restore a compatibility wrapper over the new split API.
In `packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift`:
- [BLOCKING] packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282: Do not use Platform Payment account indexes for Core sends
For non-`platformToPlatform` flows this fallback derives `senderAccountIndex` from BLAST Platform Payment address rows, and the nearby comment says those flows ignore it. That is no longer true: the `.coreToCore` path passes this same value to `CoreTransactionBuilder.setFunding(... accountType: .bip44 ...)` and `buildSigned(... accountType: .bip44 ...)`. A normal Core payment can therefore try to spend BIP44 account N where N came from an unrelated Platform Payment account, either failing because that BIP44 account is missing or spending from the wrong Core account if it exists. Keep the Platform Payment account selection scoped to `platformToPlatform`, and choose the BIP44 funding account from Core account state for `.coreToCore` or preserve account 0.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:258-313: Preserve persister-load retry classification across FFI
`PlatformWalletError::PersisterLoad` wraps the typed `PersistenceError`, which carries transient versus fatal classification through `kind()` and `is_transient()`, and `platform_wallet_manager_load_from_persistor` still returns hard load failures through `unwrap_result_or_return!(result)`. This blanket `From<PlatformWalletError>` conversion has no `PersisterLoad(_)` arm, so retryable storage contention and permanent persisted-state failures both become `ErrorUnknown`; Swift maps that to `.unknown(String)`. Launch callers therefore cannot branch on retryability without parsing display text. Add dedicated persister-load FFI result codes, or another structured ABI contract that carries retryability, and mirror it in Swift.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348: Keep same-account funding selection and reservation atomic
`core_wallet_tx_builder_set_funding` selects spendable UTXOs into a long-lived builder, but the reservation is not taken until `core_wallet_tx_builder_build_signed` later calls `build_signed`. The comment above this function documents the gap: the wallet lock is released between the FFI calls, so two concurrent builders for the same account can both select the same free UTXO before either reserves it. That leaves the public split-builder API relying on foreign callers to serialize same-account sends, while the Swift wrapper does not enforce that contract. Move funding selection into the signed-build step under one wallet/account critical section, or add a per-account send mutex or structured API so selection and reservation remain atomic for FFI callers.
`ManagedCoreWallet.sendToAddresses` is gone; three integration tests still called it and would fail to compile under `-warnings-as-errors`. Route them through the canonical `CoreTransactionBuilder` → `buildSigned` → `broadcastTransaction` flow via a shared `TestWalletWrapper.send(...)` helper (BIP44 account 0), mirroring `SendViewModel`'s `.coreToCore` case. Test intent is preserved verbatim: the two `waitForNewTxid` sites keep discarding the result, and the persister-restart test derives its txid from the signed tx's consensus-serialized `data` — the same `sha256d(tx_bytes)` identity it asserted before. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nt index
For every non-`platformToPlatform` flow, `senderAccountIndex` was derived from
key-class-0 Platform Payment accounts (`addressBalances.filter { keyClass == 0 }`)
and then fed — for `.coreToCore` — into
`CoreTransactionBuilder.setFunding(accountType: .bip44, accountIndex:)`. That
crosses two distinct account namespaces: a Platform-Payment index is not a BIP44
Core account index, so the wrong Core account could fund the payment.
Core-to-Core has no account picker and funds the default BIP44 account, so
resolve the non-platform path to account 0. The adjacent comment claimed every
other flow "ignores senderAccountIndex" — false for `.coreToCore`; rewrite it to
document both consumers and their separate namespaces.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…he FFI `PlatformWalletError::PersisterLoad(PersistenceError)` had no dedicated arm in `From<PlatformWalletError> for PlatformWalletFFIResult`, so it fell to `ErrorUnknown` and Swift only ever saw `.unknown(String)` — losing the transient-vs-fatal distinction the Rust side already proves (rehydration_load.rs: RT-PersisterLoad-Transient / -Permanent). Add two by-value codes — `ErrorPersisterLoadTransient` (22) and `ErrorPersisterLoadFatal` (23) — split on `PersistenceError::is_transient()` (Fatal/Constraint/LockPoisoned all read non-transient, i.e. do-not-retry). The typed Display still travels in the message string; no ABI/out-param change, matching the `AddressNonceMismatch` precedent. Mirror both codes and their `PlatformWalletError` cases into the Swift binding; cbindgen regenerates the `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_*` constants from the enum. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…let-rehydration # Conflicts: # packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/CoreSendIntegrationTests.swift # packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/PersisterRestartClassificationIntegrationTests.swift # packages/swift-sdk/SwiftTests/SwiftDashSDKIntegrationTests/Core/SpvRestartIntegrationTests.swift
|
Merged Worth flagging for reviewers: Also picked up #4038 (multi-wallet shielded sub-wallet binding), which touches 🤖 Co-authored by Claudius the Magnificent AI Agent |
There was a problem hiding this comment.
Code Review
Prior Finding Reconciliation: all four prior findings from a985d235 were re-checked against eb3c7ae8. Prior-1, prior-2, and prior-3 are FIXED by the latest push. Prior-4 remains a real split funding-selection/reservation race, but it is explicitly documented in the PR and at the FFI call site as an intentionally deferred follow-up, so it is not carried forward as a blocking review finding for this head.
Source: reviewers: claude opus general (failed: extra-usage quota), codex gpt-5.5 general (parsed despite nonzero exit), claude opus rust-quality (failed: extra-usage quota), codex gpt-5.5 rust-quality (ok), claude opus ffi-engineer (failed: extra-usage quota), codex gpt-5.5 ffi-engineer (ok). Verifier: claude opus failed extra-usage quota; codex gpt-5.5 completed.
Carried-Forward Prior Findings: none. New Findings In Latest Delta: one blocking seedless rehydration correctness issue.
🔴 1 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/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3251-3328: Do not register wallets after dropping persisted UTXOs
The UTXO restore loop still treats some persisted UTXO rows as warn-and-continue: an undecodable script_pubkey is skipped, and a UTXO whose account cannot be found in the reconstructed snapshot is skipped. After that, the function recomputes the wallet balance from only the routed subset and returns a successfully loaded wallet. UTXOs are part of the PR's seedless restore contract and the source of the spendable balance; loading after dropping any non-legacy funds row silently under-restores funds, which contradicts the stated fail-closed/no-silent-zero behavior. These cases should return a per-wallet `PersistenceError` from `build_wallet_start_state` so `FFIPersister::load` records the wallet as `CorruptPersistedRow` instead of registering partial state.
There was a problem hiding this comment.
Code Review
Source: reviewers: Claude/Opus general, rust-quality, and ffi-engineer failed due extra-usage quota (resets Jul 10, 8am America/Chicago); Codex gpt-5.5 general, rust-quality, and ffi-engineer completed. Verifier: Claude/Opus failed for the same quota reason; Codex gpt-5.5 completed. Specialists: rust-quality, ffi-engineer.
Incremental + cumulative review at ccc95e3f2e893108a79a9a0367cd36e8ab0b40af, reconciling prior review eb3c7ae835b1df9388fdf5e3d0d2c7c14fea2611. The latest delta is Swift-only (opaque-pointer migration plus shielded multi-wallet display/bind updates); after reviewing those changes and running git diff --check eb3c7ae8..ccc95e3f, I found no new latest-delta findings.
Prior Finding Reconciliation:
- STILL VALID:
packages/rs-platform-wallet-ffi/src/persistence.rs:3242-3364still drops non-legacy malformed or unroutable persisted UTXO rows, recomputes balance from only routed rows, and returns a loadable wallet snapshot. - Older findings from
a985d235remain as reconciled in the previous review: Swift integration tests, Core-send account index, and PersisterLoad FFI classification are fixed; the split funding-selection/reservation race is intentionally deferred and not carried forward.
Carried-Forward Prior Findings:
- blocking: Do not register wallets after dropping persisted UTXOs.
New Findings In Latest Delta:
- None.
1 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/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3242-3364: Do not register wallets after dropping persisted UTXOs
Prior-1 is still valid. `build_wallet_start_state` still converts malformed txids, undecodable script_pubkeys, and UTXOs whose resolved account is absent from the reconstructed snapshot into warn-and-continue skips, then recomputes balances from only the routed subset and returns a loadable `ClientWalletStartState`. Persisted UTXOs are the authoritative seedless restore spendable set; silently dropping non-legacy funds rows can register a wallet with understated balance and missing inputs instead of classifying the persisted wallet row as corrupt. Only explicitly supported legacy removed account tags should be droppable.
Addresses three self-review action items left on PR #3692: - Rename the `ClientWalletStartState::core_wallet_info` field back to `wallet_info` across the crate, the FFI persister, load path, tests and README. The field is a pure Rust identifier (struct derives only `Debug`, no serde, no `#[serde(rename)]`, no byte-key encoding), so the rename is cosmetic and touches no on-disk/wire format. - Condense `manager/rehydrate.rs` commentary from 53 to 22 comment lines, cutting narrative/redundant prose while preserving the load-bearing trust-boundary security rationale. - Rewrite the `tests/rehydration_load.rs` module doc to be self-contained, dropping the "Item E" / "after the seedless rework" historical framing. No behavior change. fmt/check/clippy (-D warnings) clean; the 14 rehydration_load tests and rehydrate unit tests pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Vq23cSxwwL4zSimo491Dr
There was a problem hiding this comment.
Code Review
Source: reviewers: Claude/Opus general, rust-quality, and ffi-engineer failed due extra-usage quota (resets Jul 10, 8am America/Chicago); Codex gpt-5.5 general, rust-quality, and ffi-engineer completed. Verifier: Claude/Opus failed for the same quota reason; Codex gpt-5.5 completed. Specialists: rust-quality, ffi-engineer.
Incremental + cumulative review at f8ee5512a77d9795720fb0daa7e1b839d61c0bba, reconciling prior review ccc95e3f2e893108a79a9a0367cd36e8ab0b40af. The latest delta mostly renames core_wallet_info to wallet_info and adjusts SPV clear-storage behavior. No new latest-delta-specific findings were verified.
Prior Finding Reconciliation:
- STILL VALID:
packages/rs-platform-wallet-ffi/src/persistence.rs:3252-3328still drops malformed/unroutable persisted UTXO rows, recomputes balances from only routed UTXOs, and returns a loadable wallet state.
Carried-Forward Prior Findings:
- blocking: Do not register wallets after dropping persisted UTXOs.
New Findings In Latest Delta:
- None.
Additional Cumulative Findings:
- blocking: Swift filters persisted wallets with missing account manifests before Rust can classify them as skipped/corrupt load rows.
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/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3252-3328: Do not register wallets after dropping persisted UTXOs
`build_wallet_start_state` still warns and continues when a persisted UTXO has an undecodable `script_pubkey` or resolves to an account that is absent from the reconstructed funds snapshot. The function then recomputes balances from only the routed subset and returns a loadable `ClientWalletStartState`, so `load_from_persistor` can register a wallet with missing spendable inputs and understated balance. Persisted UTXOs are the seedless restore spendable set, not a disposable cache; non-legacy structural row failures should reject the wallet snapshot and surface through the load-skip/corrupt-row outcome instead of constructing degraded state.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletPersistenceHandler.swift:3898-3902: Pass missing-manifest wallets through to Rust
`loadWalletList()` filters out every `PersistentWallet` that lacks a non-empty `accountExtendedPubKeyBytes`, and returns success with `(nil, 0, false)` if all rows are in that state. That bypasses Rust's `build_watch_only_wallet` empty-manifest handling, which maps the row to `CorruptKind::MissingManifest` and reports it through `LoadOutcomeFFI`; Swift then sees a clean empty load and `lastLoadSkippedWallets` remains empty even though a persisted wallet row exists but cannot be reconstructed. For the restore contract added by this PR, the wallet row should be handed to Rust with zero accounts so the user-visible skipped-wallet outcome is preserved.
|
Logic moved to platform-wallet-storage, #3968 . Closing. |
Why this PR exists
rs-platform-wallet's in-memory wallet is a pure cache — empty every time the process starts. Before this PR, the only way to refill it from local storage was to hand back a realWallet, which meant handing back key material. A client that wants to show a previously-synced wallet's balance and history at launch had no way to do that without the seed already unlocked.Why
ClientWalletStartStategained new fields — and why dashwallet-ios never needed themThe old shape of this struct carried a real
Wallet— actual key material — alongside aManagedWalletInfofor everything else. That made "load without the seed" a contradiction: the type itself required a seed-bearing object to exist. This PR replaces theWalletfield withnetwork+birth_height(plain facts) and anaccount_manifest(the account list as public xpubs only) — enough forWallet::new_watch_only()to rebuild a fully-functional watch-only wallet with no key material anywhere on the load path. Signing keys are derived later, on demand, only when something actually needs to sign. The old info field is kept (renamedcore_wallet_info) since it never held key material to begin with.This is specifically a Rust↔Swift FFI-boundary problem, and dashwallet-ios doesn't have one. Its wallet engine (DashSync) is native Objective-C/Swift, running in the same process as the app, and persists wallet state (address pools, UTXO/tx graph, sync watermarks) directly as Core Data/SQLite objects it reads in place — there's no serialization boundary to cross, so there was never a reason to design a "keyless" wire-format type for it.
rs-platform-walletdoes cross that boundary (into dash-evo-tool's Rust code, and into Swift for the new unified SDK), so it needs an explicit, type-enforced guarantee that no key material can leak across it — which is exactly what the restructuredClientWalletStartStatenow provides.What was done
This is a simple, focused PR: it only changes how a wallet is loaded from local storage, and improves the error handling around that load.
PlatformWalletManager::load_from_persistor(), which reconstructs every persisted wallet asWallet::new_watch_only(...)from its stored account xpubs and full snapshot (ClientWalletStartState.core_wallet_info) — no seed, no signing-key derivation, on the load path.load_from_persistornow returns a 3-state outcome (Loaded/Partial/NoneUsable) instead of a near-boolean result, so callers can tell "loaded cleanly," "loaded but something needs attention," and "nothing to load yet" apart.ClientWalletStartStatefields (identity_keys,contacts) whose data is already restored earlier in the load path.ClientWalletStartState, load-result types) that feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968's storage-reader implementation consumes.Folded-in fix: address-nonce errors are now typed
Bundled here at the user's request; unrelated to the load work above. Address-funds operations (shielding, identity top-up, transfers) use an optimistic nonce — submit
fetched + 1, retry if Platform rejects it as stale. That rejection used to arrive as a flattened string, so a caller couldn't distinguish it from any other failure or recover the expected nonce to retry correctly. It's now a typedPlatformWalletError::AddressNonceMismatch(Rust), a dedicated FFI result code, and a matching Swift error case — all purely additive.Known follow-up (not fixed in this PR)
Testing
cargo fmt --all --check;cargo clippy --all-targets --all-features -- -D warningsclean onplatform-wallet,platform-wallet-ffi, andrs-sdk-ffi.cargo test --all-features:platform-wallet528 passed / 0 failed;platform-wallet-ffi154 passed / 0 failed.Swift caveat: no Swift/iOS toolchain in this environment — the Swift-side changes are pattern-exact against existing arms/signatures and the cbindgen naming convention, but not compile-verified. An iOS
xcodebuildsmoke build is recommended before merge.Breaking changes
Relative to the current
ClientWalletStartState/load_from_persistorshape onv4.1-dev:ClientWalletStartStateno longer carries aWallet(key material). It now carriesnetwork,birth_height, and a keylessaccount_manifestinstead — callers constructing the old shape (dash-evo-tool, the iOS SDK persister) need to move to the new keyless fields.load_from_persistor's result is now a 3-state outcome (Loaded/Partial/NoneUsable) instead of a 2-state one.ClientWalletStartState.identity_keysand.contactsare removed — callers restoring identity keys and DashPay contact state should stop populating these fields; that restoration now happens earlier, inbuild_wallet_identity_bucket.Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent