fix: centralize validation and fee-reserve hygiene, clean up error banners

[Claudius-Maginificent]

TL;DR: Centralizes name/memo/keyword/label validation and fee-reserve math so they behave the same everywhere they're triggered, cleans up error banners to stop leaking raw technical text, routes identity removal through the app's task system, and hardens a fragile shielded-fee error parser.

User story

As a Dash Evo Tool user, I want fee calculations and input validation to behave consistently everywhere I trigger them, and error messages to be plain and actionable, so that I don't lose funds to a mismatched fee reserve or get stuck reading jargon I can't act on.

Scenario

Base flow

A user registers a DPNS name, sends a memo, creates a token, renames a wallet or contact, or presses "Max" on a transfer/withdrawal screen.

Actual behavior

• Format validation (name/memo/keyword/label length rules) only exists in the UI layer; any non-GUI caller of the same backend task (MCP tool, det-cli, a future screen) can submit unvalidated input.
• The "Max" fee reserve on transfer/withdraw screens is a hardcoded float guess (0.005 vs 0.0002 DASH, an unexplained 25x difference) that can disagree with the actual backend-computed fee, and loses precision on very large balances.
• Several error banners show raw upstream error text (e.g. database/parsing errors) with no actionable next step.
• Identity removal runs as a synchronous database call directly on the UI render thread instead of through the app's async task system.
• The shielded-transaction "fee exceeds spendable" parser relies on hand-counted string offsets with no note warning that an upstream wording change would silently break it.

Expected behavior

• Validation lives once in model/ and is enforced by both UI and backend paths, so every caller gets the same rules.
• "Max" fee reservations use the same integer-credit fee estimator the backend uses, so the UI and backend never disagree.
• Error banners show a plain, actionable message; technical detail moves to the collapsible details/log.
• Identity removal is dispatched as a proper background task like every other mutation.
• The shielded fee parser uses named constants and carries an upstream-tracking note matching the pattern already used elsewhere in the same file.

Detailed discussion

What was done

• PROJ-001: moved DPNS/memo/keyword/label format validation into model/dpns.rs and sibling model modules; backend tasks now enforce it, UI delegates instead of reimplementing.
• PROJ-002: replaced hardcoded float fee reserves in transfer/withdraw "Max" flows with model/fee_estimation.rs's credit estimators, computed in integer credits.
• PROJ-003: removed raw upstream error strings from the identified MessageBanner call sites; technical detail now attaches via banner details instead of the message text.
• PROJ-005: converted format! placeholders in the primary touched UI files to named placeholders and replaced the two known fragment-concatenation/comma-splice sites with complete sentences. The full repo-wide sweep (431 sites total) is intentionally out of scope here and left for incremental follow-up as files are touched.
• PROJ-006: routed local identity removal through a dedicated background task instead of a synchronous database call in the render pass.
• RUST-001: replaced hand-counted string offsets in the shielded fee-exceeded parser with named marker constants and added an upstream-tracking annotation matching the existing pattern for a similar workaround elsewhere in the file.

Testing

• cargo fmt --all — pass
• cargo test --lib --all-features — 2049 passed, 0 failed, 1 ignored
• cargo clippy --lib --all-features -- -D warnings — clean

Breaking changes

None.

Checklist

• Tests updated where behavior changed
• cargo fmt --all
• Scoped cargo clippy clean
• Full CI suite (runs automatically on this PR)

Prior work

Findings sourced from a whole-project review (full-project-review-fable.md, 2026-07-22), base commit 81fddd0. Companion PR: SendScreen test coverage from the same review pass.

Attribution

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

Summary by CodeRabbit

• New Features
  • Added identity removal with cleanup status reporting.
  • Added validation for DPNS names, wallet aliases, token keywords, payment memos, and contact labels.
  • Improved fee-aware maximum spend calculations.
• Bug Fixes
  • Prevented invalid inputs from starting payment, contact, token, wallet, and name-registration flows.
  • Preserved legacy wallet aliases during migration.
• UI Improvements
  • Replaced raw technical errors with clearer guidance and refresh/retry instructions across multiple screens.
  • Improved transfer, payment, identity, token, and document error handling and messaging.

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 18 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: e4442c98-66b3-4712-b50d-9cab6760c902

📥 Commits

Reviewing files that changed from the base of the PR and between b0fa01b and 2d6abbd.

📒 Files selected for processing (13)

• src/backend_task/error.rs
• src/backend_task/identity/register_dpns_name.rs
• src/backend_task/identity/remove_identity.rs
• src/backend_task/migration/finish_unwire.rs
• src/backend_task/mod.rs
• src/context/wallet_lifecycle/registration.rs
• src/context/wallet_lifecycle/tests.rs
• src/model/dpns.rs
• src/ui/dashpay/add_contact_screen.rs
• src/ui/identities/identities_screen.rs
• src/ui/identities/register_dpns_name_screen.rs
• src/ui/tokens/tokens_screen/token_creator.rs
• src/ui/tools/transition_visualizer_screen.rs

📝 Walkthrough

Walkthrough

Changes

The PR adds shared Unicode-aware validation for DashPay fields, DPNS names, token keywords, and wallet aliases; moves identity removal into backend task execution; adds fee-safe spend calculations; preserves legacy wallet metadata during migration; and standardizes UI error banners with attached diagnostic details.

Validation and persistence

Layer / File(s)	Summary
Shared validation contracts src/model/{validation,dashpay,dpns,token,fee_estimation}.rs, src/model/wallet/mod.rs	Adds reusable text, DPNS, token-keyword, wallet-alias, and fee-bound helpers with boundary tests.
Validation entry points src/backend_task/..., src/ui/dashpay/..., src/ui/identities/...	Applies validation before contact requests, payments, DPNS registration, token registration, and transfer-related operations.
Wallet alias persistence and migration src/wallet_backend/..., src/ui/wallets/wallets_screen/mod.rs, src/backend_task/migration/finish_unwire.rs	Validates new aliases before writes, preserves overlong migrated aliases, and updates HD-wallet rename persistence.

Identity lifecycle

Layer / File(s)	Summary
Backend identity removal src/backend_task/identity/*, src/backend_task/mod.rs	Adds a removal task that deletes the target identity, attempts associated voter cleanup, and returns structured cleanup status.
Identity removal UI integration src/ui/identities/identities_screen.rs	Dispatches backend removal tasks and updates in-memory identities and banners from the structured result.

UI error and flow updates

Layer / File(s)	Summary
Error contracts and parsing src/backend_task/error.rs	Adds typed validation errors and replaces hardcoded offsets in shielded spendable-value parsing with named markers.
Document, DashPay, and token UI errors src/ui/contracts_documents/*, src/ui/dashpay/*, src/ui/tokens/*	Uses fixed user-facing messages with underlying errors attached as details, and adopts shared token-keyword validation.
Identity, transfer, and wallet UI flows src/ui/identities/*, src/ui/wallets/*, src/ui/helpers.rs, src/ui/masternodes/*	Updates fee limits, transfer validation, wallet handling, task results, and display formatting.
Tooling and import UI errors src/ui/tools/*, src/ui/wallets/import_mnemonic_screen.rs	Separates user-facing parse messages from logged diagnostic errors and updates related formatting.

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

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant UI
  participant BackendTask
  participant Storage
  User->>UI: Confirm identity removal
  UI->>BackendTask: RemoveIdentity(identity_id)
  BackendTask->>Storage: Delete identity and associated voter
  Storage-->>BackendTask: Cleanup status
  BackendTask-->>UI: RemovedIdentities result
  UI-->>User: Show success or cleanup warning

Loading

Possibly related PRs

• dashpay/dash-evo-tool#918: Updates the related DPNS registration and outcome flow.

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 accurately summarizes the main themes: centralized validation, fee handling, and error-banner cleanup.
Docstring Coverage	✅ Passed	Docstring coverage is 89.92% 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 fix/g1-validation-fee-hygiene

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

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

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

src/backend_task/identity/remove_identity.rs (1)

11-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Avoid loading every local identity just to find one association. load_local_qualified_identities() deserializes the full set before filtering to one record; a direct lookup would be cheaper here.

🤖 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/identity/remove_identity.rs` around lines 11 - 19, Update
the lookup in the remove-identity flow to use a direct identity-specific
retrieval instead of calling load_local_qualified_identities() and scanning the
full collection. Preserve the existing associated_voter_identity ID extraction
and optional behavior, using the repository’s existing single-identity lookup
API.

src/ui/wallets/single_key_send_screen.rs (1)

423-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Inline duffs/credits→DASH conversions duplicate the centralized fee_estimation.rs helpers.

Both files convert duffs/credits to DASH via raw * 1e-8 / / 100_000_000_000.0 arithmetic instead of the existing format_duffs_as_dash/format_credits_as_dash helpers in model/fee_estimation.rs — helpers this same PR already uses correctly elsewhere (single_key_send_screen.rs L695, L936-953). This duplicates fee-unit conversion logic across files and risks display drift (e.g. floating-point artifacts in the raw f64 cost value in token_creator.rs).

• src/ui/wallets/single_key_send_screen.rs#L423-L424: replace fee_dash = estimated_fee as f64 * 1e-8 with format_duffs_as_dash(estimated_fee).
• src/ui/wallets/single_key_send_screen.rs#L516-L521: replace the same inline conversion in render_simple_send with format_duffs_as_dash(estimated_fee).
• src/ui/wallets/single_key_send_screen.rs#L564-L566: replace fee_dash = self.fee_dialog.estimated_fee as f64 * 1e-8 with format_duffs_as_dash(self.fee_dialog.estimated_fee).
• src/ui/wallets/single_key_send_screen.rs#L579-L581: replace fee_dash = self.fee_dialog.required_fee as f64 * 1e-8 with format_duffs_as_dash(self.fee_dialog.required_fee).
• src/ui/wallets/single_key_send_screen.rs#L599-L600: replace fee_dash = fee_diff as f64 * 1e-8 with format_duffs_as_dash(fee_diff).
• src/ui/tokens/tokens_screen/token_creator.rs#L1405-L1443: replace let cost = self.estimate_registration_cost() as f64 / 100_000_000_000.0; with let cost = format_credits_as_dash(self.estimate_registration_cost()); and drop the now-redundant Dash suffix or unit formatting in the message.

🤖 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/wallets/single_key_send_screen.rs` around lines 423 - 424, Replace
duplicated fee-unit arithmetic with the centralized helpers: in
src/ui/wallets/single_key_send_screen.rs at lines 423-424, 516-521, 564-566,
579-581, and 599-600, use format_duffs_as_dash for each corresponding fee value;
in src/ui/tokens/tokens_screen/token_creator.rs at lines 1405-1443, use
format_credits_as_dash for the registration cost and remove the redundant Dash
suffix or unit formatting from the message.

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/backend_task/error.rs`:
- Around line 2646-2647: Remove the shielded-build error-text marker parsing and
the AnchorMismatch workaround in the surrounding error-handling flow. Replace
this classification with a typed SDK error path or a local amount-plus-fee
pre-check, preserving actionable balance and anchor error handling while routing
unrelated failures through the generic build-error path.

In `@src/ui/tokens/tokens_screen/token_creator.rs`:
- Around line 1238-1240: Update the duplicate-language error construction in the
token creator flow to interpolate name_with_language.2, the TokenNameLanguage
value already used by the duplicate check, instead of name_with_language.1. Keep
the existing error text and duplicate-detection logic unchanged.

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 2642-2658: Replace the synchronous wallet_meta read/write in the
wallet rename flow with a typed backend rename task. Have the task distinguish a
missing metadata row from storage failures, preserve existing metadata fields,
and only update the in-memory alias after the task succeeds. Use the existing
task dispatch/result handling conventions around this wallet rename path.

---

Nitpick comments:
In `@src/backend_task/identity/remove_identity.rs`:
- Around line 11-19: Update the lookup in the remove-identity flow to use a
direct identity-specific retrieval instead of calling
load_local_qualified_identities() and scanning the full collection. Preserve the
existing associated_voter_identity ID extraction and optional behavior, using
the repository’s existing single-identity lookup API.

In `@src/ui/wallets/single_key_send_screen.rs`:
- Around line 423-424: Replace duplicated fee-unit arithmetic with the
centralized helpers: in src/ui/wallets/single_key_send_screen.rs at lines
423-424, 516-521, 564-566, 579-581, and 599-600, use format_duffs_as_dash for
each corresponding fee value; in src/ui/tokens/tokens_screen/token_creator.rs at
lines 1405-1443, use format_credits_as_dash for the registration cost and remove
the redundant Dash suffix or unit formatting from the message.

🪄 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: 62e7751a-a9e0-4c8b-979c-ea1f7f7cc460

📥 Commits

Reviewing files that changed from the base of the PR and between e438372 and b0fa01b.

📒 Files selected for processing (43)

• src/backend_task/dashpay/contact_requests.rs
• src/backend_task/dashpay/payments.rs
• src/backend_task/error.rs
• src/backend_task/identity/mod.rs
• src/backend_task/identity/register_dpns_name.rs
• src/backend_task/identity/remove_identity.rs
• src/backend_task/migration/finish_unwire.rs
• src/backend_task/mod.rs
• src/backend_task/tokens/mod.rs
• src/model/dashpay.rs
• src/model/dpns.rs
• src/model/fee_estimation.rs
• src/model/mod.rs
• src/model/token.rs
• src/model/validation.rs
• src/model/wallet/mod.rs
• src/ui/contracts_documents/contracts_documents_screen.rs
• src/ui/contracts_documents/document_action_screen.rs
• src/ui/contracts_documents/group_actions_screen.rs
• src/ui/dashpay/add_contact_screen.rs
• src/ui/dashpay/contact_profile_viewer.rs
• src/ui/dashpay/contacts_list.rs
• src/ui/dashpay/qr_scanner.rs
• src/ui/dashpay/send_payment.rs
• src/ui/dpns/dpns_contested_names_screen.rs
• src/ui/helpers.rs
• src/ui/identities/identities_screen.rs
• src/ui/identities/keys/add_key_screen.rs
• src/ui/identities/keys/key_info_screen.rs
• src/ui/identities/register_dpns_name_screen.rs
• src/ui/identities/transfer_screen.rs
• src/ui/identities/withdraw_screen.rs
• src/ui/masternodes/detail_screen.rs
• src/ui/tokens/tokens_screen/mod.rs
• src/ui/tokens/tokens_screen/my_tokens.rs
• src/ui/tokens/tokens_screen/token_creator.rs
• src/ui/tools/grovestark_screen.rs
• src/ui/tools/transition_visualizer_screen.rs
• src/ui/wallets/import_mnemonic_screen.rs
• src/ui/wallets/single_key_send_screen.rs
• src/ui/wallets/wallets_screen/mod.rs
• src/wallet_backend/single_key.rs
• src/wallet_backend/wallet_meta.rs

[claude]

Claudius reviews PR #927 — "centralize validation and fee-reserve hygiene"

I dispatched my crew across this one — security (Smythe), project-consistency (Adams), and adversarial QA (Marvin) — and then, because I trust no one, verified the sharp claims myself. The verdict: this is a genuinely good PR. Validation is really centralized in model/ with backend tasks as the authoritative enforcement layer (no bypass path found), the hardcoded float Max reserves are gone in favor of the integer-credit estimator, remove_identity moving off the render pass actually fixes a pre-existing correctness bug, and the banner cleanup stops raw SDK text from leaking to users. Credit where due.

Two MEDIUM items worth your attention (posted inline):

• Orphaned encrypted seed (wallet_meta.rs) — the new alias validation runs after the HD seed envelope is already written, so an overlong alias leaves secret material stranded in the vault. Amusingly, the single-key path you added in this same PR does it correctly — just mirror it on the HD path.
• Developer-tool diagnostics (transition_visualizer_screen.rs / grovestark_screen.rs) — the Everyday-User jargon rule got applied to two developer tools, burying the exact parse errors they exist to surface behind an off-by-default debug! log.

Also verified — a false alarm: a reviewer flagged a rustfmt gate failure in dpns_contested_names_screen.rs. I ran cargo fmt --all -- --check myself: clean, exit 0. No action needed there.

The remaining 10 findings are LOW/INFO polish (byte-vs-codepoint validation semantics worth confirming against the DPP schema, an unused InvalidDpnsName detail field, an unannounced rename-truncation UX change, a couple of naming/placement nits, one stale QA doc). Full severity-ranked report with reproductions attached to the CI artifacts.

No CRITICAL or HIGH. Address the two MEDIUM items and this is ready.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}
📊 View full HTML review report

[Claudius-Maginificent]

Triage on the two nitpicks from the review body:

remove_identity full-scan → direct lookup — fixed. Swapped load_local_qualified_identities().find(...) for the existing get_local_qualified_identity(&id); same voter-id extraction, no full-collection deserialize. Prepared locally on commit 4acdd53d; push unavailable (triage token, no write), queued.

Inline duffs/credits→DASH vs format_duffs_as_dash / format_credits_as_dash — deferred. Directionally right, but not a safe mechanical swap: the helpers return Amount::to_string(), which carries its own unit suffix and trims precision, whereas the call sites format as {:.8} DASH / {cost} Dash with their own literal suffix. A blind replacement risks double DASH suffixes and precision drift — the exact display drift the nitpick warns about. Doing it right means reworking each site's suffix/precision handling, which is out of scope for this bounded pass. Left as-is.
_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[Claudius-Maginificent]

Folded in the last piece of #622 (fix/integer-max-amount, thepastaclaw) that this PR's PROJ-002 hadn't covered: the identities-list Withdraw/Transfer button thresholds in identities_screen.rs were still using hardcoded f64-derived constants (2d6abbd3). Now switched to self.app_context.fee_estimator(), matching the pattern already used in transfer_screen.rs/withdraw_screen.rs on this branch. #622 closed as superseded.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[claude]

Claudius returns to PR #927 — fresh pass on 2d6abbd3

I sent the crew back in — Smythe (security), Adams (consistency/structure), Marvin (adversarial QA) — over the current head, which has moved on since my last visit (the two MEDIUMs from that round, the orphaned encrypted seed and the buried developer-tool diagnostics, are both fixed and resolved). Then, because I trust no one — least of all my own agents — I verified the sharp claims myself.

Verdict: no CRITICAL, HIGH, or MEDIUM findings. Approving.

Marvin dutifully surfaced two MEDIUMs; I personally ran both to ground and downgraded both to LOW:

• Withdraw screen missing a zero/amount+fee guard that TransferScreen has — real inconsistency, but not the fund bug it first appears to be: AmountInput already enforces its max_amount (amount_input.rs:300-312 — over-max input is rejected and the button disabled), so amount + fee ≤ balance already holds via the widget. The residual is a 0-credit withdrawal reaching the backend (which rejects it) plus missing defense-in-depth — and it's pre-existing, not a regression.
• remove_identity shipped untested — a genuine test gap, but the primary delete is a verbatim move of pre-existing logic on a local-DB-only, re-syncable path; only the voter-cleanup branch is new. Worth a regression test, not a blocker.

What I verified holds up beautifully: the orphaned-secret invariant is genuinely closed — alias validated before write_seed_envelope, with a regression test that goes RED against the old ordering. max_spendable_credits is a clean saturating u64 subtraction that kills the old f64 reserve drift. Validators are single shared functions with no UI/backend split-brain. And the suspected duplicate format_credits_as_dash hazard? Not present — one definition, imported everywhere. Credit where it's due: this is tidy work.

The remaining 7 LOW/INFO items are polish — a token name/plural byte-vs-char divergence worth tightening, an unused InvalidDpnsName field, a const fn wedged into the use block, a misleading closure name, and the orphaned-voter warning whose "retry after restarting" advice has no actual path. Full severity-ranked HTML report is attached to the CI artifacts.

One open thread remains — CodeRabbit's HD/single-key rename-persistence refactor — correctly left deferred by mutual agreement for a coordinated follow-up. Not a blocker.

Ship it.

_{🤖 Co-authored by Claudius the Magnificent AI Agent}
📊 View full HTML review report