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

[Claudius-Maginificent]

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 — 18 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

None. New crate, opt-in. Existing NoPlatformPersistence remains in place and is unchanged. The new platform-wallet/serde feature is 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 if my code contains any (N/A — no breaking changes)
• 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 wallet storage with migrations, integrity checks, online backup/restore, auto-backup retention and pruning, and configurable buffering/flush modes
  • CLI for database maintenance (migrate, backup, restore, prune, inspect)

• Documentation

  • Added README and secrets guidance for the wallet storage component

• Tests

  • Large integration and unit test suite covering persistence, backup/restore, foreign-key integrity, error classification, and CLI smoke tests

• Chores

  • CI/workspace and Docker build updated to include the new storage package

[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 Rust crate providing SQLite-backed wallet storage with embedded migrations, buffered writes, online backup/restore and retention, per-area schema writers/readers, a maintenance CLI, comprehensive integration/unit tests, workspace/CI/Docker wiring, and optional serde support in platform-wallet changeset types.

Changes

SQLite Wallet Storage

Layer / File(s)	Summary
All changes (review checkpoint) packages/rs-platform-wallet-storage/..., packages/rs-platform-wallet/..., .github/workflows/*, Cargo.toml, Dockerfile	New SQLite wallet-storage crate (migrations, persister, backup/restore, CLI, schema writers/readers, utils, tests) plus workspace/CI/Docker wiring and serde feature additions in platform-wallet.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI
  participant SqlitePersister
  participant SQLite
  participant FS as Backups FS
  User->>CLI: run subcommand (migrate/backup/restore/prune/inspect)
  CLI->>SqlitePersister: open/config (PRAGMAs, auto-backup dir)
  SqlitePersister->>SQLite: migrate/apply/inspect
  SqlitePersister->>FS: backup/restore/prune files
  SqlitePersister-->>CLI: result/rows/backup path
  CLI-->>User: exit code + stdout/stderr

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Suggested labels

ready for final review

Suggested reviewers

• shumkov
• QuantumExplorer

Poem

A rabbit with pockets of bytes and keys,
Stitches a burrow in SQLite trees.
It backs up the moon, restores the dawn,
Prunes old shadows from the meadow lawn.
With serde carrots and tests that sing,
Wallets hop safely—flush, and spring! 🥕✨

✨ 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}

[codecov]

Codecov Report

❌ Patch coverage is 75.92138% with 588 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.07%. Comparing base (12fd1a3) to head (b4ffcbc).
⚠️ Report is 1 commits behind head on v3.1-dev.

Files with missing lines	Patch %	Lines
...-wallet-storage/src/bin/platform-wallet-storage.rs	55.65%	102 Missing ⚠️
...rs-platform-wallet-storage/src/sqlite/persister.rs	81.31%	88 Missing ⚠️
...tform-wallet-storage/src/sqlite/schema/contacts.rs	50.70%	70 Missing ⚠️
...tform-wallet-storage/src/sqlite/schema/accounts.rs	33.66%	67 Missing ⚠️
...orm-wallet-storage/src/sqlite/schema/core_state.rs	73.39%	62 Missing ⚠️
...es/rs-platform-wallet-storage/src/sqlite/backup.rs	83.62%	37 Missing ⚠️
...orm-wallet-storage/src/sqlite/schema/identities.rs	79.54%	27 Missing ⚠️
...wallet-storage/src/sqlite/schema/platform_addrs.rs	84.93%	25 Missing ⚠️
...rm-wallet-storage/src/sqlite/schema/wallet_meta.rs	72.36%	21 Missing ⚠️
...rm-wallet-storage/src/sqlite/schema/asset_locks.rs	85.08%	17 Missing ⚠️
... and 9 more

Additional details and impacted files

@@             Coverage Diff              @@
##           v3.1-dev    #3625      +/-   ##
============================================
- Coverage     87.16%   87.07%   -0.10%     
============================================
  Files          2607     2628      +21     
  Lines        319420   321663    +2243     
============================================
+ Hits         278413   280073    +1660     
- Misses        41007    41590     +583     

Components	Coverage Δ
dpp	87.67% <ø> (-0.01%)	⬇️
drive	85.95% <ø> (ø)
drive-abci	89.60% <ø> (ø)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.20% <ø> (+0.03%)	⬆️
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	49.16% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

Code Review

New SQLite-backed wallet persister crate. Code is generally careful (safe_cast discipline, chmod 600, staged-file integrity rechecks), but several validated issues remain: two security DoS vectors at the untrusted-backup boundary (unbounded staging copy and unbounded blob decode), restore ordering that deletes WAL/SHM before the staged recheck, a handful of silent as u32 truncations that bypass the crate's own safe_cast discipline, and a few reader paths that trust blobs over typed columns. No blocking consensus or FFI issues. Five additional lower-value nitpicks were dropped to fit the 10-comment budget.

🟡 9 suggestion(s) | 💬 1 nitpick(s)

Note: Inline review_poster failed (Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, head_sha, verified, dry_run=dry_run)
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 107, in post_review
r), so I posted the same verified findings as a top-level review body.

suggestion: `restore_from` stages the full source into the wallet directory before checking it is a wallet backup

packages/rs-platform-wallet-storage/src/sqlite/backup.rs:63-175

The source integrity_check runs on the original handle (line 73), then std::io::copy streams the entire source into a NamedTempFile next to the destination (line 119), and only then does the schema-history / max-version gate run on the staged copy (lines 139-170). An attacker who can supply a backup file to the CLI or any FFI/UI wrapper can pass a valid-but-huge SQLite database; this code will fill the wallet directory's filesystem with it before rejecting it as SchemaHistoryMissing or SchemaVersionUnsupported. The schema/version gate should run against the source handle first (cheap query, no disk cost), keeping the staged recheck only as the TOCTOU-safe final gate.

_{source: ['codex']}

suggestion: Blob decoder has no size cap — restored rows can drive arbitrary allocations

packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs:29-37

blob::decode calls bincode::serde::decode_from_slice(..., bincode::config::standard()) with no length limit. The trailing-bytes guard catches truncation but does not bound allocations during decode. Since restored backups are an explicit trust boundary, a hostile backup can craft core_transactions.record_blob (or any other blob column) so that a normal read (e.g. core_state::get_tx_record, asset-lock proof lookup) materializes huge Vecs and crashes the wallet on every open. Configure a conservative byte limit via bincode::config::standard().with_limit::<N>() and consider validating per-column blob sizes at load time.

_{source: ['codex']}

suggestion: WAL/SHM siblings deleted before staged copy passes integrity + schema recheck

packages/rs-platform-wallet-storage/src/sqlite/backup.rs:99-171

Step 3 unlinks <dest>-wal and <dest>-shm (lines 101-112), step 4 stages the source into a temp file, and step 5 re-runs PRAGMA integrity_check plus schema-history checks on the staged file. If the recheck fails, the staged temp file drops naturally and the live destination main DB is untouched — but its WAL/SHM are already gone. For a WAL-mode database with un-checkpointed pages, that loses durable state without any persist actually happening. Reorder so the staged file is fully validated first, and only delete WAL/SHM in the small window immediately before tmp.persist(dest_db_path).

_{source: ['claude']}

suggestion: `core_sync_state` reload uses `as u32`, bypassing the crate's safe_cast discipline

packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:160-164

upsert_sync_state reads existing last_processed_height and synced_height as Option<i64> and converts via x as u32, which silently truncates >u32::MAX and wraps negatives. Every other reader in the crate routes the i64→u32 step through safe_cast::i64_to_u64 + u32::try_from(...).map_err(IntegerOverflow) (see core_utxos.height, asset_locks.account_index, platform_addrs). The monotonic-max merge then feeds the wrapped value back into the row, turning a one-time corruption into a permanently-stuck (or wildly advanced) sync checkpoint. Use the same checked conversion here.

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

suggestion: `wallet_metadata.fetch` truncates `birth_height` via `as u32`

packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs:67-70

fetch reads birth_height as i64 and returns height as u32, silently wrapping out-of-range or negative values. Same durable-boundary invariant as core_sync_state — the writer takes entry.birth_height (currently u32) directly, but if the upstream type widens, or a row is touched externally, the reader will accept nonsense without surfacing IntegerOverflow. Use u32::try_from(height).map_err(|_| WalletStorageError::IntegerOverflow { field: "wallet_metadata.birth_height", .. }) for consistency with the rest of the crate.

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

suggestion: Auto-backup filenames collide on a one-second boundary and silently overwrite

packages/rs-platform-wallet-storage/src/sqlite/backup.rs:24-38

utc_timestamp() formats %Y%m%dT%H%M%SZ with second precision. manual_backup_filename() (wallet-<ts>.db) and auto_backup_filename(PreRestore) (pre-restore-<ts>.db) carry no other disambiguator. Two backups in the same second collide, and run_to opens the destination via Connection::open(dest) + Backup::new(...), which clobbers existing pages with the new source. The prior backup is lost without any error. (PreDelete/PreMigration happen to be uniquely keyed by wallet_id/version.) Either append a pid + nanosecond suffix, or open the file with OpenOptions::new().create_new(true) (or SQLITE_OPEN_EXCLUSIVE) and surface a typed BackupFileExists error.

_{source: ['claude']}

suggestion: `list_active` is byte-for-byte identical to `load_state` — no status filter

packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:143-164

The doc comment says: "Return non-Used asset locks per wallet … consumed locks leave via AssetLockChangeSet::removed." But the SQL has no WHERE status … clause and the function body is identical to load_state (lines 114-135). Either the docs are misleading and one of the two should be deleted (routing callers to the other), or the intended status != 'consumed' filter was dropped and needs to be re-added with a test that distinguishes the two. As written, list_active is dead code that duplicates surface area without adding behaviour.

_{source: ['claude']}

suggestion: Asset-lock reader never cross-checks typed columns against the lifecycle blob

packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:85-109

Each row stores (outpoint, account_index) twice — once in indexed columns and once inside lifecycle_blob (AssetLockEntry::out_point, account_index). decode_row decodes both but never verifies entry.out_point == outpoint or entry.account_index == account_index. A torn write, a partial migration, or a manually edited row will silently load into the wrong account bucket while the in-memory TrackedAssetLock reports a different account index. The module docstring promises fail-hard on corruption; add an equality check that returns WalletStorageError::BlobDecode when the typed columns disagree with the blob.

_{source: ['codex']}

suggestion: `identities.load_state` reads `identity_id` column then discards it

packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:90-99

The SELECT lists identity_id, entry_blob, tombstoned, but the column is bound as _identity_id and never used — everything is reconstructed from entry_blob. The typed column is duplicated for indexed lookups; if it diverges from the blob (torn write, partial migration), the reload silently loads an IdentityEntry under a key that disagrees with the indexed column. Either drop the column from the SELECT entirely (making the divergence impossible to silently mask), or compare identity_id.as_slice() == entry.id.as_slice() and fail hard when they disagree.

_{source: ['codex']}

nitpick: `identity_keys.derivation_blob` is always written NULL — dead schema column

packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:67-108

The apply UPSERT writes derivation_blob = NULL on insert (line 76) and derivation_blob = NULL on conflict (line 80). Derivation indices are instead serialized inside public_key_blob via IdentityKeyWire::derivation_indices. The dedicated column declared in V001 is therefore unreachable schema surface — readers never query it, writers never set it. Either (a) split derivation_indices out of the wire blob and populate the column so it can be queried/indexed without decoding, or (b) document the abandonment inline (and consider dropping it in a future migration). The current shape will mislead anyone inspecting the schema.

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

[lklimek]

we don't want to have delete wallet in the cli

[Claudius-Maginificent]

Done in 36577d2884 — the delete-wallet subcommand is removed from the CLI (Cmd enum no longer has the variant); its old test was repurposed to assert the subcommand is gone. Leaving open for your review.

[lklimek]

These barrel limitations are a show-stoppers. We should ensure PRAGMA
// foreign_keys = ON is always set (check during startup + use some feature flags for sqlite that enforece it), and if barrel doesn't correctly support that - drop barrel and write schema sql manually. We don't want to use triggers instead of native constraint only because barrel is limiting ourselves

[Claudius-Maginificent]

Done in b5a8439932 — barrel dropped entirely, schema rewritten with native composite FKs via refinery (migrations/V001__initial.rs). PRAGMA foreign_keys=ON is enforced and read back at every read-write open through the open_conn choke-point (src/sqlite/conn.rs), raising ForeignKeysNotEnforced if it doesn't stick. Note: the prescribed composite ON DELETE SET NULL FK for core_utxos→core_transactions is unimplementable in SQLite (SET NULL nulls all composite-FK columns, tripping the NOT NULL on wallet_id), so that one relationship uses a single-column AFTER-DELETE trigger nulling only spent_in_txid; everything else is native FK. Leaving open for your review.

[lklimek]

Merge this migration into v001, we didn't release yet, no need to split this.

[Claudius-Maginificent]

Done in b5a8439932 — V002 collapsed into V001; there is now a single migration. Leaving open for your review.

[thepastaclaw]

Code Review

Cumulative incremental review at f78f2e6. The latest delta (689a61e..f78f2e6) only gates a test-only contacts reader behind __test-helpers and adds serde derives to ShieldedChangeSet/SubwalletId/ShieldedNote — no new defects in the delta itself. Prior findings: CMT-001 (delete_wallet buffer drain), CMT-002 (staged restore validation), CMT-003 (blob trailing-byte rejection), CMT-004 (root-safe tc054), and the shielded-serde / contacts-gating prior findings are all FIXED at current head. Carried-forward STILL-VALID cumulative findings dominate the report: a logic bug in load() that drops wallets whose only persisted sync field is last_known_recent_block (convergent claude+codex), a missing forward-schema-version gate in open() that's already present in restore_from, an unbounded bincode decode on attacker-influenceable blobs, a concurrent-store race during delete_wallet, WAL/SHM removed before staged validation, and several minor cumulative issues. Overflow: 4 verified findings (wallet_metadata.fetch truncation, IdentityKeyWire trailing-bytes uniformity gap, identity_keys.derivation_blob dead column, identities.load_state discards identity_id) trimmed by 10-comment budget; rolled into related findings or noted here.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 f78f2e6
STDOUT:

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

Reviewed commit: f78f2e6

🟡 7 suggestion(s) | 💬 3 nitpick(s)

Verified findings

suggestion: Carried-forward: load() drops wallets whose only persisted sync watermark is last_known_recent_block

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

The reconstruction predicate count > 0 || addrs.sync_height > 0 || addrs.sync_timestamp > 0 ignores the third watermark, addrs.last_known_recent_block. The writer in platform_addrs::apply upserts the platform_address_sync row whenever any of sync_height, sync_timestamp, or last_known_recent_block is Some(_), so it is legitimately possible to persist a row whose only non-zero field is last_known_recent_block. Such a wallet is silently dropped from the rehydrated start state and resumes from a colder checkpoint than was actually persisted. Either include the third field in the predicate or rely on wallet_meta::list_ids (already the source of addrs_all) to gate which wallets surface — the count is tracked separately via tracing.

        for (wallet_id, (addrs, count)) in addrs_all {
            if count > 0
                || addrs.sync_height > 0
                || addrs.sync_timestamp > 0
                || addrs.last_known_recent_block > 0
            {
                addresses_loaded += count;
                state.platform_addresses.insert(wallet_id, addrs);
            }
        }

suggestion: Carried-forward: open() trusts forward-schema databases that restore_from would reject

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

restore_from validates MAX(version) <= max_supported on the staged file (backup.rs:150-170), but open() performs no equivalent check. It probes for refinery_schema_history, counts pending migrations, optionally takes a pre-migration backup, and runs refinery::run — but never compares the on-disk max version against the embedded migration set. If a DB written by a newer release is on disk with all currently-known migrations already present, pending_count is zero, refinery is a no-op, and the older code opens it and proceeds to decode blob columns shaped for a newer schema. Since the crate intentionally dropped per-blob revision tags and ties blob compatibility to migration version, this is a defense-in-depth gap on the direct-open trust boundary that already exists on the restore path. Add the same SchemaVersionUnsupported gate to open().

suggestion: Carried-forward: blob::decode has no size limit — restored backups can drive arbitrary allocations

packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs (line 29)

decode calls bincode::serde::decode_from_slice(blob, bincode::config::standard()) with no length cap. bincode::config::standard() is NoLimit, so a corrupted or hostile row whose length prefixes claim multi-exabyte allocations will cause decode_from_slice to attempt the allocation and OOM-kill the wallet host. Threat model: the live DB is owner-only (mode 0o600), but restore_from explicitly accepts user-supplied .db files after only integrity_check and schema-history validation. A crafted backup that passes restore can force OOM on the next load(), get_core_tx_record, contacts read, asset-lock read, etc. The CMT-003 trailing-byte fix doesn't help here. Configure a conservative cap via bincode::config::standard().with_limit::<N>() (e.g. 16 MiB — well above any legitimate row) and/or pre-validate per-column blob sizes at the SQL boundary.

suggestion: Carried-forward: delete_wallet releases the connection mutex between drain, existence check, and delete transaction

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

delete_wallet_inner drains the buffer (line 292), then opens a fresh connection scope for the existence check (lines 297-310), then a second scope for the auto-backup (lines 311-321), then a third call to self.conn()? to take the delete transaction (line 322). The persister is Arc<dyn PlatformWalletPersistence> and explicitly Send + Sync, so another thread can call store(wallet_id, cs) between the drain and the DELETE commit. The racing store() repopulates the buffer for wallet_id; in FlushMode::Immediate it also writes directly to the same DB. Result: a delete_wallet returns success but the per-wallet rows can reappear (from the racing immediate flush) or be persisted by a subsequent commit_writes, silently undoing the operator's deletion. Hold the connection mutex (or a per-wallet flush-suppression lock) across the entire drain → existence-check → delete-transaction sequence so concurrent stores either block or fail loudly.

suggestion: Carried-forward: WAL/SHM siblings deleted before the staged file passes integrity + schema recheck

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 99)

Step 3 (lines 101-112) unconditionally unlinks <dest>-wal and <dest>-shm. Steps 4-5 then stage the source and re-validate (lines 117-171). If validation fails on the staged copy (integrity_check failure, missing schema_history, or SchemaVersionUnsupported), the staged temp file drops cleanly and the live destination main DB is untouched — but its WAL/SHM are already gone. For a WAL-mode database with un-checkpointed pages, that loses durable state without any persist having occurred. Reorder so that WAL/SHM deletion happens immediately before tmp.persist(dest_db_path) at line 174, after the staged file has passed all checks.

nitpick: Carried-forward: restore_from streams the entire source via std::io::copy before its schema/version gate

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 63)

The cheap pre-staging step 1 only runs PRAGMA integrity_check on the source. The schema-history / max-version gate (lines 139-170) runs only on the staged copy, after std::io::copy(&mut src_file, tmp.as_file_mut()) at line 119 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI or any FFI/UI consumer) fills the wallet directory's filesystem with the staged copy before being rejected as SchemaHistoryMissing / SchemaVersionUnsupported. The TOCTOU-safe final gate on the staged file should stay, but a cheap schema-history + max-version sniff against the first source handle would fail fast and avoid the staged-copy DoS. Keep the staged recheck as the authoritative gate.

suggestion: Carried-forward: upsert_sync_state truncates i64 via `as u32`, bypassing the crate's safe_cast discipline

packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs (line 161)

Existing last_processed_height / synced_height are read as Option<i64> and converted via x as u32 on line 163, silently truncating values >u32::MAX and wrapping negatives. Every other reader in this crate routes the i64→u32 step through safe_cast::* + u32::try_from(...).map_err(IntegerOverflow) (see core_utxos.height and asset_locks.account_index at lines 102-107 and 244-256 of this file). The monotonic-max merge on lines 168-175 then feeds the wrapped value back into the row, turning a one-time corruption into a permanently-stuck or wildly-advanced sync checkpoint. Use checked conversion and surface WalletStorageError::IntegerOverflow. The same pattern applies to wallet_meta::fetch at line 70 (height as u32) — fix consistently.

suggestion: Carried-forward: decode_row never cross-checks typed columns against the lifecycle blob

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

Each asset-lock row stores (outpoint, account_index) twice — once in indexed columns and once inside lifecycle_blob (AssetLockEntry::out_point, account_index). decode_row decodes outpoint from op_bytes (line 90), then constructs TrackedAssetLock using entry.out_point and entry.account_index from the blob (lines 93, 95), and never verifies entry.out_point == outpoint or entry.account_index == account_index against the typed column it converted on lines 102-107. A torn write, partial migration, or manual edit will silently load into the wrong account bucket while the TrackedAssetLock reports a different outpoint/account than the indexed column the caller used to find it. The module's fail-hard-on-corruption invariant warrants an explicit equality check returning WalletStorageError::BlobDecode when typed columns disagree with the blob.

nitpick: Carried-forward: list_active is byte-for-byte identical to load_state — no status filter

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

The docstring at lines 137-142 says list_active returns 'non-Used asset locks per wallet … consumed locks leave via AssetLockChangeSet::removed'. But the SQL at lines 147-150 has no WHERE status … clause and the body at 151-162 is identical to load_state (lines 122-133); status_str doesn't even define a Used variant. The two functions are dead-code duplicates referenced only by separate integration tests. Either delete one and route callers to the survivor, or — if the filter was intentional and dropped — restore the WHERE status NOT IN ('consumed') clause and add a test that distinguishes the two readers. As written, list_active is dead surface area.

nitpick: Carried-forward: auto-backup filenames collide on a one-second boundary and silently overwrite

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 24)

utc_timestamp() formats with %Y%m%dT%H%M%SZ (second precision). manual_backup_filename() (wallet-<ts>.db) and auto_backup_filename(BackupKind::PreRestore) (pre-restore-<ts>.db) carry no other disambiguator. Two backups in the same second collide, and run_to opens the destination via Connection::open(dest) (line 49) + Backup::new(...) (line 53), which clobbers existing pages with the new source. The prior backup is lost without any error. PreDelete is insulated by wallet_id and PreMigration by the from/to pair, so the collision surface is wallet-* (rapid manual backups) and pre-restore-* (e.g. retry-after-failure during a restore). Either append a pid + nanosecond suffix or open the destination with OpenOptions::new().create_new(true) and surface a typed BackupFileExists error.

🤖 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/persister.rs`:673-678: Carried-forward: load() drops wallets whose only persisted sync watermark is last_known_recent_block
  The reconstruction predicate `count > 0 || addrs.sync_height > 0 || addrs.sync_timestamp > 0` ignores the third watermark, `addrs.last_known_recent_block`. The writer in `platform_addrs::apply` upserts the `platform_address_sync` row whenever any of `sync_height`, `sync_timestamp`, or `last_known_recent_block` is `Some(_)`, so it is legitimately possible to persist a row whose only non-zero field is `last_known_recent_block`. Such a wallet is silently dropped from the rehydrated start state and resumes from a colder checkpoint than was actually persisted. Either include the third field in the predicate or rely on `wallet_meta::list_ids` (already the source of `addrs_all`) to gate which wallets surface — the count is tracked separately via tracing.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:128-155: Carried-forward: open() trusts forward-schema databases that restore_from would reject
  `restore_from` validates `MAX(version) <= max_supported` on the staged file (backup.rs:150-170), but `open()` performs no equivalent check. It probes for `refinery_schema_history`, counts pending migrations, optionally takes a pre-migration backup, and runs `refinery::run` — but never compares the on-disk max version against the embedded migration set. If a DB written by a newer release is on disk with all currently-known migrations already present, `pending_count` is zero, refinery is a no-op, and the older code opens it and proceeds to decode blob columns shaped for a newer schema. Since the crate intentionally dropped per-blob revision tags and ties blob compatibility to migration version, this is a defense-in-depth gap on the direct-open trust boundary that already exists on the restore path. Add the same `SchemaVersionUnsupported` gate to `open()`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:29-37: Carried-forward: blob::decode has no size limit — restored backups can drive arbitrary allocations
  `decode` calls `bincode::serde::decode_from_slice(blob, bincode::config::standard())` with no length cap. `bincode::config::standard()` is `NoLimit`, so a corrupted or hostile row whose length prefixes claim multi-exabyte allocations will cause `decode_from_slice` to attempt the allocation and OOM-kill the wallet host. Threat model: the live DB is owner-only (mode 0o600), but `restore_from` explicitly accepts user-supplied `.db` files after only `integrity_check` and schema-history validation. A crafted backup that passes restore can force OOM on the next `load()`, `get_core_tx_record`, contacts read, asset-lock read, etc. The CMT-003 trailing-byte fix doesn't help here. Configure a conservative cap via `bincode::config::standard().with_limit::<N>()` (e.g. 16 MiB — well above any legitimate row) and/or pre-validate per-column blob sizes at the SQL boundary.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:283-346: Carried-forward: delete_wallet releases the connection mutex between drain, existence check, and delete transaction
  `delete_wallet_inner` drains the buffer (line 292), then opens a fresh connection scope for the existence check (lines 297-310), then a second scope for the auto-backup (lines 311-321), then a third call to `self.conn()?` to take the delete transaction (line 322). The persister is `Arc<dyn PlatformWalletPersistence>` and explicitly `Send + Sync`, so another thread can call `store(wallet_id, cs)` between the drain and the DELETE commit. The racing `store()` repopulates the buffer for `wallet_id`; in `FlushMode::Immediate` it also writes directly to the same DB. Result: a `delete_wallet` returns success but the per-wallet rows can reappear (from the racing immediate flush) or be persisted by a subsequent `commit_writes`, silently undoing the operator's deletion. Hold the connection mutex (or a per-wallet flush-suppression lock) across the entire drain → existence-check → delete-transaction sequence so concurrent stores either block or fail loudly.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:99-175: Carried-forward: WAL/SHM siblings deleted before the staged file passes integrity + schema recheck
  Step 3 (lines 101-112) unconditionally unlinks `<dest>-wal` and `<dest>-shm`. Steps 4-5 then stage the source and re-validate (lines 117-171). If validation fails on the staged copy (integrity_check failure, missing schema_history, or `SchemaVersionUnsupported`), the staged temp file drops cleanly and the live destination main DB is untouched — but its WAL/SHM are already gone. For a WAL-mode database with un-checkpointed pages, that loses durable state without any persist having occurred. Reorder so that WAL/SHM deletion happens immediately before `tmp.persist(dest_db_path)` at line 174, after the staged file has passed all checks.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:63-175: Carried-forward: restore_from streams the entire source via std::io::copy before its schema/version gate
  The cheap pre-staging step 1 only runs `PRAGMA integrity_check` on the source. The schema-history / max-version gate (lines 139-170) runs only on the staged copy, after `std::io::copy(&mut src_file, tmp.as_file_mut())` at line 119 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI or any FFI/UI consumer) fills the wallet directory's filesystem with the staged copy before being rejected as `SchemaHistoryMissing` / `SchemaVersionUnsupported`. The TOCTOU-safe final gate on the staged file should stay, but a cheap schema-history + max-version sniff against the first source handle would fail fast and avoid the staged-copy DoS. Keep the staged recheck as the authoritative gate.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:161-164: Carried-forward: upsert_sync_state truncates i64 via `as u32`, bypassing the crate's safe_cast discipline
  Existing `last_processed_height` / `synced_height` are read as `Option<i64>` and converted via `x as u32` on line 163, silently truncating values >u32::MAX and wrapping negatives. Every other reader in this crate routes the i64→u32 step through `safe_cast::*` + `u32::try_from(...).map_err(IntegerOverflow)` (see `core_utxos.height` and `asset_locks.account_index` at lines 102-107 and 244-256 of this file). The monotonic-max merge on lines 168-175 then feeds the wrapped value back into the row, turning a one-time corruption into a permanently-stuck or wildly-advanced sync checkpoint. Use checked conversion and surface `WalletStorageError::IntegerOverflow`. The same pattern applies to `wallet_meta::fetch` at line 70 (`height as u32`) — fix consistently.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:85-109: Carried-forward: decode_row never cross-checks typed columns against the lifecycle blob
  Each asset-lock row stores `(outpoint, account_index)` twice — once in indexed columns and once inside `lifecycle_blob` (`AssetLockEntry::out_point`, `account_index`). `decode_row` decodes `outpoint` from `op_bytes` (line 90), then constructs `TrackedAssetLock` using `entry.out_point` and `entry.account_index` from the blob (lines 93, 95), and never verifies `entry.out_point == outpoint` or `entry.account_index == account_index` against the typed column it converted on lines 102-107. A torn write, partial migration, or manual edit will silently load into the wrong account bucket while the `TrackedAssetLock` reports a different outpoint/account than the indexed column the caller used to find it. The module's fail-hard-on-corruption invariant warrants an explicit equality check returning `WalletStorageError::BlobDecode` when typed columns disagree with the blob.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:143-164: Carried-forward: list_active is byte-for-byte identical to load_state — no status filter
  The docstring at lines 137-142 says `list_active` returns 'non-`Used` asset locks per wallet … consumed locks leave via `AssetLockChangeSet::removed`'. But the SQL at lines 147-150 has no `WHERE status …` clause and the body at 151-162 is identical to `load_state` (lines 122-133); `status_str` doesn't even define a `Used` variant. The two functions are dead-code duplicates referenced only by separate integration tests. Either delete one and route callers to the survivor, or — if the filter was intentional and dropped — restore the `WHERE status NOT IN ('consumed')` clause and add a test that distinguishes the two readers. As written, `list_active` is dead surface area.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:24-38: Carried-forward: auto-backup filenames collide on a one-second boundary and silently overwrite
  `utc_timestamp()` formats with `%Y%m%dT%H%M%SZ` (second precision). `manual_backup_filename()` (`wallet-<ts>.db`) and `auto_backup_filename(BackupKind::PreRestore)` (`pre-restore-<ts>.db`) carry no other disambiguator. Two backups in the same second collide, and `run_to` opens the destination via `Connection::open(dest)` (line 49) + `Backup::new(...)` (line 53), which clobbers existing pages with the new source. The prior backup is lost without any error. `PreDelete` is insulated by wallet_id and `PreMigration` by the from/to pair, so the collision surface is `wallet-*` (rapid manual backups) and `pre-restore-*` (e.g. retry-after-failure during a restore). Either append a pid + nanosecond suffix or open the destination with `OpenOptions::new().create_new(true)` and surface a typed `BackupFileExists` error.

[coderabbitai]

Actionable comments posted: 2

🤖 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/sqlite/backup.rs`:
- Around line 60-81: The file creates the backup file with
OpenOptions::create_new(true) which uses the process umask (0o666 masked) and
leaves a race window before apply_secure_permissions; update the OpenOptions
creation in the run_to (or the block that calls
OpenOptions::new().write(true).create_new(true).open(dest)) to set the file mode
to 0o600 on Unix by calling std::os::unix::fs::OpenOptionsExt::mode(0o0_600)
(guarded by #[cfg(unix)]), add the necessary use import, and keep the existing
apply_secure_permissions(dest)? call as defense-in-depth so the file is created
atomically with secure permissions even if the process crashes before the later
chmod.

In `@packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs`:
- Around line 130-138: The test currently only checks that out.status.success()
is false, which is too broad; change it to explicitly assert the clap exit code
by adding an assertion that out.status.code() == Some(2) (use assert_eq! on
out.status.code(), Some(2)) before you inspect stderr, then proceed to build
stderr from out.stderr and assert it contains "unrecognized" or "unexpected"
using the existing checks on the stderr variable; reference the existing
variables out and stderr in the test so you validate the specific clap
unknown-subcommand exit code.

🪄 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: 76cfd9d2-1318-4bf4-bf55-d7d7ed39820d

📥 Commits

Reviewing files that changed from the base of the PR and between f78f2e6 and df202ea.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (22)

• packages/rs-platform-wallet-storage/Cargo.toml
• packages/rs-platform-wallet-storage/README.md
• 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/sqlite/backup.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/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/core_state.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/wallet_meta.rs
• packages/rs-platform-wallet-storage/tests/common/mod.rs
• packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.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_migrations.rs

💤 Files with no reviewable changes (1)

• packages/rs-platform-wallet-storage/Cargo.toml

✅ Files skipped from review due to trivial changes (1)

• packages/rs-platform-wallet-storage/src/sqlite/mod.rs

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

On Unix, when Rust std::fs::OpenOptions::create_new(true)creates a file withoutOpenOptionsExt::mode(...), what permissions are used, and can OpenOptionsExt::mode(0o600) set the file to 0600 at creation time?

💡 Result:

When Rust’s std::fs::OpenOptions::create_new(true) creates a new file on Unix and you don’t specify OpenOptionsExt::mode(...), Rust uses a default mode of 0o666 for the new file; the OS then applies the process umask to mask those bits, producing the final permissions [1]. So, at creation time the file permissions are effectively: (0o666 & ~umask) (ignoring any default ACL behavior) [2]. Can OpenOptionsExt::mode(0o600) set the file to 0600 at creation time? Yes. OpenOptionsExt::mode sets “the mode bits that a new file will be created with”; if no mode is set, 0o666 is used, but if you do set mode, that value is used as the permission bits for the new file and then umask masks it to the final permissions [1]. Therefore, OpenOptionsExt::mode(0o600) will request 0600 as the base mode; if your umask allows it, the created file will be 0600, otherwise umask may clear some bits (but it cannot add bits) [1][2].

Citations:

• 1: https://doc.rust-lang.org/std/os/unix/fs/trait.OpenOptionsExt.html
• 2: https://man7.org/linux/man-pages/man2/umask.2.html

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate where apply_secure_permissions is defined and how it sets perms.
rg -n "fn apply_secure_permissions|apply_secure_permissions\(" packages/rs-platform-wallet-storage/src/sqlite/backup.rs packages/rs-platform-wallet-storage/src/sqlite -S || true

# Show the relevant section around the new create_new/open call in backup.rs.
sed -n '1,220p' packages/rs-platform-wallet-storage/src/sqlite/backup.rs | nl -ba | sed -n '40,140p' || true

Repository: dashpay/platform

Length of output: 789

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Read apply_secure_permissions implementation
sed -n '1,120p' packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs | cat -n

# 2) Read the targeted area in backup.rs with line numbers
sed -n '1,140p' packages/rs-platform-wallet-storage/src/sqlite/backup.rs | cat -n

# 3) Read the second call site region for context (optional but quick)
sed -n '160,240p' packages/rs-platform-wallet-storage/src/sqlite/backup.rs | cat -n

Repository: dashpay/platform

Length of output: 10991

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "OpenOptionsExt::mode\\(|PermissionsExt::from_mode\\(|chmod\\(0o600\\)|0o600" packages/rs-platform-wallet-storage/src/sqlite -S

Repository: dashpay/platform

Length of output: 322

---

Set 0o600 at destination creation time to avoid an umask-dependent permissions window.

In packages/rs-platform-wallet-storage/src/sqlite/backup.rs run_to, OpenOptions::new().create_new(true).open(dest) creates dest with the default Unix mode (0o666) masked by the process umask until apply_secure_permissions(dest) runs (it sets 0o600 via set_permissions on Unix). If the process crashes/errors between open(dest) and that call, the backup can remain with lax permissions. Set OpenOptionsExt::mode(0o600) on Unix when creating the file, and keep apply_secure_permissions as defense in depth.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 60 -
81, The file creates the backup file with OpenOptions::create_new(true) which
uses the process umask (0o666 masked) and leaves a race window before
apply_secure_permissions; update the OpenOptions creation in the run_to (or the
block that calls OpenOptions::new().write(true).create_new(true).open(dest)) to
set the file mode to 0o600 on Unix by calling
std::os::unix::fs::OpenOptionsExt::mode(0o0_600) (guarded by #[cfg(unix)]), add
the necessary use import, and keep the existing apply_secure_permissions(dest)?
call as defense-in-depth so the file is created atomically with secure
permissions even if the process crashes before the later chmod.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the clap usage exit code explicitly

The test currently accepts any non-success status, so runtime failures unrelated to unknown-subcommand handling could still pass. Please assert Some(2) before checking stderr content.

Proposed patch

-    assert!(
-        !out.status.success(),
-        "delete-wallet should no longer be a recognised subcommand"
-    );
+    assert_eq!(
+        out.status.code(),
+        Some(2),
+        "expected clap usage exit code 2 for removed subcommand; got {:?}",
+        out.status.code()
+    );
     let stderr = String::from_utf8_lossy(&out.stderr);

Based on learnings, Rust test naming in this repo does not require a `should_` prefix.

📝 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

	assert!(
	!out.status.success(),
	"delete-wallet should no longer be a recognised subcommand"
	);
	let stderr = String::from_utf8_lossy(&out.stderr);
	assert!(
	stderr.contains("unrecognized") || stderr.contains("unexpected"),
	"expected clap unknown-subcommand error, got stderr: `{stderr}`"
	);
	assert_eq!(
	out.status.code(),
	Some(2),
	"expected clap usage exit code 2 for removed subcommand; got {:?}",
	out.status.code()
	);
	let stderr = String::from_utf8_lossy(&out.stderr);
	assert!(
	stderr.contains("unrecognized") || stderr.contains("unexpected"),
	"expected clap unknown-subcommand error, got stderr: `{stderr}`"
	);

🤖 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_cli_smoke.rs` around lines
130 - 138, The test currently only checks that out.status.success() is false,
which is too broad; change it to explicitly assert the clap exit code by adding
an assertion that out.status.code() == Some(2) (use assert_eq! on
out.status.code(), Some(2)) before you inspect stderr, then proceed to build
stderr from out.stderr and assert it contains "unrecognized" or "unexpected"
using the existing checks on the stderr variable; reference the existing
variables out and stderr in the test so you validate the specific clap
unknown-subcommand exit code.

[thepastaclaw]

Code Review

Verified all carried-forward findings against worktree at df202ea. Three prior bugs (load() predicate, sync_height u32 truncation, backup filename collision) were confirmed FIXED. Seven prior findings remain valid in current head — all concentrated in the pure-Rust storage crate. No new defects introduced by the latest delta. The asymmetry between identity_keys (added IdentityKeyEntryMismatch this delta) and asset_locks (no equivalent cross-check) is real and worth surfacing.

🟡 5 suggestion(s) | 💬 2 nitpick(s)

7 additional finding(s)

suggestion: open() trusts forward-schema databases that restore_from would reject

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

restore_from validates MAX(version) <= max_supported on the staged file (backup.rs:174-194), but open() performs no equivalent check. It only probes for refinery_schema_history, counts pending migrations via the embedded set, optionally takes a pre-migration backup, and runs refinery::run. If a DB written by a newer release is on disk with all currently-known migrations already present, pending_count is zero, refinery is a no-op, and the older binary proceeds to decode blob columns shaped for a newer schema. Since this crate intentionally dropped per-blob revision tags (blob.rs:4-6) and ties blob compatibility entirely to migration version, this is a real defense-in-depth gap on the direct-open trust boundary that's already closed on the restore path. Add the same SchemaVersionUnsupported gate to open() immediately after had_schema_history is observed, before refinery::run executes.

suggestion: blob::decode has no allocation cap — restored backups can drive arbitrary allocations

packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs (line 29)

decode calls bincode::serde::decode_from_slice(blob, bincode::config::standard()) with no length cap; bincode::config::standard() is NoLimit. A corrupted or hostile row whose internal length prefixes (inside Vec/String/maps) claim multi-exabyte sizes will cause decode_from_slice to attempt the allocation and OOM-kill the wallet host. The trailing-byte guard at lines 31-35 runs only after a successful decode and does not bound the in-decode allocation. Threat model: the live DB file is owner-only (mode 0o600), but SqlitePersister::restore_from explicitly accepts user-supplied .db files after only integrity_check + schema-history/version gates — neither validates per-row blob contents. A crafted backup that passes restore can OOM the wallet on the next load(), get_core_tx_record, asset-lock read, or contacts read. Mitigate via bincode::config::standard().with_limit::<N>() (e.g. 16 MiB — well above any legitimate row) and/or pre-validate length(blob) at the SQL boundary before calling decode.

suggestion: delete_wallet releases the connection mutex between drain, existence check, backup, and delete transaction

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

delete_wallet_inner drains the buffer (line 294), then opens a fresh self.conn()? scope for the existence check (lines 299-312), then a separate self.conn()? for the auto-backup (lines 316-323), then a third for the DELETE transaction. The persister is exposed via Arc<dyn PlatformWalletPersistence> and is documented Send + Sync, so another thread can call store(wallet_id, cs) between any of these scopes. The racing store() repopulates the per-wallet buffer; in FlushMode::Immediate it also writes directly to the same DB. Outcomes: (a) the racing store lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) the racing store only buffers — the next commit_writes() persists the buffered changeset, potentially under a freshly-recreated wallet_metadata parent. The buffer-drain fix this PR shipped closes the resurrection-from-stale-buffer path, but not the resurrect-via-racing-store window. No malicious actor required — a background sync task running parallel to a user-initiated delete is the realistic trigger. Hold the connection mutex (or a per-wallet flush-suppression lock) across the entire drain → existence-check → backup → delete-transaction sequence so concurrent stores either block or fail with a typed error.

suggestion: WAL/SHM siblings deleted before staged file passes integrity + schema recheck

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 125)

Step 3 (lines 125-138) unconditionally unlinks <dest>-wal and <dest>-shm. Steps 4-5 then stage the source via std::io::copy and re-validate (integrity_check, schema_history presence, MAX(version) <= max_supported) on the staged copy at lines 148-195. If staged validation fails — integrity_check failure, missing schema_history, or SchemaVersionUnsupported — the temp file drops cleanly and the live destination main DB is untouched, but its WAL/SHM are already gone. For a WAL-mode database with un-checkpointed pages, that loses durable state without any persist having occurred. The function mutates external state before the fallible validation phase it depends on, so the error path is not rollback-safe. Move WAL/SHM deletion to immediately before tmp.persist(dest_db_path) at line 198, only after the staged file has passed all checks.

suggestion: decode_row never cross-checks typed columns against the lifecycle blob (asymmetric with new identity_keys writer guard)

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

Each asset-lock row stores (outpoint, account_index) twice — once in indexed columns and once inside lifecycle_blob (AssetLockEntry::out_point, account_index). decode_row decodes the typed outpoint from op_bytes at line 90 but then constructs TrackedAssetLock using entry.out_point and entry.account_index from the blob (lines 93, 95), never comparing the decoded typed outpoint and converted account_index against the blob fields. The typed account_index is range-checked for u32 overflow (lines 102-107) but never against entry.account_index. A torn write, partial migration, or manual row edit will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated Hard-fail on the first decode error contract calls for an explicit entry.out_point == outpoint && entry.account_index == account_index assertion returning WalletStorageError::BlobDecode on divergence. Notably the parallel concern WAS addressed for identity_keys in commit ce8ca25 (IdentityKeyEntryMismatch, identity_keys.rs:87-94); apply the same fail-hard policy symmetrically on the asset-lock read side.

nitpick: restore_from streams the entire source via std::io::copy before its schema/version gate

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 92)

Step 1 (lines 97-102) only runs PRAGMA integrity_check on the source — the schema-history / max-version gate (lines 163-194) runs only on the STAGED copy, after std::io::copy(&mut src_file, tmp.as_file_mut()) at line 145 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI / FFI / UI consumer) fills the wallet directory's filesystem with the staged copy before being rejected as SchemaHistoryMissing / SchemaVersionUnsupported. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the first source handle (already opened at line 97) would fail fast and avoid the staged-copy disk-exhaustion DoS.

nitpick: list_active is byte-for-byte identical to load_state

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

The SQL at lines 147-150 and the decode loop at 151-163 are identical to load_state at 117-135. The updated docstring (lines 137-142) now says Every status variant the changeset writes is considered 'active': consumed locks leave via AssetLockChangeSet::removed — which makes the missing WHERE status … clause intentional, but also makes list_active semantically identical to load_state. The two functions are dead-code duplicates referenced only by separate integration tests (tests/sqlite_persist_roundtrip.rs:377 uses list_active, tests/sqlite_load_reconstruction.rs uses load_state). Either delete one and route callers to the survivor, or make list_active actually narrow the query so the name distinguishes it from load_state.

🤖 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/persister.rs`:130-157: open() trusts forward-schema databases that restore_from would reject
  `restore_from` validates `MAX(version) <= max_supported` on the staged file (backup.rs:174-194), but `open()` performs no equivalent check. It only probes for `refinery_schema_history`, counts pending migrations via the embedded set, optionally takes a pre-migration backup, and runs `refinery::run`. If a DB written by a newer release is on disk with all currently-known migrations already present, `pending_count` is zero, refinery is a no-op, and the older binary proceeds to decode blob columns shaped for a newer schema. Since this crate intentionally dropped per-blob revision tags (blob.rs:4-6) and ties blob compatibility entirely to migration version, this is a real defense-in-depth gap on the direct-open trust boundary that's already closed on the restore path. Add the same `SchemaVersionUnsupported` gate to `open()` immediately after `had_schema_history` is observed, before `refinery::run` executes.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:29-37: blob::decode has no allocation cap — restored backups can drive arbitrary allocations
  `decode` calls `bincode::serde::decode_from_slice(blob, bincode::config::standard())` with no length cap; `bincode::config::standard()` is `NoLimit`. A corrupted or hostile row whose internal length prefixes (inside `Vec`/`String`/maps) claim multi-exabyte sizes will cause `decode_from_slice` to attempt the allocation and OOM-kill the wallet host. The trailing-byte guard at lines 31-35 runs only after a successful decode and does not bound the in-decode allocation. Threat model: the live DB file is owner-only (mode 0o600), but `SqlitePersister::restore_from` explicitly accepts user-supplied `.db` files after only `integrity_check` + schema-history/version gates — neither validates per-row blob contents. A crafted backup that passes restore can OOM the wallet on the next `load()`, `get_core_tx_record`, asset-lock read, or contacts read. Mitigate via `bincode::config::standard().with_limit::<N>()` (e.g. 16 MiB — well above any legitimate row) and/or pre-validate `length(blob)` at the SQL boundary before calling `decode`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:285-348: delete_wallet releases the connection mutex between drain, existence check, backup, and delete transaction
  `delete_wallet_inner` drains the buffer (line 294), then opens a fresh `self.conn()?` scope for the existence check (lines 299-312), then a separate `self.conn()?` for the auto-backup (lines 316-323), then a third for the DELETE transaction. The persister is exposed via `Arc<dyn PlatformWalletPersistence>` and is documented `Send + Sync`, so another thread can call `store(wallet_id, cs)` between any of these scopes. The racing `store()` repopulates the per-wallet buffer; in `FlushMode::Immediate` it also writes directly to the same DB. Outcomes: (a) the racing `store` lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) the racing `store` only buffers — the next `commit_writes()` persists the buffered changeset, potentially under a freshly-recreated `wallet_metadata` parent. The buffer-drain fix this PR shipped closes the resurrection-from-stale-buffer path, but not the resurrect-via-racing-store window. No malicious actor required — a background sync task running parallel to a user-initiated delete is the realistic trigger. Hold the connection mutex (or a per-wallet flush-suppression lock) across the entire drain → existence-check → backup → delete-transaction sequence so concurrent stores either block or fail with a typed error.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:125-195: WAL/SHM siblings deleted before staged file passes integrity + schema recheck
  Step 3 (lines 125-138) unconditionally unlinks `<dest>-wal` and `<dest>-shm`. Steps 4-5 then stage the source via `std::io::copy` and re-validate (integrity_check, schema_history presence, MAX(version) <= max_supported) on the staged copy at lines 148-195. If staged validation fails — integrity_check failure, missing schema_history, or `SchemaVersionUnsupported` — the temp file drops cleanly and the live destination main DB is untouched, but its WAL/SHM are already gone. For a WAL-mode database with un-checkpointed pages, that loses durable state without any persist having occurred. The function mutates external state before the fallible validation phase it depends on, so the error path is not rollback-safe. Move WAL/SHM deletion to immediately before `tmp.persist(dest_db_path)` at line 198, only after the staged file has passed all checks.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:85-109: decode_row never cross-checks typed columns against the lifecycle blob (asymmetric with new identity_keys writer guard)
  Each asset-lock row stores `(outpoint, account_index)` twice — once in indexed columns and once inside `lifecycle_blob` (`AssetLockEntry::out_point`, `account_index`). `decode_row` decodes the typed `outpoint` from `op_bytes` at line 90 but then constructs `TrackedAssetLock` using `entry.out_point` and `entry.account_index` from the blob (lines 93, 95), never comparing the decoded typed outpoint and converted `account_index` against the blob fields. The typed `account_index` is range-checked for u32 overflow (lines 102-107) but never against `entry.account_index`. A torn write, partial migration, or manual row edit will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated `Hard-fail on the first decode error` contract calls for an explicit `entry.out_point == outpoint && entry.account_index == account_index` assertion returning `WalletStorageError::BlobDecode` on divergence. Notably the parallel concern WAS addressed for identity_keys in commit ce8ca25753 (`IdentityKeyEntryMismatch`, identity_keys.rs:87-94); apply the same fail-hard policy symmetrically on the asset-lock read side.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:92-195: restore_from streams the entire source via std::io::copy before its schema/version gate
  Step 1 (lines 97-102) only runs `PRAGMA integrity_check` on the source — the schema-history / max-version gate (lines 163-194) runs only on the STAGED copy, after `std::io::copy(&mut src_file, tmp.as_file_mut())` at line 145 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI / FFI / UI consumer) fills the wallet directory's filesystem with the staged copy before being rejected as `SchemaHistoryMissing` / `SchemaVersionUnsupported`. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the first source handle (already opened at line 97) would fail fast and avoid the staged-copy disk-exhaustion DoS.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:137-164: list_active is byte-for-byte identical to load_state
  The SQL at lines 147-150 and the decode loop at 151-163 are identical to `load_state` at 117-135. The updated docstring (lines 137-142) now says `Every status variant the changeset writes is considered 'active': consumed locks leave via AssetLockChangeSet::removed` — which makes the missing `WHERE status …` clause intentional, but also makes `list_active` semantically identical to `load_state`. The two functions are dead-code duplicates referenced only by separate integration tests (`tests/sqlite_persist_roundtrip.rs:377` uses `list_active`, `tests/sqlite_load_reconstruction.rs` uses `load_state`). Either delete one and route callers to the survivor, or make `list_active` actually narrow the query so the name distinguishes it from `load_state`.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[thepastaclaw]

Code Review

Latest delta (df202ea..4c9664d) is Cargo manifest cleanup only — drops the unused key-wallet-manager dependency from platform-wallet-storage. Zero Rust source changes. All 7 prior findings carry forward unchanged at HEAD 4c9664d, verified line-by-line against persister.rs, backup.rs, blob.rs, and asset_locks.rs. Two suggestions on the restore/import trust boundary (forward-schema gate gap in open(), unbounded bincode decode) have the strongest signal (both reviewers converge at ≥0.85 confidence).

🟡 5 suggestion(s) | 💬 2 nitpick(s)

7 additional finding(s)

suggestion: open() lacks the forward-schema version gate that restore_from enforces

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

SqlitePersister::open() (persister.rs:130-157) only checks whether refinery_schema_history exists and counts pending embedded migrations before calling migrations::run(). It never compares the on-disk MAX(version) against the highest embedded migration. By contrast, backup::restore_from() enforces exactly that gate on the staged file at backup.rs:174-194 (SchemaVersionUnsupported).

Consequence: an older binary pointed at a database produced by a newer release with all currently-known migrations already applied will treat pending_count == 0 as a clean reopen, refinery becomes a no-op, and the older code then decodes blob columns shaped for a newer schema. Because this crate intentionally dropped per-blob revision tags (blob.rs:4-6: "Schema evolution is gated by the refinery migration version on the database as a whole — there is no per-blob revision tag"), the migration version IS the entire compatibility boundary. The direct-open path is the only ingress that bypasses it.

Fix: add the same MAX(version) <= max_supported check to open() immediately after had_schema_history is observed, before migrations::run() executes, returning WalletStorageError::SchemaVersionUnsupported.

suggestion: blob::decode has no allocation cap — restored backups can drive unbounded allocations

packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs (line 29)

blob::decode() (blob.rs:30) calls bincode::serde::decode_from_slice(blob, bincode::config::standard()). bincode::config::standard() is NoLimit, so attacker-controlled length prefixes inside Vec/String/maps will drive the allocator before any validation runs. The trailing-byte check at lines 31-35 only fires after a successful decode and cannot bound the in-decode allocation.

This matters because SqlitePersister::restore_from() (persister.rs:196) explicitly imports user-supplied .db files after only PRAGMA integrity_check plus schema-history/version gates (backup.rs:97-194) — none of which validate per-row blob contents. A crafted backup that passes restore can OOM the wallet host on the next load(), asset-lock read, contacts read, or transaction-record read.

Fix: switch to bincode::config::standard().with_limit::<N>() with a generous-but-finite cap (e.g. 16 MiB, well above any legitimate row), and/or guard length(blob) at the SQL boundary before calling decode.

suggestion: restore_from deletes destination WAL/SHM before the staged file passes validation

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 125)

backup.rs:127-138 unconditionally unlinks <dest>-wal and <dest>-shm BEFORE the source is staged (line 143) and BEFORE the staged file is revalidated (integrity_check at 157, schema_history check at 163-172, max-version check at 174-193). If the staged copy fails any of those later checks, the early-return paths drop the temp file naturally and leave the live destination main DB untouched — but the WAL/SHM siblings are already gone.

For a WAL-mode destination with uncheckpointed pages, that loses durable recent state without any persist having occurred. The function mutates external state before the fallible validation phase it depends on, so the error path is not rollback-safe. A caller who attempts a restore from a damaged or incompatible backup ends up with a corrupted live wallet despite restore returning an error.

Fix: move the WAL/SHM unlink to immediately before tmp.persist(dest_db_path) at line 198, only after the staged file has passed every check.

suggestion: asset_locks::decode_row never cross-checks typed columns against the lifecycle blob

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

Each asset_locks row stores (outpoint, account_index) twice — once in indexed columns and once inside lifecycle_blob (AssetLockEntry::out_point, account_index). decode_row (asset_locks.rs:85-109) decodes the typed outpoint from op_bytes at line 90, then constructs TrackedAssetLock using entry.out_point (line 93) and entry.account_index (line 95) from the blob. The typed account_index is u32-range-checked at lines 102-107 but never compared to entry.account_index; the typed outpoint is never compared to entry.out_point at all.

A torn write, partial migration, manual repair, or restored corrupted backup will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated contract — "Hard-fail behaviour: a malformed outpoint, blob, or out-of-range account index returns a typed WalletStorageError. Every caller propagates that error — corruption is never silently skipped" (lines 82-84) — is not actually enforced for typed/blob divergence.

The symmetric concern was addressed for identity_keys in commit ce8ca25 (IdentityKeyEntryMismatch at identity_keys.rs:87-94). Apply the same fail-hard policy here: assert entry.out_point == outpoint && entry.account_index == account_index before constructing TrackedAssetLock, returning WalletStorageError::BlobDecode on divergence.

suggestion: delete_wallet_inner releases the connection mutex between drain, existence check, backup, and delete

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

delete_wallet_inner (persister.rs:285-348) drains the per-wallet buffer at line 294, then opens THREE separate self.conn()? scopes: existence check (line 300, guard dropped at line 312), optional auto-backup (line 316, dropped at end of else-branch), and the DELETE transaction (line 324). The persister is Arc<Mutex<Connection>>, exposed via Arc<dyn PlatformWalletPersistence>, and documented Send + Sync — so another thread can call store(wallet_id, cs) between any of these scopes.

The racing store() repopulates the per-wallet buffer; in FlushMode::Immediate it also writes directly to the same DB. Two race outcomes: (a) a racing immediate flush lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) a racing buffered store leaves data for the next commit_writes() to persist, potentially under a freshly-recreated wallet_metadata parent. The CMT-001 buffer-drain fix closed the stale-buffer resurrection path but not the racing-store window.

No malicious actor needed — a background sync task running in parallel with a user-initiated delete is the realistic trigger. Hold the connection mutex (or introduce a per-wallet flush-suppression lock) across drain → existence-check → backup → delete-transaction so concurrent stores either block or fail with a typed error.

nitpick: restore_from streams the entire source before its cheap schema/version gate

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 92)

backup.rs:97-102 only runs PRAGMA integrity_check on the source handle. The schema-history presence check (lines 163-172) and MAX(version) <= max_supported gate (lines 174-193) run only on the STAGED copy, after std::io::copy(&mut src_file, tmp.as_file_mut()) at line 145 has already streamed the entire source into a NamedTempFile in the destination's parent directory.

A caller passing a valid-but-huge SQLite database (CLI, FFI consumer, UI import flow) fills the wallet partition with the staged copy before being rejected as SchemaHistoryMissing / SchemaVersionUnsupported. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the first source handle already opened at line 97 would fail fast and avoid the staged-copy disk-exhaustion cost for obviously incompatible inputs.

nitpick: list_active is byte-for-byte identical to load_state

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

asset_locks.rs:147-150 (SQL) and 151-163 (decode loop) are identical to load_state at 118-135. The current docstring (lines 137-142) explicitly notes that every persisted status is considered active because consumed locks leave through AssetLockChangeSet::removed — which makes the missing WHERE status … clause intentional, but also makes list_active semantically identical to load_state.

The two functions are duplicates referenced only by separate integration tests (tests/sqlite_persist_roundtrip.rs:377 uses list_active, tests/sqlite_load_reconstruction.rs uses load_state). Either collapse one into the other, or have list_active enforce a narrower SQL contract so its name reflects behavior that differs from load_state.

🤖 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/persister.rs`:130-157: open() lacks the forward-schema version gate that restore_from enforces
  `SqlitePersister::open()` (persister.rs:130-157) only checks whether `refinery_schema_history` exists and counts pending embedded migrations before calling `migrations::run()`. It never compares the on-disk `MAX(version)` against the highest embedded migration. By contrast, `backup::restore_from()` enforces exactly that gate on the staged file at backup.rs:174-194 (`SchemaVersionUnsupported`).

Consequence: an older binary pointed at a database produced by a newer release with all currently-known migrations already applied will treat `pending_count == 0` as a clean reopen, refinery becomes a no-op, and the older code then decodes blob columns shaped for a newer schema. Because this crate intentionally dropped per-blob revision tags (blob.rs:4-6: "Schema evolution is gated by the refinery migration version on the database as a whole — there is no per-blob revision tag"), the migration version IS the entire compatibility boundary. The direct-open path is the only ingress that bypasses it.

Fix: add the same `MAX(version) <= max_supported` check to `open()` immediately after `had_schema_history` is observed, before `migrations::run()` executes, returning `WalletStorageError::SchemaVersionUnsupported`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:29-37: blob::decode has no allocation cap — restored backups can drive unbounded allocations
  `blob::decode()` (blob.rs:30) calls `bincode::serde::decode_from_slice(blob, bincode::config::standard())`. `bincode::config::standard()` is `NoLimit`, so attacker-controlled length prefixes inside `Vec`/`String`/maps will drive the allocator before any validation runs. The trailing-byte check at lines 31-35 only fires after a successful decode and cannot bound the in-decode allocation.

This matters because `SqlitePersister::restore_from()` (persister.rs:196) explicitly imports user-supplied `.db` files after only `PRAGMA integrity_check` plus schema-history/version gates (backup.rs:97-194) — none of which validate per-row blob contents. A crafted backup that passes restore can OOM the wallet host on the next `load()`, asset-lock read, contacts read, or transaction-record read.

Fix: switch to `bincode::config::standard().with_limit::<N>()` with a generous-but-finite cap (e.g. 16 MiB, well above any legitimate row), and/or guard `length(blob)` at the SQL boundary before calling `decode`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:125-198: restore_from deletes destination WAL/SHM before the staged file passes validation
  backup.rs:127-138 unconditionally unlinks `<dest>-wal` and `<dest>-shm` BEFORE the source is staged (line 143) and BEFORE the staged file is revalidated (integrity_check at 157, schema_history check at 163-172, max-version check at 174-193). If the staged copy fails any of those later checks, the early-return paths drop the temp file naturally and leave the live destination main DB untouched — but the WAL/SHM siblings are already gone.

For a WAL-mode destination with uncheckpointed pages, that loses durable recent state without any persist having occurred. The function mutates external state before the fallible validation phase it depends on, so the error path is not rollback-safe. A caller who attempts a restore from a damaged or incompatible backup ends up with a corrupted live wallet despite restore returning an error.

Fix: move the WAL/SHM unlink to immediately before `tmp.persist(dest_db_path)` at line 198, only after the staged file has passed every check.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:85-109: asset_locks::decode_row never cross-checks typed columns against the lifecycle blob
  Each `asset_locks` row stores `(outpoint, account_index)` twice — once in indexed columns and once inside `lifecycle_blob` (`AssetLockEntry::out_point`, `account_index`). `decode_row` (asset_locks.rs:85-109) decodes the typed `outpoint` from `op_bytes` at line 90, then constructs `TrackedAssetLock` using `entry.out_point` (line 93) and `entry.account_index` (line 95) from the blob. The typed `account_index` is u32-range-checked at lines 102-107 but never compared to `entry.account_index`; the typed outpoint is never compared to `entry.out_point` at all.

A torn write, partial migration, manual repair, or restored corrupted backup will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated contract — "Hard-fail behaviour: a malformed outpoint, blob, or out-of-range account index returns a typed WalletStorageError. Every caller propagates that error — corruption is never silently skipped" (lines 82-84) — is not actually enforced for typed/blob divergence.

The symmetric concern was addressed for identity_keys in commit ce8ca25753 (`IdentityKeyEntryMismatch` at identity_keys.rs:87-94). Apply the same fail-hard policy here: assert `entry.out_point == outpoint && entry.account_index == account_index` before constructing `TrackedAssetLock`, returning `WalletStorageError::BlobDecode` on divergence.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:285-348: delete_wallet_inner releases the connection mutex between drain, existence check, backup, and delete
  `delete_wallet_inner` (persister.rs:285-348) drains the per-wallet buffer at line 294, then opens THREE separate `self.conn()?` scopes: existence check (line 300, guard dropped at line 312), optional auto-backup (line 316, dropped at end of else-branch), and the DELETE transaction (line 324). The persister is `Arc<Mutex<Connection>>`, exposed via `Arc<dyn PlatformWalletPersistence>`, and documented `Send + Sync` — so another thread can call `store(wallet_id, cs)` between any of these scopes.

The racing `store()` repopulates the per-wallet buffer; in `FlushMode::Immediate` it also writes directly to the same DB. Two race outcomes: (a) a racing immediate flush lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) a racing buffered store leaves data for the next `commit_writes()` to persist, potentially under a freshly-recreated `wallet_metadata` parent. The CMT-001 buffer-drain fix closed the stale-buffer resurrection path but not the racing-store window.

No malicious actor needed — a background sync task running in parallel with a user-initiated delete is the realistic trigger. Hold the connection mutex (or introduce a per-wallet flush-suppression lock) across drain → existence-check → backup → delete-transaction so concurrent stores either block or fail with a typed error.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:92-195: restore_from streams the entire source before its cheap schema/version gate
  backup.rs:97-102 only runs `PRAGMA integrity_check` on the source handle. The schema-history presence check (lines 163-172) and `MAX(version) <= max_supported` gate (lines 174-193) run only on the STAGED copy, after `std::io::copy(&mut src_file, tmp.as_file_mut())` at line 145 has already streamed the entire source into a `NamedTempFile` in the destination's parent directory.

A caller passing a valid-but-huge SQLite database (CLI, FFI consumer, UI import flow) fills the wallet partition with the staged copy before being rejected as `SchemaHistoryMissing` / `SchemaVersionUnsupported`. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the first source handle already opened at line 97 would fail fast and avoid the staged-copy disk-exhaustion cost for obviously incompatible inputs.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:143-164: list_active is byte-for-byte identical to load_state
  asset_locks.rs:147-150 (SQL) and 151-163 (decode loop) are identical to `load_state` at 118-135. The current docstring (lines 137-142) explicitly notes that every persisted status is considered active because consumed locks leave through `AssetLockChangeSet::removed` — which makes the missing `WHERE status …` clause intentional, but also makes `list_active` semantically identical to `load_state`.

The two functions are duplicates referenced only by separate integration tests (`tests/sqlite_persist_roundtrip.rs:377` uses `list_active`, `tests/sqlite_load_reconstruction.rs` uses `load_state`). Either collapse one into the other, or have `list_active` enforce a narrower SQL contract so its name reflects behavior that differs from `load_state`.

Inline posting hit GitHub HTTP 422, so I posted the same verified findings as a top-level review body.

[github-actions]

✅ DashSDKFFI.xcframework built for this PR.

• Workflow run: https://github.com/dashpay/platform/actions/runs/26316422853
• Artifacts: DashSDKFFI.xcframework (folder), DashSDKFFI.xcframework.zip, xc_checksum.txt

SwiftPM (host the zip at a stable URL, then use):

.binaryTarget(
  name: "DashSDKFFI",
  url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
  checksum: "99edb0b68b3be9d5370194b9817d830f62d9347ccdf9acabf3e445022ba22d37"
)

Xcode manual integration:

• Download 'DashSDKFFI.xcframework' artifact from the run link above.
• Drag it into your app target (Frameworks, Libraries & Embedded Content) and set Embed & Sign.
• If using the Swift wrapper package, point its binaryTarget to the xcframework location or add the package and place the xcframework at the expected path.

[thepastaclaw]

Code Review

Latest delta (4c9664d..b4ffcbc) is a single test-only commit that adds out_dir.canonicalize() to TC-031 for macOS /var → /private/var symlink path symmetry. No production code, schema, or FFI changes. All prior structural findings reverified against the current head and remain valid. Carried-forward prior findings: open() lacks the forward-schema MAX(version) gate that restore_from enforces (STILL VALID); blob::decode uses uncapped bincode::config::standard() (STILL VALID); restore_from unlinks WAL/SHM siblings before staged-file revalidation (STILL VALID, blocking — data-loss path); asset_locks::decode_row never cross-checks typed columns against the blob (STILL VALID, asymmetric with identity_keys' new IdentityKeyEntryMismatch guard); delete_wallet_inner releases the connection mutex between drain/existence-check/backup/DELETE scopes (STILL VALID); restore_from streams full source before its cheap schema gate (STILL VALID); list_active is byte-identical to load_state (STILL VALID nit). New cumulative findings raised by codex specialists: delete_wallet_inner drops the drained buffered changeset on any pre-delete failure path (blocking — data-loss); identity_keys::into_entry ignores the inner bincode consumed count for public_key_bincode (strict-decode gap); apply_secure_permissions only chmods the main DB file but the default WAL journal leaves -wal/-shm siblings exposed to the process umask. Zero new findings in the latest delta itself.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 b4ffcbc
STDOUT:

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

Reviewed commit: b4ffcbc

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

Verified findings

blocking: [CARRIED-FORWARD] restore_from unlinks WAL/SHM siblings before the staged copy passes validation

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 125)

backup.rs:127-138 unconditionally removes <dest>-wal and <dest>-shm before the source is staged (line 143-145) and before the staged file is revalidated (integrity_check at 157, schema_history presence at 163-172, MAX(version) gate at 174-193). If any of those staged checks fail, the early-return drops the temp file and leaves the destination main DB untouched — but the WAL/SHM siblings are already gone. The crate's default journal mode is WAL (config.rs:91, default JournalMode::Wal), so un-checkpointed committed pages live only in *.db-wal; deleting that sibling before knowing the replacement is good is durable data loss on a destructive operation. The function mutates external state before the fallible validation it depends on. Move the sibling unlink to immediately before tmp.persist(dest_db_path) at line 198, only after every staged check has succeeded.

blocking: delete_wallet_inner drops the drained buffered changeset on any pre-delete failure

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

Line 294 moves the buffered changeset out via self.buffer.take_for_flush(&wallet_id)? and never restores it. Every subsequent fallible step before tx.commit() returns through ? and the owned changeset is dropped: WalletNotFound cannot fire after a successful take (had_buffered would be true), but run_auto_backup at 317-322 (I/O error, AutoBackupDisabled, etc.), self.conn()? at 324, conn.transaction()? at 325, the per-table COUNT queries at 331-338, wallet_meta::delete at 341, and tx.commit()? at 342 all return Err with the drained value silently dropped. Result: the operator's delete fails (correctly reporting the backup or SQL error), but the wallet's pending writes vanish from the buffer even though no DELETE was committed. flush_inner already uses the take/restore pattern — apply the same discipline here: hold the drained changeset in a local Option, restore it into the buffer on every error path, and only consume it after the delete transaction commits.

suggestion: [CARRIED-FORWARD] open() lacks the forward-schema version gate that restore_from enforces

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

SqlitePersister::open() at persister.rs:130-157 probes for refinery_schema_history, counts pending embedded migrations, optionally takes a pre-migration backup, and runs refinery::run(). It never compares the on-disk MAX(version) against the highest embedded migration. restore_from() enforces exactly that gate on the staged file at backup.rs:174-194 via SchemaVersionUnsupported. Consequence: an older binary opening a database produced by a newer release with all currently-known migrations already applied sees pending_count == 0, refinery is a no-op, and the older binary then decodes blob columns shaped for a newer schema. Per blob.rs:4-6 this crate intentionally dropped per-blob revision tags — the migration version IS the entire compatibility boundary, and the direct-open path is the only ingress that bypasses it. Add the same MAX(version) <= max_supported check inside open() immediately after had_schema_history is observed, returning SchemaVersionUnsupported before migrations::run() executes.

suggestion: [CARRIED-FORWARD] blob::decode has no allocation cap — restored backups can drive unbounded allocations

packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs (line 29)

blob.rs:30 calls bincode::serde::decode_from_slice(blob, bincode::config::standard()). bincode::config::standard() is NoLimit, so attacker-controlled length prefixes inside Vec/String/maps drive the allocator before any validation runs. The trailing-byte check at lines 31-35 only fires after a successful decode and does not bound the in-decode allocation. SqlitePersister::restore_from() explicitly imports user-supplied .db files after only PRAGMA integrity_check + schema-history/version gates (backup.rs:97-194) — none validate per-row blob contents. A crafted backup that passes restore can OOM the wallet host on the next load() or per-row read. Switch to bincode::config::standard().with_limit::<N>() with a generous-but-finite cap (e.g. 16 MiB, well above any legitimate row), and/or pre-check length(blob) at the SQL boundary before calling decode.

suggestion: [CARRIED-FORWARD] decode_row never cross-checks typed columns against the lifecycle blob (asymmetric with identity_keys writer guard)

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

Each asset-lock row stores (outpoint, account_index) twice — once in indexed columns and once inside lifecycle_blob (AssetLockEntry::out_point, account_index). decode_row decodes the typed outpoint from op_bytes at line 90 but then constructs TrackedAssetLock using entry.out_point (line 93) and entry.account_index (line 95) from the blob. The typed account_index is u32-range-checked at lines 102-107 but never compared to entry.account_index; the typed outpoint is never compared to entry.out_point at all. A torn write, partial migration, manual repair, or restored corrupted backup will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated hard-fail-on-corruption contract (lines 82-84) is not actually enforced for typed/blob divergence. The symmetric concern was addressed for identity_keys (IdentityKeyEntryMismatch at identity_keys.rs:87-94). Apply the same fail-hard policy here: assert entry.out_point == outpoint && entry.account_index == account_index before constructing TrackedAssetLock, returning WalletStorageError::BlobDecode on divergence.

suggestion: [CARRIED-FORWARD] delete_wallet_inner releases the connection mutex between drain, existence check, backup, and delete transaction

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

delete_wallet_inner drains the buffer (line 294), then opens three separate self.conn()? scopes: existence check (lines 299-312, guard dropped at line 312), optional auto-backup (lines 313-323), and the DELETE transaction (lines 324-325). The persister is Arc<Mutex<Connection>>, exposed via Arc<dyn PlatformWalletPersistence> and documented Send + Sync. Another thread can call store(wallet_id, cs) between any of these scopes. The racing store() repopulates the per-wallet buffer; in FlushMode::Immediate it also writes directly to the same DB. Outcomes: (a) a racing immediate flush lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) a racing buffered store leaves data for the next commit_writes() to persist under a freshly-recreated wallet_metadata parent. The CMT-001 buffer-drain fix closed the stale-buffer resurrection path but not the racing-store window. No malicious actor needed — a background sync task running parallel to a user-initiated delete is the realistic trigger. Hold the connection mutex (or introduce a per-wallet flush-suppression lock) across drain → existence-check → backup → delete-transaction so concurrent stores either block or fail with a typed error.

suggestion: identity_keys::into_entry ignores the inner bincode `consumed` count for public_key_bincode

packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs (line 53)

blob::decode was hardened to reject trailing bytes in public_key_blob, but IdentityKeyWire::into_entry at identity_keys.rs:54-55 decodes the nested public_key_bincode with bincode::decode_from_slice(...) and binds consumed to _. A corrupt or forward-incompatible inner payload whose IdentityPublicKey prefix is valid will deserialize successfully even with garbage trailing bytes inside the inner blob. This contradicts the crate's hard-fail-on-corruption load policy and leaves identity_keys weaker than every other blob-decoding path. Compare consumed == self.public_key_bincode.len() and return WalletStorageError::blob_decode("unexpected trailing bytes in identity_keys.public_key_bincode") on mismatch.

suggestion: apply_secure_permissions does not cover WAL/SHM sidecars

packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs (line 12)

apply_secure_permissions chmods only the file at path (the main DB). The crate's default journal mode is Wal (config.rs:91, default JournalMode::Wal), so recent committed writes that have not been checkpointed live in <path>-wal and <path>-shm. Those siblings are created by SQLite under the process umask and never tightened by this helper. On a multi-user host with a common 022 umask, another local user can read the WAL even after the main .db file has been reduced to 0600, leaking wallet metadata, contacts, address state, and transaction-record blobs persisted by this crate. Extend apply_secure_permissions (or add a sibling helper invoked from the same call sites — open, run_to, restore_from) to chmod <path>-wal and <path>-shm to 0o600 if they exist.

nitpick: [CARRIED-FORWARD] restore_from streams the entire source before its cheap schema/version gate

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 92)

backup.rs:97-102 only runs PRAGMA integrity_check on the source. The schema-history presence check (163-172) and MAX(version) <= max_supported gate (174-193) run only on the STAGED copy, after std::io::copy(&mut src_file, tmp.as_file_mut()) at line 145 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI, FFI consumer, UI import flow) fills the wallet partition with the staged copy before being rejected as SchemaHistoryMissing / SchemaVersionUnsupported. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the source handle already opened at line 97 would fail fast and avoid the staged-copy disk-exhaustion cost for obviously incompatible inputs.

nitpick: [CARRIED-FORWARD] list_active is byte-for-byte identical to load_state

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

asset_locks.rs:147-150 (SQL) and 151-163 (decode loop) are identical to load_state at 118-135. The current docstring (137-142) explicitly notes that every persisted status is considered active because consumed locks leave through AssetLockChangeSet::removed — which makes the missing WHERE status … clause intentional, but also makes list_active semantically identical to load_state. The two are duplicates referenced only by separate integration tests (tests/sqlite_persist_roundtrip.rs:377 uses list_active, tests/sqlite_load_reconstruction.rs uses load_state). Either collapse one into the other and route callers to the survivor, or have list_active enforce a narrower SQL contract so its name reflects behavior that differs from load_state.

🤖 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/src/sqlite/backup.rs`:125-138: [CARRIED-FORWARD] restore_from unlinks WAL/SHM siblings before the staged copy passes validation
  backup.rs:127-138 unconditionally removes `<dest>-wal` and `<dest>-shm` before the source is staged (line 143-145) and before the staged file is revalidated (integrity_check at 157, schema_history presence at 163-172, MAX(version) gate at 174-193). If any of those staged checks fail, the early-return drops the temp file and leaves the destination main DB untouched — but the WAL/SHM siblings are already gone. The crate's default journal mode is WAL (config.rs:91, default JournalMode::Wal), so un-checkpointed committed pages live only in `*.db-wal`; deleting that sibling before knowing the replacement is good is durable data loss on a destructive operation. The function mutates external state before the fallible validation it depends on. Move the sibling unlink to immediately before `tmp.persist(dest_db_path)` at line 198, only after every staged check has succeeded.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:285-323: delete_wallet_inner drops the drained buffered changeset on any pre-delete failure
  Line 294 moves the buffered changeset out via `self.buffer.take_for_flush(&wallet_id)?` and never restores it. Every subsequent fallible step before `tx.commit()` returns through `?` and the owned changeset is dropped: `WalletNotFound` cannot fire after a successful take (had_buffered would be true), but `run_auto_backup` at 317-322 (I/O error, AutoBackupDisabled, etc.), `self.conn()?` at 324, `conn.transaction()?` at 325, the per-table COUNT queries at 331-338, `wallet_meta::delete` at 341, and `tx.commit()?` at 342 all return Err with the drained value silently dropped. Result: the operator's delete fails (correctly reporting the backup or SQL error), but the wallet's pending writes vanish from the buffer even though no DELETE was committed. `flush_inner` already uses the take/restore pattern — apply the same discipline here: hold the drained changeset in a local `Option`, restore it into the buffer on every error path, and only consume it after the delete transaction commits.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:130-157: [CARRIED-FORWARD] open() lacks the forward-schema version gate that restore_from enforces
  `SqlitePersister::open()` at persister.rs:130-157 probes for `refinery_schema_history`, counts pending embedded migrations, optionally takes a pre-migration backup, and runs `refinery::run()`. It never compares the on-disk `MAX(version)` against the highest embedded migration. `restore_from()` enforces exactly that gate on the staged file at backup.rs:174-194 via `SchemaVersionUnsupported`. Consequence: an older binary opening a database produced by a newer release with all currently-known migrations already applied sees `pending_count == 0`, refinery is a no-op, and the older binary then decodes blob columns shaped for a newer schema. Per blob.rs:4-6 this crate intentionally dropped per-blob revision tags — the migration version IS the entire compatibility boundary, and the direct-open path is the only ingress that bypasses it. Add the same `MAX(version) <= max_supported` check inside `open()` immediately after `had_schema_history` is observed, returning `SchemaVersionUnsupported` before `migrations::run()` executes.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:29-37: [CARRIED-FORWARD] blob::decode has no allocation cap — restored backups can drive unbounded allocations
  blob.rs:30 calls `bincode::serde::decode_from_slice(blob, bincode::config::standard())`. `bincode::config::standard()` is `NoLimit`, so attacker-controlled length prefixes inside `Vec`/`String`/maps drive the allocator before any validation runs. The trailing-byte check at lines 31-35 only fires after a successful decode and does not bound the in-decode allocation. `SqlitePersister::restore_from()` explicitly imports user-supplied `.db` files after only `PRAGMA integrity_check` + schema-history/version gates (backup.rs:97-194) — none validate per-row blob contents. A crafted backup that passes restore can OOM the wallet host on the next `load()` or per-row read. Switch to `bincode::config::standard().with_limit::<N>()` with a generous-but-finite cap (e.g. 16 MiB, well above any legitimate row), and/or pre-check `length(blob)` at the SQL boundary before calling `decode`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:85-109: [CARRIED-FORWARD] decode_row never cross-checks typed columns against the lifecycle blob (asymmetric with identity_keys writer guard)
  Each asset-lock row stores `(outpoint, account_index)` twice — once in indexed columns and once inside `lifecycle_blob` (`AssetLockEntry::out_point`, `account_index`). `decode_row` decodes the typed `outpoint` from `op_bytes` at line 90 but then constructs `TrackedAssetLock` using `entry.out_point` (line 93) and `entry.account_index` (line 95) from the blob. The typed `account_index` is u32-range-checked at lines 102-107 but never compared to `entry.account_index`; the typed outpoint is never compared to `entry.out_point` at all. A torn write, partial migration, manual repair, or restored corrupted backup will silently load the asset lock into the wrong account bucket while reporting a different outpoint than the indexed column the caller selected by. The module's stated hard-fail-on-corruption contract (lines 82-84) is not actually enforced for typed/blob divergence. The symmetric concern was addressed for `identity_keys` (`IdentityKeyEntryMismatch` at identity_keys.rs:87-94). Apply the same fail-hard policy here: assert `entry.out_point == outpoint && entry.account_index == account_index` before constructing `TrackedAssetLock`, returning `WalletStorageError::BlobDecode` on divergence.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:285-348: [CARRIED-FORWARD] delete_wallet_inner releases the connection mutex between drain, existence check, backup, and delete transaction
  `delete_wallet_inner` drains the buffer (line 294), then opens three separate `self.conn()?` scopes: existence check (lines 299-312, guard dropped at line 312), optional auto-backup (lines 313-323), and the DELETE transaction (lines 324-325). The persister is `Arc<Mutex<Connection>>`, exposed via `Arc<dyn PlatformWalletPersistence>` and documented `Send + Sync`. Another thread can call `store(wallet_id, cs)` between any of these scopes. The racing `store()` repopulates the per-wallet buffer; in `FlushMode::Immediate` it also writes directly to the same DB. Outcomes: (a) a racing immediate flush lands rows AFTER the DELETE commits — wallet rows reappear, operator believes deletion succeeded; (b) a racing buffered store leaves data for the next `commit_writes()` to persist under a freshly-recreated `wallet_metadata` parent. The CMT-001 buffer-drain fix closed the stale-buffer resurrection path but not the racing-store window. No malicious actor needed — a background sync task running parallel to a user-initiated delete is the realistic trigger. Hold the connection mutex (or introduce a per-wallet flush-suppression lock) across drain → existence-check → backup → delete-transaction so concurrent stores either block or fail with a typed error.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:53-65: identity_keys::into_entry ignores the inner bincode `consumed` count for public_key_bincode
  `blob::decode` was hardened to reject trailing bytes in `public_key_blob`, but `IdentityKeyWire::into_entry` at identity_keys.rs:54-55 decodes the nested `public_key_bincode` with `bincode::decode_from_slice(...)` and binds `consumed` to `_`. A corrupt or forward-incompatible inner payload whose `IdentityPublicKey` prefix is valid will deserialize successfully even with garbage trailing bytes inside the inner blob. This contradicts the crate's hard-fail-on-corruption load policy and leaves `identity_keys` weaker than every other blob-decoding path. Compare `consumed == self.public_key_bincode.len()` and return `WalletStorageError::blob_decode("unexpected trailing bytes in identity_keys.public_key_bincode")` on mismatch.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs`:12-23: apply_secure_permissions does not cover WAL/SHM sidecars
  `apply_secure_permissions` chmods only the file at `path` (the main DB). The crate's default journal mode is `Wal` (config.rs:91, default JournalMode::Wal), so recent committed writes that have not been checkpointed live in `<path>-wal` and `<path>-shm`. Those siblings are created by SQLite under the process umask and never tightened by this helper. On a multi-user host with a common 022 umask, another local user can read the WAL even after the main `.db` file has been reduced to 0600, leaking wallet metadata, contacts, address state, and transaction-record blobs persisted by this crate. Extend `apply_secure_permissions` (or add a sibling helper invoked from the same call sites — `open`, `run_to`, `restore_from`) to chmod `<path>-wal` and `<path>-shm` to 0o600 if they exist.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:92-195: [CARRIED-FORWARD] restore_from streams the entire source before its cheap schema/version gate
  backup.rs:97-102 only runs `PRAGMA integrity_check` on the source. The schema-history presence check (163-172) and `MAX(version) <= max_supported` gate (174-193) run only on the STAGED copy, after `std::io::copy(&mut src_file, tmp.as_file_mut())` at line 145 has already streamed the entire source into a NamedTempFile in the destination's parent directory. A caller passing a valid-but-huge SQLite database (CLI, FFI consumer, UI import flow) fills the wallet partition with the staged copy before being rejected as `SchemaHistoryMissing` / `SchemaVersionUnsupported`. The TOCTOU-safe final gate on the staged file should stay (binding the version check to the bytes actually persisted), but a cheap schema-history + max-version sniff against the source handle already opened at line 97 would fail fast and avoid the staged-copy disk-exhaustion cost for obviously incompatible inputs.
- [NITPICK] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:143-164: [CARRIED-FORWARD] list_active is byte-for-byte identical to load_state
  asset_locks.rs:147-150 (SQL) and 151-163 (decode loop) are identical to `load_state` at 118-135. The current docstring (137-142) explicitly notes that every persisted status is considered active because consumed locks leave through `AssetLockChangeSet::removed` — which makes the missing `WHERE status …` clause intentional, but also makes `list_active` semantically identical to `load_state`. The two are duplicates referenced only by separate integration tests (`tests/sqlite_persist_roundtrip.rs:377` uses `list_active`, `tests/sqlite_load_reconstruction.rs` uses `load_state`). Either collapse one into the other and route callers to the survivor, or have `list_active` enforce a narrower SQL contract so its name reflects behavior that differs from `load_state`.