fix(ui): Send Dash screen improvements + unified shield screen merge

[lklimek]

Summary

• Send screen improvements: Multi-address Platform→Shielded allocation (with per-operation fee deduction), self-send prevention, highest-balance identity default, identity autocomplete in destinations, per-frame DB query optimization, platform address bootstrapping on wallet creation
• Merged feat/unified-shield-screen: Centralized fee estimation (fee_estimation.rs), theme-aware colors replacing hardcoded DashColors::SUCCESS, parallel batch nonce handling, iterative note selection, shield screen hardening with banners and fee-aware max, source_address field on ShieldFromAssetLock
• Merged v1.0-dev: Picks up PR feat(ui): unified shield screen with address selection #801 (unified shield screen with address selection)

Key changes

• Fee estimation centralized in model/fee_estimation.rs — both platform state transition fees and shielded fee calculations
• Max amount for Core→Shielded and Core→Core sends now deducts estimated fees
• All hardcoded success colors replaced with theme-aware success_color(dark_mode) for dark mode support
• Identity self-send guard added at dispatch time with user-friendly error message
• Shield screen uses graceful locks, fee-aware max, and progress banners

Review fixes applied

After code review triage, the following fixes were applied:

Backend (ae7b3471)

• SEC-001: Documented [0u8; 36] memo parameter at all 6 ZK proof builder call sites (confirmed as structured memo, not entropy)
• SEC-003: Replaced assume_checked() with require_network() for Core address validation in MCP withdrawal tools
• CODE-002: Replaced 24 lock .unwrap() calls with ? or .unwrap_or_else() in bundle.rs + shielded.rs
• CODE-009: Extracted extract_spends_and_anchor() helper, eliminating 3x witness extraction duplication
• PROJ-003: Added source_address parameter to MCP ShieldedShieldFromCore tool
• CODE-006: Added INTENTIONAL(CODE-006) comment for bootstrap address type check

UI (f5edcd31)

• CODE-001: Multi-address Platform→Shielded allocation now deducts per-operation fee per address
• CODE-002: Replaced lock .unwrap() with .ok()? in shield/send/shielded screens (5 files)
• CODE-003: Zero-amount shield blocked via .is_some_and(|v| v > 0)
• CODE-004: Fee headroom now uses fee_multiplier_permille from context instead of hardcoded 2x
• CODE-007: Balance/nonce cached in ShieldScreen struct, refreshed on events instead of per-frame RwLock reads
• PROJ-002: refresh_on_arrival() implemented for all 3 shielded screens

Test plan

• Manual test: Send screen Core→Shielded flow — verify max amount correctly deducts shield fees
• Manual test: Send screen Platform→Shielded with multi-address allocation — verify per-address fee deduction
• Manual test: Identity self-send prevention — verify error message appears
• Manual test: Dark mode — verify all theme-aware colors render correctly
• Manual test: Shield screen — verify zero-amount is rejected, fee headroom uses context multiplier
• Manual test: Navigate between shielded screens — verify balances refresh on arrival
• cargo clippy --all-features --all-targets -- -D warnings passes ✅
• cargo +nightly fmt --all passes ✅

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

Summary by CodeRabbit

• New Features

  • Platform-to-shielded transfers can aggregate funds from multiple source addresses; tasks run sequentially when needed.

• Improvements

  • Identity source now defaults to highest-balance identity.
  • Clearer insufficient-balance and self-send messages.
  • Destination input and send UI initialization more efficient and responsive.
  • Shielded screens refresh on arrival and use cached balances for snappier UI.
  • More robust lock handling to avoid panics and improve stability.
  • Reduced log verbosity for cookie-based RPC auth failures.
  • Better post-broadcast confirmation handling for shielded operations.

• Documentation

  • Clarified AddressInput autocomplete covers Core and Platform addresses; guidance added for identities and shielded data.

[coderabbitai]

Warning

Rate limit exceeded

@lklimek has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 45 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 45 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: daae7f79-65c3-45e6-8bb8-117ad63c5c0a

📥 Commits

Reviewing files that changed from the base of the PR and between 47b9fc1 and ed08be7.

📒 Files selected for processing (2)

• src/ui/wallets/shield_screen.rs
• src/ui/wallets/unshield_credits_screen.rs

📝 Walkthrough

Walkthrough

Lowered some cookie-auth log levels, broadened wallet bootstrap conditions, implemented multi-source Platform→Shielded allocation with fee headroom and sequential tasks, converted many panic-on-poison lock usages to fallible handling, added UI wallet caching/refresh hooks, and improved Core address parsing/validation.

Changes

Cohort / File(s)	Summary
Auth logging src/backend_task/core/mod.rs, src/context/mod.rs	Downgraded cookie-auth failure logs from debug! to trace! and adjusted message text; fallback to user/pass unchanged.
Wallet bootstrap src/context/wallet_lifecycle.rs	Bootstrap now runs when known_addresses is empty OR no watched_addresses entry with DerivationPathReference::PlatformPayment; switched to if let Ok(mut guard) = wallet.write() lock pattern.
Platform→Shielded aggregation & send UI src/ui/wallets/send_screen.rs, src/ui/wallets/shield_screen.rs, src/ui/wallets/shielded_send_screen.rs, src/ui/wallets/unshield_credits_screen.rs	Greedy allocation across multiple platform source addresses with per-op fee headroom; builds one task per source and returns sequential execution when >1. UI: non-panicking mutex usage, cached balances, refresh_on_arrival hooks, refined identity default selection and destination widget allocation.
Shielded backend safety & refactor src/backend_task/shielded/bundle.rs, src/context/shielded.rs	Replaced many unwrap() lock usages with fallible handling (? or controlled into_inner()), added extract_spends_and_anchor(...) helper, added confirmation wait (StateTransitionProofResult) after broadcasts (warnings on wait failures).
UI refresh & docs src/ui/mod.rs, src/ui/components/address_input.rs	Shielded screens now refresh on arrival; AddressInput::with_wallets doc clarified to Core+Platform only; removed TODO. UI lock reads made non-panicking with safe fallbacks.
MCP tools: address parsing & params src/mcp/tools/identity.rs, src/mcp/tools/shielded.rs	Core address parsing now uses parse::<Address<NetworkUnchecked>>() + .require_network(...) with clearer InvalidParam errors; ShieldFromCoreParams gained source_address: Option<String> to optionally restrict UTXO selection.
Minor messaging & wording src/ui/wallets/send_screen.rs, src/backend_task/core/mod.rs	Adjusted self-send wording and small log/message changes; replaced a few lock().unwrap() usages with non-panicking variants.

Sequence Diagram(s)

sequenceDiagram
  participant UI as "UI (Send/Shield screen)"
  participant App as "AppContext"
  participant Wallets as "Wallets store"
  participant Backend as "BackendTask Dispatcher"
  participant Shielded as "ShieldedState / CommitmentTree"

  UI->>App: request shield_action(amount, optional source filter)
  App->>Wallets: read platform addresses & balances
  Wallets-->>App: list of addresses with balances
  App->>App: sort by balance, compute fee headroom, greedy allocate across addresses
  App->>Backend: create 1..N ShieldCredits tasks (per allocated address)
  Backend->>Shielded: request anchor & witnesses (extract_spends_and_anchor)
  Shielded-->>Backend: anchor + spends
  Backend->>Shielded: submit shield operations (sequential if >1)
  Shielded-->>Backend: task results (wait_for_response StateTransitionProofResult)
  Backend-->>UI: final status/result

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• fix(shielded): fee and transfer calculation correctness issues #795 — Adds confirmation waits after shielded broadcasts and addresses shielded transaction correctness; overlaps with confirmation-wait changes in bundle.rs.

Possibly related PRs

• fix(ui): wallet screen identity, state management, and input validation (#797) #800 — Overlaps in wallet/shielded UI, bootstrap logic, and mutex/lock handling changes.

Suggested reviewers

• ogabrielides

Poem

🐰 I hopped through locks with a careful sniff,
I gathered addresses, richest ones first, swift,
No panics now — notes find their tree,
Tasks stride in sequence, shields bloom safely,
A rabbit’s small cheer for the tidy drift!

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately reflects the main changes: Send Dash screen improvements and unified shield screen merge are the primary objectives described in the PR summary.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/send-dash-screen-improvements

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/ui/wallets/send_screen.rs`:
- Around line 1552-1582: The current allocation loop builds multiple
ShieldedTask::ShieldCredits tasks from sorted_addrs and sends them via
mark_sending into BackendTasks with Sequential mode, but
run_backend_tasks_sequential will continue on errors causing irrecoverable
partial shielding; before creating tasks in the loop (or before returning
BackendTasks) pre-validate each platform address balance against amount to spend
plus estimated fee per ShieldCredits call (use the same fee estimation logic as
the shield flow) and reject/adjust the overall send if any address cannot cover
its share, or collapse to a single task when only one address can cover the full
amount; alternatively, if partial sends are permitted, switch to an execution
flow that records expected per-task effects and update display_task_result to
properly handle BackendTaskSuccessResult::Multiple with mixed success/failure
(and ensure the UI clearly reports which addresses succeeded and which failed)
so users are not surprised by irreversible partial shielding.
- Around line 1555-1572: The loop that builds ShieldedTask::ShieldCredits tasks
allocates spend = remaining.min(*balance) without reserving per-task fees, so
tasks can be created that lack funds for fees; update this allocation to use the
centralized fee estimator (PlatformFeeEstimator from model/fee_estimation.rs)
the same way allocate_platform_addresses/send_platform_to_platform do: estimate
per-shield-task fee and subtract it from available balance when computing spend,
skip an address if balance <= fee, and ensure remaining is reduced by spend (not
including reserved fees) while each created ShieldedTask includes amounts that
leave room for its fee.

🪄 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: 529a8d83-6fd5-473b-bf00-5d4c4bf390c3

📥 Commits

Reviewing files that changed from the base of the PR and between 29ef6e3 and 358ef68.

📒 Files selected for processing (5)

• src/backend_task/core/mod.rs
• src/context/mod.rs
• src/context/wallet_lifecycle.rs
• src/ui/components/address_input.rs
• src/ui/wallets/send_screen.rs

[thepastaclaw]

Review Gate

Commit: ed08be78

• ⏳ Debounce: 2m ago (need 30m)

• ⏳ CI checks: checks still running

• ✅ CodeRabbit review: comment found

• Run review now (check to override)

[coderabbitai]

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)

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

329-344: ⚠️ Potential issue | 🟠 Major

Keep batch failure text non-technical.

ShieldStage::label() renders error verbatim, and these paths now fill it with raw to_string() output plus strings like “nonce mismatch” and “Skipped: earlier nonce failed”. That leaks SDK/internal details into the progress UI. Please keep error as a short action-oriented sentence and move diagnostics to tracing and st_json, which you already expose separately.

As per coding guidelines: "All user-facing strings (labels, messages, tooltips, errors) must be simple, complete sentences." and "Never include technical details in user-facing error messages — no raw error strings, stack traces, SDK internals, or error codes. Attach via BannerHandle::with_details(e) instead."

Also applies to: 414-429, 449-473, 485-487

🤖 Prompt for AI Agents

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

In `@src/ui/wallets/shield_screen.rs` around lines 329 - 344, The code currently
injects raw error strings into ShieldStage::Failed (e.g., error: e.clone(),
st_json: None) which leaks technical details to the UI; instead, replace those
verbatim error assignments in the places that set ShieldStage::Failed (the match
arms that lock stage around ShieldStage::Failed) with a short, user-facing
sentence (e.g., "Failed to submit batch; please retry.") assigned to the error
field, serialize the original error into st_json (e.g., st_json:
Some(e.to_string()) or a structured diagnostic) and emit the full error to
logs/tracing and/or attach it to any BannerHandle via
BannerHandle::with_details(e) so diagnostics remain available but not shown
verbatim in ShieldStage::label(). Ensure changes are applied to all similar
blocks referenced (the shown block and the other occurrences noted).

♻️ Duplicate comments (2)

src/ui/wallets/send_screen.rs (2)

1545-1580: ⚠️ Potential issue | 🔴 Critical

Reject partial Platform→Shielded allocations before returning tasks.

The upfront check uses raw balances, but the loop subtracts a shield fee from every address. If that fee-adjusted capacity is smaller than amount_credits, remaining stays positive and we still enqueue a partial set of ShieldCredits tasks. That can irreversibly shield less than the user requested. Fail when allocation ends with nonzero remaining, and move the fee-headroom calculation into a shared helper so it stays consistent with the configured multiplier.

🛠 Suggested guard

         for (platform_addr, _, balance) in &sorted_addrs {
             if remaining == 0 {
                 break;
             }
             let available = balance.saturating_sub(per_op_fee);
             if available == 0 {
                 continue;
             }
             let spend = remaining.min(available);
             tasks.push(BackendTask::ShieldedTask(
                 crate::backend_task::shielded::ShieldedTask::ShieldCredits {
                     seed_hash,
                     amount: spend,
                     from_address: *platform_addr,
                     nonce_override: None,
                 },
             ));
             remaining -= spend;
         }
+
+        if remaining > 0 {
+            let max_sendable = amount_credits.saturating_sub(remaining);
+            return Err(format!(
+                "Insufficient platform balance. Need {} but only {} is available after estimated shield fees.",
+                format_credits_as_dash(amount_credits),
+                format_credits_as_dash(max_sendable),
+            ));
+        }

As per coding guidelines: "All fee estimation logic must be centralized in model/fee_estimation.rs — never inline fee math in UI or backend task code."

🤖 Prompt for AI Agents

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

In `@src/ui/wallets/send_screen.rs` around lines 1545 - 1580, The allocation
currently uses raw balances (sorted_addrs) but subtracts per-op fee inline
(shielded_fee_for_actions) when building tasks, which can leave remaining > 0
and produce a partial set of ShieldCredits tasks; change this by moving the fee
headroom calculation into the centralized helper in model/fee_estimation.rs (add
a function to compute per-address available balance given the configured
multiplier), use that helper when checking total capacity and when iterating
(use the same per_op_fee logic consistently instead of calling
shielded_fee_for_actions inline), and if after the loop remaining != 0 return an
Err indicating insufficient fee-adjusted capacity rather than returning partial
BackendTask::ShieldedTask::ShieldCredits results.

---

1583-1589: ⚠️ Potential issue | 🟠 Major

Sequence multiple shields through a batch-aware result path.

BackendTasks(...Sequential) turns one send into multiple independent ShieldCredits operations, but the screen still treats each ShieldedCreditsShielded result as a complete send. A two-address shield will finish with the last fragment’s amount in the success screen, and a later failure can hide earlier successful fragments entirely. This needs an aggregate batch result or explicit accumulation in display_task_result() / display_message().

🤖 Prompt for AI Agents

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

In `@src/ui/wallets/send_screen.rs` around lines 1583 - 1589, The current branch
returns multiple ShieldCredits as separate BackendTasks which leads
display_task_result()/display_message() to treat each ShieldedCreditsShielded as
a complete send; change the flow to produce and handle a batch-aware result:
when tasks.len() > 1 return a batch result (e.g., BackendTasksBatch or include
an aggregate enum variant) instead of BackendTasks(...Sequential), and update
display_task_result() and display_message() to recognize that batch variant and
accumulate per-fragment outcomes (sum amounts for successful fragments, collect
per-recipient success/failure and errors) so the success screen shows the
aggregated total and partial failures rather than only the last fragment; refer
to BackendTasks, BackendTask, ShieldCredits, display_task_result(), and
display_message() when locating and implementing these changes.

🧹 Nitpick comments (1)

src/mcp/tools/shielded.rs (1)

32-33: Use a full sentence for the source_address schema description.

This doc comment is exposed in the generated tool schema, so the parenthetical fragment is harder to localize cleanly.

✏️ Suggested wording

-    /// Optional Core address to fund from (restricts UTXO selection to this address)
+    /// Optional Core address to fund the transaction from. When provided, UTXO selection is restricted to this address.

As per coding guidelines, "All user-facing strings (labels, messages, tooltips, errors) must be simple, complete sentences."

🤖 Prompt for AI Agents

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

In `@src/mcp/tools/shielded.rs` around lines 32 - 33, The doc comment for the
struct field `source_address` is a fragment; change it to a full sentence so the
generated tool schema shows a complete user-facing description. Update the
comment above `pub source_address: Option<String>,` in src/mcp/tools/shielded.rs
to something like: "Optional Core address to fund from. This restricts UTXO
selection to the specified address." or an equivalent single-sentence
description that preserves the parenthetical meaning.

🤖 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/context/shielded.rs`:
- Around line 110-117: The mutex poison handlers in bump_platform_address_nonce
and set_platform_address_nonce must stop silently recovering (the
unwrap_or_else(|e| e.into_inner()) calls around self.wallets.read() and
wallet_arc.write()) and instead propagate lock-poison via
TaskError::LockPoisoned; change each helper to return Result<..., TaskError> (or
Result<(), TaskError>) and replace the unwrap_or_else calls with ?-style
propagation (e.g., self.wallets.read().map_err(|_| TaskError::LockPoisoned)? and
wallet_arc.write().map_err(|_| TaskError::LockPoisoned)?). If the API cannot
change to return a Result, log a clear warning before any recovery (mirroring
context_provider.rs) and avoid persisting recovered state without explicit
caller handling; apply the same changes to the other helper that uses the same
pattern.

In `@src/mcp/tools/identity.rs`:
- Around line 468-474: The current map_err closures on the Core address parsing
and require_network calls leak parser/SDK error details by interpolating {e};
change both to return generic McpToolError::InvalidParam messages (e.g.,
"Invalid Core address" and "Core address does not match active network") without
including the error variable, updating the map_err closures that produce
McpToolError::InvalidParam for the parser path and the require_network() call so
they no longer interpolate or propagate the original error string.

In `@src/mcp/tools/shielded.rs`:
- Around line 94-100: Replace user-facing error strings that interpolate the
underlying parser error (e.g., map_err closures producing
McpToolError::InvalidParam with format!("... {e}")) with generic, stable
messages that do not include the SDK/parser error text; specifically update the
map_err after the Core address parse and the map_err after require_network to
return fixed messages like "Invalid source Core address" and "Source address
does not match active network" (and make the same change for the similar
occurrences around lines 522–527), keeping McpToolError::InvalidParam but
removing any `{e}` or underlying error details.

---

Outside diff comments:
In `@src/ui/wallets/shield_screen.rs`:
- Around line 329-344: The code currently injects raw error strings into
ShieldStage::Failed (e.g., error: e.clone(), st_json: None) which leaks
technical details to the UI; instead, replace those verbatim error assignments
in the places that set ShieldStage::Failed (the match arms that lock stage
around ShieldStage::Failed) with a short, user-facing sentence (e.g., "Failed to
submit batch; please retry.") assigned to the error field, serialize the
original error into st_json (e.g., st_json: Some(e.to_string()) or a structured
diagnostic) and emit the full error to logs/tracing and/or attach it to any
BannerHandle via BannerHandle::with_details(e) so diagnostics remain available
but not shown verbatim in ShieldStage::label(). Ensure changes are applied to
all similar blocks referenced (the shown block and the other occurrences noted).

---

Duplicate comments:
In `@src/ui/wallets/send_screen.rs`:
- Around line 1545-1580: The allocation currently uses raw balances
(sorted_addrs) but subtracts per-op fee inline (shielded_fee_for_actions) when
building tasks, which can leave remaining > 0 and produce a partial set of
ShieldCredits tasks; change this by moving the fee headroom calculation into the
centralized helper in model/fee_estimation.rs (add a function to compute
per-address available balance given the configured multiplier), use that helper
when checking total capacity and when iterating (use the same per_op_fee logic
consistently instead of calling shielded_fee_for_actions inline), and if after
the loop remaining != 0 return an Err indicating insufficient fee-adjusted
capacity rather than returning partial BackendTask::ShieldedTask::ShieldCredits
results.
- Around line 1583-1589: The current branch returns multiple ShieldCredits as
separate BackendTasks which leads display_task_result()/display_message() to
treat each ShieldedCreditsShielded as a complete send; change the flow to
produce and handle a batch-aware result: when tasks.len() > 1 return a batch
result (e.g., BackendTasksBatch or include an aggregate enum variant) instead of
BackendTasks(...Sequential), and update display_task_result() and
display_message() to recognize that batch variant and accumulate per-fragment
outcomes (sum amounts for successful fragments, collect per-recipient
success/failure and errors) so the success screen shows the aggregated total and
partial failures rather than only the last fragment; refer to BackendTasks,
BackendTask, ShieldCredits, display_task_result(), and display_message() when
locating and implementing these changes.

---

Nitpick comments:
In `@src/mcp/tools/shielded.rs`:
- Around line 32-33: The doc comment for the struct field `source_address` is a
fragment; change it to a full sentence so the generated tool schema shows a
complete user-facing description. Update the comment above `pub source_address:
Option<String>,` in src/mcp/tools/shielded.rs to something like: "Optional Core
address to fund from. This restricts UTXO selection to the specified address."
or an equivalent single-sentence description that preserves the parenthetical
meaning.

🪄 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: ec525b8c-9a49-4e52-87f9-490902c610a8

📥 Commits

Reviewing files that changed from the base of the PR and between 358ef68 and f5edcd3.

📒 Files selected for processing (10)

• src/backend_task/shielded/bundle.rs
• src/context/shielded.rs
• src/context/wallet_lifecycle.rs
• src/mcp/tools/identity.rs
• src/mcp/tools/shielded.rs
• src/ui/mod.rs
• src/ui/wallets/send_screen.rs
• src/ui/wallets/shield_screen.rs
• src/ui/wallets/shielded_send_screen.rs
• src/ui/wallets/unshield_credits_screen.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/context/wallet_lifecycle.rs

[thepastaclaw]

Code Review

Verified 3 findings (1 blocking, 2 suggestions). The convergent multi-address allocation underfunding bug is confirmed — Platform→Shielded allocation accepts amounts that exceed post-fee spendable balance and can produce empty or under-funded task lists.

Reviewed commit: 2c9fe49

🔴 1 blocking | 🟡 2 suggestion(s)

1 additional finding

🟡 suggestion: Unified send requires a shielded destination for Core/Platform->Shielded even though the destination is not used

src/ui/wallets/send_screen.rs (lines 1419-1427)

Verified. Both shielding handlers require validated_destination to be a shielded address (1419-1427 and 1525-1530), but the comments state shielding deposits into the wallet's own shielded pool, and neither backend task includes the destination address. ShieldFromAssetLock is created only with { seed_hash, amount_duffs, source_address: None } (1448-1453), and ShieldCredits is created only with { seed_hash, amount, from_address, nonce_override } (1571-1577). The UI therefore asks for a shielded destination that has no effect on execution.

🤖 Prompt for all review comments with AI agents

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

In `src/ui/wallets/send_screen.rs`:
- [BLOCKING] lines 1520-1590: Platform->Shielded allocation can silently underfund the requested amount or dispatch an empty task list
  Verified. `send_platform_to_shielded()` first accepts any request where `amount_credits <= total_available` using the raw sum of balances (`1545-1552`), but then allocates each address with `balance.saturating_sub(per_op_fee)` (`1566`). Unlike the platform->platform and platform->core paths, it never checks whether `remaining` is still nonzero after allocation and never rejects `tasks.is_empty()`. As a result, a request can be accepted even when the post-fee spendable total is insufficient, producing either a partial sequence of `ShieldCredits` tasks for less than the requested amount or `AppAction::BackendTasks([])` if every address balance is at or below the reserved fee.
- [SUGGESTION] lines 1554-1559: Platform->Shielded per-address fee reservation ignores the current fee multiplier
  Verified. The per-operation headroom is computed as `shielded_fee_for_actions(2, PlatformVersion::latest())` with no multiplier adjustment. In contrast, the shield screen reserves `shielded_fee_for_actions(2, PlatformVersion::latest())` and then applies `self.app_context.fee_multiplier_permille().max(1000)` before subtracting it from the available balance (`src/ui/wallets/shield_screen.rs:777-786`). This means unified Platform->Shielded can overestimate spendable balance whenever the network fee multiplier is above 1x.
- [SUGGESTION] lines 1419-1427: Unified send requires a shielded destination for Core/Platform->Shielded even though the destination is not used
  Verified. Both shielding handlers require `validated_destination` to be a shielded address (`1419-1427` and `1525-1530`), but the comments state shielding deposits into the wallet's own shielded pool, and neither backend task includes the destination address. `ShieldFromAssetLock` is created only with `{ seed_hash, amount_duffs, source_address: None }` (`1448-1453`), and `ShieldCredits` is created only with `{ seed_hash, amount, from_address, nonce_override }` (`1571-1577`). The UI therefore asks for a shielded destination that has no effect on execution.

[coderabbitai]

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/shielded/bundle.rs (1)

490-503: ⚠️ Potential issue | 🟠 Major

Remove the pending-finality entry on every Step 3 failure.

After tx_id is inserted into transactions_waiting_for_finality, any failure in Step 3 (core_client.read() or send_raw_transaction()) exits through ? and leaves a stale pending entry behind. That makes the finality tracker inconsistent with a failed asset-lock submission, unlike the cleanup pattern already used in src/backend_task/core/create_asset_lock.rs.

Suggested fix

-    app_context
-        .core_client
-        .read()
-        .map_err(|_| TaskError::LockPoisoned {
-            resource: "core_client",
-        })?
-        .send_raw_transaction(&asset_lock_transaction)?;
+    let broadcast_result = (|| -> Result<(), TaskError> {
+        app_context
+            .core_client
+            .read()
+            .map_err(|_| TaskError::LockPoisoned {
+                resource: "core_client",
+            })?
+            .send_raw_transaction(&asset_lock_transaction)?;
+        Ok(())
+    })();
+
+    if let Err(e) = broadcast_result {
+        if let Ok(mut proofs) = app_context.transactions_waiting_for_finality.lock() {
+            proofs.remove(&tx_id);
+        } else {
+            tracing::warn!(
+                "Failed to clean up finality tracking for asset lock transaction {tx_id}"
+            );
+        }
+        return Err(e);
+    }

🤖 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 490 - 503, After inserting
tx_id into transactions_waiting_for_finality, any early return from
app_context.core_client.read() or .send_raw_transaction(&asset_lock_transaction)
can leave a stale entry; modify the Step 3 call sites so that on any error you
first remove the pending entry (lock
app_context.transactions_waiting_for_finality and remove tx_id) before returning
the TaskError. Concretely, replace the chained map_err/? usage around
app_context.core_client.read() and the subsequent send_raw_transaction call with
explicit error handling: attempt core_client.read(), on Err remove tx_id then
return TaskError::LockPoisoned; then call send_raw_transaction, and on Err
remove tx_id then propagate the send error. Use the existing symbols
transactions_waiting_for_finality, tx_id, app_context.core_client.read(),
send_raw_transaction, and asset_lock_transaction to locate and update the code
paths.

🤖 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/shielded/bundle.rs`:
- Around line 828-846: Replace the current error messages that pipe
commitment-tree internals (from tree.witness() and tree.anchor()) into
TaskError::ShieldedMerkleWitnessUnavailable by returning a single non-technical,
user-facing message (e.g. "Unable to verify shielded note; please retry or
contact support") and move the original low-level error (the `e` value) into
diagnostic/details metadata via BannerHandle::with_details(e) or equivalent;
update both the map_err branches used around merkle_path (the tree.witness(...)
closure) and the anchor = tree.anchor() error mapping so they produce the fixed
everyday-user message while attaching the original error as details.

---

Outside diff comments:
In `@src/backend_task/shielded/bundle.rs`:
- Around line 490-503: After inserting tx_id into
transactions_waiting_for_finality, any early return from
app_context.core_client.read() or .send_raw_transaction(&asset_lock_transaction)
can leave a stale entry; modify the Step 3 call sites so that on any error you
first remove the pending entry (lock
app_context.transactions_waiting_for_finality and remove tx_id) before returning
the TaskError. Concretely, replace the chained map_err/? usage around
app_context.core_client.read() and the subsequent send_raw_transaction call with
explicit error handling: attempt core_client.read(), on Err remove tx_id then
return TaskError::LockPoisoned; then call send_raw_transaction, and on Err
remove tx_id then propagate the send error. Use the existing symbols
transactions_waiting_for_finality, tx_id, app_context.core_client.read(),
send_raw_transaction, and asset_lock_transaction to locate and update the code
paths.

🪄 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: 70e37ff0-1aba-4c5e-ae95-f00964de3ff8

📥 Commits

Reviewing files that changed from the base of the PR and between 827bfa8 and 47b9fc1.

📒 Files selected for processing (1)

• src/backend_task/shielded/bundle.rs

[Copilot]

Pull request overview

This PR improves wallet send + shielded UX/performance and merges the unified shield screen work by hardening lock handling, centralizing fee estimation usage in UI flows, and enhancing platform→shielded sends (multi-address allocation + fee headroom). It also extends MCP shield tools with an optional Core source address and makes wallet address bootstrapping more robust.

Changes:

• Add refresh-on-arrival behavior for shielded screens and cache wallet balances/nonces to avoid per-frame lock reads.
• Implement multi-address greedy allocation for Platform→Shielded sends with per-operation fee headroom and improved self-send prevention.
• Harden backend/UI locking and shield transition flow (lock poisoning handling, witness extraction helper, optional confirmation wait, reduced cookie-auth log verbosity).

Reviewed changes

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

Show a summary per file

File	Description
src/ui/wallets/unshield_credits_screen.rs	Avoid panicking on shielded state lock + refresh max balance when arriving.
src/ui/wallets/shielded_send_screen.rs	Avoid panicking on shielded state lock + refresh max balance when arriving.
src/ui/wallets/shield_screen.rs	Cache nonce/balances and refresh them on arrival/source change/task completion; fee headroom uses context multiplier; block zero-amount.
src/ui/wallets/send_screen.rs	Multi-address Platform→Shielded allocation with per-op fee reservation; self-send prevention; destination input preloading for fewer per-frame queries.
src/ui/mod.rs	Wire shielded screens into refresh() / refresh_on_arrival() dispatch.
src/ui/components/address_input.rs	Clarify with_wallets() only provides Core/Platform autocomplete and how to add identities/shielded entries.
src/mcp/tools/shielded.rs	Add optional source_address for Core→Shielded tool; tighten Core address parsing/network validation.
src/mcp/tools/identity.rs	Tighten Core address parsing/network validation for identity withdrawal tool.
src/context/wallet_lifecycle.rs	Broaden wallet address bootstrapping to cover wallets missing PlatformPayment derivations.
src/context/shielded.rs	Replace lock panics with poisoned-lock recovery or ? propagation in shielded context operations.
src/context/mod.rs	Reduce cookie-auth failure log from debug → trace.
src/backend_task/shielded/bundle.rs	Reduce panics on locks, dedupe witness extraction, add best-effort confirmation wait after broadcasts.
src/backend_task/core/mod.rs	Reduce cookie-auth failure log from debug → trace.

---

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