feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968
Open
Claudius-Maginificent wants to merge 208 commits into
Open
feat(platform-wallet-storage): embeddable SQLite persistence backend with seedless rehydration#3968Claudius-Maginificent wants to merge 208 commits into
Claudius-Maginificent wants to merge 208 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a Tier-2 secret-envelope format and hardens secret storage, while renaming the SQLite wallet root to ChangesSecrets
SQLite
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lklimek
changed the title
3 tasks
…persistor [#3692, clean on v3.1-dev] Squashed net-diff of feat/platform-wallet-rehydration onto v3.1-dev base (1653b89). Includes all merged commits: • changeset: CoreChangeSet, ClientWalletStartState, addresses_derived wiring • rehydrate: seedless watch-only wallet rebuild + apply_persisted_core_state • load_outcome: LoadOutcome / SkipReason / CorruptKind • manager/load: load_from_persistor implementation • manager/mod: PlatformWalletManager wiring • events: PlatformEvent + on_wallet_skipped_on_load concrete handler • error: RehydrateRowError relocated from manager::rehydrate • core_bridge: warn_if_non_default_account generalised to &[T] slice • FFI: persistence + manager bindings • Swift: PlatformWalletManager load() bridging • tests: rehydration_load integration suite • misc: .cargo/audit.toml, .gitignore fmt + clippy (-D warnings) + cargo test: all pass. Tree verified byte-for-byte identical to feat/platform-wallet-rehydration HEAD. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…3968, independent on v3.1-dev] Storage-crate half of the rehydration work, rebuilt to stand alone on v3.1-dev: SqlitePersister::load() wiring + per-area readers (accounts, core_state, identities, asset_locks, contacts, identity_keys) that reconstruct the keyless ClientWalletStartState. Independence on v3.1-dev required two deliberate stubs — the reshaped ClientWalletStartState drops wallet/wallet_info, breaking two base consumers; both are resolved by #3692 in the dash-evo-tool integration: - manager/load.rs: whole-body todo!("keyless rehydration lands in #3692") - ffi/persistence.rs: tail-only todo!("seeded FFI restore path lands in #3692") — keeps the 8 builder helpers live (no dead_code under -D warnings) and minimizes the #3692 merge conflict Cross-crate manager-apply e2e tests in sqlite_core_state_reader.rs are gated behind a new off-by-default `rehydration-apply` feature (enabled in the integrated stack); storage-level load_state assertions run standalone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
lklimek
force-pushed
the
feat/platform-wallet-rehydration
branch
from
June 29, 2026 13:44
52cdad9 to
83f7d4f
Compare
lklimek
force-pushed
the
feat/platform-wallet-storage-rehydration
branch
from
June 29, 2026 13:44
3d57f73 to
2f2a74a
Compare
5 tasks
…ncrete handlers only (#3692 review) Remove `PlatformEventHandler::on_platform_event` (the generic backward-compat escape hatch) and `PlatformEventManager::on_platform_event` entirely. `on_wallet_skipped_on_load` now has a plain no-op default, matching the pattern used by every other concrete handler on the trait. `PlatformEvent` is kept: it is `pub`, re-exported from `lib.rs`, and not present in the FFI or Swift layer — no dead-code warning applies to public items, and removing it would be a needless churn of the public API. Not a breaking change vs v3.1-dev: `on_platform_event` was only ever on this branch (absent from origin/v3.1-dev). Doc comments in manager/load.rs and manager/mod.rs updated to point to `on_wallet_skipped_on_load` instead of the removed method/event wrapper. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…g-seed gate (#3692 review) The rehydrate module header and the rehydration_load test header both claimed the wrong-seed gate was "deferred to separate FFI work and is not part of this path." That gate now exists on the resolver-backed signing entrypoints (sign_with_mnemonic_resolver + the FFI resolver sign path). Reword to say wrong-seed validation lives there; the seedless load path never sees the seed. Docs-only, no behaviour change. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt HashSet (#3692 review) apply_persisted_core_state filtered new_utxos against spent_utxos with a nested `any()`, making the unspent projection O(new × spent). Collect the spent outpoints into a HashSet once and do O(1) membership lookups — behaviour is identical (Copy OutPoint, Hash + Eq), just linear. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ehydrate (#3692 review) The FFI build_wallet_start_state decoded the persisted core address pools (used flags + derived addresses) into a temp wallet_info, but the keyless ClientWalletStartState forwarded only the account manifest + UTXO/height projection — the pool used-state was dropped. apply_persisted_core_state then marked addresses used ONLY from currently-unspent UTXOs, so a previously-used address whose funds were since spent came back marked unused and could be handed out again as a fresh receive address: an address-reuse privacy leak. Carry the used-state through: - Add ClientWalletStartState::used_core_addresses (Vec<Address>, empty default) — a flat snapshot of every pool-marked-used address. - Populate it in the FFI projection from the already-decoded pools. - apply_persisted_core_state now marks used the UNION of unspent-UTXO addresses + used_core_addresses (new param), deriving deep slots via the existing horizon walk. Renamed extend_pools_for_restored_utxos -> extend_pools_for_restored_addresses since it now resolves both sources. Empty used_core_addresses preserves the prior unspent-only behaviour, so the native/SQLite path is unchanged until #3968 wires its pool readers to populate this field (cross-PR follow-up; no regression). Also fixes the O(new x spent) unspent filter via an outpoint HashSet. Test: rehydration_restores_persisted_used_state_for_spent_out_address asserts an in-window and a deep spent-out address come back used, and that the empty-snapshot baseline does NOT mark them. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t wallet on load (#3692 review) load_from_persistor mapped EVERY insert_wallet error (including key_wallet_manager::WalletError::WalletExists) to a fatal WalletCreation + 'load break + full rollback. So a second load_from_persistor — or a load run while the wallet is already in memory — aborted the whole batch instead of being a no-op. Match WalletExists specifically and treat it as already-satisfied: record the wallet as loaded and `continue` to the next row. It was not inserted by this pass, so it stays out of the rollback set and a later hard-fail never evicts the pre-existing wallet. Mirrors the create-path idempotent handling in wallet_lifecycle. Test: rt_idempotent_repeat_restore loads the same persister twice and asserts the second call returns Ok with the wallet still present. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ting the batch (#3692 review) FFIPersister::load looped `build_wallet_start_state(entry)?`, so ONE corrupt SwiftData row (e.g. a malformed account_xpub that aborts decode) failed the ENTIRE load() — every wallet, every launch. The manager already documents per-wallet skip (LoadOutcome::skipped + on_wallet_skipped_on_load, returns Ok), but the FFI never reached it. Make the FFI loop per-entry resilient: on a per-row build failure record the wallet as skipped and continue. Errors from build_wallet_start_state are inherently per-row (decode/projection of THAT entry), so this never swallows a whole-load failure. The skip travels to the manager through a new ClientStartState::skipped channel (Vec<(WalletId, SkipReason)>, empty default); load_from_persistor folds it into LoadOutcome::skipped and fires on_wallet_skipped_on_load. Reason is CorruptPersistedRow{DecodeError} — PersistenceError's Display is structural (no row bytes / key material). Cross-PR: ClientStartState derives Default so #3968's `::default()` build still compiles; a destructure there needs `skipped: _` (follow-up). Test: rt_persister_skipped_folds_into_outcome asserts a persister-rejected row surfaces in LoadOutcome::skipped + fires the event while the healthy wallet still loads and the call returns Ok. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…stive (#3692 review) Semver hygiene for the new, unreleased load surface so future variants don't break downstream matches: add #[non_exhaustive] to SkipReason, CorruptKind, LoadOutcome (load_outcome.rs) and PlatformEvent (events.rs). Consequence: the FFI skip_reason_code match (a downstream crate) is no longer exhaustive over the now-non_exhaustive SkipReason/CorruptKind, so add catch-all arms mapping future variants to generic codes (199 corrupt kind, 200 skip reason) until the mapping is extended. matches!() in tests is unaffected (it carries an implicit wildcard). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dex pool extension on rehydrate (#3692 review) QA flagged that the existing real-manager rehydration test defaults the address-pool payload and is structurally blind to #1, and that the pool-DEPTH fix (dash-evo-tool#829 Bug 2 / PR #830) had no regression guard. Add two distinct, focused tests through apply_persisted_core_state: - rehydration_used_state_survives_spent_utxo (#1, address-reuse): builds a ClientWalletStartState whose in-window address received funds that were then SPENT (new_utxos cancelled by spent_utxos → zero balance) and routes used_core_addresses through the field. Asserts the in-window + a deep (idx 30) address come back marked USED even with zero balance, and that the empty-snapshot baseline does NOT mark them. Replaces the weaker no-UTXO variant so the used flag is proven independent of a live UTXO. - rt_deep_index_utxos_extend_pools_on_rehydration (DEPTH): unspent UTXOs on walkable ladders past the eager 0..=gap_limit window (external -> idx 84, internal -> idx 90). Asserts the deep slots are derived into their pools and Sum(per-address visible) == balance.total == Sum(persisted) — no deep-index undercount. Test-only; the production fix already exists. Also: drop the stale "(from wallet_metadata)" table reference on the ClientWalletStartState::network doc (backend-agnostic now). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
review) Remove rt_deep_index_utxos_extend_pools_on_rehydration: the deep-index pool-extension scenario is already guarded by the pre-existing rehydration_extends_pools_to_cover_deep_index_utxos and rehydration_coinjoin_single_pool_deep_index. The existing 30->60 horizon extension already exercises the recursive walk, so a deeper ladder added no new code path — pure duplication. Keeps the #1 address-reuse test (rehydration_used_state_survives_spent_utxo). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eview) After the on_platform_event removal, the `PlatformEvent` enum had zero references repo-wide — events flow through the concrete `PlatformEventHandler` methods (`on_wallet_skipped_on_load`, etc.), not a dispatched enum. Remove the enum (and the `#[non_exhaustive]` just added to it) plus its `lib.rs` re-export. Its only variant, `WalletSkippedOnLoad`, went with it; the `on_wallet_skipped_on_load(wallet_id, &SkipReason)` handler and `SkipReason` itself stay. No imports orphaned — `SkipReason` and `WalletId` are still used by `PlatformEventHandler` / `PlatformEventManager`. Verified: `git grep PlatformEvent` over rs-platform-wallet, -ffi and swift-sdk is empty (only `PlatformEventHandler` / `PlatformEventManager` remain). <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Claudius-Maginificent
marked this pull request as ready for review
June 29, 2026 16:00
Claudius-Maginificent
requested review from
QuantumExplorer,
ZocoLini,
lklimek,
llbartekll and
shumkov
as code owners
June 29, 2026 16:00
Collaborator
|
🕓 Ready for review — 54 ahead in queue (commit 904e549) |
thepastaclaw
suggested changes
Jun 29, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The PR adds the storage-side keyless load readers, but it also replaces two externally reachable restore paths with unconditional panics. The new rehydration readers are mostly wired, but several fail-hard corruption checks are missing where typed SQLite columns can disagree with decoded blobs.
🔴 2 blocking | 🟡 6 suggestion(s)
Findings not posted inline (2)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key —load_state()selectsidentity_idbut discards it, then decodesentry_bloband routes the restored identity usingentry.id. The writer rejectsIdentityEntryvalues whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader shou... - [SUGGESTION]
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys — The contacts reader keys pending rows from(owner_id, contact_id)but stores the decodedContactRequestwithout checking its sender and recipient IDs. During apply, sent requests are inserted underentry.request.recipient_idand incoming requests underentry.request.sender_id, so a row wh...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:13-15: Public manager restore API now panics
`load_from_persistor()` is a public restore entry point returning `Result<(), PlatformWalletError>`, but this PR replaces the previous implementation with `todo!()`. The exported C ABI function `platform_wallet_manager_load_from_persistor` calls this method directly, and the Swift `loadFromPersistor()` wrapper calls that exported function, so any app invoking persisted wallet restore aborts instead of receiving a typed error. If this branch intentionally defers keyless manager rehydration to #3692, the public API still needs to fail closed with an error rather than panic across the FFI boundary.
In `packages/rs-platform-wallet-ffi/src/persistence.rs`:
- [BLOCKING] packages/rs-platform-wallet-ffi/src/persistence.rs:3389-3390: FFI persister load panics after receiving restore rows
`FFIPersister::load()` calls `build_wallet_start_state()` for every wallet returned by the Swift `on_load_wallet_list_fn` callback, and this function now reaches an unconditional `todo!()` after partially reconstructing the entry. This path is externally reachable through restore and shielded binding flows that call `persister.load()`. A panic here can unwind toward `extern "C"` callers and abort the process instead of returning the existing `PersistenceError`/`PlatformWalletFFIResult` failure path.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs:143-150: Identity reader trusts blob identity over the row key
`load_state()` selects `identity_id` but discards it, then decodes `entry_blob` and routes the restored identity using `entry.id`. The writer rejects `IdentityEntry` values whose blob ID disagrees with the typed column, but a restored or corrupted SQLite row can bypass the writer. The reader should enforce the same column-vs-blob check, including wallet scope when `entry.wallet_id` is set, so semantic corruption fails the load instead of hydrating the wrong identity.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs:168-169: Identity-key reader does not verify decoded entries match row columns
`load_state()` reconstructs `(identity_id, key_id)` from the SQL row, decodes `public_key_blob`, and inserts the decoded entry without checking that the blob carries the same identity, key id, wallet id, or public-key hash. The apply path later ignores the changeset map key and routes by fields from the decoded `IdentityKeyEntry`, so a semantically inconsistent row can attach a public key to the wrong identity or carry a hash that disagrees with the indexed column. Mirror the writer-side consistency checks on read before inserting into the changeset.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs:245-266: Contact reader does not validate request IDs against row keys
The contacts reader keys pending rows from `(owner_id, contact_id)` but stores the decoded `ContactRequest` without checking its sender and recipient IDs. During apply, sent requests are inserted under `entry.request.recipient_id` and incoming requests under `entry.request.sender_id`, so a row whose blob disagrees with the typed columns rehydrates under a different counterparty and later tombstones for the row key will not clear it. Established rows should also verify their outgoing and incoming requests match the same `(owner, contact)` relationship before accepting the row.
In `packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs:316-325: Oversized BLOB rows are materialized before the size cap runs
The new load readers fetch BLOB columns directly into `Vec<u8>` and only then call `blob::decode()`, whose 16 MiB cap runs after rusqlite has already allocated and copied the value. A restored or locally modified SQLite DB can therefore store a huge `record_blob` or other `*_blob` value that passes SQLite integrity checks and forces large process allocations on startup before returning `BlobTooLarge`. Use a shared bounded read helper or select `length(blob_column)` first, as the KV path already does, before materializing BLOB contents.
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs (1)
165-180: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCount
identity_keysbywallet_idnow that the table is wallet-scoped.
identity_keysmoved onto(wallet_id, identity_id, key_id), but this smoke test still routes it through thevia_identitypath. That means the assertion would still pass if the row were written with the wrongwallet_idas long asidentity_idmatched, so the new schema contract is not actually being exercised here.Suggested fix
let via_identity = [ - "identity_keys", "token_balances", "dashpay_profiles", "dashpay_payments_overlay", ];🤖 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_migrations.rs` around lines 165 - 180, The smoke test still treats identity_keys as identity-scoped, but the schema now scopes it by wallet_id. Update the test logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query path instead of the via_identity branch, while keeping the other tables that still depend on identities routed through identity_id. Use the existing via_identity handling in the loop over cases to locate and adjust the count_sql selection.packages/rs-platform-wallet-storage/SCHEMA.md (1)
507-513: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe soft-cascade note overstates cleanup for identity-scoped metadata.
meta_identityandmeta_tokendo not carrywallet_id, so a wallet delete only reaches them through existingidentitiesrows. If metadata was written before anidentitiesrow ever existed, that cleanup path never fires; the orphan-metadata section above already documents exactly that case.Suggested wording
-`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that -brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, -`meta_platform_address`) by `wallet_id`, and the FK cascade through -`identities` fires a per-identity trigger that brooms `meta_identity` + -`meta_token` by `identity_id`. Both legs key on the id alone, so a wallet -delete cleans its metadata transitively whether or not the typed parent -was ever written and regardless of any contact's lifecycle state. +`wallets` row fires a wallet-rooted `AFTER DELETE` trigger that +brooms the wallet-scoped tables (`meta_wallet`, `meta_contact`, +`meta_platform_address`) by `wallet_id`, and the FK cascade through +existing `identities` rows fires a per-identity trigger that brooms +`meta_identity` + `meta_token` by `identity_id`. That means wallet-scoped +metadata is cleaned regardless of typed-parent existence, while +identity-scoped metadata still requires an `identities` row to exist.🤖 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/SCHEMA.md` around lines 507 - 513, The soft-cascade description in SCHEMA.md overstates what a wallet delete cleans up for identity-scoped metadata. Update the note near the wallet/identity trigger flow to say that `wallets` deletion only reaches `meta_identity` and `meta_token` through existing `identities` rows and that orphan metadata written before an `identities` row exists is not covered; align the wording with the existing orphan-metadata section and reference the `wallets` trigger and the `identities` FK cascade path.packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs (1)
27-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed on corrupted platform-payment registration rows.
This helper trusts the typed
account_indexcolumn but never verifies that the decodedAccountRegistrationEntryis actually aPlatformPaymententry for that same index.all_platform_payment_registrations()feedsplatform_addrs::load_all(), so a tampered row will currently rehydrate under the typed index with the blob's xpub instead of trippingAccountRegistrationEntryMismatch.Suggested fix
fn decode_platform_payment_row( account_index: i64, xpub_bytes: &[u8], ) -> Result<PlatformPaymentRegistration, WalletStorageError> { let account_index = crate::sqlite::util::safe_cast::i64_to_u32( "account_registrations.account_index", account_index, )?; let entry: AccountRegistrationEntry = blob::decode(xpub_bytes)?; + if account_type_db_label(&entry.account_type) != "platform_payment" + || account_index(&entry.account_type) != account_index + { + return Err(WalletStorageError::AccountRegistrationEntryMismatch); + } Ok((account_index, entry.account_xpub)) }🤖 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/accounts.rs` around lines 27 - 36, `decode_platform_payment_row` currently decodes the blob and returns the typed `account_index` without checking that the `AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update this helper to validate the decoded `AccountRegistrationEntry` matches the expected `PlatformPayment` variant and index, and return `AccountRegistrationEntryMismatch` if it does not. Keep the existing `safe_cast::i64_to_u32` conversion, but make `all_platform_payment_registrations()` fail closed by rejecting any corrupted or mismatched row instead of rehydrating it.packages/rs-platform-wallet-storage/src/sqlite/backup.rs (2)
243-263: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftDo not delete WAL/SHM before the replacement is guaranteed.
If sibling removal succeeds and
tmp.persist(dest_db_path)then fails, the original main DB remains but its WAL/SHM may be gone, losing committed WAL-mode state. The restore path needs a rollback-safe swap strategy or a SQLite-native restore that does not destructively unlink siblings before the main replacement succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 243 - 263, The restore flow in `backup.rs` removes `-wal`/`-shm` siblings before `tmp.persist(dest_db_path)`, which can leave the original DB intact but its WAL-mode state lost if persist fails. Change the `restore` logic to use a rollback-safe replacement strategy: do not unlink siblings until the destination swap is guaranteed, or replace the whole SQLite set atomically via a SQLite-native restore path. Keep the fix localized around the sibling cleanup and `tmp.persist` sequence so the operation remains all-or-nothing.
361-374: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply
keep_last_nas a floor, not a ceiling.With both
keep_last_nandmax_ageset, line 373 still requirespass_count, so backups beyond the newest N are deleted even when they are withinmax_age. That contradicts the new floor semantics.Proposed fix
- let pass_count = match policy.keep_last_n { - Some(n) => idx < n, - None => true, - }; let pass_age = match policy.max_age { Some(max) => now.duration_since(ts).map(|d| d <= max).unwrap_or(true), - None => true, + None => policy.keep_last_n.is_none(), }; - if within_floor || (pass_count && pass_age) { + if within_floor || pass_age {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 361 - 374, In backup pruning logic in the `retain_backups` flow, `keep_last_n` is still being treated like a ceiling because the deletion condition requires `pass_count` even when `max_age` is also set. Update the condition around `within_floor`, `pass_count`, and `pass_age` so that the newest N backups are always kept as a floor and any backup within the age limit is also retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in this block.packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)
143-150: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate typed identity columns against the blob during load.
load_stateignores the selectedidentity_id, so a corrupted row whose typed column andentry_blob.iddiverge is silently rehydrated under the blob value. Also reject a blobwallet_idthat disagrees with the scoped wallet.Proposed fix
- let _identity_id: Vec<u8> = row.get(0)?; + let identity_id: Vec<u8> = row.get(0)?; let payload: Vec<u8> = row.get(1)?; let tombstoned: i64 = row.get(2)?; if tombstoned != 0 { continue; } + let typed_id = <[u8; 32]>::try_from(identity_id.as_slice()) + .map_err(|_| WalletStorageError::blob_decode("identities.identity_id is not 32 bytes"))?; let entry: IdentityEntry = blob::decode(&payload)?; + if entry.id.as_bytes() != &typed_id { + return Err(WalletStorageError::IdentityEntryIdMismatch); + } + if let Some(entry_wallet_id) = entry.wallet_id { + if entry_wallet_id != *wallet_id { + return Err(WalletStorageError::WalletIdMismatch { + expected: *wallet_id, + found: entry_wallet_id, + }); + } + } let managed = managed_identity_from_entry(&entry, wallet_id);🤖 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 143 - 150, The load path in load_state is trusting the blob too much and currently ignores the selected identity_id, so mismatched typed columns can be silently rehydrated under the blob value. Update the row handling in load_state to validate that the typed identity_id matches entry_blob.id before decoding into IdentityEntry, and also verify the blob wallet_id matches the wallet_id scope passed into managed_identity_from_entry. If either check fails, reject the row instead of continuing.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
299-326: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the open-path registry before restore.
restore_from_innercan replacedest_db_pathwhile a liveSqlitePersisterin this process still owns the same DB. Check the registry up front and returnAlreadyOpen; otherwise the live handle/buffer can diverge from the restored file.Proposed fix outline
+ let registered_path = dest_db_path + .canonicalize() + .unwrap_or_else(|_| dest_db_path.to_path_buf()); + if open_path_registry() + .lock() + .unwrap_or_else(|p| p.into_inner()) + .contains(®istered_path) + { + return Err(WalletStorageError::AlreadyOpen { + path: registered_path, + }); + } + if !skip_backup && dest_db_path.exists() {🤖 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 299 - 326, restore_from_inner currently restores the database without checking whether the destination path is already owned by a live SqlitePersister, which can leave an in-memory handle out of sync with the replaced file. Add an upfront registry lookup in restore_from_inner for dest_db_path and return WalletStorageError::AlreadyOpen when the path is already registered, before any backup or restore work begins. Keep the change localized around restore_from_inner and the open-path registry used by SqlitePersister so existing live handles are protected from restore-time replacement.
🧹 Nitpick comments (4)
packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs (1)
91-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
synced_heightas well aslast_processed_height.This test writes both fields, but only validates one of them. If
load()stops wiringsynced_height, the round-trip still passes.Suggested assertion
assert_eq!(slice.core_state.new_utxos.len(), 1); assert_eq!(slice.core_state.new_utxos[0].value(), 777_000); + assert_eq!(slice.core_state.synced_height, Some(50)); assert_eq!(slice.core_state.last_processed_height, Some(50));🤖 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_load_wiring.rs` around lines 91 - 127, The round-trip test in `sqlite_load_wiring.rs` only verifies `last_processed_height` from `state.wallets.get(&w).core_state` even though `synced_height` is also written into `CoreChangeSet`; update the existing load assertions to check both fields after `p2.load()` so `load()` wiring regressions for `synced_height` are caught alongside `last_processed_height`.packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs (1)
93-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the overlay stays out of the rehydrated identity.
This currently proves only that
load()still returns the wallet's core state. If a regression starts mergingdashpay_profilesinto the loaded identity payload, this test still passes. Please also assert that the seeded identity is present afterload()and that its DashPay profile remains absent for the overlay-only write case.🤖 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_dashpay_overlay_contract.rs` around lines 93 - 108, The current test around persister.load() only verifies wallet.core_state, so it can miss regressions where dashpay_profiles gets merged into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to also inspect the loaded identity payload for the seeded wallet after load() and assert that the identity is still present while its DashPay profile remains absent in this overlay-only write scenario. Use the existing persister.load(), wallets.get(&w), and any identity fields already available in the loaded state to make the check explicit.packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs (1)
67-72: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlso assert that the failed pre-flush left nothing durable.
Restoring the buffer is only half of the contract here. If
apply_changeset_to_txever leaks thewalletsinsert before thecore_sync_statefailure, this test still passes and leaves duplicate-on-retry state behind.Suggested assertion block
assert!( persister.buffer_has_changeset_for_test(&w), "buffered changeset must be restored after a real pre-flush apply failure" ); + + let conn = persister.lock_conn_for_test(); + let wallets_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM wallets WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + let core_rows: i64 = conn + .query_row( + "SELECT COUNT(*) FROM core_sync_state WHERE wallet_id = ?1", + rusqlite::params![w.as_slice()], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(wallets_rows, 0, "failed pre-flush must not durably create the wallet row"); + assert_eq!(core_rows, 0, "failed pre-flush must not durably create child rows");🤖 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_delete_real_apply_failure.rs` around lines 67 - 72, The test currently only verifies the buffered changeset is restored, but it should also verify that a failed pre-flush did not persist any durable state. In sqlite_delete_real_apply_failure.rs, extend the existing scenario around the failed delete so it checks the database/transaction state after the apply failure and confirms no `wallets` insert or other durable side effects remain from `apply_changeset_to_tx`. Keep the existing `persister.buffer_has_changeset_for_test(&w)` assertion, and add a second assertion in the same test that validates the storage is clean after the failure so retry does not see duplicate-on-retry state.packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)
813-814: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the query-budget documentation.
load()currently performs multiple reader calls inside thefor wallet_id in wallet_idsloop, so the query count grows with wallet count. Reword this to avoid promising constant query budget.🤖 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 813 - 814, Update the query-budget comment in the load path so it no longer claims constant cost with wallet count; the current load() flow iterates over wallet_ids and performs multiple reader calls per wallet, so reword the documentation to describe that it has per-wallet read/query work rather than a fixed query budget. Keep the note near the wallet_ids loop/load() implementation and make sure the wording matches the actual behavior of the reader calls.
🤖 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-ffi/src/persistence.rs`:
- Around line 3389-3390: The temporary restore stub in the persistence restore
flow should not panic via todo!(); replace it with a recoverable typed error so
callers receive a PersistenceError instead of crashing. Update the restore-path
branch that currently ignores identity_manager and unused_asset_locks to return
an appropriate PersistenceError variant (or equivalent error conversion) from
the same function/method, keeping the signature consistent and preserving the
existing error handling path.
In `@packages/rs-platform-wallet-storage/README.md`:
- Around line 165-168: The README wording around the manager-side rehydration
flow is too strong for this PR because the manager/FFI load path is still
stubbed. Update the description near the watch-only rebuild note to clearly mark
the manager-side `load_from_persistor`/`Wallet::new_watch_only` application as
pending or follow-up work, and keep the current text scoped to the storage-side
behavior only.
In `@packages/rs-platform-wallet-storage/src/kv.rs`:
- Around line 62-65: The key-length validation in validate_key currently assumes
Rust chars().count() matches SQLite length() for all strings, but embedded NULs
break that equivalence. Update the key precheck to explicitly reject keys
containing \0 before comparing length, or adjust the validation/comment so it no
longer claims the same key set; keep the logic aligned with the SQL CHECK
constraint in kv.rs.
In `@packages/rs-platform-wallet-storage/src/secrets/error.rs`:
- Around line 3-5: The file-level non-leakage docs in error.rs are too broad for
the current Io behavior: they claim variants never carry a stringified source,
but Io::fmt/rendering still exposes the underlying source text. Update the docs
to carve out the Io exception, or change Io’s display implementation/tests so it
no longer includes the source string, keeping the wording aligned with the
actual Error and Io rendering behavior.
- Around line 88-91: The UnsupportedEnvelopeVersion error currently truncates
the envelope version to u8, so update the error variant in error.rs to store the
full u32 version value instead. Then adjust the envelope parsing call site that
constructs UnsupportedEnvelopeVersion to pass the original Envelope.version
without narrowing, keeping the reported version accurate in the error message.
In `@packages/rs-platform-wallet-storage/src/secrets/file/format.rs`:
- Around line 21-22: The docs for the nested BTreeMap format currently imply
duplicate (wallet_id, label) pairs are prevented entirely, but the read path
still accepts duplicate JSON keys and serde collapses them. Update the
documentation near the format description to state that uniqueness is only
guaranteed by serialization, or change the deserialization logic in the file
format/parser code to explicitly reject duplicate keys, and make the behavior
match the tests and the intended API.
In `@packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- Around line 628-654: The post-persist Unix handling in the vault write path is
swallowing parent-directory fsync failures and returning success, which makes
`put`/`delete`/`rekey` report a durable commit when only the rename succeeded.
Update the flow around the `persist()`/`sync_all()` block to surface a distinct
“committed but not durable” result or otherwise keep the in-memory commit behind
the durability boundary, and make sure the caller can tell when
`fs::File::open(parent)` or `sync_all()` fails instead of only logging via
`tracing::warn!`.
In `@packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- Around line 255-266: The reprotect method in SecretStore currently does a
non-atomic read-then-write using get_secret followed by set_secret, which can
overwrite concurrent updates with stale plaintext. Update reprotect to use an
atomic backend-specific reprotect/CAS path, or add a version check so the write
only succeeds if the entry has not changed since get_secret; reference
SecretStore::reprotect, get_secret, and set_secret when wiring the fix.
In `@packages/rs-platform-wallet-storage/src/secrets/wire/envelope.rs`:
- Around line 136-141: The scheme-0 plaintext path in the envelope handling
still leaves temporary Vec<u8> buffers unwiped, including the
Unprotected(plaintext.to_vec()) branch and the ExpectedProtectedButUnsealed arm.
Update the envelope logic in the encode/decode flow around the Envelope and
Payload handling to use zeroizing storage for these plaintext temporaries or
explicitly wipe them before drop, while keeping SecretBytes::new only for the
final encoded blob.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 179-199: `persist`/`open` currently treats `has_schema_history()`
as the only brand-new-vs-existing check, so a pre-existing non-wallet SQLite
file with no `refinery_schema_history` can still be migrated. Add an explicit
guard in the `had_schema_history` decision path to reject existing SQLite files
that lack wallet schema history, using the same `conn`/`has_schema_history` flow
and returning a typed wallet storage error before any backup, integrity check,
or `migrations::run()` work begins.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 143-154: The sync-state write path in core_state should treat
last_applied_chain_lock monotonically, not as a blind overwrite. Update the
CoreChangeSet-to-DB flow around upsert_sync_state so the stored chain-lock is
max-merged with the existing row (using the same chain-lock height comparison
logic as the height watermarks) before persisting. Apply this behavior wherever
last_applied_chain_lock is written in the affected core_state update functions
so the persisted chain-lock cannot regress.
- Around line 40-41: The `decode_from_slice` handling in
`last_applied_chain_lock` is too permissive because it accepts a valid prefix
and ignores any appended data. Update this decoding path in `core_state.rs` to
mirror the other blob decoders: after calling `bincode::decode_from_slice` for
`ChainLock`, verify the returned consumed length matches `bytes.len()` and treat
any mismatch as corruption by returning `None` instead of loading the state.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 151-169: Mirror the writer-side validation in load_state by
checking that each decoded public_key_blob matches the row’s typed columns
before inserting into cs.upserts. After decode_entry(&payload), verify the
entry’s identity_id, key_id, wallet_id, and public_key_hash against the values
from the identity_keys query, and return a WalletStorageError if any mismatch is
found. Keep the checks local to load_state and use the existing decode_entry,
Identifier::from, and KeyID::try_from flow so inconsistent rows are rejected
instead of loaded silently.
In `@packages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rs`:
- Around line 46-82: The sqlite_accounts_reader test is too weak because both
AccountRegistrationEntry fixtures use the same xpub and the assertions only
check set membership, so row reordering or xpub/row mixups can still pass.
Update the test to use distinct xpub fixtures for each entry and assert the
loaded manifest in the expected order, using the accounts::load_state result and
the existing AccountType variants to verify each row maps to the correct xpub.
In `@packages/rs-platform-wallet/src/changeset/client_wallet_start_state.rs`:
- Line 33: The doc comment on the wallet start state field still references the
old wallet_metadata table. Update the comment in client_wallet_start_state.rs to
point to the renamed wallets table instead, keeping the wording aligned with the
field’s source of truth and using the existing comment near the network field to
locate it.
In `@packages/rs-platform-wallet/src/manager/load.rs`:
- Around line 8-14: The public rehydration entry point
PlatformWalletManager::load_from_persistor currently panics via todo!, which
turns a caller error into a runtime abort. Replace the todo! with a recoverable
Result path by returning an explicit PlatformWalletError for the unsupported
stub state, or otherwise gate/remove this API until keyless rehydration in
PlatformWalletManager is implemented. Ensure callers receive an error instead of
a panic.
---
Outside diff comments:
In `@packages/rs-platform-wallet-storage/SCHEMA.md`:
- Around line 507-513: The soft-cascade description in SCHEMA.md overstates what
a wallet delete cleans up for identity-scoped metadata. Update the note near the
wallet/identity trigger flow to say that `wallets` deletion only reaches
`meta_identity` and `meta_token` through existing `identities` rows and that
orphan metadata written before an `identities` row exists is not covered; align
the wording with the existing orphan-metadata section and reference the
`wallets` trigger and the `identities` FK cascade path.
In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- Around line 243-263: The restore flow in `backup.rs` removes `-wal`/`-shm`
siblings before `tmp.persist(dest_db_path)`, which can leave the original DB
intact but its WAL-mode state lost if persist fails. Change the `restore` logic
to use a rollback-safe replacement strategy: do not unlink siblings until the
destination swap is guaranteed, or replace the whole SQLite set atomically via a
SQLite-native restore path. Keep the fix localized around the sibling cleanup
and `tmp.persist` sequence so the operation remains all-or-nothing.
- Around line 361-374: In backup pruning logic in the `retain_backups` flow,
`keep_last_n` is still being treated like a ceiling because the deletion
condition requires `pass_count` even when `max_age` is also set. Update the
condition around `within_floor`, `pass_count`, and `pass_age` so that the newest
N backups are always kept as a floor and any backup within the age limit is also
retained, using the existing `policy.keep_last_n` and `policy.max_age` checks in
this block.
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 299-326: restore_from_inner currently restores the database
without checking whether the destination path is already owned by a live
SqlitePersister, which can leave an in-memory handle out of sync with the
replaced file. Add an upfront registry lookup in restore_from_inner for
dest_db_path and return WalletStorageError::AlreadyOpen when the path is already
registered, before any backup or restore work begins. Keep the change localized
around restore_from_inner and the open-path registry used by SqlitePersister so
existing live handles are protected from restore-time replacement.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs`:
- Around line 27-36: `decode_platform_payment_row` currently decodes the blob
and returns the typed `account_index` without checking that the
`AccountRegistrationEntry` is a `PlatformPayment` for that same index. Update
this helper to validate the decoded `AccountRegistrationEntry` matches the
expected `PlatformPayment` variant and index, and return
`AccountRegistrationEntryMismatch` if it does not. Keep the existing
`safe_cast::i64_to_u32` conversion, but make
`all_platform_payment_registrations()` fail closed by rejecting any corrupted or
mismatched row instead of rehydrating it.
In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 143-150: The load path in load_state is trusting the blob too much
and currently ignores the selected identity_id, so mismatched typed columns can
be silently rehydrated under the blob value. Update the row handling in
load_state to validate that the typed identity_id matches entry_blob.id before
decoding into IdentityEntry, and also verify the blob wallet_id matches the
wallet_id scope passed into managed_identity_from_entry. If either check fails,
reject the row instead of continuing.
In `@packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs`:
- Around line 165-180: The smoke test still treats identity_keys as
identity-scoped, but the schema now scopes it by wallet_id. Update the test
logic in sqlite_migrations.rs so identity_keys uses the wallet_id COUNT query
path instead of the via_identity branch, while keeping the other tables that
still depend on identities routed through identity_id. Use the existing
via_identity handling in the loop over cases to locate and adjust the count_sql
selection.
---
Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 813-814: Update the query-budget comment in the load path so it no
longer claims constant cost with wallet count; the current load() flow iterates
over wallet_ids and performs multiple reader calls per wallet, so reword the
documentation to describe that it has per-wallet read/query work rather than a
fixed query budget. Keep the note near the wallet_ids loop/load() implementation
and make sure the wording matches the actual behavior of the reader calls.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rs`:
- Around line 93-108: The current test around persister.load() only verifies
wallet.core_state, so it can miss regressions where dashpay_profiles gets merged
into the rehydrated identity. Update the sqlite_dashpay_overlay_contract test to
also inspect the loaded identity payload for the seeded wallet after load() and
assert that the identity is still present while its DashPay profile remains
absent in this overlay-only write scenario. Use the existing persister.load(),
wallets.get(&w), and any identity fields already available in the loaded state
to make the check explicit.
In
`@packages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rs`:
- Around line 67-72: The test currently only verifies the buffered changeset is
restored, but it should also verify that a failed pre-flush did not persist any
durable state. In sqlite_delete_real_apply_failure.rs, extend the existing
scenario around the failed delete so it checks the database/transaction state
after the apply failure and confirms no `wallets` insert or other durable side
effects remain from `apply_changeset_to_tx`. Keep the existing
`persister.buffer_has_changeset_for_test(&w)` assertion, and add a second
assertion in the same test that validates the storage is clean after the failure
so retry does not see duplicate-on-retry state.
In `@packages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rs`:
- Around line 91-127: The round-trip test in `sqlite_load_wiring.rs` only
verifies `last_processed_height` from `state.wallets.get(&w).core_state` even
though `synced_height` is also written into `CoreChangeSet`; update the existing
load assertions to check both fields after `p2.load()` so `load()` wiring
regressions for `synced_height` are caught alongside `last_processed_height`.
🪄 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: edc85543-e83f-4a54-88ef-17800859c720
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (79)
packages/rs-platform-wallet-ffi/src/persistence.rspackages/rs-platform-wallet-storage/.cargo/audit.tomlpackages/rs-platform-wallet-storage/Cargo.tomlpackages/rs-platform-wallet-storage/README.mdpackages/rs-platform-wallet-storage/SCHEMA.mdpackages/rs-platform-wallet-storage/SECRETS.mdpackages/rs-platform-wallet-storage/migrations/V001__initial.rspackages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rspackages/rs-platform-wallet-storage/src/kv.rspackages/rs-platform-wallet-storage/src/lib.rspackages/rs-platform-wallet-storage/src/secrets/error.rspackages/rs-platform-wallet-storage/src/secrets/file/crypto.rspackages/rs-platform-wallet-storage/src/secrets/file/format.rspackages/rs-platform-wallet-storage/src/secrets/file/mod.rspackages/rs-platform-wallet-storage/src/secrets/keyring.rspackages/rs-platform-wallet-storage/src/secrets/mod.rspackages/rs-platform-wallet-storage/src/secrets/secret.rspackages/rs-platform-wallet-storage/src/secrets/store.rspackages/rs-platform-wallet-storage/src/secrets/wire/aad.rspackages/rs-platform-wallet-storage/src/secrets/wire/config.rspackages/rs-platform-wallet-storage/src/secrets/wire/envelope.rspackages/rs-platform-wallet-storage/src/secrets/wire/kdf.rspackages/rs-platform-wallet-storage/src/secrets/wire/mod.rspackages/rs-platform-wallet-storage/src/sqlite/backup.rspackages/rs-platform-wallet-storage/src/sqlite/config.rspackages/rs-platform-wallet-storage/src/sqlite/conn.rspackages/rs-platform-wallet-storage/src/sqlite/error.rspackages/rs-platform-wallet-storage/src/sqlite/kv.rspackages/rs-platform-wallet-storage/src/sqlite/migrations.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/wallets.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_api.rspackages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rspackages/rs-platform-wallet-storage/tests/secrets_scan.rspackages/rs-platform-wallet-storage/tests/sqlite_account_zero_attribution.rspackages/rs-platform-wallet-storage/tests/sqlite_accounts_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_asset_locks_filter.rspackages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rspackages/rs-platform-wallet-storage/tests/sqlite_check_constraints.rspackages/rs-platform-wallet-storage/tests/sqlite_commit_writes_lock_poison_shortcircuit.rspackages/rs-platform-wallet-storage/tests/sqlite_compile_time.rspackages/rs-platform-wallet-storage/tests/sqlite_contacts_keys_rehydration.rspackages/rs-platform-wallet-storage/tests/sqlite_core_state_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_dashpay_overlay_contract.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_cross_process_exclusion.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_partial_commit_window.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_real_apply_failure.rspackages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rspackages/rs-platform-wallet-storage/tests/sqlite_error_classification.rspackages/rs-platform-wallet-storage/tests/sqlite_fk_changeset_ordering.rspackages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rspackages/rs-platform-wallet-storage/tests/sqlite_identity_keys_reader.rspackages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rspackages/rs-platform-wallet-storage/tests/sqlite_load_wiring.rspackages/rs-platform-wallet-storage/tests/sqlite_migrations.rspackages/rs-platform-wallet-storage/tests/sqlite_money_column_overflow_on_read.rspackages/rs-platform-wallet-storage/tests/sqlite_object_metadata.rspackages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rspackages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rspackages/rs-platform-wallet-storage/tests/sqlite_qa_identity_tombstone.rspackages/rs-platform-wallet-storage/tests/sqlite_second_open_guard.rspackages/rs-platform-wallet-storage/tests/sqlite_structural_hardening.rspackages/rs-platform-wallet-storage/tests/sqlite_wallet_db_identity.rspackages/rs-platform-wallet/src/changeset/client_wallet_start_state.rspackages/rs-platform-wallet/src/manager/load.rs
…riminators to stop distinct-variant collapse (#3968 review) account_registrations keyed on (wallet_id, account_type, account_index) only. PlatformPayment key classes and DashPay (user, friend) identity pairs share that key across genuinely distinct accounts, so the ON CONFLICT DO UPDATE silently overwrote one with another — a restored wallet lost accounts (data loss). Chose option (a) widen-PK over fail-loud: a wallet legitimately holds multiple DashpayReceivingFunds accounts (one per contact) at the same index, so failing the collision would reject valid multi-contact wallets. Add key_class, user_identity_id, friend_identity_id as NOT NULL columns with sentinel defaults (0 / zeroblob) so non-discriminated variants still dedup on re-persist, and widen the PK to include them. The reader cross-checks every typed PK column against the decoded blob and orders deterministically. V001 edited in place (on-disk format unshipped). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
…ss pool rows PR #4127 replaced the retired dedicated-batch mechanism for platform-node Ed25519 keys with the generic account_address_pools pipeline, but only wired it up for the FFI backend. The SQLite backend silently dropped AddressInfo::public_key on every store()/load() round trip, so ProviderPlatformKeys pools came back empty after a restart -- these keys are SLIP-10 hardened-only and cannot be re-derived from a watch-only xpub. Adds migration V005 (nullable public_key/key_type columns on core_address_pool), persists the typed key alongside each pool row, and restores AbsentHardened platform-node entries during load() by reconstructing their AddressInfo the same way populate_platform_node_pool does at registration time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
thepastaclaw
suggested changes
Jul 15, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 7956bb8, current-source validation keeps one blocking lifecycle defect and two storage suggestions, all carried forward from the prior review; no new latest-delta finding was established. The manager remains reusable after its persistence machinery is shut down, while the account-domain and post-restore WAL issues remain present; provider-key persistence remains explicitly deferred to #4113.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/manager/load.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/manager/load.rs:42: [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
Both the initial persister error path here and hydration rollback at line 206 call `self.shutdown().await`. That permanently cancels the wallet-event adapter and consumes its join handle in `manager/mod.rs:381-385`; only `PlatformWalletManager::new` spawns the adapter. The FFI load entry point merely borrows the manager and returns the error, while `WalletManagerStore.activate` catches that error, caches the same manager at lines 206-207, and returns the cached instance on later same-SDK activation at lines 162-175. Wallet operations can therefore continue through a live handle while core `WalletEvent` changes no longer reach the persister, silently losing UTXO, transaction, used-address, and sync-watermark updates. Keep or restart the callback machinery on recoverable use, or invalidate the FFI handle so callers must reconstruct the manager.
| // so a reconstruct on the same path doesn't hit `AlreadyOpen` | ||
| // masking this error. | ||
| tracing::debug!(error = ?e, "persister load failed during rehydration"); | ||
| self.shutdown().await; |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: [carried-forward: prior-manager-load-shutdown] Keep the retained manager operational after load failure
Both the initial persister error path here and hydration rollback at line 206 call self.shutdown().await. That permanently cancels the wallet-event adapter and consumes its join handle in manager/mod.rs:381-385; only PlatformWalletManager::new spawns the adapter. The FFI load entry point merely borrows the manager and returns the error, while WalletManagerStore.activate catches that error, caches the same manager at lines 206-207, and returns the cached instance on later same-SDK activation at lines 162-175. Wallet operations can therefore continue through a live handle while core WalletEvent changes no longer reach the persister, silently losing UTXO, transaction, used-address, and sync-watermark updates. Keep or restart the callback machinery on recoverable use, or invalidate the FFI handle so callers must reconstruct the manager.
source: ['codex']
The dedicated node-key-batch mechanism this review audited (V004__provider_key_accounts.rs, ProviderKeyRegistrationBlob, provider_platform_node_keys) was retired by commits b361e11/a536a26cba. The doc still made present-tense claims (e.g. "tests/sqlite_v004_migration.rs: 3/3 pass") about tests and code paths that no longer exist. Adds an explicit superseded banner so a reader doesn't mistake historical findings for current coverage. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
UPSERT_POOL_SQL overwrote public_key/key_type unconditionally on conflict, unlike the adjacent monotonic `used = MAX(...)`. A second store() with a different key at the same (wallet_id, account_type, account_index, key_class, user_identity_id, friend_identity_id, pool_type, address_index) silently clobbered the first with no error -- and these hardened-derivation keys cannot be re-derived from a watch-only xpub if lost. apply_pools() now pre-checks typed rows (key_type IS NOT NULL) against any existing row at the same key and rejects the whole flush via a new WalletStorageError::TypedPoolKeyConflict when the incoming public_key/key_type disagrees with what's already stored, mirroring ProviderKeyAccountConflict's existing fail-closed pattern for the sibling account-level xpub. Identical re-writes (idempotent resync) and fresh indices are unaffected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
populate_platform_node_pool (registration time) and restore_provider_platform_node_pool (SQLite load time) hand-duplicated the same 14-field AddressInfo construction and AbsentHardened pool insertion across a crate boundary, and had already drifted: one silently no-op'd when the account's hardened pool was missing, the other hard-errored. Extracts the shared logic into insert_platform_node_pool_entry, called from both sites. Unifies the missing-pool case to a hard error in both places -- a ProviderPlatformKeys account without its AbsentHardened pool is an invariant violation worth surfacing, not silently ignoring. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014cKDZc6zhCkstdqE1HcSuU
lklimek
added a commit
to dashpay/dash-evo-tool
that referenced
this pull request
Jul 16, 2026
* fix(migration): prompt for wallet passwords before completing migration
Restores the migration password prompt reverted from PR #887 so it can be
reworked in isolation. This commit is the original implementation verbatim;
the review findings that caused the revert are fixed in the commits that
follow.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
* fix(migration): free the password prompt from the SPV overlay, add a skip
Two defects found by real-world testing of the migration password prompt.
The SPV progress overlay and the passphrase modal both painted at
`egui::Order::Foreground`. Suppressing only `ProgressOverlay::claim_input`
released the keyboard but left the overlay's pointer sink and dim/card layers
live, so they swallowed clicks aimed at the password field — the prompt was
visible but unusable whenever migration ran alongside an SPV sync (which is
always, at boot). A blocking secret prompt now owns the whole interaction
surface: while one is active the overlay stays logically in its stack but
paints no dimmer, pointer sink, card, or focus trap, and claims no keyboard.
Queued ordinary secret prompts are promoted before the frame's overlay
decision, so their first visible frame is protected too.
The prompt was also inescapable: a user who had forgotten a wallet password
could not proceed, and the only exit the UI permitted was deleting the wallet.
"Skip this wallet" now records the seed hash in a per-run exclusion set, drops
it from the published pending list, and wakes the migration task — so skipping
the last wallet still completes the migration and writes the sentinel. A
skipped wallet stays closed, keeps its legacy protected envelope, and is
registered upstream on a later ordinary unlock via the existing
`handle_wallet_unlocked` -> `bootstrap_wallet_addresses_jit` chokepoint.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
* wip(migration): headless fail-fast, legacy read-only, registration single-flight
INCOMPLETE — DO NOT MERGE. Committed to preserve work across a session
restart; the Codex job producing it was cancelled mid-edit.
State: compiles clean (clippy --all-features --all-targets -D warnings, exit 0),
but the full test gate is RED (exit 101, 1771 passed / 2 failed):
context::wallet_lifecycle::tests::migrated_protected_wallet_blocks_migration_until_password_submission
context::wallet_lifecycle::tests::protected_wallet_registers_upstream_on_unlock_without_restart
Both are tests the legacy-read-only and registration-race changes must rewrite;
the job was cancelled partway through that rewrite. Whoever picks this up must
finish those two and re-run the full gate before trusting any of it.
Intended scope (per review findings + owner directives):
- P1 headless fail-fast: migration must refuse, not block, when a protected
wallet needs a password and no interactive prompt exists (mcp/resolve.rs
drives the same migration with no egui frame loop -> det-cli hung forever).
- P2 legacy DB strictly read-only: never DROP/DELETE/UPDATE the pre-migration
database; write only the new store/vault. Makes a skipped or abandoned
migration cost the user nothing.
- P3 registration race: single-flight per wallet (unlock spawned a
fire-and-forget registration while migration inline-awaited its own for the
same wallet -> spurious RegistrationIncomplete, reproduced as a real failure).
- P4 split MigrationState::is_running(), which silently came to mean
"running OR blocked on a human"; five callers inherited the conflation.
- P5 cross-wallet password bleed (modal state keyed on window title), swallowed
re-encryption failure, unified lock-poisoning policy.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
* fix(shielded): gate fund-moving shielded tasks at the backend chokepoint
`run_shielded_task` had no capability check, so the five state-changing
shielded operations were reachable from any caller that dispatches a
`ShieldedTask` directly. The MCP shielded tools do exactly that, bypassing
the UI gate at `ui/wallets/shielded_tab.rs`. Shielded operations are not
defined on any current network, so `ShieldFromAssetLock` would create an
asset lock committing real L1 funds and then attempt a state transition no
network can settle — stranding the funds and burning the fee.
Enforce `FeatureGate::ShieldedOperations` as the first statement of
`run_shielded_task`, before any wallet or backend access, mirroring the
`RootKeyDerivationRefused` guard in `backend_task/wallet/mod.rs`. The UI
gate stays as defense in depth.
Scope is exactly the five fund movers: `ShieldedTask` carries only
write variants. Shielded init, sync, balance and address reads reach the
coordinator through their own paths and stay ungated, so shielded funds
remain viewable wherever the wallet runs.
Add `TaskError::ShieldedOperationsUnavailable` and a regression test that
dispatches a write task the way an MCP tool does; it is confirmed failing
without the guard, proving the refusal precedes backend access.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dashpay): stop erasing accepted accounts, double-submits and silent declines
Three independent defects in the DashPay contact flow.
Contact-info write silently erased the accepted-account allow-list.
`resolve_accepted_accounts` collapsed four distinct states — no document,
missing privateData, decrypt failure, deserialize failure — into an empty
Vec, which `create_or_update_contact_info` then re-encrypted and wrote back
over the live Platform document. Any present-but-unreadable payload (e.g. a
contact whose privateData was written by another DashPay client) lost its
allow-list irreversibly on the next rename or unhide. Only an absent document
now yields an empty list; a present payload that cannot be read aborts the
write with a typed `DashPayContactInfoRead` error. The test that asserted the
data-losing behaviour is inverted, and the missing/undecodable payload states
get their own regressions.
A failed task released every request guard, allowing a paid double-submit.
`display_task_error` cleared all Accept/Decline/Cancel guards on any error, so
an unrelated concurrent failure re-enabled an in-flight Accept and a second
click bought a second state transition. Failures from the three request actions
now carry their request ID in `DashPayContactRequestActionFailed`, so only the
guard named by the error is released. Guards no longer matched by a result
expire on a timeout instead of being cleared wholesale, so a lost result cannot
strand a row forever.
A declined request reappeared after refresh. `reject_contact_request` logged and
swallowed a failed `dashpay_mark_declined` write and still reported success,
even though that local marker is the only thing that retires the row — Platform
keeps the `contactRequest` document forever. The failure now propagates, matching
the sibling `mark_withdrawn` cancel path.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): report a corrupted wallet envelope as damage, not a wrong password
A password-protected wallet whose at-rest envelope is corrupted (truncated or
otherwise wrong-length) failed the AES-GCM tag check and surfaced as "The
password is incorrect", trapping the user in a retry loop whose only escape was
deleting the wallet. Structural damage is now classified before the AEAD can
mistake it for a bad password.
- decrypt_message takes the caller's known plaintext length and rejects an
impossible ciphertext/tag or salt length as DecryptError::Malformed.
- WalletSeed::open returns the typed EncryptionError instead of a flattened
String, so callers branch on the variant rather than on message text.
- The unlock popup maps Malformed to the same "saved data looks damaged, re-add
it from your recovery phrase" sentence the unprotected path already shows, and
keeps the password hint on the wrong-password branch only.
Finish the two migration lifecycle tests left red at the previous checkpoint.
Both now install a TestPrompt::never(), which panics if asked and so pins the
contract that migration defers to the UI-owned unlock flow instead of driving a
secret prompt itself:
- the protected wallet waits, is then skipped, and data.db is asserted
byte-unchanged, holding the legacy database strictly read-only;
- the unlock path joins the migration's single registration flight
(registration_attempt_count() == 1).
Verified green on the full workspace suite (2023 passed, 0 failed), the
all-features/all-targets lint gate with warnings denied, and the nightly
formatter check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dashpay): preserve saved contact details and keep paid actions guarded
The contactInfo document is written whole, so every writer decides the fate
of the fields it does not edit. Decline, withdraw, unhide and rename each
rebuilt the payload from scratch, erasing the nickname, note and
accepted-account list stored by the user or by another DashPay client.
Replace the implicit `Vec<u32> -> AcceptedAccounts::Replace` coercion, which
made the destructive path the short one, with an explicit `ContactInfoUpdate`
that states field by field what is preserved and what is replaced. Visibility
flips now preserve everything else; only the contact-details form, which owns
the whole form, replaces.
A payload this client cannot read is no longer either silently overwritten or
a permanent dead-end: the write aborts, the user is told, and confirming an
explicit, danger-styled dialog re-runs the write with an overwrite policy, so
a contact with unreadable details can still be unhidden, declined or renamed.
The v0 parser now rejects unknown versions, invalid UTF-8, non-canonical
flags and foreign trailing bytes instead of decoding them as absent details.
Paid request actions (Accept, Decline, Cancel) keep their in-flight guard
across a routine tab switch or refresh, which previously released it and made
the row clickable again while its state transition was still running. An
identity, wallet or network change still clears the guards, since they belong
to the identity being left. Task results reach only the screen that is visible
when they land, so the wall-clock backstop is retained: without it, an action
resolved while the user was on another screen would strand its row with dead
buttons for the rest of the session.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): unlock a cold-booted protected wallet with its correct password
A password-protected wallet hydrates from a secret-free model: the
Tier-2/Protected arm of cold-boot reconstruction carries a placeholder
envelope, because the real secret stays in the vault. The unlock popup
verified the password against that placeholder, so after the first
restart the CORRECT password was reported as damaged data and the owner
was permanently locked out of the wallet.
Verify the password only through the secret chokepoint, which reads the
real stored envelope, and flip the in-memory seed open solely after that
succeeds (`mark_open_after_verification`). The popup maps the resulting
typed error to user copy structurally — wrong password vs damaged vault —
instead of pre-checking the model.
Operation-only unlocks now forget the session seed through an RAII guard,
so an early return or panic in the reconciliation subtask can no longer
strand a plaintext seed in the cache. A migration unlock is operation-only
too: that prompt offers no "keep unlocked" choice, so it must not silently
retain the seed for the session.
Regression cover, both entry points against a real cold boot: the context
API and the unlock popup itself. The popup test fails (correct password →
Pending) if the model pre-check is ever reintroduced.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): block input outside a non-dismissible modal prompt
Removing the progress overlay's pointer sink (so it could not cover a
secret prompt it had triggered) also removed the only barrier in front of
the app: while the storage update paused on the migration password prompt,
clicks still reached the wallet screen behind it.
Give the modal its own barrier instead. A non-dismissible `modal_chrome`
window installs a full-screen pointer sink and registers itself as egui's
modal layer, so every layer beneath it is ignored for interaction while
the window itself — drawn above the sink — stays fully interactive.
Dismissible dialogs keep their existing click-outside behaviour.
The kittest asserts the widget beneath the prompt does NOT register a
click, and that the prompt's own controls remain hittable.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(migration): keep wallet work gated while the storage update awaits a password
`is_running()` had become an alias for `is_executing()`, which reports
`false` while the migration is paused on `AwaitingWalletPasswords`. Every
caller that meant "the storage update has not finished yet" therefore
opened up mid-migration: wallet-touching backend tasks slipped past the
`WalletStorageNotReady` gate and hit a half-migrated vault, the MCP
wait/join logic stopped waiting, and the wallets screen offered
Create/Import CTAs against a wallet list about to be rehydrated.
Replace it with `is_in_progress()` — `Running | AwaitingWalletPasswords` —
and use it at all three sites. `is_executing()` keeps its narrow meaning
for callers that really do mean "a step is running right now".
Covered by a test dispatching an MCP-style wallet task during
`AwaitingWalletPasswords` and asserting `WalletStorageNotReady`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): collect the redundant legacy seed envelope and stop overpromising data removal
Legacy seed-envelope garbage collection, restored for the vault copy only.
Once the current-format secret is durable — the raw seam, a Tier-2 sealed
envelope, or an eager/lazy migration write — the superseded `envelope.v1`
row in the SAME vault is deleted best-effort, so a seed has exactly one
current copy at rest instead of an indefinitely retained duplicate. A
cold-boot scheme probe repeats the sweep after an interrupted run. The
pre-update `data.db` is NOT touched: it stays a read-only recovery
artifact.
Stop promising deletions the app no longer performs. "Remove Wallet" and
"Clear Database" said they erase all local data, while an earlier
version's read-only recovery database — which may still hold wallet
recovery data — stays on disk; the copy now says so. "Clear Platform
Addresses" is disabled rather than pretending to work: its only
implementation wrote to that read-only database.
The two remaining legacy-database writers are signposted as test-only;
neither has a production caller.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): make every passphrase prompt own the interaction surface
The blocking progress overlay yields to ANY passphrase prompt — the gate is
`has_blocking_secret_prompt()`, true for cancellable and non-dismissible prompts
alike — and paints no dimmer, pointer sink, or focus trap while one is up. But
the replacement barrier was wired to dismissability (`blocks_input: !cancellable`),
so the ordinary just-in-time unlock prompt, which is cancellable, installed no
sink at all: pointer and keyboard fell straight through to the panels the overlay
exists to freeze.
Dismissal and input-blocking are orthogonal. `blocks_input` is now unconditional;
`cancellable` still governs only Cancel / X / Escape / click-outside, which read
raw pointer input and are unaffected by the sink.
The two comments asserting the prompt "supplies its own input barrier" described a
precondition the code did not establish; they now describe what it does.
Covered by a kittest that presses a control behind a cancellable prompt while an
overlay is raised (RED before this change: the control activated), plus one
pinning that the prompt still dismisses from its own Cancel button.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(migration): scope an unlocked seed to the storage update, not to one subtask
A wallet unlocked for the storage update has two consumers of the seed it just
promoted: the unlock gesture's own `wallet_unlock_registration` subtask, and the
update's `bootstrap_loaded_wallets()` pass, which re-enters the seed scope for
the very wallet it prompted for. Their lifetimes overlap in an order nobody
controls, yet the seed's lifetime was owned outright by the subtask's RAII guard.
Whichever finished first evicted the seed from under the other — and a cache miss
on a protected scope prompts, so the update raised a background passphrase prompt
for a wallet the user had just unlocked. If the user ticked "keep unlocked" on
that second prompt, it also silently restored the session-long retention the
migration prompt deliberately withholds.
Retention shorter than the session is now enforced by `SecretLease`, a ref-counted
claim at the secret chokepoint: each consumer holds a clone and the seed is
forgotten when the last one drops. The storage update takes its own lease for the
wallets it prompted for (`WalletUnlockRetention::UntilStorageUpdateComplete`) and
releases it on every exit path, so neither consumer can strand the other, and the
unlock still does not outlive the update.
The regression test drives the losing interleaving explicitly: the unlock subtask
is joined to completion first, then the update's pass must resolve the seed from
the session cache with zero prompts, and the seed must be gone once the run's
lease is released.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dashpay): release the request guard when a dispatch is refused pre-dispatch
The storage-update gate rejects every wallet-touching task — `DashPayTask`
included — with a bare `WalletStorageNotReady`, before it reaches
`run_dashpay_task`, the only place that wraps a failure into
`DashPayContactRequestActionFailed { request_id, .. }`. That typed variant is
also the only one `release_request_guard_for_error` matched, so a contact
request's Accept / Decline / Cancel clicked during the first launch after an
upgrade claimed a guard nothing would ever release: the row's buttons went dead
for the full five-minute in-flight timeout, long after the update finished.
A pre-dispatch refusal names no request precisely because nothing ran, which is
exactly the condition under which a blanket release is safe. `clear_in_flight` is
restored for that one match arm only — every other failure still keeps its guard,
so an unrelated error cannot re-enable a row whose paid action may still be in
flight.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(backend): pin ShieldedTask inside the wallet-touching migration gate
The shielded family is refused during a storage update only because it is listed
in `is_wallet_touching`; nothing failed if a refactor dropped that membership.
Sibling of `wallet_task_is_rejected_while_migration_awaits_password`, dispatching
a shielded write while migration awaits passwords.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): state the disabled-tool reason once, as one translation unit
"Clear Platform Addresses" explained its own unavailability twice in the same
row — a tooltip ("...because...") and an italic label ("...while...") — giving a
translator two units for one idea, and drifting on the word that carries the
meaning: the tool is disabled permanently, so "while" is wrong. Keeps the
always-visible label (a tooltip on a disabled control is easy to miss) with the
permanent reading, and drops the tooltip.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: correct the data-deletion promise and record the migration change set
NET-019 still promised to "permanently delete all local data" and that "the
action cannot be undone", which the shipped Clear Database dialog now
contradicts: it discloses that an earlier version's read-only recovery database
stays on the device and may still contain wallet recovery data. The story now
matches the dialog and its sibling WAL-007 — the population it is written for
(clearing a machine before handing it on) is the one it most misleads.
UX-001 described the progress block yielding its pointer sink to a passphrase
prompt but never said the prompt installs its own in its place, reading as if the
click-through hole were still open. It now states the hand-off as an invariant,
for every prompt, dismissible or not — an unwritten invariant is how that hole
was reopened the first time.
CHANGELOG covered only the DashPay change set. Adds the two user-visible ones it
missed: the per-wallet password prompt on the first launch after an upgrade (with
its safe skip path), the read-only recovery database that "Clear Database" and
"Remove Wallet" no longer erase and the developer tool that is disabled as a
result, and the shielded refusal message.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(wallet): warn that SecretLease::lease() refcounts per call, not per scope
Independent verification of the SEC-002 fix (integration composition review)
found that lease() mints a fresh Arc on every call — two unrelated consumers
calling lease(scope) directly get two independent refcounts, so the first to
drop can evict the secret while the second is still relying on it. Not
currently reachable (the one call site correctly clones), but the type can't
enforce the invariant, so the next new consumer would reach for the public
lease() API and silently reintroduce the exact race SEC-002 just closed.
Document the footgun at the point of call rather than leave it undiscoverable.
* test(ui): prove the secret prompt's transition-frame click-through
egui resolves each frame's click at begin_pass against the previous frame's
widget geometry and modal layer. On the frame a passphrase prompt first
renders, the control beneath still existed last frame with no sink and no
modal layer above it, so the click completes on it before modal_chrome
installs the sink — mirroring AppState::update, where the visible screen
renders before render_secret_prompt.
- transition_frame_click_leaks_through_a_newly_activated_prompt: RED repro,
parked #[ignore]; un-ignore once the barrier is installed before the
visible screen renders on the activation frame.
- primed_prompt_blocks_the_same_injected_click_sequence: control (green) —
the identical injected click, with the prompt primed one frame earlier, is
absorbed. Isolates the leak to the transition frame, not the test harness.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(backend): refuse unavailable shielded ops early and scope DashPay gate rejections to one request
Two migration-gate refinements in run_backend_task, both closing bot-review
findings on PR #893.
Shielded pre-check: a shielded fund movement now short-circuits with
ShieldedOperationsUnavailable as the very first thing run_backend_task does —
before ensure_wallet_backend materializes seeds, registers upstream, and binds
Orchard for every loaded wallet just to run an op the app refuses. is_available
is a side-effect-free config read, safe before backend init; the in-handler gate
in run_shielded_task stays as belt-and-suspenders. Shielded ops are unavailable
on every network today, so this pre-check also precedes the migration gate: a
shielded write during a storage update now gets the accurate "not available"
message instead of a misleading "wait for the update".
DashPay guard scoping: the migration gate now tags a rejected contact action
(Accept/Reject/Cancel) with DashPayContactRequestActionFailed carrying its
request ID, so the Identity Hub releases only that request's in-flight guard.
The previous stopgap blanket-cleared every guard on a bare WalletStorageNotReady,
which could re-enable a different contact action's row while its paid state
transition was genuinely still in flight. release_request_guard_for_error drops
the blanket-clear arm; the now-unused clear_in_flight is removed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): drop the transition-frame click when a passphrase prompt activates
egui resolves each frame's click at begin_pass against the previous frame's
widget geometry and modal layer, before update() runs. On the frame a passphrase
prompt first renders, the previous frame had no prompt and no input sink, so a
press-then-release completing now still lands on the control beneath — the modal
installs its sink one frame too late, and reordering the render within the frame
cannot help.
AppState::update now detects the prompt-activation rising edge (covering both the
just-in-time unlock and the migration password prompt, via
has_blocking_secret_prompt) and calls drop_activation_frame_pointer_click, which
clears this frame's pending pointer input before the screen beneath runs. A
widget only reports a click while a Released event is still in input.pointer, so
dropping it strands the leaked click; keyboard input is left intact for the
freshly focused password field, and the sink covers every later frame.
Un-ignores the transition-frame repro (now GREEN) and adds a migration-prompt
sibling; the primed-prompt and yielding-overlay sink tests still pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(ui): pin passphrase activation wiring in the real AppState update loop
The two existing transition-frame repro tests mirror
drop_activation_frame_pointer_click directly in a hand-rolled closure — they
never drive AppState::update(), so the production rising-edge call site in
app.rs was untested: deleting it left the suite green. Add
appstate_jit_prompt_activation_drops_transition_frame_click and
appstate_migration_prompt_activation_drops_transition_frame_click, which
mount a real AppState via build_eframe, activate a prompt through the actual
JIT (test_set_secret_prompt_active) and migration (MigrationStatus) paths,
and assert a click completed on the activation frame does not reach the
welcome screen beneath. Independently confirmed both fail when the app.rs
call site is neutralized and pass when restored.
Also corrects a stale doc comment/assertion in hub_screen.rs left over from
3e69b2fd, which removed the blanket WalletStorageNotReady guard-release arm:
the comment still described a "blanket release... scoped to refusals that
prove nothing is running" that no longer exists in the code.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(wallets): keep dialogs open on trigger clicks
Fix SND-003, WAL-005, and WAL-006 by ignoring outside-click dismissal on each dialog's opening frame.
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
* build(deps): bump platform to PR3968 tip (d18020f5), pulls in the AssetLockProof rehydration fix
Updates dash-sdk / rs-sdk-trusted-context-provider / platform-wallet /
platform-wallet-storage git pins from 93b967f9 to d18020f5
(dashpay/platform#3968 tip), which includes the AssetLockEntryWire fix
for the AssetLockProof deserialize_any bug (dashpay/platform#4133) that
was blocking wallet rehydration on every relaunch once any asset lock
existed.
Adapts to unrelated upstream API drift pulled in by the same bump:
DataContractJsonConversionMethodsV0::to_json(&self, platform_version)
was removed as part of dpp's JSON/Value conversion trait unification
(dashpay/platform#3573, already a known pre-existing lint debt in this
branch). The canonical replacement for "give me this contract's
current wire-format JSON" is
DataContractInSerializationFormat::try_from_platform_versioned(...)
+ serde_json::to_value(...) — updated the 3 affected call sites
(contract_chooser_panel.rs x2, update_contract_screen.rs,
token_creator.rs); from_json usage elsewhere is unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* test(ui): make the opening-click regression test exercise the real guard
opening_click_does_not_immediately_dismiss previously only asserted that
seeding PassphraseModalState with an armed ModalOpeningGuard left the cache
entry readable — a plain data-cache round trip that never called
clicked_outside_window_after_open and could not fail regardless of the
guard's behavior. Rewrite it to simulate an actual outside click via
egui::RawInput and call the real function: the opening click must be
swallowed once, then a later check against the same pending click must
detect it normally. Also drops the internal commit-SHA reference in the
adjacent comment per the "describe present state, not history" convention.
Found by an independent adversarial review of this branch's merge (QA-001,
QA-002).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(contracts): don't panic when contracts can't load on Update Contract screen
UpdateDataContractScreen::new() called app_context.get_contracts().expect(...),
panicking the whole process when the contracts store errors (e.g. an unwired
wallet backend returns Err(WalletBackendNotYetWired)). Degrade gracefully
instead: fall back to an empty contract list and show a calm, actionable
MessageBanner with the underlying error attached via with_details(), matching
the established pattern in document_action_screen.rs and
group_actions_screen.rs.
QA-002. Implemented by Codex Sol, committed by the coordinator (this
sandbox's git metadata for the worktree is read-only, a recurring
environment constraint this session).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(dashpay): accept all integer encodings for contact-request key indices
derive_contact_payment_address() extracted senderKeyIndex/recipientKeyIndex
with a strict match on Value::U32, but network-fetched documents decode
integers as Value::I128, so extraction always failed with "Missing
senderKeyIndex" and DashPay payments could never succeed. Fixed by using the
canonical platform_value helper (to_integer::<u32>()) already used for the
same fields in contact_requests.rs, extracted into a small pure helper
(read_contact_request_key_indices) and unit-tested against I128/U32/I64.
Swept the rest of the DashPay backend for the same strict-match fragility and
converted three more sites the same way: contact_info.rs and contacts.rs
(derivationEncryptionKeyIndex/rootEncryptionKeyIndex) and
auto_accept_handler.rs (accountReference, previously handled with a manual
five-arm match — now the same single helper call).
DPY-006. Implemented by Codex Sol, committed by the coordinator (this
sandbox's git metadata for the worktree is read-only, a recurring
environment constraint this session).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(mcp): hydrate saved wallets before wallet-facing tools read them
ListWalletsTool::invoke (and every other tool that calls resolve::wallet(),
which reads ctx.wallets) ran before any SPV gate wired the wallet backend —
ctx.wallets is only populated inside WalletBackend::new via
AppContext::ensure_wallet_backend, and core_wallets_list deliberately skips
resolve::ensure_spv_synced. A fresh standalone det-cli process therefore
always reported {"wallets":[]} even with wallets already persisted to disk.
Added resolve::ensure_wallets_hydrated(), which wires the backend via
ctx.ensure_wallet_backend() with a throwaway sender — no SPV start, no sync
wait, idempotent on repeat calls — and called it ahead of every resolve::wallet()
call site that wasn't already behind ensure_spv_synced (17 tools across
wallet.rs, identity.rs, and shielded.rs). Updated docs/MCP.md and docs/CLI.md
to describe the new hydrate-on-demand behavior.
Verified with the exact det-cli two-process smoke flow from CLAUDE.md: import
a wallet in one process, list wallets in a fresh process against the same
data dir, confirm it appears with the expected seed hash and alias.
MCP-001. Implemented by Codex Sol, committed by the coordinator (this
sandbox's git metadata for the worktree is read-only, a recurring
environment constraint this session).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(ui): stop the opening click from immediately cancelling confirmation dialogs (IDN-006, TOK-005, TOK-011, TOK-018)
Transfer Funds, token creation/registration, token claiming, and stop-tracking
all rendered their confirmation popup in the same egui frame as the button
click that triggered it. clicked_outside_window() read that still-active
click as a dismissal, so the dialog opened and cancelled itself within the
same frame -- visually indistinguishable from the button being a no-op.
Fixes it the same way passphrase_modal.rs's opening-click bug was fixed:
ConfirmationDialog now carries a ModalOpeningGuard, armed on construction and
consulted via clicked_outside_window_after_open() instead of the raw
outside-click check. The data-contract JSON popup gets its own guard for the
same reason.
Also fixes a compounding bug in the token creator: it rebuilt a brand-new
ConfirmationDialog (and therefore a freshly-armed guard) every single frame
via Option::insert(), which meant the dialog could never observe its own
post-opening frame. Switched to get_or_insert_with() so the dialog persists
across frames once created.
TOK-011 and TOK-018 share the same ConfirmationDialog component, so the fix
covers their reported no-op behavior without separate changes.
Implemented by Codex Sol, committed by the coordinator after independent
review of every hunk and a from-scratch fmt/clippy/test verification pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(ui): wire missing identity navigation and fix stale post-refresh state (IDN-008, IDN-013a, DPN-008, IDN-009)
Navigation gap (IDN-008, IDN-013a, DPN-008): KeysScreen and the "My
usernames" list existed and worked but had no reachable route from Identity
Settings -> Advanced in the current Identity Hub build. Adds "Manage keys"
and "View all usernames" entries. Also corrects an inverted gate on the
Transfer screen's key-info button -- it only appeared when the identity had
*no* transfer key, backwards from the intended "manage the key you have"
flow. Key Protection (IDN-013a) was already fully implemented; it just
needed the same navigation fix to become reachable.
Refresh staleness (IDN-009): "Refresh identity data" fetched fresh state
from the network and persisted it correctly, but the backend task returned
the stale pre-refresh identity to the UI instead of the newly-fetched one,
and the Settings screen's own selected-identity cache only updated when the
identity's ID changed -- never on same-ID refreshes, which is the only kind
a refresh produces. Combined, a refresh could add a new on-chain key and the
UI would still show the old key count indefinitely. Fixed both: the backend
now returns the updated identity, and Settings reconciles same-ID refreshes
instead of only replacing on an ID change.
Implemented by Codex Sol, committed by the coordinator after independent
review of every hunk and a from-scratch fmt/clippy/test verification pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(backend): stop silent hangs and panics in backend tasks (HANG-CLASS, IDN-002, MN-001, DOC-004, TOK-003)
Task-panic watchdog (HANG-CLASS): handle_backend_task/handle_backend_tasks
spawned work via tokio::task::spawn_blocking but dropped the returned
JoinHandle, so a panic inside the spawned closure vanished silently -- the
UI just hung forever with no error. The handle is now kept and awaited by a
managed watcher task; a panic or cancellation surfaces as a new typed
TaskError::BackendTaskFailed with a calm, actionable banner. The raw panic
payload is redacted from diagnostics (BackendTaskJoinError's Debug/Display
only expose task id / cancelled / panicked, never the panic message itself)
to avoid leaking arbitrary panic content into logs.
Network-request timeouts (IDN-002, MN-001, DOC-004, TOK-003): identity
loads (primary, voter, and DPNS-name fetches), document fetches, and token
lookups could all hang indefinitely on a stalled network call with no
feedback. Added a shared await_network_request_with_timeout helper (90s,
NETWORK_REQUEST_TIMEOUT) used at every affected call site, each mapping to
its own typed, actionable TaskError variant.
Token balance refresh needed more care than a plain timeout: the upstream
sync is not safely cancellable -- dropping it mid-flight could leave
is_syncing permanently stuck, trading a hang for silently-disabled sync
forever. Added await_managed_network_request_with_timeout: the request runs
as a detached, task-manager-tracked spawn; only the caller's *wait* on it
times out, so the sync itself always runs to completion even after the UI
gives up on it. A new token_balance_refresh_in_flight flag (RAII guard,
cleared on drop even if the refresh panics) also gives the refresh
single-flight protection so overlapping requests can't race.
Also fixed two more sites with the same forbidden string-match anti-pattern
DOC-004 was originally reported against (matching literal banner text
instead of message type to know when an in-flight fetch failed): the main
Tokens screen's RefreshingStatus and the token-claims screen's FetchStatus
both had the identical fragility and are fixed the same way.
Implemented by Codex Sol, committed by the coordinator after independent
review of every hunk and a from-scratch fmt/clippy/test verification pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* docs(user-stories): drop transient review-ID citation from UX-001
SEC-004 is an internal review-finding ID with no meaning outside the
review artifact that produced it — doesn't belong in a durable spec.
Flagged by Claudius-Maginificent's PR894 review.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(app): stop migration frame race and preserve vote eligibility across migration
A single per-frame migration-state snapshot now backs every input-claim
and rendering decision (has_blocking_secret_prompt, claim_overlay_input,
ProgressOverlay::render_global, MigrationReconciler::update_banner)
instead of each call re-reading live state — closing a window where a
mid-frame migration transition could let the underlying screen consume
input for a frame where a blocking prompt was about to appear.
The periodic scheduled-vote sweep now defers while migration is in
progress instead of running unconditionally and silently skipping votes
whose imported identity isn't loaded yet. On migration completion, a
recovery sweep casts any vote whose normal 120s eligibility window
overlapped the deferred period, so a password prompt left open past that
window no longer permanently drops the vote.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(dashpay): make contact-request decline/cancel idempotent
DashPay decline and cancel each broadcast a paid visibility transition
(contactInfo hide) before writing a local retire marker. A crash, retry,
or a second UI surface between the broadcast and the marker could
re-broadcast the hide and re-pay. Guard the flow so the paid hide runs at
most once per request:
- Add a durable per-request recovery journal (ContactRequestActionPhase)
in the DashPay k/v sidecar, scoped to the acting identity, so a retry
resumes at the last committed phase instead of re-broadcasting.
- Add a request-wide async lock plus a process-local in-flight claim so
concurrent declines/cancels serialize on one paid hide.
- Paginate contactInfo lookup and reuse it for a hidden-state probe so a
corrective unhide only fires when the contact is actually hidden.
- Correlate a panicked paid action back to its request id via
DashPayContactRequestActionFailed so the Hub releases only that guard,
and route contact-request results/errors to a hidden Hub screen.
- Retain paid-action guards across view resets; release only on the
correlated terminal result.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): fail Clear-Database safely and hydrate legacy wallets for MCP
Two wallet-readiness gaps:
- clear_network_database silently no-op'd its wallet-secret, DashPay
sidecar, and shielded cleanup when the wallet backend was not yet wired,
so "Clear Database" could report success while persisted secrets from an
earlier run survived. Require the wired backend up front and return the
dedicated WalletDataClearUnavailable error, leaving all state intact for
a safe retry.
- Standalone/headless MCP never awaited the cold-start legacy-data
migration, so legacy wallets were invisible to wallet reads. Hydrate and
finish the pending migration in ensure_wallets_hydrated, converting a
terminal MigrationState::Failed back into its typed task error.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(app): correlate task results to their originating operation
A backend-task error was routed purely by message type, so a concurrent
task's failure could trip an unrelated screen's in-flight status, and the
token-balance refresh guard could strand forever on a true hang.
- Introduce BackendTaskContext, attributed to each dispatch, and carry it
on TaskResult::{Success,Error}. Add display_backend_task_result /
display_backend_task_error so screens correlate a result to the exact
operation (document query, token-balance refresh, reward-estimate pair)
instead of matching on message text.
- Document, token, and claims screens now clear their in-flight status
only when the failing/completing task matches the pending one; an
unrelated failure no longer clears a genuine refresh banner.
- Suppress a duplicate token-balance refresh only while the first is
pending, and give the hung-refresh guard honest restart guidance.
Composes with the DashPay request-id correlation: forward_backend_task_join_error
now carries both the optional request id and the task context.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): also wipe identity private keys on Clear Database (SEC-001)
Clear Database wiped seeds, single-keys, DashPay overlays, and shielded
state but never removed identity private keys — the identity_key_priv.*
vault entries or the det:identity:* records. Those keys are Tier-1 keyless
(plaintext-recoverable) by default and include masternode voting/owner/
payout keys, so a user who chose to erase all local data still left
fund-control keys recoverable on disk.
The clear-all sweep already fans out over local_identity_ids() to drop
each identity's DashPay overlays; call the existing public helper
delete_local_qualified_identity for each identity in that same loop. It
runs clear_identity_vault_keys (-> IdentityKeyView::delete_all, wiping the
vault key bytes) and purges the identity scope + index (removing the
det:identity:* records), closing both halves of the gap.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): report partial failures when Clear Database can't delete every secret (SEC-002)
forget_wallet_local_state and forget_all_wallets_local logged each failed
per-secret delete but returned success unconditionally, so a failed seed,
single-key, or identity-key delete was still reported to the user as a
completed wipe — leaving recoverable secrets on disk behind a false
"cleared" message.
Accumulate delete failures instead of swallowing them:
- forget_wallet_local_state keeps attempting every step (resilient) but
returns the first failure so a partial wipe is never reported as clean.
- forget_all_wallets_local returns a ClearAllOutcome carrying the upstream
ids to remove plus every delete failure.
- clear_network_database collects those failures and the per-identity
wipe failures, still clears the in-memory maps, then returns the new
typed TaskError::WalletDataClearIncomplete { failed, #[source] first_error }
when anything failed. Its Display tells the user to restart and retry.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(dashpay): accept both decrypt-failure variants in unreadable-private-data test
read_contact_info_private_data decrypts contactInfo privateData with
unauthenticated AES-256-CBC + PKCS7 and a random IV. A wrong-key decrypt
usually fails PKCS7 unpadding (DecryptFailed), but roughly 1 in 256 the
random IV produces valid-looking padding and the garbage plaintext then
fails to parse (DeserializeFailed). Both mean the same thing — the stored
payload is present but unreadable, so the write aborts.
Two tests asserted ONLY DecryptFailed, so they flaked ~0.3% of full-suite
runs (confirmed: 4 failures in 1500 isolated runs before, 0 in 2500
after). Widen both assertions to accept DecryptFailed OR DeserializeFailed;
the product code's abort behavior is unchanged.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(contracts): isolate update_contract_screen degrade test from shared contract state
constructor_degrades_when_contracts_cannot_be_loaded asserts
known_contracts.is_empty(), which only holds when get_contracts() fails:
on success it always returns the pinned system contracts (dpns,
dashpay, ...) and "dashpay" is not in the constructor's excluded set. The
test built a real AppState, which wires the wallet backend asynchronously
inside the test's Tokio runtime, so whether get_contracts() saw a wired
backend (success -> non-empty) or not (error -> empty) raced the
constructor — the flake (green in the integration gate, red in CI).
Construct the screen from a backend-less test_app_context instead: with no
wallet backend wired, get_contracts() deterministically fails and the
constructor degrades to an empty list, which is exactly the path this test
names. Drops the DASH_EVO_DATA_DIR env-var dance and its module-local lock
entirely (0 failures in 60 isolated runs, was intermittently red).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallet): report Clear-Database failure when the identity index can't be listed (SEC-VERIFY-001)
If local_identity_ids() fails, clear_network_database skips every
per-identity key wipe — yet it still returned Ok(()), so every identity's
private keys (incl. masternode voting/owner/payout) could survive behind a
false "cleared" message: the exact false-success class SEC-001/SEC-002
close, gated behind a listing error.
Push the listing error into the failures accumulator so it surfaces as
TaskError::WalletDataClearIncomplete instead of a silent success. The
warn log is kept.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): add Back navigation to the Manage Keys screen (dead-end lockout)
KeysScreen renders a read-only key list pushed onto the screen stack but
returned AppAction::None unconditionally, trapping the user with no way
back to the identity view. Add a Back control in the header row that
returns AppAction::PopScreen, matching the sibling read-only detail
screens (e.g. contact_profile_viewer). A kittest asserts the button
renders and its click pops the screen off the stack.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): show fee estimate and total before sending Dash (SND-005)
The Send Dash screen dispatched a payment with no fee or total shown, so
the user committed without seeing what would leave their balance. Add a
fee summary rendered directly above the Send button:
- Simple mode: estimated network fee, total deducted, and (when the fee
is taken out of the amount, e.g. Core -> Platform) what the recipient
receives. Covers every cleanly-estimable source/destination pair
(Core->Core/Platform/Shielded, Platform->Platform/Core/Shielded,
Identity->Core/Platform/Identity), reusing the same
model::fee_estimation estimators the amount field's "Max" reserve uses
so the two never disagree. Combinations whose fee depends on inputs the
backend selects at send time (identity top-ups, shielded spends) show a
neutral "calculated when you send" note instead of a wrong number.
- Advanced mode: estimated network fee for the count-driven paths
(Core->Core, Platform->Platform), else the same neutral note.
All fee math stays in model::fee_estimation; FeePreview only arranges
already-estimated numbers for display. Pure unit tests cover the on-top
vs deducted-from-amount total/recipient semantics and saturation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dashpay): report the correct cause when Add Contact can't resolve the recipient (NEW-002)
Sending a contact request to a recipient with no DashPay decryption key
raised DashPayError::MissingDecryptionKey, whose message ("Your identity
is missing a decryption key required for contacts") blamed the SENDER —
even though the sender's keys are fine and it is the RECIPIENT
(to_identity) that lacks the key. The Add Contact screen compounded the
error by offering an "Add Decryption Key" button that would add a key to
the sender's own identity, a remedy that cannot fix a recipient-side gap.
Rename the variant to RecipientMissingDecryptionKey and reword it to
correctly attribute the failure to the recipient with an actionable,
jargon-free message. Drop it from requires_user_action() and remove the
misleading self-remedy button — the sender has no key to add; the message
tells them to ask the recipient to finish setting up their profile. Both
error classifiers and their tests are updated to the renamed variant.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: correct SND-005 fee-estimate criterion to match inline pre-send summary
The SND-005 acceptance criterion described the fee estimate as "shown in
confirmation dialog", but the HD-wallet Send Dash screen surfaces it
inline above the Send button (simple and advanced modes) before dispatch.
Reword the criterion to match the implemented behavior.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): guard reusable modal components against opening-frame dismiss (NEW-003)
InfoPopup and SelectionDialog closed themselves on the same frame they
opened: the click that opened the popup lands outside the not-yet-rendered
window rect, so the unguarded clicked_outside_window() check fired true on
the opening frame and dismissed the popup before it was ever visible
(e.g. the token "More Info" popup never appeared).
Both components are value-constructed every frame from consumer-held state,
so a persistent ModalOpeningGuard field cannot survive across frames. Add
clicked_outside_window_after_open_by_id(), which records the last render
pass in egui temp memory keyed by a stable id and skips the outside-click
check on the opening frame — detected as a gap in rendering, so it re-arms
automatically however the popup was previously dismissed, with no teardown.
Fixing this inside the two components fixes every consumer at once
(InfoPopup: 13 call sites; SelectionDialog: no current consumers, so this
is preventive). Unit tests cover the opening-frame skip and the re-arm.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(ui): guard screen-level popups against opening-frame dismiss (NEW-003)
Six screen-level popups used the unguarded clicked_outside_window(), so the
click that opened them was seen as an outside click on the first render
frame and dismissed them before they appeared. Give each a persistent
ModalOpeningGuard field, arm it where the popup's open state is set, and
switch the check to clicked_outside_window_after_open() — mirroring the
existing wallets_screen rename-dialog and receive-dialog pattern.
Sites fixed:
- contracts_documents_screen: "Select Properties" fields dropdown
- dashpay/profile_screen: avatar-URL popup
- identities_screen: edit-alias modal (both open buttons)
- tokens my_tokens: "More Info" token popup and reward-explanation popup
- wallets add_new_wallet_screen: "Fund Wallet" receive popup
- wallets_screen/dialogs: fund-platform-address and mine-blocks dialogs
(the receive dialog in the same file was already guarded)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(dashpay): show payment-history amount in DASH, not raw duffs (NEW-004)
The DashPay payment-history row printed the raw duffs value with a "Dash"
label — a 0.001 DASH payment rendered as "-100000 Dash" — because the
amount (duffs, from DashPayPaymentHistory) was formatted with `{} Dash`
and no unit conversion. Format it with `format_duffs_as_dash` so it reads
"-0.001 DASH". Fixed in both the Pay screen history (the live path) and
the contact-details history (currently unpopulated, fixed defensively);
documented that the `Credits`-aliased field actually holds duffs.
Counterparty label (NEW-004 part 2) is left as scoped follow-up: the
payment history resolves names against saved DashPay contacts only, so a
recipient paid by DPNS username (not a mutual contact) shows
"Unknown (<prefix>)". Resolving it needs a DPNS lookup by identity id or
persisting the send-time name — deeper plumbing than this cached-read
path — so it is marked with a TODO(NEW-004) rather than forced.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallets): confirm single-key removal and refresh asset locks after creation
WAL-007: single-key wallets had a separate Remove handler that called
forget() immediately, bypassing the HD wallet's confirmation state
entirely. Both wallet types now route through the same pending-removal
state feeding the existing danger confirmation modal.
ALK-002: Loaded(empty) was a terminal cache state as reported, but the
actual navigation gap was that PopScreenAndRefresh invokes
refresh_on_arrival(), not refresh(), which is where the cache
invalidation previously lived. The selected wallet's asset-lock cache
entry is now invalidated on root-screen arrival so a freshly created
lock is picked up on the next render.
Cherry-picked from 26937906 onto fix/snd-003-receive-inert. Conflict
resolution: the single-key-remove-button block was refactored into
request_selected_wallet_removal() (theirs); the snd003 branch's
customized HD-removal confirmation message (the earlier-version
read-only recovery-database note) was preserved into that method's HD
branch. The branch's NEW-003 rename-dialog ModalOpeningGuard usage in
this file is untouched.
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallets): restore password-modal focus without leaking background input
NEW-005 (release-blocking): the wallet-unlock / JIT secret password field
could not receive keyboard focus or typed input. `modal_chrome` registered a
separate full-screen "sink" Area as egui's modal layer; because that sink was
a different layer than the `egui::Window` holding the field, the window
resolved *below* the modal layer, so egui's `Memory::allows_interaction`
silently denied the `TextEdit` focus (and clicks on it).
Register the window's OWN layer as the modal layer instead: comparing a layer
against itself is `Equal`, so the modal's fields always resolve at/above the
modal layer and stay focusable, while every lower layer is blocked. The
full-screen sink is retained — moved to `Order::Middle`, strictly below the
`Order::Foreground` window — because it is load-bearing for background input
blocking: egui's `layer_id_at` only redirects a below-modal click to the modal
layer when some interactable area covers that position, so without full-screen
coverage a click landing outside the centered window would fall through to the
app beneath.
Add a kittest regression, passphrase_modal_password_field_focuses_and_blocks_background,
asserting the field takes focus and receives typed text (surfaced via Submit),
the modal layer is the window's own layer (not the sink), and a widget behind
the modal receives none of it. The pre-existing background-blocking and
dismissal kittests continue to pass, confirming the sink still blocks input.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(migration): gate startup migration on a minimum saved-data version
DET ran its startup data migration without checking the on-disk data
version, so migrating from an unsupported (too-old) version failed in
confusing ways. Read settings.database_version before migrating and gate
at both entry points (legacy-settings import and FinishUnwire) before any
sentinel or state is written; the legacy data.db is opened read-only.
Data versions 11..=40 migrate directly (v0.9.3 = 11 = the supported floor,
already migratable). Older data is rejected with an actionable "install
Dash Evo Tool 0.9.3 first" message; newer data fails closed. Fresh installs
(initialize writes v38 before the gate) are never rejected; a corrupt DB
missing the version row fails closed.
Typed errors SavedDataTooOld/SavedDataTooNew and LegacyDataTooOld/
LegacyDataTooNew carry numeric context and #[source] only (no user strings
in variants). Adds src/model/data_migration.rs (pure version classification).
Implemented by Codex (gpt-5.6-sol, high effort); data-safety reviewed
(gate-before-mutation, exact 11..=40 boundaries, fail-fast-no-write,
fresh-install-safe) — 0 blocking findings.
Co-Authored-By: Codex gpt-5.6-sol <noreply@openai.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* test(migration): cover the too-new fail-fast and upper-accept boundary
Marvin flagged two LOW gaps in the startup DB-version gate. This feature is not
GUI-testable, so tests are its primary safety net.
- Add an async `finish_unwire::run` test (too_new_database_version_is_rejected_
before_migration) mirroring the existing too-old test: a version above the
ceiling (41) is rejected before any pass runs, surfacing the typed
SavedDataTooNew / LegacyDataTooNew chain, and NEITHER the completion sentinel
NOR any migration state is written (state stays Idle).
- Add an upper-accept boundary test (max_supported_database_version_is_accepted_
for_direct_migration): MAX_DIRECT_MIGRATION_VERSION (40) — the top of the
accepted 11..=40 range — is accepted, and the first version above it is
rejected as too new. Previously only 11-accept and 41-too-new were pinned; the
top of the accept range was never asserted accepted.
QA-001: document the deliberate headroom on MAX_DIRECT_MIGRATION_VERSION (40)
above DEFAULT_DB_VERSION (38), so data from a slightly newer build (39, 40) still
migrates rather than failing closed.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(settings): add developer-only Wipe Platform Data control (NEW-006)
Wire the previously-orphaned SystemTask::WipePlatformData to a developer-only
button on the Identity Hub Settings tab, gated to Devnet (the backend
wipe_devnet handler only clears devnet identities, tokens, and user contracts).
Guarded by a type-"WIPE"-to-confirm destructive dialog via a new
ConfirmationDialog::require_confirmation_text builder. Implemented by Codex Sol.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* feat(contacts): add View Profile action to Identity Hub contacts (NEW-007 / DPY-005)
Each active-contact row now offers a View Profile action that opens the working
ContactProfileViewerScreen for the selected contact, alongside the existing Pay
action. Reuses the same viewer the legacy DashPay paths use; the orphaned
ContactDetailsScreen is left untouched. Implemented by Codex Sol.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(wallet): fail-closed on identity-key wipe and surface Clear-Database failures (SEC-001, SEC-002)
clear_identity_vault_keys now returns Result and propagates instead of
swallowing vault read/decode/delete errors, so identity removal (Clear
Database, identities screen, masternode detail, migration) reports incomplete
rather than clean when private keys — including masternode voting/owner/payout
keys — cannot be deleted. IdentityKeyView::delete_all attempts every key and
returns the first error instead of short-circuiting. DashPay sidecar/overlay
delete failures in clear_network_database are now accumulated into the failures
list (SEC-002) rather than warn-only. Adds a masternode-removal regression test
that injects a vault-key delete failure. Fixes found by security review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* test: serialize DASH_EVO_DATA_DIR mutation with one shared lock
Replace per-module mutexes guarding DASH_EVO_DATA_DIR with a single crate-wide
lock in a new test_support module. Module-local locks let tests in different
modules race on the process-global env var under parallel execution, causing
intermittent AppState::new failures (e.g. add_token_by_id_screen's
display_task_result test). One shared lock serializes them deterministically.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(ui): give each modal a unique guard id so popups don't dismiss each other (QA-001)
The opening-frame dismiss guard keyed on a single global Id shared by every
InfoPopup and SelectionDialog, so two independent popups on one screen (e.g. the
profile screen's Profile-Guidelines and Avatar-Guidelines info popups) shared
render-history state: closing one via outside-click then opening the other on
the next frame dismissed the second on its own opening frame. Each InfoPopup and
SelectionDialog now takes a caller-provided per-instance Id (mirroring
passphrase_modal), so guards no longer collide. Adds a two-popup regression test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(ui): move Wipe Platform Data beside its sibling network controls (NEW-006)
The developer-only "Wipe Platform Data" control (story NET-011) wipes data
for the whole devnet, but was rendered on the Identity Hub -> Settings tab,
a per-identity screen. Its siblings are network-scoped and live on the
Network Chooser: "Clear {Network} Database" (NET-019) in the "Database
Maintenance" section and "Clear SPV Data" (NET-020).
Move the control into "Database Maintenance", directly after the Clear
Database button, matching that file's danger-button styling and its
existing selected_role.at_least(UserRole::Developer) gating idiom.
Gating is unchanged and stays deliberately narrow: Developer role AND
Devnet. The devnet condition is load-bearing, not cosmetic, because the
backend wipe_devnet() is devnet-scoped
(delete_all_local_qualified_identities_in_devnet / _tokens_in_devnet /
clear_user_contracts).
Gate is now enforced in three places: the render check, a re-check in
show_wipe_platform_data_confirmation that dismisses the dialog if the gate
stops holding (so a dialog opened on Devnet cannot fire after a network
switch), and a fail-closed wipe_platform_data_action.
The type-WIPE confirmation and all user-facing wording are unchanged. Both
unit tests move to ui::network_chooser_screen::tests and now assert the
negative cases in both directions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
* fix(platform): keep cause-less transition results unconfirmed (#897)
A state transition can be accepted for broadcast while the separate result wait fails. Treat SDK broadcast-error envelopes without a structured consensus cause as submitted but unconfirmed, and direct the user to verify completion before retrying.
Co-authored-by: Codex GPT-5 <noreply@openai.com>
* docs(qa): PR892 user-story QA campaign — full retest record (175/175 stories) (#895)
* docs(qa): scaffold PR892 user-story QA campaign checklist
Populated progress.md with all 123 stories from docs/user-stories.md
(112 Implemented to test, 11 Gap pre-marked N/A). Note: source brief
referenced 152 stories incl. UX/IDH/MN categories that don't exist in
the current doc — proceeding with the doc as it actually is.
* docs(qa): confirm PR892 tx-history regression fix; WAL/SND/NET spot checks
Critical result: full quit + cold-boot relaunch on the same data dir now
correctly re-renders transaction history (was the PR892 bug). Verified
with 3 real testnet transactions via the Pasta faucet.
Also: NET-001 (switch networks) PASS, WAL-001/004/010/011/016/023/024
PASS, SND-001 PASS (nav only), SND-003 (Receive button) FAIL — no QR
code or modal appears, reproduced 3x.
* docs(qa): add shared campaign context for delegated per-category agents
* docs(qa): complete remaining WAL user-story QA pass (PR892)
Finishes WAL-002/003/005/006/007/008/012/013/017-020/021/022 a…
… writes Three thepastaclaw/Codex review findings on PR #4117, independently re-verified against this exact code before fixing: - apply_pools()'s conflict guard only ran when the incoming row was typed, so a later untyped (public_key: None) snapshot at an index that already held a persisted typed key skipped the guard and UPSERT_POOL_SQL's unconditional SET nulled out both columns -- irreversibly discarding hardened key material that can't be regenerated from the account xpub alone. The conflict check now always runs, and TYPED_POOL_CONFLICT_SQL treats a NULL incoming key_type as conflicting with any persisted typed row (typed-over-NULL upgrade and same-typed-value idempotent re-writes remain allowed). - The writer persisted each PublicKeyType variant's bytes with no width check, while the reader's check_fixed_width enforced the 33/32/48-byte widths on load -- so a malformed entry committed successfully and durably poisoned the database on the next load. Validate the variant-specific width before the upsert. - load_typed_pool_entries's queries filtered on `key_type IS NOT NULL` only, silently excluding a row that had public_key bytes but a NULL discriminator instead of rejecting it -- contradicting this loader's own documented fail-hard corruption policy. Select on either column being non-NULL and hard-error on mismatched nullability. Regression tests cover all three: an untyped write can't erase a persisted typed key, malformed ECDSA/EdDSA/BLS widths never reach the database, and mismatched nullability in either direction fails the load. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
…rehydration thepastaclaw/Codex review finding on PR #4117, independently re-verified against this exact code before fixing. insert_platform_node_pool_entry() is the shared helper SQLite restore uses to rehydrate a persisted platform-node pool entry, passing through the persisted `used` flag. It only set AddressInfo.used and highest_generated, hardcoding used_at: None regardless of `used` and never advancing pool.used_indices/highest_used. A restored used entry therefore reported used = true per-row while the pool reported last_used_index = -1, and key_wallet's mark_used/mark_index_used both no-op on an already-used row, so nothing could repair it afterward. Mirror the FFI restore path's used_at: Some(0) convention and advance used_indices/highest_used whenever used is true. New regression test restores a used entry and asserts the pool bookkeeping matches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Codex Sol <noreply@openai.com>
PR #851 (out-of-order UTXO spend fix, #649) and PR #818 (address reservation on hand-out) are sibling forks off `dev` and neither is a superset of the other, so pinning to either alone drops the other's fix. Created dashpay/rust-dashcore branch `dash-evo-tool` (0091c4a9) merging PR #909 (out-of-order UTXO spend recorded nowhere — phantom unspent balance, #649 — supersedes #851's stalled draft with a smaller, cleaner fix for the same defect) with PR #818 (address reservation on hand-out), verified clean (auto-merge, no conflicts) with clippy + full key-wallet/ key-wallet-manager test suites green. Re-pin to `dev` directly once both land there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adopt AddressState across wallet, storage, and FFI paths, and restore used_indices and highest_used when rehydrating used platform-node keys. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Reserve each platform payment address atomically when it is handed out so concurrent callers cannot receive the same unfunded address. Co-Authored-By: OpenAI Codex <noreply@openai.com>
… (PR #4117 squash + v4.1-dev + protocol v13 landed upstream during this work) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> # Conflicts: # packages/rs-platform-wallet-storage/src/sqlite/schema/core_pool.rs # packages/rs-platform-wallet-storage/tests/sqlite_core_pool_writer.rs # packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs
…4.1-dev # Conflicts: # packages/rs-platform-wallet-ffi/src/persistence.rs # packages/rs-platform-wallet-storage/src/sqlite/persister.rs # packages/rs-platform-wallet/src/changeset/mod.rs # packages/rs-platform-wallet/src/manager/load.rs
AddressInfo::is_used() collapsed Reserved{at} and Available to the same
`used=0` row, silently discarding the reservation timestamp on every
write. Add a nullable reserved_at column and persist it, clearing it
whenever a row is (or becomes) used so a stale Reserved snapshot can
never resurrect a reservation on an already-used address.
The read/restore path is intentionally left untouched: threading the
restored value into the live wallet requires widening
insert_platform_node_pool_entry's signature in rs-platform-wallet,
tracked separately in #4188 to keep this change scoped to
platform-wallet-storage.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…okkeeping on downgrade Two proven correctness gaps from PR #3968's review: - load_from_persistor (the sole app-launch rehydration entry point) never retried a transient persister error, unlike the sibling registration path which wraps load_persisted() in retry_transient with the documented rationale that a load is an idempotent read. A single transient blip at launch aborted rehydration for every wallet. - insert_platform_node_pool_entry left stale used_indices/highest_used when an index transitioned used=true -> used=false across two calls, violating the same invariant its sibling used->used fix enforces. Also: typed PlatformNodePoolError now propagates through PlatformWalletError instead of being flattened to a string (with matching #Errors doc update), rebuild_provider_key_account gained direct BLS/EdDSA and mismatch-rejection tests, and a duplicate inline now_secs() computation was consolidated to the existing helper. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…er errors, gate asset-lock proof size The FFI boundary collapsed every persister error (transient or fatal) to a single generic code, discarding exactly the retry-classification this PR's PersistenceErrorKind introduces -- so a production Swift/iOS host had no signal to build its own retry loop around a transient rehydration blip. Add ErrorPersisterTransient/ErrorPersisterFatal FFI codes routed through PersistenceError::is_transient(), recursing through PersisterRestore's nested wrapping, and mirror both new codes on the Swift side. Also gate the AssetLockProof bincode decode in asset_lock_manager_recover behind an explicit 16 MiB length check plus a matching decode-budget limit, closing the one bincode decode boundary the RUSTSEC-2025-0141 suppression rationale didn't actually cover -- and correct that rationale to describe the real guarded surface. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… decode, doc corrections - SEC-002: SqlitePersister::open_inner now shares the same parent-directory permission check the secrets vault already enforced (rejects a group/other-writable parent before precreate_secure), closing the one asymmetry between the two on-disk artifacts. - CODE-002: introduce schema::id32() as the single "stored column -> [u8;32]" decoder, carrying both the column name and actual length in one structured WalletStorageError::InvalidWalletIdLength variant, and route every reader through it instead of eight inline, inconsistently-erroring copies. - CODE-003: restore INTEGER-discriminant parity tests for AddressPoolType and PublicKeyType so an added variant fails a test instead of silently hitting a frozen runtime CHECK rejection; migration SQL is untouched. - SEC-004 / PROJ-004: document the vault's shared rekey fault domain and why the version/generation read accessors stay test-only. - PROJ-001/002/003/005: SCHEMA.md and README.md corrected for the real V001 table count (23, including pending_contact_crypto/ignored_senders and the fifth CHECK domain), the V005/V006 migration log entries, the actual load_prekeyed reconstruction path, and the full V001-V006 migration set. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…review-findings # Conflicts: # packages/rs-platform-wallet/src/changeset/traits.rs # packages/rs-platform-wallet/src/wallet/identity/network/payments.rs
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
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.
TL;DR: Adds a durable, embeddable SQLite storage backend for Dash Platform wallet state, so identities, contacts, keys, and balances survive an app restart instead of being lost or re-derived from scratch.
User story
As a Dash Platform wallet developer, I want my wallet's Platform data (identities, contacts, keys, balances, core sync progress) to survive an app/node restart, so that users don't lose data or sit through a full re-sync every time the app starts.
Scenario
Base flow
A Dash Platform wallet app registers identities and contacts, tracks balances and asset locks, and derives Platform payment addresses as the user interacts with it.
Actual behavior
platform-walletdefines the persistence trait (PlatformWalletPersistence) and the manager-sideload_from_persistor()entry point, but ships no production storage backend. Restart the app and its Platform identities and contacts are gone until re-derived, the address-reuse guard resets, and core sync restarts from scratch — there is no on-disk durability, no backup/restore, and no schema-migration path for wallet state.Expected behavior
That state is durably persisted to a local SQLite database (one
.dbfile can hold many wallets), with online backup/restore and automatic schema migration. Restarting the app restores everything seedlessly and signing works immediately post-load — and no private-key material is ever written to the database (signing material stays in the OS keyring or an encrypted vault).Detailed discussion
What was done
Originally split from #3692; rebased directly on
v4.1-dev, which already carries the shared rehydration scaffolding (ClientStartState,load_from_persistor, theWalletType::ExternalSignablemodel) — this PR is the storage half, the net change againstv4.1-devis almost entirely the new crate.Adds
rs-platform-wallet-storage— a self-contained, embeddable SQLite persistence backend implementingPlatformWalletPersistence.Persister & seedless rehydration
SqlitePersister(usable asArc<dyn PlatformWalletPersistence>,Send + Sync, object-safe) with configurable journal / synchronous / flush modes, a retention policy, and auto-backup.load()reconstructs each wallet external-signable from its persisted account manifest (Wallet::new_external_signable, no seed required), then layers the persisted core-state projection — UTXOs, sync watermarks, chainlock, used-address pool depth — viaapply_persisted_core_state. Prekeyed identity/contact joins mean signing works immediately post-load with no key re-sync.Schema & migrations (refinery; additive; version-pinned with golden schema-freeze fingerprints)
__initial— per-wallet tables keyed bywallet_id, nativeFOREIGN KEY … ON DELETE CASCADE; identity-owned tables cascade viaidentities.wallet_id.__address_height_pin— addsplatform_addresses.as_of_height(address double-count fix).__unified(additive, sequenced after V002) —core_address_pool(first-class per-index address-pool rows with ausedflag, giving real per-account UTXO attribution in place of an account-0 approximation),meta_data_versions(per-(wallet_id, domain)monotonic sequence for cache invalidation), andmeta_store_generation(restore-regenerated store token).__invitations(additive, from thev4.1-devmerge) — adds theinvitationstable for DIP-13 DashPay invitations (inviter-side records; no key material stored, the voucher key is HD-re-derivable fromfunding_index).__pool_public_key(additive) — adds nullablepublic_key/key_typecolumns tocore_address_pool, closing the gap where pre-derived platform-node Ed25519 keys were silently dropped on SQLite round-trips (closes rs-platform-wallet-storage: persist provider_key_account_registrations (BLS/EdDSA provider-key accounts) #4113).__pool_reserved_at(additive) — adds a nullablereserved_atcolumn tocore_address_pool, persistingAddressState::Reserved's timestamp (rust-dashcore PR fix(rs-dpp): Remove overflows #818) instead of silently collapsing it to the same row shape asAvailable. Write-only for now — theON CONFLICTclears it whenever a row is/becomesused, so a staleReservedsnapshot can never resurrect a reservation on an already-used address. The read/restore path deliberately isn't wired to consume it yet; see "Deferred" below.Trust boundary & robustness (the
.dbis untrusted input at load)load(): any row that fails to decode, or an out-of-rangewallet_id, aborts the whole call with a typedWalletStorageError— no silent per-row skip, no partialOk.SIZE_LIMIT_BYTES) cap on KV values and BLOB decode, per-columnlength()pre-read gates before materialization, a bincode decode bounded by aLimitconfig that rejects trailing bytes, and a 32 MiBSQLITE_LIMIT_LENGTHconnection backstop.Secrets
SecretStore/EncryptedFileStore: Argon2id KDF + XChaCha20-Poly1305 AEAD envelope,zeroized, overkeyring-core), so signing material never touches the wallet.db.Changes outside the storage crate (kept as narrow as the underlying fixes allow — the design intent is still to leave
platform-walletuntouched wherever the storage crate alone can carry a change)platform-wallet: two real fixes, both forced by shared logic the storage crate calls into rather than owning itself —insert_platform_node_pool_entry(the helper SQLite-restore, FFI-restore, and plain key registration all share) now correctly restoresused_indices/highest_usedpool bookkeeping for an already-used platform-node key on rehydration (previously only a regression test existed for this; the production code was unfixed — closes a real PR fix(platform-wallet-storage): persist provider key accounts and platform-node public keys (closes #4113) #4117 review finding), andnext_unused_receive_addressnow reserves the address it hands out (AddressPool::next_unused_and_reserve) instead of leaving it unreserved, closing a hand-out race now that upstreamkey_walletsupports address reservation. Otherwise: doc-comment only (new_watch_only→new_external_signable).swift-sdk: the Platform-wallet load FFI consumer reconciled to the shipped 1-argplatform_wallet_manager_load_from_persistorcontract; one real fix inSendTransactionView(stop funding core-to-core sends from a Platform-Payment index); the rest are doc-comment renames.rust-dashcorepinned to adash-evo-toolintegration branch (dashpay/rust-dashcore, commit0091c4a9) combining PR chore(release): update changelog and bump version to 0.24.0-dev.18 #909 (out-of-order UTXO spend fix, feat(wasm-dpp): implement asset lock proof bindings #649 — supersedes the previously-referenced feat(rs-dpp): migrate fees from js-dpp v0.24 #851 draft, which stalled) with PR fix(rs-dpp): Remove overflows #818 (address reservation on hand-out, the APInext_unused_receive_addressabove now uses). Still an interim state: neither upstream PR has merged todevyet, so the pin points at a branch, not a merged/tagged commit — re-pin todevdirectly once both land. Root.cargo/audit.tomlacknowledgesRUSTSEC-2025-0141(bincode unmaintained — an informational advisory, mitigated by the size caps + fail-hardload()above).Review remediation (independent grumpy-review: 6 specialist passes, 31 raw findings consolidated to 22 — 0 CRITICAL, 1 HIGH, 5 MEDIUM, 16 LOW)
platform-wallet-storage: parent-directory permission gate (rejects a group/other-writable parent dir) shared by the SQLite persister and the encrypted secret file store; a sharedid32()decode helper replacing 7 duplicated inline 32-byte-ID decodes; restoredAddressPoolType/PublicKeyTypeinteger-discriminant parity tests;SCHEMA.md/README.mddoc-drift corrections (table count, migration-log rows, rekey fault-domain note).platform-wallet:load_from_persistornow retries a transient persister failure during startup instead of failing rehydration outright;insert_platform_node_pool_entryclears staleused_indices/highest_usedbookkeeping when a pool entry downgrades from used;populate_platform_node_poolpropagates a typed error instead of stringly-typed; a hand-rolled timestamp computation replaced with the sharednow_secs()helper.platform-wallet-ffi: dedicated FFI result codes distinguishing transient vs. fatal persister errors (mirrored in the SwiftPlatformWalletErrorenum); a 16 MiB size gate on asset-lock proof bytes before bincode decodes them.cargo fmt/clippy -D warnings/full test suite re-run per crate, specific new test names confirmed passing in the raw log.rust-dashcorebranch pin, the address-reservation lifecycle gap) are already covered above — see "Infra" and "Deferred" — and intentionally left as documented follow-up rather than duplicated here.Deferred (TODO-marked, no regression)
wallet_id) — tracked in feat(platform-wallet): manifest integrity checksum (Risk-6/R12.5 follow-up) #3992.reserved_atcorrectly, but nothing releases or sweeps a stale reservation, and nothing threads the restored value back into the live wallet (that needs a signature change inrs-platform-wallet'sinsert_platform_node_pool_entry, deliberately kept out of this storage-only PR). rust-dashcore already ships the release/sweep API end to end (AddressPool/ManagedCoreFundsAccount/ManagedWalletInfo), it's just not called from anywhere yet. Tracked in Address-reservation lifecycle is half-wired: no release/sweep, no FFI representation #4188.Test-only fast KDF for
SecretStore(closes #4111)dash-evo-tool'swallet_backendtests) drove realSecretStore::{set,get,set_secret,get_secret,reprotect}flows and paid a production-strength Argon2id derivation (64 MiB) on every call — individually 5-23s, with no way to opt out.SecretStore::file_mock/EncryptedFileStore::open_mock, gated behind#[cfg(any(test, feature = "test-util"))](newtest-utilCargo feature). A mock-constructed store derives at the enforced floor params instead of the shipped default — the fastest configurationenforce_boundsstill accepts — for both the vault-unlock derivation and the per-secret Tier-2 wrap, with no change to any public method signature. A caller swapsfile→file_mockat construction; every subsequent call is transparently fast.KdfParamsstayspub(crate)); the fix is entirely crate-side so no downstream consumer implements its own fast-KDF logic. A single sharedKdfParams::floor_target()helper is the crate's only definition of "fastest legal Argon2id params" — the three previously-duplicated private#[cfg(test)]copies were deleted and their ~31 call sites rewritten against it.cfg!(debug_assertions)-based runtime guard (a runtime value, present in every profile — unlikedebug_assert!) lives onfloor_target()itself, the single choke point every caller (mock constructors and internal tests alike) goes through. Iftest-utilever leaks into a release build via feature unification, the guard panics loudly instead of silently handing back weak crypto;open_mockevaluates it before the passphrase check so a blank passphrase can't mask the panic.argon2is the only workspace crate consumer of theargon2dependency, so added it to the rootCargo.toml's existing[profile.dev.package.*] opt-level = 3list (alongsidehalo2_proofs/orchard/pasta_curves/etc.) — narrow in practice despite being a workspace-level stanza, since nothing else in the tree compilesargon2. Composes multiplicatively with the mock-KDF floor params: measured 149.19s → 13.51s (~11x) onplatform-wallet-storage's 266 KDF-heavy lib unit tests.Testing
cargo clippy --package platform-wallet --package platform-wallet-storage --package platform-wallet-ffi --package rs-unified-sdk-ffi --all-features --locked -- --no-deps -D warnings— clean (matches this repo's CI invocation exactly).cargo test --package platform-wallet --package platform-wallet-storage --package platform-wallet-ffi --all-features— 1513 passed / 0 failed, current as of the latestv4.1-devmerge and the review-remediation fixes above.Swift SDK build) — FFI symbols were matched by hand against the Rustextern "C"surface.rust-dashcorepin above, and two data-durability edge cases (identitiestable missing a uniqueness constraint on(wallet_id, identity_index), andload_from_persistor's failure path not being recoverable by the shipped Swift reference caller).Breaking changes
None in this PR's net diff. The storage crate is purely additive; the
platform-wallettouches are two bug fixes (pool-bookkeeping restore, address-reservation hand-out — see above) plus doc-comment renames, none changing a public signature;swift-sdkis comment renames plus one localized send-funding fix. (TheWalletType::ExternalSignablemodel this crate consumes already lives inv4.1-dev.)Checklist
For repository code-owners and collaborators only
Prior work
key-walletPRs (UTXO-spend fix, address reservation) this PR's interimdash-evo-toolpin combines.reserved_atfix (V006) deliberately leaves for later.Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes