feat: expand walletkit-db into shared storage-primitives crate#400
feat: expand walletkit-db into shared storage-primitives crate#400danielle-tfh wants to merge 32 commits into
Conversation
- README: rewrite walletkit-db blurb to generic-only; drop dangling sub-crate README pointer. - walletkit-db: re-export public types at crate root (Connection, Transaction, Statement, Row, StepResult, Value, cipher) and alias Error/Result back to DbError/DbResult for back-compat and to remove per-call-site aliasing in consumers. - vault: revert compute_content_id signature back to BlobKind (was weakened to u8 for no boundary reason); restore BlobKind::as_i64() and simplify the i64 cast call sites. - credential_storage: drop dead Ok(self.lock.lock()?) wrap. - vault: move BACKUP_TABLES into vault/schema.rs next to the schema it mirrors; revert blob_objects/credential_records ordering churn. - keys: relocate the 'Key structure' architecture doc block that was deleted from traits.rs in this refactor. - walletkit-db tests: add round-trip and non-empty-destination rejection coverage for cipher::export/import_plaintext_copy with a generic table. - docs: fix private intra-doc link to ffi module. Verified: cargo fmt, cargo clippy (all/default/no-default features), cargo test --workspace --lib (--features legacy-nullifiers,v3), cargo build --no-default-features, cargo deny (bans/licenses/sources), cargo doc --all-features (RUSTDOCFLAGS=-Dwarnings) all pass.
Add Vault opener with consumer-supplied schema, content-addressed Blobs (with frozen content_id derivation), CBOR KeyEnvelope + init_or_open_envelope_key, cross-process Lock (native flock / WASM no-op), and plain-Rust Keystore + AtomicBlobStore traits for consumers that need their own FFI surface to adapt onto.
Delete walletkit-core/src/storage/envelope.rs and lock.rs (whole-file moves into walletkit-db). Re-export StorageLock/StorageLockGuard from walletkit-db with the existing walletkit-core names so the public Rust API stays intact. Storage glue: - StorageKeys::init now wraps walletkit_db::init_or_open_envelope_key, with thin Keystore + AtomicBlobStore adapters that bridge the uniffi-annotated DeviceKeystore + AtomicBlobStore traits to the plain-Rust trait surface walletkit-db expects. - VaultDb is reshaped as a thin wrapper over walletkit_db::Vault::open with a credential schema callback. Blob writes go through walletkit_db::Blobs::put; orphan-blob cleanup queries stay schema-aware in walletkit-core. blob_objects DDL moves to Blobs::ensure_schema; vault/schema.rs keeps only vault_meta and credential_records. - ContentId is re-exported from walletkit-db (now a newtype). Internal call sites use cid.as_bytes() / AsRef<[u8]>; on-disk SHA-256 derivation is unchanged. - StorageError gains From<walletkit_db::StoreError> mapping each generic variant; FFI-exposed StorageError surface is unchanged. - New on-disk-format guard test in vault/tests.rs: stores a credential with deterministic inputs, asserts the credential_blob content_id matches a frozen SHA-256, closes and reopens the vault, fetches back, and asserts byte-equality with the original payload.
- vault: drop Vault struct; expose open_vault(...) -> StoreResult<Connection> as a free function. Consumers compose schema ops directly on the Connection.
- blobs: drop Blobs namespace struct, ContentId newtype, and as_bytes/into_bytes methods. Surface is now blobs::{ensure_schema, put, get} module functions plus pub type ContentId = [u8; 32]. blobs::put / blobs::get return StoreResult; blobs::put takes now: u64 (no SQL i64 leak).
- traits: Keystore/AtomicBlobStore shapes match walletkit-core's uniffi traits (Vec<u8> / String). walletkit-core bridges via small pass-through newtypes (Ks, Bs) — Rust's orphan rule blocks a blanket impl across crates.
- error: From<walletkit_db::StoreError> for StorageError collapses to Self::VaultDb(err.to_string()); the parallel-enum mirror is gone. keys.rs tests updated to match VaultDb.
- envelope: KeyEnvelope fields back to pub(crate) — public methods are the surface.
- lib: cipher canonical path is walletkit_db::cipher (sqlite module is now private inside the crate). params! macro updated to walk the public re-export.
- lock.rs: comment dividers restored — pure move from walletkit-core.
- vault/tests.rs: provenance comment on the frozen credential_blob content_id hex (verified to match main's compute_content_id; reproducible via shasum).
- credential_storage.rs: drop the Ok(?) wrap on guard().
- New tests: test_lock_serializes_across_threads ported from the pre-PR walletkit-core/src/storage/lock.rs (don't delete tests).
Net: ~−25 lines in walletkit-core and ~−25 lines on net in walletkit-db relative to the previous commit on this branch. Behavior and on-disk format unchanged: walletkit-core ships 123 lib tests, walletkit-db ships 18 lib tests, all green.
1. error.rs: restore 1-1 From<walletkit_db::StoreError> variant mapping. The previous Self::VaultDb(err.to_string()) collapse erased variant identity that hosts depend on for UX (Crypto vs InvalidEnvelope vs Keystore vs CorruptedVault, etc.). keys.rs tests restored to match specific variants.
2. blobs::get(conn, cid: &[u8]): lax input — accept any byte slice so callers reading content_id out of another table column (Vec<u8> from column_blob) don't need copy_from_slice into a [u8; 32].
3. tests.rs: new test_key_envelope_cbor_bytes_frozen asserting the canonical KeyEnvelope serializes to a hard-coded hex string. Round-trip alone misses field-order or type drift; this catches it.
4. lib.rs: drop KeyEnvelope from public re-exports — fields are pub(crate), so external consumers couldn't read them anyway; only init_or_open_envelope_key is the surface.
5. blobs::delete(conn, cid: &[u8]): orphan-blob GC. Consumers handling status transitions (status: Enrolled → Unverified, etc.) call this instead of writing raw SQL.
6. README.md: walletkit-db description matches Cargo.toml ("Encrypted on-device storage primitives ...") — old wording was stale from the pre-refactor PR #396 framing.
7. vault/mod.rs: drop the redundant let conn = &self.conn aliasing introduced during the refactor.
8. traits.rs: tighten the doc note about consumer adapters — orphan rule blocks a blanket impl across crates, so consumers need a small newtype.
Verified: cargo fmt, cargo clippy -D warnings (all/default/no-default-features), cargo test --workspace --lib --features walletkit-core/legacy-nullifiers --features walletkit-core/v3 (123 + 19 = 142 tests), cargo doc -Dwarnings.
Lax &[u8] input was silently matching no row on the wrong length. 3-line guard up front turns it into an explicit StoreError so callers see the programmer error at the call site instead of a missed read or no-op delete.
Short, intended-usage-first README focused on the four-step pattern a new consumer follows (lock, envelope key, open_vault, blobs::*) plus the public surface and on-disk format guarantee. Cargo.toml points at the per-crate README so it surfaces on crates.io instead of the workspace top-level one.
README should describe the crate for any consumer, not name a specific future one. Replaces the OrbKit mention + envelope filename/AD example with generic placeholders.
…ultDb to CredentialVault Makes the credential-specific wrapper distinct from walletkit-db's generic vault primitive. The directory used to read like "the vault"; now it reads like "the credential vault" — same shape, clearer intent. StorageError::VaultDb variant kept under its existing name (FFI-exposed; host bindings depend on it).
…tartup, encryption, threat model Adds the substantive context a consumer needs to understand what the crate is doing under the four-step pattern: the key hierarchy (K_device / K_intermediate / AD), the cold and warm startup sequences, what sqlite3mc actually does after PRAGMA key, and the threat model with its defense-in-depth levers. Content distilled from the OrbKit storage layer notes. OrbKit-specific material (write points, OrbPcpStore mapping) deliberately left out — README describes the crate for any consumer.
Up-front explanation of what each piece is — Vault, Envelope, Lock, blob_objects table, Keystore + AtomicBlobStore traits — and how they interact at open / store / read / delete time. Reading the rest of the README without this primer was harder than it needed to be.
…ate}
Replace the open_vault free function with a Vault struct that owns its Lock. Mutations run under a freshly-acquired guard via Vault::mutate(closure); reads bypass the lock via Vault::read(). The type system now enforces "mutations only under lock" instead of relying on documented caller discipline.
- walletkit-db: open_vault free fn dropped; Vault::{open, read, mutate}. init_or_open_envelope_key takes &Lock and acquires internally rather than requiring a witness &LockGuard.
- walletkit-core: every CredentialVault mutator drops its _lock: &StorageLockGuard parameter. CredentialStore stops plumbing a guard into vault.* (cache still takes &guard until the corruption-policy fix). The Vault is opened by passing a Lock::clone() from CredentialStoreInner.
Behavior unchanged: the same flock file is acquired around the same SQL operations. The lock is now acquired per-mutation rather than held across multi-call sequences, but each mutation is already its own SQL transaction so SQLite's own serialization covers the gap. The bootstrap race (two processes generating competing envelopes on first install) is still covered by the lock acquisition inside init_or_open_envelope_key and Vault::open.
On-disk format and host FFI surface unchanged. 123 walletkit-core lib tests pass, 20 walletkit-db lib tests pass.
CacheDb now wraps walletkit_db::Vault instead of opening Connection directly. Mutators (merkle_cache_put, session_seed_put, replay_guard_set) drop the _lock: &StorageLockGuard witness parameter and run their bodies inside self.vault.mutate(|conn| { ... }). Readers use self.vault.read().
open_or_rebuild moves to using Vault::open with the cache-specific rebuild-on-corruption policy in walletkit-core (one level above the generic vault opener). The corruption-handling policy where it belongs: in the consumer that knows cache contents are regenerable. walletkit-db stays policy-neutral; no CorruptionPolicy enum on Vault::open.
CredentialStore drops the dead let guard = self.guard()? bindings from init, merkle_cache_put, replay_guard_set, and the export/import paths. destroy_storage keeps self.guard() because it's the one place we still need an externally-held lock (filesystem deletes, not SQL transactions). The CredentialStoreInner.lock field stays for that case and to clone into Vault/Cache at construction.
Behavior unchanged: same flock around the same SQL operations; same SQL ran in the same order. On-disk format byte-stable. 123 walletkit-core + 20 walletkit-db lib tests pass; clippy clean on all/default/no-default-features; doc clean with -Dwarnings.
…impl Hosts pattern-match on StorageError variants for UX. Collapsing the From<walletkit_db::StoreError> impl into a single VaultDb(String) variant erases that variant identity at the FFI boundary; that path was tried in an earlier review and reverted. The mirror is intentional; future readers should not try to flatten it without a coordinated host change. The doc comment names the contract so the next reviewer doesn't relitigate it.
The Concepts section described opening at a high level; the Startup sequence section repeats the same flow in more detail. Removed the duplicate from Concepts and link forward to Startup sequence. Concepts now covers runtime ops only (store / read / delete).
…contract Two changes: - Add a 'Host-side contract for multi-consumer isolation' section spelling out what hosts MUST do (distinct keystore entry / AD / file paths per consumer) and the enforcement split (walletkit-db cryptographically enforces AD binding; everything else is host discipline). Names the IssuerKit layer as the long-term home for code-level enforcement. - Tighten the whole README: drop the redundant 'how they interact at runtime' walkthrough (the usage code example is the same content), collapse the encryption deep-dive to one paragraph, compress the startup sequence from numbered lists to two short paragraphs. Net ~40% shorter.
Drop the 'what walletkit-db enforces vs what it can't' table from the public README. It enumerates the host-side gap and reads like CVE-shopping documentation. The three host requirements (distinct keystore entry, distinct AD, distinct files) stay; the positive guarantee (AD binding) stays. Section renamed to 'Per-consumer isolation' and shrunk from ~25 lines to ~10.
Old comment was 13 lines of defensive explanation. Replaces with 6: states the intent (don't flatten, hosts depend on variants) and names the future move (extract to walletkit-ffi-shared when a second uniffi-exporting consumer ships).
cc7354c to
53e95ff
Compare
Five inline review comments on PR #400: - traits.rs: make Keystore AEAD requirement explicit (name AES-GCM / ChaCha20-Poly1305 in the doc instead of "integrity-protect"). - traits.rs: rename Keystore::seal/open_sealed param from associated_data to aad (standard AEAD term). Avoids collision with the credential-domain BlobKind::AssociatedData. - traits.rs: document why AtomicBlobStore is a trait the host implements rather than walletkit-db calling std::fs directly (no std::fs on wasm32; hosts own where data lives). - tests.rs: replace ASCII-art // ---- xxx ---- separators inside mod primitives with a //! module-level doc; promote the meaningful prose (XorKeystore explainer) to a /// doc on the struct. - sqlite/error.rs: rename the internal alias from Result to DbResult so it no longer shadows std::result::Result. Drops the "Result as DbResult" indirection at the crate root and at internal callers; the canonical name is now just DbResult everywhere.
The pre-PR walletkit-core/src/storage/keys.rs had a TODO flagging that k_intermediate.to_vec() leaves key material in a non-zeroized Vec<u8>. The comment got dropped when the cold-start path moved into walletkit-db::envelope. The underlying issue still exists: the trait shape (Vec<u8>) is what walletkit-core's uniffi DeviceKeystore requires, and the allocator doesn't zero on drop. Restore the comment, updated for the new location, naming both possible fixes (change the trait shape, or wrap the Vec in Zeroizing).
…er serialization
Per Paolo's review on vault.rs:73-74: SQLite handles cross-process writer serialization itself in WAL mode (cipher::configure_connection sets journal_mode = WAL). Vault::mutate's flock acquisition was redundant with that for normal mutations.
Vault collapses to a single accessor:
- pub struct Vault { conn: Connection } — no lock field.
- Vault::open(path, key, ensure_schema) — no Lock parameter.
- Vault::connection(&self) -> &Connection — replaces both read() and mutate().
Lock stays in walletkit-db as a primitive. init_or_open_envelope_key still acquires it internally for the bootstrap race (two processes generating competing envelopes on first install — SQLite isn't involved in envelope writes). CredentialStore acquires self.guard() explicitly around export_plaintext / import_plaintext for file-op atomicity at the ATTACH boundaries.
Removes self.vault.mutate(|conn| { ... }) wrapping across credential_vault and cache (~50 LOC). Method bodies become flat. CredentialVault::new and CacheDb::new no longer take a Lock argument.
On-disk format and host FFI unchanged. 141 walletkit-core + 19 walletkit-db lib tests pass.
| } | ||
|
|
||
| #[test] | ||
| fn test_credential_vault_on_disk_format_guard() { |
There was a problem hiding this comment.
This feels like a pre-refactor vs post-refactor consistency check which is surely useful during the actual refactor - but now is just confusing. I think we should remove it and instead prefer tests that validate input data against specific expected outputs
There was a problem hiding this comment.
The test is validating input --> output. I think there was a comment confusing you.
Let me know what you think now?
- Test relocation: walletkit-db primitive tests move next to their modules (blobs/envelope/lock/vault). Cross-crate format guard in credential_vault is reframed as a permanent on-disk test, not a refactor-time consistency check. - raw_connection() escape hatch dropped; tests use vault.connection() via child-module visibility. - #[allow] -> #[expect(..., reason = ...)] on store_credential. - Stale Vault::mutate / lock-enforced-mutation references swept from README, lib.rs, lock.rs module doc, vault.rs connection() doc, and credential_vault export/import method docs (which claimed lock acquisition that actually lives one layer up in CredentialStore). - #![allow(clippy::redundant_clone)] dropped (no actual hits). - test_content_id_determinism deleted; superseded by the frozen-byte test now living in walletkit-db/src/blobs.rs.
Move the constant from credential_vault/schema.rs to credential_vault/mod.rs so it lives next to export_plaintext / import_plaintext, the only call sites. schema.rs keeps the maintenance reminder via [`super::BACKUP_TABLES`].
… drop The earlier sed-based rewrite from .raw_connection() to .vault.connection() left chained calls that rustfmt wants split across lines.
The four-function helpers module had a single consumer (mod.rs) and added a layer with no encapsulation payoff. Moves map_record, to_i64, to_u64, and map_db_err to the bottom of mod.rs as private free functions and deletes the file.
Five sections distilled from the storage-primitives review: - On-disk format is byte-stable (with the frozen-byte test locations) - AEAD terminology (name the primitive, not "encrypt") - aad vs associated_data name overload - SQLite WAL handles writer serialization; don't layer flock - Per-consumer isolation lives in host wiring, not walletkit-db
Collapse the five expanded sections into one Code style block with bullet points. Same invariants, less detail, broader strokes.
Captures the patterns the storage-primitives review surfaced so they're visible to any future contributor (Claude or human) opening this repo: crate boundary, on-disk format invariance, no-flock-around-SQLite, per- consumer host wiring, #[expect] over #[allow], inline single-consumer helpers, colocate constants with consumers, tests next to code.
… Lock doc
The {} blocks in CredentialVault methods existed to scope a lock guard that
no longer exists (dropped with Vault::mutate in 5c3031a). Removing them
flattens the bodies and ?-propagation reads naturally now.
Also updates the inner Lock struct doc to stop describing itself as
serializing mutations; that framing was already corrected at the module
level but missed inside the imp module.
| - **`#[expect(lint, reason = "...")]` over `#[allow(lint)]`.** `#[expect]` fails to compile when the suppression is no longer needed, so dead suppressions don't accumulate. | ||
| - **Inline single-consumer helper modules.** A `mod foo;` file with `pub(super) fn`s used only by its parent adds boundary without payoff; put the functions as private free `fn`s at the bottom of the parent module. | ||
| - **Colocate constants with the code that reads them, not their conceptual home.** A `const` used only by `mod.rs` belongs in `mod.rs`, not in a sibling `schema.rs`. | ||
| - **Tests live next to the code they test.** One `#[cfg(test)] mod tests` per source file, including inside `cfg`-gated blocks. For code gated by `#[cfg(not(target_arch = "wasm32"))]` (e.g. `Lock`'s native `imp` module), tests go inside that same block so they only compile for the platform they cover. |
There was a problem hiding this comment.
too perscriptive - we don't need to colocate test code within the same cfg gated module
| - **`walletkit-db` is consumer-agnostic.** It owns `blob_objects` and the storage primitives (vault, envelope, lock, traits). Credential-specific tables, schemas, and APIs live in `walletkit-core/storage/credential_vault`. Don't put consumer logic in `walletkit-db`, and don't put primitives in consumer crates. | ||
| - **`#[expect(lint, reason = "...")]` over `#[allow(lint)]`.** `#[expect]` fails to compile when the suppression is no longer needed, so dead suppressions don't accumulate. | ||
| - **Inline single-consumer helper modules.** A `mod foo;` file with `pub(super) fn`s used only by its parent adds boundary without payoff; put the functions as private free `fn`s at the bottom of the parent module. | ||
| - **Colocate constants with the code that reads them, not their conceptual home.** A `const` used only by `mod.rs` belongs in `mod.rs`, not in a sibling `schema.rs`. |
There was a problem hiding this comment.
I don't agree with this at all 😅
my earlier comment about BACKUP_TABLES I think argued exactly for the opposite - i.e. collocating backup specific tables alongside backup code
| - **Per-consumer isolation is host wiring.** Separate keystore entry, AD label, and envelope/vault/lock files. `walletkit-db` enforces only the AEAD-AD binding. | ||
| - **`walletkit-db` is consumer-agnostic.** It owns `blob_objects` and the storage primitives (vault, envelope, lock, traits). Credential-specific tables, schemas, and APIs live in `walletkit-core/storage/credential_vault`. Don't put consumer logic in `walletkit-db`, and don't put primitives in consumer crates. | ||
| - **`#[expect(lint, reason = "...")]` over `#[allow(lint)]`.** `#[expect]` fails to compile when the suppression is no longer needed, so dead suppressions don't accumulate. | ||
| - **Inline single-consumer helper modules.** A `mod foo;` file with `pub(super) fn`s used only by its parent adds boundary without payoff; put the functions as private free `fn`s at the bottom of the parent module. |
There was a problem hiding this comment.
I guess I agree but it feels weird to write it down
| - **Don't layer a flock around SQLite writes.** WAL mode serializes writers itself. The `Lock` primitive is only for the envelope-init bootstrap and operations that mix SQL with filesystem state. | ||
| - **Per-consumer isolation is host wiring.** Separate keystore entry, AD label, and envelope/vault/lock files. `walletkit-db` enforces only the AEAD-AD binding. | ||
| - **`walletkit-db` is consumer-agnostic.** It owns `blob_objects` and the storage primitives (vault, envelope, lock, traits). Credential-specific tables, schemas, and APIs live in `walletkit-core/storage/credential_vault`. Don't put consumer logic in `walletkit-db`, and don't put primitives in consumer crates. | ||
| - **`#[expect(lint, reason = "...")]` over `#[allow(lint)]`.** `#[expect]` fails to compile when the suppression is no longer needed, so dead suppressions don't accumulate. |
Motivation
OrbKit's planned PCP store needs an encrypted on-device store but cannot depend on walletkit-core (credential schema, uniffi surface). Expanding walletkit-db into the shared storage-primitives crate beats publishing a sibling. Supersedes #396, which went the opposite direction (narrowing).
What changed
walletkit-dbexposes the building blocks any encrypted on-device store needs:Vault::{open, read, mutate}— owns the connection and the lock.mutateruns the closure under a freshly-acquired lock guard;readreturns&Connectiondirectly (SQLite WAL handles concurrent readers). Type system enforces "no mutation without lock" — the_lock: &LockGuardwitness parameter pattern is gone.blobs::{ensure_schema, put, get, delete, compute_content_id}pluspub type ContentId = [u8; 32].init_or_open_envelope_key+KeyEnvelope(CBOR). Acquires the lock internally.Lock/LockGuard(nativeflock/LockFileEx, WASM no-op). Re-exported from walletkit-core under the existingStorageLock/StorageLockGuardnames, so walletkit-core's public Rust API doesn't change.KeystoreandAtomicBlobStoretraits.StoreError/StoreResult.walletkit-core/src/storageshrinks accordingly.envelope.rsandlock.rsare gone (moved into walletkit-db).vault/is renamedcredential_vault/,VaultDbis renamedCredentialVault, and the cache wrapswalletkit_db::Vaulttoo. BothCredentialVaultandCacheDblost their_lock: &StorageLockGuardparameters across every mutator. Cache-specific rebuild-on-corruption policy stays in walletkit-core (one level aboveVault::open), so walletkit-db remains policy-neutral. Two thin newtypes (Ks,Bs) bridge the uniffi-annotated traits onto walletkit-db's plain-Rust ones; Rust's orphan rule rules out a blanket impl.The credential schema, the
BACKUP_TABLESlist, the FFI surface, and the documented mirror betweenwalletkit-core::StorageErrorandwalletkit_db::StoreErrorall stay in walletkit-core. TheFrom<StoreError>impl is variant-by-variant because hosts pattern-match onStorageErrorvariants for UX; the doc comment on that impl explains the contract.On-disk format is byte-stable. Frozen-byte tests pin
compute_content_idand theKeyEnvelopeCBOR layout;credential_vault/tests.rspins a credential round-trip. Existing user databases keep working without migration.walletkit-dbships with a per-crate README covering concepts (vault / envelope / lock / blobs / traits), architecture, key hierarchy, startup sequence, encryption mechanism (sqlite3mc), and threat model.Size note
+2,200 / -1,109looks big but is mostly non-production:walletkit-db/src/sqlite/andwalletkit-core/src/storage/credential_vault/are renames-with-edits rather than net new code (caught withgit diff -M).Production Rust net of moves and tests is closer to ~600 lines added, mostly the four new primitives modules in walletkit-db (vault, blobs, envelope, traits) and the corresponding rewires in walletkit-core's
keys.rs,credential_vault/mod.rs, andcache/.Closes #396.