fix(identity): clear DashPay and token-list state when an identity is removed - #963
fix(identity): clear DashPay and token-list state when an identity is removed#963Claudius-Maginificent wants to merge 4 commits into
Conversation
…movals The Identity Hub's Settings tab shipped an "Unload this identity from this device" button that was permanently disabled behind a "coming soon" tooltip, with a confirmation dialog that did nothing when confirmed. Wire it to the existing `IdentityTask::RemoveIdentity` task — the same one the legacy identities screen already dispatches. No new backend variant. The target identity is snapshotted when the button is clicked, bound to the dialog in `PendingIdentityUnload`. The confirmation is answered frames later and the hub's selection can move in between, so re-reading the selection at confirm time would unload the wrong identity. Also fixes a removal-ordering bug that this second, more prominent entry point would have made much easier to hit. `delete_local_qualified_identity` cleared the vault keys FIRST, so a failure in either later step left a zombie: an identity still indexed, visible and loadable, whose private keys were already gone for good. The vault delete now runs last, once the identity is unlisted and drained. The ordering could not simply be swapped. The delete set lives in the very blob `purge_identity_scope` drops, so clearing after the purge would have silently stranded every secret in the vault instead of deleting it. The placements are therefore read up front and deleted at the end, which keeps both properties: nothing irreversible before the identity is gone, and no orphaned secrets after. Removal outcome copy moves to `ui::identities` so the hub and the legacy screen share one wording, and the hub drops the app-wide selection when it names a removed identity. Tests: - `a_failed_identity_delete_never_destroys_the_vault_keys` — corrupts the enumeration index to fail the delete partway; confirmed RED against the old ordering (keys already destroyed), green after. - `a_successful_identity_delete_leaves_no_orphaned_vault_key` — guards the other half: deferring the vault delete must not turn it into a no-op. - `confirming_an_unload_targets_the_identity_snapshotted_at_click_time` and two siblings covering the storage-update refusal and single-shot resolution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… removed Removing an identity drained its own k/v scope but left every DashPay overlay and token-list preference behind, orphaned and unreachable. `dashpay.rs` documented these as reaped "by the upstream soft-cascade when the owner identity row is deleted". That cascade fires on a row DELETE against the upstream `identities` table, which DET never issues — as `delete_local_qualified_identity`'s own docs already stated. The DashPay sidecar was written against a guarantee this path does not provide, which is why no cleanup was ever wired. The module comment now says what actually happens instead of naming a mechanism that never runs. Most of the fix was calling code that already existed: `dashpay_clear_owner_overlays` already swept the six identity-scoped families and was only ever called from the network-wide wipe. It could not reach the seventh, `addr_map`, which is Global-scoped with the owner inside the key, so that sweep is added to the same helper — a no-op for the wipe, which already drops every Global `det:dashpay:` key. `timestamps` is deliberately left alone: it is keyed by entity and shared between owners, so pruning it per owner would delete another identity's data. Also drops the removed identity's token-list state. A "stopped tracking" marker names a `(token, identity)` pair; left behind it silently re-hides that token if the identity is ever loaded again, and the user cannot see the marker to clear it. The saved ordering is pruned for the same reason `remove_token` prunes it — unlike the identity ordering, `load_token_order` does not self-heal. Both sweeps are best-effort and run after the vault-key delete. They are local preferences, not secrets, and ordering them before the irreversible wipe would let a failed memo delete stop it — leaving private keys on disk during "delete all local data". The unload confirmation no longer promises recoverability it cannot deliver. "You can load it again later" is false for an identity whose only saved keys were the vault entries the removal destroys: the identity comes back, the ability to use it does not. Both the dialog and its tooltip now say the identity stays on Dash Platform without implying it stays usable. Tests (both confirmed RED first): - `removing_an_identity_clears_its_dashpay_overlays` — asserts at scope level, so it covers the family set rather than a hardcoded prefix list, plus a second owner whose overlays must survive. - `removing_an_identity_forgets_its_token_list_state` — dismissal and ordering, with another identity's dismissal of the same token left intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 — Opus deferred (commit 679960d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The happy-path cleanup is scoped correctly and its regression tests cover both deletion and bystander isolation. Two in-scope reliability issues remain: cleanup failures are reported as successful removal without reconciliation, and concurrent token-state writers can recreate or restore the state being removed.
Source: reviewer backend gpt-5.6-sol (Codex general and Rust-quality lanes); final verifier backend grok-4.5; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus 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)
🔴 1 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/context/identity_db.rs`:
- [BLOCKING] src/context/identity_db.rs:1435-1450: Cleanup failures are reported as successful identity removal
Both newly added cleanup calls discard their typed errors, after which `delete_local_qualified_identity` returns `Ok(())`, `remove_identity` emits `RemovedIdentities`, and the UI shows a success message. If a key listing, deletion, or token-order write fails, the removed identity's overlays or dismissal markers survive and reappear when the identity is imported again—the behavior this PR is intended to eliminate. Unlike `remove_unowned_identity`, these sidecars have no boot-time reconciliation pass. The key-wipe ordering requirement does not require swallowing the errors: cleanup already runs after `delete_all`, and `clear_network_database` already aggregates identity-deletion failures while continuing with the remaining identities. Run all independent cleanup and key-wipe operations, then surface a typed partial-cleanup outcome or persist a durable retry marker instead of reporting unconditional success.
In `src/context/contract_token_db.rs`:
- [SUGGESTION] src/context/contract_token_db.rs:791-799: Token cleanup races concurrent token-state mutations
This cleanup is a multi-operation list/delete sweep followed by a read/filter/write of the global token-order vector, but backend tasks run independently and no shared lifecycle guard covers these token-state writers. A `stop_tracking_token_balance` task can finish its awaited unwatch after this sweep and recreate the removed identity's marker. Two identity removals can also read the same `TOKEN_ORDER_KEY` snapshot, each remove a different identity, and have the final whole-vector write restore entries removed by the other task; `save_token_order` can similarly overwrite the cleaned vector with stale screen state. The per-identity `identity_record_lock` cannot serialize operations for different identities or other token writers. Coordinate cleanup with every token-state writer using an atomic transaction, a shared mutation lock plus deletion-state validation, or an equivalent lifecycle mechanism, and cover the ordering with deterministic concurrency tests.
| if let Err(error) = self | ||
| .wallet_backend() | ||
| .and_then(|backend| backend.dashpay_clear_owner_overlays(identifier)) | ||
| { | ||
| tracing::warn!( | ||
| identity_id = %identifier, | ||
| ?error, | ||
| "Removed identity left its DashPay contact overlays behind" | ||
| ); | ||
| } | ||
| if let Err(error) = super::contract_token_db::forget_identity_token_state(&kv, identifier) { | ||
| tracing::warn!( | ||
| identity_id = %identifier, | ||
| ?error, | ||
| "Removed identity left its token list preferences behind" | ||
| ); |
There was a problem hiding this comment.
🔴 Blocking: Cleanup failures are reported as successful identity removal
Both newly added cleanup calls discard their typed errors, after which delete_local_qualified_identity returns Ok(()), remove_identity emits RemovedIdentities, and the UI shows a success message. If a key listing, deletion, or token-order write fails, the removed identity's overlays or dismissal markers survive and reappear when the identity is imported again—the behavior this PR is intended to eliminate. Unlike remove_unowned_identity, these sidecars have no boot-time reconciliation pass. The key-wipe ordering requirement does not require swallowing the errors: cleanup already runs after delete_all, and clear_network_database already aggregates identity-deletion failures while continuing with the remaining identities. Run all independent cleanup and key-wipe operations, then surface a typed partial-cleanup outcome or persist a durable retry marker instead of reporting unconditional success.
source: ['codex']
| for key in kv | ||
| .list(DetScope::Global, Some(TOKEN_UNTRACKED_PREFIX)) | ||
| .map_err(token_err)? | ||
| { | ||
| if parse_untracked_key(&key).is_some_and(|pair| pair.identity_id == *identity_id) { | ||
| kv.delete(DetScope::Global, &key).map_err(token_err)?; | ||
| } | ||
| } | ||
| prune_token_order(kv, |(_, identity)| identity != identity_id) |
There was a problem hiding this comment.
🟡 Suggestion: Token cleanup races concurrent token-state mutations
This cleanup is a multi-operation list/delete sweep followed by a read/filter/write of the global token-order vector, but backend tasks run independently and no shared lifecycle guard covers these token-state writers. A stop_tracking_token_balance task can finish its awaited unwatch after this sweep and recreate the removed identity's marker. Two identity removals can also read the same TOKEN_ORDER_KEY snapshot, each remove a different identity, and have the final whole-vector write restore entries removed by the other task; save_token_order can similarly overwrite the cleaned vector with stale screen state. The per-identity identity_record_lock cannot serialize operations for different identities or other token writers. Coordinate cleanup with every token-state writer using an atomic transaction, a shared mutation lock plus deletion-state validation, or an equivalent lifecycle mechanism, and cover the ordering with deterministic concurrency tests.
source: ['codex']
3 participants
TL;DR: Removing an identity from this device now also clears the DashPay contact/profile overlays and "hidden token" markers it left behind, and the unload confirmation no longer overpromises what survives.
User story
As an everyday user, I want removing an identity to actually remove its local DashPay contact/profile state and dismissed-token markers, to achieve a clean slate when I re-add the same identity later instead of silently stale leftovers.
Scenario
Base flow
A user unloads an identity (via #962's newly-wired button, or the existing legacy Remove control), then later re-imports the same identity.
Actual behavior
Local DashPay overlay state (contact requests, blocked/declined lists, address-index and address-map entries, request-action markers, private memos) and per-identity "stop tracking this token" markers are keyed by identity id in a k/v sidecar that the removal path never touches. They silently survive removal and reappear, stale, on re-import — e.g. a token the user previously hid stays hidden with no visible reason why.
Separately, the unload confirmation dialog claims the identity can always be "load[ed] again later" — false for an imported or masternode identity whose only private-key copy is the vault entry the removal path (correctly, per #962) destroys. The identity record comes back; the ability to use it does not.
Expected behavior
Removing an identity clears its DashPay-scoped sidecar state and its per-token dismissal markers, without touching any other identity's data. The confirmation copy no longer promises recoverability the removal path can't deliver.
Detailed discussion
What was done
delete_local_qualified_identity(src/context/identity_db.rs) now calls the identity-removal cleanup after the irreversible vault-key delete, not before — a fallible sidecar sweep must never be able to abort the key wipe (this matters becauseclear_network_database's "delete all local data" path also routes through this function for every identity). Sweep failures are logged as warnings, matching the existing best-effort precedent for the wallet's unowned-identity mirror removal in the same function.dashpay_clear_owner_overlays(owner)helper (src/wallet_backend/dashpay.rs) into identity removal — it already swept 6 of 8 DashPay sidecar families but was previously only called from the full network wipe.det:dashpay:addr_map:<owner>:<address>), which is Global-scoped with the owner embedded in the key and so was invisible to the existing Identity-scope sweep. No-op for the network-wipe caller, which already sweeps alldet:dashpay:keys globally.forget_identity_token_state, dropping the removed identity's "stopped tracking" token markers and saved token-ordering entries (unlike identity ordering, this list does not self-heal on its own).contestant/contested_name(public contest data cached locally, not per-user state),det:identity_order:v1(already self-healing),det:dashpay:timestamps:anddet:contact_profile:/det:avatar:(Global caches shared across owners — pruning by identity would delete a different identity's data), and scheduled DPNS votes (already handled by fix(identity): wire the Identity Hub unload button and stop zombie removals #962).Stacked on #962 (
fix/cannot-delete-identity) — not independently mergeable until that lands first.Known follow-up, not in this PR: wallet-owned identities are never removed from their wallet's upstream
IdentityManager, which post-refactor is what actually owns contact/profile/payment state for those identities. That's a separate, larger change touching the wallet seam with its own ordering/failure analysis — tracked for a future PR, not bundled here to keep this one narrowly scoped and reviewable.Testing
cargo test --lib --all-features— 2349 passed, 0 failed (widened from a narrow scope deliberately:delete_local_qualified_identityhas six production call sites, so a shared-function change needed the full lib suite, not just the touched module's tests).cargo clippy --lib --tests --all-features -- -D warnings— clean.cargo fmt --all.Breaking changes
None.
Checklist
cargo fmt --allPrior work
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent