Skip to content

feat(identity): pending DPNS registration indicator, hint-text sizing, and social-profile fixes - #918

Merged
lklimek merged 15 commits into
v1.0-devfrom
feat/dpns-pending-pill-and-hint-typography
Jul 23, 2026
Merged

feat(identity): pending DPNS registration indicator, hint-text sizing, and social-profile fixes#918
lklimek merged 15 commits into
v1.0-devfrom
feat/dpns-pending-pill-and-hint-typography

Conversation

@Claudius-Maginificent

@Claudius-Maginificent Claudius-Maginificent commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

TL;DR: Show a "Pending" indicator for a requested-but-not-yet-awarded DPNS username, fix illegibly small hint text, clean up a confusing Contacts CTA with a dead button, and give social-profile saves real progress/success/error feedback — plus fix two review-found bugs in that new feedback (a banner that never went away, and a false success toast). Further fixes: the DPNS registration screen now tells you correctly whether your username was registered outright or is still awaiting a community vote, the Pending tooltip explains that Dash masternodes decide contested names, a pending request now counts toward the "Pick a username" onboarding step, and the identity badge on the Settings tab is properly aligned. Also fixes two wallet bugs found while live-testing this PR: a "Create Asset Lock" button that sat in the wrong place on the Wallets screen, and a Fund Platform Address dialog (plus Receive and Mine) that could silently vanish and reset when picking certain addresses from its dropdown.

User story

As a user who requested a username and its contest hasn't resolved yet, I want to see that my request is pending rather than looking like I never requested one, so I know it wasn't lost.

As a user saving my social profile, I want an honest, correctly-timed progress/success/error banner, so I can trust whether my changes actually saved.

As a user who registers a DPNS username that turns out to be contested, I want to be told honestly that it's pending a community vote rather than being told it's registered, so I don't believe I own a name I might still lose.

As a user funding a Platform address from an existing asset lock, I want the dialog to stay open no matter which address I pick from the list, so a stray click doesn't silently lose my in-progress attempt.

Scenario

Base flow

A user requests a DPNS username while its contest is still open; separately, a user opens Contacts to set up a display profile and saves it; separately, a user registers a DPNS username that other people also want; separately, a user opens the Wallets screen to fund a Platform address from an existing asset lock.

Actual behavior

A pending username looked identical to never having requested one. Onboarding hint text rendered at ~9px, hard to read. The Contacts setup card had a mis-worded CTA and a "Why?" button that did nothing when clicked. Saving a social profile gave no feedback even though it succeeded on Platform — and in review, that turned out to be worse than just "no feedback": the "Saving…" banner could stay on screen forever alongside the outcome banner, and a stale save result could pop a false "saved" toast after switching identities mid-save. Separately, registering a contested username showed "DPNS Name Registered!" even though the name was only submitted for a community vote, with no mention that masternodes decide the outcome, and the onboarding checklist kept showing "Pick a username" as incomplete despite the pending request. The identity badge on the Settings tab also sat awkwardly in its own row, misaligned with the rest of the column. Separately, on the Wallets screen, the "Create Asset Lock" button sat in the Asset Locks card's header instead of below the list like every other action button on that screen, and picking certain addresses from the Fund Platform Address dialog's dropdown — particularly ones near the bottom of a longer list — silently closed and reset the whole dialog, discarding the in-progress funding attempt with no error shown. The Receive and Mine dialogs had the same latent flaw with their own popups.

Expected behavior

A "Pending" pill (with an ETA tooltip) shows on the Identity Home hero card, Identities list, and Settings tab. Hint text is legible. The Contacts CTA is clear with no dead controls. Saving a profile shows progress, then exactly one accurate success/error banner that actually clears when the operation finishes — no lingering banners, no false positives. Registering a contested username now shows honest, distinct copy for "registered outright" vs. "submitted, pending a community vote"; the Pending tooltip explains that Dash masternodes vote on the outcome; a pending request counts toward the "Pick a username" checklist step; and the Settings identity badge sits properly aligned beside its heading. The "Create Asset Lock" button now sits below the asset-lock list in every state (loading, empty, and populated), consistent with the rest of the screen. The Fund Platform Address, Receive, and Mine dialogs stay open through address selection regardless of which row is picked, closing only via an explicit Cancel/Close control or the window's own close button.

Detailed discussion

What was done

Feature (first 3 commits on this branch):

  • model/contested_name.rs gains pure pending_username_for/pending_username_in decision logic + a human ETA formatter, surfaced as a "Pending" pill on the Identity Home hero card, Identities list, and Settings tab.
  • Typography::hint(): a new, documented 14px token for instructional hint text, replacing raw .small() (~9px) at the 5 genuine instructional-hint call sites (the other 67 unrelated .small() sites were deliberately left alone).
  • Contacts gate card: CTA reworded; the non-functional "Why?" button and its dead expand/collapse scaffolding removed (confirmed via investigation that it could never actually open its panel).
  • Social-profile save: added progress/success/error banners (previously silent); fixed ProfileCache to update from a save's result, not just a load; added Avatar-URL format/size guidance.
  • Settings identity-type badge: now a read-only accent pill instead of a Button-styled fake pill.

Review fixes (4th commit, addressing claudius:grumpy-review consolidation):

  • SEC-001 (blocking): the PROFILE_SAVING progress banner was never cleared on success or error — it sat on screen forever next to the outcome banner, contradicting it. Now cleared on both paths via a shared clear_profile_saving_banner() helper wired into AppState's generic success/error dispatch.
  • CODE-007 (blocking): the "Your social profile is saved." toast fired on identity-ID match alone, even when on_profile_saved() correctly returned None for a stale result (e.g. switching away from and back to an identity mid-save) — showing a false success toast. Moved inside the on_profile_saved() guard so it only fires for a genuine, just-completed save.
  • CODE-005 (medium): the pending-username lookup rescanned and re-deserialized the entire cached-contest store on every egui repaint across 3 render paths. Added an in-memory AppContext::pending_dpns_usernames cache, refreshed once per contest-cache update, instead of scanning per frame.
  • CODE-006 (medium): an identity with multiple simultaneous pending contests silently showed only whichever one the cache iterator returned first, with no indication another existed. Replaced first-match with a deterministic priority (earliest known decision time, then name as a stable tiebreaker).
  • SEC-002 (low, defense-in-depth): a network-sourced pending username was rendered with no neutralization of Unicode bidi/control characters. Added a sanitizer (strips control + bidi-override chars, clamps to 63 chars) at every render site.
  • PROJ-002/PROJ-003/PROJ-004 (low, copy/catalog): fixed the Contacts CTA grammar/terminology drift, added the missing pill.rs catalog entry to ui/components/README.md, and rewrote the pending-username ETA phrase as complete, i18n-clean sentences instead of a spliced fragment.
  • CALL-001 (info): call-tree walk confirmed the Why? teardown left no orphaned callers — no action needed.

Further fixes (5th–6th commits, addressing additional user-reported issues):

  • Identity Settings badge alignment: the "User identity" pill (already using the shared accent_pill) sat in its own row above "Username," throwing off the Settings columns. It now sits right-aligned beside the "Username" heading, sharing the Home hero's label, tooltip, and accent source.
  • DPNS registration outcome: the registration screen always said "DPNS Name Registered!" even when the submitted name entered contested voting. It now classifies the outcome via the DPNS contract's own contested-vote-poll predicate at submission time (not the separately-refreshed contest cache, which could look stale) and shows outcome-specific copy — "Your username is registered. You can use it now." or "Your username request was submitted. Other people can also request this name, so the community will vote on who receives it. Check the Pending label on your identity for updates."
  • Pending-pill tooltip clarity: every ETA bucket (and the unknown-ETA fallback) now explicitly states that Dash masternodes vote on who receives the contested name, instead of a vague "being confirmed" message.
  • Onboarding checklist credit: a pending (submitted but not yet decided) username registration now completes the "Pick a username" step, with subtext explaining the request is pending while masternodes vote; if the pending request later disappears without a win, the step correctly reverts to incomplete.

Wallet dialog fixes (7th commit, addressing further issues found in live QA of this PR):

  • Fund Platform Address / Receive / Mine dialogs silently closing: each dialog's address selector (a ComboBox popup, or AddressInput's autocomplete overlay) renders as its own floating layer that can extend past the modal window's edge for rows further down the list. The outside-click-to-close check only tested whether a click landed inside the window's own rectangle, so clicking a row that rendered outside it (most likely the last row in a longer list) registered as an "outside click" and force-closed and fully reset the dialog — losing the in-progress selection with no error shown. All three dialogs now rely solely on an explicit Cancel/Close control and the window's native close button; Receive gained a Close button it was missing. selection_dialog.rs, which already handled this correctly, was left untouched and used as the reference pattern. Audited all 14 call sites of this outside-click helper across the app; only these three had a floating popup and needed the change.
  • "Create Asset Lock" button placement: it sat in the Asset Locks card's header row, inconsistent with every other section on this screen (e.g. "+ Add Receiving Address," which sits below its table). Moved below the asset-lock list/empty-state in every state, and switched from a bare button to the project's standard StyledButton.
  • New kittest regressions confirm the last dropdown row keeps the Fund dialog open and selects that address, and that the asset-lock action renders below the empty state.

Testing

  • New/updated unit + kittest coverage across the feature, each review fix, and the further fixes — all green via the narrowest relevant scope, with new/changed test names individually confirmed passing in the verification logs (not just an aggregate count).
  • cargo clippy --all-features --all-targets -- -D warnings clean (and scoped --lib --test kittest re-checks for the further fixes), cargo fmt --all clean.
  • Independently re-verified by security-engineer-smythe and qa-engineer-marvin during grumpy-review consolidation.
  • Wallet dialog fixes: cargo test --test kittest --all-features and cargo clippy --bin dash-evo-tool --all-features -- -D warnings both clean; 260 kittest cases passed including the two new regressions, individually confirmed in the log.

Breaking changes

None.

Checklist

  • Tests added (TDD)
  • i18n-ready, no jargon in user-facing strings
  • No secrets in diff

Prior work

Attribution

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • DPNS username registration now distinguishes between immediate availability and a pending community-voting state.
    • Pending DPNS usernames now show a “Pending” badge across identity views (hero, identity list/name cell, identity cards, settings, and onboarding checklist), including an estimated readiness time when available.
    • Social profile saving now shows clearer progress and success feedback, and saved profile details update immediately.
  • Bug Fixes

    • Pending username UI is safer and correctly formatted (with improved badge tooltip messaging).
    • Profile-saving notifications/banners are dismissed correctly after both success and failure, scoped to the matching identity.

lklimek and others added 4 commits July 21, 2026 09:35
Instructional hint text — the short "what to do / why" line under a
primary label — was rendered with egui's raw RichText::small(), which
resolves to egui's ~9px default and reads too small. This was not using
the app's centralized Typography scale at all.

Add a dedicated Typography::hint() token (SCALE_SM, 14px) so this text
class is pinned to the centralized scale, and migrate the Add-contact
error tips to it. Other .small() call sites (timestamps, tags, incidental
labels) are intentionally left untouched — only genuine instructional
hint text moves to the new token.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A contested DPNS name that was requested but not yet awarded leaves an
identity with empty dpns_names (owned) while its alias is optimistically
set to "name.dash". The Identity Home hero card therefore showed
"No username yet — Pick a username" even though the user had already
chosen a name — indistinguishable from never having requested one.

Distinguish requested-but-unawarded from never-requested and owned:

- Pure detection in model/contested_name.rs: pending_username_for /
  pending_username_in (a contender is pending when the contest is not
  WonBy/Locked) plus an ETA humanizer, approximate_time_until.
- Read-only AppContext wrappers pending_dpns_username_for /
  pending_dpns_usernames over the ongoing-contest cache.
- Reusable "Pending" pill in ui/components/pill.rs (accent_pill extracted
  from the hero's own pill helper), shown on the hero card next to the
  requested @name and in the Identities list Name cell. Tooltip states the
  name is being confirmed and gives an estimated ready time when known.
- Onboarding checklist reflects the pending state — a "being confirmed"
  subtext and no "pick a name" nag button — instead of implying nothing
  was done. Its instructional subtext also adopts Typography::hint().
- New user story DPN-010.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…utton

On the Contacts gate card:

- The primary CTA "Add a display name" is reworded to "Setup display
  profile" — it opens the profile editor, not just a name field.
- The "Why?" button was non-functional: `render_gated` received its
  `why_toggled` response but only had a TODO that never persisted the
  `expanded` flag, so the explanation panel could never open. Rather
  than wire dead UI, remove the button, its panel, and all the now-unused
  scaffolding (WHY_* copy, `expanded`/`with_expanded`, `resolved_why_label`,
  `GateCardAction::WhyToggled`, the `why_toggled` response field).

Tests updated: gate-card unit tests drop the Why?-toggle assertions and
lock the new CTA copy; the IT-CONTACTS-01 integration test asserts the
reworded CTA renders and the "Why?" button no longer does.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d page polish

Fixes the "Save social profile" flow on the Identity Hub Settings tab and
adds the requested guidance/indicators. Log evidence showed the backend
save genuinely succeeds ("Profile created: doc_id=…, revision=1") but the
app gave no feedback and looked unsaved on revisit. Root causes and fixes:

- No confirmation: app.rs routes DashPayProfileUpdated to the screen with
  no generic success banner, and the hub's arm set none. Now it shows a
  success banner. A progress banner is shown on dispatch (no auto-dismiss,
  since the save can take minutes) and is replaced by the success/error
  banner when the task finishes.
- Looked unsaved app-wide / on revisit: the profile cache's record_result
  only consumes LoadProfile results; a save arrives as
  DashPayProfileUpdated(id) with no fields, so the cache kept the pre-save
  profile. `on_profile_saved` now returns the committed fields and the hub
  refreshes the cache via new `ProfileCache::record_saved`, so the hero,
  Contacts gate, and this tab on re-entry all reflect the save. (There is
  no stuck guard: the Save button is purely !invalid && dirty; the "stuck"
  perception was the missing feedback plus the correct post-save disable.)
- Avatar URL field: adds accurate guidance (public square image; JPEG/PNG/
  WebP/GIF; 256×256+ recommended) using the Typography::hint() token.
- Identity-type badge: rendered as a read-only accent pill (matching the
  Home hero card) instead of an input-looking button that read as editable.
- Pending username: when the identity owns no name but has a requested-but-
  unawarded one, shows the requested @name with a "Pending" pill (reusing
  the shared pending-DPNS detection) instead of the register CTA.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@lklimek, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 11 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c3e28c5a-faa7-46e9-83c5-4e04b2913ae4

📥 Commits

Reviewing files that changed from the base of the PR and between e914b5b and 95cb1fa.

📒 Files selected for processing (2)
  • src/backend_task/migration/finish_unwire.rs
  • src/backend_task/migration/v093_upgrade.rs
📝 Walkthrough

Walkthrough

The PR adds DPNS registration outcome classification and pending username tracking, exposes pending status throughout identity UI, updates onboarding and tooltips, and improves DashPay profile-save feedback. It also simplifies the social-profile gate, changes wallet dialog interactions, and updates related tests and documentation.

Changes

DPNS registration and pending-state pipeline

Layer / File(s) Summary
DPNS outcome and pending-state model
src/model/dpns.rs, src/model/contested_name.rs
DPNS registrations now report Registered or PendingCommunityVote; contested-name data produces prioritized, sanitized pending username records with coarse ETA values.
Pending-state cache and backend wiring
src/context/..., src/backend_task/...
AppContext caches pending usernames, refreshes them after contested-resource queries, and exposes filtered lookup methods. Registration results now carry the outcome in a struct-shaped backend result.
Registration validation
tests/backend-e2e/...
Backend tests match the new structured RegisteredDpnsName result shape.

Pending username presentation

Layer / File(s) Summary
Shared pending pill
src/ui/components/pill.rs, src/ui/components/mod.rs, src/ui/components/README.md
Adds reusable accent and pending pill renderers with tooltip text and ETA handling.
Identity and username surfaces
src/ui/identities/..., src/ui/identity/{home.rs,identity_hero_card.rs,settings.rs}
Pending usernames appear in identity lists, hero cards, and Settings; owned handles take precedence over pending requests.
Onboarding and registration results
src/ui/identity/onboarding_checklist.rs, src/ui/identities/register_dpns_name_screen.rs, tests/kittest/register_dpns_name_screen.rs
Pending requests complete the username checklist step without an action button, while registration success copy reflects the classified outcome.

DashPay profile-save feedback

Layer / File(s) Summary
Profile update lifecycle
src/backend_task/mod.rs, src/ui/identity/settings.rs, src/ui/identity/profile_cache.rs, src/ui/identity/hub_screen.rs, src/app.rs
Profile updates receive a dedicated backend context, show a persistent saving banner, clear it on success or error, and immediately update the profile cache after confirmed saves.
Profile update tests
src/app.rs, src/ui/identity/{hub_screen.rs,settings.rs}
Tests cover banner cleanup, stale results, committed profile fields, and cache refresh behavior.

Social-profile gate and instructional UI

Layer / File(s) Summary
Single-action social-profile gate
src/ui/identity/{contacts.rs,social_profile_gate_card.rs}, tests/kittest/identity_hub_contacts.rs
The gate now exposes only “Set up your social profile”; the “Why?” interaction and related state are removed.
Hint typography
src/ui/theme.rs, src/ui/dashpay/add_contact_screen.rs, src/ui/identity/onboarding_checklist.rs
Adds Typography::hint() and applies it to instructional and error guidance text.

Wallet dialog interactions

Layer / File(s) Summary
Explicit wallet dialog controls
src/ui/wallets/wallets_screen/{asset_locks.rs,dialogs.rs}
Asset-lock actions use shared styling, and wallet dialogs now close through explicit controls rather than outside-click handling.
Wallet UI tests
tests/kittest/wallets_screen.rs
Wallet fixtures use randomized zeroized seeds, and funding-dialog tests verify selection remains open until the explicit fund action.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested labels: claudius-review

Suggested reviewers: lklimek

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly captures the main identity-focused changes: pending DPNS indicators, hint-text sizing, and social-profile fixes.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/dpns-pending-pill-and-hint-typography

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lklimek
lklimek marked this pull request as ready for review July 21, 2026 10:11
@thepastaclaw

thepastaclaw commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — 11 ahead in queue (commit e914b5b)
Queue position: 12/23 · 3 reviews active
ETA: start ~14:30 UTC · complete ~14:50 UTC (median 20m across 30 recent reviews; 3 slots)
Queued 20h 36m ago · Last checked: 2026-07-23 13:10 UTC

Ensure social-profile save banners finish cleanly, memoize pending DPNS names, and sanitize pending-name display text.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>
lklimek and others added 2 commits July 21, 2026 13:28
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Classify the DPNS registration outcome (Registered vs PendingCommunityVote)
via the document type's own contested-vote-poll predicate, replacing the
misleading unconditional "DPNS Name Registered!" success panel with
outcome-specific copy. Explain that Dash masternodes vote on contested
names in the Pending pill's tooltip. Credit a pending (submitted but still
contested) registration as completing the "Pick a username" onboarding
step, with voting-status subtext.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/ui/identity/settings.rs (1)

