fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result#807
fix(mcp): use PlatformAddress in PlatformAddressBalances backend task result#807lklimek wants to merge 1 commit into
Conversation
|
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 13 minutes and 24 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 (4)
✨ 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
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()ifPlatformAddressconversion 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>
lklimek
force-pushed
the
fix/mcp-platform-addresses-bech32m
branch
from
d0c54ce to
6be5388
Compare
lklimek
changed the title
|
✅ Review complete (commit 6be5388) |
There was a problem hiding this comment.
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.
| PlatformAddress::try_from(addr.clone()) | ||
| .ok() | ||
| .map(|pa| (pa, (info.balance, info.nonce))) |
There was a problem hiding this comment.
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.
| 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
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| .filter_map(|(addr, info)| { | ||
| PlatformAddress::try_from(addr.clone()) | ||
| .ok() | ||
| .map(|pa| (pa, (info.balance, info.nonce))) | ||
| }) |
There was a problem hiding this comment.
🟡 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
| .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.
| 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); |
There was a problem hiding this comment.
💬 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
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped fix that centralizes Core->Platform address conversion in the backend task producer. The type swap is sound and consumers are coherently updated. Two real concerns: a silent error-swallow in the new filter_map (both agents converge here), and missing regression coverage for the bech32m output contract. Two architecture nitpicks about duplicated/round-trip conversion logic are accurate but out of scope for this bugfix.
Reviewed commit: 6be5388
🟡 2 suggestion(s) | 💬 2 nitpick(s)
2 additional findings
🟡 suggestion: No regression test for bech32m output and PlatformAddress round-trip
src/mcp/tools/wallet.rs (lines 333-369)
This PR fixes a user-visible serialization bug (MCP platform_addresses_list returning Core/Base58 instead of bech32m) by changing the type in BackendTaskSuccessResult::PlatformAddressBalances, converting in the backend, formatting via to_bech32m_string() here, and converting back in the wallet screen. That cross-module behavior can regress quietly without a test that locks in the contract. Add a focused test that asserts FetchPlatformBalances produces tdash1.../dash1... strings from a PlatformAddressBalances result and preserves the associated balance/nonce values, so a future refactor cannot reintroduce Core formatting at this boundary.
💬 nitpick: Producer-side logging duplicates the Core->Platform conversion done later
src/backend_task/wallet/fetch_platform_address_balances.rs (lines 100-113)
Lines 102-113 iterate provider.found_balances() (Core Address) and re-implement Core->Platform->bech32m via PlatformAddress::try_from(addr.clone()).map(|p| p.to_bech32m_string(self.network)).unwrap_or_else(|_| addr.to_string()). The block at lines 173-177 then performs the same conversion over a superset of the same wallet's addresses. Logging after balances is built — iterating over the already-typed PlatformAddress keys — would remove this duplication and the only place a non-bech32m address could appear in logs (the unwrap_or_else fallback). Pure refactor; no behavior change.
🤖 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 170-178: filter_map silently drops addresses whose PlatformAddress conversion fails
The new `.filter_map(|(addr, info)| PlatformAddress::try_from(addr.clone()).ok().map(...))` discards entries whose `try_from` returns `Err` with no log and no error. Previously this mapping was infallible, so every `wallet.platform_address_info` entry was guaranteed to round-trip back to consumers. After the change, an unsupported address type silently disappears from both the MCP `platform_addresses_list` response and the wallet UI refresh — exactly the failure mode that issue #652 (referenced two lines above) was meant to prevent (UI losing nonces for missing addresses).
In practice, DET only derives BIP32 P2PKH addresses for platform payment, so this should not fire today. But this function already returns `Result<_, TaskError>` and a dedicated `TaskError::AddressConversionFailed { #[source] source: Box<ProtocolError> }` variant exists at `src/backend_task/error.rs:342` for exactly this case. Either propagate the error via that variant, or at minimum log a `tracing::warn!` when an entry is dropped — matching the `tracing::warn!` style used elsewhere in this same block (lines 121, 149, 160).
In `src/mcp/tools/wallet.rs`:
- [SUGGESTION] lines 333-369: No regression test for bech32m output and PlatformAddress round-trip
This PR fixes a user-visible serialization bug (MCP `platform_addresses_list` returning Core/Base58 instead of bech32m) by changing the type in `BackendTaskSuccessResult::PlatformAddressBalances`, converting in the backend, formatting via `to_bech32m_string()` here, and converting back in the wallet screen. That cross-module behavior can regress quietly without a test that locks in the contract. Add a focused test that asserts `FetchPlatformBalances` produces `tdash1...`/`dash1...` strings from a `PlatformAddressBalances` result and preserves the associated balance/nonce values, so a future refactor cannot reintroduce Core formatting at this boundary.
| wallet | ||
| .platform_address_info | ||
| .iter() | ||
| .map(|(addr, info)| (addr.clone(), (info.balance, info.nonce))) | ||
| .filter_map(|(addr, info)| { | ||
| PlatformAddress::try_from(addr.clone()) | ||
| .ok() | ||
| .map(|pa| (pa, (info.balance, info.nonce))) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
🟡 Suggestion: filter_map silently drops addresses whose PlatformAddress conversion fails
The new .filter_map(|(addr, info)| PlatformAddress::try_from(addr.clone()).ok().map(...)) discards entries whose try_from returns Err with no log and no error. Previously this mapping was infallible, so every wallet.platform_address_info entry was guaranteed to round-trip back to consumers. After the change, an unsupported address type silently disappears from both the MCP platform_addresses_list response and the wallet UI refresh — exactly the failure mode that issue #652 (referenced two lines above) was meant to prevent (UI losing nonces for missing addresses).
In practice, DET only derives BIP32 P2PKH addresses for platform payment, so this should not fire today. But this function already returns Result<_, TaskError> and a dedicated TaskError::AddressConversionFailed { #[source] source: Box<ProtocolError> } variant exists at src/backend_task/error.rs:342 for exactly this case. Either propagate the error via that variant, or at minimum log a tracing::warn! when an entry is dropped — matching the tracing::warn! style used elsewhere in this same block (lines 121, 149, 160).
💡 Suggested change
| wallet | |
| .platform_address_info | |
| .iter() | |
| .map(|(addr, info)| (addr.clone(), (info.balance, info.nonce))) | |
| .filter_map(|(addr, info)| { | |
| PlatformAddress::try_from(addr.clone()) | |
| .ok() | |
| .map(|pa| (pa, (info.balance, info.nonce))) | |
| }) | |
| .collect() | |
| wallet | |
| .platform_address_info | |
| .iter() | |
| .map(|(addr, info)| { | |
| PlatformAddress::try_from(addr.clone()) | |
| .map(|pa| (pa, (info.balance, info.nonce))) | |
| .map_err(|e| crate::backend_task::error::TaskError::AddressConversionFailed { | |
| source: Box::new(e), | |
| }) | |
| }) | |
| .collect::<Result<_, _>>()? |
source: ['claude', 'codex']
🤖 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 170-178: filter_map silently drops addresses whose PlatformAddress conversion fails
The new `.filter_map(|(addr, info)| PlatformAddress::try_from(addr.clone()).ok().map(...))` discards entries whose `try_from` returns `Err` with no log and no error. Previously this mapping was infallible, so every `wallet.platform_address_info` entry was guaranteed to round-trip back to consumers. After the change, an unsupported address type silently disappears from both the MCP `platform_addresses_list` response and the wallet UI refresh — exactly the failure mode that issue #652 (referenced two lines above) was meant to prevent (UI losing nonces for missing addresses).
In practice, DET only derives BIP32 P2PKH addresses for platform payment, so this should not fire today. But this function already returns `Result<_, TaskError>` and a dedicated `TaskError::AddressConversionFailed { #[source] source: Box<ProtocolError> }` variant exists at `src/backend_task/error.rs:342` for exactly this case. Either propagate the error via that variant, or at minimum log a `tracing::warn!` when an entry is dropped — matching the `tracing::warn!` style used elsewhere in this same block (lines 121, 149, 160).
| // Convert PlatformAddress back to Core Address for wallet storage | ||
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Platform->Core roundtrip in the UI consumer undercuts the stated simplification
The PR description frames the change as "conversion happens once at the source, not at every consumer", but this consumer still converts Platform->Core via to_address_with_network(network) because wallet.platform_address_info and Wallet::set_platform_address_info are keyed by Core Address. So the producer does Core->Platform, the UI converts back Platform->Core, and set_platform_address_info (src/model/wallet/mod.rs:2229) immediately converts Address->PlatformAddress again to canonicalize/dedupe.
This is functionally correct (P2PKH/P2SH on a single network roundtrips losslessly) but means every new PlatformAddressBalances consumer that wants to update wallet state will repeat this dance. Out of scope for a bugfix PR — worth filing as follow-up to migrate wallet.platform_address_info to be keyed on PlatformAddress directly.
source: ['claude']
Summary
platform_addresses_listMCP tool returned Core address format (yXxx...) instead of Platform bech32m format (tdash1.../dash1...)BackendTaskSuccessResult::PlatformAddressBalancescarriedBTreeMap<Address<NetworkChecked>, ...>(Core addresses), forcing every consumer to convert independentlyBTreeMap<PlatformAddress, ...>— conversion happens once at the source, not at every consumerChanges (4 files)
backend_task/mod.rs—PlatformAddressBalancesvariant now usesPlatformAddresskeysbackend_task/wallet/fetch_platform_address_balances.rs— Converts Core→Platform at the source viaPlatformAddress::try_from()mcp/tools/wallet.rs— Simplified to directaddr.to_bech32m_string(network)callui/wallets/wallets_screen/mod.rs— Converts Platform→Core viato_address_with_network()for wallet storage (model still uses Core keys internally)Test plan
platform_addresses_listMCP tool and verify addresses are returned in bech32m format (tdash1...on testnet,dash1...on mainnet)cargo clippy --all-features --all-targets -- -D warningspasses🤖 Co-authored by Claudius the Magnificent AI Agent