fix(identity): make Key Info reachable everywhere and keep key labels/holdings consistent (#889) - #945
Merged
Merged
Conversation
Issue #889: a v0.9.3 identity that was already partially loaded before the upgrade keeps its remaining keys stranded in `data.db`, because the migration skips a present identity wholesale. That rule is correct — three earlier reconcile heuristics each failed by inferring user intent from record shape. `compute_recovery_plan` only lists what the legacy record holds and the modern one does not, keyed on the `(target, key_id)` map slot and on `None` associations; it never decides whether an item should come back. That decision travels back in as an item-level allowlist, so the absence-versus-removal ambiguity lands with the only party holding the provenance. `apply_recovery_plan` is additive by construction, not by discipline: it starts from the fresh modern record, inserts with `or_insert` so the modern copy wins every collision, recomputes candidacy rather than trusting the allowlist, and has no write path at all for alias, status, the dpp identity, DPNS names, or the wallet link. A voting key drags its voter-identity link along, since one without the other cannot vote. M1-M10 from the design doc's test matrix. M2/M5/M6/M9/M10 were confirmed RED against a naive legacy-wins merge before this implementation landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ecoder Issue #889: the recovery flow needs one identity out of the preserved legacy `data.db`, not the whole table. A second hand-written reader would be free to drift from the importer's idea of which rows are readable — and then recovery could merge from a record the migration never considered importable, or miss one it did. So the per-row decode moves into `decode_identity_row` and both readers call it, along with one shared `SELECT` that binds the same pair of accepted network spellings. `read_identity_row` adds only the `id` predicate on top. `LegacyIdentityLookup` keeps "nothing to recover" and "the row will not decode" apart. An absent table, an absent row, an observed-identity cache row and a NULL blob are all `Absent` — the ordinary answer. Only a genuinely corrupt row is `Unreadable`, so no caller can mistake corruption for an empty record and merge against it. `open_legacy_read_only` is hoisted next to `database/mod.rs`'s existing one, so `SQLITE_OPEN_READ_ONLY` is a property of the one function that opens the file rather than of each caller remembering the flag. The importer's 12 existing row-decode tests are the regression net for the extraction; 8 new tests cover the lookup, including one asserting both readers decode the same row identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… data Issue #889: an identity that was only partially loaded before the v1.0 upgrade keeps its remaining keys in `data.db`, reachable by nothing — the load form rejects the duplicate ProTxHash, and the per-key screen wants a WIF the user may no longer hold. This is the way in, and it is entirely user-driven. `CheckLegacyRecovery` lists; `RecoverLegacyIdentityData` writes. Detection is read-only, offline and side-effect-free, so a screen can dispatch it on arrival and hide the affordance when the plan comes back empty. Candidacy is recomputed inside the executing task rather than trusted from the caller: only `recomputed-candidates ∩ approved` is merged, so an approval that went stale since the preview is reported rather than acted on, and an item the user never approved is never restored even when it is missing. An empty allowlist is refused, not widened into "restore everything". The protection-downgrade trip that sank an earlier reconcile attempt is unreachable here by branch condition, not by care: the flow branches on the same predicate the at-rest guard evaluates. On a protected identity the password is verified up front through the shipped prompt, the resident-plaintext preflight runs before any vault write, and every merged key is sealed under that password before the record is persisted — so the encoder never sees plaintext on a protected identity. On a keyless identity there is no protected key for the guard to protect. Cancel, wrong password and headless all fail closed with the stored record byte-identical. Exactly one record write, so the merge either lands whole or changes nothing. No new persisted state: eligibility recomputes from the two existing stores, so a re-run is a no-op and `data.db` — opened read-only — stays a repeatable recovery source. `reject_resident_identity_plaintext` widens to `pub(super)`; it now guards both seal boundaries. Two test-only readers expose the at-rest blob and the wallet link, which the hydrated record cannot show. B1-B12 from the design doc's test matrix, on the offline wired `AppContext` harness, plus the excluded-key and empty-allowlist cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…node page Issue #889: the restore path exists but nothing reaches it. The node detail page is the canonical surface — its operator is the one holding a node that cannot vote because its voting key stayed behind in the previous version's saved data. The offer is passive and contextual, never a launch-time nag. On opening a node the view dispatches the read-only detection task once, and only on an install that actually has a previous-version database; the section renders in the Keys area only when detection found something, and retires itself once a restore lands, because eligibility recomputes from the two stores every time. For a node with no voting key that becomes the primary remedy: the missing-voter message points at restoring, which asks nothing of an operator who no longer has the key, and the existing WIF prompt keeps its place as the fallback for one who does. `LegacyRecoveryState` (`ui/state/`) owns the fetch state — dispatch-once, in-flight, offered, retryable — and `LegacyRecoverySection` (`ui/components/`) renders it, per the placement rule's render/no-render split. A failed restore returns to its offer so a mistyped identity password can be corrected and retried; a failed detection does not re-ask. A preview result is routed into the open detail view rather than triggering the list screen's reload-and-reopen, which would rebuild the view, re-dispatch its check and never settle. A completed restore does reload, since the store changed, and the rebuilt view's fresh check finds nothing left. `RecoveryItemDescriptor::is_voting_key` factors out the rule `label` already applied, so "is this the voting key" has one definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
Issue #889: the node page covers the masternode case, but a `User` identity that was partially loaded before the upgrade has no node page. The Key Info screen is the issue's own suggested location and the one surface every identity type reaches — from the Identities list for a user identity, and through Manage keys for a node. The offer is identity-scoped rather than key-scoped, so it renders whatever the opened key's own state is: what it lists are precisely the keys the identity does *not* hold, which no per-key view could show. Both surfaces share `LegacyRecoveryState` and `LegacyRecoverySection`, so detection, the allowlist and the copy have one definition each. Dispatch follows this screen's established queue-then-drain pattern, alongside the protect/unprotect ones. The check and a restore can never contend for the frame's single `AppAction`: the check goes out once on arrival, and a restore only after a press. A completed restore re-probes the protection status, since the restored keys have just landed in the vault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…ration gap Issue #889: the migration design's §7 named this limitation and deferred it to "a dedicated, provenance-aware flow". That flow now exists, so the limitation gets its resolution rather than staying an open pointer to an issue — with the one clarification a reader of that section needs: the importer itself did not change, skip-if-present still stands, and nothing about recovery runs at migration or launch time. The design record moves into `docs/ai-design/` under this repo's dated-directory convention, so the cross-reference resolves inside the repo. U1 covers the widget from the outside: an empty plan renders nothing, items are named by role with the voter link folded into its voting key, pressing Restore approves exactly the previewed set, an in-flight restore has no button to press twice, unrestorable keys are listed with no restore offered, and an install with no previous-version data never even asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The plan admitted any legacy key the modern record happened not to hold,
keyed on the map slot alone. Nothing checked that the saved private half
derived the public half it was stored with, that the public half was still
a live key of the identity, or that a wallet-derivation reference named a
wallet this install holds — the exact checks the manual "type the WIF"
path this flow replaces already enforces.
That matters because `masternode_key_presence` reports a role as held from
the record alone. Restoring a key the node rotated away from flipped the
role to "present", retiring both the recovery offer and the missing-voter
remedy, and the operator found out from a rejected transaction. The voter
link alone did the same thing with no key behind it at all.
Failing candidates now go to `excluded` with their own reason and are
never restorable. `AppliedRecovery` also reports that `excluded` list, so
an approval naming something that was never restorable is no longer also
reported as a stale approval ("already back in place" — the opposite
answer), and the caller stops recomputing the same plan to read one field
off it. Whatever plaintext the legacy record still holds after a merge is
zeroized instead of being dropped intact.
The item labels and exclusion copy move out of `model/` into the UI, where
they now delegate to the shipped `role_label_and_tip` vocabulary rather
than a second mapping that disagreed with it on the same screen ("Payout
key" next to "Payout address key"), and two rows in one role are told
apart by key id instead of rendering identically.
Test fixtures move onto genuinely derived key pairs with the chain's
public keys published on the record — a correspondence check that passes
against placeholder key material verifies nothing.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
`recover_legacy_identity_data` took `migration_run` for the whole async span, prompt included. The delete path it copied that pattern from is fully synchronous and holds the mutex for microseconds; this held it for as long as a modal sat in front of the user. Two independent reviewers reproduced the fallout: removing an *unrelated* identity failed with "storage is still being updated" when no migration was running, and `finish_unwire`'s awaiting acquire could park indefinitely. The flow is now a preflight and one fully synchronous critical section. The preflight reads, dry-runs the merge to see whether anything would be restored at all, decides whether a password is needed, and prompts — holding nothing. The write section then re-acquires the guard, re-checks the migration state, re-reads the record, merges again and writes, all without an await in between. Re-reading is not just tidiness. The prompt window let any other writer land in between — refresh, DPNS registration, transfer, none of which take the load claim — and the write of a pre-prompt snapshot silently reverted it. It also makes the no-resurrection guarantee explicit: a delete during the prompt now surfaces as `IdentityNotFoundLocally` instead of being undone by the upsert, which until now held only as a side effect of how long the lock was kept. The at-rest protection predicate is evaluated over the merged record, the one the encoder's downgrade guard actually sees, rather than over the pre-merge record. If the identity gained password protection during the prompt the verified password no longer covers what is about to be written, so the restore stops with a typed `LegacyRecoveryIdentityChanged` rather than sealing under a password for a different state. Also: the nothing-to-recover success no longer records a `Failed` load, the preview zeroizes the stranded plaintext it decodes on every screen open instead of releasing it intact, and the "nothing restored" banner is worded to be true of both outcomes that produce it. Both concurrency tests were confirmed RED against the old lock scope, reproducing the reviewers' exact failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
Three ways the two shipped surfaces disagreed with the store after a restore, and the state machine behind them had no tests at all. The Key Info screen keeps the identity clone it opened with, and its own add-key and remove-key paths persist that whole clone. This feature added a second writer to the same record reachable from the same screen: restore keys, then add or remove any key, and the pre-restore copy was written back — the restored keys gone, right after a banner said they were back. The completion now re-reads the record and the key on screen from it. The node page never hears about a restore run from the Key Info screen it pushed: the pushed screen is on top, so it takes the result. Coming back left the page offering keys already restored and warning about a voting key it now held. Arrival re-reads the node and re-arms its check; vote selections and any open prompt survive, which a blanket rebuild would have discarded. The `recovery_completed()` call in the result handler goes with it — the reload that follows re-opens the view and re-arms the check anyway, so it was mutating a view discarded three statements later. Writing the state machine's tests turned up a third one: `completed()` re-armed unconditionally, so calling it on an install with no previous-version database armed a check that must never run. Harmless until arrival started calling it; now `Unavailable` stays unavailable. The Restore button loses its ellipsis: on a keyless identity — the common case this feature exists for — it commits immediately rather than opening anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The committed design record was landed marked "implemented" with exactly one edit — the status line — so the repo's permanent account of this feature described a dispatch model, a progress mechanism, a module set and a gating rule that were all deliberately changed, plus the pre-review merge rules that the fix round replaced. A §10 now records all nine departures with the reasoning that produced them, rather than rewriting §1–§9 into a story nobody actually followed; the status line says which of the two is authoritative. §7's claim that `finish_unwire.rs` is untouched is reconciled with §4.2's hoist in the same document: the importer's behaviour is unchanged, the file is not. `reject_resident_identity_plaintext`'s header claimed to guard "every boundary that seals an identity's keys". It does not — the merge-load path seals without it. The header now says which boundaries it covers, and that path carries a TODO describing the gap. Fixing it is a separate issue: it is pre-existing code, unrelated to this feature. Also: user story and CHANGELOG pick up the correspondence check and the non-blocking prompt, the components catalog names the copy this feature's component now owns, and three private rustdoc blocks come back inside the internal-commentary budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The correspondence check that keeps a rotated-away key out of the offer resolved a voter/operator-target key's identity as "the modern link, or the legacy one if there is no modern link". In the case the check exists for -- a masternode whose record carries no voter link -- that fallback read the identity snapshot out of the same unauthenticated legacy blob as the key under test, so any self-consistent pair passed. A voting key the chain retired months before the upgrade still reads as live in that snapshot, and restoring it flips the node's voting role to "present", retiring the manual remedy that would actually fix the node. reference_identity now reads the modern record only. A key on a voter or operator identity the modern record does not link to has no admissible witness and is excluded as LinkedIdentityUnverified, carrying the same "load this identity again and enter the key" remedy as its neighbours. Fetching the linked identity from Platform was rejected twice over. It would put the network in the middle of an offline, read-only preview that every screen arrival dispatches, and model/ is pure by contract. More decisively, it would not establish the property: a voter identity's id is derived from its voting key (Identifier::create_voter_identifier), so rotating the key on-chain creates a *different* voter identity rather than retiring a key on the existing one. Fetching the legacy-named voter identity would confirm the stale key against its own orphaned identity. Behaviour change worth noticing on its own: a masternode loaded from its ProTxHash alone no longer gets its voter-identity-target voting key back in one click. The key is listed as unverifiable, the voter link goes with it, the node keeps reporting the role as missing, and render_missing_voter keeps offering manual entry -- the honest outcome, since nothing available offline can tell that key from one the chain replaced. A record that does carry the voter link (a re-load keeps it) restores the key as before. Grouping follows the same rule: voter_association_is_grouped now keys on the voting role, the predicate that already decides whether the link is worth offering, so the two cannot disagree about which key the link belongs to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
… existing FetchState::Unavailable was documented as the state a fresh install never leaves, but no shipped install could reach it. AppState::boot_inputs opens an existing data.db read-only *or* creates one and runs the fresh-install schema ladder, so db_file_path().exists() is true for every user, and every identity or node screen dispatched a detection task against a table the ladder had just created empty. The kittest that claimed otherwise passed only because the `testing` feature substitutes an in-memory database whose path is None -- a test of the substitution, not of the gate. AppContext::has_legacy_identities now asks whether the legacy `identity` table holds a local row for this network: one SELECT EXISTS over the same filter read_identities uses (so "there is something here" and "here it is" cannot drift), false when the table does not exist at all, answered once per context and cached. A probe that errors arms the offer instead of retiring it -- the detection task reports its own typed error, while a silent false would withdraw a recovery the user's data still supports. The kittest is replaced by lib tests over a real file-backed data.db, which assert the premise the old gate missed (the file is there and the gate still says no) and cover the network scoping a file check could never have. Confirmed red against the old gate before the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
…estore Two ways a restore's result reached the wrong screen state. KeyInfoScreen had an empty refresh(), which refresh_on_arrival defaults to, so returning to a screen that missed its result never re-read the record. That screen persists the whole identity clone it opened with on every key add or remove, so the clone -- predating the restore -- was written back over the restored keys on the next edit, silently and with a successful save to show for it. It now re-reads on arrival, re-probes the protection line and re-arms its check, exactly as the masternode detail view already does. The LegacyRecoveryCompleted arm discarded identity_id, unlike the LegacyRecoveryCandidates arm directly above it, which routes through LegacyRecoveryState::offered's identity check. Results reach whichever screen is visible when they arrive, not the screen that dispatched them, so a restore dispatched from identity A's Key Info screen could land on identity B's: B showed "your keys have been restored to this identity" for a restore that was never B's, and B's own in-flight recovery state was reset under it. The attribution rule now lives once, in LegacyRecoveryState as completed_for -- the twin of offered -- so both call sites share it rather than each remembering to check. The masternodes list applies the same rule through shows_node: a completion for an identity no card and no open detail names is dropped, banner, reload and detail rebuild included. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
TaskError::LegacyRecoveryIdentityChanged is listed in persist_legacy_recovery's rustdoc and spelled out in design.md 10.6, but was the only documented error branch of that function with no test -- every sibling (IdentityLoadInProgress, WalletStorageNotReady, IdentityNotFoundLocally, LegacyIdentityUnreadable, the prompt's cancel and unavailable errors, IdentityKeyProtectionIncomplete, the deleted identity, the undecodable row) has one. B13 drives the 10.6 scenario: the dry run on a keyless identity decides no password is needed, a ProtectIdentityKeys lands before the write section re-reads the record, and the write refuses with the typed error and a byte-identical stored blob. It calls persist_legacy_recovery directly with the None the dry run produced, as B5 calls verify_recovery_password directly: a Tier-1 restore never prompts, so there is no await point between the two where a concurrent task could be made to land deterministically. The dry run is exercised first so the None under test is the one production carries. Verified the test fails when the fail-closed branch is removed, rather than merely restating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
The voter-target fix landed one half of the rule: a key on a voter or operator identity the modern record does not link to has no witness outside the legacy file, so it is excluded. The operator *link* itself was offered with no gate at all, which left the same hole open one step further out. Restoring it writes the legacy file's own claim into the modern record. The next preview then reads that claim back as the outside witness for the same file's operator keys, and every one of them becomes a candidate — self-vouching in two moves instead of one. Nothing in production ever writes `associated_operator_identity` fresh, so an offered operator link was always in that unverifiable state. An operator link the modern record does not carry is now excluded as `LinkedIdentityUnverified`, like the keys held on that identity. Also renames M8, whose name and opening sentence claimed it proved the whole legacy key set is offered while its own assertions exclude two of five items.
The node page carried a second missing-voter message offering restore as the primary remedy, chosen whenever the recovery plan held a voting-role candidate. Since the voter-key check began reading the modern record only, that candidate exists only when the record already carries the voter link — and a record carrying the voter link is written in the same branch that writes the voting key, so it is never in the missing-voter state to begin with. The two conditions are mutually exclusive: no user, and no test, has ever seen that message or its companion tooltip. Gone, along with the predicate behind them. The remaining message is the honest one and still matches what the screen offers: the in-place "Add voting key" prompt right underneath it, which needs no re-load. Restoring a voting key held on a separate voter identity needs an on-chain check this flow deliberately does not make; that is issue #942, not something this branch pretends to do.
… nothing A plan holding only unrestorable items — now the ordinary shape for a masternode loaded from its ProTxHash — opened with "Some keys ... haven't been brought across", followed by no list, no button, and then a second heading for the items it cannot bring back. A lead-in that promises an action the section never offers reads as a broken screen. That case gets its own lead-in, which introduces the list itself, so the second heading appears only when there really are two lists to keep apart. The reason an item cannot be restored also stops being hover-only. It is the single sentence that tells the user what to do instead, and a tooltip reaches neither touch nor keyboard; it is now an inline line under each item. The reason for an unverifiable linked identity is reworded to read correctly for the operator link as well as for a key held on one.
The user story and the changelog both said restoring is offered as the first remedy for a node with no voting key, with typing the key in as the fallback. For the flagship case — a masternode loaded from its ProTxHash alone — that is never true: the voting key sits on a separate voting identity that only the previous version's data names, so it is listed as one that cannot be brought back and entering it by hand is the only remedy. Naming owner, voting and payout together in the same breath carried the same implication and is dropped. Both now describe what happens, and point at issue #942 for the on-chain check that would let the voting-key case be closed safely later. The design record gains the two shipped deviations behind this: §2.1's "restore first, WIF as fallback" branch removed as unreachable, and the operator link excluded as unverifiable for the same reason its keys are.
The identity hub replaced the legacy Identities screen's per-key popup — the only route into KeyInfoScreen that did not depend on the identity already holding a usable key — with Settings > Advanced > "Manage keys". That destination was a read-only table with no route onward, and RootScreenIdentities is deliberately out of the nav, so the popup it still hosts is unreachable. Every remaining route (transfer, withdraw, the token screens) is gated on holding a key of the kind that action needs, which is false for precisely the identities issue #889's restore offer exists to help: a user identity with stranded keys could reach its own keys only in Developer view, through a send-money screen. KeysScreen now carries the QualifiedIdentity, so it can say which keys this device holds and open KeyInfoScreen with that key's private material. One row per key, named in the shared role vocabulary, held state stated in words rather than by colour alone, ungated. The restore offer renders on the list itself, above the rows: it is scoped to the identity, and making a user pick an arbitrary key to discover an identity-level offer repeats the defect one level up. It stays on KeyInfoScreen too, which self-extinguishes and is where the masternode path lands. The screen also moves inside the app chrome (top panel, left panel, island_central_panel). That is not cosmetic: island_central_panel is the only caller of MessageBanner::show_global, so on a raw CentralPanel the restore reports nothing at all — a failed restore would be indistinguishable from a successful one. key_role_label, manage_keys_labels and the key enumeration move from detail_screen into ui::masternodes beside the vocabulary they belong to, so the two "Manage keys" surfaces cannot drift apart; the masternode kittest covering that list is the regression guard for the move. The transfer screen's no-key branch gains a "Manage keys" signpost. Its available_transfer_keys() gate is untouched — it still correctly says the identity cannot send; the branch simply stops being a dead end.
…s list The offer self-extinguishes on the next check whether the restore landed or failed, so its disappearance says nothing about the outcome. Only the banner distinguishes the two, and the banner only reaches the user because the screen is built on island_central_panel — the sole caller of MessageBanner::show_global. Cover both outcomes explicitly, and pin the render channel itself: reverting the screen to a bare CentralPanel fails the new guard, which is what makes it worth having. Also pin AC-6 across the two surfaces that name keys. The restore offer and the keys list render from different modules over different types, and nothing but their shared derivation from role_label_and_tip keeps them saying the same words; a private re-implementation on either side would read plausibly and drift in silence. Key Info remains outside that assertion: it does not retain the PrivateKeyTarget its key was opened with, so it cannot tell a voter-identity key from a main-identity one, and naming it in role words today would risk labelling a voting key as an authentication key. Threading the target through its nine call sites is the fix, and it is wider than this change.
…running app The component tests prove two things separately: the screen sets a banner on a finished restore, and it renders whatever banner its context carries. Neither closes the seam between them — that the context the screen writes its outcome into is the context it renders from. A restore reporting into one context while the screen renders another would pass both and still tell the user nothing. This drives the real app to the keys list, delivers a completed restore through visible_screen_mut the way AppState does, and asserts the success text appears on the screen; then banners a task error on the app context, as AppState does for a failed restore, and asserts that appears too. The offer retires itself either way, so the banner is the only thing that distinguishes them. Reverting the screen to a bare CentralPanel fails this test, which is what makes it a guard rather than a decoration.
…s filed SEC-001. The keys list resolves each key's PrivateKeyTarget from the identity it walked, then discarded it; KeyInfoScreen re-derived one from the key's purpose. impl From<Purpose> for PrivateKeyTarget sends every voting key to the voter identity, so for a Purpose::VOTING key filed on the main identity — a supported shape, which masternode_key_presence reads as voting readiness on its own — the two disagree. The list reports the key as held, Key Info re-reads the record on arrival, looks in a store the key was never filed under, finds nothing, and reports it missing. On the screen built to answer that question, about a key whose private half is listed one screen earlier. KeyInfoScreen now accepts the resolved target via with_target and uses it for every lookup, falling back to the old derivation when no caller supplied one, so the call sites this change does not touch keep their current behaviour. Only the keys list passes it. The same derivation still governs the pre-existing callers' reads, writes and deletes — including a delete that can land on a different key when voter and main key-id spaces overlap — which is filed separately. Also from QA: - SEC-002: the held/not-held column called get_cloned_private_key_data_and_ wallet_info, copying raw key bytes out of the vault unscrubbed, every frame for every key, to read is_some(). KeyStorage::has answers it without a copy. - QA-005: display_message ended a restore on any MessageType::Error. Results are not screen-affine, so an unrelated task's failure re-armed Restore mid-flight and it could be dispatched twice. Attribution moves to display_task_error, which receives the typed error; it returns false so the user still gets AppState's banner. Residual documented on is_recovery_error: the same variant from another task during a restore still ends it, which needs the task's identity to reach the screen. - QA-003: the completion-banner test asserted only that some banner existed. It now pins attribution instead — another identity's restore reports nothing here — since fresh_app_context drops the harness its context belongs to and the text cannot be read back at that level. The running-app test already pins the text. - QA-006/007: coverage for the transfer-screen signpost and for the Expert-only detail row in both directions. - Empty-state copy said "no keys saved on this device", which in this screen's own per-key vocabulary means held=false. An empty record is a different statement; the test that codified the wrong string is fixed with it. - QA-002: "island_central_panel is the only caller of show_global" is false — contracts_documents_screen calls it directly. Corrected where it was written down as justification. - key_role_label back to pub(crate); ui/masternodes' module header no longer implies its key vocabulary is Expert-Mode-scoped, since an Everyday identity screen depends on it. - Docs: §2.1 and §7 row 1 restored as written, per the document's own proposal-plus-deviations model, with §10.13 carrying the correction. CHANGELOG no longer claims transfer was the only route to a key's page, and no longer promises signing or password protection to an identity that holds no keys yet.
… own words Track A of round 3. Leaving a key now pops with a refresh, and the keys list re-reads both halves of its state on the way back. Only re-reading the record would show a key restored from the pushed Key Info screen as held while still offering to restore it — and pressing Restore then reports there was nothing left to do. Consolidating into refresh() also retires the refresh_on_arrival override, which never ran: the framework dispatches that hook for root screens, and this screen is pushed. KeyInfoScreen gains a with_parent breadcrumb crumb so the trail leads back to the screen the key was opened from. Parameterised, not hardcoded: it has nine parents and naming one of them for all of them would mislabel the other eight, which keep their two-level trail unchanged. Key role words now follow the identity that owns the key. The DIP-3 wording describes masternode registration duties — updating a registration, receiving node rewards — and on a user identity that does not read as jargon, it asserts the user owns a masternode. A user's TRANSFER key is a Transfer key, not a payout address. Both the keys list and the restore offer take the same vocabulary from the same identity: scoping only one would have a user's offer say "Payout address key" six lines above a list saying "Transfer key", which is the two-names-for-one-key defect the shared vocabulary exists to prevent. Empty-state copy no longer says "no keys saved on this device" — these rows are the identity's on-chain public keys, and that phrase means held=false per-row on this same screen. Track B, reads only, after verifying the read/write split against the source rather than the report: :746 (feeds the vault chokepoint, whose consequence is signing with and displaying the wrong key's material) and :887 (reload) are reads and now resolve the target the caller established. The keys list also resolves held-ness against both conventions, since real installs hold material written under each; looking in a second place can only find material that is already there. The writes are deliberately reverted to the purpose-derived target, which the previous commit had changed. QualifiedIdentity::sign and can_sign_with resolve that way, so material stored anywhere else is material that can never sign: for a voting key filed on the main identity, the previous commit would have taken a hand-entered private key and stored it where the signer never looks. Removal keeps the derived target for the same reason, which leaves wrong-key removal as a documented residual. Both sites carry the reasoning, and reconciling the two conventions is a migration this PR is not the place for.
The keys list gained typed error attribution while the Key Info screen kept ending a restore on any MessageType::Error, so the two screens hosting the same identity-scoped offer disagreed about whether that restore had ended. An unrelated task's failure still re-armed Restore on one of them. Attribution moves onto LegacyRecoveryState::owns_error, which already knows the identity it belongs to, and both screens call it from display_task_error. One implementation, so the two hosts cannot drift; returning false keeps AppState's banner, because the user still has to see the error. Key Info keeps its own blanket handling for the separate protection-migration gate, which is a different state machine with a different in-flight guard.
…e everywhere Two reviewers found the same hole in the two-candidate held-state lookup. It derived its fallback target by looking key_id up in the main identity's key set even while checking a voter-identity row, and KeyStorage::has only proves a slot is filled, not whose material fills it. A main identity holding a voting key at id N alongside a linked voting identity holding its own key at id N would report the main key as held on the strength of the other key's private half — and hand that material to Key Info. The loop already has the key, so the fallback comes from key.purpose(), and each candidate now has to hold this exact public key. Reverting to slot presence fails the new test. AC-6's third surface: the Key Info page still named its key by raw Debug purpose, so the list and the restore offer agreed with each other and not with the page they open. It now takes the same identity-scoped vocabulary, with the raw purpose kept as Expert detail rather than dropped. The CHANGELOG's known-limitation note said what the app does internally and left the reader nothing to do about it. It now names the situation, what can go wrong, and the check that resolves it — and drops the claim that saving uses "the older of the two places", which is not what makes saving risky. The collision with a same-numbered key is.
The restore offer is hosted by three screens — the keys list, Key Info, and masternode detail — and each names the keys it offers to bring back. Two of them were scoped to the identity's vocabulary; Key Info was not. A plain user's transfer key was therefore offered as a "Payout address key" there, asserting the user owns a masternode, and contradicting the keys list one screen away, which calls the same key a "Transfer key". Someone deciding whether to restore a key should not have to work out that the two names are the same key. The literal fix is one argument, but a third site forgetting the same call is a design problem, so the footgun goes with it: `vocabulary` moves from a builder option defaulting to the masternode wording to a required constructor argument. Omitting it is now a compile error rather than a plausible-looking wrong label, and a fourth host cannot inherit masternode words by accident. A required argument catches a *missing* vocabulary, not a *wrong* one, which is exactly what happened here — so the guard is at the host: the new test drives Key Info through the route the user takes, arms the offer, and reads the words off the rendered offer. Reinstating the wrong vocabulary fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stored copy of a public key is a snapshot taken when its private half was saved. A Platform identity public key is immutable once added with one exception — disabling rewrites `disabled_at` — so full-struct equality between the snapshot and the live key stopped matching the moment a key was disabled or rotated, and the row reported a key this device demonstrably holds as missing. Same symptom as the voter-identity lookup bug, different trigger. It fails safe, understating possession rather than over-, which is why it survived the earlier fix. `same_key` therefore excludes the one field that legitimately moves, and nothing else. Comparing just the id and the key material would also fix this and reopen a worse hole in the other direction: `id` is already the lookup key, and a main identity's voting key and a linked voter identity's key can carry identical `data`, leaving `purpose` as the only thing telling them apart. Conflating those reports the main identity's key as held on the strength of a different key's private half and hands that material to Key Info — the exact defect this lookup exists to prevent. Verified both ways: the narrow id+data comparison passes the new disabled-key test and fails `a_same_numbered_key_on_the_voter_identity_is_not_mistaken_for_this_one`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…list gave it The detail view's key rows already knew where each key's private half was — they looked there to decide whether it was held — but dropped that on the way to `KeyInfoScreen`, which then re-derived the location from the key's purpose. The two disagree for a key filed on the voter identity whose purpose is not `VOTING`, and `role_label_and_tip` exists precisely because that shape occurs: a key on a voter identity is the node's voting key whatever its purpose field says. So the list said "Voting key" and the page one click later said "Authentication key", about one key. The same wrong target drives the page's own re-read, so it also reported a key the device demonstrably holds as missing — the identity-path defect, on the masternode path. Forwarding the row's target closes both. The new test drives the real route (card → detail → key row → Key Info) rather than the pieces, since this drifted where nothing crossed the two surfaces; reverting the fix fails it on the name assertion. Also corrects a comment falsified by this branch: the identity keys list is no longer "the static read-only KeysScreen table". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s surface `identity_keys()`'s doc comment claimed that sharing key enumeration is "what keeps the two surfaces agreeing on which keys are held". It wasn't true. Enumeration was shared; resolution was not. The identity keys list tried both filing conventions and verified the match, while the masternode detail view passed the structural target straight through — so a voting key held on the main identity but filed by purpose derivation, which is what entering one by hand produces, read as saved on the keys list and missing on the node's page. So `filed_at` and `same_key` move out of `keys_screen` to sit beside `identity_keys` as `key_filed_at`, and both surfaces call it. Enumerate alike, resolve alike. The doc comments on both now describe what the code does: the structural target is half of pairing a public key with its private material, and this is the other half. Only reads change. The two conventions still disagree on the write path; reconciling that is a migration, tracked separately. Coverage at both levels: a unit test next to the function pins all four properties of the rule (either convention found, wrong key at the same id rejected, `disabled_at` tolerated), and a kittest drives the node's real route to prove the surfaces now agree. Reverting `detail_screen` to the raw structural target fails the kittest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… silent gap `same_key` compares an identity public key field by field, deliberately skipping `disabled_at`. Written as a chain of accessor calls, a field added upstream would simply not be compared: two keys differing only in that field would compare equal, and this function decides which stored private material belongs to which key. Destructuring `IdentityPublicKeyV0` exhaustively, without `..`, moves that decision to whoever adds the field — the build stops until they say whether it identifies a key or, like `disabled_at`, only describes its state. Verified the guard bites: dropping a field from the pattern fails with E0027 "pattern does not mention field", which is exactly what an upstream addition would produce. Behaviour is unchanged, so the existing tests hold as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lklimek
marked this pull request as ready for review
July 30, 2026 08:03
Collaborator
|
🔍 Review in progress — actively reviewing now (commit af84ea6) |
lklimek
added
the
claudius-review
Contributor
📝 WalkthroughWalkthroughIdentity key management now uses qualified identities, per-key navigation, vocabulary-aware labels, and resolved private-key targets. KeysScreen coordinates legacy recovery offers, backend results, identity reloads, and refreshes. Settings, transfer, and masternode screens provide entry points with expanded kittest coverage and documentation. ChangesIdentity key management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant KeysScreen
participant RecoveryBackend
participant KeyInfoScreen
Settings->>KeysScreen: open qualified identity
KeysScreen->>RecoveryBackend: check or restore missing keys
RecoveryBackend-->>KeysScreen: result or error
KeysScreen->>KeyInfoScreen: open selected key with resolved target
KeyInfoScreen-->>KeysScreen: return and refresh
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review — 20 findings (0 critical, 0 high, 10 medium, 7 low, 3 info)
Let me open with something I do not say often: this is good work. The core resolver is genuinely well built. same_key's exhaustive destructure without .. — so that a field added upstream breaks the build rather than silently widening a key match — is exactly the paranoia a wallet deserves, and the reasoning is argued in rustdoc rather than asserted. Excluding disabled_at cannot produce a false positive, because data is still compared and that is what actually identifies a key. key_filed_at reads only the public half, so a per-frame resolution never drags private bytes out of the vault. No plaintext keys, seeds or mnemonics anywhere in the diff. Typed TaskError matching throughout, no error-string parsing, no tokio::spawn smuggled past BackendTask, no secret-seam bypass. The kittest coverage asserts rendered content rather than merely driving code paths.
Now the part where I earn my keep.
All three reviewers independently converged on the same theme: the PR builds the right rule and then does not apply it everywhere. That is not a stylistic quibble — every finding below is a place where this PR's own newly-declared invariant is contradicted by this PR's own code.
The four I would fix before merge:
| Finding | Why | |
|---|---|---|
| 🟠 | SEC-001 — filed_at.unwrap_or(target) in keys_screen.rs:222 and detail_screen.rs:737 |
key_filed_at returning None is a positive statement that no material for this key is held. The fallback then does an unchecked map lookup on that same slot — so a row reading "not saved on this device" can hand KeyInfoScreen another key's private key to display, sign with, and delete. |
| 🟠 | CALL-001 — owns_error applied to 2 of 3 hosts |
The rustdoc you just wrote says "every screen hosting an offer has to answer it the same way". list_screen.rs:632 binds _error and discards it. The invariant ships already violated. |
| 🟠 | QA-001 — first_protectable_key never got the resolver |
Combined with && short-circuiting, the "Add password protection…" button is not merely misrouted — it is never rendered, for exactly the key shape your own comments cite as the motivation for key_filed_at. Status line says "unprotected"; there is no button. |
| 🟠 | SEC-003 — KeyInfoScreen::refresh() is empty |
You added new state to refresh_on_arrival, a hook AppState dispatches only for root screens. KeyInfoScreen is never one. Your sibling screen in this same PR documents that exact rule and applies it correctly. The two regression tests call the hook by hand, so green CI proves nothing here. |
Worth resolving shortly after: SEC-002 (reads go through the resolved target, writes and deletes do not — the silent no-op on "Remove private key" is the bit the documented residual doesn't cover), DOC-001 ("the reconciliation TODO" exists nowhere in the repo — please file the issue and cite it), DOC-002 (IDN-008 is the story this PR actually implements and now misdescribes it in both directions), CODE-001 (three hand-maintained copies of the recovery-host boilerplate — CALL-001 is the receipt).
Longer horizon: PROJ-001 (a pure, cross-domain key-resolution rule living inside the Expert-gated masternodes screen module — the four-line doc header apologising for it is the placement policy pushing back), PROJ-003 (i18n fragment assembly, blast radius newly quadrupled), plus PROJ-002, CODE-002, QA-002 and RUST-001 in the full report.
Ten MEDIUM findings are posted inline. The seven LOW and three INFO items — including a stale src/ui/components/README.md signature, ephemeral review IDs baked into test doc comments, a PARENT_CRUMB constant not used for its own crumb, and an untested tie-break in key_filed_at — live in the full report rather than cluttering the diff.
Method note: every finding was verified by reading head-vs-base sources. Nothing was compiled or executed — this review runs in an ephemeral sandbox with no warm Rust cache, and this repository's CI already runs the full suite on the PR as the verification backstop. Where a claim could not be settled without execution (CODE-002), the finding says so and is rated conservatively.
Fix the four, and this is a genuinely strong piece of work. The rule you built is right; it just needs to be obeyed in all the places you declared it applies.
🤖 Reviewed by Claudius the Magnificent — three parallel specialists (security, project consistency, adversarial QA), consolidated and severity-adjudicated. Full report: review-report/report.html.
| None => button, | ||
| }; | ||
| if button.clicked() { | ||
| let opened_at = filed_at.clone().unwrap_or_else(|| target.clone()); |
Contributor
There was a problem hiding this comment.
🟠 MEDIUM — SEC-001: the unwrap_or fallback quietly undoes everything key_filed_at just did for you
You built a resolver whose entire virtue is that it accepts a slot only when same_key confirms the stored public half actually belongs to this key. Then, two lines later, you throw that away:
let filed_at = key_filed_at(&self.identity, &target, &key);
let held = if filed_at.is_some() { HELD } else { NOT_HELD };
// ...
let opened_at = filed_at.clone().unwrap_or_else(|| target.clone());
let holding = self.identity.private_keys
.get_cloned_private_key_data_and_wallet_info(&(opened_at.clone(), key.id()));get_cloned_private_key_data_and_wallet_info is a bare map lookup. It performs no same_key check. So filed_at == None has two causes and this code cannot tell them apart:
- (a) the slot is empty — the fetch returns
None. Harmless. - (b) the slot is occupied by a different key —
same_keyrejected it, yet the fetch happily returnsSome(that other key's PrivateKeyData).
Case (b) is precisely the state the shared resolver exists to detect. Reachable shape on a masternode: the main identity carries a Purpose::VOTING key at id N whose private half was filed under the purpose-derived convention at (PrivateKeyOnVoterIdentity, N), while the linked voter identity has its own, different key at the same id N. Both probes land on the same slot, same_key correctly returns false, the row correctly renders "not saved on this device" — and then opened_at falls back and hands KeyInfoScreen the main identity's voting key material.
detail_screen.rs:737 has the identical shape (key_filed_at(...).unwrap_or(target)).
Why this matters more than a UI glitch: the user reads "this key is not saved on this device", clicks through to find out what to do about it, and lands on a screen showing a copyable WIF — for a different key of the same node. "Sign Message" signs with it. "Remove private key from DET" deletes it. A user tidying up a key they were told they don't hold destroys one they do.
Suggested fix — None is a positive statement, so carry it through:
let holding = filed_at.as_ref().and_then(|at| {
self.identity.private_keys
.get_cloned_private_key_data_and_wallet_info(&(at.clone(), key.id()))
});…and only call .with_target(...) when filed_at is Some. Then add a kittest whose fixture puts key A's material in the slot key B structurally occupies and asserts the pushed KeyInfoScreen has private_key_data == None. It must go RED against current master-of-this-branch first, or it isn't pinning anything.
(Verified by reading head sources; nothing was compiled in this environment — CI owns that.)
| .get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id())) | ||
| .is_some() | ||
| }) | ||
| identity_keys(&self.identity) |
Contributor
There was a problem hiding this comment.
🟠 MEDIUM — QA-001: first_protectable_key never got the resolver, and && short-circuiting turns that into a vanishing button
You updated open_key_info_with_mode (line 737) to go through key_filed_at. first_protectable_key got only the cosmetic move from self.identity_keys() to the shared identity_keys(&self.identity) — its lookup is still raw and structural-only:
identity_keys(&self.identity)
.into_iter()
.find(|(target, key)| {
self.identity.private_keys
.get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id()))
.is_some()
})On its own that would just mean a wrong pick. The call site at lines 659-664 makes it worse:
if tier.offers_add_protection()
&& let Some((target, key)) = self.first_protectable_key()
&& ui.button("Add password protection…").clicked()&& short-circuits, so when first_protectable_key() returns None the ui.button(...) call never executes — the button is not merely unclickable, it is never rendered. Meanwhile tier.offers_add_protection() is computed independently by protection_tier(), which walks private_keys.keys_set() directly and is therefore correct.
Net effect for a node whose only held key is filed exclusively under the derived convention — which is to say, "a main-identity voting key entered by hand", the exact scenario your own comment at line 734 cites as the motivation for key_filed_at: the status line reads "Keys: unprotected" and there is no button to do anything about it. No error, no explanation, no CTA. A security-hardening feature made silently unreachable for the very configuration this PR was built around.
Suggested fix: resolve held-ness through key_filed_at inside first_protectable_key, mirroring open_key_info_with_mode. While you're there, consider lifting ui.button(...) out of the && chain — a rendering call hiding inside a condition chain is a foot-gun that will bite again the next time one of those predicates gets stricter.
| // app would have used. That it can therefore remove a | ||
| // different key's material, when a voter identity carries | ||
| // the same key id, is the known residual of the split | ||
| // conventions — the reconciliation TODO owns it. |
Contributor
There was a problem hiding this comment.
🟠 MEDIUM — SEC-002: reads now go through self.target(), writes and deletes still go through the derived one. "Remove private key" can silently no-op.
I want to be fair to the deliberate part first: the PR body says vault write/delete stays on the purpose-derived convention and the stacked follow-up replaces it wholesale. That's a defensible call, and this comment documents it. What that decision does not cover is the failure mode it creates in the meantime.
At the base revision, with_target/fn target don't exist and purpose().into() is used everywhere — so display and delete pointed at the same slot and agreed with each other. This PR routes the read paths through self.target() and leaves the write paths on purpose().into(). The divergence between them is new.
The two conventions differ for exactly one shape: a Purpose::VOTING key on the main identity. Not hypothetical — this PR's own kittest key_info_agrees_with_the_list_about_a_voting_key_held_on_the_main_identity builds precisely it. On that fixture the screen opens with target = Main, displays the material from (Main, 0), renders "Remove private key from DET" — and removes (Voter, 0). Two outcomes:
(Voter, 0)is empty → silent no-op.self.private_key_data = Nonemakes the screen look like the key is gone, the clone is persisted, and the material stays on disk at(Main, 0). The keys list one screen away will cheerfully report it as saved on this device on the next visit.(Voter, 0)holds the voter identity's own key → that key's private half is destroyed instead, while the displayed one survives.
This comment acknowledges (2). Outcome (1) is the one that worries me more, and it isn't mentioned: "I removed my private key from this device" is a security action a user takes before handing over a machine or reducing exposure. Reporting success when nothing happened is the single worst way for it to fail. The add path has the same shape in reverse — a key entered here lands in a slot this screen doesn't read back, so it reverts to "Enter Private Key" for a key it just saved.
Suggested fix — either:
- (a) move write/delete onto
self.target()and migrateQualifiedIdentity::sign/can_sign_within the same change; or - (b) if that must wait for the stacked PR, keep this PR internally consistent: gate the Remove control off, or fail closed with an explicit message, whenever
self.target() != self.key.purpose().into(). Offering a destructive action on a key whose read and write slots disagree is the bit that shouldn't ship.
Either way, worth a test that opens a VOTING-on-main key, presses Remove, and asserts the stored record no longer holds the material the screen displayed.
| // `QualifiedIdentity::sign` and `can_sign_with` look the key up that | ||
| // way, so material stored anywhere else is material that can never | ||
| // sign. Writing the resolved target instead needs every reader | ||
| // migrated with it — see the reconciliation TODO. |
Contributor
There was a problem hiding this comment.
🟠 MEDIUM — DOC-001: "the reconciliation TODO" does not exist
Two comments defer a vault write/delete correctness residual to a tracker that isn't in the repository:
- here, line 1090 — "see the reconciliation TODO"
- line 1298 — "the reconciliation TODO owns it"
Grepping the whole repo for TODO finds no such marker. The nearest things are prose in src/ui/masternodes/mod.rs:261 ("tracked separately") and unrelated wallet/banner reconciliation. git blame won't help the next reader either — the referent was never committed.
CHANGELOG.md:119-120 does the same thing in user-facing prose: "This will be closed by the in-progress key-placement resolution fix" — no issue number, in a changelog that cites #942 and #889 by number four lines away.
This is a deliberate, documented residual on a path that deletes private key material. Documenting it was the right instinct; the problem is that a residual naming no tracker is a residual nobody is assigned to close. After merge it survives as prose only, and the known limitation the CHANGELOG warns users about has no issue a maintainer can triage or schedule.
Suggested fix: file the follow-up issue, replace both comments with TODO(#NNN):, and cite the same number at CHANGELOG.md:119 where #942 already sets the precedent. (Please use the issue number, not a review-finding ID — those are reassigned every run and go dead on merge.)
| /// Reads the stored public half only, never the private one: fetching the entry | ||
| /// would clone the raw private key out of the vault unscrubbed, and this runs | ||
| /// every frame for every key. | ||
| pub fn key_filed_at( |
Contributor
There was a problem hiding this comment.
🟠 MEDIUM — PROJ-001: a cross-domain key-resolution rule parked inside the Expert-gated masternodes screen module
identity_keys, same_key and key_filed_at are pure, stateless derivations over QualifiedIdentity and its KeyStorage. No egui. No AppContext. No Sdk. No DB. And they now live in src/ui/masternodes/mod.rs — an Expert-Mode-gated screen domain module — while being consumed from ui/identities/keys/keys_screen.rs, key_info_screen.rs and ui/components/legacy_recovery_section.rs.
CLAUDE.md's DET Module Placement Policy puts stateless data derivation in model/, and names "business logic (signing, filtering, state derivation) in UI or database layers" as an anti-pattern not to add new instances of. Deciding which key store holds a key's private half is state derivation by any reading. And the precedent is one directory over: QualifiedIdentity::masternode_key_presence() (src/model/qualified_identity/mod.rs:579) derives exactly this class of fact and lives in model/.
The module's own doc header is the tell. It now needs four lines to explain why an Expert-gated module exports helpers that ungated screens depend on:
The gate covers the screens, not this module's shared key helpers […] those name, enumerate and resolve the keys of any identity and are used from ungated surfaces…
That's not a documentation gap; that's the placement policy pushing back. The visibility spread reinforces it — identity_keys/key_filed_at/manage_keys_labels/role_label_and_tip/disambiguate_role_labels are pub, key_role_label is pub(crate), same_key is private. Six functions, three visibilities, one conceptual rule.
Why it's worth moving now rather than later: the next developer asking "how does this app decide a key is held on this device?" has no reason to open the masternodes screen module. A second, divergent implementation is precisely how the bug this PR fixes came to exist — your own commit message says so. Also, the rule's unit tests currently hand-build a full QualifiedIdentity inside a UI module's #[cfg(test)] block to exercise a pure function; in model/ that fixture would be shared with the existing masternode_key_presence tests.
Suggested fix: move identity_keys, same_key and key_filed_at (with their tests) into src/model/qualified_identity/, ideally as QualifiedIdentity methods next to masternode_key_presence. Keep the presentational vocabulary in the UI layer, but out of ui/masternodes/ — it names keys for every identity type now, so something like src/ui/identities/keys/vocabulary.rs matches what it actually does. Settle the family on one visibility while you're in there.
Contributor
github-actions
Bot
removed
the
claudius-review
…navigation # Conflicts: # CHANGELOG.md # docs/ai-design/2026-07-28-legacy-identity-recovery/design.md # docs/user-stories.md # src/backend_task/identity/recover_legacy_keys.rs # src/model/legacy_recovery.rs # src/ui/components/legacy_recovery_section.rs # src/ui/identities/keys/key_info_screen.rs # src/ui/masternodes/detail_screen.rs # src/ui/masternodes/list_screen.rs # src/ui/masternodes/mod.rs # src/ui/state/legacy_recovery.rs # tests/kittest/legacy_recovery_section.rs
4 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/masternodes/detail_screen.rs`:
- Around line 707-717: Update first_protectable_key to use the same
key-file-location resolver as open_key_info_with_mode, including key_filed_at
when determining whether a held key is protectable. Preserve returning the first
matching PrivateKeyTarget/key pair, and ensure the Add password protection
button in the surrounding UI is not conditionally skipped solely because this
predicate returns None.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ef58bfbf-9e58-418f-a4a2-50b86626af5b
📒 Files selected for processing (17)
CHANGELOG.mddocs/ai-design/2026-07-28-legacy-identity-recovery/design.mddocs/user-stories.mdsrc/ui/components/legacy_recovery_section.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/identities/keys/keys_screen.rssrc/ui/identities/transfer_screen.rssrc/ui/identity/settings.rssrc/ui/masternodes/detail_screen.rssrc/ui/masternodes/mod.rssrc/ui/mod.rssrc/ui/state/legacy_recovery.rstests/kittest/keys_screen.rstests/kittest/legacy_recovery_section.rstests/kittest/main.rstests/kittest/masternode_tab.rstests/kittest/transfer_screen.rs
Comment on lines
707
to
+717
| fn first_protectable_key( | ||
| &self, | ||
| ) -> Option<(PrivateKeyTarget, dash_sdk::platform::IdentityPublicKey)> { | ||
| self.identity_keys().into_iter().find(|(target, key)| { | ||
| self.identity | ||
| .private_keys | ||
| .get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id())) | ||
| .is_some() | ||
| }) | ||
| identity_keys(&self.identity) | ||
| .into_iter() | ||
| .find(|(target, key)| { | ||
| self.identity | ||
| .private_keys | ||
| .get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id())) | ||
| .is_some() | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
first_protectable_key still bypasses key_filed_at.
open_key_info_with_mode (Line 751) now resolves the filing location, but this lookup remains structural-only. Combined with the && chain at Lines 673-675 — where ui.button(...) only executes if this returns Some — a node whose only held key is filed under the purpose-derived convention shows "Keys: unprotected" with no Add-protection CTA at all.
♻️ Align with the shared resolver
identity_keys(&self.identity)
.into_iter()
.find(|(target, key)| {
- self.identity
- .private_keys
- .get_cloned_private_key_data_and_wallet_info(&(target.clone(), key.id()))
- .is_some()
+ key_filed_at(&self.identity, target, key).is_some_and(|filed_at| {
+ self.identity
+ .private_keys
+ .get_cloned_private_key_data_and_wallet_info(&(filed_at, key.id()))
+ .is_some()
+ })
})Also consider lifting ui.button("Add password protection…") out of the && chain so a stricter predicate cannot silently remove the control.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/masternodes/detail_screen.rs` around lines 707 - 717, Update
first_protectable_key to use the same key-file-location resolver as
open_key_info_with_mode, including key_filed_at when determining whether a held
key is protectable. Preserve returning the first matching PrivateKeyTarget/key
pair, and ensure the Add password protection button in the surrounding UI is not
conditionally skipped solely because this predicate returns None.
Collaborator
There was a problem hiding this comment.
Verified against the merged PR head af84ea6b7a3ef276ef8f416c93eee6ea2dbafa3e (merge commit 06e64e774b5c1ae51a51413bc42857e689863821): this finding remains valid. first_protectable_key still does the raw (target, key_id) lookup, while the Add-protection button is still inside the short-circuiting && let Some(...) chain. The three commits added after the review did not touch this path, so a held key found only through key_filed_at can still show “Keys: unprotected” without the CTA. Since #945 is now merged, this needs a follow-up fix.
Contributor
There was a problem hiding this comment.
@thepastaclaw, agreed — the follow-up is warranted. first_protectable_key should use key_filed_at before checking private-key material, matching open_key_info_with_mode; the Add-protection control should also remain visible independently of whether a protectable key is found.
Would you like me to open a GitHub issue for the follow-up fix, assigned to you?
🐇🔍
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash-evo-tool PR: 945
File: src/ui/masternodes/detail_screen.rs:716-726
Timestamp: 2026-07-30T18:31:49.789Z
Learning: In `src/ui/masternodes/detail_screen.rs`, any check for whether a masternode key’s private material is held must resolve its storage location with `key_filed_at(&QualifiedIdentity, &PrivateKeyTarget, &IdentityPublicKey)`, rather than directly looking up the structural `(PrivateKeyTarget, key_id)` pair. Private material can be stored under the purpose-derived location, and UI entry points such as the Add password protection flow must use the same resolution rule as `open_key_info_with_mode`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
…e keys whole
Three defects a review found on the Key Info navigation work.
Key Info's arrival re-read hung off `refresh_on_arrival`, which `AppState`
dispatches only to root screens. This screen is always pushed onto the screen
stack, so the reload, the protection-status reset and the offer re-arm were
unreachable: a write landing off-screen was erased by the next ordinary key
edit, which persists the whole identity clone the screen opened with. The body
moves onto `refresh`, which does reach a pushed screen; `refresh_on_arrival`
delegates there by default, so both hooks still run it. The regression test
moves to kittest and drives the app's own `TaskResult::Refresh` dispatch, since
hand-calling the hook passed against the bug.
The offer to restore stranded keys was hand-copied across its three hosts:
render, result routing, error routing, dispatch. The render chain becomes
`legacy_recovery_section::host_offer` and the attribution rules become
`LegacyRecoveryState::absorb_result` / `absorb_error`, leaving each screen only
its own side-effects. A shared rule applied to two hosts of three is what caused
an earlier bug here; a fourth host now cannot repeat it.
Key role labels were assembled from fragments — a bare role word, an English
noun, a state, a positional key id — and two surfaces interpolated a raw
`Purpose` through `Debug`. `role_label_and_tip` now returns whole `&'static str`
captions keyed on vocabulary, purpose and retired state, so composing one at a
callsite is impossible rather than discouraged, and matches every `Purpose` by
name so a variant added upstream breaks this build instead of reaching a user as
`{:?}`. The raw purpose keeps its place on Key Info as its own Expert field.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The catalog entry for viewing identity keys still described criteria that predate a reachable route into a key's own page — this branch is what first gives `KeysScreen` a way into `KeyInfoScreen` at all. The real criteria had landed under IDN-020, a legacy-migration story, and IDN-008 also overstated the Everyday view: the new list gates key id, purpose, security level, type and read-only behind the Expert view. Restates the criteria as what ships — reachable from the identity's Settings → Advanced without changing interface mode, every key openable whether or not the device holds it, role and held state in words, on-chain specifics as Expert detail — and cross-references IDN-020 for the restore offer rather than duplicating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…fixes Covers the two user-facing fixes from this round's review-comment pass on PR #945: the stale-clone refresh wiring and the whole-caption key-role labels. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd
lklimek
enabled auto-merge (squash)
July 30, 2026 16:34
lklimek
disabled auto-merge
July 30, 2026 16:35
orchardpaytl
pushed a commit
to orchardpaytl/orchardpay
that referenced
this pull request
Jul 30, 2026
…/holdings consistent (dashpay#889) (dashpay#945) * feat(model): add the legacy identity recovery plan and additive merge Issue dashpay#889: a v0.9.3 identity that was already partially loaded before the upgrade keeps its remaining keys stranded in `data.db`, because the migration skips a present identity wholesale. That rule is correct — three earlier reconcile heuristics each failed by inferring user intent from record shape. `compute_recovery_plan` only lists what the legacy record holds and the modern one does not, keyed on the `(target, key_id)` map slot and on `None` associations; it never decides whether an item should come back. That decision travels back in as an item-level allowlist, so the absence-versus-removal ambiguity lands with the only party holding the provenance. `apply_recovery_plan` is additive by construction, not by discipline: it starts from the fresh modern record, inserts with `or_insert` so the modern copy wins every collision, recomputes candidacy rather than trusting the allowlist, and has no write path at all for alias, status, the dpp identity, DPNS names, or the wallet link. A voting key drags its voter-identity link along, since one without the other cannot vote. M1-M10 from the design doc's test matrix. M2/M5/M6/M9/M10 were confirmed RED against a naive legacy-wins merge before this implementation landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(database): read one legacy identity row through the shared row decoder Issue dashpay#889: the recovery flow needs one identity out of the preserved legacy `data.db`, not the whole table. A second hand-written reader would be free to drift from the importer's idea of which rows are readable — and then recovery could merge from a record the migration never considered importable, or miss one it did. So the per-row decode moves into `decode_identity_row` and both readers call it, along with one shared `SELECT` that binds the same pair of accepted network spellings. `read_identity_row` adds only the `id` predicate on top. `LegacyIdentityLookup` keeps "nothing to recover" and "the row will not decode" apart. An absent table, an absent row, an observed-identity cache row and a NULL blob are all `Absent` — the ordinary answer. Only a genuinely corrupt row is `Unreadable`, so no caller can mistake corruption for an empty record and merge against it. `open_legacy_read_only` is hoisted next to `database/mod.rs`'s existing one, so `SQLITE_OPEN_READ_ONLY` is a property of the one function that opens the file rather than of each caller remembering the flag. The importer's 12 existing row-decode tests are the regression net for the extraction; 8 new tests cover the lookup, including one asserting both readers decode the same row identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(identity): restore keys stranded in the previous version's saved data Issue dashpay#889: an identity that was only partially loaded before the v1.0 upgrade keeps its remaining keys in `data.db`, reachable by nothing — the load form rejects the duplicate ProTxHash, and the per-key screen wants a WIF the user may no longer hold. This is the way in, and it is entirely user-driven. `CheckLegacyRecovery` lists; `RecoverLegacyIdentityData` writes. Detection is read-only, offline and side-effect-free, so a screen can dispatch it on arrival and hide the affordance when the plan comes back empty. Candidacy is recomputed inside the executing task rather than trusted from the caller: only `recomputed-candidates ∩ approved` is merged, so an approval that went stale since the preview is reported rather than acted on, and an item the user never approved is never restored even when it is missing. An empty allowlist is refused, not widened into "restore everything". The protection-downgrade trip that sank an earlier reconcile attempt is unreachable here by branch condition, not by care: the flow branches on the same predicate the at-rest guard evaluates. On a protected identity the password is verified up front through the shipped prompt, the resident-plaintext preflight runs before any vault write, and every merged key is sealed under that password before the record is persisted — so the encoder never sees plaintext on a protected identity. On a keyless identity there is no protected key for the guard to protect. Cancel, wrong password and headless all fail closed with the stored record byte-identical. Exactly one record write, so the merge either lands whole or changes nothing. No new persisted state: eligibility recomputes from the two existing stores, so a re-run is a no-op and `data.db` — opened read-only — stays a repeatable recovery source. `reject_resident_identity_plaintext` widens to `pub(super)`; it now guards both seal boundaries. Two test-only readers expose the at-rest blob and the wallet link, which the hydrated record cannot show. B1-B12 from the design doc's test matrix, on the offline wired `AppContext` harness, plus the excluded-key and empty-allowlist cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * feat(masternodes): offer the previous version's stranded keys on the node page Issue dashpay#889: the restore path exists but nothing reaches it. The node detail page is the canonical surface — its operator is the one holding a node that cannot vote because its voting key stayed behind in the previous version's saved data. The offer is passive and contextual, never a launch-time nag. On opening a node the view dispatches the read-only detection task once, and only on an install that actually has a previous-version database; the section renders in the Keys area only when detection found something, and retires itself once a restore lands, because eligibility recomputes from the two stores every time. For a node with no voting key that becomes the primary remedy: the missing-voter message points at restoring, which asks nothing of an operator who no longer has the key, and the existing WIF prompt keeps its place as the fallback for one who does. `LegacyRecoveryState` (`ui/state/`) owns the fetch state — dispatch-once, in-flight, offered, retryable — and `LegacyRecoverySection` (`ui/components/`) renders it, per the placement rule's render/no-render split. A failed restore returns to its offer so a mistyped identity password can be corrected and retried; a failed detection does not re-ask. A preview result is routed into the open detail view rather than triggering the list screen's reload-and-reopen, which would rebuild the view, re-dispatch its check and never settle. A completed restore does reload, since the store changed, and the rebuilt view's fresh check finds nothing left. `RecoveryItemDescriptor::is_voting_key` factors out the rule `label` already applied, so "is this the voting key" has one definition. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * feat(identities): offer stranded legacy keys on the Key Info screen Issue dashpay#889: the node page covers the masternode case, but a `User` identity that was partially loaded before the upgrade has no node page. The Key Info screen is the issue's own suggested location and the one surface every identity type reaches — from the Identities list for a user identity, and through Manage keys for a node. The offer is identity-scoped rather than key-scoped, so it renders whatever the opened key's own state is: what it lists are precisely the keys the identity does *not* hold, which no per-key view could show. Both surfaces share `LegacyRecoveryState` and `LegacyRecoverySection`, so detection, the allowlist and the copy have one definition each. Dispatch follows this screen's established queue-then-drain pattern, alongside the protect/unprotect ones. The check and a restore can never contend for the frame's single `AppAction`: the check goes out once on arrival, and a restore only after a press. A completed restore re-probes the protection status, since the restored keys have just landed in the vault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * docs(identity): record the legacy-key recovery flow and close the migration gap Issue dashpay#889: the migration design's §7 named this limitation and deferred it to "a dedicated, provenance-aware flow". That flow now exists, so the limitation gets its resolution rather than staying an open pointer to an issue — with the one clarification a reader of that section needs: the importer itself did not change, skip-if-present still stands, and nothing about recovery runs at migration or launch time. The design record moves into `docs/ai-design/` under this repo's dated-directory convention, so the cross-reference resolves inside the repo. U1 covers the widget from the outside: an empty plan renders nothing, items are named by role with the voter link folded into its voting key, pressing Restore approves exactly the previewed set, an in-flight restore has no button to press twice, unrestorable keys are listed with no restore offered, and an install with no previous-version data never even asks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): only offer legacy keys that still correspond to the identity The plan admitted any legacy key the modern record happened not to hold, keyed on the map slot alone. Nothing checked that the saved private half derived the public half it was stored with, that the public half was still a live key of the identity, or that a wallet-derivation reference named a wallet this install holds — the exact checks the manual "type the WIF" path this flow replaces already enforces. That matters because `masternode_key_presence` reports a role as held from the record alone. Restoring a key the node rotated away from flipped the role to "present", retiring both the recovery offer and the missing-voter remedy, and the operator found out from a rejected transaction. The voter link alone did the same thing with no key behind it at all. Failing candidates now go to `excluded` with their own reason and are never restorable. `AppliedRecovery` also reports that `excluded` list, so an approval naming something that was never restorable is no longer also reported as a stale approval ("already back in place" — the opposite answer), and the caller stops recomputing the same plan to read one field off it. Whatever plaintext the legacy record still holds after a merge is zeroized instead of being dropped intact. The item labels and exclusion copy move out of `model/` into the UI, where they now delegate to the shipped `role_label_and_tip` vocabulary rather than a second mapping that disagreed with it on the same screen ("Payout key" next to "Payout address key"), and two rows in one role are told apart by key id instead of rendering identically. Test fixtures move onto genuinely derived key pairs with the chain's public keys published on the record — a correspondence check that passes against placeholder key material verifies nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(identity): stop holding the storage guard across the password prompt `recover_legacy_identity_data` took `migration_run` for the whole async span, prompt included. The delete path it copied that pattern from is fully synchronous and holds the mutex for microseconds; this held it for as long as a modal sat in front of the user. Two independent reviewers reproduced the fallout: removing an *unrelated* identity failed with "storage is still being updated" when no migration was running, and `finish_unwire`'s awaiting acquire could park indefinitely. The flow is now a preflight and one fully synchronous critical section. The preflight reads, dry-runs the merge to see whether anything would be restored at all, decides whether a password is needed, and prompts — holding nothing. The write section then re-acquires the guard, re-checks the migration state, re-reads the record, merges again and writes, all without an await in between. Re-reading is not just tidiness. The prompt window let any other writer land in between — refresh, DPNS registration, transfer, none of which take the load claim — and the write of a pre-prompt snapshot silently reverted it. It also makes the no-resurrection guarantee explicit: a delete during the prompt now surfaces as `IdentityNotFoundLocally` instead of being undone by the upsert, which until now held only as a side effect of how long the lock was kept. The at-rest protection predicate is evaluated over the merged record, the one the encoder's downgrade guard actually sees, rather than over the pre-merge record. If the identity gained password protection during the prompt the verified password no longer covers what is about to be written, so the restore stops with a typed `LegacyRecoveryIdentityChanged` rather than sealing under a password for a different state. Also: the nothing-to-recover success no longer records a `Failed` load, the preview zeroizes the stranded plaintext it decodes on every screen open instead of releasing it intact, and the "nothing restored" banner is worded to be true of both outcomes that produce it. Both concurrency tests were confirmed RED against the old lock scope, reproducing the reviewers' exact failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(ui): keep the restore offer and the record it wrote in step Three ways the two shipped surfaces disagreed with the store after a restore, and the state machine behind them had no tests at all. The Key Info screen keeps the identity clone it opened with, and its own add-key and remove-key paths persist that whole clone. This feature added a second writer to the same record reachable from the same screen: restore keys, then add or remove any key, and the pre-restore copy was written back — the restored keys gone, right after a banner said they were back. The completion now re-reads the record and the key on screen from it. The node page never hears about a restore run from the Key Info screen it pushed: the pushed screen is on top, so it takes the result. Coming back left the page offering keys already restored and warning about a voting key it now held. Arrival re-reads the node and re-arms its check; vote selections and any open prompt survive, which a blanket rebuild would have discarded. The `recovery_completed()` call in the result handler goes with it — the reload that follows re-opens the view and re-arms the check anyway, so it was mutating a view discarded three statements later. Writing the state machine's tests turned up a third one: `completed()` re-armed unconditionally, so calling it on an install with no previous-version database armed a check that must never run. Harmless until arrival started calling it; now `Unavailable` stays unavailable. The Restore button loses its ellipsis: on a keyless identity — the common case this feature exists for — it commits immediately rather than opening anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * docs(identity): record what the recovery flow actually shipped The committed design record was landed marked "implemented" with exactly one edit — the status line — so the repo's permanent account of this feature described a dispatch model, a progress mechanism, a module set and a gating rule that were all deliberately changed, plus the pre-review merge rules that the fix round replaced. A §10 now records all nine departures with the reasoning that produced them, rather than rewriting §1–§9 into a story nobody actually followed; the status line says which of the two is authoritative. §7's claim that `finish_unwire.rs` is untouched is reconciled with §4.2's hoist in the same document: the importer's behaviour is unchanged, the file is not. `reject_resident_identity_plaintext`'s header claimed to guard "every boundary that seals an identity's keys". It does not — the merge-load path seals without it. The header now says which boundaries it covers, and that path carries a TODO describing the gap. Fixing it is a separate issue: it is pre-existing code, unrelated to this feature. Also: user story and CHANGELOG pick up the correspondence check and the non-blocking prompt, the components catalog names the copy this feature's component now owns, and three private rustdoc blocks come back inside the internal-commentary budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): stop the legacy blob from vouching for its own voter key The correspondence check that keeps a rotated-away key out of the offer resolved a voter/operator-target key's identity as "the modern link, or the legacy one if there is no modern link". In the case the check exists for -- a masternode whose record carries no voter link -- that fallback read the identity snapshot out of the same unauthenticated legacy blob as the key under test, so any self-consistent pair passed. A voting key the chain retired months before the upgrade still reads as live in that snapshot, and restoring it flips the node's voting role to "present", retiring the manual remedy that would actually fix the node. reference_identity now reads the modern record only. A key on a voter or operator identity the modern record does not link to has no admissible witness and is excluded as LinkedIdentityUnverified, carrying the same "load this identity again and enter the key" remedy as its neighbours. Fetching the linked identity from Platform was rejected twice over. It would put the network in the middle of an offline, read-only preview that every screen arrival dispatches, and model/ is pure by contract. More decisively, it would not establish the property: a voter identity's id is derived from its voting key (Identifier::create_voter_identifier), so rotating the key on-chain creates a *different* voter identity rather than retiring a key on the existing one. Fetching the legacy-named voter identity would confirm the stale key against its own orphaned identity. Behaviour change worth noticing on its own: a masternode loaded from its ProTxHash alone no longer gets its voter-identity-target voting key back in one click. The key is listed as unverifiable, the voter link goes with it, the node keeps reporting the role as missing, and render_missing_voter keeps offering manual entry -- the honest outcome, since nothing available offline can tell that key from one the chain replaced. A record that does carry the voter link (a re-load keeps it) restores the key as before. Grouping follows the same rule: voter_association_is_grouped now keys on the voting role, the predicate that already decides whether the link is worth offering, so the two cannot disagree about which key the link belongs to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(identity): gate the recovery offer on legacy rows, not on data.db existing FetchState::Unavailable was documented as the state a fresh install never leaves, but no shipped install could reach it. AppState::boot_inputs opens an existing data.db read-only *or* creates one and runs the fresh-install schema ladder, so db_file_path().exists() is true for every user, and every identity or node screen dispatched a detection task against a table the ladder had just created empty. The kittest that claimed otherwise passed only because the `testing` feature substitutes an in-memory database whose path is None -- a test of the substitution, not of the gate. AppContext::has_legacy_identities now asks whether the legacy `identity` table holds a local row for this network: one SELECT EXISTS over the same filter read_identities uses (so "there is something here" and "here it is" cannot drift), false when the table does not exist at all, answered once per context and cached. A probe that errors arms the offer instead of retiring it -- the detection task reports its own typed error, while a silent false would withdraw a recovery the user's data still supports. The kittest is replaced by lib tests over a real file-backed data.db, which assert the premise the old gate missed (the file is there and the gate still says no) and cover the network scoping a file check could never have. Confirmed red against the old gate before the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(ui): re-read the key screen on arrival and attribute a finished restore Two ways a restore's result reached the wrong screen state. KeyInfoScreen had an empty refresh(), which refresh_on_arrival defaults to, so returning to a screen that missed its result never re-read the record. That screen persists the whole identity clone it opened with on every key add or remove, so the clone -- predating the restore -- was written back over the restored keys on the next edit, silently and with a successful save to show for it. It now re-reads on arrival, re-probes the protection line and re-arms its check, exactly as the masternode detail view already does. The LegacyRecoveryCompleted arm discarded identity_id, unlike the LegacyRecoveryCandidates arm directly above it, which routes through LegacyRecoveryState::offered's identity check. Results reach whichever screen is visible when they arrive, not the screen that dispatched them, so a restore dispatched from identity A's Key Info screen could land on identity B's: B showed "your keys have been restored to this identity" for a restore that was never B's, and B's own in-flight recovery state was reset under it. The attribution rule now lives once, in LegacyRecoveryState as completed_for -- the twin of offered -- so both call sites share it rather than each remembering to check. The masternodes list applies the same rule through shows_node: a completion for an identity no card and no open detail names is dropped, banner, reload and detail rebuild included. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * test(identity): pin the mid-flight protection guard against regression TaskError::LegacyRecoveryIdentityChanged is listed in persist_legacy_recovery's rustdoc and spelled out in design.md 10.6, but was the only documented error branch of that function with no test -- every sibling (IdentityLoadInProgress, WalletStorageNotReady, IdentityNotFoundLocally, LegacyIdentityUnreadable, the prompt's cancel and unavailable errors, IdentityKeyProtectionIncomplete, the deleted identity, the undecodable row) has one. B13 drives the 10.6 scenario: the dry run on a keyless identity decides no password is needed, a ProtectIdentityKeys lands before the write section re-reads the record, and the write refuses with the typed error and a byte-identical stored blob. It calls persist_legacy_recovery directly with the None the dry run produced, as B5 calls verify_recovery_password directly: a Tier-1 restore never prompts, so there is no await point between the two where a concurrent task could be made to land deterministically. The dry run is exercised first so the None under test is the one production carries. Verified the test fails when the fail-closed branch is removed, rather than merely restating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd * fix(model): stop the legacy blob from vouching for its own operator key The voter-target fix landed one half of the rule: a key on a voter or operator identity the modern record does not link to has no witness outside the legacy file, so it is excluded. The operator *link* itself was offered with no gate at all, which left the same hole open one step further out. Restoring it writes the legacy file's own claim into the modern record. The next preview then reads that claim back as the outside witness for the same file's operator keys, and every one of them becomes a candidate — self-vouching in two moves instead of one. Nothing in production ever writes `associated_operator_identity` fresh, so an offered operator link was always in that unverifiable state. An operator link the modern record does not carry is now excluded as `LinkedIdentityUnverified`, like the keys held on that identity. Also renames M8, whose name and opening sentence claimed it proved the whole legacy key set is offered while its own assertions exclude two of five items. * fix(masternodes): drop the missing-voter message that can never be shown The node page carried a second missing-voter message offering restore as the primary remedy, chosen whenever the recovery plan held a voting-role candidate. Since the voter-key check began reading the modern record only, that candidate exists only when the record already carries the voter link — and a record carrying the voter link is written in the same branch that writes the voting key, so it is never in the missing-voter state to begin with. The two conditions are mutually exclusive: no user, and no test, has ever seen that message or its companion tooltip. Gone, along with the predicate behind them. The remaining message is the honest one and still matches what the screen offers: the in-place "Add voting key" prompt right underneath it, which needs no re-load. Restoring a voting key held on a separate voter identity needs an on-chain check this flow deliberately does not make; that is issue dashpay#942, not something this branch pretends to do. * fix(ui): let the recovery section read as a whole when it can restore nothing A plan holding only unrestorable items — now the ordinary shape for a masternode loaded from its ProTxHash — opened with "Some keys ... haven't been brought across", followed by no list, no button, and then a second heading for the items it cannot bring back. A lead-in that promises an action the section never offers reads as a broken screen. That case gets its own lead-in, which introduces the list itself, so the second heading appears only when there really are two lists to keep apart. The reason an item cannot be restored also stops being hover-only. It is the single sentence that tells the user what to do instead, and a tooltip reaches neither touch nor keyboard; it is now an inline line under each item. The reason for an unverifiable linked identity is reworded to read correctly for the operator link as well as for a key held on one. * docs(identity): stop promising a voting-key restore that cannot happen The user story and the changelog both said restoring is offered as the first remedy for a node with no voting key, with typing the key in as the fallback. For the flagship case — a masternode loaded from its ProTxHash alone — that is never true: the voting key sits on a separate voting identity that only the previous version's data names, so it is listed as one that cannot be brought back and entering it by hand is the only remedy. Naming owner, voting and payout together in the same breath carried the same implication and is dropped. Both now describe what happens, and point at issue dashpay#942 for the on-chain check that would let the voting-key case be closed safely later. The design record gains the two shipped deviations behind this: §2.1's "restore first, WIF as fallback" branch removed as unreachable, and the operator link excluded as unverifiable for the same reason its keys are. * fix(identity): give the identity keys list a way into each key The identity hub replaced the legacy Identities screen's per-key popup — the only route into KeyInfoScreen that did not depend on the identity already holding a usable key — with Settings > Advanced > "Manage keys". That destination was a read-only table with no route onward, and RootScreenIdentities is deliberately out of the nav, so the popup it still hosts is unreachable. Every remaining route (transfer, withdraw, the token screens) is gated on holding a key of the kind that action needs, which is false for precisely the identities issue dashpay#889's restore offer exists to help: a user identity with stranded keys could reach its own keys only in Developer view, through a send-money screen. KeysScreen now carries the QualifiedIdentity, so it can say which keys this device holds and open KeyInfoScreen with that key's private material. One row per key, named in the shared role vocabulary, held state stated in words rather than by colour alone, ungated. The restore offer renders on the list itself, above the rows: it is scoped to the identity, and making a user pick an arbitrary key to discover an identity-level offer repeats the defect one level up. It stays on KeyInfoScreen too, which self-extinguishes and is where the masternode path lands. The screen also moves inside the app chrome (top panel, left panel, island_central_panel). That is not cosmetic: island_central_panel is the only caller of MessageBanner::show_global, so on a raw CentralPanel the restore reports nothing at all — a failed restore would be indistinguishable from a successful one. key_role_label, manage_keys_labels and the key enumeration move from detail_screen into ui::masternodes beside the vocabulary they belong to, so the two "Manage keys" surfaces cannot drift apart; the masternode kittest covering that list is the regression guard for the move. The transfer screen's no-key branch gains a "Manage keys" signpost. Its available_transfer_keys() gate is untouched — it still correctly says the identity cannot send; the branch simply stops being a dead end. * test(identity): pin the restore outcome and the key naming on the keys list The offer self-extinguishes on the next check whether the restore landed or failed, so its disappearance says nothing about the outcome. Only the banner distinguishes the two, and the banner only reaches the user because the screen is built on island_central_panel — the sole caller of MessageBanner::show_global. Cover both outcomes explicitly, and pin the render channel itself: reverting the screen to a bare CentralPanel fails the new guard, which is what makes it worth having. Also pin AC-6 across the two surfaces that name keys. The restore offer and the keys list render from different modules over different types, and nothing but their shared derivation from role_label_and_tip keeps them saying the same words; a private re-implementation on either side would read plausibly and drift in silence. Key Info remains outside that assertion: it does not retain the PrivateKeyTarget its key was opened with, so it cannot tell a voter-identity key from a main-identity one, and naming it in role words today would risk labelling a voting key as an authentication key. Threading the target through its nine call sites is the fix, and it is wider than this change. * test(identity): report both restore outcomes on the keys list in the running app The component tests prove two things separately: the screen sets a banner on a finished restore, and it renders whatever banner its context carries. Neither closes the seam between them — that the context the screen writes its outcome into is the context it renders from. A restore reporting into one context while the screen renders another would pass both and still tell the user nothing. This drives the real app to the keys list, delivers a completed restore through visible_screen_mut the way AppState does, and asserts the success text appears on the screen; then banners a task error on the app context, as AppState does for a failed restore, and asserts that appears too. The offer retires itself either way, so the banner is the only thing that distinguishes them. Reverting the screen to a bare CentralPanel fails this test, which is what makes it a guard rather than a decoration. * fix(identity): make the keys list and Key Info agree on where a key is filed SEC-001. The keys list resolves each key's PrivateKeyTarget from the identity it walked, then discarded it; KeyInfoScreen re-derived one from the key's purpose. impl From<Purpose> for PrivateKeyTarget sends every voting key to the voter identity, so for a Purpose::VOTING key filed on the main identity — a supported shape, which masternode_key_presence reads as voting readiness on its own — the two disagree. The list reports the key as held, Key Info re-reads the record on arrival, looks in a store the key was never filed under, finds nothing, and reports it missing. On the screen built to answer that question, about a key whose private half is listed one screen earlier. KeyInfoScreen now accepts the resolved target via with_target and uses it for every lookup, falling back to the old derivation when no caller supplied one, so the call sites this change does not touch keep their current behaviour. Only the keys list passes it. The same derivation still governs the pre-existing callers' reads, writes and deletes — including a delete that can land on a different key when voter and main key-id spaces overlap — which is filed separately. Also from QA: - SEC-002: the held/not-held column called get_cloned_private_key_data_and_ wallet_info, copying raw key bytes out of the vault unscrubbed, every frame for every key, to read is_some(). KeyStorage::has answers it without a copy. - QA-005: display_message ended a restore on any MessageType::Error. Results are not screen-affine, so an unrelated task's failure re-armed Restore mid-flight and it could be dispatched twice. Attribution moves to display_task_error, which receives the typed error; it returns false so the user still gets AppState's banner. Residual documented on is_recovery_error: the same variant from another task during a restore still ends it, which needs the task's identity to reach the screen. - QA-003: the completion-banner test asserted only that some banner existed. It now pins attribution instead — another identity's restore reports nothing here — since fresh_app_context drops the harness its context belongs to and the text cannot be read back at that level. The running-app test already pins the text. - QA-006/007: coverage for the transfer-screen signpost and for the Expert-only detail row in both directions. - Empty-state copy said "no keys saved on this device", which in this screen's own per-key vocabulary means held=false. An empty record is a different statement; the test that codified the wrong string is fixed with it. - QA-002: "island_central_panel is the only caller of show_global" is false — contracts_documents_screen calls it directly. Corrected where it was written down as justification. - key_role_label back to pub(crate); ui/masternodes' module header no longer implies its key vocabulary is Expert-Mode-scoped, since an Everyday identity screen depends on it. - Docs: §2.1 and §7 row 1 restored as written, per the document's own proposal-plus-deviations model, with §10.13 carrying the correction. CHANGELOG no longer claims transfer was the only route to a key's page, and no longer promises signing or password protection to an identity that holds no keys yet. * fix(identity): return from a key to a current list, in the identity's own words Track A of round 3. Leaving a key now pops with a refresh, and the keys list re-reads both halves of its state on the way back. Only re-reading the record would show a key restored from the pushed Key Info screen as held while still offering to restore it — and pressing Restore then reports there was nothing left to do. Consolidating into refresh() also retires the refresh_on_arrival override, which never ran: the framework dispatches that hook for root screens, and this screen is pushed. KeyInfoScreen gains a with_parent breadcrumb crumb so the trail leads back to the screen the key was opened from. Parameterised, not hardcoded: it has nine parents and naming one of them for all of them would mislabel the other eight, which keep their two-level trail unchanged. Key role words now follow the identity that owns the key. The DIP-3 wording describes masternode registration duties — updating a registration, receiving node rewards — and on a user identity that does not read as jargon, it asserts the user owns a masternode. A user's TRANSFER key is a Transfer key, not a payout address. Both the keys list and the restore offer take the same vocabulary from the same identity: scoping only one would have a user's offer say "Payout address key" six lines above a list saying "Transfer key", which is the two-names-for-one-key defect the shared vocabulary exists to prevent. Empty-state copy no longer says "no keys saved on this device" — these rows are the identity's on-chain public keys, and that phrase means held=false per-row on this same screen. Track B, reads only, after verifying the read/write split against the source rather than the report: :746 (feeds the vault chokepoint, whose consequence is signing with and displaying the wrong key's material) and :887 (reload) are reads and now resolve the target the caller established. The keys list also resolves held-ness against both conventions, since real installs hold material written under each; looking in a second place can only find material that is already there. The writes are deliberately reverted to the purpose-derived target, which the previous commit had changed. QualifiedIdentity::sign and can_sign_with resolve that way, so material stored anywhere else is material that can never sign: for a voting key filed on the main identity, the previous commit would have taken a hand-entered private key and stored it where the signer never looks. Removal keeps the derived target for the same reason, which leaves wrong-key removal as a documented residual. Both sites carry the reasoning, and reconciling the two conventions is a migration this PR is not the place for. * fix(identity): one attribution rule for both hosts of the recovery offer The keys list gained typed error attribution while the Key Info screen kept ending a restore on any MessageType::Error, so the two screens hosting the same identity-scoped offer disagreed about whether that restore had ended. An unrelated task's failure still re-armed Restore on one of them. Attribution moves onto LegacyRecoveryState::owns_error, which already knows the identity it belongs to, and both screens call it from display_task_error. One implementation, so the two hosts cannot drift; returning false keeps AppState's banner, because the user still has to see the error. Key Info keeps its own blanket handling for the separate protection-migration gate, which is a different state machine with a different in-flight guard. * fix(identity): identify a key by its public half, and name it the same everywhere Two reviewers found the same hole in the two-candidate held-state lookup. It derived its fallback target by looking key_id up in the main identity's key set even while checking a voter-identity row, and KeyStorage::has only proves a slot is filled, not whose material fills it. A main identity holding a voting key at id N alongside a linked voting identity holding its own key at id N would report the main key as held on the strength of the other key's private half — and hand that material to Key Info. The loop already has the key, so the fallback comes from key.purpose(), and each candidate now has to hold this exact public key. Reverting to slot presence fails the new test. AC-6's third surface: the Key Info page still named its key by raw Debug purpose, so the list and the restore offer agreed with each other and not with the page they open. It now takes the same identity-scoped vocabulary, with the raw purpose kept as Expert detail rather than dropped. The CHANGELOG's known-limitation note said what the app does internally and left the reader nothing to do about it. It now names the situation, what can go wrong, and the check that resolves it — and drops the claim that saving uses "the older of the two places", which is not what makes saving risky. The collision with a same-numbered key is. * fix(identity): Key Info names a user's keys the way the keys list does The restore offer is hosted by three screens — the keys list, Key Info, and masternode detail — and each names the keys it offers to bring back. Two of them were scoped to the identity's vocabulary; Key Info was not. A plain user's transfer key was therefore offered as a "Payout address key" there, asserting the user owns a masternode, and contradicting the keys list one screen away, which calls the same key a "Transfer key". Someone deciding whether to restore a key should not have to work out that the two names are the same key. The literal fix is one argument, but a third site forgetting the same call is a design problem, so the footgun goes with it: `vocabulary` moves from a builder option defaulting to the masternode wording to a required constructor argument. Omitting it is now a compile error rather than a plausible-looking wrong label, and a fourth host cannot inherit masternode words by accident. A required argument catches a *missing* vocabulary, not a *wrong* one, which is exactly what happened here — so the guard is at the host: the new test drives Key Info through the route the user takes, arms the offer, and reads the words off the rendered offer. Reinstating the wrong vocabulary fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): a disabled key the device holds is still reported as held The stored copy of a public key is a snapshot taken when its private half was saved. A Platform identity public key is immutable once added with one exception — disabling rewrites `disabled_at` — so full-struct equality between the snapshot and the live key stopped matching the moment a key was disabled or rotated, and the row reported a key this device demonstrably holds as missing. Same symptom as the voter-identity lookup bug, different trigger. It fails safe, understating possession rather than over-, which is why it survived the earlier fix. `same_key` therefore excludes the one field that legitimately moves, and nothing else. Comparing just the id and the key material would also fix this and reopen a worse hole in the other direction: `id` is already the lookup key, and a main identity's voting key and a linked voter identity's key can carry identical `data`, leaving `purpose` as the only thing telling them apart. Conflating those reports the main identity's key as held on the strength of a different key's private half and hands that material to Key Info — the exact defect this lookup exists to prevent. Verified both ways: the narrow id+data comparison passes the new disabled-key test and fails `a_same_numbered_key_on_the_voter_identity_is_not_mistaken_for_this_one`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(masternodes): a key opened from a node's page keeps the name the list gave it The detail view's key rows already knew where each key's private half was — they looked there to decide whether it was held — but dropped that on the way to `KeyInfoScreen`, which then re-derived the location from the key's purpose. The two disagree for a key filed on the voter identity whose purpose is not `VOTING`, and `role_label_and_tip` exists precisely because that shape occurs: a key on a voter identity is the node's voting key whatever its purpose field says. So the list said "Voting key" and the page one click later said "Authentication key", about one key. The same wrong target drives the page's own re-read, so it also reported a key the device demonstrably holds as missing — the identity-path defect, on the masternode path. Forwarding the row's target closes both. The new test drives the real route (card → detail → key row → Key Info) rather than the pieces, since this drifted where nothing crossed the two surfaces; reverting the fix fails it on the name assertion. Also corrects a comment falsified by this branch: the identity keys list is no longer "the static read-only KeysScreen table". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(identity): one held-key resolution rule for every Manage keys surface `identity_keys()`'s doc comment claimed that sharing key enumeration is "what keeps the two surfaces agreeing on which keys are held". It wasn't true. Enumeration was shared; resolution was not. The identity keys list tried both filing conventions and verified the match, while the masternode detail view passed the structural target straight through — so a voting key held on the main identity but filed by purpose derivation, which is what entering one by hand produces, read as saved on the keys list and missing on the node's page. So `filed_at` and `same_key` move out of `keys_screen` to sit beside `identity_keys` as `key_filed_at`, and both surfaces call it. Enumerate alike, resolve alike. The doc comments on both now describe what the code does: the structural target is half of pairing a public key with its private material, and this is the other half. Only reads change. The two conventions still disagree on the write path; reconciling that is a migration, tracked separately. Coverage at both levels: a unit test next to the function pins all four properties of the rule (either convention found, wrong key at the same id rejected, `disabled_at` tolerated), and a kittest drives the node's real route to prove the surfaces now agree. Reverting `detail_screen` to the raw structural target fails the kittest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor(identity): make an upstream key field a build failure, not a silent gap `same_key` compares an identity public key field by field, deliberately skipping `disabled_at`. Written as a chain of accessor calls, a field added upstream would simply not be compared: two keys differing only in that field would compare equal, and this function decides which stored private material belongs to which key. Destructuring `IdentityPublicKeyV0` exhaustively, without `..`, moves that decision to whoever adds the field — the build stops until they say whether it identifies a key or, like `disabled_at`, only describes its state. Verified the guard bites: dropping a field from the pattern fails with E0027 "pattern does not mention field", which is exactly what an upstream addition would produce. Behaviour is unchanged, so the existing tests hold as written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(identity): reach Key Info's re-read, share the restore offer, name keys whole Three defects a review found on the Key Info navigation work. Key Info's arrival re-read hung off `refresh_on_arrival`, which `AppState` dispatches only to root screens. This screen is always pushed onto the screen stack, so the reload, the protection-status reset and the offer re-arm were unreachable: a write landing off-screen was erased by the next ordinary key edit, which persists the whole identity clone the screen opened with. The body moves onto `refresh`, which does reach a pushed screen; `refresh_on_arrival` delegates there by default, so both hooks still run it. The regression test moves to kittest and drives the app's own `TaskResult::Refresh` dispatch, since hand-calling the hook passed against the bug. The offer to restore stranded keys was hand-copied across its three hosts: render, result routing, error routing, dispatch. The render chain becomes `legacy_recovery_section::host_offer` and the attribution rules become `LegacyRecoveryState::absorb_result` / `absorb_error`, leaving each screen only its own side-effects. A shared rule applied to two hosts of three is what caused an earlier bug here; a fourth host now cannot repeat it. Key role labels were assembled from fragments — a bare role word, an English noun, a state, a positional key id — and two surfaces interpolated a raw `Purpose` through `Debug`. `role_label_and_tip` now returns whole `&'static str` captions keyed on vocabulary, purpose and retired state, so composing one at a callsite is impossible rather than discouraged, and matches every `Purpose` by name so a variant added upstream breaks this build instead of reaching a user as `{:?}`. The raw purpose keeps its place on Key Info as its own Expert field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(user-stories): point IDN-008 at the key list that shipped The catalog entry for viewing identity keys still described criteria that predate a reachable route into a key's own page — this branch is what first gives `KeysScreen` a way into `KeyInfoScreen` at all. The real criteria had landed under IDN-020, a legacy-migration story, and IDN-008 also overstated the Everyday view: the new list gates key id, purpose, security level, type and read-only behind the Expert view. Restates the criteria as what ships — reachable from the identity's Settings → Advanced without changing interface mode, every key openable whether or not the device holds it, role and held state in words, on-chain specifics as Expert detail — and cross-references IDN-020 for the restore offer rather than duplicating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(changelog): document the Key Info refresh and label-consistency fixes Covers the two user-facing fixes from this round's review-comment pass on PR dashpay#945: the stale-clone refresh wiring and the whole-caption key-role labels. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9RDy2kYvhYUdFhEYZ1QRd --------- Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 06e64e7)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR: Makes the Key Info screen reachable from every place a key can be seen (not only Developer view), and fixes several places where the keys list and the masternode/Key Info screens could disagree about which key is held, what to call it, or which key's data to show.
User story
As an Everyday User, I want to open a key's details from wherever I can see that key — the identity's keys list or a masternode's page — and see the same name and the same "is this saved on my device" answer every time, to achieve confidence that what the app tells me about my keys is accurate and that I can actually reach the screen that lets me manage them.
Scenario
Base flow
An identity or masternode can hold keys of several kinds (authentication, voting, owner, payout, etc.), some belonging to the identity itself and some to a linked voter or operator identity. Two screens show information about these keys: the identity's own keys list, and — for masternodes — the node's detail page, which links out to the same Key Info screen.
Actual behavior
Expected behavior
Detailed discussion
What was done
Continues #889's legacy-key-recovery work (#941) with a dedicated, standard-view "Manage keys" screen and a set of consistency fixes surfaced during review of that work:
KeysScreen(ui/identities/keys/keys_screen.rs) hosts the identity's key list with standard app chrome (top/left panels), reachable without Developer view; rows and Back use proper stacked-screen refresh so returning from Key Info always re-reads the identity and re-evaluates the recovery offer.key_filed_at/same_key(ui/masternodes/mod.rs), used by both the identity keys list and the masternode detail screen: probes both historical filing conventions for a key and accepts a candidate only when its stored public-key material actually matches — comparing every field exceptdisabled_at(which legitimately changes when a key is disabled on-chain, so excluding it stops a rotated-but-held key from being misreported as not held), while still treating differingpurposeas a genuine mismatch (the one case where two keys can otherwise share both slot id and public-key data).key_role_label/manage_keys_labels) scoped by identity type so a masternode's key is described in masternode terms and a plain identity's key in generic terms, applied consistently across the keys list, the masternode detail page, the Key Info page itself, and both hosts of the "restore stranded keys" offer.LegacyRecoverySection's vocabulary is now a required constructor argument (not an optional default), so a future call site can't silently regress to the wrong wording.KeyInfoScreenreads/signs through the resolved target consistently across all its internal call sites, closing a narrow but real divergence where the screen's display path and its sign/save path could resolve to different stored copies of a key.Testing
Unit and UI (
kittest) coverage added for: navigation/refresh correctness (including the exact stale-offer regression this fixes), the shared resolution rule (both filing conventions, a same-numbered-key collision case, and a disabled-but-held key), vocabulary consistency across all offer/list/detail surfaces, and the empty-keys state.cargo test --all-features --lib --test kittestandcargo clippy --all-features --all-targets -- -D warningsclean;cargo fmt --allapplied.Review status
An automated review posted 10 findings after this branch merged #941's just-landed hardening. Verified independently against current code and cross-referenced against the stacked follow-up PR #946 to avoid duplicate work: 4 were already fixed by #946 (the unsafe fallback in the key-resolution helpers, the vanishing Add-protection CTA, the read/write target divergence in Key Info, and a now-dangling TODO left over from that same divergence) — nothing to do here, they land automatically when this PR merges #946. 1 was superseded by this branch's own merge of #941's error-attribution fix. Of the 5 genuinely open findings, 4 are fixed in this PR:
KeyInfoScreen's refresh-on-return now actually runs (it was wired to a hook the app never dispatches for how this screen is actually shown, so a write that landed off-screen could be silently erased by the next edit); the three recovery-offer hosts' near-duplicated render/dispatch/error-handling code is now a shared helper instead of three hand-copies; key-role captions are now whole, complete phrases instead of assembled fragments (closing an i18n gap and two places a raw internal enum name could reach the screen); anddocs/user-stories.md's IDN-008 now describes what this PR actually ships instead of stale pre-PR criteria. The 5th (relocating a pure key-enumeration helper out of the Expert-gated masternodes module) remains open, deferred as a minor structural cleanup.Breaking changes
None for end users.
LegacyRecoverySection::newgained a required parameter (internal component, not part of any public API) — all call sites updated.role_label_and_tip(internal helper) now returns&'static strinstead ofStringand takes an addeddisabledparameter.Checklist
cargo fmt --allcargo clippy --all-features --all-targets -- -D warningsdocs/user-stories.mdupdated (IDN-020)CHANGELOG.mdupdateddocs/ai-design/2026-07-28-legacy-identity-recovery/)Prior work
CHANGELOG.mdand closed there.Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes