refactor: rename Dash network entries to Mainnet#500
Open
xdustinface wants to merge 1 commit intov0.42-devfrom
Open
refactor: rename Dash network entries to Mainnet#500xdustinface wants to merge 1 commit intov0.42-devfrom
Dash network entries to Mainnet#500xdustinface wants to merge 1 commit intov0.42-devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request systematically renames the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #500 +/- ##
=============================================
+ Coverage 64.46% 66.89% +2.42%
=============================================
Files 313 313
Lines 64752 64757 +5
=============================================
+ Hits 41742 43317 +1575
+ Misses 23010 21440 -1570
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
xdustinface
force-pushed
the
fix/dash-mainnet
branch
2 times, most recently
from
99debb8 to
864ab58
Compare
xdustinface
force-pushed
the
chore/fix-network-docs
branch
from
b8a8fc5 to
11c5fdb
Compare
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
added
the
merge-conflict
xdustinface
force-pushed
the
fix/dash-mainnet
branch
from
864ab58 to
720a095
Compare
github-actions
bot
removed
the
merge-conflict
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
key-wallet/src/derivation.rs (2)
137-153:⚠️ Potential issue | 🟠 MajorPropagate invalid identity indexes instead of panicking.
from_hardened_idxrejects values>= 2^31. These twounwrap()calls make a fallible library API panic on bad input even though the method already returnsResult<_>.Suggested fix
// Convert to DerivationPath and append indices let mut full_path = crate::bip32::DerivationPath::from(path); - full_path.push(crate::bip32::ChildNumber::from_hardened_idx(identity_index).unwrap()); - full_path.push(crate::bip32::ChildNumber::from_hardened_idx(key_index).unwrap()); + full_path.push( + crate::bip32::ChildNumber::from_hardened_idx(identity_index) + .map_err(|e| Error::InvalidDerivationPath(e.to_string()))?, + ); + full_path.push( + crate::bip32::ChildNumber::from_hardened_idx(key_index) + .map_err(|e| Error::InvalidDerivationPath(e.to_string()))?, + ); self.derive(&full_path) }As per coding guidelines, "Avoid
unwrap()andexpect()in library code; use proper error types (e.g.,thiserror)" and "Use the?operator for error propagation, provide context in error messages, never panic in library code, and returnResult<T>for all fallible operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/derivation.rs` around lines 137 - 153, The two unwrap() calls in identity_authentication_key cause panics when from_hardened_idx fails for values >= 2^31; replace them with fallible handling by calling from_hardened_idx(...) and using the ? operator (or map_err to a suitable crate::Error variant) to propagate the error instead of panicking, e.g. map the child-number error into a new or existing Error variant (e.g., InvalidChildIndex or similar) so full_path construction returns Err on bad identity_index or key_index rather than panic.
331-350:⚠️ Potential issue | 🟠 MajorDon't silently fall back to mainnet coin type for unknown networks.
The
_ => 5branch derives mainnet paths for any network variant that is not explicitly listed, which can mix networks silently. This helper already returnsResult<_>, so unsupported networks should fail the same way the other derivation helpers do.Suggested fix
let coin_type = match network { Network::Mainnet => 5, Network::Testnet | Network::Devnet | Network::Regtest => 1, - _ => 5, // Default to Dash + _ => return Err(Error::InvalidNetwork), };As per coding guidelines, "Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/derivation.rs` around lines 331 - 350, The match in for_network_and_type currently uses a catch-all `_ => 5` which silently treats unknown Network variants as mainnet; change it so the code returns an error for unsupported networks instead of defaulting to coin_type 5. Update the coin_type match in for_network_and_type to only handle Network::Mainnet and the explicit test variants and make the default arm return an Err (e.g., an error indicating Unsupported network) so the function's Result<DerivationPath> fails for unknown networks; reference the for_network_and_type method and the Network enum when making this change.key-wallet-ffi/src/derivation_tests.rs (1)
587-591:⚠️ Potential issue | 🟡 MinorAdd null checks before converting C string pointers at lines 587–591.
derivation_xpriv_to_stringcan return null on error (seetest_derivation_xpriv_to_string_null_input), and passing null toCStr::from_ptrcauses undefined behavior. The code must check both pointers before conversion, matching the safe pattern used elsewhere in this file.Suggested fix
let main_str = unsafe { derivation_xpriv_to_string(xprv_main, &mut error) }; let test_str = unsafe { derivation_xpriv_to_string(xprv_test, &mut error) }; + assert!(!main_str.is_null()); + assert!(!test_str.is_null()); let main_string = unsafe { CStr::from_ptr(main_str) }.to_str().unwrap(); let test_string = unsafe { CStr::from_ptr(test_str) }.to_str().unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/derivation_tests.rs` around lines 587 - 591, derivation_xpriv_to_string may return null, so before calling CStr::from_ptr on main_str and test_str you must null-check both pointers (main_str and test_str) and handle the error path similarly to other tests in this file: if either is null, read/format the error from the existing error variable and fail the test (or return) rather than calling CStr::from_ptr; only convert to &CStr and call to_str().unwrap() when the pointer is non-null. Ensure you reference the same error reporting logic used elsewhere so the test mirrors the safe pattern.key-wallet/src/account/account_type.rs (1)
259-264:⚠️ Potential issue | 🟠 MajorReject unsupported networks before computing
coin_type.This still coerces any non-
Mainnetinput to coin type1, soStandard,CoinJoin, and provider-key derivations can silently fall back to testnet while the identity/platform branches below now returnInvalidNetwork. Please make this branch explicit so all derivation paths fail consistently on unsupported networks.As per coding guidelines, "Always validate network consistency when deriving or validating addresses; never mix mainnet and testnet operations".🔧 Suggested fix
- let coin_type = if network == Network::Mainnet { - 5 - } else { - 1 - }; + let coin_type = match network { + Network::Mainnet => 5, + Network::Testnet | Network::Devnet | Network::Regtest => 1, + _ => return Err(crate::error::Error::InvalidNetwork), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/account/account_type.rs` around lines 259 - 264, In derivation_path ensure you explicitly validate Network instead of coercing non-Mainnet to coin_type 1; update the coin_type logic in derivation_path(Network) to match on Network (e.g., Network::Mainnet => coin_type = 5, Network::Testnet => coin_type = 1) and return Err(crate::error::Error::InvalidNetwork) for any other/unsupported variants so Standard, CoinJoin and provider-key branches don't silently fall back to testnet; reference derivation_path, Network, coin_type, and crate::error::Error::InvalidNetwork when making the change.
🧹 Nitpick comments (5)
dash-spv/tests/transaction_calculation_test.rs (1)
20-23: Add a testnet variant for these regression cases.After switching these checks to
Network::Mainnet, this file no longer exercises the same address-parsing/require_networkpath on testnet. A small parameterized companion case would keep the regression aligned with the repo rule to cover both networks in test code.As per coding guidelines "Test both mainnet and testnet configurations".
Also applies to: 122-125, 180-183, 213-216, 233-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/transaction_calculation_test.rs` around lines 20 - 23, The test hardcodes Address::from_str(...).require_network(Network::Mainnet) which skips exercising the testnet path; change the failing single-case to a parameterized companion (or loop) that runs the same assertions for both Network::Mainnet and Network::Testnet by deriving watched_address for each network using Address::from_str(...).require_network(network) and reusing the same test logic; apply the same pattern to the other occurrences that were switched to Mainnet (the other Address::from_str(...).require_network(...) sites mentioned) so each regression case runs against both networks.dash-spv/tests/handshake_test.rs (1)
16-17: Add a testnet path to this handshake suite.These updates keep the coverage mainnet-only, so the renamed network path is never exercised against the testnet branch in this network-focused test file. Please add a small
Network::Testnetvariant or parameterize the applicable tests over both networks.As per coding guidelines,
**/*test*.rs: Test both mainnet and testnet configurations.Also applies to: 57-58, 76-77, 89-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/handshake_test.rs` around lines 16 - 17, The tests in handshake_test.rs only exercise Peer::connect with Network::Mainnet; add a Testnet path by parameterizing the tests or duplicating the calls to cover Network::Testnet as well. Update each call site using Peer::connect(peer_addr, 10, Network::Mainnet) (and the other occurrences around the file) to loop over or table-drive both Network::Mainnet and Network::Testnet so the handshake suite runs for both networks, or add equivalent assertions for Network::Testnet; ensure you reference Network::Testnet in the same test functions that currently use Network::Mainnet (including the sites noted at the other ranges) so both variants are executed by the test runner.dash-spv-ffi/src/config.rs (1)
116-122: Source the default port from shared network constants.This FFI path still duplicates per-network P2P ports inline. With the network rename touching these mappings again, it is easy for this table to drift from the canonical network constants over time. Please route this through a shared network→default-port helper or existing network constants instead of hardcoded literals.
As per coding guidelines,
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv-ffi/src/config.rs` around lines 116 - 122, The default_port binding currently hardcodes per-network P2P ports against cfg.network; replace this literal table with a call into the shared network constants/helper (e.g. use a function like network::default_p2p_port(cfg.network) or shared::network::p2p_port_for(cfg.network)) so the value is sourced from the canonical network constants instead of inline numbers; update the code that sets default_port (the let default_port = ... expression) to call that shared helper and remove the hardcoded match arms.dash-spv/src/network/tests.rs (1)
10-17: Cover the testnet constructor path here as well.This rename is network-specific, but
test_peer_creationnow only exercisesNetwork::Mainnet, so a regression in the testnet branch would still pass unnoticed.As per coding guidelines, "`**/*test*.rs`: Test both mainnet and testnet configurations".🧪 Suggested expansion
- let peer = Peer::new(addr, timeout, Network::Mainnet); - - assert!(!peer.is_connected()); - assert_eq!(peer.address(), addr); + for network in [Network::Mainnet, Network::Testnet] { + let peer = Peer::new(addr, timeout, network); + assert!(!peer.is_connected()); + assert_eq!(peer.address(), addr); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/src/network/tests.rs` around lines 10 - 17, The test_peer_creation only constructs a Peer with Network::Mainnet; add a second assertion path that constructs a Peer using Peer::new(addr, timeout, Network::Testnet) and verifies the same properties (not connected and address matches) so both network variants are covered; update the test_peer_creation function to create two peers (one with Network::Mainnet and one with Network::Testnet) and assert is_connected() is false and address() equals addr for each.dash-spv/tests/test_handshake_logic.rs (1)
8-16: Add a testnet case for the handshake constructor path.This file still only exercises
Network::Mainnet, even thoughHandshakeManager::new(...)is network-parameterized. Please add aTestnetvariant here as well, or table-drive the test over both networks.Suggested test shape
#[test] fn test_handshake_state_transitions() { - let mut handshake = HandshakeManager::new(Network::Mainnet, MempoolStrategy::BloomFilter, None); - - // Initial state should be Init - assert_eq!(*handshake.state(), HandshakeState::Init); - - // After reset, should be back to Init - handshake.reset(); - assert_eq!(*handshake.state(), HandshakeState::Init); + for network in [Network::Mainnet, Network::Testnet] { + let mut handshake = + HandshakeManager::new(network, MempoolStrategy::BloomFilter, None); + + assert_eq!(*handshake.state(), HandshakeState::Init); + handshake.reset(); + assert_eq!(*handshake.state(), HandshakeState::Init); + } }As per coding guidelines,
Test both mainnet and testnet configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dash-spv/tests/test_handshake_logic.rs` around lines 8 - 16, The test currently only constructs HandshakeManager with Network::Mainnet; add coverage for Network::Testnet by either duplicating the assertions with a second instance (e.g., let mut handshake = HandshakeManager::new(Network::Testnet, MempoolStrategy::BloomFilter, None)) or convert test_handshake_state_transitions into a table-driven loop that iterates over [Network::Mainnet, Network::Testnet], creating a HandshakeManager for each and asserting initial state is HandshakeState::Init and that reset() returns it to Init; reference the HandshakeManager::new constructor, the Network enum (Testnet), state() and reset() methods when making 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-spv-ffi/FFI_API.md`:
- Around line 244-247: The generated FFI docs are inconsistent: the enum was
renamed but references to FFINetwork and its variants are stale. Regenerate the
API docs from the updated headers/comments (the sources that define
FFIClientConfig and the network enum) so all occurrences of the enum, its
variants, and the Get-network description (which mentions defaulting to
FFINetwork::Mainnet) are updated consistently across the file; search for
symbols FFINetwork and FFIClientConfig in the source headers, run the doc
generator, and verify the produced FFI_API.md contains the updated enum name and
variant list everywhere.
In `@dash-spv-ffi/src/platform_integration.rs`:
- Around line 184-190: The activation heights are hardcoded here in the local
match (variable height) and drift from the authoritative values in
dash/src/sml/llmq_type/network.rs; replace this local match with a call to the
shared helper/constant exported from that module (e.g. a public function like
get_activation_height(network: dashcore::Network) or a public constant
map/lookup such as ACTIVATION_HEIGHTS.get(network)). Import the symbol from
dash::sml::llmq_type::network, remove the duplicate match in
platform_integration.rs, and set height by calling the shared helper so Devnet
and other networks use the single source of truth. Ensure the used helper
returns the same types and semantics (Devnet -> 1 or “active immediately”) as
the core module.
In `@dash-spv/src/network/discovery.rs`:
- Around line 34-36: The match in discovery.rs that sets (seeds, port) currently
hardcodes ports (9999/19999) in the Network::Mainnet/Testnet arms; update it to
fetch the port from the centralized network parameters instead of inlining
numbers—use the shared Network params API (e.g., a port getter on the Network
enum or the global network params constant) when constructing (seeds, port) so
MAINNET_DNS_SEEDS and TESTNET_DNS_SEEDS remain paired with their canonical port
from the shared config rather than hardcoded literals.
In `@dash/examples/handshake.rs`:
- Around line 32-35: The example hardcodes Network::Mainnet.magic() when
constructing RawNetworkMessage (e.g., the first_message and the later message
around line 53); instead, add a network selection variable in main() (e.g., let
network = /* choose Network from args or env */) and thread that variable into
both message constructions by using network.magic() rather than
Network::Mainnet.magic(), ensuring both messages use the same selected Network
and updating any downstream uses that assumed Mainnet.
In `@dash/src/address.rs`:
- Around line 1002-1006: The bech32 HRP for Testnet/Devnet is inconsistent
between fmt_internal() and Address::from_str(): fmt_internal() emits "tb" while
parsing expects "td", breaking round-trips; update the bech32_hrp selection in
the formatting path (the bech32_hrp match that calls self.network()) to use the
same HRP string used by Address::from_str() for Network::Testnet and
Network::Devnet (and mirror the same change where the duplicate match appears
around the other occurrence), ensuring both formatting and parsing use the
identical HRP for each Network variant.
- Around line 1595-1597: The test currently sets key.compressed = false then
asserts Address::p2wpkh(&key, Mainnet) returns Err(Error::UncompressedPubkey)
but needs to exercise the P2SH-wrapped path; change the negative assertion to
call Address::p2shwpkh(&key, Mainnet) instead of Address::p2wpkh so that
Address::p2shwpkh is verified to reject uncompressed keys (keep the same
Err(Error::UncompressedPubkey) expectation and the same modified key/compressed
setup).
In `@key-wallet-ffi/FFI_API.md`:
- Around line 2349-2352: The current FFI getters account_get_network,
managed_core_account_get_network, and managed_platform_account_get_network
silently return FFINetwork::Mainnet when given a null FFIAccount pointer; change
each to explicitly detect a null pointer and fail instead of returning Mainnet
by either (a) returning a sentinel/invalid enum such as FFINetwork::Unknown
(preferred) or (b) returning an error/result via the existing FFI
error-reporting mechanism (or setting a global/last-error) so callers can detect
misuse; update the null-check branches in account.rs and managed_account.rs to
perform this explicit failure path and document the new sentinel/error behavior.
In `@key-wallet-ffi/include/key_wallet_ffi.h`:
- Around line 108-110: Add a deprecated compatibility alias for the renamed enum
constant so existing C/Swift consumers keep compiling: in the typedef enum that
now defines MAINNET and TESTNET, add a transitional DASH alias that maps to the
same numeric value as MAINNET (e.g., DASH = MAINNET or DASH = 0) and annotate it
as deprecated (via a comment and/or compiler attribute like
__attribute__((deprecated)) when supported) so the public symbol name remains
available while signaling users to migrate from DASH to MAINNET.
In `@key-wallet-ffi/src/types.rs`:
- Around line 41-45: Replace the wildcard fallback converting upstream Network
to FFINetwork with a fallible, explicit mapping: implement TryFrom<Network> for
FFINetwork (or change the conversion function currently doing the match) so each
known variant (Network::Mainnet, ::Testnet, ::Regtest, ::Devnet) maps explicitly
and any other (future) variant returns an Err (use a descriptive error type or
crate::Error type); update call sites that relied on the infallible conversion
to handle the Result instead of assuming Mainnet.
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 802-805: The code currently sets an error via FFIError::set_error
when manager is null but returns a real network value FFINetwork::Mainnet;
instead add an explicit sentinel variant (e.g., FFINetwork::Unknown or
FFINetwork::Invalid) to the FFINetwork enum, update any serde/FFI mappings, and
change the null-check to return that sentinel rather than Mainnet; keep the
FFIError::set_error call (with FFIErrorCode::InvalidInput) intact so callers can
detect the error distinct from a valid network.
In `@key-wallet/examples/basic_usage.rs`:
- Around line 103-106: The printed network label is stale: after calling
parsed.clone().require_network(DashNetwork::Mainnet) you print "Network: Dash"
using the checked variable; update that println! to reflect the renamed variant
(e.g., print "Network: Mainnet") so it matches the DashNetwork::Mainnet call and
the "Network: Testnet" message used elsewhere.
In `@key-wallet/tests/bip32_tests.rs`:
- Line 13: Current tests only create a master key with
ExtendedPrivKey::new_master(Network::Mainnet, &seed) and miss Testnet coverage;
add parallel assertions that instantiate
ExtendedPrivKey::new_master(Network::Testnet, &seed) (and any derived variables
named `master`/`xprv`/`xpub` in the same test cases) and assert
serialization/deserialization and expected version bytes for Testnet just like
Mainnet checks so both network-specific behaviors are validated; update all
referenced tests (the occurrences around the master-key creation and the cases
at the other mentioned locations) to run the same assertions for
Network::Testnet.
---
Outside diff comments:
In `@key-wallet-ffi/src/derivation_tests.rs`:
- Around line 587-591: derivation_xpriv_to_string may return null, so before
calling CStr::from_ptr on main_str and test_str you must null-check both
pointers (main_str and test_str) and handle the error path similarly to other
tests in this file: if either is null, read/format the error from the existing
error variable and fail the test (or return) rather than calling CStr::from_ptr;
only convert to &CStr and call to_str().unwrap() when the pointer is non-null.
Ensure you reference the same error reporting logic used elsewhere so the test
mirrors the safe pattern.
In `@key-wallet/src/account/account_type.rs`:
- Around line 259-264: In derivation_path ensure you explicitly validate Network
instead of coercing non-Mainnet to coin_type 1; update the coin_type logic in
derivation_path(Network) to match on Network (e.g., Network::Mainnet =>
coin_type = 5, Network::Testnet => coin_type = 1) and return
Err(crate::error::Error::InvalidNetwork) for any other/unsupported variants so
Standard, CoinJoin and provider-key branches don't silently fall back to
testnet; reference derivation_path, Network, coin_type, and
crate::error::Error::InvalidNetwork when making the change.
In `@key-wallet/src/derivation.rs`:
- Around line 137-153: The two unwrap() calls in identity_authentication_key
cause panics when from_hardened_idx fails for values >= 2^31; replace them with
fallible handling by calling from_hardened_idx(...) and using the ? operator (or
map_err to a suitable crate::Error variant) to propagate the error instead of
panicking, e.g. map the child-number error into a new or existing Error variant
(e.g., InvalidChildIndex or similar) so full_path construction returns Err on
bad identity_index or key_index rather than panic.
- Around line 331-350: The match in for_network_and_type currently uses a
catch-all `_ => 5` which silently treats unknown Network variants as mainnet;
change it so the code returns an error for unsupported networks instead of
defaulting to coin_type 5. Update the coin_type match in for_network_and_type to
only handle Network::Mainnet and the explicit test variants and make the default
arm return an Err (e.g., an error indicating Unsupported network) so the
function's Result<DerivationPath> fails for unknown networks; reference the
for_network_and_type method and the Network enum when making this change.
---
Nitpick comments:
In `@dash-spv-ffi/src/config.rs`:
- Around line 116-122: The default_port binding currently hardcodes per-network
P2P ports against cfg.network; replace this literal table with a call into the
shared network constants/helper (e.g. use a function like
network::default_p2p_port(cfg.network) or
shared::network::p2p_port_for(cfg.network)) so the value is sourced from the
canonical network constants instead of inline numbers; update the code that sets
default_port (the let default_port = ... expression) to call that shared helper
and remove the hardcoded match arms.
In `@dash-spv/src/network/tests.rs`:
- Around line 10-17: The test_peer_creation only constructs a Peer with
Network::Mainnet; add a second assertion path that constructs a Peer using
Peer::new(addr, timeout, Network::Testnet) and verifies the same properties (not
connected and address matches) so both network variants are covered; update the
test_peer_creation function to create two peers (one with Network::Mainnet and
one with Network::Testnet) and assert is_connected() is false and address()
equals addr for each.
In `@dash-spv/tests/handshake_test.rs`:
- Around line 16-17: The tests in handshake_test.rs only exercise Peer::connect
with Network::Mainnet; add a Testnet path by parameterizing the tests or
duplicating the calls to cover Network::Testnet as well. Update each call site
using Peer::connect(peer_addr, 10, Network::Mainnet) (and the other occurrences
around the file) to loop over or table-drive both Network::Mainnet and
Network::Testnet so the handshake suite runs for both networks, or add
equivalent assertions for Network::Testnet; ensure you reference
Network::Testnet in the same test functions that currently use Network::Mainnet
(including the sites noted at the other ranges) so both variants are executed by
the test runner.
In `@dash-spv/tests/test_handshake_logic.rs`:
- Around line 8-16: The test currently only constructs HandshakeManager with
Network::Mainnet; add coverage for Network::Testnet by either duplicating the
assertions with a second instance (e.g., let mut handshake =
HandshakeManager::new(Network::Testnet, MempoolStrategy::BloomFilter, None)) or
convert test_handshake_state_transitions into a table-driven loop that iterates
over [Network::Mainnet, Network::Testnet], creating a HandshakeManager for each
and asserting initial state is HandshakeState::Init and that reset() returns it
to Init; reference the HandshakeManager::new constructor, the Network enum
(Testnet), state() and reset() methods when making the change.
In `@dash-spv/tests/transaction_calculation_test.rs`:
- Around line 20-23: The test hardcodes
Address::from_str(...).require_network(Network::Mainnet) which skips exercising
the testnet path; change the failing single-case to a parameterized companion
(or loop) that runs the same assertions for both Network::Mainnet and
Network::Testnet by deriving watched_address for each network using
Address::from_str(...).require_network(network) and reusing the same test logic;
apply the same pattern to the other occurrences that were switched to Mainnet
(the other Address::from_str(...).require_network(...) sites mentioned) so each
regression case runs against both networks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15b845c3-30eb-46f6-8a60-62cd918f498f
📒 Files selected for processing (76)
dash-spv-ffi/FFI_API.mddash-spv-ffi/dash_spv_ffi.hdash-spv-ffi/include/dash_spv_ffi.hdash-spv-ffi/src/bin/ffi_cli.rsdash-spv-ffi/src/config.rsdash-spv-ffi/src/platform_integration.rsdash-spv-ffi/tests/test_config.rsdash-spv-ffi/tests/test_types.rsdash-spv-ffi/tests/test_utils.rsdash-spv-ffi/tests/unit/test_configuration.rsdash-spv-ffi/tests/unit/test_error_handling.rsdash-spv-ffi/tests/unit/test_type_conversions.rsdash-spv/src/chain/chain_work.rsdash-spv/src/client/config.rsdash-spv/src/client/config_test.rsdash-spv/src/client/lifecycle.rsdash-spv/src/main.rsdash-spv/src/mempool_filter.rsdash-spv/src/network/discovery.rsdash-spv/src/network/tests.rsdash-spv/src/test_utils/network.rsdash-spv/src/validation/header.rsdash-spv/tests/handshake_test.rsdash-spv/tests/peer_test.rsdash-spv/tests/test_handshake_logic.rsdash-spv/tests/transaction_calculation_test.rsdash/embedded/src/main.rsdash/examples/handshake.rsdash/src/address.rsdash/src/bip143.rsdash/src/blockdata/constants.rsdash/src/blockdata/transaction/mod.rsdash/src/blockdata/transaction/outpoint.rsdash/src/consensus/params.rsdash/src/crypto/key.rsdash/src/crypto/sighash.rsdash/src/network/constants.rsdash/src/network/message.rsdash/src/sign_message.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/taproot.rsfuzz/fuzz_targets/dash/deserialize_script.rskey-wallet-ffi/FFI_API.mdkey-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/account.rskey-wallet-ffi/src/account_tests.rskey-wallet-ffi/src/derivation_tests.rskey-wallet-ffi/src/lib_tests.rskey-wallet-ffi/src/managed_account.rskey-wallet-ffi/src/types.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/tests/debug_addr.rskey-wallet-ffi/tests/integration_test.rskey-wallet/CLAUDE.mdkey-wallet/README.mdkey-wallet/examples/basic_usage.rskey-wallet/src/account/account_type.rskey-wallet/src/bip32.rskey-wallet/src/bip38.rskey-wallet/src/bip38_tests.rskey-wallet/src/derivation.rskey-wallet/src/derivation_slip10.rskey-wallet/src/managed_account/address_pool.rskey-wallet/src/psbt/mod.rskey-wallet/src/tests/account_tests.rskey-wallet/src/tests/backup_restore_tests.rskey-wallet/src/tests/edge_case_tests.rskey-wallet/src/tests/integration_tests.rskey-wallet/src/wallet/mod.rskey-wallet/tests/address_tests.rskey-wallet/tests/bip32_tests.rskey-wallet/tests/derivation_tests.rskey-wallet/tests/mnemonic_tests.rs
Comment on lines
+244
to
+247
| Gets the network type from the configuration # Safety - `config` must be a valid pointer to an FFIClientConfig or null - If null, returns FFINetwork::Mainnet as default | ||
|
|
||
| **Safety:** | ||
| - `config` must be a valid pointer to an FFIClientConfig or null - If null, returns FFINetwork::Dash as default | ||
| - `config` must be a valid pointer to an FFIClientConfig or null - If null, returns FFINetwork::Mainnet as default |
Contributor
There was a problem hiding this comment.
Regenerate the API docs; the enum rename is still stale later in the file.
Lines 244-247 now say a null config defaults to FFINetwork::Mainnet, but the type summary at Line 924 still describes FFINetwork as (Dash, Testnet, Regtest, Devnet). Please rerun the doc generator from the updated headers/comments so the published FFI reference stays self-consistent.
Also applies to: 924-924
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv-ffi/FFI_API.md` around lines 244 - 247, The generated FFI docs are
inconsistent: the enum was renamed but references to FFINetwork and its variants
are stale. Regenerate the API docs from the updated headers/comments (the
sources that define FFIClientConfig and the network enum) so all occurrences of
the enum, its variants, and the Get-network description (which mentions
defaulting to FFINetwork::Mainnet) are updated consistently across the file;
search for symbols FFINetwork and FFIClientConfig in the source headers, run the
doc generator, and verify the produced FFI_API.md contains the updated enum name
and variant list everywhere.
Comment on lines
184
to
190
| // Platform activation heights per network | ||
| let height = match network { | ||
| dashcore::Network::Dash => 1_888_888, // Mainnet (placeholder - needs verification) | ||
| dashcore::Network::Mainnet => 1_888_888, // Mainnet (placeholder - needs verification) | ||
| dashcore::Network::Testnet => 1_289_520, // Testnet confirmed height | ||
| dashcore::Network::Devnet => 1, // Devnet starts immediately | ||
| _ => 0, // Unknown network | ||
| dashcore::Network::Devnet => 1, // Devnet starts immediately | ||
| _ => 0, // Unknown network | ||
| }; |
Contributor
There was a problem hiding this comment.
Centralize platform activation heights instead of duplicating literals.
Line 186-Line 189 reintroduce network thresholds here, and they already drift from the core logic in dash/src/sml/llmq_type/network.rs (Devnet is 1 here but treated as active immediately there). That can make the FFI report a different activation height than the Rust sync code is actually using.
Please source this from one shared constant/helper and return that value here, rather than maintaining a second match table.
As per coding guidelines, **/*.rs: Never hardcode network parameters, addresses, or keys in Rust code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv-ffi/src/platform_integration.rs` around lines 184 - 190, The
activation heights are hardcoded here in the local match (variable height) and
drift from the authoritative values in dash/src/sml/llmq_type/network.rs;
replace this local match with a call to the shared helper/constant exported from
that module (e.g. a public function like get_activation_height(network:
dashcore::Network) or a public constant map/lookup such as
ACTIVATION_HEIGHTS.get(network)). Import the symbol from
dash::sml::llmq_type::network, remove the duplicate match in
platform_integration.rs, and set height by calling the shared helper so Devnet
and other networks use the single source of truth. Ensure the used helper
returns the same types and semantics (Devnet -> 1 or “active immediately”) as
the core module.
Comment on lines
34
to
36
| let (seeds, port) = match network { | ||
| Network::Dash => (MAINNET_DNS_SEEDS, 9999), | ||
| Network::Mainnet => (MAINNET_DNS_SEEDS, 9999), | ||
| Network::Testnet => (TESTNET_DNS_SEEDS, 19999), |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Stop inlining the P2P ports in discovery.
This rename touches production network-selection code, and the match arm still hardcodes 9999/19999. Please source the port from shared network constants or network params so seed selection and port metadata stay centralized.
As per coding guidelines, Never hardcode network parameters, addresses, or keys in Rust code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash-spv/src/network/discovery.rs` around lines 34 - 36, The match in
discovery.rs that sets (seeds, port) currently hardcodes ports (9999/19999) in
the Network::Mainnet/Testnet arms; update it to fetch the port from the
centralized network parameters instead of inlining numbers—use the shared
Network params API (e.g., a port getter on the Network enum or the global
network params constant) when constructing (seeds, port) so MAINNET_DNS_SEEDS
and TESTNET_DNS_SEEDS remain paired with their canonical port from the shared
config rather than hardcoded literals.
Comment on lines
32
to
35
| let first_message = message::RawNetworkMessage { | ||
| magic: Network::Dash.magic(), | ||
| magic: Network::Mainnet.magic(), | ||
| payload: version_message, | ||
| }; |
Contributor
There was a problem hiding this comment.
Don't pin the handshake example to mainnet magic.
Line 33 and Line 53 hardcode mainnet framing, so this example will fail against testnet/regtest peers even though the input is just host:port. Please flow a selected Network into both messages instead of baking in Network::Mainnet.
🔧 Minimal direction
- magic: Network::Mainnet.magic(),
+ magic: network.magic(),
...
- magic: Network::Mainnet.magic(),
+ magic: network.magic(),Add network selection earlier in main() and thread it into the message construction path.
As per coding guidelines, **/*.rs: Never hardcode network parameters, addresses, or keys in Rust code.
Also applies to: 52-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/examples/handshake.rs` around lines 32 - 35, The example hardcodes
Network::Mainnet.magic() when constructing RawNetworkMessage (e.g., the
first_message and the later message around line 53); instead, add a network
selection variable in main() (e.g., let network = /* choose Network from args or
env */) and thread that variable into both message constructions by using
network.magic() rather than Network::Mainnet.magic(), ensuring both messages use
the same selected Network and updating any downstream uses that assumed Mainnet.
Comment on lines
1002
to
1006
| let bech32_hrp = match self.network() { | ||
| Network::Dash => "ds", | ||
| Network::Mainnet => "ds", | ||
| Network::Testnet | Network::Devnet => "tb", | ||
| Network::Regtest => "dsrt", | ||
| }; |
Contributor
There was a problem hiding this comment.
Use the same Testnet/Devnet Bech32 HRP for formatting and parsing.
fmt_internal() emits tb1..., but Address::from_str() only recognizes td1.... That means generated testnet/devnet segwit addresses will not round-trip through this library.
🔧 Suggested fix
let bech32_hrp = match self.network() {
Network::Mainnet => "ds",
- Network::Testnet | Network::Devnet => "tb",
+ Network::Testnet | Network::Devnet => "td",
Network::Regtest => "dsrt",
};Also applies to: 1336-1340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dash/src/address.rs` around lines 1002 - 1006, The bech32 HRP for
Testnet/Devnet is inconsistent between fmt_internal() and Address::from_str():
fmt_internal() emits "tb" while parsing expects "td", breaking round-trips;
update the bech32_hrp selection in the formatting path (the bech32_hrp match
that calls self.network()) to use the same HRP string used by
Address::from_str() for Network::Testnet and Network::Devnet (and mirror the
same change where the duplicate match appears around the other occurrence),
ensuring both formatting and parsing use the identical HRP for each Network
variant.
Comment on lines
108
to
110
| typedef enum { | ||
| DASH = 0, | ||
| MAINNET = 0, | ||
| TESTNET = 1, |
Contributor
There was a problem hiding this comment.
Keep a transitional DASH alias in the public C API.
Renaming the exported enum constant to MAINNET breaks existing C/Swift consumers at compile time even though the numeric value is unchanged. Please add a deprecated compatibility alias in the generated header (or document the break very explicitly in release notes) before merging this public API rename.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/include/key_wallet_ffi.h` around lines 108 - 110, Add a
deprecated compatibility alias for the renamed enum constant so existing C/Swift
consumers keep compiling: in the typedef enum that now defines MAINNET and
TESTNET, add a transitional DASH alias that maps to the same numeric value as
MAINNET (e.g., DASH = MAINNET or DASH = 0) and annotate it as deprecated (via a
comment and/or compiler attribute like __attribute__((deprecated)) when
supported) so the public symbol name remains available while signaling users to
migrate from DASH to MAINNET.
Comment on lines
+41
to
+45
| Network::Mainnet => FFINetwork::Mainnet, | ||
| Network::Testnet => FFINetwork::Testnet, | ||
| Network::Regtest => FFINetwork::Regtest, | ||
| Network::Devnet => FFINetwork::Devnet, | ||
| _ => FFINetwork::Dash, | ||
| _ => FFINetwork::Mainnet, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect the upstream Network enum shape:"
rg -nC3 --glob '*.rs' 'pub enum Network|enum Network|non_exhaustive'
echo
echo "Inspect the FFI conversion implementation:"
rg -nC4 --glob '*.rs' 'impl From<Network> for FFINetwork|_ => FFINetwork::Mainnet'Repository: dashpay/rust-dashcore
Length of output: 20006
Replace the wildcard fallback with a fallible conversion or exhaustive handling.
The upstream Network enum is marked #[non_exhaustive], allowing future variants to be added without breaking the library API. The wildcard arm _ => FFINetwork::Mainnet silently coerces any unknown/future variant to Mainnet, exposing wrong network semantics to FFI callers. Either convert this to a fallible type (e.g., TryFrom) that returns an error for unrecognized variants, or handle each known variant explicitly without a catch-all.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/src/types.rs` around lines 41 - 45, Replace the wildcard
fallback converting upstream Network to FFINetwork with a fallible, explicit
mapping: implement TryFrom<Network> for FFINetwork (or change the conversion
function currently doing the match) so each known variant (Network::Mainnet,
::Testnet, ::Regtest, ::Devnet) maps explicitly and any other (future) variant
returns an Err (use a descriptive error type or crate::Error type); update call
sites that relied on the infallible conversion to handle the Result instead of
assuming Mainnet.
Comment on lines
802
to
805
| if manager.is_null() { | ||
| FFIError::set_error(error, FFIErrorCode::InvalidInput, "Manager is null".to_string()); | ||
| return FFINetwork::Dash; // Default fallback | ||
| return FFINetwork::Mainnet; // Default fallback | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid returning a valid network on error.
When manager is null, this reports an error but still returns FFINetwork::Mainnet. FFI callers that pass error = NULL or forget to inspect it will silently treat the failure as a real mainnet manager, which is a risky default for wallet/network routing.
Possible direction
if manager.is_null() {
FFIError::set_error(error, FFIErrorCode::InvalidInput, "Manager is null".to_string());
- return FFINetwork::Mainnet; // Default fallback
+ return FFINetwork::Invalid;
}This needs a matching Invalid/Unknown sentinel in FFINetwork so callers can distinguish an error from a real network.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet-ffi/src/wallet_manager.rs` around lines 802 - 805, The code
currently sets an error via FFIError::set_error when manager is null but returns
a real network value FFINetwork::Mainnet; instead add an explicit sentinel
variant (e.g., FFINetwork::Unknown or FFINetwork::Invalid) to the FFINetwork
enum, update any serde/FFI mappings, and change the null-check to return that
sentinel rather than Mainnet; keep the FFIError::set_error call (with
FFIErrorCode::InvalidInput) intact so callers can detect the error distinct from
a valid network.
Comment on lines
+103
to
106
| if let Ok(checked) = parsed.clone().require_network(DashNetwork::Mainnet) { | ||
| println!(" Parsed address: {}", checked); | ||
| println!(" Type: {:?}", checked.address_type()); | ||
| println!(" Network: Dash"); |
Contributor
There was a problem hiding this comment.
Update output string to match renamed variant.
Line 103 correctly uses DashNetwork::Mainnet, but line 106 still prints "Network: Dash". For consistency with the rename and with line 110 (which prints "Network: Testnet"), consider updating the output.
Proposed fix
if let Ok(checked) = parsed.clone().require_network(DashNetwork::Mainnet) {
println!(" Parsed address: {}", checked);
println!(" Type: {:?}", checked.address_type());
- println!(" Network: Dash");
+ println!(" Network: Mainnet");
} else if let Ok(checked) = parsed.require_network(DashNetwork::Testnet) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Ok(checked) = parsed.clone().require_network(DashNetwork::Mainnet) { | |
| println!(" Parsed address: {}", checked); | |
| println!(" Type: {:?}", checked.address_type()); | |
| println!(" Network: Dash"); | |
| if let Ok(checked) = parsed.clone().require_network(DashNetwork::Mainnet) { | |
| println!(" Parsed address: {}", checked); | |
| println!(" Type: {:?}", checked.address_type()); | |
| println!(" Network: Mainnet"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/examples/basic_usage.rs` around lines 103 - 106, The printed
network label is stale: after calling
parsed.clone().require_network(DashNetwork::Mainnet) you print "Network: Dash"
using the checked variable; update that println! to reflect the renamed variant
(e.g., print "Network: Mainnet") so it matches the DashNetwork::Mainnet call and
the "Network: Testnet" message used elsewhere.
| // Test vector from BIP32 | ||
| let seed = hex::decode("000102030405060708090a0b0c0d0e0f").unwrap(); | ||
| let master = ExtendedPrivKey::new_master(Network::Dash, &seed).unwrap(); | ||
| let master = ExtendedPrivKey::new_master(Network::Mainnet, &seed).unwrap(); |
Contributor
There was a problem hiding this comment.
Add Network::Testnet coverage alongside these renamed mainnet cases.
These tests now validate only Network::Mainnet, even though this PR changes network-specific behavior. Please add parallel Network::Testnet assertions as well, especially for master-key serialization/deserialization where network version bytes differ.
As per coding guidelines, **/*test*.rs: Test both mainnet and testnet configurations.
Also applies to: 42-42, 60-60, 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/tests/bip32_tests.rs` at line 13, Current tests only create a
master key with ExtendedPrivKey::new_master(Network::Mainnet, &seed) and miss
Testnet coverage; add parallel assertions that instantiate
ExtendedPrivKey::new_master(Network::Testnet, &seed) (and any derived variables
named `master`/`xprv`/`xpub` in the same test cases) and assert
serialization/deserialization and expected version bytes for Testnet just like
Mainnet checks so both network-specific behaviors are validated; update all
referenced tests (the occurrences around the master-key creation and the cases
at the other mentioned locations) to run the same assertions for
Network::Testnet.
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
1 participant
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.
Align the network names with Dash Core. Finally! This bugged me since i started to work on this repo haha.
Based on:
Networkentries #499Summary by CodeRabbit
Release Notes
Refactor
Tests