feat(platform-wallet): add platform-wallet-storage crate (sqlite persister)#3625
Draft
Claudius-Maginificent wants to merge 17 commits into
Draft
feat(platform-wallet): add platform-wallet-storage crate (sqlite persister)#3625Claudius-Maginificent wants to merge 17 commits into
Claudius-Maginificent wants to merge 17 commits into
Conversation
New workspace crate `platform-wallet-sqlite` implementing the
`PlatformWalletPersistence` trait against a bundled SQLite backend, plus
a `platform-wallet-sqlite` maintenance CLI.
Highlights
- Per-wallet in-memory buffer with `Merge`-respecting `store` + atomic
per-wallet `flush` (one SQLite transaction per call).
- `FlushMode::{Immediate, Manual}` with `commit_writes` aggregating
dirty wallets in deterministic order.
- Online backup via `rusqlite::backup::Backup::run_to_completion`,
source-validating `restore_from`, `prune_backups` retention with
AND-semantics, automatic pre-migration and pre-delete backups (with
typed `AutoBackupDisabled` refusal when `auto_backup_dir = None`).
- Refinery-driven barrel migrations under `migrations/`; FK enforcement
emulated with triggers because barrel's column builder doesn't emit
composite-key `FK` clauses portably on SQLite.
- `delete_wallet` cascade with `DeleteWalletReport`; `inspect_counts`
surface for the CLI.
- CLI: `migrate`, `backup`, `restore`, `prune`, `inspect`,
`delete-wallet` with `--yes` destructive-op guards, humantime
retention parsing, and stdout/stderr/exit-code conventions matching
the spec.
- 52 tests across 8 files plus compile-time assertions cover every
FR/NFR except the ones blocked on upstream `serde`/`bincode`
derives or a `Wallet::from_persisted` constructor (tracked in
TODOs in `persister.rs::load` and the test modules' module-docs).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o.toml Phase 2.2 fix wave — addresses Adams' BLOCK findings. - PROJ-001: add `platform-wallet-sqlite` to both `--package` lists in `tests-rs-workspace.yml` (coverage run and the Ubuntu 4-shard fallback) so CI actually executes the crate's tests. - PROJ-002: append `packages/rs-platform-wallet-sqlite` to every enumerated `COPY --parents` block in the Dockerfile (the chef prepare stage, the artifact-build stage, and the rs-dapi stage). Workspace `Cargo.toml` already lists the member; chef would fail with "directory not found" without these copies. - PROJ-003: allow `wallet-sqlite` in the PR-title conventional- scopes list (matches the existing `feat(wallet-sqlite): …` commit). - PROJ-004: align `dash-sdk` feature flags with sibling `rs-platform-wallet` (`dashpay-contract`, `dpns-contract`); document why `dpp`, `dash-sdk`, and `bincode` are direct deps (they're actually used — Adams' "unused" claim was wrong for all three); drop the redundant `serde` feature from bincode. - PROJ-005: gate `lock_conn_for_test` and `config_for_test` behind `cfg(any(test, feature = "test-helpers"))` plus a new `test-helpers` dev feature; the crate's own `[dev-dependencies]` self-include now activates it for integration tests, so downstream consumers cannot reach the helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2.2 fix wave — addresses Diziet, Marvin, Smythe, Trillian BLOCKs.
Library
- D-01: new `SqlitePersister::delete_wallet_skip_backup(wallet_id)`
entry point that intentionally skips the auto-backup. The CLI's
`--no-auto-backup` now uses it instead of mutating
`auto_backup_dir` to `None` (which collided with the
`AutoBackupDisabled` refusal path and silently broke the flag).
- D-02: `delete_wallet` checks `wallet_metadata` existence BEFORE
running the auto-backup. Refusing on an unknown wallet id no
longer leaves an orphaned `.db` in the auto-backup directory.
- D-03: `restore_from` try-acquires an exclusive file lock on the
destination via `fs2::FileExt::try_lock_exclusive` and raises
`RestoreDestinationLocked` if the file is held. Falls through on
filesystems without advisory locking.
- D-04: `restore_from` reads the source DB's max
`refinery_schema_history.version` and raises
`SchemaVersionUnsupported { found, expected_range }` when it
exceeds the highest embedded migration version.
- SEC-001: `restore_from` stages via
`tempfile::NamedTempFile::new_in(parent)` plus `persist`. The
previous predictable `<dest>.db.restore-tmp` filename was a
symlink-plant TOCTOU window.
- DOC-007 / DOC-008: rustdoc on `RetentionPolicy` explains the
AND-semantics; `DeleteWalletReport.backup_path` documents that
`None` ONLY happens via the new skip-backup entry point.
CLI
- D-05: `-v`/`-vv`/`-vvv`/`-q` wired to a `tracing_subscriber::fmt`
subscriber that writes to stderr with an `EnvFilter` defaulted
from the flag count (`warn` / `info` / `debug` / `trace`); `-q`
forces `error`.
- `delete-wallet --no-auto-backup` now routes through
`delete_wallet_skip_backup` and prints empty stdout (no backup
path) with the `warning: auto-backup skipped (--no-auto-backup)`
line on stderr.
Tests
- QA-001: new TC-023 in `tests/buffer_semantics.rs` — registers a
`commit_hook` on the write connection (rusqlite `hooks` feature),
then drives a flush whose changeset touches `core_sync_state`,
`wallet_metadata`, and `token_balances`. The hook MUST fire
exactly once. Atomicity is now empirically verified.
- QA-008: `tests/load_reconstruction.rs::tc043_*` rewritten to
store non-empty `ContactChangeSet` and `TokenBalanceChangeSet`
payloads (the previous Defaults were `is_empty()` and got
skipped by the buffer). The test now reopens the persister,
directly SQL-queries `contacts_sent` and `token_balances` rows,
and asserts `ClientStartState.platform_addresses` stays empty.
- SEC-006: new `tests/secrets_scan.rs` greps every file under
`src/schema/` and `migrations/` for the substrings `private`,
`mnemonic`, `seed`, `xpriv`, `secret`. A small allow-list lets
doc comments mention the boundary while catching genuine slips.
Docs
- DOC-002: README CLI synopsis adds an explicit sentence about
`--yes` being REQUIRED for destructive subcommands, plus a
logging-flag blurb.
- DOC-016: new per-crate `CHANGELOG.md` with `[Unreleased]` section
enumerating the additions and security fixes from this fix wave
(the workspace CHANGELOG is generated from Conventional Commits).
- SECRETS.md audit-hooks section updated to point at
`tests/secrets_scan.rs` and the TC-082 lint test by file:line.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. 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:
📝 WalkthroughWalkthroughAdds a new SQLite-backed wallet storage crate with migrations, backup/restore, CLI, and tests; integrates it into workspace, CI, and Docker. Also introduces an optional serde feature to platform-wallet for serializing changesets and related types using adapters. ChangesSQLite Wallet Storage Implementation
Workspace & Build System Integration
Optional Serde Serialization in platform-wallet
Sequence Diagram(s)sequenceDiagram
participant CLI (platform-wallet-storage)
participant SqlitePersister
participant SQLite DB
participant FS (backups)
CLI (platform-wallet-storage)->>SqlitePersister: migrate/open(db, config)
SqlitePersister->>SQLite DB: apply pragmas + run embedded migrations
CLI (platform-wallet-storage)->>SqlitePersister: backup/restore/prune/delete/inspect
SqlitePersister->>FS (backups): create/prune backups or restore atomically
SqlitePersister->>SQLite DB: read/write per-table data in transactions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Claudius-Maginificent
changed the title
Add a new `serde` Cargo feature on `platform-wallet`. When enabled, every type carried in a `PlatformWalletChangeSet` gains `serde::Serialize` / `serde::Deserialize` derives via `#[cfg_attr(feature = "serde", derive(...))]`: - `CoreChangeSet`, `IdentityChangeSet`, `IdentityEntry`, `IdentityKeysChangeSet`, `IdentityKeyEntry`, `IdentityKeyDerivationIndices`, `ContactChangeSet`, `ContactRequestEntry`, `SentContactRequestKey`, `ReceivedContactRequestKey`, `PlatformAddressChangeSet`, `PlatformAddressBalanceEntry`, `AssetLockChangeSet`, `AssetLockEntry`, `TokenBalanceChangeSet`, `WalletMetadataEntry`, `AccountRegistrationEntry`, `AccountAddressPoolEntry`, and the top-level `PlatformWalletChangeSet`. - Per-identity / DashPay leaf types referenced inside those changesets: `BlockTime`, `IdentityStatus`, `DpnsNameInfo`, `DashPayProfile`, `ContactRequest`, `EstablishedContact`, `PaymentEntry`, `PaymentDirection`, `PaymentStatus`, `AssetLockStatus`. The feature activates `key-wallet/serde` (which transitively flips `dashcore/serde` and `dash-network/serde`) so every upstream leaf type already wired with `#[cfg_attr(feature = "serde", ...)]` (TransactionRecord, Utxo, InstantLock, AccountType, AddressInfo, AddressPoolType, ExtendedPubKey, Network) round-trips cleanly. Two upstream types lack their own serde feature and use `#[serde(with = ...)]` adapters in the new `src/changeset/serde_adapters.rs` module: - `AssetLockFundingType` (key-wallet, no `serde` derive) — encoded as a stable u8 tag matching the prior hand-rolled blob layout. - `AddressFunds` (dash-sdk re-export, no serde derive) — encoded as a `(nonce, balance)` shadow struct. One field is marked `#[serde(skip)]`: - `CoreChangeSet::addresses_derived` carries `key_wallet_manager::DerivedAddress`, which has no serde derive AND no `key-wallet-manager/serde` feature to activate. The breadcrumb is written to a typed table by persisters, not via a changeset blob, so skipping costs nothing. `cargo build -p platform-wallet` (no features) and `cargo build -p platform-wallet --features serde` both build clean. `cargo test -p platform-wallet` passes (8 lib tests, 121 integration tests) with and without the new feature. The change is opt-in; the default-feature build is byte-identical to its prior shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allet-storage and restructure for future secrets submodule PURE rename + restructure — no functional code changes. Carves out a spot for a future `SecretStore` (sketched in `SECRETS.md`) to land as a `secrets` submodule inside the same crate, rather than a separate `platform-wallet-secrets` crate. Crate metadata - Cargo package name: `platform-wallet-sqlite` → `platform-wallet-storage`. - Crate directory: `packages/rs-platform-wallet-sqlite/` → `packages/rs-platform-wallet-storage/`. - Binary name: `platform-wallet-sqlite` → `platform-wallet-storage`. Module layout - Everything SQLite-related is now under `src/sqlite/`: `mod.rs` (new — re-exports the submodules), `persister.rs`, `buffer.rs`, `config.rs`, `error.rs`, `migrations.rs`, `backup.rs`, and `schema/`. The `migrations/` Rust-file directory stays at the crate root because `refinery::embed_migrations!` resolves its path relative to `Cargo.toml`. - `src/lib.rs` exposes `pub mod sqlite;` plus root re-exports of the common types (`SqlitePersister`, `SqlitePersisterConfig`, `FlushMode`, `SqlitePersisterError`, `RetentionPolicy`, `PruneReport`, `DeleteWalletReport`, `AutoBackupOperation`, `JournalMode`, `Synchronous`) so most consumer imports stay identical — only the crate name in `Cargo.toml` changes for them. A `// pub mod secrets;` marker reserves the future module slot. Cargo features - `sqlite` (default) — enables the SQLite persister + every backend- specific optional dep (`rusqlite`, `refinery`, `barrel`, `dpp`, `dash-sdk`, `key-wallet`, `key-wallet-manager`, `dashcore`, `bincode`, `fs2`, `tempfile`, `chrono`, `sha2`). - `cli` (default) — enables the maintenance binary; implies `sqlite`. - `secrets` — reserved, no code yet. - `test-helpers` — crate-private accessors (unchanged semantics); now implies `sqlite`. - `cargo build -p platform-wallet-storage --no-default-features` builds the bare crate cleanly (verified). Tests - Renamed `tests/<name>.rs` → `tests/sqlite_<name>.rs` (9 files) so the future `secrets_<name>.rs` files won't collide. `secrets_scan.rs` and `tests/common/` keep their names. - `secrets_scan.rs` updated to scan `src/sqlite/schema/` (the new location of the schema writers) and `migrations/`. Carved out `src/secrets/` from the scan up front — that future submodule WILL legitimately contain the words `private`, `mnemonic`, `seed`. Workspace integration - `Cargo.toml` workspace `members` entry renamed. - `Dockerfile`: three `COPY --parents` blocks updated. - `.github/workflows/tests-rs-workspace.yml`: two `--package` lines updated. - `.github/workflows/pr.yml`: added `wallet-storage` alongside the existing `wallet-sqlite` allow-list entry (both coexist so PRs pending against either name pass). Gate output - `cargo fmt --all -- --check` clean. - `cargo build -p platform-wallet-storage` clean. - `cargo build -p platform-wallet-storage --no-default-features` clean. - `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean. - `cargo test -p platform-wallet-storage` — 54 tests, 0 failures. - `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean. - `cargo check --workspace --offline` clean. - `cargo metadata` no longer exposes the old `platform-wallet-sqlite` package name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hand-rolled encoder Replace the hand-rolled `BlobWriter` / `BlobReader` plumbing under `src/sqlite/schema/` with a single `bincode::serde::encode_to_vec` call per row, acting on the serde-derived changeset types in `platform-wallet` (enabled via that crate's `serde` feature, added in the preceding commit). The encoder swap is the technical-debt cleanup the workflow-feature plan called for. Wire format - Every `_blob` column now starts with a 1-byte schema-revision tag (`blob::BLOB_REV = 1`) followed by the bincode-serde body. The tag lets future migrations swap encoders without losing existing rows; unknown revisions surface as `SqlitePersisterError::Serialization`. - `blob::encode<T: Serialize>` and `blob::decode<T: DeserializeOwned>` are the only public entry points; the previous per-field `u8/u32/u64/bytes/opt_*/str` walker is gone. - The outpoint helpers (`encode_outpoint` / `decode_outpoint`) stay in `blob.rs` because outpoints serve as primary-key fragments — they were never `_blob` payloads to begin with. Per-schema-file delta - `accounts.rs`: dropped the manual `BlobWriter` for both `AccountRegistrationEntry` and `AccountAddressPoolEntry`; each row now encodes the full entry via `blob::encode`. Schema-stable typed columns (`account_type`, `account_index`, `pool_type`) still mirror the entry for direct SQL lookups. - `asset_locks.rs`: collapsed the funding-type-tag / tx-consensus / proof-bincode three-part hand-rolled blob into a single `blob::encode(&AssetLockEntry)` call. `funding_type` rides through the new `platform_wallet::changeset::serde_adapters::asset_lock_funding_type` adapter; `Transaction` and `AssetLockProof` round-trip via their own serde derives. ~30 LOC removed. - `contacts.rs`: each `_blob` cell now stores the `ContactRequestEntry` / `EstablishedContact` directly. - `core_state.rs`: `core_transactions.record_blob` now encodes the full `TransactionRecord`; `core_instant_locks.islock_blob` encodes the `InstantLock` via dashcore's serde derive (which was always there, gated on `dashcore/serde` — flipped on by `platform-wallet/ serde`). The placeholder-record decoder gymnastics in `get_tx_record` collapse into a one-line `blob::decode` call. - `dashpay.rs`: `dashpay_profiles.profile_blob` encodes the whole `DashPayProfile`; `dashpay_payments_overlay.overlay_blob` encodes each `PaymentEntry`. - `identities.rs`: `entry_blob` encodes the full `IdentityEntry`; new `fetch` helper for tests. - `identity_keys.rs`: dpp's `IdentityPublicKey` uses `serde(tag = "$formatVersion")` which bincode-serde's `deserialize_any` requirement can't navigate. Solution: an in-crate wire shape (`IdentityKeyWire`) pre-encodes that one field via dpp's native `bincode::Encode/Decode` derives while everything else stays on bincode-serde. Same "one blob per row" property; one layer of indirection for the offending field. Unblocked tests (Marvin's previously-deferred TC-002..TC-014) - TC-007 — `IdentityKeyEntry` round-trip including the public key, hash, and DIP-9 derivation breadcrumbs; plus an inline NFR-10 substring scan that asserts the blob contains no `private`/`mnemonic`/`seed`/`xpriv` ASCII. - TC-009 — `PlatformAddressBalanceEntry` round-trip including the `AddressFunds` (via the `address_funds` serde adapter). - TC-010 — `AssetLockEntry` round-trip including the embedded `Transaction`, `AssetLockFundingType` (via the `asset_lock_funding_type` adapter), and `AssetLockStatus`. - TC-012 — `DashPayProfile` + `PaymentEntry` round-trip through the dashpay tables. - TC-014 — `AccountRegistrationEntry` round-trip including the full `ExtendedPubKey` (via key-wallet's serde derive). Gate output - `cargo fmt --all -- --check` clean. - `cargo build -p platform-wallet-storage` clean. - `cargo build -p platform-wallet-storage --no-default-features` clean. - `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean. - `cargo test -p platform-wallet-storage` — 60 tests, 0 failures (up from 54 before this commit; +5 new TCs in `sqlite_persist_roundtrip.rs` plus +1 in the blob.rs lib-test suite). - `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean. - `cargo check --workspace --offline` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Claudius-Maginificent
changed the title
…tion version for forward-compat The refinery migration version on the database already gates schema evolution at the right granularity — every row in every `_blob` column is written by code at the same revision, so a per-blob revision byte was redundant. Changes - `src/sqlite/schema/blob.rs`: remove the `BLOB_REV` constant and its prepend / strip logic. `encode<T>` is now a one-line wrapper over `bincode::serde::encode_to_vec`; `decode<T>` is the matching pair over `decode_from_slice`. Net: ~30 LOC dropped from the module. - Drop the two unit tests (`decode_rejects_unknown_rev`, `decode_rejects_empty_blob`) that exercised the rev-tag logic exclusively — the behaviour they covered no longer exists. The `encode_decode_roundtrip` and `outpoint_roundtrip` tests stay. - `src/sqlite/schema/mod.rs`: update the module-level encoding-policy doc to drop the "1-byte schema-rev tag" framing and explain that schema evolution is gated by the refinery migration version instead. - `src/sqlite/schema/asset_locks.rs`: drop the analogous comment about the rev tag in that module's header. `encode_outpoint` / `decode_outpoint` are untouched — they're a separate concern (typed-column primary-key encoding, fixed layout for indexed lookups, never blob payloads). Migration concern: NONE. The crate is unreleased; no existing on-disk `.db` files carry the BLOB_REV byte. Anyone with a wallet-storage test database between the previous commit and this one needs to delete it — flagged in the workspace CHANGELOG. Gate - `cargo fmt --all -- --check` clean. - `cargo build -p platform-wallet-storage` clean. - `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean. - `cargo test -p platform-wallet-storage` — 58 tests, 0 failures (down from 60: the two dropped tests were rev-tag-specific). - `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `prune` subcommand returns to the unconditional shape: walk the
backup directory, apply the retention policy, unlink, print removed
paths to stdout. Operators who want a preview can list the directory
themselves before running.
Changes
- `src/bin/platform-wallet-storage.rs`: drop the `dry_run: bool`
field on `PruneArgs`, the `if args.dry_run { ... }` branch in
`run_prune`, and the `list_backup_dir_for_dry_run` helper (only
caller was the dry-run branch).
- `README.md`: trim `[--dry-run]` from the `prune` synopsis line.
- `CHANGELOG.md`: note the flag removal in `[Unreleased]`.
No CLI smoke test referenced `--dry-run`, so the 58-test count is
unchanged. Gate is clean: fmt / build / bin build / 58 tests / clippy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allet-storage rename PROJ-002: `CoreChangeSet.addresses_derived` doc block referenced `rs-platform-wallet-sqlite::schema::core_state`, the path the crate had before `8e0830626d` renamed it to `rs-platform-wallet-storage` and regrouped the module layout under `sqlite/`. The rename swept every import + Cargo.toml + workflow file but missed this single doc-string in the sister crate, which a grep-driven reader would follow to a dead path. Replace with the current canonical path: `platform_wallet_storage::sqlite::schema::core_state`. No code change. No test change. Independently cherry-pickable into the future upstream PR alongside `e26945cfdf` (the original serde-feature commit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Error, atomic variants, propagate SQL errors
Atomic-variant error type per the dash-evo-tool error pattern
(`~/git/dash-evo-tool/CLAUDE.md` §Error messages): every variant
carries the upstream error via `#[source]` (or `#[from]` when the
conversion is the only thing the trait does), never via a
stringified copy. Variants do not contain user-facing-prose
`String` fields — the `#[error("...")]` attribute provides the
renderable `Display` form, the typed fields carry diagnostics.
Resolves CODE-002, SEC-002, PROJ-001, CODE-004, CODE-008 (partial),
SEC-001 (library half — CLI half in Commit D). Annotates CODE-001
with INTENTIONAL per triage decision.
Error type
- `SqlitePersisterError` → `WalletStorageError`. The old name lives
as a `#[deprecated]` type alias so existing callers compile during
the migration; tests in this crate already use the new name.
- Split `Sqlite` callers into `IntegrityCheckRunFailed`,
`SourceOpenFailed`, and the generic `Sqlite { source }`. The
`IntegrityCheckFailed { check_output: String }` variant becomes
`IntegrityCheckFailed { report: String }` — the SQLite-returned
diagnostic text is not a user-facing message; the rename
clarifies that.
- `Serialization(String)` (a stringified bincode error) split into
`BincodeEncode { source: bincode::error::EncodeError }`,
`BincodeDecode { source: bincode::error::DecodeError }`, and
`BlobDecode { reason: &'static str }` for typed-column structural
errors. `&'static str` is acceptable per the policy — it's a
compile-time identifier, not a user message.
- `InvalidWalletId(String)` split into `InvalidWalletIdHex { source:
hex::FromHexError }` and `InvalidWalletIdLength { actual: usize }`.
- `ConfigInvalid(&'static str)` → `ConfigInvalid { reason: &'static str }`.
- `SchemaVersionUnsupported { found: i64, expected_range: String }`
→ `SchemaVersionUnsupported { found: i64, max_supported: i64 }`.
- New variants: `HashDecode { source: dashcore::hashes::Error }`,
`ConsensusCodec { source: dashcore::consensus::encode::Error }`,
`IntegerOverflow { field: &'static str, value: u64, target:
SafeCastTarget }`, `LoadIncomplete { unimplemented: &'static
[&'static str] }`.
- `From` impls added for every typed source so `?`-style propagation
works at every writer / reader boundary.
- `From<WalletStorageError> for PersistenceError` renders the full
`#[source]` chain via a private `DisplayChain` helper instead of
losing the inner-error context to a single `Display` call.
Safe-cast helper (SEC-002)
- New module `src/sqlite/util/safe_cast.rs` with `u64_to_i64(field:
&'static str, value: u64) -> Result<i64, WalletStorageError>` and
the inverse. Every durable-boundary cast in writers/readers now
routes through these — schema/platform_addrs (balance, sync_height,
sync_timestamp, last_known_recent_block, nonce, account_index,
address_index), schema/asset_locks (amount_duffs, account_index),
schema/token_balances (balance), schema/core_state (utxo.value,
utxo.height, account_index), schema/identities (no u64 columns —
identity_index is u32, uses `i64::from`).
- Lossless `u32 → i64` casts swapped to `i64::from(...)` so static
conversions stay clearly distinct from fallible-cast sites.
Error propagation (CODE-002)
- Every `query_row(...).unwrap_or(default)` that previously
swallowed real SQL errors (busy-timeout, corrupt, decode) now
uses `.optional()?.unwrap_or(default)` — `optional()?` collapses
ONLY the genuine "no rows returned" case into `None`; every other
error propagates as `WalletStorageError::Sqlite`.
- `current_schema_version` and `count_pending` now return
`Result<_, WalletStorageError>` instead of swallowing into
`Option`. Migrate / open paths surface those errors instead of
silently re-running every migration on a corrupt schema-history.
- `delete_wallet_inner` existence check + per-table row-count
queries use `.optional()?` so a corrupt child table fails loudly
instead of reporting 0 rows removed.
Auto-backup dedup (CODE-004)
- `run_auto_backup` extracted as a standalone function in
`persister.rs`. Both the open-time (`PreMigration`) and library-
time (`PreDelete`, new `PreRestore`) paths call it. The previous
`unreachable!("OpenMigration not callable via run_auto_backup")`
branch is gone — there is no longer a closed-over self that
prevents the open path from reusing the helper.
- `BackupKind::PreRestore` variant added; `is_backup_file` /
retention recognise the `pre-restore-` prefix.
LoadIncomplete (PROJ-001)
- `LOAD_UNIMPLEMENTED: &[&str]` pub-const lists the
`ClientStartState` field paths the persister does not yet
reconstruct (`["ClientStartState::wallets"]` today).
- Trait-impl `load()` rustdoc explicitly documents the partial-
reconstruction caveat at the top, points at `LOAD_UNIMPLEMENTED`,
and emits a `tracing::warn!` on every call until the upstream
`Wallet::from_persisted` lands.
- New `WalletStorageError::LoadIncomplete` variant exists for
callers that want to surface the gap as a typed value (not
returned from `load` itself per the trait contract — see rustdoc).
restore_from auto-backup (SEC-001 library half)
- `SqlitePersister::restore_from(dest, src, auto_backup_dir)` —
takes a pre-restore auto-backup of the live destination before
staging the source over it. Refuses with
`AutoBackupDisabled { operation: Restore }` when `auto_backup_dir`
is `None`. New `SqlitePersister::restore_from_skip_backup(dest,
src)` for the CLI's `--no-auto-backup` flag (added to RestoreArgs
here for the corresponding CLI surface).
- `backup::restore_from` keeps the source-validation +
destination-lock + staged-tempfile + atomic-persist shape; the
pre-restore backup is taken by the persister's `_inner` before
calling into `backup::restore_from`. (SEC-004 — staged-tempfile
integrity recheck + chmod 600 — also lands in this commit.)
Write probe (CODE-008)
- `ensure_dir`'s predictable `.platform-wallet-storage-write-probe`
filename replaced by `tempfile::NamedTempFile::new_in(dir)` —
unguessable name per probe, no race against concurrent persister
opens.
CODE-001 INTENTIONAL annotation
- Inline comment on the `Mutex<Connection>` declaration documents
the accept-risk decision: single connection serializes reads
through the write lock, acceptable for current per-wallet
workload, revisit if read contention becomes measurable.
Test sweep
- Every `tests/sqlite_*.rs` file migrated from `SqlitePersisterError`
to `WalletStorageError`. The deprecated alias still resolves but
emits `#[deprecated]` warnings under `-D deprecated`; live code
uses the new name. Restore tests call
`SqlitePersister::restore_from_skip_backup` to avoid threading an
`auto_backup_dir` through fixture helpers.
Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean (default features).
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 62 tests, 0 failures
(+4 from new safe_cast unit tests).
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… migration tracking SEC-003: V001 emulates FK INSERT parent-existence + AFTER-DELETE cascade via triggers but doesn't cover `UPDATE wallet_id` on `wallet_metadata` or `UPDATE identity_id` on `identity_keys` / `dashpay_profiles`. The persister's own writers never mutate those columns, but if a future migration accidentally introduces such an UPDATE the result is silent orphaning of child rows. New migration `V002__defensive_update_triggers.rs` installs `BEFORE UPDATE OF <id>` triggers on each that raise the canonical `RAISE(ABORT, 'FOREIGN KEY constraint failed')` — same idiom V001 uses for the parent-existence check, so downstream string matching stays stable. V001 stays untouched per the append-only migration policy. Also: `build.rs` emits `cargo:rerun-if-changed` for each file under `migrations/`. `refinery::embed_migrations!` is a proc-macro evaluated at compile time; Cargo doesn't track file-system reads inside proc macros, so without this build-script directive, adding/editing a migration file fails to trigger a rebuild of the embedded list. Discovered while wiring V002 — `tc025` failed against a stale cache until `migrations.rs` was manually touched. The build-script closes that gap. Gate - `cargo fmt --all -- --check` clean. - `cargo build -p platform-wallet-storage` clean. - `cargo test -p platform-wallet-storage` — 62 tests, 0 failures. - `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…caping, scope allow-list, stable enum labels, docs)
Closes the cleanup batch from the Phase-2.8 triage report:
PROJ-003, PROJ-004, SEC-005, SEC-006, CODE-003, DOC-002, DOC-005,
plus a related DOC-001 correction (FK README claim).
PROJ-003 — Remove `wallet-sqlite` from `.github/workflows/pr.yml`.
The three historical commits using that scope are already on the
branch; future commits in this crate use `wallet-storage`. No
reason to keep a deprecated name in the allow-list.
PROJ-004 — Delete `packages/rs-platform-wallet-storage/CHANGELOG.md`.
The user explicitly stated we don't maintain per-crate CHANGELOGs;
the workspace-level CHANGELOG.md is generated from Conventional
Commits and remains the single source of truth.
SEC-005 — Delete the substring-scan block in
`tests/sqlite_persist_roundtrip.rs::tc007_identity_key_entry_roundtrip`.
bincode wire bytes carry no field names, so the substring scan
against `public_key_blob` conveyed intent but enforced nothing.
The load-bearing NFR-10 check is `tests/secrets_scan.rs`, which
greps schema source files. Comment in tc007 redirects readers
there.
SEC-006 — Replace hand-rolled JSON in `run_inspect --format json`
with `serde_json::json!`. `serde_json` added as an optional dep
gated by the `cli` feature. Today's input is safe (table names are
compile-time identifiers; wallet ids are hex), but any future
addition that flows user-controlled bytes into the printer would
break the previous escape-less `print!`.
CODE-003 — `format!("{:?}", entry.account_type)` /
`format!("{:?}", entry.pool_type)` replaced with new pub(crate)
helpers `account_type_db_label(&AccountType) -> &'static str` and
`pool_type_db_label(&AddressPoolType) -> &'static str` in
`schema/accounts.rs`. Both are exhaustive `match` expressions —
adding a variant upstream fails to compile here, forcing an
explicit label decision rather than silent `Debug`-format drift.
`schema/core_state.rs` (derived-addresses writer) uses the same
helpers.
DOC-002 — `tests/secrets_scan.rs` docstring updated: scan path is
`src/sqlite/schema/` not `src/schema/`. Explicitly carves out files
in `src/sqlite/` outside `schema/` plus the future `src/secrets/`
slot as out-of-scope.
DOC-005 — README `--no-default-features` paragraph rewritten:
factual description of what the bare crate provides today (nothing
public), no future-feature framing per user's "no future
placeholders" rule.
DOC-001 (bonus correction) — README schema section updated to
reflect V002's defensive UPDATE triggers. The previous "identical
to native FKs" claim was false on UPDATE before V002; with V002
landed the claim becomes accurate and the section explicitly cites
both migrations.
INTENTIONAL annotations already in place from Commits B/C —
CODE-001 (single connection serialises reads) at
`src/sqlite/persister.rs:78-84`; CODE-007 (prune fails-fast) at
`src/sqlite/backup.rs:200-204`. PROJ-005's accept-risk rationale
is captured inline above the `lock_conn_for_test` accessor at
`src/sqlite/persister.rs:299-307`.
Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo build -p platform-wallet-storage --no-default-features` clean.
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 62 tests, 0 failures.
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
@coderabbitai review all |
Contributor
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (11)
packages/rs-platform-wallet-storage/src/sqlite/config.rs (1)
111-118: 💤 Low valueMinor: Simplify empty parent check.
The
.filter(|p| !p.as_os_str().is_empty())check at line 114 is defensive, butPath::parent()should not return an emptyOsStrin practice for well-formed paths. The fallback to"."is safe regardless.Consider simplifying to:
let parent = db_path.parent().unwrap_or_else(|| Path::new(".")); parent.join("backups").join("auto")However, the current implementation is safe and may handle edge cases in path handling, so this is optional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs` around lines 111 - 118, The function default_auto_backup_dir contains an overly defensive check when computing parent from db_path; simplify by using db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and then join "backups"/"auto" off that parent. Update the local variable used (currently named parent) to hold the &Path from parent() rather than mapping to a PathBuf, and then call parent.join("backups").join("auto") to produce the final PathBuf.packages/rs-platform-wallet-storage/tests/common/mod.rs (1)
47-56: 💤 Low valueHardcoded
'testnet'may collide with tests asserting onnetwork.
ensure_wallet_metaalways writesnetwork = 'testnet'. Tests that later assert on wallet metadata or persist aWalletMetadataEntrywith a different network (e.g.,tc023_one_flush_is_one_transactionwritesNetwork::Testnetand would silently match, but other tests writingMainnetwould observe a stale row from this helper). Consider takingnetworkas a parameter (defaulting to"testnet") so call sites can match their changeset's expectations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/common/mod.rs` around lines 47 - 56, The helper ensure_wallet_meta currently hardcodes network = 'testnet', which can collide with tests expecting other networks; change the signature of ensure_wallet_meta(persister: &SqlitePersister, wallet_id: &WalletId) to accept a network parameter (e.g., network: &str) with callers passing "testnet" where appropriate, update the INSERT SQL call in ensure_wallet_meta to bind the network parameter instead of the literal 'testnet', and update all test call sites (or add a convenience wrapper) to pass the correct network values (e.g., "mainnet" or "testnet") so tests see consistent wallet_metadata rows.packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs (2)
188-231: 💤 Low valueTwo
if let Cmd::Migratebranches — fold them.The
Migratecommand is special-cased twice in succession (lines 219–231 to tweakconfig/ validate flags, then lines 235–244 to actually run it). Folding both into a singleif let Cmd::Migrate(m) = &cli.cmd { ... return ... }block (or arun_migratehelper) would keep all of the migrate-specific logic in one place and remove theunreachable!()arm on line 247.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs` around lines 188 - 231, The migrate handling is split across two places; fold the two separate `if let Cmd::Migrate` blocks into one contiguous block so all migrate-specific validation and invocation live together. Locate the existing `if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate `auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of `SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into the same block where the migrate command is executed (or replace both with a single `run_migrate` helper that accepts `db`, `m`, `config`, and `auto_backup_dir`), then perform the `run_migrate` call and return its Result immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and the `unreachable!()` arm.
122-134: 💤 Low valueReject uppercase / odd-length hex consistently.
parse_wallet_idcheckss.len() == 64then defers tohex::decode, which accepts both upper- and lower-case but not mixed-case-with-non-hex obviously — that's fine. However, the error message on line 124 leaks the raw user-supplied string back into stderr; if this binary is ever invoked from a context that pipes secrets-like opaque IDs, echoing them is undesirable. Consider dropping the trailing(`{}`)from the message and just naming the length, matching thewallet id is not valid hexstyle on line 130.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs` around lines 122 - 134, In parse_wallet_id, stop echoing the raw input in the length error and validate/normalize hex consistently: when s.len() != 64 return an Err that only mentions the expected length (do not include `s`), and replace relying solely on hex::decode with an explicit check that all chars are lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id is not valid hex" error for decoding failures but ensure messages follow the same non-leaking style.packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs (2)
273-278: 💤 Low valueRemove redundant
_unused_btreemapdead-code shim.
BTreeMapis now actually used intc023_one_flush_is_one_transaction(see line 308), so the#[allow(dead_code)] fn _unused_btreemapworkaround can be deleted along with its comment.♻️ Suggested removal
-// Mark the unused `BTreeMap` import as used in case future expansion of -// this test file needs it. -#[allow(dead_code)] -fn _unused_btreemap() -> BTreeMap<u32, u32> { - BTreeMap::new() -} -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs` around lines 273 - 278, Remove the redundant dead-code shim `_unused_btreemap` and its explanatory comment: delete the `#[allow(dead_code)] fn _unused_btreemap() -> BTreeMap<u32, u32> { BTreeMap::new() }` block since `BTreeMap` is now actually used in the test `tc023_one_flush_is_one_transaction`, making the shim unnecessary; ensure imports remain and run tests to confirm nothing else depends on that helper.
154-180: 💤 Low valueProptest opens a fresh SQLite DB per case — consider reusing one persister.
fresh_persister()runs once per proptest case (×64), each migrating a brand-new on-disk SQLite DB. That makes this test materially slower and more fragile on tmpfs/CI than necessary. Since the property is purely about monotonic-max merge oncore_sync_statefor a single wallet, you could hoist persister creation out of the proptest closure and just use a freshwidper case (or reset the row), keeping the same DB across cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs` around lines 154 - 180, Test currently calls fresh_persister() inside the proptest closure causing a new on-disk SQLite DB per case; move persister creation out of the proptest! Create the persister once before proptest (call fresh_persister() once to get persister/_tmp/_path), then inside the proptest closure use a new wallet id (wid) per case or clear/reset the core_sync_state row (use ensure_wallet_meta and persister.store with core_with_height) so each case reuses the same DB connection and only varies the wallet or row content; update references to fresh_persister, wid, ensure_wallet_meta and persister accordingly.packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)
72-102: 💤 Low value
ensure_existswrites outside the buffer/flush transaction boundary.
ensure_existstakes a&Connectionand runs an immediateINSERT OR IGNORErather than participating in the in-memory merge buffer + per-flush transaction model used byapply. This is fine for the documented use case (tests poking the DB before exercisingidentity_keys), but worth a doc note so production callers don't reach for it and accidentally couple stub writes to a different durability boundary than the rest of a changeset flush.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs` around lines 72 - 102, ensure_exists currently performs an immediate INSERT OR IGNORE on a plain &Connection, which bypasses the in-memory merge buffer and per-flush transaction used by apply and can cause durability/ordering mismatches; update ensure_exists to participate in the same flush boundary by either accepting and using the existing merge buffer/flush transaction or a transactional handle (e.g., a &Transaction or buffer API used by apply) and executing the INSERT within that transaction, or if intentionally intended only for tests, add a clear doc comment on ensure_exists describing that it writes immediately to the DB and must not be used by production code that relies on the merge-buffer + per-flush transaction model (reference ensure_exists, apply, and the in-memory merge buffer/flush transaction behavior in your comment).packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs (1)
20-26: ⚡ Quick winConsider widening
SafeCastTargetto coveru32-bound writes.Several call sites (e.g.,
asset_locks::list_active,core_state::list_unspent_utxos) need to fail when ani64won't fit inu32and currently reportSafeCastTarget::U64, which is wrong. Adding aU32variant here (and a smalli64_to_u32helper) would let those readers go through this module with an accurate target label and consistent error shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs` around lines 20 - 26, The SafeCastTarget enum currently only lists I64 and U64 but callers need a U32 target; add a new variant SafeCastTarget::U32 and implement a small helper function i64_to_u32 that mirrors existing i64_to_u64/i64_to_i64 behavior (validate range, return Result<u32, SafeCastError> and report SafeCastTarget::U32 on failure). Update any match/log paths in this module that construct errors to use the new U32 variant so callers like asset_locks::list_active and core_state::list_unspent_utxos can call the helper and produce the correct error shape/message.packages/rs-platform-wallet-storage/migrations/V001__initial.rs (2)
185-191: 💤 Low valueComment references
inject_custombut the code appends raw DDL.The comment claims constraints/indexes are layered "via
inject_custom", but the implementation just concatenates DDL afterm.make::<Sqlite>(). Either switch to barrel'sinject_customAPI or update the comment to reflect what's actually happening so future maintainers don't go looking for aninject_customcall site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs` around lines 185 - 191, The comment mentions layering constraints/indexes "via `inject_custom`" but the code builds raw DDL by concatenating into `tail` after calling `m.make::<Sqlite>()`; either (A) change the implementation to use Barrel's `inject_custom` API to append the constraints/indexes instead of manual string concatenation (replace the `tail` concatenation and use `m.inject_custom(...)` at the appropriate spot), or (B) update the comment to accurately describe the current behavior (mention that raw DDL is appended to `tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so future maintainers are not misled; locate references to `tail`, `m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify accordingly.
281-326: 💤 Low valueUnused
_colsparameter inparent_checkclosure.The closure takes
_cols: &[&str]but never uses it; every call passes&["wallet_id"]. Consider removing the parameter, or actually using it to parameterize the FK column name so the closure can support FK relationships keyed on something other thanwallet_id(which would let you fold the bespokeidentity_keys/dashpay_profilestriggers below into the same helper).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs` around lines 281 - 326, The closure parent_check currently accepts an unused parameter _cols: &[&str]; either remove that parameter from parent_check and all its callers (keep it as parent_check(child: &str) and continue using wallet_id inside the generated trigger SQL), or update parent_check to accept cols: &[&str] and use cols[0] (or join cols) in place of the hard-coded wallet_id everywhere (including the WHEN clauses and DELETE WHERE clause) so the same helper can generate triggers for identities, identity_keys, dashpay_profiles, etc.; ensure you update the for loop callers accordingly to pass either no cols (if removed) or the appropriate slice like &["wallet_id"] or &["identity_id"] where needed.packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs (1)
102-107: ⚡ Quick win
SafeCastTarget::U64is misleading for au32overflow.The cast here is
i64 → u32, but the surfacedtargetisSafeCastTarget::U64. Operators reading the error will see"u64"and assume the value didn't fit inu64, when in fact it didn't fit inu32. Consider adding aU32variant toSafeCastTarget(and a correspondingi64_to_u32helper insafe_cast) so the error type accurately describes the target, and to avoid repeating thistry_frompattern at every reader. This same issue recurs incore_state.rsforcore_utxos.heightandcore_utxos.account_index.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs` around lines 102 - 107, The IntegerOverflow error currently reports SafeCastTarget::U64 for an i64→u32 conversion in asset_locks.rs (see the u32::try_from usage producing WalletStorageError::IntegerOverflow), which is misleading; add a U32 variant to crate::sqlite::util::safe_cast::SafeCastTarget and implement a dedicated i64_to_u32 helper in the safe_cast module that performs the conversion and returns WalletStorageError::IntegerOverflow with target = SafeCastTarget::U32 on failure, then replace the manual u32::try_from usages (e.g., the account_index conversion in asset_locks.rs and the similar conversions in core_state.rs for core_utxos.height and core_utxos.account_index) to call the new helper so errors accurately report the intended target type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 235-244: The current migrate branch uses peek_schema_version which
swallows all errors into None causing applied to be unreliable; change
peek_schema_version to return Result<Option<i64>, E> (i.e., Result<Option<i64>,
Box<dyn Error> or a concrete error type) and update the call site in the
Cmd::Migrate block: keep using the old behavior for pre_version (call
peek_schema_version before opening the DB if you must tolerate missing table),
but after SqlitePersister::open(config.clone()) call the new peek_schema_version
and propagate or error out on Err so the post_version probe cannot be silently
treated as 0; compute applied only when post_version is Ok(Some/_), and map
errors to the CLI error flow (use map_open_err_for_cli or similar) instead of
unwrap_or(0) so the printed "applied: N" is trustworthy.
- Around line 288-300: The CLI-level pre-check args.out.is_file() in run_backup
is redundant and does not prevent the TOCTOU race because backup_to() itself
checks exists() and ultimately calls run_to() -> Connection::open(dest) which
can be raced; either remove the args.out.is_file() guard from run_backup, or
(preferred) harden the persistence path by modifying backup_to()/run_to() to
perform atomic file creation (e.g., use OpenOptions::create_new() or equivalent)
when opening the destination instead of relying on exists()/Connection::open,
ensuring the creation fails if the file was created concurrently.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 517-520: The code that rebuilds state in load() uses
schema::platform_addrs::count_per_wallet and then inserts addrs into
state.platform_addresses only if count > 0 || addrs.sync_height > 0 ||
addrs.sync_timestamp > 0, which drops wallets whose only persisted platform
state is addrs.last_known_recent_block; update the reconstruction condition to
also check addrs.last_known_recent_block (e.g., include ||
addrs.last_known_recent_block > 0) so that entries with only
last_known_recent_block are preserved when inserting into
state.platform_addresses.
- Around line 412-468: flush_inner currently calls self.buffer.drain(wallet_id)
and discards the changeset (cs) before opening the DB transaction, so any
failure after draining (e.g., during schema::...::apply or tx.commit()) loses
the buffered changes; change the logic so the buffer is only removed after
tx.commit() succeeds: either fetch/clone/peek the changeset (use a
non-destructive read API instead of buffer.drain) into cs, execute the schema
apply calls and tx.commit(), and only then call buffer.drain(wallet_id) or
buffer.remove(wallet_id) to clear it; if you must drain early, ensure every
error path re-inserts/requeues the drained cs back into the buffer (e.g.,
buffer.insert(wallet_id, cs)) before returning the PersistenceError, referencing
flush_inner, self.buffer.drain(wallet_id), cs, and tx.commit() to locate the
edits.
- Around line 269-326: Before checking existence/backing up/deleting in
delete_wallet_inner, reconcile in-memory buffered writes for the target wallet:
call the component that persists or discards pending buffer entries (e.g.,
flush/commit_writes or discard_buffered_writes) for wallet_id before the initial
conn() existence check and before run_auto_backup; propagate any error from that
operation so the delete aborts on flush failures. Locate delete_wallet_inner and
insert the flush/discard call at the top (prior to the SELECT 1 and
run_auto_backup usages) and ensure the rest of the logic (PER_WALLET_TABLES
counting, schema::wallet_meta::delete, tx.commit) operates on the now-consistent
persisted state.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:
- Around line 26-28: The decode function currently ignores the bytes-consumed
result from bincode::serde::decode_from_slice, allowing trailing garbage to pass
as valid; update the blob::decode implementation (the decode function that calls
bincode::serde::decode_from_slice) to check the second tuple element
(bytes_consumed) against blob.len() and return a WalletStorageError (e.g.,
Err(WalletStorageError::CorruptedBlob) or similar existing error variant) when
bytes_consumed != blob.len(), otherwise return the decoded value; ensure the
error path uses the same error type returned on bincode failures to keep the
function signature consistent.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 108-136: The upsert_utxo function currently writes account_index
as 0 into core_utxos causing list_unspent_utxos to misassign UTXOs; fix by
populating account_index before inserting: either (a) perform a lookup/join
against core_derived_addresses inside upsert_utxo (match wallet_id +
script/address) to derive the correct account_index and use that value instead
of 0, or (b) modify the Utxo/CoreChangeSet pipeline so the writer that calls
upsert_utxo receives and passes the owning account_index, or (c) add a
deterministic backfill writer that runs after writes but before any
list_unspent_utxos calls to update core_utxos.account_index from
core_derived_addresses; update upsert_utxo (and any callers of
upsert_utxo/CoreChangeSet) to ensure account_index is never left as the
hardcoded 0.
- Around line 138-174: The read in upsert_sync_state currently uses lp.map(|x| x
as u32) / sy.map(|x| x as u32) which silently truncates out-of-range i64 values;
change the rusqlite row mapping to return Option<i64> for both heights (keep the
closure in query_row returning (Option<i64>, Option<i64>)), then after the
query_row.optional()? unwrap_or((None,None)) convert each Option<i64> to
Option<u32> using a checked helper (reuse i64_to_u64 + u32::try_from or add
i64_to_u32) and map conversion errors to WalletStorageError::IntegerOverflow so
the function returns an error rather than silently wrapping before using lp and
sy in the INSERT/ON CONFLICT params.
- Around line 27-45: The code iterates cs.spent_utxos and calls upsert_utxo(tx,
wallet_id, utxo, true) when the outpoint is missing, which will materialize a
synthetic UTXO with account_index = 0; document this intent at the call site or
add an explicit guard flag to make the exceptional behavior obvious. Update the
block in the loop that handles cs.spent_utxos to either (a) add a clear comment
above the else branch referencing upsert_utxo and why creating a spent-only
placeholder with account_index = 0 is safe and will be corrected later, or (b)
wrap the else branch in a condition/flag (e.g., only_insert_spent_placeholders)
and pass that flag through to upsert_utxo so callers must opt into creating
synthetic rows; reference cs.spent_utxos, upsert_utxo, and the
core_utxos/account_index = 0 behavior in the comment or the new guard.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 42-64: fetch currently only selects entry_blob and discards the
tombstoned column referenced in the doc; change fetch to SELECT entry_blob,
tombstoned from identities and return the tombstone flag alongside the decoded
IdentityEntry (e.g. update the signature to Result<Option<(IdentityEntry,
bool)>, WalletStorageError> or a small wrapper type), decode payload with
blob::decode for the entry, map the tombstoned SQL value to a bool, and update
the doc comment to reflect the new return shape (ensure you still use
rusqlite::OptionalExtension and propagate errors as before).
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 72-89: The upsert loop writes typed key columns from the loop key
((identity_id, key_id), wallet_id) but serializes the payload from entry (via
IdentityKeyWire::from_entry), which can produce mismatches; ensure the
serialized wire and the SQL key are consistent by either normalizing the wire
values to the loop key or rejecting mismatches before persistence: construct or
overwrite IdentityKeyWire fields using the loop's wallet_id/identity_id/key_id
(from cs.upserts key and wallet_id variable) prior to blob::encode, or validate
that entry.identity_id, entry.key_id and entry.wallet_id exactly match the loop
key and return an error if they differ, then proceed to tx.execute.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- Around line 19-37: The loop over cs.addresses currently ignores each entry's
own wallet_id and always writes using the outer wallet_id; add a fast-fail check
at the top of the loop that compares entry.wallet_id to the outer wallet_id (the
variables named entry and wallet_id in the cs.addresses loop) and return an
error if they differ (include a descriptive message mentioning the mismatched
wallet ids and the PlatformAddressBalanceEntry), before executing the INSERT;
this prevents silently writing mixed-wallet entries.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs`:
- Around line 45-51: The row mapper in the wallet_metadata reader must reject
malformed rows instead of coercing them: in the stmt.query_map closure that
reads wallet_id (currently into bytes and wid) and birth_height (converted with
as u32), validate that wallet_id.bytes.len() == 32 and that birth_height is
within u32 bounds before converting; if either check fails return a real error
(e.g., a StorageError::CorruptedRow or map a descriptive
rusqlite::Error::InvalidParameterName/Other) from the closure so the query fails
instead of producing a zeroed wallet id or truncated height. Locate the closure
that does row.get(0) / copy_from_slice and the code that casts birth_height to
u32 and replace the coercion with explicit checks and an early Err(...) return
with a typed, descriptive error.
In `@packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs`:
- Around line 87-106: The test's assertion that delete_wallet returns
AutoBackupDirUnwritable is flaky when run as root because chmod 0o500 doesn't
block root; modify the test in sqlite_auto_backup.rs (the block that creates
unwritable dir, calls SqlitePersisterConfig::with_auto_backup_dir,
SqlitePersister::open, and persister.delete_wallet) to detect running as root
(e.g., check geteuid()==0 on Unix) and in that case either skip the
exact-variant assertion or assert only that an error occurred, otherwise keep
the existing matches!(... AutoBackupDirUnwritable { .. }) check; this ensures
deterministic behavior without changing SqlitePersister/delete_wallet logic.
---
Nitpick comments:
In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- Around line 185-191: The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.
- Around line 281-326: The closure parent_check currently accepts an unused
parameter _cols: &[&str]; either remove that parameter from parent_check and all
its callers (keep it as parent_check(child: &str) and continue using wallet_id
inside the generated trigger SQL), or update parent_check to accept cols:
&[&str] and use cols[0] (or join cols) in place of the hard-coded wallet_id
everywhere (including the WHEN clauses and DELETE WHERE clause) so the same
helper can generate triggers for identities, identity_keys, dashpay_profiles,
etc.; ensure you update the for loop callers accordingly to pass either no cols
(if removed) or the appropriate slice like &["wallet_id"] or &["identity_id"]
where needed.
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 188-231: The migrate handling is split across two places; fold the
two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.
- Around line 122-134: In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.
In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs`:
- Around line 111-118: The function default_auto_backup_dir contains an overly
defensive check when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Around line 102-107: The IntegerOverflow error currently reports
SafeCastTarget::U64 for an i64→u32 conversion in asset_locks.rs (see the
u32::try_from usage producing WalletStorageError::IntegerOverflow), which is
misleading; add a U32 variant to crate::sqlite::util::safe_cast::SafeCastTarget
and implement a dedicated i64_to_u32 helper in the safe_cast module that
performs the conversion and returns WalletStorageError::IntegerOverflow with
target = SafeCastTarget::U32 on failure, then replace the manual u32::try_from
usages (e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 72-102: ensure_exists currently performs an immediate INSERT OR
IGNORE on a plain &Connection, which bypasses the in-memory merge buffer and
per-flush transaction used by apply and can cause durability/ordering
mismatches; update ensure_exists to participate in the same flush boundary by
either accepting and using the existing merge buffer/flush transaction or a
transactional handle (e.g., a &Transaction or buffer API used by apply) and
executing the INSERT within that transaction, or if intentionally intended only
for tests, add a clear doc comment on ensure_exists describing that it writes
immediately to the DB and must not be used by production code that relies on the
merge-buffer + per-flush transaction model (reference ensure_exists, apply, and
the in-memory merge buffer/flush transaction behavior in your comment).
In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs`:
- Around line 20-26: The SafeCastTarget enum currently only lists I64 and U64
but callers need a U32 target; add a new variant SafeCastTarget::U32 and
implement a small helper function i64_to_u32 that mirrors existing
i64_to_u64/i64_to_i64 behavior (validate range, return Result<u32,
SafeCastError> and report SafeCastTarget::U32 on failure). Update any match/log
paths in this module that construct errors to use the new U32 variant so callers
like asset_locks::list_active and core_state::list_unspent_utxos can call the
helper and produce the correct error shape/message.
In `@packages/rs-platform-wallet-storage/tests/common/mod.rs`:
- Around line 47-56: The helper ensure_wallet_meta currently hardcodes network =
'testnet', which can collide with tests expecting other networks; change the
signature of ensure_wallet_meta(persister: &SqlitePersister, wallet_id:
&WalletId) to accept a network parameter (e.g., network: &str) with callers
passing "testnet" where appropriate, update the INSERT SQL call in
ensure_wallet_meta to bind the network parameter instead of the literal
'testnet', and update all test call sites (or add a convenience wrapper) to pass
the correct network values (e.g., "mainnet" or "testnet") so tests see
consistent wallet_metadata rows.
In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs`:
- Around line 273-278: Remove the redundant dead-code shim `_unused_btreemap`
and its explanatory comment: delete the `#[allow(dead_code)] fn
_unused_btreemap() -> BTreeMap<u32, u32> { BTreeMap::new() }` block since
`BTreeMap` is now actually used in the test
`tc023_one_flush_is_one_transaction`, making the shim unnecessary; ensure
imports remain and run tests to confirm nothing else depends on that helper.
- Around line 154-180: Test currently calls fresh_persister() inside the
proptest closure causing a new on-disk SQLite DB per case; move persister
creation out of the proptest! Create the persister once before proptest (call
fresh_persister() once to get persister/_tmp/_path), then inside the proptest
closure use a new wallet id (wid) per case or clear/reset the core_sync_state
row (use ensure_wallet_meta and persister.store with core_with_height) so each
case reuses the same DB connection and only varies the wallet or row content;
update references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 78dea89d-cb94-44d0-8590-02ed68920ad8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (55)
.github/workflows/pr.yml.github/workflows/tests-rs-workspace.ymlCargo.tomlDockerfilepackages/rs-platform-wallet-storage/Cargo.tomlpackages/rs-platform-wallet-storage/README.mdpackages/rs-platform-wallet-storage/SECRETS.mdpackages/rs-platform-wallet-storage/build.rspackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/migrations/V002__defensive_update_triggers.rspackages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rspackages/rs-platform-wallet-storage/src/lib.rspackages/rs-platform-wallet-storage/src/sqlite/backup.rspackages/rs-platform-wallet-storage/src/sqlite/buffer.rspackages/rs-platform-wallet-storage/src/sqlite/config.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/migrations.rspackages/rs-platform-wallet-storage/src/sqlite/mod.rspackages/rs-platform-wallet-storage/src/sqlite/persister.rspackages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rspackages/rs-platform-wallet-storage/src/sqlite/schema/blob.rspackages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rspackages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rspackages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identities.rspackages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rspackages/rs-platform-wallet-storage/src/sqlite/schema/mod.rspackages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rspackages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rspackages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rspackages/rs-platform-wallet-storage/src/sqlite/util/mod.rspackages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rspackages/rs-platform-wallet-storage/tests/common/mod.rspackages/rs-platform-wallet-storage/tests/secrets_scan.rspackages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rspackages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rspackages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rspackages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet/Cargo.tomlpackages/rs-platform-wallet/src/changeset/changeset.rspackages/rs-platform-wallet/src/changeset/mod.rspackages/rs-platform-wallet/src/changeset/serde_adapters.rspackages/rs-platform-wallet/src/wallet/asset_lock/tracked.rspackages/rs-platform-wallet/src/wallet/identity/types/block_time.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rspackages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rspackages/rs-platform-wallet/src/wallet/identity/types/key_storage.rs
Comment on lines
+235
to
+244
| if let Cmd::Migrate(_) = &cli.cmd { | ||
| let pre_version = peek_schema_version(&db); | ||
| let _persister = SqlitePersister::open(config.clone()).map_err(map_open_err_for_cli)?; | ||
| let post_version = peek_schema_version(&db); | ||
| let applied = post_version | ||
| .unwrap_or(0) | ||
| .saturating_sub(pre_version.unwrap_or(0)) as usize; | ||
| println!("applied: {applied}"); | ||
| return Ok(ExitCode::SUCCESS); | ||
| } |
Contributor
There was a problem hiding this comment.
applied: N is unreliable when peek_schema_version swallows errors.
peek_schema_version returns Option<i64> and maps every failure (file not present, table not yet created, lock contention, etc.) to None, which is then coerced to 0 via unwrap_or(0). On a fresh DB the pre-migration query fails (refinery_schema_history doesn't exist yet) and returns None → 0; if the post-migration query also fails for any transient reason it likewise yields 0, so the CLI happily prints applied: 0 even after a successful migration. Consider returning Result<Option<i64>, _> from peek_schema_version post-open (the table is guaranteed to exist by then) and error out if the post-version probe fails, so the reported count is trustworthy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`
around lines 235 - 244, The current migrate branch uses peek_schema_version
which swallows all errors into None causing applied to be unreliable; change
peek_schema_version to return Result<Option<i64>, E> (i.e., Result<Option<i64>,
Box<dyn Error> or a concrete error type) and update the call site in the
Cmd::Migrate block: keep using the old behavior for pre_version (call
peek_schema_version before opening the DB if you must tolerate missing table),
but after SqlitePersister::open(config.clone()) call the new peek_schema_version
and propagate or error out on Err so the post_version probe cannot be silently
treated as 0; compute applied only when post_version is Ok(Some/_), and map
errors to the CLI error flow (use map_open_err_for_cli or similar) instead of
unwrap_or(0) so the printed "applied: N" is trustworthy.
Comment on lines
+288
to
+300
| fn run_backup(persister: &SqlitePersister, args: BackupArgs) -> Result<ExitCode, CliError> { | ||
| if args.out.is_file() { | ||
| return Err(CliError::runtime(format!( | ||
| "backup destination exists and refuses to overwrite: {}", | ||
| args.out.display() | ||
| ))); | ||
| } | ||
| let path = persister | ||
| .backup_to(&args.out) | ||
| .map_err(|e| CliError::runtime(e.to_string()))?; | ||
| println!("{}", path.display()); | ||
| Ok(ExitCode::SUCCESS) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether backup_to itself rejects existing destinations atomically.
ast-grep --pattern $'pub fn backup_to($$$) $$$'
rg -nP '\bbackup_to\s*\(' --type=rust -C3Repository: dashpay/platform
Length of output: 7883
🏁 Script executed:
#!/bin/bash
# Find backup::run_to implementation
rg -nP 'fn run_to' --type=rust -A15 packages/rs-platform-wallet-storage/src/sqlite/backup.rsRepository: dashpay/platform
Length of output: 824
CLI check args.out.is_file() is redundant; the real TOCTOU issue lies in backup_to() itself.
The backup_to() function checks exists() before calling run_to(), which then calls Connection::open(dest). Since Connection::open() does not use atomic file creation (O_EXCL equivalent), a race exists between the exists() check and the actual file creation. Another process could create args.out between these calls, leading to unexpected truncation or overwrite.
The CLI pre-check is_file() doesn't mitigate this—it only adds a redundant guard that backup_to()'s broader exists() check already provides. If tightening the guarantee is desired, either remove the CLI check entirely or harden backup::run_to() to use atomic file creation (e.g., via OpenOptions::create_new()).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`
around lines 288 - 300, The CLI-level pre-check args.out.is_file() in run_backup
is redundant and does not prevent the TOCTOU race because backup_to() itself
checks exists() and ultimately calls run_to() -> Connection::open(dest) which
can be raced; either remove the args.out.is_file() guard from run_backup, or
(preferred) harden the persistence path by modifying backup_to()/run_to() to
perform atomic file creation (e.g., use OpenOptions::create_new() or equivalent)
when opening the destination instead of relying on exists()/Connection::open,
ensuring the creation fails if the file was created concurrently.
Comment on lines
+63
to
+103
| let src = Connection::open_with_flags( | ||
| src_backup, | ||
| rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY | rusqlite::OpenFlags::SQLITE_OPEN_URI, | ||
| ) | ||
| .map_err(|source| WalletStorageError::SourceOpenFailed { source })?; | ||
| run_integrity_check(&src, |report| WalletStorageError::IntegrityCheckFailed { | ||
| report, | ||
| })?; | ||
| let has_schema = src | ||
| .query_row( | ||
| "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'refinery_schema_history'", | ||
| [], | ||
| |_| Ok(()), | ||
| ) | ||
| .optional()? | ||
| .is_some(); | ||
| if !has_schema { | ||
| return Err(WalletStorageError::SchemaHistoryMissing); | ||
| } | ||
| let max_supported = crate::sqlite::migrations::embedded_migrations() | ||
| .iter() | ||
| .map(|(v, _)| *v as i64) | ||
| .max() | ||
| .unwrap_or(0); | ||
| let source_version: Option<i64> = src | ||
| .query_row( | ||
| "SELECT MAX(version) FROM refinery_schema_history", | ||
| [], | ||
| |row| row.get(0), | ||
| ) | ||
| .optional()? | ||
| .flatten(); | ||
| if let Some(v) = source_version { | ||
| if v > max_supported { | ||
| return Err(WalletStorageError::SchemaVersionUnsupported { | ||
| found: v, | ||
| max_supported, | ||
| }); | ||
| } | ||
| } | ||
| drop(src); |
Contributor
There was a problem hiding this comment.
Validate and restore the same source file.
restore_from checks one src_backup handle, drops it, then reopens the path for the actual copy. That leaves a TOCTOU window where the source can be swapped after integrity/schema validation, and the staged recheck would still accept a different-but-valid database. Copy from the already-validated source connection/handle, or otherwise bind the later copy to the same file identity you validated first.
Also applies to: 145-147
Comment on lines
+269
to
+326
| fn delete_wallet_inner( | ||
| &self, | ||
| wallet_id: WalletId, | ||
| skip_backup: bool, | ||
| ) -> Result<DeleteWalletReport, WalletStorageError> { | ||
| // Existence check FIRST — refusing on an unknown wallet must | ||
| // not waste a backup file. `.optional()?` propagates real SQL | ||
| // errors (busy / corrupt) instead of swallowing them. | ||
| { | ||
| let conn = self.conn()?; | ||
| let exists = conn | ||
| .query_row( | ||
| "SELECT 1 FROM wallet_metadata WHERE wallet_id = ?1", | ||
| rusqlite::params![wallet_id.as_slice()], | ||
| |_| Ok(()), | ||
| ) | ||
| .optional()? | ||
| .is_some(); | ||
| if !exists { | ||
| return Err(WalletStorageError::WalletNotFound { wallet_id }); | ||
| } | ||
| } | ||
| let backup_path = if skip_backup { | ||
| None | ||
| } else { | ||
| let conn = self.conn()?; | ||
| run_auto_backup( | ||
| &conn, | ||
| self.config.auto_backup_dir.as_deref(), | ||
| BackupKind::PreDelete { wallet_id }, | ||
| AutoBackupOperation::DeleteWallet, | ||
| )? | ||
| }; | ||
| let mut conn = self.conn()?; | ||
| let tx = conn.transaction()?; | ||
| let mut rows_removed_per_table = BTreeMap::new(); | ||
| for &table in PER_WALLET_TABLES { | ||
| // SQL injection note: `table` comes from a `&'static | ||
| // &'static str` constant compiled into the binary. There | ||
| // is no user input on this path. | ||
| let n: i64 = tx | ||
| .query_row( | ||
| &format!("SELECT COUNT(*) FROM {table} WHERE wallet_id = ?1"), | ||
| rusqlite::params![wallet_id.as_slice()], | ||
| |row| row.get(0), | ||
| ) | ||
| .optional()? | ||
| .unwrap_or(0); | ||
| rows_removed_per_table.insert(table, usize::try_from(n).unwrap_or(usize::MAX)); | ||
| } | ||
| crate::sqlite::schema::wallet_meta::delete(&tx, &wallet_id)?; | ||
| tx.commit()?; | ||
| Ok(DeleteWalletReport { | ||
| wallet_id, | ||
| backup_path, | ||
| rows_removed_per_table, | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Reconcile buffered writes before deleting a wallet.
This path only checks and deletes persisted rows. Any pending buffered changes for wallet_id survive the delete, so the pre-delete backup misses them, WalletNotFound can be returned for a newly buffered wallet, and a later flush/commit_writes can recreate rows after a successful delete. The buffer needs to be flushed or explicitly discarded before the existence check/backup/delete sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 269
- 326, Before checking existence/backing up/deleting in delete_wallet_inner,
reconcile in-memory buffered writes for the target wallet: call the component
that persists or discards pending buffer entries (e.g., flush/commit_writes or
discard_buffered_writes) for wallet_id before the initial conn() existence check
and before run_auto_backup; propagate any error from that operation so the
delete aborts on flush failures. Locate delete_wallet_inner and insert the
flush/discard call at the top (prior to the SELECT 1 and run_auto_backup usages)
and ensure the rest of the logic (PER_WALLET_TABLES counting,
schema::wallet_meta::delete, tx.commit) operates on the now-consistent persisted
state.
Comment on lines
+412
to
+468
| fn flush_inner(&self, wallet_id: &WalletId) -> Result<(), PersistenceError> { | ||
| let cs = self | ||
| .buffer | ||
| .drain(wallet_id) | ||
| .map_err(PersistenceError::from)?; | ||
| let Some(cs) = cs else { return Ok(()) }; | ||
| let mut conn = self.conn().map_err(PersistenceError::from)?; | ||
| let tx = conn | ||
| .transaction() | ||
| .map_err(WalletStorageError::from) | ||
| .map_err(PersistenceError::from)?; | ||
| if let Some(meta) = cs.wallet_metadata.as_ref() { | ||
| schema::wallet_meta::upsert(&tx, wallet_id, meta).map_err(PersistenceError::from)?; | ||
| } | ||
| if !cs.account_registrations.is_empty() { | ||
| schema::accounts::apply_registrations(&tx, wallet_id, &cs.account_registrations) | ||
| .map_err(PersistenceError::from)?; | ||
| } | ||
| if !cs.account_address_pools.is_empty() { | ||
| schema::accounts::apply_pools(&tx, wallet_id, &cs.account_address_pools) | ||
| .map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(core) = cs.core.as_ref() { | ||
| schema::core_state::apply(&tx, wallet_id, core).map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(identities) = cs.identities.as_ref() { | ||
| schema::identities::apply(&tx, wallet_id, identities) | ||
| .map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(keys) = cs.identity_keys.as_ref() { | ||
| schema::identity_keys::apply(&tx, wallet_id, keys).map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(contacts) = cs.contacts.as_ref() { | ||
| schema::contacts::apply(&tx, wallet_id, contacts).map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(addrs) = cs.platform_addresses.as_ref() { | ||
| schema::platform_addrs::apply(&tx, wallet_id, addrs).map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(locks) = cs.asset_locks.as_ref() { | ||
| schema::asset_locks::apply(&tx, wallet_id, locks).map_err(PersistenceError::from)?; | ||
| } | ||
| if let Some(balances) = cs.token_balances.as_ref() { | ||
| schema::token_balances::apply(&tx, wallet_id, balances) | ||
| .map_err(PersistenceError::from)?; | ||
| } | ||
| if cs.dashpay_profiles.is_some() || cs.dashpay_payments_overlay.is_some() { | ||
| schema::dashpay::apply( | ||
| &tx, | ||
| wallet_id, | ||
| cs.dashpay_profiles.as_ref(), | ||
| cs.dashpay_payments_overlay.as_ref(), | ||
| ) | ||
| .map_err(PersistenceError::from)?; | ||
| } | ||
| tx.commit() | ||
| .map_err(WalletStorageError::from) | ||
| .map_err(PersistenceError::from)?; |
Contributor
There was a problem hiding this comment.
Do not drop the buffered changeset before commit succeeds.
flush_inner drains the wallet buffer before opening the transaction. If any later step fails, the caller gets an error but the changeset is already gone, so the write cannot be retried and data is lost. Please keep the changes buffered until tx.commit() succeeds, or requeue the drained changeset on every failure path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines 412
- 468, flush_inner currently calls self.buffer.drain(wallet_id) and discards the
changeset (cs) before opening the DB transaction, so any failure after draining
(e.g., during schema::...::apply or tx.commit()) loses the buffered changes;
change the logic so the buffer is only removed after tx.commit() succeeds:
either fetch/clone/peek the changeset (use a non-destructive read API instead of
buffer.drain) into cs, execute the schema apply calls and tx.commit(), and only
then call buffer.drain(wallet_id) or buffer.remove(wallet_id) to clear it; if
you must drain early, ensure every error path re-inserts/requeues the drained cs
back into the buffer (e.g., buffer.insert(wallet_id, cs)) before returning the
PersistenceError, referencing flush_inner, self.buffer.drain(wallet_id), cs, and
tx.commit() to locate the edits.
Comment on lines
+42
to
+64
| /// Decode a single `identities` row back to its [`IdentityEntry`]. | ||
| /// | ||
| /// Returns `Ok(None)` if no row matches. Tombstoned rows decode to | ||
| /// `Some(entry)`; the caller inspects the dedicated `tombstoned` | ||
| /// column to discriminate when needed. | ||
| pub fn fetch( | ||
| conn: &Connection, | ||
| wallet_id: &WalletId, | ||
| identity_id: &[u8; 32], | ||
| ) -> Result<Option<IdentityEntry>, WalletStorageError> { | ||
| use rusqlite::OptionalExtension; | ||
| let row: Option<Vec<u8>> = conn | ||
| .query_row( | ||
| "SELECT entry_blob FROM identities WHERE wallet_id = ?1 AND identity_id = ?2", | ||
| params![wallet_id.as_slice(), &identity_id[..]], | ||
| |row| row.get(0), | ||
| ) | ||
| .optional()?; | ||
| match row { | ||
| None => Ok(None), | ||
| Some(payload) => Ok(Some(blob::decode(&payload)?)), | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
fetch can't actually expose the tombstoned column the doc tells callers to inspect.
The doc comment says callers should "inspect the dedicated tombstoned column to discriminate when needed", but fetch only SELECTs entry_blob and returns Option<IdentityEntry> — the tombstone bit is silently dropped. A caller using fetch to decide whether an identity is live will treat tombstoned rows as present-and-active. Either return (IdentityEntry, bool) / a small struct that carries the tombstone flag, filter WHERE tombstoned = 0 if that's the intended semantics, or update the doc to point at a different accessor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs` around
lines 42 - 64, fetch currently only selects entry_blob and discards the
tombstoned column referenced in the doc; change fetch to SELECT entry_blob,
tombstoned from identities and return the tombstone flag alongside the decoded
IdentityEntry (e.g. update the signature to Result<Option<(IdentityEntry,
bool)>, WalletStorageError> or a small wrapper type), decode payload with
blob::decode for the entry, map the tombstoned SQL value to a bool, and update
the doc comment to reflect the new return shape (ensure you still use
rusqlite::OptionalExtension and propagate errors as before).
Comment on lines
+72
to
+89
| for ((identity_id, key_id), entry) in &cs.upserts { | ||
| let wire = IdentityKeyWire::from_entry(entry)?; | ||
| let entry_blob = blob::encode(&wire)?; | ||
| tx.execute( | ||
| "INSERT INTO identity_keys \ | ||
| (wallet_id, identity_id, key_id, public_key_blob, public_key_hash, derivation_blob) \ | ||
| VALUES (?1, ?2, ?3, ?4, ?5, NULL) \ | ||
| ON CONFLICT(wallet_id, identity_id, key_id) DO UPDATE SET \ | ||
| public_key_blob = excluded.public_key_blob, \ | ||
| public_key_hash = excluded.public_key_hash, \ | ||
| derivation_blob = NULL", | ||
| params![ | ||
| wallet_id.as_slice(), | ||
| identity_id.as_slice(), | ||
| i64::from(*key_id), | ||
| entry_blob, | ||
| &entry.public_key_hash[..], | ||
| ], |
Contributor
There was a problem hiding this comment.
Validate the upsert key against the serialized payload.
Line 73 serializes entry.identity_id, entry.key_id, and entry.wallet_id, but Lines 83-89 write the row key from ((identity_id, key_id), wallet_id). A malformed changeset can therefore persist one key in the typed columns and a different one inside public_key_blob, which leaves the row internally inconsistent and breaks blob-based round-trips. Please normalize the wire values from the tuple/argument or reject mismatches before encoding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`
around lines 72 - 89, The upsert loop writes typed key columns from the loop key
((identity_id, key_id), wallet_id) but serializes the payload from entry (via
IdentityKeyWire::from_entry), which can produce mismatches; ensure the
serialized wire and the SQL key are consistent by either normalizing the wire
values to the loop key or rejecting mismatches before persistence: construct or
overwrite IdentityKeyWire fields using the loop's wallet_id/identity_id/key_id
(from cs.upserts key and wallet_id variable) prior to blob::encode, or validate
that entry.identity_id, entry.key_id and entry.wallet_id exactly match the loop
key and return an error if they differ, then proceed to tx.execute.
Comment on lines
+19
to
+37
| for entry in &cs.addresses { | ||
| tx.execute( | ||
| "INSERT INTO platform_addresses \ | ||
| (wallet_id, account_index, address_index, address, balance, nonce) \ | ||
| VALUES (?1, ?2, ?3, ?4, ?5, ?6) \ | ||
| ON CONFLICT(wallet_id, address) DO UPDATE SET \ | ||
| account_index = excluded.account_index, \ | ||
| address_index = excluded.address_index, \ | ||
| balance = excluded.balance, \ | ||
| nonce = excluded.nonce", | ||
| params![ | ||
| wallet_id.as_slice(), | ||
| i64::from(entry.account_index), | ||
| i64::from(entry.address_index), | ||
| entry.address.as_bytes(), | ||
| safe_cast::u64_to_i64("platform_addresses.balance", entry.funds.balance)?, | ||
| i64::from(entry.funds.nonce), | ||
| ], | ||
| )?; |
Contributor
There was a problem hiding this comment.
Reject mixed-wallet platform address entries.
Each PlatformAddressBalanceEntry carries its own wallet_id, but this write path ignores it and always persists under the outer wallet_id. If a mismatched changeset ever slips through, the rows are silently written into the wrong wallet. Please fail fast here instead of trusting the caller.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`
around lines 19 - 37, The loop over cs.addresses currently ignores each entry's
own wallet_id and always writes using the outer wallet_id; add a fast-fail check
at the top of the loop that compares entry.wallet_id to the outer wallet_id (the
variables named entry and wallet_id in the cs.addresses loop) and return an
error if they differ (include a descriptive message mentioning the mismatched
wallet ids and the PlatformAddressBalanceEntry), before executing the INSERT;
this prevents silently writing mixed-wallet entries.
Comment on lines
+45
to
+51
| let rows = stmt.query_map([], |row| { | ||
| let bytes: Vec<u8> = row.get(0)?; | ||
| let mut wid = [0u8; 32]; | ||
| if bytes.len() == 32 { | ||
| wid.copy_from_slice(&bytes); | ||
| } | ||
| Ok(wid) |
Contributor
There was a problem hiding this comment.
Reject malformed wallet_metadata rows instead of coercing them.
These reads currently turn bad persisted data into plausible values: a non-32-byte wallet_id becomes [0; 32], and birth_height as u32 will wrap/truncate invalid integers. That can make a corrupted backup load as the wrong wallet or the wrong scan height. Please validate both fields and return a typed error on bad rows.
Also applies to: 68-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs` around
lines 45 - 51, The row mapper in the wallet_metadata reader must reject
malformed rows instead of coercing them: in the stmt.query_map closure that
reads wallet_id (currently into bytes and wid) and birth_height (converted with
as u32), validate that wallet_id.bytes.len() == 32 and that birth_height is
within u32 bounds before converting; if either check fails return a real error
(e.g., a StorageError::CorruptedRow or map a descriptive
rusqlite::Error::InvalidParameterName/Other) from the closure so the query fails
instead of producing a zeroed wallet id or truncated height. Locate the closure
that does row.get(0) / copy_from_slice and the code that casts birth_height to
u32 and replace the coercion with explicit checks and an early Err(...) return
with a typed, descriptive error.
Comment on lines
+87
to
+106
| std::fs::create_dir(&unwritable).unwrap(); | ||
| // chmod 0500 (r-x------) — we cannot write to it. | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| let mut perms = std::fs::metadata(&unwritable).unwrap().permissions(); | ||
| perms.set_mode(0o500); | ||
| std::fs::set_permissions(&unwritable, perms).unwrap(); | ||
| } | ||
| let cfg = SqlitePersisterConfig::new(&path).with_auto_backup_dir(Some(unwritable.clone())); | ||
| let persister = SqlitePersister::open(cfg).unwrap(); | ||
| let w = wid(0xE2); | ||
| ensure_wallet_meta(&persister, &w); | ||
| let err = persister.delete_wallet(w); | ||
| #[cfg(unix)] | ||
| { | ||
| assert!( | ||
| matches!(err, Err(WalletStorageError::AutoBackupDirUnwritable { .. })), | ||
| "expected AutoBackupDirUnwritable, got {err:?}" | ||
| ); |
Contributor
There was a problem hiding this comment.
This unwritable-dir assertion is brittle under root.
On Unix, chmod 0500 in Lines 87-94 still allows writes when the tests run as UID 0, which is common in Dockerized CI. That makes the AutoBackupDirUnwritable assertion nondeterministic even if the implementation is correct. Please skip the strict branch for root or force the error through a setup that cannot be bypassed by privileges.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs` around lines
87 - 106, The test's assertion that delete_wallet returns
AutoBackupDirUnwritable is flaky when run as root because chmod 0o500 doesn't
block root; modify the test in sqlite_auto_backup.rs (the block that creates
unwritable dir, calls SqlitePersisterConfig::with_auto_backup_dir,
SqlitePersister::open, and persister.delete_wallet) to detect running as root
(e.g., check geteuid()==0 on Unix) and in that case either skip the
exact-variant assertion or assert only that an error occurred, otherwise keep
the existing matches!(... AutoBackupDirUnwritable { .. }) check; this ensures
deterministic behavior without changing SqlitePersister/delete_wallet logic.
Routine forward-integration. Cargo.lock reconciliation only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment-tightening pass per claudius:coding-best-practices, scoped to PR #3625's own additions: - sqlite_buffer_semantics.rs: drop `_unused_btreemap` placeholder + its "future expansion" comment. `BTreeMap` is genuinely used elsewhere in the file (line 301 — `balances` map), so the import stays. Removes a speculative-future-state comment and an empty helper that exists only to silence a phantom lint. - sqlite_load_reconstruction.rs: fix stale cross-reference. Module doc said "tracked in a TODO in persister.rs::load", but the actual signal is the `LOAD_UNIMPLEMENTED` constant + tracing::warn. Replace with the accurate present-state pointer. Plus a single rustfmt fix in `packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs` that fell out of the v3.1-dev merge — the textual auto-merge produced a 3-arg call spread across 5 lines that rustfmt collapses to one line. Not a logic change. Rules driving the changes: - present-state, not history (sqlite_load_reconstruction.rs) - comment only when meaningful — dropping speculative placeholders (sqlite_buffer_semantics.rs) Quality gates: `cargo fmt --all` clean, `cargo check --workspace` green, `cargo clippy -p platform-wallet -p platform-wallet-storage --tests --no-deps -- -D warnings` green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
…reation SEC-011 (Smythe audit, MEDIUM): the restore path already applied `chmod 0o600` after writing the SQLite file (`backup.rs::restore_from`), but the initial-create path in `SqlitePersister::open` and the backup-create path in `backup::run_to` did not. Both relied on the process umask, which can leave a newly created DB world- or group-readable. Extracts the existing inline `#[cfg(unix)]` + `Permissions::from_mode(0o600)` block into a small helper `sqlite::util::permissions::apply_secure_permissions` (no-op on non-Unix) and calls it at all three sites. The restore path keeps its existing semantics — it just delegates to the helper now — so the file mode no longer depends on the process umask anywhere a SQLite file is created or replaced by this crate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
SEC-011 fix landed (
|
…let-sqlite-persistor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
platform-wallet'sPlatformWalletPersistencetrait (packages/rs-platform-wallet/src/changeset/traits.rs:118) shipped with onlyNoPlatformPersistence— a no-op stub. The canonical SQLite implementation lived downstream insidedash-evo-tooland was not reusable by FFI consumers, desktop apps, tests, or CLI tooling. This PR introduces a new workspace crate,platform-wallet-storage, that owns the canonical SQLite persister today and reserves a submodule slot for a futureSecretStore(OS keyring + encrypted file backends) so storage concerns for the wallet stack live under one roof.User story
As a wallet integrator (Rust embedder, FFI consumer, or desktop-app author), I want a ready-made storage crate for
platform-walletso that I can persist wallet state across restarts without writing my own schema, my own migration pipeline, or my own backup/restore tooling — and I want the persister to take an automatic backup before anything destructive so I cannot accidentally lose data on a bad migration or a wrong wallet ID. Later, I want the same crate to offer aSecretStoreabstraction so I do not have to roll my own OS-keyring integration on every platform.What was done?
Two crates touched
platform-wallet(packages/rs-platform-wallet/) — added an optionalserdeCargo feature that gates#[derive(Serialize, Deserialize)]on every changeset type. Feature is off by default; library behaviour unchanged when not activated. Cherry-pickable into its own upstream PR via commite26945cfdf.platform-wallet-storage(packages/rs-platform-wallet-storage/, new) — the actual storage crate. Module layout:Cargo features:
default = ["sqlite", "cli"].--no-default-featuresproduces a bare crate so futuresecrets-only consumers can opt in just to that.Highlights
SqlitePersisterimplementsPlatformWalletPersistence(sync,Send + Sync, object-safe behindArc<dyn …>).tests/secrets_scan.rs), contacts, platform addresses, asset locks, token balances, DashPay overlays, wallet metadata, account registrations, address pools. Single.dbholds many wallets.ON DELETE CASCADE. Composite-key parent-existence + cascade are trigger-emulated (barrel's sqlite3 backend cannot emit compositeFKclauses portably;PRAGMA foreign_keys = ONonly enforces FKs declared inCREATE TABLE).Mutex<HashMap<WalletId, PlatformWalletChangeSet>>merging via upstreamMergeimpls; oneflush()call = one SQLite transaction.FlushMode::{Manual, Immediate}switch +commit_writes()end-of-batch hook.rusqlite::backup::Backup::run_to_completion. Retention bykeep_last_n+max_ageAND-semantics. TOCTOU-safe restore viatempfile::NamedTempFile::new_in(parent_dir)+ atomicpersist.open, pre-delete indelete_wallet. Library returns typedAutoBackupDisabledifauto_backup_dir = None; CLI escape hatch is a separatedelete_wallet_skip_backupsibling method (does NOT mutate config — keeps library safe-by-default).platform-wallet-storagewithbackup,restore,prune,inspect,migrate,delete-walletsubcommands. Destructive ops require--yes.-v/-qwired totracing_subscriber. Unix stream conventions.bincode::serde::encode_to_vecover serde-derived changeset types No hand-rolled binary format. One carve-out: dpp'sIdentityPublicKeyuses#[serde(tag = "$formatVersion")]which bincode-serde rejects, soidentity_keys.rsuses a wire-shape struct that re-encodes that one field via dpp's native bincode 2 derives — one blob per row preserved.SECRETS.md. The reservedsrc/secrets/slot is the only directory exempt from thesecrets_scan.rsforbidden-substring grep.Cargo.tomlmembers,DockerfileCOPY --parentsblocks (3 stages),.github/workflows/tests-rs-workspace.yml(--packagelists in both shards),.github/workflows/pr.ymlallowed-scope (wallet-storage).Commits (strict file boundaries for clean cherry-pick)
0f9437cd44adf421257ccea9ddad4de26945cfdfpackages/rs-platform-wallet/— independently cherry-pickable8e0830626d74acc8152b5bac6e304d540decf6524cfec30375bd4216dbe2f58e78459387f38c0f15Workflow
Built end-to-end via the
claudius:workflow-featureskill: Diziet's Requirements + DX spec (26 FRs, 10 NFRs, 15-variant error catalogue) → Marvin's 83-case test specification → Bilby's implementation → parallel Phase-3 QA wave (Marvin / Smythe / Adams / Diziet / Trillian) → Bilby's fix wave (resolved 2 CRITICAL from Adams, 1 HIGH from Diziet, 2 HIGH from Marvin, 3 HIGH from Adams, 4 MEDIUM from Smythe / Trillian / Diziet) → reshape (crate rename + serde swap) → Lessons Learned (18 memories persisted).How Has This Been Tested?
--no-default-features) — clean.Coverage gained in the encoder swap (previously deferred per-sub-changeset round-trips, now green): TC-007 (identity keys), TC-009 (platform addresses), TC-010 (asset locks incl. embedded
Transaction), TC-012 (DashPay overlays), TC-014 (account registration with fullExtendedPubKey).Deferred (with rationale, follow-up PRs)
ClientStartState.walletsreconstruction inload()— blocked on a future upstreamWallet::from_persisted. All data IS persisted; only the wallet rehydration step is gated.TODOinpersister.rs::load.lock_conn_for_testtest-helper accessor.InstantLock,Transactiondirect round-trips) — kept onconsensus::Encodablefor those bytes (canonical Bitcoin-style wire format); a future PR may unify on serde if dashcore gains compatible derives.bincode 2.0.1advisory (RUSTSEC-2025-0141, unmaintained) — workspace concern, not crate-specific.key_wallet::Wallet'sWalletType::Mnemonicdoes not zeroize its seed fields on drop. Out of scope; flagged for upstream follow-up.Breaking Changes
None. New crate, opt-in. Existing
NoPlatformPersistenceremains in place and is unchanged. The newplatform-wallet/serdefeature is off by default.Checklist
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Tests
Chores