feat: AddressInput component, RPC error handling, shielded retry/sync improvements

[lklimek]

Summary

Adds a reusable AddressInput component, improves RPC error handling, and refines shielded pool operations with retry logic and post-operation syncing.

New: AddressInput Component

• Unified address input (src/ui/components/address_input.rs) with autocomplete, type detection (Core/Platform/Shielded/Identity), DPNS resolution, network validation, and balance display
• Domain types (src/model/address.rs) — AddressKind enum and ValidatedAddress type for address classification and validation
• UI Components README (src/ui/components/README.md) — catalog of all reusable components
• Migrated WalletSendScreen, UnshieldCreditsScreen, and mine dialog to use AddressInput

New: AmountInput Migration

• ShieldCreditsScreen, ShieldFromAssetLockScreen, ShieldedSendScreen, and UnshieldCreditsScreen replaced hand-rolled amount parsing with the AmountInput component

RPC Error Handling

• Connection errors include host:port — dedicated CoreRpcConnectionFailed variant with is_rpc_connection_error() detector; rpc_error_with_url() enriches connection failures across all RPC call sites
• RPC error visibility — chain lock errors surfaced on Networks tab as "Error: {message}" instead of bare "Disconnected"; rpc_last_error field added to ConnectionStatus
• Network password save — RPC password input works for all networks (was hardcoded to Regtest); stale error banners cleared on reconnect; warning banner shown if config save fails; ZMQ not disconnected on save
• Config file permissions — chmod 0600 applied to .env on Unix after save (contains RPC credentials)

Shielded Pool Improvements

• Anchor retry helper — with_anchor_retry() extracts shared state management (take/run/mark-spent/put-back) from transfer, unshield, and withdrawal tasks, with automatic retry on ShieldedAnchorMismatch
• Post-operation note sync — all shielded screens queue a SyncNotes task after successful operations so balances update without manual refresh
• Safety-net resync — queries all notes (including spent) to avoid false-positive resync triggers; propagates DB errors instead of silently swallowing
• Commitment tree clearing — refactored to check table existence before DELETE; moved out of network-switch transaction

Error Message Improvements

• IdentityInsufficientBalance shows amounts in DASH instead of raw credit integers
• AssetLockOutPointInsufficientBalance shows amounts in DASH with actionable guidance
• ShieldedFeeExceedsBalance shows fee in DASH and suggests reducing amount
• ShieldedInsufficientPoolNotes uses plain language ("network needs more activity")
• ShieldedAnchorMismatch says "wallet data is slightly outdated"
• ShieldedAddressInsufficientFunds shows available/required in DASH
• New dedicated variants: WalletUtxoReloadFailed, WalletBalanceRecalculationFailed
• WalletKeyDerivationFailed now preserves error chain via #[source]

Other Changes

• Consolidated format_credits_as_dash — uses Amount::dash_from_credits() with trimmed trailing zeros
• networks_address_compatible() — now pub(crate) in model/wallet/mod.rs, used in wallet dialogs so Testnet/Devnet/Regtest addresses are accepted interchangeably
• SPV balance tracking — spv_balance_known flag distinguishes "not yet synced" (None) from "synced with zero balance" (Some(0))
• Funding method dropdown — ComboBox height set to 200px so all options display without scrollbar
• Wallet switch refresh — switching wallets in RPC mode triggers a Core refresh
• Transaction filtering — transactions list filtered to only show entries involving the selected wallet's addresses, with defensive stale-Arc check
• Zero-balance auto-show — new wallets with <5 addresses automatically show all addresses
• Database — shared_connection() narrowed to pub(crate); TransactionStatus::from_u8 logs unknown values
• DB migration tests — regression tests for v27->v33 migration and fresh v33 install
• Tracing — detailed logging added to all shielded bundle operations; log levels corrected per coding best practices

Test plan

• Switch between Mainnet/Testnet/Regtest, change RPC password on each — verify save works and ZMQ stays connected
• Stop Dash Core, trigger an RPC call — verify banner shows host:port in error; details visible only in developer mode
• Enter wrong RPC password — verify "check your RPC password" message appears; fix password and verify error clears
• Save RPC password when config file is read-only — verify warning banner ("changes apply for this session only")
• Top Up Identity > "Select funding method" dropdown shows all options without scrollbar
• Attempt state transition with insufficient identity balance — verify message shows available/required in DASH (e.g., "0.5 DASH") not raw integers
• Attempt unshield with amount that leaves no room for fee — verify message shows fee in DASH and suggests reducing amount
• Attempt unshield when pool has fewer notes than minimum — verify plain-language message (no "notes" or "pool" jargon)
• On Regtest, paste a tdash-prefixed address in wallet send dialog — verify it is accepted
• Open Wallets screen — verify Balances and Shielded tabs render correctly
• Set DB version to 27 manually, restart app — verify migrations run cleanly to v33 (idempotent)
• Fresh install — verify DB created at v33 with all tables present
• On a regtest node with no chain locks, check Networks tab — verify RPC shows "Error: ..." instead of bare "Disconnected"
• Connect to a node with chain locks — verify RPC shows "Connected" and error clears
• Send screen: type a partial address — verify autocomplete dropdown shows matching wallet addresses with balances and type badges
• Send screen: switch source (Core/Platform/Shielded) — verify allowed address types change and address input resets
• Send screen: paste wrong-network address — verify validation error appears
• Unshield screen: enter a platform address — verify "Platform address" indicator; enter a Core address — verify "Core address" indicator
• Mine dialog: verify address selector shows wallet addresses with balances
• Shielded transfer: after successful send, verify balance updates without manual refresh (post-op sync)
• Wallet switch: select a different wallet — verify transactions refresh automatically in RPC mode
• New wallet: create a wallet with no funds — verify addresses are shown (not blank)

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

Summary by CodeRabbit

Release Notes

• New Features

  • Added AddressInput component with address validation, autocomplete, and support for Core, Platform, Shielded, and Identity address types.
  • Improved RPC error detection and display—connection failures now surface meaningful error messages to users.

• Bug Fixes

  • Fixed balance state tracking with explicit spv_balance_known flag to prevent premature balance display.
  • Improved transaction filtering for selected wallets to prevent display of unrelated transactions.

• Improvements

  • Enhanced error messages for wallet and shielded operations.
  • Consolidated address and amount input handling across send screens using structured components instead of string parsing.
  • Improved database schema migrations and commitment tree management.
  • Better balance formatting using standardized DASH display.

• Documentation

  • Added UI Components Catalog documenting available UI components and usage patterns.

[claude]

Round 4 review complete — 3 inline comments posted (1 HIGH, 2 MEDIUM). The clear_network_data ordering bug is the only blocker. See review body for full summary.

[Copilot]

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/ui/wallets/wallets_screen/mod.rs:1

• Filtering by outputs-to-known-addresses can hide legitimate outgoing transactions that spend the entire wallet balance with no change output (i.e., none of the outputs belong to the wallet, but the inputs do). That would make the UI incorrectly show “No transactions found” for real activity. A safer approach is to scope by wallet ownership metadata if you have it (e.g., an is_ours flag / per-tx wallet association), or include input-side checks (vin/outpoints) where available. Also, doing script→address parsing across all tx outputs every frame can become expensive for large histories; consider caching relevant_indices when the transaction set changes instead of recomputing during render.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[thepastaclaw]

Code Review

Three prior blocking findings (anchor retry state leak, RPC password session-apply, and commitment tree missing-table crash) are fixed. The global commitment tree clear remains as a known design trade-off. The new wallet UI changes introduce a mine-dialog regression for fresh wallets and a narrow transaction-filter gap. Advanced platform sends lack network validation. No true blockers remain — all findings are suggestions or nitpicks.

Reviewed commit: 810a686

🟡 6 suggestion(s) | 💬 2 nitpick(s)

2 additional findings

🟡 suggestion: Advanced platform sends discard the parsed network without validation

src/ui/wallets/send_screen.rs (lines 2590-2637)

Both send_advanced_utxo_to_platform (line 2590) and send_advanced_platform_to_platform (line 2635) parse platform addresses via PlatformAddress::from_bech32m_string() which returns (PlatformAddress, Network), but both discard the network with .map(|(addr, _)| addr). By contrast, AddressInput.validate_platform() (address_input.rs:526-537) checks the bech32m prefix against the expected network before parsing. The advanced paths bypass AddressInput and read from raw text fields. A user pasting a mainnet dash1... address while on testnet would get a confusing backend error instead of an immediate validation message. No actual cross-network funds risk (the SDK would reject it), but poor UX.

💬 nitpick: Developer 'Resync Notes' button also clears global commitment tree

src/ui/wallets/shielded_tab.rs (line 558)

Line 558 calls clear_commitment_tree_tables() which deletes shared table data for all wallets, same as the safety-net in initialize_shielded_wallet. Developer-mode only, so low user impact, but worth noting as part of the same global-clear pattern.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/context/shielded.rs`:
- [SUGGESTION] lines 246-268: Safety-net resync clears shared commitment tree for all wallets
  When `initialize_shielded_wallet()` detects a wallet with `last_synced_index > 0` but zero persisted notes, it calls `clear_commitment_tree_tables()` (line 254). Verified in `src/database/shielded.rs:170-188`: this function runs `DELETE FROM` on four shared tables (`commitment_tree_shards`, `commitment_tree_cap`, `commitment_tree_checkpoints`, `commitment_tree_checkpoint_marks_removed`) with no per-wallet filtering. If wallet A triggers this recovery, wallet B loses its commitment tree state and must resync. The trigger condition is narrow (only fires when DB state is internally inconsistent), so in practice this mainly affects edge-case recovery scenarios. The worst case is a forced resync (reconstructible from network), not data loss. Acknowledged as a conscious design trade-off after the revert in 712049bc from per-wallet files back to shared DB.

In `src/ui/wallets/wallets_screen/dialogs.rs`:
- [SUGGESTION] lines 1288-1293: Mine dialog offers no addresses for fresh or zero-balance wallets
  Verified: `AddressInput::extract_wallet_entries()` (address_input.rs:352) pulls Core addresses exclusively from `wallet.address_balances`, which only contains addresses with current UTXOs. For a fresh wallet, `address_balances` is empty while `known_addresses` contains 48+ bootstrapped BIP44 addresses. Combined with `.with_selection_only(true)` at line 1292, the user cannot select OR type an address. This is a chicken-and-egg problem: the mine dialog exists to fund a wallet, but requires the wallet to already have funded addresses. Regtest-only impact (mining doesn't work on mainnet/testnet), but broken for its primary use case.

In `src/ui/wallets/wallets_screen/mod.rs`:
- [SUGGESTION] lines 1234-1247: Transaction filter drops send-all transactions with no change output
  The filter at line 1234 keeps only transactions with at least one output matching `wallet_guard.known_addresses`. A legitimate send-all transaction where total input equals output + fee (no change output back to the wallet) would be filtered out and invisible in the transaction list. This is display-only — no funds risk. The edge case is narrow (requires exact-amount sends or dust-to-fee scenarios), and the comment at line 1231-1233 acknowledges the design: outputs-only checking is intentional as a cross-wallet contamination guard. Worth noting for completeness but low real-world frequency.

In `src/ui/wallets/send_screen.rs`:
- [SUGGESTION] lines 2590-2637: Advanced platform sends discard the parsed network without validation
  Both `send_advanced_utxo_to_platform` (line 2590) and `send_advanced_platform_to_platform` (line 2635) parse platform addresses via `PlatformAddress::from_bech32m_string()` which returns `(PlatformAddress, Network)`, but both discard the network with `.map(|(addr, _)| addr)`. By contrast, `AddressInput.validate_platform()` (address_input.rs:526-537) checks the bech32m prefix against the expected network before parsing. The advanced paths bypass AddressInput and read from raw text fields. A user pasting a mainnet `dash1...` address while on testnet would get a confusing backend error instead of an immediate validation message. No actual cross-network funds risk (the SDK would reject it), but poor UX.

In `src/backend_task/core/mod.rs`:
- [SUGGESTION] lines 489-494: chain_lock_rpc_error embeds raw error Display into user-facing url field
  Line 492 formats the url as `format!("{}:{} ({})", c.core_host, c.core_rpc_port, e)`, embedding the raw `dashcore_rpc::Error` Display text into the `url` field. Since `CoreRpcConnectionFailed`'s `#[error]` attribute includes `{url}` in the user-facing message, OS-level error text (e.g., "Connection refused (os error 111)") leaks to the user. Additionally, `source: None` is passed because the function borrows `&e` and can't move it, breaking the error chain. The function signature should take `e` by value to preserve the source.

In `src/backend_task/error.rs`:
- [SUGGESTION] lines 820-836: Fee-exceeds-spendable detection relies on fragile English substring parsing
  Verified: `parse_fee_exceeds_spendable` extracts numeric fields by searching for hard-coded English substrings ("amount ", "fee ", "exceeds total spendable value ") inside the SDK error text. If the upstream SDK changes wording, the parser silently fails and falls through to the generic `ShieldedTransitionBuildFailed` variant. The graceful degradation means this isn't a crash risk, but the user would see a generic error instead of the specific fee/balance breakdown. Not actionable until the SDK exposes structured error data — noting for awareness.

[coderabbitai]

Actionable comments posted: 18

Caution

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

⚠️ Outside diff range comments (3)

src/ui/network_chooser_screen.rs (1)

468-477: ⚠️ Potential issue | 🟠 Major

Do not show raw dashmate/config errors in a global banner.

read_dashmate_rpc_password() returns file-system paths and parser details. Passing &e straight into MessageBanner::set_global() leaks that technical text to end users instead of giving them a short action-oriented message.

As per coding guidelines "Never expose raw backend/database errors to users. Use a user-friendly message in the banner and attach technical details via BannerHandle::with_details()."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/network_chooser_screen.rs` around lines 468 - 477, The global banner
currently displays raw errors returned from read_dashmate_rpc_password() (see
the Auto Update button branch), leaking filesystem/parser details; change the
error handling so MessageBanner::set_global() receives a short, user-friendly
message like "Failed to read Dashmate RPC password — check your config" while
attaching the original error text as technical details via
BannerHandle::with_details() (or similar API) so the detailed e is not shown to
end users; update the match arm that logs and sets the banner (and keep
tracing::error(e) for logs) to call MessageBanner::set_global() with the
friendly message and pass e into BannerHandle::with_details().

src/backend_task/shielded/bundle.rs (1)

440-445: ⚠️ Potential issue | 🟠 Major

Round the reserved platform fee up before converting to duffs.

This floors platform_fee_credits to whole duffs before the 20% cushion. A fee below 1 duff reserves 0, and 1500 credits still reserves only 1 duff, so asset_lock_duffs can be underfunded despite the safety margin.

Suggested fix

-    let platform_fee_duffs = (platform_fee_credits / CREDITS_PER_DUFF).saturating_mul(120) / 100;
+    let platform_fee_credits_with_margin = platform_fee_credits
+        .saturating_mul(120)
+        .saturating_add(99)
+        / 100;
+    let platform_fee_duffs = platform_fee_credits_with_margin
+        .saturating_add(CREDITS_PER_DUFF - 1)
+        / CREDITS_PER_DUFF;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/shielded/bundle.rs` around lines 440 - 445,
platform_fee_credits is being floored to duffs before applying the 20% cushion,
which can underfund asset_lock_duffs; change the calculation of
platform_fee_duffs to compute the 20%‑increased credits first and then round up
when converting to duffs (i.e. perform ceil((platform_fee_credits * 1.20) /
CREDITS_PER_DUFF)), using integer math and saturating multiplications to avoid
overflow; keep references: platform_fee_credits, platform_fee_duffs,
CREDITS_PER_DUFF, asset_lock_duffs, and amount_duffs when updating the formula.

src/ui/wallets/wallets_screen/mod.rs (1)

1198-1221: ⚠️ Potential issue | 🟠 Major

Don't block transaction rendering on Arc::ptr_eq.

Wallet selection is keyed by seed_hash, and registration paths create a new Arc for the same wallet. When that happens, this guard hides the transaction list until the user manually re-selects, even though the canonical wallet is already in app_context.wallets. Resolve/render from the canonical entry by hash, or swap self.selected_wallet over to it, instead of treating pointer mismatch as stale data.

💡 Suggested fix

-        let Some(wallet_arc) = self.selected_wallet.as_ref() else {
+        let Some(wallet_arc) = self.selected_wallet.clone() else {
             ui.label("Select a wallet to view its transaction history.");
             return;
         };

-        let wallet_guard = wallet_arc.read().unwrap();
-        let selected_seed_hash = wallet_guard.seed_hash();
-        let arc_matches = self
+        let selected_seed_hash = wallet_arc.read().unwrap().seed_hash();
+        let canonical_wallet = self
             .app_context
             .wallets
             .read()
             .ok()
             .and_then(|wallets| wallets.get(&selected_seed_hash).cloned())
-            .is_some_and(|canonical| Arc::ptr_eq(wallet_arc, &canonical));
-        if !arc_matches {
-            tracing::warn!(
-                "selected_wallet Arc does not match app_context.wallets — skipping transaction render"
-            );
-            ui.label("Wallet data is being updated. Please re-select the wallet.");
-            return;
+            .unwrap_or_else(|| wallet_arc.clone());
+        if !Arc::ptr_eq(&wallet_arc, &canonical_wallet) {
+            self.selected_wallet = Some(canonical_wallet.clone());
+            self.cached_tx_indices = None;
         }
+        let wallet_guard = canonical_wallet.read().unwrap();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/wallets/wallets_screen/mod.rs` around lines 1198 - 1221, The current
guard rejects rendering when Arc::ptr_eq(wallet_arc, &canonical) is false, which
wrongly hides transactions for the same wallet registered as a new Arc; instead,
look up the canonical Arc by seed_hash from app_context.wallets and use it for
rendering (or replace self.selected_wallet with that canonical Arc) so rendering
proceeds for the canonical wallet entry; update the logic around selected_wallet
/ wallet_arc (the variables referenced in this block) to resolve by seed_hash
and fall back to the canonical Arc rather than returning on pointer inequality.

♻️ Duplicate comments (3)

src/ui/wallets/wallets_screen/dialogs.rs (1)

1288-1293: ⚠️ Potential issue | 🟠 Major

selection_only can leave the mine dialog unusable for fresh wallets.

This disables typing and delegates all choices to AddressInput. If that component only offers balance-backed Core entries, a fresh or zero-balance wallet has nothing to select, so the mining dialog cannot bootstrap the wallet it is meant to fund.

#!/bin/bash
# Verify which wallet addresses AddressInput exposes and how the mine dialog configures it.
rg -n -C3 'extract_wallet_entries|address_balances|known_addresses|with_selection_only|with_wallets' src

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/wallets/wallets_screen/dialogs.rs` around lines 1288 - 1293, The mine
dialog currently calls AddressInput::new(...).with_selection_only(true) which
prevents typing and makes the dialog unusable for fresh/zero-balance wallets;
change the configuration so typing is allowed when the targeted wallet has no
selectable Core addresses — e.g., remove or set with_selection_only(false) for
the AddressInput instance used in the mine dialog (the
AddressInput::new(...).with_wallets(&[wallet]) call), or conditionally set
with_selection_only based on whether the wallet has any Core balance-backed
entries (use the same wallet/address inspection helpers such as
extract_wallet_entries/address_balances/known_addresses to detect emptiness) so
new wallets can type an address to bootstrap funding.

src/backend_task/core/mod.rs (1)

489-494: ⚠️ Potential issue | 🟠 Major

Keep url to host:port; move the raw RPC error into details.

format!("{}:{} ({})", ...) makes the user-facing connection error expose OS/library text instead of just the endpoint, and source: None drops the diagnostic chain. Keep the endpoint and the low-level error in separate fields.

#!/bin/bash
# Inspect the current TaskError shape and all constructors/callers.
rg -n -C3 'CoreRpcConnectionFailed|chain_lock_rpc_error|rpc_error_with_url' src

As per coding guidelines "Never put technical details (raw error strings, stack traces, SDK internals, or error codes) in error messages — attach via BannerHandle::with_details(e)."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/core/mod.rs` around lines 489 - 494, The current
CoreRpcConnectionFailed builds url with the raw RPC error text and drops the
error by using source: None; change the formatted url to only the host:port (use
the same config mapping but remove " ({})"), and preserve the low‑level error by
populating the source (or attaching details) instead of discarding it — e.g.
return TaskError::CoreRpcConnectionFailed { url, source: Some(e.into()) } or
ensure the error is passed to BannerHandle::with_details(e) so the diagnostic
chain is kept; locate this change around is_rpc_connection_error(...) and the
TaskError::CoreRpcConnectionFailed constructor.

src/context/shielded.rs (1)

254-258: ⚠️ Potential issue | 🟠 Major

Avoid the shared, non-transactional tree wipe in initialization.

clear_commitment_tree_tables() clears the global commitment-tree tables on the shared connection (src/database/shielded.rs:170-188), and it does so table-by-table without a transaction. If one wallet hits this recovery path, other shielded wallets on the same network get forced into resync, and a failure halfway through leaves the tree half-reset. This reset needs a wallet-scoped helper or a single transaction before it is safe to call here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/context/shielded.rs` around lines 254 - 258, The call to
self.db.clear_commitment_tree_tables() in shielded.rs uses a shared,
non-transactional global wipe and must be replaced with a wallet-scoped,
transactional reset to avoid cross-wallet interference and partial wipes; update
the code to either (A) call a new DB helper like
clear_wallet_commitment_tree(wallet_id: WalletId) that only deletes rows for
this wallet, or (B) wrap the existing multi-table deletion in a single database
transaction (via your DB connection's transaction API) so all table clears
succeed or rollback together, and propagate any error into
TaskError::ShieldedTreeUpdateFailed as before; change the call site in the
method where self.db.clear_commitment_tree_tables() is invoked to use the new
wallet-scoped helper or the transactional operation.

🧹 Nitpick comments (5)

src/database/initialization.rs (4)

1145-1148: Redundant assertion — DEFAULT_DB_VERSION is already 33.

Line 1146 asserts version == DEFAULT_DB_VERSION and line 1147 asserts version == 33. Since DEFAULT_DB_VERSION is 33, one assertion is sufficient.

Proposed fix

         assert_eq!(version, DEFAULT_DB_VERSION);
-        assert_eq!(version, 33);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/initialization.rs` around lines 1145 - 1148, The two assertions
checking the initialized DB version are redundant; remove one and keep the
semantic one using the constant. Replace the pair of asserts on variable
`version` with a single assertion `assert_eq!(version, DEFAULT_DB_VERSION);`
(remove the literal `assert_eq!(version, 33);`) so the test uses the named
constant `DEFAULT_DB_VERSION` instead of a magic number; locate this in the
initialization test where `version` is asserted.

---

928-932: Note: Schema default mismatch between migration and fresh install.

The migration adds status with DEFAULT 2 (Confirmed), while the fresh CREATE TABLE in initialize_wallet_transactions_table likely uses DEFAULT 0 (Unconfirmed). Per the relevant code snippet from src/database/wallet.rs:435-471, all INSERTs explicitly provide the status value, so this mismatch has no runtime impact. However, it creates a latent schema inconsistency between migrated and fresh databases.

This was noted in PR review comments as a non-blocking suggestion. Consider aligning the defaults in a future cleanup if desired.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/initialization.rs` around lines 928 - 932, The migration ALTER
TABLE statement that adds the status column uses DEFAULT 2 while the fresh
CREATE TABLE in initialize_wallet_transactions_table uses DEFAULT 0, creating a
schema inconsistency; update the ALTER TABLE call (the conn.execute that runs
"ALTER TABLE wallet_transactions ADD COLUMN status INTEGER NOT NULL DEFAULT 2")
to use the same default as the fresh table (change DEFAULT 2 to DEFAULT 0) or
alternatively align initialize_wallet_transactions_table to DEFAULT 2 so both
code paths match—pick one and make the defaults consistent across
initialize_wallet_transactions_table and the migration SQL.

---

925-937: Duplicate comments should be consolidated.

The same explanatory comment appears twice: once before conn.execute (lines 925-927) and once inside it (lines 929-931). Remove one to avoid redundancy.

Proposed fix

         if !has_status {
-            // DEFAULT 2 (Confirmed) for migration: existing transactions predate status
-            // tracking and are assumed confirmed. Fresh installs use DEFAULT 0 (Unconfirmed)
-            // in the CREATE TABLE (wallet.rs).
             conn.execute(
                 // DEFAULT 2 (Confirmed) for migration: existing transactions predate status
                 // tracking and are assumed confirmed. Fresh installs use DEFAULT 0 (Unconfirmed)
                 // in the CREATE TABLE (wallet.rs).
                 "ALTER TABLE wallet_transactions ADD COLUMN status INTEGER NOT NULL DEFAULT 2",
                 [],
             )?;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/initialization.rs` around lines 925 - 937, Consolidate the
duplicated explanatory comment about default status for the migration by
removing one of the two identical comment blocks surrounding the ALTER TABLE
call: keep a single comment either immediately above the conn.execute invocation
or inside the SQL argument, and remove the other; target the code around
conn.execute(...) performing "ALTER TABLE wallet_transactions ADD COLUMN status
INTEGER NOT NULL DEFAULT 2" in initialization.rs to make this change.

---

984-1006: Consider using parameterized query in assert_column_exists for consistency.

assert_table_exists uses params![table] for the table name, but assert_column_exists uses string formatting. While this is test-only code with hardcoded values, using consistent parameterization would be cleaner.

Proposed fix using pragma_table_xinfo with params

Note: SQLite's pragma_table_info doesn't accept the table name as a parameter in the same query. An alternative is to use a subquery pattern or accept this inconsistency given it's test code with controlled inputs. The current approach is acceptable for test helpers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/database/initialization.rs` around lines 984 - 1006, The
assert_column_exists helper should avoid interpolating the column name directly;
update assert_column_exists to parameterize the column argument (keep the table
name formatting if PRAGMA can't be parameterized) by changing the query to
format!("SELECT COUNT(*) FROM pragma_table_info('{table}') WHERE name=?1") and
pass params![column] to query_row, so the function assert_column_exists(conn,
table, column) uses a parameter for the column name similar to
assert_table_exists while leaving table handling as-is.

src/model/address.rs (1)

74-76: Keep the new model type independent of crate::ui.

src/model/address.rs now imports crate::ui::helpers::is_platform_address_string(), while src/ui/components/address_input.rs already depends on this model module. That flips the dependency direction and makes the address domain types harder to reuse outside the UI. Please move the prefix helper into src/model/address.rs or a shared non-UI helper instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/model/address.rs` around lines 74 - 76, The model now depends on
crate::ui via is_platform_address_string(trimmed); remove that UI dependency by
moving the Bech32m platform-prefix helper into the model (e.g., add a private
pub(crate) fn is_platform_address_string or a small helpers module inside
src/model/address.rs) and replace the call in the AddressKind detection with
that local helper. Also update any callers in src/ui/components/address_input.rs
(and other UI code) to import the helper from the model (e.g.,
model::address::is_platform_address_string or
model::address::helpers::is_platform_address_string) so the dependency flows
from UI -> model only. Ensure no other references to crate::ui::helpers remain
in src/model/address.rs.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend_task/core/mod.rs`:
- Around line 187-203: The early return when chain_lock_rpc_error detects an
auth/connection failure prevents ConnectionStatus::handle_task_result from
receiving successful CoreItem::ChainLocks payloads; instead of returning
Err(task_err) inside the active_rpc_error check in the match on
self.network/active_result, propagate the RPC failure down the success path by
constructing the rpc_error payload (e.g., set active_rpc_error =
Some(task_err.to_string()) or the same TaskError-wrapping payload expected by
CoreItem::ChainLocks) and continue so the function returns Ok(...) with the
ChainLocks result; update the branch using
Self::chain_lock_rpc_error(active_config, e) to not return but assign/convert
the error into the rpc_error value consumed by
ConnectionStatus::handle_task_result.

In `@src/context/connection_status.rs`:
- Around line 448-450: Replace storing the raw rpc_error string into the UI
state: stop calling self.set_rpc_last_error(rpc_error.clone()) with the raw text
and instead convert rpc_error into a user-facing one-sentence summary or an
error-kind enum (e.g., map transport/auth/timeout cases to concise messages) and
pass that summary to set_rpc_last_error (or introduce set_rpc_last_error_kind).
Keep the original rpc_error only in logs or attach it as details via
BannerHandle::with_details(e) so UI shows a calm sentence while full debug info
is retained for diagnostics.

In `@src/context/shielded.rs`:
- Around line 240-248: The safety-net should force a full resync when the
in-memory notes are empty even if there is historical data in storage, unless
that persisted data contains any unspent/change notes; change the check after
calling self.db.get_all_shielded_notes(&seed_hash, &network_str) so that you
inspect the returned rows (all_notes) and only skip the resync when there exists
at least one unspent/change note (e.g., !all_notes.iter().all(|note|
note.is_spent) or filtering for note.unspent/change), otherwise proceed with the
full resync; adjust the logic around state.last_synced_index,
state.notes.is_empty(), all_notes.is_empty() to use this unspent-note test
instead of treating any persisted row as sufficient.

In `@src/model/address.rs`:
- Around line 69-76: The shielded-address check in the address classification is
case-sensitive and lets uppercase inputs fall through to the platform check;
update the logic in the function that returns AddressKind (the block using the
local variable `trimmed`) so the shielded prefix test is performed
case-insensitively (e.g., compare a lowercase form of `trimmed` or perform a
case-insensitive starts_with) for "dash1z" and "tdash1z" before calling
crate::ui::helpers::is_platform_address_string; keep the shielded check first so
uppercase "DASH1Z..." / "TDASH1Z..." are correctly classified as
AddressKind::Shielded and avoid sending them to validate_platform().

In `@src/ui/components/address_input.rs`:
- Around line 1131-1136: The test function detect_identity_when_enabled is flaky
because Identifier::random() can sometimes produce a string classified as
DetectedType::Core; update the test to reuse the same guard logic used in the
model (i.e., regenerate identifiers until detect_address_type(&id_str, true) is
not DetectedType::Core) before asserting equality to DetectedType::Identity,
referencing the test name detect_identity_when_enabled, the Identifier::random()
call, detect_address_type, and DetectedType::Identity so the test only asserts
on a guaranteed non-Core identity string.
- Around line 467-620: Several validation messages in validate_core,
validate_platform, validate_shielded and the fallback branch (e.g. "This address
belongs to a different network." and "This does not look like a valid address.")
only diagnose the problem; update each returned user-facing string to follow the
required format "What happened + what to do" and include a concrete action. For
example, in validate_core and the network-check branches of
validate_platform/validate_shielded replace the network-only messages with
something like "This address is for a different network. Please use an address
for this network." and for parse failures replace generic "does not look like a
valid address" with "This does not look like a valid address. Please check the
address and try again or paste a different address." Ensure you update all
occurrences returned from validate_core, validate_platform, validate_shielded
and the Unknown/fallback branches so every message contains both a description
and a concrete next step.
- Around line 463-493: Detected identity IDs can be misclassified as Core and
get rejected before identity validation; to fix this, special-case the
identity-only input: if self.enabled_kinds.as_slice() == [AddressKind::Identity]
then bypass the detected-kind enabled check and call
self.validate_identity(trimmed) directly (return its result) instead of relying
on detect_address_type/detected; keep the existing detect_address_type and
normal flow for other enabled_kinds, and ensure references to
detect_address_type, detected, AddressKind::Identity, enabled_kinds, and
validate_identity are used to locate the change.
- Around line 497-505: validate_core currently uses
addr.require_network(self.network) but may keep a Network::Testnet address when
the app network is Devnet/Regtest; after the Ok(checked) from require_network(),
compare checked.network() to self.network and, if they differ due to
Testnet-on-Devnet/Regtest, reconstruct a normalized address using
Address::new(self.network, checked.payload().clone()) before wrapping it in
ValidatedAddress::Core (mirror the logic from check_address_for_network()).
Ensure you replace the original checked with the normalized address when
producing ValidatedAddress::Core.
- Around line 553-586: validate_shielded currently checks prefixes and length
before parsing but doesn't normalize Bech32m case, so uppercase shielded
addresses will be rejected; mirror the normalization in validate_platform by
detecting mixed-case per BIP-350 and converting trimmed to lowercase (use the
same helper/pattern used in validate_platform) before computing expected_prefix,
performing starts_with checks, and calling OrchardAddress::from_bech32m_string;
ensure the returned ValidatedAddress::Shielded stores the canonical (lowercased)
form and update any references to AddressKind::detect guidance so routing and
storage consistently use the normalized form.

In `@src/ui/mod.rs`:
- Around line 818-821: The current invalidate_address_input() helper only drops
the widget wrapper and does not clear the cached validated_destination, so
rebinding app_context in the Screen::WalletSendScreen (and the analogous
Screen::UnshieldCreditsScreen) branches can leave a destination validated
against the old context; update the invalidate_address_input() implementation to
also set validated_destination to None/clear it (the same field used by the send
screen’s full reset) and then call that updated helper from the
Screen::WalletSendScreen and the other screen branch so the cached validation is
fully cleared on context switch.

In `@src/ui/network_chooser_screen.rs`:
- Around line 418-425: When switching networks (compare self.current_network and
prev_network) ensure the password input is cleared if no config entry exists:
after attempting Config::load_from(&self.mainnet_app_context.data_dir) and
config.config_for_network(self.current_network), if either fails then call
self.dashmate_password_input.set_text(String::new()) (or equivalent clear) so
the previous network's password is not retained; otherwise keep the existing
behavior of setting the password from network_config.core_rpc_password.

In `@src/ui/wallets/send_screen.rs`:
- Around line 471-474: invalidate_address_input currently only clears
self.address_input but leaves self.validated_destination populated, risking
stale validated addresses after a network change; update the
invalidate_address_input method to also set self.validated_destination = None so
both the AddressInput and its validated_destination are cleared (mirroring
behavior in reset_form and the source selection handlers) and ensure any
downstream logic that reads validated_destination will see None until
revalidated.

In `@src/ui/wallets/shield_credits_screen.rs`:
- Around line 360-363: The follow-up note sync is only queued from the success
path, missing cases where a batch has some successes (e.g., parallel runs not
calling display_task_result() or sequential runs finishing with an error after
earlier successes); update the batch completion handling so when the batch
transitions to complete you check if batch_succeeded > 0 and then queue a single
SyncNotes refresh (use the same AppAction::BackendTask wrapping) regardless of
the terminal event, instead of only queuing from display_task_result();
reference and adjust the logic around pending_refresh_task,
display_task_result(), and the place where batch completion is detected to set
or enqueue the SyncNotes task.

In `@src/ui/wallets/shield_from_asset_lock_screen.rs`:
- Around line 118-126: The Max amount uses full core_balance_duffs *
CREDITS_PER_DUFF so the Max button can exceed what shield_from_asset_lock() can
actually spend; modify the logic that computes max_credits (used when calling
amount_input.set_max_amount(...)) to subtract the estimated asset-lock fee
reserve (or a constant RESERVE_DUFFS converted to credits) before passing it to
AmountInput::set_max_amount, and ensure the estimate is derived from the same
fee calculation used by shield_from_asset_lock() so the reserve matches runtime
behavior.
- Around line 68-72: The current code takes pending_refresh_task into action and
then overwrites action with AppAction::PopScreen when the user clicks Done,
which drops a queued ShieldedTask::SyncNotes; instead, preserve the
pending_refresh_task by chaining the PopScreen after the BackendTask: when
pending_refresh_task.is_some() create a combined action that runs
AppAction::BackendTask(task) followed by AppAction::PopScreen (e.g.,
create/return a sequence/chained action or push the PopScreen to run after the
BackendTask), and apply the same change for both sites that build action (the
block using pending_refresh_task around
pending_refresh_task.take().map(AppAction::BackendTask) and the other occurrence
at lines 108-113) so the sync task is not lost when Done is clicked.

In `@src/ui/wallets/shielded_send_screen.rs`:
- Around line 236-245: The ShieldedNotesSynced branch currently only logs and
does not update sender-side state; when matching
BackendTaskSuccessResult::ShieldedNotesSynced { seed_hash, new_notes, balance }
if seed_hash == self.seed_hash, update the screen state by setting
self.max_balance = Some(balance) (or otherwise store the new balance), set
self.balance_update_pending = false to clear the pending flag, and optionally
record recipient-specific reminders in a separate field if you still want a
recipient-side note; ensure these updates occur in the same match arm that
currently only calls tracing::debug so the success view reflects the completed
refresh.

In `@src/ui/wallets/unshield_credits_screen.rs`:
- Around line 79-83: The current code always replaces any existing
self.pending_refresh_task-backed AppAction::BackendTask with
AppAction::PopScreen on Done; instead, preserve a queued BackendTask (e.g.,
SyncNotes) if present by only using AppAction::PopScreen when
pending_refresh_task is None. Update the logic around
pending_refresh_task.take().map(AppAction::BackendTask).unwrap_or(AppAction::None)
to return the existing BackendTask when Some and otherwise return PopScreen, and
apply the same fix to the second occurrence around the Done handler (the block
referenced at lines ~121-132) so you don't drop the queued sync.
- Around line 174-183: The current code passes the full shielded balance to
AmountInput via set_max_amount(Some(self.max_balance)) which ignores needed
transition/core fees and can let the user pick a Max that later fails; change it
to compute a fee-reserved maximum (e.g., let max_reserve =
self.max_balance.saturating_sub(estimated_fee)) and pass
set_max_amount(Some(max_reserve)) instead; locate the amount input creation
(AmountInput::new / amount_input variable) and replace use of self.max_balance
with a fee-adjusted value obtained from an existing fee estimator (e.g., a
method like estimate_transition_fee() or a stored field) or a conservative
constant if no estimator exists, ensuring the amount value (self.amount) and UI
validation reflect that reservation.

---

Outside diff comments:
In `@src/backend_task/shielded/bundle.rs`:
- Around line 440-445: platform_fee_credits is being floored to duffs before
applying the 20% cushion, which can underfund asset_lock_duffs; change the
calculation of platform_fee_duffs to compute the 20%‑increased credits first and
then round up when converting to duffs (i.e. perform ceil((platform_fee_credits
* 1.20) / CREDITS_PER_DUFF)), using integer math and saturating multiplications
to avoid overflow; keep references: platform_fee_credits, platform_fee_duffs,
CREDITS_PER_DUFF, asset_lock_duffs, and amount_duffs when updating the formula.

In `@src/ui/network_chooser_screen.rs`:
- Around line 468-477: The global banner currently displays raw errors returned
from read_dashmate_rpc_password() (see the Auto Update button branch), leaking
filesystem/parser details; change the error handling so
MessageBanner::set_global() receives a short, user-friendly message like "Failed
to read Dashmate RPC password — check your config" while attaching the original
error text as technical details via BannerHandle::with_details() (or similar
API) so the detailed e is not shown to end users; update the match arm that logs
and sets the banner (and keep tracing::error(e) for logs) to call
MessageBanner::set_global() with the friendly message and pass e into
BannerHandle::with_details().

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1198-1221: The current guard rejects rendering when
Arc::ptr_eq(wallet_arc, &canonical) is false, which wrongly hides transactions
for the same wallet registered as a new Arc; instead, look up the canonical Arc
by seed_hash from app_context.wallets and use it for rendering (or replace
self.selected_wallet with that canonical Arc) so rendering proceeds for the
canonical wallet entry; update the logic around selected_wallet / wallet_arc
(the variables referenced in this block) to resolve by seed_hash and fall back
to the canonical Arc rather than returning on pointer inequality.

---

Duplicate comments:
In `@src/backend_task/core/mod.rs`:
- Around line 489-494: The current CoreRpcConnectionFailed builds url with the
raw RPC error text and drops the error by using source: None; change the
formatted url to only the host:port (use the same config mapping but remove "
({})"), and preserve the low‑level error by populating the source (or attaching
details) instead of discarding it — e.g. return
TaskError::CoreRpcConnectionFailed { url, source: Some(e.into()) } or ensure the
error is passed to BannerHandle::with_details(e) so the diagnostic chain is
kept; locate this change around is_rpc_connection_error(...) and the
TaskError::CoreRpcConnectionFailed constructor.

In `@src/context/shielded.rs`:
- Around line 254-258: The call to self.db.clear_commitment_tree_tables() in
shielded.rs uses a shared, non-transactional global wipe and must be replaced
with a wallet-scoped, transactional reset to avoid cross-wallet interference and
partial wipes; update the code to either (A) call a new DB helper like
clear_wallet_commitment_tree(wallet_id: WalletId) that only deletes rows for
this wallet, or (B) wrap the existing multi-table deletion in a single database
transaction (via your DB connection's transaction API) so all table clears
succeed or rollback together, and propagate any error into
TaskError::ShieldedTreeUpdateFailed as before; change the call site in the
method where self.db.clear_commitment_tree_tables() is invoked to use the new
wallet-scoped helper or the transactional operation.

In `@src/ui/wallets/wallets_screen/dialogs.rs`:
- Around line 1288-1293: The mine dialog currently calls
AddressInput::new(...).with_selection_only(true) which prevents typing and makes
the dialog unusable for fresh/zero-balance wallets; change the configuration so
typing is allowed when the targeted wallet has no selectable Core addresses —
e.g., remove or set with_selection_only(false) for the AddressInput instance
used in the mine dialog (the AddressInput::new(...).with_wallets(&[wallet])
call), or conditionally set with_selection_only based on whether the wallet has
any Core balance-backed entries (use the same wallet/address inspection helpers
such as extract_wallet_entries/address_balances/known_addresses to detect
emptiness) so new wallets can type an address to bootstrap funding.

---

Nitpick comments:
In `@src/database/initialization.rs`:
- Around line 1145-1148: The two assertions checking the initialized DB version
are redundant; remove one and keep the semantic one using the constant. Replace
the pair of asserts on variable `version` with a single assertion
`assert_eq!(version, DEFAULT_DB_VERSION);` (remove the literal
`assert_eq!(version, 33);`) so the test uses the named constant
`DEFAULT_DB_VERSION` instead of a magic number; locate this in the
initialization test where `version` is asserted.
- Around line 928-932: The migration ALTER TABLE statement that adds the status
column uses DEFAULT 2 while the fresh CREATE TABLE in
initialize_wallet_transactions_table uses DEFAULT 0, creating a schema
inconsistency; update the ALTER TABLE call (the conn.execute that runs "ALTER
TABLE wallet_transactions ADD COLUMN status INTEGER NOT NULL DEFAULT 2") to use
the same default as the fresh table (change DEFAULT 2 to DEFAULT 0) or
alternatively align initialize_wallet_transactions_table to DEFAULT 2 so both
code paths match—pick one and make the defaults consistent across
initialize_wallet_transactions_table and the migration SQL.
- Around line 925-937: Consolidate the duplicated explanatory comment about
default status for the migration by removing one of the two identical comment
blocks surrounding the ALTER TABLE call: keep a single comment either
immediately above the conn.execute invocation or inside the SQL argument, and
remove the other; target the code around conn.execute(...) performing "ALTER
TABLE wallet_transactions ADD COLUMN status INTEGER NOT NULL DEFAULT 2" in
initialization.rs to make this change.
- Around line 984-1006: The assert_column_exists helper should avoid
interpolating the column name directly; update assert_column_exists to
parameterize the column argument (keep the table name formatting if PRAGMA can't
be parameterized) by changing the query to format!("SELECT COUNT(*) FROM
pragma_table_info('{table}') WHERE name=?1") and pass params![column] to
query_row, so the function assert_column_exists(conn, table, column) uses a
parameter for the column name similar to assert_table_exists while leaving table
handling as-is.

In `@src/model/address.rs`:
- Around line 74-76: The model now depends on crate::ui via
is_platform_address_string(trimmed); remove that UI dependency by moving the
Bech32m platform-prefix helper into the model (e.g., add a private pub(crate) fn
is_platform_address_string or a small helpers module inside
src/model/address.rs) and replace the call in the AddressKind detection with
that local helper. Also update any callers in src/ui/components/address_input.rs
(and other UI code) to import the helper from the model (e.g.,
model::address::is_platform_address_string or
model::address::helpers::is_platform_address_string) so the dependency flows
from UI -> model only. Ensure no other references to crate::ui::helpers remain
in src/model/address.rs.

🪄 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: 3018d3c6-3e6d-4144-8955-8e7a22015c44

📥 Commits

Reviewing files that changed from the base of the PR and between 462be55 and c7a5ca7.

📒 Files selected for processing (35)

• CLAUDE.md
• src/app.rs
• src/backend_task/core/mod.rs
• src/backend_task/error.rs
• src/backend_task/identity/mod.rs
• src/backend_task/shielded/bundle.rs
• src/backend_task/shielded/sync.rs
• src/config.rs
• src/context/connection_status.rs
• src/context/mod.rs
• src/context/shielded.rs
• src/database/initialization.rs
• src/database/mod.rs
• src/database/shielded.rs
• src/database/wallet.rs
• src/model/address.rs
• src/model/amount.rs
• src/model/fee_estimation.rs
• src/model/mod.rs
• src/model/wallet/mod.rs
• src/ui/components/README.md
• src/ui/components/address_input.rs
• src/ui/components/mod.rs
• src/ui/identities/add_new_identity_screen/mod.rs
• src/ui/identities/top_up_identity_screen/mod.rs
• src/ui/mod.rs
• src/ui/network_chooser_screen.rs
• src/ui/wallets/send_screen.rs
• src/ui/wallets/shield_credits_screen.rs
• src/ui/wallets/shield_from_asset_lock_screen.rs
• src/ui/wallets/shielded_send_screen.rs
• src/ui/wallets/unshield_credits_screen.rs
• src/ui/wallets/wallets_screen/address_table.rs
• src/ui/wallets/wallets_screen/dialogs.rs
• src/ui/wallets/wallets_screen/mod.rs

[lklimek]

Review comments handled in #791