fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result by lklimek · Pull Request #807 · dashpay/dash-evo-tool

lklimek · 2026-03-31T14:05:42Z

Summary

Bug: platform_addresses_list MCP tool returned Core address format (yXxx...) instead of Platform bech32m format (tdash1... / dash1...)
Root cause: BackendTaskSuccessResult::PlatformAddressBalances carried BTreeMap<Address<NetworkChecked>, ...> (Core addresses), forcing every consumer to convert independently
Fix: Changed the backend task result to carry BTreeMap<PlatformAddress, ...> — conversion happens once at the source, not at every consumer

Changes (4 files)

backend_task/mod.rs — PlatformAddressBalances variant now uses PlatformAddress keys
backend_task/wallet/fetch_platform_address_balances.rs — Converts Core→Platform at the source via PlatformAddress::try_from()
mcp/tools/wallet.rs — Simplified to direct addr.to_bech32m_string(network) call
ui/wallets/wallets_screen/mod.rs — Converts Platform→Core via to_address_with_network() for wallet storage (model still uses Core keys internally)

Test plan

Call platform_addresses_list MCP tool and verify addresses are returned in bech32m format (tdash1... on testnet, dash1... on mainnet)
Verify wallet screen still displays platform address balances correctly after refresh
cargo clippy --all-features --all-targets -- -D warnings passes

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

coderabbitai · 2026-03-31T14:05:49Z

Warning

Rate limit exceeded

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

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

⌛ How to resolve this issue?

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

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

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

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

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 85f65858-11e4-430d-8127-c137789ca0db

📥 Commits

Reviewing files that changed from the base of the PR and between 92c34f5 and 6be5388.

📒 Files selected for processing (4)

src/backend_task/mod.rs
src/backend_task/wallet/fetch_platform_address_balances.rs
src/mcp/tools/wallet.rs
src/ui/wallets/wallets_screen/mod.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/mcp-platform-addresses-bech32m

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copilot

Pull request overview

Fixes the platform_addresses_list MCP tool to return Platform (DIP-18) bech32m-encoded addresses (dash1... / tdash1...) instead of Core base58 address strings by converting Core Address values through PlatformAddress::try_from(...).to_bech32m_string(network) with a safe fallback.

Changes:

Convert returned platform addresses to bech32m using the current app network.
Add a conversion fallback to Core addr.to_string() if PlatformAddress conversion fails (avoids panics / preserves prior behavior on error).

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

… result Move Core->Platform address conversion from consumers into the backend task producer. The PlatformAddressBalances variant now carries BTreeMap<PlatformAddress, (u64, u32)> instead of BTreeMap<Address, ...>, so the MCP tool can call to_bech32m_string() directly and the UI consumer converts back to Core Address only for wallet storage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

thepastaclaw · 2026-04-01T11:06:04Z

✅ Review complete (commit 6be5388)

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-04-01T11:10:28Z

+                    PlatformAddress::try_from(addr.clone())
+                        .ok()
+                        .map(|pa| (pa, (info.balance, info.nonce)))


PlatformAddress::try_from(addr.clone()).ok() inside filter_map silently drops entries that fail Core→Platform conversion, which can cause the task to return an incomplete balances map (and hide/lose previously stored balances/nonces) without any signal. Consider making conversion failure explicit (e.g., collect into a Result and return TaskError::AddressConversionFailed, or at least log a warning and/or trigger cleanup of the invalid wallet entry) so consumers don’t get partial results.

Suggested change

PlatformAddress::try_from(addr.clone())

.ok()

.map(|pa| (pa, (info.balance, info.nonce)))

match PlatformAddress::try_from(addr.clone()) {

Ok(pa) => Some((pa, (info.balance, info.nonce))),

Err(e) => {

tracing::warn!(

?addr,

error = %e,

"Failed to convert wallet address to PlatformAddress; dropping from balances result"

);

None

}

}

thepastaclaw

Code Review

PR correctly moves Core→Platform address conversion to the producer side, fixing bech32m serialization in MCP. The .map() → .filter_map(.ok()) change introduces silent balance drops on conversion failure (convergent finding across agents). The type change also introduces a network-mismatch regression: previously Address carried the network from the backend; now PlatformAddress is network-agnostic and the UI rebinds from the current context, which can differ after a network switch during the async task.

Reviewed commit: 6be5388

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

1 additional finding

💬 nitpick: Redundant scoped `use PlatformAddress` import — now imported at module level

src/backend_task/wallet/fetch_platform_address_balances.rs (line 103)

Verified. Line 8 imports PlatformAddress at module scope (added by this PR). The scoped use dash_sdk::dpp::address_funds::PlatformAddress; at line 103 inside the logging loop is now redundant. Pre-existing code, but this PR is the right place to clean it up since it added the module-level import.

🤖 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/backend_task/wallet/fetch_platform_address_balances.rs`:
- [SUGGESTION] lines 173-177: Silent address drop via filter_map + .ok() loses data without trace
  Convergent finding (claude + codex). Verified at lines 173-177: `PlatformAddress::try_from(addr.clone()).ok()` silently drops any address that isn't P2PKH/P2SH. For wallet-derived addresses this should never fail (they're always P2PKH), but if it did, the balance would vanish from the result while the task reports success. The old code used `.map()` which was infallible. Consider logging at warn level so the failure is observable.

Note: the logging block at lines 102-113 already has the exact same pattern (`PlatformAddress::try_from(...).unwrap_or_else(|_| addr.to_string())`) — it logs a fallback rather than dropping. The return-path should be at least as defensive.

thepastaclaw · 2026-04-01T11:30:47Z

+                .filter_map(|(addr, info)| {
+                    PlatformAddress::try_from(addr.clone())
+                        .ok()
+                        .map(|pa| (pa, (info.balance, info.nonce)))
+                })


🟡 Suggestion: Silent address drop via filter_map + .ok() loses data without trace

Convergent finding (claude + codex). Verified at lines 173-177: PlatformAddress::try_from(addr.clone()).ok() silently drops any address that isn't P2PKH/P2SH. For wallet-derived addresses this should never fail (they're always P2PKH), but if it did, the balance would vanish from the result while the task reports success. The old code used .map() which was infallible. Consider logging at warn level so the failure is observable.

Note: the logging block at lines 102-113 already has the exact same pattern (PlatformAddress::try_from(...).unwrap_or_else(|_| addr.to_string())) — it logs a fallback rather than dropping. The return-path should be at least as defensive.

💡 Suggested change

Suggested change

.filter_map(|(addr, info)| {

PlatformAddress::try_from(addr.clone())

.ok()

.map(|pa| (pa, (info.balance, info.nonce)))

})

.filter_map(|(addr, info)| {

match PlatformAddress::try_from(addr.clone()) {

Ok(pa) => Some((pa, (info.balance, info.nonce))),

Err(e) => {

tracing::warn!("Skipping address {addr} — not convertible to PlatformAddress: {e}");

None

}

}

})

_{source: unknown}

🤖 Fix this 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/backend_task/wallet/fetch_platform_address_balances.rs`: - [SUGGESTION] lines 173-177: Silent address drop via filter_map + .ok() loses data without trace Convergent finding (claude + codex). Verified at lines 173-177: `PlatformAddress::try_from(addr.clone()).ok()` silently drops any address that isn't P2PKH/P2SH. For wallet-derived addresses this should never fail (they're always P2PKH), but if it did, the balance would vanish from the result while the task reports success. The old code used `.map()` which was infallible. Consider logging at warn level so the failure is observable. Note: the logging block at lines 102-113 already has the exact same pattern (`PlatformAddress::try_from(...).unwrap_or_else(|_| addr.to_string())`) — it logs a fallback rather than dropping. The return-path should be at least as defensive.

thepastaclaw · 2026-04-01T11:30:47Z

+                    let network = self.app_context.network();
+                    for (platform_addr, (balance, nonce)) in balances {
+                        let core_addr = platform_addr.to_address_with_network(network);
+                        wallet.set_platform_address_info(core_addr, balance, nonce);


💬 Medium: Network-mismatch regression: UI rebinds PlatformAddress with current network, not originating network

Verified regression introduced by this PR. Before the change, balances was BTreeMap<Address<NetworkChecked>, ...> — the address carried the network from the backend task's AppContext, which is captured at dispatch time (app.rs:925). The old handler passed addresses directly to wallet.set_platform_address_info() with no network rebinding.

Now PlatformAddress is network-agnostic (stores only 20-byte hashes, confirmed in SDK source). Line 2885 reads self.app_context.network() which reflects the current screen context — change_context() swaps this Arc on network switch. If the user switches networks while the fetch is in flight, to_address_with_network() would encode the addresses under the wrong network.

The MCP path (wallet.rs:354) is safe because ctx is a snapshot captured at invocation start.

Fix: embed the originating Network in the PlatformAddressBalances result variant and use it in the UI handler instead of reading from the current context.

_{source: unknown}

lklimek requested a review from Copilot March 31, 2026 14:10

Copilot started reviewing on behalf of lklimek March 31, 2026 14:10 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

lklimek force-pushed the fix/mcp-platform-addresses-bech32m branch from d0c54ce to 6be5388 Compare March 31, 2026 14:29

lklimek changed the title ~~fix(mcp): convert platform addresses to bech32m format~~ fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result Mar 31, 2026

lklimek requested a review from Copilot April 1, 2026 11:05

lklimek marked this pull request as ready for review April 1, 2026 11:05

Copilot started reviewing on behalf of lklimek April 1, 2026 11:06 View session

Copilot AI reviewed Apr 1, 2026

View reviewed changes

thepastaclaw reviewed Apr 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result#807

fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result#807
lklimek wants to merge 1 commit intov1.0-devfrom
fix/mcp-platform-addresses-bech32m

lklimek commented Mar 31, 2026 •

edited

Loading

Uh oh!

coderabbitai bot commented Mar 31, 2026 •

edited

Loading

Rate limit exceeded

Uh oh!

Copilot AI left a comment

Uh oh!

thepastaclaw commented Apr 1, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 1, 2026

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Apr 1, 2026

Uh oh!

thepastaclaw Apr 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-                    PlatformAddress::try_from(addr.clone())
-                        .ok()
-                        .map(|pa| (pa, (info.balance, info.nonce)))
+                    match PlatformAddress::try_from(addr.clone()) {
+                        Ok(pa) => Some((pa, (info.balance, info.nonce))),
+                        Err(e) => {
+                            tracing::warn!(
+                                ?addr,
+                                error = %e,
+                                "Failed to convert wallet address to PlatformAddress; dropping from balances result"
+                            );
+                            None
+                        }
+                    }

Conversation

lklimek commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes (4 files)

Test plan

Uh oh!

coderabbitai bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rate limit exceeded

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

thepastaclaw commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

1 additional finding

Uh oh!

thepastaclaw Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lklimek commented Mar 31, 2026 •

edited

Loading

coderabbitai bot commented Mar 31, 2026 •

edited

Loading

thepastaclaw commented Apr 1, 2026 •

edited

Loading