refactor(dash-network): move Network struct inside the dashcore crate#497
refactor(dash-network): move Network struct inside the dashcore crate#497
Conversation
📝 WalkthroughWalkthroughThis PR removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dash/src/sml/quorum_entry/quorum_modifier_type.rs (1)
101-113: Return contract is now misleading.After Line 108,
new_quorum_modifier_typehas noErrpath but still returnsResultand documentsQuorumValidationError. Please either (a) simplify the return type in a breaking-change window, or (b) keepResultbut explicitly document it as compatibility-only and remove the stale error-case docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/sml/quorum_entry/quorum_modifier_type.rs` around lines 101 - 113, The function new_quorum_modifier_type currently always returns Ok(...) but its signature returns Result<LLMQModifierType, QuorumValidationError>, which is misleading; either (A) make it a simple non-fallible constructor by changing the signature to return LLMQModifierType and remove any QuorumValidationError references and stale docs (update callers of new_quorum_modifier_type accordingly), or (B) preserve the Result for compatibility but change the doc/comments on new_quorum_modifier_type to explicitly state the Err branch is compatibility-only (remove stale error-case docs) and keep the implementation as-is; locate the function and related symbols LLMQModifierType, QuorumValidationError, work_block_height, and network.core_v20_activation_height to make the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dash/src/network/constants.rs`:
- Around line 176-186: The FromStr impl for Network must be no-std safe: replace
impl std::str::FromStr for Network with impl core::str::FromStr for Network and
change the associated error type to a new ParseNetworkError enum (e.g., pub enum
ParseNetworkError { UnknownNetwork(&'static str) }) instead of String;
derive/implement Error/Display using thiserror (#[derive(thiserror::Error,
Debug)] with a #[error("Unknown network type: {0}")] variant) or implement
core::fmt::Display manually, then update the impl to return
Err(ParseNetworkError::UnknownNetwork(s)) on the default arm and set type Err =
ParseNetworkError; ensure all references to FromStr import core::str::FromStr
where needed and avoid any heap allocation types so builds work with no-std.
---
Nitpick comments:
In `@dash/src/sml/quorum_entry/quorum_modifier_type.rs`:
- Around line 101-113: The function new_quorum_modifier_type currently always
returns Ok(...) but its signature returns Result<LLMQModifierType,
QuorumValidationError>, which is misleading; either (A) make it a simple
non-fallible constructor by changing the signature to return LLMQModifierType
and remove any QuorumValidationError references and stale docs (update callers
of new_quorum_modifier_type accordingly), or (B) preserve the Result for
compatibility but change the doc/comments on new_quorum_modifier_type to
explicitly state the Err branch is compatibility-only (remove stale error-case
docs) and keep the implementation as-is; locate the function and related symbols
LLMQModifierType, QuorumValidationError, work_block_height, and
network.core_v20_activation_height to make the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4a783cd-72a8-41df-8a32-fe085ed87513
📒 Files selected for processing (43)
.codecov.yml.github/ci-groups.yml.github/ci-no-std.yml.github/workflows/sanitizer.ymlAGENTS.mdCLAUDE.mdCargo.tomlREADME.mddash-network-ffi/Cargo.tomldash-network-ffi/README.mddash-network-ffi/build.rsdash-network-ffi/src/dash_network_ffiFFI.modulemapdash-network-ffi/src/lib.rsdash-network/Cargo.tomldash-network/README.mddash-network/src/lib.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash/Cargo.tomldash/src/address.rsdash/src/blockdata/constants.rsdash/src/blockdata/transaction/mod.rsdash/src/consensus/params.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/lib.rsdash/src/network/constants.rsdash/src/sml/llmq_type/mod.rsdash/src/sml/llmq_type/network.rsdash/src/sml/masternode_list/apply_diff.rsdash/src/sml/masternode_list/from_diff.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/quorum_entry/quorum_modifier_type.rsdash/src/test_utils/address.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/derivation.rskey-wallet/Cargo.tomlkey-wallet/IMPLEMENTATION_SUMMARY.mdkey-wallet/src/bip32.rskey-wallet/src/derivation_bls_bip32.rskey-wallet/src/derivation_slip10.rskey-wallet/src/dip9.rskey-wallet/src/lib.rs
💤 Files with no reviewable changes (15)
- .codecov.yml
- .github/ci-no-std.yml
- dash-network-ffi/build.rs
- key-wallet/IMPLEMENTATION_SUMMARY.md
- dash-network-ffi/README.md
- dash-spv/src/client/lifecycle.rs
- .github/ci-groups.yml
- dash-network-ffi/Cargo.toml
- dash-network/src/lib.rs
- dash-spv/src/sync/masternodes/manager.rs
- key-wallet-ffi/Cargo.toml
- dash-network-ffi/src/lib.rs
- dash-network/Cargo.toml
- dash-network/README.md
- dash-network-ffi/src/dash_network_ffiFFI.modulemap
| impl std::str::FromStr for Network { | ||
| type Err = String; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| match s.to_lowercase().as_str() { | ||
| "dash" | "mainnet" => Ok(Network::Dash), | ||
| "testnet" | "test" => Ok(Network::Testnet), | ||
| "devnet" | "dev" => Ok(Network::Devnet), | ||
| "regtest" => Ok(Network::Regtest), | ||
| _ => Err(format!("Unknown network type: {}", s)), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for no_std support in lib.rs and project structure
cd dash 2>/dev/null || cd .
git ls-files | grep -E '(lib\.rs|Cargo\.toml)' | head -20Repository: dashpay/rust-dashcore
Length of output: 106
🏁 Script executed:
# Verify no-std configuration and check current FromStr implementation
rg -n 'no_std|no-std' --type rust dash/src/lib.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 353
🏁 Script executed:
# Check the actual FromStr implementation
rg -n -A10 'impl.*FromStr.*for.*Network' dash/src/network/constants.rsRepository: dashpay/rust-dashcore
Length of output: 546
🏁 Script executed:
# Check if thiserror is already a dependency
rg -n 'thiserror' dash/Cargo.tomlRepository: dashpay/rust-dashcore
Length of output: 83
🏁 Script executed:
# Check the full Network enum definition
rg -n -B5 'enum Network' dash/src/network/constants.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 329
🏁 Script executed:
# Check how constants.rs is included in the module tree
rg -n 'mod network|mod constants' dash/src/lib.rsRepository: dashpay/rust-dashcore
Length of output: 85
🏁 Script executed:
# Check if the FromStr impl or Network enum has any cfg guards
rg -n -B10 'impl.*FromStr.*for.*Network' dash/src/network/constants.rs | grep -E 'cfg|impl'Repository: dashpay/rust-dashcore
Length of output: 105
🏁 Script executed:
# Check if String is used elsewhere in the codebase and if it's guarded
rg -n 'Result.*String' dash/src/network/constants.rsRepository: dashpay/rust-dashcore
Length of output: 47
🏁 Script executed:
# Verify: does std::str::FromStr actually resolve in no-std?
# Search for any existing use of core::str::FromStr in the codebase
rg -n 'core::str::FromStr|from_str' dash/src/ | head -20Repository: dashpay/rust-dashcore
Length of output: 1931
🏁 Script executed:
# Check the Cargo.toml to understand feature flags better
cat dash/Cargo.toml | grep -A20 '^\[features\]'Repository: dashpay/rust-dashcore
Length of output: 900
🏁 Script executed:
# Check if network module or constants have any cfg guards that might prevent compilation
rg -n -B5 'mod network' dash/src/lib.rsRepository: dashpay/rust-dashcore
Length of output: 173
🏁 Script executed:
# Check network/mod.rs for conditional compilation
cat dash/src/network/mod.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 899
🏁 Script executed:
# Specifically check if FromStr impl is guarded by cfg
rg -n -B20 'impl std::str::FromStr for Network' dash/src/network/constants.rs | head -30Repository: dashpay/rust-dashcore
Length of output: 724
🏁 Script executed:
# Verify the issue: try to understand what happens when no_std is enabled
# Check if there's any std::str usage elsewhere that's guarded
rg -n 'std::str::FromStr' dash/src/Repository: dashpay/rust-dashcore
Length of output: 778
Replace std::str::FromStr with core::str::FromStr and use a typed error enum for no-std compatibility.
The crate supports no-std (feature at line 46 of src/lib.rs), but the Network::FromStr implementation at line 176 uses std::str::FromStr, which is unavailable when std is disabled. Additionally, using String as the error type breaks no-std builds since it requires std allocation. The codebase already uses core::str::FromStr elsewhere (e.g., sign_message.rs:213), and thiserror is available as a dependency.
Create a ParseNetworkError enum and implement core::str::FromStr instead:
Proposed fix
+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)]
+pub enum ParseNetworkError {
+ #[error("unknown network type: {0}")]
+ UnknownNetwork(String),
+}
+
+impl core::str::FromStr for Network {
+ type Err = ParseNetworkError;
-impl std::str::FromStr for Network {
- type Err = String;
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s.to_lowercase().as_str() {
"dash" | "mainnet" => Ok(Network::Dash),
"testnet" | "test" => Ok(Network::Testnet),
"devnet" | "dev" => Ok(Network::Devnet),
"regtest" => Ok(Network::Regtest),
- _ => Err(format!("Unknown network type: {}", s)),
+ _ => Err(ParseNetworkError::UnknownNetwork(s.to_owned())),
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/src/network/constants.rs` around lines 176 - 186, The FromStr impl for
Network must be no-std safe: replace impl std::str::FromStr for Network with
impl core::str::FromStr for Network and change the associated error type to a
new ParseNetworkError enum (e.g., pub enum ParseNetworkError {
UnknownNetwork(&'static str) }) instead of String; derive/implement
Error/Display using thiserror (#[derive(thiserror::Error, Debug)] with a
#[error("Unknown network type: {0}")] variant) or implement core::fmt::Display
manually, then update the impl to return
Err(ParseNetworkError::UnknownNetwork(s)) on the default arm and set type Err =
ParseNetworkError; ensure all references to FromStr import core::str::FromStr
where needed and avoid any heap allocation types so builds work with no-std.
There was a problem hiding this comment.
since this is a refactor where i only move code from one crate into another, I dont think we should address this now. A PR standardizing the Error system following Rust standards with thiserror would be more appropriate
There was a problem hiding this comment.
@ZocoLini, understood — since this is a direct code migration, deferring error system improvements to a dedicated PR makes sense. Would you like me to open a follow-up GitHub issue to track the ParseNetworkError / core::str::FromStr no-std compatibility work for Network in dash/src/network/constants.rs?
✏️ Learnings added
Learnt from: ZocoLini
Repo: dashpay/rust-dashcore PR: 497
File: dash/src/network/constants.rs:176-186
Timestamp: 2026-03-06T17:20:25.174Z
Learning: In the dashpay/rust-dashcore repository, the `FromStr` implementation for `Network` in `dash/src/network/constants.rs` currently uses `std::str::FromStr` with `String` as the error type, which is not no-std compatible. The PR author (ZocoLini) has intentionally deferred fixing this to a separate PR that will standardize the error system using `thiserror` and `core::str::FromStr` across the codebase. Do not flag this as a critical issue in the current refactor PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #497 +/- ##
=============================================
+ Coverage 63.05% 63.99% +0.94%
=============================================
Files 313 313
Lines 64878 64914 +36
=============================================
+ Hits 40907 41540 +633
+ Misses 23971 23374 -597
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
Makes sense, just few things i noticed. The coverage badge in the readme should be addressed and the renaming can be reverted i guess? Also, left few suggestions for comment changes of the network entries while you move them around.
| } | ||
|
|
||
| fn v20_activation_height(&self) -> u32 { | ||
| pub fn core_v20_activation_height(&self) -> u32 { |
There was a problem hiding this comment.
the method was implemented in two places, in daschore was called v20_activation_height and in dash-network was called core_v20_activation_height so I had to pick one. To avoid possible future confusions with platform versions (since it looks like at any moment platform stuff can be added to rust-dashcore) i decided to use the one that make it clear that this v20 activation height method is only meant for core related logic
There was a problem hiding this comment.
I don't think this will be relevant for platform. If ever there would probably be a separate network structure for platform in here but i doubt. Especially now that it's in the core crate.
ZocoLini
force-pushed
the
refactor/dash-network
branch
from
2c609ec to
1ac4908
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dash/src/network/constants.rs (2)
155-160: Spell outNetwork::Devnet | Network::Regtestinstead of_.
_ => 0means any future internalNetworkvariant will silently inherit an activation height of zero. For metadata like this, forcing an explicit arm is safer and keeps new variants from slipping through unnoticed.♻️ Suggested diff
pub fn core_v20_activation_height(&self) -> u32 { match self { Network::Dash => 1_987_776, Network::Testnet => 905_100, // Devnet and regtest activate V20 immediately - _ => 0, + Network::Devnet | Network::Regtest => 0, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/network/constants.rs` around lines 155 - 160, Replace the wildcard match arm in core_v20_activation_height so new Network variants can't silently get zero; explicitly match Network::Devnet and Network::Regtest instead of using `_`. Update the match in the core_v20_activation_height method on the Network enum to use the explicit arm `Network::Devnet | Network::Regtest => 0` (keeping the existing Dash and Testnet arms) so any future variants will cause a compiler error until handled.
128-150: Makeknown_genesis_block_hashstatic instead of decoding hex on every call.This path allocates, reverses bytes, and hits
expect()twice for fixed network constants. Storing the hashes as fixed[u8; 32]values (or sharing constants withdash/src/blockdata/constants.rs) would keep the method panic-free and remove an easy source of metadata drift.
As per coding guidelines "Avoidunwrap()andexpect()in library code; use explicit error types (e.g.,thiserror)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash/src/network/constants.rs` around lines 128 - 150, known_genesis_block_hash currently decodes hex, reverses and calls expect() on every invocation causing allocations and panics; instead define static fixed [u8; 32] constants (already reversed to match internal endianness) for each network (or reuse the shared constants in dash/src/blockdata/constants.rs) and have known_genesis_block_hash return Option<BlockHash> constructed from those arrays without any hex decoding or expect() calls; update the function to reference these constants (e.g., NETWORK_DASH_GENESIS, NETWORK_TESTNET_GENESIS, NETWORK_REGTEST_GENESIS) and construct the BlockHash via a non-panicking constructor or by converting the array directly so the method is allocation-free and panic-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dash/src/network/constants.rs`:
- Around line 155-160: Replace the wildcard match arm in
core_v20_activation_height so new Network variants can't silently get zero;
explicitly match Network::Devnet and Network::Regtest instead of using `_`.
Update the match in the core_v20_activation_height method on the Network enum to
use the explicit arm `Network::Devnet | Network::Regtest => 0` (keeping the
existing Dash and Testnet arms) so any future variants will cause a compiler
error until handled.
- Around line 128-150: known_genesis_block_hash currently decodes hex, reverses
and calls expect() on every invocation causing allocations and panics; instead
define static fixed [u8; 32] constants (already reversed to match internal
endianness) for each network (or reuse the shared constants in
dash/src/blockdata/constants.rs) and have known_genesis_block_hash return
Option<BlockHash> constructed from those arrays without any hex decoding or
expect() calls; update the function to reference these constants (e.g.,
NETWORK_DASH_GENESIS, NETWORK_TESTNET_GENESIS, NETWORK_REGTEST_GENESIS) and
construct the BlockHash via a non-panicking constructor or by converting the
array directly so the method is allocation-free and panic-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c75912c0-fd39-4007-a78c-402053a7ee99
📒 Files selected for processing (43)
.codecov.yml.github/ci-groups.yml.github/ci-no-std.yml.github/workflows/sanitizer.ymlAGENTS.mdCLAUDE.mdCargo.tomlREADME.mddash-network-ffi/Cargo.tomldash-network-ffi/README.mddash-network-ffi/build.rsdash-network-ffi/src/dash_network_ffiFFI.modulemapdash-network-ffi/src/lib.rsdash-network/Cargo.tomldash-network/README.mddash-network/src/lib.rsdash-spv/src/client/lifecycle.rsdash-spv/src/sync/masternodes/manager.rsdash/Cargo.tomldash/src/address.rsdash/src/blockdata/constants.rsdash/src/blockdata/transaction/mod.rsdash/src/consensus/params.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/lib.rsdash/src/network/constants.rsdash/src/sml/llmq_type/mod.rsdash/src/sml/llmq_type/network.rsdash/src/sml/masternode_list/apply_diff.rsdash/src/sml/masternode_list/from_diff.rsdash/src/sml/masternode_list_engine/mod.rsdash/src/sml/quorum_entry/quorum_modifier_type.rsdash/src/test_utils/address.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/derivation.rskey-wallet/Cargo.tomlkey-wallet/IMPLEMENTATION_SUMMARY.mdkey-wallet/src/bip32.rskey-wallet/src/derivation_bls_bip32.rskey-wallet/src/derivation_slip10.rskey-wallet/src/dip9.rskey-wallet/src/lib.rs
💤 Files with no reviewable changes (15)
- dash-spv/src/sync/masternodes/manager.rs
- dash-network-ffi/README.md
- key-wallet/IMPLEMENTATION_SUMMARY.md
- dash-network-ffi/build.rs
- .github/ci-no-std.yml
- dash-network-ffi/src/lib.rs
- key-wallet-ffi/Cargo.toml
- .github/ci-groups.yml
- dash-spv/src/client/lifecycle.rs
- dash-network-ffi/src/dash_network_ffiFFI.modulemap
- .codecov.yml
- dash-network/Cargo.toml
- dash-network/src/lib.rs
- dash-network-ffi/Cargo.toml
- dash-network/README.md
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (14)
- key-wallet/src/bip32.rs
- dash/src/sml/masternode_list/apply_diff.rs
- dash/src/sml/quorum_entry/quorum_modifier_type.rs
- dash/src/blockdata/transaction/mod.rs
- dash/Cargo.toml
- dash/src/crypto/key.rs
- dash/src/sml/masternode_list_engine/mod.rs
- .github/workflows/sanitizer.yml
- key-wallet/src/derivation_slip10.rs
- dash/src/sml/llmq_type/mod.rs
- dash/src/sml/llmq_type/network.rs
- dash/src/test_utils/address.rs
- dash/src/consensus/params.rs
- README.md
2 participants
core_v20_activation_heightandv20_activation_heightmethods into one fixing a little inconsistencySummary by CodeRabbit