feat: trait for wallet info #107
Conversation
WalkthroughMSRV raised from 1.80 to 1.89 across docs/config. Workspace blsful patch removed; dash-spv now pins blsful via git and adds key-wallet dependency. SPV wallet stack is generalized: WalletManager and SPVWalletManager introduced with WalletInfoInterface; examples/tests updated to construct SPVWalletManager via with_base. Several dash iterator APIs gain explicit lifetimes. Numerous test-only import/mutability cleanups. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant WM as WalletManager<ManagedWalletInfo>
participant SPV as SPVWalletManager<ManagedWalletInfo>
participant Client as DashSpvClient
App->>WM: WalletManager::<ManagedWalletInfo>::new()
App->>SPV: SPVWalletManager::with_base(WM)
App->>Client: DashSpvClient::new(storage, network, Arc<RwLock<SPV>>)
Client-->>SPV: sync/process
SPV-->>WM: wallet ops via WalletInfoInterface
sequenceDiagram
participant SPV as SPVWalletManager<T>
participant WM as WalletManager<T>
participant WI as WalletInfoInterface (T)
SPV->>WM: process_block(...)
WM->>WI: monitored_addresses()/utxos()/balance()
WM-->>SPV: updates/state via trait methods
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 11
🔭 Outside diff range comments (9)
dash/src/taproot.rs (1)
724-731: Doc typo: refer to ScriptLeaves<'> not TapTreeIter<'>The method returns ScriptLeaves<'>, but the doc mentions TapTreeIter<'>. Update the docs to avoid confusion.
Apply this diff to fix the docs:
- /// Returns [`TapTreeIter<'_>`] iterator for a taproot script tree, operating in DFS order over - /// tree [`ScriptLeaf`]s. + /// Returns [`ScriptLeaves<'_>`] iterator for a taproot script tree, operating in DFS order over + /// [`ScriptLeaf`]s.dash-spv/src/client/watch_manager_test.rs (5)
94-96: Remove needless clone of Address.
addris not reused; move it intoWatchItem::addressto satisfy clippy’sneedless_clone.Apply this diff:
- let item = WatchItem::address(addr.clone()); + let item = WatchItem::address(addr);
115-117: Remove needless clone of Address.
addrisn’t used after constructing the watch item; avoid cloning.- let item = WatchItem::address(addr.clone()); + let item = WatchItem::address(addr);
143-144: Remove needless clone of Address.Clippy will flag this; pass by move.
- let item = WatchItem::address(addr.clone()); + let item = WatchItem::address(addr);
213-215: Remove needless clone of Address.
addrisn’t reused after creatingitem.- let item = WatchItem::address(addr.clone()); + let item = WatchItem::address(addr);
243-245: Remove needless clone of Address for heighted watch item.Same rationale as above; move
addr.- let item = WatchItem::address_from_height(addr.clone(), 100000); + let item = WatchItem::address_from_height(addr, 100000);dash-spv/examples/filter_sync.rs (1)
19-27: Avoid hardcoded addresses in code; make watch address configurablePer repository guidelines, don’t hardcode addresses/keys. Read from CLI args or env var and validate network.
Example change using an env var:
- // Parse a Dash address to watch - let watch_address = Address::<dashcore::address::NetworkUnchecked>::from_str( - "Xan9iCVe1q5jYRDZ4VSMCtBjq2VyQA3Dge", - )?; + // Parse a Dash address to watch from WATCH_ADDRESS env var + let watch_address_str = std::env::var("WATCH_ADDRESS") + .map_err(|_| "Set WATCH_ADDRESS to a valid Dash address")?; + let watch_address = + Address::<dashcore::address::NetworkUnchecked>::from_str(&watch_address_str)?;And keep the require_network check below.
dash-spv/src/lib.rs (1)
1-1: Align workspace MSRV declarationsOnly dash-spv/Cargo.toml declares
rust-version = "1.89"; no other crate or a rootrust-toolchainfile pins the MSRV. Per our guidelines (MSRV = 1.80), please unify the workspace:
- dash-spv/Cargo.toml (line 9): change
- rust-version = "1.89" + rust-version = "1.80"- If you intend to bump MSRV to 1.89 for all crates, add a workspace‐level
rust-version = "1.89"(in the rootCargo.toml) or introduce arust-toolchain.tomlat the repo root, and update CI to use that toolchain.Ensure CI’s Rust toolchain matches the chosen MSRV across the entire workspace.
key-wallet-manager/src/wallet_manager/mod.rs (1)
184-234: Hardcoded mnemonic creates an insecure wallet (must not ship to production)create_wallet uses a fixed, well-known BIP39 mnemonic. Any wallet created by this function is trivially compromised. This violates the “no hardcoded keys” guideline and is a high-risk footgun.
Recommended options:
- Option A (preferred): Require a mnemonic and delegate to create_wallet_from_mnemonic, or provide a new create_wallet_random (uses secure randomness) behind std only.
- Option B: Gate the fixed mnemonic path behind cfg(any(test, feature = "dev-seed")), and make non-test builds fail fast.
Minimal change (gating) within this function:
- // For now, create a wallet with a fixed test mnemonic - // In production, you'd generate a random mnemonic or use new_random with proper features - let test_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; + // WARNING: Test-only mnemonic. Gate behind test/dev-seed to avoid shipping insecure wallets. + #[cfg(not(any(test, feature = "dev-seed")))] + { + return Err(WalletError::InvalidParameter( + "create_wallet with a fixed seed is disabled in non-test builds".to_string(), + )); + } + #[cfg(any(test, feature = "dev-seed"))] + let test_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about";Follow-up (optional): Add a secure constructor, e.g., create_wallet_random(..) using a CSPRNG where available.
🧹 Nitpick comments (34)
dash-spv/src/bloom/tests.rs (2)
611-620: Test always passes; make the assertion meaningful
assert!(result.is_ok() || result.is_err())is tautological and provides no signal. Decide on intended behavior for a 0.0 false-positive rate and assert that outcome.Options:
- If 0.0 should be rejected (recommended): assert is_err().
- If 0.0 should be clamped to a minimum internally: assert Ok and possibly validate resulting filter characteristics.
Example (reject case):
#[test] fn test_builder_zero_false_positive_rate() { let builder = BloomFilterBuilder::new().false_positive_rate(0.0); let result = builder.build(); assert!(result.is_err(), "false_positive_rate=0.0 should be rejected to avoid undefined behavior"); }
3-4: Avoid nestedtestsmodule to reduce module inceptionThis file is already
tests.rsunderbloom/, and then declaresmod tests { ... }, resulting inbloom::tests::tests::.... While harmless, it can confuse IDE navigation and coverage tooling and may trigger clippy’s module_inception lint.Consider removing the inner
mod testsand keeping the test items at the file root under a single#[cfg(test)].dash-spv/src/client/watch_manager_test.rs (1)
275-277: Nit: Drop redundant type annotation onaddresses.Type is evident from the initializer; this keeps the test concise and avoids potential clippy nags.
- let addresses: Vec<Address> = - vec![test_address(Network::Dash), test_address2(Network::Dash)]; + let addresses = vec![test_address(Network::Dash), test_address2(Network::Dash)];dash-spv/src/chain/reorg.rs (1)
541-545: Test imports updated — verifyHashis actually used
- Switching to
crate::storage::MemoryStoragealigns with current storage APIs.- The added
use dashcore_hashes::Hash;in this tests module might be redundant, as the tests don’t appear to call trait methods requiring it. If unused, remove to avoid clippy’s unused-import warning.Apply this diff if
Hashis unused here:use dashcore::Network; - use dashcore_hashes::Hash;key-wallet/src/tests/performance_tests.rs (2)
150-158: Verbose println! in tests — consider gating to reduce CI noiseThe additional prints are useful locally but can clutter CI logs. Consider using a verbosity flag/env var or a logging facade (e.g., tracing with env-filter) so these lines are quiet by default.
365-372: Transaction checking metrics: same note on println! noiseNice diagnostics. Same suggestion to gate or log at INFO/DEBUG to avoid noisy CI output.
dash-spv/src/sync/validation_test.rs (3)
1-4: Consider moving these “integration” tests under tests/ folderFile-level docstring says “Integration tests”, but these are unit tests under src guarded by #[cfg(test)]. Prefer placing integration tests under dash-spv/tests/ to align with repository testing conventions.
Note (from learnings): integration tests for network operations should live under dash-spv/tests/**.
252-255: Cache perf loop doesn’t exercise the validator; current test is a no-opYou create 10,000 BlockHash values but never call into the validator, so this measures loop overhead rather than cache performance. If there’s a public API intended to interact with the cache (e.g., a validate/check method), call it here; otherwise the test is misleading.
Would you like me to propose a minimal public helper on ChainLockValidator to drive cache insert/hit paths for this perf harness?
253-253: BlockHash creation collapses to 256 unique values
BlockHash::from([i as u8; 32])repeats every 256 iterations, which makes the dataset highly non-unique. If you intend to exercise cache churn, use a wider mapping.For example:
- let hash = BlockHash::from([i as u8; 32]); + let mut bytes = [0u8; 32]; + bytes[..8].copy_from_slice(&(i as u64).to_be_bytes()); + let hash = BlockHash::from(bytes);key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
151-157: Direct field access is fine; consider a read-only accessor for consistencyAccessing
self.accountsdirectly is OK here, but for consistency with other methods, a read-only accessor onManagedWalletInfocould make this impl uniform and more mockable in tests.
86-106: Doc comments are helpful; consider clarifying ownership semanticsMethods like get_utxos, get_balance, and get_transaction_history would benefit from short notes on whether they allocate/clone or return references. This helps implementors optimize and avoid unnecessary copies.
key-wallet-manager/tests/integration_test.rs (2)
96-105: Make the expected error explicit instead of returning earlyEarly returns make the test pass without verifying subsequent expectations. Prefer asserting the exact error kind so the intent is preserved.
Apply:
- if let Err(ref e) = address1 { - match e { - WalletError::InvalidNetwork => { - // This is expected given the current implementation - // The managed wallet info doesn't initialize account collections - return; - } - _ => panic!("Unexpected error: {:?}", e), - } - } + if let Err(ref e) = address1 { + assert!(matches!(e, WalletError::InvalidNetwork), "Unexpected error: {:?}", e); + return; + }Repeat similarly for the change address block below.
66-70: Brittle assertion on number of accounts“Asserting 10 accounts (9 default + 1 added)” tightly couples the test to current defaults. Consider asserting “>= 1” or checking properties (e.g., that index 1 exists) to reduce brittleness.
key-wallet-manager/src/lib.rs (1)
24-28: Module naming consistency: trait vs interfaceThis crate declares
pub mod wallet_info_trait, while the key-wallet crate introduceswallet_info_interface. Consider aligning names (trait vs interface) or re-exporting the interface here to avoid confusion for consumers.Optionally:
pub use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;dash-spv/examples/filter_sync.rs (1)
26-27: Handle network requirement without unwrap in exampleAvoid unwraps in examples to prevent abrupt exits; propagate the error with context instead.
- .watch_address(watch_address.clone().require_network(Network::Dash).unwrap()) + .watch_address( + watch_address + .clone() + .require_network(Network::Dash) + .map_err(|_| "WATCH_ADDRESS is not a mainnet Dash address")?, + )dash-spv/tests/header_sync_test.rs (1)
311-314: SPVWalletManager initialization now correctly uses with_baseThis aligns with the new API and keeps the wallet wrapped in Arc<RwLock<...>> as expected by DashSpvClient.
Optionally make the generic explicit on SPVWalletManager to avoid relying on a type alias/default generic (if any):
- let wallet = Arc::new(RwLock::new(SPVWalletManager::with_base(WalletManager::< - ManagedWalletInfo, - >::new()))); + let wallet = Arc::new(RwLock::new(SPVWalletManager::<ManagedWalletInfo>::with_base( + WalletManager::<ManagedWalletInfo>::new(), + )));dash-spv/tests/integration_real_node_test.rs (1)
42-44: with_base construction is consistent and correctThe wallet is now composed from a base WalletManager. This should integrate cleanly with DashSpvClient.
If SPVWalletManager doesn’t have a default type parameter or type alias, consider making the specialization explicit:
- let wallet = Arc::new(RwLock::new(SPVWalletManager::with_base(WalletManager::< - ManagedWalletInfo, - >::new()))); + let wallet = Arc::new(RwLock::new(SPVWalletManager::<ManagedWalletInfo>::with_base( + WalletManager::<ManagedWalletInfo>::new(), + )));Additionally, ensure the DashSpvClient type alias/definition continues to accept SPVWalletManager without an explicit generic parameter in the return type of create_test_client.
dash-spv/tests/instantsend_integration_test.rs (1)
34-34: Missing imports for WalletManager and ManagedWalletInfo in this fileThe updated construction references WalletManager and ManagedWalletInfo but they aren’t imported here. While the file is gated by cfg(skip_instantsend_tests) and won’t compile, it’s safer to add the imports now to keep the file ready when re-enabled.
Proposed additions near the other use statements at the top of this file:
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; use key_wallet_manager::wallet_manager::WalletManager;Want me to open a follow-up to update these tests to the new SPVWalletManager API once the required wallet methods are finalized?
key-wallet/src/wallet/managed_wallet_info/mod.rs (6)
124-149: Use saturating_add when summing balances to avoid debug overflowsThe current additions (+=) on u64 can panic in debug builds and wrap in release builds if a pathological sum is reached. While practically unlikely, using saturating_add is a safe, zero-cost improvement and silences potential clippy/panic concerns.
Apply this diff inside the loop:
- if utxo.is_locked { - locked += value; - } else if utxo.is_confirmed { - confirmed += value; - } else { - unconfirmed += value; - } + if utxo.is_locked { + locked = locked.saturating_add(value); + } else if utxo.is_confirmed { + confirmed = confirmed.saturating_add(value); + } else { + unconfirmed = unconfirmed.saturating_add(value); + }
156-180: Mirror the saturating_add fix in calculate_balanceSame rationale as update_balance: prefer saturating_add in this verification path to avoid inconsistent behavior between debug/release and to keep clippy happy.
- if utxo.is_locked { - locked += value; - } else if utxo.is_confirmed { - confirmed += value; - } else { - unconfirmed += value; - } + if utxo.is_locked { + locked = locked.saturating_add(value); + } else if utxo.is_confirmed { + confirmed = confirmed.saturating_add(value); + } else { + unconfirmed = unconfirmed.saturating_add(value); + }
197-208: BTreeSet<&Utxo> requires Utxo: Ord; consider returning OutPoint or deduping differentlyUsing BTreeSet<&Utxo> requires Ord for &Utxo (derived from Utxo: Ord). If Utxo does not implement Ord, this will not compile. Even if it does, dedup semantics will follow Utxo’s Ord implementation rather than object identity. If you only need a unique list, consider collecting OutPoints or returning a Vec<&Utxo> and deduping by OutPoint.
If you choose to return OutPoints:
- pub fn get_utxos(&self) -> BTreeSet<&Utxo> { - let mut utxos = BTreeSet::new(); + pub fn get_utxos(&self) -> BTreeSet<crate::OutPoint> { + let mut utxos = BTreeSet::new(); // Collect UTXOs from all accounts across all networks for collection in self.accounts.values() { for account in collection.all_accounts() { - utxos.extend(account.utxos.values()); + utxos.extend(account.utxos.keys().copied()); } } utxos }Follow-up: get_spendable_utxos would then map those OutPoints back to references if needed, or you can keep the current signature and switch to Vec<&Utxo> with stable ordering for callers that require determinism.
248-256: Avoid lossy u64 → i64 casts for amountsCasting with
as i64wraps on overflow. While Dash amounts fit in i64, usingi64::try_from(or saturating conversion) communicates intent and avoids future pitfalls if the code is reused with different units.- tx.total_received as i64, + i64::try_from(tx.total_received).unwrap_or(i64::MAX),Repeat at the other two call sites in this method.
Also applies to: 266-273, 281-289
330-341: Optionally dedup monitored addressesDepending on how account.get_all_addresses() is implemented, you may gather duplicates. If you expect uniqueness, consider deduping via a set then collecting back to Vec to avoid redundant watch entries downstream.
Example:
- let mut addresses = Vec::new(); + use alloc::collections::BTreeSet; + let mut set = BTreeSet::new(); if let Some(collection) = self.accounts.get(&network) { for account in collection.all_accounts() { - addresses.extend(account.get_all_addresses()); + set.extend(account.get_all_addresses()); } } - addresses + set.into_iter().collect()
146-149: Comment is misleading about overflow handlingThe comment says “ignoring overflow errors,” but any overflow would happen during the preceding u64 additions, not in WalletBalance::new. After adopting saturating_add (suggested above), adjust the comment to reflect that.
dash-spv/tests/simple_header_test.rs (1)
17-17: Consider making the node address configurable via env for CI stabilityHardcoding
127.0.0.1:9999is fine locally, but configurable via env (with a sane default) improves CI portability and avoids accidental failures.-const DASH_NODE_ADDR: &str = "127.0.0.1:9999"; +const DASH_NODE_ADDR: &str = env!("DASH_NODE_ADDR_DEFAULT"); // set in build.rs or: +// Alternatively, read at runtime: +// const DEFAULT_ADDR: &str = "127.0.0.1:9999"; +// let addr = std::env::var("DASH_NODE_ADDR").unwrap_or_else(|_| DEFAULT_ADDR.to_string());key-wallet-manager/tests/spv_integration_tests.rs (2)
73-82: Prefer asserting wallet creation results instead of discarding with .ok()Dropping errors hides regressions. In tests, assert success to catch issues early.
- spv.base.create_wallet(wallet_id, "Test Wallet".to_string(), Network::Testnet).ok(); + spv.base + .create_wallet(wallet_id, "Test Wallet".to_string(), Network::Testnet) + .expect("create_wallet should succeed");Apply similarly at other occurrences in this file.
140-159: Queue operations: minimal check is fine; consider asserting resulting queue length for stronger guaranteesThe current assertions are correct. Optionally, add an assertion about overall queue size/state after each operation to tighten the contract and preempt off-by-one regressions.
dash-spv/tests/chainlock_simple_test.rs (1)
55-57: Adopts SPVWalletManager::with_base(WalletManager::::new()) consistently.Construction aligns with the new generic API and keeps the Arc<RwLock<...>> wrapping intact. Consider a small helper to DRY this across tests.
You could factor wallet creation into a small helper in this test module:
fn make_test_wallet() -> Arc<RwLock<SPVWalletManager>> { Arc::new(RwLock::new(SPVWalletManager::with_base( WalletManager::<ManagedWalletInfo>::new(), ))) }Also applies to: 107-109
dash-spv/tests/multi_peer_test.rs (1)
213-216: Assertion message contradicts the expectation.You assert MAX_PEERS == 3 but the message says “Default max peers should be 12”. Fix the message or the expectation to avoid confusion.
Apply this diff to align the message with the assertion:
- assert_eq!(MAX_PEERS, 3, "Default max peers should be 12"); + assert_eq!(MAX_PEERS, 3, "Default max peers should be 3");If the intended default is actually 12, change the assertion to 12 and adjust upstream config accordingly.
dash-spv/src/main.rs (2)
226-230: Hardcoded example addresses in the CLI should be gated or externalized.Guidelines require avoiding hardcoded network addresses/keys. The “--add-example-addresses” block hardcodes addresses in this binary. Consider a feature-gated dev-only path (e.g., cfg(feature = "dev-examples")) or loading from a file/env to keep production clean.
A minimal approach is to gate the argument and the “example addresses” block behind a cargo feature:
In Cargo.toml:
[features]
dev-examples = []Around the arg definition and the example-addresses block:
#[cfg(feature = "dev-examples")]
{ … }Alternatively, read addresses from a JSON file in the data-dir when the flag is provided.
226-230: Inconsistent height text for the example-address override.Elsewhere in this file, the code uses 200_000 for address_from_height but the log string says “from height 1,000,000”. Make these consistent.
Update the string literal to 200,000 (or change the numeric to 1_000_000) to match intent. For example:
- " (from height 1,000,000)" + " (from height 200,000)"dash-spv/tests/wallet_integration_test.rs (1)
66-67: Consider avoiding direct field access to base for better encapsulationAccessing wallet_manager.base leaks the wrapper’s internals. Prefer forwarding methods or implementing AsRef/AsMut/Deref for SPVWalletManager to expose selected functionality.
For example, add these on SPVWalletManager (no breaking changes; tests can then use methods instead of touching base):
impl<T: WalletInfoInterface> SPVWalletManager<T> { pub fn wallet_count(&self) -> usize { self.base.wallet_count() } pub fn total_balance(&self) -> u64 { self.base.get_total_balance() } }Then in the test:
assert_eq!(wallet_manager.wallet_count(), 0); let balance = wallet_manager.total_balance();key-wallet-manager/src/spv_wallet_manager.rs (1)
35-38: Public base field leaks internal representationMaking base public invites tight coupling and breaks invariants if you later need to enforce SPV-specific behavior around wallet operations.
Option A (non-breaking): Add AsRef/AsMut and forwarding methods, and start migrating call sites away from direct field access.
impl<T: WalletInfoInterface> AsRef<WalletManager<T>> for SPVWalletManager<T> { fn as_ref(&self) -> &WalletManager<T> { &self.base } } impl<T: WalletInfoInterface> AsMut<WalletManager<T>> for SPVWalletManager<T> { fn as_mut(&mut self) -> &mut WalletManager<T> { &mut self.base } }Option B (breaking): Make base private and provide targeted APIs on SPVWalletManager. Consider this post-merge to minimize disruption.
key-wallet-manager/src/wallet_manager/mod.rs (1)
21-21: Unused import likely to trip clippy -D warningsWalletTransactionChecker is imported but not used in this module.
-use key_wallet::transaction_checking::{TransactionContext, WalletTransactionChecker}; +use key_wallet::transaction_checking::TransactionContext;
| @@ -1 +1 @@ | |||
| msrv = "1.80" | |||
| msrv = "1.89" | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
MSRV bump in clippy config looks good; verify workspace and CI alignment.
The clippy MSRV setting matches the PR’s stated bump to 1.89. Please ensure all crates’ rust-version fields and CI toolchains are updated accordingly.
Run this script to verify MSRV consistency across the repo and surface stragglers (old 1.29/1.80 mentions, CI pins, etc.):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check Cargo.toml rust-version fields =="
rg -nP '^\s*rust-version\s*=\s*".*"' -g 'Cargo.toml' -S -C2 || true
rg -nP '^\s*rust-version\s*=\s*".*"' -S -C2 --glob '!**/target/**' --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' --type toml || true
echo
echo "== Expect rust-version = \"1.89\" everywhere =="
rg -nP '^\s*rust-version\s*=\s*"1\.(29|80)"' --type toml -C2 || echo "OK: no rust-version set to 1.29/1.80 found"
echo
echo "== Check clippy.toml MSRV =="
rg -n '^\s*msrv\s*=\s*".*"' clippy.toml -C1 || true
echo
echo "== Find stale MSRV mentions in docs (1.29 or 1.80) =="
rg -nP '\b1\.(29|80)\b' README.md CLAUDE.md dash-spv/CLAUDE.md -C2 || echo "OK: no stale MSRV mentions in primary docs"
echo
echo "== Check CI workflow pins (toolchain, rust-version) =="
fd -HI --type f '.ya?ml' .github | xargs -r rg -nP '(toolchain|rust-version|rust:)'Length of output: 3917
MSRV updates verified; docs and CI need alignment
- Rust-version fields: dash-spv/Cargo.toml is updated to 1.89; no other crates declare a rust-version field.
- Docs: README.md still references “Rust 1.29” (badge at line 15 and MSRV section at lines 105–109). Update to “1.89”.
- CI workflows: .github/workflows/rust.yml (matrix entry
rust: 1.85.0) and fuzz.yml (toolchain: 1.85) should be bumped to 1.89 or include an explicit 1.89 job.
🤖 Prompt for AI Agents
In clippy.toml around lines 1-1, update project documentation and CI to match
the new MSRV: change the README.md Rust badge text at line 15 and the MSRV
section content at lines 105–109 from “1.29” to “1.89”, update
.github/workflows/rust.yml matrix entry `rust: 1.85.0` to `1.89` (or add a 1.89
job), and update .github/workflows/fuzz.yml `toolchain: 1.85` to `1.89`;
optionally add `rust-version = "1.89"` to other crates' Cargo.toml files if they
should advertise the MSRV, then run CI to verify the changes.
| let config = ValidationConfig::default(); | ||
| let mut engine = ValidationEngine::new(config); | ||
| let engine = ValidationEngine::new(config); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unused variable to satisfy clippy -D warnings
engine is created and never used. This will trip clippy with -D warnings. Either prefix with _ or remove it.
Apply:
- let engine = ValidationEngine::new(config);
+ let _engine = ValidationEngine::new(config);📝 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.
| let config = ValidationConfig::default(); | |
| let mut engine = ValidationEngine::new(config); | |
| let engine = ValidationEngine::new(config); | |
| let config = ValidationConfig::default(); | |
| let _engine = ValidationEngine::new(config); |
🤖 Prompt for AI Agents
In dash-spv/src/sync/validation_test.rs around lines 221 to 223, the local
variable `engine` is created but never used which triggers Clippy -D warnings;
fix by either removing the unused binding entirely or renaming it to `_engine`
(or `_`) so the compiler treats it as intentionally unused; update the line `let
engine = ValidationEngine::new(config);` accordingly and run tests/clippy to
confirm the warning is resolved.
| pub mod spv_wallet_manager; | ||
| pub mod wallet_info_trait; | ||
| pub mod wallet_interface; | ||
| pub mod wallet_manager; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the module exists and is included in Cargo
Ensure key-wallet-manager/src/wallet_info_trait.rs (or a folder module) exists and is compiled; otherwise this will fail to build.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the module file(s)
fd -a '^wallet_info_trait(\.rs|/mod\.rs)?$' key-wallet-manager/src -0 | xargs -0 -I{} sh -c 'echo "Found: {}"; sed -n "1,80p" "{}" || true'Length of output: 163
I couldn’t find any wallet_info_trait definitions or files. Let’s list all Rust sources and hunt for any “wallet_info” mentions to confirm:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) All .rs files under key-wallet-manager/src:"
fd --type f --extension rs key-wallet-manager/src
echo
echo "2) Any occurrences of “wallet_info” in code or filenames:"
fd -a 'wallet_info' key-wallet-manager/src || true
rg -n 'wallet_info' key-wallet-manager/src || trueLength of output: 568
Let’s use a more reliable file‐system scan to locate any wallet_info_trait module definitions:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) All .rs files under key-wallet-manager/src:"
find key-wallet-manager/src -type f -name '*.rs'
echo
echo "2) Any occurrences of 'wallet_info_trait' in filenames:"
find key-wallet-manager/src -type f -name '*wallet_info_trait*' || true
echo
echo "3) Any directories named 'wallet_info_trait':"
find key-wallet-manager/src -type d -name 'wallet_info_trait' || true
echo
echo "4) Any occurrences of 'wallet_info_trait' in file contents:"
rg -n 'wallet_info_trait' key-wallet-manager/src || trueLength of output: 1504
Missing wallet_info_trait module – build will fail
The declaration
- key-wallet-manager/src/lib.rs:25
pub mod wallet_info_trait;
is not backed by a file or folder (src/wallet_info_trait.rs or src/wallet_info_trait/mod.rs). Yet this module is imported in several places:
- key-wallet-manager/src/spv_wallet_manager.rs:18
- key-wallet-manager/src/wallet_manager/process_block.rs:1
- key-wallet-manager/src/wallet_manager/transaction_building.rs:4
- key-wallet-manager/src/wallet_manager/mod.rs:19
Please add the missing module file (e.g. src/wallet_info_trait.rs defining WalletInfoInterface) or adjust/remove these references so the crate compiles.
🤖 Prompt for AI Agents
In key-wallet-manager/src/lib.rs around lines 24 to 28, the declared module `pub
mod wallet_info_trait;` is missing its backing file, causing build failures
where that module is referenced; add a new file `src/wallet_info_trait.rs` (or
`src/wallet_info_trait/mod.rs`) that defines the expected `WalletInfoInterface`
trait and any types/functions other modules import, and export them with the
proper `pub` visibility, or alternatively remove/adjust all references to
`wallet_info_trait` in the listed files to point to an existing module; ensure
Cargo builds after updating imports/paths.
| use crate::wallet_info_trait::WalletInfoInterface; | ||
| use crate::wallet_interface::WalletInterface; | ||
| use crate::wallet_manager::{WalletId, WalletManager}; | ||
| use key_wallet::transaction_checking::TransactionContext; | ||
| use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; | ||
| use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; | ||
|
|
There was a problem hiding this comment.
Duplicate WalletInfoInterface imports cause name collision
You import WalletInfoInterface twice from different paths. This will shadow/collide and prevent compilation.
Keep only one source of truth. Prefer the crate-level re-export to decouple from the deep key_wallet path.
-use crate::wallet_info_trait::WalletInfoInterface;
+use crate::wallet_info_trait::WalletInfoInterface;
@@
-use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo;If the crate re-export isn’t guaranteed, alternatively remove the crate import and keep the key_wallet path, but do not keep both.
📝 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.
| use crate::wallet_info_trait::WalletInfoInterface; | |
| use crate::wallet_interface::WalletInterface; | |
| use crate::wallet_manager::{WalletId, WalletManager}; | |
| use key_wallet::transaction_checking::TransactionContext; | |
| use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; | |
| use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; | |
| use crate::wallet_info_trait::WalletInfoInterface; | |
| use crate::wallet_interface::WalletInterface; | |
| use crate::wallet_manager::{WalletId, WalletManager}; | |
| use key_wallet::transaction_checking::TransactionContext; | |
| use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; |
🤖 Prompt for AI Agents
In key-wallet-manager/src/spv_wallet_manager.rs around lines 18 to 24, there are
two imports of WalletInfoInterface from different paths which collide; remove
the duplicate so only one source-of-truth import remains—prefer the crate-level
re-export (use crate::wallet_info_trait::WalletInfoInterface) and delete the
deep key_wallet path import, or if the crate re-export is not available keep
only use
key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
ensure any references compile after removing the other import.
| //! Transaction building functionality for the wallet manager | ||
| use super::{WalletError, WalletId, WalletManager}; | ||
| use crate::wallet_info_trait::WalletInfoInterface; |
There was a problem hiding this comment.
Duplicate import of WalletInfoInterface; keep the crate-local one.
Two imports of the same trait under the same name will shadow and cause compile errors. Keep the trait from crate::wallet_info_trait to decouple from key_wallet internals.
Apply this diff:
use super::{WalletError, WalletId, WalletManager};
use crate::wallet_info_trait::WalletInfoInterface;
@@
-use key_wallet::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;Also applies to: 10-10
🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_manager/transaction_building.rs around line 4
(and also check line 10), there is a duplicate import of WalletInfoInterface;
remove the duplicate that imports the same trait from the external key_wallet
path and keep the crate-local import use
crate::wallet_info_trait::WalletInfoInterface; — delete the other use statement
(at or near line 10) so only the crate-local trait is imported and there are no
shadowing/duplicate imports.
| use key_wallet_manager::spv_wallet_manager::{SPVSyncStatus, SPVWalletManager}; | ||
| use key_wallet_manager::wallet_interface::WalletInterface; | ||
| use key_wallet_manager::wallet_manager::WalletId; | ||
| use key_wallet_manager::wallet_manager::{WalletId, WalletManager}; |
There was a problem hiding this comment.
Unused import will fail with -D warnings
use key_wallet_manager::wallet_interface::WalletInterface; is unused and will cause CI failures if clippy is run with -D warnings (as per guidelines). Remove it.
- use key_wallet_manager::wallet_interface::WalletInterface;📝 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.
| use key_wallet_manager::wallet_manager::{WalletId, WalletManager}; | |
| use key_wallet_manager::wallet_manager::{WalletId, WalletManager}; |
🤖 Prompt for AI Agents
In key-wallet-manager/tests/spv_integration_tests.rs around line 15, the import
use key_wallet_manager::wallet_interface::WalletInterface; is unused and will
cause CI failures under -D warnings; remove that unused import line (leave only
the used imports such as WalletId and WalletManager) so the test file compiles
without unused-import warnings.
| fn update_last_synced(&mut self, timestamp: u64) { | ||
| self.update_last_synced(timestamp); | ||
| } | ||
|
|
||
| fn monitored_addresses(&self, network: Network) -> Vec<DashAddress> { | ||
| self.monitored_addresses(network).into_iter().collect() | ||
| } | ||
|
|
||
| fn get_utxos(&self) -> Vec<Utxo> { | ||
| self.get_utxos().into_iter().cloned().collect() | ||
| } | ||
|
|
||
| fn get_balance(&self) -> WalletBalance { | ||
| self.get_balance() | ||
| } | ||
|
|
||
| fn update_balance(&mut self) { | ||
| self.update_balance(); | ||
| } | ||
|
|
||
| fn get_transaction_history(&self) -> Vec<&TransactionRecord> { | ||
| self.get_transaction_history() | ||
| } |
There was a problem hiding this comment.
Forwarders risk unintended recursion; use UFCS to call inherent methods
Several impl methods call same-named methods on self. While Rust prefers inherent methods over trait methods, this pattern is brittle and easy to regress into infinite recursion if any inherent method is renamed/removed. Use fully qualified syntax to explicitly call the inherent ManagedWalletInfo methods and remove redundant collect/cloned calls.
Apply:
fn update_last_synced(&mut self, timestamp: u64) {
- self.update_last_synced(timestamp);
+ ManagedWalletInfo::update_last_synced(self, timestamp);
}
fn monitored_addresses(&self, network: Network) -> Vec<DashAddress> {
- self.monitored_addresses(network).into_iter().collect()
+ ManagedWalletInfo::monitored_addresses(self, network)
}
fn get_utxos(&self) -> Vec<Utxo> {
- self.get_utxos().into_iter().cloned().collect()
+ // If the inherent method returns &Vec<Utxo>, clone the Vec.
+ // If it returns Vec<Utxo>, this still compiles (Utxo must implement Clone).
+ ManagedWalletInfo::get_utxos(self).clone()
}
fn get_balance(&self) -> WalletBalance {
- self.get_balance()
+ ManagedWalletInfo::get_balance(self)
}
fn update_balance(&mut self) {
- self.update_balance();
+ ManagedWalletInfo::update_balance(self);
}
fn get_transaction_history(&self) -> Vec<&TransactionRecord> {
- self.get_transaction_history()
+ ManagedWalletInfo::get_transaction_history(self)
}
fn create_unsigned_payment_transaction(
&mut self,
wallet: &Wallet,
network: Network,
account_index: u32,
account_type_pref: Option<AccountTypePreference>,
recipients: Vec<(Address, u64)>,
fee_level: FeeLevel,
current_block_height: u32,
) -> Result<Transaction, TransactionError> {
- self.create_unsigned_payment_transaction(
- wallet,
- network,
- account_index,
- account_type_pref,
- recipients,
- fee_level,
- current_block_height,
- )
+ ManagedWalletInfo::create_unsigned_payment_transaction(
+ self,
+ wallet,
+ network,
+ account_index,
+ account_type_pref,
+ recipients,
+ fee_level,
+ current_block_height,
+ )
}Additionally, consider aligning the trait return types exactly with the inherent methods (e.g., Vec vs &Vec) to avoid extra clones and to satisfy clippy’s “needless_collect”/“needless_clone”.
Also applies to: 159-178
🤖 Prompt for AI Agents
In key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs around
lines 127 to 149 (and similarly for 159-178), several trait impl methods
currently forward to same-named methods on self which causes unintended
recursion; change those calls to use UFCS (e.g., <ManagedWalletInfo as
ManagedWalletInfo>::method(self, ...)) to explicitly invoke the type’s inherent
methods, and at the same time remove redundant .into_iter().collect()/.cloned()
wrappers by matching the trait return types to the inherent method signatures
(or adjust the inherent methods to return the trait-expected type) so no
needless_clones or needless_collects are necessary. Ensure mutable methods take
&mut self in the UFCS call and that return types are aligned to avoid extra
allocations.
| ## Minimum Supported Rust Version (MSRV) | ||
|
|
||
| This library should always compile with any combination of features on **Rust 1.80**. | ||
| This library should always compile with any combination of features on **Rust 1.89**. |
There was a problem hiding this comment.
Inconsistent MSRV in README: 1.89 here, but earlier section still says 1.29 and the badge shows 1.29+.
Fix the conflicting “Minimum Supported Rust Version (MSRV)” earlier in the README and update/remove the rustc 1.29+ badge to avoid misleading users.
Apply this change within the edited section to keep intent clear:
-This library should always compile with any combination of features on **Rust 1.89**.
+This library should always compile with any combination of features on **Rust 1.89**.Additionally, adjust the earlier MSRV section and badge (outside this hunk). Suggested updates:
- Replace the historical 1.29 MSRV section with a short note pointing to the new MSRV and remove the old dependency pinning commands that are no longer relevant.
- Update the rustc version badge from 1.29+ to 1.89+ or remove it if it cannot be kept current.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In README.md around line 137, the MSRV statement conflicts with earlier content
that still references Rust 1.29 and a rustc badge showing 1.29+; update the
earlier MSRV section and badge to match the new MSRV (Rust 1.89) and remove any
obsolete dependency pinning commands. Replace the historical 1.29 MSRV paragraph
with a short note directing readers to the current MSRV (1.89) and delete the
old cargo/rustup pin commands that no longer apply, and update the rustc badge
to 1.89+ (or remove the badge if it cannot be reliably maintained) so the README
is consistent.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs (1)
42-51: Add a deprecated public wrapper and update all remaining call sitesWe still have references to the old
create_unsigned_payment_transactionAPI in the following locations—these must either be updated to call the new internal method directly or routed through a deprecated public wrapper to avoid breaking downstream consumers:
- key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (line 93, 322)
- key-wallet-manager/src/wallet_manager/transaction_building.rs (lines 18, 37)
To smooth the transition, add this wrapper alongside the internal method:
impl ManagedWalletInfo { #[deprecated( note = "public API moved to WalletManager<T>; use create_unsigned_payment_transaction_internal" )] pub fn create_unsigned_payment_transaction( &mut self, wallet: &Wallet, network: Network, account_index: u32, account_type_pref: Option<AccountTypePreference>, recipients: Vec<(Address, u64)>, fee_level: FeeLevel, current_block_height: u32, ) -> Result<Transaction, TransactionError> { self.create_unsigned_payment_transaction_internal( wallet, network, account_index, account_type_pref, recipients, fee_level, current_block_height, ) } }Then update the calls in the interface and manager modules to either use this wrapper or invoke the new internal method directly.
key-wallet-manager/src/wallet_manager/mod.rs (1)
193-207: Remove hardcoded test mnemonic from a public API (security risk)Shipping a fixed mnemonic in library code violates “no hardcoded keys” and risks accidental fund loss.
Suggested options:
- Accept a mnemonic parameter for this constructor, or
- Generate a random mnemonic using a secure RNG behind a feature gate, or
- Gate the test mnemonic behind
#[cfg(any(test, feature = "dev-mnemonic"))]and return an error otherwise.Example (minimal gating):
- // For now, create a wallet with a fixed test mnemonic - // In production, you'd generate a random mnemonic or use new_random with proper features - let test_mnemonic = "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about"; - let mnemonic = - Mnemonic::from_phrase(test_mnemonic, key_wallet::mnemonic::Language::English) - .map_err(|e| WalletError::WalletCreation(e.to_string()))?; + #[cfg(any(test, feature = "dev-mnemonic"))] + let mnemonic = Mnemonic::from_phrase( + "abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about", + key_wallet::mnemonic::Language::English + ).map_err(|e| WalletError::WalletCreation(e.to_string()))?; + #[cfg(not(any(test, feature = "dev-mnemonic")))] + return Err(WalletError::InvalidParameter( + "create_wallet requires a provided mnemonic or feature 'dev-mnemonic'".to_string() + ));If you prefer, I can draft a secure RNG path using whatever API
key_wallet::Mnemonicexposes (e.g., new_random).
♻️ Duplicate comments (2)
key-wallet-manager/src/spv_wallet_manager.rs (1)
21-23: Imports look consistent; duplicate trait import issue resolved.Only one WalletInfoInterface import remains, avoiding the earlier collision reported in prior reviews.
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
19-19: Trait should be Send + Sync to be usable across threads/async contextsWallet info types are shared across async tasks in the SPV manager. Add
Send + Syncbounds at the trait boundary.-pub trait WalletInfoInterface: Sized + WalletTransactionChecker { +pub trait WalletInfoInterface: Sized + WalletTransactionChecker + Send + Sync {
🧹 Nitpick comments (11)
key-wallet/src/wallet/mod.rs (1)
398-398: Correct: Updated set_description to accept Option.This matches the new API. Consider adding a small test case to verify clearing the description (set_description(None)) updates state as expected.
key-wallet-manager/src/wallet_manager/process_block.rs (1)
6-9: Generic impl over T: WalletInfoInterface looks correct.
- Importing WalletInfoInterface and constraining the impl enables trait-backed wallet info types.
- The change is minimal and aligns with the trait-based architecture.
Optional: if you re-export WalletInfoInterface at the crate level, prefer importing from the local re-export to reduce deep path coupling.
key-wallet-manager/src/spv_wallet_manager.rs (2)
103-123: Drop unnecessary T: Default bound on new() for broader usability.new() doesn’t use T::default; requiring T: Default is an unnecessary restriction and can preclude using non-Default wallet info types (e.g., ManagedWalletInfo likely isn’t Default).
Apply this diff:
- /// Create a new SPV wallet manager (only available when T implements Default) - pub fn new() -> Self - where - T: Default, - { - Self::with_base(WalletManager::new()) - } + /// Create a new SPV wallet manager + pub fn new() -> Self { + Self::with_base(WalletManager::new()) + }
216-220: Align Default impl with new(): remove T: Default bound.Since new() doesn’t require T: Default after the above change, make Default available for all T: WalletInfoInterface.
Apply this diff:
-impl<T: WalletInfoInterface + Default> Default for SPVWalletManager<T> { +impl<T: WalletInfoInterface> Default for SPVWalletManager<T> { fn default() -> Self { Self::new() } }key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (4)
150-161: Optional deduplication of monitored addressesIf accounts can produce overlapping monitored addresses, consider returning a unique set to avoid redundant network queries.
- fn monitored_addresses(&self, network: Network) -> Vec<DashAddress> { - let mut addresses = Vec::new(); + fn monitored_addresses(&self, network: Network) -> Vec<DashAddress> { + use alloc::collections::BTreeSet; + let mut set = BTreeSet::new(); if let Some(collection) = self.accounts.get(&network) { // Collect from all accounts using the account's get_all_addresses method for account in collection.all_accounts() { - addresses.extend(account.get_all_addresses()); + set.extend(account.get_all_addresses()); } } - addresses + set.into_iter().collect() }
207-210: Don’t silently mask overflow in balance calculationsSwallowing
WalletBalance::newerrors by defaulting can hide overflow or invariants violations. Prefer saturating sums and explicit error handling/logging (behindstd).- // Update balance, ignoring overflow errors as we're recalculating from scratch - self.balance = WalletBalance::new(confirmed, unconfirmed, locked) - .unwrap_or_else(|_| WalletBalance::default()); + // Update balance with saturating sums to avoid overflows and surface errors in debug builds. + #[cfg(debug_assertions)] + { + if let Err(e) = WalletBalance::new(confirmed, unconfirmed, locked) { + #[cfg(feature = "std")] + eprintln!("WalletBalance::new failed: {}", e); + } + } + self.balance = + WalletBalance::new(confirmed, unconfirmed, locked).unwrap_or_default();
322-341: Forwarder OK; consider UFCS if an inherent method name ever collidesThis calls an inherent
*_internalmethod, avoiding recursion. If you later rename it to match the trait method, use UFCS to disambiguate.Example if needed in the future:
ManagedWalletInfo::create_unsigned_payment_transaction(self, wallet, network, account_index, account_type_pref, recipients, fee_level, current_block_height)
55-60: Verified: Utxo has a total, deterministic ordering
Utxo in key-wallet/src/utxo.rs provides a manualOrdimplementation—delegating toself.outpoint.cmp(&other.outpoint)—soBTreeSet<&Utxo>is backed by a valid, stable total order. No changes are required.Optional refactor:
- If you only ever consume the outpoints, you could simplify the interface to return
BTreeSet<OutPoint>instead ofBTreeSet<&Utxo>for clarity.key-wallet-manager/src/wallet_manager/mod.rs (3)
775-784: Outdated comment: no clone occurs hereThe code returns the set borrowed from wallet info; the comment says “clone them.”
- // Get UTXOs from the wallet info and clone them + // Get UTXOs from the wallet info let utxos = wallet_info.utxos();
859-866: Optional: Deduplicate monitored addresses across walletsIf multiple wallets monitor the same address, you’ll send duplicates to the network layer.
- pub fn monitored_addresses(&self, network: Network) -> Vec<Address> { - let mut addresses = Vec::new(); - for info in self.wallet_infos.values() { - addresses.extend(info.monitored_addresses(network)); - } - addresses - } + pub fn monitored_addresses(&self, network: Network) -> Vec<Address> { + use alloc::collections::BTreeSet; + let mut set = BTreeSet::new(); + for info in self.wallet_infos.values() { + set.extend(info.monitored_addresses(network)); + } + set.into_iter().collect() + }
820-826: Pass description as-is instead of wrapping in SomeMinor cleanup:
set_descriptiontakes anOption<String>. No need to wrap.- if let Some(desc) = description { - managed_info.set_description(Some(desc)); - } + if description.is_some() { + managed_info.set_description(description); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
key-wallet-manager/src/spv_wallet_manager.rs(6 hunks)key-wallet-manager/src/wallet_manager/mod.rs(16 hunks)key-wallet-manager/src/wallet_manager/process_block.rs(1 hunks)key-wallet-manager/src/wallet_manager/transaction_building.rs(1 hunks)key-wallet-manager/tests/integration_test.rs(8 hunks)key-wallet/src/transaction_checking/wallet_checker.rs(1 hunks)key-wallet/src/wallet/managed_wallet_info/mod.rs(1 hunks)key-wallet/src/wallet/managed_wallet_info/transaction_building.rs(1 hunks)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs(1 hunks)key-wallet/src/wallet/mod.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- key-wallet-manager/src/wallet_manager/transaction_building.rs
- key-wallet-manager/tests/integration_test.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Do not hardcode network parameters, addresses, or keys
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet/src/wallet/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/spv_wallet_manager.rs
**/*.rs
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations
Use conditional compilation feature flags for optional features
Code must pass clippy with -D warnings (no clippy warnings allowed)
Format code with rustfmt and keep formatting clean
Maintain MSRV 1.80 (code must compile on Rust 1.80)
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet-manager/src/wallet_manager/process_block.rskey-wallet/src/wallet/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/spv_wallet_manager.rs
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}
📄 CodeRabbit Inference Engine (CLAUDE.md)
{key-wallet/**,key-wallet-ffi/**,swift-dash-core-sdk/**}/**/*.{rs,swift}: Use secure random number generation for keys
Never log or expose private keys
Files:
key-wallet/src/transaction_checking/wallet_checker.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/mod.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/src/wallet/managed_wallet_info/mod.rs
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/Cargo.toml : Set MSRV (minimum supported Rust version) to 1.80 via the rust-version field
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Code must be rustfmt-formatted (cargo fmt --check must pass)
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/{network,storage}/**/*.rs : Expose swappable implementations behind trait objects (e.g., Arc<dyn NetworkManager>, Arc<dyn StorageManager>) for runtime polymorphism
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: swift-dash-core-sdk/Examples/DashHDWalletExample/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:15:57.225Z
Learning: Applies to swift-dash-core-sdk/Examples/DashHDWalletExample/**/*.swift : When accessing SPV functionality, use DashSDK public APIs instead of direct client access; add public wrapper methods to DashSDK if needed
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the tests/ directory and organize them as standalone test files
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/storage/**/*.rs : Respect storage paths: headers in headers/, filters in filters/, and state in state/; persist chain state and sync progress between runs
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Provide integration tests for network operations
Applied to files:
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
📚 Learning: 2025-08-16T04:13:21.866Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-16T04:13:21.866Z
Learning: Applies to {rpc-integration-test/**,dash-spv/tests/**}/**/*.rs : Ensure tests cover both mainnet and testnet configurations
Applied to files:
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the tests/ directory and organize them as standalone test files
Applied to files:
key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
📚 Learning: 2025-08-16T04:14:57.808Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:57.808Z
Learning: Applies to dash-spv/**/*.rs : Clippy warnings must be treated as errors (cargo clippy ... -D warnings must pass)
Applied to files:
key-wallet-manager/src/spv_wallet_manager.rs
🧬 Code Graph Analysis (4)
key-wallet-manager/src/wallet_manager/process_block.rs (1)
dash-spv/src/client/block_processor_test.rs (2)
wallet(151-151)wallet(345-345)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (3)
key-wallet/src/wallet/managed_wallet_info/mod.rs (2)
with_name(62-72)new(49-59)key-wallet-manager/src/wallet_manager/transaction_building.rs (1)
create_unsigned_payment_transaction(18-57)key-wallet/src/account/transaction_record.rs (1)
new_confirmed(54-74)
key-wallet-manager/src/wallet_manager/mod.rs (2)
key-wallet/src/wallet/managed_wallet_info/mod.rs (2)
new(49-59)with_name(62-72)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (10)
with_name(21-21)with_name(107-109)name(26-26)name(114-116)birth_height(38-38)birth_height(130-132)wallet_id(23-23)wallet_id(110-112)utxos(56-56)utxos(163-174)
key-wallet-manager/src/spv_wallet_manager.rs (2)
key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
new(49-59)key-wallet-manager/src/wallet_manager/mod.rs (4)
new(68-74)new(102-108)default(61-63)default(95-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_sha1)
🔇 Additional comments (6)
key-wallet/src/wallet/mod.rs (1)
135-135: Good: Importing the trait enables method resolution in tests.Bringing WalletInfoInterface into scope is necessary to call its methods (e.g., set_description) on ManagedWalletInfo in this module.
key-wallet-manager/src/spv_wallet_manager.rs (3)
34-37: Solid: Generic SPVWalletManager with default type parameter.Defaulting T to ManagedWalletInfo while exposing base: WalletManager provides flexibility without breaking existing users.
51-60: From<WalletManager> implementation is clean and initializes SPV state correctly.The SPV-specific maps and counters are properly initialized.
223-295: WalletInterface impl over T: WalletInfoInterface + Send + Sync + 'static looks good.
- Delegation to base manager is clear.
- Stats updates and filter-match caching are straightforward.
key-wallet/src/wallet/managed_wallet_info/mod.rs (1)
10-10: Good addition: exposing the wallet info trait moduleExposing
wallet_info_interfacehere keeps the public API coherent and discoverable for downstream crates.key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (1)
239-294: Check TransactionRecord.is_ours semantics for matured transactionsYou set
is_ourstofalsewhile the comment says “Not ours (we received)”. Receiving to our addresses typically implies “ours.” Please confirm the intended meaning of this flag.If “ours” means “any of our addresses are involved,” consider:
- false, // Not ours (we received) + true, // Received to our addressesOr compute
is_oursbased on whether affected account sets are non-empty.
| use key_wallet::wallet::managed_wallet_info::{ManagedWalletInfo, TransactionRecord}; | ||
| use key_wallet::WalletBalance; | ||
| use key_wallet::{Account, AccountType, Address, Mnemonic, Network, Wallet, WalletConfig}; | ||
| use std::collections::BTreeSet; |
There was a problem hiding this comment.
Use alloc, not std, for BTreeSet (aligns with no_std support elsewhere)
This module already uses alloc::collections for maps. Importing std::collections::BTreeSet breaks no_std builds.
-use std::collections::BTreeSet;
+use alloc::collections::BTreeSet;📝 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.
| use std::collections::BTreeSet; | |
| use alloc::collections::BTreeSet; |
🤖 Prompt for AI Agents
In key-wallet-manager/src/wallet_manager/mod.rs around line 18, the file imports
std::collections::BTreeSet which breaks no_std builds; replace that import with
alloc::collections::BTreeSet to match the module's use of alloc for other
collections, removing the std import and ensuring the crate has alloc enabled in
its Cargo.toml and features so the alloc::collections::BTreeSet symbol resolves.
| pub(crate) use super::account_checker::TransactionCheckResult; | ||
| use super::transaction_router::TransactionRouter; | ||
| use crate::wallet::immature_transaction::ImmatureTransaction; | ||
| use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; |
There was a problem hiding this comment.
Remove unused WalletInfoInterface import to satisfy clippy -D warnings.
The trait isn’t referenced in this module. With clippy set to deny warnings, this unused import will fail CI.
Apply this diff to remove the unused import:
-use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;📝 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.
| use crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface; | |
| // (The unused import has been removed.) |
🤖 Prompt for AI Agents
In key-wallet/src/transaction_checking/wallet_checker.rs around line 9, the
import use
crate::wallet::managed_wallet_info::wallet_info_interface::WalletInfoInterface;
is unused and triggers clippy -D warnings; remove this unused
WalletInfoInterface import from the file (delete that use line) so the module
compiles without the clippy denial error.
| use crate::wallet::ManagedWalletInfo; | ||
| use crate::{Network, Utxo, Wallet, WalletBalance}; | ||
| use dashcore::{Address as DashAddress, Address, Transaction}; | ||
| use std::collections::BTreeSet; |
There was a problem hiding this comment.
Use alloc, not std, for collections in no_std-compatible crates
key-wallet uses alloc::collections elsewhere. Importing std::collections::BTreeSet will break no_std builds.
Apply:
- use std::collections::BTreeSet;
+ use alloc::collections::BTreeSet;📝 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.
| use std::collections::BTreeSet; | |
| use alloc::collections::BTreeSet; |
🤖 Prompt for AI Agents
In key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs around
line 16, the file currently imports std::collections::BTreeSet which breaks
no_std builds; change the import to use alloc::collections::BTreeSet instead,
and if the crate does not already reference alloc add `extern crate alloc;` at
the top (or ensure the crate root exposes alloc) so the alloc::collections
import resolves correctly; keep the rest of the code using BTreeSet unchanged.
The idea here is to make the wallet info generic so we can use wallets with platform info during sync.
Summary by CodeRabbit
New Features
Chores
Documentation
Tests
Refactor