feat(identity-hub): unified Identities hub + app-scoped wallet/identity switcher - #842
Merged
lklimek merged 54 commits intoJul 1, 2026
Merged
Conversation
lklimek
marked this pull request as draft
April 23, 2026 11:33
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a unified Identities Hub UI (feature-gated), extensive design/implementation documentation, many new UI components (breadcrumb/identity pills, hero, picker, rows, cards, tab bar, checklist, etc.), a multi-tab IdentityHubScreen (Home/Contacts/Activity/Settings) with onboarding/picker flows, app routing integration, and kittest suites. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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 |
![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: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/user-stories.md (1)
7-20:⚠️ Potential issue | 🟡 MinorAdd the new IDH section to the table of contents.
The
Identities Hub (IDH)section is added at Line 1043 but is not linked from the TOC.Proposed docs fix
- [Identity Operations (IDN)](`#identity-operations-idn`) - [DPNS (DPN)](`#dpns-dpn`) - [DashPay (DPY)](`#dashpay-dpy`) +- [Identities Hub (IDH)](`#identities-hub-idh`) - [Token Operations (TOK)](`#token-operations-tok`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-stories.md` around lines 7 - 20, The TOC is missing an entry for the new "Identities Hub (IDH)" section; add a new bullet linking to it (use the anchor format consistent with other entries) by inserting "- [Identities Hub (IDH)](`#identities-hub-idh`)" into the Table of Contents (place it near "Identity Operations (IDN)" for logical grouping) so the "Identities Hub (IDH)" section is reachable from the TOC.
🟡 Minor comments (17)
docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md-352-352 (1)
352-352:⚠️ Potential issue | 🟡 MinorReserve or restore section B.5.
The document jumps from
B.4.1toB.6; add aB.5 — reservedheading or renumber the following sections to keep cross-references stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md` at line 352, The document skips B.5 and jumps from B.4.1 to the "B.6 Activity tab (Frame 5)" heading, so add a placeholder heading "B.5 — reserved" immediately before the existing B.6 heading (or alternatively renumber the subsequent headings to restore a continuous sequence); update any internal cross-references that reference B.6/B.4.1 as needed so numbering remains stable and consistent.docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md-12-13 (1)
12-13:⚠️ Potential issue | 🟡 MinorClarify final IA versus Phase 1 coexistence.
These lines describe replacing Dashpay and Identities with a single
Identitiesnav entry, but this PR currently addsIdentity Hubalongside the existing entries. Add an explicit phased-rollout note so follow-up implementation and tests do not treat the scaffold label as a spec violation.Suggested wording
-This spec collapses the current two left-nav entries — **Dashpay** and **Identities** — into -one unified section called **Identities**. +This spec describes the final target state: the current **Dashpay** and **Identities** +left-nav entries collapse into one unified section called **Identities**. During Phase 1, +the implementation may expose a separate **Identity Hub** entry alongside the legacy entries +so the scaffold can be reviewed without removing existing flows.Also applies to: 40-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md` around lines 12 - 13, Add a short phased-rollout note to clarify that the spec's final IA replaces the two nav entries ("Dashpay" and "Identities") with a single "Identities" section, while the current PR may temporarily introduce the scaffold label "Identity Hub" alongside existing entries; update the document near the existing sentence and the similar block at lines referenced as also applies to (the other paragraph) to explicitly state that "Identity Hub" is a temporary scaffold for Phase 1 and will be removed/merged in the final rollout so implementations and tests should treat its presence as non-normative.docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md-532-533 (1)
532-533:⚠️ Potential issue | 🟡 MinorTighten the incomplete sentence.
Minimum required shown dynamicallyreads like a missing noun; make it explicit.Suggested wording
- `UseWalletBalance`. Minimum required shown dynamically. + `UseWalletBalance`. The minimum required amount is shown dynamically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md` around lines 532 - 533, The sentence fragment "Minimum required shown dynamically" is incomplete; update the wording in the "Fund the identity" step to include an explicit noun (e.g., "amount" or "funds"). Locate the line that mentions UseWalletBalance and replace the fragment with a complete phrase such as "Minimum required amount shown dynamically" (or "Minimum required funds shown dynamically") so the instruction reads: "UseWalletBalance. Minimum required amount shown dynamically." Ensure the change appears inline within the Add funds wizard (§B.9) description.src/ui/identity/mod.rs-15-17 (1)
15-17:⚠️ Potential issue | 🟡 MinorUpdate this feature-gate note to match routing behavior.
identity-hubdoes more than control the left-nav entry:src/app.rsalso omitsRootScreenIdentityHubfrommain_screenswhen the feature is disabled. This note should mention both, or it will mislead future changes around persisted root-screen handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identity/mod.rs` around lines 15 - 17, Update the module-level feature-gate note in the identity module to accurately state that the `identity-hub` feature not only controls the left-nav entry but also causes `app.rs` to omit the `RootScreenIdentityHub` variant from the `main_screens` collection, so toggling the feature won’t produce unreachable variants in the root screen enum; modify the comment at the top of the identity module (where the current note lives) to mention both behaviors and reference `RootScreenIdentityHub` and `main_screens` to prevent future confusion when persisting root-screen state.src/ui/components/identity_pill.rs-14-37 (1)
14-37:⚠️ Potential issue | 🟡 MinorKeep
display_label()from returning an empty label.The doc contract says the resolver never returns an empty string, but Line 153 proves an empty ID produces
"". Since this is public UI label logic, use a defensive fallback instead of rendering an invisible pill.Proposed defensive fallback
pub fn display_label( local_nickname: Option<&str>, dpns_handle: Option<&str>, identity_id_base58: &str, ) -> String { @@ if let Some(handle) = dpns_handle.map(str::trim).filter(|s| !s.is_empty()) { return handle.to_string(); } - shorten_id(identity_id_base58) + let identity_id_base58 = identity_id_base58.trim(); + if identity_id_base58.is_empty() { + return "Unknown identity".to_string(); + } + shorten_id(identity_id_base58) }- fn all_empty_collapses_to_empty_id() { - // Defensive: the id is the guaranteed-non-optional fallback, so an - // empty id yields an empty label. Callers must not pass an empty id. + fn all_empty_uses_unknown_identity_fallback() { let label = display_label(None, None, ""); - assert_eq!(label, ""); + assert_eq!(label, "Unknown identity"); }Also applies to: 149-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/identity_pill.rs` around lines 14 - 37, The display_label function can still return an empty string when identity_id_base58 is empty; update display_label to defensively handle an empty/whitespace identity_id_base58 before calling shorten_id (or make shorten_id return a non-empty fallback). Specifically, in display_label (and similarly used sites), if identity_id_base58.trim().is_empty() return a stable non-empty placeholder (e.g. "Unknown identity" or similar), otherwise call shorten_id(identity_id_base58); alternatively ensure shorten_id never yields "" and returns the raw id or a placeholder when given empty input.docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md-117-181 (1)
117-181:⚠️ Potential issue | 🟡 MinorSeparate planned per-tab kittests from the tests actually added.
This section names per-tab files and expectations that are not present in the PR’s actual
tests/kittest/identity_hub.rsscaffold coverage. Mark these as planned follow-ups or update the file references so the traceability matrix does not overstate current coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md` around lines 117 - 181, The doc lists per-tab kittests (IT-ONBOARD-01, IT-HOME-01, IT-CONTACTS-01, IT-ACTIVITY-01, IT-SETTINGS-01, IT-NAV-01) and specific file paths (e.g., tests/kittest/identity_hub_onboarding.rs) that are not actually added to the PR (only tests/kittest/identity_hub.rs scaffold exists); either mark each of those per-tab test entries as "planned" follow-ups or update the file references to point to the actual scaffold (tests/kittest/identity_hub.rs) and adjust the traceability matrix accordingly so it does not overstate coverage — update the headings or add TODO tags next to each IT-XXXX identifier in 03-test-case-spec.md to reflect the true status.tests/kittest/identity_hub.rs-56-70 (1)
56-70:⚠️ Potential issue | 🟡 MinorMake this test exercise the dispatcher it claims to guard.
The current assertion is tautological, so removing the
ScreenType::create_screenarm would not fail this test as long as the enum variant remains. Either construct the existing kittestAppContextfixture and assert the createdScreen::IdentityHubScreen(_), or rename this as a compile-only enum guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kittest/identity_hub.rs` around lines 56 - 70, The test identity_hub_screen_type_creates_hub_screen is currently tautological; change it to actually call ScreenType::create_screen with the real kittest AppContext fixture and assert the returned Screen is the IdentityHub variant (e.g., verify Screen::IdentityHubScreen(_) via matches! or an if let), or if you intentionally want a compile-only guard rename the test accordingly; specifically locate the ScreenType::create_screen call and the test function and replace the enum-only assert with constructing the existing AppContext test fixture (the same one used by identity_hub_mounts_and_renders), invoke ScreenType::create_screen(app_ctx) and assert the result is Screen::IdentityHubScreen(_).docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md-153-160 (1)
153-160:⚠️ Potential issue | 🟡 MinorUse the hyphenated activity-feed feature name.
Line 156 uses
identity_hub_activity_feed; the Cargo feature documented for the PR isidentity-hub-activity-feed.Proposed docs fix
-**Preconditions**: one identity, `identity_hub_activity_feed` flag off. +**Preconditions**: one identity, `identity-hub-activity-feed` flag off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md` around lines 153 - 160, Replace the incorrect underscored Cargo feature name used in the test spec—change the string "identity_hub_activity_feed" to the hyphenated feature name "identity-hub-activity-feed" in the IT-ACTIVITY-01 test documentation (the Activity tab shell renders test in identity_hub_activity), ensuring the documented feature matches the PR's actual Cargo feature.docs/ai-design/2026-04-23-identity-hub-impl/02-ux-plan.md-92-95 (1)
92-95:⚠️ Potential issue | 🟡 MinorUse the Cargo feature name here.
Line 95 documents
identity_hub_activity_feed, but the PR’s Cargo feature isidentity-hub-activity-feed. Keeping the hyphenated name avoids invalid feature flags in follow-up work.Proposed docs fix
- screen." Feature flag: `identity_hub_activity_feed`, off by default. + screen." Feature flag: `identity-hub-activity-feed`, off by default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/02-ux-plan.md` around lines 92 - 95, The doc uses the underscored feature name identity_hub_activity_feed but the Cargo feature is identity-hub-activity-feed; update the text so the feature name matches the Cargo feature (use identity-hub-activity-feed everywhere the feature is referenced, e.g., in the MVP constraint sentence and any follow-up mentions) and verify this exactly matches the feature key in Cargo.toml to avoid invalid feature flag references.tests/kittest/identity_hub.rs-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorUse the actual legacy Dashpay root variant.
Line 45 checks
RootScreenDashPayProfile, which is a DashPay profile screen, not the legacy root nav entry. This test should guard coexistence withRootScreenDashpay.Proposed test fix
- let legacy_dashpay = RootScreenType::RootScreenDashPayProfile; + let legacy_dashpay = RootScreenType::RootScreenDashpay;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kittest/identity_hub.rs` around lines 44 - 48, The test uses the wrong DashPay variant; replace RootScreenType::RootScreenDashPayProfile with the true legacy root nav variant RootScreenType::RootScreenDashpay so the assertions compare RootScreenIdentityHub against both RootScreenIdentities and RootScreenDashpay; update the variable name (e.g., legacy_dashpay) accordingly and keep the two assert_ne! checks to ensure the new hub variant does not equal the legacy Dashpay root.docs/user-stories.md-1053-1084 (1)
1053-1084:⚠️ Potential issue | 🟡 MinorDo not mark planned hub flows as implemented yet.
The header says
[Implemented]means present in the current codebase, but these stories describe Home tab content, social-profile cards, Contacts gating, and bulk identity creation paths that the PR summary calls out as follow-up scaffold work. Mark them[Gap]or split out smaller scaffold-only implemented stories.Minimal status correction
-### IDH-002: Identity home at a glance [Implemented] +### IDH-002: Identity home at a glance [Gap]-### IDH-004: Opt in to DashPay social profile [Implemented] +### IDH-004: Opt in to DashPay social profile [Gap]-### IDH-005: Developer bulk identity creation [Implemented] +### IDH-005: Developer bulk identity creation [Gap]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/user-stories.md` around lines 1053 - 1084, Update the status annotations for the identity-hub stories that were incorrectly marked as implemented: change the headers for IDH-002, IDH-003, IDH-004, and IDH-005 in docs/user-stories.md from “[Implemented]” to “[Gap]” (or split each into a scaffold-only sub-story and leave a small implemented stub), and ensure any PR summary or checklist references these stories as follow-up scaffold work rather than completed features so Home tab content, social-profile cards, Contacts gating, and developer bulk-creation paths are not claimed as implemented.docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md-40-41 (1)
40-41:⚠️ Potential issue | 🟡 MinorPoint the enum mapping step at the module that owns it.
RootScreenType::to_int/from_intlive insrc/ui/mod.rsin the provided implementation context, notsrc/database/settings.rs. This step should reference the owning module and mention settings only if a persistence test is being updated there.Proposed docs fix
-6. **`src/database/settings.rs`** — extend `RootScreenType::from_int` / `to_int` mapping - (next free integer). No schema change. +6. **`src/ui/mod.rs`** — extend `RootScreenType::from_int` / `to_int` mapping + (next free integer). No schema change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md` around lines 40 - 41, Update the docs step to point at the actual owner of the enum mapping: change the reference from src/database/settings.rs to src/ui/mod.rs and state that RootScreenType::to_int and RootScreenType::from_int are implemented there; only mention updating settings persistence or tests in src/database/settings.rs if you also alter saved values or persistence tests — otherwise limit the instruction to editing src/ui/mod.rs where the RootScreenType mapping lives.docs/ai-design/2026-04-22-identity-dashpay-redesign/README.md-3-12 (1)
3-12:⚠️ Potential issue | 🟡 MinorAlign the README with the coexistence rollout.
This says Dashpay and Identities are collapsed into a single
Identitiesnav entry, but this PR preserves both legacy entries and adds the new hub alongside them. Future implementers may remove the wrong nav item if this stays stale.Proposed wording update
-Identities section of Dash Evo Tool 2. The redesign collapses the current two left-nav -entries — Dashpay and Identities — into a single **Identities** section with four tabs: -Home, Contacts, Activity, and Settings. +Identities section of Dash Evo Tool 2. The scaffold introduces a new **Identity Hub** +entry alongside the existing Dashpay and Identities entries, with the hub organized into +four tabs: Home, Contacts, Activity, and Settings.-profile existing. The nav label remains `Identities` (plural, unchanged from the codebase). +profile existing. During the coexistence rollout, the legacy `Identities` nav entry remains +available and the new hub uses its own nav entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/README.md` around lines 3 - 12, Update the README to reflect the coexistence rollout: change the statement that Dashpay and Identities are collapsed into a single "Identities" nav entry to instead explain that the redesign introduces a new unified Identities hub while preserving the legacy "Dashpay" and "Identities" left-nav entries (both remain present alongside the new hub), and add a note about future removal being optional; reference the nav labels "Dashpay", "Identities", and the new "Identities" hub (or "Identities" section with tabs Home, Contacts, Activity, Settings) so implementers know the current rollout preserves legacy entries.docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md-66-69 (1)
66-69:⚠️ Potential issue | 🟡 MinorCorrect feature flag names to match Cargo.toml throughout the document.
Feature names in
Cargo.tomlareidentity-hubandidentity-hub-activity-feed(hyphens, no prefix). Update all references:
- Lines 66, 68:
feat-identity-hub→identity-hub;identity_hub_activity_feed→identity-hub-activity-feed- Line 171:
identity_hub_activity_feed→identity-hub-activity-feed- Lines 234–235 (feature table): same corrections
Line 66–69 diff
-- Add `feat-identity-hub` feature to `Cargo.toml` (default-enabled so the hub is visible +- Add `identity-hub` feature to `Cargo.toml` (default-enabled so the hub is visible by default; can be disabled for quick compile). -- Add `identity_hub_activity_feed` feature (default off) — gates the unified activity +- Add `identity-hub-activity-feed` feature (default off) — gates the unified activity aggregator (stub tab content when off).Line 234–235 diff
-| `feat-identity-hub` | Cargo feature | on | entire hub module compilation + nav entry | -| `identity_hub_activity_feed` | Cargo feature | off | unified activity aggregator rendering | +| `identity-hub` | Cargo feature | on | entire hub module compilation + nav entry | +| `identity-hub-activity-feed` | Cargo feature | off | unified activity aggregator rendering |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md` around lines 66 - 69, The document uses incorrect feature-flag identifiers; update every occurrence of the wrong names to match Cargo.toml by replacing `feat-identity-hub` with `identity-hub` and `identity_hub_activity_feed` (or any underscore/feat-prefixed variants) with `identity-hub-activity-feed`; specifically fix the occurrences called out in the review (the feature mentions around the identity hub description, the unified activity aggregator reference, and the feature table entries) so all references use the exact hyphenated names `identity-hub` and `identity-hub-activity-feed`.docs/ai-design/2026-04-23-identity-hub-impl/01-requirements.md-26-27 (1)
26-27:⚠️ Potential issue | 🟡 MinorAlign the new left-nav label with the implementation plan.
This document says the new entry is also labeled
Identities, while the PR summary calls itIdentity Hub. Keeping two visibleIdentitiesentries during coexistence is ambiguous for users and kittest assertions; please make the requirement and acceptance criteria use one unambiguous label.Also applies to: 150-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/01-requirements.md` around lines 26 - 27, Update FR-1 and related acceptance criteria to use a single unambiguous label: change the new left-nav entry text from `Identities` to `Identity Hub` (or vice versa to match the PR summary) throughout the document including references at lines noted (e.g., FR-1 and the acceptance criteria around lines 150-152), and explicitly state that during coexistence the old `Identities` and `Dashpay` entries remain visible while the new `Identity Hub` entry is shown to avoid duplicate `Identities` labels.src/ui/identity/hub_screen.rs-155-164 (1)
155-164:⚠️ Potential issue | 🟡 MinorMake this test exercise
IdentityHubScreen, not just the enum.The test can pass even if
IdentityHubScreen::selected_tab()orIdentityHubScreen::select_tab()is broken, because it only reassigns a localIdentityHubTab. Please instantiate the screen with the existing app-context test harness, or move the state transition into a helper that can be tested withoutAppContext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/identity/hub_screen.rs` around lines 155 - 164, The test currently only manipulates the IdentityHubTab enum and therefore doesn't validate IdentityHubScreen's state methods; update the test to instantiate an IdentityHubScreen via the existing test harness (or a minimal AppContext-free constructor) and call IdentityHubScreen::selected_tab() and IdentityHubScreen::select_tab() to verify transitions between IdentityHubTab::Home and IdentityHubTab::Settings; if creating a full AppContext is heavy, extract the tab state logic into a small helper (e.g., a TabState struct or impl on IdentityHubScreen that can be new() in tests) and exercise that helper instead so the test actually invokes the same code paths used by IdentityHubScreen.docs/ai-design/2026-04-23-identity-hub-impl/01-requirements.md-126-146 (1)
126-146:⚠️ Potential issue | 🟡 MinorDon’t mark full tab flows as implemented while they are still scaffolded.
US-IDH-002throughUS-IDH-006describe balances, Send actions, picker switching, DashPay opt-in, Developer Mode bulk creation, and a unified activity timeline as implemented. The provided hub/tab code still renders under-construction placeholders, so these should be downgraded to scaffolded / gap-follow-up status or split into separate shipped-vs-follow-up stories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ai-design/2026-04-23-identity-hub-impl/01-requirements.md` around lines 126 - 146, Update the status tags and split shipped vs follow-up work for the listed requirements: change US-IDH-002, US-IDH-003, US-IDH-004, and US-IDH-005 from `[Implemented]` to a scaffolded status like `[Scaffolded]` (or `[Gap-follow-up]` where appropriate) and set US-IDH-006 explicitly to `[Gap-follow-up]`; also split each item into two lines or paired stories (a short “UI shell shipped” story and a separate “backend/aggregation follow-up” story) so the README accurately reflects that the hub/tab UI is a placeholder and the full flows (balances, Send action, identity picker behavior, DashPay opt-in backend, Dev Mode bulk creation, unified Activity aggregation) are pending backend work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app.rs`:
- Around line 556-575: The persisted selected_main_screen may refer to a screen
that's not registered when the identity-hub feature is disabled, causing
active_root_screen_mut() to panic; modify the initialization to first build the
map of registered screens (the chain that produces (RootScreenType, Screen)
entries including the conditional RootScreenType::RootScreenIdentityHub /
Screen::IdentityHubScreen) and then resolve selected_main_screen by checking the
built map and falling back to a safe default registered key if the persisted
value is missing. Ensure the lookup uses the constructed map rather than
assuming persisted selected_main_screen is valid.
In `@src/ui/components/breadcrumb_pill.rs`:
- Around line 233-263: The click detection bug comes from reading the outer
frame's Response (frame.show(...).response) instead of the inner Label response
that actually carries Sense::click(); update the code to capture the
InnerResponse returned by frame.show (use the value with .inner from the
closure) and use that inner Response for tooltip selection, widget_info and
clicked() logic (keep identifiers: frame.show, frame_response/frame_inner,
ui.add(egui::Label::new(rich).sense(sense)), BreadcrumbPillMode::Interactive and
BreadcrumbPillResponse::new) so interactive pills check inner.clicked() rather
than the frame's response.
In `@src/ui/components/identity_pill.rs`:
- Around line 57-95: IdentityPill currently eagerly stores a BreadcrumbPill and
returns BreadcrumbPillResponse; change it to the lazy pattern by storing the
domain/config fields (local_nickname: Option<String>, dpns_handle:
Option<String>, identity_id_base58: String) plus any builder-set options
(tooltip, accessible_name, mode) instead of BreadcrumbPill, keep the builder
methods (with_tooltip, with_accessible_name, with_mode) to mutate those stored
fields, construct the BreadcrumbPill inside show() using display_label(...) and
the stored options, and return a new IdentityPillResponse type that implements
ComponentResponse (wrapping whatever data you need from the inner
BreadcrumbPillResponse) rather than exposing BreadcrumbPillResponse directly;
update new(), show(), and the public API to use IdentityPillResponse and ensure
no eager UI objects are stored on the struct.
In `@src/ui/identity/hub_screen.rs`:
- Around line 46-52: The current landing() treats any load error as zero
identities by using unwrap_or(0); instead, change landing() to match the result
of app_context.load_local_qualified_identities() so successful Ok(vec) maps to
HubLanding::from_identity_count(vec.len()), while Err(e) surfaces a
user-friendly MessageBanner (use MessageBanner API) and attach the technical
error via BannerHandle::with_details(e) so users see a calm onboarding message
distinct from a real zero count and developers can inspect the attached details;
update code around the landing function and remove unwrap_or(0), referencing
load_local_qualified_identities, HubLanding::from_identity_count, MessageBanner,
and BannerHandle::with_details to implement this behavior.
- Around line 66-144: The ScreenLike impl for IdentityHubScreen currently only
implements ui() so add the missing trait methods: implement
display_task_result(&mut self, result: BackendTaskSuccessResult) to forward or
handle backend results (update state or route to banner system),
display_message(&mut self, msg: &str, ty: MessageType) to enqueue/route messages
to the hub banner/state, refresh(&mut self) and refresh_on_arrival(&mut self) to
reload any cached data or reset UI state (call existing refresh helpers if
present), and change_context(&mut self, app_context: &Arc<AppContext>) to update
self.app_context and trigger a refresh; add all these methods to the impl
ScreenLike for IdentityHubScreen so the hub participates in task result routing,
context changes, and banner lifecycle as required by the project guidelines.
---
Outside diff comments:
In `@docs/user-stories.md`:
- Around line 7-20: The TOC is missing an entry for the new "Identities Hub
(IDH)" section; add a new bullet linking to it (use the anchor format consistent
with other entries) by inserting "- [Identities Hub (IDH)](`#identities-hub-idh`)"
into the Table of Contents (place it near "Identity Operations (IDN)" for
logical grouping) so the "Identities Hub (IDH)" section is reachable from the
TOC.
---
Minor comments:
In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.md`:
- Line 352: The document skips B.5 and jumps from B.4.1 to the "B.6 Activity tab
(Frame 5)" heading, so add a placeholder heading "B.5 — reserved" immediately
before the existing B.6 heading (or alternatively renumber the subsequent
headings to restore a continuous sequence); update any internal cross-references
that reference B.6/B.4.1 as needed so numbering remains stable and consistent.
- Around line 12-13: Add a short phased-rollout note to clarify that the spec's
final IA replaces the two nav entries ("Dashpay" and "Identities") with a single
"Identities" section, while the current PR may temporarily introduce the
scaffold label "Identity Hub" alongside existing entries; update the document
near the existing sentence and the similar block at lines referenced as also
applies to (the other paragraph) to explicitly state that "Identity Hub" is a
temporary scaffold for Phase 1 and will be removed/merged in the final rollout
so implementations and tests should treat its presence as non-normative.
- Around line 532-533: The sentence fragment "Minimum required shown
dynamically" is incomplete; update the wording in the "Fund the identity" step
to include an explicit noun (e.g., "amount" or "funds"). Locate the line that
mentions UseWalletBalance and replace the fragment with a complete phrase such
as "Minimum required amount shown dynamically" (or "Minimum required funds shown
dynamically") so the instruction reads: "UseWalletBalance. Minimum required
amount shown dynamically." Ensure the change appears inline within the Add funds
wizard (§B.9) description.
In `@docs/ai-design/2026-04-22-identity-dashpay-redesign/README.md`:
- Around line 3-12: Update the README to reflect the coexistence rollout: change
the statement that Dashpay and Identities are collapsed into a single
"Identities" nav entry to instead explain that the redesign introduces a new
unified Identities hub while preserving the legacy "Dashpay" and "Identities"
left-nav entries (both remain present alongside the new hub), and add a note
about future removal being optional; reference the nav labels "Dashpay",
"Identities", and the new "Identities" hub (or "Identities" section with tabs
Home, Contacts, Activity, Settings) so implementers know the current rollout
preserves legacy entries.
In `@docs/ai-design/2026-04-23-identity-hub-impl/01-requirements.md`:
- Around line 26-27: Update FR-1 and related acceptance criteria to use a single
unambiguous label: change the new left-nav entry text from `Identities` to
`Identity Hub` (or vice versa to match the PR summary) throughout the document
including references at lines noted (e.g., FR-1 and the acceptance criteria
around lines 150-152), and explicitly state that during coexistence the old
`Identities` and `Dashpay` entries remain visible while the new `Identity Hub`
entry is shown to avoid duplicate `Identities` labels.
- Around line 126-146: Update the status tags and split shipped vs follow-up
work for the listed requirements: change US-IDH-002, US-IDH-003, US-IDH-004, and
US-IDH-005 from `[Implemented]` to a scaffolded status like `[Scaffolded]` (or
`[Gap-follow-up]` where appropriate) and set US-IDH-006 explicitly to
`[Gap-follow-up]`; also split each item into two lines or paired stories (a
short “UI shell shipped” story and a separate “backend/aggregation follow-up”
story) so the README accurately reflects that the hub/tab UI is a placeholder
and the full flows (balances, Send action, identity picker behavior, DashPay
opt-in backend, Dev Mode bulk creation, unified Activity aggregation) are
pending backend work.
In `@docs/ai-design/2026-04-23-identity-hub-impl/02-ux-plan.md`:
- Around line 92-95: The doc uses the underscored feature name
identity_hub_activity_feed but the Cargo feature is identity-hub-activity-feed;
update the text so the feature name matches the Cargo feature (use
identity-hub-activity-feed everywhere the feature is referenced, e.g., in the
MVP constraint sentence and any follow-up mentions) and verify this exactly
matches the feature key in Cargo.toml to avoid invalid feature flag references.
In `@docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md`:
- Around line 117-181: The doc lists per-tab kittests (IT-ONBOARD-01,
IT-HOME-01, IT-CONTACTS-01, IT-ACTIVITY-01, IT-SETTINGS-01, IT-NAV-01) and
specific file paths (e.g., tests/kittest/identity_hub_onboarding.rs) that are
not actually added to the PR (only tests/kittest/identity_hub.rs scaffold
exists); either mark each of those per-tab test entries as "planned" follow-ups
or update the file references to point to the actual scaffold
(tests/kittest/identity_hub.rs) and adjust the traceability matrix accordingly
so it does not overstate coverage — update the headings or add TODO tags next to
each IT-XXXX identifier in 03-test-case-spec.md to reflect the true status.
- Around line 153-160: Replace the incorrect underscored Cargo feature name used
in the test spec—change the string "identity_hub_activity_feed" to the
hyphenated feature name "identity-hub-activity-feed" in the IT-ACTIVITY-01 test
documentation (the Activity tab shell renders test in identity_hub_activity),
ensuring the documented feature matches the PR's actual Cargo feature.
In `@docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md`:
- Around line 40-41: Update the docs step to point at the actual owner of the
enum mapping: change the reference from src/database/settings.rs to
src/ui/mod.rs and state that RootScreenType::to_int and RootScreenType::from_int
are implemented there; only mention updating settings persistence or tests in
src/database/settings.rs if you also alter saved values or persistence tests —
otherwise limit the instruction to editing src/ui/mod.rs where the
RootScreenType mapping lives.
- Around line 66-69: The document uses incorrect feature-flag identifiers;
update every occurrence of the wrong names to match Cargo.toml by replacing
`feat-identity-hub` with `identity-hub` and `identity_hub_activity_feed` (or any
underscore/feat-prefixed variants) with `identity-hub-activity-feed`;
specifically fix the occurrences called out in the review (the feature mentions
around the identity hub description, the unified activity aggregator reference,
and the feature table entries) so all references use the exact hyphenated names
`identity-hub` and `identity-hub-activity-feed`.
In `@docs/user-stories.md`:
- Around line 1053-1084: Update the status annotations for the identity-hub
stories that were incorrectly marked as implemented: change the headers for
IDH-002, IDH-003, IDH-004, and IDH-005 in docs/user-stories.md from
“[Implemented]” to “[Gap]” (or split each into a scaffold-only sub-story and
leave a small implemented stub), and ensure any PR summary or checklist
references these stories as follow-up scaffold work rather than completed
features so Home tab content, social-profile cards, Contacts gating, and
developer bulk-creation paths are not claimed as implemented.
In `@src/ui/components/identity_pill.rs`:
- Around line 14-37: The display_label function can still return an empty string
when identity_id_base58 is empty; update display_label to defensively handle an
empty/whitespace identity_id_base58 before calling shorten_id (or make
shorten_id return a non-empty fallback). Specifically, in display_label (and
similarly used sites), if identity_id_base58.trim().is_empty() return a stable
non-empty placeholder (e.g. "Unknown identity" or similar), otherwise call
shorten_id(identity_id_base58); alternatively ensure shorten_id never yields ""
and returns the raw id or a placeholder when given empty input.
In `@src/ui/identity/hub_screen.rs`:
- Around line 155-164: The test currently only manipulates the IdentityHubTab
enum and therefore doesn't validate IdentityHubScreen's state methods; update
the test to instantiate an IdentityHubScreen via the existing test harness (or a
minimal AppContext-free constructor) and call IdentityHubScreen::selected_tab()
and IdentityHubScreen::select_tab() to verify transitions between
IdentityHubTab::Home and IdentityHubTab::Settings; if creating a full AppContext
is heavy, extract the tab state logic into a small helper (e.g., a TabState
struct or impl on IdentityHubScreen that can be new() in tests) and exercise
that helper instead so the test actually invokes the same code paths used by
IdentityHubScreen.
In `@src/ui/identity/mod.rs`:
- Around line 15-17: Update the module-level feature-gate note in the identity
module to accurately state that the `identity-hub` feature not only controls the
left-nav entry but also causes `app.rs` to omit the `RootScreenIdentityHub`
variant from the `main_screens` collection, so toggling the feature won’t
produce unreachable variants in the root screen enum; modify the comment at the
top of the identity module (where the current note lives) to mention both
behaviors and reference `RootScreenIdentityHub` and `main_screens` to prevent
future confusion when persisting root-screen state.
In `@tests/kittest/identity_hub.rs`:
- Around line 56-70: The test identity_hub_screen_type_creates_hub_screen is
currently tautological; change it to actually call ScreenType::create_screen
with the real kittest AppContext fixture and assert the returned Screen is the
IdentityHub variant (e.g., verify Screen::IdentityHubScreen(_) via matches! or
an if let), or if you intentionally want a compile-only guard rename the test
accordingly; specifically locate the ScreenType::create_screen call and the test
function and replace the enum-only assert with constructing the existing
AppContext test fixture (the same one used by identity_hub_mounts_and_renders),
invoke ScreenType::create_screen(app_ctx) and assert the result is
Screen::IdentityHubScreen(_).
- Around line 44-48: The test uses the wrong DashPay variant; replace
RootScreenType::RootScreenDashPayProfile with the true legacy root nav variant
RootScreenType::RootScreenDashpay so the assertions compare
RootScreenIdentityHub against both RootScreenIdentities and RootScreenDashpay;
update the variable name (e.g., legacy_dashpay) accordingly and keep the two
assert_ne! checks to ensure the new hub variant does not equal the legacy
Dashpay root.
🪄 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
Run ID: 2948ac86-677c-4a26-8e51-6efc8109f48a
📒 Files selected for processing (28)
Cargo.tomldocs/ai-design/2026-04-22-identity-dashpay-redesign/README.mddocs/ai-design/2026-04-22-identity-dashpay-redesign/design-spec.mddocs/ai-design/2026-04-22-identity-dashpay-redesign/wireframe.htmldocs/ai-design/2026-04-23-identity-hub-impl/01-requirements.mddocs/ai-design/2026-04-23-identity-hub-impl/02-ux-plan.mddocs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.mddocs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.mddocs/user-stories.mdsrc/app.rssrc/ui/components/README.mdsrc/ui/components/breadcrumb_pill.rssrc/ui/components/identity_pill.rssrc/ui/components/left_panel.rssrc/ui/components/mod.rssrc/ui/identity/activity.rssrc/ui/identity/contacts.rssrc/ui/identity/home.rssrc/ui/identity/hub_screen.rssrc/ui/identity/landing.rssrc/ui/identity/mod.rssrc/ui/identity/onboarding.rssrc/ui/identity/picker.rssrc/ui/identity/settings.rssrc/ui/identity/tabs.rssrc/ui/mod.rstests/kittest/identity_hub.rstests/kittest/main.rs
lklimek
marked this pull request as ready for review
April 23, 2026 11:51
Collaborator
|
✅ Review complete (commit 5f3772c) |
lklimek
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (4)
src/ui/components/activity_row.rs (2)
220-238: Route component styling throughComponentStyles.The row handles dark mode, but it computes surface/stroke styling directly with
DashColors. Please align this with the shared component styling layer so component theming stays centralized.As per coding guidelines,
src/ui/components/**/*.rs: “UI components must ... support both light and dark mode viaComponentStyles”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/activity_row.rs` around lines 220 - 238, The component currently computes surface and stroke using DashColors (in functions like accent_color, and the block creating surface, stroke, and Frame) which bypasses the shared theming layer; update the code to take and use the shared ComponentStyles (or an existing self.styles) for color and stroke values instead of DashColors so theming is centralized—use the styles' surface/background color for Frame.fill, use a styles.border or styles.card.stroke for the normal stroke, and use a styles.error-border (or equivalent) when matches!(self.status, ActivityRowStatus::Failed); keep the existing Frame construction (fill, stroke, corner_radius, margins, shadow) but source the color and stroke values from ComponentStyles rather than DashColors or hard-coded calls like DashColors::surface/DashColors::border_light/DashColors::ERROR.
75-82: Keep response flags encapsulated to preserve the action invariant.
clicked_bodyandclicked_retryare public mutable fields, whileactionis derived from them once. Downstream mutation can make these fields disagree withhas_changed()/changed_value(). Prefer private fields plus accessors.♻️ Proposed API tightening
pub struct ActivityRowResponse { /// Whether the user clicked the row body or the expand chevron. - pub clicked_body: bool, + clicked_body: bool, /// Whether the user clicked the `Retry` button (only ever `true` for /// `Failed` rows). - pub clicked_retry: bool, + clicked_retry: bool, /// The resolved action, if any. action: Option<ActivityRowAction>, }impl ActivityRowResponse { + /// Whether the user clicked the row body or the expand chevron. + pub fn clicked_body(&self) -> bool { + self.clicked_body + } + + /// Whether the user clicked the `Retry` button. + pub fn clicked_retry(&self) -> bool { + self.clicked_retry + } + /// The resolved action, if any. pub fn action(&self) -> Option<ActivityRowAction> { self.action } }As per coding guidelines,
src/ui/components/**/*.rs: “UI components must ... avoid public mutable fields and eager initialization”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/activity_row.rs` around lines 75 - 82, Make clicked_body and clicked_retry private on ActivityRowResponse and replace direct field access with accessor methods (e.g., pub fn clicked_body(&self) -> bool and pub fn clicked_retry(&self) -> bool) and controlled setters if mutation is needed; keep action private and stop eager initialization by computing or updating action inside changed_value()/has_changed() or inside the setters so the invariant (action matches flags) cannot be violated by downstream mutation. Update any call sites that read or write clicked_body/clicked_retry to use the new accessors/setters and ensure has_changed() and changed_value() derive their result from the authoritative state (computed or maintained by the setters) rather than stale public fields.tests/kittest/identity_hub.rs (1)
61-72: Make this guard exercisecreate_screen.This test only compares
ScreenType::IdentityHubto itself, so dropping theIdentityHubarm fromScreenType::create_screenwould not fail here. Either callScreenType::IdentityHub.create_screen(...)with a liveAppContextand match the returned screen variant, or rename this as an enum-availability guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/kittest/identity_hub.rs` around lines 61 - 72, The current test only self-compares ScreenType::IdentityHub and doesn't exercise ScreenType::create_screen; update the test (identity_hub_screen_type_creates_hub_screen) to construct a minimal/live AppContext, call ScreenType::IdentityHub.create_screen(&app_context) and assert the returned Screen is the IdentityHub variant (or pattern-match to ensure it yields the identity hub Screen), or alternatively rename the test to indicate it only checks enum availability; reference ScreenType::IdentityHub, create_screen, AppContext and the related identity_hub_mounts_and_renders test when implementing the minimal context.src/ui/components/identity_picker_card.rs (1)
241-247: Avoid stringly-typed dispatch betweenbadge_labelanddraw_type_badge.
badge_label()mapsIdentityType→&'static str, and thendraw_type_badgere-matches on those exact literals ("Masternode","Evonode") to pick fill/stroke/text colors. If a newIdentityTypevariant is added, the label text is ever changed, or it gets localized downstream, the color branch will silently fall through to the default (user) palette with no compile-time signal.Pass the
IdentityType(or a small localBadgeKind) through to the drawing helper so the palette is driven by the type, not by stringly comparing the visible label.♻️ Suggested refactor
- // Top row: avatar (left) + badge (right). - ui.horizontal(|ui| { - draw_monogram(ui, &heading, self.display_name.is_some(), dark_mode); - ui.add_space(8.0); - ui.with_layout(egui::Layout::right_to_left(egui::Align::TOP), |ui| { - draw_type_badge(ui, badge_label, dark_mode); - }); - }); + // Top row: avatar (left) + badge (right). + ui.horizontal(|ui| { + draw_monogram(ui, &heading, self.display_name.is_some(), dark_mode); + ui.add_space(8.0); + ui.with_layout(egui::Layout::right_to_left(egui::Align::TOP), |ui| { + draw_type_badge(ui, self.identity_type, badge_label, dark_mode); + }); + }); @@ -fn draw_type_badge(ui: &mut Ui, label: &str, dark_mode: bool) { - let (fill, stroke_color) = match label { - "Masternode" => (DashColors::PLATFORM_PURPLE, DashColors::PLATFORM_PURPLE), - "Evonode" => (DashColors::DASH_BLUE, DashColors::DASH_BLUE), - _ => ( - DashColors::surface_elevated(dark_mode), - DashColors::border(dark_mode), - ), - }; - let text_color = if matches!(label, "Masternode" | "Evonode") { - Color32::WHITE - } else { - DashColors::text_primary(dark_mode) - }; +fn draw_type_badge(ui: &mut Ui, kind: IdentityType, label: &str, dark_mode: bool) { + let (fill, stroke_color, text_color) = match kind { + IdentityType::Masternode => ( + DashColors::PLATFORM_PURPLE, + DashColors::PLATFORM_PURPLE, + Color32::WHITE, + ), + IdentityType::Evonode => ( + DashColors::DASH_BLUE, + DashColors::DASH_BLUE, + Color32::WHITE, + ), + IdentityType::User => ( + DashColors::surface_elevated(dark_mode), + DashColors::border(dark_mode), + DashColors::text_primary(dark_mode), + ), + };Also applies to: 405-428
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/components/identity_picker_card.rs` around lines 241 - 247, badge_label currently returns a & 'static str and draw_type_badge re-matches on those literals; change draw_type_badge to accept the IdentityType (or add a small local enum BadgeKind) instead of a label string and drive color selection by matching on that type (match on IdentityType::User / ::Masternode / ::Evonode) so adding/localizing variants won't break palette selection; keep badge_label solely for the displayed text and update all call sites (including the other occurrence around the 405-428 region) to pass the IdentityType/BadgeKind to draw_type_badge while still using badge_label() for the rendered label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user-stories.md`:
- Line 15: The TOC entry "[Identities Hub (IDH)](`#identities-hub-idh`)" is out of
sync with the actual "## Identities Hub (IDH)" section placement; either move
the "## Identities Hub (IDH)" section up so it appears before Token Operations
to match the TOC or move the TOC entry to the bottom so it matches the section
order, and ensure the section is separated with the standard '---' delimiter
consistent with other sections.
In `@src/ui/components/activity_row.rs`:
- Around line 401-458: The test ut_activity_row_01_failed_row_has_retry_button
currently only checks the Retry button and labels; add an assertion that the
Failed variant renders the danger border color by verifying the failed row's
rendered border/stroke equals DashColors::ERROR (inspect the failed_response
render metadata or query the harness for the element matching the failed row
label and assert its border color/stroke/style is DashColors::ERROR). Locate
this check around the existing failed_response/has_retry assertions (references:
ut_activity_row_01_failed_row_has_retry_button, ActivityRowStatus::Failed,
ActivityRow::has_retry, DashColors::ERROR) and fail the test if the border color
differs.
In `@src/ui/components/contact_row.rs`:
- Around line 20-34: Add an impl of the shared ComponentResponse trait for
ContactRowResponse (matching the pattern used by other identity hub components)
and implement its required methods so the component can be routed uniformly;
specifically implement changed_value to return Some(contact_id.clone()) when any
of clicked, send_clicked, or overflow_clicked is true, and return None otherwise
(also ensure any other trait methods follow the same semantics used by existing
components and reference ContactRowResponse and ContactRow::show for where
responses are produced).
In `@src/ui/components/identity_hero_card.rs`:
- Around line 244-268: with_avatar_bytes currently flips
avatar_uses_initials_fallback by setting avatar_bytes although the rendering
pipeline still paints initials; change the state so tests reflect actual
renderability: instead of directly setting avatar_bytes in with_avatar_bytes,
store the incoming bytes in a pending field (e.g., avatar_pending_bytes) or set
a separate avatar_texture_ready flag and only mark avatar_bytes (or
avatar_texture_ready = true) when the texture pipeline reports the image is
ready; update avatar_uses_initials_fallback to check the readiness flag (e.g.,
has_social_profile() && avatar_texture_ready is false) rather than mere presence
of raw bytes; apply the same change to the duplicate logic referenced at lines
441-449 so state and rendering stay consistent.
In `@src/ui/components/onboarding_checklist.rs`:
- Around line 247-304: The row is only clickable via the label; update
paint_step_row so the entire horizontal row area (including the circle and
padding) uses a single Sense::click() and drive clicked from that response
instead of label_resp. Follow the pattern in contact_row.rs: obtain the row
rect/response (e.g., call ui.allocate_rect or use
ui.interact/ui.allocate_exact_size for the full horizontal area or reuse the
rect that contains the circle and extend it to the row width), replace the
label's Sense::click() with a non-click label, and set clicked =
row_response.clicked() (keep the existing circle drawing and visual logic
intact).
In `@src/ui/components/request_card.rs`:
- Around line 49-59: RequestCardResponse currently exposes ad-hoc public
booleans; change it to a private-field struct that implements the
ComponentResponse trait by exposing a single typed action via changed_value()
and accessor for the id; introduce a RequestAction (e.g., Accepted, Declined,
Cancelled, None) enum and replace accepted/declined/cancelled booleans with a
single action field inside RequestCardResponse, make fields private, implement
ComponentResponse for RequestCardResponse with changed_value() returning
Option<RequestAction>, and update RequestCard::show (and any call sites) to
construct and return the new RequestCardResponse so callers consume the action
uniformly like other UI components.
In `@src/ui/components/social_profile_gate_card.rs`:
- Around line 49-56: SocialProfileGateCardResponse must implement the shared
ComponentResponse trait so it can be handled like other hub components; add an
impl ComponentResponse for SocialProfileGateCardResponse that forwards the
trait's primary/auxiliary query methods to this struct’s fields (map the trait's
primary/action accessor to primary_clicked and expose the “Why?” toggle via the
trait’s auxiliary/flag accessor or equivalent), and update any call sites
expecting a ComponentResponse to accept SocialProfileGateCardResponse.
In `@src/ui/identity/contacts.rs`:
- Around line 45-50: ContactsState currently only has load_requested and the
search query is recreated each render causing typed text to disappear; add a
persistent string field (e.g., search or search_query) to ContactsState and
initialize it in the Default/derive so it survives renders, then replace the
ephemeral local `search` variable used in the UI render/handler code with
ContactsState::search (update places that read/write the query — likely in the
contacts view and its input handlers around the code that previously created
`search` each render, and in the tab state usage referenced near the 196-205
region) so the input bindings update and read the stored field instead of a
transient local.
In `@src/ui/identity/home.rs`:
- Around line 96-104: The code currently chooses the hero identity by calling
first_loaded_identity(app_context), which ignores a hub-selected identity;
change the logic to first check the hub-selected identity (the value the picker
sets on the hub via app_context) and use that if present, falling back to
first_loaded_identity(app_context) only when no hub selection exists; update the
same pattern used later (around the block referenced at 417-422) so both the
hero and action handlers use the hub-selected identity from app_context instead
of always using first_loaded_identity, and preserve the render_empty(ui,
dark_mode) fallback when neither is available.
- Around line 215-222: The UI currently opens DPNS username registration when
paint_social_profile_card returns true; change the navigation to push the
profile editor instead so the card (which asks for display name, bio, avatar)
leads to the profile editor. Replace the
AppAction::AddScreen(ScreenType::RegisterDpnsName(RegisterDpnsNameSource::Identities).create_screen(app_context))
call with an AddScreen that creates the profile editor screen (use the existing
ScreenType variant/method for editing profiles — e.g., ScreenType::EditProfile
or ProfileEditor.create_screen(app_context) as appropriate), and apply the same
replacement at the other occurrence referenced (lines ~259-263) so both
no-profile card flows route to the profile editor rather than RegisterDpnsName;
ensure no checklist action (SetDisplayName) is toggled to skipped by this
change.
In `@src/ui/identity/hub_screen.rs`:
- Around line 149-214: The match arm for HubLanding::Home | HubLanding::Picker
is discarding the AppAction returned by the selected tab and also lumps Picker
with Home, making picker unreachable; change the logic so HubLanding::Picker is
handled separately (so the picker grid can be rendered) and when rendering
selected tab (IdentityHubTab::Home/Contacts/Activity/Settings) capture and
return the tab's AppAction instead of always returning AppAction::None, while
preserving existing state-reset behavior when selected_tab changes (references:
HubLanding, self.selected_tab, IdentityHubTabBar::new(...).show,
super::home::render, super::home::apply_outcome, self.contacts_state.reset, and
self.settings_tab.render).
In `@src/ui/identity/picker.rs`:
- Around line 135-138: compute_column_count currently divides available_width by
(CARD_MIN_WIDTH + GRID_GAP) which undercounts when the last card doesn't need a
trailing gap; update the calculation to add GRID_GAP to available_width before
dividing so the last column is allowed without a trailing gap (e.g. compute
columns as ((available_width + GRID_GAP) / (CARD_MIN_WIDTH + GRID_GAP)).floor()
as usize) while keeping the min of 1; adjust the function using the existing
names compute_column_count, CARD_MIN_WIDTH, and GRID_GAP.
- Around line 124-127: The code currently writes captured_selection (which can
be None) into selected_id_out on every render; only set the caller's slot when a
real click produced a non-None selection. Change the logic around
selected_id_out and captured_selection so you only assign when
captured_selection is Some(value) (e.g. if let (Some(slot), Some(selection)) =
(selected_id_out, captured_selection) { *slot = selection; }) or guard with
captured_selection.is_some() before dereferencing selected_id_out, thereby
avoiding overwriting the caller's existing selection with None.
In `@src/ui/identity/settings.rs`:
- Around line 647-651: The ensure_selected method currently always picks the
first local identity from app_context.load_local_qualified_identities(); change
it to prefer the hub-selected (active) identity id threaded into SettingsTab:
add an active_identity_id field to SettingsTab (or accept it as a parameter),
use that id to look up the matching identity from app_context (e.g., resolve via
load_local_qualified_identities() then find(|id| id.id == active_identity_id)),
set that as incoming when present, and only fall back to
identities.first().cloned() if no match is found; update callers that construct
SettingsTab or call ensure_selected to pass the active id.
- Around line 128-130: Several methods on SettingsTab (e.g., render, and the
other methods at the indicated ranges) have their parameters ordered as (&mut
self, ui: &mut Ui, app_context: &Arc<AppContext>) — change these to place
app_context immediately after self (i.e., &mut self, app_context:
&Arc<AppContext>, ui: &mut Ui); update the function signatures for render and
the other affected methods (refer to the SettingsTab methods around the noted
ranges) and then update every internal call sites (including ensure_selected and
any callers) to pass app_context as the first argument after self to keep the
repository convention consistent.
- Around line 287-302: The form is being marked clean immediately after
dispatching AppAction::BackendTask(DashPayTask::UpdateProfile), which prevents
retry if the backend fails; remove the immediate assignments to
self.original_display_name, self.original_bio, and self.original_avatar_url from
the save.clicked() branch and instead update those original_* fields only after
the UpdateProfile task completes successfully (e.g., in the code path that
handles the backend response or profile refresh), so the save button remains
enabled until a successful backend confirmation.
- Around line 232-263: The UI counters currently pass byte lengths (using
.len()) to counter(ui, ...) while validation_error() uses .chars().count(),
causing mismatch for non-ASCII input; update the three counter calls for
self.edit_display_name, self.edit_bio, and self.edit_avatar_url to use
.chars().count() so counter(...) and validation_error() use the same
character-counting logic (keep MAX_DISPLAY_NAME, MAX_BIO, MAX_AVATAR_URL and the
counter function unchanged).
In `@tests/kittest/identity_hub_activity.rs`:
- Around line 56-68: Add an assertion for the missing "Platform" filter chip so
the test verifies all four chips; locate the block that calls
harness.query_by_label("All"), ("Payments"), ("Funding") in the
identity_hub_activity test and add a similar assert for
harness.query_by_label("Platform"). Use the same assert!(...is_some(), "... must
render the `Platform` filter chip") pattern and message to match existing
assertions (refer to harness.query_by_label and the surrounding Activity tab
filter assertions).
In `@tests/kittest/identity_hub_home.rs`:
- Around line 31-44: The helper mount_hub leaves the welcome screen enabled so
AppState::update() renders welcome_screen.ui instead of the selected RootScreen,
preventing IdentityHubScreen from being exercised; fix by disabling the welcome
screen in the harness setup (e.g., set app.show_welcome_screen = false or call
the equivalent setter on dash_evo_tool::app::AppState before returning app in
mount_hub) and apply the same change to the other helper block at 55-63.
In `@tests/kittest/identity_hub_settings.rs`:
- Around line 31-45: mount_hub_on_settings currently creates AppState via
AppState::new which may leave show_welcome_screen true and never selects
IdentityHubTab::Settings, so the test renders the onboarding surface instead of
the Settings tab; fix by mirroring the onboarding test’s welcome bypass after
creating the app (e.g., set app.show_welcome_screen = false or call the same
helper used in onboarding) and explicitly set the hub tab to
IdentityHubTab::Settings (or set app.selected_identity_hub_tab =
IdentityHubTab::Settings) before calling harness.run_steps so the Settings tab
is actually mounted in mount_hub_on_settings.
---
Nitpick comments:
In `@src/ui/components/activity_row.rs`:
- Around line 220-238: The component currently computes surface and stroke using
DashColors (in functions like accent_color, and the block creating surface,
stroke, and Frame) which bypasses the shared theming layer; update the code to
take and use the shared ComponentStyles (or an existing self.styles) for color
and stroke values instead of DashColors so theming is centralized—use the
styles' surface/background color for Frame.fill, use a styles.border or
styles.card.stroke for the normal stroke, and use a styles.error-border (or
equivalent) when matches!(self.status, ActivityRowStatus::Failed); keep the
existing Frame construction (fill, stroke, corner_radius, margins, shadow) but
source the color and stroke values from ComponentStyles rather than DashColors
or hard-coded calls like
DashColors::surface/DashColors::border_light/DashColors::ERROR.
- Around line 75-82: Make clicked_body and clicked_retry private on
ActivityRowResponse and replace direct field access with accessor methods (e.g.,
pub fn clicked_body(&self) -> bool and pub fn clicked_retry(&self) -> bool) and
controlled setters if mutation is needed; keep action private and stop eager
initialization by computing or updating action inside
changed_value()/has_changed() or inside the setters so the invariant (action
matches flags) cannot be violated by downstream mutation. Update any call sites
that read or write clicked_body/clicked_retry to use the new accessors/setters
and ensure has_changed() and changed_value() derive their result from the
authoritative state (computed or maintained by the setters) rather than stale
public fields.
In `@src/ui/components/identity_picker_card.rs`:
- Around line 241-247: badge_label currently returns a & 'static str and
draw_type_badge re-matches on those literals; change draw_type_badge to accept
the IdentityType (or add a small local enum BadgeKind) instead of a label string
and drive color selection by matching on that type (match on IdentityType::User
/ ::Masternode / ::Evonode) so adding/localizing variants won't break palette
selection; keep badge_label solely for the displayed text and update all call
sites (including the other occurrence around the 405-428 region) to pass the
IdentityType/BadgeKind to draw_type_badge while still using badge_label() for
the rendered label.
In `@tests/kittest/identity_hub.rs`:
- Around line 61-72: The current test only self-compares ScreenType::IdentityHub
and doesn't exercise ScreenType::create_screen; update the test
(identity_hub_screen_type_creates_hub_screen) to construct a minimal/live
AppContext, call ScreenType::IdentityHub.create_screen(&app_context) and assert
the returned Screen is the IdentityHub variant (or pattern-match to ensure it
yields the identity hub Screen), or alternatively rename the test to indicate it
only checks enum availability; reference ScreenType::IdentityHub, create_screen,
AppContext and the related identity_hub_mounts_and_renders test when
implementing the minimal context.
🪄 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
Run ID: 6d381ea3-fb66-4dcc-a1b9-9a8ed9cac583
📒 Files selected for processing (33)
Cargo.tomldocs/ai-design/2026-04-23-identity-hub-impl/02-ux-plan.mddocs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.mddocs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.mddocs/user-stories.mdsrc/app.rssrc/ui/components/README.mdsrc/ui/components/activity_row.rssrc/ui/components/breadcrumb_pill.rssrc/ui/components/contact_row.rssrc/ui/components/identity_hero_card.rssrc/ui/components/identity_hub_tab_bar.rssrc/ui/components/identity_picker_add_card.rssrc/ui/components/identity_picker_card.rssrc/ui/components/identity_pill.rssrc/ui/components/mod.rssrc/ui/components/onboarding_checklist.rssrc/ui/components/request_card.rssrc/ui/components/social_profile_gate_card.rssrc/ui/identity/activity.rssrc/ui/identity/contacts.rssrc/ui/identity/home.rssrc/ui/identity/hub_screen.rssrc/ui/identity/mod.rssrc/ui/identity/picker.rssrc/ui/identity/settings.rstests/kittest/identity_hub.rstests/kittest/identity_hub_activity.rstests/kittest/identity_hub_contacts.rstests/kittest/identity_hub_home.rstests/kittest/identity_hub_onboarding.rstests/kittest/identity_hub_settings.rstests/kittest/main.rs
✅ Files skipped from review due to trivial changes (5)
- tests/kittest/main.rs
- src/ui/components/README.md
- docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md
- src/ui/identity/mod.rs
- docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
| - [Identity Operations (IDN)](#identity-operations-idn) | ||
| - [DPNS (DPN)](#dpns-dpn) | ||
| - [DashPay (DPY)](#dashpay-dpy) | ||
| - [Identities Hub (IDH)](#identities-hub-idh) |
Contributor
There was a problem hiding this comment.
Keep the TOC order and section placement in sync.
The TOC places “Identities Hub” before Token Operations, but the actual ## Identities Hub (IDH) section is appended after Programmatic Access and is not separated by the usual ---. Move the section to match the TOC order, or move the TOC entry to the bottom with the section.
Also applies to: 1053-1053
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/user-stories.md` at line 15, The TOC entry "[Identities Hub
(IDH)](`#identities-hub-idh`)" is out of sync with the actual "## Identities Hub
(IDH)" section placement; either move the "## Identities Hub (IDH)" section up
so it appears before Token Operations to match the TOC or move the TOC entry to
the bottom so it matches the section order, and ensure the section is separated
with the standard '---' delimiter consistent with other sections.
Comment on lines
+401
to
+458
| /// UT-ACTIVITY-ROW-01 — Failed row renders a Retry button and a | ||
| /// danger-stroke border. | ||
| /// | ||
| /// The test covers three variants in a single harness run so we | ||
| /// exercise the Normal, Expanded, and Failed render paths together, | ||
| /// as called out in the test-case spec. | ||
| #[test] | ||
| fn ut_activity_row_01_failed_row_has_retry_button() { | ||
| let mut harness = Harness::builder() | ||
| .with_size(egui::vec2(480.0, 400.0)) | ||
| .build_ui(|ui| { | ||
| let mut normal = ActivityRow::new(ActivityRowKind::Payment, "Sent 0.1 DASH") | ||
| .with_subtitle("To @alice") | ||
| .with_timestamp("5 min ago"); | ||
| let normal_response = normal.show(ui); | ||
| assert!(normal_response.inner.action().is_none()); | ||
|
|
||
| let mut expanded = ActivityRow::new(ActivityRowKind::Funding, "Added funds") | ||
| .with_subtitle("From wallet") | ||
| .with_timestamp("1 h ago") | ||
| .with_status(ActivityRowStatus::Expanded) | ||
| .with_detail("Advanced details."); | ||
| let expanded_response = expanded.show(ui); | ||
| assert!(expanded_response.inner.action().is_none()); | ||
|
|
||
| let mut failed = | ||
| ActivityRow::new(ActivityRowKind::Payment, "Could not send 0.1 DASH to @bob") | ||
| .with_timestamp("just now") | ||
| .with_status(ActivityRowStatus::Failed) | ||
| .with_detail( | ||
| "The network did not accept this payment. \ | ||
| Your balance is unchanged. Check your connection \ | ||
| and try again, or try a smaller amount.", | ||
| ); | ||
| let failed_response = failed.show(ui); | ||
| assert!(failed.has_retry()); | ||
| // No interaction simulated, so no action yet. | ||
| assert!(failed_response.inner.action().is_none()); | ||
| }); | ||
| harness.run(); | ||
|
|
||
| // The Retry button must be present — it is the distinguishing | ||
| // affordance of the Failed variant. | ||
| assert!( | ||
| harness.query_by_label("Retry").is_some(), | ||
| "Failed row must render a Retry button" | ||
| ); | ||
| // Normal and Expanded titles must render. | ||
| assert!(harness.query_by_label("Sent 0.1 DASH").is_some()); | ||
| assert!(harness.query_by_label("Added funds").is_some()); | ||
| assert!( | ||
| harness | ||
| .query_by_label("Could not send 0.1 DASH to @bob") | ||
| .is_some() | ||
| ); | ||
| // The expanded detail text must be visible. | ||
| assert!(harness.query_by_label("Advanced details.").is_some()); | ||
| } |
Contributor
There was a problem hiding this comment.
Assert the danger border promised by this test case.
UT-ACTIVITY-ROW-01 requires both the Retry button and danger-colored border, but this test only verifies the Retry button and labels. A regression from the failed-row DashColors::ERROR border to the default border would still pass.
🧪 Proposed coverage improvement
impl ActivityRow {
+ fn border_stroke(&self, dark_mode: bool) -> Stroke {
+ if matches!(self.status, ActivityRowStatus::Failed) {
+ Stroke::new(1.0, DashColors::ERROR)
+ } else {
+ Stroke::new(1.0, DashColors::border_light(dark_mode))
+ }
+ }
+
fn accent_color(&self, _dark_mode: bool) -> Color32 {
match (self.status, self.kind) {
(ActivityRowStatus::Failed, _) => DashColors::ERROR,- let stroke = if matches!(self.status, ActivityRowStatus::Failed) {
- Stroke::new(1.0, DashColors::ERROR)
- } else {
- Stroke::new(1.0, DashColors::border_light(dark_mode))
- };
+ let stroke = self.border_stroke(dark_mode); let mut failed =
ActivityRow::new(ActivityRowKind::Payment, "Could not send 0.1 DASH to `@bob`")
.with_timestamp("just now")
.with_status(ActivityRowStatus::Failed)
.with_detail(
"The network did not accept this payment. \
Your balance is unchanged. Check your connection \
and try again, or try a smaller amount.",
);
+ assert_eq!(failed.border_stroke(false).color, DashColors::ERROR);
let failed_response = failed.show(ui);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/components/activity_row.rs` around lines 401 - 458, The test
ut_activity_row_01_failed_row_has_retry_button currently only checks the Retry
button and labels; add an assertion that the Failed variant renders the danger
border color by verifying the failed row's rendered border/stroke equals
DashColors::ERROR (inspect the failed_response render metadata or query the
harness for the element matching the failed row label and assert its border
color/stroke/style is DashColors::ERROR). Locate this check around the existing
failed_response/has_retry assertions (references:
ut_activity_row_01_failed_row_has_retry_button, ActivityRowStatus::Failed,
ActivityRow::has_retry, DashColors::ERROR) and fail the test if the border color
differs.
Comment on lines
+56
to
+68
| // Filter chips — called out verbatim in the test-case spec. | ||
| assert!( | ||
| harness.query_by_label("All").is_some(), | ||
| "Activity tab must render the `All` filter chip" | ||
| ); | ||
| assert!( | ||
| harness.query_by_label("Payments").is_some(), | ||
| "Activity tab must render the `Payments` filter chip" | ||
| ); | ||
| assert!( | ||
| harness.query_by_label("Funding").is_some(), | ||
| "Activity tab must render the `Funding` filter chip" | ||
| ); |
Contributor
There was a problem hiding this comment.
Assert the Platform chip too.
The Activity tab contract includes All, Payments, Funding, and Platform; this test omits Platform, so one filter can disappear while IT-ACTIVITY-01 stays green.
🧪 Proposed test coverage addition
assert!(
harness.query_by_label("Funding").is_some(),
"Activity tab must render the `Funding` filter chip"
);
+ assert!(
+ harness.query_by_label("Platform").is_some(),
+ "Activity tab must render the `Platform` filter chip"
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Filter chips — called out verbatim in the test-case spec. | |
| assert!( | |
| harness.query_by_label("All").is_some(), | |
| "Activity tab must render the `All` filter chip" | |
| ); | |
| assert!( | |
| harness.query_by_label("Payments").is_some(), | |
| "Activity tab must render the `Payments` filter chip" | |
| ); | |
| assert!( | |
| harness.query_by_label("Funding").is_some(), | |
| "Activity tab must render the `Funding` filter chip" | |
| ); | |
| // Filter chips — called out verbatim in the test-case spec. | |
| assert!( | |
| harness.query_by_label("All").is_some(), | |
| "Activity tab must render the `All` filter chip" | |
| ); | |
| assert!( | |
| harness.query_by_label("Payments").is_some(), | |
| "Activity tab must render the `Payments` filter chip" | |
| ); | |
| assert!( | |
| harness.query_by_label("Funding").is_some(), | |
| "Activity tab must render the `Funding` filter chip" | |
| ); | |
| assert!( | |
| harness.query_by_label("Platform").is_some(), | |
| "Activity tab must render the `Platform` filter chip" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/kittest/identity_hub_activity.rs` around lines 56 - 68, Add an
assertion for the missing "Platform" filter chip so the test verifies all four
chips; locate the block that calls harness.query_by_label("All"), ("Payments"),
("Funding") in the identity_hub_activity test and add a similar assert for
harness.query_by_label("Platform"). Use the same assert!(...is_some(), "... must
render the `Platform` filter chip") pattern and message to match existing
assertions (refer to harness.query_by_label and the surrounding Activity tab
filter assertions).
Comment on lines
+31
to
+44
| fn mount_hub() -> Harness<'static, dash_evo_tool::app::AppState> { | ||
| let rt = tokio::runtime::Runtime::new().expect("Failed to create tokio runtime"); | ||
| let _guard = rt.enter(); | ||
|
|
||
| let mut harness = Harness::builder().with_max_steps(100).build_eframe(|ctx| { | ||
| let mut app = dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone()) | ||
| .expect("Failed to create AppState") | ||
| .with_animations(false); | ||
| app.selected_main_screen = RootScreenType::RootScreenIdentityHub; | ||
| app | ||
| }); | ||
| harness.set_size(egui::vec2(1280.0, 800.0)); | ||
| harness.run_steps(10); | ||
| harness |
Contributor
There was a problem hiding this comment.
Bypass the welcome screen before claiming hub coverage.
Unlike identity_hub_onboarding.rs, this helper leaves show_welcome_screen enabled. On a fresh test DB, AppState::update() renders welcome_screen.ui(ctx) instead of the selected root screen, so home_tab_mounts_without_panic() may not exercise IdentityHubScreen at all.
Proposed test fix
let mut app = dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone())
.expect("Failed to create AppState")
.with_animations(false);
+ app.show_welcome_screen = false;
+ app.welcome_screen = None;
app.selected_main_screen = RootScreenType::RootScreenIdentityHub;
appAlso applies to: 55-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/kittest/identity_hub_home.rs` around lines 31 - 44, The helper
mount_hub leaves the welcome screen enabled so AppState::update() renders
welcome_screen.ui instead of the selected RootScreen, preventing
IdentityHubScreen from being exercised; fix by disabling the welcome screen in
the harness setup (e.g., set app.show_welcome_screen = false or call the
equivalent setter on dash_evo_tool::app::AppState before returning app in
mount_hub) and apply the same change to the other helper block at 55-63.
Comment on lines
+31
to
+45
| fn mount_hub_on_settings() -> Harness<'static, dash_evo_tool::app::AppState> { | ||
| let rt = tokio::runtime::Runtime::new().expect("Failed to create tokio runtime"); | ||
| let _guard = rt.enter(); | ||
|
|
||
| let mut harness = Harness::builder().with_max_steps(100).build_eframe(|ctx| { | ||
| let mut app = dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone()) | ||
| .expect("Failed to create AppState") | ||
| .with_animations(false); | ||
| app.selected_main_screen = RootScreenType::RootScreenIdentityHub; | ||
| app | ||
| }); | ||
| harness.set_size(egui::vec2(1280.0, 800.0)); | ||
| // Run a few frames so the onboarding landing renders first; later steps | ||
| // simulate the user clicking the Settings tab. | ||
| harness.run_steps(5); |
Contributor
There was a problem hiding this comment.
Make this actually mount the Settings tab.
AppState::new() can start with show_welcome_screen = true, and this helper never selects IdentityHubTab::Settings, so the test can pass while only rendering the welcome/onboarding surface instead of the Settings tab. Mirror the onboarding test’s welcome bypass and explicitly select the hub tab before running frames.
Proposed test fix
-use dash_evo_tool::ui::RootScreenType;
+use dash_evo_tool::ui::{RootScreenType, Screen};
@@
let mut app = dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone())
.expect("Failed to create AppState")
.with_animations(false);
+ app.show_welcome_screen = false;
+ app.welcome_screen = None;
app.selected_main_screen = RootScreenType::RootScreenIdentityHub;
+ if let Some(Screen::IdentityHubScreen(hub)) =
+ app.main_screens.get_mut(&RootScreenType::RootScreenIdentityHub)
+ {
+ hub.select_tab(IdentityHubTab::Settings);
+ } else {
+ panic!("Identity Hub screen must be registered for this test");
+ }
appAlso applies to: 57-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/kittest/identity_hub_settings.rs` around lines 31 - 45,
mount_hub_on_settings currently creates AppState via AppState::new which may
leave show_welcome_screen true and never selects IdentityHubTab::Settings, so
the test renders the onboarding surface instead of the Settings tab; fix by
mirroring the onboarding test’s welcome bypass after creating the app (e.g., set
app.show_welcome_screen = false or call the same helper used in onboarding) and
explicitly set the hub tab to IdentityHubTab::Settings (or set
app.selected_identity_hub_tab = IdentityHubTab::Settings) before calling
harness.run_steps so the Settings tab is actually mounted in
mount_hub_on_settings.
thepastaclaw
suggested changes
Apr 23, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
Validated findings against the checked-out code (HEAD b71e1fe). The PR has five genuine correctness issues: the picker branch is dead code so multi-identity users silently operate on the first identity; two quick-action tooltips don't match the flows they route to; network switches leave the Contacts tab in a stale 'already loaded' state; the populated Contacts path never dispatches LoadContactRequests; and every tab independently resolves its own 'current identity' via identities.first(), which will defeat the picker once it is wired. Tests are shallow smoke tests that cannot catch any of these. A few lower-severity issues (per-frame SQLite reads, unreachable!() in a UI path, dispatcher weakness) round out the set.
Reviewed commit: b71e1fe
🔴 5 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/ui/identity/hub_screen.rs`:
- [BLOCKING] lines 149-217: `HubLanding::Picker` is dead code — multi-identity users never see the picker
`landing()` returns `HubLanding::Picker` for 2+ identities, but `ui()` collapses `HubLanding::Home | HubLanding::Picker` into the same match arm and never calls `super::picker::render`. A wallet with multiple identities therefore cannot choose which identity the hub operates on — it silently targets whichever identity is first in storage order. The fully implemented picker grid in `src/ui/identity/picker.rs` is unreachable from production code.
- [SUGGESTION] lines 79-102: `landing()` reloads all qualified identities from SQLite on every frame, plus 3 more per-frame reloads in tabs
`landing()` calls `load_local_qualified_identities()` unconditionally every repaint, and Home (home.rs:582), Contacts (contacts.rs:162), and Settings (settings.rs:649) each make their own independent call in the same frame. At 60 fps that is 4 SQLite reads per frame per hub surface. Cache the identity list on the hub once per refresh (or on explicit invalidation) and pass slices into tab renderers.
In `src/ui/identity/settings.rs`:
- [BLOCKING] lines 647-651: Tabs independently pick the 'current identity' via `identities.first()`
Settings (line 651), Home (home.rs:594), and Contacts (contacts.rs:436) each re-resolve their target identity by calling `load_local_qualified_identities()` and taking `.first()`. Identity selection is not owned by the hub. Even once the picker is wired (see related finding), tab content will not be bound to the identity the user actually selected — each tab re-picks its own 'current' identity from collection order on every frame. The hub screen needs to own a `selected_identity` and pass it (or a slice/index) into tab renderers.
In `src/ui/mod.rs`:
- [BLOCKING] lines 921-924: Network switches leave the hub's Contacts tab in a permanently 'already loaded' state
`change_context` for `Screen::IdentityHubScreen` only swaps `screen.app_context` and returns — it does not call `screen.refresh()` or reset `contacts_state`. Because Contacts gates its `LoadContacts` dispatch on `state_guard.load_requested` (contacts.rs:318), the Contacts tab will never refetch for the new network if it had already loaded once on the previous network. The populated shell will keep showing data for the old network's identity until the user manually refreshes. The screen should call `self.refresh()` (or at least `self.contacts_state.reset()`) when the context changes.
In `src/ui/identity/contacts.rs`:
- [BLOCKING] lines 313-325: Contacts populated path never dispatches `LoadContactRequests`
The module comment at line 240–242 states the populated shell is backed by both `DashPayTask::LoadContacts` *and* `LoadContactRequests`, but only `LoadContacts` is dispatched at line 320. The 'Received requests' (line 255) and 'Sent requests' (line 301) sections therefore cannot hydrate from backend data — they will display placeholder empty-state copy even when real requests exist. Either dispatch `LoadContactRequests` alongside `LoadContacts`, or remove the receive/sent sections from the shell until that task is wired.
- [SUGGESTION] lines 224-228: `unreachable!()` in a UI dispatch path can panic the entire app
`render_gated` uses `unreachable!("GateSetUpProfile should not map to OpenScreen")` as the catch-all arm. It is genuinely unreachable today, but this is a UI click handler — a future dispatcher change that adds `OpenScreen` to this button would turn a click into an app-level panic rather than a dead click. Prefer a no-op branch with a warning log (or an exhaustive match keyed by variant) so the UI fails safely.
In `src/ui/identity/home.rs`:
- [BLOCKING] lines 177-194: `Send` / `Receive` quick-action tooltips do not match the flows they open
`home_button_kind` maps `HomeButton::Send -> OpenScreen(Transfer)` and `HomeButton::Receive -> OpenScreen(TopUp)`. `HomeScreenKind::Transfer` opens `ScreenType::TransferScreen` (identity→identity credits transfer) and `HomeScreenKind::TopUp` opens `TopUpIdentity` (move *wallet* funds *into* the identity — explicitly 'move wallet Dash into the identity' per the doc comment at line 155). The rendered tooltips at home.rs:304 ('Send Dash to a contact, username, or address.') and home.rs:313 ('Show a QR code or your username so someone can pay you.') promise very different behavior. Receive in particular is the opposite of TopUp — TopUp sends wallet funds into the identity, not an inbound payment surface. Either correct the routing or change the tooltip copy to reflect what actually happens.
- [SUGGESTION] lines 163-170: `HomeButtonKind::is_dead` cannot detect wrong-screen routing
`is_dead` only flags `Outcome(HomeOutcome::None)` and treats every `OpenScreen(_)` as live. Because of that, the dispatcher unit tests (described in the enum doc comment as the 'ground-truth check' after Wave 2's dead-on-arrival buttons) actively pin the wrong `Send -> Transfer` / `Receive -> TopUp` mapping in place. Consider asserting the expected `HomeScreenKind` per button, not just 'non-None'.
In `tests/kittest/identity_hub.rs`:
- [SUGGESTION] lines 12-73: Hub kittests are smoke tests that cannot catch any of the regressions above
`mount_hub()` uses default `AppState::new`, so the DB has zero identities and `landing()` resolves to `Onboarding` — the tests never render the Home, Picker, Contacts, Activity, or Settings surfaces they purport to cover. `identity_hub_mounts_and_renders` only asserts non-panic, and `legacy_nav_entries_coexist_with_hub` is a pure enum check. None of the dead-picker, wrong-screen-routing, network-switch, or `LoadContactRequests`-missing bugs above would be caught. Add fixtures that seed 1 and 2+ identities in the local DB and assert that the rendered node tree contains the expected tab labels / picker grid / request sections.
Comment on lines
+12
to
+73
| fn mount_hub() -> Harness<'static, dash_evo_tool::app::AppState> { | ||
| let rt = tokio::runtime::Runtime::new().expect("Failed to create tokio runtime"); | ||
| let _guard = rt.enter(); | ||
|
|
||
| let mut harness = Harness::builder().with_max_steps(100).build_eframe(|ctx| { | ||
| let mut app = dash_evo_tool::app::AppState::new(ctx.egui_ctx.clone()) | ||
| .expect("Failed to create AppState") | ||
| .with_animations(false); | ||
| app.selected_main_screen = RootScreenType::RootScreenIdentityHub; | ||
| app | ||
| }); | ||
| harness.set_size(egui::vec2(1280.0, 800.0)); | ||
| harness.run_steps(10); | ||
| harness | ||
| } | ||
|
|
||
| /// IT-ONBOARD-01 / IT-HOME-01 combined smoke: the hub renders without | ||
| /// panicking on the default first-run database (no identities loaded → should | ||
| /// render the onboarding empty state). | ||
| #[test] | ||
| fn identity_hub_mounts_and_renders() { | ||
| let _harness = mount_hub(); | ||
| // If `mount_hub` returned without panicking, the hub compiled-in and | ||
| // rendered. More detailed assertions land with the per-tab content work. | ||
| } | ||
|
|
||
| /// IT-NAV-01: The nav must keep the legacy `Identities` and `Dashpay` entries | ||
| /// alongside the new hub so users can toggle between old and new. | ||
| #[test] | ||
| fn legacy_nav_entries_coexist_with_hub() { | ||
| // We don't need to drive the UI for this one — it's a pure enum check. | ||
| // The `RootScreenType` enum must contain all three coexisting variants. | ||
| let legacy_identities = RootScreenType::RootScreenIdentities; | ||
| // `RootScreenDashpay` is the legacy root nav entry for DashPay; the other | ||
| // `RootScreenDashPay*` variants are sub-screens within that section. | ||
| let legacy_dashpay_root = RootScreenType::RootScreenDashpay; | ||
| let new_hub = RootScreenType::RootScreenIdentityHub; | ||
| assert_ne!(legacy_identities, new_hub); | ||
| assert_ne!(legacy_dashpay_root, new_hub); | ||
| // Round-trip the new variant through on-disk encoding to verify the | ||
| // persistence contract is stable. | ||
| let encoded = new_hub.to_int(); | ||
| let decoded = RootScreenType::from_int(encoded).expect("hub variant must decode"); | ||
| assert_eq!(new_hub, decoded); | ||
| } | ||
|
|
||
| /// The hub screen must be reachable from the existing `create_screen` | ||
| /// dispatch table. This asserts the wiring that AppState::new relies on. | ||
| #[test] | ||
| fn identity_hub_screen_type_creates_hub_screen() { | ||
| // Guard against a future refactor that silently drops the hub case from | ||
| // `ScreenType::create_screen`. If that happens, this test regresses. | ||
| use dash_evo_tool::ui::ScreenType; | ||
| // The assert-that-it-compiles-and-matches is enough; the dispatcher | ||
| // expects a Screen with IdentityHub variant. | ||
| let screen_type = ScreenType::IdentityHub; | ||
| assert_eq!(screen_type, ScreenType::IdentityHub); | ||
| // `ScreenType::create_screen` requires a live AppContext; instead of | ||
| // constructing one here we verify through the enum discriminant. The end- | ||
| // to-end wiring is exercised by `identity_hub_mounts_and_renders`. | ||
| let _ = screen_type; | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Hub kittests are smoke tests that cannot catch any of the regressions above
mount_hub() uses default AppState::new, so the DB has zero identities and landing() resolves to Onboarding — the tests never render the Home, Picker, Contacts, Activity, or Settings surfaces they purport to cover. identity_hub_mounts_and_renders only asserts non-panic, and legacy_nav_entries_coexist_with_hub is a pure enum check. None of the dead-picker, wrong-screen-routing, network-switch, or LoadContactRequests-missing bugs above would be caught. Add fixtures that seed 1 and 2+ identities in the local DB and assert that the rendered node tree contains the expected tab labels / picker grid / request sections.
source: ['claude-general', 'codex-general', 'claude-rust-quality']
🤖 Fix this 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 `tests/kittest/identity_hub.rs`:
- [SUGGESTION] lines 12-73: Hub kittests are smoke tests that cannot catch any of the regressions above
`mount_hub()` uses default `AppState::new`, so the DB has zero identities and `landing()` resolves to `Onboarding` — the tests never render the Home, Picker, Contacts, Activity, or Settings surfaces they purport to cover. `identity_hub_mounts_and_renders` only asserts non-panic, and `legacy_nav_entries_coexist_with_hub` is a pure enum check. None of the dead-picker, wrong-screen-routing, network-switch, or `LoadContactRequests`-missing bugs above would be caught. Add fixtures that seed 1 and 2+ identities in the local DB and assert that the rendered node tree contains the expected tab labels / picker grid / request sections.
Comment on lines
+163
to
+170
| impl HomeButtonKind { | ||
| /// A button is "dead" when pressing it would produce neither an | ||
| /// `AppAction::AddScreen` nor a meaningful [`HomeOutcome`]. This is the | ||
| /// invariant checked by the dead-button unit test. | ||
| pub fn is_dead(self) -> bool { | ||
| matches!(self, HomeButtonKind::Outcome(HomeOutcome::None)) | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: HomeButtonKind::is_dead cannot detect wrong-screen routing
is_dead only flags Outcome(HomeOutcome::None) and treats every OpenScreen(_) as live. Because of that, the dispatcher unit tests (described in the enum doc comment as the 'ground-truth check' after Wave 2's dead-on-arrival buttons) actively pin the wrong Send -> Transfer / Receive -> TopUp mapping in place. Consider asserting the expected HomeScreenKind per button, not just 'non-None'.
source: ['claude-general']
🤖 Fix this 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/identity/home.rs`:
- [SUGGESTION] lines 163-170: `HomeButtonKind::is_dead` cannot detect wrong-screen routing
`is_dead` only flags `Outcome(HomeOutcome::None)` and treats every `OpenScreen(_)` as live. Because of that, the dispatcher unit tests (described in the enum doc comment as the 'ground-truth check' after Wave 2's dead-on-arrival buttons) actively pin the wrong `Send -> Transfer` / `Receive -> TopUp` mapping in place. Consider asserting the expected `HomeScreenKind` per button, not just 'non-None'.
Comment on lines
+79
to
+102
| pub(crate) fn landing(&mut self, ctx: &Context) -> HubLanding { | ||
| match self.app_context.load_local_qualified_identities() { | ||
| Ok(identities) => { | ||
| // Clear any previously-shown error banner now that loading works. | ||
| self.load_error_banner.take_and_clear(); | ||
| let landing = HubLanding::from_identity_count(identities.len()); | ||
| self.last_good_landing = landing; | ||
| landing | ||
| } | ||
| Err(e) => { | ||
| // Idempotent: set_global de-duplicates by text, so repainting | ||
| // this frame after frame does not spam banners. | ||
| let handle = MessageBanner::set_global( | ||
| ctx, | ||
| "Could not load your identities from this device. Try refreshing or \ | ||
| reopening the app.", | ||
| MessageType::Error, | ||
| ); | ||
| handle.with_details(&e); | ||
| self.load_error_banner = Some(handle); | ||
| self.last_good_landing | ||
| } | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: landing() reloads all qualified identities from SQLite on every frame, plus 3 more per-frame reloads in tabs
landing() calls load_local_qualified_identities() unconditionally every repaint, and Home (home.rs:582), Contacts (contacts.rs:162), and Settings (settings.rs:649) each make their own independent call in the same frame. At 60 fps that is 4 SQLite reads per frame per hub surface. Cache the identity list on the hub once per refresh (or on explicit invalidation) and pass slices into tab renderers.
source: ['claude-general', 'claude-rust-quality']
🤖 Fix this 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/identity/hub_screen.rs`:
- [SUGGESTION] lines 79-102: `landing()` reloads all qualified identities from SQLite on every frame, plus 3 more per-frame reloads in tabs
`landing()` calls `load_local_qualified_identities()` unconditionally every repaint, and Home (home.rs:582), Contacts (contacts.rs:162), and Settings (settings.rs:649) each make their own independent call in the same frame. At 60 fps that is 4 SQLite reads per frame per hub surface. Cache the identity list on the hub once per refresh (or on explicit invalidation) and pass slices into tab renderers.
Comment on lines
+224
to
+228
| ContactsButtonKind::OpenScreen(_) => { | ||
| // Not possible today (dispatcher returns SwitchHubTab), but | ||
| // exhaustive match future-proofs the gate CTA. | ||
| unreachable!("GateSetUpProfile should not map to OpenScreen"); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: unreachable!() in a UI dispatch path can panic the entire app
render_gated uses unreachable!("GateSetUpProfile should not map to OpenScreen") as the catch-all arm. It is genuinely unreachable today, but this is a UI click handler — a future dispatcher change that adds OpenScreen to this button would turn a click into an app-level panic rather than a dead click. Prefer a no-op branch with a warning log (or an exhaustive match keyed by variant) so the UI fails safely.
source: ['claude-rust-quality']
🤖 Fix this 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/identity/contacts.rs`:
- [SUGGESTION] lines 224-228: `unreachable!()` in a UI dispatch path can panic the entire app
`render_gated` uses `unreachable!("GateSetUpProfile should not map to OpenScreen")` as the catch-all arm. It is genuinely unreachable today, but this is a UI click handler — a future dispatcher change that adds `OpenScreen` to this button would turn a click into an app-level panic rather than a dead click. Prefer a no-op branch with a warning log (or an exhaustive match keyed by variant) so the UI fails safely.
Comment on lines
+192
to
+335
| let mut action = AppAction::None; | ||
| let dark_mode = ui.ctx().style().visuals.dark_mode; | ||
|
|
||
| section_heading(ui, "Social profile", dark_mode); | ||
| ui.label( | ||
| RichText::new("This information is visible to everyone on Dash Platform.") | ||
| .small() | ||
| .color(DashColors::text_secondary(dark_mode)), | ||
| ); | ||
| ui.add_space(8.0); | ||
|
|
||
| // Avatar block — placeholder glyph + "Change photo" ghost button. | ||
| // The actual file-picker wiring lives in `ProfileScreen`; we surface | ||
| // the button and let the user click through to the legacy edit path | ||
| // in a follow-up (no backend task needed yet). | ||
| ui.horizontal(|ui| { | ||
| ui.label(RichText::new("👤").size(48.0).color(DashColors::DEEP_BLUE)); | ||
| ui.vertical(|ui| { | ||
| let btn = ComponentStyles::add_secondary_button(ui, "Change photo", dark_mode) | ||
| .clickable_tooltip(TIP_CHANGE_PHOTO); | ||
| if btn.clicked() { | ||
| // Route to legacy DashPay Profile screen for the full | ||
| // image-upload flow. This is NOT a backend task and does | ||
| // not violate the "additive only" rule. | ||
| action = AppAction::SetMainScreen( | ||
| crate::ui::RootScreenType::RootScreenDashPayProfile, | ||
| ); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| ui.add_space(8.0); | ||
|
|
||
| // Display name input. | ||
| ui.label(RichText::new("Display name").color(DashColors::text_primary(dark_mode))); | ||
| let display_name = ui.add( | ||
| TextEdit::singleline(&mut self.edit_display_name) | ||
| .hint_text("How should people see your name?") | ||
| .desired_width(f32::INFINITY), | ||
| ); | ||
| counter( | ||
| ui, | ||
| self.edit_display_name.len(), | ||
| MAX_DISPLAY_NAME, | ||
| dark_mode, | ||
| ); | ||
| let _ = display_name; // response not needed beyond widget side-effects | ||
|
|
||
| ui.add_space(8.0); | ||
|
|
||
| // Bio textarea. | ||
| ui.label(RichText::new("About").color(DashColors::text_primary(dark_mode))); | ||
| ui.add( | ||
| TextEdit::multiline(&mut self.edit_bio) | ||
| .hint_text(format!("A short description, up to {MAX_BIO} characters.")) | ||
| .desired_width(f32::INFINITY) | ||
| .desired_rows(4), | ||
| ); | ||
| counter(ui, self.edit_bio.len(), MAX_BIO, dark_mode); | ||
|
|
||
| ui.add_space(8.0); | ||
|
|
||
| // Avatar URL — we include it so users can still set the avatar when | ||
| // the file-picker flow is not yet available from this tab. Kept under | ||
| // the visual avatar block so it is clearly secondary. | ||
| ui.label(RichText::new("Avatar URL").color(DashColors::text_primary(dark_mode))); | ||
| ui.add( | ||
| TextEdit::singleline(&mut self.edit_avatar_url) | ||
| .hint_text("https://example.com/avatar.jpg") | ||
| .desired_width(f32::INFINITY), | ||
| ); | ||
| counter(ui, self.edit_avatar_url.len(), MAX_AVATAR_URL, dark_mode); | ||
|
|
||
| ui.add_space(12.0); | ||
|
|
||
| // Save / Delete buttons row. | ||
| let invalid = self.validation_error().is_some(); | ||
| let dirty = self.has_changes(); | ||
| let can_save = !invalid && dirty; | ||
| let save_tooltip = if !dirty { | ||
| TIP_SAVE_NO_CHANGES.to_string() | ||
| } else if invalid { | ||
| TIP_SAVE_INVALID.to_string() | ||
| } else { | ||
| "Save your social profile to DashPay.".to_string() | ||
| }; | ||
|
|
||
| ui.horizontal(|ui| { | ||
| let save = | ||
| ComponentStyles::add_primary_button_enabled(ui, can_save, "Save social profile"); | ||
| let save = if can_save { | ||
| save.clickable_tooltip(save_tooltip) | ||
| } else { | ||
| save.disabled_tooltip(save_tooltip) | ||
| }; | ||
| if save.clicked() && can_save { | ||
| action = AppAction::BackendTask(BackendTask::DashPayTask(Box::new( | ||
| DashPayTask::UpdateProfile { | ||
| identity: identity.clone(), | ||
| display_name: string_if_set(&self.edit_display_name), | ||
| bio: string_if_set(&self.edit_bio), | ||
| avatar_url: string_if_set(&self.edit_avatar_url), | ||
| }, | ||
| ))); | ||
| // Mirror the new state as the baseline so the save button | ||
| // disables again until the next edit. The backend completion | ||
| // round-trip will refresh the profile for real. | ||
| self.original_display_name = self.edit_display_name.clone(); | ||
| self.original_bio = self.edit_bio.clone(); | ||
| self.original_avatar_url = self.edit_avatar_url.clone(); | ||
| } | ||
|
|
||
| ui.add_space(12.0); | ||
|
|
||
| // GATED: DashPayTask::DeleteProfile does not exist (2026-04-23). | ||
| // Render as a non-interactive danger-style link so Alex can see | ||
| // the affordance and knows it is planned. | ||
| // TODO(identity-hub): wire once DashPayTask::DeleteProfile lands. | ||
| let delete = ui | ||
| .add_enabled( | ||
| false, | ||
| egui::Button::new( | ||
| RichText::new("Delete social profile").color(DashColors::ERROR), | ||
| ) | ||
| .fill(egui::Color32::TRANSPARENT) | ||
| .stroke(egui::Stroke::NONE), | ||
| ) | ||
| .disabled_tooltip(format!("{TIP_DELETE_PROFILE} {GATED_COMING_SOON}")); | ||
| if delete.clicked() { | ||
| // Unreachable while disabled; defensive — open the confirm | ||
| // dialog so, once the backend exists, this path activates | ||
| // with a single-line change (remove `add_enabled(false, …)`). | ||
| self.confirm_delete_profile = Some( | ||
| ConfirmationDialog::new( | ||
| "Delete social profile", | ||
| "Remove the display name, bio, and avatar from DashPay. Your \ | ||
| identity, usernames, and balance stay intact. Are you sure?", | ||
| ) | ||
| .confirm_text(Some("Delete")) | ||
| .cancel_text(Some("Keep")) | ||
| .danger_mode(true), | ||
| ); | ||
| } | ||
| }); |
Collaborator
There was a problem hiding this comment.
💬 Nitpick: Several settings sub-renderers assign action = ... instead of action |= ...
Some settings sub-renderers use plain assignment when building AppAction, where sibling code (and the rest of the hub) uses |=. Current call sites happen to tolerate this, but it is a footgun if a sub-renderer ever produces more than one action in a frame — the earlier action will be silently dropped. Switch to |= for consistency with the rest of the hub.
source: ['claude-general']
lklimek
marked this pull request as draft
April 27, 2026 08:55
Collapse today's separate Identities and Dashpay nav entries into a unified Identities section with Home / Contacts / Activity / Settings tabs, a two-pill Wallet + Identity switcher, and an identity-first naming model where DashPay profile is an optional social-profile overlay. Adds wireframe.html (8 frames, persona + theme toggles), design-spec.md (IA, screen-by-screen, wording audit, tooltip catalog), and README.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes Diziet and Adams audit findings:
- Gate F4/F8 Advanced blocks so Alex never sees raw IDs (.adv class)
- Wire aria-describedby on ~60 tooltip triggers (screen-reader access)
- Add tooltips to identity-type, network, and connection badges
- Add .alex-only CSS gate + class on insight banner (P3)
- Drop protx: jargon prefix from Masternode ID chips in F4 and F8
- Adopt spec wallet-pill phrase "Funded by {wallet_name}" across all frames
- Add topbar Refresh icon-button to F3/F4/F5/F6/F7/F8
- Fix F3/F4 switcher pills: tooltip wrappers + aria-haspopup parity
- Add secondary actions row (Add funds / Send to wallet / Send to another identity) on F3 and F4
- Remove dead enabled-Review markup (FG4)
- Wording: "No social profile yet", skip-link, funding subtitle, activity subtitle
- Nits: dead white-space rule, redundant inline style, redundant font-weight load
- Spec: Shadow alpha intentional deviation documented in §E and §F
- Spec: §B.9 Add-funds wizard with all 4 funding methods and persona gating
- Spec: §B.10 Create-identity wizard flow
- Spec: §B.11 Load-existing-identity with all 3 load modes
- Spec: §B.8 Voter-identity keys (Masternode/Evonode), Local nickname, Auto-accept-proof
- Spec: §B.2/B.3 secondary actions row authoritative with entry-point rationale
- Spec: §B.5 Add-by-username accepts raw Identity ID
- Spec: §B.13 Pick-a-username with contested detection, fee preview, vote explanation
- Spec: Onboarding checklist steps enumerated with per-step visibility rules
- Spec: Identity pill dropdown ordering rule + inline search threshold in §A.3
- Spec: §G closed questions G6-G9 for deferred and decided items
- README: design-decisions section for shadow alphas, secondary actions, local nickname
- Catalog §D: entries #83-86 for secondary Home actions and topbar refresh
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Shorten wallet-pill copy from "Funded by Main Wallet" to "Main Wallet" - Collapse wallet + identity switcher into a single horizontal row - Add F3 Identity picker grid as default landing when ≥2 identities exist with per-identity cards (avatar/monogram, name, balance, type pill) and an "Add a new identity" card - Spec §A.4 default-landing rules (0/1/≥2 identities); §B.14 picker screen Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Fold wallet + identity switcher into the breadcrumb itself; placeholders "(no wallet yet)", "(no identity yet)", "(choose an identity)" when empty - Remove the separate switcher row under the breadcrumb from all frames - Move App chrome zoom to the last frame (F8) and rename to "App chrome reference" - Consolidate F3 Identity Home: one canonical frame covering both social-profile-set and no-social-profile states via annotation - Replace Contacts gated-state frame with a populated Contacts page showing received requests (2), active contacts (5), and sent requests (2) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Requirements, UX plan, test-case spec, and dev plan for the new Identities hub UI section (4-tab hub: Home/Contacts/Activity/Settings) derived from docs/ai-design/2026-04-22-identity-dashpay-redesign/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the foundation for the unified Identities hub UI section (docs/ai-design/2026-04-23-identity-hub-impl/). This commit only introduces the compile-time scaffold; subsequent commits wire up the left-nav entry, AppState registration, and per-tab content. Changes: - New Cargo features `identity-hub` (default on) and `identity-hub-activity-feed` (default off, gates the unified activity timeline backend aggregator that does not exist yet). - New `RootScreenType::RootScreenIdentityHub` variant with stable on-disk encoding `27` and round-trip tests. - New `ScreenType::IdentityHub` and `Screen::IdentityHubScreen` variants; all `ScreenLike` dispatch arms plus `change_context` extended. Macro `set_ctx!` receives the new variant via the `skip` list since the explicit match arm in `change_context` already handles it. - New `src/ui/identity/` module with a `ScreenLike` implementation that dispatches by loaded-identity count (onboarding/home/picker) and renders a placeholder tab bar plus per-tab stubs. The module is unconditionally compiled so the enum dispatch stays exhaustive; the `identity-hub` feature only controls nav visibility, which lands in a follow-up commit. - Eight unit tests covering tab ordering, labels, accessible descriptions, default variant, `HubLanding` state transitions, and `RootScreenType` round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completes the coexistence wiring for the new Identities hub. Users now see three identity-related entries in the sidebar: - `Dashpay` (legacy, feature-gated via existing FeatureGate) - `Identities` (legacy identities screen, unchanged) - `Identity Hub` (new, only when the `identity-hub` feature is on) Feature gating at the Cargo level — not a runtime FeatureGate — because the new hub doesn't share a predicate with the other entries. The entry is inserted in the button array immediately after the legacy `Identities` entry so the three identity-related items cluster together. AppState::new() inserts an `IdentityHubScreen` into `main_screens` via an iterator chain that is empty when the feature is disabled, so the screen map never contains an unreachable entry. No existing screen is modified. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces two reusable components for the Identities hub breadcrumb switcher (design-spec §A.3): - `BreadcrumbPill` — label + optional icon + chevron, three visual modes (Interactive / Subdued / Placeholder). Self-contained theming for light + dark mode. Builder methods for icon, tooltip, accessible name, and mode override. - `IdentityPill` — thin wrapper that resolves the identity label via the priority rule: Local nickname → DPNS username → shortened Identity ID (design-spec §G6). Label resolution is a pure function (`display_label`) so it is unit-testable without egui context. Both components follow `docs/COMPONENT_DESIGN_PATTERN.md`: private fields with builder methods, a `ComponentResponse`-implementing response struct, no direct egui state leakage. 16 unit tests added: mode toggling, label priority ordering (nickname/DPNS/id, empty, whitespace), raw-id shortening (head 5 + "…" + tail 3), and response round-trip. No existing code paths modified; the components are exposed via `src/ui/components/mod.rs` and can now be consumed by the hub (breadcrumb switcher composition in a follow-up). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three integration tests register the new hub with AppState and verify: 1. `identity_hub_mounts_and_renders` — AppState with the hub selected as the active root screen renders ten frames without panicking on the default empty database (onboarding path). 2. `legacy_nav_entries_coexist_with_hub` — verifies the enum contains all three coexisting variants and the on-disk encoding for the new hub variant round-trips through `from_int` / `to_int`. 3. `identity_hub_screen_type_creates_hub_screen` — guards against a refactor that drops the hub case from the `create_screen` dispatch. These are the minimum acceptance tests for the current scaffold. The per-tab assertions (IT-HOME-01, IT-CONTACTS-01, IT-ACTIVITY-01, IT-SETTINGS-01 from the test-case spec) arrive alongside each tab's content in follow-up commits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `src/ui/components/README.md`: new section documenting `BreadcrumbPill` and `IdentityPill` with their label priority rule and modes. - `docs/user-stories.md`: new IDH section with six stories covering first-time setup, identity home, multi-identity switching, optional social profile, dev-mode bulk creation, and the gated unified activity timeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five Major findings + several Minor docs fixes.
Majors:
- `src/app.rs` — build the `main_screens` BTreeMap first, then resolve
`selected_main_screen` by checking the built map. Falls back to
`RootScreenIdentities` if the persisted value is not registered.
This prevents `active_root_screen_mut()` from panicking when a user
previously selected the hub and the `identity-hub` feature is later
disabled.
- `src/ui/components/breadcrumb_pill.rs` — use `.inner` (the Label's
Response) instead of `.response` (the Frame's outer Response) to
capture clicks. In egui, `Frame::show(...).response` only senses
`Sense::hover` — child-widget click sensing is not inherited, so
the previous code silently broke click detection on all interactive
pills. The inner Label has `.sense(Sense::click())` applied; reading
from it makes the click fire as intended.
- `src/ui/components/identity_pill.rs` — refactor to the lazy-init
component pattern. Previously the struct eagerly stored a
`BreadcrumbPill`; now it stores the domain fields (local_nickname,
dpns_handle, identity_id_base58) plus builder-set options (tooltip,
accessible_name, mode) and constructs the inner `BreadcrumbPill`
inside `show()`. Returns a new `IdentityPillResponse` implementing
`ComponentResponse` instead of leaking `BreadcrumbPillResponse`.
Also: `display_label` now falls back to a stable `Unknown identity`
placeholder when given an empty id, so the pill is never invisible.
- `src/ui/identity/hub_screen.rs::landing()` — stops swallowing load
errors via `unwrap_or(0)`. On failure it surfaces a calm
`MessageBanner` ("Could not load your identities from this device.
Try refreshing or reopening the app.") with the error details
attached via `BannerHandle::with_details`, and reuses the last-known-
good landing so a real zero-identity account is still distinguishable
from a broken one.
- `src/ui/identity/hub_screen.rs::impl ScreenLike` — adds explicit
`refresh`, `refresh_on_arrival`, `display_message`,
`display_task_result`, and `display_task_error` implementations.
`refresh` clears any stale load-error banner so the next `landing()`
attempt can try again cleanly. The others are scaffold no-ops with
comments explaining why.
Minors:
- `tests/kittest/identity_hub.rs`: guard against the correct legacy
DashPay root variant (`RootScreenDashpay`), not the sub-screen
`RootScreenDashPayProfile`.
- `src/ui/identity/mod.rs`: feature-gate note now documents BOTH
integration sites (`left_panel.rs` nav entry + `app.rs main_screens`
registration) so future changes cannot accidentally produce
unreachable variants.
- `src/ui/components/breadcrumb_pill.rs`: `BreadcrumbPillResponse::new`
promoted to `pub(crate)` so the `IdentityPill` wrapper and tests can
fabricate responses without running egui.
- `docs/ai-design/2026-04-23-identity-hub-impl/`: corrected feature
names (`identity-hub` / `identity-hub-activity-feed`, not
underscored) across ux-plan, test-case-spec, and dev-plan. Fixed
dev-plan step 6 to point at `src/ui/mod.rs` (where `RootScreenType`
lives) rather than `src/database/settings.rs`.
- `docs/user-stories.md`: added Identities Hub TOC entry; downgraded
IDH-002..005 from `[Implemented]` to `[Gap]` since only the
scaffolds ship in this PR (follow-up work does the tab content).
Tests: 485 lib + 75 integration + 3 kittest passing. `cargo clippy
--all-features --all-targets -- -D warnings` clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce the four-tab horizontal bar for the Identities hub and wire the
onboarding empty state as the landing view when no identities are loaded.
The new `IdentityHubTabBar` component follows the project component
pattern (private fields, builder methods, `ComponentResponse`-based
response) and reuses existing theme tokens only — `DashColors::DASH_BLUE`
for the selected fill, `border_light` outline for unselected tabs, and
`Typography::SCALE_SM` / `Shape::RADIUS_MD` / `Spacing::SM` for layout.
Each tab surfaces `IdentityHubTab::accessible_description()` as its
clickable tooltip for screen-reader parity.
`IdentityHubScreen` now renders the new bar in place of the scaffold's
inline `selectable_label` preview. Selection still lives on the screen,
mirroring the existing controlled-component pattern.
Onboarding copy was already in place per T3 scaffolding; this change
keeps the strings verbatim from design-spec §B.1 and confirms the
developer-mode footer is gated on `AppContext::is_developer_mode()`.
Tests:
- UT-TABS-01 — unit test asserts the bar's selection contract through
its `ComponentResponse`, plus builder / default-state coverage.
- IT-ONBOARD-01 — new kittest under `tests/kittest/identity_hub_onboarding.rs`
mounts `AppState`, forces `RootScreenIdentityHub`, and asserts the
heading + both CTAs render while the developer-mode footer stays
hidden on the default persona.
Refs: docs/ai-design/2026-04-23-identity-hub-impl/04-dev-plan.md (T5),
docs/ai-design/2026-04-23-identity-hub-impl/03-test-case-spec.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements T7 from the identity-hub dev plan. - New IdentityPickerCard component: avatar/monogram, identity-type badge, heading (display_name -> DPNS -> shortened id), sub-line, tabular balance, wireframe "Opens Identity Home" hint. Full card is a single click target; response carries the identity id. - New IdentityPickerAddCard component: dashed border default, solid Dash-blue on hover, fixed design-spec strings, click reports add_requested. - Picker grid now renders a responsive flow of identity cards followed by the add card, auto-fitting columns based on available width (design-spec rule minmax(260px, 1fr)). Add card click routes to the existing AddNewIdentityScreen via AppAction::AddScreen - no new screen introduced. Unit tests: UT-PICKER-01/02/03 plus heading/sub-line edge cases, response round-trip, column-count monotonicity. 17 new tests, all passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T10 ships the Activity tab shell and its reusable row component: - `src/ui/components/activity_row.rs` — a 48 px compact row with `Payment`, `Funding`, and `PlatformOp` kinds and `Normal`, `Expanded`, and `Failed` statuses. `Failed` rows render a `Retry` small button and a danger-stroke border, matching design-spec §B.6. The component reports `ToggleExpand` or `Retry` actions via `ComponentResponse`. - `src/ui/identity/activity.rs` — filter-chip row (All / Payments / Funding / Platform) with `All` as the default reset, plus a gated empty state. When `identity-hub-activity-feed` is off (default) the tab points users to the legacy DashPay Payments screen; when on, it renders an aggregator placeholder — no new backend aggregator is introduced (additive-only rule). Tests: - UT-ACTIVITY-ROW-01 — covers Normal, Expanded, and Failed render paths with Retry-button presence assertion (plus 6 smaller unit tests over response semantics and builder composition). - IT-ACTIVITY-01 — kittest asserting the three required filter chips and the gated empty-state copy render on the default feature set. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add T9 of the identity-hub implementation plan: the Contacts tab renders either a centered social-profile gate card (when the active identity has no DashPay profile) or a three-section populated shell (received · active · sent) per design-spec §B.4 / §B.4.1. New flat components in `src/ui/components/`: - `social_profile_gate_card` — centered card with handle-aware body, `Add a display name` primary CTA, and a toggleable `Why?` panel. - `request_card` — received (amber strip + Accept/Decline) and sent (blue strip + Pending pill + Cancel request) variants. - `contact_row` — clickable list row with avatar monogram, display name, `@handle`, optional last-payment hint, and Send + overflow actions; response carries the contact id for click routing. The populated-state shell dispatches the existing `DashPayTask::LoadContacts` via `AppAction::BackendTask` — no new backend variants are added (explicitly scoped out for T9). Interactive accept / decline / cancel flows are deferred to T10 with inline TODO markers. Tests: - UT-GATE-01, UT-REQUEST-CARD-01, UT-CONTACT-ROW-01 (unit). - IT-CONTACTS-01 (kittest) — mounts `contacts::render_gated` directly and asserts the gate heading + primary button render while the populated section headings stay absent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements T8 from the identity-hub dev plan: - `IdentityHeroCard` — gradient hero (DASH_BLUE → PLATFORM_PURPLE at 14 %, RADIUS_XL, Shadow::elevated()) with two variants: social-profile-set (96 px avatar + display name + @handle) and no-social- profile (type-glyph monogram + optional `Pick a username` prompt). Renders an identity-type badge pill and optional network pill. Follows `docs/COMPONENT_DESIGN_PATTERN.md` with private fields, builder methods, and a response struct implementing `ComponentResponse`. - `OnboardingChecklist` — three-step strip (Pick a username · Set a display name · Add your first contact) with check-mark / empty-circle bullets and a dismiss button. Response carries activation / dismissal intent. - Identity Home tab (`src/ui/identity/home.rs`) — full wiring: hero card, Send / Receive / Add contact quick actions (Add contact gated behind a social profile per §B.3), Add funds / Send to wallet / Send to another identity secondary ghost actions, inline `Set up your social profile` card in the no-profile variant, the onboarding checklist (hidden once dismissed or complete), a recent-activity preview (empty-state for now; wired to flip to the Activity tab), and an Advanced details expander listing raw Identity ID, revision, and key count. - `IdentityHubScreen` owns a small `HomeState` (dismiss flag, skip-social flag, advanced toggle) so tab switches don't wipe per-tab UX state. Dismissal is ephemeral in memory — no DB schema change. Tests (UT-HERO-01..02, UT-CHECKLIST-01..02): 18 unit tests across the two new components + 7 tests in the home module cover the state machine and credit-to-DASH formatter. Kittest IT-HOME-01 mounts the hub, asserts the Home outcome API surface, and pins the four-tab label order. Strings are copied verbatim from design-spec §B.2 / §B.3 / §C / §D. All design choices documented in module-level comments. No backend_task changes — tab dispatches reuse existing TransferScreen / TopUpIdentity / WithdrawalScreen / RegisterDpnsName screens until the dedicated Send sheet (§B.7) lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements T11 of the identity-hub impl plan. Renders a two-column layout inside the central island: social profile (left) and username + aliases (right), with a full-width Advanced expander below that contains identity type + raw ID, keys summary, refresh action, and a danger-zone card. Backend integration is strictly additive — the save path dispatches the existing DashPayTask::UpdateProfile, the refresh path uses IdentityTask::RefreshIdentity, and the register-username CTA routes to the existing RegisterDpnsNameScreen. Controls without a matching backend task (Delete social profile, Add / Remove / Make-primary alias, Unload identity from this device) are rendered as non-interactive affordances with disabled_tooltips explaining that the action is coming, and marked with TODO(identity-hub) comments so the backend follow-up can search for them. Copy comes verbatim from the design spec (§B.8 and §D tooltip catalog). The hub screen now owns a SettingsTab and dispatches through its stateful render so edit drafts persist across frames. Tests: - 10 unit tests in src/ui/identity/settings.rs covering validation thresholds, dirty tracking, string helpers, and the identity-type badge. - One kittest in the same module asserts the three required section headings (Social profile / Username / Aliases / Advanced) render via a build_ui harness — this covers the IT-SETTINGS-01 label assertions without bootstrapping a full identity fixture. - IT-SETTINGS-01 in tests/kittest/identity_hub_settings.rs exercises the AppState-level mount path on the Settings tab and verifies the hub continues to render without panicking on a fresh database. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r tab entry The Contacts populated shell previously dispatched `DashPayTask::LoadContacts` on every paint — flooding the backend channel and hammering the SDK. Introduce `ContactsState` owned by the hub with a `load_requested` flag set on first dispatch; reset on tab switch or `refresh_on_arrival`. This keeps the dispatch additive (no new backend task variant) while making it safe to paint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cts gate deep link Introduce a feature-gated `AppAction::SwitchIdentityHubTab` variant that lets in-hub deep links hop between sub-tabs through the normal action-dispatch channel, rather than coupling sibling tabs to each other. `AppState::update` resolves the currently-visible screen and, when it is the Identity Hub, forwards the tab switch via `IdentityHubScreen::select_tab`. Wire the Contacts tab's social-profile gate card to emit `SwitchIdentityHubTab(Settings)` when the user clicks the primary CTA — that is where display name and avatar editing lives. The T8 Home-tab "See all activity" link already hops tabs synchronously via `HomeOutcome`, so no additional deep-link plumbing is needed there. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
T9 shipped the `RequestCard` component with accept/decline/cancel response flags, but the contacts tab currently renders only empty-state placeholder copy — there is no request data feeding the component yet. Document where the wiring belongs: `AcceptContactRequest` / `RejectContactRequest` backend variants exist; a `CancelContactRequest` variant does not and is explicitly deferred to a later wave per integration constraints. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…unit tests; fix QA-002 misleading docstring QA-001 (MED): add fixture-free `syncing_global_writes_selection_to_app_context` unit test inside `identity_selector::tests`. Calls `sync_to_global()` directly (private access in same module) with in-memory `QualifiedIdentity` structs and a bare `AppContext` (no Harness, no wallet-backend wiring needed — KV persistence gracefully skips). Proves the write-back path without private keys or DB insertion. QA-003 (LOW): add `with_app_default_inert_when_global_id_not_in_candidate_list` unit test. Verifies that when the app-scoped identity is absent from the selector's candidate list, `app_default_seed()` returns `None` — locks the wallet-membership guard that `CreateAssetLockScreen` relies on (R1). QA-002 (LOW): correct the misleading `contacts_list_defaults_to_app_scoped_identity` docstring in `dashpay_screen.rs`. It called itself a "write-back canary" but only tests seeding; updated to accurately describe seeding coverage and point to QA-001 for write-back coverage. QA-004 (LOW): update deferred TODO comments in `contract_screen.rs`, `tokens_screen.rs`, and `tools_screen.rs` to reference the new QA-001 unit test so readers know write-back is now covered at the component level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The app-scoped selected identity now drives every operate-as screen (W2-W5), completing the multi-identity switching story. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…test
Replace the unit-test-only QA-001 proof with a proper kittest that exercises
the full rendering path: a genuine ComboBox change → `sync_to_global()` →
`AppContext::set_selected_identity()`.
`tests/kittest/identity_selector.rs` — `combo_change_writes_selection_to_app_context`:
- Phase 1: initial render with buffer pre-seeded to Alice must NOT invoke
`set_selected_identity` (seeding ≠ write-back).
- Phase 2: `get_by_value("Alice").click()` opens the ComboBox popup,
`get_by_label("Bob").click()` selects Bob; asserts `ctx.selected_identity_id()
== Some(bob_id)` after `harness.run()`.
Setup pattern: `build_eframe` + `run_steps(5)` to fully wire `ensure_wallet_backend`
(and drain `restore_selected_identity_from_kv`) BEFORE seeding the identity, so the
async initialization race does not overwrite the seed.
Unit test `syncing_global_writes_selection_to_app_context` in `identity_selector.rs`
is kept and re-scoped to the *mechanism* (`sync_to_global()` method). The new kittest
covers the *rendering gate* (`combo_changed || text_response.changed()` at line 321).
Also updates `identity_selector.rs` docstring to cross-reference the kittest.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
…ts · T30 tooltip copy T28 (blocking): IdentityHubScreen.change_context now calls screen.refresh() after swapping app_context. Without this the contacts load-guard stayed set, so the Contacts tab would show stale "already loaded" data forever after a network switch. T29 (blocking): render_populated dispatches DashPayTask::LoadContactRequests alongside LoadContacts on first tab entry. Previously only LoadContacts was fired, so the Received and Sent sections could never hydrate from the backend. T30 (blocking): corrected Quick-action tooltip copy to match actual routes. The "Send" button routes to the identity Transfer screen (identity→identity credits, not wallet-Dash send), and "Receive" routes to TopUpIdentity (wallet→identity credits, not a QR-code/receive-address screen). Chose option (b) — update copy to match the current implementation — because option (a) requires adding a new QR- generator entry point that does not yet exist in the hub context. The "Send to another identity" secondary action already has an accurate tooltip; this aligns the primary row to the same standard. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…acement · T09 avatar image rendering V1: Remove `ui.set_min_height(200.0)` from IdentityHeroCard so the hero card sizes to its actual content. The fixed 200 px floor caused a large empty gradient slab in the no-social-profile variant and made the card unnecessarily tall even in the profile-set variant. V2: Move the "Set up your social profile" inline card to be immediately below the hero (before the quick-actions row) rather than buried after the secondary-actions row. Together with V1 this produces the compact hero+prompt visual the wireframe shows for the no-profile state — no empty gap, no hidden prompt. T09: IdentityHeroCard.paint_avatar_or_monogram now actually renders avatar bytes when with_avatar_bytes() is called. Previously the field was stored but ignored; the render always fell through to the initials monogram, making avatar_uses_initials_fallback() lie when bytes were present. Implementation: - Decode PNG/JPEG bytes via the `image` crate (already a dependency). - Cache the TextureHandle in the egui context keyed by a FNV-1a hash of the bytes so decode runs exactly once per unique avatar. - Paint via egui::Image::corner_radius (48 px = perfect circle clip). - Overlay the same accent ring used by the initials monogram. - Fall back to initials on decode failure, so avatar_uses_initials_fallback() stays honest. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ility
V3 / T10: Rework the onboarding checklist to match wireframe §B.2:
Heading: "Get set up" → "Finish setting up your identity"
Dismiss: bare "×" symbol → labelled "Hide this for now" link so the intent
is explicit (existing dismiss logic preserved, tooltip unchanged).
Per-item subtext: each step now renders a short descriptive line below the
title — e.g. "This is how you appear to contacts." for SetDisplayName —
which the wireframe shows for both pending and done states. For done
PickUsername the subtext reads "You are @{handle}." when the identity has a
DPNS name (injected via the new with_handle() builder), falling back to
"Your username is set." when the handle is not available yet.
Inline action buttons: pending steps render an underlined link-style action
button (e.g. "Set display name", "Add a contact") so the user can act from
the checklist without hunting for the entry point.
Full-row clickability (T10): the bullet circle and surrounding whitespace now
participate in the click sense via egui UiBuilder::sense — not just the
label text. The inline action button additionally emits its own click, and
both produce ChecklistAction::Activated so the hub routes correctly.
No existing tests broke; all 9 checklist unit tests pass.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hree response types ContactRowResponse (T08): implements ComponentResponse<DomainType = String> - has_changed() → any of clicked/send_clicked/overflow_clicked is true - changed_value() → &self.contact_id (the echoed identifier) - is_valid() / error_message() → trivial (no validation) RequestCardResponse (T11): introduces typed RequestAction enum (Accepted, Declined, Cancelled) as the ComponentResponse::DomainType alongside the existing public booleans. All existing call sites that read response.accepted / .declined / .cancelled compile unchanged. The typed action is available via response.action() and via ComponentResponse changed_value() (populated by show() into a private action_cache field so the borrow can return a &Option<RequestAction>). Added action_derives_from_ booleans unit test. SocialProfileGateCardResponse (T12): introduces typed GateCardAction enum (PrimaryClicked, WhyToggled) via the same private action_cache field pattern. Existing call sites (contacts.rs response.primary_clicked) are unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…aseline
T17 (picker.rs): `captured_selection` was unconditionally written to the
caller's `selected_id_out` slot, including on frames with no click
(`None`). This would silently clear any selection the caller had set.
Fix: only update the slot when `captured_selection` is `Some` — i.e. when
the user actually clicked a card this frame.
T21 (settings.rs + hub_screen.rs): `SettingsTab` was mirroring
`original_display_name/bio/avatar_url` at the moment the Save button was
clicked. This meant a failed `UpdateProfile` backend task left the baseline
wrong: the Save button immediately disabled itself even though no server
round-trip succeeded. Fix:
- Remove the premature mirror-on-click in settings.rs
- Add `SettingsTab::on_profile_saved()` which moves the mirror to the
moment of confirmed success
- Add `SettingsTab::selected_identity()` accessor for identity matching
- Wire `on_profile_saved()` in `IdentityHubScreen::display_task_result()`
on `BackendTaskSuccessResult::DashPayProfileUpdated`, guarded by identity
ID comparison to reject stale results from prior selections
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…edit fields Problem: on_profile_saved() was mirroring the current edit fields as the new baseline. If the user kept typing after clicking Save, the in-flight success would commit never-saved edits as if they had been saved (silent data loss). Reproduced as a failing test: left="Alicia Smith" right="Alicia". Fix: - Add `pending_save: Option<(String, String, String)>` to SettingsTab, set to a snapshot of (display_name, bio, avatar_url) at the moment Save is clicked. - on_profile_saved() now pops pending_save and commits THAT snapshot as the original_* baseline; if the user has kept typing, the edit fields are untouched so Save re-enables for the remaining edits. - pending_save is cleared on identity switch (ensure_selected) so a stale success from the old identity cannot corrupt the new identity's baseline. Tests: - IT-SETTINGS-02: submitted snapshot vs current edits distinction - IT-SETTINGS-03: pending_save cleared on identity switch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… dispatch The previous pass dispatched DashPayTask::LoadContactRequests alongside LoadContacts so the Received/Sent sections could hydrate. However: - BackendTaskSuccessResult::DashPayContactRequests is consumed ONLY by the old ui/dashpay/contact_requests.rs screen; the hub's display_task_result routes it nowhere. - The Received and Sent sections still render hardcoded empty-state labels. - Result: a real SDK round-trip fires on every Contacts tab entry with zero user-visible benefit. Revert the dispatch. Add a TODO(identity-hub/T29) comment listing the three wiring steps needed before re-adding it: (1) cache on ContactsState, (2) hub display_task_result handler, (3) real RequestCard rows. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ct, gate-card dedup QA-004 / T08 — ContactRowResponse ComponentResponse contract - contact_id was echo-set unconditionally at show() start, breaking the ComponentResponse invariant: changed_value() must be Some only when has_changed() is true. - Fix: initialise response with contact_id = None; set it alongside the flag only when a click is actually detected (body/Send/overflow). - Add UT-CONTACT-ROW-04: no-click default has contact_id = None. QA-003 / T09 — avatar_uses_initials_fallback() stays honest on decode failure - was: `has_social_profile() && avatar_bytes.is_none()` — returned false (claims a real image) even when bytes were present but undecodable. - Add avatar_decode_ok: bool field; with_avatar_bytes() probes the bytes via image::load_from_memory (probe only, GPU upload still lazy). avatar_uses_initials_fallback() now also returns true when decode fails. - Add UT-HERO-03: valid 1×1 PNG → decode_ok true, fallback false. - Add UT-HERO-04: corrupt bytes → decode_ok false, fallback true. - Add QA-007 resource note in try_paint_avatar_image doc comment. QA-006 — V2/V3: suppress social-profile gate card when checklist visible - With both V2 and V3 applied, a no-profile Home shows the inline gate card (SetUpSocialProfile → Settings) AND the checklist's "Set a display name" step — the same action twice on one screen. - Fix: add checklist_covers_profile guard so the gate card is only shown when dismissed_checklist is true (i.e. the checklist is hidden). When the checklist is visible it handles the profile-setup affordance alone. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…28 test Two residual LOWs from Marvin's second QA pass, both independent of T29. settings.rs — SettingsTab::clear_pending_save(): A failed UpdateProfile left pending_save dangling. A later DashPayProfileUpdated from any path (e.g. legacy ProfileScreen "Change photo") would commit the stale submitted snapshot as the new baseline. Adding clear_pending_save() and wiring it into hub_screen::display_task_error closes the window: on any task error, the stale snapshot is cleared so it can't corrupt a future success. hub_screen.rs — display_task_error: Calls settings_tab.clear_pending_save(). Clearing when pending_save is None is a no-op, so this is safe to call on every error regardless of which task failed. contacts.rs — t28_reset_clears_load_guard (test): Guards the T28 fix (change_context → refresh → contacts_state.reset() re-enables the load dispatch). Previously untested; now pinned to prevent silent regressions if reset() loses the load_requested = false line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ts (T29) Depends on the Identity Hub UI introduced in #842 (src/ui/identity/). ContactsState (contacts.rs): - ContactRequestEntry: { counterpart_id, request_id, relative_time } — a lightweight cache entry derived from a raw DashPayContactRequests document. counterpart_id: Base58 display label (sender for incoming, recipient for outgoing) until profile-name integration lands. relative_time: pre-formatted via format_relative_time from doc.created_at / updated_at. - incoming: Vec<ContactRequestEntry> — populated by record_requests(), cleared by reset() so a refresh or identity/network switch doesn't leave stale rows. - outgoing: Vec<ContactRequestEntry> — same lifecycle as incoming. - record_requests(incoming, outgoing): converts Vec<(Identifier, Document)>: incoming sender = doc.owner_id(); outgoing recipient = doc.properties()["toUserId"]; timestamp via format_relative_time. - reset() now clears incoming + outgoing alongside the load guard. render_populated (contacts.rs): - Snapshots state_guard.incoming / outgoing before closures to avoid re-borrow conflicts. - Received section: iterates entries, renders RequestCard::received per row with abbreviated counterpart_id; empty-state label when list is empty. - Sent section: same with RequestCard::sent. - Section headings carry " · N" count when N > 0. - Dispatches LoadContacts + LoadContactRequests together on first paint (guarded: fires once per tab-entry, reset by refresh/network-switch). - Deferred TODOs: Accept/Decline wiring (variants exist, button not wired); Cancel (DashPayTask::CancelContactRequest not yet present). hub_screen::display_task_result (hub_screen.rs): - Restructured body to use match &result; existing DashPayProfileUpdated arm preserved unchanged. - New DashPayContactRequests { incoming, outgoing } arm calls contacts_state.record_requests(...) to hydrate the caches. Helpers: - abbreviate_id(): first 8 chars + "…" for long Base58 IDs; identity for short IDs (≤ 10 chars). Tests: - t28_reset_clears_load_guard_and_caches: reset() clears guard + both caches. - abbreviate_id_shortens_long_ids: helper unit test. - section_headings_include_count_when_populated: heading format with count. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
Review findings addressed — commit
|
| Thread | Finding |
|---|---|
| T28 | src/ui/mod.rs — network switch now calls screen.refresh() on IdentityHubScreen, resetting Contacts state so it re-dispatches after a network switch |
| T29 | src/ui/identity/contacts.rs — populated path now dispatches both LoadContacts and LoadContactRequests; Received and Sent sections hydrate correctly (genuinely implemented, not deferred) |
| T30 | src/ui/identity/home.rs — Send/Receive tooltip copy now matches the actual routed flow |
7 Majors resolved (coderabbitai)
| Thread | Finding |
|---|---|
| T08 | src/ui/identity/contact_row.rs — ComponentResponse implemented for ContactRowResponse |
| T09 | src/ui/identity/identity_hero_card.rs — avatar-byte / with_avatar_bytes state consistency fixed; readiness flag added |
| T10 | src/ui/identity/onboarding_checklist.rs — click area extended to full row (circle, icon, and whitespace), not just label text |
| T11 | src/ui/identity/request_card.rs — ComponentResponse implemented for RequestCardResponse |
| T12 | src/ui/identity/social_profile_gate_card.rs — ComponentResponse implemented for SocialProfileGateCardResponse |
| T17 | src/ui/identity/picker.rs — selected_id_out only updated on an actual click, not overwritten on every no-click frame |
| T21 | src/ui/identity/settings.rs — original_* fields now mirrored in display_task_result on success, not pre-emptively on Save click |
4 "outdated" threads closed (confirmed fixed at HEAD)
| Thread | Finding |
|---|---|
| T16 | src/ui/identity/hub_screen.rs — HubView::Picker routes to picker::render(); tab actions propagate correctly |
| T22 | src/ui/identity/settings.rs — ensure_selected uses resolve_selected_identity(), not identities.first() |
| T26 | src/ui/identity/hub_screen.rs — HubLanding::Picker is no longer dead code |
| T27 | src/ui/identity/settings.rs — all three tabs use resolve_selected_identity() |
Deferred to follow-up
The remaining minor, nitpick, and suggestion threads are acknowledged and deferred:
- TOC ordering (T06) —
user-stories.mdsection placement - Search persistence (T13) —
ContactsState.search_querynot yet persisted - Byte-vs-char counts (T20) —
.len()vs.chars().count()in settings validation - Kittest welcome screen (T24, T25) —
mount_hub()helpers needshow_welcome_screen = false unreachable!()in contacts (T34) — should be awarn!()soft-fallbackaction |=vsaction =(T35) — potential silent action drop in settings render- Per-frame SQLite reload (T33, tracked TODO IDH-003) —
landing()double-loads identities - Error border assertion (T07), column-count formula (T18),
ui/app_contextparam order (T19), Platform chip assertion (T23), kittest fixture seeding (T31),is_deadrouting (T32)
🤖 Co-authored by Claudius the Magnificent AI Agent
…ity (W2–W5) (#868) * docs(identity): app-scoped selection screen-migration plan (W2-W5) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(contracts): obey app-scoped selected identity in register/update/document screens (W2) Batch B1 of the W2-W5 migration plan. Each screen seeds its initial identity from `selected_identity_id()` (fallback: first loaded) and adds `.syncing_global()` to its `IdentitySelector` so a user pick propagates back to the app-scoped selection. - `RegisterDataContractScreen::new()`: seed from selected_identity_id - `UpdateDataContractScreen::new()`: seed from selected_identity_id - `DocumentActionScreen::new()`: seed via resolve_selected_identity() when None - All three IdentitySelectors: `.syncing_global(self.app_context.clone())` Tests: 3 kittests in tests/kittest/contract_screen.rs asserting each screen defaults to the app-scoped identity on construction (seeding direction). Write-back direction deferred (TODO: private-key fixture, TI-1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(identity): default DPNS registration to the app-scoped identity (W2) Batch B2: `RegisterDpnsNameScreen::new()` seeds `selected_qualified_identity` from `selected_identity_id()` (fallback: first loaded) so the DPNS registration screen opens on the identity the user last operated as, not always the first DB row. The `IdentitySelector` in `render_identity_id_selection` now carries `.syncing_global()` so a user pick writes back to the app-scoped selection. Test: extended `tests/kittest/register_dpns_name_screen.rs` with `dpns_registration_defaults_to_app_scoped_identity`, which seeds two identities, sets the second as the global selection, constructs the screen, and asserts it opens on the second identity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(dashpay): sync DashPay screens with the app-scoped selected identity (W3) Batch B3: all 7 DashPay screens now seed `selected_identity` from the app-scoped selection on construction and write user picker changes back via `syncing_global`. Screens migrated (seed in `new()` + `refresh()`; `syncing_global` on selector): - `AddContactScreen::new()` + `new_with_identity_id()` — seeded; selector syncs - `ContactsList::new()` + `refresh()` — seed prefers scoped id over first - `ContactRequests::new()` + `refresh()` — seed prefers scoped id over first - `PaymentHistory::new()` + `refresh()` — seed prefers scoped id over first - `ProfileScreen::new()` + `refresh()` — seed prefers scoped id over first - `QRCodeGeneratorScreen::new()` — seeded; selector syncs - `QRScannerScreen::new()` — previously `None`; now seeds from scoped id `selected_identity` made `pub` on each struct for test verification (consistent with `RegisterDataContractScreen` / `DocumentActionScreen` precedent). Tests (9 assertions in `tests/kittest/dashpay_screen.rs`): each screen opens on the second of two seeded identities when it is set as the app-scoped selection. Write-back canary deferred (TI-1 / private-key fixture gap; TODO added). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(tokens): default the token creator to the app-scoped identity (W4) Batch B4: `TokensScreen::new()` seeds `selected_identity` from the app-scoped identity on construction (falling back to the first loaded identity). Changes: - `mod.rs`: after the struct literal, look up the preferred id in the already- built `identities` BTreeMap and populate `selected_identity` + `identity_id_string`. - `token_creator.rs` (simple mode): added `.syncing_global()` to the `IdentitySelector` so a user pick writes back to the app-scoped selection. - `token_creator.rs` (advanced mode): snapshot before/after `add_identity_key_chooser` and call `set_selected_identity` on change (helper uses raw ComboBox, not IdentitySelector, so manual write-back is needed). - `selected_identity` made `pub` on `TokensScreen` for test verification. Test: `tests/kittest/tokens_screen.rs` — `token_creator_defaults_to_app_scoped_identity` seeds two identities, sets the second as the global selection, constructs the screen, and asserts it opens on the second identity. Write-back requires a private-key fixture (TI-1 gap; TODO). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat(wallets,tools): seed wallet-scoped and tool screens from the app-scoped identity (W5) Batch B5: three READ-only screens now seed their identity selector from the app-scoped selection subject to their specific membership guards. No screen uses `syncing_global` (R1/R4 reasons documented inline). Changes: - `create_asset_lock_screen.rs`: Added `.with_app_default(&self.app_context)` to the "Identity to top up" selector. Guard: seeds only if the global id is in this wallet's identity list (IdentitySelector handles the check). No `new()` pre-fill change needed. - `grovestark_screen.rs`: Manual EdDSA-guarded seed in `new()` and `refresh_identities()`. The global identity is used iff it passes the EdDSA-key filter; otherwise falls back to first EdDSA identity or `None`. `selected_identity` made `pub` for test verification. - `send_screen.rs`: Manual wallet-membership-guarded seed at render-time: when the wallet-scoped identity list is built each frame, seeds `selected_identity` from the global id iff it is among this wallet's identities; otherwise `selected_identity` stays `None`. Tests: `tests/kittest/tools_screen.rs` — `grovestark_does_not_seed_non_eddsa_identity`: seeds two basic identities (no EdDSA keys), sets the second as the global selection, constructs the screen, and asserts `selected_identity == None` (R4 guard). Positive-seed and wallet-membership tests deferred (EdDSA-key fixture and WalletFixture gaps; TODOs in test file). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(identity): lock session-local and no-sync identity pickers (W5) Batch B6: regression-lock tests and the K3 comment on GroupActionsScreen. GroupActionsScreen: - Added one-line K3 comment to the IdentitySelector: session-local screen, no `with_app_default` or `syncing_global` by design. - `selected_identity` made `pub` for test verification. Tests: - `contract_screen::group_actions_does_not_seed_from_global_identity` (K3 lock): seeds two identities, sets the second as global, creates `GroupActionsScreen`, asserts `selected_identity == None` and the global selection is unchanged. This pins the session-local behaviour against future drift. - Updated `tokens_screen.rs` doc-comment to document the B6 N/A regression-lock reasoning: the 6 N/A token recipient/target/member selectors are covered by the `default_selector_has_no_sync_target` unit test in identity_selector.rs; a structural note captures this invariant here. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: apply nightly fmt across W2-W5 migration files Formatting-only commit: `cargo +nightly fmt --all` on all files touched during the B1-B6 migration batches. No functional changes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(identity-selector): add QA-001 write-back and QA-003 inert-lock unit tests; fix QA-002 misleading docstring QA-001 (MED): add fixture-free `syncing_global_writes_selection_to_app_context` unit test inside `identity_selector::tests`. Calls `sync_to_global()` directly (private access in same module) with in-memory `QualifiedIdentity` structs and a bare `AppContext` (no Harness, no wallet-backend wiring needed — KV persistence gracefully skips). Proves the write-back path without private keys or DB insertion. QA-003 (LOW): add `with_app_default_inert_when_global_id_not_in_candidate_list` unit test. Verifies that when the app-scoped identity is absent from the selector's candidate list, `app_default_seed()` returns `None` — locks the wallet-membership guard that `CreateAssetLockScreen` relies on (R1). QA-002 (LOW): correct the misleading `contacts_list_defaults_to_app_scoped_identity` docstring in `dashpay_screen.rs`. It called itself a "write-back canary" but only tests seeding; updated to accurately describe seeding coverage and point to QA-001 for write-back coverage. QA-004 (LOW): update deferred TODO comments in `contract_screen.rs`, `tokens_screen.rs`, and `tools_screen.rs` to reference the new QA-001 unit test so readers know write-back is now covered at the component level. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(user-stories): mark IDH-003 multi-identity switching implemented The app-scoped selected identity now drives every operate-as screen (W2-W5), completing the multi-identity switching story. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * test(identity-selector): add QA-001 egui-kittest write-back keystone test Replace the unit-test-only QA-001 proof with a proper kittest that exercises the full rendering path: a genuine ComboBox change → `sync_to_global()` → `AppContext::set_selected_identity()`. `tests/kittest/identity_selector.rs` — `combo_change_writes_selection_to_app_context`: - Phase 1: initial render with buffer pre-seeded to Alice must NOT invoke `set_selected_identity` (seeding ≠ write-back). - Phase 2: `get_by_value("Alice").click()` opens the ComboBox popup, `get_by_label("Bob").click()` selects Bob; asserts `ctx.selected_identity_id() == Some(bob_id)` after `harness.run()`. Setup pattern: `build_eframe` + `run_steps(5)` to fully wire `ensure_wallet_backend` (and drain `restore_selected_identity_from_kv`) BEFORE seeding the identity, so the async initialization race does not overwrite the seed. Unit test `syncing_global_writes_selection_to_app_context` in `identity_selector.rs` is kept and re-scoped to the *mechanism* (`sync_to_global()` method). The new kittest covers the *rendering gate* (`combo_changed || text_response.changed()` at line 321). Also updates `identity_selector.rs` docstring to cross-reference the kittest. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
lklimek
marked this pull request as ready for review
July 1, 2026 10:06
lklimek
merged commit Jul 1, 2026
637dc60
into
docs/platform-wallet-migration-design
6 checks passed
thepastaclaw
suggested changes
Jul 1, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The PR resolves all six blocking regressions from the prior review (dead picker, per-tab .first(), network-switch stale Contacts, missing LoadContactRequests, mismatched Send/Receive tooltips, and the wave-migration coverage). Two new blocking issues remain in the wallet/identity reconciliation seam: selecting an HD-owned identity leaves the previously persisted single-key wallet pointer in place (so the Wallets screen restores the stale single-key wallet), and selecting an empty HD wallet still resolves to another wallet's identity via the global first-identity fallback. Several lower-severity residuals from the prior review are still current (contacts unreachable!(), sender-identity wallet not refreshed in AddContact, ProfileCache stuck on load failure, action-accumulator overwrites).
🔴 2 blocking | 🟡 6 suggestion(s) | 💬 1 nitpick(s)
Findings not posted inline (1)
These findings could not be anchored to the current diff, but they are still part of this review.
- [SUGGESTION]
src/ui/dashpay/add_contact_screen.rs:307-340: Changing sender identity keeps the previous identity's wallet reference — The auto-select branch recomputesselected_keyfor the newly chosen identity but only refreshesselected_walletwhenself.selected_wallet.is_none()(line 328). Once the initial identity populates a wallet, selecting a different sender identity via the app-scoped selector at line 302 chang...
Carried-forward findings already raised (1)
These findings were not re-posted as new inline comments because an existing review thread already covers them.
- [NITPICK] (deduped existing open thread)
src/ui/identity/settings.rs:231-583: Settings sub-renderers still overwriteactioninstead of merging with|=—render_social_profile,render_username_and_aliases, andrender_advancedstill use plain assignment (action = AppAction::AddScreen(..),action = AppAction::BackendTask(..)) at lines 235, 316, 434, 571, and 583. The outer callers merge via|=, so no live bug — but a future edit that add...
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/context/mod.rs`:
- [BLOCKING] src/context/mod.rs:1089-1135: HD-wallet/identity selection leaves stale single-key wallet pointer, corrupting Wallets restore
`set_selected_identity()` (line 1107) and `set_selected_hd_wallet()` (line 1134) both re-persist the selected-wallet blob using `self.current_single_key_hash()` — the stale in-memory single-key hash is copied through untouched. `WalletsBalancesScreen::new()` (mod.rs:194–208) checks `selected_single_key_hash` first and returns as soon as it finds a matching single-key wallet.
Repro: user picks a single-key wallet (persists both `SelectedWallet.wallet_hash=None` and `single_key_hash=Some`), then goes to the hub and picks an HD-owned identity. `set_selected_identity` derives and writes the HD `selected_wallet_hash` but does not clear `selected_single_key_hash`. The persisted blob now claims both an HD wallet and a single-key wallet. Returning to the Wallets screen, the single-key branch fires first and reopens the old single-key wallet — the breadcrumb/hub is on HD wallet A, but the Wallets screen is on single-key wallet B. `WalletsBalancesScreen::select_hd_wallet()` (mod.rs:313–316) already encodes the invariant that these two pointers are mutually exclusive; the app-scoped setters must do the same. Clear `selected_single_key_hash` (and persist it as `None`) whenever `set_selected_identity` reconciles to an HD wallet or `set_selected_hd_wallet` writes `Some(hash)`.
- [BLOCKING] src/context/mod.rs:1058-1066: Empty selected wallet still resolves to a different wallet's identity via global fallback
`set_selected_hd_wallet(Some(empty_wallet))` correctly reconciles `selected_identity_id` to `None` (line 1131–1132). But `resolve_selected_identity()` at line 1060 loads *all* qualified identities across every wallet and hands them to `resolve_selected`, which falls back to `loaded.first().copied()`. So when wallet B is explicitly selected and empty while wallet A has exactly one identity, the hub sees `loaded_count == 1`, `has_explicit_active == false`, `effective_view()` returns `Home`, and Home operates as wallet A's identity while the breadcrumb wallet segment displays wallet B.
This breaks the PR-stated invariant that switching to an empty wallet forces the picker. The 'no identity chosen yet' vs 'the selected wallet has no identity' cases need to be distinguished — e.g., have `resolve_selected_identity()` (or the hub's view computation) scope the loaded-identities set to the selected wallet when a wallet is explicitly selected, so an empty selected wallet resolves to `None` and the picker is shown.
In `src/ui/dashpay/add_contact_screen.rs`:
- [SUGGESTION] src/ui/dashpay/add_contact_screen.rs:307-340: Changing sender identity keeps the previous identity's wallet reference
The auto-select branch recomputes `selected_key` for the newly chosen identity but only refreshes `selected_wallet` when `self.selected_wallet.is_none()` (line 328). Once the initial identity populates a wallet, selecting a *different* sender identity via the app-scoped selector at line 302 changes the key while leaving the old wallet and `wallet_open_attempted` in place. The unlock UI can then check or unlock wallet A while the backend task is built with identity/key B, breaking the identity → owning-wallet invariant this PR establishes.
Refresh `selected_wallet` whenever the identity changed:
- [SUGGESTION] src/ui/dashpay/add_contact_screen.rs:307-340: Changing sender identity keeps the previous identity's wallet reference
The auto-select branch recomputes `selected_key` for the newly chosen identity but only refreshes `selected_wallet` when `self.selected_wallet.is_none()` (line 328). Once the initial identity populates a wallet, selecting a *different* sender identity via the app-scoped selector at line 302 changes the key while leaving the old wallet and `wallet_open_attempted` in place. The unlock UI can then check or unlock wallet A while the backend task is built with identity/key B, breaking the identity → owning-wallet invariant this PR establishes.
Refresh `selected_wallet` whenever the identity changed:
In `src/ui/identity/profile_cache.rs`:
- [SUGGESTION] src/ui/identity/profile_cache.rs:71-104: `ProfileCache::in_flight` is never cleared on `LoadProfile` failure — every subsequent profile load is deadlocked
`dispatch_pending` early-returns while `in_flight.is_some()`. The slot is only cleared by `record_result` (on a `DashPayProfile` success arrival) or `reset()` (hub refresh / breadcrumb switch / picker click). There is no path for a *failed* `LoadProfile` task to clear `in_flight` — `hub_screen::display_task_error` (line 354–366) only clears `pending_save`. A single load failure (network hiccup, contract unavailable, SDK error) therefore permanently jams the cache for the rest of the session: `wanted` keeps growing while `dispatch_pending` refuses to fire; every miss silently returns `None` (Contacts gate stays visible, Home name resolution blank, Settings load never completes) until the user refreshes.
Add a `record_failure()` method on `ProfileCache` that clears `in_flight` and drops the failed id from `requested`, and invoke it from `hub_screen::display_task_error` when the failing task was a `DashPayTask::LoadProfile`.
In `src/ui/identity/contacts.rs`:
- [SUGGESTION] src/ui/identity/contacts.rs:296-301: `unreachable!()` in the Contacts gate CTA can panic the whole app on a dispatcher-table edit
`render_gated` ends its exhaustive match on `contacts_button_kind(GateSetUpProfile)` with `unreachable!("GateSetUpProfile should not map to OpenScreen")`. `contacts_button_kind` is a hand-maintained public dispatcher; any future edit that routes `GateSetUpProfile` through `OpenScreen` turns a normal UI click into an immediate process panic instead of a dead click. UI dispatch paths should degrade to `AppAction::None` (optionally with a `tracing::warn!`) so a dispatcher drift produces a harmless button, not a crashed application. Prior finding still open at head.
- [SUGGESTION] src/ui/identity/contacts.rs:171-188: Outgoing request rows render an all-zero identifier when `toUserId` is missing
`record_requests` extracts the outgoing counterpart with `doc.properties().get("toUserId").and_then(|v| v.to_identifier().ok()).unwrap_or_default()`. `Identifier::default()` is an all-zeros ID; when the document is malformed (or a future contract update renames the field), Sent-requests rows silently render 'Sent to 11111111…' and are keyed by an all-zero request. Cosmetic today because the row is display-only, but once the follow-up wires 'Cancel' onto those rows the user can fire a cancel against `Identifier::default()`. Filter these entries out at the collector so bad backend rows do not reach the UI (`filter_map` on the outgoing collection, dropping any row where `toUserId` fails to parse).
- [SUGGESTION] src/ui/identity/contacts.rs:162-187: `ts = 0` fallback is passed through `format_relative_time` as a real epoch
`let ts = doc.created_at().or_else(|| doc.updated_at()).unwrap_or(0);` then `crate::ui::dashpay::format_relative_time(ts)`. When both timestamps are absent (older documents predating the field, or malformed docs), `ts = 0` is formatted as a valid timestamp (1970-01-01 → '55 years ago' or similar). Treat missing timestamps as `None`: `let ts = doc.created_at().or_else(|| doc.updated_at()); let relative_time = ts.map(crate::ui::dashpay::format_relative_time).unwrap_or_default();` and let the row render an empty relative-time slot.
Comment on lines
+1089
to
+1135
| pub fn set_selected_identity(&self, id: Option<Identifier>) { | ||
| if let Ok(mut g) = self.selected_identity_id.lock() { | ||
| *g = id; | ||
| } | ||
| // Reconcile the derived wallet to the identity's owner. A wallet-less | ||
| // (imported-by-id) identity has NO owning wallet, so the pointer must | ||
| // be cleared to `None` — otherwise the breadcrumb wallet pill keeps | ||
| // showing the previous identity's wallet while Home/operate-as use the | ||
| // wallet-less one (the pill and the active identity disagree). Selecting | ||
| // an identity always writes the owner (`None` included); only clearing | ||
| // the identity (`id == None`) leaves the wallet pointer untouched. | ||
| if let Some(id) = id { | ||
| let owner = self.owning_wallet_hash(id); | ||
| if let Ok(mut g) = self.selected_wallet_hash.lock() { | ||
| *g = owner; | ||
| } | ||
| } | ||
| self.persist_selected_identity_kv(id); | ||
| self.persist_selected_wallet_kv( | ||
| self.selected_wallet_hash(), | ||
| self.current_single_key_hash(), | ||
| ); | ||
| } | ||
|
|
||
| /// Set the selected HD wallet and reconcile the active identity to that | ||
| /// wallet's identities (keep-if-owned, else its first identity, else | ||
| /// `None` → picker). Writes both mutexes directly and persists both blobs | ||
| /// once; never calls a sibling setter (R5). | ||
| pub fn set_selected_hd_wallet(&self, hash: Option<WalletSeedHash>) { | ||
| if let Ok(mut g) = self.selected_wallet_hash.lock() { | ||
| *g = hash; | ||
| } | ||
| let reconciled = match hash { | ||
| Some(h) => { | ||
| let ids: Vec<Identifier> = self | ||
| .load_local_qualified_identities_for_wallet(&h) | ||
| .map(|v| v.iter().map(|qi| qi.identity.id()).collect()) | ||
| .unwrap_or_default(); | ||
| crate::model::selected_identity::resolve_selected(self.selected_identity_id(), &ids) | ||
| } | ||
| None => None, | ||
| }; | ||
| if let Ok(mut g) = self.selected_identity_id.lock() { | ||
| *g = reconciled; | ||
| } | ||
| self.persist_selected_wallet_kv(hash, self.current_single_key_hash()); | ||
| self.persist_selected_identity_kv(reconciled); |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: HD-wallet/identity selection leaves stale single-key wallet pointer, corrupting Wallets restore
set_selected_identity() (line 1107) and set_selected_hd_wallet() (line 1134) both re-persist the selected-wallet blob using self.current_single_key_hash() — the stale in-memory single-key hash is copied through untouched. WalletsBalancesScreen::new() (mod.rs:194–208) checks selected_single_key_hash first and returns as soon as it finds a matching single-key wallet.
Repro: user picks a single-key wallet (persists both SelectedWallet.wallet_hash=None and single_key_hash=Some), then goes to the hub and picks an HD-owned identity. set_selected_identity derives and writes the HD selected_wallet_hash but does not clear selected_single_key_hash. The persisted blob now claims both an HD wallet and a single-key wallet. Returning to the Wallets screen, the single-key branch fires first and reopens the old single-key wallet — the breadcrumb/hub is on HD wallet A, but the Wallets screen is on single-key wallet B. WalletsBalancesScreen::select_hd_wallet() (mod.rs:313–316) already encodes the invariant that these two pointers are mutually exclusive; the app-scoped setters must do the same. Clear selected_single_key_hash (and persist it as None) whenever set_selected_identity reconciles to an HD wallet or set_selected_hd_wallet writes Some(hash).
source: ['codex']
Comment on lines
+1058
to
+1066
| /// Resolve the active identity every operate-as read uses: the selected | ||
| /// identity when still loaded, else the first loaded identity, else `None`. | ||
| pub fn resolve_selected_identity(&self) -> Option<QualifiedIdentity> { | ||
| let identities = self.load_local_qualified_identities().ok()?; | ||
| let ids: Vec<Identifier> = identities.iter().map(|qi| qi.identity.id()).collect(); | ||
| let chosen = | ||
| crate::model::selected_identity::resolve_selected(self.selected_identity_id(), &ids)?; | ||
| identities.into_iter().find(|qi| qi.identity.id() == chosen) | ||
| } |
Collaborator
There was a problem hiding this comment.
🔴 Blocking: Empty selected wallet still resolves to a different wallet's identity via global fallback
set_selected_hd_wallet(Some(empty_wallet)) correctly reconciles selected_identity_id to None (line 1131–1132). But resolve_selected_identity() at line 1060 loads all qualified identities across every wallet and hands them to resolve_selected, which falls back to loaded.first().copied(). So when wallet B is explicitly selected and empty while wallet A has exactly one identity, the hub sees loaded_count == 1, has_explicit_active == false, effective_view() returns Home, and Home operates as wallet A's identity while the breadcrumb wallet segment displays wallet B.
This breaks the PR-stated invariant that switching to an empty wallet forces the picker. The 'no identity chosen yet' vs 'the selected wallet has no identity' cases need to be distinguished — e.g., have resolve_selected_identity() (or the hub's view computation) scope the loaded-identities set to the selected wallet when a wallet is explicitly selected, so an empty selected wallet resolves to None and the picker is shown.
source: ['codex']
Comment on lines
+71
to
+104
| pub fn dispatch_pending(&mut self) -> AppAction { | ||
| if self.in_flight.is_some() { | ||
| return AppAction::None; | ||
| } | ||
| let Some(identity) = self.wanted.pop() else { | ||
| return AppAction::None; | ||
| }; | ||
| let id = identity.identity.id(); | ||
| self.requested.insert(id); | ||
| self.in_flight = Some(id); | ||
| AppAction::BackendTask(BackendTask::DashPayTask(Box::new( | ||
| DashPayTask::LoadProfile { identity }, | ||
| ))) | ||
| } | ||
|
|
||
| /// Record a `LoadProfile` result against the in-flight identity. Returns | ||
| /// `true` when the result was consumed (a load was in flight). | ||
| pub fn record_result(&mut self, result: &BackendTaskSuccessResult) -> bool { | ||
| let BackendTaskSuccessResult::DashPayProfile(data) = result else { | ||
| return false; | ||
| }; | ||
| let Some(id) = self.in_flight.take() else { | ||
| return false; | ||
| }; | ||
| let fields = data | ||
| .clone() | ||
| .map(|(display_name, bio, avatar_url)| ProfileFields { | ||
| display_name, | ||
| bio, | ||
| avatar_url, | ||
| }); | ||
| self.loaded.insert(id, fields); | ||
| true | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: ProfileCache::in_flight is never cleared on LoadProfile failure — every subsequent profile load is deadlocked
dispatch_pending early-returns while in_flight.is_some(). The slot is only cleared by record_result (on a DashPayProfile success arrival) or reset() (hub refresh / breadcrumb switch / picker click). There is no path for a failed LoadProfile task to clear in_flight — hub_screen::display_task_error (line 354–366) only clears pending_save. A single load failure (network hiccup, contract unavailable, SDK error) therefore permanently jams the cache for the rest of the session: wanted keeps growing while dispatch_pending refuses to fire; every miss silently returns None (Contacts gate stays visible, Home name resolution blank, Settings load never completes) until the user refreshes.
Add a record_failure() method on ProfileCache that clears in_flight and drops the failed id from requested, and invoke it from hub_screen::display_task_error when the failing task was a DashPayTask::LoadProfile.
source: ['claude']
Comment on lines
+296
to
+301
| ContactsButtonKind::OpenScreen(_) => { | ||
| // Not possible today (dispatcher returns SwitchHubTab), but | ||
| // exhaustive match future-proofs the gate CTA. | ||
| unreachable!("GateSetUpProfile should not map to OpenScreen"); | ||
| } | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: unreachable!() in the Contacts gate CTA can panic the whole app on a dispatcher-table edit
render_gated ends its exhaustive match on contacts_button_kind(GateSetUpProfile) with unreachable!("GateSetUpProfile should not map to OpenScreen"). contacts_button_kind is a hand-maintained public dispatcher; any future edit that routes GateSetUpProfile through OpenScreen turns a normal UI click into an immediate process panic instead of a dead click. UI dispatch paths should degrade to AppAction::None (optionally with a tracing::warn!) so a dispatcher drift produces a harmless button, not a crashed application. Prior finding still open at head.
Suggested change
| ContactsButtonKind::OpenScreen(_) => { | |
| // Not possible today (dispatcher returns SwitchHubTab), but | |
| // exhaustive match future-proofs the gate CTA. | |
| unreachable!("GateSetUpProfile should not map to OpenScreen"); | |
| } | |
| } | |
| ContactsButtonKind::OpenScreen(_) => { | |
| tracing::warn!("GateSetUpProfile unexpectedly mapped to OpenScreen; ignoring click"); | |
| return AppAction::None; | |
| } |
source: ['claude', 'codex']
Comment on lines
+171
to
+188
| self.outgoing = outgoing | ||
| .into_iter() | ||
| .map(|(req_id, doc)| { | ||
| let to_id = doc | ||
| .properties() | ||
| .get("toUserId") | ||
| .and_then(|v| v.to_identifier().ok()) | ||
| .unwrap_or_default() | ||
| .to_string(Encoding::Base58); | ||
| let ts = doc.created_at().or_else(|| doc.updated_at()).unwrap_or(0); | ||
| ContactRequestEntry { | ||
| counterpart_id: to_id, | ||
| request_id: req_id.to_string(Encoding::Base58), | ||
| relative_time: crate::ui::dashpay::format_relative_time(ts), | ||
| } | ||
| }) | ||
| .collect(); | ||
| } |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: Outgoing request rows render an all-zero identifier when toUserId is missing
record_requests extracts the outgoing counterpart with doc.properties().get("toUserId").and_then(|v| v.to_identifier().ok()).unwrap_or_default(). Identifier::default() is an all-zeros ID; when the document is malformed (or a future contract update renames the field), Sent-requests rows silently render 'Sent to 11111111…' and are keyed by an all-zero request. Cosmetic today because the row is display-only, but once the follow-up wires 'Cancel' onto those rows the user can fire a cancel against Identifier::default(). Filter these entries out at the collector so bad backend rows do not reach the UI (filter_map on the outgoing collection, dropping any row where toUserId fails to parse).
source: ['claude']
Comment on lines
+162
to
+187
| let ts = doc.created_at().or_else(|| doc.updated_at()).unwrap_or(0); | ||
| ContactRequestEntry { | ||
| counterpart_id: doc.owner_id().to_string(Encoding::Base58), | ||
| request_id: req_id.to_string(Encoding::Base58), | ||
| relative_time: crate::ui::dashpay::format_relative_time(ts), | ||
| } | ||
| }) | ||
| .collect(); | ||
|
|
||
| self.outgoing = outgoing | ||
| .into_iter() | ||
| .map(|(req_id, doc)| { | ||
| let to_id = doc | ||
| .properties() | ||
| .get("toUserId") | ||
| .and_then(|v| v.to_identifier().ok()) | ||
| .unwrap_or_default() | ||
| .to_string(Encoding::Base58); | ||
| let ts = doc.created_at().or_else(|| doc.updated_at()).unwrap_or(0); | ||
| ContactRequestEntry { | ||
| counterpart_id: to_id, | ||
| request_id: req_id.to_string(Encoding::Base58), | ||
| relative_time: crate::ui::dashpay::format_relative_time(ts), | ||
| } | ||
| }) | ||
| .collect(); |
Collaborator
There was a problem hiding this comment.
🟡 Suggestion: ts = 0 fallback is passed through format_relative_time as a real epoch
let ts = doc.created_at().or_else(|| doc.updated_at()).unwrap_or(0); then crate::ui::dashpay::format_relative_time(ts). When both timestamps are absent (older documents predating the field, or malformed docs), ts = 0 is formatted as a valid timestamp (1970-01-01 → '55 years ago' or similar). Treat missing timestamps as None: let ts = doc.created_at().or_else(|| doc.updated_at()); let relative_time = ts.map(crate::ui::dashpay::format_relative_time).unwrap_or_default(); and let the row render an empty relative-time slot.
source: ['claude']
lklimek
added a commit
that referenced
this pull request
Jul 16, 2026
* refactor(ui): trim dev-history comments and dedupe identity home helpers
Rewrite sprint/wave/task-numbering comments in home.rs and hub_screen.rs as
present-tense invariants (git blame covers history); PR #842 kept as the one
citable regression reference. Move the duplicated network_label helper from
home.rs/import_single_key.rs into ui/theme.rs next to network_label_color.
Rename home.rs's format_credits_as_dash to format_credits_short to stop
colliding in name (but not behavior) with model::fee_estimation's version.
Drop the #[cfg(test)]-only forwarding shim and the dead-code import-pinning
stub in home.rs.
* refactor(wallet_backend): delete dead one-impl PersistedWalletLoader seam
The `loader: Arc<dyn PersistedWalletLoader>` field injected the backend
into itself through a trait object: the sole production impl,
`UpstreamFromPersisted`, was a unit struct whose whole body was
`backend.load_from_persistor_seedless(ctx).await`. No test substituted a
loader double — the cold-boot tests drive `load_from_persistor_seedless`
directly — so the seam bought nothing.
Remove the `PersistedWalletLoader` trait, the `UpstreamFromPersisted`
unit struct, the `loader` field and its constructor parameter, and the
two object-safety/`Default` compile-check tests. `register_persisted_wallets`
now calls `self.load_from_persistor_seedless(ctx)` directly. The
DET-opaque `LoadedWallets` / `PersistedLoadSkip` outcome types stay in
`loader.rs`. Stale doc/comment references to the removed type point at
`load_from_persistor_seedless` instead. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(wallet_backend): dedupe InMemoryKv test fake across 14 files
Consolidate 13 byte-for-byte (or near-identical) copies of the
in-memory KvStore test fake into one canonical `kv_test_support`
module, following the existing `leak_test_support` shared-fixture
pattern. The shared fixture's `list_keys` always sorts — matching
kv.rs's own doc comment (which the duplicated unsorted copies
silently violated) and the 5 copies that already sorted explicitly;
no caller depended on unsorted (insertion) order.
`single_key.rs`'s fixture is a structurally different fake (global-only
BTreeMap<String, Vec<u8>> with scope assertions) and is left as-is.
`FailingKv` in kv.rs is not duplicated elsewhere and is left in place.
Registering the shared module requires one additive line in
wallet_backend/mod.rs (mirroring the leak_test_support declaration) so
context/ and backend_task/migration/ can reach it too.
* refactor(wallet_backend): split god-impl into shielded/identity_ops/payments
The single `impl WalletBackend` block in `mod.rs` spanned ~3,100 lines
across many domains. Following the pattern `hydration.rs` / `dashpay.rs`
already established (each an additional `impl WalletBackend` block in its
own sibling file), relocate three domain groups verbatim — no visibility,
ownership, or behavior change:
- `shielded.rs` — the 14 Orchard shielded-pool methods (already fenced).
- `identity_ops.rs` — register/top-up identity, ensure-managed, and the
platform-address funding methods (incl. the private
`provision_identity_funding_account` helper).
- `payments.rs` — send_payment / create_asset_lock_proof /
broadcast_transaction / assert_can_sign and the
`derive_private_key_from_held` helper (the funds-signing path).
Domain-exclusive private helpers stay in `mod.rs` (`map_shielded_op_error`,
`map_identity_*`, `map_platform_address_fund_error`, `DEFAULT_BIP44_ACCOUNT`)
and are reached from the sibling modules via `use super::` — child modules
see ancestor privates, exactly as `dashpay.rs` reaches `self.inner.*`.
Shared methods (`hd_scope`, `resolve_wallet`) remain in `mod.rs`.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(wallets): remove redundant balance-consistency health check
The end-of-SPV-sync health check compared each wallet's authoritative
Core/Platform totals against a second, independently derived per-category
total and warned the user when they disagreed. A runtime detector that
tells the user the app's own bookkeeping disagrees with itself is a
symptom, not a fix — the follow-up commit makes the per-account breakdown
single-sourced, so the two figures can no longer diverge by construction.
Removes:
- AppState::run_wallet_balance_health_check + the dedupe signature/banner
fields and BALANCE_HEALTH_WARNING copy
- collect_wallet_balance_mismatches / balance_health_signature and their tests
- BackendTaskSuccessResult::WalletBalanceHealthCheckRequested and the
EventBridge SyncComplete emitter that produced it
- the model::wallet::balance_consistency module (no other callers)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(wallets): single-source the per-account balance breakdown
The wallets screen derived its per-account breakdown a second time from
the legacy in-memory `Wallet` model (watched_addresses + per-address
platform_address_info), in parallel with the backend's authoritative
totals. The two derivations had diverged in shipped history, which is
what motivated the (now removed) health check.
Source the breakdown entirely from the backend:
- collect_account_summaries now takes only the display snapshot's
address_balances + address_paths (no `&Wallet`) and computes the Core
per-category totals from that single dataset. Platform credits leave
the breakdown entirely: the Platform tab reads the one authoritative
figure, AppContext::platform_balance_duffs — the exact value the wallet
header shows — so the tab and header can no longer disagree.
- The Platform tab is shown from the coordinator snapshot signal
(a positive balance or a completed platform-address sync), preserving
the empty/receive tab without consulting the legacy model.
Behavior is preserved for the numbers users see; the change is the data
source. The legacy `Wallet` model's balance/UTXO fields are no longer read
by the breakdown (see report for its remaining uses).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(context): correct platform_sync_info doc to "sync completed", not "funds found"
The cursor advances on every successful platform-address sync pass regardless
of whether it found funded addresses (see event_bridge summary_ok_sync_cursors),
so `Some` means "a sync pass completed," not "a funded address was reported."
The old wording misleads anyone reasoning about the Platform-tab visibility gate
that reads this accessor (QA-005).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallets): read header total and per-account breakdown from one snapshot generation
QA-001: the snapshot's headline balance and its UTXO-derived per-address
breakdown were two independently-refreshed fields. `recompute` always read the
lock-free `wallet.balance()` atomics for `.balance`, but only refreshed
`.address_balances` when `try_state()` won the lock — on contention it carried a
stale breakdown forward beside a fresh total. The wallet-header total
(`core_balance_duffs` -> `.balance.total`) and the Core tab sum
(`collect_account_summaries` over `.address_balances`) could therefore disagree
for the duration of any lock-contention window. The atomics are also maintained
by a separate event handler that can lag the wallet state under its own map
contention, so even off-contention the two sources could skew.
Fix: derive the headline balance from `state.balance()` (the stored
`WalletCoreBalance`, refreshed alongside every UTXO mutation under the same
wallet-manager write lock) read from the SAME `try_state()` guard as
`state.utxos()`. Balance and breakdown now reflect one generation of wallet
state. On contention, carry the ENTIRE prior (already-consistent) snapshot
forward — balance included — instead of splicing a fresh total onto a stale
breakdown. The non-blocking property is preserved: `try_state()` still yields
`None` under contention and never blocks the event callback.
QA-003: replace the tautological pinned test. The old
`two_funded_bip44_accounts_keep_distinct_per_account_totals` summed hand-fed
literals back to themselves and asserted zero Platform facts. Add
`header_total_reconciles_with_core_tab_breakdown_through_real_accessors`, which
publishes a realistic snapshot through the real `publish` seam — including a
funded address outside the generated-path window — and asserts the Core-tab sum
from `collect_account_summaries` equals the exact `.balance.total` the header
renders, proving no funded address is dropped. The old test's misleading
header-agreement comment is corrected to describe what it actually pins.
Adds `contention_carries_whole_prior_snapshot_...` and a store round-trip test
covering the carry-forward invariant.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(wallets): reconcile default-mode tabs and show Platform tab on load
Extracts the tab-visibility logic into a pure, unit-testable `plan_account_tabs`
so these rules have real coverage (the existing kittest suite is generic
frame-stability smoke with no wallet loaded).
QA-004: the Platform tab was gated on `platform_balance_duffs > 0 ||
platform_sync_info(..).is_some()`, so a freshly created/imported wallet showed
no Platform tab — not even empty, no receive address — until the first async
platform-address sync pass completed. If sync was slow, failed, or the network
was unreachable, the user's only in-app route to their Platform receive address
stayed hidden indefinitely. Every HD wallet unconditionally bootstraps a
platform-payment address at load (`Wallet::bootstrap_known_addresses`), so the
tab is now shown immediately (empty until funded).
QA-002: any balance in a non-visible ("system") category — `Other(Unknown)`,
CoinJoin, etc. — was only reachable via the developer-mode System tab. A
default-mode user saw a header total that included those funds with no visible
tab summing to them, exactly the drift class the deleted health check warned
about. Default mode now surfaces a consolidated "Other" tab whenever a
non-visible category holds funds, so the visible tabs always reconcile the
header total. The System-tab content gains a short explanation and lists only
funded categories in default mode.
QA-006: add a de-dup guard so the dedicated Platform push never produces a
second Platform tab if a future upstream bump folds the platform-payment pool
into `all_accounts()`. Pinned by `plan_account_tabs` tests plus an upstream
tripwire test (in snapshot.rs) asserting today's exclusion still holds.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs(wallets): note the accepted first-contended-recompute zero-snapshot edge case
When the very first recompute for a wallet loses the try_state() race there is no
prior snapshot to carry forward, so the all-zero default is published and the
wallet is marked as having a snapshot. A wallet with genuine prior funds can then
render "0 DASH, synced" for one event cycle before the next event wins the lock
and publishes the real balance. Documented as a known, accepted, self-healing
edge case.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(identity): show spendable balance, not total, on funding screens
show_wallet_balance() on both the Create-Identity and Top-Up
"use wallet balance" screens read DetWalletBalance.total, while the
insufficient-funds banner, gate, and picker on the same screens all
read .spendable(). With immature or CoinJoin-locked funds present,
the user saw a positive "Wallet Balance" directly contradicted by a
"not enough Dash" banner one line below. Switch both to .spendable()
so all four surfaces agree.
Raised on PR #869 review (thepastaclaw); verified still live post-merge.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* docs(wallets): document deferred non-remember-unlock registration gap
Full-repo audit CODE-005 (untriaged) found that handle_wallet_unlocked's
None-passphrase early return skips drive_unlock_registration, so a
non-remember unlock never re-registers the wallet with the upstream SPV
backend until next launch. User decision 2026-07-08: defer rather than
fix now, to keep the adjacent CODE-024 cleanup (Wave 11) unconstrained.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallets): tidy wallet-backend caches and trim dead surface
Wave 9 of the wallet-backend architecture audit — caches & nits.
- Avatar cache eviction now orders entries by a sibling `det:avatar_ts:`
timestamp index (small i64 reads) instead of deserializing every cached
image's full bytes on each put. put/invalidate/clear/evict keep the index
in lock-step with the byte entries.
- Trim test-only / over-public wallet-backend surface: gate `wallet_count`
and `AuthPubkeyCacheView::delete` behind `#[cfg(test)]`; make the
`AVATAR_TTL_MS`, `MAX_AVATAR_ENTRIES`, and `DASHPAY_REQUEST_EXPIRY_DAYS`
constants private; delete the unused `shielded_activity`/`shielded_notes`
stubs; correct the stale `ensure_wallets_registered` migration-engine doc.
- Name the Devnet/Regtest SPV P2P ports and collapse `spv_primary_peer_socket`
into one match with an early `_ => None`.
- Unify the two SPV error-snippet truncations under one
`SPV_ERROR_SNIPPET_MAX` constant and drop the shadowing `use` lines in
`on_platform_address_sync_completed`.
- Drop the dead `TokenBalanceSnapshot` re-export.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): consolidate identity_db kv access, errors, and vote helpers
Wave 6 of the architecture audit — context/identity_db.rs plus the shared
kv-access surface it touches.
- CODE-001: replace the `format!("{:?}", identity_type)` Debug-repr
discriminator (four writers) and the hardcoded string filters with a
stable `IdentityType::as_tag()` / `from_tag()` mapping. Writer and filter
now share one source of truth, immune to a variant rename. Adds a
round-trip test.
- CODE-007: extract `hydrate_stored_identity()`, shared by the bulk-load
and single-get paths so both reconstruct an identity identically.
- CODE-014: introduce per-domain `err`-style free fns (`identity_err`,
`scheduled_vote_err`, `top_up_err`, `contract_err`, `token_err`,
`contest_err`) replacing the verbose `.map_err(|source| …)` closures and
the fully-qualified error paths; collapse the three `*_kv()` accessors
into one `AppContext::det_kv()`.
- CODE-035: add `scheduled_vote_keys()` and `remove_vote_voter_from_index()`
helpers; the three hand-rolled list/delete/prune loops shrink to a few
lines.
- CODE-041: delete the dead `load_local_qualified_identities_in_wallets`
and its speculative `#[allow(dead_code)]`.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(ui): delete dead UI surface and dedup icon loaders
Wave 20 dead-UI-surface cleanup:
- Delete the unused `left_wallet_panel.rs` (no opener anywhere) and its
module declaration.
- Consolidate the three copied `Assets`/`load_icon`/`load_svg_icon`
RustEmbed icon loaders into one shared `components/icons.rs`; drop the
dead copy in `top_panel.rs` and the duplicate in `left_panel.rs`.
- Remove dead styled widgets `GlassCard`, `HeroSection`, `AnimatedIcon`,
`AnimatedGradientCard` and `styled_text_edit_multiline`, plus their
catalog rows in `components/README.md`.
- Collapse `StyledButton` to its single reachable configuration (drop the
never-constructed `ButtonVariant`/`ButtonSize` enums and arms); remove
the commented-out `StyledCard` builders and dead `title` field/branch;
remove the inert `GradientButton::glow` field/builder and its no-op
call site in `left_panel.rs`.
- Drop the six blanket `#[allow(dead_code)]` blocks in `theme.rs`. This is
a library crate, so `dead_code` never fires on `pub` items; clippy
`--all-features --all-targets -D warnings` is clean without them, so no
per-item `#[expect(dead_code)]` is warranted (it would be unfulfilled).
- Delete five zero-caller `helpers.rs` items (`BUTTON_ADJUSTMENT_PADDING_TOP`,
`render_key_selector`, `is_platform_address`, `PLATFORM_ADDRESS_HINT`,
`PLATFORM_ADDRESS_EXAMPLES`); the `is_platform_address_string` re-export
stays (four live callers).
- Delete `OptionBannerExt::replace` (unused alias + its essay); drop the
duplicate inherent `AmountInputResponse::{is_valid,has_changed}` (the
`ComponentResponse` impl provides them); `add_connection_indicator`
returns `()`.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet_backend): dedup funds paths and harden error typing (Wave 2)
Wave 2 of the architecture audit — wallet-backend funds paths and error
integrity. Behavior-preserving throughout; funds/secret-adjacent code kept
conservative.
- CODE-004: extract `WalletBackend::seed_wallet` for the repeated
`from_seed_bytes` construction; replace fabricated
`PlatformWalletError::{WalletCreation,AssetLockTransaction,InvalidIdentityData}`
string wraps with typed `SeedWalletBuildFailed` /
`IdentityFundingAccountProvisionFailed` variants carrying the real
`#[source]` (`key_wallet::Error`).
- CODE-011: replace `unreachable!("handled by pre-flight")` in
`map_shielded_op_error` with a defensive `WalletBackend` fallthrough so an
ambiguous shielded-spend result can never panic a funds path.
- CODE-012: rewrite the DashPay adapter module doc in present tense and fix
the inverted sidecar-scope comment (2 Global / 4 Identity families).
- CODE-015: delete dead `flush_persister` (+ orphaned
`WalletPersistenceFlushFailed`) and `broadcast_transaction`; keep the
documented single-key signing chokepoint `sign_single_key` (intentional,
test-exercised infra awaiting single-key send) but fix its
source-discarding `map_err` via a typed
`SingleKeySignFailed { #[source] DetSignerError }`.
- CODE-025: parse identity funding intent once into a local `Funding` enum —
the repeated `AccountType` matches, `unreachable!` arms and now-impossible
`UnsupportedIdentityFundingAccount` disappear; fix the semantically-wrong
`WalletRegistrationXpubMismatch` reuse in `upstream_identity_from_seed`
(-> `WalletStateInconsistent`).
- CODE-029: collapse `record_sent`/`record_incoming_contact_request` into one
`record_contact_request(..., direction)` over a 2-variant enum; the public
names stay thin wrappers.
- CODE-034: extract a `with_managed` closure helper for the 4x DashPay view
find-wallet -> kv -> state -> managed_identity preamble.
- CODE-046: collapse the byte-identical `contact_sidecar_key` into
`sidecar_key`.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet): scope test-only single-key signing helpers to tests
`SingleKeyView::sign_with`, `raw_key_bytes`, and the `sign_message_with_raw_key`
free fn were production-visible but reachable only from unit tests — production
JIT single-key signing goes through `SecretAccess` + `DetSigner`, which signs
inline. Move all three behind `#[cfg(test)]` (next to the tests module) and gate
their now-test-only `Message`/`Signature` imports. Delete the dead
`has_passphrase` method (zero callers) and its stale doc.
Audit R2 CODE-010.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet): dedup secret decrypt, bincode framing, prompt meta & key mapping
Wave 1 of the R2 architecture audit — secret access & signing surface dedup.
No behavior change: error mappings and length validation are preserved exactly.
- CODE-003: extract one `decrypt_message` + two-variant `DecryptError` in
model/wallet/encryption.rs; route all three AES-GCM legacy readers
(decrypt_hd_seed, SingleKeyEntry::decrypt, ClosedKeyItem::decrypt_seed)
through it. Those readers are retained deliberately — they decode secrets
already written to users' disks pending the lazy Tier-2 re-wrap, so this is
the safe local dedup, NOT the upstream per-secret migration. See
docs/ai-design/2026-07-08-secret-decrypt-dedup. Wave-16 CODE-087 still applies
unchanged (encryption.rs is kept).
- CODE-031: make `identity_key_from_bytes` and `identity_flavored` pub(crate);
route IdentityKeyView::get/get_protected through the shared length check
(standardizing get's mapping onto IdentityKeyMalformed); drop the hand-rolled
SecretSeam->IdentityKeyVault map in seal_new_identity_key_with_password.
- CODE-038: extract `versioned_bincode::{encode_tagged, decode_tagged_or}`;
wire wallet_seed_store and SingleKeyEntry encode/decode through it, per-format
fallback staying a closure.
- CODE-039: maybe_remember takes the plaintext by value and moves it into the
cache box (copied once, not twice); the duplicated per-variant boxing match
is gone.
- CODE-050: merge the identical WalletPromptMeta/IdentityPromptMeta into one
PromptMeta.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet-backend): dedup KV/sidecar layer
Consolidate the machinery behind the network-scoped KV sidecars and
offline caches so five near-identical patterns live in one place.
- Generic `SidecarView<V>` (new `sidecar.rs`): the wallet-meta,
identity-meta, and auth-pubkey-cache views become thin typed wrappers
parameterised by infix, value type, scope, and error mapper. Domains
needing a legacy-format fallback override `SidecarValue::read`
(WalletMeta's dual-format migration + one-shot re-store).
- One `network_prefix` in `kv.rs`, imported at 7 sites (SPV storage
paths, migration sentinels, the four sidecar keys). The MCP
`network_display_name` keeps its distinct Regtest->"local" mapping.
- `kv_get_logged` / `kv_get_or_default` in `kv.rs` replace seven
read-then-default/None call sites (selected wallet/identity, DashPay
timestamps, avatar + contact-profile caches, auth-pubkey cache via the
generic view).
- One `map_kv_storage_error` funnel behind all five sidecar error
mappers; adds `TaskError::ContactProfileCacheStorage` so contact-profile
failures stop misreporting as `AvatarCacheStorage`.
- One `bip44_account0_xpub` helper replaces four copies of the
fund-routing gate's BIP44 account-0 predicate (2 production + 2 test).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(dashpay): typed errors + move derivation to model/ (Wave 13)
Mechanical typed-error and placement pass across the DashPay backend
subdomain, plus supporting cleanups.
- CODE-068 + CODE-051: move the pure DIP-14/DIP-15 derivation files to
`model/dashpay_derivation/{dip14,hd}.rs` and introduce a typed
`DerivationError` (`#[from]` bip32/secp256k1), replacing ~stringly
`Result<_, String>` derivation signatures. Wire it into `DashPayError`.
- CODE-051: replace the four `DashPayError::Internal { message }` boundary
wraps with typed `TaskError` at their source functions
(`payments::load_payment_history`, `register_dashpay_addresses_for_identity`,
`generate_auto_accept_proof`); add a typed `KeyVerificationError` for the
three identity voting/owner/payout key checks; carry the per-vote
`DPNSVoteResults` error as `Arc<TaskError>` instead of a pre-formatted
String.
- CODE-053: carry DashPay payment amounts as `u64` duffs end-to-end
(task field, backend fn, `DashPayPaymentSent` success variant); the UI
resolves duffs at its edge — no f64 crosses the backend boundary.
- CODE-055: drop the redundant `encryption_tests.rs` (its three checks are
already covered by superior `#[cfg(test)]` tests in `encryption.rs`);
delete the module decl and the dead task builders.
- CODE-059: migrate the two UI callers to `Display`; delete `user_message()`.
- CODE-060: delete `ckd_pub_256`, the legacy `send_payment_to_contact`
wrapper (rename `_impl`), the dead `profile::send_payment` /
`identity_to_child_number`, `From<String>`, the `ToDashPayError` trait,
the `DashPayResult` alias, the unused error-factory helpers, and the
now-unconstructed `Internal` + dead taxonomy variants, with their
speculative allows.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallets): single-source fee/amount/dpns and drop dead send surface
Wave 4 architecture-audit cleanup. Consolidates duplicated fee, amount, and
DPNS logic onto single sources of truth and removes a dead send dialog.
- CODE-090: replace the 9-arg estimate_contract_create_detailed with a
ContractComponents struct; delete the argument-ignoring from_platform_version
(no callers); /1000 -> /CREDITS_PER_DUFF; derive CREDITS_PER_DASH from
DASH_DECIMAL_PLACES.
- CODE-095: move estimate_platform_fee, both transition-based estimators,
AddressAllocationResult and allocate_platform_addresses[_with_fee] out of
send_screen.rs into model/fee_estimation.rs; hoist ESTIMATED_BYTES_PER_INPUT
to one module constant. Add unit tests for the convergence, shortfall and
fee-payer/destination paths.
- CODE-093: consolidate all credits/duffs->DASH display formatting on the model
formatters (add format_duffs_as_dash); delete the private format_dash/
format_credits copies, the local CREDITS_PER_DUFF re-declaration and the
hand-rolled float conversions; one trimmed-Amount precision policy.
- CODE-077: Amount::partial_cmp returns None unless is_same_token, keeping
ordering consistent with equality.
- CODE-092: strip_dash_suffix branches on has_dash_suffix and slices once;
validate_dpns_input returns a fieldless NonDashDomainError.
- CODE-103: move account_summary into ui/state (non-widget view state).
- CODE-099: delete the unreachable SendDialogState, render_send_dialog,
prepare_send_action, the field/init/render call and orphaned imports.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(dashpay): unify avatar pipeline, profile validation, delete dead editor
Wave 17 — DashPay UI cleanup (three findings).
PROJ-003 + CODE-097 — single avatar pipeline. Add a rendering `Avatar`
component (`ui/components/avatar.rs`) backed by an `AvatarCache` fetch state
(`ui/state/avatar_cache.rs`) that dispatches through the App Task System
(`DashPayTask::FetchAvatar` → `BackendTaskSuccessResult::DashPayAvatar`).
Disk-cache consult/populate now happens once, in the backend task
(`avatar_processing::fetch_avatar_cached`). The three screens
(contact_profile_viewer, contacts_list, profile_screen) delegate to the
component instead of each running a raw `tokio::spawn` with its own texture
map and decode/stash copies. No orphaned frame-loop spawns remain.
CODE-094 — one profile-field validator. Add `validate_profile_fields` plus
`MAX_*_CHARS` constants and `ProfileFieldError` to `model/dashpay.rs`
(char-count, matching the protocol). The backend size check and both editors
(profile_screen, identity settings) delegate to it, collapsing four divergent
copies — including the avatar-URL cap, unified on the DIP-0015 value of 2048.
Empty display name is decided legal (matches the backend and DIP-0015).
CODE-100 — delete the dead `contact_info_editor.rs` (no opener anywhere) and
its `ui/mod.rs` wiring. Extract the shared nickname/note/hidden save into
`persist_contact_private_info`, used by the three live inline editors
(contact_profile_viewer, contact_details, contacts_list).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(tokens): honest fees, shared token executor, grouped contract params
Structural fee honesty and token-op deduplication across the token backend:
- FeeResult.actual_fee is now Option<u64> with a FeeResult::estimated_only
constructor; the 22 sites that faked an actual fee by passing the estimate
twice now report an estimate only, so the UI/MCP no longer claim a settled
fee the platform never returned.
- Extract AppContext::execute_token_op: a single executor owning the SDK-call
error mapping, post-broadcast side effects, and the fee tail. All 11 token
state-transition ops shrink to their builder setup plus a delegation.
- Introduce TokenContractParams, replacing the 26-field RegisterTokenContract
variant and the 26-argument build_data_contract_v1_with_one_token with one
grouped struct; delete the always-NotTradeable marketplace_trade_mode field
and its dead selector (its match arms were identical).
- run_token_task now matches the owned task, moving fields into handlers
instead of cloning, aligning with document.rs/dashpay.rs.
Consumers (send screen, MCP identity/masternode outputs, e2e assertions)
updated for the Option-typed actual fee.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(backend): typed contract recovery + document mutation helper
Remove fragile string parsing and duplication from the contract/document/
contested-names recovery paths:
- Delete extract_contract_id_from_error (parsed the contract id out of an
error message string). register/update now use data_contract.id(), already
in scope before broadcast, and share one AppContext::recover_contract_after_
proof_error helper built on log_drive_proof_error with named refetch delays.
- Add a shared log_contested_proof_error for the GroveDB proof-failure shape
the three contested-resource queries surface, collapsing three identical
logging blocks; run_contested_resource_task now matches the owned task.
- Extract DocumentTask::fetch_document_for_mutation (fetch-by-id + bump
revision) used by transfer/purchase/set-price, and convert the six
multi-field DocumentTask tuple variants to named-field struct variants for
readable construction at the UI call sites.
- Document/contract fee tails switch to FeeResult::estimated_only.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(model): enforce model-layer purity and tidy MCP tool boundary
Wave 15 of the architecture audit — strip upward edges out of `model/`,
type the MCP tool errors, and consolidate the tool helpers.
- PROJ-005: move `RootScreenType`/`ThemeMode` into `model::settings`
(re-exported from `ui`); invert the `model → backend_task` error edges in
`passphrase.rs` and `wallet/mod.rs` via model-local error enums wired into
`TaskError` with manual `From` impls. `qualified_identity` keeps `TaskError`
because the `SecretAccess::with_secret` chokepoint contract requires it
(documented).
- CODE-070: relocate `FeatureGate` + predicate to `context/` (dead
`FeatureGateUiExt` deleted); move token `From` conversions next to their UI
types; `IdentityStatus → Color32` mapping to `ui::theme`; model-local
`MasternodeInputError` converted to `McpToolError` at the tool boundary.
- CODE-072: move GroveSTARK proof generation/verification into
`backend_task::grovestark` (data types + serialization stay in `model`),
drop 59 info-level hex dumps, type `GroveSTARKError` with `#[source]` fields.
- CODE-073: `qualified_identity_public_key` extracts `find_wallet_path` and
warn-skips malformed network-supplied key data instead of panicking.
- CODE-080/081/082/083: delete dead items (gate test-only ones with
`#[cfg(test)]`), collapse the auth-key accessors behind
`authentication_keys_matching`, drop dead `RequestType` u8 conversions and
rename the module to `request_type`, delegate `borrow_decode` to `decode`.
- CODE-079: `tool_ctx()` returns `McpToolError` natively — kills the lossy
`McpError → String` round-trip at all 26 tool invocations.
- CODE-085: fold the blank-`network` check into `resolve::require_network`;
merge `validate_amount`/`validate_credits` into `validate_positive_amount`.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(app): collapse boot/reconcilers, fold context_provider, drop identity-hub flag
Wave 12 of the full-repo architecture audit — app.rs boot path, feature
flags, and the context provider.
PROJ-001: collapse the four duplicated eager wallet-backend init shapes
(boot, network switch, post-onboarding auto-start, manual Connect) into one
`AppState::spawn_backend_init` parameterized by a `BackendInitReason`. Extract
the per-frame reconcilers accreted on `AppState` into small `update()`-style
structs under `src/app/reconcilers.rs` — `AccessibilityActivator`,
`SpvBlockReconciler`, `ConnectionBanner`, and `MigrationReconciler` (the latter
two return a `BackendTask` for `AppState` to dispatch, keeping the channel
chokepoint in one place). Reduces `AppState` from ~13 flat reconciler fields to
four cohesive members; no behavior change.
PROJ-004: move scheduled-vote casting off the UI thread into a new
`ContestedResourceTask::CastDueScheduledVotes` backend task. The 60s tick only
dispatches; the DB query, identity load, and casting run in the task; the DPNS
Scheduled Votes screen learns progress via `display_task_result`
(`ScheduledVotesInProgress` then per-vote `CastScheduledVote`). Removes the two
frame-aborting `return`s and names the 2-minute lateness window
(`SCHEDULED_VOTE_MAX_LATENESS_MS`).
PROJ-006: delete the `identity-hub` and `identity-hub-activity-feed` Cargo
features and every `cfg` fallback branch — the hub is now always built and
registered; the Activity tab renders its gated "coming soon" message
unconditionally.
PROJ-008: fold `context_provider_spv.rs` into `context_provider.rs`. Drop the
unused `db` field and `_db` params, make `SpvProvider::new` infallible, and
replace the `Mutex` with an `Arc<ArcSwapOption>` so the manual `Clone` impl and
lock-poison plumbing disappear. Keeps the `SYSTEM_CONTRACT_COUNT` compile check.
PROJ-009: give dir/env/logger boot setup a single owner,
`boot::prepare_environment()`, called from both `main` and
`AppState::boot_inputs`; the logger's `Once` guard keeps the second call a
no-op. Preserves the `testing`-cfg `boot_inputs` split.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(backend_task): update stale proof_log_item import after Wave 15 rename
Missed in the Wave 15 merge commit -- contested_names/mod.rs's
log_contested_proof_error (added by Wave 3) imported RequestType from
the old model::proof_log_item path, which Wave 15 renamed to
model::request_type. Same collision class as the two request_type
import conflicts resolved in that merge, just not flagged by git
since the lines didn't overlap.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): tidy contract_token_db — typed encode error, dedup, param & fn moves
Wave 7 of the round-2 architecture audit on context/contract_token_db.rs:
- CODE-018: propagate TokenConfiguration encode failures through a new typed
TaskError::TokenConfigSerialization variant instead of silently no-oping;
drop the pre-C7 tombstone comment.
- CODE-019: extract load_sorted_tokens(); both token listers map its output,
killing the warn-vs-silent drift on unparseable token keys.
- CODE-030: drop three dead parameters — get_contracts pagination
(limit/offset, all callers passed None,None), ConnectionStatus::tooltip_text
app_context, and request_to_det_contact's unused ContactRequest.
- CODE-033: move remove_wallet to context/wallet_lifecycle.rs, next to
register_wallet and its existing tests.
- CODE-044: iterate the system contracts for the five get_contracts insertion
blocks; fold the five system-contract load blocks in context/mod.rs into one
local closure.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(backend_task): consolidate error.rs and replace message-as-protocol with typed signals
Wave 14 — error.rs consolidation + typed messages.
CODE-061: add `consensus_cause(&SdkError) -> Option<&ConsensusError>` used by all
three extraction sites; replace the intermediate `ConsensusKind` enum in
`From<SdkError>` with per-arm constructor closures so each of the 13
consensus→TaskError mappings is written exactly once.
CODE-066: replace `TaskError::MustRetry(String)` with the typed
`CoreWalletAutoDetected { wallet_name }` — the sentence lives in `#[error(...)]`,
no user-facing String field. Update the app.rs consumer and the unit test.
CODE-062: delete the `NO_IDENTITIES_FOUND` string constant and the
message-text comparison in tokens_screen; route the no-identities case through
the existing typed `TaskError::NoIdentitiesFound` via a new `display_task_error`
hook (also fixes a latent refresh-spinner hang).
CODE-065: add typed success variants `AssetLockBroadcast { txid }`,
`DashPayAddressesRegistered { addresses, contacts, errors }`, and
`IdentitiesLoaded { count }`; backends stop composing conditional grammar
fragments and sentence assembly moves to the UI/app layer as complete
i18n-clean strings. create_asset_lock_screen reads `txid` from the variant
instead of parsing it out of the message string.
CODE-069: delete the empty `impl BackendTaskSuccessResult {}` and collapse the
identity `match` in `run_backend_tasks_sequential` to a direct push.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): consolidate wallet_lifecycle orchestration (Wave 11)
Wave 11 placement/dedup pass over context/wallet_lifecycle.rs, no behavior
change to production flows.
- Fold the start_spv -> spawn_backend_start -> run_backend_start chain into
the async ensure_wallet_backend_and_start_spv chokepoint (the only
production start path); retarget the two orphaned tests onto the
wallet_backend() wiring gate and the wiring-does-not-start invariant.
- handle_wallet_unlocked now takes `&str` instead of `Option<&str>`, making
the inert no-passphrase case unrepresentable: delete the two provably
no-op `(wallet, None)` callers (cold-boot bootstrap loop and
try_open_wallet_no_password) and gate the unlock popup's call to the
keep-unlocked branch only. The deferred non-remember-registration note
moves verbatim to the popup's non-remember branch, where that decision
now lives. try_open_wallet_no_password keeps a dead `_app_context` param
for now (TODO(cleanup) logged) to avoid a ~40-callsite UI sweep mid-batch.
- Hoist one shared `copy_dir_recursive` test helper (two nested copies) and
route the two inline `INSERT INTO wallet` blocks through the existing
seed_legacy_unprotected_hd_wallet_row helper.
- Strip the issue7 test's eprintln scaffolding, rewrite its assertions in
present-tense guard voice (a pass is now the invariant, not a "reproduced"
bug), and rename migration_status' failed_state test to state what it
asserts.
- Add a module-doc note marking wallet_lifecycle.rs as the intentional thin
AppContext-delegation layer, distinct from wallet_backend's upstream
orchestration.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(identities): consolidate funding-method chooser + preserve explicit picks
Wave 5 — identity funding screens + PR-869 fold-ins.
CODE-096: move `FundingMethod` (enum, labels, `default_funding_state`) into
`funding_common.rs` so the create-identity and top-up screens share one type;
extract a `wallet_selection_combo` picker (parameterised label + enabled) used
by all three screens and an `actionable_asset_locks` gate shared by both
unused-funding pickers; fix the ungrammatical wallet-selection heading to an
i18n-clean sentence.
F2: sweep the residual RwLock-poisoning `.read()/.write().unwrap()` pattern in
the funding-screen cluster to graceful forms — `TopUpIdentityScreen` gains
`current_step`/`set_step`/`current_funding_method` helpers; the balance and
asset-lock sub-screens and both `by_platform_address` step sites degrade
instead of panicking; `update_wallet`'s `.expect("wallet lock poisoned")`
becomes `is_ok_and`.
F3: track whether the user has explicitly chosen a funding method. On a wallet
switch the create screen recomputes its default pre-selection only while no
explicit choice has been made; once a method is picked, a switch preserves it
untouched (new pure `funding_method_after_switch`, unit-tested for both cases).
Removes the anchoring `TODO(bilby)`.
F4: the single-wallet Top-Up default now uses
`spendable_covers_minimum(spendable, estimate_identity_topup())` instead of
`snapshot_has_balance`, so a dust/locked balance is never pre-selected then
immediately blocked.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): remove write-only Core-RPC health plumbing (CODE-016)
The rpc_online / rpc_last_error fields, their getters/setters,
update_from_chainlocks, the handle_task_result ChainLocks arm, the
set_rpc_last_error(None) writer in core, and the dead reset_timer had
zero readers anywhere — connection health is sourced entirely from SPV
and DAPI state. Delete them; the surviving ChainLock arm just refreshes
overall state.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(context): make settings updaters atomic, drop dead shims (CODE-027, CODE-042)
CODE-027: the update_* methods did a read-modify-write across two
separate lock acquisitions (get_app_settings then set_app_settings),
so concurrent updates could lose writes. Introduce update_app_settings,
which runs the whole read → mutate → persist cycle under one held
cached_settings write guard (the same SettingsCacheGuard scheme), and
route every updater through it. Factor the uncached load out of
get_app_settings so both paths share it. Add a sequential RMW test and
a concurrent-flip test proving no field update is lost.
CODE-042: delete the zero-caller get_settings legacy shim,
update_user_mode, and update_show_evonode_tools.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): extract shared platform-address seeding loop (CODE-023)
apply_platform_address_push and warm_start_platform_addresses carried an
identical per-address inner loop (hash → canonical address → info-write →
signer registration), differing only in set- vs seed-if-absent semantics.
Extract seed_platform_address_entries taking the info-write as a closure;
both entry points become thin batch adapters.
Also collapses default_platform_version's four identical match arms to the
constant (CODE-045).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(context): name DPNS contest-duration constants (CODE-040)
Replace the inline 14-day / 90-minute literals in
contest_duration_for_network with named MAINNET_CONTEST_DURATION and
NON_MAINNET_CONTEST_DURATION constants carrying a spec reference, and
note that the joinable window is the first half of the contest.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor: small context/kv one-liner cleanups (CODE-045)
- Inline ConnectionStatus::spv_connected (status.is_active()) at its sole
caller and delete the wrapper; drop the now-unused import.
- delete_scheduled_vote / insert_scheduled_votes take &str / &[..] instead
of &String / &Vec, dropping the clippy::ptr_arg allow.
- Delete the map_kv_error identity wrapper; call sites use
KvAdapterError::Store directly, with its rationale folded into the
variant doc.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(ui): Wave 19 — dedup UI component surface
CODE-107: Extract one generic add_subscreen_chooser_panel + nav_button
(subscreen_chooser_panel.rs); the four dashpay/dpns/tokens/tools panels
now build item lists and delegate. Move ToolsSubscreen next to the tools
screens (ui/tools/mod.rs), matching its siblings.
CODE-109: Extract modal_chrome (backdrop + centered bordered Window) from
passphrase_modal; rebase passphrase_modal and the confirmation/selection/
info dialogs onto it. Dedupe the NOTHING sentinel into modal_chrome. Drop
ConfirmationDialog's write-only `status` field (current_value never read it).
CODE-110: Rewrite ScreenType::eq as explicit payload arms + discriminant
equality (no `_ => false`). Collapse the seven ScreenLike delegation methods
onto one exhaustive delegate_to_screen! macro — a missing variant is now a
compile error. (Macro, not a dyn accessor, to preserve inherent-method
resolution for screens with an inherent refresh.)
CODE-113: Extract key_eligibility, render_key_combo, render_no_eligible_key_group
and render_info_section in ui/helpers.rs; the two key choosers and two success
screens delegate. Unify the divergent no-eligible-key wording.
CODE-115: Wire the contract-chooser right-click via Response::context_menu on
the header row (Copy Hex / Copy JSON), removing the unreachable manual Window
and its three dead state fields; derive Default; flatten the font-size ladder.
CODE-117: Extract stage_progress + window_fraction in network_chooser_screen;
the five progress calculators shrink to thin wrappers.
Also folds in a stray cargo-fmt import-order fix in
backend_task/contested_names/mod.rs (Wave 15 rename residue).
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet): Wave 16 — wallet model internals
Model-layer cleanups across the wallet crate. All error paths in
model/wallet now carry typed WalletError up to the TaskError boundary;
the platform-payment registration, coin-type mapping, and AES-GCM
envelope shapes are single-sourced.
- CODE-071: model/wallet/mod.rs derivation/registration functions return
Result<_, WalletError> instead of stringly errors (~17 KeyDerivation
erasures + 5 bare erasures gone). New typed WalletError variants:
PublicKeyParse, AccountDerivationPath, PlatformAddressConversion,
AddressNetworkMismatch. TaskError::WalletAddressProviderSetupFailed
now holds a typed #[source] instead of a String. database/utxo.rs
get_utxos_by_address returns rusqlite::Result.
- CODE-076: delete Wallet::coin_type + four inline network matches; call
the canonical coin_type_for_network everywhere.
- CODE-078: extract Wallet::register_platform_payment_entry; the five
duplicated known/watched insert blocks collapse to one call. The
vestigial post-T-W-01 `register` param on
generate_platform_receive_address_with_seed is dropped.
- CODE-087: replace the (Vec<u8>,Vec<u8>,Vec<u8>) crypto triple (and its
type_complexity allows) with a named EncryptedEnvelope struct.
- CODE-088: one summary log per platform-address sync; the per-key dumps
in WalletAddressProvider::on_address_found and QualifiedIdentity::sign
move behind their failure paths, off the hot loop.
- CODE-089: delete the unused DASH_SECRET_MESSAGE constant.
- CODE-091: cross-link + status-mark the three single-key modules (LIVE
imported-key sidecar vs the LEGACY Decision-#7 runtime/DB pair).
cargo clippy --all-features --all-targets -D warnings clean; full lib
suite (1364 tests) green.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(wallet): fold shielded screens into unified send screen (CODE-098)
The three standalone shielded screens (shield / shielded-send / unshield)
duplicated flow logic the unified WalletSendScreen already owns end to end
(dispatch + result handling for every shield/unshield/private-send path).
Consolidate them into routes on the one canonical send screen.
- Add `SendFlow` preset {General, Shield, ShieldedSend, Unshield}. A preset
locks the source — and, for Shield, auto-targets the wallet's own pool with
a sentinel shielded destination (the shield dispatch ignores the destination
address) — so the screen shows only the controls that flow needs while
reusing the unified validation, fee/amount limits, and dispatch.
- Extract shared shielded-recipient parsing into
`model::address::parse_shielded_recipient` (Bech32m or 43-byte hex) so the
send dispatch and any validation path cannot diverge.
- Route the Shielded tab's Shield / Send (Private) / Unshield buttons to open
the unified send screen pre-configured for the flow.
- `ScreenType::WalletSendScreen` now carries the `SendFlow`; remove the three
`ScreenType`/`Screen` shielded variants and delete the standalone screen
files.
Behavioral parity preserved for all five shielded dispatches. One delta:
raw-hex shielded recipient entry is dropped from the UI (AddressInput accepts
canonical Bech32m only); the parser still accepts hex at dispatch.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* refactor(tokens): Wave 18 — unify token action screens + dedup rules renderers
Collapse the seven near-identical single-shot token action screens (pause,
resume, freeze, unfreeze, destroy-frozen-funds, burn, mint) onto one
parameterised scaffold, and merge the two duplicated control-rule renderers.
- Add `TokenActionScreen<A: TokenAction>` (ui/tokens/token_action_screen.rs):
the shared shell — authorization resolution, wallet unlock, advanced key
chooser, per-action form slot, public note, fee estimate, confirmation
dialog, success screen and status. Each action supplies only its diffs
(labels, rules accessor, form, task builder, success variant) via the
`TokenAction` trait; the seven screens become thin `type` aliases + a small
action struct. Net ~3,900 lines removed.
- Fold the token authorization check into the shared scaffold's one call site
via the previously zero-caller `check_token_authorization`; also route
`update_token_config`'s inline resolver through it.
- Merge `render_mint_control_change_rules_ui` into
`render_control_change_rules_ui` via an optional `MintRecipientSection`,
deleting the ~180-line duplicate; update all call sites.
- `change_context` now rebinds token-screen context through `.common` via a
`set_app_context` setter and a `common_set` macro arm.
Deferred (TODO markers in-file): set_token_price migration onto the scaffold
(large pricing form; verb-based auth message template does not fit "set price"
grammatically) and the TokensScreen god-struct per-subscreen split.
cargo fmt + clippy (all-features, all-targets) clean; lib unit tests and the
kittest suite pass.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* fix(wallet): address QA findings on shielded send-flow fold
- QA-002: Shield-from-Platform "Max" now reserves the two-action shielded-fee
headroom (>50M credits) instead of the plain platform-transfer estimate
(~8M). ShieldFromBalance pays the shield fee from the same balance as the
amount, so the old reserve under-shot ~6x and a Max attempt was rejected
upstream. New model helper `shield_from_balance_fee_headroom` keeps the fee
math in model/fee_estimation.rs. Shield-from-Core parity was already correct.
- QA-001: restore raw-hex shielded-recipient entry. `AddressKind::detect` and
`AddressInput::validate_shielded` now accept the 43-byte (86-hex-char) form
via the shared `parse_shielded_recipient`, matching what the old private-send
screen advertised.
- QA-003: on a network switch, `WalletSendScreen` now drops the wallet seed
hash, source, destination, and amount (`reset_for_network_switch`) so a
preset flow can no longer resurrect stale cross-network state / balance.
Tests: model coverage for the fee headroom (asserts the shielded-fee reserve,
not the transfer estimate) and for hex shielded detection at both the model
and AddressInput layers.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* docs(comments): Wave 21 — comment/narration hygiene sweep
Strip development-history narration and ephemeral review IDs from comments;
keep present-state invariants only. No executable logic changed.
- CODE-021: rewrite archaeological rationale (det_platform_signer, snapshot,
wallet_backend/mod, secret_seam, wallet_seed_store, wallet_lifecycle
stop_spv); dedup the AlreadyOpen restart-in-place rationale to one copy.
- CODE-067: present-state the Phase B/D/E + Post-D4c narration in
backend_task/shielded/mod.rs and dashpay.rs; stale "until Phase-E lands"
reworded now the push writer exists.
- CODE-084: drop RUST-001 / 6a2818cd IDs from fee_estimation.rs and
wallet/single_key.rs comments; keep the durable TS-DBG-01 test-spec ID.
- CODE-102: delete the shielded_tab tombstone (keep "Fund-moving results
only."); present-state the "replaces the dropped/legacy…" docs in
wallets_screen, contacts_list, contact_details.
- CODE-106: drop INTENTIONAL(CMT-010/RUST-003/CODE-003) prefixes and FIX N
markers in theme, message_banner, address_input, add_new_identity_screen,
key_info_screen.
- PROJ-007: remove the removed-subsystem ZMQ sentence and the "RPC, ZMQ"
ConnectionStatus residue from CLAUDE.md; protoc v25.2+ verified against CI.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
* docs(changelog): add round-2 audit user-facing changes
Adds Changed/Fixed entries for the genuinely user-observable outcomes
of the round-2 architecture audit (21 waves + CODE-098): the shielded
screens' fold into the unified Send screen, DashPay's now-optional
display name, two identity-funding UX fixes, the My Tokens
loading-spinner hang, and a settings-save race. Internal refactors,
dedup, and error-typing from the same round are intentionally omitted.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(identity): confine hero card gradient to card bounds
The Identity Hub hero card painted its 14%-opacity gradient band using
`ui.max_rect()` — the available space, not the card's content bounds — so
on the Home tab the band bled downward through every sibling widget below
the card (quick actions, onboarding checklist, recent activity).
Reserve a shape slot before laying out content, then fill it afterward
from `ui.min_rect()`, confining the band to the card's actual bounds. Add
unit tests asserting the strips stay within the given rect.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* refactor(errors): replace stringly-typed error variants with typed sources
Convert the largest cluster of `detail: String` TaskError variants to typed
`#[source]`/`#[from]` variants and delete the dead ones, tightening the
Display/Debug separation and reducing `result_large_err` pressure.
- platform_info: introduce a `WithdrawalParseError` source enum (platform-value
field errors, missing/invalid timestamps, unrecognized status, boxed
ProtocolError for the daily limit). `WithdrawalDocumentParsingError` now wraps
it via `#[from]`; the two duplicated per-document formatting blocks are
extracted into `format_withdrawal_line` / `format_completed_withdrawal_line`.
`ShieldedSyncFailed` now carries `Box<SdkError>`.
- key input: move `verify_key_input` out of the backend into `model/key_input.rs`
as the stateless single source of truth, returning a typed `KeyInputError`
(NotHex / BadWif / UnsupportedLength) with complete, i18n-ready sentences.
`KeyInputValidationFailed` becomes a transparent `#[from]` wrap, removing the
double-naming and fragment concatenation; callers use `?`. Unit-tested.
- dashpay: delete the unconstructed `reason: String` variants BroadcastFailed,
QueryFailed, PlatformError, RateLimited and their dead retry-classification
clone arms; make the sole remaining recoverable variant `NetworkError`
fieldless.
- delete unconstructed TaskError variants (WalletUtxoReloadFailed,
WalletBalanceRecalculationFailed, WalletPaymentFailed, UtxoUpdateFailed,
SerializationError, RpcProviderCreationFailed, AssetLockTransactionBuildFailed,
ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed,
ShieldedMerkleWitnessUnavailable); make InvalidPrivateKey and
NetworkContextCreationFailed fieldless (their strings were hardcoded, not
upstream errors).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* docs(contested-names): point contract-not-found TODOs at issue #875
The 'contract not found when querying from value with contract info'
substring match in the three contested-name query retry loops is a
deliberate workaround: the condition originates server-side as
QuerySyntaxError::DataContractNotFound and reaches the client only as a
gRPC Internal status with message text, so no client-side structural SDK
variant exists to match on yet. Reference the tracking issue at all three
sites.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(wallet): unify lock-poison recovery, route single-key I/O through the secret seam, and type encryption key hygiene
Three wallet-backend hardening changes.
- Lock-poison policy: add `wallet_backend/poison.rs` with `read_recover` /
`write_recover`, which recover a poisoned `RwLock` guard instead of erroring.
The single-key in-memory index and the secret session cache guard derived,
rebuildable state, so recovery is correct and self-healing. Route the
single-key index (imports, alias, forget, rehydrate, list) and the
`SecretAccess` session cache (lookup, eviction, remember) through it, deleting
the two lying poison mappings — `ImportedKeyNotFound` (told the user to
re-import) and `SecretDecryptFailed` (claimed a decrypt failure).
- Secret seam: route every raw vault access in `single_key.rs` through
`SecretSeam::{put_secret, get_secret, put_secret_protected, delete_secret}`
instead of hand-rolled `SecretStore` calls, so imported keys honor the same
chokepoint and failure variant (`TaskError::SecretSeam`) as their siblings.
The verify-passphrase wrong-password signal is preserved by matching the
seam's typed `SecretSeam` source structurally.
- Encryption hygiene: `derive_password_key` returns `Zeroizing<Vec<u8>>` and
`ClosedKeyItem::decrypt_seed` returns `Zeroizing<[u8; 64]>` (copied straight
into the zeroizing buffer, no bare stack copy), so the derived AES key and the
decrypted seed wipe on drop. Introduce a typed `EncryptionError`
(WrongPassword / Malformed / KeyDerivation / Encryption, Everyday-User
messages) and return it from the encryption primitives and the two
`ClosedSingleKey` crypto methods, replacing their `Result<_, String>`. The
broad `SingleKeyData::open` / `WalletSeed::open` / `SingleKeyWallet::new`
String APIs (pre-existing model/wallet debt that mixes crypto with key-parse
errors) render the typed error through `Display` at the boundary; a full
type-through of those APIs is deferred.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix(address): centralize network-prefix validation in model/ and gate the credits-to-address MCP destination
Two coupled changes around address↔network validation.
- Single source of truth: add `validate_platform_address_for_network` and its
Orchard twin `validate_orchard_address_for_network` to `model/address.rs`,
returning a typed `AddressNetworkMismatch` (Everyday-User message). They parse
the bech32m HRP (`dash1…` mainnet, `tdash1…` testnet, case-insensitive) and
reject a cross-network address. `address_input.rs` delegates its hand-rolled
`validate_platform` / `validate_shielded` prefix checks to these, keeping the
same banner copy; the GUI's format/length/case checks stay put.
- Close the MCP network gap (funds-adjacent): `identity_credits_to_address`
decoded the destination with `PlatformAddress::from_bech32m_string` and no
network check, so a mainnet `dash1…` address could be paid on testnet (or
vice-versa). Gate the parsed destination against `ctx.network()` via the new
model validator, returning `InvalidParam` on mismatch — mirroring the withdraw
tool's Core `require_network` guard. The required `network` param only pins the
active network; it never validated the destination, which was the gap.
The `resolve::validate_address` first-Base58-char Core heuristic is left as-is:
it is a network-agnostic format sanity check with no network parameter, and Core
network validation is already performed via `Address::require_network` at each
tool. Adding a model Core validator would duplicate that with no clean
delegation target, so it is deferred rather than widened here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* refactor(wallet): add AppContext::wallet_arc helper; log skipped corrupt address rows; drop per-item logging loops
Three wallet-layer cleanups from PR review.
- Dedup wallet lookup: add `AppContext::wallet_arc(&self, &WalletSeedHash) ->
Result<Arc<RwLock<Wallet>>, TaskError>` as the single source of truth for the
"look up a wallet arc or `WalletNotFound`" pattern, replacing 15 copy-paste
blocks across backend_task/{wallet,identity,shielded} and context. The helper
recovers a poisoned lock via `wallet_backend::poison::read_recover` rather than
erroring: the in-memory wallet map is rebuildable, so recovery matches the
Wave-2 poison discipline for rebuildable state. This is a behavior change on
the rare poison path — the replaced sites previously surfaced `LockPoisoned`
via `.read()?`; they now self-heal, consistent with `mcp::resolve::wallet_arc`
(which now delegates to this helper and re-wraps the id-bearing MCP error).
- Loud failure on corrupt rows: `database/wallet.rs` `get_wallets` skipped
unparseable address rows silently. Silently dropping an address could hide a
missing key and lead to lost funds, so log a `warn!` (row index + error) and
continue, matching the wallet_backend hydration skip-logging discipline.
- Drop per-item logging loops: `fetch_platform_address_balances` logged every
address/balance/nonce and `transfer_platform_credits` logged every input
address/amount at `info!`. Both keep their aggregate summary line; the
per-address financial detail is removed — it violates the no-loop logging rule
and should not sit in plaintext logs at the default level.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: replace panicking unwrap/expect in production paths with error propagation and graceful UI degradation
Eliminate every bare `.unwrap()` on the non-test production paths (~150 sites)
so a broken invariant degrades instead of aborting the process. Applied by
category:
- Lock poison recovery: extend `wallet_backend/poison.rs` with a `Mutex`
`lock_recover` free fn plus ergonomic `RwLockRecover` (`read_recover` /
`write_recover`) and `MutexRecover` (`lock_recover`) extension traits. Route
the wallet/step/settings `RwLock` and the DPNS/identity/token-search `Mutex`
screen-state locks — all rebuildable in-memory state — through them, so a
thread panicking mid-update never wedges the UI. Unify the coordinator gate's
action mutex on the same recovery policy (its `should_fire` already tolerated
poison via `is_ok_and`, while `arm`/`try_fire`/`reset` panicked). The
`Mutex<Connection>` gains a `Database::locked_conn()` helper that recovers the
guard: a `rusqlite::Connection` carries no invariant a panic can break, so
recovering avoids cascading one unrelated panic into every later DB call.
- Graceful UI fallback: egui frame paths return `AppAction`/`BackendTask` and
cannot `?`. The `Option::unwrap()` selection reads now guard with `let-else` /
`if let` and degrade — an actionable `MessageBanner`, an early return, or a
logged skip — instead of panicking. The document-action builders share a new
`require_selections()` helper that returns `BackendTask::None` with a banner
when a selection is missing.
- Invariant expects: unwraps that are provably infallible after a preceding
guard, on constants, or on a construction invariant are upgraded to
`.expect("invariant: …")` so a future regression fails loudly with context
rather than a bare panic. BIP32 child indices are documented as `< 2^31`
(hardened constants, small coin type, and hash indices masked with
`& 0x7FFFFFFF`).
The `dashpay_increment_send_index` mutex keeps its documented fail-loud
poison contract: a panic mid-increment can leave the address-index counter
inconsistent, and surfacing that is safer than handing out a duplicate index.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* chore: remove ephemeral review-IDs, tombstones, speculative dead_code, and a redundant predicate
Four small cleanups; every deletion grep-verified against both the lib and the
integration-test crates first.
- Ephemeral review-IDs: strip the per-run tags from committed source — the
`RUST-002` tag on the app.rs banner-flash TODO (the substance and the #660
issue link stay) and the `CODE-027` prefixes on two settings_db doc comments
(replaced with a plain statement of the read-modify-write guarantee). The
standards-body `RUSTSEC-2025-0141` reference is deliberately kept.
- Tombstones: drop the two deleted-code tombstones in `database/wallet.rs`
`get_wallets` (git is the record) and renumber the trailing "step 8" to
"step 4" so the step comments read coherently.
- Dead code: delete genuinely-unused speculative items and the code they guard
— three unconstructed `BackendTaskSuccessResult` variants, the `CoreTask::
GetBestChainLock` variant plus its handler / PartialEq arm / classification
test (and the now-orphaned `AppContext::rpc_error_with_url` its handler was
the sole caller of), a duplicate `decrypt_private_data`, an unused avatar
`to_grayscale`, three unused `KeyStorage` accessors, an unused token-info
constructor, a write-only `core_address` field, and a dead visualizer search
field. Items that turned out to be live are handled correctly instead of
deleted: `AppAction::Refresh`, `IdentityTask::SearchIdentityFromWallet`,
`KeyStorage::keys_set`, and `send_screen`'s `selected_wallet_seed_hash` had
stale `#[allow(dead_code)]` (the attribute is simply removed); `CORE_APPLICATION`
is non-Linux-only, so it becomes `#[cfg(not(target_os = "linux"))]` and is only
compiled where used; and `MessageBanner::{has_global, set_auto_dismiss}` plus
`build_identity_registration` are exercised by the kittest / backend-e2e
integration-test crates (a separate compilation the lib does not see), so they
keep `#[allow(dead_code)]` with a reason comment rather than `#[expect]`, which
would be unfulfilled under the `--all-targets` gate.
- Redundant predicate: inline `is_distinct_change_candidate` (a one-line
…
This was referenced Jul 20, 2026
Merged
feat(identity): pending DPNS registration indicator, hint-text sizing, and social-profile fixes
#918
Merged
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
2 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.
Why this PR exists
identities.first()), ignoring any user choice.docs/platform-wallet-migration-design; review/merge feat: rewrite Dash Evo Tool onto the new platform-wallet #860 first. The diff shown here is the hub + switcher on top of feat: rewrite Dash Evo Tool onto the new platform-wallet #860.platform-wallet(recurring sync, DIP-15 compact xpub, key-purpose interop) — the backend this hub's Contacts/DashPay surfaces consume.What was done
Unified Identities hub (
src/ui/identity/, Cargo featureidentity-hub, default on)identity-hub-activity-feed, default off). Settings: social profile, advanced toggles, danger zone.App-scoped selected wallet + identity (the substantive new capability)
model/selected_identity.rs, separatedet:selected_identity:v1KV blob) mirroring the existing selected-wallet mechanism.AppContextgains getters/setters andresolve_selected_identity()as the single source of truth for "who am I operating as".Identities › ‹wallet› › ‹identity›is the wallet + identity switcher: working dropdowns (wallet list + "Set up another wallet"; identity list scoped to the selected wallet, an "identities without a wallet on this device" group, inline search at ≥7, "add another identity" footer, dev-mode entries), with placeholder / subdued / interactive states per design-spec §A.3.IdentitySelectorgains opt-inwith_app_default()/syncing_global(): operate-as pickers can default to and sync the global selection, while recipient/target pickers opt out and stay byte-identical to before — no accidental sync (e.g. choosing a payment recipient never changes who you are).User story
Imagine opening Dash Evo Tool on a fresh profile: the sidebar shows Identity Hub, landing on a welcome card, and the topbar breadcrumb reads
Identities › (no wallet yet) › (no identity yet). Once you have wallets and identities, that breadcrumb becomes a live switcher — pick a wallet, pick an identity, and the hub follows your choice (and, after the follow-up PR, so does every operate-as screen). No more guessing which identity an action runs as.Testing
cargo clippy --all-features --all-targets -- -D warnings— cleancargo build --all-featuresandcargo build --no-default-features— clean (hub feature-gates elide cleanly)cargo +nightly fmt --all --check— cleancargo test --lib --all-features— 1223 passedcargo test --test kittest --all-features— 168 passedBreaking changes
AppContext(a latent single-key persist gap is fixed in passing).Follow-ups (deferred, tracked)
WalletFixturetest builder to unblockIT-SWITCH-01/02(loaded-HD-wallet switching kittests).landing()path.KeysScreen+left_wallet_panel.rsconfirmed unreachable — separate dead-code cleanup.pending_saveclear indisplay_task_errortoUpdateProfilefailures only (it currently clears on any task error — cosmetic "Save stays enabled after a successful save" edge, no data loss); add achange_context → refresh → resetintegration test (the T28 wiring is fixed but only unit-covered at thereset()leaf); in the Contacts request rows, skip malformed outgoing docs (zero-id row) and handlets=0timestamps.Checklist
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent