chore: update dashcore deps to v0.42#3375
Merged
Merged
Conversation
- Add [patch] section pointing to local rust-dashcore checkout - Rename Network::Dash to Network::Mainnet across all packages to match the dashcore API change Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update rust-dashcore dependency rev to 42eb1d69 which includes: - Network::Dash renamed to Network::Mainnet - &mut Wallet → &Wallet in WalletTransactionChecker::check_core_transaction Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
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 (1)
📝 WalkthroughWalkthroughThis PR updates workspace dependency revisions and systematically replaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/wasm-dpp2/src/core/network.rs (1)
124-134:⚠️ Potential issue | 🔴 CriticalAdd explicit handling for
Network::Dashvariant in theFrom<Network> for NetworkWasmimpl.The
Network::Dashvariant is actively used in production code (e.g.,packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs:98). If aNetwork::Dashvalue flows through this conversion, theunreachable!()at line 131 will panic at runtime.Add explicit mapping for
Network::Dash:Network::Dash => NetworkWasm::Mainnet,or choose an appropriate
NetworkWasmvariant based on the intended semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/wasm-dpp2/src/core/network.rs` around lines 124 - 134, The From<Network> for NetworkWasm impl's from() currently panics on unknown variants; add an explicit arm for Network::Dash (mapping it to the intended NetworkWasm variant, e.g., NetworkWasm::Mainnet) inside the match so Dash is handled instead of falling through to the unreachable!(); update the match in the impl From<Network> for NetworkWasm (the from function) to include Network::Dash => NetworkWasm::<appropriate_variant>.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-drive-abci/src/config.rs`:
- Around line 291-295: The parser match for network_name must explicitly accept
the "dash" alias (add a `"dash" => Ok(Network::Dash)` arm) so legacy configs
normalize to Network::Dash, and the default_for_network logic must not let
Network::Dash fall through to testnet—add an explicit `Network::Dash =>
Self::default_dash()` (or the appropriate Dash defaults) arm instead of relying
on the catch-all `_ => Self::default_testnet()`, updating any helper name to the
existing default method for Dash to ensure Dash is routed correctly.
---
Outside diff comments:
In `@packages/wasm-dpp2/src/core/network.rs`:
- Around line 124-134: The From<Network> for NetworkWasm impl's from() currently
panics on unknown variants; add an explicit arm for Network::Dash (mapping it to
the intended NetworkWasm variant, e.g., NetworkWasm::Mainnet) inside the match
so Dash is handled instead of falling through to the unreachable!(); update the
match in the impl From<Network> for NetworkWasm (the from function) to include
Network::Dash => NetworkWasm::<appropriate_variant>.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d55a649-563f-48fa-8896-fc6d65a307c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
Cargo.tomlpackages/rs-dpp/src/address_funds/orchard_address.rspackages/rs-dpp/src/address_funds/platform_address.rspackages/rs-dpp/src/core_subsidy/mod.rspackages/rs-dpp/src/data_contract/associated_token/token_perpetual_distribution/reward_distribution_type/validation/v0/mod.rspackages/rs-dpp/src/identity/identity_public_key/key_type.rspackages/rs-drive-abci/src/abci/handler/finalize_block.rspackages/rs-drive-abci/src/abci/handler/info.rspackages/rs-drive-abci/src/abci/handler/prepare_proposal.rspackages/rs-drive-abci/src/abci/handler/process_proposal.rspackages/rs-drive-abci/src/config.rspackages/rs-drive-abci/src/execution/engine/consensus_params_update/v0/mod.rspackages/rs-drive-abci/src/execution/engine/consensus_params_update/v1/mod.rspackages/rs-drive-abci/src/execution/platform_events/protocol_upgrade/check_for_desired_protocol_upgrade/v0/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/action_validation/document/document_create_transition_action/state_v1/mod.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/tests/document/creation.rspackages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/identity_create_from_addresses/tests.rspackages/rs-drive-abci/tests/strategy_tests/strategy.rspackages/rs-drive/src/config.rspackages/rs-drive/src/drive/document/insert_contested/add_contested_document_for_contract_operations/v0/mod.rspackages/rs-drive/src/drive/votes/cleanup/remove_all_votes_given_by_identities/v0/mod.rspackages/rs-sdk-ffi/src/crypto/mod.rspackages/rs-sdk-ffi/src/identity/helpers.rspackages/rs-sdk-ffi/src/sdk.rspackages/rs-sdk-ffi/src/signer_simple.rspackages/rs-sdk-ffi/src/system/queries/platform_status.rspackages/rs-sdk-ffi/src/system/status.rspackages/rs-sdk-trusted-context-provider/src/lib.rspackages/rs-sdk-trusted-context-provider/src/provider.rspackages/rs-sdk/src/sdk.rspackages/strategy-tests/src/transitions.rspackages/wasm-dpp2/src/core/network.rspackages/wasm-sdk/src/context_provider.rspackages/wasm-sdk/src/sdk.rs
Fix formatting in provider.rs and add "dash" as a network name alias mapping to Network::Mainnet in config.rs for legacy config compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-drive-abci/src/config.rs (1)
291-295: Add a regression test for the new mainnet canonicalization path.This PR changes both parsing (
"dash"->Network::Mainnet) and defaults (PlatformConfig::default()now resolving to mainnet), but the current tests do not assert either behavior. A small focused unit test here would make this migration much harder to regress.🧪 Minimal regression test
+ #[test] + fn test_mainnet_canonicalization() { + #[derive(serde::Deserialize)] + struct NetworkOnly { + #[serde(deserialize_with = "super::from_str_to_network_with_aliases")] + network: Network, + } + + let parsed: NetworkOnly = serde_json::from_str(r#"{"network":"dash"}"#).unwrap(); + assert_eq!(parsed.network, Network::Mainnet); + assert_eq!(super::PlatformConfig::default().network, Network::Mainnet); + }Also applies to: 633-635, 825-828
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-drive-abci/src/config.rs` around lines 291 - 295, Add a focused unit test in the same module (packages/rs-drive-abci/src/config.rs) that asserts the new canonicalization and default behavior: verify that parsing "dash" (via the match branch that maps network_name.as_str() to Network::Mainnet) yields Network::Mainnet and that PlatformConfig::default() returns a config whose network resolves to Network::Mainnet; use the existing Network::from_str or the public parse path and PlatformConfig::default() in the test to prevent regressions of the "dash" -> Network::Mainnet mapping and the default mainnet change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-drive-abci/src/config.rs`:
- Around line 291-295: Add a focused unit test in the same module
(packages/rs-drive-abci/src/config.rs) that asserts the new canonicalization and
default behavior: verify that parsing "dash" (via the match branch that maps
network_name.as_str() to Network::Mainnet) yields Network::Mainnet and that
PlatformConfig::default() returns a config whose network resolves to
Network::Mainnet; use the existing Network::from_str or the public parse path
and PlatformConfig::default() in the test to prevent regressions of the "dash"
-> Network::Mainnet mapping and the default mainnet change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c52e48a-b78c-4526-a285-d8eb9a145489
📒 Files selected for processing (2)
packages/rs-drive-abci/src/config.rspackages/rs-sdk-trusted-context-provider/src/provider.rs
thepastaclaw
suggested changes
Mar 17, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review
The dashcore v0.42 dependency bump correctly renames Network::Dash → Network::Mainnet across 35 files and adds a backwards-compatible "dash" alias in the config deserializer. However, the migration is incomplete: 13 references to the removed Network::Dash variant remain (1 in production code, 12 in tests), which will fail to compile if the deprecated alias is fully removed in dashcore v0.42. The config alias is also case-sensitive, which may break existing deployments using capitalized network names.
Reviewed commit: 2c8820c
🔴 1 blocking | 🟡 2 suggestion(s) | 💬 1 nitpick(s)
4 additional finding(s) omitted (not in diff).
🤖 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 `packages/rs-platform-wallet/src/platform_wallet_info/matured_transactions.rs`:
- [BLOCKING] line 98: Network::Dash not updated — breaks mainnet identity key derivation if alias removed
This production code selects the identity authentication derivation path based on network. Line 98 still matches on `Network::Dash` instead of `Network::Mainnet`. If dashcore v0.42 removed the `Dash` variant entirely (as indicated by this PR's rename of all other occurrences), this won't compile. Even if `Dash` exists as a deprecated alias, it should be updated for consistency — all other call sites were migrated. If the alias is eventually removed, mainnet wallet users would fall through to the `_ =>` catch-all which returns an error, breaking mainnet identity key derivation.
In `packages/rs-drive-abci/src/execution/engine/consensus_params_update/mod.rs`:
- [SUGGESTION] line 95: 12 test references to Network::Dash not updated
The `consensus_params_update` test module contains 12 occurrences of `Network::Dash` (lines 95, 115, 133, 168, 190, 205, 216, 334, 358, 488, 501, 529) that were missed in the rename. These tests validate mainnet consensus parameter updates. All should be updated to `Network::Mainnet` for consistency with the rest of the migration. If the deprecated alias is removed in a future dashcore release, these tests will fail to compile.
In `packages/rs-drive-abci/src/config.rs`:
- [SUGGESTION] lines 290-296: Network alias matching is case-sensitive — may reject previously valid config values
The `from_str_to_network_with_aliases` function matches on `network_name.as_str()` directly, so `"Dash"`, `"DASH"`, or `"Mainnet"` would not be caught by the explicit `"dash" | "mainnet"` arm. If any existing deployment configs use capitalized network names (which previously worked via dashcore's case-handling), they'll fall through to `Network::from_str()` which may also reject them. Consider normalizing to lowercase before matching.
- Replace remaining Network::Dash with Network::Mainnet in tests and platform_wallet_info - Make network alias parsing case-insensitive in config deserializer - Add comprehensive alias coverage (dash, main, local, test, dev) - Fix WalletTransactionChecker trait signature (&mut Wallet -> &Wallet) - Fix drive-proof-verifier unused import and ContestedVote field visibility - Apply cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
thepastaclaw
approved these changes
Mar 17, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Code Review — Incremental (push)
All three findings from the prior review have been addressed:
- ✅ [BLOCKING]
Network::Dashin matured_transactions.rs → updated toNetwork::Mainnet - ✅ [SUGGESTION] 12 test references to
Network::Dash→ all 12 updated toNetwork::Mainnet - ✅ [SUGGESTION] Case-sensitive network alias matching → now uses
.to_lowercase()before matching
Additional changes in this push (ContestedVote fields made pub, &mut Wallet → &Wallet signature update, UFCS for RPITIT trait resolution) are correct adaptations to the dashcore v0.42 API. No remaining Network::Dash references found in the codebase.
Reviewed commit: 278cb4b
Collaborator
Author
Addressed review commentsNetwork::Dash in wasm-dpp2 network.rs — This is a false positive. Network alias parsing in config.rs — Fixed in 278cb4b:
Remaining Network::Dash references — Fixed in 278cb4b: updated |
…f-verifier The PlatformSerialize/PlatformDeserialize derive macros generate code that references ProtocolError, but it was not imported in the mocks feature gate of types.rs, causing compilation failures. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v3.1-dev #3375 +/- ##
============================================
+ Coverage 75.87% 75.88% +0.01%
============================================
Files 2912 2914 +2
Lines 283860 284096 +236
============================================
+ Hits 215375 215585 +210
- Misses 68485 68511 +26
🚀 New features to boost your workflow:
|
Contributor
|
✅ 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: "18af3aaf70bcc7c6458326210604e3b0833724fa6c809cbe9be4e66b3ecc5d79"
)Xcode manual integration:
|
thepastaclaw
approved these changes
Mar 17, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Incremental Review — ed2f98d4 (push onto approved PR)
Change: Adds missing ProtocolError import for mocks feature in rs-drive-proof-verifier/src/types.rs.
Assessment: Trivial compilation fix — the ProtocolError type was already used in this module under the mocks feature but not imported after the dashcore dependency bump. No logic change, no new findings.
Prior findings: All 3 findings from initial review were resolved in the previous push (278cb4b).
Verdict: ✅ Still APPROVE — no new issues.
github-project-automation
Bot
moved this from In review / testing
to Done
in Platform team
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Update rust dashcore to latest dev version.
What was done?
How Has This Been Tested?
With existing tests
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit