Uh oh!
There was an error while loading. Please reload this page.
feat(platform-wallet)!: [NO MERGE] consumer hardening — CODE-001/003-callsite/017/018 + PROJ-001 FFI + CODE-008/012/013 - #3750
Closed
Claudius-Maginificent wants to merge 110 commits into
Closed
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>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>
…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>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>
…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>
…let-sqlite-persistor
…e_cached writers, functional load() (#3643) Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3633) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: QuantumExplorer <quantum@dash.org>
…oto messages (#3654) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…c) (#3652) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # Cargo.lock
…let-sqlite-persistor
delete_wallet_inner never reconciled the in-memory buffer, so a buffered-only wallet returned WalletNotFound, the pre-delete backup excluded buffered writes, and a later commit_writes/flush resurrected the just-deleted wallet's rows. Per Nagatha ARCH-001: drain-and-discard the target's buffered changeset FIRST via the existing Buffer::take_for_flush (no new method, no deprecated drain alias, no restore on the delete path), then widen the existence gate to "buffered OR persisted". The drain is unconditional of FlushMode and runs before the skip_backup branch; locks stay strictly sequential (buffer lock released before conn lock). Regression tests (tests/sqlite_delete_buffer_reconcile.rs): - buffered_only_delete_is_ok_and_no_resurrection - pre_delete_backup_excludes_buffered_writes - delete_unknown_wallet_is_not_found - immediate_after_failed_flush_delete_drains_buffer Refs: PR #3625 thread r3221229558 Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…CMT-002) restore_from validated schema-history presence and the max-supported version on the first source handle, then dropped it and re-read the path for the staged copy. A swap during that window could persist an internally-valid but forward-version / schema-history-missing DB (validate-then-reopen TOCTOU). Per Nagatha ARCH-002: MOVE (not duplicate) both gates off `src` and into the existing step-5 staged block, after run_integrity_check on the staged copy and before the block closes, reusing the same `staged` connection (no third handle). All validation now binds to the exact bytes being persisted. The cheap pre-staging integrity check on `src` is retained (non-load-bearing, optional per ARCH-002 q6). Regression tests (tests/sqlite_restore_staged_validation.rs): - forward_version_rejected_destination_unchanged - missing_schema_history_rejected_destination_unchanged - valid_backup_roundtrips Refs: PR #3625 thread r3221229556 Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…MT-003) blob::decode discarded bincode's bytes-consumed value, so a valid prefix decoded successfully even when the BLOB column held trailing garbage — silently accepting corrupt or forward-incompatible payloads across every blob-backed column. Compare consumed against blob.len() and return the existing typed WalletStorageError::blob_decode on mismatch, mirroring the strict length check in the sibling decode_outpoint. Regression test (src/sqlite/schema/blob.rs): - decode_rejects_trailing_bytes Refs: PR #3625 thread r3221229573 Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ities upsert Document that the `wallet_id = COALESCE(excluded.wallet_id, identities.wallet_id)` upsert clause is the intended merge semantic for orphan-identity-to-wallet promotion (NULL `wallet_id` is allowed per the V002 CODE-002 design). Existing `wallet_id` survives a re-upsert; a freshly known `wallet_id` fills NULL on promotion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… orphan-cleanup contract Document that orphan `token_balances` cleanup is the host's responsibility — no automatic prune API is offered. V002 cascades only through `identities` (the `wallet_id` column was dropped from `token_balances`), so hosts that delete identities out-of-band must prune the corresponding rows themselves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (CODE-017 follow-up) Helper added in 6934404 was never called, tripping -D dead-code on macOS strict profile and blocking Swift SDK build on PR #3743. Wiring fulfills the original perf intent (drop N+1 persister.load() at shielded bind time). - Add PlatformWallet::bind_shielded_with_snapshot taking a pre-loaded Arc<ShieldedSyncStartState>; bind_shielded keeps its public signature by delegating with None. - Promote PlatformWalletManager::cached_persisted_shielded from pub(super) to pub so the FFI layer (the only direct host of bind_shielded) can fetch the shared snapshot once per call. - platform_wallet_manager_bind_shielded now pulls the cached snapshot alongside the coordinator and feeds it through, so every wallet's bind reuses the same Arc instead of issuing its own load(). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ar chmod test (APFS compat) tc_code_011_a previously used raw non-UTF-8 bytes (0xff 0xfe ...) which APFS rejects with EILSEQ. The test's real intent is to verify OsString sidecar concatenation + chmod survive non-ASCII paths — a valid-UTF-8 multi-byte sequence (ÿþ → \xc3\xbf\xc3\xbe) exercises the same codepath on both Linux ext4/btrfs and macOS APFS without losing coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on persistence trait Trait-level contract: passing WalletId::default() to PlatformWalletPersistence methods marks the entity as orphan (no parent wallet). V002 schema permits this; higher layers (FFI) may enforce stricter rules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ommit/delete reports Follow-up to d0ed23b: extend the WalletId::default() = orphan convention to PlatformWalletPersistence::flush parameter, commit_writes return doc, CommitReport field docs, and DeleteWalletReport.wallet_id field.
…let-sqlite-persistor # Conflicts: # packages/rs-platform-wallet/Cargo.toml
…epastaclaw-hardening # Conflicts: # packages/rs-platform-wallet/src/manager/wallet_lifecycle.rs
…3) — moved to follow-up PR Phase 2 of PR #3743 slim. Restores consumer-side state to origin/feat/platform-wallet-sqlite-persistor for these files. The trait surface (delete_wallet, commit_writes, typed PersistenceError) stays so the storage crate still compiles; only the consumer call-sites and caching/retry plumbing revert. - src/error.rs: drop PersistorMissingWalletRehydration + WalletRegistrationFailed variants - src/manager/load.rs: drop CODE-001 orphan-bucket gate and CODE-017 cache plumbing - src/manager/mod.rs: drop CODE-017 cache fields on PlatformWalletManager - src/manager/wallet_lifecycle.rs: drop CODE-018 retry, CODE-003 wired remove_wallet call site, cache-invalidation hook - src/wallet/platform_wallet.rs: drop CODE-017 bind_shielded_with_snapshot shim - FFI src/shielded_sync.rs: drop CODE-017 cached_persisted_shielded wiring Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ck sites (CODE-012/013)" This reverts commit 6602cdb.
…lush + commit/delete reports" This reverts commit 5747a98.
…vention on persistence trait" This reverts commit d0ed23b.
…8 consumer-integration test moved to follow-up PR) This test brings rs-platform-wallet as a dev-dep specifically to exercise the consumer flow against the storage backend, and it depends on the PROJ-001 wallet-aware register_identity_with_wallet helper. Now that the PROJ-001 consumer-side wiring has been reverted from this PR (kept for the follow-up consolidated PR), the test no longer compiles. Other storage-crate-internal round-trip coverage (sqlite_persist_roundtrip, sqlite_load_reconstruction, etc.) remains. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…/CODE-017/CODE-018) + PROJ-001 FFI - CODE-001: refuse silent drop of orphan platform_addresses on load - CODE-003 (call-site): wire remove_wallet to persister.delete_wallet - CODE-017: cache ClientStartState slices to drop register_wallet N+1 load() - CODE-018: retry transient + undo on fatal store error in register_wallet - PROJ-001: FFI register_identity requires wallet_id (Rust + Swift + SwiftExampleApp) - 5 consumer integration tests covering the above Originally landed on PR #3743 hardening branch; extracted here to keep #3743 focused on the rs-platform-wallet-storage crate landing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… (CODE-012/013) Originally landed on PR #3743; extracted here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ommit/delete reports Originally landed on PR #3743; extracted here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on persistence trait Originally landed on PR #3743; extracted here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tegration tests (CODE-008) Originally landed on PR #3743; extracted here. Lives under rs-platform-wallet-storage/tests/ but brings rs-platform-wallet as dev-dep to exercise the consumer flow against the storage backend — fundamentally a consumer-integration test, follows the consumer code. 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…let-sqlite-persistor
…istor' into fix/3625-thepastaclaw-hardening
… into feat/platform-wallet-consumer-hardening
Base automatically changed from
fix/3625-thepastaclaw-hardening to
feat/platform-wallet-sqlite-persistorMay 27, 2026 09:42
7 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
This PR contains various minor out-of-scope fixes, to be examined later.
Summary
Consumer-side hardening for the platform-wallet stack, extracted from PR #3743 to keep that PR focused on the
rs-platform-wallet-storagecrate landing + the minimal trait surface that crate needs.Stack chain
When #3625 merges, #3743 rebases onto v3.1-dev. When #3743 merges, this PR rebases onto #3625's merged tip. Standard chain — each layer cleanly isolates a concern.
What's in this PR
Consumer logic (rs-platform-wallet)
platform_addresseson load —PlatformWalletError::PersistorMissingWalletRehydrationraised when persister returns addresses but no wallets.remove_walletto callpersister.delete_wallet(...)(the trait method itself ships in feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface #3743).ClientStartStateslices inPlatformWalletManagerto dropregister_walletN+1load()calls; helperstake_persisted_platform_addresses,cached_persisted_shielded,invalidate_persisted_for_wallet.register_wallet—PlatformWalletError::WalletRegistrationFailed { wallet_id, reason }.FFI (rs-platform-wallet-ffi)
platform_wallet_manager_identity_sync_register_identitynow requireswallet_id_ptr: *const u8(32 bytes, rejects null + all-zero sentinel), routes to wallet-awareregister_identity_with_wallet. Breaking C ABI change.Swift SDK
PlatformWalletManagerIdentitySync.swiftgainswalletId: Dataparameter,TokenActions.swiftdoc-example updated,IdentityDetailView.swiftsourceswalletIdfromidentity.wallet?.walletId.Trait docs (rs-platform-wallet)
WalletId::default()= orphan convention onPlatformWalletPersistence::{store, flush, get_core_tx_record, delete_wallet, commit_writes}.CommitReport.{succeeded, failed, still_pending}andDeleteWalletReport.wallet_id.Tests
packages/rs-platform-wallet/tests/:load_from_persistor.rs,persistence_error_taxonomy.rs,persister_load_cache.rs,register_wallet_failure.rs,remove_wallet_delete.rs.packages/rs-platform-wallet-storage/tests/round_trip_consumer.rs(CODE-008) — lives under storage/tests/ but brings rs-platform-wallet as dev-dep.Breaking changes
platform_wallet_manager_identity_sync_register_identitygains requiredwallet_id_ptrparameter betweenidentity_id_ptrandtoken_ids_ptr. Swift wrappers updated. Out-of-wallet identities are now rejected at this entry point (orphan-aware registration is a future follow-up — see platform-wallet: orphan identities cannot be loaded — persistor writes them, but no consumer-side reader #3745).PlatformWalletErrorvariants (PersistorMissingWalletRehydration,WalletRegistrationFailed).Test plan
cargo fmt --all -- --checkcargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets -- -D warningscargo test -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi— 424 passed, 0 failed (incl. all 5round_trip_consumertests and the 5 new consumer integration test files)Related
feat/platform-wallet-sqlite-persistor) — root of the stack: persister base.fix/3625-thepastaclaw-hardening) — immediate base: rs-platform-wallet-storage crate + trait surface (typedPersistenceError,delete_wallet,commit_writes) this PR's consumer code depends on.🤖 Generated with Claude Code