refactor: split key wallet #74
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update introduces the new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FFI (key-wallet-ffi)
participant KeyWallet
participant Dash
User->>FFI (key-wallet-ffi): Call (e.g., create_mnemonic, derive_address)
FFI (key-wallet-ffi)->>KeyWallet: Invoke Rust key-wallet API
KeyWallet-->>FFI (key-wallet-ffi): Return result or error
FFI (key-wallet-ffi)-->>User: Return value in target language
User->>Dash: Use Dash API (e.g., derive key)
Dash->>KeyWallet: Call re-exported bip32/dip9/address functions
KeyWallet-->>Dash: Return key/address
Dash-->>User: Return result
Possibly related PRs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (2)
dash/src/psbt/mod.rs (1)
560-569: Same conversion duplication hereApply the
From<key_wallet::Network> for Networkrefactor above to keep these two implementations in sync and avoid future drift.key-wallet/src/mnemonic.rs (1)
122-124: Validation shares the same language-mapping pitfall
validate()relies on the brokenFrom<Language>implementation and will wrongly accept/deny non-English phrases.Fixing the mapping above will resolve this as well.
🧹 Nitpick comments (23)
key-wallet/summary.md (1)
1-2: Heading ends with a colon – drop it to satisfy markdownlint and conventional style
TheMD026hint is correct: headings should not trail punctuation. Simply remove the colon; the prose that follows can explain the bullet-list or sentence.-# Summary of changes made to make bip32.rs work as a standalone crate: +# Summary of changes made to make `bip32.rs` work as a standalone crate🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
dash/src/psbt/map/global.rs (1)
58-62: Preferextend_from_slice(&fingerprint.to_bytes())for clarity/alloc hygiene
Vec<u8>::extendover an array consumes an iterator, creating an intermediate. A slice avoid that micro-overhead and is clearer.- ret.extend(fingerprint.to_bytes()); + ret.extend_from_slice(&fingerprint.to_bytes());key-wallet/src/test_macros.rs (1)
6-10: Drop the redundantuse serde_json;inside the macro.The item import is superfluous because the crate is already accessible with the absolute path. Removing it trims one statement from every macro expansion.
- use serde_json; - - let encoded = serde_json::to_value(&$var).unwrap(); + let encoded = serde_json::to_value(&$var).unwrap();key-wallet/README.md (1)
18-38: Docs compile-fail as shown.The snippet uses
?inmain-style code without returning aResult, socargo test --docwill fail. Wrap it infn main() -> anyhow::Result<()> { … }or add# fn main() -> Result<(), Box<dyn std::error::Error>> { … # Ok(()) # }guards to keep doctests green.dash/src/psbt/mod.rs (1)
618-619: Typo in error message – “bip23” instead of “bip32”- Bip32(ref e) => write_err!(f, "a bip23 error"; e), + Bip32(ref e) => write_err!(f, "a bip32 error"; e),Minor, but leaks into user-visible
Displayoutput.
Fix before publishing a release.key-wallet-ffi/Cargo.toml (1)
18-24: Duplicateuniffidependency declarations inflate build time
uniffiis declared three times (dependencies,build-dependencies,dev-dependencies).
If the versions/features ever diverge the crate will compile UniFFI thrice.[workspace.dependencies] uniffi = { version = "0.27", features = ["cli", "build", "bindgen-tests"] }Then in this manifest just add
uniffi = { workspace = true }to the three dependency sections.key-wallet/tests/address_tests.rs (2)
24-27: Prefix-based assertions are fragile
assert!(addr_str.starts_with('X'));hard-codes the main-net prefix character.
If Dash ever changes its Base58 version byte (unlikely but possible in forks/devnets) the test fails for reasons unrelated to address logic.
Compare the actual version byte instead:let version = base58ck::decode_check(&addr_str).unwrap()[0]; assert_eq!(version, Network::Dash.p2pkh_version());
88-93: Magic indices make the script-shape test brittleUsing fixed indices (
[23],[24]) hides intent.
Recommend explicit slice pattern matching:assert_eq!( &script_pubkey[..], &[ 0x76, 0xa9, 0x14, // DUP HASH160 push20 &address.hash[..], // 20-byte hash 0x88, 0xac // EQUALVERIFY CHECKSIG ] .concat() );Improves readability and prevents off-by-one errors if the script format changes.
key-wallet/tests/mnemonic_tests.rs (2)
3-5: Remove unused import to silence warnings
ExtendedPrivKeyis imported but never used in this module – this will trigger a compiler warning (or a build failure if#![deny(warnings)]is enabled).-use key_wallet::{ExtendedPrivKey, Network}; +use key_wallet::Network;
54-64:#[ignore]hides the most important happy-path testThe generation path is disabled to avoid the
getrandomrequirement at test-time.
Consider:
- Moving this test behind a separate cargo feature, e.g.
use-entropy, so CI can still run it.- Or replace the entropy source with a deterministic stub (
rand_chacha::ChaCha20Rng::seed_from_u64(0)).This keeps coverage while remaining deterministic.
key-wallet/tests/bip32_tests.rs (1)
26-38: Add negative test for 256-bit hex indices
ChildNumber::from_strhas rather complex hex-parsing logic.
Including an invalid 256-bit component such asm/0xFF/1would exercise this branch and catch regressions.key-wallet/examples/basic_usage.rs (1)
14-19: Hard-coding the well-known BIP-39 test vector in examples invites copy-paste into productionDevelopers occasionally re-use example code verbatim.
Consider adding a conspicuous comment or generating a random mnemonic instead to avoid accidental fund loss.key-wallet/src/error.rs (1)
24-27:Base58variant discards the underlying causeReturning just
Base58loses error context (checksum mismatch vs. invalid char, etc.).
Expose the originalbase58::Error(or at least itsDisplay) to aid debugging.-Base58, +Base58(String),and convert via
Error::Base58(e.to_string()).key-wallet/Cargo.toml (2)
11-18:getrandompulled in unconditionally by thestdfeatureBecause
stdis part ofdefault, thegetrandomcrate is always compiled.
If the intention was to keep entropy generation optional (for deterministic builds or WASM), consider movinggetrandombehind its own feature and keepstdfocused on allocation/std-io only.
1-9: Specify MSRV withrust-versionAdd an explicit
rust-versionkey (e.g.rust-version = "1.74").
This lets cargo refuse to compile on older compilers and documents the MSRV policy.key-wallet/src/derivation.rs (1)
26-41: Method call relies on name collision – use UFCS for clarityInside
impl KeyDerivation for ExtendedPrivKey, callingself.derive_privdepends on inherent-vs-trait resolution rules.
Use fully-qualified syntax to make intent explicit and future-proof:bitcoin::bip32::ExtendedPrivKey::derive_priv(self, secp, path)(or import the type alias).
key-wallet-ffi/src/lib.rs (1)
16-18: Consider documenting the initialization function's purposeThe
initialize()function is empty. If no global initialization is needed, consider adding a comment explaining this, or remove the function if it's not required by the FFI interface.-pub fn initialize() { - // Any global initialization if needed -} +/// Initialize the key-wallet FFI library. +/// Currently no initialization is required, but this function is kept for future compatibility. +pub fn initialize() { + // No initialization required at this time +}key-wallet-ffi/README.md (6)
1-4: Refine README title to match crate naming convention.The title currently uses “Key Wallet FFI”—consider using the exact crate name
key-wallet-ffiin backticks for clarity and consistency with other Rust README files (e.g.,# key-wallet-ffi FFI Bindings).
30-39: Clean up trailing whitespace and align code block comments.The Bash snippets under “Generate bindings” have inconsistent trailing spaces (e.g., line 35). Removing extra spaces and ensuring uniform indentation improves Markdown rendering and avoids lint warnings.
56-74: Add a Ruby usage example to match supported languages.You list Ruby under “Supported Languages,” but there’s no Ruby snippet. Adding a minimal Ruby example showing mnemonic creation and wallet usage will align documentation with the feature list.
95-111: Include Python error-handling example.The Python snippet demonstrates basic calls, but callers need to catch exceptions from FFI (e.g.,
InvalidMnemonic,InvalidDerivationPath). Consider showing atry/exceptblock to handle these error variants.
115-122: Ensure proper Markdown list formatting under “Core Types.”The bullets under Core Types lack a blank line after the heading, which can trigger Markdown lint warnings. Add an empty line between the heading and the list for better readability:
-### Core Types -- `Mnemonic`: BIP39 mnemonic phrase handling +### Core Types + +- `Mnemonic`: BIP39 mnemonic phrase handling🧰 Tools
🪛 LanguageTool
[uncategorized] ~117-~117: Loose punctuation mark.
Context: ... Reference ### Core Types -Mnemonic: BIP39 mnemonic phrase handling - `HDWal...(UNLIKELY_OPENING_PUNCTUATION)
123-128: Apply consistent list formatting in the “Enums” section.Similarly, insert a blank line after Enums before listing the enum items to adhere to Markdown best practices and eliminate loose punctuation warnings:
-### Enums - - `Network`: Dash, Testnet, Regtest, Devnet +### Enums + +- `Network`: Dash, Testnet, Regtest, Devnet🧰 Tools
🪛 LanguageTool
[uncategorized] ~125-~125: Loose punctuation mark.
Context: ...dress generation ### Enums -Network: Dash, Testnet, Regtest, Devnet - `Langu...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
Cargo.toml(1 hunks)dash/Cargo.toml(4 hunks)dash/examples/bip32.rs(0 hunks)dash/src/lib.rs(1 hunks)dash/src/psbt/map/global.rs(1 hunks)dash/src/psbt/mod.rs(5 hunks)dash/tests/psbt.rs(2 hunks)key-wallet-ffi/Cargo.toml(1 hunks)key-wallet-ffi/README.md(1 hunks)key-wallet-ffi/build.rs(1 hunks)key-wallet-ffi/src/key_wallet.udl(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/tests/ffi_tests.rs(1 hunks)key-wallet/Cargo.toml(1 hunks)key-wallet/README.md(1 hunks)key-wallet/examples/basic_usage.rs(1 hunks)key-wallet/src/address.rs(1 hunks)key-wallet/src/bip32.rs(19 hunks)key-wallet/src/derivation.rs(1 hunks)key-wallet/src/dip9.rs(1 hunks)key-wallet/src/error.rs(1 hunks)key-wallet/src/lib.rs(1 hunks)key-wallet/src/mnemonic.rs(1 hunks)key-wallet/src/test_macros.rs(1 hunks)key-wallet/summary.md(1 hunks)key-wallet/tests/address_tests.rs(1 hunks)key-wallet/tests/bip32_tests.rs(1 hunks)key-wallet/tests/derivation_tests.rs(1 hunks)key-wallet/tests/mnemonic_tests.rs(1 hunks)
💤 Files with no reviewable changes (1)
- dash/examples/bip32.rs
🧰 Additional context used
🧬 Code Graph Analysis (8)
dash/src/psbt/map/global.rs (1)
key-wallet/src/bip32.rs (2)
fingerprint(1596-1598)fingerprint(1884-1886)
dash/tests/psbt.rs (1)
key-wallet/src/bip32.rs (1)
new_master(1334-1347)
key-wallet/tests/address_tests.rs (4)
key-wallet/src/address.rs (7)
new(167-171)p2pkh(69-76)p2sh(79-85)test_address_parsing(227-234)from_str(102-127)to_string(88-99)script_pubkey(130-151)key-wallet/src/derivation.rs (3)
new(52-57)new(145-150)from_seed(60-63)key-wallet/src/bip32.rs (15)
from_str(281-315)from_str(740-796)from_str(1098-1107)from_str(1898-1901)from_str(1913-1916)len(1141-1143)from(78-80)from(245-247)from(658-668)from(672-687)from(1055-1057)from(1061-1063)from(1067-1069)from(1321-1323)from_priv(1612-1624)key-wallet/src/error.rs (2)
from(59-61)from(65-67)
key-wallet/tests/derivation_tests.rs (6)
key-wallet-ffi/src/lib.rs (4)
from_seed(251-257)from_phrase(146-152)new(138-144)new(369-373)key-wallet/src/derivation.rs (5)
from_seed(60-63)master_key(66-68)coinjoin_account(102-114)new(52-57)new(145-150)key-wallet/src/mnemonic.rs (2)
from_phrase(85-92)from_str(130-133)key-wallet/src/address.rs (2)
from_str(102-127)new(167-171)key-wallet/src/bip32.rs (5)
from_str(281-315)from_str(740-796)from_str(1098-1107)from_str(1898-1901)from_str(1913-1916)key-wallet/tests/bip32_tests.rs (1)
test_public_key_derivation(57-71)
key-wallet/tests/mnemonic_tests.rs (5)
key-wallet/src/mnemonic.rs (6)
validate(122-124)phrase(95-97)from_phrase(85-92)word_count(100-102)generate(44-67)generate(71-82)key-wallet-ffi/src/lib.rs (1)
from_phrase(146-152)key-wallet/src/bip32.rs (1)
len(1141-1143)dash/src/address.rs (1)
network(834-836)dash/src/taproot.rs (1)
depth(1052-1055)
key-wallet/examples/basic_usage.rs (6)
key-wallet/src/mnemonic.rs (5)
from_phrase(85-92)phrase(95-97)word_count(100-102)from_str(130-133)from(27-33)key-wallet/src/bip32.rs (17)
encode(1430-1436)encode(1716-1722)from_str(281-315)from_str(740-796)from_str(1098-1107)from_str(1898-1901)from_str(1913-1916)from(78-80)from(245-247)from(658-668)from(672-687)from(1055-1057)from(1061-1063)from(1067-1069)from(1321-1323)from_hardened_idx(525-533)len(1141-1143)key-wallet/src/derivation.rs (4)
from_seed(60-63)new(52-57)new(145-150)coinjoin_account(102-114)key-wallet/src/address.rs (3)
new(167-171)p2pkh(69-76)from_str(102-127)key-wallet/src/error.rs (2)
from(59-61)from(65-67)key-wallet/src/dip9.rs (1)
from(64-66)
key-wallet/tests/bip32_tests.rs (3)
key-wallet/src/bip32.rs (11)
master(1156-1158)new_master(1334-1347)child(1167-1171)from_hardened_idx(525-533)from_str(281-315)from_str(740-796)from_str(1098-1107)from_str(1898-1901)from_str(1913-1916)from_priv(1612-1624)from_normal_idx(511-519)key-wallet/src/mnemonic.rs (1)
from_str(130-133)key-wallet/tests/derivation_tests.rs (1)
test_public_key_derivation(96-109)
key-wallet/src/bip32.rs (5)
key-wallet-ffi/src/lib.rs (7)
derive(276-283)from(30-37)from(53-64)from(74-79)from(108-129)to_string(234-242)to_string(341-343)key-wallet/src/derivation.rs (3)
derive(76-78)format(154-155)format(162-163)key-wallet/src/error.rs (3)
from(59-61)from(65-67)fmt(33-44)key-wallet/src/address.rs (4)
fmt(155-157)from_str(102-127)format(196-197)to_string(88-99)dash/src/base58.rs (2)
encode_check(388-391)decode_check(308-328)
🪛 markdownlint-cli2 (0.17.2)
key-wallet/summary.md
1-1: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
🪛 LanguageTool
key-wallet-ffi/README.md
[uncategorized] ~117-~117: Loose punctuation mark.
Context: ... Reference ### Core Types - Mnemonic: BIP39 mnemonic phrase handling - `HDWal...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~125-~125: Loose punctuation mark.
Context: ...dress generation ### Enums - Network: Dash, Testnet, Regtest, Devnet - `Langu...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_witness)
🔇 Additional comments (12)
Cargo.toml (1)
1-3: Workspace addition looks correct – verifycargo xtask/ CI scripts pick it up
Nothing blocking, but double-check any custom tooling that assumes a fixed member list.key-wallet/src/dip9.rs (1)
25-27: Import path update LGTM
Networklives underaddress, and this avoids cyclic deps. No further action.dash/Cargo.toml (1)
18-19: Edition2024is not yet stabilised.Until the Rust 2024 edition is released on stable, crates published with
edition = "2024"will fail to compile for the vast majority of users. Consider downgrading to2021(or gating the value on a nightly toolchain).-edition = "2024" +edition = "2021"dash/tests/psbt.rs (1)
134-135: Seed size sanity-check.
ExtendedPrivKey::new_masterfollows BIP-32, which assumes a seed (normally 64 bytes from BIP-39), not a raw secret key. Passingsecret_bytes()(32 bytes) happens to work for the specific test vector, but other vectors may diverge. Ensure this is intentional and documented, or use a full 64-byte seed for generality.key-wallet/examples/basic_usage.rs (1)
88-97: Inconsistent path andis_changeflag
is_change = trueimplies change chain, yet the derivation path supplied (44'/5'/0') points to the account node, not the external/change branch (…/0or…/1).
Either:
- Append
/1before feeding the path togenerate_range, or- Pass
falsefor receive addresses.Clarifying this keeps the example aligned with BIP-44 semantics.
key-wallet/src/lib.rs (1)
14-17: Missingtest_macros.rswill breakcargo test
#[cfg(test)] #[macro_use] mod test_macros;expects a siblingtest_macros.rs.
If that file is not committed, tests won’t compile.key-wallet-ffi/src/lib.rs (1)
107-130: LGTM! Comprehensive error mapping with forward compatibilityThe error conversion implementation properly maps all specific error types and includes a catch-all pattern for future error variants.
key-wallet-ffi/src/key_wallet.udl (1)
1-158: Well-structured UniFFI interface definitionThe UDL file properly defines all interfaces, enums, and error types matching the Rust implementation. The use of named constructors and error annotations is correct.
key-wallet/src/bip32.rs (3)
707-735: LGTM! Clear formatting for 256-bit child numbersThe Display implementation correctly formats 256-bit child numbers with the
0xprefix for clarity.
1150-1154: LGTM! Useful method for building derivation pathsThe
pushmethod is a valuable addition that enables incremental construction of derivation paths.
1083-1083: Good improvement for no_std compatibilityUsing
core::slice::Iterinstead ofstd::slice::Iterimproves compatibility with no_std environments.key-wallet-ffi/README.md (1)
43-52:❓ Verification inconclusive
Verify Android build command correctness and platform support.
The
cargo ndkinvocation might require flags like--platformor a different syntax depending on thecargo-ndkversion. Also consider documenting Linux/Windows build steps if users target those OSes.
🏁 Script executed:
#!/bin/bash # Skip cloning: only checking installed cargo-ndk syntax cargo ndk --helpLength of output: 265
Verify Android build command syntax and expand OS build instructions
Please confirm that the
cargo ndkinvocation in your README matches the syntax required by the installedcargo-ndkversion (e.g., you may need a--platform <API_LEVEL>flag or updated subcommands). Additionally, consider adding brief build instructions for Linux and Windows targets to cover all supported platforms.• Ensure the Android build line works as intended:
cargo ndk -t arm64-v8a -t armeabi-v7a -t x86_64 -t x86 --platform <API_LEVEL> -o ./jniLibs build --release• Add examples for:
- Linux:
cargo build --release --target x86_64-unknown-linux-gnu- Windows:
cargo build --release --target x86_64-pc-windows-msvc
| let derived_priv = PrivateKey { | ||
| compressed: true, | ||
| network: match ext_derived.network { | ||
| key_wallet::Network::Dash => Network::Dash, | ||
| key_wallet::Network::Testnet => Network::Testnet, | ||
| key_wallet::Network::Regtest => Network::Regtest, | ||
| key_wallet::Network::Devnet => Network::Devnet, | ||
| }, | ||
| inner: ext_derived.private_key, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hand-rolled network mapping – implement From once.
Pattern-matching every time a conversion is needed is brittle and error-prone. Add a simple From impl (or TryFrom if ever fallible) inside dashcore and use ext_derived.network.into() here.
-let derived_priv = PrivateKey {
- compressed: true,
- network: match ext_derived.network {
- key_wallet::Network::Dash => Network::Dash,
- key_wallet::Network::Testnet => Network::Testnet,
- key_wallet::Network::Regtest => Network::Regtest,
- key_wallet::Network::Devnet => Network::Devnet,
- },
- inner: ext_derived.private_key,
-};
+let derived_priv = PrivateKey {
+ compressed: true,
+ network: ext_derived.network.into(),
+ inner: ext_derived.private_key,
+};This keeps the test lean and centralises future maintenance to a single location.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In dash/tests/psbt.rs around lines 330 to 339, the network conversion is done
via manual pattern matching, which is error-prone. To fix this, implement a From
trait for converting key_wallet::Network to Network inside the dashcore module,
then replace the match expression with a simple call to
ext_derived.network.into() to centralize and simplify the conversion logic.
key-wallet-ffi/tests/ffi_tests.rs
Outdated
| /* Temporarily disabled due to uniffi build issues | ||
| use key_wallet_ffi::{Mnemonic, Language, Network, HDWallet, AddressGenerator}; | ||
| use std::sync::Arc; | ||
| #[test] | ||
| fn test_mnemonic_ffi() { | ||
| // Test mnemonic validation | ||
| let valid_phrase = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about".to_string(); | ||
| let is_valid = Mnemonic::validate(valid_phrase.clone(), Language::English).unwrap(); | ||
| assert!(is_valid); | ||
| // Test creating from phrase | ||
| let mnemonic = Mnemonic::from_phrase(valid_phrase, Language::English).unwrap(); | ||
| assert_eq!(mnemonic.get_word_count(), 12); | ||
| // Test seed generation | ||
| let seed = mnemonic.to_seed("".to_string()); | ||
| assert_eq!(seed.len(), 64); | ||
| } | ||
| #[test] | ||
| fn test_hd_wallet_ffi() { | ||
| // Create wallet from seed | ||
| let seed = vec![0u8; 64]; | ||
| let wallet = HDWallet::from_seed(seed, Network::Testnet).unwrap(); | ||
| // Test deriving keys | ||
| let path = "m/44'/1'/0'/0/0".to_string(); | ||
| let privkey = wallet.derive_priv_key(path.clone()).unwrap(); | ||
| let pubkey = wallet.derive_pub_key(path).unwrap(); | ||
| assert!(!privkey.is_empty()); | ||
| assert!(!pubkey.is_empty()); | ||
| } | ||
| #[test] | ||
| fn test_address_generator_ffi() { | ||
| let seed = vec![0u8; 64]; | ||
| let wallet = Arc::new(HDWallet::from_seed(seed, Network::Testnet).unwrap()); | ||
| let generator = AddressGenerator::new(wallet, 0, 0, false).unwrap(); | ||
| // Test address generation | ||
| let addresses = generator.generate_addresses(5).unwrap(); | ||
| assert_eq!(addresses.len(), 5); | ||
| } | ||
| */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider #[ignore]/conditional compilation instead of commenting-out the real tests
Commenting-out the entire test suite disables all compile-time checking and bit-rots quickly.
Prefer one of:
#[cfg(feature = "uniffi")]
#[test]
fn test_mnemonic_ffi() { /* … */ }
#[test]
#[ignore = "UniFFI scaffolding not generated on CI yet"]
fn test_hd_wallet_ffi() { /* … */ }This keeps the code in the build, catches syntax breakage early, and still lets CI pass while UniFFI integration is stabilised.
The placeholder test can then be dropped.
🤖 Prompt for AI Agents
In key-wallet-ffi/tests/ffi_tests.rs from lines 3 to 49, the entire test suite
is commented out, which disables compile-time checks and risks bit rot. Instead,
re-enable the tests by using conditional compilation attributes like
#[cfg(feature = "uniffi")] on tests that require UniFFI, and apply #[ignore]
with a descriptive reason on tests that should not run yet. This approach keeps
the tests in the build for syntax validation while allowing CI to pass, avoiding
the need for placeholder tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
key-wallet/src/bip32.rs (1)
799-817:⚠️ Potential issue
serde(de)serialisation loses 256-bit child numbers.Serialising a
ChildNumberviau32::from(*self).serialize(serializer)forces every variant into a
u32. ForNormal256/Hardened256this collapses the real 256-bit index tou32::MAX; deserialisation cannot recover the original value.At minimum this is data-loss; at worst it silently corrupts keys.
Suggested direction:
- Encode to/from the canonical string form used by
Display/FromStr(e.g."0x…"), or- Add a dedicated struct with explicit fields for
serdesupport.Fail serialisation if the variant cannot be represented losslessly rather than mangling it.
♻️ Duplicate comments (1)
key-wallet/src/bip32.rs (1)
184-218: 🛠️ Refactor suggestionHex-decoding code is still duplicated – refactor into a shared helper.
The manual two-nibble dance appears both in
ChainCode(lines 184-218) andFingerprint(lines 286-314) and again later inChildNumber::from_str(lines 752-773). The previous review already pointed this out. Consolidate it in one utility to avoid maintenance headaches and reduce attack surface for parser bugs.Also applies to: 286-314
🧹 Nitpick comments (2)
key-wallet/src/bip32.rs (1)
748-773: Re-implementing ad-hoc hex parsing – use existing crates.The manual byte-wise parsing in
ChildNumber::from_stris error-prone and repeats logic already present elsewhere. Preferbitcoin_hashes::hex::FromHex,hexcrate, or the shared helper suggested earlier.Cleaner, safer, and removes ~40 lines of brittle code.
key-wallet-ffi/src/lib_tests.rs (1)
60-66: Avoid brittle prefix assertions for address tests.
assert!(address_str.starts_with('y'))ties the test to the current testnet version byte.
If Dash changes its testnet P2PKH prefix (or you swap to another network), this unit test will break even thoughAddressis correct.Prefer property-based checks already available:
assert_eq!(address.get_network(), Network::Testnet);or round-trip via
Address::from_stringas you do below.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dash/src/lib.rs(1 hunks)dash/src/psbt/mod.rs(5 hunks)dash/tests/psbt.rs(2 hunks)key-wallet-ffi/Cargo.toml(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/lib_tests.rs(1 hunks)key-wallet-ffi/tests/ffi_tests.rs(1 hunks)key-wallet/src/bip32.rs(19 hunks)key-wallet/src/test_macros.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- dash/tests/psbt.rs
- key-wallet/src/test_macros.rs
- key-wallet-ffi/tests/ffi_tests.rs
- dash/src/lib.rs
- dash/src/psbt/mod.rs
- key-wallet-ffi/Cargo.toml
- key-wallet-ffi/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_ripemd160)
| impl fmt::Debug for Fingerprint { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "Fingerprint({}))", self) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same formatting bug for Fingerprint::Debug.
Identical duplicated-parenthesis issue here.
- write!(f, "Fingerprint({}))", self)
+ write!(f, "Fingerprint({})", self)📝 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.
| impl fmt::Debug for Fingerprint { | |
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |
| write!(f, "Fingerprint({}))", self) | |
| } | |
| } | |
| impl fmt::Debug for Fingerprint { | |
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | |
| write!(f, "Fingerprint({})", self) | |
| } | |
| } |
🤖 Prompt for AI Agents
In key-wallet/src/bip32.rs around lines 272 to 276, the Debug implementation for
Fingerprint has an extra closing parenthesis in the format string. Remove the
duplicated closing parenthesis in the write! macro so the output string
correctly formats as "Fingerprint({})" without the extra parenthesis.
| impl fmt::Debug for ChainCode { | ||
| fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| write!(f, "ChainCode({}))", self) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix the extra parenthesis in Debug implementation for ChainCode.
fmt::Debug currently renders "ChainCode())", note the doubled ).
- write!(f, "ChainCode({}))", self)
+ write!(f, "ChainCode({})", self)🤖 Prompt for AI Agents
In key-wallet/src/bip32.rs around lines 105 to 109, the Debug implementation for
ChainCode has an extra closing parenthesis in the format string causing output
like "ChainCode())". Remove the extra parenthesis from the write! macro format
string so it correctly outputs "ChainCode()".
key-wallet-ffi/Cargo.toml
Outdated
| uniffi = { version = "0.27", features = ["build"] } | ||
|
|
||
| [dev-dependencies] | ||
| uniffi = { version = "0.27", features = ["bindgen-tests"] } No newline at end of file |
There was a problem hiding this comment.
| uniffi = { version = "0.27", features = ["bindgen-tests"] } | |
| uniffi = { version = "0.27", features = ["bindgen-tests"] } | |
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
key-wallet/src/bip32.rs (2)
689-716: 🛠️ Refactor suggestionReuse the existing hex-parsing helper instead of ad-hoc code
Manual nibble parsing is duplicated here and elsewhere; it is error-prone and misses validations (e.g. odd-length strings are silently padded with ‘0’).
Callcrate::utils::parse_hex_bytesinstead:-// Simple hex decoder -let hex_bytes = hex_str - .as_bytes() - ... - .collect::<Result<Vec<u8>, Error>>()?; -if hex_bytes.len() != 32 { - return Err(Error::InvalidChildNumberFormat); -} -let mut index_bytes = [0u8; 32]; -index_bytes[32 - hex_bytes.len()..].copy_from_slice(&hex_bytes); +let mut index_bytes = [0u8; 32]; +crate::utils::parse_hex_bytes(hex_str, &mut index_bytes) + .map_err(|_| Error::InvalidChildNumberFormat)?;
1038-1046:⚠️ Potential issue
from_str("")panics
parts.next().unwrap()will panic for an empty string.
ReturnInvalidDerivationPathFormatinstead of panicking.-let mut parts = path.split('/'); -// First parts must be `m`. -if parts.next().unwrap() != "m" { +let mut parts = path.split('/'); +let first = parts.next().ok_or(Error::InvalidDerivationPathFormat)?; +if first != "m" {
♻️ Duplicate comments (6)
dash/src/sml/llmq_type/network.rs (3)
23-30: Duplicate of previous concern – see above.
33-40: Duplicate of previous concern – see above.
43-50: Duplicate of previous concern – see above.key-wallet/src/derivation.rs (1)
132-136: Avoidunwrap()– propagate the error insteadBoth pushes use
ChildNumber::from_hardened_idx(..).unwrap().
If either index ≥ 0x8000_0000 the call will error andunwrap()will panic, crashing the process.-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()))?, +);key-wallet/src/bip32.rs (2)
106-108: Extra)inDebugoutput
fmt::DebugprintsChainCode(())).
Remove the duplicated parenthesis.-write!(f, "ChainCode({}))", self) +write!(f, "ChainCode({})", self)
242-244: Same double-parenthesis bug inFingerprint::Debug-write!(f, "Fingerprint({}))", self) +write!(f, "Fingerprint({})", self)
🧹 Nitpick comments (11)
dash-network-ffi/build.rs (1)
1-3: Missingcargo:rerun-if-changedhampers incremental buildsAdd the directive so Cargo regenerates scaffolding when the UDL changes.
fn main() { + println!("cargo:rerun-if-changed=src/dash_network.udl"); uniffi::generate_scaffolding("src/dash_network.udl").unwrap(); }dash/src/crypto/key.rs (1)
401-405: Defaulting unknown networks to Testnet WIF prefix can cause fund lossUnknown/future networks will generate WIFs with prefix
239, indistinguishable from Testnet/Devnet/Regtest keys.
Better: refuse to format or emit an explicit error for unrecognised variants so users don’t import a key on the wrong chain.- _ => 239, + _ => return Err(fmt::Error), // or choose a more descriptive error pathkey-wallet/src/utils.rs (2)
4-11: Hex-digit helper duplicates existing logicThere is an almost identical conversion routine in
key-wallet/src/bip32.rs(see lines 692-712).
Consider re-exportingparse_hex_digitfrom this module and reusing it
there to keep the implementation in one place (DRY).
14-25: Return a typed error instead of&'static str
parse_hex_bytescurrently bubbles up string literals.
Using a small enum (e.g.HexError { InvalidLen, InvalidChar })
will give callers the ability to pattern-match without string
comparisons and makes future localisation possible.-pub(crate) fn parse_hex_bytes(hex_str: &str, output: &mut [u8]) -> Result<(), &'static str> { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum HexError { InvalidLen, InvalidChar } + +pub(crate) fn parse_hex_bytes(hex_str: &str, output: &mut [u8]) -> Result<(), HexError> { @@ - return Err("invalid hex length"); + return Err(HexError::InvalidLen); @@ - let high = parse_hex_digit(chunk[0]).ok_or("invalid hex character")?; - let low = parse_hex_digit(chunk[1]).ok_or("invalid hex character")?; + let high = parse_hex_digit(chunk[0]).ok_or(HexError::InvalidChar)?; + let low = parse_hex_digit(chunk[1]).ok_or(HexError::InvalidChar)?;dash-network/README.md (1)
22-23: Hard-coded version in docs
dash-network = "0.39.6"is unlikely to exist on crates.io yet.
Consider using"0.39"or"{ path = \"../dash-network\" }"in early
releases to avoid copy-paste confusion.dash-network-ffi/src/dash_network.udl (1)
30-35: Method name might cause ambiguity
boolean is_core_v20_active(u32 block_height);does not encode what height is expected (core vs chain). Consider renaming tois_core_v20_active_at_heightor documenting the unit explicitly to avoid confusion in FFI consumers.dash-network/src/lib.rs (1)
91-103:FromStrshould not useStringas an error type
type Err = String;forces callers to allocate and pattern-match on free-form text.
Introduce a small enum instead so downstream code can handle errors robustly:+#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum ParseNetworkError { + #[error("unknown network type")] + Unknown, +} impl std::str::FromStr for Network { - type Err = String; + type Err = ParseNetworkError; ... - _ => Err(format!("Unknown network type: {}", s)), + _ => Err(ParseNetworkError::Unknown),dash/src/network/constants.rs (1)
71-99: Name collision with anotherNetworkExt
key-wallet/src/address.rsalso defines aNetworkExttrait.
Although they live in different crates, shared users importing both crates may face ambiguous-import errors.
Consider renaming this trait (e.g.,DashNetworkExt) or pre-re-exporting it under a distinct module.key-wallet/src/address.rs (1)
30-40: Hard-coding default versions masks future networksThe fallback branch (
_ =>) repeats the testnet bytes.
Ifdash_networkgrows new variants the address versions will be wrong.
Prefer an explicitunreachable!()to detect missing mappings at compile-time.key-wallet-ffi/src/lib.rs (1)
459-466: Defaulting unknown networks toTestnethides bugsIf a future network variant is added, returning
Testnetwill silently mis-report. Prefer an explicit error or anUnknownenum value.key-wallet-ffi/src/key_wallet.udl (1)
10-17: Document enum ordering or add explicit valuesFFI consumers will assume a fixed numeric value for each
Networkvariant; changes in Rust ordering will break ABI. Add a comment in UDL or explicit numeric tags to mirror therepr(u8)change in Rust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
Cargo.toml(1 hunks)dash-network-ffi/Cargo.toml(1 hunks)dash-network-ffi/README.md(1 hunks)dash-network-ffi/build.rs(1 hunks)dash-network-ffi/src/dash_network.udl(1 hunks)dash-network-ffi/src/lib.rs(1 hunks)dash-network-ffi/uniffi-bindgen.rs(1 hunks)dash-network/Cargo.toml(1 hunks)dash-network/README.md(1 hunks)dash-network/src/lib.rs(1 hunks)dash/Cargo.toml(4 hunks)dash/examples/handshake.rs(3 hunks)dash/src/address.rs(4 hunks)dash/src/blockdata/constants.rs(3 hunks)dash/src/blockdata/transaction/mod.rs(1 hunks)dash/src/blockdata/transaction/outpoint.rs(1 hunks)dash/src/consensus/params.rs(2 hunks)dash/src/crypto/key.rs(3 hunks)dash/src/crypto/sighash.rs(1 hunks)dash/src/lib.rs(2 hunks)dash/src/network/constants.rs(5 hunks)dash/src/psbt/mod.rs(5 hunks)dash/src/sml/llmq_type/mod.rs(2 hunks)dash/src/sml/llmq_type/network.rs(1 hunks)dash/src/sml/masternode_list/from_diff.rs(2 hunks)dash/src/sml/masternode_list/quorum_helpers.rs(1 hunks)dash/src/sml/masternode_list/scores_for_quorum.rs(1 hunks)dash/src/sml/masternode_list_engine/message_request_verification.rs(2 hunks)dash/src/sml/masternode_list_engine/mod.rs(1 hunks)dash/tests/psbt.rs(2 hunks)fuzz/fuzz_targets/dash/deserialize_script.rs(1 hunks)key-wallet-ffi/Cargo.toml(1 hunks)key-wallet-ffi/build.rs(1 hunks)key-wallet-ffi/src/key_wallet.udl(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet/Cargo.toml(1 hunks)key-wallet/src/address.rs(1 hunks)key-wallet/src/bip32.rs(23 hunks)key-wallet/src/derivation.rs(1 hunks)key-wallet/src/dip9.rs(1 hunks)key-wallet/src/lib.rs(1 hunks)key-wallet/src/mnemonic.rs(1 hunks)key-wallet/src/utils.rs(1 hunks)key-wallet/tests/mnemonic_tests.rs(1 hunks)
✅ Files skipped from review due to trivial changes (15)
- key-wallet/src/dip9.rs
- fuzz/fuzz_targets/dash/deserialize_script.rs
- dash/src/blockdata/transaction/outpoint.rs
- dash/examples/handshake.rs
- dash/src/sml/masternode_list/scores_for_quorum.rs
- dash/src/crypto/sighash.rs
- dash-network-ffi/uniffi-bindgen.rs
- dash/src/blockdata/transaction/mod.rs
- dash/src/sml/masternode_list/from_diff.rs
- dash/src/sml/masternode_list/quorum_helpers.rs
- dash/src/sml/masternode_list_engine/message_request_verification.rs
- dash-network-ffi/Cargo.toml
- dash-network/Cargo.toml
- dash-network-ffi/README.md
- dash/src/sml/masternode_list_engine/mod.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- Cargo.toml
- dash/tests/psbt.rs
- dash/Cargo.toml
- key-wallet-ffi/Cargo.toml
- key-wallet/tests/mnemonic_tests.rs
- dash/src/psbt/mod.rs
- dash/src/lib.rs
- key-wallet/Cargo.toml
- key-wallet/src/lib.rs
- key-wallet/src/mnemonic.rs
🧰 Additional context used
🧬 Code Graph Analysis (5)
dash/src/sml/llmq_type/mod.rs (1)
dash/src/address.rs (1)
network(834-836)
key-wallet-ffi/build.rs (2)
dash-network-ffi/uniffi-bindgen.rs (1)
main(1-3)dash-network-ffi/build.rs (1)
main(1-3)
key-wallet/src/utils.rs (1)
key-wallet/src/bip32.rs (1)
hex_str(692-712)
key-wallet/src/derivation.rs (3)
key-wallet/src/bip32.rs (24)
derive_priv(1298-1308)derive_pub(1575-1585)into(778-783)new_master(1274-1287)from(78-80)from(214-216)from(598-608)from(612-627)from(995-997)from(1001-1003)from(1007-1009)from(1261-1263)from_hardened_idx(465-473)index(123-125)index(131-133)index(139-141)index(147-149)index(155-157)index(270-272)index(278-280)index(286-288)index(294-296)index(302-304)index(962-964)key-wallet/src/dip9.rs (1)
from(64-66)key-wallet/src/mnemonic.rs (2)
from(28-40)from_phrase(92-99)
key-wallet-ffi/src/lib.rs (6)
dash-network-ffi/src/lib.rs (4)
initialize(9-11)from(23-30)new(58-62)to_string(76-78)key-wallet/src/mnemonic.rs (10)
from(28-40)from_str(147-150)phrase(102-104)validate(139-141)inner(103-103)from_phrase(92-99)generate(51-74)generate(78-89)word_count(107-109)to_seed(122-126)key-wallet/src/error.rs (2)
from(59-61)from(65-67)key-wallet/src/bip32.rs (29)
from(78-80)from(214-216)from(598-608)from(612-627)from(995-997)from(1001-1003)from(1007-1009)from(1261-1263)from(1267-1269)from_str(250-255)from_str(680-736)from_str(1038-1047)from_str(1838-1841)from_str(1853-1856)from_priv(1552-1564)index(123-125)index(131-133)index(139-141)index(147-149)index(155-157)index(270-272)index(278-280)index(286-288)index(294-296)index(302-304)index(962-964)from_hardened_idx(465-473)from_normal_idx(451-459)child(1107-1111)key-wallet/src/address.rs (5)
new(165-169)from_str(100-125)to_string(86-97)p2pkh(67-74)generate_range(177-209)dash-network/src/lib.rs (1)
from_str(94-102)
🪛 LanguageTool
dash-network/README.md
[uncategorized] ~54-~54: Loose punctuation mark.
Context: ... test network ### Features - default: Enables std - std: Standard library...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deser_net_msg)
🔇 Additional comments (3)
dash/src/sml/llmq_type/mod.rs (1)
1-1: Publicly re-exportingnetworkseems unnecessary – considerpub(crate)Making the sub-module public widens the crate’s public API and locks you into a semver commitment. If the intent is purely internal reuse, prefer
pub(crate) mod network;(or keep it private) to avoid accidental external dependence.key-wallet-ffi/build.rs (1)
1-4: 👍cargo:rerun-if-changedadded – incremental rebuilds now workThe build script now properly invalidates when
key_wallet.udlchanges.
Looks good.dash/src/address.rs (1)
1142-1147:is_valid_for_networkreturns false positives on future variantsThe final wildcard branch (
_ => false) is good, but the preceding
pattern-matching logic assumes the set of variants is closed,
otherwise the early arms may accept an unsupported pair.
After switching to exhaustive matches (see previous comment) this
function will also be safer.
| kw::Error::Base58 => KeyWalletError::AddressError { | ||
| message: "Base58 encoding error".into(), | ||
| }, | ||
| kw::Error::InvalidNetwork => KeyWalletError::AddressError { | ||
| message: "Invalid network".into(), | ||
| }, |
There was a problem hiding this comment.
Pattern-matching wrong Base58 variant – won’t compile
kw::Error::Base58 carries the underlying error (Base58(err)); matching it without binding is a type mismatch.
-kw::Error::Base58 => KeyWalletError::AddressError {
- message: "Base58 encoding error".into(),
-},
+kw::Error::Base58(err) => KeyWalletError::AddressError {
+ message: err.to_string(),
+},📝 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.
| kw::Error::Base58 => KeyWalletError::AddressError { | |
| message: "Base58 encoding error".into(), | |
| }, | |
| kw::Error::InvalidNetwork => KeyWalletError::AddressError { | |
| message: "Invalid network".into(), | |
| }, | |
| kw::Error::Base58(err) => KeyWalletError::AddressError { | |
| message: err.to_string(), | |
| }, | |
| kw::Error::InvalidNetwork => KeyWalletError::AddressError { | |
| message: "Invalid network".into(), | |
| }, |
🤖 Prompt for AI Agents
In key-wallet-ffi/src/lib.rs around lines 181 to 186, the pattern match on
kw::Error::Base58 is incorrect because it does not bind the inner error value,
causing a type mismatch. Fix this by changing the pattern to
kw::Error::Base58(err) to capture the inner error, then use err as needed when
constructing the KeyWalletError::AddressError variant.
|
@CodeRabbit rereview |
|
Tip For best results, initiate chat on the files or code changes. @QuantumExplorer I'll provide a comprehensive re-review of this key wallet refactoring. This is a significant architectural change that deserves careful analysis. 🏗️ Architecture & DesignExcellent modularization strategy! The split into separate crates is well-designed:
The dependency graph is clean and logical, with proper separation of concerns. 🔍 Code Quality AnalysisStrengths:
Areas of Interest:Network handling improvements: // Good: Added fallback arms for unknown network variants
match self.network {
Network::Dash => /* ... */,
Network::Testnet => /* ... */,
_ => /* fallback */ // Prevents panics on new variants
}Serialization consistency: // Changed from .as_bytes() to .to_bytes() for explicit conversion
fingerprint.to_bytes() // More explicit and consistent🧪 Testing & DocumentationOutstanding test coverage:
Documentation quality:
🔒 Security ConsiderationsPositive security aspects:
🚀 Performance & EfficiencyThe refactoring appears performance-neutral:
🔄 Migration PathSmooth migration strategy:
📋 Minor Recommendations
✅ VerdictThis is a high-quality refactoring that:
The architectural decisions are sound, implementation is robust, and the migration path is well-planned. This refactoring positions the codebase well for future development and cross-language integration. Recommendation: Approve 🎉 The modularization effort successfully achieves its goals while maintaining code quality and adding significant value through FFI capabilities. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
key-wallet-ffi/src/lib.rs (1)
165-185:⚠️ Potential issue
kw::Error::Base58mapping loses error context
kw::Error::Base58carries the underlyingbase58ck::Error.
Dropping it replaces useful details (“checksum mismatch”, “invalid character”) with a generic string.- kw::Error::Base58 => KeyWalletError::AddressError { - message: "Base58 encoding error".into(), - }, + kw::Error::Base58(err) => KeyWalletError::AddressError { + message: err.to_string(), + },Keeps FFI consumers informed and eases debugging.
CLAUDE.md (1)
120-126: Grammar nit: “Prefer to composition over inheritance”-Use async/await for asynchronous operations -Prefer composition over inheritance +Use async/await for asynchronous operations +Prefer composition to inheritance🧰 Tools
🪛 LanguageTool
[grammar] ~125-~125: Consider using “to” with “prefer”.
Context: ...ync/await for asynchronous operations - Prefer composition over inheritance ### Memory and State Management - Use ...(PREFER_OVER_TO)
🧹 Nitpick comments (5)
key-wallet-ffi/src/lib.rs (1)
268-273: Coin-type hard-code covers Testnet but still wrong for Devnet / RegtestSLIP-44 assigns coin-type
1for all test networks, but some software (e.g. Dash Core) expects:
- Main-net –
5- Testnet & Regtest –
1- Devnet –
1 + devnetIndex(configurable)At minimum, document the limitation or expose
coin_typeas a parameter so SDK users can set the correct value for their dev-net instance.Also applies to: 288-293
key-wallet-ffi/build-ios.sh (2)
5-11: Addset -uand-o pipefailfor safer bash execution-set -e +set -euo pipefailPrevents the script from continuing on unset variables or failed pipeline commands.
Minor but avoids hard-to-diagnose build artefacts.
24-33: Uselipo -output <dest> -create …for clarityWhile the current order works, the more conventional flag order improves readability:
-lipo -create \ - target/x86_64-apple-ios/release/libkey_wallet_ffi.a \ - target/aarch64-apple-ios-sim/release/libkey_wallet_ffi.a \ - -output target/universal/release/libkey_wallet_ffi_sim.a +lipo -output target/universal/release/libkey_wallet_ffi_sim.a -create \ + target/x86_64-apple-ios/release/libkey_wallet_ffi.a \ + target/aarch64-apple-ios-sim/release/libkey_wallet_ffi.aCLAUDE.md (1)
268-268: Markdownlint: convert bare URL to linkReplace the raw URL with markdown link syntax to satisfy MD034.
For more information about SPARC methodology, see [SPARC docs](https://github.com/ruvnet/claude-code-flow/docs/sparc.md).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
268-268: Bare URL used
null(MD034, no-bare-urls)
NETWORK_HANDLING_FIXES.md (1)
98-98: Excessive exclamation marksTone down to keep docs professional.
- now explicitly panic or use unreachable!, preventing silent misconfigurations + now explicitly panic or use `unreachable!()`, preventing silent misconfigurations🧰 Tools
🪛 LanguageTool
[style] ~98-~98: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2366 characters long)
Context: ... now explicitly panic or use unreachable!, preventing silent misconfigurations - ...(EN_EXCESSIVE_EXCLAMATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignore(1 hunks)CLAUDE.md(1 hunks)NETWORK_HANDLING_FIXES.md(1 hunks)dash-network-ffi/src/lib.rs(1 hunks)dash-network/src/lib.rs(1 hunks)dash/src/address.rs(4 hunks)dash/src/blockdata/constants.rs(3 hunks)dash/src/consensus/params.rs(2 hunks)dash/src/sml/llmq_type/network.rs(1 hunks)key-wallet-ffi/build-ios.sh(1 hunks)key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/lib_tests.rs(1 hunks)key-wallet-ffi/tests/ffi_tests.rs(1 hunks)key-wallet/examples/basic_usage.rs(1 hunks)key-wallet/src/address.rs(1 hunks)key-wallet/tests/address_tests.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- dash-network-ffi/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- dash/src/consensus/params.rs
- dash/src/blockdata/constants.rs
- key-wallet-ffi/tests/ffi_tests.rs
- key-wallet/tests/address_tests.rs
- dash/src/sml/llmq_type/network.rs
- key-wallet/examples/basic_usage.rs
- key-wallet-ffi/src/lib_tests.rs
- dash/src/address.rs
- dash-network/src/lib.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
key-wallet-ffi/src/lib.rs (5)
dash-network-ffi/src/lib.rs (4)
from(23-30)from(34-42)new(58-62)to_string(76-78)key-wallet/src/derivation.rs (6)
derive(76-78)new(52-57)new(149-154)from_seed(60-63)format(158-159)format(166-167)key-wallet/src/bip32.rs (29)
from(78-80)from(214-216)from(598-608)from(612-627)from(995-997)from(1001-1003)from(1007-1009)from(1261-1263)from(1267-1269)from_str(250-255)from_str(680-736)from_str(1038-1047)from_str(1838-1841)from_str(1853-1856)from_priv(1552-1564)index(123-125)index(131-133)index(139-141)index(147-149)index(155-157)index(270-272)index(278-280)index(286-288)index(294-296)index(302-304)index(962-964)from_hardened_idx(465-473)from_normal_idx(451-459)child(1107-1111)key-wallet/src/address.rs (5)
new(174-178)from_str(100-134)to_string(86-97)p2pkh(67-74)generate_range(186-218)key-wallet/src/mnemonic.rs (8)
from_str(147-150)phrase(102-104)inner(103-103)from_phrase(92-99)generate(51-74)generate(78-89)word_count(107-109)to_seed(122-126)
🪛 LanguageTool
NETWORK_HANDLING_FIXES.md
[style] ~98-~98: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2366 characters long)
Context: ... now explicitly panic or use unreachable!, preventing silent misconfigurations - ...
(EN_EXCESSIVE_EXCLAMATION)
CLAUDE.md
[grammar] ~125-~125: Consider using “to” with “prefer”.
Context: ...ync/await for asynchronous operations - Prefer composition over inheritance ### Memory and State Management - Use ...
(PREFER_OVER_TO)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
268-268: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_amount)
| impl NetworkExt for Network { | ||
| /// Get P2PKH version byte | ||
| fn p2pkh_version(&self) -> u8 { | ||
| match self { | ||
| Network::Dash => 76, // 'X' prefix | ||
| Network::Testnet => 140, // 'y' prefix | ||
| Network::Devnet => 140, // 'y' prefix | ||
| Network::Regtest => 140, // 'y' prefix | ||
| _ => 140, // default to testnet version | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove silent fall-backs for unknown Network variants
Both p2pkh_version() and p2sh_version() use a wildcard arm that silently maps every future / unknown Network to the test-net prefix.
That re-introduces the very bug this PR tries to eradicate elsewhere and may route main-net funds to a test-net address.
- Network::Regtest => 140,
- _ => 140, // default to testnet version
+ Network::Regtest => 140,
+ other => unreachable!("Unhandled network variant {other:?}")Make the match exhaustive (or panic) so problems are caught early.
Same change applies to p2sh_version().
Also applies to: 42-50
🤖 Prompt for AI Agents
In key-wallet/src/address.rs around lines 30 to 40 and 42 to 50, the match
statements in p2pkh_version() and p2sh_version() use a wildcard arm that
silently defaults unknown Network variants to the testnet prefix, which can
cause incorrect address generation. Remove the wildcard arm and make the match
exhaustive by explicitly handling all known Network variants. For any unknown
variants, either add a panic or return an error to catch issues early and
prevent silent fallback to testnet prefixes.
There was a problem hiding this comment.
@QuantumExplorer Understood, I'll respect your decision to keep the fallback behavior for unknown network variants. Thank you for the clarification.
✏️ Learnings added
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.115Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
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.
| // Infer network and address type from version byte | ||
| let (network, address_type) = match version { | ||
| 76 => (Network::Dash, AddressType::P2PKH), // Dash mainnet P2PKH | ||
| 16 => (Network::Dash, AddressType::P2SH), // Dash mainnet P2SH | ||
| 140 => { | ||
| // Could be testnet, devnet, or regtest P2PKH | ||
| // Default to testnet, but this is ambiguous | ||
| (Network::Testnet, AddressType::P2PKH) | ||
| } | ||
| 19 => { | ||
| // Could be testnet, devnet, or regtest P2SH | ||
| // Default to testnet, but this is ambiguous | ||
| (Network::Testnet, AddressType::P2SH) | ||
| } | ||
| _ => return Err(Error::InvalidAddress(format!("Unknown version byte: {}", version))), |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Ambiguous version-byte ⇒ wrong network for Devnet / Regtest
from_str hard-codes
140 → Testnet and 19 → Testnet (P2SH) even though the same prefixes are used by Devnet and Regtest.
Address::from_str("8…") coming from a dev-net wallet would therefore be parsed as Testnet and later rejected by FFI when the caller specifies Devnet.
Consider:
- Returning
Err(…)when the version byte is shared by multiple networks and no disambiguation is possible; or - Adding a parameter / overload that lets the caller supply the expected network for validation.
Either way, avoid silently guessing.
Fail fast on ambiguous version bytes or require explicit network
Currently, version bytes 140 (P2PKH) and 19 (P2SH) are unconditionally mapped to Network::Testnet, but those same prefixes are also used by Devnet and Regtest. As a result, parsing an address from Devnet or Regtest will silently default to Testnet and later be rejected by FFI when the caller expects Devnet/Regtest.
Action items:
- In
key-wallet/src/address.rs(lines 112–126), replace the hard-coded Testnet fallback for140and19with one of:- Returning
Err(Error::InvalidAddress(...))for these ambiguous version bytes, or - Adding an overload/parameter that lets callers supply the expected
Networkfor disambiguation.
- Returning
- Add or update tests to cover Devnet and Regtest address parsing paths.
🤖 Prompt for AI Agents
In key-wallet/src/address.rs around lines 112 to 126, the code currently maps
version bytes 140 and 19 directly to Network::Testnet, but these bytes are
ambiguous as they are also used by Devnet and Regtest. To fix this, modify the
match logic to return an Err(Error::InvalidAddress(...)) when encountering these
ambiguous version bytes instead of defaulting to Testnet, or alternatively,
refactor the function to accept an additional parameter specifying the expected
Network to disambiguate the version byte. Also, add or update tests to verify
correct behavior for Devnet and Regtest addresses.
There was a problem hiding this comment.
@QuantumExplorer Understood! I'll acknowledge your decision to keep the current address parsing implementation as-is. The ambiguous version byte handling will remain with the testnet default for shared prefixes.
🐰
╱|、
(˚ˎ 。7
|、˜〵
じしˍ,)ノ
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
key-wallet-ffi/src/lib_tests.rs (2)
16-18: Preferexpectto make failures self-describing
unwrap()hides the context ifvalidate_mnemonicunexpectedly returnsErr.
Usingexpectsurfaces the precise test section that failed.-let is_valid = validate_mnemonic(valid_phrase.clone(), Language::English).unwrap(); +let is_valid = validate_mnemonic(valid_phrase.clone(), Language::English) + .expect("Mnemonic validation unexpectedly failed");
32-34: Zero-filled seed is not representative and may mask edge-casesFeeding
vec![0u8; 64]toHDWallet::from_seedmeets the length requirement but does not mirror
real BIP-39 output.
Consider generating the seed from the mnemonic used in the previous test or at least a
non-trivial byte pattern to improve coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
key-wallet-ffi/src/lib.rs(1 hunks)key-wallet-ffi/src/lib_tests.rs(1 hunks)key-wallet-ffi/tests/ffi_tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet-ffi/tests/ffi_tests.rs
- key-wallet-ffi/src/lib.rs
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
🔇 Additional comments (1)
key-wallet-ffi/src/lib_tests.rs (1)
152-157: Cross-network address acceptance could hide real regressionsThe test asserts that parsing a Testnet address succeeds for Devnet and Regtest as well.
If the prefix mapping ever diverges, this will silently pass and fail in production.Recommend adding a negative assertion when/if distinct prefixes are introduced, or
document explicitly that the same prefix across these networks is a deliberate rule.
…hcore into refactor/splitKeyWallet
Summary by CodeRabbit
New Features
key-walletRust library for Dash key derivation and wallet management, supporting BIP32/BIP39, mnemonic handling, HD wallets, and Dash-specific derivation paths.key-wallet-fficrate providing FFI bindings for cross-language usage (Swift, Kotlin, Python, Ruby) with extensive API coverage.dash-networkcrate centralizing Dash network type definitions, magic bytes, and protocol version activation info.Bug Fixes
Refactor
Networktype todash-networkcrate and extended it via traits for Dash-specific functionality.key-walletcrate.Tests
Documentation
key-wallet,key-wallet-ffi, anddash-networkcrates.