refactor(key-wallet-manager): move key-wallet-manager crate into key-wallet#503
Open
refactor(key-wallet-manager): move key-wallet-manager crate into key-wallet#503
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the standalone 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
key-wallet/src/test_utils/wallet.rs (1)
20-130: Add colocated smoke tests for the new mock wallets.
MockWalletandNonMatchingMockWalletnow define behavior that other tests will rely on, but this file does not lock any of that behavior down locally. A small#[cfg(test)]module coveringset_effect, block/tx recording, and synced-height updates would make these shared fixtures much safer to reuse.As per coding guidelines,
**/*.rs:Keep unit tests in the source code alongside implementation using #[cfg(test)] modulesandWrite unit tests for new functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/test_utils/wallet.rs` around lines 20 - 130, Add a small #[cfg(test)] mod tests in this file that asserts the key behaviors of MockWallet and NonMatchingMockWallet: use MockWallet::new(), call set_effect(txid, net, addresses) and verify transaction_effect(tx) returns the tuple; call process_block(...) and process_mempool_transaction(...) and verify processed_blocks and processed_transactions contain the expected items; check synced_height() before and after update_synced_height(...); and add a sanity test for NonMatchingMockWallet::new() that ensures process_block/process_mempool_transaction return defaults and synced_height/update_synced_height behave as expected. Ensure tests reference the exact symbols MockWallet::set_effect, MockWallet::process_block, MockWallet::process_mempool_transaction, MockWallet::transaction_effect, MockWallet::processed_blocks, MockWallet::processed_transactions, MockWallet::synced_height, MockWallet::update_synced_height, and NonMatchingMockWallet similarly.key-wallet-ffi/src/wallet_manager.rs (1)
1-1: Outdated comment references the old crate name.The comment still refers to "key-wallet-manager crate" but this module has been moved into
key-wallet.📝 Suggested fix
-//! FFI bindings for WalletManager from key-wallet-manager crate +//! FFI bindings for WalletManager from the key-wallet crate's manager module🤖 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` at line 1, Update the outdated module-level doc comment that currently says "FFI bindings for WalletManager from key-wallet-manager crate" to reflect the new crate name "key-wallet"; edit the top-of-file/module doc comment in wallet_manager.rs (the FFI bindings comment) so it references "key-wallet" instead of "key-wallet-manager" and ensure any other inline docs in the same file mentioning the old crate name (e.g., in comments around WalletManager) are updated likewise.key-wallet/src/manager/mod.rs (1)
603-765: Consider extracting common address generation logic.
get_receive_addressandget_change_addressshare nearly identical structure with repeatedAccountTypePreferencematching logic. This is existing code not introduced by this PR, but worth noting for future cleanup.A helper method could unify the common pattern:
fn get_address_with_preference<F>( &mut self, wallet_id: &WalletId, account_index: u32, account_type_pref: AccountTypePreference, mark_as_used: bool, address_fn: F, ) -> Result<AddressGenerationResult, WalletError> where F: Fn(&mut ManagedAccount, &ExtendedPubKey) -> Result<Address, ...>Also applies to: 768-929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/manager/mod.rs` around lines 603 - 765, The get_receive_address and get_change_address functions duplicate the AccountTypePreference matching and address selection/marking logic; extract that shared flow into a generic helper (suggested name get_address_with_preference) that accepts parameters wallet_id, account_index, account_type_pref, mark_as_used and a small address_fn closure (to call ManagedAccount::next_receive_address or next_change_address using the wallet account's account_xpub), returns AddressGenerationResult, and performs the same AccountTypeUsed handling and mark-as-used behavior; update get_receive_address to call this new helper (and likewise get_change_address) and reuse the existing enums/types (AccountTypePreference, AccountTypeUsed, AddressGenerationResult, ManagedAccount methods) to keep behavior identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/Cargo.toml`:
- Around line 48-49: Cargo manifest must enable tokio's sync feature and gate
tokio and rayon behind the crate's std feature: in Cargo.toml update the tokio
dependency to include features = ["macros", "rt", "sync"] and make both tokio
and rayon optional = true, then add them to the std feature (e.g., features.std
= ["tokio", "rayon"]); this will satisfy uses of tokio::sync::broadcast in
key-wallet/src/manager/mod.rs and tokio::sync::Mutex in
key-wallet/src/test_utils/wallet.rs while preserving no_std compatibility
declared in src/lib.rs.
In `@key-wallet/src/lib.rs`:
- Line 46: The module `manager` is exposed unconditionally but depends on
std/tokio/rayon; guard its declaration so it is only compiled when the "std"
feature is enabled by adding a cfg attribute to the `pub mod manager;` line
(e.g., use #[cfg(feature = "std")] before the declaration) and ensure any
internal files (`mod.rs`, `matching.rs`, `process_block.rs`) that use
std/tokio/rayon are likewise behind the same feature flag if they contain
standalone declarations; keep the symbol name `manager` unchanged so references
resolve when the "std" feature is active.
---
Nitpick comments:
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Line 1: Update the outdated module-level doc comment that currently says "FFI
bindings for WalletManager from key-wallet-manager crate" to reflect the new
crate name "key-wallet"; edit the top-of-file/module doc comment in
wallet_manager.rs (the FFI bindings comment) so it references "key-wallet"
instead of "key-wallet-manager" and ensure any other inline docs in the same
file mentioning the old crate name (e.g., in comments around WalletManager) are
updated likewise.
In `@key-wallet/src/manager/mod.rs`:
- Around line 603-765: The get_receive_address and get_change_address functions
duplicate the AccountTypePreference matching and address selection/marking
logic; extract that shared flow into a generic helper (suggested name
get_address_with_preference) that accepts parameters wallet_id, account_index,
account_type_pref, mark_as_used and a small address_fn closure (to call
ManagedAccount::next_receive_address or next_change_address using the wallet
account's account_xpub), returns AddressGenerationResult, and performs the same
AccountTypeUsed handling and mark-as-used behavior; update get_receive_address
to call this new helper (and likewise get_change_address) and reuse the existing
enums/types (AccountTypePreference, AccountTypeUsed, AddressGenerationResult,
ManagedAccount methods) to keep behavior identical.
In `@key-wallet/src/test_utils/wallet.rs`:
- Around line 20-130: Add a small #[cfg(test)] mod tests in this file that
asserts the key behaviors of MockWallet and NonMatchingMockWallet: use
MockWallet::new(), call set_effect(txid, net, addresses) and verify
transaction_effect(tx) returns the tuple; call process_block(...) and
process_mempool_transaction(...) and verify processed_blocks and
processed_transactions contain the expected items; check synced_height() before
and after update_synced_height(...); and add a sanity test for
NonMatchingMockWallet::new() that ensures
process_block/process_mempool_transaction return defaults and
synced_height/update_synced_height behave as expected. Ensure tests reference
the exact symbols MockWallet::set_effect, MockWallet::process_block,
MockWallet::process_mempool_transaction, MockWallet::transaction_effect,
MockWallet::processed_blocks, MockWallet::processed_transactions,
MockWallet::synced_height, MockWallet::update_synced_height, and
NonMatchingMockWallet similarly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f3f374e-9049-400c-87d6-46b71367e0cc
📒 Files selected for processing (62)
Cargo.tomldash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/README.mdkey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet/Cargo.tomlkey-wallet/README.mdkey-wallet/TODO.mdkey-wallet/examples/wallet_creation.rskey-wallet/missing_tests.mdkey-wallet/src/lib.rskey-wallet/src/manager/events.rskey-wallet/src/manager/matching.rskey-wallet/src/manager/mod.rskey-wallet/src/manager/process_block.rskey-wallet/src/manager/wallet_interface.rskey-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (6)
- dash-spv-ffi/Cargo.toml
- key-wallet-manager/src/test_utils/mod.rs
- key-wallet-manager/README.md
- key-wallet-manager/src/lib.rs
- key-wallet-manager/src/test_utils/wallet.rs
- key-wallet-manager/Cargo.toml
key-wallet/Cargo.toml
Outdated
Comment on lines
+48
to
+49
| tokio = { version = "1", features = ["macros", "rt"] } | ||
| rayon = { version = "1.11" } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Tokio 1.x, is tokio::sync::Mutexgated behind thesync feature, and do Tokio/Rayon require the Rust standard library?
💡 Result:
- Yes. In Tokio 1.x,
tokio::sync::Mutex(andtokio::syncgenerally) is only available when the crate featuresyncis enabled. [1][2] - Yes, both Tokio and Rayon require Rust’s standard library (
std).- Tokio’s crate code uses
stdAPIs (e.g.,std::task,std::future,std::os) and it is not a#![no_std]crate. [3] - Rayon is fundamentally built around the standard library’s threading support (e.g., its releases reference use of
std::thread::scope, and the crate’s docs explicitly frame Rayon as extending/mirroringstd). [4][5]
- Tokio’s crate code uses
Sources
- Tokio docs for
tokio::sync::Mutex(feature-gated tosync). (docs.rs) - Tokio
tokio::syncmodule docs (feature-gated tosync). (docs.rs) - Tokio source showing direct
std::...usage. (docs.rs) - Rayon release notes referencing
std::thread::scope. (docs.rs) - Rayon crate docs describing mirroring/extending
std. (docs.rs)
Citations:
- 1: https://docs.rs/tokio/%2A/tokio/sync/struct.Mutex.html?utm_source=openai
- 2: https://docs.rs/tokio/latest/tokio/sync/index.html?utm_source=openai
- 3: https://docs.rs/crate/tokio/latest/source/src/lib.rs
- 4: https://docs.rs/crate/rayon/latest/source/RELEASES.md?utm_source=openai
- 5: https://docs.rs/crate/rayon/latest/source/src/lib.rs
🏁 Script executed:
# Check the Cargo.toml structure and features
cd key-wallet && cat -n Cargo.toml | head -60Repository: dashpay/rust-dashcore
Length of output: 3159
🏁 Script executed:
# Verify tokio::sync::Mutex usage in test_utils
grep -r "tokio::sync::Mutex" key-wallet/src/test_utils/ || echo "Not found"Repository: dashpay/rust-dashcore
Length of output: 124
🏁 Script executed:
# Check if there are any references to no_std or std feature gating
grep -r "no_std\|#!\[no_std\]" key-wallet/src/ || echo "No no_std references"Repository: dashpay/rust-dashcore
Length of output: 497
🏁 Script executed:
# Check where rayon is used
grep -r "rayon" key-wallet/src/ --include="*.rs" || echo "No rayon references in src/"Repository: dashpay/rust-dashcore
Length of output: 161
🏁 Script executed:
# Check where tokio is used
grep -r "tokio" key-wallet/src/ --include="*.rs" | head -20Repository: dashpay/rust-dashcore
Length of output: 1768
🏁 Script executed:
# Check if test-utils are conditionally compiled
grep -B2 -A2 "mod test_utils\|test-utils" key-wallet/src/lib.rsRepository: dashpay/rust-dashcore
Length of output: 159
Add Tokio's sync feature and gate both dependencies behind the std feature.
tokio::sync::broadcast is used in key-wallet/src/manager/mod.rs (production code) and tokio::sync::Mutex is used in key-wallet/src/test_utils/wallet.rs, but the manifest only enables macros and rt for tokio—the missing sync feature will cause compilation failures. Additionally, both tokio and rayon are unconditional dependencies that require the standard library, which breaks the crate's declared no_std support (see src/lib.rs:1). These dependencies should be gated behind the std feature.
Example fix
-tokio = { version = "1", features = ["macros", "rt"] }
-rayon = { version = "1.11" }And update the std feature (line 13) to include them:
-std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand"]
+std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand", "tokio", "rayon"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/Cargo.toml` around lines 48 - 49, Cargo manifest must enable
tokio's sync feature and gate tokio and rayon behind the crate's std feature: in
Cargo.toml update the tokio dependency to include features = ["macros", "rt",
"sync"] and make both tokio and rayon optional = true, then add them to the std
feature (e.g., features.std = ["tokio", "rayon"]); this will satisfy uses of
tokio::sync::broadcast in key-wallet/src/manager/mod.rs and tokio::sync::Mutex
in key-wallet/src/test_utils/wallet.rs while preserving no_std compatibility
declared in src/lib.rs.
| pub mod error; | ||
| pub mod gap_limit; | ||
| pub mod managed_account; | ||
| pub mod manager; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== key-wallet/Cargo.toml features =="
sed -n '1,220p' key-wallet/Cargo.toml
echo
echo "== key-wallet/src/lib.rs =="
sed -n '1,90p' key-wallet/src/lib.rs
echo
echo "== std/tokio/rayon usage under key-wallet/src/manager =="
fd . key-wallet/src/manager -e rs -x sh -c '
file="$1"
echo "--- ${file}"
rg -n "use std::|std::|tokio|rayon" "$file" || true
' sh {}Repository: dashpay/rust-dashcore
Length of output: 5825
Guard manager behind the std feature flag.
The manager module directly uses std, tokio, and rayon (visible in mod.rs, matching.rs, and process_block.rs), yet it is exposed unconditionally at line 46 while the crate declares no_std support on line 7. This creates a mismatch: users building with default-features = false will encounter a module that depends on unavailable APIs.
Wrap the module declaration with:
- pub mod manager;
+ #[cfg(feature = "std")]
+ pub mod manager;📝 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
| pub mod manager; | |
| #[cfg(feature = "std")] | |
| pub mod manager; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/lib.rs` at line 46, The module `manager` is exposed
unconditionally but depends on std/tokio/rayon; guard its declaration so it is
only compiled when the "std" feature is enabled by adding a cfg attribute to the
`pub mod manager;` line (e.g., use #[cfg(feature = "std")] before the
declaration) and ensure any internal files (`mod.rs`, `matching.rs`,
`process_block.rs`) that use std/tokio/rayon are likewise behind the same
feature flag if they contain standalone declarations; keep the symbol name
`manager` unchanged so references resolve when the "std" feature is active.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #503 +/- ##
=============================================
- Coverage 66.86% 66.56% -0.31%
=============================================
Files 313 318 +5
Lines 64775 65830 +1055
=============================================
+ Hits 43314 43819 +505
- Misses 21461 22011 +550
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
ZocoLini
force-pushed
the
refactor/manager-crate-moved
branch
2 times, most recently
from
faf9db2 to
9db818d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
key-wallet/README.md (1)
36-47: Consider consolidating duplicate "Watch-Only" documentation.The "Watch-Only Support" feature is documented in both the new "Wallet Manager" section (line 44) and the existing "Account Types" section (line 59). Consider consolidating these references to avoid redundancy and improve documentation maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` around lines 36 - 47, Remove the duplicate "Watch-Only Support" text by consolidating the description into a single canonical place: either keep the concise bullet under "Wallet Manager" (the "Watch-Only Support" bullet) or the more detailed entry under "Account Types", then remove the other duplicate reference; update the retained entry to mention both wallet-level watch-only monitoring and account-level watch-only behavior so consumers can find all relevant info in one spot, and add a cross-reference line in the other section ("Wallet Manager" or "Account Types") pointing to the canonical section.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@key-wallet/README.md`:
- Around line 36-47: Remove the duplicate "Watch-Only Support" text by
consolidating the description into a single canonical place: either keep the
concise bullet under "Wallet Manager" (the "Watch-Only Support" bullet) or the
more detailed entry under "Account Types", then remove the other duplicate
reference; update the retained entry to mention both wallet-level watch-only
monitoring and account-level watch-only behavior so consumers can find all
relevant info in one spot, and add a cross-reference line in the other section
("Wallet Manager" or "Account Types") pointing to the canonical section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e8c176c-1384-4fd0-aea4-8f6c9e60ccd8
📒 Files selected for processing (62)
Cargo.tomldash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/README.mdkey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet/Cargo.tomlkey-wallet/README.mdkey-wallet/TODO.mdkey-wallet/examples/wallet_creation.rskey-wallet/missing_tests.mdkey-wallet/src/lib.rskey-wallet/src/manager/events.rskey-wallet/src/manager/matching.rskey-wallet/src/manager/mod.rskey-wallet/src/manager/process_block.rskey-wallet/src/manager/wallet_interface.rskey-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (6)
- key-wallet-manager/src/test_utils/mod.rs
- dash-spv-ffi/Cargo.toml
- key-wallet-manager/Cargo.toml
- key-wallet-manager/README.md
- key-wallet-manager/src/lib.rs
- key-wallet-manager/src/test_utils/wallet.rs
✅ Files skipped from review due to trivial changes (1)
- dash-spv/src/sync/blocks/pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (28)
- dash-spv/src/sync/events.rs
- key-wallet-ffi/src/transaction.rs
- dash-spv/src/sync/filters/sync_manager.rs
- dash-spv/tests/dashd_sync/setup.rs
- dash-spv/src/client/core.rs
- dash-spv/src/sync/filters/batch.rs
- dash-spv-ffi/src/client.rs
- dash-spv/src/client/transactions.rs
- dash-spv/src/client/sync_coordinator.rs
- dash-spv/src/validation/filter.rs
- dash-spv/src/sync/blocks/manager.rs
- dash-spv/src/sync/filters/pipeline.rs
- dash-spv/src/sync/blocks/sync_manager.rs
- dash-spv/src/sync/filters/batch_tracker.rs
- key-wallet/Cargo.toml
- key-wallet/src/lib.rs
- Cargo.toml
- key-wallet-ffi/src/wallet_manager.rs
- key-wallet-ffi/tests/test_error_conversions.rs
- key-wallet/src/manager/events.rs
- dash-spv/tests/dashd_sync/tests_basic.rs
- key-wallet/src/test_utils/mod.rs
- dash-spv/src/client/queries.rs
- dash-spv/examples/simple_sync.rs
- dash-spv/tests/wallet_integration_test.rs
- dash-spv/src/client/lifecycle.rs
- dash-spv/src/main.rs
- key-wallet-ffi/Cargo.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet/README.md (2)
226-236:⚠️ Potential issue | 🟡 MinorAdd the
managermodule to the Key Modules section.The "Key Modules" section doesn't list the
managermodule, which now contains the wallet manager functionality moved from thekey-wallet-managercrate. This should be documented alongside the other core modules.📚 Suggested addition
- `utxo`: UTXO set management - `gap_limit`: Address discovery gap limit tracking +- `manager`: Multi-wallet management and coordination🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` around lines 226 - 236, The README's "Key Modules" list is missing the new manager module; add an entry for `manager` alongside the other modules to document the wallet manager functionality migrated from the key-wallet-manager crate — update the Key Modules bullet list to include `manager`: Wallet manager (e.g., "manager: Wallet manager and coordination of wallets/accounts") so readers can find the module and its purpose.
74-201:⚠️ Potential issue | 🟠 MajorUpdate code examples to match the actual refactored API.
The README examples contain multiple API incompatibilities that will fail to compile:
Line 93: Import is incorrect. Change
use key_wallet::managed_account::ManagedAccount;touse key_wallet::managed_account::ManagedCoreAccount;(the exported type name isManagedCoreAccount, notManagedAccount)Line 96: Update instantiation from
ManagedAccount::from_account(&account)toManagedCoreAccount::from_account(&account)Line 101: Method signature changed.
generate_receive_addresses(10)should benext_receive_addresses(Some(&account.extended_public_key()), 10, true)(requires the extended public key, count, and whether to add to state)Line 82:
wallet.create_account()doesn't exist on Wallet. The method is onManagerinstead.Line 147:
derive_address_at_pool()does not exist. For CoinJoin accounts, use the address pools from theManagedAccountType::CoinJoinvariant directly, or usenext_address()method on the managed account.Line 187: Change
wallet.to_managed_wallet_info()toManagedWalletInfo::from_wallet(&wallet)(static constructor, not instance method)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` around lines 74 - 201, The README code examples use outdated API names and call sites: replace ManagedAccount with ManagedCoreAccount (change import key_wallet::managed_account::ManagedCoreAccount and use ManagedCoreAccount::from_account(&account)); update address generation to call next_receive_addresses(Some(&account.extended_public_key()), count, add_to_state) instead of generate_receive_addresses; stop calling wallet.create_account(...) — create accounts via Manager (use Manager::create_account or the Manager instance method); for CoinJoin accounts use the ManagedAccountType::CoinJoin address pools or call the managed account's next_address() rather than derive_address_at_pool(); and replace wallet.to_managed_wallet_info() with ManagedWalletInfo::from_wallet(&wallet). Ensure all examples reference these symbols (ManagedCoreAccount, next_receive_addresses, Manager::create_account, ManagedAccountType::CoinJoin / next_address, ManagedWalletInfo::from_wallet) so they compile against the refactored API.
♻️ Duplicate comments (1)
key-wallet/Cargo.toml (1)
48-49:⚠️ Potential issue | 🟠 MajorGate
tokioandrayonbehind thestdfeature forno_stdcompatibility.The crate declares
#![no_std]support insrc/lib.rs, but bothtokioandrayonrequire the standard library. While the code using these dependencies is already feature-gated with#[cfg(feature = "std")](e.g.,broadcast::Senderinmanager/mod.rsandrayoninmanager/matching.rs), the dependencies themselves are unconditional, which will pull instdeven when thestdfeature is disabled.🛠️ Proposed fix
-tokio = { version = "1", features = ["macros", "rt", "sync"] } -rayon = { version = "1.11" } +tokio = { version = "1", features = ["macros", "rt", "sync"], optional = true } +rayon = { version = "1.11", optional = true }Then update the
stdfeature to include them:-std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand"] +std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "dash-network/std", "rand", "tokio", "rayon"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/Cargo.toml` around lines 48 - 49, Make the Cargo.toml conditionalize the tokio and rayon dependencies behind the "std" feature so they aren't pulled into no_std builds; update the dependency declarations for tokio and rayon to be optional or feature-gated and add them to the "std" feature list, ensuring the existing code guarded by #[cfg(feature = "std")] (e.g., usages like broadcast::Sender in manager::mod and rayon in manager::matching) still compiles when "std" is enabled.
🧹 Nitpick comments (5)
key-wallet/src/test_utils/wallet.rs (2)
109-109: UseCoreBlockHeighttype alias for consistency.Same issue as
MockWallet::process_block- use the type alias for consistency with the trait signature and other methods.Suggested fix
- async fn process_block(&mut self, _block: &Block, _height: u32) -> BlockProcessingResult { + async fn process_block(&mut self, _block: &Block, _height: CoreBlockHeight) -> BlockProcessingResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/test_utils/wallet.rs` at line 109, The process_block signature in MockWallet uses a raw u32 for the height parameter; change the parameter type to the CoreBlockHeight type alias to match the trait and other implementations by updating async fn process_block(&mut self, _block: &Block, _height: u32) -> BlockProcessingResult to use _height: CoreBlockHeight (keep parameter name and async/return types unchanged) so the signature is consistent with the trait and other methods.
55-55: UseCoreBlockHeighttype alias for consistency.The
heightparameter usesu32directly, but the trait signature usesCoreBlockHeight(seewallet_interface.rs:40), and other methods in this impl (synced_height,update_synced_height) correctly use the type alias. Using the alias maintains consistency and adapts automatically if the underlying type changes.Suggested fix
- async fn process_block(&mut self, block: &Block, height: u32) -> BlockProcessingResult { + async fn process_block(&mut self, block: &Block, height: CoreBlockHeight) -> BlockProcessingResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/test_utils/wallet.rs` at line 55, The parameter type for process_block currently uses u32; change it to the CoreBlockHeight type alias to match the trait signature and other methods (e.g., synced_height, update_synced_height) so the impl remains consistent and resilient to future alias changes—update the function signature async fn process_block(&mut self, block: &Block, height: CoreBlockHeight) -> BlockProcessingResult and adjust any internal usages of height if necessary to compile.key-wallet/src/manager/mod.rs (1)
1104-1137: Handle feature-gatedcrate::Errorvariants explicitly.The fallback arm at Line 1135 now absorbs
Error::Slip10andError::BLSwhenever those features are enabled, and it will also silently genericize any futurecrate::Errorvariants. Please map those variants explicitly and drop the wildcard so this conversion stays intentional and compile-time checked.As per coding guidelines "Use proper error types (thiserror) and propagate errors appropriately".♻️ Suggested direction
match err { Error::InvalidMnemonic(msg) => WalletError::InvalidMnemonic(msg), Error::InvalidDerivationPath(msg) => { WalletError::InvalidParameter(format!("Invalid derivation path: {}", msg)) } @@ Error::Serialization(msg) => { WalletError::InvalidParameter(format!("Serialization error: {}", msg)) } Error::Bip32(e) => WalletError::AccountCreation(format!("BIP32 error: {}", e)), + #[cfg(feature = "eddsa")] + Error::Slip10(e) => WalletError::AccountCreation(format!("SLIP-0010 error: {}", e)), + #[cfg(feature = "bls")] + Error::BLS(e) => WalletError::AccountCreation(format!("BLS error: {}", e)), Error::Secp256k1(e) => WalletError::AccountCreation(format!("Secp256k1 error: {}", e)), Error::Base58 => WalletError::InvalidParameter("Base58 decoding error".to_string()), Error::NoKeySource => { WalletError::InvalidParameter("No key source available".to_string()) } - #[allow(unreachable_patterns)] - _ => WalletError::InvalidParameter(format!("Key wallet error: {}", err)), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/manager/mod.rs` around lines 1104 - 1137, The From<crate::Error> impl for WalletError currently uses a wildcard arm that swallows feature-gated variants (e.g., Error::Slip10, Error::BLS) and future errors; update the match in the From<crate::Error> for WalletError (the conversion function) to explicitly handle Error::Slip10 and Error::BLS (and any other current crate::Error variants) with appropriate WalletError constructors (e.g., map to WalletError::AccountCreation or WalletError::InvalidParameter as appropriate), and remove the trailing wildcard arm so all crate::Error variants are exhaustively matched at compile time.key-wallet/README.md (2)
42-47: Add usage examples for the wallet manager.The "Wallet Manager" subsection documents new functionality but lacks concrete usage examples. Consider adding a code example in the "Usage Examples" section showing how to use the
managermodule APIs (e.g., creating a WalletManager, managing multiple wallets, etc.).Would you like me to draft an example usage section for the wallet manager functionality?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` around lines 42 - 47, Add concrete usage examples for the Wallet Manager by creating a short "Usage Examples" snippet that demonstrates the manager module APIs: show instantiation of WalletManager (e.g., new WalletManager(...)), creating/managing multiple wallets (methods like createWallet, importWallet, listWallets), configuring watch-only accounts and BIP44 account creation (methods like enableWatchOnly, createBIP44Account), and performing common ops (getBalance, sync, removeWallet). Place this example under the "Usage Examples" section in README.md, use realistic argument placeholders and brief comments to illustrate flow, and reference the WalletManager class and its key methods so readers can map the example to the implementation.
22-22: Clarify that multi-wallet management is in themanagermodule.The architecture point mentions multi-wallet management but doesn't indicate this functionality is provided by a specific module. Consider updating to reference the
managermodule explicitly.📝 Suggested clarification
-- **Multi-wallet management**: Manages multiple wallets, each containing multiple accounts +- **Multi-wallet management** (`manager` module): Manages multiple wallets, each containing multiple accounts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/README.md` at line 22, Update the README bullet to state that multi-wallet management is implemented in the manager module; specifically change the line "**Multi-wallet management**: Manages multiple wallets, each containing multiple accounts" to mention the manager module (e.g., "**Multi-wallet management (manager module)**: The manager module handles multiple wallets, each containing multiple accounts") and optionally add a short pointer to the manager module's API or docs for more details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet-ffi/src/error.rs`:
- Around line 194-196: Update the doc comment above the impl From for FFIError
to reference the moved type path key_wallet::manager::WalletError (instead of
key_wallet::WalletError); locate the impl From<key_wallet::manager::WalletError>
for FFIError and change the comment text to accurately state that it converts
key_wallet::manager::WalletError to FFIError so readers are pointed to the
correct API.
---
Outside diff comments:
In `@key-wallet/README.md`:
- Around line 226-236: The README's "Key Modules" list is missing the new
manager module; add an entry for `manager` alongside the other modules to
document the wallet manager functionality migrated from the key-wallet-manager
crate — update the Key Modules bullet list to include `manager`: Wallet manager
(e.g., "manager: Wallet manager and coordination of wallets/accounts") so
readers can find the module and its purpose.
- Around line 74-201: The README code examples use outdated API names and call
sites: replace ManagedAccount with ManagedCoreAccount (change import
key_wallet::managed_account::ManagedCoreAccount and use
ManagedCoreAccount::from_account(&account)); update address generation to call
next_receive_addresses(Some(&account.extended_public_key()), count,
add_to_state) instead of generate_receive_addresses; stop calling
wallet.create_account(...) — create accounts via Manager (use
Manager::create_account or the Manager instance method); for CoinJoin accounts
use the ManagedAccountType::CoinJoin address pools or call the managed account's
next_address() rather than derive_address_at_pool(); and replace
wallet.to_managed_wallet_info() with ManagedWalletInfo::from_wallet(&wallet).
Ensure all examples reference these symbols (ManagedCoreAccount,
next_receive_addresses, Manager::create_account, ManagedAccountType::CoinJoin /
next_address, ManagedWalletInfo::from_wallet) so they compile against the
refactored API.
---
Duplicate comments:
In `@key-wallet/Cargo.toml`:
- Around line 48-49: Make the Cargo.toml conditionalize the tokio and rayon
dependencies behind the "std" feature so they aren't pulled into no_std builds;
update the dependency declarations for tokio and rayon to be optional or
feature-gated and add them to the "std" feature list, ensuring the existing code
guarded by #[cfg(feature = "std")] (e.g., usages like broadcast::Sender in
manager::mod and rayon in manager::matching) still compiles when "std" is
enabled.
---
Nitpick comments:
In `@key-wallet/README.md`:
- Around line 42-47: Add concrete usage examples for the Wallet Manager by
creating a short "Usage Examples" snippet that demonstrates the manager module
APIs: show instantiation of WalletManager (e.g., new WalletManager(...)),
creating/managing multiple wallets (methods like createWallet, importWallet,
listWallets), configuring watch-only accounts and BIP44 account creation
(methods like enableWatchOnly, createBIP44Account), and performing common ops
(getBalance, sync, removeWallet). Place this example under the "Usage Examples"
section in README.md, use realistic argument placeholders and brief comments to
illustrate flow, and reference the WalletManager class and its key methods so
readers can map the example to the implementation.
- Line 22: Update the README bullet to state that multi-wallet management is
implemented in the manager module; specifically change the line "**Multi-wallet
management**: Manages multiple wallets, each containing multiple accounts" to
mention the manager module (e.g., "**Multi-wallet management (manager module)**:
The manager module handles multiple wallets, each containing multiple accounts")
and optionally add a short pointer to the manager module's API or docs for more
details.
In `@key-wallet/src/manager/mod.rs`:
- Around line 1104-1137: The From<crate::Error> impl for WalletError currently
uses a wildcard arm that swallows feature-gated variants (e.g., Error::Slip10,
Error::BLS) and future errors; update the match in the From<crate::Error> for
WalletError (the conversion function) to explicitly handle Error::Slip10 and
Error::BLS (and any other current crate::Error variants) with appropriate
WalletError constructors (e.g., map to WalletError::AccountCreation or
WalletError::InvalidParameter as appropriate), and remove the trailing wildcard
arm so all crate::Error variants are exhaustively matched at compile time.
In `@key-wallet/src/test_utils/wallet.rs`:
- Line 109: The process_block signature in MockWallet uses a raw u32 for the
height parameter; change the parameter type to the CoreBlockHeight type alias to
match the trait and other implementations by updating async fn
process_block(&mut self, _block: &Block, _height: u32) -> BlockProcessingResult
to use _height: CoreBlockHeight (keep parameter name and async/return types
unchanged) so the signature is consistent with the trait and other methods.
- Line 55: The parameter type for process_block currently uses u32; change it to
the CoreBlockHeight type alias to match the trait signature and other methods
(e.g., synced_height, update_synced_height) so the impl remains consistent and
resilient to future alias changes—update the function signature async fn
process_block(&mut self, block: &Block, height: CoreBlockHeight) ->
BlockProcessingResult and adjust any internal usages of height if necessary to
compile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f133b11a-a075-4c24-b627-a4b0fbdbf750
📒 Files selected for processing (62)
Cargo.tomldash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/README.mdkey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet/Cargo.tomlkey-wallet/README.mdkey-wallet/TODO.mdkey-wallet/examples/wallet_creation.rskey-wallet/missing_tests.mdkey-wallet/src/lib.rskey-wallet/src/manager/events.rskey-wallet/src/manager/matching.rskey-wallet/src/manager/mod.rskey-wallet/src/manager/process_block.rskey-wallet/src/manager/wallet_interface.rskey-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (6)
- key-wallet-manager/src/test_utils/mod.rs
- dash-spv-ffi/Cargo.toml
- key-wallet-manager/Cargo.toml
- key-wallet-manager/README.md
- key-wallet-manager/src/test_utils/wallet.rs
- key-wallet-manager/src/lib.rs
✅ Files skipped from review due to trivial changes (2)
- dash-spv/src/client/mempool.rs
- dash-spv/src/sync/blocks/pipeline.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- key-wallet/src/lib.rs
- dash-spv-ffi/src/client.rs
- key-wallet-ffi/src/transaction.rs
- dash-spv/src/sync/filters/batch_tracker.rs
- dash-spv-ffi/src/callbacks.rs
- dash-spv/src/sync/filters/pipeline.rs
- dash-spv/src/sync/blocks/manager.rs
- dash-spv/tests/dashd_sync/tests_basic.rs
- dash-spv/src/sync/filters/batch.rs
- key-wallet-ffi/Cargo.toml
- key-wallet/tests/integration_test.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- key-wallet-ffi/src/wallet_manager.rs
- dash-spv/examples/spv_with_wallet.rs
- key-wallet/src/test_utils/mod.rs
- dash-spv/tests/dashd_sync/helpers.rs
ZocoLini
force-pushed
the
refactor/manager-crate-moved
branch
from
9db818d to
6933ecd
Compare
ZocoLini
changed the title
ZocoLini
force-pushed
the
refactor/manager-crate-moved
branch
from
6933ecd to
158b247
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
refactor/manager-crate-moved
branch
from
158b247 to
d356618
Compare
github-actions
bot
removed
the
merge-conflict
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/manager/mod.rs (1)
382-410:⚠️ Potential issue | 🟠 MajorReject imported xprvs whose network does not match the manager.
This import path inserts the derived wallet without checking that its embedded network matches
self.network. A mainnet xprv can therefore be added to a testnet manager and later participate in the wrong address/filter flow.Suggested fix
let wallet = Wallet::from_extended_key(extended_priv_key, account_creation_options) .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + + if wallet.network != self.network { + return Err(WalletError::InvalidNetwork); + } // Compute wallet ID from the wallet's root public key let wallet_id = wallet.compute_wallet_id();Please mirror the same guard in the xpub/bytes import entry points as well. 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/manager/mod.rs` around lines 382 - 410, In import_wallet_from_extended_priv_key, after parsing the ExtendedPrivKey (ExtendedPrivKey::from_str) and before creating the Wallet (Wallet::from_extended_key), validate that the extended private key's network equals the manager's network (self.network) and return an appropriate WalletError (e.g., WalletError::InvalidParameter or a network-specific error) if it does not match; apply the same network-consistency guard to the xpub and raw-bytes import entry points (the corresponding import_xpub... / import_wallet_from_bytes... functions) so no mainnet keys can be added to a testnet manager or vice versa.
♻️ Duplicate comments (1)
key-wallet/Cargo.toml (1)
47-48:⚠️ Potential issue | 🟠 MajorGate
tokioandrayonbehind thestdfeature to preserveno_stdcompatibility.Both
tokioandrayonrequire the standard library, but they're added as unconditional dependencies whilekey-walletdeclaresno_stdsupport. Themanagermodule uses these dependencies unconditionally (e.g.,rayon::preludeinmatching.rs,tokio::sync::broadcastinmod.rs), which will breakno_stdbuilds.Proposed fix
[features] default = ["std"] -std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "rand"] +std = ["dashcore_hashes/std", "secp256k1/std", "bip39/std", "getrandom", "rand", "tokio", "rayon"] ... -tokio = { version = "1", features = ["macros", "rt", "sync"] } -rayon = { version = "1.11" } +tokio = { version = "1", features = ["macros", "rt", "sync"], optional = true } +rayon = { version = "1.11", optional = true }Additionally, the
managermodule declaration inlib.rsshould be gated with#[cfg(feature = "std")].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/Cargo.toml` around lines 47 - 48, Gate the tokio and rayon dependencies behind the std feature in Cargo.toml (make them optional and #[cfg(feature = "std")]-only consumers) and protect all uses in the manager module: add #[cfg(feature = "std")] to the manager module declaration in lib.rs and wrap std-only imports/usages in manager/mod.rs and manager/matching.rs (e.g., rayon::prelude and tokio::sync::broadcast) with the same feature gate so no_std builds never pull those symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/missing_tests.md`:
- Around line 1-3: Update the checklist references to use the new module paths
under the reorganized key-wallet crate: replace any old top-level mentions of
wallet_manager.rs or wallet.rs with their post-move module paths (e.g.,
key_wallet::manager::wallet_manager or key_wallet::wallet::wallet) and verify
items that reference functions/structs (such as WalletManager, Wallet,
Manager::new, Wallet::load) use the fully qualified names in the new hierarchy;
scan the document for all occurrences of old paths and update them to
key_wallet::manager::* or key_wallet::wallet::* as appropriate so the checklist
matches the new module structure.
In `@key-wallet/src/manager/mod.rs`:
- Around line 12-35: The file currently imports std::collections::BTreeSet and
std::str::FromStr unconditionally which breaks no_std builds; either gate the
entire manager module behind the "std" feature by adding #[cfg(feature = "std")]
at the top of this manager module, or (preferably) add #[cfg(feature = "std")]
to the two problematic imports (the lines importing BTreeSet and FromStr) so
those symbols are only pulled in when feature = "std"; update any code that
depends on those imports accordingly (look for references to BTreeSet and
FromStr in this module such as usages in functions and types).
In `@README.md`:
- Around line 29-35: Remove the Git conflict markers (<<<<<<<, =======, >>>>>>>)
in the README table and resolve the merge by keeping the incoming change: the
"wallet" row should list only "key-wallet" (remove "key-wallet-manager"), and
the "ffi" row should list "dash-network-ffi, dash-spv-ffi, key-wallet-ffi";
ensure the codecov badge links remain unchanged and that there are no leftover
conflict markers anywhere in the file.
---
Outside diff comments:
In `@key-wallet/src/manager/mod.rs`:
- Around line 382-410: In import_wallet_from_extended_priv_key, after parsing
the ExtendedPrivKey (ExtendedPrivKey::from_str) and before creating the Wallet
(Wallet::from_extended_key), validate that the extended private key's network
equals the manager's network (self.network) and return an appropriate
WalletError (e.g., WalletError::InvalidParameter or a network-specific error) if
it does not match; apply the same network-consistency guard to the xpub and
raw-bytes import entry points (the corresponding import_xpub... /
import_wallet_from_bytes... functions) so no mainnet keys can be added to a
testnet manager or vice versa.
---
Duplicate comments:
In `@key-wallet/Cargo.toml`:
- Around line 47-48: Gate the tokio and rayon dependencies behind the std
feature in Cargo.toml (make them optional and #[cfg(feature = "std")]-only
consumers) and protect all uses in the manager module: add #[cfg(feature =
"std")] to the manager module declaration in lib.rs and wrap std-only
imports/usages in manager/mod.rs and manager/matching.rs (e.g., rayon::prelude
and tokio::sync::broadcast) with the same feature gate so no_std builds never
pull those symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8fda84d9-c1ed-4311-9b47-6fb65fd5728d
📒 Files selected for processing (66)
.codecov.yml.github/ci-groups.ymlCargo.tomlREADME.mddash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/README.mdkey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet/Cargo.tomlkey-wallet/README.mdkey-wallet/TODO.mdkey-wallet/examples/wallet_creation.rskey-wallet/missing_tests.mdkey-wallet/src/lib.rskey-wallet/src/manager/events.rskey-wallet/src/manager/matching.rskey-wallet/src/manager/mod.rskey-wallet/src/manager/process_block.rskey-wallet/src/manager/wallet_interface.rskey-wallet/src/missing_tests.mdkey-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (10)
- dash-spv-ffi/Cargo.toml
- key-wallet-manager/README.md
- key-wallet/src/missing_tests.md
- .codecov.yml
- .github/ci-groups.yml
- key-wallet-manager/src/test_utils/wallet.rs
- key-wallet/TODO.md
- key-wallet-manager/Cargo.toml
- key-wallet-manager/src/lib.rs
- key-wallet-manager/src/test_utils/mod.rs
✅ Files skipped from review due to trivial changes (1)
- dash-spv/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (26)
- dash-spv/src/sync/filters/pipeline.rs
- dash-spv/tests/dashd_sync/helpers.rs
- dash-spv/src/sync/filters/sync_manager.rs
- key-wallet/src/lib.rs
- dash-spv/src/sync/events.rs
- dash-spv/src/client/lifecycle.rs
- key-wallet/examples/wallet_creation.rs
- key-wallet/src/manager/events.rs
- key-wallet-ffi/src/error.rs
- key-wallet/tests/spv_integration_tests.rs
- key-wallet-ffi/src/wallet_manager_tests.rs
- dash-spv/examples/simple_sync.rs
- dash-spv/src/client/transactions.rs
- Cargo.toml
- dash-spv/examples/filter_sync.rs
- dash-spv/tests/peer_test.rs
- dash-spv/src/client/sync_coordinator.rs
- dash-spv/src/sync/filters/batch_tracker.rs
- dash-spv/examples/spv_with_wallet.rs
- dash-spv-ffi/src/callbacks.rs
- key-wallet/src/test_utils/mod.rs
- dash-spv/src/client/core.rs
- key-wallet/src/test_utils/wallet.rs
- key-wallet-ffi/Cargo.toml
- dash-spv/src/client/events.rs
- key-wallet-ffi/tests/test_error_conversions.rs
Comment on lines
+1
to
+3
| # Missing Tests in key-wallet | ||
|
|
||
| ## 1. WalletManager Multi-Wallet Tests (`wallet_manager.rs`) | ||
| ## 1. WalletManager Multi-Wallet Tests |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where the manager-related files live after the crate move.
# Expected: either the listed filenames still exist under key-wallet,
# or the search shows the new module paths that this doc should reference.
fd 'wallet_manager\.rs|transaction_builder\.rs|utxo\.rs|coin_selection\.rs|fee\.rs|integration_tests\.rs' .
printf '\n--- manager module declarations/usages ---\n'
rg -n --type=rust -C2 '\bmod manager\b|\bpub mod manager\b|\bkey_wallet::manager\b|\bWalletManager\b'Repository: dashpay/rust-dashcore
Length of output: 50377
Verify that referenced module paths align with the new manager structure.
The files exist post-move under key-wallet, but are now organized as submodules within the manager and wallet hierarchies (e.g., wallet_manager.rs is now part of key_wallet::manager). Please confirm the checklist items reference the correct post-merge paths using the new module structure for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/missing_tests.md` around lines 1 - 3, Update the checklist
references to use the new module paths under the reorganized key-wallet crate:
replace any old top-level mentions of wallet_manager.rs or wallet.rs with their
post-move module paths (e.g., key_wallet::manager::wallet_manager or
key_wallet::wallet::wallet) and verify items that reference functions/structs
(such as WalletManager, Wallet, Manager::new, Wallet::load) use the fully
qualified names in the new hierarchy; scan the document for all occurrences of
old paths and update them to key_wallet::manager::* or key_wallet::wallet::* as
appropriate so the checklist matches the new module structure.
Comment on lines
+12
to
35
| mod events; | ||
| mod matching; | ||
| mod process_block; | ||
|
|
||
| pub use crate::wallet_manager::matching::{check_compact_filters_for_addresses, FilterMatchKey}; | ||
| mod wallet_interface; | ||
|
|
||
| pub use events::WalletEvent; | ||
| pub use matching::{check_compact_filters_for_addresses, FilterMatchKey}; | ||
| pub use wallet_interface::{BlockProcessingResult, WalletInterface}; | ||
|
|
||
| use crate::account::AccountCollection; | ||
| use crate::transaction_checking::TransactionContext; | ||
| use crate::wallet::managed_wallet_info::transaction_building::AccountTypePreference; | ||
| use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; | ||
| use crate::wallet::managed_wallet_info::{ManagedWalletInfo, TransactionRecord}; | ||
| use crate::Utxo; | ||
| use crate::{Account, AccountType, Address, ExtendedPrivKey, Mnemonic, Network, Wallet}; | ||
| use crate::{ExtendedPubKey, WalletCoreBalance}; | ||
| use alloc::collections::BTreeMap; | ||
| use alloc::string::String; | ||
| use alloc::vec::Vec; | ||
| use dashcore::blockdata::transaction::Transaction; | ||
| use dashcore::prelude::CoreBlockHeight; | ||
| use key_wallet::account::AccountCollection; | ||
| use key_wallet::transaction_checking::TransactionContext; | ||
| use key_wallet::wallet::managed_wallet_info::transaction_building::AccountTypePreference; | ||
| use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; | ||
| use key_wallet::wallet::managed_wallet_info::{ManagedWalletInfo, TransactionRecord}; | ||
| use key_wallet::wallet::WalletType; | ||
| use key_wallet::Utxo; | ||
| use key_wallet::{Account, AccountType, Address, ExtendedPrivKey, Mnemonic, Network, Wallet}; | ||
| use key_wallet::{ExtendedPubKey, WalletCoreBalance}; | ||
| use std::collections::BTreeSet; | ||
| use std::str::FromStr; |
Contributor
There was a problem hiding this comment.
Gate the moved manager module for std or remove the remaining unconditional std imports.
This module is now part of key-wallet, but it still pulls in std::collections::BTreeSet / std::str::FromStr while the rest of the file is clearly written to support alloc and #[cfg(feature = "std")]. That will break the crate's no_std build as soon as manager is compiled there.
Suggested fix
-use std::collections::BTreeSet;
-use std::str::FromStr;
+use alloc::collections::BTreeSet;
+use core::str::FromStr;As per coding guidelines, "Use conditional compilation with feature flags for optional features".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/manager/mod.rs` around lines 12 - 35, The file currently
imports std::collections::BTreeSet and std::str::FromStr unconditionally which
breaks no_std builds; either gate the entire manager module behind the "std"
feature by adding #[cfg(feature = "std")] at the top of this manager module, or
(preferably) add #[cfg(feature = "std")] to the two problematic imports (the
lines importing BTreeSet and FromStr) so those symbols are only pulled in when
feature = "std"; update any code that depends on those imports accordingly (look
for references to BTreeSet and FromStr in this module such as usages in
functions and types).
ZocoLini
force-pushed
the
refactor/manager-crate-moved
branch
from
d356618 to
2e685ae
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/manager/mod.rs (1)
205-286:⚠️ Potential issue | 🟠 MajorDo not export full wallets as raw bincode.
When
downgrade_to_pubkey_walletisfalse, Line 273 serializes the fullWallet, so this API still returns private key material as plain bytes. Please keep this API pubkey-only, or move full-wallet export behind an explicit encrypted-backup path instead.🔐 Minimal guard
pub fn create_wallet_from_mnemonic_return_serialized_bytes( &mut self, mnemonic: &str, passphrase: &str, birth_height: CoreBlockHeight, account_creation_options: crate::wallet::initialization::WalletAccountCreationOptions, downgrade_to_pubkey_wallet: bool, allow_external_signing: bool, ) -> Result<(Vec<u8>, WalletId), WalletError> { + if !downgrade_to_pubkey_wallet { + return Err(WalletError::InvalidParameter( + "serializing wallets with private key material is not supported".to_string(), + )); + } + use crate::wallet::WalletType; use zeroize::Zeroize;As per coding guidelines, "Never serialize or log private keys in production; use public keys or key fingerprints for identification instead".
♻️ Duplicate comments (2)
key-wallet/missing_tests.md (1)
1-3:⚠️ Potential issue | 🟡 MinorUpdate the checklist to match the post-move module layout.
After renaming this document to
key-wallet, the checklist still points to the old filename-centric layout (wallet_manager.rs,transaction_builder.rs, etc.), which no longer reflects the manager-module structure introduced by this PR. Please rewrite these entries to reference the currentkey_wallet::manager/ in-crate module locations so the doc stays actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/missing_tests.md` around lines 1 - 3, Update the checklist entries in this document to reference the new in-crate module layout instead of old filename-centric names: replace mentions of files like wallet_manager.rs and transaction_builder.rs with their module paths under key_wallet (e.g., key_wallet::manager::WalletManager, key_wallet::manager::TransactionBuilder or other types now under key_wallet::manager), adjust any test targets to point to the manager module tests, and ensure each checklist item maps to the current public API symbols (e.g., WalletManager, TransactionBuilder methods) so the checklist is actionable with the post-move module structure.key-wallet/src/lib.rs (1)
46-46:⚠️ Potential issue | 🟠 MajorGate
managerbehindstd.This still exposes
key_wallet::managerinno_stdbuilds even though the moved module depends onstd-only pieces. Please keep the public module behindfeature = "std"until the manager code is actuallyno_std-safe.🧩 Minimal fix
- pub mod manager; + #[cfg(feature = "std")] + pub mod manager;As per coding guidelines, "Use conditional compilation with feature flags for optional features".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/lib.rs` at line 46, The public module declaration `pub mod manager;` is exposed in no_std builds but depends on std; wrap that declaration with conditional compilation so it is only compiled when the "std" feature is enabled (use #[cfg(feature = "std")] on the `pub mod manager;` line), and add #[cfg_attr(docsrs, doc(cfg(feature = "std")))] if you want the docs to show the requirement; also make any uses or re-exports of `manager` conditional on the same feature so nothing references it in no_std builds.
🧹 Nitpick comments (2)
key-wallet/src/manager/process_block.rs (1)
1-5: Import paths correctly updated for crate-internal references.The import path changes align with the module restructuring. One minor style note: line 3 uses the full submodule path
transaction_router::TransactionRouter, whileTransactionRouteris re-exported directly fromcrate::transaction_checking(permod.rsline 14). This is consistent with howTransactionContextis imported on line 4.💡 Optional: Use re-exported path for consistency
-use crate::transaction_checking::transaction_router::TransactionRouter; +use crate::transaction_checking::TransactionRouter;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/manager/process_block.rs` around lines 1 - 5, The import of TransactionRouter should use the re-exported path for consistency with TransactionContext: replace the explicit submodule import of transaction_router::TransactionRouter with importing TransactionRouter directly from crate::transaction_checking (same module used for TransactionContext) so both types are imported via the same re-exported module; update the use statement importing TransactionRouter at the top of process_block.rs accordingly.key-wallet-ffi/src/wallet_manager.rs (1)
470-494: Avoid drifting from the canonicalWalletError→FFIErrorconversion.This partial match duplicates
key-wallet-ffi/src/error.rsand the_arm collapses any otherWalletErrorvariant into a generic wallet error. That makes the FFI contract drift as soon asWalletErrorchanges again.♻️ Possible simplification
Err(e) => { - // Convert the error to FFI error - match e { - key_wallet::manager::WalletError::WalletExists(_) => { - FFIError::set_error( - error, - FFIErrorCode::InvalidState, - "Wallet already exists in the manager".to_string(), - ); - } - key_wallet::manager::WalletError::InvalidParameter(msg) => { - FFIError::set_error( - error, - FFIErrorCode::SerializationError, - format!("Failed to deserialize wallet: {}", msg), - ); - } - _ => { - FFIError::set_error( - error, - FFIErrorCode::WalletError, - format!("Failed to import wallet: {:?}", e), - ); - } - } + let ffi_error = match e { + key_wallet::manager::WalletError::InvalidParameter(msg) => FFIError::error( + FFIErrorCode::SerializationError, + format!("Failed to deserialize wallet: {}", msg), + ), + other => other.into(), + }; + if !error.is_null() { + unsafe { + *error = ffi_error; + } + } false }🤖 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 470 - 494, This code duplicates and collapses WalletError mapping; instead of matching WalletError variants inline, call the canonical converter in key-wallet-ffi/src/error.rs that maps key_wallet::manager::WalletError into the proper FFIError/FFIErrorCode and sets the provided error pointer (i.e., delegate to the shared WalletError→FFI conversion helper rather than using the local match with FFIError::set_error and the generic `_` arm) so all WalletError variants remain covered centrally and future variants are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/tests/integration_test.rs`:
- Around line 6-7: The manager module is unconditionally exported but depends on
std; update the crate root so the manager module is only compiled when the std
feature is enabled by adding a cfg gate for feature = "std" before the pub mod
manager declaration (i.e., change the unconditional pub mod manager to be behind
#[cfg(feature = "std")]); also review uses of key_wallet::manager in integration
tests and either enable the std feature for those tests or gate the test imports
with #[cfg(feature = "std")] so references to WalletManager, WalletInterface,
and WalletError only occur when the std feature is active.
---
Duplicate comments:
In `@key-wallet/missing_tests.md`:
- Around line 1-3: Update the checklist entries in this document to reference
the new in-crate module layout instead of old filename-centric names: replace
mentions of files like wallet_manager.rs and transaction_builder.rs with their
module paths under key_wallet (e.g., key_wallet::manager::WalletManager,
key_wallet::manager::TransactionBuilder or other types now under
key_wallet::manager), adjust any test targets to point to the manager module
tests, and ensure each checklist item maps to the current public API symbols
(e.g., WalletManager, TransactionBuilder methods) so the checklist is actionable
with the post-move module structure.
In `@key-wallet/src/lib.rs`:
- Line 46: The public module declaration `pub mod manager;` is exposed in no_std
builds but depends on std; wrap that declaration with conditional compilation so
it is only compiled when the "std" feature is enabled (use #[cfg(feature =
"std")] on the `pub mod manager;` line), and add #[cfg_attr(docsrs,
doc(cfg(feature = "std")))] if you want the docs to show the requirement; also
make any uses or re-exports of `manager` conditional on the same feature so
nothing references it in no_std builds.
---
Nitpick comments:
In `@key-wallet-ffi/src/wallet_manager.rs`:
- Around line 470-494: This code duplicates and collapses WalletError mapping;
instead of matching WalletError variants inline, call the canonical converter in
key-wallet-ffi/src/error.rs that maps key_wallet::manager::WalletError into the
proper FFIError/FFIErrorCode and sets the provided error pointer (i.e., delegate
to the shared WalletError→FFI conversion helper rather than using the local
match with FFIError::set_error and the generic `_` arm) so all WalletError
variants remain covered centrally and future variants are handled consistently.
In `@key-wallet/src/manager/process_block.rs`:
- Around line 1-5: The import of TransactionRouter should use the re-exported
path for consistency with TransactionContext: replace the explicit submodule
import of transaction_router::TransactionRouter with importing TransactionRouter
directly from crate::transaction_checking (same module used for
TransactionContext) so both types are imported via the same re-exported module;
update the use statement importing TransactionRouter at the top of
process_block.rs accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef79b28b-5a34-4681-8dc1-b1f21d868c23
📒 Files selected for processing (66)
.codecov.yml.github/ci-groups.ymlCargo.tomlREADME.mddash-spv-ffi/Cargo.tomldash-spv-ffi/src/callbacks.rsdash-spv-ffi/src/client.rsdash-spv-ffi/tests/test_wallet_manager.rsdash-spv/Cargo.tomldash-spv/examples/filter_sync.rsdash-spv/examples/simple_sync.rsdash-spv/examples/spv_with_wallet.rsdash-spv/src/client/core.rsdash-spv/src/client/events.rsdash-spv/src/client/lifecycle.rsdash-spv/src/client/mempool.rsdash-spv/src/client/mod.rsdash-spv/src/client/queries.rsdash-spv/src/client/sync_coordinator.rsdash-spv/src/client/transactions.rsdash-spv/src/lib.rsdash-spv/src/main.rsdash-spv/src/sync/blocks/manager.rsdash-spv/src/sync/blocks/pipeline.rsdash-spv/src/sync/blocks/sync_manager.rsdash-spv/src/sync/events.rsdash-spv/src/sync/filters/batch.rsdash-spv/src/sync/filters/batch_tracker.rsdash-spv/src/sync/filters/manager.rsdash-spv/src/sync/filters/pipeline.rsdash-spv/src/sync/filters/sync_manager.rsdash-spv/src/sync/sync_coordinator.rsdash-spv/src/validation/filter.rsdash-spv/tests/dashd_sync/helpers.rsdash-spv/tests/dashd_sync/setup.rsdash-spv/tests/dashd_sync/tests_basic.rsdash-spv/tests/peer_test.rsdash-spv/tests/wallet_integration_test.rskey-wallet-ffi/Cargo.tomlkey-wallet-ffi/src/error.rskey-wallet-ffi/src/transaction.rskey-wallet-ffi/src/wallet_manager.rskey-wallet-ffi/src/wallet_manager_tests.rskey-wallet-ffi/tests/test_error_conversions.rskey-wallet-manager/Cargo.tomlkey-wallet-manager/README.mdkey-wallet-manager/src/lib.rskey-wallet-manager/src/test_utils/mod.rskey-wallet-manager/src/test_utils/wallet.rskey-wallet/Cargo.tomlkey-wallet/README.mdkey-wallet/TODO.mdkey-wallet/examples/wallet_creation.rskey-wallet/missing_tests.mdkey-wallet/src/lib.rskey-wallet/src/manager/events.rskey-wallet/src/manager/matching.rskey-wallet/src/manager/mod.rskey-wallet/src/manager/process_block.rskey-wallet/src/manager/wallet_interface.rskey-wallet/src/missing_tests.mdkey-wallet/src/test_utils/mod.rskey-wallet/src/test_utils/wallet.rskey-wallet/tests/integration_test.rskey-wallet/tests/spv_integration_tests.rskey-wallet/tests/test_serialized_wallets.rs
💤 Files with no reviewable changes (10)
- .github/ci-groups.yml
- key-wallet/src/missing_tests.md
- .codecov.yml
- dash-spv-ffi/Cargo.toml
- key-wallet-manager/src/test_utils/wallet.rs
- key-wallet-manager/Cargo.toml
- key-wallet-manager/src/test_utils/mod.rs
- key-wallet/TODO.md
- key-wallet-manager/README.md
- key-wallet-manager/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (20)
- dash-spv/src/sync/filters/pipeline.rs
- dash-spv-ffi/src/callbacks.rs
- dash-spv/src/lib.rs
- key-wallet/src/manager/events.rs
- key-wallet/src/test_utils/mod.rs
- dash-spv/src/client/transactions.rs
- key-wallet-ffi/Cargo.toml
- dash-spv/tests/wallet_integration_test.rs
- dash-spv/src/sync/filters/batch.rs
- key-wallet-ffi/src/transaction.rs
- dash-spv/src/validation/filter.rs
- key-wallet/examples/wallet_creation.rs
- Cargo.toml
- key-wallet/tests/spv_integration_tests.rs
- dash-spv/examples/filter_sync.rs
- dash-spv-ffi/tests/test_wallet_manager.rs
- dash-spv/src/main.rs
- dash-spv/src/client/lifecycle.rs
- key-wallet/src/test_utils/wallet.rs
- key-wallet-ffi/tests/test_error_conversions.rs
Comment on lines
+6
to
+7
| use key_wallet::manager::WalletInterface; | ||
| use key_wallet::manager::{WalletError, WalletManager}; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== key-wallet features =="
sed -n '/^\[features\]/,/^\[/p' key-wallet/Cargo.toml || true
echo
echo "== key-wallet crate attrs / manager export =="
rg -n -C3 '#!\[no_std\]|pub mod manager|mod manager|cfg\(feature *= *"std"\)|cfg\(not\(feature *= *"std"\)\)' \
key-wallet/src/lib.rs
echo
echo "== manager module std-only usage =="
fd -e rs . key-wallet/src | xargs rg -n -C1 'tokio|std::|std::sync|cfg\(feature *= *"std"\)|cfg\(not\(feature *= *"std"\)\)' | \
rg 'manager|wallet_manager|managed_wallet'Repository: dashpay/rust-dashcore
Length of output: 7975
Gate key_wallet::manager behind the std feature to preserve no_std support.
The manager module is exported unconditionally at key-wallet/src/lib.rs:46 (pub mod manager;), but contains unconditional std imports that break compilation when the std feature is disabled. For example, manager/mod.rs has unconditional use std::collections::BTreeSet and use std::str::FromStr, while manager/matching.rs has unconditional use std::collections::{BTreeSet, HashMap}. This contradicts the crate's stated no_std support (the std feature is optional and not required by default in some contexts).
Change line 46 in key-wallet/src/lib.rs to:
#[cfg(feature = "std")]
pub mod manager;This matches how other std-only functionality (like transaction building) is gated, and aligns with the coding guideline: "Use conditional compilation with feature flags for optional features."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/tests/integration_test.rs` around lines 6 - 7, The manager module
is unconditionally exported but depends on std; update the crate root so the
manager module is only compiled when the std feature is enabled by adding a cfg
gate for feature = "std" before the pub mod manager declaration (i.e., change
the unconditional pub mod manager to be behind #[cfg(feature = "std")]); also
review uses of key_wallet::manager in integration tests and either enable the
std feature for those tests or gate the test imports with #[cfg(feature =
"std")] so references to WalletManager, WalletInterface, and WalletError only
occur when the std feature is active.
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.
Just moved key-wallet-manager into the new key-wallet manager module.
Summary by CodeRabbit
Refactor
Chores
New Features
Documentation