feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968
Open
Claudius-Maginificent wants to merge 138 commits into
Open
feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968Claudius-Maginificent wants to merge 138 commits into
Claudius-Maginificent wants to merge 138 commits into
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR overhauls ChangesSecrets: Tier-2 envelope, error taxonomy, EncryptedFileStore hardening
SQLite: wallets rename, keyless rehydration, persister hardening
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant wrap
participant derive_key
participant AEAD
participant unwrap
Caller->>wrap: plaintext, wallet_id, label, password?
wrap->>wrap: enforce MAX_PLAINTEXT_LEN
wrap->>derive_key: passphrase, random salt, KdfParams
derive_key-->>wrap: SecretBytes key
wrap->>AEAD: seal(key, nonce, plaintext, Tier2Aad)
AEAD-->>wrap: ciphertext
wrap-->>Caller: bincode Envelope
Caller->>unwrap: blob, wallet_id, label, password?
unwrap->>unwrap: bincode decode + version check
unwrap->>derive_key: passphrase, stored salt, KdfParams
derive_key-->>unwrap: SecretBytes key
unwrap->>AEAD: open(key, nonce, ciphertext, Tier2Aad)
AEAD-->>unwrap: plaintext or WrongPassword
unwrap-->>Caller: SecretBytes
sequenceDiagram
participant SqlitePersister
participant wallets_list_ids
participant accounts_load_state
participant core_state_load_state
participant identities_load_state
participant asset_locks_load_unconsumed
participant contacts_load_state
participant identity_keys_load_state
SqlitePersister->>wallets_list_ids: enumerate wallet IDs
loop per wallet
SqlitePersister->>accounts_load_state: account_manifest
SqlitePersister->>core_state_load_state: CoreChangeSet
SqlitePersister->>identities_load_state: IdentityManagerStartState
SqlitePersister->>asset_locks_load_unconsumed: unused_asset_locks
SqlitePersister->>contacts_load_state: ContactChangeSet
SqlitePersister->>identity_keys_load_state: IdentityKeysChangeSet
end
SqlitePersister-->>Caller: ClientStartState.wallets
Possibly related issues
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
changed the title
3 tasks
…persistor [#3692, clean on v3.1-dev] Squashed net-diff of feat/platform-wallet-rehydration onto v3.1-dev base (1653b89). Includes all merged commits: • changeset: CoreChangeSet, ClientWalletStartState, addresses_derived wiring • rehydrate: seedless watch-only wallet rebuild + apply_persisted_core_state • load_outcome: LoadOutcome / SkipReason / CorruptKind • manager/load: load_from_persistor implementation • manager/mod: PlatformWalletManager wiring • events: PlatformEvent + on_wallet_skipped_on_load concrete handler • error: RehydrateRowError relocated from manager::rehydrate • core_bridge: warn_if_non_default_account generalised to &[T] slice • FFI: persistence + manager bindings • Swift: PlatformWalletManager load() bridging • tests: rehydration_load integration suite • misc: .cargo/audit.toml, .gitignore fmt + clippy (-D warnings) + cargo test: all pass. Tree verified byte-for-byte identical to feat/platform-wallet-rehydration HEAD. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…3968, independent on v3.1-dev] Storage-crate half of the rehydration work, rebuilt to stand alone on v3.1-dev: SqlitePersister::load() wiring + per-area readers (accounts, core_state, identities, asset_locks, contacts, identity_keys) that reconstruct the keyless ClientWalletStartState. Independence on v3.1-dev required two deliberate stubs — the reshaped ClientWalletStartState drops wallet/wallet_info, breaking two base consumers; both are resolved by #3692 in the dash-evo-tool integration: - manager/load.rs: whole-body todo!("keyless rehydration lands in #3692") - ffi/persistence.rs: tail-only todo!("seeded FFI restore path lands in #3692") — keeps the 8 builder helpers live (no dead_code under -D warnings) and minimizes the #3692 merge conflict Cross-crate manager-apply e2e tests in sqlite_core_state_reader.rs are gated behind a new off-by-default `rehydration-apply` feature (enabled in the integrated stack); storage-level load_state assertions run standalone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
52cdad9 to
83f7d4f
Compare
lklimek
force-pushed
the
feat/platform-wallet-storage-rehydration
branch
from
3d57f73 to
2f2a74a
Compare
5 tasks
…ncrete handlers only (#3692 review) Remove `PlatformEventHandler::on_platform_event` (the generic backward-compat escape hatch) and `PlatformEventManager::on_platform_event` entirely. `on_wallet_skipped_on_load` now has a plain no-op default, matching the pattern used by every other concrete handler on the trait. `PlatformEvent` is kept: it is `pub`, re-exported from `lib.rs`, and not present in the FFI or Swift layer — no dead-code warning applies to public items, and removing it would be a needless churn of the public API. Not a breaking change vs v3.1-dev: `on_platform_event` was only ever on this branch (absent from origin/v3.1-dev). Doc comments in manager/load.rs and manager/mod.rs updated to point to `on_wallet_skipped_on_load` instead of the removed method/event wrapper. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g-seed gate (#3692 review) The rehydrate module header and the rehydration_load test header both claimed the wrong-seed gate was "deferred to separate FFI work and is not part of this path." That gate now exists on the resolver-backed signing entrypoints (sign_with_mnemonic_resolver + the FFI resolver sign path). Reword to say wrong-seed validation lives there; the seedless load path never sees the seed. Docs-only, no behaviour change. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt HashSet (#3692 review) apply_persisted_core_state filtered new_utxos against spent_utxos with a nested `any()`, making the unspent projection O(new × spent). Collect the spent outpoints into a HashSet once and do O(1) membership lookups — behaviour is identical (Copy OutPoint, Hash + Eq), just linear. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ehydrate (#3692 review) The FFI build_wallet_start_state decoded the persisted core address pools (used flags + derived addresses) into a temp wallet_info, but the keyless ClientWalletStartState forwarded only the account manifest + UTXO/height projection — the pool used-state was dropped. apply_persisted_core_state then marked addresses used ONLY from currently-unspent UTXOs, so a previously-used address whose funds were since spent came back marked unused and could be handed out again as a fresh receive address: an address-reuse privacy leak. Carry the used-state through: - Add ClientWalletStartState::used_core_addresses (Vec<Address>, empty default) — a flat snapshot of every pool-marked-used address. - Populate it in the FFI projection from the already-decoded pools. - apply_persisted_core_state now marks used the UNION of unspent-UTXO addresses + used_core_addresses (new param), deriving deep slots via the existing horizon walk. Renamed extend_pools_for_restored_utxos -> extend_pools_for_restored_addresses since it now resolves both sources. Empty used_core_addresses preserves the prior unspent-only behaviour, so the native/SQLite path is unchanged until #3968 wires its pool readers to populate this field (cross-PR follow-up; no regression). Also fixes the O(new x spent) unspent filter via an outpoint HashSet. Test: rehydration_restores_persisted_used_state_for_spent_out_address asserts an in-window and a deep spent-out address come back used, and that the empty-snapshot baseline does NOT mark them. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t wallet on load (#3692 review) load_from_persistor mapped EVERY insert_wallet error (including key_wallet_manager::WalletError::WalletExists) to a fatal WalletCreation + 'load break + full rollback. So a second load_from_persistor — or a load run while the wallet is already in memory — aborted the whole batch instead of being a no-op. Match WalletExists specifically and treat it as already-satisfied: record the wallet as loaded and `continue` to the next row. It was not inserted by this pass, so it stays out of the rollback set and a later hard-fail never evicts the pre-existing wallet. Mirrors the create-path idempotent handling in wallet_lifecycle. Test: rt_idempotent_repeat_restore loads the same persister twice and asserts the second call returns Ok with the wallet still present. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ting the batch (#3692 review) FFIPersister::load looped `build_wallet_start_state(entry)?`, so ONE corrupt SwiftData row (e.g. a malformed account_xpub that aborts decode) failed the ENTIRE load() — every wallet, every launch. The manager already documents per-wallet skip (LoadOutcome::skipped + on_wallet_skipped_on_load, returns Ok), but the FFI never reached it. Make the FFI loop per-entry resilient: on a per-row build failure record the wallet as skipped and continue. Errors from build_wallet_start_state are inherently per-row (decode/projection of THAT entry), so this never swallows a whole-load failure. The skip travels to the manager through a new ClientStartState::skipped channel (Vec<(WalletId, SkipReason)>, empty default); load_from_persistor folds it into LoadOutcome::skipped and fires on_wallet_skipped_on_load. Reason is CorruptPersistedRow{DecodeError} — PersistenceError's Display is structural (no row bytes / key material). Cross-PR: ClientStartState derives Default so #3968's `::default()` build still compiles; a destructure there needs `skipped: _` (follow-up). Test: rt_persister_skipped_folds_into_outcome asserts a persister-rejected row surfaces in LoadOutcome::skipped + fires the event while the healthy wallet still loads and the call returns Ok. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stive (#3692 review) Semver hygiene for the new, unreleased load surface so future variants don't break downstream matches: add #[non_exhaustive] to SkipReason, CorruptKind, LoadOutcome (load_outcome.rs) and PlatformEvent (events.rs). Consequence: the FFI skip_reason_code match (a downstream crate) is no longer exhaustive over the now-non_exhaustive SkipReason/CorruptKind, so add catch-all arms mapping future variants to generic codes (199 corrupt kind, 200 skip reason) until the mapping is extended. matches!() in tests is unaffected (it carries an implicit wildcard). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dex pool extension on rehydrate (#3692 review) QA flagged that the existing real-manager rehydration test defaults the address-pool payload and is structurally blind to #1, and that the pool-DEPTH fix (dash-evo-tool#829 Bug 2 / PR #830) had no regression guard. Add two distinct, focused tests through apply_persisted_core_state: - rehydration_used_state_survives_spent_utxo (#1, address-reuse): builds a ClientWalletStartState whose in-window address received funds that were then SPENT (new_utxos cancelled by spent_utxos → zero balance) and routes used_core_addresses through the field. Asserts the in-window + a deep (idx 30) address come back marked USED even with zero balance, and that the empty-snapshot baseline does NOT mark them. Replaces the weaker no-UTXO variant so the used flag is proven independent of a live UTXO. - rt_deep_index_utxos_extend_pools_on_rehydration (DEPTH): unspent UTXOs on walkable ladders past the eager 0..=gap_limit window (external -> idx 84, internal -> idx 90). Asserts the deep slots are derived into their pools and Sum(per-address visible) == balance.total == Sum(persisted) — no deep-index undercount. Test-only; the production fix already exists. Also: drop the stale "(from wallet_metadata)" table reference on the ClientWalletStartState::network doc (backend-agnostic now). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
review) Remove rt_deep_index_utxos_extend_pools_on_rehydration: the deep-index pool-extension scenario is already guarded by the pre-existing rehydration_extends_pools_to_cover_deep_index_utxos and rehydration_coinjoin_single_pool_deep_index. The existing 30->60 horizon extension already exercises the recursive walk, so a deeper ladder added no new code path — pure duplication. Keeps the #1 address-reuse test (rehydration_used_state_survives_spent_utxo). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eview) After the on_platform_event removal, the `PlatformEvent` enum had zero references repo-wide — events flow through the concrete `PlatformEventHandler` methods (`on_wallet_skipped_on_load`, etc.), not a dispatched enum. Remove the enum (and the `#[non_exhaustive]` just added to it) plus its `lib.rs` re-export. Its only variant, `WalletSkippedOnLoad`, went with it; the `on_wallet_skipped_on_load(wallet_id, &SkipReason)` handler and `SkipReason` itself stay. No imports orphaned — `SkipReason` and `WalletId` are still used by `PlatformEventHandler` / `PlatformEventManager`. Verified: `git grep PlatformEvent` over rs-platform-wallet, -ffi and swift-sdk is empty (only `PlatformEventHandler` / `PlatformEventManager` remain). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
Collaborator
|
✅ Review complete (commit f2779ca) |
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The PR adds the storage-side keyless load readers, but it also replaces two externally reachable restore paths with unconditional panics. The new rehydration readers are mostly wired, but several fail-hard corruption checks are missing where typed SQLite columns can disagree with decoded blobs.
🔴 2 blocking | 🟡 6 suggestion(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key —load_state()selectsidentity_idbut discards it, then decodesentry_bloband routes the restored identity usingentry.id. The writer rejectsIdentityEntryvalues whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader shou... - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys — The contacts reader keys pending rows from(owner_id, contact_id)but stores the decodedContactRequestwithout checking its sender and recipient IDs. During apply, sent requests are inserted underentry.request.recipient_idand incoming requests underentry.request.sender_id, so a row wh...
🤖 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`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:13-15: Public manager restore API now panics
`load_from_persistor()` is a public restore entry point returning `Result<(), PlatformWalletError>`, but this PR replaces the previous implementation with `todo!()`. The exported C ABI function `platform_wallet_manager_load_from_persistor` calls this method directly, and the Swift `loadFromPersistor()` wrapper calls that exported function, so any app invoking persisted wallet restore aborts instead of receiving a typed error. If this branch intentionally defers keyless manager rehydration to #3692, the public API still needs to fail closed with an error rather than panic across the FFI boundary.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3389-3390: FFI persister load panics after receiving restore rows
`FFIPersister::load()` calls `build_wallet_start_state()` for every wallet returned by the Swift `on_load_wallet_list_fn` callback, and this function now reaches an unconditional `todo!()` after partially reconstructing the entry. This path is externally reachable through restore and shielded binding flows that call `persister.load()`. A panic here can unwind toward `extern "C"` callers and abort the process instead of returning the existing `PersistenceError`/`PlatformWalletFFIResult` failure path.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:168-169: Identity-key reader does not verify decoded entries match row columns
`load_state()` reconstructs `(identity_id, key_id)` from the SQL row, decodes `public_key_blob`, and inserts the decoded entry without checking that the blob carries the same identity, key id, wallet id, or public-key hash. The apply path later ignores the changeset map key and routes by fields from the decoded `IdentityKeyEntry`, so a semantically inconsistent row can attach a public key to the wrong identity or carry a hash that disagrees with the indexed column. Mirror the writer-side consistency checks on read before inserting into the changeset.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:316-325: Oversized BLOB rows are materialized before the size cap runs
The new load readers fetch BLOB columns directly into `Vec<u8>` and only then call `blob::decode()`, whose 16 MiB cap runs after rusqlite has already allocated and copied the value. A restored or locally modified SQLite DB can therefore store a huge `record_blob` or other `*_blob` value that passes SQLite integrity checks and forces large process allocations on startup before returning `BlobTooLarge`. Use a shared bounded read helper or select `length(blob_column)` first, as the KV path already does, before materializing BLOB contents.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs (1)
165-180: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCount
identity_keysbywallet_idnow that the table is wallet-scoped.
identity_keysmoved onto(wallet_id, identity_id, key_id), but this smoke test still routes it through thevia_identitypath. That means the assertion would still pass if the row were written with the wrongwallet_idas long asidentity_idmatched, so the new schema contract is not actually being exercised here.Suggested fix
let via_identity = [ - "identity_keys", "token_balances", "dashpay_profiles", "dashpay_payments_overlay", ];🤖 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-storage/tests/sqlite_migrations.rs` around lines 165 - 180, The smoke test still treats identity_keys as identity-scoped, but the schema now scopes it by wallet_id. Update the test logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query path instead of the via_identity branch, while keeping the other tables that still depend on identities routed through identity_id. Use the existing via_identity handling in the loop over cases to locate and adjust the count_sql selection.packages/rs-platform-wallet-storage/SCHEMA.md (1)
507-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe soft-cascade note overstates cleanup for identity-scoped metadata.
meta_identityandmeta_tokendo not carrywallet_id, so a wallet delete only reaches them through existingidentitiesrows. If metadata was written before anidentitiesrow ever existed, that cleanup path never fires; the orphan-metadata section above already documents exactly that case.Suggested wording
-`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that -brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, -`meta_platform_address`) by `wallet_id`, and the FK cascade through -`identities` fires a per-identity trigger that brooms `meta_identity` + -`meta_token` by `identity_id`. Both legs key on the id alone, so a wallet -delete cleans its metadata transitively whether or not the typed parent -was ever written and regardless of any contact's lifecycle state. +`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that +brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, +`meta_platform_address`) by `wallet_id`, and the FK cascade through +existing `identities` rows fires a per-identity trigger that brooms +`meta_identity` + `meta_token` by `identity_id`. That means wallet-scoped +metadata is cleaned regardless of typed-parent existence, while +identity-scoped metadata still requires an `identities` row to exist.🤖 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-storage/SCHEMA.md` around lines 507 - 513, The soft-cascade description in SCHEMA.md overstates what a wallet delete cleans up for identity-scoped metadata. Update the note near the wallet/identity trigger flow to say that `wallets` deletion only reaches `meta_identity` and `meta_token` through existing `identities` rows and that orphan metadata written before an `identities` row exists is not covered; align the wording with the existing orphan-metadata section and reference the `wallets` trigger and the `identities` FK cascade path.packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs (1)
27-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed on corrupted platform-payment registration rows.
This helper trusts the typed
account_indexcolumn but never verifies that the decodedAccountRegistrationEntryis actually aPlatformPaymententry for that same index.all_platform_payment_registrations()feedsplatform_addrs::load_all(), so a tampered row will currently rehydrate under the typed index with the blob's xpub instead of trippingAccountRegistrationEntryMismatch.Suggested fix
fn decode_platform_payment_row( account_index: i64, xpub_bytes: &[u8], ) -> Result<PlatformPaymentRegistration, WalletStorageError> { let account_index = crate::sqlite::util::safe_cast::i64_to_u32( "account_registrations.account_index", account_index, )?; let entry: AccountRegistrationEntry = blob::decode(xpub_bytes)?; + if account_type_db_label(&entry.account_type) != "platform_payment" + || account_index(&entry.account_type) != account_index + { + return Err(WalletStorageError::AccountRegistrationEntryMismatch); + } Ok((account_index, entry.account_xpub)) }🤖 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-storage/src/sqlite/schema/accounts.rs` around lines 27 - 36, `decode_platform_payment_row` currently decodes the blob and returns the typed `account_index` without checking that the `AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update this helper to validate the decoded `AccountRegistrationEntry` matches the expected `PlatformPayment` variant and index, and return `AccountRegistrationEntryMismatch` if it does not. Keep the existing `safe_cast::i64_to_u32` conversion, but make `all_platform_payment_registrations()` fail closed by rejecting any corrupted or mismatched row instead of rehydrating it.packages/rs-platform-wallet-storage/src/sqlite/backup.rs (2)
243-263: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not delete WAL/SHM before the replacement is guaranteed.
If sibling removal succeeds and
tmp.persist(dest_db_path)then fails, the original main DB remains but its WAL/SHM may be gone, losing committed WAL-mode state. The restore path needs a rollback-safe swap strategy or a SQLite-native restore that does not destructively unlink siblings before the main replacement succeeds.🤖 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-storage/src/sqlite/backup.rs` around lines 243 - 263, The restore flow in `backup.rs` removes `-wal`/`-shm` siblings before `tmp.persist(dest_db_path)`, which can leave the original DB intact but its WAL-mode state lost if persist fails. Change the `restore` logic to use a rollback-safe replacement strategy: do not unlink siblings until the destination swap is guaranteed, or replace the whole SQLite set atomically via a SQLite-native restore path. Keep the fix localized around the sibling cleanup and `tmp.persist` sequence so the operation remains all-or-nothing.
361-374: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply
keep_last_nas a floor, not a ceiling.With both
keep_last_nandmax_ageset, line 373 still requirespass_count, so backups beyond the newest N are deleted even when they are withinmax_age. That contradicts the new floor semantics.Proposed fix
- let pass_count = match policy.keep_last_n { - Some(n) => idx < n, - None => true, - }; let pass_age = match policy.max_age { Some(max) => now.duration_since(ts).map(|d| d <= max).unwrap_or(true), - None => true, + None => policy.keep_last_n.is_none(), }; - if within_floor || (pass_count && pass_age) { + if within_floor || pass_age {🤖 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-storage/src/sqlite/backup.rs` around lines 361 - 374, In backup pruning logic in the `retain_backups` flow, `keep_last_n` is still being treated like a ceiling because the deletion condition requires `pass_count` even when `max_age` is also set. Update the condition around `within_floor`, `pass_count`, and `pass_age` so that the newest N backups are always kept as a floor and any backup within the age limit is also retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in this block.packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)
143-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate typed identity columns against the blob during load.
load_stateignores the selectedidentity_id, so a corrupted row whose typed column andentry_blob.iddiverge is silently rehydrated under the blob value. Also reject a blobwallet_idthat disagrees with the scoped wallet.Proposed fix
- let _identity_id: Vec<u8> = row.get(0)?; + let identity_id: Vec<u8> = row.get(0)?; let payload: Vec<u8> = row.get(1)?; let tombstoned: i64 = row.get(2)?; if tombstoned != 0 { continue; } + let typed_id = <[u8; 32]>::try_from(identity_id.as_slice()) + .map_err(|_| WalletStorageError::blob_decode("identities.identity_id is not 32 bytes"))?; let entry: IdentityEntry = blob::decode(&payload)?; + if entry.id.as_bytes() != &typed_id { + return Err(WalletStorageError::IdentityEntryIdMismatch); + } + if let Some(entry_wallet_id) = entry.wallet_id { + if entry_wallet_id != *wallet_id { + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: entry_wallet_id, + }); + } + } let managed = managed_identity_from_entry(&entry, wallet_id);🤖 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-storage/src/sqlite/schema/identities.rs` around lines 143 - 150, The load path in load_state is trusting the blob too much and currently ignores the selected identity_id, so mismatched typed columns can be silently rehydrated under the blob value. Update the row handling in load_state to validate that the typed identity_id matches entry_blob.id before decoding into IdentityEntry, and also verify the blob wallet_id matches the wallet_id scope passed into managed_identity_from_entry. If either check fails, reject the row instead of continuing.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
299-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the open-path registry before restore.
restore_from_innercan replacedest_db_pathwhile a liveSqlitePersisterin this process still owns the same DB. Check the registry up front and returnAlreadyOpen; otherwise the live handle/buffer can diverge from the restored file.Proposed fix outline
+ let registered_path = dest_db_path + .canonicalize() + .unwrap_or_else(|_| dest_db_path.to_path_buf()); + if open_path_registry() + .lock() + .unwrap_or_else(|p| p.into_inner()) + .contains(®istered_path) + { + return Err(WalletStorageError::AlreadyOpen { + path: registered_path, + }); + } + if !skip_backup && dest_db_path.exists() {🤖 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-storage/src/sqlite/persister.rs` around lines 299 - 326, restore_from_inner currently restores the database without checking whether the destination path is already owned by a live SqlitePersister, which can leave an in-memory handle out of sync with the replaced file. Add an upfront registry lookup in restore_from_inner for dest_db_path and return WalletStorageError::AlreadyOpen when the path is already registered, before any backup or restore work begins. Keep the change localized around restore_from_inner and the open-path registry used by SqlitePersister so existing live handles are protected from restore-time replacement.
🧹 Nitpick comments (4)
packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs (1)
91-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
synced_heightas well aslast_processed_height.This test writes both fields, but only validates one of them. If
load()stops wiringsynced_height, the round-trip still passes.Suggested assertion
assert_eq!(slice.core_state.new_utxos.len(), 1); assert_eq!(slice.core_state.new_utxos[0].value(), 777_000); + assert_eq!(slice.core_state.synced_height, Some(50)); assert_eq!(slice.core_state.last_processed_height, Some(50));🤖 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-storage/tests/sqlite_load_wiring.rs` around lines 91 - 127, The round-trip test in `sqlite_load_wiring.rs` only verifies `last_processed_height` from `state.wallets.get(&w).core_state` even though `synced_height` is also written into `CoreChangeSet`; update the existing load assertions to check both fields after `p2.load()` so `load()` wiring regressions for `synced_height` are caught alongside `last_processed_height`.packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs (1)
93-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the overlay stays out of the rehydrated identity.
This currently proves only that
load()still returns the wallet's core state. If a regression starts mergingdashpay_profilesinto the loaded identity payload, this test still passes. Please also assert that the seeded identity is present afterload()and that its DashPay profile remains absent for the overlay-only write case.🤖 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-storage/tests/sqlite_dashpay_overlay_contract.rs` around lines 93 - 108, The current test around persister.load() only verifies wallet.core_state, so it can miss regressions where dashpay_profiles gets merged into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to also inspect the loaded identity payload for the seeded wallet after load() and assert that the identity is still present while its DashPay profile remains absent in this overlay-only write scenario. Use the existing persister.load(), wallets.get(&w), and any identity fields already available in the loaded state to make the check explicit.packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs (1)
67-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert that the failed pre-flush left nothing durable.
Restoring the buffer is only half of the contract here. If
apply_changeset_to_txever leaks thewalletsinsert before thecore_sync_statefailure, this test still passes and leaves duplicate-on-retry state behind.Suggested assertion block
assert!( persister.buffer_has_changeset_for_test(&w), "buffered changeset must be restored after a real pre-flush apply failure" ); + + let conn = persister.lock_conn_for_test(); + let wallets_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM wallets WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + let core_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(wallets_rows, 0, "failed pre-flush must not durably create the wallet row"); + assert_eq!(core_rows, 0, "failed pre-flush must not durably create child rows");🤖 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-storage/tests/sqlite_delete_real_apply_failure.rs` around lines 67 - 72, The test currently only verifies the buffered changeset is restored, but it should also verify that a failed pre-flush did not persist any durable state. In sqlite_delete_real_apply_failure.rs, extend the existing scenario around the failed delete so it checks the database/transaction state after the apply failure and confirms no `wallets` insert or other durable side effects remain from `apply_changeset_to_tx`. Keep the existing `persister.buffer_has_changeset_for_test(&w)` assertion, and add a second assertion in the same test that validates the storage is clean after the failure so retry does not see duplicate-on-retry state.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
813-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the query-budget documentation.
load()currently performs multiple reader calls inside thefor wallet_id in wallet_idsloop, so the query count grows with wallet count. Reword this to avoid promising constant query budget.🤖 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-storage/src/sqlite/persister.rs` around lines 813 - 814, Update the query-budget comment in the load path so it no longer claims constant cost with wallet count; the current load() flow iterates over wallet_ids and performs multiple reader calls per wallet, so reword the documentation to describe that it has per-wallet read/query work rather than a fixed query budget. Keep the note near the wallet_ids loop/load() implementation and make sure the wording matches the actual behavior of the reader calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-ffi/src/persistence.rs`:
- Around line 3389-3390: The temporary restore stub in the persistence restore
flow should not panic via todo!(); replace it with a recoverable typed error so
callers receive a PersistenceError instead of crashing. Update the restore-path
branch that currently ignores identity_manager and unused_asset_locks to return
an appropriate PersistenceError variant (or equivalent error conversion) from
the same function/method, keeping the signature consistent and preserving the
existing error handling path.
In `@packages/rs-platform-wallet-storage/README.md`:
- Around line 165-168: The README wording around the manager-side rehydration
flow is too strong for this PR because the manager/FFI load path is still
stubbed. Update the description near the watch-only rebuild note to clearly mark
the manager-side `load_from_persistor`/`Wallet::new_watch_only` application as
pending or follow-up work, and keep the current text scoped to the storage-side
behavior only.
In `@packages/rs-platform-wallet-storage/src/kv.rs`:
- Around line 62-65: The key-length validation in validate_key currently assumes
Rust chars().count() matches SQLite length() for all strings, but embedded NULs
break that equivalence. Update the key precheck to explicitly reject keys
containing \0 before comparing length, or adjust the validation/comment so it no
longer claims the same key set; keep the logic aligned with the SQL CHECK
constraint in kv.rs.
In `@packages/rs-platform-wallet-storage/src/secrets/error.rs`:
- Around line 3-5: The file-level non-leakage docs in error.rs are too broad for
the current Io behavior: they claim variants never carry a stringified source,
but Io::fmt/rendering still exposes the underlying source text. Update the docs
to carve out the Io exception, or change Io’s display implementation/tests so it
no longer includes the source string, keeping the wording aligned with the
actual Error and Io rendering behavior.
- Around line 88-91: The UnsupportedEnvelopeVersion error currently truncates
the envelope version to u8, so update the error variant in error.rs to store the
full u32 version value instead. Then adjust the envelope parsing call site that
constructs UnsupportedEnvelopeVersion to pass the original Envelope.version
without narrowing, keeping the reported version accurate in the error message.
In `@packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- Around line 21-22: The docs for the nested BTreeMap format currently imply
duplicate (wallet_id, label) pairs are prevented entirely, but the read path
still accepts duplicate JSON keys and serde collapses them. Update the
documentation near the format description to state that uniqueness is only
guaranteed by serialization, or change the deserialization logic in the file
format/parser code to explicitly reject duplicate keys, and make the behavior
match the tests and the intended API.
In `@packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- Around line 628-654: The post-persist Unix handling in the vault write path is
swallowing parent-directory fsync failures and returning success, which makes
`put`/`delete`/`rekey` report a durable commit when only the rename succeeded.
Update the flow around the `persist()`/`sync_all()` block to surface a distinct
“committed but not durable” result or otherwise keep the in-memory commit behind
the durability boundary, and make sure the caller can tell when
`fs::File::open(parent)` or `sync_all()` fails instead of only logging via
`tracing::warn!`.
In `@packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- Around line 255-266: The reprotect method in SecretStore currently does a
non-atomic read-then-write using get_secret followed by set_secret, which can
overwrite concurrent updates with stale plaintext. Update reprotect to use an
atomic backend-specific reprotect/CAS path, or add a version check so the write
only succeeds if the entry has not changed since get_secret; reference
SecretStore::reprotect, get_secret, and set_secret when wiring the fix.
In `@packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs`:
- Around line 136-141: The scheme-0 plaintext path in the envelope handling
still leaves temporary Vec<u8> buffers unwiped, including the
Unprotected(plaintext.to_vec()) branch and the ExpectedProtectedButUnsealed arm.
Update the envelope logic in the encode/decode flow around the Envelope and
Payload handling to use zeroizing storage for these plaintext temporaries or
explicitly wipe them before drop, while keeping SecretBytes::new only for the
final encoded blob.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 179-199: `persist`/`open` currently treats `has_schema_history()`
as the only brand-new-vs-existing check, so a pre-existing non-wallet SQLite
file with no `refinery_schema_history` can still be migrated. Add an explicit
guard in the `had_schema_history` decision path to reject existing SQLite files
that lack wallet schema history, using the same `conn`/`has_schema_history` flow
and returning a typed wallet storage error before any backup, integrity check,
or `migrations::run()` work begins.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 143-154: The sync-state write path in core_state should treat
last_applied_chain_lock monotonically, not as a blind overwrite. Update the
CoreChangeSet-to-DB flow around upsert_sync_state so the stored chain-lock is
max-merged with the existing row (using the same chain-lock height comparison
logic as the height watermarks) before persisting. Apply this behavior wherever
last_applied_chain_lock is written in the affected core_state update functions
so the persisted chain-lock cannot regress.
- Around line 40-41: The `decode_from_slice` handling in
`last_applied_chain_lock` is too permissive because it accepts a valid prefix
and ignores any appended data. Update this decoding path in `core_state.rs` to
mirror the other blob decoders: after calling `bincode::decode_from_slice` for
`ChainLock`, verify the returned consumed length matches `bytes.len()` and treat
any mismatch as corruption by returning `None` instead of loading the state.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 151-169: Mirror the writer-side validation in load_state by
checking that each decoded public_key_blob matches the row’s typed columns
before inserting into cs.upserts. After decode_entry(&payload), verify the
entry’s identity_id, key_id, wallet_id, and public_key_hash against the values
from the identity_keys query, and return a WalletStorageError if any mismatch is
found. Keep the checks local to load_state and use the existing decode_entry,
Identifier::from, and KeyID::try_from flow so inconsistent rows are rejected
instead of loaded silently.
In `@packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs`:
- Around line 46-82: The sqlite_accounts_reader test is too weak because both
AccountRegistrationEntry fixtures use the same xpub and the assertions only
check set membership, so row reordering or xpub/row mixups can still pass.
Update the test to use distinct xpub fixtures for each entry and assert the
loaded manifest in the expected order, using the accounts::load_state result and
the existing AccountType variants to verify each row maps to the correct xpub.
In `@packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs`:
- Line 33: The doc comment on the wallet start state field still references the
old wallet_metadata table. Update the comment in client_wallet_start_state.rs to
point to the renamed wallets table instead, keeping the wording aligned with the
field’s source of truth and using the existing comment near the network field to
locate it.
In `@packages/rs-platform-wallet/src/manager/load.rs`:
- Around line 8-14: The public rehydration entry point
PlatformWalletManager::load_from_persistor currently panics via todo!, which
turns a caller error into a runtime abort. Replace the todo! with a recoverable
Result path by returning an explicit PlatformWalletError for the unsupported
stub state, or otherwise gate/remove this API until keyless rehydration in
PlatformWalletManager is implemented. Ensure callers receive an error instead of
a panic.
---
Outside diff comments:
In `@packages/rs-platform-wallet-storage/SCHEMA.md`:
- Around line 507-513: The soft-cascade description in SCHEMA.md overstates what
a wallet delete cleans up for identity-scoped metadata. Update the note near the
wallet/identity trigger flow to say that `wallets` deletion only reaches
`meta_identity` and `meta_token` through existing `identities` rows and that
orphan metadata written before an `identities` row exists is not covered; align
the wording with the existing orphan-metadata section and reference the
`wallets` trigger and the `identities` FK cascade path.
In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- Around line 243-263: The restore flow in `backup.rs` removes `-wal`/`-shm`
siblings before `tmp.persist(dest_db_path)`, which can leave the original DB
intact but its WAL-mode state lost if persist fails. Change the `restore` logic
to use a rollback-safe replacement strategy: do not unlink siblings until the
destination swap is guaranteed, or replace the whole SQLite set atomically via a
SQLite-native restore path. Keep the fix localized around the sibling cleanup
and `tmp.persist` sequence so the operation remains all-or-nothing.
- Around line 361-374: In backup pruning logic in the `retain_backups` flow,
`keep_last_n` is still being treated like a ceiling because the deletion
condition requires `pass_count` even when `max_age` is also set. Update the
condition around `within_floor`, `pass_count`, and `pass_age` so that the newest
N backups are always kept as a floor and any backup within the age limit is also
retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in
this block.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 299-326: restore_from_inner currently restores the database
without checking whether the destination path is already owned by a live
SqlitePersister, which can leave an in-memory handle out of sync with the
replaced file. Add an upfront registry lookup in restore_from_inner for
dest_db_path and return WalletStorageError::AlreadyOpen when the path is already
registered, before any backup or restore work begins. Keep the change localized
around restore_from_inner and the open-path registry used by SqlitePersister so
existing live handles are protected from restore-time replacement.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- Around line 27-36: `decode_platform_payment_row` currently decodes the blob
and returns the typed `account_index` without checking that the
`AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update
this helper to validate the decoded `AccountRegistrationEntry` matches the
expected `PlatformPayment` variant and index, and return
`AccountRegistrationEntryMismatch` if it does not. Keep the existing
`safe_cast::i64_to_u32` conversion, but make
`all_platform_payment_registrations()` fail closed by rejecting any corrupted or
mismatched row instead of rehydrating it.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 143-150: The load path in load_state is trusting the blob too much
and currently ignores the selected identity_id, so mismatched typed columns can
be silently rehydrated under the blob value. Update the row handling in
load_state to validate that the typed identity_id matches entry_blob.id before
decoding into IdentityEntry, and also verify the blob wallet_id matches the
wallet_id scope passed into managed_identity_from_entry. If either check fails,
reject the row instead of continuing.
In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs`:
- Around line 165-180: The smoke test still treats identity_keys as
identity-scoped, but the schema now scopes it by wallet_id. Update the test
logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query
path instead of the via_identity branch, while keeping the other tables that
still depend on identities routed through identity_id. Use the existing
via_identity handling in the loop over cases to locate and adjust the count_sql
selection.
---
Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 813-814: Update the query-budget comment in the load path so it no
longer claims constant cost with wallet count; the current load() flow iterates
over wallet_ids and performs multiple reader calls per wallet, so reword the
documentation to describe that it has per-wallet read/query work rather than a
fixed query budget. Keep the note near the wallet_ids loop/load() implementation
and make sure the wording matches the actual behavior of the reader calls.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs`:
- Around line 93-108: The current test around persister.load() only verifies
wallet.core_state, so it can miss regressions where dashpay_profiles gets merged
into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to
also inspect the loaded identity payload for the seeded wallet after load() and
assert that the identity is still present while its DashPay profile remains
absent in this overlay-only write scenario. Use the existing persister.load(),
wallets.get(&w), and any identity fields already available in the loaded state
to make the check explicit.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs`:
- Around line 67-72: The test currently only verifies the buffered changeset is
restored, but it should also verify that a failed pre-flush did not persist any
durable state. In sqlite_delete_real_apply_failure.rs, extend the existing
scenario around the failed delete so it checks the database/transaction state
after the apply failure and confirms no `wallets` insert or other durable side
effects remain from `apply_changeset_to_tx`. Keep the existing
`persister.buffer_has_changeset_for_test(&w)` assertion, and add a second
assertion in the same test that validates the storage is clean after the failure
so retry does not see duplicate-on-retry state.
In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs`:
- Around line 91-127: The round-trip test in `sqlite_load_wiring.rs` only
verifies `last_processed_height` from `state.wallets.get(&w).core_state` even
though `synced_height` is also written into `CoreChangeSet`; update the existing
load assertions to check both fields after `p2.load()` so `load()` wiring
regressions for `synced_height` are caught alongside `last_processed_height`.
🪄 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: edc85543-e83f-4a54-88ef-17800859c720
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-storage/.cargo/audit.tomlpackages/rs-platform-wallet-storage/Cargo.tomlpackages/rs-platform-wallet-storage/README.mdpackages/rs-platform-wallet-storage/SCHEMA.mdpackages/rs-platform-wallet-storage/SECRETS.mdpackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rspackages/rs-platform-wallet-storage/src/kv.rspackages/rs-platform-wallet-storage/src/lib.rspackages/rs-platform-wallet-storage/src/secrets/error.rspackages/rs-platform-wallet-storage/src/secrets/file/crypto.rspackages/rs-platform-wallet-storage/src/secrets/file/format.rspackages/rs-platform-wallet-storage/src/secrets/file/mod.rspackages/rs-platform-wallet-storage/src/secrets/keyring.rspackages/rs-platform-wallet-storage/src/secrets/mod.rspackages/rs-platform-wallet-storage/src/secrets/secret.rspackages/rs-platform-wallet-storage/src/secrets/store.rspackages/rs-platform-wallet-storage/src/secrets/wire/aad.rspackages/rs-platform-wallet-storage/src/secrets/wire/config.rspackages/rs-platform-wallet-storage/src/secrets/wire/envelope.rspackages/rs-platform-wallet-storage/src/secrets/wire/kdf.rspackages/rs-platform-wallet-storage/src/secrets/wire/mod.rspackages/rs-platform-wallet-storage/src/sqlite/backup.rspackages/rs-platform-wallet-storage/src/sqlite/config.rspackages/rs-platform-wallet-storage/src/sqlite/conn.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/kv.rspackages/rs-platform-wallet-storage/src/sqlite/migrations.rspackages/rs-platform-wallet-storage/src/sqlite/persister.rspackages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/src/sqlite/schema/blob.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rspackages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rspackages/rs-platform-wallet-storage/src/sqlite/schema/mod.rspackages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rspackages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rspackages/rs-platform-wallet-storage/src/sqlite/schema/wallets.rspackages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rspackages/rs-platform-wallet-storage/tests/common/mod.rspackages/rs-platform-wallet-storage/tests/secrets_api.rspackages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rspackages/rs-platform-wallet-storage/tests/secrets_scan.rspackages/rs-platform-wallet-storage/tests/sqlite_account_zero_attribution.rspackages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_asset_locks_filter.rspackages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rspackages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rspackages/rs-platform-wallet-storage/tests/sqlite_commit_writes_lock_poison_shortcircuit.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rspackages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_partial_commit_window.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rspackages/rs-platform-wallet-storage/tests/sqlite_error_classification.rspackages/rs-platform-wallet-storage/tests/sqlite_fk_changeset_ordering.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rspackages/rs-platform-wallet-storage/tests/sqlite_identity_keys_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_money_column_overflow_on_read.rspackages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rspackages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rspackages/rs-platform-wallet-storage/tests/sqlite_second_open_guard.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet-storage/tests/sqlite_wallet_db_identity.rspackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/manager/load.rs
…riminators to stop distinct-variant collapse (#3968 review) account_registrations keyed on (wallet_id, account_type, account_index) only. PlatformPayment key classes and DashPay (user, friend) identity pairs share that key across genuinely distinct accounts, so the ON CONFLICT DO UPDATE silently overwrote one with another — a restored wallet lost accounts (data loss). Chose option (a) widen-PK over fail-loud: a wallet legitimately holds multiple DashpayReceivingFunds accounts (one per contact) at the same index, so failing the collision would reject valid multi-contact wallets. Add key_class, user_identity_id, friend_identity_id as NOT NULL columns with sentinel defaults (0 / zeroblob) so non-discriminated variants still dedup on re-persist, and widen the PK to include them. The reader cross-checks every typed PK column against the decoded blob and orders deterministically. V001 edited in place (on-disk format unshipped). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
thepastaclaw
suggested changes
Jul 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers=general:claude/opus(failed), general:codex/gpt-5.5(ok), ffi-engineer:claude/opus(failed), ffi-engineer:codex/gpt-5.5(ok); verifier=codex/gpt-5.5.
Reviewed commit: 0503f80d. Prior reviewed commit: eacc851d. Incremental + cumulative review reconciled the prior findings and the latest delta separately.
Verified HEAD 0503f80 against the prior review and current source. The previous storage compile blocker is fixed and cargo check -p platform-wallet-storage --lib passes, but the latest Rust FFI surface shrink left Swift call sites and enum mirrors stale, which will break the Swift SDK against regenerated bindings. Nine prior findings remain valid and are carried forward; prior-7 is outdated because the manifest/snapshot comparison helper was removed.
🔴 3 blocking | 🟡 9 suggestion(s)
12 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:475-477: Swift still calls the removed load-outcome FFI ABI
The latest Rust FFI delta changed platform_wallet_manager_load_from_persistor to take only manager_handle and removed LoadOutcomeFFI plus platform_wallet_load_outcome_free. Swift still constructs LoadOutcomeFFI, passes &outcome as a second argument, and defers the removed free function. With regenerated bindings these identifiers and the old function signature disappear, so the Swift SDK will not compile; with stale bindings the caller no longer matches the exported Rust ABI.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:122-127: Swift still mirrors FFI result codes removed from Rust
Rust PlatformWalletFFIResultCode now stops at ErrorTransactionBroadcastUnconfirmed = 20 before NotFound and ErrorUnknown; ErrorAddressNonceMismatch, ErrorPersisterLoadTransient, and ErrorPersisterLoadFatal were removed from the enum and from the PlatformWalletError mapping. Swift still switches on the generated constants for those removed variants, so a regenerated DashSDKFFI module will not provide these names and this file will fail to compile. The change also drops the typed retry/do-not-retry semantics Swift previously consumed for nonce races and persister load failures.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:293-338: contacts::load_state materializes contact blobs before any length gate
The reader selects outgoing_request, incoming_request, and accepted_accounts directly into Option<Vec<u8>> before checking their length. blob::decode applies the bincode cap only after SQLite has already allocated each Vec, so a corrupted wallet database can force oversized allocations during rehydration paths that load contact state.
In `packages/rs-platform-wallet/src/wallet/platform_wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet.rs:1192-1200: load_and_apply_persisted discards wallet state it promises to reload
The documentation says a second hydration after late account bootstrap picks up the rest, but the implementation destructures ClientStartState with wallets: _ and applies only platform_addresses. That drops the reloaded per-wallet slice for this wallet, including managed wallet state, identity manager state, and unused asset locks, so the recommended late-hydration entry point does not replay the state it promises.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1612-4121: FFI restore arrays use host counts before typed slice bounds
The restore path passes Swift/C-supplied typed counts directly into slice::from_raw_parts for wallet entries, account specs, identity entries, payments, ignored senders, contact profiles, contacts, accepted-account arrays, identity keys, and nested byte/u32 buffers. Rust requires the full constructed slice to fit within isize::MAX bytes before from_raw_parts is called; the existing byte-buffer validation does not protect these typed arrays, so malformed C callers can violate Rust slice preconditions before validation returns an FFI error.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:375-385: core_instant_locks materializes txid before fixed-width validation
The instant-lock reader reads txid into Vec<u8> before verifying that it is exactly 32 bytes. Only islock_blob is gated with length() before materialization, so a corrupted database can allocate an oversized txid blob before Txid::from_slice rejects it.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:188-227: asset-lock readers materialize outpoint before fixed-width validation
load_unconsumed and list_active read outpoint into Vec<u8> before enforcing the fixed 36-byte outpoint encoding. The lifecycle blob is length-gated, but the outpoint column is not, leaving the same oversized-allocation issue on corrupted wallet databases.
In `packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/identity/state/manager/apply.rs:47-78: identity updates do not migrate bucket placement
When apply_identity_entry finds an existing identity, it updates scalar/profile/contact fields and returns without checking whether entry.wallet_id or entry.identity_index changed. An identity first observed out of wallet and later replayed with a wallet association can remain in the wrong bucket, while fresh inserts do place the identity from entry.wallet_id.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:72-77: manager-level unused asset-lock restoration lacks round-trip coverage
The manager load path flattens unused_asset_locks into PlatformWalletInfo.tracked_asset_locks, but the prior rehydration integration test file was removed and the remaining storage tests only prove the persister can read asset-lock buckets. A regression that drops or mis-buckets locks at the manager load boundary would still be missed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Core/Wallet/WalletStorage.swift:39-41: production keychain service is environment-overridable
WalletStorage.keychainService reads DASH_KEYCHAIN_SERVICE from the process environment in the shipping SDK path. macOS or CLI-embedded contexts can inherit an unexpected environment and silently redirect wallet keychain reads and writes to a different service namespace, making existing wallets appear missing under a changed environment.
In `packages/rs-platform-wallet/src/wallet/core/wallet.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/core/wallet.rs:115-117: widened gap-limit addresses are generated but never persisted
set_gap_limit mutates the managed account and may generate addresses required by the wider gap limit, but it discards the returned address list and emits no durable address-pool changeset. A caller can widen the gap, hand out a newly generated address, then reload from persisted rows that do not include that pool expansion.
In `packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/transaction_builder.rs:308-452: transaction-builder FFI paths create Rust slices before size guards
core_wallet_tx_builder_set_special_payload calls std::slice::from_raw_parts(payload_bytes, payload_len) directly on a public C ABI length, and core_wallet_tx_builder_add_inputs_from_outpoints does the same for typed OutPointFFI entries. A hostile or corrupted Swift/C caller can pass lengths that violate Rust's from_raw_parts safety contract before bincode or account-ownership validation returns a PlatformWalletFFIResult.
GitHub refused normal inline diff review for this oversized PR; posted exact-SHA body-only fallback. review_poster error: Traceback (most recent call last):
Reviewed commit: 0503f80
lklimek
changed the base branch from
feat/platform-wallet-rehydration
to
v4.1-dev
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.cargo/audit.toml:
- Around line 2-5: Keep the workspace-root RustSec ignore in .cargo/audit.toml
until the audit workflow is changed to target the package config. The current
rustsec/audit-check@v1 job reads the root audit config, so removing the ignore
from the root will cause RUSTSEC-2025-0141 to fail CI again; preserve the
existing ignore entry there and only move it after the workflow update is in
place.
🪄 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: 6991c1f7-1b73-44d2-b5ad-26517ca9b71d
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpackages/rs-platform-wallet-storage/tests/fixtures/populated_v001.dbis excluded by!**/*.db
📒 Files selected for processing (62)
.cargo/audit.toml.gitignoreCargo.tomlpackages/rs-platform-wallet-storage/Cargo.tomlpackages/rs-platform-wallet-storage/README.mdpackages/rs-platform-wallet-storage/SCHEMA.mdpackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/migrations/V003__unified.rspackages/rs-platform-wallet-storage/src/kv.rspackages/rs-platform-wallet-storage/src/secrets/error.rspackages/rs-platform-wallet-storage/src/secrets/file/format.rspackages/rs-platform-wallet-storage/src/secrets/file/mod.rspackages/rs-platform-wallet-storage/src/secrets/store.rspackages/rs-platform-wallet-storage/src/secrets/wire/envelope.rspackages/rs-platform-wallet-storage/src/sqlite/backup.rspackages/rs-platform-wallet-storage/src/sqlite/conn.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/migrations.rspackages/rs-platform-wallet-storage/src/sqlite/persister.rspackages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/src/sqlite/schema/blob.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rspackages/rs-platform-wallet-storage/src/sqlite/schema/mod.rspackages/rs-platform-wallet-storage/src/sqlite/schema/pending_contact_crypto.rspackages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rspackages/rs-platform-wallet-storage/src/sqlite/schema/versions.rspackages/rs-platform-wallet-storage/src/sqlite/util/mod.rspackages/rs-platform-wallet-storage/src/sqlite/util/wallet.rspackages/rs-platform-wallet-storage/tests/fixture_gen.rspackages/rs-platform-wallet-storage/tests/fixtures/.gitignorepackages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rspackages/rs-platform-wallet-storage/tests/sqlite_blob_size_gate_on_load.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rspackages/rs-platform-wallet-storage/tests/sqlite_core_pool_writer.rspackages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rspackages/rs-platform-wallet-storage/tests/sqlite_error_classification.rspackages/rs-platform-wallet-storage/tests/sqlite_fk_changeset_ordering.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_db_rejection.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rspackages/rs-platform-wallet-storage/tests/sqlite_migration_execution.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet-storage/tests/sqlite_pool_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rspackages/rs-platform-wallet-storage/tests/sqlite_restore_open_path_guard.rspackages/rs-platform-wallet-storage/tests/sqlite_schema_pinning.rspackages/rs-platform-wallet-storage/tests/sqlite_store_generation.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet-storage/tests/sqlite_used_core_addresses.rspackages/rs-platform-wallet-storage/tests/sqlite_v003_isolation.rspackages/rs-platform-wallet-storage/tests/sqlite_v003_migration.rspackages/rs-platform-wallet-storage/tests/sqlite_version_bump.rspackages/rs-platform-wallet/src/manager/load_outcome.rs
💤 Files with no reviewable changes (20)
- packages/rs-platform-wallet-storage/tests/fixtures/.gitignore
- packages/rs-platform-wallet-storage/src/sqlite/schema/pending_contact_crypto.rs
- packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs
- packages/rs-platform-wallet-storage/tests/sqlite_blob_size_gate_on_load.rs
- packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
- packages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
- packages/rs-platform-wallet-storage/tests/fixture_gen.rs
- packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
- packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/versions.rs
- packages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rs
- packages/rs-platform-wallet-storage/tests/sqlite_core_pool_writer.rs
- packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
- packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
✅ Files skipped from review due to trivial changes (2)
- packages/rs-platform-wallet-storage/src/sqlite/conn.rs
- packages/rs-platform-wallet-storage/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
- packages/rs-platform-wallet-storage/Cargo.toml
- packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
- packages/rs-platform-wallet-storage/src/sqlite/backup.rs
- packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs
- packages/rs-platform-wallet-storage/src/sqlite/error.rs
- packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
- packages/rs-platform-wallet-storage/src/secrets/store.rs
- packages/rs-platform-wallet-storage/SCHEMA.md
- packages/rs-platform-wallet-storage/src/secrets/error.rs
- packages/rs-platform-wallet-storage/src/secrets/file/mod.rs
thepastaclaw
suggested changes
Jul 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers=general:claude/opus(failed), general:codex/gpt-5.5(ok), ffi-engineer:claude/opus(failed), ffi-engineer:codex/gpt-5.5(ok); verifier=codex/gpt-5.5.
Reviewed commit: ee8390b0. Prior reviewed commit: 0503f80d.
Verified HEAD ee8390b and the 0503f80..ee8390b delta. I found no new in-scope issues in the latest token mint/direct-purchase delta; the in-scope carried-forward problems are the two Swift SDK compile blockers where PR code still references Rust FFI symbols and enum variants that are absent at current head. Several other prior items are either outside the active PR diff against the current v4.1-dev base or are not valid after accounting for the SQLite connection-wide length limit.
Blocking: 2 | Suggestions: 0 | Nitpicks: 0
Prior reconciliation:
- Carried forward: the two Swift FFI blockers below remain valid at the current head.
- New findings in latest delta: none verified in
0503f80d..ee8390b0. - Resolved prior findings: prior-0.
- Dropped/outdated/deferred prior findings: contacts::load_state materializes contact blobs before any length gate; core_instant_locks materializes txid before fixed-width validation; asset-lock readers materialize outpoint before fixed-width validation; load_and_apply_persisted discards wallet state it promises to reload; FFI restore arrays use host counts before typed slice bounds; identity updates do not migrate bucket placement; manager-level unused asset-lock restoration lacks round-trip coverage; production keychain service is environment-overridable; widened gap-limit addresses are generated but never persisted; transaction-builder FFI paths create Rust slices before size guards; manifest check compares account types but not xpubs.
Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:475-477: Swift calls a removed load-outcome FFI ABI
This PR adds Swift code that constructs `LoadOutcomeFFI`, calls `platform_wallet_manager_load_from_persistor(handle, &outcome)`, and frees it with `platform_wallet_load_outcome_free`. The Rust export at current head takes only `manager_handle`, and `rs-platform-wallet-ffi` no longer defines `LoadOutcomeFFI` or the free function. Regenerated bindings will not contain these identifiers, so the Swift SDK will fail to compile; stale bindings would also call the wrong ABI.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:122-127: Swift mirrors FFI result-code variants that Rust no longer exports
This file switches on generated constants for `ERROR_ADDRESS_NONCE_MISMATCH`, `ERROR_PERSISTER_LOAD_TRANSIENT`, and `ERROR_PERSISTER_LOAD_FATAL`, but the Rust `PlatformWalletFFIResultCode` enum now ends at `ErrorTransactionBroadcastUnconfirmed = 20` before `NotFound` and `ErrorUnknown`. Regenerated `DashSDKFFI` bindings will not provide those constants, so this file will fail to compile.
Reviewed commit: ee8390b07a9296ae3b28509e6eb0b82c2ca01291. Prior reviewed commit: 0503f80df5529b1d4f91e58c49abe08cde7b283f.
GitHub refused normal diff-based posting for this oversized PR (PullRequest.diff too_large), so this exact-SHA review is posted as a body-only review.
…nt + txid rehydration reads (#3968 review) Extend the file's read-before-cap BLOB discipline to two fixed-reference columns that were materialized before any length check: - asset_locks `outpoint` (load_state / load_unconsumed / list_active): the bincode-varint outpoint blob is now gated with `blob::check_size` before `row.get`, matching the sibling `lifecycle_blob` gate and core_state's unspent-UTXO reader. A tampered oversize value is rejected before the Vec is allocated instead of after. - core_instant_locks `txid` (load_state): a raw 32-byte hash, now gated with `blob::check_fixed_width(.., 32, ..)` — an oversize column raises BlobTooLarge ahead of the alloc rather than materializing then failing in Txid::from_slice. Mirrors the platform_addresses fixed-width gate. Adds a RED-first repro (`load_state_rejects_oversize_instant_lock_txid`): without the gate an oversize txid surfaces as BlobDecode after a multi-MiB allocation; with it, BlobTooLarge before the alloc. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ion rustdoc A leading `+` in a doc-list continuation line was parsed as a nested markdown list marker, tripping `clippy::doc_lazy_continuation` under the pinned 1.92 toolchain. Reword `+ wallet totals` to `and wallet totals` (semantically identical) to clear the lint at its root. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t.toml (#3968 review) rustsec/audit-check runs from the repo root and only reads the root .cargo/audit.toml — a crate-local audit.toml is never consulted by CI, so the existing ignore in packages/rs-platform-wallet-storage/.cargo/audit.toml did nothing to suppress the nightly security-audit-rust.yml workflow. Move the RUSTSEC-2025-0141 (bincode unmaintained) acknowledgement, with its full rationale, to the root config where CI actually reads it. Delete the now-redundant crate-local file rather than leave a dead config that looks authoritative but isn't. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…let-storage-rehydration # Conflicts: # packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift
thepastaclaw
suggested changes
Jul 9, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers=general:claude/opus(failed), general:codex/gpt-5.5(ok), ffi-engineer:claude/opus(failed), ffi-engineer:codex/gpt-5.5(ok); verifier=codex/gpt-5.5.
Reviewed commit: fa9f3378. Prior reviewed commit: a9c500f8.
Carried-forward prior findings remain valid for the Swift/Rust load ABI mismatch, the Swift persister-load result-code mirror, and both stale SQLite prepare allow-list entries. The latest delta fixed the AddressNonceMismatch portion of the result-code mismatch, but the persister-load constants remain Swift-only. One additional in-scope cumulative issue is valid: empty-manifest wallet rows are still registered by the manager even though the persister comments and load outcome model say they should be skipped.
🔴 5 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:475-477: Swift calls a removed load-outcome FFI ABI
Swift still constructs `LoadOutcomeFFI`, calls `platform_wallet_manager_load_from_persistor(handle, &outcome)`, and frees the outcome with `platform_wallet_load_outcome_free`. The current Rust export takes only `manager_handle`, and the Rust FFI crate does not define `LoadOutcomeFFI` or `platform_wallet_load_outcome_free`. Regenerated bindings will not contain these identifiers, so the Swift SDK will fail to compile; stale bindings would call a two-argument ABI against a one-argument Rust function.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:124-127: Swift mirrors persister-load result codes Rust does not export
Rust now exports `ErrorAddressNonceMismatch = 21`, so that part of the prior result-code finding is fixed. Swift still switches on `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT` and `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL`, but Rust's `PlatformWalletFFIResultCode` jumps from `ErrorAddressNonceMismatch = 21` to `NotFound = 98` and `ErrorUnknown = 99`. Regenerated `DashSDKFFI` bindings will not provide the two persister-load constants, so this file will still fail to compile.
In `packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs:40: Update the asset-lock prepare allow-list for the outpoint length gate
The asset-lock readers now prepare SQL beginning with `SELECT length(outpoint), outpoint, account_index`, but this allow-list entry still matches only the old `SELECT outpoint, account_index` substring. `tc_p1_003_prepare_cached_in_writers` exempts read-only `.prepare(` call sites only when the local three-line probe contains an allow-list substring, so the three asset-lock readers are now reported as offenders.
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs:84-87: Update the instant-lock prepare allow-list for the txid length gate
The `core_instant_locks` reader now uses `SELECT length(txid), txid, length(islock_blob), islock_blob`, but this allow-list entry still matches the old `SELECT txid, length(islock_blob), islock_blob` text. Because the guard test matches by substring, the changed `core_state.rs` query is no longer exempt and will be reported as a `.prepare(` offender.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:64-70: Empty-manifest wallet rows are registered instead of skipped
`SqlitePersister::load` creates a placeholder external-signable wallet when `account_manifest.is_empty()` and documents that the manager will re-check the empty manifest and skip the wallet as `MissingManifest`. The manager does not perform that check: it destructures every `ClientWalletStartState`, builds `PlatformWalletInfo`, inserts the wallet into `WalletManager`, initializes platform addresses, and publishes a `PlatformWallet` handle. A crash-created wallet row with no account registration therefore becomes a live empty wallet on restart instead of being skipped, contradicting the load outcome contract and exposing account-less wallets to callers that expect only restorable wallets.
_GitHub refused normal diff-based posting for this oversized PR; posted exact-SHA body-only review. Recovery reason: review_poster dry-run failed: 09, in post_review
diff_text = _gh("pr", "diff", str(pr_number), "--repo", repo)
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 94, in _gh
raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}")
RuntimeError: gh pr diff 3968 --repo dashpay/platform failed: could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of lines (20000) (https://api.github.com/repos/dashpay/platform/pulls/3968)
PullRequest.diff too_large
_
…rs tests Collapse the RehydrationPoolMismatch #[error(...)] attribute onto one line, alphabetize the platform_wallet import in rehydration_used_state_survives_spent_utxo, and wrap the over-width assert_eq! in the missing-account test — the three diffs cargo fmt --check flagged on the macOS CI job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thepastaclaw
suggested changes
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Incremental + cumulative review at 4ce099e3 after prior fa9f3378. The latest delta is formatting/import-only in storage test code, but the current head still needs the prior unresolved blockers carried forward.
Source: reviewers=general:claude/opus(failed), general:codex/gpt-5.5(ok; exit 5), ffi-engineer:claude/opus(failed), ffi-engineer:codex/gpt-5.5(ok); verifier=codex/gpt-5.5
Reviewed commit: 4ce099e3
Prior reviewed commit: fa9f3378
Prior Findings Reconciliation
All 5 prior findings from fa9f3378 are STILL VALID at 4ce099e3. No prior findings were fixed, outdated, or intentionally deferred in this delta.
Carried-Forward Prior Findings
- prior-1: blocking - Swift calls a removed load-outcome FFI ABI
- prior-2: blocking - Swift mirrors persister-load result codes Rust does not export
- prior-3: blocking - Update the asset-lock prepare allow-list for the outpoint length gate
- prior-4: blocking - Update the instant-lock prepare allow-list for the txid length gate
- prior-5: blocking - Empty-manifest wallet rows are registered instead of skipped
New Findings In Latest Delta
None.
🔴 5 blocking
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManager.swift:475-477: Swift calls a removed load-outcome FFI ABI
Swift constructs `LoadOutcomeFFI`, passes it as a second argument to `platform_wallet_manager_load_from_persistor`, and frees it with `platform_wallet_load_outcome_free`. The current Rust export takes only `manager_handle` and returns `PlatformWalletFFIResult`; the Rust FFI crate does not define `LoadOutcomeFFI` or `platform_wallet_load_outcome_free`. Regenerated `DashSDKFFI` bindings will not contain these identifiers, so the Swift SDK fails to compile; stale bindings would call a two-argument ABI against a one-argument Rust symbol.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift`:
- [BLOCKING] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift:124-127: Swift mirrors persister-load result codes Rust does not export
Rust now exports `ErrorAddressNonceMismatch = 21`, so that portion of the prior issue is fixed. Swift still switches on `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_TRANSIENT` and `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_FATAL`, but Rust's `PlatformWalletFFIResultCode` jumps from `ErrorAddressNonceMismatch = 21` directly to `NotFound = 98` and `ErrorUnknown = 99`. Regenerated bindings will not provide the two persister-load constants, so this file still fails to compile.
In `packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs:40: Update the asset-lock prepare allow-list for the outpoint length gate
The asset-lock readers now prepare SQL beginning with `SELECT length(outpoint), outpoint, account_index`, but this allow-list entry still matches only the old `SELECT outpoint, account_index` substring. `tc_p1_003_prepare_cached_in_writers` exempts read-only `.prepare(` call sites only when the local three-line probe contains an allow-list substring, so the three asset-lock readers are now reported as offenders.
- [BLOCKING] packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs:84-87: Update the instant-lock prepare allow-list for the txid length gate
The `core_instant_locks` reader now uses `SELECT length(txid), txid, length(islock_blob), islock_blob`, but this allow-list entry still matches the old `SELECT txid, length(islock_blob), islock_blob` text. Because the guard test matches by substring, the changed `core_state.rs` query is no longer exempt and will be reported as a `.prepare(` offender.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:64-70: Empty-manifest wallet rows are registered instead of skipped
`SqlitePersister::load` creates a placeholder external-signable wallet when `account_manifest.is_empty()` and documents that the manager will skip it as `MissingManifest`. The manager does not perform that check: it destructures every `ClientWalletStartState`, builds `PlatformWalletInfo`, inserts the wallet into `WalletManager`, initializes platform addresses, and publishes a `PlatformWallet` handle. A crash-created wallet row with no account registration therefore becomes a live empty wallet on restart instead of being skipped, contradicting the load outcome contract and exposing account-less wallets to callers that expect only restorable wallets.
_GitHub refused or skipped normal inline posting for this oversized PR diff; posted exact-SHA body-only review. Recovery reason: review_poster dry-run failed: 09, in post_review
diff_text = _gh("pr", "diff", str(pr_number), "--repo", repo)
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 94, in _gh
raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}")
RuntimeError: gh pr diff 3968 --repo dashpay/platform failed: could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of lines (20000) (https://api.github.com/repos/dashpay/platform/pulls/3968)
PullRequest.diff too_large
_
The persistence move relocated apply_persisted_core_state into this crate as a pub(super) helper, but the integration test still called the vanished platform_wallet::manager::rehydrate:: path — a compile error only under --all-features (the macOS coverage job), invisible to default-feature clippy. Re-export the helper publicly behind the rehydration-apply feature (kept crate-private in normal builds), point the two test call sites at the new platform_wallet_storage::sqlite::util path, and correct the now-inaccurate "manager-apply leg" comments — the function no longer lives in the manager. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
README claimed the manager-side rebuild was "not yet wired" and "lands in #3692" — false: load_from_persistor reconstructs and registers every persisted wallet today. Rewrite to present state. Update the three Wallet::new_watch_only references (README, wallet_lifecycle comment, Swift loadWalletList doc) to the renamed Wallet::new_external_signable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ntract The LoadOutcome feature was removed Rust-side; Swift was the straggler, calling FFI symbols that no longer exist and referencing result codes 22/23 that the Rust `PlatformWalletFFIResultCode` never defines (it stops at ErrorAddressNonceMismatch = 21, then NotFound = 98 / ErrorUnknown = 99). - PlatformWalletManager.swift: `loadFromPersistor()` now calls the real 1-arg `platform_wallet_manager_load_from_persistor(handle)` and checks its returned `PlatformWalletFFIResult`. Dropped the phantom `LoadOutcomeFFI` out-param, `platform_wallet_load_outcome_free`, the skipped-wallet collection loop, the `SkippedWalletOnLoad` struct, and the now-unpopulatable `lastLoadSkippedWallets` published property. - PlatformWalletResult.swift: removed the `errorPersisterLoadTransient` (22) / `errorPersisterLoadFatal` (23) result-code and error cases plus their `PLATFORM_WALLET_FFI_RESULT_CODE_ERROR_PERSISTER_LOAD_*` mappings, which map to cbindgen constants that are never generated. Swift is not compilable in this environment; symbols matched by hand against the Rust `#[no_mangle] extern "C"` signatures. CI's Swift build is the compile check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d SQL `tc_p1_003_prepare_cached_in_writers` regressed because this PR added `length(<col>)` pre-read size gates to SELECTs in two schema files but the `READ_ONLY_PREPARE_ALLOWED` substrings still matched the pre-gate SQL: - asset_locks.rs: all three readers (load_active / load_unconsumed / list_active) now read `SELECT length(outpoint), outpoint, account_index, length(lifecycle_blob), lifecycle_blob, status`; the old `SELECT outpoint, account_index` substring no longer matched. - core_state.rs: the instant-lock reader gained a `length(txid)` gate, so `SELECT length(txid), txid, length(islock_blob), islock_blob` replaces the stale `SELECT txid, length(islock_blob), islock_blob`. Self-check accuracy only — the gate intent is preserved, no logic change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion docs The `core_address_pool` table (V003) landed in THIS PR, and the writer now resolves `core_utxos.account_index` against it. Several docs still described the pre-table world: - wallet.rs `apply_persisted_core_state`: dropped the false "attribution is lost at write time (account_index is always 0)" framing and the stale `TODO(#3986)` blocker. The mapping IS persisted; this reader just doesn't read `account_index` back yet — restated as a deliberate present-state follow-up. - core_state.rs `load_state`: `account_index` IS carried by `core_utxos`; corrected the "not carried" claim and split the flag list accordingly. - sqlite_account_zero_attribution.rs: removed the false "no address→account lookup table" / "hardcoded" claims — the table exists; a fresh gap-limit-edge address simply has no pool row, so the writer falls back to account 0, which is exactly what this test pins. Doc-only; no behavior change — the reader still restores present-state balances onto a single funds account, preserving the zero-attribution test contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ment The comment claimed the manager skips empty-manifest wallets "as MissingManifest" and cited rt_corrupt_row_skipped_and_other_loads — a test that does not exist. The real behavior (locked in by tc043 / tc_p4_006 / tc_p4_007) is to register the wallet as an external-signable placeholder: an empty account_manifest is not necessarily orphaned — a platform-only wallet (identity + contacts, no core accounts) legitimately has one, and its platform-side state must still rehydrate. State that truth; keep the task #14 TODO as the deferred orphaned-row-recovery product decision. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ests.sh `security default-keychain -d user` exits non-zero on a self-hosted runner that has no user default keychain; under `set -euo pipefail` the command substitution aborted the whole script before any Swift build ran, failing the Swift SDK job. Make the capture non-fatal — the restore trap already skips an empty previous-default value, so capturing empty is harmless. Surfaced now because this PR's Swift FFI changes make the Swift SDK build run (previously skipped); the keychain preamble itself is unchanged base CI infra. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…etError Re-add the PlatformWalletError::PersisterLoad(#[from] PersistenceError) variant dropped in the load-outcome cleanup. thiserror's #[from] regenerates the From<PersistenceError> conversion that downstream consumers (dash-evo-tool) rely on to `?`-propagate persister-load failures, and the variant keeps the typed retry classification (PersistenceErrorKind) instead of flattening to a string. Maps to ErrorUnknown across the FFI via the existing catch-all arm. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add platform-wallet-storage to the allowed PR-title scopes so titles like feat(platform-wallet-storage): ... validate. Note: pr.yml runs on pull_request_target, so this takes effect for PR-title checks only once it lands on the base branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claudius-Maginificent
changed the title
…let-storage-rehydration
thepastaclaw
suggested changes
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers=general:claude/opus(failed), general:codex/gpt-5.5(ok), security-auditor:claude/opus(failed), security-auditor:codex/gpt-5.5(ok), rust-quality:claude/opus(failed), rust-quality:codex/gpt-5.5(ok); verifier=codex/gpt-5.5.
Carried-forward prior findings: prior-1 is STILL VALID at a503bd7; the SQLite load path still persists core_utxos.account_index but discards it during production wallet rehydration. New latest-delta findings: the newly added PlatformWalletError::PersisterLoad variant is not wired into load_from_persistor, so the retry classification it was added to preserve is still erased. Cumulative review found no additional in-scope defects beyond those two load-path issues.
Prior reconciliation: prior-1 is STILL VALID at a503bd7b and is carried forward below.
New findings in latest delta: one suggestion in packages/rs-platform-wallet/src/manager/load.rs: PlatformWalletError::PersisterLoad was added in the latest delta, but load_from_persistor still flattens persister load failures to WalletCreation(String), so typed retry classification remains erased.
Posting note: normal inline review path was unavailable locally (609, in post_review diff_text = _gh("pr", "diff", str(pr_number), "--repo", repo) File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 94, in _gh raise RuntimeError(f"gh {' '.join(args)} failed: {result.stderr.strip()}") RuntimeError: gh pr diff 3968 --repo dashpay/platform failed: could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of lines (20000) (https://api.github.com/repos/dashpay/platform/pulls/3968) PullRequest.diff too_large), so this exact-SHA review is body-only while preserving the verified findings.
🔴 1 blocking | 🟡 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-storage/src/sqlite/util/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:204-214: Restored UTXOs ignore the persisted owning account
prior-1 STILL VALID. The writer stores `core_utxos.account_index`, but `schema::core_state::load_state` still selects only outpoint/value/script/height and returns a flat `CoreChangeSet`. `apply_persisted_core_state` then inserts every unspent restored UTXO into `all_funding_accounts_mut().next()`, so a UTXO owned by account 1 is restored under the first funding account after restart. Although the comments label this as a deferred re-warm, the production load path exposes the wallet before any enforced full sync, so account-indexed balances, spending/signing, reservation cleanup, and address used-state can observe the wrong account namespace.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:40-45: load_from_persistor still erases persister load classification
The latest delta adds `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` specifically so rehydration callers keep `PersistenceErrorKind` and `is_transient()` retry classification, but this entry point still maps `self.persister.load()` failures into `WalletCreation(String)`. That flattens transient backend failures such as SQLite busy/full/I/O errors into an opaque string and makes the new variant ineffective for the manager load path it documents. Propagate the typed error directly so Rust callers can match `PersisterLoad` and inspect the retry classification.
…rth_height arg Bump the rust-dashcore git pin from 48a07d3 to b3fb8244 (head of dashpay/rust-dashcore#851) across all workspace refs. The newer key-wallet-ffi `wallet_manager_import_wallet_from_bytes` gains a `birth_height: u32` parameter, so update the Swift caller in WalletManager.swift to pass it (defaulted to 0 = full rescan, keeping `importWallet(from:)` source-compatible). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Collaborator
Author
Review disposition — thepastaclaw latest review (2 body-only findings)The two findings in the latest review were posted body-only (the PR diff exceeded GitHub's 20k-line inline cap), so they aren't resolvable threads. Disposition after verification against current 1. Restored UTXOs routed to the first funds account on rehydration ( 2. 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
suggested changes
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Code Review
Source: reviewers = claude ffi-engineer opus (failed); claude general opus (failed); codex ffi-engineer gpt-5.5; codex general gpt-5.5; verifier = codex gpt-5.5; specialists = ffi-engineer; policy gate = review_policy.enforce_backport_prereq_policy.
Prior reconciliation: prior-1 is STILL VALID and prior-2 is STILL VALID at f2779ca. Carried-forward prior findings: the SQLite load path still discards persisted UTXO account attribution, and load_from_persistor still flattens typed persister load errors. New findings in latest delta: none; the SPV peer-tracking/FFI/Swift delta did not add a separate actionable issue from the supplied findings.
Prior Findings Reconciliation
- prior-1: STILL VALID - Restored UTXOs ignore the persisted owning account
- prior-2: STILL VALID - load_from_persistor still erases persister load classification
Carried-Forward Prior Findings
- Restored UTXOs ignore the persisted owning account
- load_from_persistor still erases persister load classification
New Findings In Latest Delta
- None.
🔴 1 blocking | 🟡 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-storage/src/sqlite/util/wallet.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/util/wallet.rs:204-214: Restored UTXOs ignore the persisted owning account
Carried-forward prior-1 is STILL VALID. The writer resolves and stores `core_utxos.account_index`, but `schema::core_state::load_state` still selects only outpoint/value/script/height into a flat `CoreChangeSet`. `apply_persisted_core_state` then inserts every unspent restored UTXO into `all_funding_accounts_mut().next()`, so a UTXO owned by a later funding account is restored under the first funding account after restart. The wallet is exposed before any enforced resync, so account-indexed balances, spending/signing, reservation cleanup, and used-address state can observe the wrong account namespace.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/manager/load.rs:40-45: load_from_persistor still erases persister load classification
Carried-forward prior-2 is STILL VALID. `PlatformWalletError::PersisterLoad(#[from] PersistenceError)` exists to preserve `PersistenceErrorKind` and `is_transient()` retry classification, but this entry point still maps `self.persister.load()` failures into `WalletCreation(String)`. That flattens transient backend failures such as SQLite busy/full/I/O errors into an opaque string and makes the typed variant ineffective for the manager load path and the Swift/Rust FFI caller that depends on it.
GitHub refused normal diff-based posting for this oversized PR (PullRequest.diff too_large), so this exact-SHA review is body-only while preserving the verified findings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this PR exists
platform-walletdefines the persistence trait (PlatformWalletPersistence) and the manager-sideload_from_persistor()entry point, but ships no production storage backend. Without one, a wallet's Platform state — identities, contacts, identity keys, tracked asset locks, balances, and core sync watermarks — has nowhere durable to live.v4.1-dev, which already carries the shared rehydration scaffolding (ClientStartState,load_from_persistor, theWalletType::ExternalSignablemodel). This PR is the storage half — the net change againstv4.1-devis almost entirely the new crate.What was done
Adds
rs-platform-wallet-storage— a self-contained, embeddable SQLite persistence backend implementingPlatformWalletPersistence. One.dbfile holds many wallets, durable across restarts, with online backup/restore and automatic schema migration, under a hard contract: no private-key material is ever written to the database (signing material stays in the OS keyring or an encrypted vault).Persister & seedless rehydration
SqlitePersister(usable asArc<dyn PlatformWalletPersistence>,Send + Sync, object-safe) with configurable journal / synchronous / flush modes, a retention policy, and auto-backup.load()reconstructs each wallet external-signable from its persisted account manifest (Wallet::new_external_signable, no seed required), then layers the persisted core-state projection — UTXOs, sync watermarks, chainlock, used-address pool depth — viaapply_persisted_core_state. Prekeyed identity/contact joins mean signing works immediately post-load with no key re-sync.Schema & migrations (refinery; additive; version-pinned with golden schema-freeze fingerprints)
__initial— per-wallet tables keyed bywallet_id, nativeFOREIGN KEY … ON DELETE CASCADE; identity-owned tables cascade viaidentities.wallet_id.__address_height_pin— addsplatform_addresses.as_of_height(address double-count fix).__unified(additive, sequenced after V002) —core_address_pool(first-class per-index address-pool rows with ausedflag, giving real per-account UTXO attribution in place of an account-0 approximation),meta_data_versions(per-(wallet_id, domain)monotonic sequence for cache invalidation), andmeta_store_generation(restore-regenerated store token).Trust boundary & robustness (the
.dbis untrusted input at load)load(): any row that fails to decode, or an out-of-rangewallet_id, aborts the whole call with a typedWalletStorageError— no silent per-row skip, no partialOk.SIZE_LIMIT_BYTES) cap on KV values and BLOB decode, per-columnlength()pre-read gates before materialization, a bincode decode bounded by aLimitconfig that rejects trailing bytes, and a 32 MiBSQLITE_LIMIT_LENGTHconnection backstop.Secrets
SecretStore/EncryptedFileStore: Argon2id KDF + XChaCha20-Poly1305 AEAD envelope,zeroized, overkeyring-core), so signing material never touches the wallet.db.Changes outside the storage crate (deliberately minimal — the design intent was to leave
platform-walletunchanged and move persistence into its own crate)platform-wallet: doc-comment only (new_watch_only→new_external_signable).swift-sdk: the Platform-wallet load FFI consumer reconciled to the shipped 1-argplatform_wallet_manager_load_from_persistorcontract; one real fix inSendTransactionView(stop funding core-to-core sends from a Platform-Payment index); the rest are doc-comment renames.rust-dashcoredependency rev bump (key-wallet out-of-order UTXO spend fix); root.cargo/audit.tomlacknowledgesRUSTSEC-2025-0141(bincode unmaintained — an informational advisory, mitigated by the size caps + fail-hardload()above).Deferred (TODO-marked, no regression)
wallet_id) — tracked in feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992.How Has This Been Tested?
cargo clippy --all-features --all-targets -- -D warningsclean acrossplatform-wallet-storage,platform-wallet-ffi, andplatform-wallet.cargo nextest -p platform-wallet-storage: 561 passed / 0 failed — covering the per-domain readers, all three migrations, the schema-freeze fingerprints, per-account attribution, the DashPay pool-PK collision regression, and the prepare-cached read-only self-check.Swift SDK build) — FFI symbols were matched by hand against the Rustextern "C"surface.Breaking Changes
None in this PR's net diff. The storage crate is purely additive; the
platform-wallet/swift-sdktouches are comment renames plus one localized send-funding fix. (TheWalletType::ExternalSignablemodel this crate consumes already lives inv4.1-dev.)Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent