feat(platform-wallet)!: add platform-wallet-storage crate (sqlite persister)

[Claudius-Maginificent]

Update — review triage CMT-001…006 + contact-table unification (latest, 2026-06-02)

Latest review-comment triage applied and pushed:

• CMT-001 (4e38975e53) — dropped the keyutils backend + the linux-keyutils-keyring-store dependency. Linux/FreeBSD now use the dbus Secret Service, with the encrypted-file vault as the explicit fallback (keyutils session keyrings are wiped on logout → persisted secrets would silently vanish across sessions).
• CMT-002 (e78eb55d70) — assert_identities_belong_to_wallet now runs in all builds (removed the cfg!(debug_assertions) gate) and propagates a typed WalletIdMismatch instead of being a debug-only check.
• CMT-004 / 005 / 006 (3fc28b9221) — FileStoreError → backend-neutral SecretStoreError (moved to secrets/error.rs); SecretStoreError::Io now carries the vault path; SecretStore Debug renders the OS arm as Os { vendor, id }.
• CMT-003 (932b923b2b) — unified the three contact tables (contacts_sent / contacts_recv / contacts_established) into a single lifecycle-state contacts table keyed (wallet_id, owner_id, contact_id) with state ∈ {sent, received, established} plus nullable request/metadata columns. A no-downgrade upsert (CASE WHEN contacts.state='established' …) preserves an established relationship against a late-arriving pending row; a matching sent+received pair auto-collapses to established. The meta_contact trigger and PER_WALLET_TABLES retarget the unified table. Verified end-to-end (Marvin QA PASS) — the feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 rehydration reader was reworked for the unified shape, 513 tests pass, no schema↔reader column drift.

Note: an earlier triage round also used the labels CMT-001…003 for different items (single-read KV get(), rollback-caveat rustdoc, bincode outpoints) — see the "Post-review refinements" line further down. The list above is the later round.

---

Update — Per-object metadata (KvStore → ObjectId, soft-cascade) (latest)

Adds a per-object-type key-value metadata facility and replaces the prior flat kv_store table. Motivation: downstream (dash-evo-tool) was hand-rolling colon-namespaced keys on a flat, untyped KV; this lifts metadata into a typed facility — while letting hosts attach metadata to an object before / independently of that object being synced into its typed table.

• Adapted the existing KvStore trait (no new trait): scope generalized from Option<&WalletId> to a &ObjectId enum — Global, Wallet, Identity, Token, Contact, PlatformAddress. Per-variant match dispatch to the backing table.
• Six dedicated meta_* tables, one per ObjectId scope (meta_global, meta_wallet, meta_identity, meta_token, meta_contact, meta_platform_address), replacing kv_store. Typed id column(s) per scope; opaque Vec<u8> values (16 MiB cap with a pre-materialization length() guard); keys 1–128 chars.
• meta_* carry NO foreign key — by design, and deliberately unlike every other table. Every other per-wallet table uses a hard FOREIGN KEY … ON DELETE CASCADE (the parent must exist at write time). The meta_* tables omit it so hosts can write metadata before/independently of the parent being synced (async sync ordering; a global-config persister whose parent tables stay empty). Parentless writes succeed — there is no insert-time parent-existence error.
• Cleanup via AFTER DELETE triggers on the five parent tables (wallet_metadata/identities/token_balances/contacts_established/platform_addresses). SQLite fires these triggers even for parent rows removed by an FK ON DELETE CASCADE, so delete_wallet still transitively purges all of a wallet's metadata — including the two-hop wallet → identities → token_balances → meta_token chain. "Metadata cannot outlive its object" is preserved; only the insert-time parent requirement is dropped. (No recursive_triggers pragma is needed — verified against the SQLite docs and by tests that assert cleanup with it OFF.)
• The five per-wallet meta_* tables are registered in PER_WALLET_TABLES, so delete_wallet's report counts them; meta_global (no parent) is excluded and survives wallet deletion.
• Additive / non-breaking within this crate. Merged into V001__initial.rs in place — no V002.

Validated end-to-end via claudius:workflow-feature (Nagatha design review → Bilby implementation → Marvin / Adams / Smythe QA, all PASS): tests cover parentless writes, native per-object cascade, one-hop and two-hop delete_wallet cleanup, and existing-trigger coexistence; both clippy gates clean (incl. --no-default-features --features sqlite,cli -- -D warnings); SQL surface injection-proof (static table names + bound params).

Post-review refinements (PR #3625 triage): single-read KV get() closes a size-cap TOCTOU (CMT-001); the cross-process rollback caveat is lifted onto the public restore_from/delete_wallet rustdoc (CMT-002); outpoint columns now encode via bincode instead of a fixed-width hand-codec (CMT-003).

---

SQLite schema reference: packages/rs-platform-wallet-storage/SCHEMA.md — Mermaid ER diagram + per-table column reference for the 25-table V001 schema (renders inline on GitHub).

Issue being fixed or feature implemented

platform-wallet's PlatformWalletPersistence trait (packages/rs-platform-wallet/src/changeset/traits.rs:118) shipped with only NoPlatformPersistence — a no-op stub. The canonical SQLite implementation lived downstream inside dash-evo-tool and was not reusable by FFI consumers, desktop apps, tests, or CLI tooling. This PR introduces a new workspace crate, platform-wallet-storage, that owns the canonical SQLite persister today and reserves a submodule slot for a future SecretStore (OS keyring + encrypted file backends) so storage concerns for the wallet stack live under one roof.

User story

As a wallet integrator (Rust embedder, FFI consumer, or desktop-app author), I want a ready-made storage crate for platform-wallet so that I can persist wallet state across restarts without writing my own schema, my own migration pipeline, or my own backup/restore tooling — and I want the persister to take an automatic backup before anything destructive so I cannot accidentally lose data on a bad migration or a wrong wallet ID. Later, I want the same crate to offer a SecretStore abstraction so I do not have to roll my own OS-keyring integration on every platform.

What was done?

Two crates touched

1. platform-wallet (packages/rs-platform-wallet/) — added an optional serde Cargo feature that gates #[derive(Serialize, Deserialize)] on every changeset type. Feature is off by default; library behaviour unchanged when not activated. Cherry-pickable into its own upstream PR via commit e26945cfdf.
2. platform-wallet-storage (packages/rs-platform-wallet-storage/, new) — the actual storage crate. Module layout:

src/
├── lib.rs               # re-exports + (placeholder) `pub mod secrets;`
├── error.rs
├── sqlite/              # everything SQLite-specific lives here
│   ├── persister.rs     # impl PlatformWalletPersistence for SqlitePersister
│   ├── buffer.rs        # per-wallet ChangeSet merge buffer
│   ├── config.rs
│   ├── migrations.rs    # refinery + barrel
│   ├── backup.rs        # online backup, restore, retention
│   ├── schema/*.rs      # one writer/reader module per logical area
│   └── migrations/*.rs
├── secrets/             # RESERVED slot for the future SecretStore submodule (not implemented)
└── bin/
    └── platform-wallet-storage.rs   # clap-based CLI

Cargo features: default = ["sqlite", "cli"]. --no-default-features produces a bare crate so future secrets-only consumers can opt in just to that.

Highlights

• Trait implementation — SqlitePersister implements PlatformWalletPersistence (sync, Send + Sync, object-safe behind Arc<dyn …>).
• Schema — 25 tables (per-wallet keyed) covering core state, identities, identity keys (public material only — verified by tests/secrets_scan.rs), contacts, platform addresses, asset locks, token balances, DashPay overlays, wallet metadata, account registrations, address pools. Single .db holds many wallets.
• Foreign keys enabled with ON DELETE CASCADE. Composite-key parent-existence + cascade are trigger-emulated (barrel's sqlite3 backend cannot emit composite FK clauses portably; PRAGMA foreign_keys = ON only enforces FKs declared in CREATE TABLE).
• Buffer-then-flush — single Mutex<HashMap<WalletId, PlatformWalletChangeSet>> merging via upstream Merge impls; one flush() call = one SQLite transaction. FlushMode::{Manual, Immediate} switch + commit_writes() end-of-batch hook.
• Migrations via refinery 0.9 + barrel. Append-only.
• Online backup / restore / retention via rusqlite::backup::Backup::run_to_completion. Retention by keep_last_n + max_age AND-semantics. TOCTOU-safe restore via tempfile::NamedTempFile::new_in(parent_dir) + atomic persist.
• Automatic backups before destructive ops — pre-migration in open, pre-delete in delete_wallet. Library returns typed AutoBackupDisabled if auto_backup_dir = None; CLI escape hatch is a separate delete_wallet_skip_backup sibling method (does NOT mutate config — keeps library safe-by-default).
• CLI bin platform-wallet-storage with backup, restore, prune, inspect, migrate, delete-wallet subcommands. Destructive ops require --yes. -v/-q wired to tracing_subscriber. Unix stream conventions.
• Encoder — bincode::serde::encode_to_vec over serde-derived changeset types No hand-rolled binary format. One carve-out: dpp's IdentityPublicKey uses #[serde(tag = "$formatVersion")] which bincode-serde rejects, so identity_keys.rs uses a wire-shape struct that re-encodes that one field via dpp's native bincode 2 derives — one blob per row preserved.
• Secrets policy documented in SECRETS.md. The reserved src/secrets/ slot is the only directory exempt from the secrets_scan.rs forbidden-substring grep.
• Workspace integration — added to Cargo.toml members, Dockerfile COPY --parents blocks (3 stages), .github/workflows/tests-rs-workspace.yml (--package lists in both shards), .github/workflows/pr.yml allowed-scope (wallet-storage).

Commits (strict file boundaries for clean cherry-pick)

SHA	Title	Scope
0f9437cd44	feat(wallet-sqlite): add platform-wallet-sqlite crate	initial crate scaffolding
adf421257c	ci(wallet-sqlite): wire crate into workspace CI, Dockerfile, and Cargo.toml	workspace integration
cea9ddad4d	fix(wallet-sqlite): library/CLI/tests/docs fix wave from Phase 3 QA	post-QA fixes
e26945cfdf	feat(platform-wallet): add optional serde derives behind serde feature	only packages/rs-platform-wallet/ — independently cherry-pickable
8e0830626d	refactor(wallet-storage): rename platform-wallet-sqlite → platform-wallet-storage and restructure for future secrets submodule	pure rename + reshape
74acc8152b	refactor(wallet-storage): use bincode-serde for BLOB columns, remove hand-rolled encoder	-313 net LOC, +6 previously-deferred tests unblocked
5bac6e304d	refactor(wallet-storage): drop per-blob schema-rev tag, rely on migration version for forward-compat	-33 net LOC, blob.rs trimmed to thin bincode-serde wrappers
540decf652	refactor(wallet-storage): drop --dry-run from prune CLI	-53 net LOC, helper + flag + docs
4cfec30375	fix(platform-wallet): correct stale crate name in doc comment after wallet-storage rename	upstream PR cherry-pick alongside e26945c
bd4216dbe2	refactor(wallet-storage): rename SqlitePersisterError → WalletStorageError, atomic variants, propagate SQL errors	error rework + safe_cast + LoadIncomplete + pre-restore backup + chmod 600 + integrity recheck
f58e784593	fix(wallet-storage): SEC-003 defensive update triggers + build-script migration tracking	V002 BEFORE-UPDATE rejection triggers on wallet_id / identity_id
87f38c0f15	chore(wallet-storage): post-review cleanup (delete CHANGELOG, JSON escaping, scope allow-list, stable enum labels, docs)	serde_json, db_label helpers, deletes CHANGELOG.md, drops wallet-sqlite scope
f45ab091ae	fix(platform-wallet-storage): drain buffer on wallet delete (CMT-001)	drain-and-discard buffered changeset first; existence gate widened to buffered-or-persisted
f18a1e4b8c	fix(platform-wallet-storage): recheck schema/version on staged copy (CMT-002)	schema-history + max-version checks moved onto the staged temp copy; closes restore TOCTOU
bed413e513	fix(platform-wallet-storage): reject trailing bytes in blob decode (CMT-003)	blob::decode enforces bincode bytes-consumed == input length
e2746c3207	test(platform-wallet-storage): make tc054 root-safe (CMT-004)	regular-file-as-parent yields ENOTDIR for every UID incl. root

Workflow

Built end-to-end via the claudius:workflow-feature skill: Diziet's Requirements + DX spec (26 FRs, 10 NFRs, 15-variant error catalogue) → Marvin's 83-case test specification → Bilby's implementation → parallel Phase-3 QA wave (Marvin / Smythe / Adams / Diziet / Trillian) → Bilby's fix wave (resolved 2 CRITICAL from Adams, 1 HIGH from Diziet, 2 HIGH from Marvin, 3 HIGH from Adams, 4 MEDIUM from Smythe / Trillian / Diziet) → reshape (crate rename + serde swap) → Lessons Learned (18 memories persisted).

How Has This Been Tested?

• `cargo test -p platform-wallet-storage` — full suite green, 0 failures, 0 ignored (1 proptest oracle in slow-running mode). Includes the regression tests added for the CodeRabbit fixes (tests/sqlite_delete_buffer_reconcile.rs, tests/sqlite_restore_staged_validation.rs, and the blob::decode trailing-bytes negative test).
• `cargo build -p platform-wallet-storage` (with and without --no-default-features) — clean.
• `cargo build -p platform-wallet-storage --bin platform-wallet-storage` — clean.
• `cargo build -p platform-wallet --features serde` — clean (opt-in feature off by default; both states verified).
• `cargo clippy -p platform-wallet-storage -p platform-wallet --all-targets -- -D warnings` — clean.
• `cargo fmt --all -- --check` — clean.
• `cargo check --workspace --offline` — clean (no regressions elsewhere).

Coverage gained in the encoder swap (previously deferred per-sub-changeset round-trips, now green): TC-007 (identity keys), TC-009 (platform addresses), TC-010 (asset locks incl. embedded Transaction), TC-012 (DashPay overlays), TC-014 (account registration with full ExtendedPubKey).

Deferred (with rationale, follow-up PRs)

• ClientStartState.wallets reconstruction in load() — blocked on a future upstream Wallet::from_persisted. All data IS persisted; only the wallet rehydration step is gated. TODO in persister.rs::load.
• A handful of fault-injection-seam test cases (TC-021 / TC-024) — partial-rollback under injected failure; queued behind the existing lock_conn_for_test test-helper accessor.
• TC-003, TC-006 (InstantLock, Transaction direct round-trips) — kept on consensus::Encodable for those bytes (canonical Bitcoin-style wire format); a future PR may unify on serde if dashcore gains compatible derives.
• Workspace-wide bincode 2.0.1 advisory (RUSTSEC-2025-0141, unmaintained) — workspace concern, not crate-specific.
• Upstream key_wallet::Wallet's WalletType::Mnemonic does not zeroize its seed fields on drop. Out of scope; flagged for upstream follow-up.

Breaking Changes

Yes — platform-wallet::changeset::traits::PersistenceError is API-breaking (CMT-019).

The error variant was reshaped from a stringly-typed tuple into a structured variant that carries a typed retry-classification kind:

// Before (origin/v3.1-dev)
pub enum PersistenceError {
    LockPoisoned,
    Backend(String),
}
impl PersistenceError {
    pub fn backend(err: impl std::fmt::Display) -> Self { /* stringify */ }
}
// `From<String> for PersistenceError` also existed.

// After (this PR)
pub enum PersistenceError {
    LockPoisoned,
    Backend {
        kind: PersistenceErrorKind,
        source: Box<dyn std::error::Error + Send + Sync>,
    },
}
pub enum PersistenceErrorKind { Transient, Fatal /* #[non_exhaustive] */ }
impl PersistenceError {
    pub fn backend(err: impl Into<Box<dyn std::error::Error + Send + Sync>>) -> Self;
    pub fn backend_with_kind(kind: PersistenceErrorKind, err: ...) -> Self;
}

Migration for downstream consumers:

• match e { PersistenceError::Backend(msg) => … } → match e { PersistenceError::Backend { kind, source } => … }.
• PersistenceError::from(String) no longer exists — callers building errors from format!(…) should use PersistenceError::backend(format!(…)) (the new constructor takes anything Into<Box<dyn Error + Send + Sync>>, including String).
• New PersistenceErrorKind lets callers branch on retry semantics (Transient ⇒ buffered state preserved, safe to retry with backoff; Fatal ⇒ unrecoverable). #[non_exhaustive] so a future kind doesn't silently break exhaustive matches.

The new crate (platform-wallet-storage) itself is opt-in. The platform-wallet/serde feature is also opt-in (off by default).

Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have added "!" to the title and described breaking changes in the corresponding section
• I have made corresponding changes to the documentation if needed (README, SECRETS.md, rustdoc on every public item)

For repository code-owners and collaborators only

• I have assigned this pull request to a milestone

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

Summary by CodeRabbit

• New Features

  • SQLite-backed wallet persistence with migrations, online backup/atomic restore, pruning, and per-wallet delete with safe pre-delete snapshots
  • CLI tool for migrate/backup/restore/prune/inspect operations
  • Key/value metadata API for app metadata and a secrets subsystem with encrypted file vault and OS keyring backends

• Documentation

  • Comprehensive README, SCHEMA.md and SECRETS.md describing behavior, backups, and secret boundaries

• Tests

  • Broad integration and unit tests covering persistence, backup/restore, permissions, and error classification

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

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

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds a new crate implementing SQLite-backed platform wallet persistence (embedded migrations, buffered writes, online backup/atomic restore, retention pruning, KV, secrets backends), a CLI, schema modules, trait/error model updates, FFI error mapping, workspace/CI/Docker wiring, and extensive tests.

Changes

SQLite Wallet Storage

Layer / File(s)

Summary

End-to-end crate and integration changes .github/workflows/*, Cargo.toml, Dockerfile, packages/rs-platform-wallet-storage/..., packages/rs-platform-wallet/..., packages/rs-platform-wallet-ffi/...

Adds the platform-wallet-storage crate (library + CLI), build script and embedded migrations, SqlitePersister with buffering/flush/delete/commit/backup/restore/prune, schema writer/readers (core, identities, contacts, asset locks, platform addresses, token balances, wallet_meta, accounts, dashpay, blob codecs), KV API & SQLite-backed KvStore, secrets subsystem (encrypted file vault + OS keyring), safe-cast/permissions helpers, trait/error model updates to carry PersistenceErrorKind, FFI error mapping, workspace/CI/Docker updates, and comprehensive unit/integration tests.

Sequence Diagram(s)

sequenceDiagram
  rect rgba(200, 230, 255, 0.5)
  participant CLI
  participant Persister
  participant SQLiteDB
  participant Secrets
  end
  CLI->>Persister: migrate / backup / restore / prune / inspect
  Persister->>SQLiteDB: open, enforce PRAGMAs, run migrations, transactional writes
  Persister->>SQLiteDB: online backup / staged restore (integrity + schema checks)
  Persister->>Secrets: optional pre-restore auto-backup or secret storage ops
  Secrets->>Persister: credential/store confirmation or errors
  Persister-->>CLI: PruneReport / exit codes / PersistenceErrorKind

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested labels

ready for final review

Suggested reviewers

• shumkov
• QuantumExplorer
• thepastaclaw

Poem

A rabbit with keys and bytes to keep,
Tunnels through tables, migrations deep.
Backups like carrots, safely stored,
Restores hop back, state is restored.
Buffers burrow, then flush on cue—
SQLite warren, shiny and new. 🥕

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/platform-wallet-sqlite-persistor

[lklimek]

@coderabbitai review all

[coderabbitai]

@lklimek Sure! I'll kick off a full review of all the changes in this PR right away.

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 15

🧹 Nitpick comments (11)

packages/rs-platform-wallet-storage/src/sqlite/config.rs (1)

111-118: 💤 Low value

Minor: Simplify empty parent check.

The .filter(|p| !p.as_os_str().is_empty()) check at line 114 is defensive, but Path::parent() should not return an empty OsStr in practice for well-formed paths. The fallback to "." is safe regardless.

Consider simplifying to:

let parent = db_path.parent().unwrap_or_else(|| Path::new("."));
parent.join("backups").join("auto")

However, the current implementation is safe and may handle edge cases in path handling, so this is optional.

🤖 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/config.rs` around lines 111 -
118, The function default_auto_backup_dir contains an overly defensive check
when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.

packages/rs-platform-wallet-storage/tests/common/mod.rs (1)

47-56: 💤 Low value

Hardcoded 'testnet' may collide with tests asserting on network.

ensure_wallet_meta always writes network = 'testnet'. Tests that later assert on wallet metadata or persist a WalletMetadataEntry with a different network (e.g., tc023_one_flush_is_one_transaction writes Network::Testnet and would silently match, but other tests writing Mainnet would observe a stale row from this helper). Consider taking network as a parameter (defaulting to "testnet") so call sites can match their changeset's expectations.

🤖 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/common/mod.rs` around lines 47 -
56, The helper ensure_wallet_meta currently hardcodes network = 'testnet', which
can collide with tests expecting other networks; change the signature of
ensure_wallet_meta(persister: &SqlitePersister, wallet_id: &WalletId) to accept
a network parameter (e.g., network: &str) with callers passing "testnet" where
appropriate, update the INSERT SQL call in ensure_wallet_meta to bind the
network parameter instead of the literal 'testnet', and update all test call
sites (or add a convenience wrapper) to pass the correct network values (e.g.,
"mainnet" or "testnet") so tests see consistent wallet_metadata rows.

packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs (2)

188-231: 💤 Low value

Two if let Cmd::Migrate branches — fold them.

The Migrate command is special-cased twice in succession (lines 219–231 to tweak config / validate flags, then lines 235–244 to actually run it). Folding both into a single if let Cmd::Migrate(m) = &cli.cmd { ... return ... } block (or a run_migrate helper) would keep all of the migrate-specific logic in one place and remove the unreachable!() arm on line 247.

🤖 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/bin/platform-wallet-storage.rs`
around lines 188 - 231, The migrate handling is split across two places; fold
the two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.

---

122-134: 💤 Low value

Reject uppercase / odd-length hex consistently.

parse_wallet_id checks s.len() == 64 then defers to hex::decode, which accepts both upper- and lower-case but not mixed-case-with-non-hex obviously — that's fine. However, the error message on line 124 leaks the raw user-supplied string back into stderr; if this binary is ever invoked from a context that pipes secrets-like opaque IDs, echoing them is undesirable. Consider dropping the trailing (`{}`) from the message and just naming the length, matching the wallet id is not valid hex style on line 130.

🤖 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/bin/platform-wallet-storage.rs`
around lines 122 - 134, In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.

packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs (2)

273-278: 💤 Low value

Remove redundant _unused_btreemap dead-code shim.

BTreeMap is now actually used in tc023_one_flush_is_one_transaction (see line 308), so the #[allow(dead_code)] fn _unused_btreemap workaround can be deleted along with its comment.

♻️ Suggested removal

-// Mark the unused `BTreeMap` import as used in case future expansion of
-// this test file needs it.
-#[allow(dead_code)]
-fn _unused_btreemap() -> BTreeMap<u32, u32> {
-    BTreeMap::new()
-}
-

🤖 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_buffer_semantics.rs` around
lines 273 - 278, Remove the redundant dead-code shim `_unused_btreemap` and its
explanatory comment: delete the `#[allow(dead_code)] fn _unused_btreemap() ->
BTreeMap<u32, u32> { BTreeMap::new() }` block since `BTreeMap` is now actually
used in the test `tc023_one_flush_is_one_transaction`, making the shim
unnecessary; ensure imports remain and run tests to confirm nothing else depends
on that helper.

---

154-180: 💤 Low value

Proptest opens a fresh SQLite DB per case — consider reusing one persister.

fresh_persister() runs once per proptest case (×64), each migrating a brand-new on-disk SQLite DB. That makes this test materially slower and more fragile on tmpfs/CI than necessary. Since the property is purely about monotonic-max merge on core_sync_state for a single wallet, you could hoist persister creation out of the proptest closure and just use a fresh wid per case (or reset the row), keeping the same DB across cases.

🤖 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_buffer_semantics.rs` around
lines 154 - 180, Test currently calls fresh_persister() inside the proptest
closure causing a new on-disk SQLite DB per case; move persister creation out of
the proptest! Create the persister once before proptest (call fresh_persister()
once to get persister/_tmp/_path), then inside the proptest closure use a new
wallet id (wid) per case or clear/reset the core_sync_state row (use
ensure_wallet_meta and persister.store with core_with_height) so each case
reuses the same DB connection and only varies the wallet or row content; update
references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.

packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)

72-102: 💤 Low value

ensure_exists writes outside the buffer/flush transaction boundary.

ensure_exists takes a &Connection and runs an immediate INSERT OR IGNORE rather than participating in the in-memory merge buffer + per-flush transaction model used by apply. This is fine for the documented use case (tests poking the DB before exercising identity_keys), but worth a doc note so production callers don't reach for it and accidentally couple stub writes to a different durability boundary than the rest of a changeset flush.

🤖 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 72 - 102, ensure_exists currently performs an immediate INSERT OR IGNORE
on a plain &Connection, which bypasses the in-memory merge buffer and per-flush
transaction used by apply and can cause durability/ordering mismatches; update
ensure_exists to participate in the same flush boundary by either accepting and
using the existing merge buffer/flush transaction or a transactional handle
(e.g., a &Transaction or buffer API used by apply) and executing the INSERT
within that transaction, or if intentionally intended only for tests, add a
clear doc comment on ensure_exists describing that it writes immediately to the
DB and must not be used by production code that relies on the merge-buffer +
per-flush transaction model (reference ensure_exists, apply, and the in-memory
merge buffer/flush transaction behavior in your comment).

packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs (1)

20-26: ⚡ Quick win

Consider widening SafeCastTarget to cover u32-bound writes.

Several call sites (e.g., asset_locks::list_active, core_state::list_unspent_utxos) need to fail when an i64 won't fit in u32 and currently report SafeCastTarget::U64, which is wrong. Adding a U32 variant here (and a small i64_to_u32 helper) would let those readers go through this module with an accurate target label and consistent error shape.

🤖 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/util/safe_cast.rs` around
lines 20 - 26, The SafeCastTarget enum currently only lists I64 and U64 but
callers need a U32 target; add a new variant SafeCastTarget::U32 and implement a
small helper function i64_to_u32 that mirrors existing i64_to_u64/i64_to_i64
behavior (validate range, return Result<u32, SafeCastError> and report
SafeCastTarget::U32 on failure). Update any match/log paths in this module that
construct errors to use the new U32 variant so callers like
asset_locks::list_active and core_state::list_unspent_utxos can call the helper
and produce the correct error shape/message.

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (2)

185-191: 💤 Low value

Comment references inject_custom but the code appends raw DDL.

The comment claims constraints/indexes are layered "via inject_custom", but the implementation just concatenates DDL after m.make::<Sqlite>(). Either switch to barrel's inject_custom API or update the comment to reflect what's actually happening so future maintainers don't go looking for an inject_custom call site.

🤖 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/migrations/V001__initial.rs` around lines
185 - 191, The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.

---

281-326: 💤 Low value

Unused _cols parameter in parent_check closure.

The closure takes _cols: &[&str] but never uses it; every call passes &["wallet_id"]. Consider removing the parameter, or actually using it to parameterize the FK column name so the closure can support FK relationships keyed on something other than wallet_id (which would let you fold the bespoke identity_keys / dashpay_profiles triggers below into the same helper).

🤖 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/migrations/V001__initial.rs` around lines
281 - 326, The closure parent_check currently accepts an unused parameter _cols:
&[&str]; either remove that parameter from parent_check and all its callers
(keep it as parent_check(child: &str) and continue using wallet_id inside the
generated trigger SQL), or update parent_check to accept cols: &[&str] and use
cols[0] (or join cols) in place of the hard-coded wallet_id everywhere
(including the WHEN clauses and DELETE WHERE clause) so the same helper can
generate triggers for identities, identity_keys, dashpay_profiles, etc.; ensure
you update the for loop callers accordingly to pass either no cols (if removed)
or the appropriate slice like &["wallet_id"] or &["identity_id"] where needed.

packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs (1)

102-107: ⚡ Quick win

SafeCastTarget::U64 is misleading for a u32 overflow.

The cast here is i64 → u32, but the surfaced target is SafeCastTarget::U64. Operators reading the error will see "u64" and assume the value didn't fit in u64, when in fact it didn't fit in u32. Consider adding a U32 variant to SafeCastTarget (and a corresponding i64_to_u32 helper in safe_cast) so the error type accurately describes the target, and to avoid repeating this try_from pattern at every reader. This same issue recurs in core_state.rs for core_utxos.height and core_utxos.account_index.

🤖 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/asset_locks.rs` around
lines 102 - 107, The IntegerOverflow error currently reports SafeCastTarget::U64
for an i64→u32 conversion in asset_locks.rs (see the u32::try_from usage
producing WalletStorageError::IntegerOverflow), which is misleading; add a U32
variant to crate::sqlite::util::safe_cast::SafeCastTarget and implement a
dedicated i64_to_u32 helper in the safe_cast module that performs the conversion
and returns WalletStorageError::IntegerOverflow with target =
SafeCastTarget::U32 on failure, then replace the manual u32::try_from usages
(e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.

🤖 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-storage/src/bin/platform-wallet-storage.rs`:
- Around line 235-244: The current migrate branch uses peek_schema_version which
swallows all errors into None causing applied to be unreliable; change
peek_schema_version to return Result<Option<i64>, E> (i.e., Result<Option<i64>,
Box<dyn Error> or a concrete error type) and update the call site in the
Cmd::Migrate block: keep using the old behavior for pre_version (call
peek_schema_version before opening the DB if you must tolerate missing table),
but after SqlitePersister::open(config.clone()) call the new peek_schema_version
and propagate or error out on Err so the post_version probe cannot be silently
treated as 0; compute applied only when post_version is Ok(Some/_), and map
errors to the CLI error flow (use map_open_err_for_cli or similar) instead of
unwrap_or(0) so the printed "applied: N" is trustworthy.
- Around line 288-300: The CLI-level pre-check args.out.is_file() in run_backup
is redundant and does not prevent the TOCTOU race because backup_to() itself
checks exists() and ultimately calls run_to() -> Connection::open(dest) which
can be raced; either remove the args.out.is_file() guard from run_backup, or
(preferred) harden the persistence path by modifying backup_to()/run_to() to
perform atomic file creation (e.g., use OpenOptions::create_new() or equivalent)
when opening the destination instead of relying on exists()/Connection::open,
ensuring the creation fails if the file was created concurrently.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 517-520: The code that rebuilds state in load() uses
schema::platform_addrs::count_per_wallet and then inserts addrs into
state.platform_addresses only if count > 0 || addrs.sync_height > 0 ||
addrs.sync_timestamp > 0, which drops wallets whose only persisted platform
state is addrs.last_known_recent_block; update the reconstruction condition to
also check addrs.last_known_recent_block (e.g., include ||
addrs.last_known_recent_block > 0) so that entries with only
last_known_recent_block are preserved when inserting into
state.platform_addresses.
- Around line 412-468: flush_inner currently calls self.buffer.drain(wallet_id)
and discards the changeset (cs) before opening the DB transaction, so any
failure after draining (e.g., during schema::...::apply or tx.commit()) loses
the buffered changes; change the logic so the buffer is only removed after
tx.commit() succeeds: either fetch/clone/peek the changeset (use a
non-destructive read API instead of buffer.drain) into cs, execute the schema
apply calls and tx.commit(), and only then call buffer.drain(wallet_id) or
buffer.remove(wallet_id) to clear it; if you must drain early, ensure every
error path re-inserts/requeues the drained cs back into the buffer (e.g.,
buffer.insert(wallet_id, cs)) before returning the PersistenceError, referencing
flush_inner, self.buffer.drain(wallet_id), cs, and tx.commit() to locate the
edits.
- Around line 269-326: Before checking existence/backing up/deleting in
delete_wallet_inner, reconcile in-memory buffered writes for the target wallet:
call the component that persists or discards pending buffer entries (e.g.,
flush/commit_writes or discard_buffered_writes) for wallet_id before the initial
conn() existence check and before run_auto_backup; propagate any error from that
operation so the delete aborts on flush failures. Locate delete_wallet_inner and
insert the flush/discard call at the top (prior to the SELECT 1 and
run_auto_backup usages) and ensure the rest of the logic (PER_WALLET_TABLES
counting, schema::wallet_meta::delete, tx.commit) operates on the now-consistent
persisted state.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:
- Around line 26-28: The decode function currently ignores the bytes-consumed
result from bincode::serde::decode_from_slice, allowing trailing garbage to pass
as valid; update the blob::decode implementation (the decode function that calls
bincode::serde::decode_from_slice) to check the second tuple element
(bytes_consumed) against blob.len() and return a WalletStorageError (e.g.,
Err(WalletStorageError::CorruptedBlob) or similar existing error variant) when
bytes_consumed != blob.len(), otherwise return the decoded value; ensure the
error path uses the same error type returned on bincode failures to keep the
function signature consistent.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 108-136: The upsert_utxo function currently writes account_index
as 0 into core_utxos causing list_unspent_utxos to misassign UTXOs; fix by
populating account_index before inserting: either (a) perform a lookup/join
against core_derived_addresses inside upsert_utxo (match wallet_id +
script/address) to derive the correct account_index and use that value instead
of 0, or (b) modify the Utxo/CoreChangeSet pipeline so the writer that calls
upsert_utxo receives and passes the owning account_index, or (c) add a
deterministic backfill writer that runs after writes but before any
list_unspent_utxos calls to update core_utxos.account_index from
core_derived_addresses; update upsert_utxo (and any callers of
upsert_utxo/CoreChangeSet) to ensure account_index is never left as the
hardcoded 0.
- Around line 138-174: The read in upsert_sync_state currently uses lp.map(|x| x
as u32) / sy.map(|x| x as u32) which silently truncates out-of-range i64 values;
change the rusqlite row mapping to return Option<i64> for both heights (keep the
closure in query_row returning (Option<i64>, Option<i64>)), then after the
query_row.optional()? unwrap_or((None,None)) convert each Option<i64> to
Option<u32> using a checked helper (reuse i64_to_u64 + u32::try_from or add
i64_to_u32) and map conversion errors to WalletStorageError::IntegerOverflow so
the function returns an error rather than silently wrapping before using lp and
sy in the INSERT/ON CONFLICT params.
- Around line 27-45: The code iterates cs.spent_utxos and calls upsert_utxo(tx,
wallet_id, utxo, true) when the outpoint is missing, which will materialize a
synthetic UTXO with account_index = 0; document this intent at the call site or
add an explicit guard flag to make the exceptional behavior obvious. Update the
block in the loop that handles cs.spent_utxos to either (a) add a clear comment
above the else branch referencing upsert_utxo and why creating a spent-only
placeholder with account_index = 0 is safe and will be corrected later, or (b)
wrap the else branch in a condition/flag (e.g., only_insert_spent_placeholders)
and pass that flag through to upsert_utxo so callers must opt into creating
synthetic rows; reference cs.spent_utxos, upsert_utxo, and the
core_utxos/account_index = 0 behavior in the comment or the new guard.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 42-64: fetch currently only selects entry_blob and discards the
tombstoned column referenced in the doc; change fetch to SELECT entry_blob,
tombstoned from identities and return the tombstone flag alongside the decoded
IdentityEntry (e.g. update the signature to Result<Option<(IdentityEntry,
bool)>, WalletStorageError> or a small wrapper type), decode payload with
blob::decode for the entry, map the tombstoned SQL value to a bool, and update
the doc comment to reflect the new return shape (ensure you still use
rusqlite::OptionalExtension and propagate errors as before).

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 72-89: The upsert loop writes typed key columns from the loop key
((identity_id, key_id), wallet_id) but serializes the payload from entry (via
IdentityKeyWire::from_entry), which can produce mismatches; ensure the
serialized wire and the SQL key are consistent by either normalizing the wire
values to the loop key or rejecting mismatches before persistence: construct or
overwrite IdentityKeyWire fields using the loop's wallet_id/identity_id/key_id
(from cs.upserts key and wallet_id variable) prior to blob::encode, or validate
that entry.identity_id, entry.key_id and entry.wallet_id exactly match the loop
key and return an error if they differ, then proceed to tx.execute.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- Around line 19-37: The loop over cs.addresses currently ignores each entry's
own wallet_id and always writes using the outer wallet_id; add a fast-fail check
at the top of the loop that compares entry.wallet_id to the outer wallet_id (the
variables named entry and wallet_id in the cs.addresses loop) and return an
error if they differ (include a descriptive message mentioning the mismatched
wallet ids and the PlatformAddressBalanceEntry), before executing the INSERT;
this prevents silently writing mixed-wallet entries.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs`:
- Around line 45-51: The row mapper in the wallet_metadata reader must reject
malformed rows instead of coercing them: in the stmt.query_map closure that
reads wallet_id (currently into bytes and wid) and birth_height (converted with
as u32), validate that wallet_id.bytes.len() == 32 and that birth_height is
within u32 bounds before converting; if either check fails return a real error
(e.g., a StorageError::CorruptedRow or map a descriptive
rusqlite::Error::InvalidParameterName/Other) from the closure so the query fails
instead of producing a zeroed wallet id or truncated height. Locate the closure
that does row.get(0) / copy_from_slice and the code that casts birth_height to
u32 and replace the coercion with explicit checks and an early Err(...) return
with a typed, descriptive error.

In `@packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs`:
- Around line 87-106: The test's assertion that delete_wallet returns
AutoBackupDirUnwritable is flaky when run as root because chmod 0o500 doesn't
block root; modify the test in sqlite_auto_backup.rs (the block that creates
unwritable dir, calls SqlitePersisterConfig::with_auto_backup_dir,
SqlitePersister::open, and persister.delete_wallet) to detect running as root
(e.g., check geteuid()==0 on Unix) and in that case either skip the
exact-variant assertion or assert only that an error occurred, otherwise keep
the existing matches!(... AutoBackupDirUnwritable { .. }) check; this ensures
deterministic behavior without changing SqlitePersister/delete_wallet logic.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- Around line 185-191: The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.
- Around line 281-326: The closure parent_check currently accepts an unused
parameter _cols: &[&str]; either remove that parameter from parent_check and all
its callers (keep it as parent_check(child: &str) and continue using wallet_id
inside the generated trigger SQL), or update parent_check to accept cols:
&[&str] and use cols[0] (or join cols) in place of the hard-coded wallet_id
everywhere (including the WHEN clauses and DELETE WHERE clause) so the same
helper can generate triggers for identities, identity_keys, dashpay_profiles,
etc.; ensure you update the for loop callers accordingly to pass either no cols
(if removed) or the appropriate slice like &["wallet_id"] or &["identity_id"]
where needed.

In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 188-231: The migrate handling is split across two places; fold the
two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.
- Around line 122-134: In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.

In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs`:
- Around line 111-118: The function default_auto_backup_dir contains an overly
defensive check when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Around line 102-107: The IntegerOverflow error currently reports
SafeCastTarget::U64 for an i64→u32 conversion in asset_locks.rs (see the
u32::try_from usage producing WalletStorageError::IntegerOverflow), which is
misleading; add a U32 variant to crate::sqlite::util::safe_cast::SafeCastTarget
and implement a dedicated i64_to_u32 helper in the safe_cast module that
performs the conversion and returns WalletStorageError::IntegerOverflow with
target = SafeCastTarget::U32 on failure, then replace the manual u32::try_from
usages (e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 72-102: ensure_exists currently performs an immediate INSERT OR
IGNORE on a plain &Connection, which bypasses the in-memory merge buffer and
per-flush transaction used by apply and can cause durability/ordering
mismatches; update ensure_exists to participate in the same flush boundary by
either accepting and using the existing merge buffer/flush transaction or a
transactional handle (e.g., a &Transaction or buffer API used by apply) and
executing the INSERT within that transaction, or if intentionally intended only
for tests, add a clear doc comment on ensure_exists describing that it writes
immediately to the DB and must not be used by production code that relies on the
merge-buffer + per-flush transaction model (reference ensure_exists, apply, and
the in-memory merge buffer/flush transaction behavior in your comment).

In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs`:
- Around line 20-26: The SafeCastTarget enum currently only lists I64 and U64
but callers need a U32 target; add a new variant SafeCastTarget::U32 and
implement a small helper function i64_to_u32 that mirrors existing
i64_to_u64/i64_to_i64 behavior (validate range, return Result<u32,
SafeCastError> and report SafeCastTarget::U32 on failure). Update any match/log
paths in this module that construct errors to use the new U32 variant so callers
like asset_locks::list_active and core_state::list_unspent_utxos can call the
helper and produce the correct error shape/message.

In `@packages/rs-platform-wallet-storage/tests/common/mod.rs`:
- Around line 47-56: The helper ensure_wallet_meta currently hardcodes network =
'testnet', which can collide with tests expecting other networks; change the
signature of ensure_wallet_meta(persister: &SqlitePersister, wallet_id:
&WalletId) to accept a network parameter (e.g., network: &str) with callers
passing "testnet" where appropriate, update the INSERT SQL call in
ensure_wallet_meta to bind the network parameter instead of the literal
'testnet', and update all test call sites (or add a convenience wrapper) to pass
the correct network values (e.g., "mainnet" or "testnet") so tests see
consistent wallet_metadata rows.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs`:
- Around line 273-278: Remove the redundant dead-code shim `_unused_btreemap`
and its explanatory comment: delete the `#[allow(dead_code)] fn
_unused_btreemap() -> BTreeMap<u32, u32> { BTreeMap::new() }` block since
`BTreeMap` is now actually used in the test
`tc023_one_flush_is_one_transaction`, making the shim unnecessary; ensure
imports remain and run tests to confirm nothing else depends on that helper.
- Around line 154-180: Test currently calls fresh_persister() inside the
proptest closure causing a new on-disk SQLite DB per case; move persister
creation out of the proptest! Create the persister once before proptest (call
fresh_persister() once to get persister/_tmp/_path), then inside the proptest
closure use a new wallet id (wid) per case or clear/reset the core_sync_state
row (use ensure_wallet_meta and persister.store with core_with_height) so each
case reuses the same DB connection and only varies the wallet or row content;
update references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.

🪄 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: 78dea89d-cb94-44d0-8590-02ed68920ad8

📥 Commits

Reviewing files that changed from the base of the PR and between 3b9fe6b and d7a88a9.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (55)

• .github/workflows/pr.yml
• .github/workflows/tests-rs-workspace.yml
• Cargo.toml
• Dockerfile
• packages/rs-platform-wallet-storage/Cargo.toml
• packages/rs-platform-wallet-storage/README.md
• packages/rs-platform-wallet-storage/SECRETS.md
• packages/rs-platform-wallet-storage/build.rs
• packages/rs-platform-wallet-storage/migrations/V001__initial.rs
• packages/rs-platform-wallet-storage/migrations/V002__defensive_update_triggers.rs
• packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs
• packages/rs-platform-wallet-storage/src/lib.rs
• packages/rs-platform-wallet-storage/src/sqlite/backup.rs
• packages/rs-platform-wallet-storage/src/sqlite/buffer.rs
• packages/rs-platform-wallet-storage/src/sqlite/config.rs
• packages/rs-platform-wallet-storage/src/sqlite/error.rs
• packages/rs-platform-wallet-storage/src/sqlite/migrations.rs
• packages/rs-platform-wallet-storage/src/sqlite/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/persister.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs
• packages/rs-platform-wallet-storage/tests/common/mod.rs
• packages/rs-platform-wallet-storage/tests/secrets_scan.rs
• packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
• packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs
• packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs
• packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs
• packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
• packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs
• packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
• packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs
• packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs
• packages/rs-platform-wallet/Cargo.toml
• packages/rs-platform-wallet/src/changeset/changeset.rs
• packages/rs-platform-wallet/src/changeset/mod.rs
• packages/rs-platform-wallet/src/changeset/serde_adapters.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
• packages/rs-platform-wallet/src/wallet/identity/types/block_time.rs
• packages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rs
• packages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rs
• packages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rs
• packages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rs
• packages/rs-platform-wallet/src/wallet/identity/types/key_storage.rs

[Claudius-Maginificent]

SEC-011 fix landed (f6e90d1fca)

Out-of-band security audit (by an automated agent surveying the sqlite persister's secret-handling posture) flagged that the restore path applies chmod 0o600 to the staged SQLite file but the initial DB creation and backup creation paths don't — files land at the process umask, which on shared systems could be world-readable.

The audit classified this as MEDIUM under the umbrella "SEC-011: missing explicit 0o600 on initial DB + backup creation".

Fix shape: extracted a small apply_secure_permissions(&Path) -> Result<(), WalletStorageError> helper in a new packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs (Unix-gated via #[cfg(unix)], mode 0o600, propagates I/O errors through the existing WalletStorageError: #[from] std::io::Error). Called at all three sites:

• persister.rs:108 after Connection::open(&config.path) — initial DB
• backup.rs:48 after Connection::open(dest) in run_to — backup
• backup.rs restore_from — refactored from the existing inline pattern to call the helper

Net change: 4 files, +35/-7 lines. cargo fmt, cargo check --tests, cargo clippy --tests -D warnings, and cargo test --lib (10/10) all clean.

Companion privacy finding flagged in the same audit pass (SEC-012: accounts.account_xpub_bytes enables tx-graph reconstruction if the SQLite file is exfiltrated) — that's a documentation/policy decision rather than a code fix. Best handled in a follow-up SECRETS.md update noting the privacy trade-off explicitly.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

Code Review

Prior finding reconciliation for 35e4a2f: carried forward STILL VALID prior findings: #1:ffi-wallet-restore-xpub-trailing-bytes, #2:ffi-asset-lock-proof-trailing-bytes, #3:kv-put-allows-unreadable-oversized-values, #4:kv-size-cap-two-read-tx-toctou, #5:sqlite-load-empty-wallets-partial-restore-untyped; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest delta 08b0ed9..35e4a2f reviewed separately; new latest-delta findings are listed below if any.

Cumulative incremental review at 35e4a2f. Latest delta (08b0ed9..35e4a2f) replaces meta_* FKs with AFTER DELETE triggers and drops the inert recursive_triggers PRAGMA. Prior-finding reconciliation: the five FK-rejected-parentless-writes findings (Wallet/Identity/Token/Contact/PlatformAddress) are FIXED by dropping the FK and adding the parentless_put_succeeds_* tests; the five carry-forward findings (kv put has no value cap, kv get two-read TOCTOU, FFI xpub trailing-bytes, FFI asset-lock proof trailing-bytes, SqlitePersister::load partial-restore untyped) are all STILL VALID at the new head — none of them were touched by the latest delta. New latest-delta findings: the soft-cascade redesign is not yet end-to-end correct for the parentless case it advertises — delete_wallet's pre-flush existence check rejects wallets whose only persisted state is pre-sync metadata, and even when wallet_metadata exists the cascade leaves parentless meta_contact / meta_platform_address rows behind while the per-table count over-reports removals. Recommending REQUEST_CHANGES on those two blocking gaps. No findings were dropped due to comment budget.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 35e4a2f --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 754, in
result = post_review(repo, pr), so I posted the same verified findings as a top-level review body._

Reviewed commit: 35e4a2f

🔴 2 blocking | 🟡 3 suggestion(s) | 💬 2 nitpick(s)

Verified findings

blocking: New latest-delta finding: wallet deletion leaks parentless meta_contact / meta_platform_address rows, and the per-table delete report over-counts

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 322)

The PR description and the meta_* doc block in V001 both promise that 'deleting a wallet transitively cleans all of its metadata', and the PR's stated purpose is to 'allow metadata writes before parent exists' (the kv.rs module doc explicitly advertises this for non-global scopes, and tests/sqlite_object_metadata.rs now pins parentless_put_succeeds_contact and parentless_put_succeeds_platform_address). The soft-cascade does not deliver that for the parentless slice in two of the five scopes:

• cascade_meta_wallet_on_wallet_delete fires on wallet_metadata directly, so parentless meta_wallet rows for wallet_id = W are cleaned when wallet_metadata is deleted. Good.
• cascade_meta_contact_on_contact_delete (V001:348-356) fires only on contacts_established per-row delete. A row written via KvStore::put(ObjectId::Contact { wallet_id: W, owner_id: O, contact_id: C }, ...) before the typed contacts_established row exists has no parent row to cascade from. When the wallet is later deleted, the FK on contacts_established cascades zero rows for that tuple, so the trigger never fires and the orphan persists.
• cascade_meta_platform_address_on_address_delete (V001:358-364) has the same shape on platform_addresses and the same gap.

Secondary effect on the DeleteWalletReport contract: delete_wallet runs count_rows_for_wallet_sql("meta_contact", DirectColumn) / ("meta_platform_address", DirectColumn) at persister.rs:480-488 before the cascade-bearing wallet_meta::delete at persister.rs:490. Those counts include the parentless rows (they match WHERE wallet_id = ?), but the cascade then can't reach them, so rows_removed_per_table over-reports actual deletions for those two tables.

Fix: add direct AFTER DELETE ON wallet_metadata triggers that wipe meta_contact and meta_platform_address by wallet_id (mirroring cascade_meta_wallet_on_wallet_delete). That makes the wallet-root cleanup catch the orphan slice whether or not the typed parent ever materialized, and reconciles the count with the actual delete shape. meta_identity / meta_token are not vulnerable to the wallet-root variant because their parents (identities, token_balances) carry FK→wallet and are themselves cascade-deleted, so any meta_identity / meta_token row whose parent existed will be cleaned; the residual parentless case there is identity-scope, not wallet-scope, and could be addressed separately if/when an identity-delete API ships.

blocking: New latest-delta finding: `delete_wallet` rejects wallets whose only persisted state is pre-sync metadata

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 386)

KvStore::put now explicitly admits parentless writes for every non-global scope (src/sqlite/kv.rs:7-10, plus the new parentless_put_succeeds_* tests). delete_wallet's pre-flush existence check still decides whether a wallet exists solely by SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1 plus the in-memory buffer, so a host that has written ObjectId::Wallet(W), ObjectId::Contact { wallet_id: W, .. }, or ObjectId::PlatformAddress { wallet_id: W, .. } metadata for W — exactly the use case the soft-cascade was introduced to enable — but has not yet seen a sync write wallet_metadata for W cannot delete that data: the call returns WalletStorageError::WalletNotFound { wallet_id: W } and the meta_* rows persist.

This interacts with the parentless-cleanup gap in V001 above: even after that is fixed, a wallet that was only populated via the kv API would still be rejected here.

Fix options: (a) extend the existence check to also probe meta_wallet, meta_contact, and meta_platform_address by wallet_id so any wallet-scoped row counts as 'exists for delete'; or (b) accept the call when any per-wallet row is present anywhere in PER_WALLET_TABLES (the loop a few lines down already enumerates them). Either way the delete path then needs to cover the parentless meta scopes — which the V001 fix above already supplies.

suggestion: [Carry-forward] `KvStore::put` accepts values that `KvStore::get` will permanently refuse to read back

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 183)

STILL VALID at 35e4a2f — the latest delta did not touch the value-cap surface. get enforces a 16 MiB ceiling (MAX_VALUE_LEN) via the length-probe at kv.rs:158-172 and returns KvError::ValueTooLarge above it. put (kv.rs:183-212) has no analogous check, and the meta_* schemas only CHECK(length(key) BETWEEN 1 AND 128) — there is no CHECK on length(value) (V001__initial.rs:272-320, unchanged by this PR). A buggy FFI host or a corrupted caller can therefore land a row that every subsequent get() will then permanently reject. Enforce the same MAX_VALUE_LEN gate on put (or encode CHECK (length(value) <= 16777216) into each meta_* schema) and return the same KvError::ValueTooLarge { found, max } so the two endpoints agree on the contract.

nitpick: [Carry-forward] CMT-006 KV size-cap still uses two separate read transactions — cross-snapshot TOCTOU window persists

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 155)

STILL VALID at 35e4a2f. get issues SELECT length(value) (kv.rs:158-164) and SELECT value (kv.rs:173-179) as two independent query_row calls. In WAL mode each runs against its own snapshot. A cross-process peer that commits an oversize row between the two reads — exactly the threat model CMT-006 documents — passes the length check at the original size and then materialises the larger BLOB at row.get::<_, Vec<u8>>(0). Cost to defend: one statement. Either fetch (length(value), value) in a single row and branch on length before row.get(1), or wrap both reads in a single conn.transaction_with_behavior(Deferred). Paired with the put size-cap above this closes the realistic damage path.

suggestion: [Carry-forward] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

STILL VALID at 35e4a2f (persistence.rs:2268, untouched by the latest delta). build_wallet_start_state decodes host-supplied account_xpub_bytes via bincode::decode_from_slice(...) and binds the consumed-byte count to _. A buffer of valid_ExtendedPubKey_prefix || junk is silently accepted and fed to Account::from_xpub. The storage crate's sqlite/schema/blob::decode already enforces consumed == input.len(); the same payload type is therefore strictly validated when it crosses the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a (WalletId, account_type, network) tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed PersistenceError::backend at restore time.

        let xpub_bytes =
            unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
        let (account_xpub, consumed): (ExtendedPubKey, usize) =
            bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
                PersistenceError::backend(format!("failed to decode account xpub: {}", e))
            })?;
        if consumed != xpub_bytes.len() {
            return Err(PersistenceError::backend(format!(
                "failed to decode account xpub: trailing {} bytes",
                xpub_bytes.len() - consumed
            )));
        }

suggestion: [Carry-forward] FFI asset-lock proof decoder silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

STILL VALID at 35e4a2f (persistence.rs:2959, untouched by the latest delta). The tracked-asset-lock restore path decodes host-supplied proof_bytes via dpp::bincode::decode_from_slice::<AssetLockProof, _>(...) and discards the consumed count (let (proof, _) =). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a valid_proof || junk payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same consumed == input.len() rejection used by the storage crate's blob decoder so non-canonical encodings fail at FFI re-hydration.

            // SAFETY: Same lifetime contract as `transaction_bytes`.
            let proof_bytes =
                unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
            let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
                proof_bytes,
                config::standard(),
            )
            .map_err(|e| {
                PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: {}",
                    e
                ))
            })?;
            if consumed != proof_bytes.len() {
                return Err(PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: trailing {} bytes",
                    proof_bytes.len() - consumed
                )));
            }
            Some(proof)
        };

nitpick: [Carry-forward] `SqlitePersister::load` returns Ok with empty wallets — partial-restore outcome is not typed

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 946)

STILL VALID at 35e4a2f. load returns Ok(ClientStartState) while leaving wallets empty / LOAD_UNIMPLEMENTED. The TODO(CMT-011) comment references PR #3692 and the tracing::info! summary at persister.rs:971-978 gives operators dashboard visibility, but callers cannot programmatically distinguish an intentionally empty DB from a partial restore that skipped wallet rehydration. Flagged at nitpick severity so this unresolved interface gap doesn't fall off the carry-forward list once #3692 lands; documented as intentionally deferred to the rehydration follow-up PR.

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

- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:322-364: New latest-delta finding: wallet deletion leaks parentless meta_contact / meta_platform_address rows, and the per-table delete report over-counts
  The PR description and the `meta_*` doc block in V001 both promise that 'deleting a wallet transitively cleans all of its metadata', and the PR's stated purpose is to 'allow metadata writes before parent exists' (the kv.rs module doc explicitly advertises this for non-global scopes, and `tests/sqlite_object_metadata.rs` now pins `parentless_put_succeeds_contact` and `parentless_put_succeeds_platform_address`). The soft-cascade does not deliver that for the parentless slice in two of the five scopes:

- `cascade_meta_wallet_on_wallet_delete` fires on `wallet_metadata` directly, so parentless `meta_wallet` rows for `wallet_id = W` are cleaned when `wallet_metadata` is deleted. Good.
- `cascade_meta_contact_on_contact_delete` (V001:348-356) fires only on `contacts_established` per-row delete. A row written via `KvStore::put(ObjectId::Contact { wallet_id: W, owner_id: O, contact_id: C }, ...)` before the typed `contacts_established` row exists has no parent row to cascade from. When the wallet is later deleted, the FK on `contacts_established` cascades zero rows for that tuple, so the trigger never fires and the orphan persists.
- `cascade_meta_platform_address_on_address_delete` (V001:358-364) has the same shape on `platform_addresses` and the same gap.

Secondary effect on the `DeleteWalletReport` contract: `delete_wallet` runs `count_rows_for_wallet_sql("meta_contact", DirectColumn)` / `("meta_platform_address", DirectColumn)` at `persister.rs:480-488` *before* the cascade-bearing `wallet_meta::delete` at `persister.rs:490`. Those counts include the parentless rows (they match `WHERE wallet_id = ?`), but the cascade then can't reach them, so `rows_removed_per_table` over-reports actual deletions for those two tables.

Fix: add direct `AFTER DELETE ON wallet_metadata` triggers that wipe `meta_contact` and `meta_platform_address` by `wallet_id` (mirroring `cascade_meta_wallet_on_wallet_delete`). That makes the wallet-root cleanup catch the orphan slice whether or not the typed parent ever materialized, and reconciles the count with the actual delete shape. `meta_identity` / `meta_token` are not vulnerable to the wallet-root variant because their parents (`identities`, `token_balances`) carry FK→wallet and are themselves cascade-deleted, so any `meta_identity` / `meta_token` row whose parent existed will be cleaned; the residual parentless case there is identity-scope, not wallet-scope, and could be addressed separately if/when an identity-delete API ships.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:386-399: New latest-delta finding: `delete_wallet` rejects wallets whose only persisted state is pre-sync metadata
  `KvStore::put` now explicitly admits parentless writes for every non-global scope (`src/sqlite/kv.rs:7-10`, plus the new `parentless_put_succeeds_*` tests). `delete_wallet`'s pre-flush existence check still decides whether a wallet exists solely by `SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1` plus the in-memory buffer, so a host that has written `ObjectId::Wallet(W)`, `ObjectId::Contact { wallet_id: W, .. }`, or `ObjectId::PlatformAddress { wallet_id: W, .. }` metadata for `W` — exactly the use case the soft-cascade was introduced to enable — but has not yet seen a sync write `wallet_metadata` for `W` cannot delete that data: the call returns `WalletStorageError::WalletNotFound { wallet_id: W }` and the meta_* rows persist.

This interacts with the parentless-cleanup gap in V001 above: even after that is fixed, a wallet that was *only* populated via the kv API would still be rejected here.

Fix options: (a) extend the existence check to also probe `meta_wallet`, `meta_contact`, and `meta_platform_address` by `wallet_id` so any wallet-scoped row counts as 'exists for delete'; or (b) accept the call when any per-wallet row is present anywhere in `PER_WALLET_TABLES` (the loop a few lines down already enumerates them). Either way the delete path then needs to cover the parentless meta scopes — which the V001 fix above already supplies.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:183-212: [Carry-forward] `KvStore::put` accepts values that `KvStore::get` will permanently refuse to read back
  STILL VALID at 35e4a2f6 — the latest delta did not touch the value-cap surface. `get` enforces a 16 MiB ceiling (`MAX_VALUE_LEN`) via the length-probe at `kv.rs:158-172` and returns `KvError::ValueTooLarge` above it. `put` (`kv.rs:183-212`) has no analogous check, and the `meta_*` schemas only `CHECK(length(key) BETWEEN 1 AND 128)` — there is no `CHECK` on `length(value)` (`V001__initial.rs:272-320`, unchanged by this PR). A buggy FFI host or a corrupted caller can therefore land a row that every subsequent `get()` will then permanently reject. Enforce the same `MAX_VALUE_LEN` gate on `put` (or encode `CHECK (length(value) <= 16777216)` into each `meta_*` schema) and return the same `KvError::ValueTooLarge { found, max }` so the two endpoints agree on the contract.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:155-180: [Carry-forward] CMT-006 KV size-cap still uses two separate read transactions — cross-snapshot TOCTOU window persists
  STILL VALID at 35e4a2f6. `get` issues `SELECT length(value)` (`kv.rs:158-164`) and `SELECT value` (`kv.rs:173-179`) as two independent `query_row` calls. In WAL mode each runs against its own snapshot. A cross-process peer that commits an oversize row between the two reads — exactly the threat model CMT-006 documents — passes the length check at the original size and then materialises the larger BLOB at `row.get::<_, Vec<u8>>(0)`. Cost to defend: one statement. Either fetch `(length(value), value)` in a single row and branch on `length` before `row.get(1)`, or wrap both reads in a single `conn.transaction_with_behavior(Deferred)`. Paired with the `put` size-cap above this closes the realistic damage path.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2266-2271: [Carry-forward] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary
  STILL VALID at 35e4a2f6 (`persistence.rs:2268`, untouched by the latest delta). `build_wallet_start_state` decodes host-supplied `account_xpub_bytes` via `bincode::decode_from_slice(...)` and binds the consumed-byte count to `_`. A buffer of `valid_ExtendedPubKey_prefix || junk` is silently accepted and fed to `Account::from_xpub`. The storage crate's `sqlite/schema/blob::decode` already enforces `consumed == input.len()`; the same payload type is therefore strictly validated when it crosses the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a `(WalletId, account_type, network)` tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed `PersistenceError::backend` at restore time.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2956-2970: [Carry-forward] FFI asset-lock proof decoder silently accepts trailing bytes
  STILL VALID at 35e4a2f6 (`persistence.rs:2959`, untouched by the latest delta). The tracked-asset-lock restore path decodes host-supplied `proof_bytes` via `dpp::bincode::decode_from_slice::<AssetLockProof, _>(...)` and discards the consumed count (`let (proof, _) =`). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a `valid_proof || junk` payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same `consumed == input.len()` rejection used by the storage crate's blob decoder so non-canonical encodings fail at FFI re-hydration.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:946-980: [Carry-forward] `SqlitePersister::load` returns Ok with empty wallets — partial-restore outcome is not typed
  STILL VALID at 35e4a2f6. `load` returns `Ok(ClientStartState)` while leaving `wallets` empty / LOAD_UNIMPLEMENTED. The TODO(CMT-011) comment references PR #3692 and the `tracing::info!` summary at `persister.rs:971-978` gives operators dashboard visibility, but callers cannot programmatically distinguish an intentionally empty DB from a partial restore that skipped wallet rehydration. Flagged at nitpick severity so this unresolved interface gap doesn't fall off the carry-forward list once #3692 lands; documented as intentionally deferred to the rehydration follow-up PR.

[thepastaclaw]

Code Review

Prior finding reconciliation for 3f69c31: carried forward STILL VALID prior findings: #1:meta-parentless-delete-leak-overcount, #3:kv-put-allows-unreadable-oversized-values, #4:ffi-wallet-restore-xpub-trailing-bytes, #5:ffi-asset-lock-proof-trailing-bytes, #7:sqlite-load-empty-wallets-partial-restore-untyped; fixed/outdated prior findings: #2:delete-wallet-rejects-presync-metadata-only-wallet(FIXED), #6:kv-size-cap-two-read-tx-toctou(FIXED); intentionally deferred prior findings: none. Latest delta 35e4a2f..3f69c31 reviewed separately; new latest-delta findings are listed below if any.

Cumulative incremental review of PR #3625 at head 3f69c31. Latest delta (35e4a2f..3f69c31) is a targeted hardening pass — CMT-001 (single-snapshot KV get), CMT-002 (cross-process rollback docs), CMT-003 (bincode outpoint encoding). Prior-finding reconciliation: 1 FIXED (KV get TOCTOU), 1 INTENTIONALLY DEFERRED with explicit TODO+PR pointer (load() rehydration → PR #3692), 5 STILL VALID — two blocking delete-path bugs (V001 cascade leaks parentless meta_contact/meta_platform_address; delete_wallet rejects metadata-only wallets), one KV put/get contract mismatch on MAX_VALUE_LEN, and two FFI restore decoders (xpub, asset-lock proof) that still discard the bincode consumed-byte count while the storage crate now strictly rejects trailing bytes. No new in-scope defects observed in the latest delta itself. All findings verified by reading the referenced lines at head.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 3f69c31 --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 754, in
result = post_review(repo, pr), so I posted the same verified findings as a top-level review body._

Reviewed commit: 3f69c31

🔴 1 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)

Verified findings

suggestion: [Carry-forward, STILL VALID] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

Verified unchanged at head 3f69c31 (persistence.rs:2266-2271). build_wallet_start_state decodes host-supplied account_xpub_bytes via bincode::decode_from_slice(...) and binds the consumed-byte count to _. A valid_ExtendedPubKey_prefix || junk buffer is silently accepted and fed to Account::from_xpub. The storage crate's sqlite/schema/blob::decode already enforces consumed == input.len() — strictly validated at the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a (WalletId, account_type, network) tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed PersistenceError::backend at restore time.

        let xpub_bytes =
            unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
        let (account_xpub, consumed): (ExtendedPubKey, usize) =
            bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
                PersistenceError::backend(format!("failed to decode account xpub: {}", e))
            })?;
        if consumed != xpub_bytes.len() {
            return Err(PersistenceError::backend(format!(
                "failed to decode account xpub: trailing {} bytes",
                xpub_bytes.len() - consumed
            )));
        }

suggestion: [Carry-forward, STILL VALID] FFI asset-lock proof decoder silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

Verified unchanged at head 3f69c31 (persistence.rs:2956-2970). The tracked-asset-lock restore path decodes host-supplied proof_bytes via dpp::bincode::decode_from_slice::<AssetLockProof, _>(...) and discards the consumed count (let (proof, _) =). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a valid_proof || junk payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same consumed == input.len() rejection used by the storage crate's blob::decode so non-canonical encodings fail at FFI re-hydration.

            // SAFETY: Same lifetime contract as `transaction_bytes`.
            let proof_bytes =
                unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
            let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
                proof_bytes,
                config::standard(),
            )
            .map_err(|e| {
                PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: {}",
                    e
                ))
            })?;
            if consumed != proof_bytes.len() {
                return Err(PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: trailing {} bytes",
                    proof_bytes.len() - consumed
                )));
            }
            Some(proof)
        };

blocking: [Carry-forward] wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-counts them

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 322)

Carried-forward prior finding (STILL VALID at 3f69c31). meta_contact/meta_platform_address still permit parentless wallet_id rows; the wallet delete trigger only deletes meta_wallet, while PER_WALLET_TABLES counts meta_contact/meta_platform_address rows before deleting wallet_metadata. At 35e4a2f, the meta_* docs promised that deleting a wallet transitively cleans all metadata while also allowing parentless metadata writes before the typed parent exists. meta_contact and meta_platform_address had only parent-object delete triggers, so rows written before contacts_established/platform_addresses existed were counted as wallet-owned but survived wallet deletion.

suggestion: [Carry-forward] KvStore::put accepts values that KvStore::get will permanently refuse to read back

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 183)

Carried-forward prior finding (STILL VALID at 3f69c31). KvStore::get still rejects oversized values, but KvStore::put has no value.len()/MAX_VALUE_LEN guard before INSERT/UPDATE. At 35e4a2f, get enforced MAX_VALUE_LEN and returned ValueTooLarge above it, while put had no analogous check. A caller could successfully persist an oversized metadata value that every future get rejected, creating a write-only key until out-of-band repair.

nitpick: [Carry-forward] SqlitePersister::load returns Ok with empty wallets — partial-restore outcome is not typed

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 946)

Carried-forward prior finding (STILL VALID at 3f69c31). SqlitePersister::load still returns Ok(ClientStartState) while leaving wallets empty / LOAD_UNIMPLEMENTED. At 35e4a2f, SqlitePersister::load still returned Ok(ClientStartState) while leaving wallets empty / LOAD_UNIMPLEMENTED. The TODO(CMT-011) and structured log made the limitation visible, but callers could not programmatically distinguish an intentionally empty DB from a partial restore that skipped wallet rehydration.

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

- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2266-2271: [Carry-forward, STILL VALID] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary
  Verified unchanged at head 3f69c314 (persistence.rs:2266-2271). `build_wallet_start_state` decodes host-supplied `account_xpub_bytes` via `bincode::decode_from_slice(...)` and binds the consumed-byte count to `_`. A `valid_ExtendedPubKey_prefix || junk` buffer is silently accepted and fed to `Account::from_xpub`. The storage crate's `sqlite/schema/blob::decode` already enforces `consumed == input.len()` — strictly validated at the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a `(WalletId, account_type, network)` tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed `PersistenceError::backend` at restore time.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2956-2970: [Carry-forward, STILL VALID] FFI asset-lock proof decoder silently accepts trailing bytes
  Verified unchanged at head 3f69c314 (persistence.rs:2956-2970). The tracked-asset-lock restore path decodes host-supplied `proof_bytes` via `dpp::bincode::decode_from_slice::<AssetLockProof, _>(...)` and discards the consumed count (`let (proof, _) =`). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a `valid_proof || junk` payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same `consumed == input.len()` rejection used by the storage crate's `blob::decode` so non-canonical encodings fail at FFI re-hydration.
- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:322-364: [Carry-forward] wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-counts them
  Carried-forward prior finding (STILL VALID at 3f69c314). meta_contact/meta_platform_address still permit parentless wallet_id rows; the wallet delete trigger only deletes meta_wallet, while PER_WALLET_TABLES counts meta_contact/meta_platform_address rows before deleting wallet_metadata. At 35e4a2f6, the meta_* docs promised that deleting a wallet transitively cleans all metadata while also allowing parentless metadata writes before the typed parent exists. meta_contact and meta_platform_address had only parent-object delete triggers, so rows written before contacts_established/platform_addresses existed were counted as wallet-owned but survived wallet deletion.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:183-212: [Carry-forward] KvStore::put accepts values that KvStore::get will permanently refuse to read back
  Carried-forward prior finding (STILL VALID at 3f69c314). KvStore::get still rejects oversized values, but KvStore::put has no value.len()/MAX_VALUE_LEN guard before INSERT/UPDATE. At 35e4a2f6, get enforced MAX_VALUE_LEN and returned ValueTooLarge above it, while put had no analogous check. A caller could successfully persist an oversized metadata value that every future get rejected, creating a write-only key until out-of-band repair.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:946-980: [Carry-forward] SqlitePersister::load returns Ok with empty wallets — partial-restore outcome is not typed
  Carried-forward prior finding (STILL VALID at 3f69c314). SqlitePersister::load still returns Ok(ClientStartState) while leaving wallets empty / LOAD_UNIMPLEMENTED. At 35e4a2f6, SqlitePersister::load still returned Ok(ClientStartState) while leaving wallets empty / LOAD_UNIMPLEMENTED. The TODO(CMT-011) and structured log made the limitation visible, but callers could not programmatically distinguish an intentionally empty DB from a partial restore that skipped wallet rehydration.

[thepastaclaw]

Correction to my review at 3f69c314: the reconciliation line in the review body misclassified prior finding #2 as fixed. I rechecked the current head; #2 is STILL VALID.

Correct prior-finding reconciliation for 35e4a2f6..3f69c314:

• STILL VALID / carried forward:
  1. [blocking] wallet deletion still leaks parentless meta_contact / meta_platform_address rows and over-counts them.
  2. [blocking] delete_wallet still rejects wallets whose only persisted state is pre-sync metadata. The later per-table count loop does not help because the wallet_metadata-only existence gate still runs first.
  3. [suggestion] KvStore::put still accepts values that KvStore::get will permanently reject as oversized.
  4. [suggestion] FFI xpub decoder still accepts trailing bytes.
  5. [suggestion] FFI asset-lock proof decoder still accepts trailing bytes.
• FIXED: build(sdk): upgrade to webpack 5 #6 KV size-cap two-read TOCTOU — get now reads length(value) and value in one query snapshot.
• INTENTIONALLY DEFERRED: feat(wallet-lib): do not sync transactions if mnemonic is absent #7 SqlitePersister::load still returns Ok with empty wallets, but current head explicitly tracks it with TODO(CMT-011) / PR feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 and a structured load summary.

No new latest-delta findings beyond the carried-forward items above. The requested review action remains REQUEST_CHANGES because both blocking metadata-delete findings are still valid at this exact SHA.

[thepastaclaw]

Code Review

Prior finding reconciliation for ffdc28b: carried forward STILL VALID prior findings: #1:meta-parentless-delete-leak-overcount, #2:ffi-wallet-restore-xpub-trailing-bytes, #3:ffi-asset-lock-proof-trailing-bytes, #4:kv-put-allows-unreadable-oversized-values, #5:sqlite-load-empty-wallets-partial-restore-untyped; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest delta 3f69c31..ffdc28b reviewed separately; new latest-delta findings are listed below if any.

Cumulative incremental review at head ffdc28b. The latest delta (3f69c31..ffdc28b) is a v3.1-dev merge (#3732 shielded snapshot work) plus a trivial cfg-gate of PlatformWalletManager::event_manager behind shielded to silence dead-code-on-shielded-off. The platform-wallet-storage crate and FFI persistence layer were untouched. Prior-finding reconciliation: all 5 prior findings re-verified at head and remain STILL VALID; codex additionally re-surfaces a related blocking issue on delete_wallet's pre-flight existence check (parentless-metadata wallets can't be deleted). No new in-scope defects introduced by the latest delta itself.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 ffdc28b --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 754, in
result = post_review(repo, pr), so I posted the same verified findings as a top-level review body._

Reviewed commit: ffdc28b

🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)

Verified findings

blocking: [Carry-forward, STILL VALID] Wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-counts removals

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 322)

Re-verified at ffdc28b (V001__initial.rs:303-364, sqlite/persister.rs:489-507). meta_contact and meta_platform_address carry a wallet_id column but no FK to wallet_metadata — by design, to allow parentless metadata writes before the typed parent is synced. The only cleanup paths for these tables are AFTER DELETE ON contacts_established (lines 348-356) and AFTER DELETE ON platform_addresses (lines 358-364); there is no AFTER DELETE ON wallet_metadata trigger that scoops these tables by wallet_id. When delete_wallet runs, the FK cascade deletes zero parent rows for parentless metadata tuples, the triggers never fire, and the orphaned rows survive wallet deletion. Worse, PER_WALLET_TABLES lists both with WalletScope::DirectColumn, so delete_wallet counts every meta_contact/meta_platform_address row where wallet_id = ? into rows_removed_per_table before invoking wallet_meta::delete — the report claims rows were removed that actually remain on disk. This directly contradicts the PR's stated invariant that 'metadata cannot outlive its object' for exactly the case (write metadata first, then sync) the soft-cascade was introduced to support. Either (a) add explicit AFTER DELETE ON wallet_metadata triggers that purge meta_contact/meta_platform_address by wallet_id, or (b) keep the FK off but exclude these two from PER_WALLET_TABLES (so the count doesn't misreport).

blocking: [Carry-forward, STILL VALID] delete_wallet pre-flight rejects wallets whose only persisted state is parentless metadata

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 405)

Re-verified at ffdc28b (sqlite/persister.rs:402-415). The existence check probes only wallet_metadata (SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1) and the in-memory buffered state. A wallet whose state lives exclusively in meta_wallet, meta_contact, or meta_platform_address — exactly the parentless-metadata case this PR introduces — returns WalletNotFound from delete_wallet, leaving the orphaned rows undeletable through the public API. Combined with the cascade gap above, this means the parentless-metadata feature has no cleanup path at all. Either include the metadata-only tables in the pre-flight existence probe, or require that wallet_metadata exist before any meta_* write is accepted.

suggestion: [Carry-forward, STILL VALID] KvStore::put accepts values that KvStore::get will permanently refuse to read back

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 186)

Re-verified at ffdc28b (sqlite/kv.rs:162-183 vs 186-215). get enforces length(value) <= MAX_VALUE_LEN in a single-snapshot query and returns KvError::ValueTooLarge above it (CMT-001/006). put has no symmetric value.len() > MAX_VALUE_LEN guard before the INSERT…ON CONFLICT — it simply binds the slice and writes. Because the schema only constrains key length, a caller can persist a value larger than 16 MiB and every subsequent get for that key will fail with ValueTooLarge, producing a write-only row until out-of-band repair. Add the size guard at the top of put so the contract is symmetric and the failure surfaces at write time.

suggestion: [Carry-forward, STILL VALID] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

Re-verified unchanged at ffdc28b (persistence.rs:2266-2271). build_wallet_start_state decodes host-supplied account_xpub_bytes via bincode::decode_from_slice(xpub_bytes, config::standard()) and binds the consumed-byte count to _. A valid_ExtendedPubKey_prefix || junk buffer is silently accepted and fed to Account::from_xpub. The storage crate's sqlite/schema/blob::decode already enforces consumed == input.len() — strict at the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host's secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a (WalletId, account_type, network) tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed PersistenceError::backend at restore time.

        let xpub_bytes =
            unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
        let (account_xpub, consumed): (ExtendedPubKey, usize) =
            bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
                PersistenceError::backend(format!("failed to decode account xpub: {}", e))
            })?;
        if consumed != xpub_bytes.len() {
            return Err(PersistenceError::backend(format!(
                "failed to decode account xpub: trailing {} bytes",
                xpub_bytes.len() - consumed
            )));
        }

suggestion: [Carry-forward, STILL VALID] FFI asset-lock proof decoder silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

Re-verified unchanged at ffdc28b (persistence.rs:2956-2970). The tracked-asset-lock restore path decodes host-supplied proof_bytes via dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard()) and discards the consumed count (let (proof, _) =). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a valid_proof || junk payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same consumed == input.len() rejection used by the storage crate's blob::decode so non-canonical encodings fail at FFI re-hydration.

            // SAFETY: Same lifetime contract as `transaction_bytes`.
            let proof_bytes =
                unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
            let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
                proof_bytes,
                config::standard(),
            )
            .map_err(|e| {
                PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: {}",
                    e
                ))
            })?;
            if consumed != proof_bytes.len() {
                return Err(PersistenceError::backend(format!(
                    "tracked asset lock: failed to decode proof: trailing {} bytes",
                    proof_bytes.len() - consumed
                )));
            }
            Some(proof)
        };

nitpick: [Carry-forward, STILL VALID] SqlitePersister::load returns Ok with empty wallets — partial-restore outcome is not typed

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 962)

Re-verified at ffdc28b (sqlite/persister.rs:962-996). load() still returns Ok(ClientStartState) while leaving wallets empty under TODO(CMT-011). The structured log (wallets_seen, wallets_rehydrated=0, wallets_pending_rehydration) makes the gap visible, but callers cannot programmatically distinguish an intentionally empty DB from a partial restore that found per-wallet rows and skipped rehydration. Until #3692 lands and Wallet::from_persisted is wired, consider returning a typed LoadIncomplete { wallets_seen, addresses_loaded } (or surfacing the state through the report) so consumers can branch on it instead of inferring from log lines.

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

- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:322-364: [Carry-forward, STILL VALID] Wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-counts removals
  Re-verified at ffdc28b80a (V001__initial.rs:303-364, sqlite/persister.rs:489-507). `meta_contact` and `meta_platform_address` carry a `wallet_id` column but no FK to `wallet_metadata` — by design, to allow parentless metadata writes before the typed parent is synced. The only cleanup paths for these tables are `AFTER DELETE ON contacts_established` (lines 348-356) and `AFTER DELETE ON platform_addresses` (lines 358-364); there is no `AFTER DELETE ON wallet_metadata` trigger that scoops these tables by `wallet_id`. When `delete_wallet` runs, the FK cascade deletes zero parent rows for parentless metadata tuples, the triggers never fire, and the orphaned rows survive wallet deletion. Worse, `PER_WALLET_TABLES` lists both with `WalletScope::DirectColumn`, so `delete_wallet` counts every `meta_contact`/`meta_platform_address` row where `wallet_id = ?` into `rows_removed_per_table` before invoking `wallet_meta::delete` — the report claims rows were removed that actually remain on disk. This directly contradicts the PR's stated invariant that 'metadata cannot outlive its object' for exactly the case (write metadata first, then sync) the soft-cascade was introduced to support. Either (a) add explicit `AFTER DELETE ON wallet_metadata` triggers that purge `meta_contact`/`meta_platform_address` by `wallet_id`, or (b) keep the FK off but exclude these two from `PER_WALLET_TABLES` (so the count doesn't misreport).
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:405-415: [Carry-forward, STILL VALID] delete_wallet pre-flight rejects wallets whose only persisted state is parentless metadata
  Re-verified at ffdc28b80a (sqlite/persister.rs:402-415). The existence check probes only `wallet_metadata` (`SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1`) and the in-memory buffered state. A wallet whose state lives exclusively in `meta_wallet`, `meta_contact`, or `meta_platform_address` — exactly the parentless-metadata case this PR introduces — returns `WalletNotFound` from `delete_wallet`, leaving the orphaned rows undeletable through the public API. Combined with the cascade gap above, this means the parentless-metadata feature has no cleanup path at all. Either include the metadata-only tables in the pre-flight existence probe, or require that `wallet_metadata` exist before any `meta_*` write is accepted.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:186-215: [Carry-forward, STILL VALID] KvStore::put accepts values that KvStore::get will permanently refuse to read back
  Re-verified at ffdc28b80a (sqlite/kv.rs:162-183 vs 186-215). `get` enforces `length(value) <= MAX_VALUE_LEN` in a single-snapshot query and returns `KvError::ValueTooLarge` above it (CMT-001/006). `put` has no symmetric `value.len() > MAX_VALUE_LEN` guard before the INSERT…ON CONFLICT — it simply binds the slice and writes. Because the schema only constrains `key` length, a caller can persist a value larger than 16 MiB and every subsequent `get` for that key will fail with `ValueTooLarge`, producing a write-only row until out-of-band repair. Add the size guard at the top of `put` so the contract is symmetric and the failure surfaces at write time.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2266-2271: [Carry-forward, STILL VALID] FFI xpub decoder silently accepts trailing bytes at the Swift→Rust restore boundary
  Re-verified unchanged at ffdc28b80a (persistence.rs:2266-2271). `build_wallet_start_state` decodes host-supplied `account_xpub_bytes` via `bincode::decode_from_slice(xpub_bytes, config::standard())` and binds the consumed-byte count to `_`. A `valid_ExtendedPubKey_prefix || junk` buffer is silently accepted and fed to `Account::from_xpub`. The storage crate's `sqlite/schema/blob::decode` already enforces `consumed == input.len()` — strict at the SQLite→Rust seam but silently accepted at the Swift→Rust seam. Corruption, schema drift, or a peer with write access to the host's secret store can plant a non-canonical encoding and restore an ambiguous wallet identity under a `(WalletId, account_type, network)` tuple the operator did not produce. Mirror the storage-crate contract so the failure surfaces as a typed `PersistenceError::backend` at restore time.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2956-2970: [Carry-forward, STILL VALID] FFI asset-lock proof decoder silently accepts trailing bytes
  Re-verified unchanged at ffdc28b80a (persistence.rs:2956-2970). The tracked-asset-lock restore path decodes host-supplied `proof_bytes` via `dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard())` and discards the consumed count (`let (proof, _) =`). Same trust-boundary class as the xpub decoder, with larger blast radius: these records resume in-flight asset-lock workflows after restart, so a `valid_proof || junk` payload is replayed into the asset-lock resumption pipeline as if canonical. Apply the same `consumed == input.len()` rejection used by the storage crate's `blob::decode` so non-canonical encodings fail at FFI re-hydration.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:962-996: [Carry-forward, STILL VALID] SqlitePersister::load returns Ok with empty wallets — partial-restore outcome is not typed
  Re-verified at ffdc28b80a (sqlite/persister.rs:962-996). `load()` still returns `Ok(ClientStartState)` while leaving `wallets` empty under `TODO(CMT-011)`. The structured log (`wallets_seen`, `wallets_rehydrated=0`, `wallets_pending_rehydration`) makes the gap visible, but callers cannot programmatically distinguish an intentionally empty DB from a partial restore that found per-wallet rows and skipped rehydration. Until #3692 lands and `Wallet::from_persisted` is wired, consider returning a typed `LoadIncomplete { wallets_seen, addresses_loaded }` (or surfacing the state through the report) so consumers can branch on it instead of inferring from log lines.

[thepastaclaw]

Code Review

Incremental review against latest head 3fc28b9. The new commits (CMT-001/004/005/006) only touch the secrets/keyring subsystem and don't address the wallet-deletion or restore-boundary findings. Re-validated against current code: findings 1–5 from the prior review at ffdc28b are STILL VALID; finding 6 (load() partial-restore typing) is INTENTIONALLY DEFERRED via TODO(CMT-011), gated on #3692, with structured-log signalling already in place. No new issues in the latest delta itself.

🔴 2 blocking | 🟡 2 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/migrations/V001__initial.rs:303-364: Carried (STILL VALID): wallet deletion leaks parentless meta_contact / meta_platform_address rows and over-reports cleanup
  Verified at current head. `meta_contact` (lines 303-311) and `meta_platform_address` (lines 313-320) carry `wallet_id` but have no FK to `wallet_metadata`, and the only cleanup triggers fire from `contacts_established` (lines 348-356) and `platform_addresses` (lines 358-364). The schema explicitly supports parentless metadata writes via `KvStore::put` on `ObjectId::Contact` / `ObjectId::PlatformAddress`, so rows without a parent in `contacts_established` / `platform_addresses` are never deleted when the wallet is removed — they outlive their wallet.

  Compounding this, `delete_wallet` in `src/sqlite/persister.rs:489-505` builds `rows_removed_per_table` by pre-counting `PER_WALLET_TABLES` (which includes `meta_contact` and `meta_platform_address` with `WalletScope::DirectColumn`) before deleting `wallet_metadata`. Because the cascade-by-trigger flow leaves the parentless rows behind, the reported count is an upper bound — `DeleteWalletReport` over-reports cleanup on the metadata-only-write path. The PR description's claim that `delete_wallet` 'counts them' so 'metadata cannot outlive its object' only holds when every metadata row has a parent.

  Fix shapes: (a) add `AFTER DELETE ON wallet_metadata` triggers that broom `meta_contact WHERE wallet_id = OLD.wallet_id` and `meta_platform_address WHERE wallet_id = OLD.wallet_id`; or (b) issue explicit `DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1` in `delete_wallet` before deleting `wallet_metadata`, and have the report use post-delete `Connection::changes()` instead of a pre-delete count.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:405-415: Carried (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata
  Verified at current head. The existence gate still probes only `wallet_metadata`, with the in-memory buffer escape via `had_buffered`:

  ```rust
  let exists_pre_flush = conn
      .query_row(
          "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
          rusqlite::params![wallet_id.as_slice()],
          |_| Ok(()),
      )
      .optional()?
      .is_some();
  if !had_buffered && !exists_pre_flush {
      return Err(WalletStorageError::WalletNotFound { wallet_id });
  }

A wallet whose only durable rows are the intentionally parentless meta_wallet / meta_contact / meta_platform_address entries (a documented and supported write pattern via KvStore::put) is rejected with WalletNotFound even though state exists on disk. Combined with finding #1, the parentless-metadata path is both undeletable here AND silently retained later (when a wallet_metadata row eventually arrives, the stale metadata persists). Buffer-only callers escape via had_buffered; persisted-metadata-only callers do not.

Widen the existence check to the metadata-bearing tables as well.

In packages/rs-platform-wallet-storage/src/sqlite/kv.rs:

• [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/kv.rs:186-215: Carried (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse
  Verified at current head. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (lines 162-183). put (lines 186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of a 17 MiB blob (MAX_VALUE_LEN is 16 MiB per the PR description) silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

  Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than discovering it later on every subsequent read.

In packages/rs-platform-wallet-ffi/src/persistence.rs:

• [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:2266-2271: Carried (STILL VALID): FFI account-xpub restore silently accepts trailing bytes
  Verified at current head. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the usize consumed-bytes count (line 2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical.

  This is inconsistent with the rest of the persistence boundary — the SQLite crate's blob::decode now rejects trailing bytes — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce consumed == xpub_bytes.len() and surface a typed mismatch error.

</details>

[thepastaclaw]

🔴 Blocking: Carried (STILL VALID): wallet deletion leaks parentless meta_contact / meta_platform_address rows and over-reports cleanup

Verified at current head. meta_contact (lines 303-311) and meta_platform_address (lines 313-320) carry wallet_id but have no FK to wallet_metadata, and the only cleanup triggers fire from contacts_established (lines 348-356) and platform_addresses (lines 358-364). The schema explicitly supports parentless metadata writes via KvStore::put on ObjectId::Contact / ObjectId::PlatformAddress, so rows without a parent in contacts_established / platform_addresses are never deleted when the wallet is removed — they outlive their wallet.

Compounding this, delete_wallet in src/sqlite/persister.rs:489-505 builds rows_removed_per_table by pre-counting PER_WALLET_TABLES (which includes meta_contact and meta_platform_address with WalletScope::DirectColumn) before deleting wallet_metadata. Because the cascade-by-trigger flow leaves the parentless rows behind, the reported count is an upper bound — DeleteWalletReport over-reports cleanup on the metadata-only-write path. The PR description's claim that delete_wallet 'counts them' so 'metadata cannot outlive its object' only holds when every metadata row has a parent.

Fix shapes: (a) add AFTER DELETE ON wallet_metadata triggers that broom meta_contact WHERE wallet_id = OLD.wallet_id and meta_platform_address WHERE wallet_id = OLD.wallet_id; or (b) issue explicit DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1 in delete_wallet before deleting wallet_metadata, and have the report use post-delete Connection::changes() instead of a pre-delete count.

Suggested change

	CREATE TABLE meta_contact (
	wallet_id BLOB NOT NULL,
	owner_id BLOB NOT NULL,
	contact_id BLOB NOT NULL,
	key TEXT NOT NULL CHECK (length(key) BETWEEN 1 AND 128),
	value BLOB NOT NULL,
	updated_at INTEGER NOT NULL DEFAULT (unixepoch()),
	PRIMARY KEY (wallet_id, owner_id, contact_id, key)
	);
	CREATE TABLE meta_platform_address (
	wallet_id BLOB NOT NULL,
	address BLOB NOT NULL,
	key TEXT NOT NULL CHECK (length(key) BETWEEN 1 AND 128),
	value BLOB NOT NULL,
	updated_at INTEGER NOT NULL DEFAULT (unixepoch()),
	PRIMARY KEY (wallet_id, address, key)
	);
	-- Soft-cascade cleanup: drop a scope's metadata when its parent object
	-- is deleted. SQLite fires these for parents removed by an FK cascade
	-- too (e.g. wallet_metadata delete → identities cascade → meta_identity
	-- trigger), so deleting a wallet cleans its metadata transitively.
	CREATE TRIGGER cascade_meta_wallet_on_wallet_delete
	AFTER DELETE ON wallet_metadata
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_wallet WHERE wallet_id = OLD.wallet_id;
	END;
	CREATE TRIGGER cascade_meta_identity_on_identity_delete
	AFTER DELETE ON identities
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_identity WHERE identity_id = OLD.identity_id;
	END;
	CREATE TRIGGER cascade_meta_token_on_token_balance_delete
	AFTER DELETE ON token_balances
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_token
	WHERE identity_id = OLD.identity_id AND token_id = OLD.token_id;
	END;
	CREATE TRIGGER cascade_meta_contact_on_contact_delete
	AFTER DELETE ON contacts_established
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_contact
	WHERE wallet_id = OLD.wallet_id
	AND owner_id = OLD.owner_id
	AND contact_id = OLD.contact_id;
	END;
	CREATE TRIGGER cascade_meta_platform_address_on_address_delete
	AFTER DELETE ON platform_addresses
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_platform_address
	WHERE wallet_id = OLD.wallet_id AND address = OLD.address;
	END;
	CREATE TRIGGER cascade_meta_contact_on_contact_delete
	AFTER DELETE ON contacts_established
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_contact
	WHERE wallet_id = OLD.wallet_id
	AND owner_id = OLD.owner_id
	AND contact_id = OLD.contact_id;
	END;
	CREATE TRIGGER cascade_meta_contact_on_wallet_delete
	AFTER DELETE ON wallet_metadata
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_contact WHERE wallet_id = OLD.wallet_id;
	END;
	CREATE TRIGGER cascade_meta_platform_address_on_address_delete
	AFTER DELETE ON platform_addresses
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_platform_address
	WHERE wallet_id = OLD.wallet_id AND address = OLD.address;
	END;
	CREATE TRIGGER cascade_meta_platform_address_on_wallet_delete
	AFTER DELETE ON wallet_metadata
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_platform_address WHERE wallet_id = OLD.wallet_id;
	END;

_{source: ['claude', 'codex']}

[thepastaclaw]

🔴 Blocking: Carried (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata

Verified at current head. The existence gate still probes only wallet_metadata, with the in-memory buffer escape via had_buffered:

let exists_pre_flush = conn
    .query_row(
        "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
        rusqlite::params![wallet_id.as_slice()],
        |_| Ok(()),
    )
    .optional()?
    .is_some();
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

A wallet whose only durable rows are the intentionally parentless meta_wallet / meta_contact / meta_platform_address entries (a documented and supported write pattern via KvStore::put) is rejected with WalletNotFound even though state exists on disk. Combined with finding #1, the parentless-metadata path is both undeletable here AND silently retained later (when a wallet_metadata row eventually arrives, the stale metadata persists). Buffer-only callers escape via had_buffered; persisted-metadata-only callers do not.

Widen the existence check to the metadata-bearing tables as well.

Suggested change

	let exists_pre_flush = conn
	.query_row(
	"SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
	rusqlite::params![wallet_id.as_slice()],
	|_| Ok(()),
	)
	.optional()?
	.is_some();
	if !had_buffered && !exists_pre_flush {
	return Err(WalletStorageError::WalletNotFound { wallet_id });
	}
	let exists_pre_flush = conn
	.query_row(
	"SELECT EXISTS(\
	SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_wallet WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_contact WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_platform_address WHERE wallet_id = ?1\
	)",
	rusqlite::params![wallet_id.as_slice()],
	|row| row.get::<_, i64>(0),
	)?
	!= 0;
	if !had_buffered && !exists_pre_flush {
	return Err(WalletStorageError::WalletNotFound { wallet_id });
	}

_{source: ['claude', 'codex']}

[thepastaclaw]

🟡 Suggestion: Carried (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse

Verified at current head. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (lines 162-183). put (lines 186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of a 17 MiB blob (MAX_VALUE_LEN is 16 MiB per the PR description) silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than discovering it later on every subsequent read.

Suggested change

	fn put(&self, scope: &ObjectId, key: &str, value: &[u8]) -> Result<(), KvError> {
	validate_key(key)?;
	let sql = ScopeSql::resolve(scope);
	let conn = self.conn().map_err(KvError::from)?;
	// Column list / placeholders / conflict target all include the
	// id columns ahead of `key`; the plain composite PK is the
	// conflict target. Upsert refreshes `updated_at` on overwrite.
	let mut cols: Vec<&str> = sql.id_cols.to_vec();
	cols.push("key");
	let col_list = cols.join(", ");
	let value_phs = (1..=cols.len())
	.map(|i| format!("?{i}"))
	.collect::<Vec<_>>()
	.join(", ");
	let conflict_target = cols.join(", ");
	let value_ph = cols.len() + 1;
	let mut params: Vec<&dyn ToSql> = sql.id_vals.iter().map(|v| v as &dyn ToSql).collect();
	params.push(&key);
	params.push(&value);
	conn.execute(
	&format!(
	"INSERT INTO {} ({col_list}, value) VALUES ({value_phs}, ?{value_ph}) \
	ON CONFLICT({conflict_target}) \
	DO UPDATE SET value = excluded.value, updated_at = unixepoch()",
	sql.table
	),
	params.as_slice(),
	)?;
	Ok(())
	}
	fn put(&self, scope: &ObjectId, key: &str, value: &[u8]) -> Result<(), KvError> {
	validate_key(key)?;
	if value.len() > MAX_VALUE_LEN {
	return Err(KvError::ValueTooLarge {
	found: value.len(),
	max: MAX_VALUE_LEN,
	});
	}
	let sql = ScopeSql::resolve(scope);
	let conn = self.conn().map_err(KvError::from)?;
	let mut cols: Vec<&str> = sql.id_cols.to_vec();
	cols.push("key");
	let col_list = cols.join(", ");
	let value_phs = (1..=cols.len())
	.map(|i| format!("?{i}"))
	.collect::<Vec<_>>()
	.join(", ");
	let conflict_target = cols.join(", ");
	let value_ph = cols.len() + 1;
	let mut params: Vec<&dyn ToSql> = sql.id_vals.iter().map(|v| v as &dyn ToSql).collect();
	params.push(&key);
	params.push(&value);
	conn.execute(
	&format!(
	"INSERT INTO {} ({col_list}, value) VALUES ({value_phs}, ?{value_ph}) \
	ON CONFLICT({conflict_target}) \
	DO UPDATE SET value = excluded.value, updated_at = unixepoch()",
	sql.table
	),
	params.as_slice(),
	)?;
	Ok(())
	}

_{source: ['claude', 'codex']}

[thepastaclaw]

🟡 Suggestion: Carried (STILL VALID): FFI account-xpub restore silently accepts trailing bytes

Verified at current head. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the usize consumed-bytes count (line 2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical.

This is inconsistent with the rest of the persistence boundary — the SQLite crate's blob::decode now rejects trailing bytes — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce consumed == xpub_bytes.len() and surface a typed mismatch error.

Suggested change

	let xpub_bytes =
	unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
	let (account_xpub, _): (ExtendedPubKey, usize) =
	bincode::decode_from_slice(xpub_bytes, config::standard())
	.map_err(|e| format!("failed to decode account xpub: {}", e))?;
	bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
	PersistenceError::backend(format!("failed to decode account xpub: {}", e))
	})?;
	let xpub_bytes =
	unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
	let (account_xpub, consumed): (ExtendedPubKey, usize) =
	bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
	PersistenceError::backend(format!("failed to decode account xpub: {}", e))
	})?;
	if consumed != xpub_bytes.len() {
	return Err(PersistenceError::backend(
	"failed to decode account xpub: trailing bytes present".to_string(),
	));
	}

_{source: ['claude', 'codex']}

[thepastaclaw]

Code Review

Prior finding reconciliation for 932b923: carried forward STILL VALID prior findings: #1:meta-parentless-delete-leak-overcount, #3:kv-put-allows-unreadable-oversized-values, #4:ffi-wallet-restore-xpub-trailing-bytes, #5:ffi-asset-lock-proof-trailing-bytes; fixed/outdated prior findings: #2:delete-wallet-rejects-presync-metadata-only-wallet(FIXED); intentionally deferred prior findings: none. Latest delta 3fc28b9..932b923 reviewed separately; new latest-delta findings are listed below if any.

Incremental review at head 932b923. The latest delta (CMT-003) unifies contacts_sent/recv/established into a single lifecycle-state contacts table; the underlying meta_*/KvStore/FFI concerns from the previous review are unchanged. Re-validated all 5 prior findings against current head: all 5 remain STILL VALID and are carried forward. One additional finding from codex on the new contacts schema is partially valid (data is preserved on disk; the lossy bucketing is in a feature-gated test reader) so it is downgraded to a suggestion and noted as a schema-design concern for the in-flight production loader. Prior status per finding: meta_contact/meta_platform_address leak — STILL VALID; delete_wallet pre-flight rejects metadata-only wallets — STILL VALID; KvStore::put missing size guard — STILL VALID; FFI xpub trailing bytes — STILL VALID; FFI asset-lock proof trailing bytes — STILL VALID; load() partial-restore typing — INTENTIONALLY DEFERRED (CMT-011, unchanged).

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 932b923 --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 754, in
result = post_review(repo, pr), so I posted the same verified findings as a top-level review body._

Reviewed commit: 932b923

🔴 1 blocking | 🟡 4 suggestion(s)

Verified findings

suggestion: Carried (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 186)

Verified at current head — code unchanged. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (lines 162-183). put (lines 186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than discovering it later on every subsequent read.

fn put(&self, scope: &ObjectId, key: &str, value: &[u8]) -> Result<(), KvError> {
    validate_key(key)?;
    if value.len() > MAX_VALUE_LEN {
        return Err(KvError::ValueTooLarge {
            found: value.len(),
            max: MAX_VALUE_LEN,
        });
    }
    let sql = ScopeSql::resolve(scope);
    let conn = self.conn().map_err(KvError::from)?;
    let mut cols: Vec<&str> = sql.id_cols.to_vec();
    cols.push("key");
    let col_list = cols.join(", ");
    let value_phs = (1..=cols.len())
        .map(|i| format!("?{i}"))
        .collect::<Vec<_>>()
        .join(", ");
    let conflict_target = cols.join(", ");
    let value_ph = cols.len() + 1;
    let mut params: Vec<&dyn ToSql> = sql.id_vals.iter().map(|v| v as &dyn ToSql).collect();
    params.push(&key);
    params.push(&value);
    conn.execute(
        &format!(
            "INSERT INTO {} ({col_list}, value) VALUES ({value_phs}, ?{value_ph}) \
             ON CONFLICT({conflict_target}) \
             DO UPDATE SET value = excluded.value, updated_at = unixepoch()",
            sql.table
        ),
        params.as_slice(),
    )?;
    Ok(())
}

suggestion: Carried (STILL VALID): FFI account-xpub restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

Verified at current head — code unchanged. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the consumed-bytes count (line 2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical.

This is inconsistent with the rest of the persistence boundary — the SQLite crate's blob::decode rejects trailing bytes — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce consumed == xpub_bytes.len() and surface a typed mismatch error.

let xpub_bytes =
    unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
let (account_xpub, consumed): (ExtendedPubKey, usize) =
    bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
        PersistenceError::backend(format!("failed to decode account xpub: {}", e))
    })?;
if consumed != xpub_bytes.len() {
    return Err(PersistenceError::backend(
        "failed to decode account xpub: trailing bytes present".to_string(),
    ));
}

suggestion: Carried (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

Verified at current head — code unchanged. The tracked asset-lock restore path (line 2959) calls dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard()) and discards the consumed count. Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid AssetLockProof encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map.

Enforce consumed == proof_bytes.len() to match the rest of the restore-boundary policy.

let proof_bytes =
    unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
    proof_bytes,
    config::standard(),
)
.map_err(|e| {
    PersistenceError::backend(format!(
        "tracked asset lock: failed to decode proof: {}",
        e
    ))
})?;
if consumed != proof_bytes.len() {
    return Err(PersistenceError::backend(
        "tracked asset lock: failed to decode proof: trailing bytes present".to_string(),
    ));
}
Some(proof)

suggestion: New latest-delta finding: unified contacts row carries a mutually-exclusive `state` label even when both pending blobs are present, so reconstruction bucketing can drop one side

packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 108)

The CMT-003 rewrite collapses sent/received/established into one row per (wallet_id, owner_id, contact_id) with a single state label and two nullable request blobs. The pending upserts in apply() (lines 113-172) intentionally leave the other blob untouched on conflict — only the matching blob + state are overwritten in the ON CONFLICT DO UPDATE clause. If a sent upsert lands first and then an incoming upsert for the same key arrives before establishment, the row ends up with state='received', incoming_request=Y, outgoing_request=X (still there from the prior upsert). The on-disk data preserves both sides, but the only bucketing key in load_state() (lines 251-299) is the state label, which is mutually exclusive — so the reader emits only the incoming_requests entry and drops the previously persisted outgoing pending half.

The lossy reader path is currently __test-helpers-gated and the production rehydration loader is still being built under CMT-011, so this is not a live data-loss bug today. However, the schema invariant the production loader is being designed against is exactly what this test reader encodes, and DashPay's mutual contact-request flow (Alice sends to Bob; Bob sends to Alice — Alice's wallet ends up with both sent and incoming for the same pair before either is established) is a documented real scenario in tests/sqlite_load_reconstruction.rs:559-576.

Before the production loader lands, decide whether the schema should (a) carry a richer state (e.g. add a mutual-pending label so the bucketing is unambiguous), (b) have the reader consult blob presence in addition to state, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the post-second-upsert state so this behaviour stops being silently untested.

blocking: [Carry-forward] wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-reports cleanup

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 303)

Carried-forward prior finding (STILL VALID at 932b923). meta_contact/meta_platform_address still allow parentless wallet_id rows; the wallet-delete trigger still does not directly purge both child meta tables, while per-wallet counting can include them before wallet_metadata deletion. At 3fc28b9, meta_contact and meta_platform_address carried wallet_id but had no FK to wallet_metadata, and cleanup triggers fired only from contacts_established/platform_addresses. Parentless metadata rows could survive wallet deletion even though the cleanup report counted them.

CREATE TRIGGER cascade_meta_contact_on_contact_delete AFTER DELETE ON contacts_established FOR EACH ROW BEGIN
  DELETE FROM meta_contact WHERE wallet_id = OLD.wallet_id AND owner_id = OLD.owner_id AND contact_id = OLD.contact_id;
END;
-- Also add wallet-delete coverage for meta_contact/meta_platform_address rows that have no typed parent yet.

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

- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:186-215: Carried (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse
  Verified at current head — code unchanged. `get` enforces `length(value) <= MAX_VALUE_LEN` and returns `KvError::ValueTooLarge { found, max }` (lines 162-183). `put` (lines 186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw `value: &[u8]` regardless of length. A `put` of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

Add a symmetric guard at the top of `put`, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than discovering it later on every subsequent read.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2266-2271: Carried (STILL VALID): FFI account-xpub restore silently accepts trailing bytes
  Verified at current head — code unchanged. `build_wallet_start_state` decodes `account_xpub_bytes` via `bincode::decode_from_slice(...)` and discards the consumed-bytes count (line 2268 binds it to `_`). A Swift→Rust restore payload whose prefix is a valid `ExtendedPubKey` encoding followed by garbage is silently accepted as canonical.

This is inconsistent with the rest of the persistence boundary — the SQLite crate's `blob::decode` rejects trailing bytes — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce `consumed == xpub_bytes.len()` and surface a typed mismatch error.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2956-2970: Carried (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes
  Verified at current head — code unchanged. The tracked asset-lock restore path (line 2959) calls `dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard())` and discards the consumed count. Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid `AssetLockProof` encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map.

Enforce `consumed == proof_bytes.len()` to match the rest of the restore-boundary policy.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:108-223: New latest-delta finding: unified contacts row carries a mutually-exclusive `state` label even when both pending blobs are present, so reconstruction bucketing can drop one side
  The CMT-003 rewrite collapses sent/received/established into one row per `(wallet_id, owner_id, contact_id)` with a single `state` label and two nullable request blobs. The pending upserts in `apply()` (lines 113-172) intentionally leave the *other* blob untouched on conflict — only the matching blob + `state` are overwritten in the `ON CONFLICT DO UPDATE` clause. If a `sent` upsert lands first and then an `incoming` upsert for the same key arrives before establishment, the row ends up with `state='received'`, `incoming_request=Y`, `outgoing_request=X` (still there from the prior upsert). The on-disk data preserves both sides, but the only bucketing key in `load_state()` (lines 251-299) is the `state` label, which is mutually exclusive — so the reader emits only the `incoming_requests` entry and drops the previously persisted outgoing pending half.

The lossy reader path is currently `__test-helpers`-gated and the production rehydration loader is still being built under CMT-011, so this is not a live data-loss bug today. However, the schema invariant the production loader is being designed against is exactly what this test reader encodes, and DashPay's mutual contact-request flow (Alice sends to Bob; Bob sends to Alice — Alice's wallet ends up with both `sent` and `incoming` for the same pair before either is established) is a documented real scenario in `tests/sqlite_load_reconstruction.rs:559-576`.

Before the production loader lands, decide whether the schema should (a) carry a richer state (e.g. add a `mutual-pending` label so the bucketing is unambiguous), (b) have the reader consult blob presence in addition to `state`, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the *post-second-upsert* state so this behaviour stops being silently untested.
- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:303-364: [Carry-forward] wallet deletion leaks parentless meta_contact/meta_platform_address rows and over-reports cleanup
  Carried-forward prior finding (STILL VALID at 932b923b). meta_contact/meta_platform_address still allow parentless wallet_id rows; the wallet-delete trigger still does not directly purge both child meta tables, while per-wallet counting can include them before wallet_metadata deletion. At 3fc28b92, meta_contact and meta_platform_address carried wallet_id but had no FK to wallet_metadata, and cleanup triggers fired only from contacts_established/platform_addresses. Parentless metadata rows could survive wallet deletion even though the cleanup report counted them.

[thepastaclaw]

Code Review Correction

Correction to my same-SHA review at 932b923b: prior finding #2 was verified STILL VALID and should be carried forward as a blocking finding. The review body’s enforced preamble accidentally omitted it, even though both reviewer agents verified it against current head.

Carried-forward prior finding #2 — STILL VALID

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (lines 405-415)

delete_wallet still rejects wallets whose only durable state is parentless metadata. The pre-flight existence gate still probes only wallet_metadata, with had_buffered as the only escape hatch:

let exists_pre_flush = conn
    .query_row(
        "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
        rusqlite::params![wallet_id.as_slice()],
        |_| Ok(()),
    )
    .optional()?
    .is_some();
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

A wallet whose only persisted rows are intentionally parentless meta_wallet, meta_contact, or meta_platform_address entries is rejected with WalletNotFound even though state exists on disk. Combined with the meta-table cascade gap from prior finding #1, the parentless-metadata path remains both undeletable here and liable to survive wallet cleanup.

Suggested fix: widen the existence check to include the per-wallet metadata-bearing tables (meta_wallet, meta_contact, meta_platform_address) before returning WalletNotFound.

Corrected reconciliation for 932b923b: prior findings #1, #2, #3, #4, and #5 are all STILL VALID and carried forward. New latest-delta finding remains the unified contacts.state mutual-pending reconstruction concern. This is a correction to the existing same-SHA review, not a fresh recheck.

[thepastaclaw]

Code Review

Prior finding reconciliation for 3166090: carried forward STILL VALID prior findings: #1:meta-parentless-delete-leak-overcount, #2:kv-put-allows-unreadable-oversized-values, #3:ffi-wallet-restore-xpub-trailing-bytes, #4:ffi-asset-lock-proof-trailing-bytes, #5:contacts-mutually-exclusive-state-drops-one-pending-side; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest delta 932b923..3166090 reviewed separately; new latest-delta findings are listed below if any.

Cumulative incremental review at head 3166090. The latest delta (932b923..3166090) is a v3.1-dev merge into the branch (dashmate, drive-abci, platform-version v8 method tables, Cargo.lock) and introduces no in-scope changes to platform-wallet / platform-wallet-storage / platform-wallet-ffi. Prior-finding reconciliation: five carried-forward findings from the 932b923 review remain STILL VALID at current head (re-verified line-by-line against V001__initial.rs:295-357, persister.rs:405-415, kv.rs:186-215, persistence.rs:2266-2271, persistence.rs:2956-2970), the CMT-003 unified-contacts state label observation remains STILL VALID, the load()/CMT-011 partial-restore item remains INTENTIONALLY DEFERRED, and five secrets/identity-ownership findings tracked by Codex are FIXED. Codex's new blocking finding on meta_contact trigger gating is the same root cause as Claude's first carried-forward finding and is merged here with both sources attributed.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 3166090 --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 854, in
result = post_review(
File ), so I posted the same verified findings as a top-level review body._

Reviewed commit: 3166090

🔴 2 blocking | 🟡 4 suggestion(s)

Verified findings

blocking: Carried-forward prior finding (STILL VALID): meta_contact / meta_platform_address rows can outlive their wallet; delete_wallet over-reports cleanup

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 295)

Re-verified at current head 3166090 — bytes-identical to 932b923. meta_contact (V001__initial.rs:295-303) and meta_platform_address (V001__initial.rs:305-312) carry wallet_id but have no FK to wallet_metadata. Cleanup relies on cascade_meta_contact_on_contact_delete (V001__initial.rs:340-349, gated WHEN OLD.state = 'established') and cascade_meta_platform_address_on_address_delete (V001__initial.rs:351-357), which fire only on deletes from contacts / platform_addresses. The new per-object metadata API explicitly supports parentless writes via KvStore::put on ObjectId::Contact / ObjectId::PlatformAddress and documents the invariant that 'metadata never outlives its object', so three classes of meta_contact rows can survive contact deletion AND whole-wallet deletion: (a) parentless rows written before any contact exists, (b) rows whose only matching contacts row is in sent/received state (the WHEN OLD.state = 'established' filter skips them), and (c) rows for contacts explicitly removed via remove_sent_contact_request before reaching established. A minimal SQLite reproduction with the current trigger leaves meta_contact count > 0 after deleting a sent contact row.

Compounding this, delete_wallet in src/sqlite/persister.rs:489-505 builds rows_removed_per_table by pre-counting PER_WALLET_TABLES (which still includes meta_contact and meta_platform_address with WalletScope::DirectColumn) before deleting wallet_metadata. Because the cascade-by-trigger flow leaves parentless / pending-state rows behind, the counts are an upper bound — DeleteWalletReport over-reports cleanup on the metadata-only / pending-contact paths, violating the API/documentation contract.

Fix shapes: (a) add AFTER DELETE ON wallet_metadata triggers that sweep both meta tables by wallet_id (and drop the WHEN OLD.state = 'established' filter on the contact trigger, or replace with a wallet-scoped sweep), or (b) issue explicit DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1 in delete_wallet before deleting wallet_metadata, and have the report use post-delete Connection::changes() instead of a pre-delete count.

CREATE TRIGGER cascade_meta_contact_on_contact_delete
AFTER DELETE ON contacts
FOR EACH ROW
BEGIN
    DELETE FROM meta_contact
        WHERE wallet_id = OLD.wallet_id
          AND owner_id = OLD.owner_id
          AND contact_id = OLD.contact_id;
END;

CREATE TRIGGER cascade_meta_contact_on_wallet_delete
AFTER DELETE ON wallet_metadata
FOR EACH ROW
BEGIN
    DELETE FROM meta_contact WHERE wallet_id = OLD.wallet_id;
END;

CREATE TRIGGER cascade_meta_platform_address_on_address_delete
AFTER DELETE ON platform_addresses
FOR EACH ROW
BEGIN
    DELETE FROM meta_platform_address
        WHERE wallet_id = OLD.wallet_id AND address = OLD.address;
END;

CREATE TRIGGER cascade_meta_platform_address_on_wallet_delete
AFTER DELETE ON wallet_metadata
FOR EACH ROW
BEGIN
    DELETE FROM meta_platform_address WHERE wallet_id = OLD.wallet_id;
END;

blocking: Carried-forward prior finding (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 405)

Re-verified at current head 3166090 — code unchanged. delete_wallet probes only wallet_metadata to decide whether the wallet exists, with the in-memory escape hatch through had_buffered:

let exists_pre_flush = conn
    .query_row(
        "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
        rusqlite::params![wallet_id.as_slice()],
        |_| Ok(()),
    )
    .optional()?
    .is_some();
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

A wallet whose only durable rows are the intentionally parentless meta_wallet / meta_contact / meta_platform_address entries (a supported pattern through KvStore::put) is rejected with WalletNotFound even though state exists on disk. Combined with the cascade gap above, the parentless-metadata path is both undeletable here AND silently retained when a wallet_metadata row eventually arrives. Buffer-only callers escape via had_buffered; persisted-metadata-only callers do not. Widen the existence check to include the per-wallet metadata-bearing tables so the API is internally consistent with the writers it accepts.

let exists_pre_flush = conn
    .query_row(
        "SELECT EXISTS(\
             SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_wallet WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_contact WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_platform_address WHERE wallet_id = ?1\
         )",
        rusqlite::params![wallet_id.as_slice()],
        |row| row.get::<_, i64>(0),
    )?
    != 0;
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

suggestion: Carried-forward prior finding (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 186)

Re-verified at current head 3166090 — code unchanged. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (kv.rs:162-183, now a single-snapshot read after CMT-001). put (kv.rs:186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than on every subsequent read.

fn put(&self, scope: &ObjectId, key: &str, value: &[u8]) -> Result<(), KvError> {
    validate_key(key)?;
    if value.len() > MAX_VALUE_LEN {
        return Err(KvError::ValueTooLarge {
            found: value.len(),
            max: MAX_VALUE_LEN,
        });
    }
    let sql = ScopeSql::resolve(scope);
    let conn = self.conn().map_err(KvError::from)?;
    let mut cols: Vec<&str> = sql.id_cols.to_vec();
    cols.push("key");
    let col_list = cols.join(", ");
    let value_phs = (1..=cols.len())
        .map(|i| format!("?{i}"))
        .collect::<Vec<_>>()
        .join(", ");
    let conflict_target = cols.join(", ");
    let value_ph = cols.len() + 1;
    let mut params: Vec<&dyn ToSql> = sql.id_vals.iter().map(|v| v as &dyn ToSql).collect();
    params.push(&key);
    params.push(&value);
    conn.execute(
        &format!(
            "INSERT INTO {} ({col_list}, value) VALUES ({value_phs}, ?{value_ph}) \
             ON CONFLICT({conflict_target}) \
             DO UPDATE SET value = excluded.value, updated_at = unixepoch()",
            sql.table
        ),
        params.as_slice(),
    )?;
    Ok(())
}

suggestion: Carried-forward prior finding (STILL VALID): FFI account-xpub restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

Re-verified at current head 3166090 — code unchanged. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the consumed-bytes count (persistence.rs:2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical.

This is inconsistent with the rest of the persistence boundary — the SQLite crate's blob::decode rejects trailing bytes (CMT-003 / bed413e) — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce consumed == xpub_bytes.len() and surface a typed mismatch error.

let xpub_bytes =
    unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
let (account_xpub, consumed): (ExtendedPubKey, usize) =
    bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
        PersistenceError::backend(format!("failed to decode account xpub: {}", e))
    })?;
if consumed != xpub_bytes.len() {
    return Err(PersistenceError::backend(
        "failed to decode account xpub: trailing bytes present".to_string(),
    ));
}

suggestion: Carried-forward prior finding (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

Re-verified at current head 3166090 — code unchanged. The tracked asset-lock restore path (persistence.rs:2959) calls dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard()) and discards the consumed count. Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid AssetLockProof encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map.

Enforce consumed == proof_bytes.len() to match the rest of the restore-boundary policy.

let proof_bytes =
    unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
    proof_bytes,
    config::standard(),
)
.map_err(|e| {
    PersistenceError::backend(format!(
        "tracked asset lock: failed to decode proof: {}",
        e
    ))
})?;
if consumed != proof_bytes.len() {
    return Err(PersistenceError::backend(
        "tracked asset lock: failed to decode proof: trailing bytes present".to_string(),
    ));
}
Some(proof)

suggestion: Carried-forward prior finding (STILL VALID): unified contacts row carries a mutually-exclusive `state` label even when both pending blobs are present

packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 108)

Re-verified at current head 3166090 — schema and apply() writer are unchanged. The CMT-003 rewrite collapses sent/received/established into one row per (wallet_id, owner_id, contact_id) with a single state label and two nullable request blobs. The pending upserts in apply() (contacts.rs:113-172) intentionally leave the other blob untouched on conflict — only the matching blob + state are overwritten by the ON CONFLICT DO UPDATE clause. If a sent upsert lands first and an incoming upsert for the same key arrives before establishment, the row ends up with state='received', incoming_request=Y, outgoing_request=X (still there from the prior upsert). On-disk data preserves both sides, but bucketing by state alone (mutually exclusive) is what the in-flight production rehydration loader will use — a CMT-003-shaped reader will emit only the incoming_requests entry and drop the previously persisted outgoing pending half.

DashPay's mutual contact-request flow (Alice→Bob, Bob→Alice; Alice's wallet sees both sent and incoming for the same pair before either is established) is a documented real scenario at tests/sqlite_load_reconstruction.rs:559-576. Before the production loader lands under CMT-011, decide whether the schema should (a) carry a richer state (e.g. add a mutual-pending label so bucketing is unambiguous), (b) have the reader consult blob presence in addition to state, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the post-second-upsert state so the behaviour stops being silently untested.

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

- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:295-357: Carried-forward prior finding (STILL VALID): meta_contact / meta_platform_address rows can outlive their wallet; delete_wallet over-reports cleanup
  Re-verified at current head 31660906 — bytes-identical to 932b923b. `meta_contact` (V001__initial.rs:295-303) and `meta_platform_address` (V001__initial.rs:305-312) carry `wallet_id` but have no FK to `wallet_metadata`. Cleanup relies on `cascade_meta_contact_on_contact_delete` (V001__initial.rs:340-349, gated `WHEN OLD.state = 'established'`) and `cascade_meta_platform_address_on_address_delete` (V001__initial.rs:351-357), which fire only on deletes from `contacts` / `platform_addresses`. The new per-object metadata API explicitly supports parentless writes via `KvStore::put` on `ObjectId::Contact` / `ObjectId::PlatformAddress` and documents the invariant that 'metadata never outlives its object', so three classes of `meta_contact` rows can survive contact deletion AND whole-wallet deletion: (a) parentless rows written before any contact exists, (b) rows whose only matching `contacts` row is in `sent`/`received` state (the `WHEN OLD.state = 'established'` filter skips them), and (c) rows for contacts explicitly removed via `remove_sent_contact_request` before reaching `established`. A minimal SQLite reproduction with the current trigger leaves `meta_contact` count > 0 after deleting a `sent` contact row.

Compounding this, `delete_wallet` in `src/sqlite/persister.rs:489-505` builds `rows_removed_per_table` by pre-counting `PER_WALLET_TABLES` (which still includes `meta_contact` and `meta_platform_address` with `WalletScope::DirectColumn`) before deleting `wallet_metadata`. Because the cascade-by-trigger flow leaves parentless / pending-state rows behind, the counts are an upper bound — `DeleteWalletReport` over-reports cleanup on the metadata-only / pending-contact paths, violating the API/documentation contract.

Fix shapes: (a) add `AFTER DELETE ON wallet_metadata` triggers that sweep both meta tables by `wallet_id` (and drop the `WHEN OLD.state = 'established'` filter on the contact trigger, or replace with a wallet-scoped sweep), or (b) issue explicit `DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1` in `delete_wallet` before deleting `wallet_metadata`, and have the report use post-delete `Connection::changes()` instead of a pre-delete count.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:405-415: Carried-forward prior finding (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata
  Re-verified at current head 31660906 — code unchanged. `delete_wallet` probes only `wallet_metadata` to decide whether the wallet exists, with the in-memory escape hatch through `had_buffered`:

```rust
let exists_pre_flush = conn
    .query_row(
        "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
        rusqlite::params![wallet_id.as_slice()],
        |_| Ok(()),
    )
    .optional()?
    .is_some();
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

A wallet whose only durable rows are the intentionally parentless meta_wallet / meta_contact / meta_platform_address entries (a supported pattern through KvStore::put) is rejected with WalletNotFound even though state exists on disk. Combined with the cascade gap above, the parentless-metadata path is both undeletable here AND silently retained when a wallet_metadata row eventually arrives. Buffer-only callers escape via had_buffered; persisted-metadata-only callers do not. Widen the existence check to include the per-wallet metadata-bearing tables so the API is internally consistent with the writers it accepts.

• [SUGGESTION] In packages/rs-platform-wallet-storage/src/sqlite/kv.rs:186-215: Carried-forward prior finding (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse
  Re-verified at current head 3166090 — code unchanged. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (kv.rs:162-183, now a single-snapshot read after CMT-001). put (kv.rs:186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted.

Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than on every subsequent read.

• [SUGGESTION] In packages/rs-platform-wallet-ffi/src/persistence.rs:2266-2271: Carried-forward prior finding (STILL VALID): FFI account-xpub restore silently accepts trailing bytes
  Re-verified at current head 3166090 — code unchanged. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the consumed-bytes count (persistence.rs:2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical.

This is inconsistent with the rest of the persistence boundary — the SQLite crate's blob::decode rejects trailing bytes (CMT-003 / bed413e) — and undermines the restore-boundary hardening this PR has been doing elsewhere. Enforce consumed == xpub_bytes.len() and surface a typed mismatch error.

• [SUGGESTION] In packages/rs-platform-wallet-ffi/src/persistence.rs:2956-2970: Carried-forward prior finding (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes
  Re-verified at current head 3166090 — code unchanged. The tracked asset-lock restore path (persistence.rs:2959) calls dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard()) and discards the consumed count. Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid AssetLockProof encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map.

Enforce consumed == proof_bytes.len() to match the rest of the restore-boundary policy.

• [SUGGESTION] In packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:108-223: Carried-forward prior finding (STILL VALID): unified contacts row carries a mutually-exclusive state label even when both pending blobs are present
  Re-verified at current head 3166090 — schema and apply() writer are unchanged. The CMT-003 rewrite collapses sent/received/established into one row per (wallet_id, owner_id, contact_id) with a single state label and two nullable request blobs. The pending upserts in apply() (contacts.rs:113-172) intentionally leave the other blob untouched on conflict — only the matching blob + state are overwritten by the ON CONFLICT DO UPDATE clause. If a sent upsert lands first and an incoming upsert for the same key arrives before establishment, the row ends up with state='received', incoming_request=Y, outgoing_request=X (still there from the prior upsert). On-disk data preserves both sides, but bucketing by state alone (mutually exclusive) is what the in-flight production rehydration loader will use — a CMT-003-shaped reader will emit only the incoming_requests entry and drop the previously persisted outgoing pending half.

DashPay's mutual contact-request flow (Alice→Bob, Bob→Alice; Alice's wallet sees both sent and incoming for the same pair before either is established) is a documented real scenario at tests/sqlite_load_reconstruction.rs:559-576. Before the production loader lands under CMT-011, decide whether the schema should (a) carry a richer state (e.g. add a mutual-pending label so bucketing is unambiguous), (b) have the reader consult blob presence in addition to state, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the post-second-upsert state so the behaviour stops being silently untested.

</details>

[lklimek]

also return tombstoned

[lklimek]

is there no impl From, Into, or similar constructor for ManagedIdentity and IdentityEntry types? I think it would be a good design pattern to apply here.

[lklimek]

ensure it's only called in tests

[lklimek]

why not TryFrom trait?

[lklimek]

Do we need the features that made us create these PER_WALLET_TABLES? I mean, are they required by the trait, or just added for user conveniance / logging / etc?

[lklimek]

add a consistency check - if wallet is provided and not default(), then do a query to ensure the identity_id actually belongs to that wallet. Apply similar check everywhere where wallet id is provided to the function but not used in the query. I guess a helper function would be useful here (double-check if it already exists).

[lklimek]

maybe we should annotate all test-only functions so that they are not usable by non-tests?

[lklimek]

Does it make sense to directly support buffered ops? Isn't there a buffering mechanism in the database itself?

[lklimek]

add annotation so that it's not reachable outside tests. Do that for all other pub functions dedicated to tests.

[lklimek]

rename to is_schema_version_supported , return true/false. Assert sounds like something used in tests

[Copilot]

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

[lklimek]

@coderabbitai review all

[coderabbitai]

@lklimek Sure! I'll kick off another full review of all the latest changes in this PR.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 14

♻️ Duplicate comments (2)

packages/rs-platform-wallet-storage/src/sqlite/migrations.rs (1)

36-38: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Mark runner() as test-only to match its documented purpose.

The rustdoc states this is "Used by tests that need to apply a subset of migrations," but the function is pub without any #[cfg(test)] gate, making it reachable from downstream crates.

#[cfg(any(test, feature = "__test-helpers"))]
pub fn runner() -> refinery::Runner {

🤖 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/migrations.rs` around lines 36
- 38, Mark the public helper function runner() as test-only by adding the
appropriate cfg attribute so it isn't exposed to downstream crates; update the
function signature for runner() (which simply returns migrations::runner()) to
be gated with #[cfg(any(test, feature = "__test-helpers"))] so it matches the
rustdoc and is only compiled for tests or the explicit test-helpers feature.

packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs (1)

53-80: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add kv_store to the per-wallet allowlist.

PER_WALLET_TABLES still omits kv_store, so the delete/inspect paths that iterate this list remain blind to per-wallet KV rows. That keeps DeleteWalletReport.rows_removed_per_table and operator-facing counts wrong even though the table is part of the on-disk schema.

🤖 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/mod.rs` around lines 53
- 80, PER_WALLET_TABLES omits the "kv_store" table so wallet delete/inspect
routines miss per-wallet KV rows; update the PER_WALLET_TABLES constant to
include the ("kv_store", WalletScope::DirectColumn) entry (or
WalletScope::ViaIdentity if KV is identity-scoped) alongside the other entries
so delete_wallet/inspect iterate it and
DeleteWalletReport.rows_removed_per_table reflects correct counts.

🧹 Nitpick comments (4)

packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs (1)

134-142: 💤 Low value

seal maps an encryption failure to SecretStoreError::Decrypt.

The encrypt path returning a Decrypt error is semantically misleading for any caller/log inspecting the variant. AEAD encryption effectively never fails here, so this is low-impact, but a dedicated/KdfFailure-style variant would read more accurately.

🤖 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/secrets/file/crypto.rs` around lines
134 - 142, The encryption path in seal maps a failure to
SecretStoreError::Decrypt, which is misleading; update the error mapping in the
call to cipher.encrypt inside the seal function (the ct = cipher.encrypt(...)
.map_err(...)? line) to use a dedicated encryption error variant (e.g., add/use
SecretStoreError::Encrypt or a KdfFailure-style variant) and propagate or
include the underlying error details in that variant so callers can distinguish
encryption failures from decryption failures.

packages/rs-platform-wallet-storage/tests/feature_flag_build.rs (1)

16-52: ⚡ Quick win

These checks are pinned to formatting instead of the feature contract.

The exact multiline contains(...) assertions will fail on harmless whitespace, key-order, or formatting changes in lib.rs/Cargo.toml even when the sqlite gating is still correct. Prefer structural assertions over literal source snippets so this only trips on a real contract break.

🤖 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/feature_flag_build.rs` around lines
16 - 52, The assertions in tests
tc_code_020_1_wallet_and_serde_deps_are_optional (and the earlier lib_src checks
that use include_str!) are brittle because they rely on exact multiline
formatting; instead parse the sources structurally: for the Cargo.toml string
(manifest) parse it with a TOML parser (or deserialize into a toml::Value) and
assert that the platform-wallet and serde entries exist with optional = true and
the sqlite feature enables dep:platform-wallet and dep:serde; for the lib.rs
string (lib_src) parse or use a lightweight regex to assert the presence of a
#[cfg(feature = "sqlite")] attribute and a corresponding pub mod sqlite; replace
the literal contains(...) checks with these structural checks in the test
functions to avoid whitespace/key-order fragility.

packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs (1)

27-49: 💤 Low value

Consider documenting the substring-matching limitation in the allow-list.

The READ_ONLY_PREPARE_ALLOWED uses substring matching against SQL statements. If SQL formatting changes (e.g., different whitespace, line breaks), the match may fail and require updating the allow-list. While this is acceptable for a test enforcement mechanism, consider adding a comment explaining that the substrings should be stable portions of the SQL (e.g., SELECT ... FROM ... clause) to minimize maintenance churn.

🤖 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_compile_time.rs` around
lines 27 - 49, The allow-list READ_ONLY_PREPARE_ALLOWED relies on substring
matching against SQL text which is brittle to formatting changes; update the
comment above READ_ONLY_PREPARE_ALLOWED to explicitly state this limitation and
give guidance to maintainers to pick stable substrings (e.g., canonical SELECT
... FROM ... clauses, avoid whitespace-sensitive fragments, prefer column+table
names) and to update entries whenever SQL formatting or line breaks change so
the test continues to match intended queries.

packages/rs-platform-wallet-storage/src/sqlite/error.rs (1)

380-395: ⚡ Quick win

Make persistence_kind() explicit too.

PersistenceError::from trusts this helper for the trait-boundary kind, but the wildcard fallback means a newly added non-transient variant silently becomes Fatal unless someone remembers to revisit this function. is_transient() and error_kind_str() already enforce explicit classification; persistence_kind() should enumerate its cases the same way, or get a matching wildcard-free invariant test.

🤖 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/error.rs` around lines 380 -
395, persistence_kind() currently uses a wildcard fallback which hides new enum
variants; change it to an exhaustive match over the PersistenceError enum (no
`_` arm) so every variant is explicitly mapped to a PersistenceErrorKind —
enumerate each variant (e.g. Self::Sqlite(rusqlite::Error::SqliteFailure(e, _))
with ErrorCode::ConstraintViolation => PersistenceErrorKind::Constraint,
Self::Migration => PersistenceErrorKind::Fatal, and any other existing variants
mapped to Transient/Constraint/Fatal as appropriate), remove the wildcard arm,
and rely on the compiler to force updates when new PersistenceError variants are
added; keep is_transient() and error_kind_str() behavior unchanged.

🤖 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-storage/migrations/V001__initial.rs`:
- Around line 340-349: The trigger cascade_meta_contact_on_contact_delete
currently only runs when OLD.state = 'established', leaving meta_contact rows
orphaned for deleted pending contacts; change the trigger definition to remove
the WHEN OLD.state = 'established' clause so the AFTER DELETE ON contacts
trigger always deletes matching rows from meta_contact (matching wallet_id,
owner_id, contact_id) regardless of contact state, ensuring KvStore-written
metadata is cleaned up when any contact row is deleted.

In `@packages/rs-platform-wallet-storage/SCHEMA.md`:
- Around line 216-217: Remove the stray/unused link reference definitions that
were accidentally left in the schema doc around the ASSET_LOCKS section; locate
the link reference lines (the markdown-style [label]: ... references) near the
ASSET_LOCKS block and delete them so the schema only contains schema definitions
(e.g., ASSET_LOCKS) and no secret-related or external link reference
definitions. Ensure no other schema sections reference those labels before
removing.

In `@packages/rs-platform-wallet-storage/SECRETS.md`:
- Around line 216-217: The two unused Markdown link reference definitions for
SecretBytes::new(...) and SecretStoreError are defined but never referenced;
either delete those definitions or update the document to use the
reference-style links. Locate the definitions for [`SecretBytes::new(...)`] and
[`SecretStoreError`] in SECRETS.md and either remove those reference lines, or
replace inline occurrences of SecretBytes::new(...) and SecretStoreError with
the bracketed reference form (e.g. [`SecretBytes::new(...)`],
[`SecretStoreError`]) so the links are actually used.

In `@packages/rs-platform-wallet-storage/src/kv.rs`:
- Around line 119-124: The put() implementation must validate value length and
reject oversized payloads: in the public trait method and all backend
implementations (the put() funcs), check if value.len() > MAX_VALUE_LEN and
return Err(KvError::ValueTooLarge) before attempting any DB/insert logic; update
unit/integration tests to assert put() returns KvError::ValueTooLarge for inputs
longer than MAX_VALUE_LEN and that get() behavior remains unchanged for valid
sizes. Ensure references to MAX_VALUE_LEN, put(), get(), and
KvError::ValueTooLarge are used so the length check occurs consistently across
backends.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 405-414: The existence check in the pre-flush path (variable
exists_pre_flush set via conn.query_row) only inspects wallet_metadata and must
be widened to also include meta_wallet, meta_contact, and meta_platform_address;
update the query used in conn.query_row (or replace with an EXISTS/UNION ALL
query) so it returns a boolean/int indicating if any of those tables contain the
wallet_id, and then set exists_pre_flush based on that result instead of only
wallet_metadata.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- Around line 120-126: The upsert's conflict-handling currently only preserves
an already-'established' state and never promotes a row when the opposite
request blob is present; update the ON CONFLICT ... DO UPDATE SET for the
prepared statements to compute state as 'established' when both request blobs
are present (consider existing contacts.incoming_request/outgoing_request and
the excluded incoming_request/outgoing_request), otherwise use excluded.state
(but still preserve an existing 'established'); apply the same logic to the
second similar upsert (the other prepare_cached around lines 155-161) so a row
is promoted to 'established' when both sides of the request are stored.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 159-171: The current lookup of account_index via lookup_stmt uses
unwrap_or_else to default to 0 for any missing mapping, which causes new unspent
UTXOs (cs.new_utxos) to be misattributed; change the logic so that when the
query returns None you only default to 0 if this UTXO is a spent-only
placeholder (the code path that writes cs.spent_utxos), otherwise return an
error to fail the write for unspent UTXOs. Concretely, replace the
unwrap_or_else(...) with explicit handling of the Optional result from
lookup_stmt.query_row(...).optional()? — if Some(v) set account_index = v; if
None and spent == true set account_index = 0; if None and spent == false return
an Err (with a clear message including wallet_id and address) so the write
aborts.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 39-66: In the loop over cs.identities (the for (id, entry) in
&cs.identities block) ensure the serialized blob's identity id matches the map
key: before calling blob::encode(entry) either validate that entry.id == *id and
return a descriptive WalletStorageError (e.g., IdentityIdMismatch) on mismatch,
or normalize by assigning the map key into the entry (entry.id = *id) so the
payload is consistent with the typed identity_id column; implement this change
just before the payload = blob::encode(entry)? line so the DB row and blob
cannot diverge.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- Around line 216-235: The current load_all function does an N+1 query by
calling wallet_meta::list_ids then invoking load_state and count_per_wallet per
wallet; replace this with a single grouped query (or a small fixed set of
queries) that joins wallet_meta (or uses its IDs) with platform_address_sync and
platform_addresses to select the sync state columns and COUNT(*) per wallet_id,
grouping by wallet_id, and then build the BTreeMap<WalletId, LoadAllEntry> from
those rows; remove per-wallet calls to load_state and count_per_wallet and keep
function name load_all, mapping the returned columns to the LoadAllEntry tuple
so startup/load becomes constant-query as required.

In `@packages/rs-platform-wallet-storage/tests/secrets_off_state.rs`:
- Around line 14-24: The test currently only touches SqlitePersister and doesn't
prove platform_wallet_storage::secrets is gone; add a negative compile-time
check that attempts to reference the secrets module and asserts compilation
fails when the "secrets" feature is off. Concretely, add a trybuild (or
compile_fail) test that, under #[cfg(not(feature = "secrets"))], compiles a
small source snippet that references platform_wallet_storage::secrets (e.g.,
platform_wallet_storage::secrets::SomeSecretType or
platform_wallet_storage::secrets::SecretsPersister) and expects a compile error;
this ensures any public re-export of platform_wallet_storage::secrets will cause
the test to fail. Ensure the test file name and test harness call use the
existing tests/secrets_off_state.rs intent so the negative check runs only with
the feature disabled.

In `@packages/rs-platform-wallet-storage/tests/secrets_scan.rs`:
- Around line 81-84: The test currently skips files that fail to read (the match
on std::fs::read_to_string with Err(_) => continue), which hides unreadable or
invalid-UTF-8 files; change that behavior so read failures cause the test to
fail: replace the continue branch with an error propagation (panic/expect/Err
return) that includes the path variable p and the read error (capture Err(e) not
Err(_)) so failures in read_to_string(p) are reported and fail the test instead
of being silently skipped.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs`:
- Around line 161-185: The test tc016_buffer_merge_oracle_smoke currently uses
fresh_persister() and never calls flush(), so it never exercises buffered flush
semantics; change the setup to use fresh_persister_with_mode(FlushMode::Manual)
instead of fresh_persister(), and after performing the loop of
persister.store(...) calls invoke persister.flush() once before acquiring the
connection and reading back synced_height/last_processed_height so the test
validates the Buffer merge path.

In `@packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs`:
- Around line 33-69: The test delete_wallet_restores_buffer_on_backup_failure
relies on filesystem permission bits (set to 0o500) to force create_dir_all to
fail, but this is bypassed when running as root in CI; update the test to
early-skip when running as root by calling is_root_via_probe() before setting up
the bad backup dir (same guard used in sqlite_backup_restore.rs) so the test is
deterministic in containers. Add or move the is_root_via_probe() helper into
tests/common/mod.rs for reuse, and change the test to call that helper and
return/skip when it reports root, leaving persister_with_bad_backup_dir and the
rest of the test unchanged.

In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs`:
- Around line 63-191: The tc027_smoke_insert_every_table test currently
hardcodes a `cases` list and misses the new meta_* per-wallet tables; update the
test (in tc027_smoke_insert_every_table) to guarantee parity with
PER_WALLET_TABLES by either (1) adding minimal INSERT statements for the missing
meta_* tables into `cases` (using the same wallet_id param pattern and valid
minimal blobs/values), or (preferred) (2) assert that the set of table names in
`cases` equals the table names in PER_WALLET_TABLES (using the existing
scope_for/ count_rows_for_wallet_sql helpers) and fail the test if they differ
so any new allowlisted table forces you to add an insert; reference the `cases`
variable, `PER_WALLET_TABLES`, `scope_for`, and `count_rows_for_wallet_sql` to
locate where to implement the parity check or add the missing inserts.

---

Duplicate comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/migrations.rs`:
- Around line 36-38: Mark the public helper function runner() as test-only by
adding the appropriate cfg attribute so it isn't exposed to downstream crates;
update the function signature for runner() (which simply returns
migrations::runner()) to be gated with #[cfg(any(test, feature =
"__test-helpers"))] so it matches the rustdoc and is only compiled for tests or
the explicit test-helpers feature.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs`:
- Around line 53-80: PER_WALLET_TABLES omits the "kv_store" table so wallet
delete/inspect routines miss per-wallet KV rows; update the PER_WALLET_TABLES
constant to include the ("kv_store", WalletScope::DirectColumn) entry (or
WalletScope::ViaIdentity if KV is identity-scoped) alongside the other entries
so delete_wallet/inspect iterate it and
DeleteWalletReport.rows_removed_per_table reflects correct counts.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs`:
- Around line 134-142: The encryption path in seal maps a failure to
SecretStoreError::Decrypt, which is misleading; update the error mapping in the
call to cipher.encrypt inside the seal function (the ct = cipher.encrypt(...)
.map_err(...)? line) to use a dedicated encryption error variant (e.g., add/use
SecretStoreError::Encrypt or a KdfFailure-style variant) and propagate or
include the underlying error details in that variant so callers can distinguish
encryption failures from decryption failures.

In `@packages/rs-platform-wallet-storage/src/sqlite/error.rs`:
- Around line 380-395: persistence_kind() currently uses a wildcard fallback
which hides new enum variants; change it to an exhaustive match over the
PersistenceError enum (no `_` arm) so every variant is explicitly mapped to a
PersistenceErrorKind — enumerate each variant (e.g.
Self::Sqlite(rusqlite::Error::SqliteFailure(e, _)) with
ErrorCode::ConstraintViolation => PersistenceErrorKind::Constraint,
Self::Migration => PersistenceErrorKind::Fatal, and any other existing variants
mapped to Transient/Constraint/Fatal as appropriate), remove the wildcard arm,
and rely on the compiler to force updates when new PersistenceError variants are
added; keep is_transient() and error_kind_str() behavior unchanged.

In `@packages/rs-platform-wallet-storage/tests/feature_flag_build.rs`:
- Around line 16-52: The assertions in tests
tc_code_020_1_wallet_and_serde_deps_are_optional (and the earlier lib_src checks
that use include_str!) are brittle because they rely on exact multiline
formatting; instead parse the sources structurally: for the Cargo.toml string
(manifest) parse it with a TOML parser (or deserialize into a toml::Value) and
assert that the platform-wallet and serde entries exist with optional = true and
the sqlite feature enables dep:platform-wallet and dep:serde; for the lib.rs
string (lib_src) parse or use a lightweight regex to assert the presence of a
#[cfg(feature = "sqlite")] attribute and a corresponding pub mod sqlite; replace
the literal contains(...) checks with these structural checks in the test
functions to avoid whitespace/key-order fragility.

In `@packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs`:
- Around line 27-49: The allow-list READ_ONLY_PREPARE_ALLOWED relies on
substring matching against SQL text which is brittle to formatting changes;
update the comment above READ_ONLY_PREPARE_ALLOWED to explicitly state this
limitation and give guidance to maintainers to pick stable substrings (e.g.,
canonical SELECT ... FROM ... clauses, avoid whitespace-sensitive fragments,
prefer column+table names) and to update entries whenever SQL formatting or line
breaks change so the test continues to match intended queries.

🪄 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: ed3c42b6-5f24-44f7-8d4c-51d8d5ce312a

📥 Commits

Reviewing files that changed from the base of the PR and between 9eca622 and 3166090.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (87)

• .github/package-filters/js-packages-no-workflows.yml
• .github/workflows/pr.yml
• .github/workflows/tests-rs-workspace.yml
• .github/workflows/tests.yml
• Cargo.toml
• Dockerfile
• packages/rs-platform-wallet-ffi/src/persistence.rs
• packages/rs-platform-wallet-storage/Cargo.toml
• packages/rs-platform-wallet-storage/README.md
• packages/rs-platform-wallet-storage/SCHEMA.md
• packages/rs-platform-wallet-storage/SECRETS.md
• packages/rs-platform-wallet-storage/build.rs
• packages/rs-platform-wallet-storage/migrations/V001__initial.rs
• packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs
• packages/rs-platform-wallet-storage/src/kv.rs
• packages/rs-platform-wallet-storage/src/lib.rs
• packages/rs-platform-wallet-storage/src/secrets/error.rs
• packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs
• packages/rs-platform-wallet-storage/src/secrets/file/format.rs
• packages/rs-platform-wallet-storage/src/secrets/file/mod.rs
• packages/rs-platform-wallet-storage/src/secrets/keyring.rs
• packages/rs-platform-wallet-storage/src/secrets/mod.rs
• packages/rs-platform-wallet-storage/src/secrets/secret.rs
• packages/rs-platform-wallet-storage/src/secrets/store.rs
• packages/rs-platform-wallet-storage/src/secrets/validate.rs
• packages/rs-platform-wallet-storage/src/sqlite/backup.rs
• packages/rs-platform-wallet-storage/src/sqlite/buffer.rs
• packages/rs-platform-wallet-storage/src/sqlite/config.rs
• packages/rs-platform-wallet-storage/src/sqlite/conn.rs
• packages/rs-platform-wallet-storage/src/sqlite/error.rs
• packages/rs-platform-wallet-storage/src/sqlite/kv.rs
• packages/rs-platform-wallet-storage/src/sqlite/migrations.rs
• packages/rs-platform-wallet-storage/src/sqlite/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/persister.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs
• packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs
• packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs
• packages/rs-platform-wallet-storage/tests/common/mod.rs
• packages/rs-platform-wallet-storage/tests/feature_flag_build.rs
• packages/rs-platform-wallet-storage/tests/persistence_error_kind_mapping.rs
• packages/rs-platform-wallet-storage/tests/secrets_api.rs
• packages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rs
• packages/rs-platform-wallet-storage/tests/secrets_guard.rs
• packages/rs-platform-wallet-storage/tests/secrets_off_state.rs
• packages/rs-platform-wallet-storage/tests/secrets_scan.rs
• packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
• packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs
• packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs
• packages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rs
• packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs
• packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rs
• packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs
• packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs
• packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs
• packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs
• packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
• packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs
• packages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rs
• packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs
• packages/rs-platform-wallet-storage/tests/sqlite_open_version_gate.rs
• packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs
• packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs
• packages/rs-platform-wallet-storage/tests/sqlite_restore_cross_process_exclusion.rs
• packages/rs-platform-wallet-storage/tests/sqlite_restore_staged_validation.rs
• packages/rs-platform-wallet-storage/tests/sqlite_trait_dispatch.rs
• packages/rs-platform-wallet/Cargo.toml
• packages/rs-platform-wallet/src/changeset/changeset.rs
• packages/rs-platform-wallet/src/changeset/mod.rs
• packages/rs-platform-wallet/src/changeset/serde_adapters.rs
• packages/rs-platform-wallet/src/changeset/traits.rs
• packages/rs-platform-wallet/src/manager/mod.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/sync/proof.rs
• packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove the established gate from contact-metadata cleanup.

KvStore::put explicitly allows metadata to be written before the parent object exists, and ObjectId::Contact does not encode contact state. With this WHEN OLD.state = 'established' clause, deleting a pending sent/received contact leaves meta_contact rows behind even though the contact object is gone.

Suggested fix

 CREATE TRIGGER cascade_meta_contact_on_contact_delete
 AFTER DELETE ON contacts
 FOR EACH ROW
-WHEN OLD.state = 'established'
 BEGIN
     DELETE FROM meta_contact
         WHERE wallet_id = OLD.wallet_id
           AND owner_id = OLD.owner_id
           AND contact_id = OLD.contact_id;
 END;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	CREATE TRIGGER cascade_meta_contact_on_contact_delete
	AFTER DELETE ON contacts
	FOR EACH ROW
	WHEN OLD.state = 'established'
	BEGIN
	DELETE FROM meta_contact
	WHERE wallet_id = OLD.wallet_id
	AND owner_id = OLD.owner_id
	AND contact_id = OLD.contact_id;
	END;
	CREATE TRIGGER cascade_meta_contact_on_contact_delete
	AFTER DELETE ON contacts
	FOR EACH ROW
	BEGIN
	DELETE FROM meta_contact
	WHERE wallet_id = OLD.wallet_id
	AND owner_id = OLD.owner_id
	AND contact_id = OLD.contact_id;
	END;

🤖 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/migrations/V001__initial.rs` around lines
340 - 349, The trigger cascade_meta_contact_on_contact_delete currently only
runs when OLD.state = 'established', leaving meta_contact rows orphaned for
deleted pending contacts; change the trigger definition to remove the WHEN
OLD.state = 'established' clause so the AFTER DELETE ON contacts trigger always
deletes matching rows from meta_contact (matching wallet_id, owner_id,
contact_id) regardless of contact state, ensuring KvStore-written metadata is
cleaned up when any contact row is deleted.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove unused link reference definitions.

These link references are not used anywhere in the document and appear out of place in schema documentation (they reference secret-related modules). Please remove them to keep the documentation clean.

🤖 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 216 - 217, Remove
the stray/unused link reference definitions that were accidentally left in the
schema doc around the ASSET_LOCKS section; locate the link reference lines (the
markdown-style [label]: ... references) near the ASSET_LOCKS block and delete
them so the schema only contains schema definitions (e.g., ASSET_LOCKS) and no
secret-related or external link reference definitions. Ensure no other schema
sections reference those labels before removing.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove or integrate unused link reference definitions.

These link references are defined but never used in the document. Either:

1. Remove them if they're not needed, or
2. Convert inline code references like "SecretBytes::new(...)" to link references like "[SecretBytes::new(...)]" to make them clickable.

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 216-216: Link and image reference definitions should be needed
Unused link or image reference definition: "secretbytes::new(...)"

(MD053, link-image-reference-definitions)

---

[warning] 217-217: Link and image reference definitions should be needed
Unused link or image reference definition: "secretstoreerror"

(MD053, link-image-reference-definitions)

🤖 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/SECRETS.md` around lines 216 - 217, The
two unused Markdown link reference definitions for SecretBytes::new(...) and
SecretStoreError are defined but never referenced; either delete those
definitions or update the document to use the reference-style links. Locate the
definitions for [`SecretBytes::new(...)`] and [`SecretStoreError`] in SECRETS.md
and either remove those reference lines, or replace inline occurrences of
SecretBytes::new(...) and SecretStoreError with the bracketed reference form
(e.g. [`SecretBytes::new(...)`], [`SecretStoreError`]) so the links are actually
used.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject oversized values in put too.

get() is required to fail on values larger than MAX_VALUE_LEN, but put() currently accepts any slice. That means a successful write can create state the same API must later refuse to read, which breaks read-after-write semantics for KV entries. Please make put() return KvError::ValueTooLarge when value.len() > MAX_VALUE_LEN and mirror that in backend implementations/tests.

🤖 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/kv.rs` around lines 119 - 124, The
put() implementation must validate value length and reject oversized payloads:
in the public trait method and all backend implementations (the put() funcs),
check if value.len() > MAX_VALUE_LEN and return Err(KvError::ValueTooLarge)
before attempting any DB/insert logic; update unit/integration tests to assert
put() returns KvError::ValueTooLarge for inputs longer than MAX_VALUE_LEN and
that get() behavior remains unchanged for valid sizes. Ensure references to
MAX_VALUE_LEN, put(), get(), and KvError::ValueTooLarge are used so the length
check occurs consistently across backends.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Widen existence check to include parentless metadata tables.

The pre-flight check only queries wallet_metadata, so a wallet whose only persisted state consists of meta_wallet, meta_contact, or meta_platform_address entries is rejected with WalletNotFound even though that is a documented and supported write pattern (via KvStore::put).

Per the carried-forward blocking finding, extend the existence check to cover all metadata-bearing tables:

Suggested change

	let exists_pre_flush = conn
	.query_row(
	"SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1",
	rusqlite::params![wallet_id.as_slice()],
	|_| Ok(()),
	)
	.optional()?
	.is_some();
	if !had_buffered && !exists_pre_flush {
	return Err(WalletStorageError::WalletNotFound { wallet_id });
	let exists_pre_flush = conn
	.query_row(
	"SELECT EXISTS(\
	SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_wallet WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_contact WHERE wallet_id = ?1\
	UNION ALL\
	SELECT 1 FROM meta_platform_address WHERE wallet_id = ?1\
	)",
	rusqlite::params![wallet_id.as_slice()],
	|row| row.get::<_, i64>(0),
	)?
	!= 0;

🤖 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 405
- 414, The existence check in the pre-flush path (variable exists_pre_flush set
via conn.query_row) only inspects wallet_metadata and must be widened to also
include meta_wallet, meta_contact, and meta_platform_address; update the query
used in conn.query_row (or replace with an EXISTS/UNION ALL query) so it returns
a boolean/int indicating if any of those tables contain the wallet_id, and then
set exists_pre_flush based on that result instead of only wallet_metadata.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This doesn't actually verify that secrets disappeared from the public API.

Compiling a file that only touches SqlitePersister still succeeds if platform_wallet_storage::secrets stays publicly exported, so the regression this test describes can slip through. Add a negative compile-time check or a source-surface assertion for the secrets exports themselves.

🤖 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/secrets_off_state.rs` around lines
14 - 24, The test currently only touches SqlitePersister and doesn't prove
platform_wallet_storage::secrets is gone; add a negative compile-time check that
attempts to reference the secrets module and asserts compilation fails when the
"secrets" feature is off. Concretely, add a trybuild (or compile_fail) test
that, under #[cfg(not(feature = "secrets"))], compiles a small source snippet
that references platform_wallet_storage::secrets (e.g.,
platform_wallet_storage::secrets::SomeSecretType or
platform_wallet_storage::secrets::SecretsPersister) and expects a compile error;
this ensures any public re-export of platform_wallet_storage::secrets will cause
the test to fail. Ensure the test file name and test harness call use the
existing tests/secrets_off_state.rs intent so the negative check runs only with
the feature disabled.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail when a scanned file cannot be read.

This still turns unreadable or invalid-UTF-8 rs/sql/md files into a silent skip, so the guardrail can pass while missing part of the schema surface. File-read failures should fail the test just like unreadable scan roots do.

Suggested fix

-        let body = match std::fs::read_to_string(&p) {
-            Ok(s) => s,
-            Err(_) => continue,
-        };
+        let body = std::fs::read_to_string(&p).unwrap_or_else(|e| {
+            panic!(
+                "secrets-scan: failed to read `{}` ({e}); scanned files must be readable",
+                p.display()
+            )
+        });

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let body = match std::fs::read_to_string(&p) {
	Ok(s) => s,
	Err(_) => continue,
	};
	let body = std::fs::read_to_string(&p).unwrap_or_else(|e| {
	panic!(
	"secrets-scan: failed to read `{}` ({e}); scanned files must be readable",
	p.display()
	)
	});

🤖 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/secrets_scan.rs` around lines 81 -
84, The test currently skips files that fail to read (the match on
std::fs::read_to_string with Err(_) => continue), which hides unreadable or
invalid-UTF-8 files; change that behavior so read failures cause the test to
fail: replace the continue branch with an error propagation (panic/expect/Err
return) that includes the path variable p and the read error (capture Err(e) not
Err(_)) so failures in read_to_string(p) are reported and fail the test instead
of being silently skipped.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This oracle never exercises the buffered flush path.

fresh_persister() is the immediate-persistence fixture elsewhere in this file, and this test never calls flush(). So it currently validates repeated immediate writes, not “N stores then flush == one merged store” as advertised. Switch this case to fresh_persister_with_mode(FlushMode::Manual) and flush once before reading back the row so the property test actually covers Buffer merge semantics.

Suggested fix

-        let (persister, _tmp, _path) = fresh_persister();
+        let (persister, _tmp, _path) = fresh_persister_with_mode(FlushMode::Manual);
         let w = wid(0x40);
         ensure_wallet_meta(&persister, &w);
         for &(sp, lp) in &heights {
             persister.store(w, changeset(core_with_height(sp, lp))).unwrap();
         }
+        persister.flush(w).unwrap();
         // Read back the persisted heights.
         let conn = persister.lock_conn_for_test();

🤖 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_buffer_semantics.rs` around
lines 161 - 185, The test tc016_buffer_merge_oracle_smoke currently uses
fresh_persister() and never calls flush(), so it never exercises buffered flush
semantics; change the setup to use fresh_persister_with_mode(FlushMode::Manual)
instead of fresh_persister(), and after performing the loop of
persister.store(...) calls invoke persister.flush() once before acquiring the
connection and reading back synced_height/last_processed_height so the test
validates the Buffer merge path.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test fails when run as root — the 0o500 write-block is bypassed by UID 0.

delete_wallet_restores_buffer_on_backup_failure depends on the 0o500 permission on does-not-exist/ making create_dir_all(bad_dir) fail. Under UID 0 (common in CI/Docker containers — this PR adds a Dockerfile), the directory-permission check is bypassed, so the auto-backup succeeds, delete_wallet returns Ok, and expect_err at Line 65 panics. The sibling tests in sqlite_backup_restore.rs explicitly guard this with is_root_via_probe(); this test should do the same (skip under root) for consistency and CI stability.

Proposed guard

 #[test]
 fn delete_wallet_restores_buffer_on_backup_failure() {
+    // Under root the 0o500 write-block is bypassed, so the auto-backup
+    // would succeed and delete_wallet would not fail. Skip in that case.
+    #[cfg(unix)]
+    if is_root_via_probe() {
+        eprintln!("skip: read-only-dir permission bypassed by root");
+        return;
+    }
     let (persister, _tmp, _path) = persister_with_bad_backup_dir();

You can lift a small is_root_via_probe() helper (as in sqlite_backup_restore.rs) into tests/common/mod.rs to avoid duplication.

🤖 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_wallet.rs` around
lines 33 - 69, The test delete_wallet_restores_buffer_on_backup_failure relies
on filesystem permission bits (set to 0o500) to force create_dir_all to fail,
but this is bypassed when running as root in CI; update the test to early-skip
when running as root by calling is_root_via_probe() before setting up the bad
backup dir (same guard used in sqlite_backup_restore.rs) so the test is
deterministic in containers. Add or move the is_root_via_probe() helper into
tests/common/mod.rs for reuse, and change the test to call that helper and
return/skip when it reports root, leaving persister_with_bad_backup_dir and the
rest of the test unchanged.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

TC-027 no longer covers the full per-wallet schema.

This test says “every declared table,” but the manual cases list stops before the newly allowlisted meta_* tables. That means schema drift in those tables can slip through while the smoke test still passes. Please either add minimal inserts for the metadata tables or assert parity against PER_WALLET_TABLES so the test fails when the allowlist grows.

🤖 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
63 - 191, The tc027_smoke_insert_every_table test currently hardcodes a `cases`
list and misses the new meta_* per-wallet tables; update the test (in
tc027_smoke_insert_every_table) to guarantee parity with PER_WALLET_TABLES by
either (1) adding minimal INSERT statements for the missing meta_* tables into
`cases` (using the same wallet_id param pattern and valid minimal blobs/values),
or (preferred) (2) assert that the set of table names in `cases` equals the
table names in PER_WALLET_TABLES (using the existing scope_for/
count_rows_for_wallet_sql helpers) and fail the test if they differ so any new
allowlisted table forces you to add an insert; reference the `cases` variable,
`PER_WALLET_TABLES`, `scope_for`, and `count_rows_for_wallet_sql` to locate
where to implement the parity check or add the missing inserts.

[thepastaclaw]

Code Review

Cumulative review at head 0466241. Latest delta (3166090..0466241) is solely a Cargo.lock version bump (platform-wallet-storage 3.1.0-dev.8 -> 4.0.0-beta.2) with no source changes — no new latest-delta findings. Re-verified all six prior findings line-by-line against the current worktree: all six remain STILL VALID. Prior FIXED findings (keyutils backend, debug-only ownership check, FileStoreError rename, missing IO path on SecretStoreError, opaque SecretStore Debug) retain their FIXED status. The load()/CMT-011 partial-rehydration item remains INTENTIONALLY DEFERRED (explicit TODO + structured warning still present). Both agents converge on the same six carried-forward findings with consistent evidence.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 0466241 --dry-run
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 854, in
result = post_review(
File ), so I posted the same verified findings as a top-level review body._

Reviewed commit: 0466241

🔴 2 blocking | 🟡 4 suggestion(s)

Verified findings

blocking: Carried-forward (STILL VALID): meta_contact / meta_platform_address rows can outlive their wallet; delete_wallet over-reports cleanup

packages/rs-platform-wallet-storage/migrations/V001__initial.rs (line 295)

Verified at head 0466241 — V001__initial.rs:295-357 is unchanged. meta_contact (295-303) and meta_platform_address (305-312) carry wallet_id but have no FK to wallet_metadata. Cleanup relies on cascade_meta_contact_on_contact_delete (340-349, gated WHEN OLD.state = 'established') and cascade_meta_platform_address_on_address_delete (351-357), which fire only on deletes from contacts / platform_addresses. Since KvStore::put on ObjectId::Contact / ObjectId::PlatformAddress explicitly supports parentless writes, three classes of meta_contact rows can survive contact deletion AND whole-wallet deletion: (a) parentless rows written before any contact exists, (b) rows whose only matching contacts row is in sent/received state (the WHEN OLD.state = 'established' filter skips them), and (c) rows for contacts removed via remove_sent_contact_request before reaching established. Same gap applies to meta_platform_address for parentless metadata. Compounding this, delete_wallet in src/sqlite/persister.rs:489-505 pre-counts PER_WALLET_TABLES (still including both meta tables) before deleting wallet_metadata. Because the cascade-by-trigger flow leaves parentless / pending-state rows behind, those counts become an upper bound — DeleteWalletReport over-reports cleanup on the metadata-only / pending-contact paths, violating the API/documentation contract ('metadata never outlives its object'). Fix shapes: add AFTER DELETE ON wallet_metadata triggers that sweep both meta tables by wallet_id and drop the WHEN OLD.state = 'established' filter (or replace with a wallet-scoped sweep); or issue explicit DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1 in delete_wallet before deleting wallet_metadata and have the report use post-delete Connection::changes() instead of a pre-delete count.

CREATE TRIGGER cascade_meta_contact_on_contact_delete
AFTER DELETE ON contacts
FOR EACH ROW
BEGIN
    DELETE FROM meta_contact
        WHERE wallet_id = OLD.wallet_id
          AND owner_id = OLD.owner_id
          AND contact_id = OLD.contact_id;
END;

CREATE TRIGGER cascade_meta_contact_on_wallet_delete
AFTER DELETE ON wallet_metadata
FOR EACH ROW
BEGIN
    DELETE FROM meta_contact WHERE wallet_id = OLD.wallet_id;
END;

CREATE TRIGGER cascade_meta_platform_address_on_address_delete
AFTER DELETE ON platform_addresses
FOR EACH ROW
BEGIN
    DELETE FROM meta_platform_address
        WHERE wallet_id = OLD.wallet_id AND address = OLD.address;
END;

CREATE TRIGGER cascade_meta_platform_address_on_wallet_delete
AFTER DELETE ON wallet_metadata
FOR EACH ROW
BEGIN
    DELETE FROM meta_platform_address WHERE wallet_id = OLD.wallet_id;
END;

blocking: Carried-forward (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 405)

Verified at head 0466241 — persister.rs:405-415 unchanged. delete_wallet probes only wallet_metadata to decide whether a wallet exists (with an in-memory escape hatch via had_buffered). A wallet whose only durable rows are the intentionally parentless meta_wallet / meta_contact / meta_platform_address entries (a supported pattern through KvStore::put) is rejected with WalletNotFound even though state exists on disk. Combined with the cascade gap above, the parentless-metadata path is both undeletable here AND silently retained when a wallet_metadata row eventually arrives. Buffer-only callers escape via had_buffered; persisted-metadata-only callers do not. Widen the existence check to include the per-wallet metadata-bearing tables so the API is internally consistent with the writers it accepts.

let exists_pre_flush = conn
    .query_row(
        "SELECT EXISTS(\
             SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_wallet WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_contact WHERE wallet_id = ?1\
             UNION ALL\
             SELECT 1 FROM meta_platform_address WHERE wallet_id = ?1\
         )",
        rusqlite::params![wallet_id.as_slice()],
        |row| row.get::<_, i64>(0),
    )?
    != 0;
if !had_buffered && !exists_pre_flush {
    return Err(WalletStorageError::WalletNotFound { wallet_id });
}

suggestion: Carried-forward (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse

packages/rs-platform-wallet-storage/src/sqlite/kv.rs (line 186)

Verified at head 0466241 — kv.rs:186-215 unchanged. get enforces length(value) <= MAX_VALUE_LEN and returns KvError::ValueTooLarge { found, max } (kv.rs:162-183). put (kv.rs:186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw value: &[u8] regardless of length. A put of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted. Add a symmetric guard at the top of put, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than on every subsequent read.

fn put(&self, scope: &ObjectId, key: &str, value: &[u8]) -> Result<(), KvError> {
    validate_key(key)?;
    if value.len() > MAX_VALUE_LEN {
        return Err(KvError::ValueTooLarge {
            found: value.len(),
            max: MAX_VALUE_LEN,
        });
    }
    let sql = ScopeSql::resolve(scope);
    let conn = self.conn().map_err(KvError::from)?;
    let mut cols: Vec<&str> = sql.id_cols.to_vec();
    cols.push("key");
    let col_list = cols.join(", ");
    let value_phs = (1..=cols.len())
        .map(|i| format!("?{i}"))
        .collect::<Vec<_>>()
        .join(", ");
    let conflict_target = cols.join(", ");
    let value_ph = cols.len() + 1;
    let mut params: Vec<&dyn ToSql> = sql.id_vals.iter().map(|v| v as &dyn ToSql).collect();
    params.push(&key);
    params.push(&value);
    conn.execute(
        &format!(
            "INSERT INTO {} ({col_list}, value) VALUES ({value_phs}, ?{value_ph}) \
             ON CONFLICT({conflict_target}) \
             DO UPDATE SET value = excluded.value, updated_at = unixepoch()",
            sql.table
        ),
        params.as_slice(),
    )?;
    Ok(())
}

suggestion: Carried-forward (STILL VALID): FFI account-xpub restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2266)

Verified at head 0466241 — persistence.rs:2266-2271 unchanged. build_wallet_start_state decodes account_xpub_bytes via bincode::decode_from_slice(...) and discards the consumed-bytes count (2268 binds it to _). A Swift→Rust restore payload whose prefix is a valid ExtendedPubKey encoding followed by garbage is silently accepted as canonical. This is inconsistent with the storage crate's hardened blob::decode, which rejects trailing bytes (CMT-003 / bed413e). Enforce consumed == xpub_bytes.len() and surface a typed mismatch error to match the rest of the restore-boundary policy.

let xpub_bytes =
    unsafe { slice_from_raw(spec.account_xpub_bytes, spec.account_xpub_bytes_len) };
let (account_xpub, consumed): (ExtendedPubKey, usize) =
    bincode::decode_from_slice(xpub_bytes, config::standard()).map_err(|e| {
        PersistenceError::backend(format!("failed to decode account xpub: {}", e))
    })?;
if consumed != xpub_bytes.len() {
    return Err(PersistenceError::backend(
        "failed to decode account xpub: trailing bytes present".to_string(),
    ));
}

suggestion: Carried-forward (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes

packages/rs-platform-wallet-ffi/src/persistence.rs (line 2956)

Verified at head 0466241 — persistence.rs:2956-2970 unchanged. The tracked asset-lock restore path calls dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard()) and discards the consumed count (2959 binds it to _). Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid AssetLockProof encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map. Enforce consumed == proof_bytes.len() to match the rest of the restore-boundary policy.

let proof_bytes =
    unsafe { slice::from_raw_parts(spec.proof_bytes, spec.proof_bytes_len) };
let (proof, consumed) = dpp::bincode::decode_from_slice::<dpp::prelude::AssetLockProof, _>(
    proof_bytes,
    config::standard(),
)
.map_err(|e| {
    PersistenceError::backend(format!(
        "tracked asset lock: failed to decode proof: {}",
        e
    ))
})?;
if consumed != proof_bytes.len() {
    return Err(PersistenceError::backend(
        "tracked asset lock: failed to decode proof: trailing bytes present".to_string(),
    ));
}
Some(proof)

suggestion: Carried-forward (STILL VALID): unified contacts row carries a mutually-exclusive `state` label even when both pending blobs are present

packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs (line 108)

Verified at head 0466241 — schema/contacts.rs apply() and reader unchanged. The CMT-003 rewrite collapses sent/received/established into one row per (wallet_id, owner_id, contact_id) with a single state label and two nullable request blobs. The pending upserts in apply() (113-172) intentionally leave the other blob untouched on conflict — only the matching blob + state are overwritten by the ON CONFLICT DO UPDATE clause. If a sent upsert lands first and an incoming upsert for the same key arrives before establishment, the row ends up with state='received', incoming_request=Y, outgoing_request=X (still there from the prior upsert). The reader at contacts.rs:243-300 then buckets exclusively by state and emits only the incoming_requests entry, silently dropping the previously persisted outgoing pending half. DashPay's mutual contact-request flow (Alice→Bob, Bob→Alice) is a documented real scenario (tests/sqlite_load_reconstruction.rs:559-576). Before the production loader lands under CMT-011, decide whether the schema should (a) carry a richer state (e.g. add a mutual-pending label so bucketing is unambiguous), (b) have the reader consult blob presence in addition to state, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the post-second-upsert state so the behaviour stops being silently untested.

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

- [BLOCKING] In `packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:295-357: Carried-forward (STILL VALID): meta_contact / meta_platform_address rows can outlive their wallet; delete_wallet over-reports cleanup
  Verified at head 04662411 — V001__initial.rs:295-357 is unchanged. `meta_contact` (295-303) and `meta_platform_address` (305-312) carry `wallet_id` but have no FK to `wallet_metadata`. Cleanup relies on `cascade_meta_contact_on_contact_delete` (340-349, gated `WHEN OLD.state = 'established'`) and `cascade_meta_platform_address_on_address_delete` (351-357), which fire only on deletes from `contacts` / `platform_addresses`. Since `KvStore::put` on `ObjectId::Contact` / `ObjectId::PlatformAddress` explicitly supports parentless writes, three classes of `meta_contact` rows can survive contact deletion AND whole-wallet deletion: (a) parentless rows written before any contact exists, (b) rows whose only matching `contacts` row is in `sent`/`received` state (the `WHEN OLD.state = 'established'` filter skips them), and (c) rows for contacts removed via `remove_sent_contact_request` before reaching `established`. Same gap applies to `meta_platform_address` for parentless metadata. Compounding this, `delete_wallet` in `src/sqlite/persister.rs:489-505` pre-counts `PER_WALLET_TABLES` (still including both meta tables) before deleting `wallet_metadata`. Because the cascade-by-trigger flow leaves parentless / pending-state rows behind, those counts become an upper bound — `DeleteWalletReport` over-reports cleanup on the metadata-only / pending-contact paths, violating the API/documentation contract ('metadata never outlives its object'). Fix shapes: add `AFTER DELETE ON wallet_metadata` triggers that sweep both meta tables by `wallet_id` and drop the `WHEN OLD.state = 'established'` filter (or replace with a wallet-scoped sweep); or issue explicit `DELETE FROM meta_contact / meta_platform_address WHERE wallet_id = ?1` in `delete_wallet` before deleting `wallet_metadata` and have the report use post-delete `Connection::changes()` instead of a pre-delete count.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:405-415: Carried-forward (STILL VALID): delete_wallet pre-flight rejects wallets whose only durable state is parentless metadata
  Verified at head 04662411 — persister.rs:405-415 unchanged. `delete_wallet` probes only `wallet_metadata` to decide whether a wallet exists (with an in-memory escape hatch via `had_buffered`). A wallet whose only durable rows are the intentionally parentless `meta_wallet` / `meta_contact` / `meta_platform_address` entries (a supported pattern through `KvStore::put`) is rejected with `WalletNotFound` even though state exists on disk. Combined with the cascade gap above, the parentless-metadata path is both undeletable here AND silently retained when a `wallet_metadata` row eventually arrives. Buffer-only callers escape via `had_buffered`; persisted-metadata-only callers do not. Widen the existence check to include the per-wallet metadata-bearing tables so the API is internally consistent with the writers it accepts.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/kv.rs`:186-215: Carried-forward (STILL VALID): KvStore::put accepts values that KvStore::get will permanently refuse
  Verified at head 04662411 — kv.rs:186-215 unchanged. `get` enforces `length(value) <= MAX_VALUE_LEN` and returns `KvError::ValueTooLarge { found, max }` (kv.rs:162-183). `put` (kv.rs:186-215) has no symmetric guard: it executes the INSERT/UPSERT with the raw `value: &[u8]` regardless of length. A `put` of an over-cap blob silently succeeds and writes a row that is permanently unreadable through the public API — the same API breaks its own read/write contract on values it accepted. Add a symmetric guard at the top of `put`, before any SQL is built or run, so callers see the size-cap violation on the write that caused it rather than on every subsequent read.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2266-2271: Carried-forward (STILL VALID): FFI account-xpub restore silently accepts trailing bytes
  Verified at head 04662411 — persistence.rs:2266-2271 unchanged. `build_wallet_start_state` decodes `account_xpub_bytes` via `bincode::decode_from_slice(...)` and discards the consumed-bytes count (2268 binds it to `_`). A Swift→Rust restore payload whose prefix is a valid `ExtendedPubKey` encoding followed by garbage is silently accepted as canonical. This is inconsistent with the storage crate's hardened `blob::decode`, which rejects trailing bytes (CMT-003 / bed413e513). Enforce `consumed == xpub_bytes.len()` and surface a typed mismatch error to match the rest of the restore-boundary policy.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:2956-2970: Carried-forward (STILL VALID): FFI asset-lock proof restore silently accepts trailing bytes
  Verified at head 04662411 — persistence.rs:2956-2970 unchanged. The tracked asset-lock restore path calls `dpp::bincode::decode_from_slice::<AssetLockProof, _>(proof_bytes, config::standard())` and discards the consumed count (2959 binds it to `_`). Same trailing-byte hazard as the xpub finding, on the AssetLockProof rehydration path: a malformed proof blob whose prefix is a valid `AssetLockProof` encoding is accepted as canonical and re-inserted into the in-memory tracked-locks map. Enforce `consumed == proof_bytes.len()` to match the rest of the restore-boundary policy.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:108-223: Carried-forward (STILL VALID): unified contacts row carries a mutually-exclusive `state` label even when both pending blobs are present
  Verified at head 04662411 — schema/contacts.rs apply() and reader unchanged. The CMT-003 rewrite collapses sent/received/established into one row per `(wallet_id, owner_id, contact_id)` with a single `state` label and two nullable request blobs. The pending upserts in `apply()` (113-172) intentionally leave the other blob untouched on conflict — only the matching blob + `state` are overwritten by the `ON CONFLICT DO UPDATE` clause. If a `sent` upsert lands first and an `incoming` upsert for the same key arrives before establishment, the row ends up with `state='received'`, `incoming_request=Y`, `outgoing_request=X` (still there from the prior upsert). The reader at contacts.rs:243-300 then buckets exclusively by `state` and emits only the `incoming_requests` entry, silently dropping the previously persisted outgoing pending half. DashPay's mutual contact-request flow (Alice→Bob, Bob→Alice) is a documented real scenario (tests/sqlite_load_reconstruction.rs:559-576). Before the production loader lands under CMT-011, decide whether the schema should (a) carry a richer state (e.g. add a `mutual-pending` label so bucketing is unambiguous), (b) have the reader consult blob presence in addition to `state`, or (c) null-out the opposite blob in the pending upserts (safe because establishment is monotonic and writes both blobs explicitly). Add an explicit assertion in the existing mutual-pending test against the post-second-upsert state so the behaviour stops being silently untested.