fix(dpp)!: make platform/orchard address decoders network-agnostic#3781
Conversation
`signing_tests` for `ShieldFromAssetLockTransition` imports `crate::shielded::builder`, which only exists under `shielded-client`, but the module was gated on `state-transition-signing` + `core_key_wallet` only. With `core_key_wallet` on but `shielded-client` off, the dpp lib-test harness failed to compile (`unresolved import crate::shielded::builder`). Add `feature = "shielded-client"` to the module's gate (both the `mod` declaration and the file's inner `#![cfg]`) so the test only compiles when its `builder` dependency is available. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`PlatformAddress::from_bech32m_string` and `OrchardAddress::from_bech32m_string` previously returned `(Self, Network)`, deriving the Network from the bech32m HRP. That value was untruthful: the `tdash` HRP is shared by Testnet, Devnet, and Regtest, so the decoder hardcoded `tdash → Testnet` and mis-reported devnet/regtest addresses as Testnet — which broke devnet (a devnet platform address tripped a wrong-network guard). A `PlatformAddress`/`OrchardAddress` is network-agnostic internally; the network is supplied only at `to_bech32m_string(network)` encode time. Both decoders now make no network claim: they validate the HRP is a recognized platform HRP (`dash`/`tdash`) and return just `Self`. The wrong-network safety check moves to the one caller that needs it, `platform-wallet::shielded_unshield_to`, as an HRP-class comparison: the recipient's HRP (segment before the final bech32 `1` separator) is matched case-insensitively against `hrp_for_network(wallet.network)`, catching e.g. a mainnet `dash1…` address pasted into a `tdash` wallet. `shielded_withdraw_to` uses a separate Base58 core-address parser and is unaffected. BREAKING: `from_bech32m_string` return type changed `(Self, Network)` → `Self`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesNetwork-agnostic address parsing with wallet-level validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
…ctor Restores the truthful subset of the network information that the network-agnostic `from_bech32m_string` decoder no longer returns. Per DIP-0018, a bech32m platform address carries no network byte and the prefix `tdash` is shared by Testnet/Devnet/Regtest, so the only network fact recoverable from an address string is mainnet vs non-mainnet. `PlatformAddress::is_mainnet_bech32m(s) -> Result<bool, ProtocolError>` classifies by HRP alone (no payload decode): `dash` → Ok(true), `tdash` → Ok(false), a non-platform HRP or a malformed/separator-less string → Err. Comparison is case-insensitive (bech32m permits all-uppercase). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…is_mainnet_bech32m `check_recipient_hrp` no longer inlines the bech32 HRP extraction and HRP-class comparison; it delegates network classification (and malformed/non-platform rejection) to `PlatformAddress::is_mainnet_bech32m`, making the dpp detector the single source of truth. The guard reduces to `addr_is_mainnet != (network == Network::Mainnet)` — exactly the previous HRP-class check (mainnet wallet requires `dash`, any non-mainnet wallet requires `tdash`). Behavior-preserving: the existing `check_recipient_hrp_tests` (network mismatch, tdash cross-acceptance, uppercase, non-platform, missing-separator, empty) pass unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review all |
|
✅ Actions performedFull review triggered. |
|
✅ Review complete (commit f983080) |
There was a problem hiding this comment.
Pull request overview
Updates platform/orchard bech32m address decoding to be network-agnostic (since tdash is shared by Testnet/Devnet/Regtest), and moves the network-class guard to the wallet caller that needs it.
Changes:
- Change
PlatformAddress::from_bech32m_string/OrchardAddress::from_bech32m_stringto return only the decoded address (no derivedNetwork) and validate only that HRP is a recognized platform HRP. - Add
PlatformAddress::is_mainnet_bech32mand use it inrs-platform-walletto enforce mainnet vs non-mainnet recipient HRP class inshielded_unshield_to, with targeted unit tests. - Feature-gate
shield_from_asset_lock_transitionsigning tests behindshielded-client; update wasm wrapper call sites for the new decoder return type.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/wasm-dpp2/src/platform_address/address.rs | Update wasm bindings to handle from_bech32m_string returning only PlatformAddress. |
| packages/rs-platform-wallet/src/wallet/platform_wallet.rs | Move wrong-network enforcement to wallet via check_recipient_hrp + add tests. |
| packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rs | Gate signing tests behind shielded-client feature to avoid compile failures. |
| packages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/mod.rs | Gate inclusion of signing tests behind shielded-client feature. |
| packages/rs-dpp/src/address_funds/platform_address.rs | Make platform bech32m decoder network-agnostic; add is_mainnet_bech32m helper + tests; update parsing/tests for new return type. |
| packages/rs-dpp/src/address_funds/orchard_address.rs | Make orchard bech32m decoder network-agnostic; update tests for new return type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-dpp/src/address_funds/platform_address.rs`:
- Around line 318-338: The is_mainnet_bech32m function currently only splits on
the last '1' and checks HRP, allowing malformed strings to pass; update
is_mainnet_bech32m to fully validate the bech32m string before classifying the
HRP: attempt to decode the input using the bech32 decoder (or the project’s
existing bech32/bech32m utility) and ensure the variant is Bech32m and the data
part is non-empty, then check hrp.eq_ignore_ascii_case(PLATFORM_HRP_MAINNET) or
PLATFORM_HRP_TESTNET and return Ok(true/false); on any decode/validation failure
return ProtocolError::DecodingError (keep check_recipient_hrp behavior unchanged
except it will now see validated addresses).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e0798f-49eb-4fcc-a3f2-093fc94f6e9f
📒 Files selected for processing (6)
packages/rs-dpp/src/address_funds/orchard_address.rspackages/rs-dpp/src/address_funds/platform_address.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/wasm-dpp2/src/platform_address/address.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3781 +/- ##
=========================================
Coverage 71.20% 71.20%
=========================================
Files 20 20
Lines 2837 2837
=========================================
Hits 2020 2020
Misses 817 817
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Pinned commit 71052a4 is a small, behavior-preserving refactor that routes check_recipient_hrp through the existing PlatformAddress::is_mainnet_bech32m helper to centralize network classification. The diff is a clean inversion (17 added / 29 removed) and the existing check_recipient_hrp_tests continue to cover mismatch, tdash cross-acceptance, uppercase, non-platform HRP, missing-separator and empty cases. The convergent finding from Codex/Claude about is_mainnet_bech32m doing HRP-only classification rather than full bech32m decoding targets a function added in an earlier commit (7a02728) whose docstring explicitly documents the HRP-only contract — the pinned commit does not introduce that behavior and the address is still rejected by the subsequent from_bech32m_string decode for any HRP-class-matching but malformed input.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/rs-dpp/src/address_funds/platform_address.rs (1)
318-338:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate the full address before classifying its HRP.
This helper only splits on the last
1, so malformed inputs likedash1!ordash1still classify as mainnet. Downstream,shielded_unshield_tocan then return a bogus network-mismatch error before the real decoder gets a chance to reject the invalid address. Reuse the strict bech32m parser here instead of HRP-only string splitting. (docs.rs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rs-dpp/src/address_funds/platform_address.rs` around lines 318 - 338, The is_mainnet_bech32m helper currently only splits on the last '1' which misclassifies malformed inputs; replace the HRP-only split with a strict bech32 decoder (use bech32::decode) inside is_mainnet_bech32m to fully validate the address, ensure the variant is Bech32m, extract the decoded HRP, then compare it against PLATFORM_HRP_MAINNET and PLATFORM_HRP_TESTNET and return Ok(true)/Ok(false) or a ProtocolError::DecodingError on invalid variant/decoding failures; keep error messages descriptive and use the existing function name and HRP constants to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-dpp/src/address_funds/orchard_address.rs`:
- Around line 100-102: OrchardAddress::from_bech32m_string currently uses
bech32::decode which accepts both Bech32 and Bech32m checksums; change it to use
the strict Bech32m API (e.g., bech32::primitives::CheckedHrpString /
decode_bech32m or equivalent decode_bech32m helper) so only Bech32m-checked
inputs succeed, map primitive decode errors into ProtocolError::DecodingError
like the current code, and update/add a unit test ensuring a Bech32
(non-Bech32m) encoded address is rejected by from_bech32m_string.
In `@packages/rs-dpp/src/address_funds/platform_address.rs`:
- Around line 259-262: PlatformAddress::from_bech32m_string currently calls
bech32::decode(s) which accepts both Bech32 and Bech32m checksums; change the
implementation to explicitly validate the checksum is Bech32m (rather than any
Bech32 variant) before proceeding. Replace the bech32::decode call in
from_bech32m_string with the bech32 primitives API that enforces Bech32m (e.g.,
the CheckedHrpstring/Bech32m-specific decode path) and map any non-Bech32m or
decode failures to ProtocolError::DecodingError so plain Bech32-checked strings
are rejected.
---
Duplicate comments:
In `@packages/rs-dpp/src/address_funds/platform_address.rs`:
- Around line 318-338: The is_mainnet_bech32m helper currently only splits on
the last '1' which misclassifies malformed inputs; replace the HRP-only split
with a strict bech32 decoder (use bech32::decode) inside is_mainnet_bech32m to
fully validate the address, ensure the variant is Bech32m, extract the decoded
HRP, then compare it against PLATFORM_HRP_MAINNET and PLATFORM_HRP_TESTNET and
return Ok(true)/Ok(false) or a ProtocolError::DecodingError on invalid
variant/decoding failures; keep error messages descriptive and use the existing
function name and HRP constants to locate the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a96225bb-7095-4d18-8312-24685dbb767e
📒 Files selected for processing (6)
packages/rs-dpp/src/address_funds/orchard_address.rspackages/rs-dpp/src/address_funds/platform_address.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/mod.rspackages/rs-dpp/src/state_transition/state_transitions/shielded/shield_from_asset_lock_transition/signing_tests.rspackages/rs-platform-wallet/src/wallet/platform_wallet.rspackages/wasm-dpp2/src/platform_address/address.rs
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "7ee5be1d943795694d3ecc9080a3288fd182f7953e0785150b8d2ef22abb95fa"
)Xcode manual integration:
|
…ess-hrp-network-decode-p # Conflicts: # packages/rs-platform-wallet/src/wallet/platform_wallet.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at head 0d64951 confirms the PR's network-agnostic decoder refactor, the wallet-side check_recipient_hrp guard delegating to PlatformAddress::is_mainnet_bech32m, the WASM binding updates, and the signing-tests gate are all intact and correct. The only PR-side delta since prior commit 71052a4 is a merge with v3.1-dev plus one new test (hrp_is_shared_across_all_non_mainnet_networks in platform_address.rs) documenting the tdash-HRP sharing invariant — directly aligned with the PR's stated goal. Prior review found no in-scope issues; no carried-forward findings, and no new defects in the latest delta or cumulatively at the current head. All six agents and CodeRabbit returned zero findings.
… drop double-prefix error
A. is_mainnet_bech32m now calls bech32::decode() instead of rsplit_once('1'),
so malformed strings like "dash1!" no longer return Ok(true) — the data
part and checksum are fully validated before the HRP is inspected.
B. Extracted classify_platform_hrp() as a pub(crate) free function that is the
single source of truth for HRP validation. Both from_bech32m_string
implementations (PlatformAddress and OrchardAddress) and is_mainnet_bech32m
delegate to it, eliminating the duplicated inline if-chain.
C. Removed the "invalid platform address: " prefix from is_mainnet_bech32m's
error messages; check_recipient_hrp already wraps with that prefix once, so
the previous code produced double-prefixed messages like
"invalid platform address: Decoding Error - invalid platform address: ...".
The final user-facing string is now clean and non-redundant.
D. Trimmed the from_bech32m_string doc comments in both platform_address.rs and
orchard_address.rs: removed the cross-reference to to_bech32m_string, added
a reference to is_mainnet_bech32m as the network-guard entry point.
Updated tests: constructed valid bech32m strings for non-platform HRP cases
(replacing the previously-used "bc1qexampledata" which now fails checksum
before HRP classification); updated separator-error assertions to match
bech32::decode's actual error messages; added dash1! regression test.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative verification at head f983080 confirms the PR's network-agnostic decoder refactor is intact and correct: classify_platform_hrp is properly factored as a pub(crate) helper and consumed by both PlatformAddress::from_bech32m_string and OrchardAddress::from_bech32m_string; is_mainnet_bech32m is now strict via a full bech32::decode (replacing the prior pre-1 HRP sniff) and its tests were updated accordingly; check_recipient_hrp continues to compare HRP class (mainnet vs non-mainnet) rather than equating Testnet/Devnet/Regtest. Prior review at 0d64951 found no in-scope issues, so there are no carried-forward prior findings; the latest delta introduces no new defects, and all six agents plus CodeRabbit returned zero findings.
Issue being fixed or feature implemented
PlatformAddress::from_bech32m_string/OrchardAddress::from_bech32m_stringreturned(Self, Network), derivingNetworkfrom the bech32m HRP. The HRPtdashis shared by Testnet/Devnet/Regtest (hrp_for_networkis non-injective), so the decoder could not truthfully determine the network and always returnedNetwork::Testnetfortdash.On devnet (paloma) a valid devnet platform address decoded as
Testnet, tripping the wrong-network guard in the wallet'sshielded_unshield_toand blocking unshield-to-transparent.What was done?
Option P — network-agnostic decoders. A
PlatformAddress/OrchardAddresscarries no network internally (network is supplied only atto_bech32m_string(network)encode time), so:from_bech32m_string(s) -> Result<Self, ProtocolError>validates the HRP is a recognized platform HRP (dash/tdash) but makes no network claim.shielded_unshield_to, via a private testable helpercheck_recipient_hrp(recipient, network)that compares the recipient's HRP class against the wallet network (PlatformAddress::hrp_for_network): rejects mainnetdash1…on atdashwallet, accepts devnettdash1…on a devnet wallet, and reports a clear "not a platform address" error for non-platform HRPs (e.g.bc1…).test(dpp): feature-gate signing_tests behind shielded-client— fixes a pre-existingv3.1-devcompile failure wheresigning_testsimportedcrate::shielded::builder(only present undershielded-client), which brokecargo test -p dpp.How Has This Been Tested?
cargo test -p dpp --lib address_funds→ 161 passed (incl.test_bech32m_invalid_hrp_fails)cargo test -p dpp --lib --features shielded-client address_funds::orchard→ 8 passedcargo test -p platform-wallet --features shielded check_recipient_hrp_tests→ 9 passed (mainnet→testnet/devnet REJECT, devnet→devnet ACCEPT, testnet→testnet ACCEPT, tdash cross-acceptance, uppercase ACCEPT,bc1…not-a-platform-address, no-separator/empty Err)cargo check+cargo clippy -- -D warningsclean on dpp, platform-wallet (--features shielded), wasm-dpp2 (changed files)shielded_unshield_to(previously rejected as wrong-network).Breaking Changes
from_bech32m_stringreturn type changes(Self, Network)→Self(hencefix(dpp)!). It is not in any prelude — external consumers reach it only viadpp::address_funds::PlatformAddress/OrchardAddress, so the break is a loud compile error, never silent.Checklist:
For repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
Summary by CodeRabbit
Breaking Changes
New Features
Improvements