412-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate "owns name → gate pending lookup" logic.

Identical to the block in src/ui/identity/home.rs (lines 282-297). Worth extracting to a shared helper — see the consolidated comment below.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ui/identity/settings.rs` around lines 412 - 427, Extract the duplicated
pending DPNS username gating and lookup from the current settings flow and the
corresponding home flow into a shared helper. Reuse that helper where
pending_username is computed, preserving the existing behavior of skipping the
lookup when any non-empty DPNS name exists and returning None on lookup failure
or absence.
src/ui/identity/home.rs (1)

282-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate "owns name → gate pending lookup" logic.

This block is identical to the one in src/ui/identity/settings.rs (lines 416-427). Extracting a shared helper would remove the duplication and also close a gap in identities_screen.rs, which currently has no equivalent guard.

See the consolidated comment below.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/ui/identity/home.rs` around lines 282 - 297, Extract the shared “identity
owns a non-empty DPNS name, otherwise read pending username” logic into a
reusable helper, then replace the duplicated block in the home and settings
flows with that helper. Reuse the helper in identities_screen.rs so pending
lookup is also gated there, preserving best-effort failure handling and
returning None when a name is already owned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/ui/identities/identities_screen.rs`:
- Around line 531-539: Centralize the ownership check in a shared AppContext
helper, such as pending_dpns_username_for_identity, that returns no pending
username when the QualifiedIdentity already has a non-blank dpns_names entry.
Update src/ui/identities/identities_screen.rs#L531-L539 to apply this guard to
batch results, and replace the duplicated inline lookup logic with the helper in
src/ui/identity/settings.rs#L412-L427 and src/ui/identity/home.rs#L282-L297.

---

Nitpick comments:
In `@src/ui/identity/home.rs`:
- Around line 282-297: Extract the shared “identity owns a non-empty DPNS name,
otherwise read pending username” logic into a reusable helper, then replace the
duplicated block in the home and settings flows with that helper. Reuse the
helper in identities_screen.rs so pending lookup is also gated there, preserving
best-effort failure handling and returning None when a name is already owned.

In `@src/ui/identity/settings.rs`:
- Around line 412-427: Extract the duplicated pending DPNS username gating and
lookup from the current settings flow and the corresponding home flow into a
shared helper. Reuse that helper where pending_username is computed, preserving
the existing behavior of skipping the lookup when any non-empty DPNS name exists
and returning None on lookup failure or absence.
🪄 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: ea1a1775-0faa-411c-9dad-9c80637e0bfa

📥 Commits

Reviewing files that changed from the base of the PR and between 0812761 and 5e97410.

📒 Files selected for processing (29)
  • docs/user-stories.md
  • src/app.rs
  • src/backend_task/contested_names/query_dpns_contested_resources.rs
  • src/backend_task/identity/register_dpns_name.rs
  • src/backend_task/mod.rs
  • src/context/contested_names_db.rs
  • src/context/mod.rs
  • src/model/contested_name.rs
  • src/model/dpns.rs
  • src/ui/components/README.md
  • src/ui/components/mod.rs
  • src/ui/components/pill.rs
  • src/ui/dashpay/add_contact_screen.rs
  • src/ui/identities/identities_screen.rs
  • src/ui/identities/register_dpns_name_screen.rs
  • src/ui/identity/contacts.rs
  • src/ui/identity/home.rs
  • src/ui/identity/hub_screen.rs
  • src/ui/identity/identity_hero_card.rs
  • src/ui/identity/onboarding_checklist.rs
  • src/ui/identity/profile_cache.rs
  • src/ui/identity/settings.rs
  • src/ui/identity/social_profile_gate_card.rs
  • src/ui/theme.rs
  • tests/backend-e2e/dashpay_tasks.rs
  • tests/backend-e2e/framework/fixtures.rs
  • tests/backend-e2e/register_dpns.rs
  • tests/kittest/identity_hub_contacts.rs
  • tests/kittest/register_dpns_name_screen.rs

Comment thread src/ui/identities/identities_screen.rs Outdated
@lklimek lklimek added the claudius-review Triggers automated code review using claudius plugin, runs as a CI job label Jul 21, 2026
lklimek and others added 2 commits July 21, 2026 16:19
…e test fixtures

- Add a short comment at the preorder-document put call site explaining why
  its conflicts intentionally stay generic (its only unique index,
  saltedDomainHash, is unrelated to usernames).
- Replace four copy-pasted DuplicateUniqueIndexError test fixtures (three in
  error.rs, one in register_dpns_name.rs) with a single shared helper in the
  existing src/test_support.rs module.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claudius has reviewed PR #918 — and deigns to be mostly impressed

A tidy, genuinely well-tested piece of work: priority determinism, tie-breaking, bidi/control sanitization, and outcome-copy all have real assertions behind them, the "Why?" teardown left zero orphaned identifiers or duplicated render blocks (a low bar this repo has tripped over before — noted and appreciated), and every changed signature has its callers updated. Four independent reviewers — security, project-consistency, adversarial QA, and docs — converged on the same short list.

2 MEDIUM findings (both inline above, both living in the very feedback/indicator code this PR set out to improve — so worth closing before merge):

  • 🟠 Identities list shows a stale "Pending" pill for a name the identity already owns. Home and Settings guard on ownership; the batch path doesn't. One shared AppContext helper fixes the gap and the copy-paste. (Also the still-open CodeRabbit thread.)
  • 🟠 "Saving…" banner clears on the wrong save — the clear matches on task type, not dispatch_id/identity, so an identity switch mid-save strips the banner owed to an in-flight save.

7 LOW/INFO (optional polish, full detail in the report): a ~230-char success sentence rendered at ui.heading() scale; a Cf zero-width-char gap in the username sanitizer (defense-in-depth, native renderer so no injection surface); a six-fold-duplicated copy sentence; a single-variant action enum left after the "Why?" removal; a public pending_username_in() exercised only by its own tests while production uses a different accumulator; and a README catalog row that omits the Identities-list consumer.

No security blockers, no correctness landmines, no secrets. Address the two ambers and this is ready. The empire approves of the trajectory. 🏆

🤖 Consolidated from a 4-agent grumpy-review. Full severity-ranked report (HTML + JSON) generated for the maintainers.
📊 View full HTML review report

Comment thread src/ui/identities/identities_screen.rs Outdated
Comment thread src/app.rs
@github-actions github-actions Bot removed the claudius-review Triggers automated code review using claudius plugin, runs as a CI job label Jul 21, 2026
lklimek added 4 commits July 22, 2026 07:58
Two review findings from PR #918 confirmed still open at HEAD:

- The Identities list's batch pending-DPNS lookup had no ownership guard,
  unlike Home and Settings, so it could show a stale "Pending" pill for a
  name that had already been awarded. Added
  `AppContext::pending_dpns_username_for_identity` and a batch
  `pending_dpns_usernames_for_identities`, and switched all three call
  sites to the shared, ownership-aware helper.

- The "Saving your social profile…" progress banner cleared on *any*
  DashPay profile-update result, so two saves in flight for different
  identities could have one identity's completion silently swallow the
  other's feedback. `BackendTaskContext::DashPayProfileUpdate` now
  carries the identity id; the clear helpers and
  `SettingsTab::clear_pending_save` only act when the result's identity
  matches the banner's/snapshot's owner.

Regression tests added for both: ownership-filtered batch lookup, and
cross-identity banner/pending-save isolation on both the success and
error paths.

Verified via cargo-cached.sh: `test --lib --all-features` (7 named
tests passing), `clippy --all-features --all-targets -- -D warnings`
(clean).

Resolves review threads:
#918 (comment)
#918 (comment)
#918 (comment)

<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Keep Receive, Fund Platform Address, and Mine dialogs open while their floating child widgets are in use. Move the styled Create Asset Lock action below the asset-lock content in every state and add UI regressions.\n\nAddresses user-reported issues on PR #918.\n\nCo-Authored-By: OpenAI Codex <noreply@openai.com>
@lklimek
lklimek enabled auto-merge (squash) July 22, 2026 15:33
@lklimek
lklimek disabled auto-merge July 22, 2026 15:33
@lklimek lklimek added the claudius-review Triggers automated code review using claudius plugin, runs as a CI job label Jul 22, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claudius has re-reviewed PR #918 — and pronounces it ready

The empire is pleased. This pass (HEAD 44485830) confirms the two MEDIUM findings from the previous round are genuinely fixed, not merely papered over:

  • 🟢 Stale "Pending" pill — the batch path now routes through the ownership-aware pending_dpns_username_for_identity / pending_dpns_usernames_for_identities helpers, and the regression test exercises a mixed owned/unowned batch rather than a vacuous single-identity case. Verified.
  • 🟢 "Saving…" banner clearing on the wrong saveBackendTaskContext::DashPayProfileUpdate now carries the identity id, and the clear helpers key on it. The new tests stand up two concurrent per-identity banners and prove clearing A's leaves B's alone. A proper race-shaped test, not a happy path.

The newest commit — the wallet-dialog popup-preservation fix — is a clean, complete fix across all three dialogs (Receive / Fund Platform / Mine): outside-click-to-close removed only where a floating popup layer could extend past the modal rect, every dialog keeps an explicit Close/Cancel/native path, and the shared guard stays live for the ~10 modals that legitimately want it. No dead code, no stranded dialogs, no fund-path logic touched.

Three independent reviewers (security · project-consistency · adversarial-QA) converged. No security, correctness, or fund-safety blockers. No secrets.

6 LOW findings — optional polish, none blocking

  • 🔵 Profile-save confirmation gap (hub_screen.rs:784) — handle_profile_updated gates the "saved" toast + cache write on the currently-selected identity rather than the task's own carried id. Switch identities mid-save and the completed save gets no confirmation (self-heals on cache-reset nav paths). The banner-clear fix already threads the carried id — the cache write could ride the same rail (it's keyed by saved_id anyway), leaving only the on-screen banner gated to selection.
  • 🔵 Receive/Mine dialogs lack the popup regression test — the fix covers all three, but only Fund Platform got a test. A future one-dialog regression would keep CI green.
  • 🔵 pending_username_in (contested_name.rs:89) — public, zero production callers, exercised only by its own tests; production runs the plural accumulator. (Recurring — flagged last round.)
  • 🔵 PendingCommunityVote success copy (register_dpns_name_screen.rs:326) — a ~185-char paragraph handed to ui.heading(). Short title in the heading, paragraph in the body slot. (Recurring.)
  • 🔵 Duplicated "Create Asset Lock" button block (asset_locks.rs:92/186) — byte-identical across two branches; extract a closure before it drifts.
  • 🔵 pill.rs catalog row (components/README.md:33) — still omits the Identities-list consumer, and disagrees with user-stories.md DPN-010 on which surfaces use the pill. (Recurring.)

None of these gate merge. Three of them are prior LOW items that survived — worth a mop-up commit if the mood strikes, but the empire will not withhold its blessing over them.

Approved. Ship it. 🏆

🤖 Consolidated from a 3-agent grumpy-review (0 security, 4 project, 2 QA → 6 LOW after dedup/severity re-assessment). Full HTML + JSON report generated for maintainers.
📊 View full HTML review report

@github-actions github-actions Bot removed the claudius-review Triggers automated code review using claudius plugin, runs as a CI job label Jul 22, 2026
…og row

The `pending_username_pill` renderer gained a third consumer in this PR —
`identities_screen.rs` — alongside `IdentityHeroCard` and Settings, but the
components catalog row still named only the first two. pill.rs's own module
doc and user-stories DPN-010 already count the Identities list, so the catalog
was the odd one out. Bring it in line with the code it describes.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/backend_task/mod.rs (1)

1320-1327: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not commit a plaintext HD seed in the test.

[0x7b; 64] is a seed literal committed to source. Generate test-only bytes at runtime and retain them in Zeroizing.

Proposed fix
+        let hd_seed = Zeroizing::new(rand::random::<[u8; 64]>());
         secret_access.remember_session(
             &scope,
-            SecretPlaintext::HdSeed(&Zeroizing::new([0x7b; 64])),
+            SecretPlaintext::HdSeed(&hd_seed),
             RememberPolicy::UntilAppClose,
         );

As per coding guidelines, “Never commit plaintext recovery phrases, private keys, passwords, seeds, or API tokens in source, tests, fixtures, or documentation.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend_task/mod.rs` around lines 1320 - 1327, Replace the hard-coded
`[0x7b; 64]` value in the `secret_access.remember_session` test with bytes
generated at runtime using an appropriate test-only generator, and keep the
generated seed wrapped in `Zeroizing`. Preserve the existing
`SecretPlaintext::HdSeed` and `RememberPolicy::UntilAppClose` behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/context/contested_names_db.rs`:
- Around line 260-286: Update the primary DPNS name selection in the identity
settings flow to skip entries whose name is blank after trimming whitespace.
Replace the direct first-entry lookup on identity.dpns_names with a
nonblank-aware search, preserving the existing pending-username fallback when no
displayed awarded name exists.

---

Outside diff comments:
In `@src/backend_task/mod.rs`:
- Around line 1320-1327: Replace the hard-coded `[0x7b; 64]` value in the
`secret_access.remember_session` test with bytes generated at runtime using an
appropriate test-only generator, and keep the generated seed wrapped in
`Zeroizing`. Preserve the existing `SecretPlaintext::HdSeed` and
`RememberPolicy::UntilAppClose` behavior.
🪄 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: ebf4c64e-e458-445f-878c-6a3f21453d17

📥 Commits

Reviewing files that changed from the base of the PR and between 5e97410 and e914b5b.

📒 Files selected for processing (15)
  • docs/user-stories.md
  • src/app.rs
  • src/backend_task/error.rs
  • src/backend_task/identity/register_dpns_name.rs
  • src/backend_task/mod.rs
  • src/context/contested_names_db.rs
  • src/test_support.rs
  • src/ui/components/README.md
  • src/ui/identities/identities_screen.rs
  • src/ui/identity/home.rs
  • src/ui/identity/hub_screen.rs
  • src/ui/identity/settings.rs
  • src/ui/wallets/wallets_screen/asset_locks.rs
  • src/ui/wallets/wallets_screen/dialogs.rs
  • tests/kittest/wallets_screen.rs
🚧 Files skipped from review as they are similar to previous changes (8)
  • src/ui/components/README.md
  • docs/user-stories.md
  • src/backend_task/identity/register_dpns_name.rs
  • src/app.rs
  • src/ui/identities/identities_screen.rs
  • src/ui/identity/home.rs
  • src/ui/identity/hub_screen.rs
  • src/ui/identity/settings.rs

Comment on lines +260 to +286
/// Return a pending DPNS username only while `identity` owns no awarded name.
pub fn pending_dpns_username_for_identity(
&self,
identity: &QualifiedIdentity,
) -> Option<PendingUsername> {
if identity_owns_dpns_name(identity) {
None
} else {
self.pending_dpns_username_for(&identity.identity.id())
.ok()
.flatten()
}
}

/// Map identities without an awarded name to their pending DPNS usernames.
pub fn pending_dpns_usernames_for_identities(
&self,
identities: &[QualifiedIdentity],
) -> HashMap<Identifier, PendingUsername> {
let identity_ids = identities
.iter()
.filter(|identity| !identity_owns_dpns_name(identity))
.map(|identity| identity.identity.id())
.collect::<Vec<_>>();
self.pending_dpns_usernames(&identity_ids)
.unwrap_or_default()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Ignore blank DPNS entries when selecting the displayed primary name.

These wrappers correctly return pending data for whitespace-only names, but src/ui/identity/settings.rs:414-558 checks identity.dpns_names.first() without the same nonblank filter. It renders an empty primary username instead of reaching the pending branch.

Update that primary-name selection to filter blank names, e.g. iter().find(|name| !name.name.trim().is_empty()).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/context/contested_names_db.rs` around lines 260 - 286, Update the primary
DPNS name selection in the identity settings flow to skip entries whose name is
blank after trimming whitespace. Replace the direct first-entry lookup on
identity.dpns_names with a nonblank-aware search, preserving the existing
pending-username fallback when no displayed awarded name exists.

`a_second_launch_after_an_unreadable_identity_preserves_user_edits_and_deletions`
called `delete_local_qualified_identity` immediately after the first
migration `run()` returned. `run()` detaches its best-effort DAPI refresh
onto a spawned task that queues for the `migration_run` guard right behind
the caller (see `spawn_dapi_refresh`), and `delete_local_qualified_identity`
claims that same guard via `try_lock`. Under CI's parallel/loaded test
execution the detached task sometimes won the race, so the delete call
transiently failed with `WalletStorageNotReady` — this PR's Test Suite job
went red on it (2079 passed, 1 failed), while the test passed reliably in
isolation.

Promote `finish_unwire`'s existing `wait_for_dapi_refresh` test helper
(already used to close this exact race in two other tests in the same
file) from private-to-`mod tests` to `pub(crate)` at module scope, and call
it from the v093_upgrade regression before the delete — same fix pattern,
reused instead of re-derived.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@lklimek
lklimek enabled auto-merge (squash) July 23, 2026 09:00
@lklimek
lklimek merged commit 7e80e60 into v1.0-dev Jul 23, 2026
5 checks passed
@lklimek
lklimek deleted the feat/dpns-pending-pill-and-hint-typography branch July 23, 2026 09:06
orchardpaytl pushed a commit to orchardpaytl/orchardpay that referenced this pull request Jul 24, 2026
…, and social-profile fixes (dashpay#918)

* feat(ui): add Typography::hint() token for instructional text

Instructional hint text — the short "what to do / why" line under a
primary label — was rendered with egui's raw RichText::small(), which
resolves to egui's ~9px default and reads too small. This was not using
the app's centralized Typography scale at all.

Add a dedicated Typography::hint() token (SCALE_SM, 14px) so this text
class is pinned to the centralized scale, and migrate the Add-contact
error tips to it. Other .small() call sites (timestamps, tags, incidental
labels) are intentionally left untouched — only genuine instructional
hint text moves to the new token.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* feat(identity): surface pending DPNS username registrations

A contested DPNS name that was requested but not yet awarded leaves an
identity with empty dpns_names (owned) while its alias is optimistically
set to "name.dash". The Identity Home hero card therefore showed
"No username yet — Pick a username" even though the user had already
chosen a name — indistinguishable from never having requested one.

Distinguish requested-but-unawarded from never-requested and owned:

- Pure detection in model/contested_name.rs: pending_username_for /
  pending_username_in (a contender is pending when the contest is not
  WonBy/Locked) plus an ETA humanizer, approximate_time_until.
- Read-only AppContext wrappers pending_dpns_username_for /
  pending_dpns_usernames over the ongoing-contest cache.
- Reusable "Pending" pill in ui/components/pill.rs (accent_pill extracted
  from the hero's own pill helper), shown on the hero card next to the
  requested @name and in the Identities list Name cell. Tooltip states the
  name is being confirmed and gives an estimated ready time when known.
- Onboarding checklist reflects the pending state — a "being confirmed"
  subtext and no "pick a name" nag button — instead of implying nothing
  was done. Its instructional subtext also adopts Typography::hint().
- New user story DPN-010.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(contacts): reword social-profile CTA and remove the dead "Why?" button

On the Contacts gate card:

- The primary CTA "Add a display name" is reworded to "Setup display
  profile" — it opens the profile editor, not just a name field.
- The "Why?" button was non-functional: `render_gated` received its
  `why_toggled` response but only had a TODO that never persisted the
  `expanded` flag, so the explanation panel could never open. Rather
  than wire dead UI, remove the button, its panel, and all the now-unused
  scaffolding (WHY_* copy, `expanded`/`with_expanded`, `resolved_why_label`,
  `GateCardAction::WhyToggled`, the `why_toggled` response field).

Tests updated: gate-card unit tests drop the Why?-toggle assertions and
lock the new CTA copy; the IT-CONTACTS-01 integration test asserts the
reworded CTA renders and the "Why?" button no longer does.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(identity-settings): social-profile save feedback, persistence, and page polish

Fixes the "Save social profile" flow on the Identity Hub Settings tab and
adds the requested guidance/indicators. Log evidence showed the backend
save genuinely succeeds ("Profile created: doc_id=…, revision=1") but the
app gave no feedback and looked unsaved on revisit. Root causes and fixes:

- No confirmation: app.rs routes DashPayProfileUpdated to the screen with
  no generic success banner, and the hub's arm set none. Now it shows a
  success banner. A progress banner is shown on dispatch (no auto-dismiss,
  since the save can take minutes) and is replaced by the success/error
  banner when the task finishes.
- Looked unsaved app-wide / on revisit: the profile cache's record_result
  only consumes LoadProfile results; a save arrives as
  DashPayProfileUpdated(id) with no fields, so the cache kept the pre-save
  profile. `on_profile_saved` now returns the committed fields and the hub
  refreshes the cache via new `ProfileCache::record_saved`, so the hero,
  Contacts gate, and this tab on re-entry all reflect the save. (There is
  no stuck guard: the Save button is purely !invalid && dirty; the "stuck"
  perception was the missing feedback plus the correct post-save disable.)
- Avatar URL field: adds accurate guidance (public square image; JPEG/PNG/
  WebP/GIF; 256×256+ recommended) using the Typography::hint() token.
- Identity-type badge: rendered as a read-only accent pill (matching the
  Home hero card) instead of an input-looking button that read as editable.
- Pending username: when the identity owns no name but has a requested-but-
  unawarded one, shows the requested @name with a "Pending" pill (reusing
  the shared pending-DPNS detection) instead of the register CTA.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(ui): resolve DPNS pending state review findings

Ensure social-profile save banners finish cleanly, memoize pending DPNS names, and sanitize pending-name display text.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

* fix(identity-settings): align identity badge with heading

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

* fix(dpns): explain contested-username voting to registering users

Classify the DPNS registration outcome (Registered vs PendingCommunityVote)
via the document type's own contested-vote-poll predicate, replacing the
misleading unconditional "DPNS Name Registered!" success panel with
outcome-specific copy. Explain that Dash masternodes vote on contested
names in the Pending pill's tooltip. Credit a pending (submitted but still
contested) registration as completing the "Pick a username" onboarding
step, with voting-status subtext.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

* refactor(platform-errors): document preorder rebrand exemption, dedupe test fixtures

- Add a short comment at the preorder-document put call site explaining why
  its conflicts intentionally stay generic (its only unique index,
  saltedDomainHash, is unrelated to usernames).
- Replace four copy-pasted DuplicateUniqueIndexError test fixtures (three in
  error.rs, one in register_dpns_name.rs) with a single shared helper in the
  existing src/test_support.rs module.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(identity): scope pending-DPNS and profile-save status to identity

Two review findings from PR dashpay#918 confirmed still open at HEAD:

- The Identities list's batch pending-DPNS lookup had no ownership guard,
  unlike Home and Settings, so it could show a stale "Pending" pill for a
  name that had already been awarded. Added
  `AppContext::pending_dpns_username_for_identity` and a batch
  `pending_dpns_usernames_for_identities`, and switched all three call
  sites to the shared, ownership-aware helper.

- The "Saving your social profile…" progress banner cleared on *any*
  DashPay profile-update result, so two saves in flight for different
  identities could have one identity's completion silently swallow the
  other's feedback. `BackendTaskContext::DashPayProfileUpdate` now
  carries the identity id; the clear helpers and
  `SettingsTab::clear_pending_save` only act when the result's identity
  matches the banner's/snapshot's owner.

Regression tests added for both: ownership-filtered batch lookup, and
cross-identity banner/pending-save isolation on both the success and
error paths.

Verified via cargo-cached.sh: `test --lib --all-features` (7 named
tests passing), `clippy --all-features --all-targets -- -D warnings`
(clean).

Resolves review threads:
dashpay#918 (comment)
dashpay#918 (comment)
dashpay#918 (comment)

<sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>

* fix(wallets): preserve popup-bearing dialogs (dashpay#918)

Keep Receive, Fund Platform Address, and Mine dialogs open while their floating child widgets are in use. Move the styled Create Asset Lock action below the asset-lock content in every state and add UI regressions.\n\nAddresses user-reported issues on PR dashpay#918.\n\nCo-Authored-By: OpenAI Codex <noreply@openai.com>

* docs(components): list the Identities-list consumer in the pill catalog row

The `pending_username_pill` renderer gained a third consumer in this PR —
`identities_screen.rs` — alongside `IdentityHeroCard` and Settings, but the
components catalog row still named only the first two. pill.rs's own module
doc and user-stories DPN-010 already count the Identities list, so the catalog
was the odd one out. Bring it in line with the code it describes.

Co-Authored-By: Claude Opus <noreply@anthropic.com>

* test(migration): fix flaky race in unreadable-identity regression test

`a_second_launch_after_an_unreadable_identity_preserves_user_edits_and_deletions`
called `delete_local_qualified_identity` immediately after the first
migration `run()` returned. `run()` detaches its best-effort DAPI refresh
onto a spawned task that queues for the `migration_run` guard right behind
the caller (see `spawn_dapi_refresh`), and `delete_local_qualified_identity`
claims that same guard via `try_lock`. Under CI's parallel/loaded test
execution the detached task sometimes won the race, so the delete call
transiently failed with `WalletStorageNotReady` — this PR's Test Suite job
went red on it (2079 passed, 1 failed), while the test passed reliably in
isolation.

Promote `finish_unwire`'s existing `wait_for_dapi_refresh` test helper
(already used to close this exact race in two other tests in the same
file) from private-to-`mod tests` to `pub(crate)` at module scope, and call
it from the v093_upgrade regression before the delete — same fix pattern,
reused instead of re-derived.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Codex GPT-5 <noreply@openai.com>
lklimek added a commit that referenced this pull request Jul 31, 2026
* fix(wallets): use selector ceiling for asset-lock Max

Query the live asset-lock TransactionBuilder for the largest credit output
its final-input coin selection accepts, and reuse that ceiling for Max and
pre-send validation across Shield and identity wallet-funding flows.

Keep snapshot balances display-only and add real-selector regressions for
unconfirmed funds that inflate the UI subtotal.

Fixes #929

Co-Authored-By: Codex GPT-5 <noreply@openai.com>

* docs: changelog entry for #929 Max fix

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(wallets): use the real height watermark in the asset-lock ceiling probe

Independent architecture audit of 3493f71 found the Max/validation dry-run
probe read `synced_height()` while the real `create_asset_lock_proof` path
reads `last_processed_height()` — a different, independently-advanced
watermark. Diverging by enough blocks could sweep a live in-flight UTXO
reservation via the coinbase-maturity/TTL logic, a real side effect from a
supposedly read-only probe. Route the probe through the same accessor the
real path uses.

Also give `AssetLockBalanceCache` a per-wallet publish generation so a cached
Max ceiling is rejected and re-queried after the wallet snapshot changes,
instead of only on screen refresh.

The hand-duplicated `ASSET_LOCK_FEE_PER_KB` constant stays: verified against
the pinned `rs-platform-wallet` source that the real path's
`DEFAULT_FEE_PER_KB` is `pub(super)` and not reachable from DET, and that
DET's existing `FeeRate::normal()` usage elsewhere (ordinary Core sends) is
an unrelated crate-level constant that only coincidentally matches today.
No importable shared source exists yet; comment updated to say so plainly
and track it as an upstream ask.

Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* refactor(wallets): replace the asset-lock ceiling binary search with a seeded probe

The prior implementation binary-searched [0, MAX_MONEY] against the real
selection strategy — correct, but ~34-51 real TransactionBuilder builds per
Max query, each holding the wallet's global async write lock.

A first attempt at trimming this hand-derived a "68-byte" padding correction
by adding placeholder credit outputs until tests passed. Rejected on review:
independently checked against the vendored `key-wallet` coin-selector and
could only confirm a single 34-byte change-output assumption in
`branch_and_bound_with_size`'s fallback path, not two, and DET's own
`create_asset_lock_proof` never touches `TransactionBuilder` directly (it's
wrapped inside `platform-wallet`'s `AssetLockManager`), so there was no local
ground truth to confirm the number generalized. A constant reverse-engineered
against RED tests until they pass is exactly the failure mode this whole
review has been catching.

Replaced with a seed-then-bisect approach that never assumes anything about
the real strategy's internal fee/size formula:
- One `SelectionStrategy::All` drain call gives a fast, verified-correct
  upper bound (the builder already drops the change address for `All`
  before sizing).
- Exponential search downward from that seed using the real default
  strategy (`BranchAndBound`, matching `create_asset_lock_proof`), then a
  short bisection to the exact boundary. Typically ~15 calls instead of
  ~34-51, all against the real code, none against a guessed formula.
- `TooManyInputs` on the seed call (more spendable UTXOs than fit one
  transaction — `All` can never succeed there) falls back to the original
  full-range search unchanged, preserving old behavior for that edge case.

All three regression tests pass with assertions unchanged, proving the
seeded result is exact:
wallet_backend::payments::tests::asset_lock_max_excludes_unconfirmed_funds_counted_by_snapshot
wallet_backend::payments::tests::asset_lock_max_uses_last_processed_height_when_sync_watermarks_diverge
ui::wallets::send_screen::tests::core_asset_lock_max_and_validation_use_builder_quote

Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(wallets): close asset-lock ceiling gaps found by CI review

Fixes four confirmed issues in the Max-ceiling probe from CI review of
PR #937 (github-actions[bot]):

- A wallet with more spendable UTXOs than fit in one transaction
  (TooManyInputs) collapsed the Max quote to a hard 0 instead of finding
  the largest amount achievable from an in-cap subset. The drain-ceiling
  seed and the full-range fallback now both route through an input-cap-aware
  search: a LargestFirst probe finds a real achievable seed, nudged by one
  well-known P2PKH input's fee as a starting point, then verified/refined
  against the actual default (BranchAndBound) strategy via real builder
  calls — never a guessed formula.
- The drain-ceiling probe's placeholder credit-output script was 0 bytes
  where the real path produces a ~25-byte P2PKH script, skewing the fee
  estimate (and therefore the quoted ceiling) in the unsafe direction.
- The asset-lock balance cache discarded its last loaded value on every
  wallet-snapshot generation bump and had no in-flight guard, so during
  active sync the funding screens could get stuck on "Checking the
  available amount..." indefinitely. It now keeps serving the last loaded
  value while a refresh runs in the background (stale-while-revalidate)
  and never dispatches a second probe while one is already in flight.
- SnapshotStore::publish cloned the whole WalletSnapshot (transaction/UTXO
  history) inside its rcu closure instead of bumping an Arc refcount, on
  every wallet event. Generation is now assigned once via a dedicated
  per-wallet counter before entering the closure, which only clones the
  Arc; an ordering guard prevents a stale generation from overwriting a
  newer one under retry.

New test: asset_lock_max_uses_an_in_cap_subset_when_the_wallet_has_too_many_utxos
(517 UTXOs, verifies the quoted max builds within the input cap and
one duff more does not).

Co-Authored-By: Codex Sol (gpt-5.6-sol) <noreply@openai.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): migrate remaining asset-lock funding flows to the builder ceiling

Extends the #929 Max fix to every screen/funding-method that dispatches
through the same asset-lock builder chokepoint but was left reading the
display-only snapshot balance, per CI review of PR #937:

- Fund Platform Address (Core wallet -> Platform address) now computes
  Max and validates the pre-send amount against the builder ceiling,
  same as Shield and Identity destinations already did.
- The ReceiveDeposit funding method on both Create Identity and Top Up
  Identity now shares the same ceiling cache as UseWalletBalance, since
  both dispatch through the identical wallet-level asset-lock builder
  downstream. Removes the separate funding_address_balance_duffs-based
  check that could accept an amount the builder would reject.
- Pre-selection affordability gates (wallet_can_afford_creation,
  wallet_balance_can_afford_top_up, and the funding-method-availability
  checks) now read the same ceiling as the "not enough Dash" banner they
  claim to match, instead of the older, larger snapshot figure. An
  unloaded ceiling does not block the option.
- The "You can use X DASH" balance headline on both funding screens now
  reads the builder ceiling once loaded, falling back to the snapshot
  figure only while it's still loading -- it no longer shows a number
  larger than what Max/validation will actually accept.
- A failed background asset-lock-max probe no longer raises a duplicate
  global error banner or resets in-flight wizard step state (confirmed:
  without this, a probe failure invoked step_after_task_failure on the
  identity screens and reset send_status on the send screen, mid-wizard).
  should_suppress_backend_task_error now suppresses it on all three
  screens; the existing inline "could not be checked" / Retry UI is the
  right surface for this background query.

Extends core_asset_lock_max_and_validation_use_builder_quote with a
Platform-destination case (Max computation and exact-boundary rejection).

Co-Authored-By: Codex Sol (gpt-5.6-sol) <noreply@openai.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: correct #929 changelog entry to match the fixed scope

The entry claimed Max "always produces an amount that goes through" and
omitted the Fund Platform Address flow (now also fixed), and reused
"spendable" -- the exact term the code comments demote in favor of the
builder-verified ceiling. Names all four fixed flows, drops the absolute
claim, and mentions the new checking/retry UI states.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs(wallet_backend): document accepted risk from upstream rust-dashcore#918

The asset-lock max-amount seed-then-bisect probes deliberately walk toward
the wallet's near-total balance, which is exactly the regime where
key-wallet's BranchAndBound find_exact_match lacks an undershoot/feasibility
prune (filed upstream, not fixed in this repo). Document the accepted risk
at the shared dry-run chokepoint pending the upstream fix, per user
direction, rather than bounding the probe here.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet_backend): shrink asset-lock probe's write-lock scope, document reservation sharing

SEC-001: asset_lock_max_amount held the wallet-manager write lock across
its entire synchronous seed-then-bisect search, causing SnapshotStore's
try_read() to fall back to carry-forward publishes that bump the
generation and discard the probe's own result -- a self-sustaining
invalidation loop. Shrink the lock to just the initial read.

CODE-002: document why dry_run_asset_lock_amount_with_strategy's
release_reservation calls are load-bearing (cloning ManagedCoreFundsAccount
shares its live ReservationSet) and stop calling the path "read-only" when
it mutates shared reservation state.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): harden AssetLockBalanceCache against generation regression and stuck in-flight requests

SEC-004: a wallet whose snapshot-generation counter restarts (e.g. after
SnapshotStore::forget_wallet + re-registration) was permanently frozen out
of future asset-lock-max queries once the cache had observed a higher
generation, comparing forever against the stale high-water mark.

SEC-005: an in-flight request whose result never routed back to store()/
mark_loading_failed() permanently blocked every future query for that
wallet regardless of how far the real snapshot generation advanced.
Confirmed trigger: in both identity receive-deposit flows, ensure_requested()
marks the probe in flight, then a same-frame AppAction replacement (the
end-of-frame tracked-lock/receive-address batch) discards that dispatch
before it ever reaches the backend -- the probe never runs, and nothing
ever calls store()/mark_loading_failed() to release it.

Fix: a snapshot-generation change now always supersedes stale in-flight/
failed tracking for the wallet -- a higher generation clears the stale
markers while preserving the last displayable amount (the refresh-in-
progress case), and a lower generation (a genuine counter restart) also
discards the now-untrustworthy cached amount. Equal-generation requests
still dedupe, and a stale response for an old generation is still ignored
once a newer generation has superseded it.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): reserve fees for Identity sends, bound ReceiveDeposit by its address, fix trapped loading state

SEC-002: send_screen's Max/validation for a Core -> Identity send reserved
no fee at all (missing AddressKind::Identity match arm), and the Platform/
Identity submit-time validations checked the raw probe ceiling instead of
the same fee-reserved ceiling their own display path already computes --
a manually typed amount (not using the Max button) could pass validation
with no fee headroom. Both submit paths now recompute the same fee their
display arm uses (identity top-up fee, Platform funding-transition fee),
rounding the credits->duffs conversion up so the reserve is never
understated.

SEC-003: the ReceiveDeposit funding method's Max ceiling used the wallet-
wide live-builder probe instead of the specific deposit address's balance,
letting the input accept more than actually arrived at that address.
Added receive_deposit_ceiling_duffs() (funding_common.rs) bounding by
min(wallet probe, deposit-address balance); UseWalletBalance is unaffected.

PROJ-002: the receive-deposit flow's "Choose a different funding method"
escape hatch was unreachable while the probe was loading, had failed, or
the wallet seed hash was momentarily unavailable -- contradicting its own
doc comment ("the user is never trapped"). The button now renders on every
path through the FundsReceived step, in both identity screens.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* test(send_screen): seed the asset-lock cache in the Core->Platform kittest route

CODE-001: the offline kittest harness never populates AssetLockBalanceCache
(no real backend), so send_core_to_platform's new pre-flight probe check
("still being checked") short-circuited before the test's actual balance
assertion, and that assertion's expected wording was stale besides (from
before this PR's validate_asset_lock_amount rewrite).

Added a #[cfg(feature = "testing")] hook, seed_asset_lock_max_amount_for_test,
that seeds the cache via its own public ensure_requested/store API (no
reaching into private fields), threaded an optional seed amount through
assert_route(), and updated the one affected test to assert the current
"You can transfer up to ..." message. Audited every other CoreWallet-sourced
kittest route: none of them hit the Platform/Shielded/Identity destination
guard that triggers the probe, so no other test needed seeding.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: correct the Max-fix changelog entry and add its user story

PROJ-003: the CHANGELOG's "Max now matches..." entry (3rd attempt) still
didn't match the shipped UI: it used an ASCII "..." instead of the actual
unicode ellipsis, named a generic "Retry" button instead of the real label
("Retry available amount check"), and omitted both the Identity-destination
Send-screen case (SEC-002) and the receive-deposit address bound (SEC-003)
now that they're fixed. Corrected against the actual strings in
send_screen.rs and both by_receive_deposit.rs/by_using_unused_balance.rs
pairs (all 5 call sites verified consistent).

PROJ-006: added SND-017 documenting the live-builder-verified Max/amount-
check behavior across Shield, Fund Platform Address, send-to-Identity, and
identity funding -- this had no user story despite being a materially
different mechanism from SND-014's Core-to-Core Max (simple network-fee
subtraction). IDN-014's existing acceptance criteria (deposit-bounded
prefill, never-trapped funding-method switch) already documented the
*intended* behavior correctly; Batch 3 of this remediation made the code
finally live up to it, so IDN-014 itself needed no changes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): reserve the identity fee at dispatch-time validation, not just display

PROJ-001: both identity funding screens' dispatch-time validation called
validate_asset_lock_amount(amount, 0, max_amount) -- a literal zero fee
reserve -- even though their own Max button (and the equivalent send_screen
paths fixed in 64cd544) already reserve the fee for display. This is a
regression against origin/v1.0-dev, not just an unfinished threading: the
pre-PR code at this exact dispatch point did reserve the fee
(max_amount_after_fee_reserve(spendable_duffs, fee_credits)) and this PR's
migration deleted that check. A manually typed amount between the fee-
reserved display ceiling and the raw builder ceiling could pass validation
and commit to an under-funded identity creation/top-up that then fails on
Platform after the asset lock is already on L1.

Both sites now recompute the same fee their display path uses
(estimate_identity_create/estimate_identity_topup), rounding credits->duffs
up so the reserve is never understated. AssetLockAmountError::Overflow is
now reachable at these sites as a side effect (previously dead with reserve
always 0).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): debounce the asset-lock probe on no-op events, stop dropping its dispatch

PROJ-007: SyncHeightAdvanced/ChainLockProcessed carry no UTXO deltas but
still bump the snapshot generation, and Batch 2's generation-supersession
fix (d02fd53) treats every bump as a reason to cancel and restart the
live-builder probe -- during active sync this can mean the (potentially
slow, per rust-dashcore#918) probe never completes. Debounce on the
wallet's spendable balance (AssetLockBalanceCache now tracks
request_spendable_duffs alongside request_generation): a generation bump
with no spendable-balance change is a no-op for dispatch purposes, while
a generation regression still forces a reload as before.

PROJ-008: a probe dispatch merged via `action |= AppAction::BackendTask(..)`
could be silently overwritten by a later same-frame action (BitOrAssign is
last-write-wins), with no recovery on an idle wallet whose generation never
advances. Both identity funding screens (add_new_identity_screen,
top_up_identity_screen) now collect the probe dispatch, tracked-asset-lock
fetches, and receive-address generation into a single end-of-frame
AppAction::BackendTasks(..., Concurrent) batch via new funding_common
helpers (can_append_concurrent_backend_tasks, append_concurrent_backend_tasks)
instead of relying on overwrite semantics. Fixed call sites: both
by_receive_deposit.rs and both by_using_unused_balance.rs (dispatch moved to
the parent mod.rs render loop, gated on which funding method actually
rendered this frame).

Independently reviewed the full diff and re-ran fmt/test/clippy myself
before committing (Codex Sol's own commit attempt failed on the sandboxed
worktree's read-only git metadata, as expected).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(ui): bound ReceiveDeposit dispatch by its address, fix the debounce cache, invalidate on network switch

Holistic review of the full PR #937 diff found 3 blocking regressions
inside this PR's own remediation commits, invisible to per-commit
review. All 3 independently re-verified by the coordinator (direct
code reads + diff review) before and after this fix.

SEC-003: ReceiveDeposit dispatch-time validation checked only the
wallet-wide asset-lock ceiling, contradicting the deposit-address bound
CHANGELOG.md and user story SND-017 both promise unconditionally (and
that the display-path Max button already enforced). Both identity
screens now compute the ceiling through one shared
`available_ceiling_duffs()` helper used by both the Max button and
dispatch validation, so the two paths cannot diverge again.

Debounce/dedup logic (SEC-004 + PROJ-008 + QA-001, all describing two
flaws in `AssetLockBalanceCache::ensure_requested`):
- The debounce signal (spendable = confirmed+unconfirmed) could not see
  a UTXO's unconfirmed-to-confirmed/InstantLock transition, since the
  builder only accepts confirmed-or-instantlocked inputs
  (require_final_inputs) -- exactly the transition the ReceiveDeposit
  flow depends on. Replaced with a real final-funds subtotal
  (`asset_lock_final_funds_duffs`, mirroring the builder's own
  eligibility filter) computed at snapshot-publish time and read
  atomically alongside the generation via a new
  `AppContext::asset_lock_probe_snapshot()` accessor -- this also
  eliminates the torn-read TOCTOU window between the old two separate
  `snapshot_generation()`/`snapshot_balance()` calls.
- A same-generation dedup check could mask a genuine debounce-signal
  change once `loaded` was already set for that generation. `loaded`/
  `in_flight`/`failed` now key on the `(generation, signal)` pair
  instead of generation alone.

SEC-005: both identity screens kept their asset-lock ceiling cache
across a network switch (WalletSeedHash is network-independent) -- a
regression against the pre-PR live per-network snapshot_balance() read.
Both screens now get explicit change_context handling
(reset_for_network_switch(), mirroring WalletSendScreen) plus
refresh()/refresh_on_arrival() overrides that invalidate the cache.

Independently reviewed the full 9-file diff by hand and re-ran the
verification sweep myself with forced genuine recompiles (fmt clean,
7/7 asset_lock_balance tests incl. 2 new regressions, 43/43 identities
tests, 22/22 send_screen tests, 12/12 kittest, clippy clean) before
committing -- Codex's own commit attempt failed on the sandboxed
worktree's read-only git metadata, as expected.

Noted, not blocking: unifying both identity screens onto the shared
available_ceiling_duffs() helper means top_up_identity_screen lost a
more specific "wallet is busy" banner for a rare lock-contention case,
falling back to the generic "still being checked" message like
add_new_identity_screen already did -- a minor UX nuance, not a
correctness regression (PROJ-004, already tracked as non-blocking).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet_backend): bound the asset-lock probe's worst-case wait with a timeout

Holistic review (SEC-001): the asset-lock ceiling probe now runs
automatically on every funding-screen render, converting the accepted
upstream BnB algorithmic-complexity risk (dashpay/rust-dashcore#918)
from rare/user-initiated into routine/automatic/uncancellable/remotely
re-triggerable. Rust cannot forcibly kill a spawn_blocking thread, so
this adds cooperative cancellation instead: a CancellationToken checked
between search-loop iterations, cancelled by a 5s timer. On timeout the
caller gets a distinguishable TaskError with an actionable message
rather than an indefinite silent wait.

Deliberately scoped: does not touch AssetLockBalanceCache (a separate,
concurrent commit rewrote that file) or the accepted-risk annotation
itself. Cancel-on-navigate-away and single-flight-per-wallet dedup are
deferred follow-ups using the same token, not built here. The timeout
message wording is draft and needs a copy pass before shipping.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: note the asset-lock probe's timeout in the Max-fix changelog entry

The Max-fix entry described the safe-Max check but not its new
worst-case bound (5s timeout, added to contain an accepted upstream
BnB algorithmic-complexity risk). Documents the user-visible outcome
for very large wallets now that the fix is complete.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet_backend): remove BnB timeout workaround, close stale-validation and reservation-TOCTOU gaps

Round-5 remediation of PR #937, following a bot review (thepastaclaw)
that found three unresolved issues in round 4's fixes:

- Remove the CancellationToken/5s-timeout workaround added to bound
  the asset-lock probe's worst-case wait against the upstream
  BranchAndBound O(2^N) risk (rust-dashcore#918). The root cause is
  now fixed upstream (rust-dashcore#919's feasibility/undershoot
  prune, confirmed present on the dash-evo-tool integration branch
  platform is being re-pinned to in a follow-up commit) so the
  client-side workaround is unnecessary complexity. A TODO(upstream-
  pin) comment flags that this assumes the pin bump lands with it.

- Close a stale-cache-serves-validation gap: AssetLockBalanceCache
  deduped re-queries only on the wallet's aggregate spendable
  subtotal, so a UTXO composition change that didn't move the
  subtotal could leave pre-send validation trusting a stale
  builder-verified ceiling. WalletSnapshot now tracks a canonical
  signature of eligible asset-lock inputs and a revision counter
  that advances only when that exact set changes; validation call
  sites (Shield, Core->Identity, Fund Platform Address, Create/Top-
  Up Identity) now require a quote matching the current UTXO
  revision via a new get_current(), while display keeps its
  existing stale-while-revalidate behavior unchanged.

- Close a reservation-release TOCTOU race: the probe previously held
  only a brief read lock to clone the account, then ran its whole
  dry-run search with no lock held at all, racing against real sends
  (which correctly hold a write lock across their own select-
  reserve). A concurrent real send and probe iteration could both
  reserve the same UTXO via the shared ReservationSet, and the
  probe's cleanup could then release the real send's live
  reservation out from under it. The probe now holds an owned
  wallet-manager write guard for its entire search, matching the
  real-send path's locking discipline.

Independently re-verified: all touched test suites pass with real
test names visible in the log (payments, asset_lock_balance,
send_screen, identities, snapshot), clippy clean, no test silently
removed beyond the one now-obsolete pre-cancellation test.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* chore(deps): bump dashpay/platform pin to include the BnB feasibility-prune fix

Re-pin dash-sdk, rs-sdk-trusted-context-provider, platform-wallet, and
platform-wallet-storage from 288a6cae4f9653d6085d2b3d6c7410210a0c95ba
to a18bd1586858ef680124e150caad6a7dc21d0b64 (feat/platform-wallet-
storage-rehydration tip), whose key-wallet dependency now tracks
rust-dashcore's dash-evo-tool integration branch (rev 34f0921e) —
confirmed to include rust-dashcore#919's suffix-sum feasibility/
undershoot prune for BranchAndBound coin selection, closing #918.

This makes the prior commit's TODO(upstream-pin) assumption real:
the asset-lock probe's worst-case search is now algorithmically
bounded upstream, matching why this PR's earlier client-side timeout
workaround was removed instead of kept.

The initially-targeted branch tip (4ca05f51) did not compile — a
signature drift between key-wallet's build_asset_lock_with_signer
and platform-wallet's own call site — so this bump waited for that
to be fixed upstream before landing here.

Verified: full workspace test suite (2472 passed, 0 failed, +2 from
round 5's own new tests, no other count changes), doc tests (7
passed), cargo fmt clean, clippy --all-features --all-targets clean.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: drop implementation-detail sentence from the Max-fix changelog entry

The "upstream search-complexity risk... fixed at its source" sentence
was internal-implementation language that broke this entry's otherwise
plain, user-observable-behavior style (per CLAUDE.md's error-message/
i18n-ready string conventions, which this changelog entry otherwise
follows closely). Removed; the entry now ends on the last user-visible
behavior (Checking/Retry banner), same as before the timeout workaround
was ever added.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet_backend): correlate asset-lock probes by request id, distinguish failed from loading, fingerprint UTXO composition

Round-6 remediation of PR #937, following a fresh CodeRabbit review of
round 5's own diff. Four findings independently verified against the
code before fixing (a fifth, a claimed unit mismatch in
top_up_identity_screen.rs, was verified to be a false positive and is
untouched — max_amount_after_fee_reserve's duffs/credits split is
intentional and correct).

- Close a stale-probe-reply mismatch race: AssetLockBalanceCache's
  store()/mark_loading_failed() matched an incoming reply against the
  cache's *current* in-flight request using only snapshot_generation.
  Because ensure_requested can legitimately re-arm at the same
  generation when UTXO composition changes (existing, tested
  behavior), a superseded probe's stale reply could be silently
  mislabeled as the current request's result once it arrived late -
  reopening the exact stale-ceiling-authorizes-a-send risk round 5
  closed, via a different mechanism. Every probe now carries a
  monotonic per-cache request_id threaded through WalletTask,
  BackendTaskContext, and BackendTaskSuccessResult, so a reply can
  only be applied to the exact request it belongs to.

- Distinguish a permanently failed probe from one still loading at
  dispatch time, consistently across Send, Create Identity, and Top
  Up Identity - previously all three always showed "still being
  checked" even when the cache had already given up and was waiting
  on an explicit Retry.

- Add the stale-UTXO-composition dispatch-rejection test to both
  identity screens, mirroring send_screen.rs's existing coverage.

- Replace snapshot.rs's per-event Vec<(OutPoint, u64)> UTXO signature
  (cloned and compared on every wallet event) with a constant-size,
  order-independent u64 fingerprint of the same set.

Independently re-verified: full diff read against the actual code
(not the diff's own claims), all touched test suites re-run for real
through the verification wrapper, no test silently removed or
weakened. Extended the round-6 fingerprint-order test with a
distinct-sets assertion (two different UTXO sets must not fingerprint
identically) that the initial diff omitted.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet): close asset-lock quote freshness gaps from round-5's lock-hold change

Round 5 fixed the reservation-release TOCTOU race by holding the wallet
manager's write lock for the probe's full search, but introduced two
second-order regressions a holistic review caught: the freshness signal
never accounted for reservations (SEC-001), and the unbounded lock hold
froze the same freshness recompute it should have protected while also
stalling real sends (SEC-002). A third finding showed the Send screen's
AppAction::BitOrAssign overwrite could silently drop a probe dispatch or
navigation action queued in the same frame (PROJ-001), and a fourth
showed a dropped probe reply left the UI stuck loading with no retry
affordance (SEC-004).

- snapshot.rs: asset_lock probes now clone the live ManagedCoreFundsAccount
  (sharing its Arc<Mutex<..>> ReservationSet) and drive it through the real
  TransactionBuilder::set_funding/build_unsigned path, so a concurrent
  send's reservation is excluded from the probe exactly as it would be
  from a real build - not a parallel reimplementation of the eligibility
  predicate. Replaces the DefaultHasher-based fingerprint with exact
  AssetLockInputState comparison.
- payments.rs: adds ProbeDeadline, checked between each individually-
  bounded builder call across the search functions, bounding worst-case
  wallet-lock hold to ~5s; on expiry returns a proven-safe lower bound
  and marks the quote is_partial rather than fabricating a value.
- asset_lock_balance.rs: AssetLockBalanceCache now validates replies
  against the OBSERVED input composition returned by the probe rather
  than dispatch-time snapshot metadata, so freshness is correct even
  when background recompute lags behind. Adds a 15s in-flight reply
  deadline that redispatches with a fresh request_id and offers Retry
  while still loading, so a dropped reply can no longer wedge the UI.
- funding_common.rs / ui/mod.rs / send_screen.rs: relocates the
  concurrent-task-append helper out of ui::identities so the Send
  screen's probe-dispatch and confirmation-dialog call sites route
  around AppAction::BitOrAssign's overwrite instead of through it.
- Bundled cheap fixes: aligns send_core_to_shielded's Overflow wording
  with the other three dispatch-validation sites, corrects CHANGELOG.md
  and docs/user-stories.md to scope the "Max" fix to the Simple
  builder-driven form (the Advanced manual-input Platform-address path
  remains governed by the Core inputs the user selects), adds coverage
  for cache invalidation and reservation-aware revision changes.

Independently verified: read the reservation-aware predicate against
the pinned key-wallet checkout to confirm SEC-001 is fully closed (not
partially), confirmed the new tests reproduce each original bug against
pre-round-7 code, and re-ran the full workspace suite for real (2487
passed, 0 failed - exactly baseline 2480 + 7 new tests). fmt and clippy
clean.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix(wallet): close round-8 blocking gaps in the asset-lock Max probe and send path

- Send screen: a user-confirmed send is no longer dropped when a same-frame
  navigation action wins; merge_confirmation_action restores the pre-existing
  |= precedence, and append_concurrent_backend_tasks warns instead of silently
  dropping a task batch.
- The asset-lock probe's ProbeDeadline now bounds the UTXO-composition
  observation too (checked between batches), starts when the blocking work
  starts rather than before it is scheduled, and the observation clones the
  account once instead of once per batch.
- A contended or failed asset-lock observation during a snapshot recompute
  carries forward only the composition field: fresh balance/UTXOs/addresses
  still publish, with debug logging on the fallback path.
- Max now reads the same current-composition accessor as dispatch validation
  (send screen and both identity funding screens), so Max can never offer an
  amount validation would refuse; CHANGELOG and SND-017 now describe the
  bounded revalidation window instead of an unconditional guarantee.

Refs #929.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix(wallet): keep asset-lock Max usable on dust wallets and stop the probe's per-frame re-dispatch loop

Two defects in the asset-lock Max probe machinery:

- The input-composition observation treated a batch whose eligible funds
  sit at or below the 546-duff dust threshold as a fatal builder error,
  turning an everyday near-drained wallet into a permanent "could not be
  checked" state whose Retry deterministically fails. Classify the
  insufficient-funds outcomes as ordinary results (matching the dry-run
  helpers' classification) and keep the batch's eligible candidates in
  the composition key, so such wallets resolve to a current zero quote.

- AssetLockBalanceCache had no backoff when a reply's observed
  composition cannot match the published snapshot key (deadline-expiry
  marker, carried-forward stale key), re-dispatching the probe every UI
  frame and repeatedly seizing the process-global observation lock for
  up to 5s per probe. After one automatic re-probe, a second consecutive
  mismatched reply now marks the entry failed and stops automatic
  dispatch until the composition actually changes or the user retries.
  The suppression is composition-keyed, not generation-keyed, so SPV
  event churn cannot re-arm it.

Co-Authored-By: Claude Fable <noreply@anthropic.com>

* docs(wallet): correct stale reserved-outpoint claim in observation doc comment

Round-11 verification (PR #937) found asset_lock_final_input_state's doc
comment still claimed the observed composition excludes reserved
outpoints. Since 9a5105d's SEC-001 fix, a sub-dust batch's InsufficientFunds
arm cannot filter reserved outpoints the way the success path does, so one
may transiently enter the key. Harmless (the key only drives quote-match
detection, never a spent amount) but the doc was wrong. Clarify both the
match arm and the function doc.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: Codex GPT-5 <noreply@openai.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants