Skip to content

feat(platform-wallet): load watch-only wallet state from local storage at startup#3692

Closed
Claudius-Maginificent wants to merge 80 commits into
v4.1-devfrom
feat/platform-wallet-rehydration
Closed

feat(platform-wallet): load watch-only wallet state from local storage at startup#3692
Claudius-Maginificent wants to merge 80 commits into
v4.1-devfrom
feat/platform-wallet-rehydration

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Why this PR exists

  • Problem: 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 real Wallet, 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.
  • What breaks without it: on iOS the Keychain is locked at launch, so a seed-requiring load isn't just slow, it's impossible until the user unlocks — "show balance on launch" fails structurally, not just slowly. Even where the seed is available, skipping this and re-deriving from scratch means minutes of chain rescan on every restart, for a wallet that was fully synced one launch ago. dash-evo-tool's own backend rewrite (feat: drive verification c bindings #860) hit this directly: reloading a wallet in watch-only mode used to fail with a missing-private-key error the moment a user tried to fund something from its balance, and its cold-start migration path needs previously-synced wallets loaded back automatically after a restart, with no seed prompt.
  • Blocking relationship: feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968 (the SQLite persistence-reader implementation) is stacked on top of this branch and depends on the final shape of the types introduced here — it isn't mergeable standalone until this lands. (Correcting this PR's own earlier text, which called feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration #3968 "independent" — it's a dependent, stacked PR.)

Why ClientWalletStartState gained new fields — and why dashwallet-ios never needed them

The old shape of this struct carried a real Wallet — actual key material — alongside a ManagedWalletInfo for everything else. That made "load without the seed" a contradiction: the type itself required a seed-bearing object to exist. This PR replaces the Wallet field with network + birth_height (plain facts) and an account_manifest (the account list as public xpubs only) — enough for Wallet::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 (renamed core_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-wallet does 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 restructured ClientWalletStartState now 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.

  • Added PlatformWalletManager::load_from_persistor(), which reconstructs every persisted wallet as Wallet::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.
  • The persisted snapshot (UTXOs, transactions, IS-locks, sync watermarks, balances) is applied as-is; a wallet that can't be loaded cleanly fails closed rather than silently coming back with a zero balance.
  • load_from_persistor now 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.
  • Added typed FFI error codes so a transient vs. fatal load failure (e.g. a corrupted local row) survives the FFI boundary as a distinct, actionable code instead of flattening to a generic "unknown error."
  • Removed dead scaffolding that predated this implementation: an unused fallback load path that no real persister ever produced, its now-unreachable error variants, and two ClientWalletStartState fields (identity_keys, contacts) whose data is already restored earlier in the load path.
  • Defines the data model (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 typed PlatformWalletError::AddressNonceMismatch (Rust), a dedicated FFI result code, and a matching Swift error case — all purely additive.

Known follow-up (not fixed in this PR)

  • Funding race: two concurrent sends from the same account can both pass the funding-selection step before either reserves its UTXOs, risking a double-spend at broadcast. Documented as a known limitation at the call site; today's single-threaded send flow isn't affected.
  • Manifest isn't authenticated: the persisted account manifest is trusted as-is, with no cryptographic binding to the wallet ID. Verified the root xpub can't be recovered from what's persisted, so there's no in-crate fix — the real fix (a persisted commitment/MAC) is tracked in feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992.
  • Chain-lock cache isn't re-verified on load: restored from local storage without re-checking its BLS/quorum signature. Low risk today since downstream network re-verification of the asset-lock proof still catches it, but it's the same underlying gap as the manifest issue above.
  • One coverage gap: no single test exercises identity-key, contact, payment, and ignored-sender restoration all at once (each is covered individually). A full test needs a large synthetic fixture; noted at the call site.

Testing

cargo fmt --all --check; cargo clippy --all-targets --all-features -- -D warnings clean on platform-wallet, platform-wallet-ffi, and rs-sdk-ffi. cargo test --all-features: platform-wallet 528 passed / 0 failed; platform-wallet-ffi 154 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 xcodebuild smoke build is recommended before merge.

Breaking changes

Relative to the current ClientWalletStartState/load_from_persistor shape on v4.1-dev:

  • ClientWalletStartState no longer carries a Wallet (key material). It now carries network, birth_height, and a keyless account_manifest instead — 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_keys and .contacts are removed — callers restoring identity keys and DashPay contact state should stop populating these fields; that restoration now happens earlier, in build_wallet_identity_bucket.

Checklist

  • Self-review performed
  • Tests added/updated
  • Docs updated where needed

🤖 Co-authored by Claudius the Magnificent AI Agent

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

The 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.

Changes

Watch-only rehydration and load outcomes

Layer / File(s) Summary
Persisted state and outcome contracts
packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs, packages/rs-platform-wallet/src/manager/load_outcome.rs, packages/rs-platform-wallet/src/error.rs, packages/rs-platform-wallet/src/changeset/client_start_state.rs, packages/rs-platform-wallet/src/lib.rs, packages/rs-platform-wallet/src/manager/mod.rs, packages/rs-platform-wallet/src/changeset/changeset.rs, packages/rs-platform-wallet/src/changeset/traits.rs, packages/rs-platform-wallet/README.md, packages/rs-platform-wallet/src/events.rs
ClientWalletStartState moves to keyless reconstruction fields, LoadOutcome/SkipReason/CorruptKind are added, ClientStartState gains skipped, and the public module/docs/event surface is updated for load-skip reporting.
Wallet rebuild and core-state replay
packages/rs-platform-wallet/src/manager/rehydrate.rs, packages/rs-platform-wallet/src/error.rs
New helpers build watch-only wallets, apply persisted core state, reconcile address pools, and add fail-closed rehydration error variants with tests.
Load loop and identity replay
packages/rs-platform-wallet/src/manager/load.rs, packages/rs-platform-wallet/src/wallet/apply.rs, packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs, packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs, packages/rs-platform-wallet/src/wallet/platform_wallet.rs
load_from_persistor now returns LoadOutcome, continues past per-row corruption, and shared identity/contact replay is delegated to a new helper.
FFI outcome structs and Swift consumption
packages/rs-platform-wallet-ffi/src/core_wallet_types.rs, packages/rs-platform-wallet-ffi/src/manager.rs, packages/rs-platform-wallet-ffi/src/persistence.rs, packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs, packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
The Rust FFI now reports skipped wallets with stable reason codes and freeable outcome storage, the persister classifies corrupt rows, and Swift stores the skipped-wallet list while skipping already-rejected ids.

Core bridge UTXO warnings

Layer / File(s) Summary
Non-default-account UTXO warnings
packages/rs-platform-wallet/src/changeset/core_bridge.rs
TransactionDetected and BlockProcessed now warn when persisted UTXOs originate from non-default account indexes, with regression tests preserving projection behavior.

Mnemonic signer and test stubs

Layer / File(s) Summary
Extended public key derivation
packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs, packages/rs-sdk-ffi/src/signer_simple.rs, packages/rs-dpp/src/state_transition/mod.rs, packages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rs, packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs
Mnemonic-based derivation gains a shared helper and extended_public_key, and the DPP signer test stubs implement the new trait method.

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
Loading

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • dashpay/platform#3634 - Also changes packages/rs-platform-wallet-ffi/src/persistence.rs in the wallet restore path and the keyless start-state projection.
  • dashpay/platform#3828 - Related to the same core_bridge.rs UTXO projection behavior and account-index handling.
  • dashpay/platform#3973 - Touches the same Cargo.toml workspace dependency pins.

Suggested labels: ready for final review

Suggested reviewers: QuantumExplorer, llbartekll, shumkov, lklimek, ZocoLini

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: seedless watch-only wallet state is loaded from local storage at startup.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/platform-wallet-rehydration

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

📖 Book Preview built successfully.

Download the preview from the workflow artifacts.
To view locally: download the artifact, unzip, and open index.html.

Updated at 2026-05-22T10:49:18.857Z

@Claudius-Maginificent

Copy link
Copy Markdown
Collaborator Author

🔁 Seedless-load rework landed — 34532e57d5..6f22c0e9f5

Per design validation against dashwallet-ios swift-sdk-integration: real iOS host loads watch-only at app launch (Keychain locked) and signs on-demand via the existing MnemonicResolverHandle vtable. The original seeded-load model (load takes &dyn SeedProvider, derives Wallet per persisted id, runs constant-time wrong-seed gate at load) did not match the real consumer. Pushed 7 commits flipping the model. Existing PR description above is stale — supersedes it for the deltas below.

What's new

  • load_from_persistor() is now seedlesspub async fn load_from_persistor(&self) -> Result<LoadOutcome, PlatformWalletError>. Manager reconstructs each persisted wallet via Wallet::new_watch_only(network, wallet_id, accounts), with accounts assembled from the keyless account_manifest (one Account::from_xpub(...) per registration entry). No seed touched on the load path.
  • Wrong-seed gate moved to the sign pathcrate::sign_gate::verify_seed_matches_wallet_id(root_pub, expected_wallet_id) -> bool (constant-time via subtle::ConstantTimeEq). Wired into all 4 resolver-fed FFI sign entrypoints:
    • sign_with_mnemonic_resolver.rs::dash_sdk_sign_with_mnemonic_resolver_and_path
    • identity_derive_and_persist.rs::dash_sdk_derive_and_persist_identity_keys
    • derive_identity_key_at_slot.rs::dash_sdk_derive_identity_key_at_slot_with_resolver
    • shielded_sync.rs::platform_wallet_manager_bind_shielded
  • SeedProvider trait + adapter + FFI shim deletedMnemonicResolverHandle (which already existed) is the sole on-demand signing contract. Files removed: rs-platform-wallet/src/seed_provider.rs, rs-platform-wallet-ffi/src/rehydration_seed_provider.rs, rs-platform-wallet-storage/src/secrets/seed_provider_adapter.rs (+ its test).
  • FFI signature: platform_wallet_manager_load_from_persistor(manager, persister, *mut LoadOutcomeFFI) — dropped the 3rd resolver arg (reverts b7508a0d47's 3-arg shape).
  • Swift PlatformWalletManager.swift aligns to the 2-arg + outparam C signature (passes nil for the outcome ptr).

ABI deltas (new in this PR, no v3.1-dev impact)

  • New result code ErrorWrongSeedForWallet = 13 in PlatformWalletFFIResultCode.
  • SkippedWalletFFI::reason_code family remapped to 100/101/102 for the new CorruptKind variants (MissingManifest, MalformedXpub, DecodeError). The old 0/1/2 (seed-availability) are gone — those skip-triggers don't exist anymore.

AR-7 hygiene

  • Load path eliminates AR-7 entirely — manager never constructs WalletType::Mnemonic|Seed; only WalletType::WatchOnly (no key material). AR-7's residual Debug concern was about derived Wallet values on the load path; that path no longer derives.
  • Sign path keeps AR-7 disciplineZeroizing + non_secure_erase() on the gate's throwaway master key on the mismatch path, before returning the error tag. No {:?} over any secret type.

Test reshape

  • tests/rehydration_load.rs — RT-WO, RT-Corrupt, RT-Z (watch-only construction, corrupt-row hard-fail, secret-hygiene). Wrong-seed scenarios removed from this file (no seed at load anymore).
  • Co-located gate tests in each of the 4 FFI sign entrypoints:
    • sign_with_mnemonic_resolver: happy + wrong + full-buffer-zero assertion
    • identity_derive_and_persist: wrong + persister-callback-never-fires assertion
    • derive_identity_key_at_slot: happy + wrong (asserts populated out_row on happy, zero/null fields on mismatch)
    • shielded_sync: wrong + null-resolver-handle-rejected (happy path requires full SDK + coordinator + commitment-tree — covered structurally by gate-fires-before-storage-touch assertion)
  • New sign_gate::tests unit module exercises CT helper directly.

What survived (unchanged by this rework)

  • Keyless PersistedWalletData / ClientWalletStartState (commit b9af9935)
  • A1/B/A2 schema readers (accounts::load_state, core_state::load_state, asset_locks::load_unconsumed)
  • F2 no-BIP44 silent-zero balance fix (commit 96a9aa90)
  • F4 dead post-insert wallet_id re-check removal (commit 62bd4754)
  • WrongSeedForDatabase typed error (re-homed to sign path)

Verification

  • cargo fmt --all --check: pass
  • cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets -- -D warnings: pass
  • cargo check --workspace: pass
  • cargo test -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi: 414 passed, 0 failed, 7 ignored
  • cargo test --doc -p platform-wallet -p platform-wallet-storage: 3 passed, 1 ignored
  • cargo check --workspace --all-features: fails on pre-existing ShieldedChangeSet: Serialize/Deserialize gap at changeset.rs:914, verified reproducible on origin/feat/platform-wallet-rehydration@34532e57 — out of scope for this rework

🤖 Co-authored by Claudius the Magnificent AI Agent

@Claudius-Maginificent Claudius-Maginificent changed the title feat(platform-wallet): add full wallet rehydration from persistor + seed (skip-and-report) feat(platform-wallet): watch-only rehydration from persistor + wallet_id sign-gate (seedless load) May 25, 2026
@lklimek lklimek force-pushed the feat/platform-wallet-rehydration branch from 0e92cb4 to f57b117 Compare May 25, 2026 11:21
@Claudius-Maginificent Claudius-Maginificent changed the title feat(platform-wallet): watch-only rehydration from persistor + wallet_id sign-gate (seedless load) feat(platform-wallet): watch-only rehydration from persistor (seedless load) May 25, 2026
Base automatically changed from feat/platform-wallet-storage-secrets to feat/platform-wallet-sqlite-persistor May 29, 2026 11:28
@lklimek lklimek added this to the v4.0.0 milestone Jun 1, 2026
Base automatically changed from feat/platform-wallet-sqlite-persistor to v3.1-dev June 9, 2026 08:13
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.21%. Comparing base (09af5ae) to head (f8ee551).

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           
Components Coverage Δ
dpp 87.72% <ø> (ø)
drive 86.14% <ø> (ø)
drive-abci 89.45% <ø> (ø)
sdk ∅ <ø> (∅)
dapi-client ∅ <ø> (∅)
platform-version ∅ <ø> (∅)
platform-value 92.20% <ø> (ø)
platform-wallet ∅ <ø> (∅)
drive-proof-verifier 49.55% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lklimek

lklimek commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

@coderabbitai review all

@thepastaclaw

thepastaclaw commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

✅ Review complete (commit f8ee551)

@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

@lklimek On it — I’ll re-review the full PR.

✅ Action performed

Full review finished.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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 k...
  • [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:159-168: Repeated restore fails on wallets already in the managerload_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 loa...
  • [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...
🤖 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.

Comment thread packages/rs-platform-wallet/src/manager/mod.rs Outdated
lklimek and others added 2 commits July 7, 2026 09:48
…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>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-313 still has no PersisterLoad(_) arm, so the typed Rust load-path retry classification falls through to ErrorUnknown before 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.

Comment thread packages/rs-platform-wallet-ffi/src/error.rs

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-313 still has no PersisterLoad(_) arm, so the typed Rust load-path retry/fatal classification falls through to ErrorUnknown before 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.sendToAddresses API 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>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-37 still calls removed ManagedCoreWallet.sendToAddresses; the same stale call remains in the other Swift integration tests.
  • STILL VALID: packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282 still derives non-platform sender account indexes from Platform Payment rows even though .coreToCore now uses the value for BIP44 funding.
  • STILL VALID: packages/rs-platform-wallet-ffi/src/error.rs:258-313 still has no PersisterLoad(_) arm, so typed Rust retry/fatal classification still flattens to ErrorUnknown at the FFI boundary.
  • STILL VALID: packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348 still 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>

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-37 still calls removed ManagedCoreWallet.sendToAddresses; the same stale call remains in the other Swift integration tests.
  • STILL VALID: packages/swift-sdk/SwiftExampleApp/SwiftExampleApp/Core/Views/SendTransactionView.swift:271-282 still derives non-platform sender account indexes from Platform Payment rows even though .coreToCore now uses the value for BIP44 funding.
  • STILL VALID: packages/rs-platform-wallet-ffi/src/error.rs:258-313 still has no PersisterLoad(_) arm, so typed Rust retry/fatal classification still flattens to ErrorUnknown at the FFI boundary.
  • STILL VALID: packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:337-348 still 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.

lklimek added a commit that referenced this pull request Jul 8, 2026
lklimek and others added 4 commits July 8, 2026 10:17
`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
@Claudius-Maginificent

Copy link
Copy Markdown
Collaborator Author

Merged v4.1-dev into this branch (ccc95e3f2e) to clear a base conflict.

Worth flagging for reviewers: v4.1-dev picked up #4037 in the meantime, which independently fixes the same sendToAddresses-removal issue as this PR's "review-comment triage batch 2" (3 Swift integration test files). Conflict resolved by dropping our duplicate TestWalletWrapper.send(toAddress:) helper in favor of #4037's send(to:) — same underlying fix, just different argument labels. No functional difference, no follow-up needed.

Also picked up #4038 (multi-wallet shielded sub-wallet binding), which touches SendTransactionView.swift at disjoint lines from this PR's account-index fix — auto-merged clean, no conflict.

🤖 Co-authored by Claudius the Magnificent AI Agent

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-3364 still drops non-legacy malformed or unroutable persisted UTXO rows, recomputes balance from only routed rows, and returns a loadable wallet snapshot.
  • Older findings from a985d235 remain 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.

Comment thread packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs Outdated
@Claudius-Maginificent Claudius-Maginificent changed the title feat(platform-wallet): watch-only rehydration from persistor (seedless load) feat(platform-wallet): load watch-only wallet state from local storage at startup Jul 8, 2026
Comment thread packages/rs-platform-wallet/src/manager/rehydrate.rs
Comment thread packages/rs-platform-wallet/tests/rehydration_load.rs Outdated
lklimek and others added 2 commits July 8, 2026 13:10
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

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-3328 still 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.

@lklimek

lklimek commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Logic moved to platform-wallet-storage, #3968 . Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants