feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface#3743
Conversation
… (CODE-004)
Replace `PersistenceError::Backend(String)` with
`Backend { kind: PersistenceErrorKind, source: Box<dyn Error + Send + Sync> }`
so the persistor's `is_transient()` contract reaches consumers
without round-tripping through a stringified message.
`PersistenceErrorKind` carries the retry policy explicitly —
`Transient` (caller MAY retry), `Constraint` (SQL constraint /
FK / integrity, caller bug), `Fatal` (everything else). The kind
enum is wildcard-free at every consumer match site so a future
addition forces an explicit classification update.
Persistor side: `From<WalletStorageError> for PersistenceError`
delegates to a new `WalletStorageError::persistence_kind` that
folds in `is_transient()` plus SQLite `ConstraintViolation`
detection. The boxed source preserves the typed `WalletStorageError`
chain so consumers can `Error::source()`-walk to the inner rusqlite
payload — the previous `DisplayChain` flattening goes away.
Backward compat:
- `PersistenceError::backend(source)` defaults to `Fatal` kind.
- `PersistenceError::backend_with_kind(kind, source)` for callers
that classify (the storage `From` impl).
- `From<String>` / `From<&str>` still work and default to `Fatal` —
keeps FFI's `format!(...).into()` sites compiling.
This is a breaking change for any out-of-tree impl that
pattern-matched `PersistenceError::Backend(String)`. In-tree consumer
call sites still only log + proceed; the semantic upgrade to inspect
`is_transient()` + retry is T-003 (Wave 2).
Tests:
- `platform-wallet/tests/persistence_error_taxonomy.rs` — 5 tests
covering TC-CODE-004-a (kind shape + exhaustiveness), -c (source
is `Send + Sync` + Display), -e (`From<String>` defaults to Fatal).
- `platform-wallet-storage/tests/persistence_error_kind_mapping.rs` —
7 tests covering TC-CODE-004-b (transient / constraint / fatal
mapping table + chain preservation) and TC-CODE-004-e
(wildcard-free invariant guard on `is_transient` / `error_kind_str`
outer-Self match via source-text parse).
- TC-CODE-004-d (consumer-side retry on Transient) is T-003 per
Nagatha's plan, not T-001.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ty refs (CODE-002)
Drops direct `wallet_id` FK columns from every identity-owned table.
The cascade chain becomes wallet_metadata → identities → identity-owned
objects so the consumer can pass the real owning `wallet_id` (or NULL
for orphan identities) without inventing the `WalletId::default()`
sentinel that V001's FK silently rejected.
Schema changes (V001 → V002):
- identities: PK (identity_id); wallet_id BLOB nullable, FK cascade
- identity_keys: drop wallet_id; PK (identity_id, key_id); FK identity_id
- dashpay_profiles: drop wallet_id; PK identity_id; FK identity_id
- dashpay_payments_overlay: drop wallet_id; PK (identity_id, payment_id); FK identity_id
- token_balances: drop wallet_id; PK (identity_id, token_id); FK identity_id
- wallet-scoped tables (accounts, core_*, platform_*, asset_locks,
contacts_*, wallet_metadata) unchanged
Sentinel-row guard: V002 refuses to run if `token_balances` carries
legacy rows with `wallet_id = X'00…00'`. Implemented via a CHECK on a
temp table; the persister open path re-classifies the failure as
`WalletStorageError::MigrationRequiresManualCleanup { table, count }`
so operators see what to drop instead of a cryptic rusqlite error.
Migration discipline: `run_for_open` disables `PRAGMA foreign_keys`
before BEGIN (and re-enables with assertion on exit). SQLite fires
`ON DELETE CASCADE` on children when their parent is dropped during
the 12-step ALTER, even under `defer_foreign_keys` — wiping the rows
we're migrating. FKs cannot be toggled inside a tx, hence the
wrapper.
Consumer side (`identity_sync.rs`): removed the
`WalletId::default()` sentinel. `IdentitySyncManager` now carries an
identity → `Option<WalletId>` map populated via the new
`register_identity_with_wallet` method; `apply_fresh_balances` looks
the wallet id up and passes it to `persister.store(...)`. The legacy
`register_identity` shim defaults to `None` (orphan) so FFI callers
continue to compile unchanged — V002's nullable
`identities.wallet_id` accepts the orphan case.
PER_WALLET_TABLES gains a `WalletScope { DirectColumn, ViaIdentity }`
discriminant; counting/inspect queries now route through
`count_rows_for_wallet_sql` so identity-scoped tables JOIN through
`identities` to find rows belonging to a wallet. `delete_wallet` is
unaffected — cascade still flows from `wallet_metadata`.
Tests:
- NEW `sqlite_v002_migration.rs` (6 TCs covering rows-preserved,
cascade-chain, orphan identity, real wallet_id token write,
sentinel-row refusal end-to-end, post-migration foreign_key_check)
- Existing migration / round-trip / delete-wallet / hardening tests
updated to the V002 schema (column lists, identity row seeding via
the new `ensure_identity` helper)
- Error-classification table extended with the new variant; gate
remains wildcard-free
Closes CODE-002.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
… on load (CODE-001) `load_from_persistor` used to iterate over the persister's `wallets` map and drain `platform_addresses` per wallet inside the loop. When the persister reports `LOAD_UNIMPLEMENTED = &["ClientStartState::wallets"]` (today's `SqlitePersister` contract, pending PR #3692), the loop body never executes — and every persisted platform-address slice silently drops at function scope. The host's per-wallet `register_wallet` re-fetch path does in fact deliver the addresses correctly, so there is no actual data loss on PR #3625 today. But the silent-drop is a code smell that will absolutely bite the next caller who reaches for `load_from_persistor` expecting the documented contract. Gate the consumer: when `wallets.is_empty() && !platform_addresses.is_empty()`, return a new typed `PlatformWalletError::PersistorMissingWalletRehydration` carrying the unimplemented-area list and the orphan slice count. The host then either waits for #3692 to land or falls back to per-wallet `register_wallet`. Tests: - TC-CODE-001-a: persister returning two orphan platform_addresses slices → `PersistorMissingWalletRehydration { orphan_addresses_count: 2 }` - Negative control: empty persister payload (the `NoPlatformPersistence` shape) still succeeds — gate only trips for the orphan case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…register_wallet (CODE-018)
`register_wallet` used to ignore the result of the registration-time
`persister.store(...)` call — just log on error and proceed with the
wallet half-registered (visible in `wallet_manager`, no
`wallet_metadata` row on disk). Every subsequent per-wallet write
then FK-violates against the missing parent (CODE-002 territory).
Drive the typed `PersistenceError` kind off the wire (introduced by
CODE-004 / T-001):
- `Transient` (e.g. `SQLITE_BUSY`): one 50ms backoff retry; if the
retry succeeds the wallet registers normally.
- `Fatal` / `Constraint` / `LockPoisoned`: undo the in-memory
`wallet_manager` insert and surface the new
`PlatformWalletError::WalletRegistrationFailed { wallet_id, reason }`
variant.
The retry path uses `tokio::time::sleep` (already in tree) and clones
the registration changeset once — `PlatformWalletChangeSet` already
derives `Clone`.
Tests:
- TC-CODE-018-a: scripted `Fatal` store → returns
`WalletRegistrationFailed`, single store attempt (no retry),
`wallet_ids()` is empty.
- TC-CODE-018-b: scripted `Transient → Ok` → succeeds, wallet appears
in `wallet_ids()`, store called at least twice (original + retry).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… trait + wire remove_wallet (CODE-003) Surface SQLite's cascade-delete through the trait so consumers don't need a concrete backend. `DeleteWalletReport` moves from `platform-wallet-storage` into `platform-wallet::changeset::traits` (the trait owns its return types) — SqlitePersister keeps its inherent `delete_wallet` (returns `WalletStorageError` for callers that want the typed error) and adds a trait impl that delegates through the existing `delete_wallet_inner` helper, mapping the `WalletStorageError` chain into a classified `PersistenceError`. `PlatformWalletManager::remove_wallet` now calls `persister.delete_wallet` after the in-memory cleanup. Error policy mirrors `register_wallet` (CODE-018): transient → one backoff retry; anything else → log structured context and continue (the user wanted this wallet gone, in-memory state is already cleaned up, orphan rows can be reaped by an admin tool out-of-band). Trait default returns an empty `DeleteWalletReport` keyed by the requested id, so backends with no per-wallet disk state (`NoPlatformPersistence`, `FFIPersister`) inherit cleanly with no explicit impl required. TC-CODE-003-1/2/3 exercise happy path + fatal-error + transient-retry through `PlatformWalletManager::remove_wallet` with a recording persister. `tests/sqlite_trait_dispatch.rs` adds two more cases for the trait default and the SQLite trait impl round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… trait (CODE-026) Surface SQLite's `commit_writes` (batch flush of every dirty wallet in `FlushMode::Manual`) through the trait so consumers using deferred-write mode can durable-flush without a concrete backend type. `CommitReport` moves from `platform-wallet-storage` into `platform-wallet::changeset::traits` alongside the trait itself. SqlitePersister splits the inherent `commit_writes` body into a private `commit_writes_inner` helper that both the inherent and trait method delegate to — no behavioral drift, no duplication. Trait default returns an empty `CommitReport`, so backends that flush inline (`FlushMode::Immediate` on SQLite) or have nothing to buffer (`NoPlatformPersistence`, `FFIPersister`) inherit cleanly with no explicit impl required. TC-CODE-026-1/2 cover the default-impl empty-report contract and the SQLite trait round-trip flushing two dirty wallets durably. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…flock; drop fs2 dep (CODE-005/007/010/015) Cross-process exclusion for `restore_from` and `delete_wallet` now uses SQLite-native `BEGIN EXCLUSIVE` against the destination DB instead of the prior `fs2::FileExt::try_lock_exclusive` advisory lock. Advisory `flock(2)` doesn't interlock with SQLite's own page locking, so a peer rusqlite Connection could race the restore swap (CODE-005) or commit rows between the pre-delete backup snapshot and the cascade delete (CODE-007). With the SQLite-native primitive peers conflict for real: they wait on `busy_timeout`, then surface BUSY / RestoreDestinationLocked. `restore_from` opens a short-lived RW connection on the destination, issues `BEGIN EXCLUSIVE` (mapped to RestoreDestinationLocked on BUSY/LOCKED), and DROPS that connection BEFORE `tmp.persist` so SQLite releases its file handle on the old inode before the atomic rename. `delete_wallet` runs the pre-delete backup BEFORE acquiring EXCLUSIVE (rusqlite's online backup can't establish itself when the source conn is in an active write tx), then takes EXCLUSIVE for the cascade. A structured `info!` fires when the wallet's row footprint differs across the EXCLUSIVE acquisition so operators can correlate peer-committed-between-snapshot-and-lock scenarios. A `TODO(T-007/CODE-006)` marks the pre-backup flush slot that T-007 will fill inside the same EXCLUSIVE window. `fs2` is removed from `Cargo.toml`, the dep feature gate, and `Cargo.lock` (zero remaining transitive uses). The stale README "advisory-lock warning" and `backup.rs` rustdoc are rewritten to describe the real (SQLite-native) primitive. The Err arm that emitted `"advisory lock unsupported on this filesystem"` (CODE-010 false-positive downgrade) is gone with the flock block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tes (CODE-006)
`delete_wallet`'s pre-delete backup snapshot now contains every buffered
write that was pending at the moment of delete — not just the
already-persisted state. Without this fix, rollback-from-backup could
not recover a wallet whose only state lived in the merge buffer
(`FlushMode::Manual` + un-flushed).
Implementation: after draining the wallet's buffered changeset, the
delete path opens its own EXCLUSIVE tx and applies the changeset via
the newly extracted `apply_changeset_to_tx` helper (factored out of
`write_changeset_in_one_tx` so both call sites share schema-write
logic). The pre-flush commits BEFORE `run_auto_backup` runs, so the
snapshot captures the previously-buffered rows. On flush failure the
buffer is restored via the existing CMT-002 restore path and the
delete is aborted with no backup left behind.
The cascade-side backup still runs before the cascade's own
`BEGIN EXCLUSIVE` (rusqlite's `Backup::new` can't establish a backup
whose source connection holds an active write tx — `sqlite3_backup_step`
deadlocks against the in-flight EXCLUSIVE). A cross-process peer that
mutates the wallet between snapshot and lock is handled by the
existing post-EXCLUSIVE re-check, which logs the footprint change and
proceeds — the cascade reports zero per-table counts when the peer
beat it to every row.
Tests (TC-CODE-006-{1,2,3}):
- `pre_delete_backup_includes_buffered_writes`: renamed from
`pre_delete_backup_excludes_buffered_writes`; assertion flipped to
require the buffered rows to appear in the backup.
- `pre_flush_failure_preserves_buffer_and_skips_backup`: primes the
pre-flush injector to fail; asserts buffer survives, backup dir is
empty, wallet still in live DB, original error propagated.
- `peer_delete_between_backup_and_exclusive_returns_ok_with_zero_counts`:
arms a post-backup hook that opens a sibling raw connection and
deletes the wallet's metadata row in the gap between snapshot and
EXCLUSIVE; asserts the cascade returns Ok with zero counts and the
backup carries the pre-peer-deletion state.
Test-only helpers added (gated by `cfg(test)` / `__test-helpers`):
- `arm_post_backup_hook` — fires once between backup and EXCLUSIVE.
- `force_next_pre_flush_to_fail` — primes a one-shot pre-flush failure.
- `buffer_has_changeset_for_test` — probes the dirty-wallets set.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…11/014/016/019)
Five small fixes in the SQLite persistor layer, batched into one commit
to keep the diff atomic (4 of 5 tasks touch `sqlite/backup.rs`).
* CODE-009 — `backup::run_to` swaps the pre-`exists()` gate for
`tempfile::persist_noclobber`. The atomic check-and-rename closes
the TOCTOU window; `AlreadyExists` maps to the typed
`BackupDestinationExists` so the CLI's contract stays intact.
* CODE-011 — Sidecar paths in `apply_secure_permissions` and the
`backup::restore_from` WAL/SHM-unlink loop are built via
`OsString::push` (non-UTF-8 path bytes round-trip intact). The
`set_permissions` / `remove_file` calls run unconditionally and
treat `ErrorKind::NotFound` as a silent no-op, removing the
`exists()` TOCTOU gate.
* CODE-014 — After every `tempfile::persist*` rename in
`backup::run_to` and `backup::restore_from`, the destination's
parent directory is `fsync`-ed via the new `fsync_parent_dir`
helper (Unix only; no-op on non-Unix). Rustdoc `# Atomicity`
sections updated to mention the parent-dir fsync.
* CODE-016 — `run_integrity_check` switches from `query_row` to
`query_map`-collect. All diagnostic rows land in the
`IntegrityCheckFailed { report }` string joined with `\n`; a
mid-stream `SqliteFailure` (DatabaseCorrupt) is treated as
end-of-stream when at least one row arrived (and appended as a
marker), preserving the existing `atom_013` semantics.
* CODE-019 — `backup::prune` now increments `kept` on a
`remove_file` failure (the file is still on disk). Documented the
invariant `kept + removed.len() == total_eligible` and the
`failed_removals ⊆ kept` subset relationship on `PruneReport`.
Tests:
* TC-CODE-009-a/b — atomic overwrite refusal + Io variant mapping
(`sqlite_backup_restore.rs`).
* TC-CODE-011-a/b/c — non-UTF-8 sidecar chmod, missing-sidecar Ok
path, source-level regression for both call sites
(`sqlite_permissions.rs`).
* TC-CODE-014-a/b — source-level fsync invocation count + rustdoc
mention assertions (`sqlite_backup_restore.rs`).
* TC-CODE-016-a — multi-page corruption yields a multi-line
diagnostic report (`sqlite_open_integrity_check.rs`).
* TC-CODE-019-a — chmod-locked prune dir produces
`kept + removed == total` with `failed_removals` populated
(`sqlite_backup_restore.rs`).
T-010 fsync is verified at the source level (regression-grade
test asserting `fsync_parent_dir(` appears in `backup.rs` at least
three times — definition + run_to + restore_from). A real
crash-durability harness is impractical in unit-test scope; the
rustdoc + source-level checks pin the contract.
Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…qlite feature (CODE-020) Mark `platform-wallet` and `serde` `optional = true` and route them through the existing `sqlite` feature via `dep:` activators. Without the feature the bare crate ships only `thiserror`, `tracing` and `hex` (20 lines of cargo tree -e normal, down from 1124 with default features), so embedders that only want the future `SecretStore` submodule no longer transitively pull `platform-wallet` -> `dpp`/`drive`/`dashcore`. Add `tests/feature_flag_build.rs` as a TC-CODE-020-1 source-level pin so a future contributor flipping the manifest back to unconditional deps trips a unit test instead of only the bare-build CI gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ces (CODE-021) The `delete-wallet` subcommand was removed from the maintenance CLI in favour of the library-only `SqlitePersister::delete_wallet` / `delete_wallet_skip_backup` APIs. The README still listed the subcommand and described `--no-auto-backup` as opting out of the pre-delete auto-backup — both stale. Scope the destructive-subcommand paragraph to `restore` only and steer readers at the library API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (CODE-022/023) The concurrent-store delete-wallet regression test silently swallowed both the `delete_wallet` Result (with `let _ = ...`) and the post-delete `COUNT(*)` query failure (with `.unwrap_or(0)`). Either mishap would have masked a real schema or API regression behind a green test — `unwrap_or(0)` is especially toxic because it would let a broken schema look like a clean wipe. Promote both call sites to fail loud: `expect(...)` for the delete itself and `unwrap_or_else(|e| panic!(...))` for the COUNT (cheaper than `.expect(&format!(...))` so clippy stays quiet). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ruption (CODE-024) The single-page corruption helper guarded against `len > 4096`, but it then reads page 2 (bytes 4096..8192) — that bound is off by one and silently corrupts a partially-allocated page on a freshly-opened DB small enough to land in the [4097, 8191] window. Tighten the assertion to `>= 8192` so the precondition matches what the helper actually does, and include the observed length in the panic message. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…per (CODE-025) The CLI binary carried its own `default_auto_backup_dir_for_cli` mirror of the persister's helper — two definitions of the same `<db_dir>/backups/auto/` resolution, one drift bug away from the CLI and the library disagreeing about where auto-backups live. Promote the persister-side helper to `pub` (the originally preferred `pub(super)` is incompatible with `pub use` re-exports up to the crate root — documented in the helper's rustdoc), re-export it from `sqlite/mod.rs` and the crate root, and switch the CLI to call the library helper. Single source of truth, zero behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fix-up for 7dff6a4 — rustfmt prefers the closing `)` on a separate line when the raw string spans multiple lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_wallet N+1 load() (CODE-017) Add an Option<BTreeMap<WalletId, PlatformAddressSyncStartState>> cache (+ shielded snapshot under cfg(feature = "shielded")) on PlatformWalletManager, populated by the first call into `ensure_persisted_state_loaded` and consumed by `load_from_persistor` and `register_wallet`. `remove_wallet` drops the per-wallet entry so a future re-registration under the same id cannot apply stale state. Before: `register_wallet` called `persister.load()` per wallet — M wallets = M full reads, each holding the connection mutex. After: exactly one `load()` for the whole register round, whether triggered by an explicit `load_from_persistor` or lazily by the first `register_wallet`. bind_shielded still calls persister.load() per-wallet; routing it through the cache requires either a manager-side bind_shielded entrypoint or back-references from PlatformWallet to the manager — out of scope for this CODE-017 patch and tracked separately. New test file `tests/persister_load_cache.rs`: - TC-CODE-017-a: load_from_persistor + N register_wallet = 1 load - TC-CODE-017-b: cold-start (no load_from_persistor) + N register = 1 load - TC-CODE-017-c: remove_wallet invalidates only the removed slice; re-register under same id triggers no additional load Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(CODE-027) DRY out the 4 in-storage `SELECT 1 FROM sqlite_master WHERE name = 'refinery_schema_history'` probes into a single `migrations::has_schema_history` helper. Callers (`open`, `count_pending`, `restore_from` source + staged) now share one implementation; the `assert_schema_version_supported` table-presence check piggy-backs on the same helper. Adds TC-CODE-027-1 (in-module unit test): helper returns false on a fresh in-memory DB and true once the table is created. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…es! nits (CODE-028/029)
CODE-029 — extend `validate_config`:
* journal_mode = Memory → reject (rollback journal in RAM is
crash-unsafe for a wallet DB)
* journal_mode = Off → reject (no rollback journal at all)
* busy_timeout = 0 → warn via `tracing::warn!` (operator
almost certainly didn't mean it; a few tests want fail-fast, so we
don't reject)
Reuses the existing `ConfigInvalid { reason: &'static str }` variant —
no new variant or field, so wildcard-free matches on
`WalletStorageError` keep compiling.
CODE-028 — replace `matches!(self.config.flush_mode, FlushMode::X)`
with `==`/`!=` at the two persister sites. FlushMode already derives
`PartialEq, Eq`, so this is a pure readability cleanup with no semantic
change.
Tests added (sqlite_persist_roundtrip.rs):
* TC-CODE-029-1 — journal_mode=Memory rejected with typed
ConfigInvalid; DB not created.
* TC-CODE-029-2 — journal_mode=Off rejected with typed
ConfigInvalid; DB not created.
* TC-CODE-029-3 — busy_timeout=Duration::ZERO opens successfully and
emits the expected `busy_timeout=0` warning (verified via
`tracing_test::traced_test`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… in favour of --no-auto-backup (CODE-030)
The CLI used to overload an empty string passed to `--auto-backup-dir`
as "disable auto-backup". That's a non-standard convention and
collides with the dedicated `--no-auto-backup` flag already exposed by
`migrate` and `restore`.
Deprecation strategy: keep the empty-string form parsing for one
release so existing operator scripts don't break overnight, but emit a
loud `warning: ... deprecated; pass --no-auto-backup instead` on
stderr the moment the legacy form is used. The arg's help text and the
README's CLI section steer new callers to `--no-auto-backup`.
Tests (sqlite_cli_smoke.rs):
* TC-CODE-030-1a — `migrate --no-auto-backup` succeeds, emits the
expected stderr notice, and writes no pre-migration snapshot.
* TC-CODE-030-1b — `--auto-backup-dir "" migrate --no-auto-backup`
still succeeds but emits the deprecation warning steering to
`--no-auto-backup`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (CODE-012/013) Mark the two half-wired-callback gaps thepastaclaw flagged on PR #3625 with TODO comments that pin them to follow-up issues. Both gaps pre-existed on v3.1-dev — #3625 didn't introduce them — and a proper fix needs FFI registration plumbing (paired (fn, free_fn) enforcement) that's out of scope for this PR. * CODE-012 — FFIPersister::load: on_load_wallet_list_fn / on_load_wallet_list_free_fn must be paired at registration time. * CODE-013 — FFIPersister::get_core_tx_record: same pairing requirement for on_get_core_tx_record_fn / on_get_core_tx_record_free_fn. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tegration tests (CODE-008) Add `tests/round_trip_consumer.rs` — the meta-fix CI safety net for PR #3625. Five `#[tokio::test]`s exercise a real `PlatformWalletManager` (consumer side from `rs-platform-wallet`) against a real `SqlitePersister` (this crate) so every consumer↔ persister contract drift now fails CI instead of slipping through: * TC-CODE-008-1 — register_wallet → drop → reopen: wallet_metadata + account_registrations + account_address_pools survive. * TC-CODE-008-2 — platform_addresses round-trip through persister.store + list_per_wallet across reopen. * TC-CODE-008-3 — identity_keys + token_balances round-trip under a registered (wallet, identity) pair; regression guard for CODE-002 (sentinel WalletId::default() FK violation). * TC-CODE-008-4 — remove_wallet cascades through storage for the removed wallet but leaves the surviving sibling untouched; regression guard for CODE-003 (remove_wallet never propagated to disk). * TC-CODE-008-5 — boot the manager twice over the same DB; the persisted wallets are still on disk after a clean reopen + a second load_from_persistor() pass; regression guard for CODE-001. Per user direction ("If possible, put it into persister crate") the test lives here so the dev-dep cycle stays one-way: `platform-wallet` ships no dependency on `platform-wallet-storage`, while the storage crate is free to pull `platform-wallet` into `[dev-dependencies]` for integration coverage. Dev-deps added: `dash-sdk` with `mocks` + `wallet` features (needed by `SdkBuilder::new_mock().build()` for the manager) and a direct `tokio` so `#[tokio::test]` resolves the macro by name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…entry point (PROJ-001) The FFI `platform_wallet_manager_identity_sync_register_identity` was still routing through the orphan-identity shim (`IdentitySyncManager::register_identity`) which records `Option<WalletId>::None` and ultimately persists the resulting `TokenBalanceChangeSet` under the all-zero `WalletId` sentinel. That defeats the purpose of the wallet-aware path (`register_identity_with_wallet`) and breaks the `wallet_metadata → identities → token_balances` cascade for any token balance learned through this FFI. This commit: * Adds a required `wallet_id_ptr` parameter (32 bytes) to the FFI entry point, rejecting null with `ErrorNullPointer` and the all-zero sentinel with `ErrorInvalidParameter`. * Routes to `register_identity_with_wallet(identity_id, Some(wallet_id), token_ids)` so the recorded parent wallet flows through every subsequent `persister.store(wallet_id, …)` call. * Updates the Swift wrapper `registerIdentityForTokenSync` to take the new `walletId: Data` argument, validate length on the Swift side, and marshal the buffer through the new FFI parameter. * Updates the SwiftExampleApp caller to source the parent wallet from `identity.wallet?.walletId`, skipping the registration (and the display-only fetch still works) when the identity is out-of-wallet. * Updates the doc-comment usage example in `TokenActions.swift`. BREAKING CHANGE: the C ABI for `platform_wallet_manager_identity_sync_register_identity` gains a new `wallet_id_ptr` parameter between `identity_id_ptr` and `token_ids_ptr`. Swift callers must pass the parent wallet id. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ities upsert Document that the `wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id)` upsert clause is the intended merge semantic for orphan-identity-to-wallet promotion (NULL `wallet_id` is allowed per the V002 CODE-002 design). Existing `wallet_id` survives a re-upsert; a freshly known `wallet_id` fills NULL on promotion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… orphan-cleanup contract Document that orphan `token_balances` cleanup is the host's responsibility — no automatic prune API is offered. V002 cascades only through `identities` (the `wallet_id` column was dropped from `token_balances`), so hosts that delete identities out-of-band must prune the corresponding rows themselves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 Dispatched macOS CI for the PROJ-001 FFI ABI break verification:
Note: PR base is |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/platform-wallet-sqlite-persistor #3743 +/- ##
=========================================================================
+ Coverage 87.17% 87.24% +0.07%
=========================================================================
Files 2607 2613 +6
Lines 319489 319709 +220
=========================================================================
+ Hits 278507 278929 +422
+ Misses 40982 40780 -202
🚀 New features to boost your workflow:
|
… (CODE-017 follow-up) Helper added in 6934404 was never called, tripping -D dead-code on macOS strict profile and blocking Swift SDK build on PR #3743. Wiring fulfills the original perf intent (drop N+1 persister.load() at shielded bind time). - Add PlatformWallet::bind_shielded_with_snapshot taking a pre-loaded Arc<ShieldedSyncStartState>; bind_shielded keeps its public signature by delegating with None. - Promote PlatformWalletManager::cached_persisted_shielded from pub(super) to pub so the FFI layer (the only direct host of bind_shielded) can fetch the shared snapshot once per call. - platform_wallet_manager_bind_shielded now pulls the cached snapshot alongside the coordinator and feeds it through, so every wallet's bind reuses the same Arc instead of issuing its own load(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ar chmod test (APFS compat) tc_code_011_a previously used raw non-UTF-8 bytes (0xff 0xfe ...) which APFS rejects with EILSEQ. The test's real intent is to verify OsString sidecar concatenation + chmod survive non-ASCII paths — a valid-UTF-8 multi-byte sequence (ÿþ → \xc3\xbf\xc3\xbe) exercises the same codepath on both Linux ext4/btrfs and macOS APFS without losing coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on persistence trait Trait-level contract: passing WalletId::default() to PlatformWalletPersistence methods marks the entity as orphan (no parent wallet). V002 schema permits this; higher layers (FFI) may enforce stricter rules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ommit/delete reports Follow-up to d0ed23b: extend the WalletId::default() = orphan convention to PlatformWalletPersistence::flush parameter, commit_writes return doc, CommitReport field docs, and DeleteWalletReport.wallet_id field.
…epastaclaw-hardening # Conflicts: # packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
…3) — moved to follow-up PR Phase 2 of PR #3743 slim. Restores consumer-side state to origin/feat/platform-wallet-sqlite-persistor for these files. The trait surface (delete_wallet, commit_writes, typed PersistenceError) stays so the storage crate still compiles; only the consumer call-sites and caching/retry plumbing revert. - src/error.rs: drop PersistorMissingWalletRehydration + WalletRegistrationFailed variants - src/manager/load.rs: drop CODE-001 orphan-bucket gate and CODE-017 cache plumbing - src/manager/mod.rs: drop CODE-017 cache fields on PlatformWalletManager - src/manager/wallet_lifecycle.rs: drop CODE-018 retry, CODE-003 wired remove_wallet call site, cache-invalidation hook - src/wallet/platform_wallet.rs: drop CODE-017 bind_shielded_with_snapshot shim - FFI src/shielded_sync.rs: drop CODE-017 cached_persisted_shielded wiring Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ck sites (CODE-012/013)" This reverts commit 6602cdb.
…lush + commit/delete reports" This reverts commit 5747a98.
…vention on persistence trait" This reverts commit d0ed23b.
…8 consumer-integration test moved to follow-up PR) This test brings rs-platform-wallet as a dev-dep specifically to exercise the consumer flow against the storage backend, and it depends on the PROJ-001 wallet-aware register_identity_with_wallet helper. Now that the PROJ-001 consumer-side wiring has been reverted from this PR (kept for the follow-up consolidated PR), the test no longer compiles. Other storage-crate-internal round-trip coverage (sqlite_persist_roundtrip, sqlite_load_reconstruction, etc.) remains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
…istor' into fix/3625-thepastaclaw-hardening
…6 bump Phase 1.5 merge of origin/v3.1-dev (#3746 — official workspace version bump 3.1.0-dev.5 → 3.1.0-dev.6) updated workspace.package.version but the lock-file entry for platform-wallet-storage wasn't regenerated. CI's --locked clippy flag caught the mismatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR lands the V002 SQLite-backed wallet persister with typed PersistenceError, EXCLUSIVE-based cross-process locking for restore/delete, atomic backup staging, and structured CommitReport/DeleteWalletReport. One genuinely problematic spot: the V002 identities upsert's COALESCE order contradicts its own doc comment and silently rebinds identities cross-wallet (cascading children with them). Several other findings are intentional, documented tradeoffs (restore-rename gap, pre-cascade backup) — keep as suggestions, not blockers. Trait-default findings are dropped: the trait doc explicitly documents the no-op default as the contract.
🔴 1 blocking | 🟡 6 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:22-32: V002 identities upsert COALESCE order contradicts the doc comment and silently re-parents identities
The comment at lines 22-24 spells out the intended semantic: "Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL." The SQL at line 29 does the opposite — `wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id)` prefers the new (`excluded`) value whenever it is non-NULL. Truth table:
- existing=NULL, new=X → X (orphan promotion, matches intent)
- existing=X, new=NULL → X (matches intent)
- existing=A, new=B → **B** (silent rebind, NOT what the comment says)
- existing=NULL, new=NULL → NULL (matches intent)
Because V002 makes identity-owned child tables (`identity_keys`, `token_balances`, `dashpay_*`) cascade through `identities(identity_id)`, a re-upsert of the same `identity_id` under a different wallet silently moves the identity AND all its descendants to the new wallet without any cross-check or warning. This is being introduced by this PR's new schema. The doc and the code must agree — either swap the COALESCE arguments to `COALESCE(identities.wallet_id, excluded.wallet_id)` (truly preserve existing, matching the comment) or rewrite the comment to describe the rebind behavior and document why it is intentional.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:11-53: identities::apply does not cross-check entry.wallet_id against the flush scope (asymmetric with identity_keys::apply)
`identity_keys::apply` (identity_keys.rs:97-111) rejects any `IdentityKeyEntry` whose `wallet_id` disagrees with the flush scope via `WalletStorageError::IdentityKeyEntryMismatch`. The equivalent writer in `identities::apply` blindly writes the scope into the typed `identities.wallet_id` column while the blob payload independently encodes `IdentityEntry.wallet_id` — so the two can disagree silently. The typed column drives cascade + per-wallet load filter, but `managed_identity_from_entry` later favors the blob's `wallet_id` via `entry.wallet_id.or(Some(*wallet_id))`. The two representations diverge on disk with no detection. Mirror the cross-check from `identity_keys::apply`, or explicitly document the blob's `wallet_id` as informational-only and the typed column as the source of truth.
In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:311-327: restore_from releases the EXCLUSIVE lock before the final rename — narrow contract gap with the documented cross-process guarantee
Step 7 drops `dest_lock_conn` (releasing SQLite's EXCLUSIVE) at lines 317-323 and step 8 then renames the staged file over `dest_db_path` at lines 325-327. Between those two statements the destination is fully unlocked. A peer that was blocked on the EXCLUSIVE lock can acquire its own write transaction in the gap and have its commit replaced by the atomic rename — a lost-update window.
The step-7 comment correctly explains why the lock is released early (otherwise the open handle pins the unlinked old inode after the rename, and on some filesystems the rename itself can fail). This is a real tradeoff that has no perfect resolution, but the PR's claim that the EXCLUSIVE lock protects "across the entire restore body" overstates the contract. Either tighten the wording to acknowledge the documented rename window and reference it from the host-app restore docs, or add an explicit `tracing::warn!` immediately before `tmp.persist` so the audit trail captures the gap. Operators must be told to quiesce peers before invoking restore.
In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:461-509: Pre-delete auto-backup is taken before the cascade EXCLUSIVE — cross-process mutations in the gap are deleted but not in the backup
`delete_wallet_inner` snapshots via `run_auto_backup` at lines 464-469 before acquiring `BEGIN EXCLUSIVE` at line 486. The lines 422-427 comment correctly explains the constraint: rusqlite's `Backup::new` deadlocks if the source connection holds its own active write tx. The post-EXCLUSIVE recheck at lines 494-509 only logs an `info` when `wallet_metadata` existence flips — a peer that updates rows without changing existence (the common case) goes unnoticed. So the cascade can remove rows that the auto-backup never captured, breaking the "safe rollback via backup" story under cross-process contention.
This is an inherent rusqlite API limitation rather than a fixable race, but the `DeleteWalletReport` advertises the backup as a recovery point. Surface the gap to callers — either extend `DeleteWalletReport` with a `peer_mutation_observed` field driven by a post-EXCLUSIVE row-count comparison, or document the limit in the type-level docs so consumers can decide whether to checkpoint a second post-cascade snapshot.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:571-579: commit_writes is a hard no-op in FlushMode::Immediate even when transient errors have left dirty buffers
`commit_writes_inner` returns an empty `CommitReport` immediately whenever `flush_mode == FlushMode::Immediate` (lines 577-579). The trait doc at traits.rs:358-377 reads "flush every dirty wallet's buffered changeset", and the canonical SQLite backend can still hold dirty buffers in Immediate mode: `store()` retries through `handle_flush_error`, which restores the failed changeset to the buffer on transient errors. Callers using the bulk API as a retry mechanism will silently skip those buffers in Immediate mode — exactly the case the API is meant to flush.
Enumerate dirty wallets unconditionally and run `flush_inner` per wallet regardless of `flush_mode`; the in-process state is the source of truth, not the configured default flush strategy. Alternatively, document that Immediate-mode `commit_writes` is by design a no-op and direct callers to use `flush(wallet_id)` to retry transient failures.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:104-112: Sentinel-scope shortcut disables the entry/scope wallet_id cross-check entirely instead of requiring entry.wallet_id == None
When the outer `wallet_id` scope is the all-zero sentinel, the `if !scope_is_sentinel && entry_wallet_id != *wallet_id` short-circuits, allowing an `IdentityKeyEntry.wallet_id` carrying ANY value to be written under that scope without raising `IdentityKeyEntryMismatch`. The comment says the intent is to let identity-only callers (no known parent) write without tripping the check, but the relaxation is broader than "unknown parent": a caller can stash a mismatched `wallet_id` into the serialized blob while the typed row is keyed only by `(identity_id, key_id)` post-V002. A stricter check would require `entry.wallet_id == None` (orphan-only) when the scope is the sentinel, matching the actual identity-only contract. Otherwise document the intentional relaxation and which call sites are allowed to use it.
In `packages/rs-platform-wallet/src/changeset/traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/traits.rs:142-152: From<String>/From<&str> for PersistenceError silently classifies every legacy call site as Fatal
The new `PersistenceError::backend(...)` helper preserves the legacy `From<String>` / `From<&str>` paths, and they default to `PersistenceErrorKind::Fatal`. The FFI persister at `rs-platform-wallet-ffi/src/persistence.rs` already uses `PersistenceError::backend(format!(...))` for several conditions, all of which now default to Fatal. That's correct for genuinely-fatal cases (tag-mismatch decoding), but a future FFI call site stringifying a busy/locked condition would be misclassified silently, defeating the typed-kind contract this PR introduces. Either remove the `From` impls so every call site has to pick a kind via `backend_with_kind`, or do a single audit pass over the FFI persister to convert each site to an explicit kind. Tracking this in the consumer-hardening follow-up PR is acceptable if noted explicitly.
Out-of-scope follow-up suggestions (5)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- All-zero WalletId sentinel is coerced to NULL on write but matched literally on read/delete —
wallet_id_to_param(identities.rs:58-64) maps[0u8;32]→ NULL, butload_state(identities.rs:114-116) bindswallet_id.as_slice()underWHERE wallet_id = ?1, so a wallet using the sentinel id sees its own identities disappear on load.count_rows_for_wallet_sql(..., ViaIdentity)JOINs through the same condition, sodelete_walletreports zero rows removed for descendant tables and the cascade silently leaves orphan rows. Unreachable from any seed-derived id; operator/test paths picking[0u8;32]directly are the only realistic exposure, and they are tracked under #3745.- Follow-up: Track under #3745 — either reject the sentinel at the trait boundary or align read/delete with the NULL representation used on write.
- V002 migration aborts without typed reclassification on cross-wallet identity_id collisions in V001 data — V001 keyed
identitiesby(wallet_id, identity_id); V002 makesidentity_idthe sole PK. TheINSERT INTO identities_v2 ... SELECT ... FROM identitiesinV002__cascade_only_identity_refs.rswill UNIQUE-violate if any identity participated in two wallets in V001, surfacing a bare rusqlite UNIQUE error rather than a typedMigrationRequiresManualCleanup(only the token_balances sentinel guard gets that treatment). Pre-existing dev-data shape risk, but operators hitting it would have no clear remediation path.- Follow-up: Open a follow-up to pre-check for cross-wallet identity_id collisions in
run_for_openand reclassify intoMigrationRequiresManualCleanupwith the offending ids, or document the manual cleanup query in the migration's comment block.
- Follow-up: Open a follow-up to pre-check for cross-wallet identity_id collisions in
- FFI result still flattens PersistenceErrorKind to a single string at the Swift/C boundary —
PlatformWalletFFIResult::from(PersistenceError)inrs-platform-wallet-ffi/src/error.rscollapses every kind into a singleErrorWalletOperationwith a stringified message. The Swift host cannot branch on retry-ability without parsing text — exactly the situation the new typed kind was added to eliminate. Pre-existing limitation; the PR description explicitly defers consumer-side hardening.- Follow-up: Extend
PlatformWalletFFIResult(or add dedicatedErrorWalletPersistenceTransient/ErrorWalletPersistenceConstraintcodes) in the consumer-hardening follow-up PR.
- Follow-up: Extend
- LockPoisoned fatal-classification branch has no end-to-end test (TC-P2-008) —
error.rs::is_transientandpersister.rs::handle_flush_errorcarry explicitTODO(qa): TC-P2-008annotations stating theLockPoisonedflow is only manually verified. The__test-helpersforce_next_flush_to_failinjector could prime a syntheticLockPoisonedand assert buffer-wipe +PersistenceError::LockPoisonedsurface deterministically.- Follow-up: Add TC-P2-008 in a test-coverage follow-up PR.
- Manual-mode store() can leak into the buffer between delete_wallet's re-drain and return —
delete_wallet_innerre-drains the buffer at persister.rs:535 after commit, butstore()inFlushMode::Manualdoesn't take the conn mutex, so a same-process caller can enqueue a fresh changeset between line 535 and the function's return. The documented contract ("discarded after delete commits") holds for stores during the delete window but not strictly until function return. Narrow window, low practical risk.- Follow-up: Track as a documentation refinement on the N-4 contract, or guard the re-drain with the buffer mutex held until function return.
| // INTENTIONAL(SEC-001): NULL wallet_id allowed per CODE-002 design; | ||
| // COALESCE upsert is the intended merge semantic for orphan-identity-to-wallet promotion. | ||
| // Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL. | ||
| let mut stmt = tx.prepare_cached( | ||
| "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ | ||
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | ||
| VALUES (?1, ?2, ?3, ?4, 0) \ | ||
| ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ | ||
| ON CONFLICT(identity_id) DO UPDATE SET \ | ||
| wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id), \ | ||
| wallet_index = excluded.wallet_index, \ | ||
| entry_blob = excluded.entry_blob, \ | ||
| tombstoned = 0", |
There was a problem hiding this comment.
🔴 Blocking: V002 identities upsert COALESCE order contradicts the doc comment and silently re-parents identities
The comment at lines 22-24 spells out the intended semantic: "Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL." The SQL at line 29 does the opposite — wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id) prefers the new (excluded) value whenever it is non-NULL. Truth table:
- existing=NULL, new=X → X (orphan promotion, matches intent)
- existing=X, new=NULL → X (matches intent)
- existing=A, new=B → B (silent rebind, NOT what the comment says)
- existing=NULL, new=NULL → NULL (matches intent)
Because V002 makes identity-owned child tables (identity_keys, token_balances, dashpay_*) cascade through identities(identity_id), a re-upsert of the same identity_id under a different wallet silently moves the identity AND all its descendants to the new wallet without any cross-check or warning. This is being introduced by this PR's new schema. The doc and the code must agree — either swap the COALESCE arguments to COALESCE(identities.wallet_id, excluded.wallet_id) (truly preserve existing, matching the comment) or rewrite the comment to describe the rebind behavior and document why it is intentional.
source: ['claude', 'codex']
| // 7. Release the SQLite-native EXCLUSIVE lock BEFORE the rename. | ||
| // Dropping `dest_lock_conn` causes SQLite to close its file | ||
| // handle on the old inode; if we kept it alive across `persist` | ||
| // the handle would point at the unlinked old inode while the | ||
| // new DB took its place — peers reopening would race the rename | ||
| // and (on some filesystems) the rename itself could fail. | ||
| if let Some(conn) = dest_lock_conn.take() { | ||
| // Best-effort rollback of the empty EXCLUSIVE tx; an error here | ||
| // means SQLite is already in trouble and `drop(conn)` covers | ||
| // the rest. Silent because the conn is about to drop anyway. | ||
| let _ = conn.execute_batch("ROLLBACK"); | ||
| drop(conn); | ||
| } | ||
|
|
||
| // 8. Persist atomically over the destination. | ||
| tmp.persist(dest_db_path) | ||
| .map_err(|e| WalletStorageError::Io(e.error))?; |
There was a problem hiding this comment.
🟡 Suggestion: restore_from releases the EXCLUSIVE lock before the final rename — narrow contract gap with the documented cross-process guarantee
Step 7 drops dest_lock_conn (releasing SQLite's EXCLUSIVE) at lines 317-323 and step 8 then renames the staged file over dest_db_path at lines 325-327. Between those two statements the destination is fully unlocked. A peer that was blocked on the EXCLUSIVE lock can acquire its own write transaction in the gap and have its commit replaced by the atomic rename — a lost-update window.
The step-7 comment correctly explains why the lock is released early (otherwise the open handle pins the unlinked old inode after the rename, and on some filesystems the rename itself can fail). This is a real tradeoff that has no perfect resolution, but the PR's claim that the EXCLUSIVE lock protects "across the entire restore body" overstates the contract. Either tighten the wording to acknowledge the documented rename window and reference it from the host-app restore docs, or add an explicit tracing::warn! immediately before tmp.persist so the audit trail captures the gap. Operators must be told to quiesce peers before invoking restore.
source: ['claude', 'codex']
| @@ -427,21 +468,62 @@ impl SqlitePersister { | |||
| AutoBackupOperation::DeleteWallet, | |||
| )? | |||
| }; | |||
| let tx = conn.transaction()?; | |||
|
|
|||
| // Test-only hook: fires between the backup snapshot and | |||
| // the cascade EXCLUSIVE so TC-CODE-006-3 can simulate a | |||
| // cross-process peer that mutates `wallet_metadata` in | |||
| // the gap rusqlite's Backup API forces us to leave open. | |||
| #[cfg(any(test, feature = "__test-helpers"))] | |||
| self.consume_post_backup_hook(); | |||
|
|
|||
| // SQLite-native EXCLUSIVE for the cascade window. Excludes | |||
| // cross-process peers (other rusqlite Connections, sibling | |||
| // `SqlitePersister`s) that would otherwise commit rows for | |||
| // `wallet_id` between the backup snapshot and the cascade. | |||
| // The in-process mutex on `conn` alone never gave that | |||
| // guarantee. Peers waiting on the lock back off via | |||
| // SQLite's `busy_timeout`. | |||
| let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Exclusive)?; | |||
|
|
|||
| // Re-confirm existence post-EXCLUSIVE: a peer could have | |||
| // either inserted (raising the wallet from non-existent to | |||
| // existent) or deleted (vanishing it) between the backup | |||
| // and the lock acquisition. If a peer just deleted the | |||
| // wallet, the cascade is a no-op — we still commit because | |||
| // the operator's intent is satisfied. | |||
| let post_lock_exists = tx | |||
| .query_row( | |||
| "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", | |||
| rusqlite::params![wallet_id.as_slice()], | |||
| |_| Ok(()), | |||
| ) | |||
| .optional()? | |||
| .is_some(); | |||
| if post_lock_exists != exists_in_db { | |||
| tracing::info!( | |||
| wallet_id = %hex::encode(wallet_id), | |||
| pre_lock_exists = exists_in_db, | |||
| post_lock_exists, | |||
| "wallet_metadata footprint changed across delete_wallet EXCLUSIVE acquisition" | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: Pre-delete auto-backup is taken before the cascade EXCLUSIVE — cross-process mutations in the gap are deleted but not in the backup
delete_wallet_inner snapshots via run_auto_backup at lines 464-469 before acquiring BEGIN EXCLUSIVE at line 486. The lines 422-427 comment correctly explains the constraint: rusqlite's Backup::new deadlocks if the source connection holds its own active write tx. The post-EXCLUSIVE recheck at lines 494-509 only logs an info when wallet_metadata existence flips — a peer that updates rows without changing existence (the common case) goes unnoticed. So the cascade can remove rows that the auto-backup never captured, breaking the "safe rollback via backup" story under cross-process contention.
This is an inherent rusqlite API limitation rather than a fixable race, but the DeleteWalletReport advertises the backup as a recovery point. Surface the gap to callers — either extend DeleteWalletReport with a peer_mutation_observed field driven by a post-EXCLUSIVE row-count comparison, or document the limit in the type-level docs so consumers can decide whether to checkpoint a second post-cascade snapshot.
source: ['claude', 'codex']
| fn commit_writes_inner(&self) -> Result<CommitReport, PersistenceError> { | ||
| let mut report = CommitReport { | ||
| succeeded: Vec::new(), | ||
| failed: Vec::new(), | ||
| still_pending: Vec::new(), | ||
| }; | ||
| if matches!(self.config.flush_mode, FlushMode::Immediate) { | ||
| if self.config.flush_mode == FlushMode::Immediate { | ||
| return Ok(report); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: commit_writes is a hard no-op in FlushMode::Immediate even when transient errors have left dirty buffers
commit_writes_inner returns an empty CommitReport immediately whenever flush_mode == FlushMode::Immediate (lines 577-579). The trait doc at traits.rs:358-377 reads "flush every dirty wallet's buffered changeset", and the canonical SQLite backend can still hold dirty buffers in Immediate mode: store() retries through handle_flush_error, which restores the failed changeset to the buffer on transient errors. Callers using the bulk API as a retry mechanism will silently skip those buffers in Immediate mode — exactly the case the API is meant to flush.
Enumerate dirty wallets unconditionally and run flush_inner per wallet regardless of flush_mode; the in-process state is the source of truth, not the configured default flush strategy. Alternatively, document that Immediate-mode commit_writes is by design a no-op and direct callers to use flush(wallet_id) to retry transient failures.
source: ['codex']
| cs: &IdentityChangeSet, | ||
| ) -> Result<(), WalletStorageError> { | ||
| if !cs.identities.is_empty() { | ||
| // V002: PK is `identity_id` alone; `wallet_id` is nullable | ||
| // and links the identity to its parent wallet for cascade. | ||
| // The sentinel-zero wallet id (`[0u8; 32]`) is the legacy | ||
| // placeholder for "no parent wallet known" — stored as NULL | ||
| // so the FK to `wallet_metadata` doesn't activate. | ||
| // INTENTIONAL(SEC-001): NULL wallet_id allowed per CODE-002 design; | ||
| // COALESCE upsert is the intended merge semantic for orphan-identity-to-wallet promotion. | ||
| // Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL. | ||
| let mut stmt = tx.prepare_cached( | ||
| "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ | ||
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | ||
| VALUES (?1, ?2, ?3, ?4, 0) \ | ||
| ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ | ||
| ON CONFLICT(identity_id) DO UPDATE SET \ | ||
| wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id), \ | ||
| wallet_index = excluded.wallet_index, \ | ||
| entry_blob = excluded.entry_blob, \ | ||
| tombstoned = 0", | ||
| )?; | ||
| let wallet_id_param = wallet_id_to_param(wallet_id); | ||
| for (id, entry) in &cs.identities { | ||
| let payload = blob::encode(entry)?; | ||
| stmt.execute(params![ | ||
| wallet_id.as_slice(), | ||
| entry.identity_index.map(i64::from), | ||
| id.as_slice(), | ||
| wallet_id_param, | ||
| entry.identity_index.map(i64::from), | ||
| payload, | ||
| ])?; | ||
| } | ||
| } | ||
| if !cs.removed.is_empty() { | ||
| let mut stmt = tx.prepare_cached( | ||
| "UPDATE identities SET tombstoned = 1 WHERE wallet_id = ?1 AND identity_id = ?2", | ||
| )?; | ||
| let mut stmt = | ||
| tx.prepare_cached("UPDATE identities SET tombstoned = 1 WHERE identity_id = ?1")?; | ||
| for id in &cs.removed { | ||
| stmt.execute(params![wallet_id.as_slice(), id.as_slice()])?; | ||
| stmt.execute(params![id.as_slice()])?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: identities::apply does not cross-check entry.wallet_id against the flush scope (asymmetric with identity_keys::apply)
identity_keys::apply (identity_keys.rs:97-111) rejects any IdentityKeyEntry whose wallet_id disagrees with the flush scope via WalletStorageError::IdentityKeyEntryMismatch. The equivalent writer in identities::apply blindly writes the scope into the typed identities.wallet_id column while the blob payload independently encodes IdentityEntry.wallet_id — so the two can disagree silently. The typed column drives cascade + per-wallet load filter, but managed_identity_from_entry later favors the blob's wallet_id via entry.wallet_id.or(Some(*wallet_id)). The two representations diverge on disk with no detection. Mirror the cross-check from identity_keys::apply, or explicitly document the blob's wallet_id as informational-only and the typed column as the source of truth.
source: ['claude']
| if let Some(entry_wallet_id) = entry.wallet_id { | ||
| if entry_wallet_id != *wallet_id { | ||
| // Treat the all-zero sentinel scope as "any wallet" so | ||
| // identity-only callers (no parent wallet) don't trip | ||
| // the cross-check. | ||
| let scope_is_sentinel = wallet_id.iter().all(|b| *b == 0); | ||
| if !scope_is_sentinel && entry_wallet_id != *wallet_id { | ||
| return Err(WalletStorageError::IdentityKeyEntryMismatch); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Sentinel-scope shortcut disables the entry/scope wallet_id cross-check entirely instead of requiring entry.wallet_id == None
When the outer wallet_id scope is the all-zero sentinel, the if !scope_is_sentinel && entry_wallet_id != *wallet_id short-circuits, allowing an IdentityKeyEntry.wallet_id carrying ANY value to be written under that scope without raising IdentityKeyEntryMismatch. The comment says the intent is to let identity-only callers (no known parent) write without tripping the check, but the relaxation is broader than "unknown parent": a caller can stash a mismatched wallet_id into the serialized blob while the typed row is keyed only by (identity_id, key_id) post-V002. A stricter check would require entry.wallet_id == None (orphan-only) when the scope is the sentinel, matching the actual identity-only contract. Otherwise document the intentional relaxation and which call sites are allowed to use it.
source: ['claude']
| impl From<String> for PersistenceError { | ||
| fn from(msg: String) -> Self { | ||
| Self::Backend(msg) | ||
| Self::backend(StringSource(msg)) | ||
| } | ||
| } | ||
|
|
||
| impl From<&str> for PersistenceError { | ||
| fn from(msg: &str) -> Self { | ||
| Self::Backend(msg.to_string()) | ||
| Self::backend(StringSource(msg.to_string())) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: From/From<&str> for PersistenceError silently classifies every legacy call site as Fatal
The new PersistenceError::backend(...) helper preserves the legacy From<String> / From<&str> paths, and they default to PersistenceErrorKind::Fatal. The FFI persister at rs-platform-wallet-ffi/src/persistence.rs already uses PersistenceError::backend(format!(...)) for several conditions, all of which now default to Fatal. That's correct for genuinely-fatal cases (tag-mismatch decoding), but a future FFI call site stringifying a busy/locked condition would be misclassified silently, defeating the typed-kind contract this PR introduces. Either remove the From impls so every call site has to pick a kind via backend_with_kind, or do a single audit pass over the FFI persister to convert each site to an explicit kind. Tracking this in the consumer-hardening follow-up PR is acceptable if noted explicitly.
source: ['claude']
| @@ -0,0 +1,149 @@ | |||
| //! V002 — cascade-only references on identity-owned tables (CODE-002). | |||
There was a problem hiding this comment.
Merge these DB settings into v001 migration, we haven't merged it yet, no need for additional migrations
| #[arg(long, value_name = "PATH", global = true)] | ||
| db: Option<PathBuf>, | ||
| /// Auto-backup directory. Pass empty string to disable. | ||
| /// Auto-backup directory. The empty-string ("") form is |
There was a problem hiding this comment.
remove ALL deprecated mechanisms, we didn't merge yet, so nothing to deprecate. Check whole PR and apply this everywhere.
| pub fn apply( | ||
| tx: &Transaction<'_>, | ||
| wallet_id: &WalletId, | ||
| _wallet_id: &WalletId, |
There was a problem hiding this comment.
Validate wallet_id matches identity ID. In case of dashpay, only identities that are inside the wallet can be used. Add a comment with that precondition.
| pub fn fetch( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| _wallet_id: &WalletId, |
There was a problem hiding this comment.
We need to validae wallet id, to ensure we don't return items belonging to other wallet. default() / NULL wallet ID is also a wallet ID :).
| ON CONFLICT(wallet_id, identity_id, key_id) DO UPDATE SET \ | ||
| (identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ | ||
| VALUES (?1, ?2, ?3, ?4, NULL) \ | ||
| ON CONFLICT(identity_id, key_id) DO UPDATE SET \ |
There was a problem hiding this comment.
This is disputable. I can imagine a case where we have the same identity inserted multiple times, once to NULL wallet, and to multiple other wallets. We should add validation to ensure we update correct identity, in correct wallet, and fail on mismatch. Apply this validation in all similar places.
| /// their direct `wallet_id` column and reach the parent wallet only | ||
| /// via the cascading FK chain `wallet_metadata → identities → …`. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum WalletScope { |
There was a problem hiding this comment.
We can merge v002 schema into v001 schema, so we don't need to track these schema changes.
| pub fn apply( | ||
| tx: &Transaction<'_>, | ||
| wallet_id: &WalletId, | ||
| _wallet_id: &WalletId, |
There was a problem hiding this comment.
Always validate WalletId to avoid reading/modifying invalid wallet.
| use crate::sqlite::persister::{PruneReport, RetentionPolicy}; | ||
| use crate::sqlite::util::permissions::apply_secure_permissions; | ||
|
|
||
| /// CODE-014: fsync the parent directory of `path` on Unix so the |
There was a problem hiding this comment.
We need a round-trip test for backup/restore if we don't have it yet.
| /// [`WalletStorageError::MigrationRequiresManualCleanup`] so operators | ||
| /// see what the migration refused on instead of a raw rusqlite error. | ||
| /// | ||
| /// V002 reshapes identity-owned tables, which involves |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Latest delta ff00639..4dc7a3a is only a Cargo.lock workspace version bump (3.1.0-dev.5 → 3.1.0-dev.6) with no Rust source changes. This is a cumulative review carrying forward all prior findings. Prior-finding status: identities V002 upsert re-parent (STILL VALID, blocking), restore_from EXCLUSIVE release window (STILL VALID, suggestion — code comment acknowledges as tradeoff), delete_wallet pre-EXCLUSIVE backup gap (STILL VALID, suggestion — same tradeoff acknowledged in code), platform_addrs per_account / orphan identities / asset-lock load gaps (STILL VALID but INTENTIONALLY DEFERRED per code comments and follow-up #3745 — moved to out_of_scope), identity_keys sentinel scope skip (STILL VALID, suggestion), identities apply blob cross-check (STILL VALID, suggestion), From Fatal default (STILL VALID, suggestion), commit_writes Immediate no-op (STILL VALID, suggestion). Earlier resolved items (pre-delete buffered-flush, integrity_check pre-migration, typed PersistenceError) remain fixed. No new defects in the latest delta. Action: REQUEST_CHANGES on the silent cross-wallet re-parenting; the rest are documentation/hardening suggestions.
🔴 1 blocking | 🟡 7 suggestion(s) | 💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:22-33: [carried-forward] V002 identities upsert silently re-parents an existing identity to a different wallet
The comment claims `Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL`, but the SQL is `wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id)`. COALESCE returns the first non-NULL argument, so when a re-upsert supplies a non-NULL `excluded.wallet_id` (the caller's flush scope, which is non-NULL for any real wallet), the existing `identities.wallet_id` is overwritten — the opposite of what the comment promises. Because V002 cascades `identity_keys`, `token_balances`, and `dashpay_*` through `identities(identity_id)`, a single duplicate-identity_id upsert under another wallet silently transfers every identity-owned descendant to the new parent with no `IdentityKeyEntryMismatch`-style guard. The intended promotion semantic (orphan → wallet, i.e. NULL → value) requires the arguments swapped.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:25-43: [carried-forward] identities::apply does not cross-check entry.wallet_id against the flush scope
`identity_keys::apply` (schema/identity_keys.rs:104-110) rejects an `IdentityKeyEntry` whose embedded `wallet_id` disagrees with the flush scope via `IdentityKeyEntryMismatch`. `identities::apply` performs no equivalent check: the typed `identities.wallet_id` column is written from the flush scope while the serialized blob independently encodes `IdentityEntry.wallet_id`, and `managed_identity_from_entry` later prefers the blob's value (`entry.wallet_id.or(Some(*wallet_id))`). Two inconsistent ownership representations can persist without an error, biting on rehydration. Mirror `identity_keys`' guard (with the same sentinel carve-out) or document that the typed column is the sole source of truth.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:58-64: [carried-forward] All-zero WalletId is stored as NULL on write but matched literally on read/delete
`wallet_id_to_param` maps the all-zero sentinel `WalletId` to NULL, so the typed column stores NULL. Both `load_state` (line 115) and `count_rows_for_wallet_sql(..., ViaIdentity)` paths bind the sentinel bytes with `WHERE wallet_id = ?1`, which in SQL never matches a NULL row. A wallet constructed with `wallet_id = [0u8;32]` (operator CLI / test fixtures) can therefore persist identity-owned descendants successfully but is unable to enumerate or cascade-delete them through the trait API — they become orphans visible to other tenants of the DB. Crypto wallet ids will never collide with `[0u8;32]` in practice, but operator/test paths bypass that guarantee. Reject the sentinel at the trait boundary or store it verbatim so read and write paths agree.
In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:311-327: [carried-forward] restore_from releases SQLite EXCLUSIVE before the atomic rename, opening a lost-update window
`restore_from` rolls back and drops `dest_lock_conn` at lines 317-323 before `tmp.persist(dest_db_path)` at 325-327. The release is deliberate (the comment explains the dangling-inode rationale), but in the window between rollback and rename, a waiting peer can promote a write transaction against the live DB and commit changes that are then silently superseded by the rename. The post-restore DB is consistent with the backup but discards the peer's commit. The code documents this is a tradeoff for atomic rename safety; surface it explicitly to consumers so operators know to quiesce peers before restore, or emit a `tracing::warn!` immediately before the persist call so the audit trail captures the gap.
In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:461-509: [carried-forward] delete_wallet pre-delete backup precedes cascade EXCLUSIVE; peer writes in the gap are deleted but not in the backup
The pre-delete backup at 461-469 runs before `BEGIN EXCLUSIVE` at line 486 because rusqlite's Backup API cannot operate alongside an in-flight write tx (acknowledged in the inline comment). The post-EXCLUSIVE recheck at 494-509 only logs a `tracing::info!` if the wallet's footprint changed, never refuses or re-snapshots. A cross-process peer that inserts identity-owned rows for the same `wallet_id` between snapshot and lock has those rows cascade-deleted but absent from `backup_path`, breaking the backup's advertised pre-delete fidelity. Either surface `peer_mutation_observed: bool` on `DeleteWalletReport` so callers can decide to take a second post-cascade snapshot, or document the gap in the consumer-facing delete docs.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:571-579: [carried-forward] commit_writes is a no-op in Immediate mode even when transient failures left dirty buffered state
`commit_writes_inner` returns an empty `CommitReport` whenever `flush_mode == FlushMode::Immediate`, on the assumption that Immediate writes are always synchronously flushed. However `handle_flush_error()` deliberately restores buffered changesets on transient failures, so an Immediate-mode caller that hit a transient error has dirty buffered state, and `commit_writes` as the documented retry entry point silently returns success without attempting to drain it. Either drain the buffer regardless of mode, or document that Immediate-mode callers must use a different retry primitive after transient failures.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:104-112: [carried-forward] sentinel scope disables the identity-key wallet ownership check entirely
When the outer flush scope is the all-zero sentinel, the `entry_wallet_id != *wallet_id` check is skipped entirely, allowing an `IdentityKeyEntry` carrying ANY non-NULL `wallet_id` to be persisted under the sentinel scope. The intent (per comment) is to relax the check for identity-only callers with no parent wallet, but the realised semantic is broader — it accepts an arbitrary embedded wallet_id rather than enforcing that the blob's `wallet_id` is also `None`. Tighten to `entry_wallet_id.is_none() || scope_is_sentinel == false` style so sentinel-scope writes must be genuinely orphan.
In `packages/rs-platform-wallet/src/changeset/traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/changeset/traits.rs:136-152: [carried-forward] From<String>/<&str> for PersistenceError silently downgrades all errors to Fatal
The blanket `From<String>` / `From<&str>` impls route through `PersistenceError::backend(...)`, which hard-codes `PersistenceErrorKind::Fatal`. The FFI persister (`rs-platform-wallet-ffi/src/persistence.rs`) uses string conversions for assorted callback failures, including conditions that could legitimately be transient (e.g. host-side busy/locked equivalents). Any such stringified error is misclassified as non-retryable, defeating the typed retry contract CODE-004 introduced. Either remove the string `From` impls so every call site picks a kind, or audit FFI persister error sites and switch transient-class cases to `backend_with_kind(...)`.
Out-of-scope follow-up suggestions (6)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- platform_addrs::load_state returns Default per_account (intentionally deferred per code comment) —
load_stateat platform_addrs.rs:166-192 hard-codesper_account: Default::default(), dropping every persistedplatform_addressesrow from the returnedPlatformAddressSyncStartState. The code comment explicitly attributes this to upstream wiring not yet round-tripping xpubs, and the PR description references the follow-up that wiresWallet::from_persisted. Pair with #3745.- Follow-up: Track in the consumer-hardening follow-up PR that wires
Wallet/ManagedWalletInforeconstruction intoClientStartState::wallets; add an end-to-end restart test throughPlatformWalletManager::load_from_persistor.
- Follow-up: Track in the consumer-hardening follow-up PR that wires
- Orphan identities (wallet_id NULL) are silently dropped by identities::load_state — V002 deliberately allows
identities.wallet_idto be NULL for orphan identities, butload_statefilters withWHERE wallet_id = ?1, so orphan rows are excluded fromout_of_wallet_identities. The PR references #3745 as the follow-up for orphan-identity load wiring.- Follow-up: Resolve via #3745 — add an orphan-bucket loader or a dedicated entry point for orphan identities.
- asset_locks::load_state would resurrect Consumed locks as unused on restart —
AssetLockManager::consume_asset_lock()intentionally retainsConsumedrows on disk for history while removing them from the in-memory tracked-lock map.schema::asset_locks::load_statecurrently rehydrates every row with no status filter, so once full wallet restore is wired this reader would reintroduce spent locks intoClientWalletStartState::unused_asset_locks. Sequenced behind the same upstream wallet-rehydration work.- Follow-up: Add a
WHERE status = 'NotYetConsumed'filter (or equivalent) as part of the consumer-hardening PR that wires asset-lock startup reconstruction.
- Follow-up: Add a
- V002 identities migration aborts with bare UNIQUE error if V001 carried duplicate identity_ids across wallets — V001 keyed
identitiesby(wallet_id, identity_id); V002 makesidentity_idthe sole PK. TheINSERT INTO identities_v2 ... SELECT ... FROM identitiesinV002__cascade_only_identity_refs.rswill hit a UNIQUE violation if any identity ever participated in two wallets in V001, but the error surfaces as bare rusqlite UNIQUE rather than the typedMigrationRequiresManualCleanupreserved for the token_balances sentinel. Operators hitting this would not know what to clean up.- Follow-up: Open a separate issue to pre-check for cross-wallet identity_id collisions in
run_for_openand re-classify intoMigrationRequiresManualCleanupwith offending identity ids, or document the manual cleanup query in the migration's comment block.
- Follow-up: Open a separate issue to pre-check for cross-wallet identity_id collisions in
- count_pending re-queries schema_history that the caller already verified —
SqlitePersister::openonly callscount_pendingwhenhad_schema_historyis true, yetcount_pendingitself starts with anotherhas_schema_history(conn)?whosefalsebranch returns the sameembedded.len()that the caller'selsearm already produces. The inner check is dead-by-construction on the live call site. Pure cleanup, no behavior change.- Follow-up: Drop the inner check or make
count_pendingprivate to a code path that owns both branches; bundle with the next routine maintenance pass.
- Follow-up: Drop the inner check or make
- delete_wallet_inner Cell<Option<...>> + IIFE state machine is harder to follow than necessary — The handoff of the drained changeset through
std::cell::Cell<Option<PlatformWalletChangeSet>>so the inner IIFE can stash it back on each failure branch and the outerrestore_bufferclosure cantake()on the catch-all path is unusual but correct. A function-local helper returningResult<DeleteWalletReport, (WalletStorageError, Option<PlatformWalletChangeSet>)>would let the outer code own restoration withoutCell. Style-only; flagged to ease maintenance because every new failure branch must currently re-verify the Cell discipline.- Follow-up: Refactor in a follow-up cleanup PR when adding the next failure branch in this function.
| // INTENTIONAL(SEC-001): NULL wallet_id allowed per CODE-002 design; | ||
| // COALESCE upsert is the intended merge semantic for orphan-identity-to-wallet promotion. | ||
| // Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL. | ||
| let mut stmt = tx.prepare_cached( | ||
| "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ | ||
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | ||
| VALUES (?1, ?2, ?3, ?4, 0) \ | ||
| ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ | ||
| ON CONFLICT(identity_id) DO UPDATE SET \ | ||
| wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id), \ | ||
| wallet_index = excluded.wallet_index, \ | ||
| entry_blob = excluded.entry_blob, \ | ||
| tombstoned = 0", | ||
| )?; |
There was a problem hiding this comment.
🔴 Blocking: [carried-forward] V002 identities upsert silently re-parents an existing identity to a different wallet
The comment claims Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL, but the SQL is wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id). COALESCE returns the first non-NULL argument, so when a re-upsert supplies a non-NULL excluded.wallet_id (the caller's flush scope, which is non-NULL for any real wallet), the existing identities.wallet_id is overwritten — the opposite of what the comment promises. Because V002 cascades identity_keys, token_balances, and dashpay_* through identities(identity_id), a single duplicate-identity_id upsert under another wallet silently transfers every identity-owned descendant to the new parent with no IdentityKeyEntryMismatch-style guard. The intended promotion semantic (orphan → wallet, i.e. NULL → value) requires the arguments swapped.
| // INTENTIONAL(SEC-001): NULL wallet_id allowed per CODE-002 design; | |
| // COALESCE upsert is the intended merge semantic for orphan-identity-to-wallet promotion. | |
| // Existing wallet_id is preserved on re-upsert; new wallet_id fills NULL. | |
| let mut stmt = tx.prepare_cached( | |
| "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ | |
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | |
| VALUES (?1, ?2, ?3, ?4, 0) \ | |
| ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ | |
| ON CONFLICT(identity_id) DO UPDATE SET \ | |
| wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id), \ | |
| wallet_index = excluded.wallet_index, \ | |
| entry_blob = excluded.entry_blob, \ | |
| tombstoned = 0", | |
| )?; | |
| let mut stmt = tx.prepare_cached( | |
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | |
| VALUES (?1, ?2, ?3, ?4, 0) \ | |
| ON CONFLICT(identity_id) DO UPDATE SET \ | |
| wallet_id = COALESCE(identities.wallet_id, excluded.wallet_id), \ | |
| wallet_index = excluded.wallet_index, \ | |
| entry_blob = excluded.entry_blob, \ | |
| tombstoned = 0", | |
| )?; |
source: ['codex']
| // 7. Release the SQLite-native EXCLUSIVE lock BEFORE the rename. | ||
| // Dropping `dest_lock_conn` causes SQLite to close its file | ||
| // handle on the old inode; if we kept it alive across `persist` | ||
| // the handle would point at the unlinked old inode while the | ||
| // new DB took its place — peers reopening would race the rename | ||
| // and (on some filesystems) the rename itself could fail. | ||
| if let Some(conn) = dest_lock_conn.take() { | ||
| // Best-effort rollback of the empty EXCLUSIVE tx; an error here | ||
| // means SQLite is already in trouble and `drop(conn)` covers | ||
| // the rest. Silent because the conn is about to drop anyway. | ||
| let _ = conn.execute_batch("ROLLBACK"); | ||
| drop(conn); | ||
| } | ||
|
|
||
| // 8. Persist atomically over the destination. | ||
| tmp.persist(dest_db_path) | ||
| .map_err(|e| WalletStorageError::Io(e.error))?; |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] restore_from releases SQLite EXCLUSIVE before the atomic rename, opening a lost-update window
restore_from rolls back and drops dest_lock_conn at lines 317-323 before tmp.persist(dest_db_path) at 325-327. The release is deliberate (the comment explains the dangling-inode rationale), but in the window between rollback and rename, a waiting peer can promote a write transaction against the live DB and commit changes that are then silently superseded by the rename. The post-restore DB is consistent with the backup but discards the peer's commit. The code documents this is a tradeoff for atomic rename safety; surface it explicitly to consumers so operators know to quiesce peers before restore, or emit a tracing::warn! immediately before the persist call so the audit trail captures the gap.
source: ['claude', 'codex']
| @@ -427,21 +468,62 @@ impl SqlitePersister { | |||
| AutoBackupOperation::DeleteWallet, | |||
| )? | |||
| }; | |||
| let tx = conn.transaction()?; | |||
|
|
|||
| // Test-only hook: fires between the backup snapshot and | |||
| // the cascade EXCLUSIVE so TC-CODE-006-3 can simulate a | |||
| // cross-process peer that mutates `wallet_metadata` in | |||
| // the gap rusqlite's Backup API forces us to leave open. | |||
| #[cfg(any(test, feature = "__test-helpers"))] | |||
| self.consume_post_backup_hook(); | |||
|
|
|||
| // SQLite-native EXCLUSIVE for the cascade window. Excludes | |||
| // cross-process peers (other rusqlite Connections, sibling | |||
| // `SqlitePersister`s) that would otherwise commit rows for | |||
| // `wallet_id` between the backup snapshot and the cascade. | |||
| // The in-process mutex on `conn` alone never gave that | |||
| // guarantee. Peers waiting on the lock back off via | |||
| // SQLite's `busy_timeout`. | |||
| let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Exclusive)?; | |||
|
|
|||
| // Re-confirm existence post-EXCLUSIVE: a peer could have | |||
| // either inserted (raising the wallet from non-existent to | |||
| // existent) or deleted (vanishing it) between the backup | |||
| // and the lock acquisition. If a peer just deleted the | |||
| // wallet, the cascade is a no-op — we still commit because | |||
| // the operator's intent is satisfied. | |||
| let post_lock_exists = tx | |||
| .query_row( | |||
| "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", | |||
| rusqlite::params![wallet_id.as_slice()], | |||
| |_| Ok(()), | |||
| ) | |||
| .optional()? | |||
| .is_some(); | |||
| if post_lock_exists != exists_in_db { | |||
| tracing::info!( | |||
| wallet_id = %hex::encode(wallet_id), | |||
| pre_lock_exists = exists_in_db, | |||
| post_lock_exists, | |||
| "wallet_metadata footprint changed across delete_wallet EXCLUSIVE acquisition" | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] delete_wallet pre-delete backup precedes cascade EXCLUSIVE; peer writes in the gap are deleted but not in the backup
The pre-delete backup at 461-469 runs before BEGIN EXCLUSIVE at line 486 because rusqlite's Backup API cannot operate alongside an in-flight write tx (acknowledged in the inline comment). The post-EXCLUSIVE recheck at 494-509 only logs a tracing::info! if the wallet's footprint changed, never refuses or re-snapshots. A cross-process peer that inserts identity-owned rows for the same wallet_id between snapshot and lock has those rows cascade-deleted but absent from backup_path, breaking the backup's advertised pre-delete fidelity. Either surface peer_mutation_observed: bool on DeleteWalletReport so callers can decide to take a second post-cascade snapshot, or document the gap in the consumer-facing delete docs.
source: ['claude', 'codex']
| let mut stmt = tx.prepare_cached( | ||
| "INSERT INTO identities (wallet_id, wallet_index, identity_id, entry_blob, tombstoned) \ | ||
| "INSERT INTO identities (identity_id, wallet_id, wallet_index, entry_blob, tombstoned) \ | ||
| VALUES (?1, ?2, ?3, ?4, 0) \ | ||
| ON CONFLICT(wallet_id, identity_id) DO UPDATE SET \ | ||
| ON CONFLICT(identity_id) DO UPDATE SET \ | ||
| wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id), \ | ||
| wallet_index = excluded.wallet_index, \ | ||
| entry_blob = excluded.entry_blob, \ | ||
| tombstoned = 0", | ||
| )?; | ||
| let wallet_id_param = wallet_id_to_param(wallet_id); | ||
| for (id, entry) in &cs.identities { | ||
| let payload = blob::encode(entry)?; | ||
| stmt.execute(params![ | ||
| wallet_id.as_slice(), | ||
| entry.identity_index.map(i64::from), | ||
| id.as_slice(), | ||
| wallet_id_param, | ||
| entry.identity_index.map(i64::from), | ||
| payload, | ||
| ])?; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] identities::apply does not cross-check entry.wallet_id against the flush scope
identity_keys::apply (schema/identity_keys.rs:104-110) rejects an IdentityKeyEntry whose embedded wallet_id disagrees with the flush scope via IdentityKeyEntryMismatch. identities::apply performs no equivalent check: the typed identities.wallet_id column is written from the flush scope while the serialized blob independently encodes IdentityEntry.wallet_id, and managed_identity_from_entry later prefers the blob's value (entry.wallet_id.or(Some(*wallet_id))). Two inconsistent ownership representations can persist without an error, biting on rehydration. Mirror identity_keys' guard (with the same sentinel carve-out) or document that the typed column is the sole source of truth.
source: ['claude', 'codex']
| if let Some(entry_wallet_id) = entry.wallet_id { | ||
| if entry_wallet_id != *wallet_id { | ||
| // Treat the all-zero sentinel scope as "any wallet" so | ||
| // identity-only callers (no parent wallet) don't trip | ||
| // the cross-check. | ||
| let scope_is_sentinel = wallet_id.iter().all(|b| *b == 0); | ||
| if !scope_is_sentinel && entry_wallet_id != *wallet_id { | ||
| return Err(WalletStorageError::IdentityKeyEntryMismatch); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] sentinel scope disables the identity-key wallet ownership check entirely
When the outer flush scope is the all-zero sentinel, the entry_wallet_id != *wallet_id check is skipped entirely, allowing an IdentityKeyEntry carrying ANY non-NULL wallet_id to be persisted under the sentinel scope. The intent (per comment) is to relax the check for identity-only callers with no parent wallet, but the realised semantic is broader — it accepts an arbitrary embedded wallet_id rather than enforcing that the blob's wallet_id is also None. Tighten to entry_wallet_id.is_none() || scope_is_sentinel == false style so sentinel-scope writes must be genuinely orphan.
source: ['claude', 'codex']
| /// String-shaped messages from legacy callers (predominantly the FFI | ||
| /// persister) flow through here. The original construction site | ||
| /// usually doesn't know whether the failure is transient or fatal, so | ||
| /// the conservative default is [`PersistenceErrorKind::Fatal`] — | ||
| /// callers that DO know the kind use [`PersistenceError::backend_with_kind`] | ||
| /// directly. | ||
| impl From<String> for PersistenceError { | ||
| fn from(msg: String) -> Self { | ||
| Self::Backend(msg) | ||
| Self::backend(StringSource(msg)) | ||
| } | ||
| } | ||
|
|
||
| impl From<&str> for PersistenceError { | ||
| fn from(msg: &str) -> Self { | ||
| Self::Backend(msg.to_string()) | ||
| Self::backend(StringSource(msg.to_string())) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] From/<&str> for PersistenceError silently downgrades all errors to Fatal
The blanket From<String> / From<&str> impls route through PersistenceError::backend(...), which hard-codes PersistenceErrorKind::Fatal. The FFI persister (rs-platform-wallet-ffi/src/persistence.rs) uses string conversions for assorted callback failures, including conditions that could legitimately be transient (e.g. host-side busy/locked equivalents). Any such stringified error is misclassified as non-retryable, defeating the typed retry contract CODE-004 introduced. Either remove the string From impls so every call site picks a kind, or audit FFI persister error sites and switch transient-class cases to backend_with_kind(...).
source: ['claude', 'codex']
| fn commit_writes_inner(&self) -> Result<CommitReport, PersistenceError> { | ||
| let mut report = CommitReport { | ||
| succeeded: Vec::new(), | ||
| failed: Vec::new(), | ||
| still_pending: Vec::new(), | ||
| }; | ||
| if matches!(self.config.flush_mode, FlushMode::Immediate) { | ||
| if self.config.flush_mode == FlushMode::Immediate { | ||
| return Ok(report); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] commit_writes is a no-op in Immediate mode even when transient failures left dirty buffered state
commit_writes_inner returns an empty CommitReport whenever flush_mode == FlushMode::Immediate, on the assumption that Immediate writes are always synchronously flushed. However handle_flush_error() deliberately restores buffered changesets on transient failures, so an Immediate-mode caller that hit a transient error has dirty buffered state, and commit_writes as the documented retry entry point silently returns success without attempting to drain it. Either drain the buffer regardless of mode, or document that Immediate-mode callers must use a different retry primitive after transient failures.
source: ['codex']
| fn wallet_id_to_param(wallet_id: &WalletId) -> Option<&[u8]> { | ||
| if wallet_id.iter().all(|b| *b == 0) { | ||
| None | ||
| } else { | ||
| Some(wallet_id.as_slice()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [carried-forward] All-zero WalletId is stored as NULL on write but matched literally on read/delete
wallet_id_to_param maps the all-zero sentinel WalletId to NULL, so the typed column stores NULL. Both load_state (line 115) and count_rows_for_wallet_sql(..., ViaIdentity) paths bind the sentinel bytes with WHERE wallet_id = ?1, which in SQL never matches a NULL row. A wallet constructed with wallet_id = [0u8;32] (operator CLI / test fixtures) can therefore persist identity-owned descendants successfully but is unable to enumerate or cascade-delete them through the trait API — they become orphans visible to other tenants of the DB. Crypto wallet ids will never collide with [0u8;32] in practice, but operator/test paths bypass that guarantee. Reject the sentinel at the trait boundary or store it verbatim so read and write paths agree.
source: ['claude']
| pub(crate) fn run_for_open( | ||
| conn: &mut rusqlite::Connection, | ||
| ) -> Result<refinery::Report, WalletStorageError> { | ||
| crate::sqlite::conn::set_foreign_keys(conn, false)?; | ||
| let result = run(conn); | ||
| // Always restore FK enforcement, even on migration error, so the | ||
| // caller's connection is in the documented state. | ||
| crate::sqlite::conn::enforce_foreign_keys(conn)?; |
There was a problem hiding this comment.
💬 Nitpick: [carried-forward] FK enforcement off-flip is not undone if the off-flip itself errors
run_for_open calls set_foreign_keys(false)? then run(conn) then unconditionally enforce_foreign_keys(on). If the off-flip itself errors via ? before reaching run, control returns with FK still on (no harm). But if a future refactor moves the unconditional re-enable later, or if set_foreign_keys(false) ever succeeds-then-errors mid-call, the connection could escape with FK off. Practical impact today is bounded because SqlitePersister::open drops the conn on error, but guarding with let _ = enforce_foreign_keys(conn); before the early-return on set_foreign_keys failure preserves the function's documented invariant against future drift.
source: ['claude']
…R not yet merged) The PR hasn't shipped — no reason for a forward-only V002 alongside V001. Folded V002's cascade-only-identity-refs shape into V001: `identities.wallet_id` nullable + indexed; identity-owned tables (`identity_keys`, `dashpay_profiles`, `dashpay_payments_overlay`, `token_balances`) drop their direct `wallet_id` FK and cascade through `identities`. Single migration on first open. Drops: - V002's FK-toggle workaround + sentinel re-classification from `migrations::run_for_open` (no schema rewrite to dodge cascades for). - `conn::set_foreign_keys` (only V002's migration body used it). - `WalletStorageError::MigrationRequiresManualCleanup` (unreachable without the sentinel guard). - V002-specific test `tests/sqlite_v002_migration.rs`. - "V002:" tombstone comments scattered across schema + tests. Addresses L1, L6, L9.
…(PR not yet merged) No SemVer obligation — the PR hasn't shipped, so there's nothing to deprecate. Dropped: - CLI `--auto-backup-dir ""` sentinel path (now `Option<PathBuf>` directly; `--no-auto-backup` is the only opt-out). - `Buffer::drain` (`#[deprecated]` alias for `take_for_flush`). - `SqlitePersisterError` type alias for `WalletStorageError`. - `#[allow(deprecated)]` shims at the lib/mod boundary. - TC-CODE-030-1b smoke test that pinned the deprecated CLI sentinel. Addresses L2.
…CE order (B1 BLOCKING, L3, L4, L5, L7, B5, B6) - B1 (BLOCKING): identities upsert now uses `COALESCE(identities.wallet_id, excluded.wallet_id)` so an existing parented row's wallet_id is preserved on re-upsert; the excluded value only fills when the on-disk row is NULL (orphan → parented promotion). Reverse re-parenting is caught by the per-entry cross-check below — no more silent re-parenting. - L4 / B5: `identities::apply` now cross-checks `entry.wallet_id` against the flush scope. Sentinel scope requires entry.wallet_id to be `None`; real scope requires it to match. Returns the existing `WalletIdMismatch` variant on disagreement. - L4: `identities::fetch` now filters by `wallet_id IS ?2` (NULL-safe) so a peer wallet's identity row with the same identity_id can never leak through. Sentinel scope matches orphan (NULL) rows. - L5 / B6: `identity_keys::apply` sentinel-branch now requires `entry.wallet_id.is_none()` instead of bypassing the check. - L3: `dashpay::apply` documents the identity-must-belong-to-wallet precondition and adds a debug-only cross-check via `assert_identities_belong_to_wallet` (release-no-op). - L7: `token_balances::apply` gets the same precondition treatment. Addresses B1 (BLOCKING), L3, L4, L5, L7, B5, B6.
…+ harden pre-delete consistency (B3, B4); drop From<String> silent-Fatal classification (B7); tighten restore_from rustdoc (B2) - B4: commit_writes_inner no longer short-circuits in Immediate mode. Buffer can be non-empty in Immediate mode when a `store()` hits a transient failure (the buffer-restore path); commit_writes is the retry path that drains those leftovers regardless of flush mode. - B7: removed From<String>/From<&str> impls on PersistenceError (and the private StringSource wrapper). Call sites must now explicitly pick kind via PersistenceError::backend() — no more silent Fatal classification of every legacy `.into()?`. Updated the half-dozen FFI call sites in `packages/rs-platform-wallet-ffi/src/persistence.rs`. - B3: pre-delete cascade now aborts with the new typed variant WalletStorageError::ConcurrentMutationDuringDelete if a peer mutated any of the wallet's PER_WALLET_TABLES row counts between the auto-backup snapshot and the EXCLUSIVE acquisition. Operator retries after quiescing the peer; the pre-mutation backup file is left in place for forensics. Tightened TC-CODE-006-3 to assert the new contract. - B2: restore_from rustdoc tightened to spell out the lock-release-before-rename trade-off — file-handle correctness over absolute lock coverage; the microsecond gap can only cost a peer's own transient write (lands in an inode unlinked by the rename). Addresses B2, B3, B4, B7.
lklimek
merged commit e014555
into
feat/platform-wallet-sqlite-persistor
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative incremental review of PR #3743 at 47972c0 against the 4dc7a3a baseline. PRIOR FINDINGS: 9 of 10 are FIXED or OUTDATED by the latest delta (B1 COALESCE re-parent, B5/L4 identities cross-check, B6 sentinel identity-keys, L5/L7 NULL-safe fetch, B3 commit_writes Immediate drain, B4 delete_wallet pre/post footprint check, B7 From Fatal default, FK off-flip dance — all confirmed at head). 1 prior finding STILL VALID (TC-P2-008 LockPoisoned test gap). NEW LATEST-DELTA FINDINGS: one confirmed blocking regression — the From<String>/From<&str> removal was not applied to four shielded-feature call sites in platform-wallet-ffi, breaking cargo check -p platform-wallet-ffi --features shielded with 4 E0277 errors (independently confirmed by both FFI agents and reproduced locally). Three new defense-in-depth gaps in the delete-consistency footprint (count-only, missing UPDATE detection), debug-only attribution enforcement on identity-owned writers, and half-wired FFI callback pair validation on the non-shielded load paths. Codex's V001-rewrite blocking finding is dropped: the base feature branch is unmerged (commit message explicitly states 'PR not yet merged') so no deployed DBs exist on the old V001 shape.
🔴 1 blocking | 🟡 6 suggestion(s) | 💬 3 nitpick(s)
4 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:1346-1446: [NEW] `platform-wallet-ffi/shielded` fails to compile after `From<String>` removal — four call sites still use `.into()` on String
Confirmed via `cargo check -p platform-wallet-ffi --features shielded` (4 × E0277). Commit 47972c0501 removed `impl From<String> for PersistenceError` and `impl From<&str> for PersistenceError` from `packages/rs-platform-wallet/src/changeset/traits.rs`, and migrated the default-features call sites in `persistence.rs` to explicit `PersistenceError::backend(...)`. Four shielded-only sites were missed:
- `persistence.rs:1352-1353` — half-wired-notes-callbacks String error
- `persistence.rs:1365-1366` — half-wired-sync-states-callbacks String error
- `persistence.rs:1378` — `on_load_shielded_notes_fn returned error code {rc}`
- `persistence.rs:1442-1446` — `on_load_shielded_sync_states_fn returned error code {rc}`
The shielded cargo feature is the build path the iOS shielded-wallet surface links against; flipping it on breaks the staticlib/cdylib for downstream consumers. The PR's stated test plan only ran clippy on default features, which is why this slipped. Fix is mechanical: rewrite each `.into()` to `PersistenceError::backend(...)` matching the non-shielded sites already converted in this same delta.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1281-1306: [NEW] `on_load_wallet_list_fn` accepts a half-wired callback pair — host-allocated restore buffer leaks if `_free_fn` is unset
The shielded-load paths at persistence.rs:1346-1368 already enforce that loader + free callbacks are wired as a pair (returning a 'must be provided together' error). The non-shielded wallet-list load at persistence.rs:1281-1306 only checks whether `on_load_wallet_list_fn` is present — it builds `LoadGuard` with `free_fn: self.callbacks.on_load_wallet_list_free_fn` even when that is `None`, and `LoadGuard::drop` becomes a no-op. A host that wires the load callback without the free callback leaks the entire Swift-owned `WalletRestoreEntryFFI` graph (entries array plus nested xpub/key/string buffers) on every startup. Mirror the shielded pair-validation here — the same one-line `is_some() != is_some()` gate, returning `PersistenceError::backend("on_load_wallet_list_fn and on_load_wallet_list_free_fn must be provided together")` — to fail fast on the half-wired state instead of leaking silently.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1544-1605: [NEW] `on_get_core_tx_record_fn` accepts a half-wired callback pair — host-allocated tx bytes leak if `_free_fn` is unset
Same pattern as the wallet-list load above and the shielded loaders' enforcement. `FFIPersister::get_core_tx_record` builds an RAII guard for the host-allocated `out_tx_bytes`/`out_tx_bytes_len`, but accepts `on_get_core_tx_record_fn` without requiring `on_get_core_tx_record_free_fn`; in that half-wired state the guard never frees non-null buffers, leaking host memory on every asset-lock proof fallback lookup. Mirror the shielded pair-validation: reject the configuration with `PersistenceError::backend("on_get_core_tx_record_fn and on_get_core_tx_record_free_fn must be provided together")` so the ABI invariant is enforced consistently across every load-style callback.
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/persistence.rs:1249-1271: [NEW] FFI callbacks return opaque `i32`, so every host-reported error is forced to `PersistenceErrorKind::Fatal` — `commit_writes`' retry contract cannot cross the boundary
The 47972c05 delta resolves the trait-level B7 symptom by deleting the blanket `From<String>` impls and forcing callers to construct `PersistenceError::backend(...)`. At the FFI boundary the underlying problem persists: `PersistenceCallbacks::on_store_fn`, `on_flush_fn`, `on_load_wallet_list_fn`, and the shielded-load callbacks all return `unsafe extern "C" fn(...) -> i32` — a single opaque code with no transient/fatal/constraint dimension. The persister consequently wraps every non-zero return via `PersistenceError::backend(format!("...returned error code {}", result))`, and `backend(...)` defaults to `Fatal`.
Consequence: a host that knows its error is transient (SwiftData CoreData store busy, transient I/O) has no way to signal that across the ABI, so the new `commit_writes` retry path treats every FFI-side failure as non-retryable and drops the buffered changeset. The trait now exposes `backend_with_kind` precisely to surface this distinction; the vtable needs a matching contract — reserved error-code ranges (e.g. `>= 1000` = transient), or an additional `out_kind: *mut u8` parameter — so hosts can opt in without ABI churn. Lower priority than the compile break; flag this as the next ABI revision target after the consumer-hardening PR lands.
In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:458-514: [NEW] `ConcurrentMutationDuringDelete` only detects row-count changes — peer UPDATEs in the lock-free window silently corrupt the pre-delete backup's restore fidelity
The new pre/post-EXCLUSIVE check in `delete_wallet_inner` (persister.rs:458-514) is a real hardening: it catches peer INSERTs/DELETEs in the rusqlite-Backup-API-mandated lock-free window and aborts with the new typed error. But `wallet_footprint` (persister.rs:1288-1305) is `Vec<(&'static str, i64)>` — per-table row counts only. A peer UPDATE (e.g. `core_utxos.spent`, `core_sync_state.synced_height`, `dashpay_profiles.profile_blob`, `identities.entry_blob`) or a paired DELETE+INSERT inside the same table leaves the count unchanged, equality passes, and the cascade proceeds. The pre-delete auto-backup captures the *pre-update* state but the cascade deletes the *post-update* state, so operator rollback from the backup silently restores stale row content.
The variant rustdoc at `error.rs` and the inline comment at persister.rs:493-500 advertise this guard as catching 'any change' in the gap, which is stronger than the implementation delivers. Two reasonable fixes:
1. Extend `wallet_footprint` to include a per-table content fingerprint (SHA-256 over `(rowid, blob_payload)` rows, or `SELECT total_changes()` / `PRAGMA data_version` deltas against the source connection across the window).
2. Tighten the error/comment wording to say 'row-count change' and document the residual UPDATE-window risk so operators know rollback-from-backup may lose silent UPDATE traffic.
The delete-cascade itself is correct; the gap only affects restore-from-backup fidelity, which is exactly the invariant this new guard exists to defend.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:737-743: [CARRY-FORWARD, STILL VALID] `LockPoisoned` fatal classification has no end-to-end test (TC-P2-008)
Prior finding from 4dc7a3a9 — `TODO(qa): TC-P2-008` still verbatim at persister.rs:737-743, and `handle_flush_error`'s fatal branch for `LockPoisoned` is unchanged. The latest delta's `commit_writes_inner` change (which now drains buffered changesets regardless of flush mode) amplifies the cost of mis-classifying a `LockPoisoned`: a poisoned mutex inside the drain loop short-circuits `still_pending` and drops every other wallet's retry. A `__test-helpers` injector that primes a `WalletStorageError::LockPoisoned` (no real panicking thread needed) and asserts (a) the failing wallet's buffer is wiped, (b) remaining wallets land in `still_pending`, and (c) the surfaced kind is `PersistenceError::LockPoisoned` would close the deterministic coverage gap.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs:32-49: [NEW] Identity-parent invariant on `dashpay` / `token_balances` writers is enforced only in debug builds
The V001 schema rewrite intentionally removes `wallet_id` from the identity-owned child tables (`identity_keys`, `dashpay_profiles`, `dashpay_payments_overlay`, `token_balances`) so they FK only on `identity_id`. To compensate, this PR adds `schema::assert_identities_belong_to_wallet` (schema/mod.rs:97-148), but both call sites gate it on `if cfg!(debug_assertions)` — token_balances.rs:37 and dashpay.rs (same pattern). In release builds the check is fully elided. A buggy caller that flushes a `TokenBalanceChangeSet` under wallet A's scope but carries an `identity_id` parented to wallet B will land the row on B's identity via the `ON CONFLICT(identity_id, token_id) DO UPDATE` path with no error — exactly the cross-wallet corruption class that the previous schema's `(wallet_id, identity_id, ...)` PK fail-fast prevented.
The per-flush `SELECT wallet_id FROM identities WHERE identity_id = ?` is a single `prepare_cached` round-trip inside the open tx — cheap enough to run unconditionally. Either drop the `cfg!(debug_assertions)` gate, or promote the helper to a CI-only check that gates production tests so attribution regressions surface in test runs rather than only in dev environments.
Out-of-scope follow-up suggestions (5)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Pre-merge sanity check: V001 was rewritten in place (V002 collapsed back) — confirm the feature branch was never deployed — Codex flagged the V001 rewrite (commit 0ff215e) as blocking on the premise that the stacked base branch
feat/platform-wallet-sqlite-persistoralready shipped the old V001 shape, making existing DBs seepending_count == 0after the rewrite. The author's commit message explicitly says 'PR not yet merged' for both V001 and the V002 collapse, and the workspacegit logconfirmsfeat/platform-wallet-sqlite-persistoritself is unmerged tov3.1-dev. Treating as a false positive for this PR. Worth a maintainer sanity check before merge: confirm no internal preview/QA build has persisted a DB under the previous V001 schema, otherwise an append-only V002 fix-up is needed instead of an in-place rewrite.- Follow-up: Maintainer confirmation only — no PR change needed unless a preview/QA build has materialized DBs under the prior V001 shape.
- Orphan-identity load path (#3745) still pending consumer-hardening follow-up —
identities::load_statestill filters byWHERE wallet_id = ?1so NULL-parent (orphan) identities written under the all-zero sentinel WalletId are unreachable through the per-wallet loader.fetch()is now NULL-safe viaIS ?2, so the read/write asymmetry on the single-row path is closed. Tracked as follow-up #3745 per the PR description; intentionally deferred.- Follow-up: Resolve via #3745: add a dedicated
load_orphans()entry point or extendload_stateto optionally include NULL-parent rows.
- Follow-up: Resolve via #3745: add a dedicated
- platform_addrs::load_state and asset_locks::load_state still drop deferred state —
platform_addrs::load_statedrops per-account state andasset_locks::load_statewould resurrect Consumed locks as unused if rehydration were wired. Both are intentionally deferred behind the consumer-hardening PR (Wallet::from_persisted).- Follow-up: Pair with consumer-hardening follow-up; add
WHERE status = 'NotYetConsumed'to the asset-lock loader at that time.
- Follow-up: Pair with consumer-hardening follow-up; add
delete_wallet_innerCell<Option<…>> + IIFE state machine has another failure branch — The new footprint-mismatch path at persister.rs:502-514 is a 4th 'restore the buffer and bail' site that must remember thedrained_slot.set(Some(cs))ritual. Pattern is still correct, but each added failure branch raises the cognitive bar for future contributors. A function-local helper returningResult<DeleteWalletReport, (WalletStorageError, Option<PlatformWalletChangeSet>)>would let the outer code own restoration withoutCellmutability.- Follow-up: Refactor on the next time
delete_wallet_innergains another failure branch.
- Follow-up: Refactor on the next time
- Sentinel WalletId (all-zero) cascade gap on
wallet_metadatadeletion —wallet_id_to_parammaps[0u8; 32]to NULL onidentitieswrites, butwallet_meta::upsertwrites the same bytes literally intowallet_metadata.wallet_id. If an operator persists wallet_metadata under the sentinel id, the FK cascade fromidentities.wallet_id REFERENCES wallet_metadata(wallet_id)won't match across NULL, leaving orphan identities (and their key/balance/dashpay children) afterdelete_wallet(sentinel). Real cryptographic wallet ids can't collide with [0u8;32], and CLI input flows through this path. Defense-in-depth only; tracked under #3745.- Follow-up: Resolve via #3745 alongside the orphan-load entry point.
| @@ -427,21 +473,77 @@ impl SqlitePersister { | |||
| AutoBackupOperation::DeleteWallet, | |||
| )? | |||
| }; | |||
| let tx = conn.transaction()?; | |||
|
|
|||
| // Test-only hook: fires between the backup snapshot and | |||
| // the cascade EXCLUSIVE so TC-CODE-006-3 can simulate a | |||
| // cross-process peer that mutates `wallet_metadata` in | |||
| // the gap rusqlite's Backup API forces us to leave open. | |||
| #[cfg(any(test, feature = "__test-helpers"))] | |||
| self.consume_post_backup_hook(); | |||
|
|
|||
| // SQLite-native EXCLUSIVE for the cascade window. Excludes | |||
| // cross-process peers (other rusqlite Connections, sibling | |||
| // `SqlitePersister`s) that would otherwise commit rows for | |||
| // `wallet_id` between the backup snapshot and the cascade. | |||
| // The in-process mutex on `conn` alone never gave that | |||
| // guarantee. Peers waiting on the lock back off via | |||
| // SQLite's `busy_timeout`. | |||
| let tx = conn.transaction_with_behavior(rusqlite::TransactionBehavior::Exclusive)?; | |||
|
|
|||
| // Re-snapshot the wallet's footprint under EXCLUSIVE and | |||
| // compare against the pre-backup snapshot. Any change means | |||
| // a peer mutated the wallet between the backup and the lock | |||
| // acquisition — the backup we just took is now inconsistent | |||
| // with the live state, so rollback-from-backup would | |||
| // silently lose those writes. Abort with the typed | |||
| // `ConcurrentMutationDuringDelete` so the operator can | |||
| // retry after quiescing the peer. | |||
| let post_lock_footprint = wallet_footprint_tx(&tx, &wallet_id)?; | |||
| if post_lock_footprint != pre_backup_footprint { | |||
| tracing::warn!( | |||
| wallet_id = %hex::encode(wallet_id), | |||
| pre_backup = ?pre_backup_footprint, | |||
| post_lock = ?post_lock_footprint, | |||
| "delete_wallet aborted: peer mutated wallet between auto-backup and EXCLUSIVE" | |||
| ); | |||
| // Roll back the empty EXCLUSIVE — no destructive work | |||
| // has happened yet inside this tx, so drop is enough, | |||
| // but be explicit. | |||
| let _ = tx.rollback(); | |||
| return Err(WalletStorageError::ConcurrentMutationDuringDelete { wallet_id }); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: [NEW] ConcurrentMutationDuringDelete only detects row-count changes — peer UPDATEs in the lock-free window silently corrupt the pre-delete backup's restore fidelity
The new pre/post-EXCLUSIVE check in delete_wallet_inner (persister.rs:458-514) is a real hardening: it catches peer INSERTs/DELETEs in the rusqlite-Backup-API-mandated lock-free window and aborts with the new typed error. But wallet_footprint (persister.rs:1288-1305) is Vec<(&'static str, i64)> — per-table row counts only. A peer UPDATE (e.g. core_utxos.spent, core_sync_state.synced_height, dashpay_profiles.profile_blob, identities.entry_blob) or a paired DELETE+INSERT inside the same table leaves the count unchanged, equality passes, and the cascade proceeds. The pre-delete auto-backup captures the pre-update state but the cascade deletes the post-update state, so operator rollback from the backup silently restores stale row content.
The variant rustdoc at error.rs and the inline comment at persister.rs:493-500 advertise this guard as catching 'any change' in the gap, which is stronger than the implementation delivers. Two reasonable fixes:
- Extend
wallet_footprintto include a per-table content fingerprint (SHA-256 over(rowid, blob_payload)rows, orSELECT total_changes()/PRAGMA data_versiondeltas against the source connection across the window). - Tighten the error/comment wording to say 'row-count change' and document the residual UPDATE-window risk so operators know rollback-from-backup may lose silent UPDATE traffic.
The delete-cascade itself is correct; the gap only affects restore-from-backup fidelity, which is exactly the invariant this new guard exists to defend.
source: ['claude', 'codex']
| pub fn apply( | ||
| tx: &Transaction<'_>, | ||
| wallet_id: &WalletId, | ||
| cs: &TokenBalanceChangeSet, | ||
| ) -> Result<(), WalletStorageError> { | ||
| if cfg!(debug_assertions) { | ||
| let touched: std::collections::BTreeSet<dpp::prelude::Identifier> = cs | ||
| .balances | ||
| .keys() | ||
| .map(|(identity_id, _)| *identity_id) | ||
| .chain( | ||
| cs.removed_balances | ||
| .iter() | ||
| .map(|(identity_id, _)| *identity_id), | ||
| ) | ||
| .collect(); | ||
| super::assert_identities_belong_to_wallet(tx, wallet_id, &touched)?; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: [NEW] Identity-parent invariant on dashpay / token_balances writers is enforced only in debug builds
The V001 schema rewrite intentionally removes wallet_id from the identity-owned child tables (identity_keys, dashpay_profiles, dashpay_payments_overlay, token_balances) so they FK only on identity_id. To compensate, this PR adds schema::assert_identities_belong_to_wallet (schema/mod.rs:97-148), but both call sites gate it on if cfg!(debug_assertions) — token_balances.rs:37 and dashpay.rs (same pattern). In release builds the check is fully elided. A buggy caller that flushes a TokenBalanceChangeSet under wallet A's scope but carries an identity_id parented to wallet B will land the row on B's identity via the ON CONFLICT(identity_id, token_id) DO UPDATE path with no error — exactly the cross-wallet corruption class that the previous schema's (wallet_id, identity_id, ...) PK fail-fast prevented.
The per-flush SELECT wallet_id FROM identities WHERE identity_id = ? is a single prepare_cached round-trip inside the open tx — cheap enough to run unconditionally. Either drop the cfg!(debug_assertions) gate, or promote the helper to a CI-only check that gates production tests so attribution regressions surface in test runs rather than only in dev environments.
source: ['claude']
| @@ -1265,9 +1264,10 @@ impl PlatformWalletPersistence for FFIPersister { | |||
| if let Some(cb) = self.callbacks.on_flush_fn { | |||
| let result = unsafe { cb(self.callbacks.context, wallet_id.as_ptr()) }; | |||
| if result != 0 { | |||
| return Err( | |||
| format!("Persistence flush callback returned error code {}", result).into(), | |||
| ); | |||
| return Err(PersistenceError::backend(format!( | |||
| "Persistence flush callback returned error code {}", | |||
| result | |||
| ))); | |||
| } | |||
There was a problem hiding this comment.
🟡 Suggestion: [NEW] FFI callbacks return opaque i32, so every host-reported error is forced to PersistenceErrorKind::Fatal — commit_writes' retry contract cannot cross the boundary
The 47972c0 delta resolves the trait-level B7 symptom by deleting the blanket From<String> impls and forcing callers to construct PersistenceError::backend(...). At the FFI boundary the underlying problem persists: PersistenceCallbacks::on_store_fn, on_flush_fn, on_load_wallet_list_fn, and the shielded-load callbacks all return unsafe extern "C" fn(...) -> i32 — a single opaque code with no transient/fatal/constraint dimension. The persister consequently wraps every non-zero return via PersistenceError::backend(format!("...returned error code {}", result)), and backend(...) defaults to Fatal.
Consequence: a host that knows its error is transient (SwiftData CoreData store busy, transient I/O) has no way to signal that across the ABI, so the new commit_writes retry path treats every FFI-side failure as non-retryable and drops the buffered changeset. The trait now exposes backend_with_kind precisely to surface this distinction; the vtable needs a matching contract — reserved error-code ranges (e.g. >= 1000 = transient), or an additional out_kind: *mut u8 parameter — so hosts can opt in without ABI churn. Lower priority than the compile break; flag this as the next ABI revision target after the consumer-hardening PR lands.
source: ['claude']
| (true, Some(found)) => { | ||
| let mut found_arr = [0u8; 32]; | ||
| if found.len() == 32 { | ||
| found_arr.copy_from_slice(&found); | ||
| } | ||
| return Err(WalletStorageError::WalletIdMismatch { | ||
| expected: [0u8; 32], | ||
| found: found_arr, | ||
| }); | ||
| } | ||
| (false, None) => { | ||
| return Err(WalletStorageError::WalletIdMismatch { | ||
| expected: *wallet_id, | ||
| found: [0u8; 32], | ||
| }); | ||
| } | ||
| (false, Some(found)) => { | ||
| if found.as_slice() != wallet_id.as_slice() { | ||
| let mut found_arr = [0u8; 32]; | ||
| if found.len() == 32 { | ||
| found_arr.copy_from_slice(&found); | ||
| } | ||
| return Err(WalletStorageError::WalletIdMismatch { | ||
| expected: *wallet_id, | ||
| found: found_arr, | ||
| }); |
There was a problem hiding this comment.
💬 Nitpick: [NEW] assert_identities_belong_to_wallet reports malformed-length parent wallet_id as the all-zero sentinel — diagnostic silently wrong for DB-invariant violations
schema/mod.rs:118-122 and 137-142 build found_arr like if found.len() == 32 { found_arr.copy_from_slice(&found); } and otherwise leave it at [0u8; 32]. When the persisted identities.wallet_id is a non-32-byte blob (column is declared BLOB, not BLOB(32), so unusual widths are storable), the error reports WalletStorageError::WalletIdMismatch { found: [0;32] } — indistinguishable from the legitimate orphan/sentinel case. The wrong-length signal — the most operationally useful diagnostic — is silently lost. Either propagate a distinct typed variant for the wrong-length case (WalletIdMalformed { len: usize }) or hex-escape the actual bytes into the error message.
source: ['claude']
| fn wallet_footprint( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<Vec<(&'static str, i64)>, WalletStorageError> { | ||
| let mut out = Vec::with_capacity(PER_WALLET_TABLES.len()); | ||
| for (table, scope) in PER_WALLET_TABLES { | ||
| let n: i64 = conn | ||
| .query_row( | ||
| &count_rows_for_wallet_sql(table, *scope), | ||
| rusqlite::params![wallet_id.as_slice()], | ||
| |row| row.get(0), | ||
| ) | ||
| .optional()? | ||
| .unwrap_or(0); | ||
| out.push((*table, n)); | ||
| } | ||
| Ok(out) | ||
| } | ||
|
|
||
| /// Same as [`wallet_footprint`] but on an open transaction so the | ||
| /// post-EXCLUSIVE re-check sees the locked snapshot. | ||
| fn wallet_footprint_tx( | ||
| tx: &rusqlite::Transaction<'_>, | ||
| wallet_id: &WalletId, | ||
| ) -> Result<Vec<(&'static str, i64)>, WalletStorageError> { | ||
| let mut out = Vec::with_capacity(PER_WALLET_TABLES.len()); | ||
| for (table, scope) in PER_WALLET_TABLES { | ||
| let n: i64 = tx | ||
| .query_row( | ||
| &count_rows_for_wallet_sql(table, *scope), | ||
| rusqlite::params![wallet_id.as_slice()], | ||
| |row| row.get(0), | ||
| ) | ||
| .optional()? | ||
| .unwrap_or(0); | ||
| out.push((*table, n)); | ||
| } | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: [NEW] wallet_footprint and wallet_footprint_tx are line-for-line duplicates
The two helpers differ only in the first parameter type — body, loop, SQL builder call, and OptionalExtension chain are identical. Both rusqlite Connection and Transaction expose query_row (the latter via Deref<Target = Connection>), so a single helper taking &Connection (or generic over a coercible source) would eliminate the duplicate. Today's correctness is fine; the cost is drift risk — a future contributor adding tracing or schema-aware logic to one copy will likely miss the other.
source: ['claude']
| .count()) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: [CARRY-FORWARD, STILL VALID] count_pending's inner has_schema_history check is dead by construction
Prior finding from 4dc7a3a — verified still present at 47972c0. The only caller (SqlitePersister::open at persister.rs:188-192) calls count_pending under if had_schema_history { … } else { pending.len() }, so the false-branch of count_pending's internal has_schema_history(conn)? check (line 1269 → return Ok(embedded.len())) is unreachable on the live call path. Every open of an existing DB pays one extra sqlite_master round-trip for the dead branch. Either drop the inner probe or make count_pending private to a code path that owns both arms.
source: ['claude']
3 participants
Summary
Lands the
rs-platform-wallet-storagecrate (SQLite-backedPlatformWalletPersistenceimplementation, V001+V002 schema, online backup/restore, EXCLUSIVE-based cross-process exclusion, typed errors with structured-failure reporting) along with the minimum trait-surface coupling onrs-platform-walletrequired to compile it.Stacked on
feat/platform-wallet-sqlite-persistor(PR #3625). When that lands, this rebases onto its merge tip.Scope (post-slim)
packages/rs-platform-wallet-storage/**(~46 files internal: src, migrations, tests, CLI)packages/rs-platform-wallet/):src/changeset/traits.rs—PersistenceError { kind: Transient|Fatal|Constraint, source }(CODE-004 typed reshape),CommitReport,DeleteWalletReport, trait methodsdelete_wallet+commit_writessrc/changeset/mod.rs— re-exports of new typessrc/wallet/asset_lock/sync/proof.rs—Backend("..".into())→backend(...)migration tied to CODE-004packages/rs-platform-wallet-ffi/src/persistence.rs— sameBackend → backend(...)migrationHighlights
PersistenceErrorwithkind: Transient | Fatal | Constraint+ typedsource: Box<dyn Error + Send + Sync>enabling retry-on-Transient + undo-on-Fatal without string parsing (CODE-004)wallet_idFK from identity-owned tables; cascade flows throughidentities.wallet_id(nullable + ON DELETE CASCADE). Orphan identities are first-class (CODE-002)fs2flock for restore — interlocks with all SQLite peers viabusy_timeout(CODE-005/007/010/015)delete_walletworks aroundrusqlite::Backupdeadlock inside outer EXCLUSIVE tx (CODE-006)CommitReport/DeleteWalletReport— hosts decide per-wallet retry policy (CODE-003/026)NamedTempFilestaging (ATOM-004), restore holds exclusive lock + chmod before persist (ATOM-005/010), prune accumulates per-file errors (ATOM-011),integrity_checkbefore migrations on open (ATOM-013),is_transientcovers I/O-class SQLite codes (ATOM-008)sqlite(default),cli(default),secrets(reserved),__test-helpers(crate-private)platform-wallet-storage(migrate / backup / restore / prune / inspect)What was moved out
Consumer-side hardening (5 test files + manager retry/cache/delete-wired call site + 2 PlatformWalletError variants), PROJ-001 FFI ABI break + Swift wrappers, CODE-017 cache, CODE-012/013 TODO comments, and trait orphan-bucket docstrings were extracted into a follow-up consumer-hardening PR to keep this one focused on the storage crate landing. See the slim-out commits at the tip of this branch.
Breaking changes
PlatformWalletPersistencetrait gainsdelete_walletandcommit_writes(withUnimplemented-returning defaults for backwards compatibility with downstream impls).PersistenceErrorrestructured to typed{ kind, source }shape (wasBackend(String)).Test plan
cargo fmt --all -- --checkcargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets -- -D warningscargo test -p platform-wallet -p platform-wallet-storage(32 test binaries, all green)e28069b606) — pendingRelated
feat/platform-wallet-sqlite-persistor) — base PR this stacks on🤖 Generated with Claude Code