fix(dashpay): preserve paid-action guards across identity switch and paginate contact info - #893
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
⛔ Blockers found — Sonnet deferred (commit 2c69239) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Three in-scope blocking issues remain at the exact head. Unsupported shielded tasks initialize wallet storage before their capability check, the migration gate loses DashPay request correlation, and newly activated passphrase prompts install their input barrier too late to prevent first-frame click-through.
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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/mod.rs`:
- [BLOCKING] src/backend_task/mod.rs:587-599: Reject unavailable shielded tasks before wallet initialization
A dispatched `ShieldedTask` reaches `ensure_wallet_backend()` before `run_shielded_task()` checks `FeatureGate::ShieldedOperations`. On a cold context, initialization opens and hydrates wallet storage, then `bootstrap_loaded_wallets()` resolves every open wallet seed and can register wallets, bind Orchard keys, and register contact accounts. The capability is unavailable on every current protocol version, so unsupported requests can still cross the wallet and secret boundary or return a terminal storage-open error before the intended refusal. Check shielded availability before this generic initialization path.
- [BLOCKING] src/backend_task/mod.rs:607-613: Preserve DashPay request identity at the migration gate
The migration gate returns a bare `WalletStorageNotReady` before `run_dashpay_task()` can attach the request ID for Accept, Reject, or Cancel. The legacy contact-request screen therefore cannot match the failure and leaves its request state and loading flag set. The Identity Hub instead clears every paid-action guard for this error, but backend tasks run concurrently and migration publishes `Running` asynchronously: request A can still be executing when request B is rejected by this gate, causing B's error to release A's guard and permit a duplicate paid submission. Wrap the gate error with the rejected DashPay action's request ID so each screen retires only that action.
In `src/app.rs`:
- [BLOCKING] src/app.rs:1752-1778: Install passphrase input barriers before rendering the screen
When a queued secret prompt becomes active, `claim_overlay_input()` deliberately yields, the underlying screen processes the frame's input, and only afterward does `render_secret_prompt()` install the modal sink. Migration password prompts follow the same sequence because `is_prompting()` yields before `update_password_prompt()` renders at line 1801. In egui 0.35, `set_modal_layer()` records the modal for the current frame while widget interaction checks the modal layer carried from the previous frame, so a sink installed after an underlying button has returned `clicked()` cannot revoke that action. Preinstall the prompt's modal barrier before invoking the visible screen; the current kittest primes the modal for a frame before clicking and therefore does not cover this transition frame.
|
@thepastaclaw both blocking findings from this review are fixed in Shielded pre-check ordering ( DashPay gate request-identity scoping ( On severity: both independently re-derived as LOW, not Blocking:
Both fixes were independently mutation-tested (revert → RED with the exact regression symptom; restore → GREEN) by an agent that didn't write them, plus a full |
|
Thanks — I see the shielded and DashPay remediation in |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Current head fixes all three previously indexed blockers. Two in-scope DashPay blockers remain: identity switching can release a paid-action guard while its task is still running, and contact-info preservation examines only the first 100 documents. Diff and formatting checks passed; focused test execution remains unavailable because the required Tenderdash download was declined.
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 (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/state/contacts_view.rs`:
- [BLOCKING] src/ui/state/contacts_view.rs:97-100: Identity switching clears guards for paid actions that are still running
`reset_for_identity_change()` clears every paid-action guard, but switching identity, wallet, or network does not cancel tasks already spawned by `AppState`. A user can start Accept, Decline, or Cancel for identity A, switch away and back while that task is running, reload the pending request, and dispatch it again because `begin_request()` no longer sees the original guard. This defeats the PR's per-request double-submit protection and can submit two paid state transitions. Retain guards independently of the selected view and key them by network, acting identity, and request ID; release only the guard named by its matching result, error, or timeout.
In `src/backend_task/dashpay/contact_info.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:400-407: Preserve silently fails for contact records beyond the first page
The owner-scoped lookup stops after 100 documents and does not paginate. `found_existing_doc` controls both the `Preserve` input at lines 473-474 and the replace-versus-create branch at line 528, so a target outside that page is treated as absent: preserved fields are resolved from empty data and the code attempts to create another document. The contract's unique owner/root/derivation index may reject that create when the truncated high-water mark collides; if the selected coordinate is free, a second document with empty nickname, note, and accepted-account data can be created. Either outcome violates this PR's contact-metadata preservation guarantee. Fetch every page in deterministic order and compute the derivation high-water mark across all owned documents before concluding that the target is absent.
|
@thepastaclaw fixed and independently verified.
Independently re-derived severity: LOW (risk 0.51 / impact 0.15 / scope 0.18, OWASP-normalized) — real and directly relevant given this PR's own contact-metadata preservation guarantee, but gated behind an identity owning 100+ established Fixed in the same commit as the identity-switch guard finding above: |
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 2760535, both carried-forward prior findings are fixed, so no prior blockers remain. The latest remediation delta introduces no new finding, but the cumulative PR range still contains three blocking legacy DashPay lifecycle and persistence defects plus one partial-success handling suggestion. Formatting and cumulative/latest diff checks passed; focused tests could not run because approval to download the pinned Tenderdash source was declined.
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 (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/dashpay/contact_requests.rs`:
- [BLOCKING] src/ui/dashpay/contact_requests.rs:160-163: Identity switching re-enables an unfinished paid request
`set_selected_identity()` clears `request_in_flight` and `request_task_in_flight`, but changing identities does not cancel the backend Accept or Reject task already spawned by `AppState`. Switching away and back reloads the still-pending request with active buttons, so it can dispatch a second paid state transition while the first remains in progress. Preserve live guards independently of the selected identity and release them only for their matching task outcome or a deliberate timeout.
- [BLOCKING] src/ui/dashpay/contact_requests.rs:955-971: Navigating away can strand the paid-action guard
This guard is released only when this persistent Contacts root screen receives the matching result. `AppState` sends ordinary task results exclusively to `visible_screen_mut()`, so navigating to My Profile, another root screen, or a detail screen before completion delivers the result elsewhere. Returning to Requests leaves `request_in_flight` set and `loading` true, and this implementation has no timeout; the spinner remains until an identity switch or restart. Move completion tracking to a task-scoped owner that survives navigation, or route correlated outcomes back to the originating screen.
In `src/ui/dashpay/contact_details.rs`:
- [BLOCKING] src/ui/dashpay/contact_details.rs:666-671: Navigating away drops the local contact-info commit
The PR moved the local nickname, note, and hidden-state write into `commit_pending_update()`, which runs only when this screen receives the successful Platform result. The loading view still exposes Back and global navigation, so the screen can be popped before completion; because results go only to the currently visible screen, the confirmed Platform write then never updates the WalletBackend private-info sidecar. Reopening the detail or using an offline view reads the stale sidecar until a network refresh happens. Commit the sidecar in the backend success path or another task-scoped owner that survives navigation.
In `src/backend_task/dashpay/contact_requests.rs`:
- [SUGGESTION] src/backend_task/dashpay/contact_requests.rs:898-899: Sidecar failure loses the paid action's partial-success state
`create_or_update_contact_info()` has already confirmed the paid Platform replacement before `mark_declined()` can fail. The resulting request-scoped error releases the UI guard, but without the local marker the immutable request appears again on reload; retrying performs another paid replacement before retrying the same marker. Cancellation has the equivalent split-brain path at lines 964-973. Represent this as a partial-completion outcome and retry only the idempotent marker through a durable marker-only task instead of reopening the paid action.
| fn display_task_result(&mut self, result: BackendTaskSuccessResult) { | ||
| self.loading = false; | ||
| let completed_request = match &result { | ||
| BackendTaskSuccessResult::DashPayContactRequestAccepted(request_id) | ||
| | BackendTaskSuccessResult::DashPayContactRequestRejected(request_id) => { | ||
| self.request_in_flight.as_ref() == Some(request_id) | ||
| } | ||
| BackendTaskSuccessResult::DashPayContactAlreadyEstablished { request_id, .. } => { | ||
| self.request_in_flight.as_ref() == Some(request_id) | ||
| } | ||
| _ => false, | ||
| }; | ||
| if completed_request { | ||
| self.request_in_flight = None; | ||
| self.request_task_in_flight = None; | ||
| } | ||
| if self.request_in_flight.is_none() { | ||
| self.loading = false; |
There was a problem hiding this comment.
🔴 Blocking: Navigating away can strand the paid-action guard
This guard is released only when this persistent Contacts root screen receives the matching result. AppState sends ordinary task results exclusively to visible_screen_mut(), so navigating to My Profile, another root screen, or a detail screen before completion delivers the result elsewhere. Returning to Requests leaves request_in_flight set and loading true, and this implementation has no timeout; the spinner remains until an identity switch or restart. Move completion tracking to a task-scoped owner that survives navigation, or route correlated outcomes back to the originating screen.
source: ['codex']
There was a problem hiding this comment.
Independently verified: still live, but out of this PR's scope — not fixed here.
This PR was rebased down to a single commit (f9d641f5) touching only src/backend_task/dashpay/contact_info.rs, src/ui/identity/hub_screen.rs, and src/ui/state/contacts_view.rs. src/ui/dashpay/contact_requests.rs is no longer in the diff — it reached the base via #894's squash-merge, so this is pre-existing base code rather than something this PR introduces or touches.
Confirming your diagnosis against current code rather than closing it as merely out-of-scope:
0fda1f67addedroute_contact_request_result_to_hidden_hub/route_contact_request_error_to_hidden_hub(app.rs:1625-1647), but that forwarding is Hub-specific — it only targetsmain_screens[RootScreenIdentityHub], neverScreen::DashPayScreen. The catch-all fallback (app.rs:1926-1931) still routes strictly tovisible_screen_mut().ContactRequests::display_task_result/display_task_error(contact_requests.rs:930-990) therefore still clearrequest_in_flight/loadingonly when the result lands on that exact instance, and this widget has no timeout.- The path is genuinely reachable, not dead code: Identity Hub → Settings → "Change photo" (
settings.rs:250-258) → Contacts tab (dashpay_subscreen_chooser_panel.rs:29) →RootScreenDashPayContacts.left_panel.rs:19-25documents that the Dashpay entries are hidden from nav but deliberately kept reachable.
Worth noting the modern Hub analog this PR does touch is engineered correctly for exactly this: ContactsState::reset_for_identity_change retains live guards and prunes only past REQUEST_IN_FLIGHT_TIMEOUT (contacts_view.rs:18-20, 81-99). The legacy widget has neither routing coverage nor a timeout.
Independently re-derived severity: LOW (risk 0.4 / impact 0.3 / scope 0.15) rather than Blocking. It is reachable in a few clicks, but the realistic worst case is a stuck spinner and disabled buttons on one de-emphasised legacy screen, self-healing on identity switch or restart — no double-pay (the backend try_claim_contact_request_action chokepoint blocks that independently of any UI flag) and no data loss.
Tracked as a follow-up against the legacy widget rather than fixed here, to keep this PR at the one commit that is actually its own.
There was a problem hiding this comment.
Resolved in this update — Navigating away can strand the paid-action guard no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| BackendTaskSuccessResult::DashPayContactInfoUpdated { | ||
| identity, | ||
| contact_id, | ||
| } => { | ||
| if identity == self.identity.identity.id() && contact_id == self.contact_id { | ||
| self.commit_pending_update(); |
There was a problem hiding this comment.
🔴 Blocking: Navigating away drops the local contact-info commit
The PR moved the local nickname, note, and hidden-state write into commit_pending_update(), which runs only when this screen receives the successful Platform result. The loading view still exposes Back and global navigation, so the screen can be popped before completion; because results go only to the currently visible screen, the confirmed Platform write then never updates the WalletBackend private-info sidecar. Reopening the detail or using an offline view reads the stale sidecar until a network refresh happens. Commit the sidecar in the backend success path or another task-scoped owner that survives navigation.
source: ['codex']
There was a problem hiding this comment.
Independently verified: the mechanism you describe is still present in the code, but the screen containing it is currently unreachable, and the file is out of this PR's scope — not fixed here.
Scope: this PR was rebased to a single commit (f9d641f5) touching only src/backend_task/dashpay/contact_info.rs, src/ui/identity/hub_screen.rs, and src/ui/state/contacts_view.rs. src/ui/dashpay/contact_details.rs reached the base via #894's squash-merge.
The bug itself is exactly as you describe, unchanged:
commit_pending_update()(contact_details.rs:208-247) remains the sole writer of the WalletBackend private-info sidecar (persist_contact_private_info,ui/dashpay/mod.rs:33-48), called only fromdisplay_task_result()onDashPayContactInfoUpdated(:673-684).- The backend task performs only the Platform document write and returns
DashPayContactInfoUpdated— no sidecar call anywhere increate_or_update_contact_info(contact_info.rs:523-711), including after this PR's rewrite of it. - "Back" is rendered unconditionally (
contact_details.rs:267-268), not gated onself.loading. - I checked
IdentityHubScreen::display_task_result'sDashPayContactInfoUpdatedarm (hub_screen.rs:559-575) as a possible alternate commit path: it only acts whenpending_contact_info_tasks.remove(&key).is_some(), never callspersist_contact_private_info, and is a no-op for a key it did not register. There is no backend-side or Hub-side fallback commit.
What changes the priority: a repo-wide search for DashPayContactDetails finds ScreenType::DashPayContactDetails defined and pattern-matched only inside src/ui/mod.rs's own enum/match machinery — zero call sites construct it, in production UI, tests, or MCP/CLI. Contact clicks route to DashPayContactProfileViewer instead (contacts_list.rs:836-837, contacts.rs:520-526). So the screen compiles and is exhaustively matched, but nothing in the shipped app can navigate to it.
Severity as shipped: INFO (risk ≈ 0, scope ≈ 0 — unreachable). As latent code it is a real landmine: restoring navigation to this screen, or reusing the screen-scoped-commit pattern, reactivates the bug with no other code change. Note contact_profile_viewer.rs:144 independently calls persist_contact_private_info — worth a look under the same lens, though outside what I verified here.
Tracked as a follow-up (fix the pattern, or delete the unreachable screen) rather than fixed in this PR.
There was a problem hiding this comment.
Resolved in this update — Navigating away drops the local contact-info commit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
* 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…
Keep live paid-action guards across identity and context changes while releasing abandoned confirmations and pruning timed-out entries. Fetch every owner-scoped contactInfo page before selecting update/create metadata. Reapplied onto the post-#894 base: retains the newer shared contact-info lookup and key-caching structure, moving full pagination ahead of metadata selection rather than returning at the first match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
2760535 to
f9d641f
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head f9d641f, two carried-forward blockers remain: hidden legacy Requests screens miss correlated completions, and popped contact-detail screens lose their local sidecar commit. The latest delta adds one blocker in the full contact-info scan; repeated protected-wallet prompts and failures from later invalid derivation indices are both valid consequences of the same loop and are combined below. The shared backend claim and durable action phases fix the other two prior findings.
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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 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 `src/backend_task/dashpay/contact_info.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:467-482: The full scan repeatedly unlocks the wallet and can fail after a match
The new full-owner scan invokes `derive_contact_info_keys()` for every valid document, including every document after `existing` has been populated. Each call enters `SecretAccess::with_secret`; with the default `RememberPolicy::None`, a protected wallet is decrypted only for that call, so an identity with 101 contactInfo documents can require 101 password submissions for one save, decline, cancel, or unhide. Continuing after a match also propagates unrelated derivation failures: the contract permits any non-negative integer, but hardened BIP32 rejects indices at or above 2^31, so a later document can abort an already-successful lookup. Compute the public high-water mark without key access, hold one secret session while deriving candidate keys, and stop deriving once the target is found.
Base advanced while this PR was open. No overlap: #900 touched model/fee_estimation.rs, ui/wallets/send_screen.rs and wallet_backend/snapshot.rs; this branch touches the DashPay contact-info and contacts-view paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
…per document Review feedback on the pagination fix. Scanning every owner-scoped contactInfo page fixed silent preservation loss past the first page, but the scan derived keys for every document — before checking whether the target had already been found, and through a fresh secret access each time. With the default RememberPolicy::None a protected wallet is decrypted per call, so an identity with 101 contactInfo documents could demand 101 password submissions for one save, decline, cancel, or unhide. Paginating made that unbounded; it was previously capped at a single page. The scan is now two passes. The first computes the derivation high-water mark from public document properties alone — no key access. The second derives candidate keys inside a single secret session and returns on the first match, so nothing is derived past the target. The session callback is FnOnce, which puts the one-session invariant in the type rather than in a comment. A derivation failure on one document no longer aborts the scan. The contract permits any non-negative integer index while hardened BIP32 rejects indices at or above 2^31, so a single such document previously discarded an already-successful lookup; those documents are now skipped. They still advance the high-water mark — the document occupies that coordinate on Platform whether or not a key can be derived for it — while indices that are missing, wrong-typed, negative, or outside u32 do not, since neither ContactInfoLookup nor the derivation API can represent them. Regression tests pin all three behaviours and were confirmed failing before the fix: one secret session across 101 candidate documents, no derivation after the match, and a late hardened index neither aborting the lookup nor corrupting the high-water mark. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018m6b2TRP1e6S3uQHcaq3Cq
e843dba
into
docs/platform-wallet-migration-design
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 2c69239, focused tests could not compile because the build requires an unavailable Tenderdash download. Carried-forward prior findings: the identity-switch and partial-success defects are fixed; the legacy spinner defect remains real but is intentionally deferred outside the current three-file PR diff; the Contact Details defect is outdated because that screen is unreachable. New findings in the latest delta: the mutable pagination cursor and underivable high-water allocation are blocking defects.
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 (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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 `src/backend_task/dashpay/contact_info.rs`:
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:521-525: Mutable pagination order can skip contact records
The scan orders documents by `$updatedAt`, then continues with the last document ID from each page. Platform resolves that ID to the document's current contents before constructing the next index query. If another contact-info operation replaces the boundary document between requests, its updated timestamp moves it later in the index, so the next page starts from the new position and skips documents between the old and new positions. Operations for different contacts or request IDs are not serialized per owner. An omitted target is therefore treated as absent, causing creation with incomplete allocation metadata; this can create a duplicate contact record or fail on an index collision. Use a stable owner/key index for pagination or serialize the complete scan-and-write operation per owner.
- [BLOCKING] src/backend_task/dashpay/contact_info.rs:486-489: Skipped invalid index still poisons new-contact allocation
The DashPay contract accepts every non-negative integer derivation index, while hardened BIP32 accepts only values below 2^31. The lookup now skips a document whose index cannot be derived, but this calculation still advances `next_derivation_index` beyond that document. When no existing document matches the requested contact, `create_or_update_contact_info` passes the resulting underivable value to key derivation and fails. One contract-valid high index therefore prevents this identity from creating contact information for every new contact. Track occupied derivable indices and choose an unused value below 2^31 instead of advancing beyond an index the key API cannot represent.
| query = query.with_order_by(OrderClause { | ||
| field: "$updatedAt".to_string(), | ||
| ascending: true, | ||
| }); | ||
| let existing_docs = fetch_all_contact_info_documents(query, |page_query| async move { |
There was a problem hiding this comment.
🔴 Blocking: Mutable pagination order can skip contact records
The scan orders documents by $updatedAt, then continues with the last document ID from each page. Platform resolves that ID to the document's current contents before constructing the next index query. If another contact-info operation replaces the boundary document between requests, its updated timestamp moves it later in the index, so the next page starts from the new position and skips documents between the old and new positions. Operations for different contacts or request IDs are not serialized per owner. An omitted target is therefore treated as absent, causing creation with incomplete allocation metadata; this can create a duplicate contact record or fail on an index collision. Use a stable owner/key index for pagination or serialize the complete scan-and-write operation per owner.
source: ['codex']
| let next_derivation_index = documents | ||
| .iter() | ||
| .filter_map(contact_info_derivation_index) | ||
| .fold(0u32, |next, index| next.max(index.saturating_add(1))); |
There was a problem hiding this comment.
🔴 Blocking: Skipped invalid index still poisons new-contact allocation
The DashPay contract accepts every non-negative integer derivation index, while hardened BIP32 accepts only values below 2^31. The lookup now skips a document whose index cannot be derived, but this calculation still advances next_derivation_index beyond that document. When no existing document matches the requested contact, create_or_update_contact_info passes the resulting underivable value to key derivation and fails. One contract-valid high index therefore prevents this identity from creating contact information for every new contact. Track occupied derivable indices and choose an unused value below 2^31 instead of advancing beyond an index the key API cannot represent.
source: ['codex']
Why this PR exists
Problem: Two DashPay defects remain open in
ContactsStateandcreate_or_update_contact_info. A paid-action double-submit guard is discarded whenever the user switches identity, wallet, or network — even while the guarded backend task is still running. Separately, the owner-scopedcontactInfolookup reads only the first page of results despite a comment claiming it reads all of them, so metadata selection is made against an incomplete set.What breaks without it:
reset_for_identity_changewipes every double-submit guard, including the live one → the action can be submitted again while the first is still running, and the user pays for it twice.contactInfodocuments beyond the first page. Editing a contact whose record sits past that page → the lookup never sees the existing document → the code either writes a fresh one (duplicating that contact's record) or proceeds without the existing field values, silently dropping the contact's accepted-accounts allow-list, nickname, and note.Blocking relationship: Stacked atop fix: rebase SND-003 onto PR893 + QA-campaign fix batches #894, which was squash-merged into this base and carried the rest of this PR's original scope with it. Everything else previously described here — the wallet cold-boot lockout, migration-gate coverage, shielded chokepoint, DashPay preservation defaults, and the passphrase transition-frame fixes — is already merged and is no longer part of this PR. What remains is the single unmerged commit below.
What was done
ContactsState::reset_for_identity_changekeeps a paid-action guard whose backend task is genuinely still running, instead of wiping the whole guard set on an identity/wallet/network switch. View-only state still clears. Guards for abandoned confirmations are released, and timed-out entries are pruned, so nothing leaks and expired guards still self-heal.contactInfo—create_or_update_contact_infofetches every page before selecting update/create metadata, so the choice is made against the complete set rather than the first 100 documents.Testing
Verification is running against the rebased tree; results will be posted here once complete. The pre-existing suite (
cargo test --all-features --workspace,clippy --all-features --all-targets -D warnings,cargo +nightly fmt --all) is the gate, plus the contacts/dashpay tests covering guard lifecycle and pagination.Breaking changes
None.
🤖 Co-authored by Claudius the Magnificent AI Agent