feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692
feat(platform-wallet): watch-only rehydration from persistor (seedless load)#3692Claudius-Maginificent wants to merge 58 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 |
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
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 —
|
Claudius-Maginificent
changed the title
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
0e92cb4 to
f57b117
Compare
Claudius-Maginificent
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #3692 +/- ##
=========================================
Coverage 87.18% 87.18%
=========================================
Files 2632 2632
Lines 327563 327563
=========================================
Hits 285592 285592
Misses 41971 41971
🚀 New features to boost your workflow:
|
lklimek
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
|
@coderabbitai review all |
|
✅ Review complete (commit 83d9eff) |
|
✅ Action performedFull review finished. |
thepastaclaw
left a comment
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/rs-platform-wallet/README.md (1)
95-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language tag to the architecture diagram fence.
Static analysis flags the fenced block as missing a language specifier (MD040).
🔧 Proposed fix
-``` +```text commands (send, register, sync, …)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/README.md` around lines 95 - 108, The fenced architecture diagram in the README is missing a language specifier and is triggering MD040. Update the fenced block that contains the diagram to use the appropriate text language tag so the diagram remains unchanged but the fence is explicitly labeled; this is the only fix needed in the README section with the architecture diagram.Source: Linters/SAST tools
packages/rs-platform-wallet/tests/rehydration_load.rs (1)
276-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared corrupt-row builder to avoid duplicated literals.
The 8-field
ClientWalletStartStatecorrupt-row literal (emptyaccount_manifest) is repeated verbatim inrt_corrupt_row_skipped_and_other_loadsandrt_z_secret_hygiene_surfaces. A small helper (e.g.fn corrupt_state() -> ClientWalletStartState) would cut duplication and the risk of the two copies drifting.Also applies to: 346-379
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet/tests/rehydration_load.rs` around lines 276 - 339, The corrupt-row `ClientWalletStartState` literal is duplicated across `rt_corrupt_row_skipped_and_other_loads` and `rt_z_secret_hygiene_surfaces`, so extract it into a shared helper to avoid drift. Add a small builder/helper like `corrupt_state()` near the existing test helpers and use it in both tests instead of repeating the 8-field struct initialization with an empty `account_manifest`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- Around line 380-383: The load path in PlatformWalletManager leaves
lastLoadSkippedWallets stale when platform_wallet_manager_load_from_persistor
fails and loadResult.check() throws. Update the failing branch around the
loadResult/check sequence to clear lastLoadSkippedWallets before rethrowing so
the published state always reflects the most recent load attempt. Use the
existing load outcome handling in PlatformWalletManager to locate the
retry/error path.
---
Nitpick comments:
In `@packages/rs-platform-wallet/README.md`:
- Around line 95-108: The fenced architecture diagram in the README is missing a
language specifier and is triggering MD040. Update the fenced block that
contains the diagram to use the appropriate text language tag so the diagram
remains unchanged but the fence is explicitly labeled; this is the only fix
needed in the README section with the architecture diagram.
In `@packages/rs-platform-wallet/tests/rehydration_load.rs`:
- Around line 276-339: The corrupt-row `ClientWalletStartState` literal is
duplicated across `rt_corrupt_row_skipped_and_other_loads` and
`rt_z_secret_hygiene_surfaces`, so extract it into a shared helper to avoid
drift. Add a small builder/helper like `corrupt_state()` near the existing test
helpers and use it in both tests instead of repeating the 8-field struct
initialization with an empty `account_manifest`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b46e26b2-2733-4046-a5c8-d7a24f463d38
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
Cargo.tomlpackages/rs-dpp/src/state_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/address_funds/address_funding_from_asset_lock_transition/signing_tests.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rspackages/rs-platform-wallet-ffi/src/core_wallet_types.rspackages/rs-platform-wallet-ffi/src/manager.rspackages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-ffi/src/wallet_restore_types.rspackages/rs-platform-wallet/README.mdpackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/changeset/traits.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/manager/load.rspackages/rs-platform-wallet/src/manager/load_outcome.rspackages/rs-platform-wallet/src/manager/rehydrate.rspackages/rs-platform-wallet/tests/rehydration_load.rspackages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rspackages/rs-sdk-ffi/src/signer_simple.rspackages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift
✅ Files skipped from review due to trivial changes (3)
- packages/rs-platform-wallet-ffi/src/wallet_restore_types.rs
- packages/rs-platform-wallet/src/changeset/traits.rs
- packages/rs-sdk-ffi/src/signer_simple.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-platform-wallet/src/manager/load_outcome.rs
- packages/rs-platform-wallet/src/error.rs
- packages/rs-platform-wallet-ffi/src/manager.rs
- packages/rs-platform-wallet/src/manager/load.rs
- packages/rs-platform-wallet/src/manager/rehydrate.rs
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
There was a problem hiding this comment.
Inline architecture notes on the new "Persistence architecture" README section: 6 points where this wording disagrees with the standing risk assessment (Model A / gate G3 is still an open decision). Each thread is marked for human resolution — agents must not auto-resolve.
🤖 Co-authored by Claudius the Magnificent AI Agent
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental verification vs prior review at 9dbca2f. The latest delta (9dbca2f..7a2e06e) is the #3980 follow-up: ClientWalletStartState gains a core_wallet_info: Option<Box<ManagedWalletInfo>> full-snapshot path so the FFI/iOS persister can hand across the pre-restored managed state verbatim (preserving per-account UTXO attribution and pool contents the flat projection cannot round-trip), a mark↔refill fixpoint fixes a real address-reuse bug in the wedge zone past the discovery horizon, RehydrateRowError is consolidated into CorruptKind, Swift now actually consumes/frees LoadOutcomeFFI, and new integration tests cover attribution preservation, idempotent reload, and mismatch skipping. Carried forward: the prior pub mod rehydrate; architectural suggestion is STILL VALID at HEAD — mod.rs:8 is unchanged and the only in-tree callers of rehydrate::* remain the sibling private mod load; (load.rs:147, :209). One new suggestion added by all three specialist agents: the PR description advertises a dedicated CorruptKind::SnapshotIdentityMismatch (FFI code 103 + Swift mapping) for the new snapshot/row cross-check, but that variant does not exist anywhere in the tree (verified via grep — zero hits) and the mismatch path at load.rs:175-190 still uses CorruptKind::DecodeError(format!(...)); the sonnet5-ffi-engineer also verified that both sides of the compare (info.wallet_id and expected_wallet_id) derive from the same untrusted entry.wallet_id field (persistence.rs:1546, :2893, :2901), so the added check is provably vacuous for the FFI path and provides no defense against the trust-boundary gap the PR description references.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed); verifier: opus; specialists: rust-quality, ffi-engineer
🟡 2 suggestion(s)
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [SUGGESTION] (deduped existing open thread)
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API with no external consumer (carried forward) — Carried forward from prior review rounds — re-verified STILL VALID at HEAD 7a2e06e (mod.rs:8is unchanged in the latest delta).pub mod rehydrate;publishes the seedless-restore helpersbuild_watch_only_walletandapply_persisted_core_stateasplatform_wallet::manager::rehydrate::*to...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:175-190: PR description claims a `CorruptKind::SnapshotIdentityMismatch` variant + FFI code 103 that don't exist; the snapshot/row cross-check is also vacuous for the FFI path
The PR description states the snapshot/row mismatch case is now surfaced via a dedicated `CorruptKind::SnapshotIdentityMismatch` (FFI code 103 + Swift mapping) `replacing the previous DecodeError conflation`. Verified against HEAD 7a2e06e9: no such variant exists anywhere in the tree (grep across `rs-platform-wallet`, `rs-platform-wallet-ffi`, and `swift-sdk` returns zero matches); `CorruptKind` still has only `MissingManifest` / `MalformedXpub` / `DecodeError(String)` (`manager/load_outcome.rs:38-49`); no FFI code 103 exists in `rs-platform-wallet-ffi/src/manager.rs` (only 100/101/102/199/200); Swift's `SkippedWalletOnLoad.reasonDescription` has no case 103; and the mismatch path at `manager/load.rs:175-190` still uses `CorruptKind::DecodeError(format!(...))` — the exact conflation the PR description says was replaced. Consequences: (1) the new regression test `rt_snapshot_wallet_id_mismatch_is_skipped` can only assert the generic `CorruptKind::DecodeError(_)` wildcard, unable to distinguish this from any other decode failure; (2) downstream FFI/Swift consumers cannot render a specific message for 'snapshot describes a different wallet' vs a generic decode failure; (3) this stretches `CorruptKind::DecodeError`'s own doc contract ('any other structural decode / projection failure ... never a raw row byte slice or a hex-encoded key') by embedding `hex::encode(info.wallet_id)` in the runtime string. Additionally, for the real FFI ingestion path the cross-check is provably vacuous: both compared quantities derive from the same untrusted `entry.wallet_id` at the same call site — `expected_wallet_id` is the map key set via `out.wallets.insert(entry.wallet_id, wallet_state)` (`persistence.rs:1546`), and `info.wallet_id` is set via `ManagedWalletInfo::from_wallet(&wallet, ...)` where `wallet = Wallet::new_external_signable(network, entry.wallet_id, accounts)` (`persistence.rs:2893, :2901`), so the check compares a value to itself. Either update the PR description to reflect what actually shipped, or add the promised `CorruptKind::SnapshotIdentityMismatch { snapshot_wallet_id, expected_wallet_id, snapshot_network, expected_network }` variant with the corresponding FFI code 103 + Swift case, wire the mismatch site to it, and switch the two rehydration integration tests to the typed variant. If the intent was defense against future persister bugs (rather than against a tampered store), the docstring should say so — the current comment 'a mismatch is a corrupt row' overstates what the check actually catches on the FFI path.
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 with no external consumer (carried forward)
Carried forward from prior review rounds — re-verified STILL VALID at HEAD 7a2e06e9 (`mod.rs:8` is unchanged in the latest delta). `pub mod rehydrate;` publishes the seedless-restore helpers `build_watch_only_wallet` and `apply_persisted_core_state` as `platform_wallet::manager::rehydrate::*` to downstream crates (including the FFI crate boundary via `rs-platform-wallet-ffi`). These helpers mutate `ManagedWalletInfo` under load-path-specific reconstruction assumptions — including the unauthenticated-manifest trust boundary explicitly documented on `PlatformWalletPersistence::load` (`changeset/traits.rs`) and on `build_watch_only_wallet` (`manager/rehydrate.rs`), and now the projection-fallback role the new PR-boundary contract explicitly marks as transitional in `README.md`. Re-verified in-tree: the only callers of `rehydrate::*` remain `super::rehydrate::build_watch_only_wallet` (`manager/load.rs:147`) and `super::rehydrate::apply_persisted_core_state` (`manager/load.rs:209`), both from the sibling private `mod load;`. Module-private visibility (`mod rehydrate;`) suffices; keeping it `pub` freezes internal recovery mechanics — including the manifest-authentication gap and the transitional projection path — as SDK/FFI-visible API without an intentional external contract, widening the blast radius of the eventual manifest-authentication fix. Since no external caller exists, this remains a drop-in visibility downgrade.
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
There was a problem hiding this comment.
Reworked the open architecture threads per maintainer feedback (final-state premise): 3 remain, now as click-applyable suggestions with source-permalink evidence; the authority/MAC point is withdrawn (invariant 6 already discloses the boundary).
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Code Review
Incremental verification vs prior review at 7a2e06e. The latest delta (7a2e06e..5460c0e) is the #3984 follow-up: CorruptKind::SnapshotIdentityMismatch is now a real typed variant (load_outcome.rs:52) wired end-to-end through FFI code 103 (rs-platform-wallet-ffi/src/manager.rs:189, :242) and Swift case 103 (PlatformWalletManager.swift:349), the mismatch site at load.rs:172-183 constructs the typed variant, snapshot_accounts_match_manifest (load.rs:338-366) extends the cross-check with an account-set comparison, and PlatformWalletError::PersisterLoad(#[from] PersistenceError) preserves the persister's typed classification instead of a flattened string. Prior-2 is FIXED. Carried forward: prior-1 (pub mod rehydrate;) STILL VALID at HEAD (mod.rs:8 unchanged, only in-tree callers remain the sibling private mod load;). Two new suggestions from the delta: (a) the account-set half of snapshot_accounts_match_manifest is provably self-referential for the only production producer of core_wallet_info — both account_manifest (persistence.rs:3468) and the snapshot's account set (from ManagedWalletInfo::from_wallet(&wallet, ...) at persistence.rs:2901) derive from the same wallet.accounts object built once from entry.accounts (persistence.rs:2893) — so it catches manager-level wiring bugs (as the two new mock-persister integration tests demonstrate) but cannot fire against a tampered SwiftData row; (b) PlatformWalletError::PersisterLoad's transient/permanent classification is dropped at the FFI boundary because the From<PlatformWalletError> for PlatformWalletFFIResult impl (rs-platform-wallet-ffi/src/error.rs:205-244) has no dedicated arm and falls through to ErrorUnknown, so Swift's loadFromPersistor cannot distinguish retryable persister hiccups from fatal ones — undermining the stated design intent of the typed-error delta for the only cross-language caller.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed); verifier: opus; specialists: rust-quality, ffi-engineer
🟡 3 suggestion(s)
Carried-forward prior findings (1)
- [SUGGESTION]
packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API with no external consumer (STILL VALID) — Carried forward from prior review rounds — re-verified STILL VALID at HEAD 5460c0e (mod.rs:8unchanged in the latest delta).pub mod rehydrate;publishes the seedless-restore helpersbuild_watch_only_walletandapply_persisted_core_stateasplatform_wallet::manager::rehydrate::*to downstream crates (including the FFI crate boundary viars-platform-wallet-ffi). These helpers mutateManagedWalletInfounder load-path-specific reconstruction assumptions — including the unauthenticated-manifest trust boundary documented onPlatformWalletPersistence::load,build_watch_only_wallet, and nowload_from_persistoritself (manager/load.rs:71-77), and the projection-fallback role the new PR-boundary contract explicitly marks as transitional. Re-verified in-tree: the only callers ofrehydrate::*remainsuper::rehydrate::build_watch_only_wallet(manager/load.rs:142) andsuper::rehydrate::apply_persisted_core_state(manager/load.rs:202), both from the sibling privatemod load;. Module-private visibility (mod rehydrate;) suffices; keeping itpubfreezes internal recovery mechanics — including the manifest-authentication gap and the transitional projection path — as SDK/FFI-visible API without an intentional external contract. Since no external caller exists, this remains a drop-in visibility downgrade.
New findings in the latest delta (2)
-
[SUGGESTION]
packages/rs-platform-wallet/src/manager/load.rs:338: New account-set cross-check is a manager-level self-consistency guard, not tamper detection — worth saying so explicitly — This delta extends the snapshot/row cross-check withsnapshot_accounts_match_manifest(load.rs:338-366) comparingaccount_manifestagainst thecore_wallet_infosnapshot'sall_managed_accounts()set. For the only in-tree producer ofcore_wallet_info: Some(...)—build_wallet_start_stateinrs-platform-wallet-ffi/src/persistence.rs— both compared quantities derive from the same in-memorywalletbuilt once in that call:account_manifestcomes fromwallet.accounts.all_accounts()(persistence.rs:3468-3476), and the snapshot's account set comes fromManagedWalletInfo::from_wallet(&wallet, ...)(persistence.rs:2901) wherewallet = Wallet::new_external_signable(network, entry.wallet_id, accounts)(persistence.rs:2893). Both trace back to the singleentry.accountsFFI array, so the check cannot fire against a tampered/corrupted SwiftData row — any tampering that alters one side alters the other identically. It does have real value: as the two new tests (rt_snapshot_account_set_mismatch_is_skipped,rt_snapshot_mismatch_skip_coexists_with_healthy_load) prove, it catches divergence in any future persister that populates the two fields via genuinely separate decode paths, or a manager-level wiring bug. But the doc comment onsnapshot_accounts_match_manifestand the trust-boundary block onload_from_persistorshould say explicitly that this check is a persister-implementation self-consistency guard, not a defense against a tampered store — otherwise readers will assume it closes the manifest-authentication gap the PR description already correctly tracks as ars-platform-wallet-storagefollow-up. Consider adding a one-line note that real defense against row tampering requires the tracked persisted MAC/commitment schema change. -
[SUGGESTION]
packages/rs-platform-wallet-ffi/src/error.rs:205:PersisterLoadtransient/permanent classification is dropped at the FFI boundary — the point of the typed-error change never reaches Swift — This delta addedPlatformWalletError::PersisterLoad(#[from] PersistenceError)specifically so the persister'sis_transient()/PersistenceErrorKindclassification survives propagation instead of being flattened to a string, motivated by callers that would want to distinguish retryable failures. The two new integration tests (rt_persister_load_transient_error_is_typed_and_retryable,rt_persister_load_permanent_error_is_typed_and_not_retryable) confirm the Rust-side plumbing. But the only cross-language consumer today isplatform_wallet_manager_load_from_persistor(rs-platform-wallet-ffi/src/manager.rs:396-419), which pipesmanager.load_from_persistor()'sResultthroughunwrap_result_or_return!→error.into()→ the blanketFrom<PlatformWalletError> for PlatformWalletFFIResultimpl atrs-platform-wallet-ffi/src/error.rs:205-244. That impl explicitly enumeratesNoSpendableInputs/OnlyOutputAddressesFunded/OnlyDustInputs,WalletAlreadyExists, and the threeShielded*variants, then falls through_ => ErrorUnknownfor everything else — including the newPersisterLoad. Swift'sloadFromPersistor()(PlatformWalletManager.swift:377-388) receivescode == ErrorUnknownplus a stringifiedDisplayand throws a genericPlatformWalletError, so the retry-vs-fatal distinction the delta was built to preserve never reaches the only caller across a language boundary. If a real product need for transient-retry doesn't exist at the Swift layer, the typed variant is purely Rust-internal and the PR description overstates its scope. If it does exist, either add a dedicatedPlatformWalletFFIResultCode(e.g.ErrorPersisterLoadTransient/ErrorPersisterLoadPermanent, or a single code + boolean out-param) and mirror it in Swift, or at minimum add an explicitPersisterLoadarm to theFromimpl that maps to a stable-code, transient-carrying variant.
Prior findings resolved (1)
- FIXED: PR description claims a
CorruptKind::SnapshotIdentityMismatchvariant + FFI code 103 that don't exist; the snapshot/row cross-check is also vacuous for the FFI path — FIXED. The typed variantCorruptKind::SnapshotIdentityMismatchis defined atpackages/rs-platform-wallet/src/manager/load_outcome.rs:52with a matchingDisplayarm at :64-66.LOAD_SKIP_REASON_SNAPSHOT_IDENTITY_MISMATCH = 103is declared atrs-platform-wallet-ffi/src/manager.rs:189and mapped inskip_reason_codeat :242, with the ABI wire value guarded byload_skip_reason_wire_values_are_stable(:591) and the mapping guarded byskip_reason_code_maps_known_kinds_to_constants(:611-613). Swift'sSkippedWalletOnLoad.reasonDescriptionmapscase 103atPlatformWalletManager.swift:349. The mismatch site atmanager/load.rs:172-183now constructs the typed variant directly instead ofCorruptKind::DecodeError(format!(...))with an embedded hex-encoded wallet_id. The cross-check was also extended with a real structuralsnapshot_accounts_match_manifestcomparison (load.rs:338-366), and two new integration tests (rt_snapshot_account_set_mismatch_is_skipped,rt_snapshot_mismatch_skip_coexists_with_healthy_load) plus the retitledrt_snapshot_wallet_id_mismatch_is_skippedassert the typed variant. The residual FFI-path vacuity concern (both sides derive from the sameentry.accounts) is superseded by a new, more specific in-scope finding above and remains tracked out-of-scope for the manifest-authentication follow-up.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/mod.rs:8: Rehydration internals are exposed as public API with no external consumer (carried forward)
Carried forward from prior review rounds — re-verified STILL VALID at HEAD 5460c0e1 (`mod.rs:8` unchanged in the latest delta). `pub mod rehydrate;` publishes the seedless-restore helpers `build_watch_only_wallet` and `apply_persisted_core_state` as `platform_wallet::manager::rehydrate::*` to downstream crates (including the FFI crate boundary via `rs-platform-wallet-ffi`). These helpers mutate `ManagedWalletInfo` under load-path-specific reconstruction assumptions — including the unauthenticated-manifest trust boundary documented on `PlatformWalletPersistence::load`, `build_watch_only_wallet`, and now `load_from_persistor` itself (`manager/load.rs:71-77`), and the projection-fallback role the new PR-boundary contract explicitly marks as transitional. Re-verified in-tree: the only callers of `rehydrate::*` remain `super::rehydrate::build_watch_only_wallet` (`manager/load.rs:142`) and `super::rehydrate::apply_persisted_core_state` (`manager/load.rs:202`), both from the sibling private `mod load;`. Module-private visibility (`mod rehydrate;`) suffices; keeping it `pub` freezes internal recovery mechanics — including the manifest-authentication gap and the transitional projection path — as SDK/FFI-visible API without an intentional external contract. Since no external caller exists, this remains a drop-in visibility downgrade.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:338: New account-set cross-check is a manager-level self-consistency guard, not tamper detection — worth saying so explicitly
This delta extends the snapshot/row cross-check with `snapshot_accounts_match_manifest` (load.rs:338-366) comparing `account_manifest` against the `core_wallet_info` snapshot's `all_managed_accounts()` set. For the only in-tree producer of `core_wallet_info: Some(...)` — `build_wallet_start_state` in `rs-platform-wallet-ffi/src/persistence.rs` — both compared quantities derive from the same in-memory `wallet` built once in that call: `account_manifest` comes from `wallet.accounts.all_accounts()` (persistence.rs:3468-3476), and the snapshot's account set comes from `ManagedWalletInfo::from_wallet(&wallet, ...)` (persistence.rs:2901) where `wallet = Wallet::new_external_signable(network, entry.wallet_id, accounts)` (persistence.rs:2893). Both trace back to the single `entry.accounts` FFI array, so the check cannot fire against a tampered/corrupted SwiftData row — any tampering that alters one side alters the other identically. It does have real value: as the two new tests (`rt_snapshot_account_set_mismatch_is_skipped`, `rt_snapshot_mismatch_skip_coexists_with_healthy_load`) prove, it catches divergence in any future persister that populates the two fields via genuinely separate decode paths, or a manager-level wiring bug. But the doc comment on `snapshot_accounts_match_manifest` and the trust-boundary block on `load_from_persistor` should say explicitly that this check is a persister-implementation self-consistency guard, not a defense against a tampered store — otherwise readers will assume it closes the manifest-authentication gap the PR description already correctly tracks as a `rs-platform-wallet-storage` follow-up. Consider adding a one-line note that real defense against row tampering requires the tracked persisted MAC/commitment schema change.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:205: `PersisterLoad` transient/permanent classification is dropped at the FFI boundary — the point of the typed-error change never reaches Swift
This delta added `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` specifically so the persister's `is_transient()` / `PersistenceErrorKind` classification survives propagation instead of being flattened to a string, motivated by callers that would want to distinguish retryable failures. The two new integration tests (`rt_persister_load_transient_error_is_typed_and_retryable`, `rt_persister_load_permanent_error_is_typed_and_not_retryable`) confirm the Rust-side plumbing. But the only cross-language consumer today is `platform_wallet_manager_load_from_persistor` (`rs-platform-wallet-ffi/src/manager.rs:396-419`), which pipes `manager.load_from_persistor()`'s `Result` through `unwrap_result_or_return!` → `error.into()` → the blanket `From<PlatformWalletError> for PlatformWalletFFIResult` impl at `rs-platform-wallet-ffi/src/error.rs:205-244`. That impl explicitly enumerates `NoSpendableInputs`/`OnlyOutputAddressesFunded`/`OnlyDustInputs`, `WalletAlreadyExists`, and the three `Shielded*` variants, then falls through `_ => ErrorUnknown` for everything else — including the new `PersisterLoad`. Swift's `loadFromPersistor()` (`PlatformWalletManager.swift:377-388`) receives `code == ErrorUnknown` plus a stringified `Display` and throws a generic `PlatformWalletError`, so the retry-vs-fatal distinction the delta was built to preserve never reaches the only caller across a language boundary. If a real product need for transient-retry doesn't exist at the Swift layer, the typed variant is purely Rust-internal and the PR description overstates its scope. If it does exist, either add a dedicated `PlatformWalletFFIResultCode` (e.g. `ErrorPersisterLoadTransient`/`ErrorPersisterLoadPermanent`, or a single code + boolean out-param) and mirror it in Swift, or at minimum add an explicit `PersisterLoad` arm to the `From` impl that maps to a stable-code, transient-carrying variant.
Co-authored-by: Claudius the Magnificent AI, on behalf of lklimek <8431764+Claudius-Maginificent@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
The 5460c0e..8bcb192 delta is README-only (wording clarifications about read-model semantics and the single-writer invariant), so no new code-level findings arise from the latest delta. Reconciling the three prior findings from history/5460c0e1: all three are re-verified STILL_VALID at HEAD — pub mod rehydrate; at manager/mod.rs:8 unchanged, snapshot_accounts_match_manifest at manager/load.rs:325-366 unchanged with the same doc block, and the From<PlatformWalletError> for PlatformWalletFFIResult impl at rs-platform-wallet-ffi/src/error.rs:205-244 unchanged with PersisterLoad still falling through to ErrorUnknown at line 240. Carried forward as suggestions; no in-scope blockers.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:205-244: `PersisterLoad` transient/permanent classification is dropped at the FFI boundary
Re-verified STILL VALID at HEAD 8bcb1929 (latest delta is README-only; the impl and match arms at error.rs:205-244 are unchanged). The prior delta added `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` specifically so `PersistenceError::is_transient()` / `PersistenceErrorKind` survives propagation instead of being flattened to a string, and the Rust-side plumbing is exercised by `rt_persister_load_transient_error_is_typed_and_retryable` / `rt_persister_load_permanent_error_is_typed_and_not_retryable`. But the only cross-language caller today, `platform_wallet_manager_load_from_persistor` in `packages/rs-platform-wallet-ffi/src/manager.rs`, pipes the error through `unwrap_result_or_return!` → `error.into()` → the blanket `From<PlatformWalletError> for PlatformWalletFFIResult` impl, which explicitly enumerates `NoSpendableInputs`/`OnlyOutputAddressesFunded`/`OnlyDustInputs`, `WalletAlreadyExists`, and the three `Shielded*` variants, then falls through `_ => ErrorUnknown` at line 240 for everything else — including `PersisterLoad`. Swift's `loadFromPersistor()` receives `code == ErrorUnknown` plus a stringified `Display` and cannot distinguish a retryable persister hiccup from a fatal one. Either add dedicated `ErrorPersisterLoadTransient` / `ErrorPersisterLoadPermanent` codes (or a single code plus a `bool transient` out-param) with a Swift-side mirror, or downscope the PR description to note the typed variant is currently Rust-internal only.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:325-366: Snapshot/manifest cross-check doc reads as tamper detection but is a self-consistency guard
Re-verified STILL VALID at HEAD 8bcb1929 — `snapshot_accounts_match_manifest` (load.rs:338-366) and its doc block (load.rs:325-337) are unchanged. For the only in-tree producer of `core_wallet_info: Some(...)` — `build_wallet_start_state` in `rs-platform-wallet-ffi/src/persistence.rs` — both `account_manifest` (from `wallet.accounts.all_accounts()`) and the snapshot's account set (from `ManagedWalletInfo::from_wallet(&wallet, ...)`) derive from the same in-memory `wallet` built once from a single `entry.accounts` FFI array. Any tampering on the persisted row affects both sides identically, so this check cannot fire against a tampered SwiftData row; it catches persister wiring bugs and would catch a future persister that decodes the two fields via genuinely separate paths. The current doc frames the check as guarding against 'a different wallet [that] must not be consumed', which reads as tamper defense and undermines the PR's own tracking of the unauthenticated-manifest gap. One line clarifying that this is a persister-implementation self-consistency guard — and that real defense against row tampering requires the tracked `rs-platform-wallet-storage` MAC/commitment change — would prevent over-trust.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer
…ojection fallback ClientWalletStartState now always carries a full ManagedWalletInfo snapshot. Make core_wallet_info non-optional (Box<ManagedWalletInfo>) and remove the core_state / used_core_addresses projection fields. The FFI/iOS persister already always supplied a snapshot, so the None branch in load_from_persistor and the projection-replay path (apply_persisted_core_state / extend_pools_for_restored_addresses in rehydrate) were dead in production. Remove them and the unit tests that exercised only that removed logic. build_watch_only_wallet and its tests stay — they still feed the snapshot path. BREAKING CHANGE: ClientWalletStartState loses core_state and used_core_addresses; core_wallet_info is no longer Option. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pensator The storage layer now resolves per-UTXO account attribution from the core_derived_addresses table (address→account_index lookup over the addresses_derived delta), so core_utxos.account_index is no longer hardcoded to 0. Remove warn_if_non_default_account, non_default_account_index, and the inline aggregated-warn block in the BlockProcessed arm — the real index is already threaded through addresses_derived, which both event arms forward. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d as skip Turn LoadOutcome from a struct into a 3-state enum — Loaded (full), Partial (some loaded, some skipped), NoneUsable (rows present, all skipped) — so callers can tell a clean load from one that left wallets behind. Add `impl From<LoadOutcome> for Result<LoadOutcome, _>` (partial and nothing-usable map to the new PlatformWalletError::LoadIncomplete) per reviewer request, plus loaded()/skipped() accessors. An already-registered wallet (idempotency check, or WalletExists at insert) is now reported as a SkipReason::AlreadyRegistered skip instead of being silently folded into loaded, so the caller can distinguish "not freshly loaded" from a genuine load. FFI: adapt manager.rs to the accessor API and add reason code 300 (already-registered); LoadOutcomeFFI count pair encodes the 3-state. Swift: fix stale-skip-state bug — assign lastLoadSkippedWallets before the throwing check() so a failed load clears prior skip state; map reason code 300. BREAKING CHANGE: LoadOutcome is now an enum; its loaded/skipped fields become accessor methods. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tack Condense the load() trust-boundary note in traits.rs: drop the "known, currently-unaddressed gap" framing and state plainly that manifest- integrity verification is the storage layer's responsibility, out of this trait's scope. Point to the platform-wallet-storage manifest-integrity work rather than re-explaining the mechanism. Expand build_watch_only_wallet's doc with the concrete key-substitution attack: a tampered/untrusted-backup store swaps a valid account_xpub while keeping expected_wallet_id, so the rebuilt wallet derives receive addresses from the attacker's key under the original id and silently redirects incoming funds. State that this crate does not defend against it and cross-reference the traits.rs note. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…doc cleanups - slice_from_raw: reject a host-supplied len > isize::MAX (from_raw_parts bound) with an empty slice instead of risking UB, mirroring decode_cmx_array's guard. - build_wallet_start_state: wrap the Account::from_xpub failure in the MalformedXpubError marker so it classifies as MalformedXpub (101) like the sibling bincode-decode step, not the generic decode error (102). - Replace an iOS-specific "SwiftData" mention with technology-agnostic "corrupt persisted row" wording. - Drop the "(SECRETS.md: ...)" reference on the keyless-handoff comment and strengthen the invariant, naming private keys, unencrypted seeds, and passwords as excluded material. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ressions - hard_fail_rolls_back_already_loaded_healthy_wallet (in-crate, since the mid-batch hard-fail needs pub(crate) platform-address types + bimap): a platform-address restore failure on a later wallet must evict an already-inserted healthy wallet from both self.wallets and wallet_manager and return Err. Nothing covered this before. - rt_empty_first_run_is_clean_with_no_handler_calls: a fresh install (default ClientStartState, no rows) loads as LoadOutcome::Loaded with empty loaded/skipped and zero skip-handler calls. - rt_concurrent_loads_register_each_wallet_exactly_once: two concurrent load passes over a shared multi-wallet snapshot both succeed, each wallet registers exactly once, and overlaps surface as AlreadyRegistered skips (exercising the check-then-act idempotency window). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ydration Applies all 23 triaged review decisions (22 fix + 1 false_positive) from the consolidated PR #3692 review report, implemented by Bilby and QA'd clean by Marvin/Smythe/Adams (0 MEDIUM+ findings).
…let-rehydration # Conflicts: # Cargo.lock # Cargo.toml # packages/rs-sdk-ffi/src/mnemonic_resolver_core_signer.rs # packages/rs-sdk-ffi/src/signer_simple.rs
There was a problem hiding this comment.
Code Review
Cumulative re-review at head 14d83af. Latest delta cleanly removes the projection-fallback path, tightens LoadOutcome to a 3-state enum with a new AlreadyRegistered skip reason, adds an integer-overflow guard on the FFI slice helper, and typed-classifies malformed xpub failures — all backed by new rollback/idempotency/concurrency tests. Carried-forward prior findings: PersisterLoad FFI classification (STILL_VALID) and pub mod rehydrate (STILL_VALID). Fixed: prior snapshot/manifest doc framing (now explicitly a self-consistency check). New latent issue in the delta: the new From<LoadOutcome> for Result conversion routes AlreadyRegistered-only skips through LoadIncomplete, which turns idempotent repeat loads into Err for any caller using ? — currently unused, but the doc comment on LoadOutcome (load_outcome.rs:91-95) actively recommends this conversion.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] (failed: codex/gpt-5.5[high]:general, codex/gpt-5.5[high]:rust-quality, codex/gpt-5.5[high]:ffi-engineer) for general + rust-quality + ffi-engineer; verifier opus/opus.
🟡 2 suggestion(s) | 💬 1 nitpick(s)
Carried-forward prior findings (2)
-
[SUGGESTION]
packages/rs-platform-wallet-ffi/src/error.rs:205-244: STILL_VALID:PersisterLoadtransient/permanent classification is dropped at the FFI boundary — Carried forward from the prior review (byte-identical at 14d83af).PlatformWalletError::PersisterLoad(PersistenceError)deliberately preservesis_transient()/PersistenceErrorKind— this PR even addedrt_persister_load_transient_error_is_typed_and_retryable/..._permanent_...to pin that contract in Rust. But the blanketimpl From<PlatformWalletError> for PlatformWalletFFIResulthas no arm for it, so it falls through to_ => ErrorUnknown. Sinceplatform_wallet_manager_load_from_persistoris the primary Rust caller that surfacesPersisterLoadto Swift, the transient-vs-fatal signal a host would need to drive an app-launch retry loop (e.g. SQLite BUSY vs decode failure) collapses into a single opaque code with only theDisplaystring to inspect — the internal test coverage of the classification has no consumer at the boundary. The newLoadIncompletevariant falls... -
[NITPICK]
packages/rs-platform-wallet/src/manager/mod.rs:8: STILL_VALID:pub mod rehydrateexposes no public items — Carried forward from the prior review (unchanged at 14d83af). After the projection-fallback removal,rehydrate.rscontains exactly one non-test item,pub(super) fn build_watch_only_wallet, plus tests.mod.rs:8still declares itpub mod rehydrate;, so external crates canuse platform_wallet::manager::rehydrate;but reach nothing inside — the module contributes an empty entry to the crate's public rustdoc surface. Neighboursmod load;andmod wallet_lifecycle;are already private; matching them here keeps behaviour identical while removing the vestigial visibility.
New findings in the latest delta (1)
- [SUGGESTION]
packages/rs-platform-wallet/src/manager/load_outcome.rs:149-166:From<LoadOutcome> for Resultmisclassifies idempotentAlreadyRegistered-only skips asLoadIncomplete—from_partsroutes anyloaded.is_empty() && !skipped.is_empty()outcome intoNoneUsable, and the newFrom<LoadOutcome> for Resultimpl unconditionally converts that (andPartial) intoErr(PlatformWalletError::LoadIncomplete). That collapses two very different situations into the same failure: (a) real corruption (CorruptPersistedRow) and (b) a fully-successful idempotent repeat where every skip isAlreadyRegistered— the exact case the newrt_idempotent_repeat_restoretest pins as expected behaviour, and the caseload.rs's own doc frames as the steady-state repeat-load path. This mismatch is amplified byLoadOutcome's own rustdoc (load_outcome.rs:91-95), which actively recommendsResult::from/.into()"when an incomplete load should read as a failure" — the first Rust caller who takes that advice will see every steady-state repeat load reported asErr. Nothing...
Prior findings resolved (1)
- Snapshot/manifest cross-check doc reads as tamper detection but is a self-consistency guard — FIXED at 14d83af. The rustdoc on
snapshot_accounts_match_manifest(load.rs:310-317) now explicitly says 'A self-consistency check between two pieces of the same persisted row, not an authenticity guard' and points to the real trust boundary inbuild_watch_only_wallet. Directly addresses the prior framing concern.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/load_outcome.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load_outcome.rs:149-166: `From<LoadOutcome> for Result` misclassifies idempotent `AlreadyRegistered`-only skips as `LoadIncomplete`
`from_parts` routes any `loaded.is_empty() && !skipped.is_empty()` outcome into `NoneUsable`, and the new `From<LoadOutcome> for Result` impl unconditionally converts that (and `Partial`) into `Err(PlatformWalletError::LoadIncomplete)`. That collapses two very different situations into the same failure: (a) real corruption (`CorruptPersistedRow`) and (b) a fully-successful idempotent repeat where every skip is `AlreadyRegistered` — the exact case the new `rt_idempotent_repeat_restore` test pins as expected behaviour, and the case `load.rs`'s own doc frames as the steady-state repeat-load path. This mismatch is amplified by `LoadOutcome`'s own rustdoc (load_outcome.rs:91-95), which actively recommends `Result::from`/`.into()` "when an incomplete load should read as a failure" — the first Rust caller who takes that advice will see every steady-state repeat load reported as `Err`. Nothing in-tree calls the conversion yet (the FFI reads `.loaded()`/`.skipped()` directly), so this is latent rather than actively breaking today, but it's new API added in this PR. Either treat an outcome whose skips are all `AlreadyRegistered` as `Ok` (arguably it's a successful no-op, not incomplete), or drop the blanket `From` impl and `LoadIncomplete` variant until a real caller needs `?`-based incomplete-load handling that distinguishes corruption from idempotency.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:205-244: STILL_VALID: `PersisterLoad` transient/permanent classification is dropped at the FFI boundary
Carried forward from the prior review (byte-identical at 14d83af). `PlatformWalletError::PersisterLoad(PersistenceError)` deliberately preserves `is_transient()` / `PersistenceErrorKind` — this PR even added `rt_persister_load_transient_error_is_typed_and_retryable` / `..._permanent_...` to pin that contract in Rust. But the blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` has no arm for it, so it falls through to `_ => ErrorUnknown`. Since `platform_wallet_manager_load_from_persistor` is the primary Rust caller that surfaces `PersisterLoad` to Swift, the transient-vs-fatal signal a host would need to drive an app-launch retry loop (e.g. SQLite BUSY vs decode failure) collapses into a single opaque code with only the `Display` string to inspect — the internal test coverage of the classification has no consumer at the boundary. The new `LoadIncomplete` variant falls into the same catch-all (not currently reachable via any FFI entry point, but it widens the same gap). Add a dedicated `PlatformWalletFFIResultCode` for `PersisterLoad` and map it (or add an accessor for the underlying `is_transient()` bit) so the fidelity survives the crossing.
In `packages/rs-platform-wallet/src/manager/mod.rs`:
- [NITPICK] packages/rs-platform-wallet/src/manager/mod.rs:8: STILL_VALID: `pub mod rehydrate` exposes no public items
Carried forward from the prior review (unchanged at 14d83af). After the projection-fallback removal, `rehydrate.rs` contains exactly one non-test item, `pub(super) fn build_watch_only_wallet`, plus tests. `mod.rs:8` still declares it `pub mod rehydrate;`, so external crates can `use platform_wallet::manager::rehydrate;` but reach nothing inside — the module contributes an empty entry to the crate's public rustdoc surface. Neighbours `mod load;` and `mod wallet_lifecycle;` are already private; matching them here keeps behaviour identical while removing the vestigial visibility.
…malformed-xpub Add a unit test for slice_from_raw's isize::MAX overflow guard, asserting a non-null pointer with an over-bound length yields an empty slice without a dereference. Add an end-to-end test that drives build_wallet_start_state with a well-formed buffer that is not a decodable ExtendedPubKey and asserts the failure classifies as CorruptKind::MalformedXpub, covering the reclassification at its real call site rather than only the classification helper in isolation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dead rehydration variants Remove PlatformWalletError::LoadIncomplete and the `From<LoadOutcome> for Result<LoadOutcome, PlatformWalletError>` impl: nothing in-tree drives the `.into()`/`?` conversion — callers read LoadOutcome via loaded()/skipped() directly — so the impl was confusing latent API surface that also erased the skip-reason distinction between a harmless already-registered repeat and genuine corruption. Reintroduce a typed incomplete-load error only when a real caller needs it. LoadOutcome/load rustdoc now point callers at the skip reasons instead of the removed conversion. Also delete the three dead rehydration error variants (RehydrationTopologyUnsupported, RehydrationPoolMismatch, RehydrationPoolTypeMismatch) — no constructors remain in the workspace — and the AddressPoolType import they were the only users of. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The module's only non-test item is `pub(super) fn build_watch_only_wallet`, used solely from `manager::load`, so it exposes nothing externally usable. Narrow the declaration to `mod rehydrate;`, matching the private sibling `mod load;` / `mod wallet_lifecycle;`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, latent API removal) Fixes the 5 LOW findings from Phase 3 QA plus a bot-flagged module-visibility nit, all triaged by the maintainer. Removes the latent LoadIncomplete conversion instead of documenting it, per an independent bot review raising the same idempotent/corruption collapse concern.
…e afcff156 Three FixedKeySigner test mocks implemented key_wallet::signer::Signer's extended_public_key, which afcff156 moved into a separate ExtendedPubKeySigner subtrait — the same break already fixed in rs-sdk-ffi's mnemonic_resolver_core_signer.rs, missed here because the post-merge verification was scoped to rs-platform-wallet/-ffi/rs-sdk-ffi and never ran a workspace-wide check. Nothing in rs-dpp calls .extended_public_key() on these mocks or requires ExtendedPubKeySigner, so the method is dead weight, not a missing impl — deleted outright along with the now-unused ExtendedPubKey imports. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Cumulative re-review at head f8c9722 reconciling the three prior findings from 14d83af. Two prior findings are FIXED by outright removal in this delta: prior-1 (From<LoadOutcome> for Result misclassification) — the impl and LoadIncomplete variant are deleted with no remaining workspace references; and prior-3 (pub mod rehydrate) — demoted to private mod rehydrate; at manager/mod.rs:8. Prior-2 (PersisterLoad transient/permanent classification dropped at the FFI boundary) is carried forward STILL_VALID: packages/rs-platform-wallet-ffi/src/error.rs:205-244 was untouched, and the blanket From<PlatformWalletError> for PlatformWalletFFIResult still routes every PersisterLoad(_) through _ => ErrorUnknown at line 240 despite the Rust-side error preserving is_transient(). No new latest-delta findings surfaced from any reviewer; the delta's two added FFI tests (slice_from_raw overflow guard, MalformedXpub end-to-end classification) are appropriately scoped regressions.
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/error.rs:205-244: `PersisterLoad` transient/permanent classification is dropped at the FFI boundary
Carried forward from the prior review — `packages/rs-platform-wallet-ffi/src/error.rs` was not touched by the 14d83af1..f8c9722c delta, verified byte-identical at head.
`PlatformWalletError::PersisterLoad(#[from] PersistenceError)` deliberately preserves `PersistenceError::is_transient()` and `PersistenceErrorKind` — the crate has dedicated tests (`rt_persister_load_transient_error_is_typed_and_retryable` / `..._permanent_...`) pinning that contract on the Rust side. But the blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` at lines 205-244 explicitly enumerates only `NoSpendableInputs`/`OnlyOutputAddressesFunded`/`OnlyDustInputs`, `WalletAlreadyExists`, and the three `Shielded*` variants; everything else — including `PersisterLoad(_)` — falls through the `_ => ErrorUnknown` catch-all at line 240.
`platform_wallet_manager_load_from_persistor` is the primary Rust caller that can surface `PersisterLoad` to Swift via this blanket conversion. A host that wants to distinguish a retryable persister hiccup (SQLite BUSY, iOS keychain locked at launch) from a permanent decode failure has no typed signal to branch on — only the rendered `Display` string, which is not a stable contract. The typed retry classification the Rust side goes out of its way to preserve therefore never reaches the host, at exactly the boundary that motivated the seedless-load design.
Suggested fix: add a dedicated `PlatformWalletFFIResultCode::ErrorPersisterLoad` (or split `ErrorPersisterLoadTransient` / `ErrorPersisterLoadPermanent`) with an explicit match arm inspecting `err.is_transient()`, or expose the `is_transient()` bit through a separate FFI accessor so the classification survives the crossing.
Source: reviewers: opus (general), claude-sonnet-5 (general, failed_or_unparseable), gpt-5.5[high] (general, failed_or_unparseable), opus (rust-quality), claude-sonnet-5 (rust-quality), gpt-5.5[high] (rust-quality, failed_or_unparseable), opus (ffi-engineer), claude-sonnet-5 (ffi-engineer), gpt-5.5[high] (ffi-engineer, failed_or_unparseable); verifier: opus; specialists: rust-quality, ffi-engineer
There was a problem hiding this comment.
Code Review
Cumulative re-review at head 83d9eff. Latest delta (f8c9722..83d9eff) is a mechanical cleanup: three test-only FixedKeySigner impls in rs-dpp lose the obsolete extended_public_key method (and its unused ExtendedPubKey import) after the pinned rust-dashcore afcff156 split extended_public_key into a separate ExtendedPubKeySigner supertrait. That change is correct, test-only, and does not touch any FFI/consensus surface — no new findings. The single carried-forward finding (prior-1: PersisterLoad transient/permanent classification collapsed to ErrorUnknown at the FFI boundary) is re-verified STILL_VALID: packages/rs-platform-wallet-ffi/src/error.rs:205-244 is unchanged, and PlatformWalletError::PersisterLoad(#[from] PersistenceError) still exists at packages/rs-platform-wallet/src/error.rs:23, so the typed retryable/permanent signal never crosses to the Swift loadFromPersistor() caller.
Source: reviewers opus/opus, sonnet5/claude-sonnet-5, codex/gpt-5.5[high] (failed: codex/gpt-5.5[high]:general, codex/gpt-5.5[high]:rust-quality, codex/gpt-5.5[high]:ffi-engineer) for general + rust-quality + ffi-engineer; verifier opus/opus.
🟡 1 suggestion(s)
Carried-forward prior findings (1)
- [SUGGESTION]
packages/rs-platform-wallet-ffi/src/error.rs:205-244: STILL_VALID:PersisterLoadtransient/permanent classification is dropped at the FFI boundary — Re-verified at head83d9effc: this file is byte-identical to the prior head, whilePlatformWalletError::PersisterLoad(#[from] PersistenceError)still preservesPersistenceError::is_transient()on the Rust side and still falls through the blanket FFI conversion's_ => ErrorUnknownarm. The latest delta only touched rs-dpp signer test stubs, so there are no new latest-delta findings.
🤖 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:205-244: Carried forward (STILL_VALID): PersisterLoad transient/permanent classification is dropped at the FFI boundary
Re-verified at head 83d9effc — this file is byte-identical to the prior head (the f8c9722c..83d9effc delta only touched rs-dpp signer test stubs).
`PlatformWalletError::PersisterLoad(#[from] PersistenceError)` at `packages/rs-platform-wallet/src/error.rs:23` deliberately preserves `PersistenceError::is_transient()` / `PersistenceErrorKind` so a `SQLITE_BUSY` hiccup or an iOS keychain-locked-at-launch race stays distinguishable from a permanent decode failure. The Rust side pins that contract with dedicated tests (`rt_persister_load_transient_error_is_typed_and_retryable` / `..._permanent_...` in `packages/rs-platform-wallet/tests/rehydration_load.rs`).
But the blanket `impl From<PlatformWalletError> for PlatformWalletFFIResult` at lines 205-244 only special-cases `NoSpendableInputs` / `OnlyOutputAddressesFunded` / `OnlyDustInputs`, `WalletAlreadyExists`, and the three `Shielded*` variants; `PersisterLoad(_)` falls through the `_ => ErrorUnknown` catch-all at line 240. `platform_wallet_manager_load_from_persistor` is the primary caller that surfaces `PersisterLoad` to Swift via this blanket conversion, so a Swift host receives `code == ErrorUnknown` plus only a `Display` string — the retry-vs-fatal signal never crosses the boundary the seedless-load design was built for.
Fix options: (a) add dedicated `PlatformWalletFFIResultCode::ErrorPersisterLoadTransient` / `ErrorPersisterLoadPermanent` variants (or a single `ErrorPersisterLoad` with a `bool transient` out-param) and a match arm here that inspects `err.is_transient()`, mirrored on the Swift side; or (b) if there is no near-term Swift consumer, downscope the typed variant in the PR description as Rust-internal only. Either shape closes the gap without disturbing existing arms.
4 participants
Why this PR exists
ClientWalletStartStateand rewrites every in-tree consumer in the same PR. The storage-reader half is feat(platform-wallet): persistence readers + seedless load() wiring (split from #3692) #3968 (also independent); the two combine on thedash-evo-toolintegration branch.What was done (
rs-platform-wallet+-ffi+ swift)PlatformWalletManager::load_from_persistor()reconstructs every persisted wallet from its full snapshot (ClientWalletStartState.core_wallet_info):Wallet::new_watch_only(...)from stored account xpubs — no seed, no signing-key derivation on the load path.load_from_persistornow returns a 3-state outcome (Loaded/Partial/NoneUsable) instead of a boolean-ish result: an already-registered wallet is reported as a distinct skip reason rather than silently folded into "loaded", and a structurally-corrupt row is skipped, not fatal.ClientWalletStartState, load-result types).fixexcept one confirmedfalse_positive): removed the obsoletewarn_if_non_default_accountcompensator now that per-account attribution is real; reframed the manifest trust-boundary andsnapshot_accounts_match_manifestdocs from "tamper detection" to accurate self-consistency framing; added an integer-overflow guard to the FFIslice_from_rawhelper; fixed xpub-error misclassification (MalformedXpubnow distinct from generic decode errors); fixed a Swift-side ordering bug where a failed load could leave stale skip-state from a prior successful load.fix): removed the latentFrom<LoadOutcome> for Result<LoadOutcome, PlatformWalletError>conversion and theLoadIncompleteerror variant entirely — nothing in-tree used it, and it silently collapsed a harmless idempotent repeat load and a genuine total-corruption failure into the sameErr; callers use.loaded()/.skipped()and inspect skip reasons directly instead. Deleted the three now-unreachablePlatformWalletErrorvariants (RehydrationTopologyUnsupported/RehydrationPoolMismatch/RehydrationPoolTypeMismatch) left over from the projection-fallback removal above. Mademanager::rehydratea private module (it exposed nothing public). Added regression tests for theslice_from_rawoverflow guard and an end-to-endMalformedXpubclassification test.Known follow-up needed (not fixed in this PR)
build_watch_only_wallet/load_from_persistortrust the persisted account manifest as-is — no cryptographic binding towallet_id. Verified the root xpub can't be recovered from what's persisted (hardened derivation only), so there's no in-crate fix; the real fix is a persisted commitment/MAC inrs-platform-wallet-storage, tracked by feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992.last_applied_chain_lockis restored from local storage without re-verifying the BLS/quorum signature; bounded by downstream network re-verification of the asset-lock proof, so low risk today but same underlying gap.Testing
cargo fmt --all --check;cargo clippy --all-targets --all-features -- -D warningsclean onplatform-wallet,platform-wallet-ffi, andrs-sdk-ffi.cargo testonplatform-wallet+platform-wallet-ffi+rs-sdk-ffi: 693 passed, 0 failed across 16 test binaries at the point the review-comment triage batch and thev4.1-devmerge landed (including new rollback/empty-first-run/concurrent-load regression tests). After the QA follow-up batch:cargo testonplatform-wallet+platform-wallet-ffi: 365 passed, 0 failed across 12 test binaries, including the new overflow-guard and end-to-endMalformedXpubtests. Independently re-verified by a separate QA pass (test execution, security audit, and structural/plan-completion review); the 5 LOW-severity findings it surfaced were triaged and fixed in the QA follow-up batch above — no MEDIUM+ findings at any point. Swift changes hand-verified against the Rust FFI contract (no Swift toolchain in this environment).Breaking changes
Two, both against this PR's own prior (unreleased) shape, targeting
v4.1-dev:ClientWalletStartState.core_wallet_infois now a requiredBox<ManagedWalletInfo>(wasOption<...>); the keyless-projection fallback fields are removed.load_from_persistor's outcome is now a 3-state enum (Loaded/Partial/NoneUsable) rather than the previous 2-state shape; the exported FFI symbol name andLoadOutcomeFFIstruct layout are unchanged (additive only).Checklist
🤖 Co-authored by Claudius the Magnificent AI Agent