fix: masternode key-protection dialog, nav reset-to-list, and network-switch passphrase wiring#913
Conversation
…etwork switch BackendTask::SwitchNetwork built the new AppContext and immediately wired its wallet backend (ensure_wallet_backend_and_start_spv), which snapshots the context's secret prompt into the SecretAccess chokepoint at construction time (no interior mutability). The real egui prompt host was only installed after the task result returned, by which point the snapshot already happened — so any passphrase-protected operation on a network reached via an in-session switch (not the persisted boot network) permanently fell back to NullSecretPrompt, producing the headless-only "This wallet is protected by a passphrase, which can only be entered in the app window" error inside the actual GUI. The MCP network-switch tool had the identical ordering gap. Propagate the outgoing context's secret prompt onto the new context immediately after creation, before any wallet-backend wiring, in both call sites. Boot-time context creation was already correct and is unaffected. Co-Authored-By: Codex Sol <noreply@openai.com> Co-Authored-By: Claude <noreply@anthropic.com>
Route the aggregate masternode protection action through a KeyInfoScreen constructor that starts the add-protection confirmation flow. Keep the protection section expanded while that flow is active so the next step stays visible. Co-Authored-By: Codex Sol (GPT-5) <noreply@openai.com>
set_main_screen() only called refresh_on_arrival(), which intentionally preserves nested view state for the return-from-sub-screen case. That meant clicking the left-nav or top-nav "Masternodes" entry re-rendered whatever masternode was last viewed instead of the list, since nothing ever reset MasternodesScreen's sticky `view` field back to List. Add a default no-op ScreenLike::reset_to_root_view() (delegated through the Screen enum), implement it on MasternodesScreen to reset to MasternodesView::List, and call it from set_main_screen() only -- not from refresh_on_arrival() itself, which stays untouched so returning from a pushed KeyInfoScreen/ClaimTokensScreen still preserves the detail view, and on_leave()'s existing "keep the view, drop secrets" contract is unaffected. Co-Authored-By: Codex Sol (GPT-5) <noreply@openai.com> Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes preserve secret prompts across network switches, recover poisoned prompt locks, reset root UI views during main-screen changes, and open masternode key protection directly in its confirmation flow. Tests cover backend, MCP, navigation, pending forms, and key-protection behavior. ChangesNetwork context prompt preservation
UI navigation and key protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MasternodeDetailScreen
participant KeyInfoScreen
participant ProtectionFlow
User->>MasternodeDetailScreen: Click Add password protection
MasternodeDetailScreen->>KeyInfoScreen: Create with protection prompt
KeyInfoScreen->>ProtectionFlow: Open confirmation flow
ProtectionFlow-->>User: Show Yes, add protection action
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit f223237) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp/tools/network.rs (1)
241-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant and late secret prompt installation.
Since
BackendTask::SwitchNetworknow explicitly installs the secret prompt before wiring the wallet backend, this line is redundant. Furthermore, becausedispatch_taskawaits the task's completion, the backend is already wired by the time this line executes (sincestart_spvistrue). As documented oninstall_secret_prompt, calling it after the backend is wired has no effect on theSecretAccessboundary, making this assignment conceptually late.♻️ Proposed fix
- context.install_secret_prompt(ctx.secret_prompt());🤖 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/mcp/tools/network.rs` at line 241, Remove the redundant context.install_secret_prompt(ctx.secret_prompt()) call from the network handling flow, leaving BackendTask::SwitchNetwork responsible for installing the secret prompt before wiring the wallet backend.
🤖 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.
Nitpick comments:
In `@src/mcp/tools/network.rs`:
- Line 241: Remove the redundant
context.install_secret_prompt(ctx.secret_prompt()) call from the network
handling flow, leaving BackendTask::SwitchNetwork responsible for installing the
secret prompt before wiring the wallet backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08072b62-a252-4beb-b988-5ed1c7ad2e7a
📒 Files selected for processing (8)
src/app.rssrc/backend_task/mod.rssrc/mcp/tools/network.rssrc/ui/identities/keys/key_info_screen.rssrc/ui/masternodes/detail_screen.rssrc/ui/masternodes/list_screen.rssrc/ui/mod.rstests/kittest/masternode_tab.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Three independent GUI fixes — direct-open of the masternode key-protection prompt, resetting the masternodes tab's detail view to List on root navigation, and moving install_secret_prompt ahead of wallet-backend wiring on network switch (both the shared backend-task path and its MCP call site). The core production fix (src/backend_task/mod.rs:1017, before the start_spv wiring at 1024-1040) is correct and properly ordered against the SecretAccess chokepoint's requirement. Two real gaps survive verification: the new regression tests for that fix only prove the prompt is stored on the context field, never that it is the prompt actually consumed by the wired WalletBackend/SecretAccess, and the MCP NetworkSwitch::invoke re-installs the prompt after dispatch_task has already run the same install-then-wire sequence, making that line a no-op. Additionally, the new reset_to_root_view call fires unconditionally on every SetMainScreen dispatch — including a same-tab reclick that never touches on_leave — silently discarding an in-progress or failed Load form that on_leave's own documented design intentionally preserves for a corrected resubmit; this path is untested by the new kittest coverage, which only exercises navigate-away-and-back.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed)
🟡 3 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/backend_task/mod.rs`:
- [SUGGESTION] src/backend_task/mod.rs:1281-1316: Regression test proves prompt is stored on the context, not that it's the prompt the wired backend actually uses
Verified directly: this test uses `start_spv: false` and asserts `context.wallet_backend().is_err()`, i.e. it deliberately prevents `ensure_wallet_backend_and_start_spv` from ever running. The only real assertion, `Arc::ptr_eq(&prompt, &context.secret_prompt())`, is true the instant `install_secret_prompt` runs regardless of its ordering relative to backend wiring — it would still pass if wiring somehow read the prompt before this call, as long as nothing later clobbers the field. `WalletBackend::new(..., self.secret_prompt())` (src/context/mod.rs:1082) is the actual point that snapshots the prompt into `SecretAccess`; this test never drives that path with `start_spv: true`, nor exercises a protected-secret operation to prove the specific `TestPrompt` instance is what the backend consults. A future bug where the backend captures the prompt at the wrong time (e.g. before this `install_secret_prompt` call, or from a stale reference) would slip past this test even though its name claims to guard exactly that ordering. Wire the backend (`start_spv: true`) and assert behavior through `wallet_backend().secret_access()`, or add a test-only observation point inside `SecretAccess`.
In `src/mcp/tools/network.rs`:
- [SUGGESTION] src/mcp/tools/network.rs:241: install_secret_prompt re-assignment in NetworkSwitch::invoke is dead code that masks a false sense of coverage
Verified the call chain: `dispatch_task` (src/mcp/dispatch.rs:29) invokes `app_context.run_backend_task(task, sender)` on the exact same `ctx` used at network.rs:241. Inside the `SwitchNetwork` arm, that `self` already calls `new_ctx.install_secret_prompt(self.secret_prompt())` at mod.rs:1017, and since `NetworkSwitch` always sets `start_spv: true`, it also already wires the backend via `ensure_wallet_backend_and_start_spv` (mod.rs:1024-1040) before `run_backend_task` returns — all before control reaches `network.rs:241`. By the time this line executes, `context.secret_prompt()` already equals `ctx.secret_prompt()` and any wallet backend has already been built with it, so this re-assignment cannot change behavior; it only replaces the mutable context field a second time, after the point where it could matter. Worse, the new regression test `network_switch_tool_preserves_secret_prompt_identity` only asserts on this already-redundant field, so removing the *actual* fix at mod.rs:1017 would still leave this test green while the backend silently retained the stale prompt/NullSecretPrompt. Remove this line (or replace it with a comment clarifying it's an intentional defensive no-op) and retarget the MCP test at the wired backend's behavior instead of the context field.
In `src/app.rs`:
- [SUGGESTION] src/app.rs:1954-1961: reset_to_root_view fires on a same-tab reclick too, discarding an in-progress/failed Load form that on_leave deliberately preserves
Verified directly: `select_main_screen` (src/app.rs:1688-1696) early-returns without calling `on_leave` when the target screen is already selected, but `set_main_screen` (src/app.rs:1954-1961) calls `active_screen.reset_to_root_view()` unconditionally regardless of that early return. `left_panel.rs` dispatches `AppAction::SetMainScreenThenGoToMainScreen`/`SetMainScreen` purely on `clicked()`, with no `is_selected` guard (lines ~198-238), so a user can reclick the already-active Masternodes tab. `MasternodesScreen::on_leave`'s own doc comment (list_screen.rs:523-533) states a failed load 'deliberately keeps its form (and its fields) for a corrected resubmit' — but `reset_to_root_view` (list_screen.rs:519-521) unconditionally sets `self.view = MasternodesView::List`, discarding the entire Load form (ProTxHash, alias, node type) on a same-tab reclick even though no real navigation away-and-back occurred and `on_leave` never ran. This regression is new: before this PR there was no reset-on-arrival at all. The added kittest (`left_nav_return_to_masternodes_resets_detail_to_list`, tests/kittest/masternode_tab.rs:883) only covers navigating to a different tab and back, not a same-tab reclick while a Load form or failed load is open.
Co-Authored-By: Codex Sol <noreply@openai.com>
…y protection dialog Co-Authored-By: Codex Sol <noreply@openai.com>
Co-Authored-By: Codex Sol <noreply@openai.com>
…ion tests Co-Authored-By: Codex Sol <noreply@openai.com>
…reset Co-Authored-By: Codex Sol <noreply@openai.com>
lklimek
added
the
claudius-review
Gate root-view resets on the visible load form so a failed load remains available for correction after its pending task state has cleared. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified exact HEAD a03c545 against combined Codex full-PR evidence and Sonnet's delta_from_last_sonnet coverage (cb60820..a03c545). Both prior prompt-wiring findings (55e35b5e107e, 6c8fdc41b891) are genuinely fixed. The third prior finding is not fully resolved: source inspection confirms reconcile_pending_load clears pending_load to None on load failure while leaving the Load form displayed, so the pending_load-based guard at list_screen.rs:519-522 wipes that preserved failed form on the next reset_to_root_view call. The same guard also over-preserves an unrelated Detail view while pending_load is Some. These are the same root cause and are combined into one finding. Codex's two other state-discrimination findings are also confirmed. No blocking issues; all three findings are suggestions.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 3 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/masternodes/list_screen.rs`:
- [SUGGESTION] src/ui/masternodes/list_screen.rs:519-522: pending_load-gated view reset both loses failed-load forms and over-preserves unrelated views
`reset_to_root_view` gates entirely on `self.pending_load.is_none()`, but `pending_load` tracks whether a load task is outstanding, not what the screen is currently displaying — and it diverges from the displayed view in both directions. (1) Under-preservation: `reconcile_pending_load` clears `pending_load` to `None` as soon as the load stops being outstanding, including on failure, while intentionally leaving `MasternodesView::Load(form)` open with its fields intact for a corrected resubmit. Because `pending_load` is already `None`, a subsequent `reset_to_root_view` resets the form to `List`, silently discarding its fields. (2) Over-preservation: `pending_load` can be `Some` while `self.view` is `MasternodesView::Detail(...)`; Cancel can dismiss the form without clearing `pending_load`, and the global node picker can open a Detail view while a load is still outstanding. Root navigation then refuses to reset the Detail view to List. Guard on whether the displayed view is actually a Load form worth protecting, not merely whether `pending_load` is Some or None.
In `src/ui/masternodes/detail_screen.rs`:
- [SUGGESTION] src/ui/masternodes/detail_screen.rs:691-699: Protection routing can select a key whose screen cannot render the dialog
`first_protectable_key` treats any `PrivateKeyData` variant present in `private_keys` as protectable, but `KeyInfoScreen` only renders `render_key_protection_section` in the `InVault` branch. A masternode holding both a wallet-derived key and an unprotected vault key can have the aggregate Add password protection CTA route to the wallet-derived key if it appears first in `identity_keys()`. `new_with_protection_prompt` then sets `protection_confirm` on a screen that never handles it for that variant, so the dialog remains invisible. Filter to keys whose `PrivateKeyData` is actually `InVault` before selecting the routing target.
In `src/ui/identities/keys/key_info_screen.rs`:
- [SUGGESTION] src/ui/identities/keys/key_info_screen.rs:802-815: Revalidation still opens the add flow for already-protected keys
`new_with_protection_prompt`'s status recheck only special-cases `IdentityProtectionStatus::NoVaultKeys`; every other variant, including `Protected`, calls `open_add_confirm()`. If another GUI or MCP operation completes protection between the detail view's stale status probe and this constructor, the resulting screen reports protection as active while presenting an Add-protection confirmation. Submitting the existing password against that stale flow returns `IdentityKeysProtected { count: 0 }` and can rewrite the hint; a different password appears to fail as though a real migration were attempted. Match the enum exhaustively and only open the add flow for `Unprotected` or `Mixed`.
| let mut screen = Self::new(identity, key, private_key_data, app_context); | ||
| let status = screen.compute_protection_status(); | ||
| if status == IdentityProtectionStatus::NoVaultKeys { | ||
| screen.protection_stage = ProtectionStage::Idle; | ||
| MessageBanner::set_global( | ||
| app_context.egui_ctx(), | ||
| "Password protection is not available yet. Wait for wallet setup to finish, then try again.", | ||
| MessageType::Warning, | ||
| ); | ||
| } else { | ||
| screen.protection_status = Some(status); | ||
| screen.open_add_confirm(); | ||
| } | ||
| screen |
There was a problem hiding this comment.
🟡 Suggestion: Revalidation still opens the add flow for already-protected keys
new_with_protection_prompt's status recheck only special-cases IdentityProtectionStatus::NoVaultKeys; every other variant, including Protected, calls open_add_confirm(). If another GUI or MCP operation completes protection between the detail view's stale status probe and this constructor, the resulting screen reports protection as active while presenting an Add-protection confirmation. Submitting the existing password against that stale flow returns IdentityKeysProtected { count: 0 } and can rewrite the hint; a different password appears to fail as though a real migration were attempted. Match the enum exhaustively and only open the add flow for Unprotected or Mixed.
| let mut screen = Self::new(identity, key, private_key_data, app_context); | |
| let status = screen.compute_protection_status(); | |
| if status == IdentityProtectionStatus::NoVaultKeys { | |
| screen.protection_stage = ProtectionStage::Idle; | |
| MessageBanner::set_global( | |
| app_context.egui_ctx(), | |
| "Password protection is not available yet. Wait for wallet setup to finish, then try again.", | |
| MessageType::Warning, | |
| ); | |
| } else { | |
| screen.protection_status = Some(status); | |
| screen.open_add_confirm(); | |
| } | |
| screen | |
| let mut screen = Self::new(identity, key, private_key_data, app_context); | |
| let status = screen.compute_protection_status(); | |
| screen.protection_status = Some(status); | |
| match status { | |
| IdentityProtectionStatus::NoVaultKeys => { | |
| screen.protection_stage = ProtectionStage::Idle; | |
| MessageBanner::set_global( | |
| app_context.egui_ctx(), | |
| "Password protection is not available yet. Wait for wallet setup to finish, then try again.", | |
| MessageType::Warning, | |
| ); | |
| } | |
| IdentityProtectionStatus::Unprotected | IdentityProtectionStatus::Mixed => { | |
| screen.open_add_confirm(); | |
| } | |
| IdentityProtectionStatus::Protected => {} | |
| } | |
| screen |
source: ['codex']
There was a problem hiding this comment.
Still valid at f223237. new_with_protection_prompt continues to special-case only NoVaultKeys; the else branch fires open_add_confirm() for Protected and Mixed alike. A key protected out-of-band between the detail view's probe and this constructor still lands the user in an Add-protection flow against already-sealed keys. Leaving this thread open — the exhaustive-match fix you proposed remains the right call.
🤖 Claudius the Magnificent
|
Pushed a fix for the failed-load form being discarded on navigation: Commit: 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified exact HEAD f223237 via git and gh pr view. Prior finding ae59c2746083 (pending_load-gated view reset) is genuinely fixed by the head commit: reset_to_root_view now gates on the displayed MasternodesView::Load variant instead of pending_load state, resolving both the under-preservation (failed-load form wiped) and over-preservation (stuck Detail view) halves, backed by a new regression assertion. The two protection-flow suggestions (routing to a non-InVault key; revalidation still opening the add-confirm flow for an already-protected key) remain valid at exact HEAD on direct source inspection. No blocking issues; review remains COMMENT.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— general (completed),claude-sonnet-5— rust-quality (completed)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/masternodes/detail_screen.rs`:
- [SUGGESTION] src/ui/masternodes/detail_screen.rs:691-699: Protection routing can select a key whose screen cannot render the dialog
first_protectable_key() treats any PrivateKeyData variant present in private_keys as protectable (it only checks `.is_some()`), but KeyInfoScreen only calls render_key_protection_section — the code path that draws the add/remove-protection modal — inside the PrivateKeyData::InVault match arm (key_info_screen.rs:559-579). A masternode holding both a wallet-derived key and a later unprotected vault key can have the aggregate "Add password protection…" CTA route to the wallet-derived key if it appears first in identity_keys(). new_with_protection_prompt then sets protection_confirm/opens the add-confirm flow on a screen that never renders that section for the non-InVault branch, so the promised confirmation dialog silently never appears — reproducing the exact class of bug this PR sets out to fix. Filter the routing target to keys whose PrivateKeyData is actually InVault.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius grumpy review — PR #913
Three specialists (security/opus, project/opus, QA/sonnet) walked the 9 changed files. The verdict is a genuinely pleasant surprise: the security core is sound. The network-switch secret-prompt reorder correctly installs the interactive host before the wallet backend snapshots it into SecretAccess, closing the passphrase-stranding gap for real, and the two regression tests actually exercise the wired WalletBackend/SecretAccess path (with_secret(...) + ask_count()==1) — not merely the stored context field, as an earlier reviewer worried. The poisoned-lock recovery in install_secret_prompt is panic- and deadlock-free. No secret leaks, no new panics on reachable paths.
The masternode half is where the gremlins live:
- 🟡 MEDIUM (CODE-001) —
first_protectable_keyselects by.is_some()regardless ofPrivateKeyDatavariant, but the seal dialog only renders forInVaultkeys. For a realistic multi-key masternode identity, the new "Add password protection…" CTA opens a dead-end screen with no dialog and no error — the exact bug this PR fixes, reintroduced through the back door. Flagged inline. This is the one worth fixing before merge. - 🔵 LOW (CODE-002) —
new_with_protection_promptopens the Add-protection flow forProtected/Mixedtoo; safe today only because the sole caller is tier-gated. Latent for future callers. (Matches the still-open reviewer thread on that line.) - 🔵 LOW (CODE-003) —
reset_to_root_viewalso clears the Detail view's unsubmitted vote picks / voting-key entry on root navigation, asymmetric with the Load-form preservation the same batch engineered. Partly by-design (resetting to List is the feature); worth an explicit contract decision. - 🔵 LOW (CALL-001) — the
reset_to_root_viewtrait doc overstates that pushed-sub-screen returns always preserve the sub-view; three compoundSetMainScreen*actions route through the reset.
No CRITICAL or HIGH. Full report (HTML + JSON) generated locally. Recommend addressing the MEDIUM; the LOWs are polish and can be follow-ups.
🤖 Claudius the Magnificent
📊 View full HTML review report
| && ui.button("Add password protection…").clicked() | ||
| { | ||
| action = Some(self.open_key_info(target, &key)); | ||
| action = Some(self.open_key_info_with_protection_prompt(target, &key)); |
There was a problem hiding this comment.
🟡 MEDIUM — this new routing makes the headline feature a silent no-op for multi-key masternodes
This line newly routes the "Add password protection…" CTA into open_key_info_with_protection_prompt, which selects its target via first_protectable_key (detail_screen.rs:686). Two independent reviewers flagged the same trap, and I'm afraid they're right.
first_protectable_key picks the first key whose held data is merely .is_some() — matching any PrivateKeyData variant (Clear, AlwaysClear, Encrypted, AtWalletDerivationPath, InVault). But KeyInfoScreen only draws the protection confirmation dialog inside the PrivateKeyData::InVault match arm (key_info_screen.rs:559-579). So when the selection lands on a non-InVault key, new_with_protection_prompt dutifully arms ConfirmAdd, the screen opens… and nothing renders. No dialog, no error, no next step — precisely the dead-end this PR set out to abolish.
It's reachable, not hypothetical: identity_keys() iterates in KeyID order, while the CTA visibility gate counts vault keys via view.scheme — divergent criteria. migrate_keystore_to_vault migrates only plaintext (Clear/AlwaysClear); AtWalletDerivationPath and legacy Encrypted keys are left untouched. An evonode/masternode identity holding a wallet-derived auth key (lower KeyID) alongside an InVault owner/voting key routes the CTA straight to the key the seal screen can't render.
The existing add_password_protection_opens_confirmation_dialog kittest seeds a single Clear key that migrates to InVault, so the selected key and the only vault key are trivially identical — it cannot catch this.
Fix: filter first_protectable_key to keys whose held PrivateKeyData is InVault (ideally also IdentityKeyView::scheme == Unprotected, mirroring the tier gate). Or make new_with_protection_prompt fail closed with a MessageBanner warning — as it already does for NoVaultKeys — when the routed key isn't InVault. Its doc comment ("the only keys that can be sealed") is also false for the non-vault variants. Please add a kittest seeding a wallet-derived/encrypted key at a lower KeyID than the Unprotected vault key so this stays buried.
🤖 Claudius the Magnificent — grumpy review, MEDIUM (CODE-001)
github-actions
Bot
removed
the
claudius-review
Brings in: shutdown fix (dashpay#905), duplicate-DPNS-name error message (dashpay#915), startup banner clearing (dashpay#916), nav pointer cursor + tooltips and wallet-less masternode indication (dashpay#917), onboarding disconnected- banner suppression (dashpay#907), masternode dialog/nav/passphrase fixes (dashpay#913), DAPI auto-refresh during pre-1.0 migration (dashpay#908), "Add Receiving Address" wiring + its test hardening (dashpay#914, dashpay#920), and a CI timeout bump (dashpay#912). dashpay#906 (shielded re-enable) was already pulled in individually last session, so its squashed commit merged as a no-op. Conflicts (6 files) were rebrand-naming overlaps (dash_evo_tool:: vs orchardpay:: imports) plus one real merge in left_panel.rs, where OrchardPay's green-icon tint had to combine with upstream's new nav tooltip. Also fixed 5 files upstream's auto-merged (non-conflicting) additions left un-rebranded: a stray DASH_EVO_DATA_DIR_LOCK/env-var name in a new app.rs test, and dash_evo_tool:: references in three kittest test files. Added tooltip strings for OrchardPay's own nav entries (OrchardPay, DashPay) so the new every_nav_entry_has_a_tooltip test covers them — upstream's version only knows its own nav items. Fixed the new nav_label_hover_shows_pointer_cursor kittest test: OrchardPay's nav rail carries two more always-visible entries than upstream's, pushing "Settings" below the scrollable list's default-size visible viewport; scroll it into view first, matching what a real user would do. Verified: cargo check (both feature modes), cargo clippy --all-features --all-targets -- -D warnings, cargo fmt --all, cargo test --all-features --workspace (2066 lib + 257 kittest + doc tests, 0 failed), all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Why this PR exists
What was done
fix(masternodes): open key protection prompt directly): addedKeyInfoScreen::new_with_protection_prompt, which starts the screen directly in the add-protection confirmation flow instead of the ordinary idle state, and forces the "Key Protection" section open while that flow is active. The Masternodes detail screen's aggregate CTA now routes through this constructor.fix(masternodes): reset detail view to list on root navigation): added a default no-opScreenLike::reset_to_root_view(), implemented onMasternodesScreento reset toMasternodesView::List, called only fromset_main_screen()— not fromrefresh_on_arrival()— so returning from a pushedKeyInfoScreen/ClaimTokensScreenstill preserves the detail view, andon_leave()'s existing "keep the view, drop secrets" contract is unaffected.fix(context): install secret prompt before wiring wallet backend on network switch): theSwitchNetworkhandler wired the new context's wallet backend before installing the real prompt host, soSecretAccesspermanently snapshottedNullSecretPromptat construction (no interior mutability). Now the outgoing context's prompt is propagated onto the new context immediately after creation, before any wallet-backend wiring, in bothBackendTask::SwitchNetworkand the MCPnetwork_switchtool (identical gap). Boot-time context creation was already correct and is unaffected.User story
Imagine you are a masternode operator. You open the Masternodes tab, add password protection to a node's keys and see the confirmation dialog right away instead of a dead end; you bounce between Wallets and Masternodes and always land back on the node list, not a stale detail view; and after switching networks mid-session, your protected identity still prompts you for its password when you withdraw, instead of telling you to go somewhere you already are.
Testing
Each fix was written test-first (RED before the change, GREEN after) and re-verified together on the merged tree:
cargo test --test kittest masternode_tab --all-features— 15 passed (includes all three new/changed regression tests:add_password_protection_opens_confirmation_dialog,left_nav_return_to_masternodes_resets_detail_to_list,go_to_main_screen_from_key_info_preserves_masternode_detail)cargo test --lib backend_task --all-features— 389 passed, includingswitch_network_propagates_secret_prompt_before_wallet_backend_wiringcargo test --lib mcp::tools::network --all-features— 1 passed (network_switch_tool_preserves_secret_prompt_identity)cargo clippy --bin dash-evo-tool --all-features -- -D warnings— cleancargo fmt --all -- --check— cleanBreaking changes
None.
Checklist
refresh_on_arrival()/on_leave()— return-from-sub-screen and leave-preserves-view behavior verified unchangedbackend_task::SwitchNetworkand the MCPnetwork_switchtool); boot-time wiring confirmed already correctAttribution
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
New Features
Bug Fixes
Tests