Skip to content

feat(ui): unified send screen — all source/destination combinations#798

Merged
lklimek merged 176 commits into
v1.0-devfrom
feat/unified-send
Mar 26, 2026
Merged

feat(ui): unified send screen — all source/destination combinations#798
lklimek merged 176 commits into
v1.0-devfrom
feat/unified-send

Conversation

@lklimek
Copy link
Copy Markdown
Contributor

@lklimek lklimek commented Mar 26, 2026

Summary

Extends the Send screen to support ALL 14 viable Source×Destination combinations through one unified interface, plus 13 new MCP/CLI tools.

Matrix (4×4)

Source / Dest Core Platform Shielded Identity
Core Wallet ✅ exists ✅ exists 🆕 shield 🆕 top up
Platform ✅ exists ✅ exists 🆕 shield 🆕 top up
Identity 🆕 withdraw 🆕 transfer ❌ v2 🆕 transfer
Shielded 🆕 withdraw ✅ exists ✅ exists ❌ v2
  • 8 new UI wirings — all backend tasks already exist
  • 13 new MCP tools — thin adapters, zero CLI changes needed
  • 0 new BackendTask variants — everything maps to existing tasks
  • Progressive disclosure — Alex: Core→Core only, Priya: transparent combos, Jordan: everything

Test plan

  • Verify all 14 combinations work (6 existing + 8 new)
  • Verify 2 impossible combinations show clear error message
  • Test progressive disclosure per persona
  • Test Identity source selection dropdown
  • Test multi-step progress UI for asset lock operations
  • Verify all 13 MCP tools via det-cli
  • Verify existing 6 combinations are not regressed

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • New Identity credit flows (top-up, platform top-up, transfer, withdraw, send-to-platform) and Shielded pool flows (shield from Core/platform, shielded transfer, unshield, withdraw).
    • Unified Send UI: Identity as a source, expanded source→destination routing, validations, fee hints, and updated result handling.
  • Documentation

    • Added Unified Send requirements & architecture and expanded MCP tools list; destructive tool network handling is now stricter.
  • Bug Fixes

    • Clearer wallet error messages, improved wallet-unlock and shielded-sync behavior, streamlined “add receive address” flow, and tighter address autocomplete filtering.

QuantumExplorer and others added 30 commits February 18, 2026 00:57
…627)

During SPV reconciliation, per_address_sum only contains addresses with
current UTXOs. Addresses whose funds were fully spent never had their
balance reset to zero, causing the address table to display stale
non-zero balances even though UTXO count correctly showed 0.

Now explicitly zeroes address_balances for any known address absent from
the refreshed UTXO map before applying current sums.

Closes #571

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…613)

* fix: handle malformed YAML gracefully in load_testnet_nodes_from_yml

Replace .expect() with match expression to avoid panicking when
.testnet_nodes.yml contains malformed YAML. Instead, logs the error
with tracing::error and returns None, allowing the application to
continue without crashing.

Closes #557

Co-authored-by: lklimek <lklimek@users.noreply.github.com>

* fix: propagate YAML parse errors to UI and remove unwrap calls

- Change load_testnet_nodes_from_yml to return Result<Option<TestnetNodes>, String>
  so parse errors display in the UI error banner instead of only logging
- Use explicit type annotation on serde_yaml_ng::from_str::<TestnetNodes>
- Replace .unwrap() with .and_then() in fill_random_hpmn/fill_random_masternode

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

---------

Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Co-authored-by: lklimek <lklimek@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* chore: move doc/ contents into docs/ and update references

Consolidate documentation under a single docs/ directory.

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

* chore: CLAUDE.md should write manual test scenarios for PRs

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
…tion (#603)

* build(flatpak): use only-arches for dynamic protoc architecture selection

Replace the fragile sed-based CI patching of the Flatpak manifest with
Flatpak's native `only-arches` source selector. The protoc module now
declares both x86_64 and aarch64 sources inline, and build-commands use
a glob pattern (`protoc-*.zip`) so no per-arch fixup is needed.

Changes:
- flatpak manifest: add aarch64 protoc source with `only-arches`,
  use glob in unzip commands, remove stale CI-patching comment
- CI workflow: remove `protoc-zip`/`protoc-sha256` matrix variables
  and the "Patch manifest for architecture" step

https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG

* fix(flatpak): use dest-filename for deterministic protoc extraction

Use dest-filename to normalize both arch-specific protoc zips to a
common name, avoiding glob expansion in build-commands. This ensures
the unzip target is deterministic regardless of shell behavior in the
Flatpak sandbox.

https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG

* build: update platform to b445b6f0 and remove rust-dashcore patches

Update dashpay/platform dependency from d6f4eb9a to b445b6f0e0bd4863
(3.0.1 → 3.1.0-dev.1). Remove the [patch] section that pinned
rust-dashcore crates to a separate rev, as the new platform commit
resolves them correctly on its own.

https://claude.ai/code/session_015AD2pCWoJdV2VDydcqFHPG

---------

Co-authored-by: Claude <noreply@anthropic.com>
Resolve conflicts preserving shielded pool features from HEAD while
adopting v1.0-dev improvements: refactored asset lock fee calculation,
mine dialog hardening, network validation, and API signature updates.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The [patch] section referenced ../platform local paths that don't exist
in CI. Since dash-sdk already depends on the feat/zk branch, all
transitive platform crates resolve correctly without patches. Also fixes
a formatting issue in address_table.rs.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The remove_utxos tests were failing because `register_test_address`
inserts into `wallet_addresses` which has a foreign key constraint on
`wallet(seed_hash)`. Added the missing `store_wallet` call so the
parent row exists before inserting child address rows.

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

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	src/backend_task/core/mod.rs
#	src/backend_task/wallet/fetch_platform_address_balances.rs
#	src/context/wallet_lifecycle.rs
#	src/database/wallet.rs
#	src/model/wallet/mod.rs
#	src/ui/wallets/wallets_screen/mod.rs
The `pub mod shielded;` declaration was accidentally removed from
`src/model/wallet/mod.rs` during the v1.0-dev merge, causing build
failures since the shielded.rs file still exists.

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

* fix(test): restore store_wallet calls lost in merge

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
…rged

Resolve conflicts in Cargo.toml (keep feat/zk branch), Cargo.lock
(regenerate with pinned platform rev 4d7b9be5), and
backend_task/mod.rs (combine TaskError wrapping with ShieldedTask).

Fix post-merge integration issues:
- SPV manager: remove stale .await on subscribe methods, add
  command_receiver channel for updated DashSpvClient::run() API
- send_screen: update SendStatus::WaitingForResult to unit variant
- network_chooser_screen: handle new SyncState::Initializing variant

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

# Conflicts:
#	.github/workflows/clippy.yml
#	.github/workflows/tests.yml
#	CLAUDE.md
#	Cargo.lock
#	Cargo.toml
#	src/app.rs
#	src/backend_task/core/mod.rs
#	src/backend_task/error.rs
#	src/backend_task/mod.rs
#	src/context/mod.rs
#	src/database/initialization.rs
#	src/spv/manager.rs
#	src/ui/components/top_panel.rs
#	src/ui/components/wallet_unlock.rs
#	src/ui/contracts_documents/add_contracts_screen.rs
#	src/ui/contracts_documents/document_action_screen.rs
#	src/ui/contracts_documents/group_actions_screen.rs
#	src/ui/contracts_documents/register_contract_screen.rs
#	src/ui/contracts_documents/update_contract_screen.rs
#	src/ui/dashpay/contact_requests.rs
#	src/ui/dashpay/profile_screen.rs
#	src/ui/dashpay/profile_search.rs
#	src/ui/identities/add_new_identity_screen/by_wallet_qr_code.rs
#	src/ui/identities/identities_screen.rs
#	src/ui/identities/keys/key_info_screen.rs
#	src/ui/mod.rs
#	src/ui/tokens/destroy_frozen_funds_screen.rs
#	src/ui/tokens/direct_token_purchase_screen.rs
#	src/ui/tokens/freeze_tokens_screen.rs
#	src/ui/tokens/mint_tokens_screen.rs
#	src/ui/tokens/pause_tokens_screen.rs
#	src/ui/tokens/resume_tokens_screen.rs
#	src/ui/tokens/set_token_price_screen.rs
#	src/ui/tokens/tokens_screen/my_tokens.rs
#	src/ui/tokens/unfreeze_tokens_screen.rs
#	src/ui/tokens/update_token_config.rs
#	src/ui/tools/masternode_list_diff_screen.rs
#	src/ui/wallets/asset_lock_detail_screen.rs
#	src/ui/wallets/create_asset_lock_screen.rs
#	src/ui/wallets/single_key_send_screen.rs
#	src/ui/wallets/wallets_screen/mod.rs
Migrate from feat/zk to 3.1-dev branch of dashpay/platform,
adapting to breaking API changes in dash-spv, key-wallet, and dpp:

- FeeLevel removed; use FeeRate::normal() directly
- DashSpvClientInterface/Command removed; use DashSpvClient directly
- SyncState::Initializing removed; replaced with WaitForEvents
- NetworkExt trait inlined into Network impl
- OrchardProver now requires wrapper struct around ProvingKey
- OrchardAddress::from_raw_bytes now returns Result
- Builder functions gain fee/platform_version params
- NullifierSyncConfig API uses NullifierSyncCheckpoint
- WalletManager.create_unsigned_payment_transaction removed;
  use TransactionBuilder directly
- Work around Send lifetime issues with spawn_blocking

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…kError

Replace all Result<T, String> error patterns in the shielded pool module
with typed TaskError variants, aligning with the codebase-wide typed error
migration (PR #739).

New TaskError variants: ShieldedNoUnspentNotes, ShieldedInsufficientBalance,
PlatformAddressNotFound, ShieldedMerkleWitnessUnavailable,
ShieldedTransitionBuildFailed, ShieldedBroadcastFailed,
ShieldedInvalidRecipientAddress, ShieldedAssetLockTimeout,
ShieldedSyncFailed, ShieldedTreeUpdateFailed, ShieldedNullifierSyncFailed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge origin/zk branch which adapts to dashpay/platform v3.1-dev API:
- OrchardProver trait wrapper (CachedProver) replaces raw ProvingKey refs
- OrchardAddress::from_raw_bytes now returns Result
- FeeLevel removed; use FeeRate::normal() directly
- WalletManager.create_unsigned_payment_transaction removed; use TransactionBuilder
- SyncState::Initializing removed; replaced with WaitForEvents
- SpvClient ownership model changed (no longer Arc-wrapped)

Resolved conflicts preserving typed TaskError variants from our branch
while adopting origin/zk's v3.1-dev API adaptations. Fixed branch name
typo (3.1-dev → v3.1-dev) in Cargo.toml.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
backport: merge v1.0-dev (including extracted non-ZK fixes) back into zk
Reserve width for Save and Auto Update buttons so the password input
doesn't consume all available space, pushing buttons off-screen.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nto zk-fixes

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	src/app.rs
#	src/backend_task/core/mod.rs
#	src/backend_task/error.rs
#	src/database/initialization.rs
#	src/spv/manager.rs
#	src/ui/wallets/wallets_screen/mod.rs
…how details

Add CoreRpcConnectionFailed variant to TaskError that includes the
configured address in the user-facing message.  Connection-refused
errors are now detected via is_rpc_connection_error() and enriched
with host:port at every RPC call site where the URL is known
(AppContext::rpc_error_with_url helper).

Details panel is now shown for all RPC-related errors regardless of
developer mode, so users can always see the technical information
they need to diagnose connectivity issues.

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

The Network Chooser screen had three bugs related to RPC password handling:

1. Password input was initialized from Regtest config only, ignoring the
   current network selection.
2. Password UI was hidden for all networks except Regtest, even though
   Mainnet/Testnet/Devnet also use RPC mode.
3. Save logic was hardcoded to update Regtest config and triggered a
   SwitchNetwork(Regtest) action, which disconnected the active network's
   ZMQ listener unnecessarily.

Now the password input shows for any network in RPC mode, reads/writes
the correct network config, reinits the RPC client in-place without
triggering a network switch, and reloads the stored password when the
user switches network tabs. The "Auto Update" (dashmate) button remains
Regtest-only since dashmate is only relevant for local networks.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek and others added 3 commits March 26, 2026 13:21
Add SND-007 through SND-013 user stories for new send combinations.
Add 10 new tools to MCP.md tool reference table.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…inations

Wire 8 new send combinations into the Send screen routing:
- Core->Shielded, Core->Identity, Platform->Shielded, Platform->Identity
- Shielded->Core, Identity->Core, Identity->Platform, Identity->Identity

Add Identity as 4th source type with selector dropdown, progressive
disclosure (developer mode gates shielded, identities shown when loaded),
and display_task_result handlers for all new backend result types.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add identity tools: topup (core/platform), transfer, withdraw, to-address.
Add shielded tools: shield-from-core, shield-from-platform, transfer,
unshield, withdraw. Add resolve::qualified_identity() helper and
validate_credits() for shared parameter validation.

Box QualifiedIdentity in SourceSelection::Identity to fix clippy
large_enum_variant warning.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Base automatically changed from feat/wallet-ui-redesign to v1.0-dev March 26, 2026 13:43
@lklimek lklimek marked this pull request as ready for review March 26, 2026 14:36
@lklimek lklimek requested a review from Copilot March 26, 2026 14:36
lklimek and others added 2 commits March 26, 2026 15:37
# Conflicts:
#	src/backend_task/core/mod.rs
#	src/context/wallet_lifecycle.rs
#	src/ui/components/address_input.rs
#	src/ui/wallets/wallets_screen/mod.rs
Restore address_input.rs from v1.0-dev (includes change/system
address filtering). Fix render_address_table calls to pass
account_filter parameter.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Extends the app’s “Send” and related UX to support the full set of viable Source×Destination flows (Core/Platform/Identity/Shielded), adds MCP tooling coverage for identity + shielded operations, and adjusts wallet/shielded UI and backend state handling to better surface balances and transaction ownership.

Changes:

  • Add Identity as a Send source and wire 8 additional send routing combinations to existing backend tasks; improve address type detection/validation messaging.
  • Add 10 new MCP tools (5 identity + 5 shielded) and register them in the MCP server; add resolver helpers.
  • Improve wallet UI (account tabs/system grouping, sync/status panels, explorer links), and make shielded state initialize on unlock + fix SPV is_ours semantics.

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
tests/backend-e2e/tx_is_ours.rs New E2E test covering SPV is_ours behavior for send/receive.
tests/backend-e2e/main.rs Registers the new E2E module.
src/ui/wallets/wallets_screen/mod.rs Refactors accounts area into tabs (incl. System), adds shielded balance into totals, updates tx filtering and header layout.
src/ui/wallets/unshield_credits_screen.rs Clears cached validated destination on network changes.
src/ui/wallets/shielded_tab.rs Moves init responsibility to backend, adds tick(), adds shielded address table + collapsible notes section.
src/ui/wallets/shielded_send_screen.rs Adds invalidate hook and improves success message w/ balance update.
src/ui/wallets/send_screen.rs Adds Identity source + routes new combinations; expands destination kinds; adds new backend result handling.
src/ui/wallets/account_summary.rs Renames labels, adds tab labels and default/dev visibility helpers; removes label field from summaries.
src/ui/tools/address_balance_screen.rs Adds invalidate hook to clear input/result on network changes.
src/ui/network_chooser_screen.rs Simplifies and hardens password auto-fill on network switch.
src/ui/mod.rs Invalidates relevant address inputs on context/network changes for more screens.
src/ui/identities/transfer_screen.rs Adds invalidate hook for input clearing on network changes.
src/ui/helpers.rs Re-exports platform address detection from model layer (removes UI-local implementation).
src/ui/components/address_input.rs Improves autocomplete source and user-facing validation messages; centralizes truncation helper; adjusts shielded-network checks.
src/model/address.rs Adds platform address detector + shared truncate helper; improves Core vs Identity detection heuristic + tests.
src/mcp/tools/shielded.rs Adds 5 shielded MCP tools (shield, transfer, unshield, withdraw).
src/mcp/tools/mod.rs Exposes new identity and shielded tool modules.
src/mcp/tools/identity.rs Adds 5 identity MCP tools (top-up, top-up-from-platform, transfer, withdraw, to-address).
src/mcp/server.rs Registers the new MCP tools with the router.
src/mcp/resolve.rs Adds qualified_identity() resolver and validate_credits().
src/database/mod.rs Avoids double-locking DB connection when clearing network data + commitment tree tables.
src/context/wallet_lifecycle.rs Initializes shielded state on unlock; queues background SyncNotes→CheckNullifiers; forces SPV is_ours=true.
src/context/shielded.rs Makes shielded init/sync/check helpers pub(crate) for lifecycle orchestration.
src/backend_task/shielded/sync.rs Deduplicates notes by position to avoid double-counting; warns on divergences.
src/backend_task/core/mod.rs Changes RPC last-error reporting and clears lingering error on success.
docs/user-stories.md Adds implemented user stories for new send combinations.
docs/ai-design/2026-03-26-unified-send/requirements.md Adds requirements doc for unified send + MCP coverage.
docs/ai-design/2026-03-26-unified-send/architecture.md Adds architecture doc describing routing + MCP tool layout.
docs/MCP.md Documents the new MCP tools.
Comments suppressed due to low confidence (1)

src/backend_task/core/mod.rs:207

  • rpc_last_error is shown in the UI even outside developer mode; changing it to format!("RPC error: {e}") risks leaking low-level details (and potentially sensitive connection/auth info, depending on dashcore_rpc::Error formatting) to end users. Recommend keeping a user-friendly sanitized message for non-dev mode and logging the detailed error separately (or only exposing the detailed string behind developer mode).
                    // Non-auth, non-connection error — show the actual error
                    // in the Networks page status display for debugging.
                    tracing::warn!(network = ?self.network, error = %e, "Chain lock query failed on active network");
                    Some(format!("RPC error: {e}"))
                } else {
                    // Successful chain lock fetch — clear any lingering RPC error
                    // so the connection status recovers after a transient outage.
                    self.connection_status.set_rpc_last_error(None);

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

Comment thread src/ui/wallets/send_screen.rs
Comment thread src/ui/wallets/send_screen.rs
Comment thread src/mcp/tools/identity.rs
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 18

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

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

⚠️ Outside diff range comments (3)
src/ui/wallets/wallets_screen/mod.rs (1)

1186-1210: ⚠️ Potential issue | 🟠 Major

Keep system sections keyed by account index.

src/ui/wallets/account_summary.rs:176-197 shows that AccountCategory::Bip32 carries a real account index. After this change, counts and balances are aggregated per category, then only matching.first().and_then(|s| s.index) is kept. The section header will therefore report totals for all Bip32 indexes, but render_address_table() only receives the first one, so addresses from the other indexes disappear from the System tab.

Also applies to: 1231-1250

🤖 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 1186 - 1210, The current
loop over all_system_categories collapses all summaries for a category (e.g.,
AccountCategory::Bip32) into one section and only keeps
matching.first().and_then(|s| s.index), which loses other Bip32 account indexes
and their addresses; change the grouping so sections are keyed by (category,
account_index) when an index exists: iterate summaries (or use precomputed
address_counts keys) and build sections per (s.category, s.index) instead of per
category, compute address_count from address_counts using that same composite
key, sum balances from matching entries for that key, and pass that index
through to render_address_table() so all Bip32 accounts remain visible (refer to
all_system_categories, precompute_address_counts, sections, summaries, and
render_address_table).
src/backend_task/core/mod.rs (2)

196-203: ⚠️ Potential issue | 🟠 Major

Keep the active-network RPC status message user-facing.

Line 203 now feeds the full dashcore_rpc::Error text into CoreItem::ChainLocks. That value is rendered in the Networks UI, so users will see transport/JSON-RPC internals instead of a short next step. Log e or attach it as technical details, but return a generic status string here.

💡 Suggested fix
-                    Some(format!("RPC error: {e}"))
+                    Some(
+                        "Could not check chain lock status. Check your Dash Core connection and try again."
+                            .to_string(),
+                    )

As per coding guidelines: User-facing error messages must follow these rules: (1) Write for Everyday User persona - no jargon. (2) Include what happened + what to do. (4) No technical details in message. Also, never expose raw backend/database errors to users.

🤖 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 196 - 203, The code returns the
raw RPC error text into CoreItem::ChainLocks (active_rpc_error) which is shown
to users; instead, replace the user-facing string Some(format!("RPC error:
{e}")) with a concise, non-technical message such as "Unable to reach the
network — retry or check your connection" and ensure the original error variable
e is still recorded via tracing::warn or attached to a non-user-facing field
(e.g., log/tracing or a technical_details/diagnostics structure) by using
Self::chain_lock_rpc_error or tracing::warn!(error = ?e) so developers retain
the full error while users see only the generic next-step guidance.

700-734: ⚠️ Potential issue | 🟠 Major

Don't collapse matchable wallet states into WalletPaymentFailed { detail: ... }.

These callsites now turn wallet-info/account lookup failures into free-form strings, even though TaskError already has dedicated variants like WalletInfoUnavailable and MissingBip44Account. That regresses the displayed guidance back to the generic payment-failed message and makes downstream handling/test coverage string-based again.

🧩 Suggested direction
-            let managed_info =
-                wm.get_wallet_info(wallet_id)
-                    .ok_or_else(|| TaskError::WalletPaymentFailed {
-                        detail: "Wallet info unavailable".to_string(),
-                    })?;
+            let managed_info = wm
+                .get_wallet_info(wallet_id)
+                .ok_or(TaskError::WalletInfoUnavailable)?;

-                .ok_or_else(|| TaskError::WalletPaymentFailed {
-                    detail: "BIP44 account missing".to_string(),
-                })?;
+                .ok_or(TaskError::MissingBip44Account {
+                    index: DEFAULT_BIP44_ACCOUNT_INDEX,
+                })?;

Add new typed variants for the wallet-object/change-address cases instead of encoding them in detail.

As per coding guidelines: Centralize user-facing error messages in dedicated TaskError variants rather than writing string literals at callsites. When adding new backend error types, add a dedicated TaskError variant rather than converting to String.

Also applies to: 804-815, 899-913

🤖 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 700 - 734, The code is converting
distinct wallet lookup/derive failures into generic WalletPaymentFailed/detail
strings; instead, map each failure to a dedicated TaskError variant: replace the
get_wallet(...) .ok_or_else(...) that returns WalletPaymentFailed("Wallet object
not found") with a new TaskError::WalletObjectMissing (or reuse an existing
WalletInfoUnavailable if appropriate), keep account-missing uses of
TaskError::MissingBip44Account (do not stringify), and replace the
derive_change_address(...).map_err(...) that wraps errors into
WalletPaymentFailed with a TaskError::ChangeAddressDerivationFailed that
captures the original derive error text; apply the same pattern to the other
similar callsites (the blocks around the other line ranges) so callers/tests can
match on typed TaskError variants instead of free-form strings.
🟡 Minor comments (3)
docs/ai-design/2026-03-26-unified-send/architecture.md-198-205 (1)

198-205: ⚠️ Potential issue | 🟡 Minor

Document the known Base58 classification precedence as an explicit caveat.

Lines 198-205 state that AddressInput "detects all four AddressKind variants" without noting that detect_address_type() assigns Core precedence over Identity when classifying Base58 strings. While this is an accepted limitation (deferred pending identity-only AddressInput mode), it should be explicitly documented here so readers understand the constraint: a Base58 string matching both Core and Identity patterns will be classified as Core, potentially misrouting an identity destination before validate_and_send() executes routing logic.

Add a note clarifying that the priority rule (Shielded > Platform > Core > Identity) applies, with a remark that this affects only edge cases where an Identity ID happens to match Base58Check encoding, and that the workaround is manual route selection via the source/destination matrix.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ai-design/2026-03-26-unified-send/architecture.md` around lines 198 -
205, Add an explicit caveat to the Architecture note stating that
detect_address_type() inside AddressInput assigns Base58 addresses with Core
precedence over Identity (priority rule: Shielded > Platform > Core > Identity),
so a Base58 string that matches both Core and Identity will be classified as
Core and may be routed incorrectly until validate_and_send() runs; mention this
only affects edge cases (Identity IDs that also look like Base58Check), and
document the manual workaround of forcing the correct route via the
source/destination selection matrix or using developer_mode to expose alternate
sources.
docs/ai-design/2026-03-26-unified-send/requirements.md-17-20 (1)

17-20: ⚠️ Potential issue | 🟡 Minor

The scope/count text still mixes the old 12-path model with the new 14-path matrix.

The executive summary/solution direction still talk about 12 combinations, while the matrix and recount later in the file define 14 viable paths and 8 new UI wirings. Keeping both versions in one requirements doc will send implementation and QA in different directions.

Also applies to: 94-105

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ai-design/2026-03-26-unified-send/requirements.md` around lines 17 - 20,
The doc mixes an outdated "12 combinations" description with the updated 14-path
matrix; update the text in the Solution Direction and Executive Summary to
consistently reference the 14-path model and 8 new UI wirings, and ensure any
counts/numbers in nearby sections (notably lines referenced around 94-105) are
corrected to match the matrix; specifically, change the phrase mentioning "ALL
12 Source x Destination combinations" to reflect the 14 combinations, and verify
references to WalletSendScreen, validate_and_send(), AddressInput, and
AmountInput align with the new Identity source addition and progressive
disclosure requirements.
docs/ai-design/2026-03-26-unified-send/requirements.md-238-246 (1)

238-246: ⚠️ Potential issue | 🟡 Minor

The CLI examples still use the stale identity top-up command name.

These snippets call det-cli identity-topup, but the tool table later in the doc defines identity_credits_topup, which the CLI will expose as identity-credits-topup. The examples will not match the shipped command surface.

As per coding guidelines, src/mcp/tools/**/*.rs: MCP tool naming follows pattern {domain}_{object}_{action} and the CLI converts underscores to hyphens.

Also applies to: 364-366

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ai-design/2026-03-26-unified-send/requirements.md` around lines 238 -
246, Update the stale CLI examples to use the correct command name derived from
the tool table and naming conventions: replace uses of "det-cli identity-topup"
with "det-cli identity-credits-topup" (CLI exposes underscores as hyphens) and
keep the expected dispatch of IdentityTask::TopUpIdentity with FundWithWallet
and the return of the new balance; also apply the same replacement for the other
occurrences noted (around the later example at the 364-366 region) so examples
match the shipped command surface and the MCP tool naming pattern
{domain}_{object}_{action}.
🧹 Nitpick comments (1)
docs/ai-design/2026-03-26-unified-send/architecture.md (1)

9-23: Add languages to the fenced blocks.

The unlabeled fences at Lines 9, 27, 45, 62, 78, 95, 110, 125, 162, 327, and 408 will keep triggering MD040. Use text, rust, or another concrete language for each block.

Also applies to: 27-41, 45-58, 62-74, 78-91, 95-106, 110-121, 125-136, 162-169, 327-335, 408-418

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/ai-design/2026-03-26-unified-send/architecture.md` around lines 9 - 23,
The markdown contains multiple unlabeled fenced code blocks (e.g., the block
showing the flow with validate_and_send(), send_core_to_shielded(),
AppAction::BackendTask(BackendTask::ShieldedTask(...)),
AppContext::run_shielded_task(), ShieldedCreditsShielded, display_task_result(),
SendStatus::Complete, and SyncNotes) that trigger MD040; fix each unlabeled
fence by adding an explicit language label (for example `text` or `rust`) to
every fenced block mentioned (lines around the code showing the UI/backend flow
and the other blocks listed in the review), ensuring each triple-backtick fence
becomes ```text or ```rust as appropriate so the linter no longer flags MD040.
🤖 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/wallet_lifecycle.rs`:
- Around line 154-173: The current unlock path calls
initialize_shielded_wallet(seed_hash) and on Err only logs and never schedules
queue_shielded_sync(seed_hash), leaving shielded state uninitialized; change
this so that on failure you both log the error and schedule a retry/backoff to
call initialize_shielded_wallet again (and then queue_shielded_sync on success).
Concretely, update the error branch around initialize_shielded_wallet(...) to:
record/emit the error (with seed_hash), schedule a retry task (exponential or
fixed backoff) that re-invokes initialize_shielded_wallet(seed_hash) and calls
queue_shielded_sync(seed_hash) on success, and ensure the retry mechanism is
resilient to repeated failures (e.g., cap retries or continue background retries
until success). Ensure you reference and reuse the existing methods
initialize_shielded_wallet and queue_shielded_sync and preserve current
success-path logging.
- Around line 139-145: The bootstrap gate in bootstrap_wallet_addresses
currently only checks guard.known_addresses.is_empty(), which skips backfilling
newer derived address classes; update bootstrap_wallet_addresses to detect
missing derived address types (or simply always invoke
Wallet::bootstrap_known_addresses when any derived classes may be absent)
instead of relying solely on is_empty(): inspect guard.known_addresses for the
specific keys/types that bootstrap_known_addresses populates (platform payment,
identity, provider, etc.) or remove the is_empty() short-circuit and call
guard.bootstrap_known_addresses(self) unconditionally (ensuring you hold the
write lock and preserve the tracing::info log).

In `@src/mcp/resolve.rs`:
- Around line 186-193: validate_credits() currently only checks amount > 0 but
tools (mcp::tools::identity::..., mcp::tools::shielded::...) must also ensure
the identity has enough balance to cover amount + estimated fee before calling
backend transfer_to_identity(); update the tool entry points to call
PlatformFeeEstimator to get the estimated_fee_for_operation and compare
available_balance (from the identity/account state) against amount_credits +
estimated_fee, returning a McpToolError::InvalidParam with a clear message if
insufficient, or add a new helper (e.g.,
validate_credits_with_fee(amount_credits, available_balance, estimator)) and use
it from the identity and shielded tool dispatchers so backend transfer functions
no longer receive obviously doomed requests.

In `@src/mcp/tools/identity.rs`:
- Around line 192-255: The platform-funded top-up path
(TopUpIdentityFromPlatformAddresses) and the other wallet tools in this file
skip the required SPV-sync precondition; call resolve::ensure_spv_synced(...)
after obtaining ctx and seed_hash (i.e., after resolve::wallet(&ctx,
&param.wallet_id) / before reading platform balances or using
resolve::wallet_arc) to poll SPV status with the repo-required behavior, and add
the same ensure_spv_synced call in the other affected handlers (the blocks
around lines shown: the TopUpIdentityFromPlatformAddresses handler and the
similar transfer/withdraw/to-address handlers) so each path enforces SPV sync
before building inputs or dispatching BackendTask::IdentityTask variants.
- Around line 26-36: The struct field `network` in IdentityTopupParams is
currently a required String; change it to an Option<String> (e.g., pub network:
Option<String>) so the generated schema makes it optional, and update any
validation logic that compares the request network to the active network to
treat None as "no network provided" (allow) and only fail when Some(value) is
present and does not match the active network; apply the same change to the
other MCP tool param structs in this file that currently declare network as
String so they become Option<String> and their validation logic follows the
"fail only on mismatch" behavior.
- Around line 206-245: The current allocator loop (the platform_balances block,
the inputs: BTreeMap creation, and the selection logic that consumes
param.amount_credits) can underfund identity_credits_topup_from_platform because
wallet.platform_address_info is keyed by Core address (duplicates overwrite in
the BTreeMap) and it also ignores fee headroom and the UI input-count limit;
replace this manual collection+selection with the shared deduplicating
platform-input allocator used elsewhere in the codebase (i.e., call the common
allocator rather than rebuilding platform_balances and inserting into inputs) so
it returns a deduped set of PlatformAddress->amount inputs and enforces fee
headroom and input-count limits before returning or erroring when remaining
funds are insufficient.
- Around line 568-572: After parsing the address with
PlatformAddress::from_bech32m_string(&param.to_address) capture its returned
network (the second tuple element) and verify it against the app network via
crate::model::wallet::networks_address_compatible(&addr_network,
&ctx.network()); if incompatible return an McpToolError::InvalidParam with a
message like "Address is for {addr_network:?} but app is on {ctx.network():?}".
Apply the same pattern to the shielded tool where
PlatformAddress::from_bech32m_string() is used, and keep the existing map_err
for parse failures (do not remove require_network() checks).

In `@src/mcp/tools/shielded.rs`:
- Around line 170-204: Call resolve::ensure_spv_synced(&ctx).await before
reading wallet state or dispatching any destructive shielded tasks: after
obtaining seed_hash via resolve::wallet(&ctx, &param.wallet_id)? and before
acquiring wallet_arc/read or creating
BackendTask::ShieldedTask(ShieldedTask::ShieldCredits { ... }) and calling
dispatch_task(&ctx, task). Add the same ensure_spv_synced(&ctx).await guard in
the other affected functions (around lines noted) so every MCP wallet tool calls
resolve::ensure_spv_synced() prior to operating on wallet state.
- Around line 172-200: After selecting the highest-balance PlatformAddress
(using resolve::wallet_arc and wallet.platform_address_info) ensure that the
chosen from_address can cover param.amount_credits plus the required fee before
constructing BackendTask::ShieldedTask(ShieldedTask::ShieldCredits); compute the
required total (amount + fee) using your existing fee estimation logic (or the
same method the backend uses), compare it to the selected address's balance, and
if insufficient return a McpToolError::InvalidParam with a clear message instead
of proceeding to create the ShieldCredits task.
- Around line 24-32: The param structs currently declare network: String but the
MCP spec requires an optional network; change network: String to network:
Option<String> on ShieldFromCoreParams and the other four param structs in this
file, update the doc comment to indicate it's optional, keep the existing
derives (Debug, Deserialize, schemars::JsonSchema, Default) so the schema marks
it optional, and then update any validation logic that referenced network to
only enforce a match against the active network when network.is_some() (i.e.,
only fail on mismatch when the caller supplies Some(value)).
- Around line 285-290: After parsing shielded and platform addresses with
OrchardAddress::from_bech32m_string and PlatformAddress::from_bech32m_string,
capture the returned network value and compare it against the active app network
(the same network used/checked by resolve::require_network); if they differ,
return an McpToolError::InvalidParam with a clear message about network
mismatch. Update the recipient_bytes and corresponding platform address handling
to preserve the parsed network when mapping, perform the compatibility check
(same logic as networks_address_compatible used in the UI), and only proceed to
to_raw_bytes()/use the address when the parsed network matches the active app
network.

In `@src/ui/wallets/send_screen.rs`:
- Around line 1639-1646: The identity send check currently compares
amount_credits against qualified_identity.identity.balance() without reserving
fees, causing exact/full-balance sends to later fail when transition fees are
applied; update the validation and any "Max" computations (references:
qualified_identity.identity.balance(), amount_credits, format_credits_as_dash)
to first subtract an estimated transaction fee (use the same fee-estimate
routine used elsewhere in this module) and enforce a practical minimum (fee +
dust) before allowing the transfer, and update the error message to reflect the
net-available balance (e.g., "Insufficient identity balance after estimated
fees: need X but have Y"); apply the same adjustment to the other occurrences
called out (the checks around lines 1679-1686, 1721-1728, 2182-2185) so both UI
preflight and backend-bound amount calculations account for estimated fees
consistently.
- Around line 1475-1481: The code in send_screen.rs turns a missing identity
lookup from app_context.get_identity_by_id(&identity_id) into a hard error that
says "Load the identity first from the Identity screen" (assigned to
qualified_identity), which forces the flow back to the Identity screen; change
the handling so that a None result is not mapped to that error—keep the lookup
result as an Option (or return Ok(None) from this block) and let the unified
send flow resolve an unresolved identity here instead of forcing navigation;
update uses of qualified_identity (the consumer logic after this lookup) to
handle Option<Identity> accordingly rather than assuming a found identity.
- Around line 1420-1445: The send_core_to_shielded handler is ignoring the
user-entered shielded destination (self.validated_destination) and only sends
seed_hash/amount to the backend; update send_core_to_shielded (and the analogous
handler around the other block you mentioned, e.g., send_asset_to_shielded) to
validate that self.validated_destination is present and include it in the
BackendTask payload (add the destination/shield_address field to the
ShieldFromAssetLock / ShieldFromAsset variants or call the appropriate backend
constructor that accepts a destination), or if shielding must always be
self-only remove the free-form destination UI. Ensure you reference
self.validated_destination when building the ShieldedTask so the backend
receives the intended target address.
- Around line 1521-1534: The guard is comparing amount_credits against the sum
of all Platform balances (total_balance) but ShieldCredits spends only from the
selected from_address; update the preflight to fetch the balance for the chosen
from_address (the same one produced by the from_address selection logic) and
compare amount_credits against that single-address balance (minus any estimated
fees if a fee variable or estimation function exists) instead of total_balance;
use the existing helper format_credits_as_dash in the error text and reference
the variables from_address, addresses, amount_credits and any fee estimate
(e.g., fee_credits or estimate_shield_fee) when building the error message.
- Around line 2048-2080: The issue is Identity destinations are misclassified as
Core because Core is listed before Identity in allowed_kinds, and
detect_address_type prefers earlier kinds; fix by reordering the AddressKind
vectors so AddressKind::Identity appears before AddressKind::Core wherever
Identity is a valid destination (cases for SourceSelection::CoreWallet,
SourceSelection::PlatformAddresses(_), and SourceSelection::Identity(_)); ensure
AddressKind::Shielded and AddressKind::Platform positions remain logical, and
verify detect_address_type is invoked with this reordered allowed_kinds so
Identity gets precedence over Core for Base58 strings.
- Around line 849-858: The explicit match arms returning hard errors for
(SourceSelection::Identity, AddressKind::Shielded) and
(SourceSelection::Shielded, AddressKind::Identity) are never reachable because
validated_destination is only populated for allowed kinds; instead, populate
validated_destination for these combinations with a validation error so the UI
can enable the send flow and surface the "not supported" guidance. Locate the
validation logic in send_screen.rs that sets validated_destination (and the
match on SourceSelection/AddressKind) and change it to set validated_destination
= Some(...) for the incompatible pairings and attach a user-facing error message
string (rather than returning Err early), so the send button becomes enabled and
the guidance is shown; keep the sources AddressKind and SourceSelection symbols
to find the code to modify.

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 474-501: The add_receiving_address method currently calls
wallet.receive_address inline (bypassing SPV registration and running on the UI
thread) and shows raw backend errors; instead dispatch the existing async
backend task (see backend_task::wallet::generate_receive_address) via the app
action path (use the AppAction that AppState handles to call
app_context.run_backend_task()) passing the selected wallet id/handle and
network so the backend runs next_bip44_receive_address() and registers the
address; then update the success/error banner to show a user-friendly message
(no raw backend/db error text) and surface any detailed error only in logs, not
in MessageBanner. Ensure you reference selected_wallet and add_receiving_address
when wiring the AppAction call.

---

Outside diff comments:
In `@src/backend_task/core/mod.rs`:
- Around line 196-203: The code returns the raw RPC error text into
CoreItem::ChainLocks (active_rpc_error) which is shown to users; instead,
replace the user-facing string Some(format!("RPC error: {e}")) with a concise,
non-technical message such as "Unable to reach the network — retry or check your
connection" and ensure the original error variable e is still recorded via
tracing::warn or attached to a non-user-facing field (e.g., log/tracing or a
technical_details/diagnostics structure) by using Self::chain_lock_rpc_error or
tracing::warn!(error = ?e) so developers retain the full error while users see
only the generic next-step guidance.
- Around line 700-734: The code is converting distinct wallet lookup/derive
failures into generic WalletPaymentFailed/detail strings; instead, map each
failure to a dedicated TaskError variant: replace the get_wallet(...)
.ok_or_else(...) that returns WalletPaymentFailed("Wallet object not found")
with a new TaskError::WalletObjectMissing (or reuse an existing
WalletInfoUnavailable if appropriate), keep account-missing uses of
TaskError::MissingBip44Account (do not stringify), and replace the
derive_change_address(...).map_err(...) that wraps errors into
WalletPaymentFailed with a TaskError::ChangeAddressDerivationFailed that
captures the original derive error text; apply the same pattern to the other
similar callsites (the blocks around the other line ranges) so callers/tests can
match on typed TaskError variants instead of free-form strings.

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 1186-1210: The current loop over all_system_categories collapses
all summaries for a category (e.g., AccountCategory::Bip32) into one section and
only keeps matching.first().and_then(|s| s.index), which loses other Bip32
account indexes and their addresses; change the grouping so sections are keyed
by (category, account_index) when an index exists: iterate summaries (or use
precomputed address_counts keys) and build sections per (s.category, s.index)
instead of per category, compute address_count from address_counts using that
same composite key, sum balances from matching entries for that key, and pass
that index through to render_address_table() so all Bip32 accounts remain
visible (refer to all_system_categories, precompute_address_counts, sections,
summaries, and render_address_table).

---

Minor comments:
In `@docs/ai-design/2026-03-26-unified-send/architecture.md`:
- Around line 198-205: Add an explicit caveat to the Architecture note stating
that detect_address_type() inside AddressInput assigns Base58 addresses with
Core precedence over Identity (priority rule: Shielded > Platform > Core >
Identity), so a Base58 string that matches both Core and Identity will be
classified as Core and may be routed incorrectly until validate_and_send() runs;
mention this only affects edge cases (Identity IDs that also look like
Base58Check), and document the manual workaround of forcing the correct route
via the source/destination selection matrix or using developer_mode to expose
alternate sources.

In `@docs/ai-design/2026-03-26-unified-send/requirements.md`:
- Around line 17-20: The doc mixes an outdated "12 combinations" description
with the updated 14-path matrix; update the text in the Solution Direction and
Executive Summary to consistently reference the 14-path model and 8 new UI
wirings, and ensure any counts/numbers in nearby sections (notably lines
referenced around 94-105) are corrected to match the matrix; specifically,
change the phrase mentioning "ALL 12 Source x Destination combinations" to
reflect the 14 combinations, and verify references to WalletSendScreen,
validate_and_send(), AddressInput, and AmountInput align with the new Identity
source addition and progressive disclosure requirements.
- Around line 238-246: Update the stale CLI examples to use the correct command
name derived from the tool table and naming conventions: replace uses of
"det-cli identity-topup" with "det-cli identity-credits-topup" (CLI exposes
underscores as hyphens) and keep the expected dispatch of
IdentityTask::TopUpIdentity with FundWithWallet and the return of the new
balance; also apply the same replacement for the other occurrences noted (around
the later example at the 364-366 region) so examples match the shipped command
surface and the MCP tool naming pattern {domain}_{object}_{action}.

---

Nitpick comments:
In `@docs/ai-design/2026-03-26-unified-send/architecture.md`:
- Around line 9-23: The markdown contains multiple unlabeled fenced code blocks
(e.g., the block showing the flow with validate_and_send(),
send_core_to_shielded(), AppAction::BackendTask(BackendTask::ShieldedTask(...)),
AppContext::run_shielded_task(), ShieldedCreditsShielded, display_task_result(),
SendStatus::Complete, and SyncNotes) that trigger MD040; fix each unlabeled
fence by adding an explicit language label (for example `text` or `rust`) to
every fenced block mentioned (lines around the code showing the UI/backend flow
and the other blocks listed in the review), ensuring each triple-backtick fence
becomes ```text or ```rust as appropriate so the linter no longer flags MD040.
🪄 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: c6052cc7-9cb4-4263-9260-fc006f1a1208

📥 Commits

Reviewing files that changed from the base of the PR and between 4d6fce8 and b5bf7c0.

📒 Files selected for processing (13)
  • docs/MCP.md
  • docs/ai-design/2026-03-26-unified-send/architecture.md
  • docs/ai-design/2026-03-26-unified-send/requirements.md
  • docs/user-stories.md
  • src/backend_task/core/mod.rs
  • src/context/wallet_lifecycle.rs
  • src/mcp/resolve.rs
  • src/mcp/server.rs
  • src/mcp/tools/identity.rs
  • src/mcp/tools/mod.rs
  • src/mcp/tools/shielded.rs
  • src/ui/wallets/send_screen.rs
  • src/ui/wallets/wallets_screen/mod.rs

Comment thread src/context/wallet_lifecycle.rs
Comment thread src/context/wallet_lifecycle.rs
Comment thread src/mcp/resolve.rs
Comment thread src/mcp/tools/identity.rs
Comment thread src/mcp/tools/identity.rs
Comment thread src/ui/wallets/send_screen.rs
Comment thread src/ui/wallets/send_screen.rs Outdated
Comment thread src/ui/wallets/send_screen.rs
Comment thread src/ui/wallets/send_screen.rs
Comment thread src/ui/wallets/wallets_screen/mod.rs
Send screen (C01/C02/C03/C05):
- Validate shielded destination before dispatching shield routes
- Check highest-address platform balance for Platform->Shielded
- Subtract estimated fee from identity source available balance
- Improve identity-not-found error to be actionable without navigation

MCP tools (C09/C10/C15):
- Add INTENTIONAL comments for tools that skip SPV sync (Platform-only)
- Add balance pre-check in shielded_shield_from_platform

Docs (C09):
- Clarify mandatory network for destructive tools in MCP.md
- Add network/SPV guidance to EXPOSING_BACKEND_TASKS.md

Verified (no changes needed):
- C12: identity_credits_topup already validates via qualified_identity()
- C16: all 10 tools registered in tool_router()
- C20: qualified_identity() resolve helper is correct

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/mcp/tools/identity.rs`:
- Around line 463-471: The parsed Core address (core_address) is currently
assumed checked via assume_checked(), which bypasses validation and allows
mismatched networks; after parsing the Address in identity.rs, call the
address's require_network(...) using the current ctx.network() (instead of
relying on resolve::validate_address or resolve::require_network on the param)
and map any error to McpToolError::InvalidParam with a message indicating the
address is for the wrong network and showing ctx.network(); remove the use of
assume_checked() so the address is validated against the app network.
🪄 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: 9f31af0d-c550-48d8-ba13-42900ec2aa00

📥 Commits

Reviewing files that changed from the base of the PR and between b5bf7c0 and df6aa5f.

📒 Files selected for processing (5)
  • docs/EXPOSING_BACKEND_TASKS.md
  • docs/MCP.md
  • src/mcp/tools/identity.rs
  • src/mcp/tools/shielded.rs
  • src/ui/wallets/send_screen.rs
✅ Files skipped from review due to trivial changes (1)
  • src/ui/wallets/send_screen.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • docs/MCP.md
  • src/mcp/tools/shielded.rs

Comment thread src/mcp/tools/identity.rs
Comment on lines +463 to +471
let core_address: dash_sdk::dashcore_rpc::dashcore::Address<
dash_sdk::dashcore_rpc::dashcore::address::NetworkUnchecked,
> = param
.to_address
.parse()
.map_err(|e| McpToolError::InvalidParam {
message: format!("Invalid Core address: {e}"),
})?;
let core_address = core_address.assume_checked();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check what resolve::validate_address does - does it validate network?
rg -A 20 "pub.*fn validate_address" src/mcp/resolve.rs

Repository: dashpay/dash-evo-tool

Length of output: 839


🏁 Script executed:

# First, verify the exact code at lines 463-471 in identity.rs
sed -n '463,471p' src/mcp/tools/identity.rs

Repository: dashpay/dash-evo-tool

Length of output: 461


🏁 Script executed:

# Search for require_network usage in the codebase
rg "require_network" --type rs -B 2 -A 2

Repository: dashpay/dash-evo-tool

Length of output: 91


🏁 Script executed:

# Check the dialogs.rs file for the suggested pattern
rg -A 10 "require_network" src/ui/wallets/wallets_screen/dialogs.rs

Repository: dashpay/dash-evo-tool

Length of output: 405


🏁 Script executed:

# Verify ctx.network() exists and what it returns
rg "fn network\(" src/ui/app_context.rs -A 5

Repository: dashpay/dash-evo-tool

Length of output: 126


🏁 Script executed:

# Find where AppContext or DashMcpService is defined
fd -t f "*.rs" -path "*/src/*" | head -20 | xargs rg "struct.*Context" | head -20

Repository: dashpay/dash-evo-tool

Length of output: 364


🏁 Script executed:

# Check how context is accessed in the identity.rs tool function around line 463
sed -n '380,475p' src/mcp/tools/identity.rs | head -100

Repository: dashpay/dash-evo-tool

Length of output: 3292


🏁 Script executed:

# Find require_network signature to understand the return type
rg "require_network" --type-list | head -5

Repository: dashpay/dash-evo-tool

Length of output: 454


🏁 Script executed:

# Search for require_network in rust files more carefully
find src -name "*.rs" -type f -exec grep -l "require_network" {} \;

Repository: dashpay/dash-evo-tool

Length of output: 415


🏁 Script executed:

# Check AppContext structure
sed -n '1,100p' src/context/mod.rs | head -50

Repository: dashpay/dash-evo-tool

Length of output: 2109


🏁 Script executed:

# Search for resolve::require_network to understand what it does
rg "fn require_network" src/mcp/resolve.rs -A 10

Repository: dashpay/dash-evo-tool

Length of output: 547


🏁 Script executed:

# Check what network() returns in AppContext
rg "fn network" src/context/mod.rs -A 3

Repository: dashpay/dash-evo-tool

Length of output: 131


🏁 Script executed:

# Look at the full require_network usage in wallets.rs to understand the pattern
rg -B 5 -A 5 "require_network.*app_context.network" src/ui/wallets/send_screen.rs

Repository: dashpay/dash-evo-tool

Length of output: 47


🏁 Script executed:

# Verify Address::require_network() signature and return type
rg "fn require_network" src/ -B 2 -A 8 | grep -A 8 "Address"

Repository: dashpay/dash-evo-tool

Length of output: 47


🏁 Script executed:

# Check if there's any other validation between line 456 and 471
sed -n '456,471p' src/mcp/tools/identity.rs

Repository: dashpay/dash-evo-tool

Length of output: 789


🏁 Script executed:

# Look at the complete require_network function to understand what it validates
rg -A 15 "pub\(crate\) fn require_network" src/mcp/resolve.rs

Repository: dashpay/dash-evo-tool

Length of output: 698


Add network validation to the parsed Core address.

The code calls resolve::require_network() to validate the param.network string, but this does not validate the address itself. A user could pass param.network="mainnet" while providing a testnet address (y...), and it would be accepted:

  • resolve::validate_address() only checks the first byte (X, 7, y, 8, or 9) — it does not verify network compatibility
  • assume_checked() bypasses all validation

Add network validation after parsing:

Suggested fix
let core_address: dash_sdk::dashcore_rpc::dashcore::Address<_> = param
    .to_address
    .parse()
    .map_err(|e| McpToolError::InvalidParam {
        message: format!("Invalid Core address: {e}"),
    })?;

let core_address = core_address
    .require_network(ctx.network().into())
    .map_err(|_| McpToolError::InvalidParam {
        message: format!(
            "Address is for wrong network; app is on {:?}",
            ctx.network()
        ),
    })?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/mcp/tools/identity.rs` around lines 463 - 471, The parsed Core address
(core_address) is currently assumed checked via assume_checked(), which bypasses
validation and allows mismatched networks; after parsing the Address in
identity.rs, call the address's require_network(...) using the current
ctx.network() (instead of relying on resolve::validate_address or
resolve::require_network on the param) and map any error to
McpToolError::InvalidParam with a message indicating the address is for the
wrong network and showing ctx.network(); remove the use of assume_checked() so
the address is validated against the app network.

lklimek and others added 5 commits March 26, 2026 17:10
…essInput

Platform payment addresses (path_reference=PlatformPayment) were
included in the Core addresses loop because they exist in
known_addresses. This caused them to appear twice — once incorrectly
tagged as "(Core)" and once correctly as "(Platform)".

Now skips platform payment addresses in the Core loop, since they're
handled separately in the Platform addresses loop below.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace blacklist (exclude system + platform) with whitelist:
- Core loop: only includes BIP44 paths (is_bip44)
- Platform loop: only includes PlatformPayment paths

Unknown or unrecognized derivation paths are excluded — safer to
hide an address than to show it with the wrong type label.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The wallet's default shielded address was accepted when pasted
(validation worked) but never appeared in the autocomplete dropdown.
Now populates AddressInput with the shielded address and balance
from shielded_states when available.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Error is already displayed by the global MessageBanner. The separate
Dismiss button was redundant and visually orphaned. Now auto-resets
send status to NotStarted so the form is immediately usable for retry.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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/components/address_input.rs`:
- Around line 370-377: The whitelist loop over guard.known_addresses currently
only checks derivation_path.is_bip44(self.network) and must also exclude
system/internal addresses; update the condition in the loop that iterates
guard.known_addresses (the code referencing is_bip44(self.network)) to intersect
with the default-visible account categories (the same visibility logic used by
path_reference/account_summary) or explicitly restore the prior system-address
exclusion so identities/providers/system paths under m/44' are not treated as
normal wallet destinations; after making the change add a regression test that
creates a system path under m/44' and verifies it is not returned by the
address-input autocomplete.
🪄 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: 763d52f3-b39d-43fb-b5a9-6b653f23f202

📥 Commits

Reviewing files that changed from the base of the PR and between abd5f9e and 9aee130.

📒 Files selected for processing (2)
  • src/ui/components/address_input.rs
  • src/ui/wallets/send_screen.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/ui/wallets/send_screen.rs

Comment on lines +370 to 377
// Whitelist approach: only include addresses whose derivation path
// matches the expected type. Unknown or unrecognized paths are excluded
// — safer to hide an address than to show it with the wrong type.

// Core addresses from known_addresses (all derived addresses).
// Balance is looked up from address_balances; addresses without UTXOs
// get balance 0. Use `with_balance_range(1..)` to show only funded
// addresses — do NOT filter at the data source.
// Change addresses (BIP44 m/44'/5'/0'/1/x) are tagged and can be
// excluded via `with_exclude_change(true)`.
// System addresses are always excluded.
// Core addresses: only BIP44 paths (m/44'/coin'/account'/change/index).
for (address, derivation_path) in &guard.known_addresses {
if system_addresses.contains(address) {
if !derivation_path.is_bip44(self.network) {
continue;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Keep the system-address guard in the Core whitelist.

Line 376 is only a structural prefix check. is_bip44(self.network) accepts any m/44'/coin'/... path, and src/ui/wallets/account_summary.rs treats that shape as authoritative before path_reference. With the previous exclusion removed, internal Identity/Provider/System paths that happen to live under BIP44 can leak back into send autocomplete as normal wallet destinations. Please intersect this with the default-visible account categories (or restore the old system-address exclusion) and add a regression test for a system path under m/44'/....

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/components/address_input.rs` around lines 370 - 377, The whitelist
loop over guard.known_addresses currently only checks
derivation_path.is_bip44(self.network) and must also exclude system/internal
addresses; update the condition in the loop that iterates guard.known_addresses
(the code referencing is_bip44(self.network)) to intersect with the
default-visible account categories (the same visibility logic used by
path_reference/account_summary) or explicitly restore the prior system-address
exclusion so identities/providers/system paths under m/44' are not treated as
normal wallet destinations; after making the change add a regression test that
creates a system path under m/44' and verifies it is not returned by the
address-input autocomplete.

When Platform rejects a shield transition with AddressInvalidNonceError,
extract the expected nonce from the error, update the cached nonce, and
retry automatically. The user no longer needs to manually retry.

The proof is rebuilt on retry (the current API doesn't support nonce-only
updates), but this avoids the UX penalty of surfacing a technical error.
Upstream SDK issue dashpay/platform#3407 tracks proper nonce handling.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.


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

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
src/ui/wallets/send_screen.rs (3)

1652-1659: ⚠️ Potential issue | 🟠 Major

Use the fee-adjusted identity balance in these final guards too.

render_amount_input() already reserves an estimated fee for Identity sources, but these handlers still compare against identity.balance(). Manual full-balance entries can still be dispatched and then fail once the fee is charged. Based on learnings, "ensure the transfer amount considers estimated transaction fees" and "validate in UI and backend calculations."

Also applies to: 1692-1699, 1734-1740

🤖 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 1652 - 1659, The final balance
checks use qualified_identity.identity.balance() directly and must instead
subtract the estimated reserved fee used by render_amount_input() so manual
"send full balance" entries don't later fail; compute a fee-adjusted balance
(e.g., fee_adjusted_identity_balance =
qualified_identity.identity.balance().saturating_sub(estimated_fee_for_identity))
and compare amount_credits against that value wherever you currently compare to
qualified_identity.identity.balance() (the blocks around the amount guard shown
and the other handlers referenced at the same locations), ensuring the same
estimated fee source used by render_amount_input() is used for consistency.

1416-1450: ⚠️ Potential issue | 🔴 Critical

These shielding routes still ignore the shielded address the user entered.

Both handlers only check that self.validated_destination is shielded. The dispatched tasks do not carry that address, and src/context/shielded.rs derives the recipient from the current wallet instead. If shielding is intentionally self-only, this screen should not present it as a free-form destination send.

Also applies to: 1510-1558


2062-2090: ⚠️ Potential issue | 🟠 Major

The destination-kind matrix still mishandles Identity routes.

Core is still before Identity, so some Base58 identity IDs will be parsed as Core addresses. And because Identity↔Shielded is filtered out here, the explicit "not yet supported" guidance in validate_and_send() remains unreachable from the UI. Based on learnings, "detect_address_type gives Core precedence over Identity when classifying Base58 strings."

🤖 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 2062 - 2090, The destination-kind
matrix currently lists AddressKind::Core before AddressKind::Identity causing
Base58 identity IDs to be misclassified and preventing the UI from reaching the
"not yet supported" branch in validate_and_send(); update the SourceSelection
match arms (CoreWallet, PlatformAddresses, Identity, Shielded) so that
AddressKind::Identity appears before AddressKind::Core in the vectors returned,
and ensure the Shielded vs Identity route is not removed/filtered out here so
validate_and_send() can receive Identity↔Shielded choices; adjust the order in
the CoreWallet and PlatformAddresses arms and the Shielded arm accordingly to
give Identity precedence over Core and keep the Shielded↔Identity path
reachable.
🧹 Nitpick comments (1)
src/context/shielded.rs (1)

405-438: Consider binding the matched error with the @ pattern for clarity.

The pattern Err(TaskError::ShieldedNonceMismatch { ref source_error }) binds only source_error, not the full error. If nonce extraction fails and you need to return the original error, use the @ pattern to capture the entire variant. This makes the intent explicit and avoids reconstructing or unwrapping.

🛠️ Suggested change
-            Err(TaskError::ShieldedNonceMismatch { ref source_error }) => {
+            Err(original_error @ TaskError::ShieldedNonceMismatch { ref source_error }) => {
                 // Extract the expected nonce from the error and retry once.
                 // The proof is already built — only the nonce in the state
                 // transition needs updating. Unfortunately the current API
                 // rebuilds the entire transition, but this avoids the user
                 // having to manually retry.
@@
                 } else {
                     // Could not extract nonce — surface the original error
-                    Err(result.unwrap_err())
+                    Err(original_error)
                 }
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/context/shielded.rs` around lines 405 - 438, Change the match arm binding
to capture the whole TaskError variant with the @ pattern (e.g. Err(e @
TaskError::ShieldedNonceMismatch { ref source_error })) so you can return the
original error directly instead of calling result.unwrap_err(); keep using
Self::extract_expected_nonce(source_error) to get the nonce, call
self.set_platform_address_nonce(&seed_hash, &from_address,
nonce.saturating_sub(1)), invoke
crate::backend_task::shielded::bundle::shield_credits(..., Some(nonce),
None).await?, then self.bump_platform_address_nonce(&seed_hash, &from_address)
and on the else branch return Err(e) to surface the original error.
🤖 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 1479-1485: The map_err closures wrapping calls to
get_identity_by_id (which produce the qualified_identity) must not interpolate
the raw error into the user-facing string; replace format!("Could not look up
identity: {e}") with a generic message like "Could not look up identity. Please
try again." and ensure the original error `e` is passed to logs or attached via
BannerHandle::with_details() (or similar error-details API) for debugging; apply
the same change to the second occurrence handling the identity lookup at the
other site (the map_err closures around get_identity_by_id and the code that
surfaces banners).

---

Duplicate comments:
In `@src/ui/wallets/send_screen.rs`:
- Around line 1652-1659: The final balance checks use
qualified_identity.identity.balance() directly and must instead subtract the
estimated reserved fee used by render_amount_input() so manual "send full
balance" entries don't later fail; compute a fee-adjusted balance (e.g.,
fee_adjusted_identity_balance =
qualified_identity.identity.balance().saturating_sub(estimated_fee_for_identity))
and compare amount_credits against that value wherever you currently compare to
qualified_identity.identity.balance() (the blocks around the amount guard shown
and the other handlers referenced at the same locations), ensuring the same
estimated fee source used by render_amount_input() is used for consistency.
- Around line 2062-2090: The destination-kind matrix currently lists
AddressKind::Core before AddressKind::Identity causing Base58 identity IDs to be
misclassified and preventing the UI from reaching the "not yet supported" branch
in validate_and_send(); update the SourceSelection match arms (CoreWallet,
PlatformAddresses, Identity, Shielded) so that AddressKind::Identity appears
before AddressKind::Core in the vectors returned, and ensure the Shielded vs
Identity route is not removed/filtered out here so validate_and_send() can
receive Identity↔Shielded choices; adjust the order in the CoreWallet and
PlatformAddresses arms and the Shielded arm accordingly to give Identity
precedence over Core and keep the Shielded↔Identity path reachable.

---

Nitpick comments:
In `@src/context/shielded.rs`:
- Around line 405-438: Change the match arm binding to capture the whole
TaskError variant with the @ pattern (e.g. Err(e @
TaskError::ShieldedNonceMismatch { ref source_error })) so you can return the
original error directly instead of calling result.unwrap_err(); keep using
Self::extract_expected_nonce(source_error) to get the nonce, call
self.set_platform_address_nonce(&seed_hash, &from_address,
nonce.saturating_sub(1)), invoke
crate::backend_task::shielded::bundle::shield_credits(..., Some(nonce),
None).await?, then self.bump_platform_address_nonce(&seed_hash, &from_address)
and on the else branch return Err(e) to surface the original error.
🪄 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: 985f9250-de0b-41aa-83f1-9ddbd619a40c

📥 Commits

Reviewing files that changed from the base of the PR and between 9aee130 and f48ba60.

📒 Files selected for processing (3)
  • src/context/shielded.rs
  • src/ui/components/address_input.rs
  • src/ui/wallets/send_screen.rs

Comment thread src/ui/wallets/send_screen.rs
Store the network's protocol version from epoch info in AppContext
(AtomicU32, updated alongside fee_multiplier). Gate the Shielded
wallet tab on supports_shielded() which requires protocol version
>= 12 (Platform v3.1+).

When the version hasn't been fetched yet (0) or the network is on
an older protocol, the Shielded tab is hidden.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@lklimek lklimek merged commit 97230f2 into v1.0-dev Mar 26, 2026
5 checks passed
@lklimek lklimek deleted the feat/unified-send branch March 26, 2026 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants