feat(mcp): unified send routing with auto-detect destination#808
feat(mcp): unified send routing with auto-detect destination#808
Conversation
Extract routing logic from WalletSendScreen into a pure synchronous resolve_send() function in src/model/send_routing.rs. Covers all 14 supported source/destination combinations plus 2 unsupported routes with user-friendly error messages. Includes platform address allocation helpers and 36 unit tests covering happy paths, error cases, balance checks, self-send prevention, and overflow safety. Also adds test-only constructors for Wallet and QualifiedIdentity to support unit testing across modules. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
New MCP tool that uses the send_routing module to route funds from any source (core, platform, shielded, identity) to any destination address type. Accepts amount in duffs and converts to credits internally for non-Core sources. Routes that require FeeContext (platform-to-platform, platform-to-identity, platform-to-shielded, platform-to-core) pass None and rely on the routing module's fallback behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ting Replace the 14 individual send_*_to_* methods in WalletSendScreen with a single call to resolve_send() from the send_routing module. The new validate_and_send() converts UI-layer SourceSelection to SendSource, builds FeeContext for platform routes, resolves destination identities, and delegates routing entirely to the pure resolve_send() function. Changes: - send_screen.rs: Remove 14 send handler methods (~900 lines), remove duplicated allocation/fee functions (now in send_routing.rs), add build_send_source(), resolve_destination_identity(), build_fee_context() - send_routing.rs: Add seed_hash to SendSource::CoreWallet (fixes the zeroed placeholder), make allocation types and fee functions pub for UI rendering code Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…llet_funds_send Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion boundary tests - Replace developer-facing "programming error" message with user-friendly text - Add MAX_PLATFORM_INPUTS boundary test (TC-SR-035) - Add multi-input within limit test (TC-SR-037) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
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 0 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR centralizes “send routing” (mapping source+destination combinations to the correct BackendTask) into a new model-layer module so both the GUI send screen and MCP tooling can reuse the same routing logic.
Changes:
- Added
src/model/send_routing.rswithresolve_send()plus platform allocation/fee helpers and unit tests. - Refactored
WalletSendScreento replace many per-route send handlers with a singlevalidate_and_send()that delegates toresolve_send(). - Added a new MCP tool
wallet_funds_send, registered it on the server, and documented it.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/model/send_routing.rs |
Introduces shared routing API (SendSource/SendRequest/resolve_send) and shared platform allocation + fee estimation helpers with tests. |
src/ui/wallets/send_screen.rs |
Migrates UI send dispatch to the shared routing layer and reuses shared allocation/fee helpers. |
src/mcp/tools/wallet.rs |
Adds wallet_funds_send tool that builds a SendRequest and dispatches tasks based on resolve_send(). |
src/mcp/server.rs |
Registers wallet_funds_send on the MCP service. |
docs/MCP.md |
Adds the new MCP tool to the tool list. |
src/model/wallet/mod.rs |
Adds Wallet::new_test_wallet() helper (tests only) for routing unit tests. |
src/model/qualified_identity/mod.rs |
Adds QualifiedIdentity::new_test_identity() helper (tests only) for routing unit tests. |
src/model/mod.rs |
Exposes the new send_routing module. |
docs/ai-design/2026-03-31-unified-send-routing/requirements.md |
Adds a design/requirements doc for the unified routing initiative. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // For Core wallet sources, amount is in duffs; otherwise it's already in credits | ||
| let amount = match &source { | ||
| SendSource::CoreWallet { .. } => self | ||
| .amount | ||
| .as_ref() | ||
| .ok_or_else(|| "Amount is required".to_string())? | ||
| .dash_to_duffs()?, | ||
| _ => amount_obj.value(), | ||
| }; | ||
|
|
||
| self.mark_sending(); | ||
| // Resolve identity for identity-destination routes | ||
| let resolved_identity = self.resolve_destination_identity(&destination)?; | ||
|
|
||
| Ok(AppAction::BackendTask(BackendTask::CoreTask( | ||
| CoreTask::SendWalletPayment { | ||
| wallet, | ||
| request: WalletPaymentRequest { | ||
| recipients: vec![recipient], | ||
| subtract_fee_from_amount: self.subtract_fee, | ||
| memo: None, | ||
| override_fee: None, | ||
| }, | ||
| }, | ||
| ))) | ||
| } | ||
|
|
||
| fn send_core_to_platform(&mut self, seed_hash: WalletSeedHash) -> Result<AppAction, String> { | ||
| let amount_duffs = self | ||
| .amount | ||
| .as_ref() | ||
| .ok_or_else(|| "Amount is required".to_string())? | ||
| .dash_to_duffs()?; | ||
| if amount_duffs == 0 { | ||
| return Err("Amount must be greater than 0".to_string()); | ||
| } | ||
| // Build fee context for Platform source routes | ||
| let fee_context = self.build_fee_context(&source, &destination); | ||
|
|
||
| // Extract validated platform address | ||
| let destination = self | ||
| .validated_destination | ||
| .as_ref() | ||
| .and_then(|v| v.as_platform().copied()) | ||
| .ok_or_else(|| "Invalid platform address".to_string())?; | ||
| let request = SendRequest { | ||
| source, | ||
| destination, | ||
| amount, | ||
| resolved_identity, | ||
| fee_context, | ||
| }; | ||
|
|
||
| // Check balance; fees will be subtracted from amount | ||
| let required = amount_duffs; | ||
| let balance = self.get_core_balance(); | ||
| if required > balance { | ||
| return Err(format!( | ||
| "Insufficient balance. Need {} (including fee) but have {}", | ||
| Self::format_dash(required), | ||
| Self::format_dash(balance) | ||
| )); | ||
| match send_routing::resolve_send(request) { | ||
| Ok(SendResult::Single(task)) => { | ||
| self.mark_sending(); | ||
| Ok(AppAction::BackendTask(task)) | ||
| } | ||
| Ok(SendResult::Sequential(tasks)) => { | ||
| self.mark_sending(); | ||
| Ok(AppAction::BackendTasks( | ||
| tasks, | ||
| crate::app::BackendTasksExecutionMode::Sequential, | ||
| )) | ||
| } | ||
| Err(e) => Err(e.to_string()), | ||
| } |
There was a problem hiding this comment.
validate_and_send() no longer applies the UI's subtract_fee option for Core→Core sends. The checkbox (and the auto-enable on Max) still updates self.subtract_fee, but resolve_send() always builds WalletPaymentRequest with subtract_fee_from_amount: false, so Max-on-core is likely to fail and the recipient amount semantics change. Consider threading subtract_fee_from_amount through SendRequest (or similar) and using it when constructing WalletPaymentRequest for Core wallet routes.
| .validated_destination | ||
| .clone() | ||
| .ok_or_else(|| { | ||
| "Invalid destination address. Use a Dash address (X.../y...) or Platform address (dash1.../tdash1...)" |
There was a problem hiding this comment.
The invalid-destination error message only mentions Core (X/y...) and Platform (dash1/tdash1) addresses, but the screen now supports Shielded (dash1z/tdash1z) and Identity destinations too. This message will be misleading for users entering those address types (or making minor typos). Update the message to include all supported destination kinds.
| "Invalid destination address. Use a Dash address (X.../y...) or Platform address (dash1.../tdash1...)" | |
| "Invalid destination. Use a Dash Core address (X.../y...), Platform address (dash1.../tdash1...), Shielded address (dash1z.../tdash1z...), or Identity destination." |
| ValidatedAddress::Identity { .. }, | ||
| ) => { | ||
| check_balance(balance_duffs, amount, true)?; | ||
|
|
||
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; |
There was a problem hiding this comment.
Core→Identity routing uses resolved_identity without verifying it matches the destination identity ID (the destination pattern ignores the id). A mismatched resolved_identity could top up the wrong identity if a caller passes the wrong object. Capture the destination id in the match and validate qi.identity.id() == id (or similar), returning a routing error on mismatch.
| ValidatedAddress::Identity { .. }, | |
| ) => { | |
| check_balance(balance_duffs, amount, true)?; | |
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; | |
| ValidatedAddress::Identity { id, .. }, | |
| ) => { | |
| check_balance(balance_duffs, amount, true)?; | |
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; | |
| // Ensure the resolved identity matches the destination identity ID | |
| if qi.identity.id() != id { | |
| return Err(SendRoutingError::IdentityNotResolved); | |
| } |
| ValidatedAddress::Identity { .. }, | ||
| ) => { | ||
| let total_balance: u64 = addresses.iter().map(|(_, _, b)| *b).sum(); | ||
| check_balance(total_balance, amount, false)?; | ||
|
|
||
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; | ||
|
|
There was a problem hiding this comment.
Platform→Identity routing also uses resolved_identity directly (destination match ignores the id), so a caller could accidentally fund the wrong identity if resolved_identity doesn’t correspond to the destination. Capture the destination id in the match arm and validate it matches the QualifiedIdentity before constructing TopUpIdentityFromPlatformAddresses.
| ValidatedAddress::Identity { .. }, | |
| ) => { | |
| let total_balance: u64 = addresses.iter().map(|(_, _, b)| *b).sum(); | |
| check_balance(total_balance, amount, false)?; | |
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; | |
| ValidatedAddress::Identity { id, .. }, | |
| ) => { | |
| let total_balance: u64 = addresses.iter().map(|(_, _, b)| *b).sum(); | |
| check_balance(total_balance, amount, false)?; | |
| let qi = resolved_identity.ok_or(SendRoutingError::IdentityNotResolved)?; | |
| if qi.id() != &id { | |
| return Err(SendRoutingError::IdentityNotResolved); | |
| } |
| #[derive(Serialize, schemars::JsonSchema)] | ||
| pub struct WalletFundsSendOutput { | ||
| /// Which route was taken (e.g. "core->core", "platform->shielded") | ||
| route: String, | ||
| /// Amount sent (in the source's native unit) | ||
| amount: u64, | ||
| /// Transaction ID if available (Core-to-Core sends) | ||
| txid: Option<String>, | ||
| /// Number of sequential tasks dispatched (>1 for multi-address Platform-to-Shielded) | ||
| tasks_dispatched: u32, |
There was a problem hiding this comment.
WalletFundsSendOutput.amount is documented as “in the source's native unit”, but the tool always returns param.amount_duffs even when the source is non-Core (where the routed amount is credits). Either rename the output field to amount_duffs (and document it as duffs), or return the actual amount passed into SendRequest (credits for non-Core) and/or include both units to avoid ambiguous logging.
| | `platform_addresses_list` | `wallet_id`, `network`? | `det-cli platform-addresses-list` | Fetch platform address balances (credits and nonces) | | ||
| | `core_funds_send` | `wallet_id`, `address`, `amount_duffs`, `network` | `det-cli core-funds-send` | Send DASH from a wallet to an address (amount in duffs) | | ||
| | `wallet_funds_send` | `wallet_id`, `address`, `amount_duffs`, `network`, `source`?, `identity_id`? | `det-cli wallet-funds-send` | Unified send: any source (core/platform/shielded/identity) to any destination address type | | ||
| | `platform_withdrawals_get` | `status`?, `network`? | `det-cli platform-withdrawals-get` | Query Platform withdrawal documents (`"queued"` or `"completed"`) | |
There was a problem hiding this comment.
The wallet_funds_send table entry claims “any source … to any destination address type”, but current implementation has important limitations (e.g., Platform-source routes that need fee context error out, and Core/Platform→Shielded operations ignore the provided shielded destination because they shield into the wallet’s own pool). Update this row (or add a nearby note) to reflect supported routes/limitations so MCP users don’t assume funds will go to an arbitrary shielded address.
| ### US-4: Amount unit clarity | ||
| **As** any caller, **I want** the routing function to accept a single canonical unit and handle conversion internally, **so that** I do not need to know whether the destination expects duffs or credits. | ||
|
|
||
| **Acceptance criteria**: | ||
| - The routing function accepts amount in **duffs** (the L1 base unit, universally understood). | ||
| - When the resolved route requires credits (platform transfers, identity top-ups), the function converts using `CREDITS_PER_DUFF` (currently 1000). | ||
| - The MCP tool documents that `amount_duffs` is always in duffs regardless of destination type. | ||
|
|
||
| ## 5. Function Signature (Conceptual) | ||
|
|
||
| ``` | ||
| resolve_send( | ||
| source: SendSource, | ||
| destination: ValidatedAddress, | ||
| amount_duffs: u64, | ||
| context: &SendContext, // wallet refs, network, fee config | ||
| ) -> Result<BackendTask, SendRoutingError> |
There was a problem hiding this comment.
US-4 / function signature in this requirements doc says resolve_send() accepts amount_duffs and converts internally, but the implemented SendRequest.amount is documented/used as “native unit” (duffs for Core, credits otherwise) and callers (UI/MCP) perform conversion before calling. Either update this doc to match the implemented API, or adjust resolve_send() to take a canonical duffs amount and handle conversion internally as specified here.
| Ok(SendResult::Single(BackendTask::CoreTask( | ||
| CoreTask::SendWalletPayment { | ||
| wallet, | ||
| request: WalletPaymentRequest { | ||
| recipients: vec![recipient], | ||
| subtract_fee_from_amount: false, | ||
| memo: None, | ||
| override_fee: None, | ||
| }, |
There was a problem hiding this comment.
Core→Core routing always builds WalletPaymentRequest with subtract_fee_from_amount: false. Since the UI and other callers may need “subtract fee” semantics (especially for “send max”), this should be configurable via SendRequest (or a dedicated field for Core payment options) rather than hard-coded.
…s_send Platform-to-Platform, Platform-to-Identity, Platform-to-Core, and Platform-to-Shielded routes require a FeeContext for proper fee estimation and platform address allocation. Without it, resolve_send() returns MissingFeeContext errors for all Platform source routes. Adds build_fee_context() mirroring the UI's SendScreen::build_fee_context() logic to construct the correct FeeContext variant based on source/destination combination. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t_funds_send Remove 12 single-route send tools now superseded by wallet_funds_send: - core_funds_send (Core->Core) - identity_credits_topup (Core->Identity) - identity_credits_topup_from_platform (Platform->Identity) - identity_credits_transfer (Identity->Identity) - identity_credits_withdraw (Identity->Core) - identity_credits_to_address (Identity->Platform) - shielded_shield_from_core (Core->Shielded) - shielded_shield_from_platform (Platform->Shielded) - shielded_transfer (Shielded->Shielded) - shielded_unshield (Shielded->Platform) - shielded_withdraw (Shielded->Core) Removes identity.rs and shielded.rs tool modules entirely (no remaining tools), cleans up server.rs registrations, and updates docs/MCP.md. The validate_address helper is now #[cfg(test)] since only tests use it; validate_credits is removed (no tests referenced it). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lklimek
changed the title
|
✅ Review complete (commit 673fb7d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #808 replaces 14 individual MCP send tools with a unified wallet_funds_send tool backed by a pure resolve_send() routing function. The routing logic is sound with 38 unit tests and clean typed errors. Two issues are blocking: the PR violates its own backward-compatibility requirement (US-3) by removing core_funds_send, and CLI docs reference deleted tools. Three design gaps are significant: Core/Platform→Shielded routes silently discard the user-provided destination address, sequential task dispatch can partially complete without reporting which steps succeeded, and all routing errors are flattened to a single MCP error code.
Reviewed commit: 69f9606
🔴 3 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
8 additional findings
🔴 blocking: PR violates its own backward-compatibility requirement (US-3)
src/mcp/tools/wallet.rs (line 1)
🔴 blocking: CLI.md references removed tools
docs/CLI.md (line 1)
🔴 blocking: Core→Shielded and Platform→Shielded silently discard the destination address
src/mcp/tools/wallet.rs (line 1)
🟡 suggestion: Sequential dispatch reports no partial progress on failure
src/mcp/tools/wallet.rs (line 1)
🟡 suggestion: All SendRoutingError variants mapped to McpToolError::InvalidParam
src/mcp/tools/wallet.rs (line 1)
🟡 suggestion: Core→Core hardcodes subtract_fee_from_amount: false — no way to send full balance
src/mcp/tools/wallet.rs (line 1)
🟡 suggestion: build_fee_context logic duplicated between MCP tool and UI
src/mcp/tools/wallet.rs (line 1)
💬 nitpick: platform_shortfall_error always returns TooManyInputs regardless of cause
src/mcp/tools/wallet.rs (line 1)
🤖 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/mcp/tools/wallet.rs`:
- [BLOCKING] line 1: PR violates its own backward-compatibility requirement (US-3)
- [BLOCKING] line 1: Core→Shielded and Platform→Shielded silently discard the destination address
- [SUGGESTION] line 1: Sequential dispatch reports no partial progress on failure
- [SUGGESTION] line 1: All SendRoutingError variants mapped to McpToolError::InvalidParam
- [SUGGESTION] line 1: Core→Core hardcodes subtract_fee_from_amount: false — no way to send full balance
- [SUGGESTION] line 1: build_fee_context logic duplicated between MCP tool and UI
In `docs/CLI.md`:
- [BLOCKING] line 1: CLI.md references removed tools
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push is a merge of v1.0-dev into the feature branch (pulls in #767 and #809). No new feature code — just branch synchronization. All prior review findings from the initial review (backward-compat break, shielded address discard, error mapping, subtract_fee_from_amount) remain unaddressed and still apply.
Reviewed commit: 673fb7d
Summary
src/model/send_routing.rs— a pure, synchronousresolve_send()function that auto-detects destination address type and returns the correctBackendTaskvariantsend_screen.rs— replace 14 individualsend_*_to_*methods with a singlevalidate_and_send()call toresolve_send()(net -914 lines)wallet_funds_sendMCP tool — unified send tool that accepts any address type, withsourceparameter (core/platform/shielded/identity, defaults to "core"). All 14 routing paths supported with proper fee context.core_funds_send, 5 identity credit tools, 5 shielded tools replaced by the single unified tool (-1337 lines)Motivation
Send routing logic (the 4x4 source x destination matrix) lived exclusively in the UI WalletSendScreen. MCP tools couldn't reuse it — each route had its own dedicated tool with duplicated parameter handling. Any new consumer (CLI, automation) would have to reimplement 14 routing paths.
Now both UI and MCP share
resolve_send()as the single source of truth, and MCP callers use one tool instead of twelve.Removed MCP Tools (replaced by
wallet_funds_send)core_funds_sendwallet_funds_send(source=core)identity_credits_topupwallet_funds_send(source=core)identity_credits_topup_from_platformwallet_funds_send(source=platform)identity_credits_transferwallet_funds_send(source=identity)identity_credits_withdrawwallet_funds_send(source=identity)identity_credits_to_addresswallet_funds_send(source=identity)shielded_shield_from_corewallet_funds_send(source=core)shielded_shield_from_platformwallet_funds_send(source=platform)shielded_transferwallet_funds_send(source=shielded)shielded_unshieldwallet_funds_send(source=shielded)shielded_withdrawwallet_funds_send(source=shielded)Test plan
send_routing.rscovering all 14 supported routes, 2 unsupported routes, error cases, balance checks, platform allocation boundary conditionscargo clippy --all-features --all-targets -- -D warningscleancargo test --all-features --workspaceall passwallet_funds_sendMCP tool for core-core, platform-platform paths🤖 Co-authored by Claudius the Magnificent AI Agent