fix: resolve various clippy warnings #128
Conversation
- Remove .clone() from BlockHash in chain_tip.rs - Remove .clone() from Header in orphan_pool.rs - Remove .clone() from Header in headers2_state.rs - Remove .clone() from Header in headers_with_reorg.rs These types implement Copy trait so cloning is unnecessary and creates unnecessary memory allocations. Fixes 4 clippy warnings.
…ck_manager.rs: drop RwLock guard before await to avoid deadlock\n- chain/checkpoints.rs: replace map_or(true/false) with is_none_or/is_some_and\n- network/mod.rs: use is_some_and (2 instances)\n- network/reputation.rs: use is_some_and\n- types.rs: use is_none_or and is_some_and\n- sync/headers_with_reorg.rs: replace unwrap after is_some with if let\n\nClarifies scope and accurately reflects all changes in the commit.
- Replace redundant closure with function reference in multi_peer.rs - Replace match expression with matches! macro in request_control.rs - Remove unnecessary let binding before return in request_control.rs - Initialize struct fields directly instead of reassignment in request_control.rs These changes improve code readability and follow Rust idioms: - Use function references instead of redundant closures - Use matches! macro for pattern matching expressions - Eliminate unnecessary intermediate bindings - Initialize structs with known values upfront Fixes 4 clippy style/readability warnings.
- Remove unnecessary u64 -> u64 cast in multi_peer.rs hasher.finish() - Remove unnecessary u32 -> u32 cast in multi_peer.rs peer_height - Remove unnecessary u32 -> u32 cast in network/mod.rs height These types were already the correct type, so casting them was unnecessary and created avoidable runtime overhead. Fixes 3 clippy unnecessary_cast warnings.
- Added missing Default implementations - Fixed map iteration patterns - Replaced or_insert_with with or_default - Simplified collapsible if statements - Replaced single match with if let Files modified: - dash-spv/src/chain/checkpoints.rs - dash-spv/src/chain/fork_detector.rs - dash-spv/src/network/connection.rs - dash-spv/src/network/message_handler.rs - dash-spv/src/network/reputation.rs - dash-spv/src/storage/sync_storage.rs - dash-spv/src/sync/filters.rs - dash-spv/src/sync/headers2_state.rs - dash-spv/src/sync/headers_with_reorg.rs - dash-spv/src/sync/sequential/request_control.rs - dash-spv/src/sync/state.rs - dash-spv/src/sync/validation_state.rs - dash-spv/src/types.rs - dash-spv/src/validation/instantlock.rs - dash-spv/src/validation/quorum.rs Reduced warnings from 31 to 10
…m_variant - BlockProcessingTask: Box<Block> and Box<Transaction> + updated call sites - SpvEvent::MempoolTransactionAdded.transaction: Box<Transaction> + constructors Clippy: large_enum_variant warnings removed; tests pass.
WalkthroughScoped lock-management and Option API modernizations across dash-spv; many types gain Default; large block/tx payloads moved to heap (Box); chainlock validation refactored to queue when masternode lists are missing; tests and CI workflows adjusted. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as caller
participant CLM as ChainLockManager
participant MLE as MasternodeListEngine
participant Q as ValidationQueue
participant CS as ChainState
caller->>CLM: extend_tip / validate_chain_lock(chainlock, height)
CLM->>MLE: check engine presence (scoped)
alt MLE present
CLM->>MLE: verify_chain_lock(chainlock, height)
alt verify OK
CLM->>CLM: validated = true
else missing MN lists error
CLM->>Q: queue_for_later(chainlock, required_height)
CLM->>CLM: validated = false
else other error
CLM-->>caller: return ValidationError
end
else No MLE
CLM->>Q: queue_for_later(chainlock, height)
CLM->>CLM: validated = false
end
CLM->>CS: store_chain_lock(chainlock, validated)
CS-->>CLM: updated state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
Use const &str for FAUCET_WALLET_NAME, TEST_WALLET_NAME, DEFAULT_WALLET_NODE_RPC_URL, DEFAULT_EVO_NODE_RPC_URL.\nUpdate API calls and comparisons to use &str directly.\nResolves Display/PartialEq errors in cargo check --tests -p integration_test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
dash-spv/src/validation/quorum.rs (1)
197-205: Fix off-by-one in cleanup_old_quorums (removes too many entries).Using heights.get(to_remove) drops at least two when to_remove == 1. Use to_remove - 1.
Apply:
- let to_remove = self.quorums.len() - self.max_cached_quorums; - let cutoff_height = heights.get(to_remove).copied().unwrap_or(0); + let to_remove = self.quorums.len() - self.max_cached_quorums; + let cutoff_height = heights.get(to_remove - 1).copied().unwrap_or(0); self.quorums.retain(|(_, height), _| *height > cutoff_height);dash-spv/src/network/reputation.rs (1)
173-176: Unreachable ban-clear path; expired bans never cleared.When ban has expired, is_banned() is false, so this block never runs. Clear by comparing now to until directly.
- // Check if ban has expired - if self.is_banned() && self.ban_time_remaining().is_none() { - self.banned_until = None; - } + // Clear expired ban timestamps + if let Some(until) = self.banned_until { + if now >= until { + self.banned_until = None; + } + }dash-spv/src/sync/sequential/request_control.rs (1)
170-176: Concurrency cap can be bypassed; active_count is never updated in loop.While condition uses a stale count; loop may send > max_concurrent requests.
- let active_count = self + let mut active_count = self .active_requests .values() .filter(|ar| self.request_phase(&ar.request.request_type) == phase_name) .count(); - // Process pending requests up to the limit - while active_count < max_concurrent && !self.pending_requests.is_empty() { + // Process pending requests up to the limit + while active_count < max_concurrent && !self.pending_requests.is_empty() { // Check rate limit if !self.check_rate_limit(&phase_name) { break; } // Get next request if let Some(request) = self.pending_requests.pop_front() { // Validate it's still allowed if !self.is_request_allowed(phase, &request.request_type) { continue; } // Send the request self.send_request(request, network, storage).await?; + active_count += 1; } }Also applies to: 177-195
dash-spv/src/sync/filters.rs (1)
105-107: Avoid potential underflow in calculate_batch_start_height.When count==0,
(len as u32 - 1)underflows before saturating_sub is called (debug builds panic). Compute the offset with saturating_sub first.- fn calculate_batch_start_height(cf_headers: &CFHeaders, stop_height: u32) -> u32 { - stop_height.saturating_sub(cf_headers.filter_hashes.len() as u32 - 1) - } + fn calculate_batch_start_height(cf_headers: &CFHeaders, stop_height: u32) -> u32 { + let count = cf_headers.filter_hashes.len() as u32; + stop_height.saturating_sub(count.saturating_sub(1)) + }
🧹 Nitpick comments (22)
dash-spv/src/terminal.rs (1)
170-182: Tiny allocation win: reserve exact-ish capacityPre-size
resultto reduce reallocations when formatting large numbers.fn format_number(n: u32) -> String { let s = n.to_string(); - let mut result = String::new(); + // digits + commas (~ one per 3 digits, excluding the first group) + let mut result = String::with_capacity(s.len() + (s.len().saturating_sub(1) / 3));dash-spv/src/sync/masternodes.rs (1)
300-301: Use sort_unstable for a slight speedupStability isn’t required before
dedup().sort_unstable()is a bit faster and uses less memory.- block_hashes.sort(); + block_hashes.sort_unstable();dash-spv/src/storage/sync_storage.rs (1)
17-21: Prefer derive(Default) over manual implSince all fields implement Default, deriving reduces boilerplate and keeps impls consistent.
-/// Simple in-memory storage for testing -pub struct MemoryStorage { +/// Simple in-memory storage for testing +#[derive(Default)] +pub struct MemoryStorage { @@ -impl Default for MemoryStorage { - fn default() -> Self { - Self::new() - } -}Note:
RwLock<HashMap<..>>implements Default, so this remains MSRV-safe.dash-spv/src/validation/instantlock.rs (1)
8-10: Minor: derive(Default) and drop manual implSince the type has no fields, deriving is simpler.
-/// Validates InstantLock messages. -pub struct InstantLockValidator { - // TODO: Add masternode list for signature verification -} +/// Validates InstantLock messages. +#[derive(Default)] +pub struct InstantLockValidator { + // TODO: Add masternode list for signature verification +} @@ -impl Default for InstantLockValidator { - fn default() -> Self { - Self::new() - } -}dash-spv/src/types.rs (5)
186-223: Simplify genesis selection (match with identical arms)The match on Network returns the same expression in all arms. Clippy will prefer a direct call.
Apply within this range:
- // Initialize with genesis block - let genesis_header = match network { - Network::Dash => { - // Use known genesis for mainnet - dashcore::blockdata::constants::genesis_block(network).header - } - Network::Testnet => { - // Use known genesis for testnet - dashcore::blockdata::constants::genesis_block(network).header - } - _ => { - // For other networks, use the existing genesis block function - dashcore::blockdata::constants::genesis_block(network).header - } - }; + // Initialize with network-specific genesis block + let genesis_header = dashcore::blockdata::constants::genesis_block(network).header;
318-323: Unnecessary Option wrapper returns always Some(empty vec)This will trigger clippy::unnecessary_wraps. Either:
- change return type to Vec and return empty vec, or
- keep signature but return None when not tracking yet, or
- add an allow for the placeholder.
Would you like a follow-up patch to switch the return type to Vec and update call sites?
456-471: Minor: use is_some_and for Option checks (clippy tidy-up)In supports_compact_filters()/supports_headers2(), prefer is_some_and over map(...).unwrap_or(false) for readability.
Example (outside this hunk):
self.services.is_some_and(|s| ServiceFlags::from(s).has(ServiceFlags::COMPACT_FILTERS))
568-599: Nit: Duration::ZERO over Duration::default()For SpvStats::default(), consider Duration::ZERO for explicitness.
Example (outside this hunk):
uptime: std::time::Duration::ZERO,
237-240: Micro: method reference in mapYou can write map(BlockHeader::block_hash) instead of a closure.
Example (outside this hunk):
self.headers.last().map(BlockHeader::block_hash)dash-spv/src/network/connection.rs (2)
139-172: Comment drift: “Don’t set socket timeouts” vs setting read_timeoutThe block comments say not to set socket timeouts, yet the code sets a read timeout (which is likely intended and correct per the warning). Consider updating the prose to avoid confusion.
Proposed wording (outside this hunk): “Avoid generic non-blocking mode; instead set a conservative read timeout (tested 100ms) to prevent partial reads.”
374-510: Blocking I/O under tokio mutex (architectural consideration)The decode path uses std::net::TcpStream + BufReader with blocking reads inside an async fn guarded by tokio::sync::Mutex. This can block executor threads under load.
When feasible, consider migrating to tokio::net::TcpStream with async read/write (or wrap blocking ops with spawn_blocking on a dedicated IO worker) to avoid head-of-line blocking across tasks.
dash-spv/src/chain/orphan_pool.rs (1)
206-209: Remove unused local and avoid double hash computation._header is computed then immediately recomputed in remove_orphan. Drop the temporary and reuse a single computed hash.
Apply:
- for header in &orphans { - let _block_hash = header.block_hash(); - self.remove_orphan(&header.block_hash()); - } + for header in &orphans { + let h = header.block_hash(); + self.remove_orphan(&h); + }dash-spv/src/network/message_handler.rs (1)
6-6: Remove redundantuse tracing;.You’re already calling
tracing::...via the path; the import is unnecessary.-use tracing;dash-spv/src/network/mod.rs (1)
132-139: Remove redundant clone when building GetQRInfo.Clippy will flag this clone; you can move the Vec and compute len beforehand.
Apply:
- let get_qr_info = GetQRInfo { - base_block_hashes: base_block_hashes.clone(), - block_request_hash, - extra_share, - }; - - let base_hashes_count = get_qr_info.base_block_hashes.len(); + let base_hashes_count = base_block_hashes.len(); + let get_qr_info = GetQRInfo { + base_block_hashes, + block_request_hash, + extra_share, + };dash-spv/src/network/multi_peer.rs (1)
1252-1254: Disambiguate inherent vs trait method to avoid confusion.Inside the trait impl, calling get_last_message_peer_id() can be ambiguous to readers. Use fully qualified call to the inherent method.
Apply:
async fn get_last_message_peer_id(&self) -> crate::types::PeerId { - // Call the instance method to avoid code duplication - self.get_last_message_peer_id().await + // Call the inherent method to avoid duplication + MultiPeerNetworkManager::get_last_message_peer_id(self).await }dash-spv/src/client/block_processor_test.rs (3)
111-113: Drop unnecessary clone when boxing the block.Clippy nit; block isn’t used after send.
Apply:
- block: Box::new(block.clone()), + block: Box::new(block),
303-305: Drop unnecessary clone when boxing the tx.tx isn’t used after send.
Apply:
- tx: Box::new(tx.clone()), + tx: Box::new(tx),
357-359: Drop unnecessary clone when boxing the block (not-found case).Same as above.
Apply:
- block: Box::new(block.clone()), + block: Box::new(block),dash-spv/src/client/block_processor.rs (1)
129-131: Avoid moving out of Box; pass references to internal processorsYou're boxing to reduce copies across the channel, but then move T out of the Box. Passing &T avoids a potentially large heap-to-stack move and aligns with the borrow-only usage inside the internals.
Apply:
- let result = self.process_block_internal(*block).await; + let result = self.process_block_internal(&block).await; ... - let result = self.process_transaction_internal(*tx).await; + let result = self.process_transaction_internal(&tx).await; @@ - async fn process_block_internal(&mut self, block: dashcore::Block) -> Result<()> { + async fn process_block_internal(&mut self, block: &dashcore::Block) -> Result<()> { - let block_hash = block.block_hash(); + let block_hash = block.block_hash(); @@ - self.update_chain_state_with_block(&block).await?; + self.update_chain_state_with_block(block).await?; @@ - async fn process_transaction_internal(&mut self, tx: dashcore::Transaction) -> Result<()> { + async fn process_transaction_internal(&mut self, tx: &dashcore::Transaction) -> Result<()> { - let txid = tx.txid(); + let txid = tx.txid(); - wallet.process_mempool_transaction(&tx, self.network).await; + wallet.process_mempool_transaction(tx, self.network).await;Also applies to: 163-164, 204-248, 251-264
dash-spv/src/sync/headers_with_reorg.rs (1)
849-853: Comment no longer matches logicCode sets storage_height = height unconditionally, but the comment says “adjust for checkpoint.” Either restore the adjustment or update the comment to reflect that height is already a storage index.
Apply:
- // When syncing from checkpoint, adjust the storage height - let storage_height = height; + // Height here is a storage index already (0-based), even when checkpoint-syncing + let storage_height = height;dash-spv/src/chain/chainlock_manager.rs (2)
211-225: Avoid string-matching on error text from MasternodeListEngineParsing e.to_string() for "No masternode lists in engine" is brittle. Prefer a typed error variant or a dedicated predicate to distinguish “missing lists” from real verification failures.
Example directions:
- If the engine exposes a typed error, match on it:
match engine.verify_chain_lock(&chain_lock) { Err(MasternodeEngineError::MissingLists { .. }) => { /* queue */ } Err(e) => return Err(ValidationError::InvalidChainLock(format!(...))), Ok(()) => { validated = true; } }
- If not, consider adding an engine.has_required_lists(required_height) check before verify_chain_lock to branch without string inspection.
77-96: Use VecDeque for O(1) eviction in pending queueremove(0) on Vec is O(n). The queue is small, but switching to VecDeque avoids unnecessary shifts.
Minimal change:
-use std::sync::{Arc, RwLock}; +use std::sync::{Arc, RwLock}; +use std::collections::VecDeque; @@ - pending_chainlocks: Arc<RwLock<Vec<ChainLock>>>, + pending_chainlocks: Arc<RwLock<VecDeque<ChainLock>>>, @@ - pending.push(chain_lock); + pending.push_back(chain_lock); @@ - let dropped = pending.remove(0); + let dropped = pending.pop_front().expect("queue not empty");
📜 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 (26)
dash-spv/src/chain/chain_tip.rs(1 hunks)dash-spv/src/chain/chainlock_manager.rs(1 hunks)dash-spv/src/chain/checkpoints.rs(2 hunks)dash-spv/src/chain/fork_detector.rs(1 hunks)dash-spv/src/chain/orphan_pool.rs(1 hunks)dash-spv/src/client/block_processor.rs(3 hunks)dash-spv/src/client/block_processor_test.rs(3 hunks)dash-spv/src/client/message_handler.rs(2 hunks)dash-spv/src/client/mod.rs(1 hunks)dash-spv/src/network/connection.rs(1 hunks)dash-spv/src/network/message_handler.rs(1 hunks)dash-spv/src/network/mod.rs(3 hunks)dash-spv/src/network/multi_peer.rs(3 hunks)dash-spv/src/network/reputation.rs(3 hunks)dash-spv/src/storage/sync_storage.rs(1 hunks)dash-spv/src/sync/filters.rs(3 hunks)dash-spv/src/sync/headers2_state.rs(1 hunks)dash-spv/src/sync/headers_with_reorg.rs(4 hunks)dash-spv/src/sync/masternodes.rs(1 hunks)dash-spv/src/sync/sequential/request_control.rs(4 hunks)dash-spv/src/sync/state.rs(1 hunks)dash-spv/src/sync/validation_state.rs(5 hunks)dash-spv/src/terminal.rs(1 hunks)dash-spv/src/types.rs(3 hunks)dash-spv/src/validation/instantlock.rs(1 hunks)dash-spv/src/validation/quorum.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/storage/sync_storage.rsdash-spv/src/sync/masternodes.rsdash-spv/src/client/mod.rsdash-spv/src/network/mod.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/network/message_handler.rsdash-spv/src/terminal.rsdash-spv/src/validation/instantlock.rsdash-spv/src/sync/state.rsdash-spv/src/client/block_processor.rsdash-spv/src/network/connection.rsdash-spv/src/chain/chain_tip.rsdash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/headers_with_reorg.rsdash-spv/src/sync/sequential/request_control.rsdash-spv/src/sync/filters.rsdash-spv/src/network/reputation.rsdash-spv/src/client/message_handler.rsdash-spv/src/network/multi_peer.rsdash-spv/src/validation/quorum.rsdash-spv/src/client/block_processor_test.rsdash-spv/src/sync/headers2_state.rsdash-spv/src/types.rsdash-spv/src/sync/validation_state.rsdash-spv/src/chain/checkpoints.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Provide both
MemoryStorageManagerandDiskStorageManagerbehind theStorageManagertrait
Files:
dash-spv/src/storage/sync_storage.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 in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/storage/sync_storage.rsdash-spv/src/sync/masternodes.rsdash-spv/src/client/mod.rsdash-spv/src/network/mod.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/network/message_handler.rsdash-spv/src/terminal.rsdash-spv/src/validation/instantlock.rsdash-spv/src/sync/state.rsdash-spv/src/client/block_processor.rsdash-spv/src/network/connection.rsdash-spv/src/chain/chain_tip.rsdash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/headers_with_reorg.rsdash-spv/src/sync/sequential/request_control.rsdash-spv/src/sync/filters.rsdash-spv/src/network/reputation.rsdash-spv/src/client/message_handler.rsdash-spv/src/network/multi_peer.rsdash-spv/src/validation/quorum.rsdash-spv/src/client/block_processor_test.rsdash-spv/src/sync/headers2_state.rsdash-spv/src/types.rsdash-spv/src/sync/validation_state.rsdash-spv/src/chain/checkpoints.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/storage/sync_storage.rsdash-spv/src/sync/masternodes.rsdash-spv/src/client/mod.rsdash-spv/src/network/mod.rsdash-spv/src/chain/fork_detector.rsdash-spv/src/chain/orphan_pool.rsdash-spv/src/network/message_handler.rsdash-spv/src/terminal.rsdash-spv/src/validation/instantlock.rsdash-spv/src/sync/state.rsdash-spv/src/client/block_processor.rsdash-spv/src/network/connection.rsdash-spv/src/chain/chain_tip.rsdash-spv/src/chain/chainlock_manager.rsdash-spv/src/sync/headers_with_reorg.rsdash-spv/src/sync/sequential/request_control.rsdash-spv/src/sync/filters.rsdash-spv/src/network/reputation.rsdash-spv/src/client/message_handler.rsdash-spv/src/network/multi_peer.rsdash-spv/src/validation/quorum.rsdash-spv/src/client/block_processor_test.rsdash-spv/src/sync/headers2_state.rsdash-spv/src/types.rsdash-spv/src/sync/validation_state.rsdash-spv/src/chain/checkpoints.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Implement DNS-first peer discovery with the specified seeds, immediate startup (no initial discovery delay), and exclusive peer mode when explicit peers are provided
Files:
dash-spv/src/network/mod.rsdash-spv/src/network/message_handler.rsdash-spv/src/network/connection.rsdash-spv/src/network/reputation.rsdash-spv/src/network/multi_peer.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/src/storage/sync_storage.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:
dash-spv/src/storage/sync_storage.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
dash-spv/src/chain/chain_tip.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/src/client/message_handler.rs
📚 Learning: 2025-06-26T16:01:37.609Z
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.
Applied to files:
dash-spv/src/client/message_handler.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/src/network/multi_peer.rsdash-spv/src/types.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/src/client/block_processor_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/src/client/block_processor_test.rs
🧬 Code graph analysis (10)
dash-spv/src/client/mod.rs (2)
dash-spv/src/client/block_processor.rs (1)
new(46-64)dash-spv/src/client/message_handler.rs (1)
new(35-59)
dash-spv/src/network/message_handler.rs (1)
dash-spv/src/network/mod.rs (1)
new(165-176)
dash-spv/src/validation/instantlock.rs (7)
dash-spv/src/types.rs (5)
default(63-79)default(420-422)default(569-598)new(181-183)new(877-897)dash-spv/src/network/reputation.rs (3)
default(124-136)default(202-204)new(209-215)dash-spv/src/sync/validation_state.rs (2)
default(135-146)default(150-152)dash-spv/src/network/message_handler.rs (2)
default(14-16)new(21-25)dash-spv/src/storage/sync_storage.rs (2)
default(18-20)new(24-31)dash-spv/src/sync/state.rs (2)
default(21-23)new(28-34)dash-spv/src/validation/quorum.rs (2)
default(71-73)new(78-83)
dash-spv/src/sync/state.rs (2)
dash-spv/src/types.rs (5)
default(63-79)default(420-422)default(569-598)new(181-183)new(877-897)dash-spv/src/storage/sync_storage.rs (2)
default(18-20)new(24-31)
dash-spv/src/chain/chain_tip.rs (1)
dash-spv/src/types.rs (1)
tip_hash(238-240)
dash-spv/src/chain/chainlock_manager.rs (1)
dash-spv/src/sync/masternodes.rs (1)
engine(459-461)
dash-spv/src/sync/headers_with_reorg.rs (2)
dash-spv/src/client/mod.rs (1)
chain_state(2351-2354)dash-spv/src/client/status_display.rs (1)
chain_state(117-120)
dash-spv/src/network/reputation.rs (3)
dash-spv/src/sync/validation_state.rs (3)
default(135-146)default(150-152)new(157-164)dash-spv/src/network/message_handler.rs (2)
default(14-16)new(21-25)dash-spv/src/storage/sync_storage.rs (2)
default(18-20)new(24-31)
dash-spv/src/network/multi_peer.rs (1)
dash-spv/src/network/peer.rs (1)
best_height(79-81)
dash-spv/src/client/block_processor_test.rs (3)
dash-spv/src/client/block_processor.rs (1)
new(46-64)dash-spv/src/client/message_handler.rs (1)
new(35-59)dash-spv/src/client/mod.rs (1)
new(174-237)
⏰ 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). (20)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: Clippy (Non-strict)
🔇 Additional comments (54)
dash-spv/src/terminal.rs (2)
174-179: LGTM: cleaner loop with enumerate()The refactor keeps behavior intact and removes manual counter mutation.
1-1: Automated fmt & clippy checks failed—manual verification requiredThe workspace-wide formatting (
cargo fmt --all -- --check) and lint (cargo clippy --workspace --all-targets --all-features -D warnings) checks could not run due to missing/proc; please manually run these commands to ensure all Rust code is properly formatted and free of clippy warnings.dash-spv/src/sync/masternodes.rs (1)
268-274: LGTM: more concise initializationUsing a vector literal is clearer and avoids multiple pushes.
dash-spv/src/sync/headers2_state.rs (2)
85-88: LGTM: move instead of cloneMoving
last_headeravoids an unnecessary clone and matches the updated API.
79-80: CompressionState::default() delegates to new()
Theimpl Default for CompressionStatecallsSelf::new(), soor_default()is equivalent toor_insert_with(CompressionState::new). No change needed.dash-spv/src/validation/instantlock.rs (1)
12-16: LGTM: Default delegating to new()Matches the broader PR pattern and helps ergonomics.
dash-spv/src/validation/quorum.rs (3)
70-74: Default delegating to new() looks good.Matches the pattern used elsewhere in the PR.
214-221: Key-only iteration in get_stats is correct.Match ergonomics handle &QuorumType; min/max tracking is fine.
1-5: Verify formatting and lints for dash-spv
Runcargo fmt --all -- --checkandcargo clippy --all-targets --all-features -- -D warningsindash-spv; ensure CI passes with no warnings or formatting errors.dash-spv/src/network/reputation.rs (3)
142-143: is_some_and usage is correct and MSRV-safe (1.89).Semantics unchanged; concise.
201-205: Default delegating to new() looks good.Consistent with the codebase.
231-234: clamp simplifies bounds correctly.Replaces min/max chain with clearer clamp.
dash-spv/src/sync/sequential/request_control.rs (4)
127-134: matches! gate is precise and easy to audit.Good consolidation of allowed (phase, request) pairs.
283-286: Inline GetBlock -> GetData return is fine.No functional change; trims a temp var.
342-347: Struct literal with Default fallback is clean.Keeps stats initialization concise.
359-367: Iterating values() for active stats is the right call.Removes unused key binding.
dash-spv/src/sync/validation_state.rs (5)
13-14: Listener type alias improves readability.Nice local alias; keeps add_listener signature readable.
118-119: Vector of aliased listeners is fine.No behavior change.
149-153: Default delegating to new() is consistent.Matches the pattern across modules.
278-279: or_default() is an idiomatic micro-cleanup.Eliminates redundant Vec::new.
334-341: Iterating over keys avoids unnecessary tuple destructuring.No issues spotted.
dash-spv/src/sync/filters.rs (3)
1462-1467: Key-only iteration over active_filter_requests is appropriate.Avoids unused value bindings; logic unchanged.
2797-2816: Key-only iteration over requested_filter_ranges is appropriate.Gap finding loop remains correct.
2908-2912: saturating_sub for gap size is safer.Prevents underflow when filter height > block height (edge cases).
dash-spv/src/types.rs (6)
149-177: Deriving Default/Clone for ChainState is good clippy hygieneRemoves boilerplate and keeps the type trivially constructible. No issues.
206-211: Good: useful initialization log at debug levelIncludes network, hash, and header count; cheap to compute and helpful for diagnostics.
296-299: Correct use of Option::is_none_or (MSRV OK)Semantics match prior logic; clearer and clippy-friendly. MSRV 1.89 covers this API.
304-305: Correct use of Option::is_some_andEquivalent behavior; reads better and silences clippy.
1-1: Verify repo-wide formatting and lints
Sandbox failed to runcargo fmtandcargo clippy(no /proc available). Please run locally:cargo fmt --all --check cargo clippy --all-targets --all-features -- -D warnings
797-808: No action required: all SpvEvent::MempoolTransactionAdded constructors use Box::new(tx) and pattern matches handle Box correctly.dash-spv/src/chain/chain_tip.rs (2)
103-106: Good: avoid unnecessary clone by dereferencing keyUsing *tip_hash here eliminates an extra allocation and keeps types aligned.
95-106: Atomic restore path looks sound; consider active-tip consistencyYou remove via HashMap::remove (not remove_tip), then reinsert on failure. Since active_tip remains the same hash, reinsertion keeps pointers valid. As a guard, calling update_active_tip() after restoration would also reassert is_active flags if future changes alter this assumption.
Would you like a quick test asserting that when the removed tip was active and add_tip fails (simulated with a stub/feature), active_tip and is_active remain consistent after restoration?
dash-spv/src/network/connection.rs (1)
516-519: Clear and concise disconnection handlingThe if let Err(NetworkError::PeerDisconnected) simplifies control flow and correctly clears state after dropping the mutex.
dash-spv/src/chain/orphan_pool.rs (1)
100-106: Returning header by value is fine if BlockHeader: Copy; please confirm.This change relies on dashcore::Header being Copy. If not, this will be a move from a borrowed OrphanBlock and won’t compile on MSRV 1.89. If Copy isn’t available, revert to clone() or refactor to return references.
dash-spv/src/network/message_handler.rs (1)
13-17: Default impl delegating to new(): LGTM.dash-spv/src/sync/state.rs (1)
20-24: Default -> new() delegation: LGTM.Consistent with the pattern across the crate.
dash-spv/src/chain/fork_detector.rs (1)
111-121: Combined checkpoint guard reads cleaner; semantics preserved.Condition ordering also avoids underflow. Looks good.
dash-spv/src/client/mod.rs (1)
1139-1141: AllProcessBlockcall sites now box theblockfield; no unboxed instances remain. LGTM.dash-spv/src/network/mod.rs (3)
241-242: Nice idiomatic Option check with is_some_and.No functional change; reads cleaner and Clippy-friendly.
288-289: Consistent ping gating with is_some_and.Straightforward, matches the connected check.
303-307: Avoided unnecessary cast in best height.Returning u32 directly is correct given PeerInfo.best_height: Option.
dash-spv/src/network/multi_peer.rs (3)
835-835: map_err(Error::Network) style is fine.More concise; no semantic change.
887-888: PeerId from hasher.finish() looks correct.Assuming PeerId wraps u64, this is cleaner than casting.
1179-1180: Use max on u32 heights.Removes unnecessary casts; logic is correct.
dash-spv/src/chain/checkpoints.rs (2)
63-71: Derive Default for CheckpointOverride.Matches previous defaults; simplifies impl.
140-141: is_none_or is clear and MSRV-safe.Logic identical; concise.
dash-spv/src/client/message_handler.rs (2)
237-244: Box large tx payload in event.Good move to reduce copies; aligns with types.
497-501: Box block for async processing.Avoids cloning large data; consistent with processor API.
dash-spv/src/client/block_processor.rs (2)
16-21: Boxing large payloads in tasks is a good moveHeap-allocating Block/Transaction reduces channel move costs and keeps enum size small. Call sites were updated consistently.
603-608: blocks_requested usage is correct This counter is intentionally incremented when issuing a block request (distinct from blocks_processed, which is bumped upon successful processing). No change needed.dash-spv/src/sync/headers_with_reorg.rs (3)
312-319: Checkpoint validation condition simplified correctlyCombines the nested condition cleanly; behavior preserved.
637-649: Effective tip height refactor reads clearerThe nested if-let removes unwrap paths and keeps checkpoint math explicit. Good.
545-545: No change required:HeaderderivesCopy, so using*genesis_headeris correct.dash-spv/src/chain/chainlock_manager.rs (1)
193-246: Good: scope the read lock to avoid holding std::sync::RwLock across awaitsThe explicit block ensures the guard is dropped before any async work. This prevents potential deadlocks and aligns with async best practices.
…ros/params) - Remove unused QuorumType and Log imports - Drop unused assert_deprecated macro - Prefix unused parameter and ignore unused fields Remaining warnings are dead_code for helper tests not invoked in main.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
rpc-integration-test/src/main.rs (2)
194-194: Redundant clone on Auth.
clone().clone()is unnecessary; a single clone suffices.Apply this diff:
- let faucet_client = Client::new(&faucet_rpc_url, wallet_node_auth.clone().clone()).unwrap(); + let faucet_client = Client::new(&faucet_rpc_url, wallet_node_auth.clone()).unwrap();
1232-1234: Strengthen wallet existence assertions.Given both wallets are used earlier, assert each explicitly for clearer failures.
- assert!(wallet_list.iter().any(|w| w == TEST_WALLET_NAME || w == FAUCET_WALLET_NAME)); - wallet_list.retain(|w| w != TEST_WALLET_NAME && !w.is_empty() && w != FAUCET_WALLET_NAME); + assert!(wallet_list.iter().any(|w| w == TEST_WALLET_NAME), "missing test wallet"); + assert!(wallet_list.iter().any(|w| w == FAUCET_WALLET_NAME), "missing faucet wallet"); + wallet_list.retain(|w| w != TEST_WALLET_NAME && !w.is_empty() && w != FAUCET_WALLET_NAME);
📜 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 (1)
rpc-integration-test/src/main.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Use proper error types with thiserror and propagate errors appropriately
Use the tokio runtime for async operations in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
rpc-integration-test/src/main.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
rpc-integration-test/src/main.rs
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/**/*.rs : Always validate network consistency; never mix mainnet and testnet operations
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/derivation_tests.rs : Place path derivation tests in tests/derivation_tests.rs
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-06-15T15:31:44.136Z
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#74
File: key-wallet/src/address.rs:30-40
Timestamp: 2025-06-15T15:31:44.136Z
Learning: In the key-wallet crate, QuantumExplorer prefers keeping wildcard arms that default unknown Network variants to testnet prefixes rather than using exhaustive matches or panics. This fallback behavior is intentional.
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/integration_real_node_test.rs : Integration tests that connect to a live Dash Core node must gracefully skip when the node at 127.0.0.1:9999 is unavailable
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Use property-based tests for complex invariants where suitable
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Use deterministic testing with known vectors (fixed seeds) in Rust tests
Applied to files:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/mnemonic_tests.rs : Place mnemonic generation and validation tests in tests/mnemonic_tests.rs
Applied to files:
rpc-integration-test/src/main.rs
⏰ 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). (20)
- GitHub Check: Clippy (Non-strict)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Core Components Tests
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deserialize_address)
🔇 Additional comments (6)
rpc-integration-test/src/main.rs (6)
15-15: Import refinement looks good.Using a focused
use log::trace;aligns with the actual call sites and avoids unused imports.
36-37: Consolidated json imports are tidy and appropriate.Bringing
ScanTxOutRequestinto the same group improves readability and keeps related RPC JSON types together.
41-45: Switch to compile-time constants is a solid Clippy-friendly move.Consts for wallet names and default RPC URLs remove unnecessary lazy statics and reduce overhead.
208-214: Load/create wallet flow reads cleaner with constants.Using
TEST_WALLET_NAMEimproves clarity; error-code handling stays intact.
1413-1413: Unused parameter handled correctly.Renaming to
_wallet_clientresolves the unused-argument lint without changing call sites.
1522-1534: Ignoring unused ProTxInfo fields is Clippy-clean.Binding
collateral_indexandoperator_rewardto_avoids unused-variable warnings while keeping pattern clarity.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
rpc-integration-test/run.sh (2)
15-35: Bind RPC/P2P to loopback to avoid exposing ports during CI.- -port=20001 \ + -bind=127.0.0.1 -port=20001 \ - -rpcport=20002 \ + -rpcbind=127.0.0.1 -rpcport=20002 \ @@ - -port=20301 \ + -bind=127.0.0.1 -port=20301 \ - -rpcport=20302 \ + -rpcbind=127.0.0.1 -rpcport=20302 \
44-55: Alternative: use -rpcwait to simplify readiness polling (optional).- local attempt=0 - while ! dash-cli -regtest -datadir="${datadir}" -rpcport="${rpcport}" getnetworkinfo >/dev/null 2>&1; do + local attempt=0 + while ! dash-cli -regtest -datadir="${datadir}" -rpcport="${rpcport}" -rpcwait -rpcclienttimeout=1 getnetworkinfo >/dev/null 2>&1; do
📜 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 (1)
rpc-integration-test/run.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/integration_real_node_test.rs : Integration tests that connect to a live Dash Core node must gracefully skip when the node at 127.0.0.1:9999 is unavailable
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the `tests/` directory
Applied to files:
rpc-integration-test/run.sh
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
Applied to files:
rpc-integration-test/run.sh
🪛 Shellcheck (0.10.0)
rpc-integration-test/run.sh
[warning] 47-47: i appears unused. Verify use (or export if used externally).
(SC2034)
⏰ 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). (21)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: Integration Tests (stable, 22.1.3)
- GitHub Check: Clippy (Non-strict)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: SPV Components Tests
PastaPastaPasta
force-pushed
the
fix/resolve-clippy
branch
from
4b50c05 to
c3abd3c
Compare
- Add dash-spv and dash-spv-ffi to strict-checks job (check/build/test with RUSTFLAGS='-D warnings' and clippy -D warnings)\n- Exclude them from the non-strict clippy run via STRICT_CRATES
File is not referenced anywhere; strict checks are configured in workflows.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/rust.yml (1)
167-190: DRY the repeated strict steps for dash-spv and dash-spv-ffi.Consider consolidating into a single looped step to reduce YAML duplication and keep future maintenance simple.
Note: RUSTFLAGS="-D warnings" applies to dependencies as well. Confirm this is intended for these crates.
- # Check dash-spv with strict warnings - - name: Check dash-spv (deny warnings) - env: - RUSTFLAGS: "-D warnings" - run: | - cargo check -p dash-spv --all-features --lib --bins --tests - cargo build -p dash-spv --all-features --lib --bins - cargo test -p dash-spv --all-features --lib --bins - - - name: Clippy dash-spv (deny all warnings) - run: cargo clippy -p dash-spv --all-features --lib --bins --tests -- -D warnings - - # Check dash-spv-ffi with strict warnings - - name: Check dash-spv-ffi (deny warnings) - env: - RUSTFLAGS: "-D warnings" - run: | - cargo check -p dash-spv-ffi --all-features --lib --bins --tests - cargo build -p dash-spv-ffi --all-features --lib --bins - cargo test -p dash-spv-ffi --all-features --lib --bins - - - name: Clippy dash-spv-ffi (deny all warnings) - run: cargo clippy -p dash-spv-ffi --all-features --lib --bins --tests -- -D warnings + - name: Strict checks for dash-spv family (deny warnings) + env: + RUSTFLAGS: "-D warnings" + run: | + set -euo pipefail + for crate in dash-spv dash-spv-ffi; do + echo "::group::Strict checks for $crate" + cargo check -p "$crate" --all-features --lib --bins --tests + cargo build -p "$crate" --all-features --lib --bins + cargo test -p "$crate" --all-features --lib --bins + cargo clippy -p "$crate" --all-features --lib --bins --tests -- -D warnings + echo "::endgroup::" + done
📜 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 (2)
.github/strict-checks.json(0 hunks).github/workflows/rust.yml(2 hunks)
💤 Files with no reviewable changes (1)
- .github/strict-checks.json
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
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.
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : Enforce Rust formatting via `cargo fmt --check` on all Rust source files
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Pre-PR checks: cargo fmt, cargo clippy, cargo test (workspace); update docs/CHANGELOG for user-facing changes
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Mirror strict CI locally if helpful: export RUSTFLAGS="-D warnings"
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to **/*.rs : Run clippy with -D warnings and fix all lints
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Lint with clippy; treat warnings as errors in CI
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to **/*.rs : Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/*.rs : Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Applied to files:
.github/workflows/rust.yml
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
.github/workflows/rust.yml
⏰ 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). (18)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: Core Components Tests
🔇 Additional comments (1)
.github/workflows/rust.yml (1)
84-84: Good: include SPV crates in STRICT_CRATES to avoid duplicate non-strict runs.This aligns with the “deny warnings” policy for dash-spv and dash-spv-ffi and keeps the non-strict Clippy job focused on the remaining crates.
- Drop unused imports in test modules to silence warnings\n- Keep necessary traits (dashcore_hashes::Hash) where required by from_byte_array calls\n- No functional changes; only cleanup
Clear 'variable does not need to be mutable' warnings by dropping mut from bindings not mutated. Kept mut where methods require &mut (e.g., process_filter_matches_and_download).
- Remove unused bindings and imports across tests\n- Simplify placeholder tests to avoid creating unused values\n- Keep call sites where useful by discarding results explicitly (let _ = ...)\n- No functional changes; compile-time hygiene only
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
dash-spv/tests/error_recovery_integration_test.rs (1)
24-38: Prefer atomics over async Mutex for counters/flags (clippy::mutex_atomic).Reduces contention and avoids nested-lock patterns.
+use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; @@ struct NetworkInterruptor { - should_interrupt: Arc<Mutex<bool>>, - interrupt_after_messages: Arc<Mutex<Option<usize>>>, - messages_count: Arc<Mutex<usize>>, + should_interrupt: Arc<AtomicBool>, + interrupt_after_messages: Arc<Mutex<Option<usize>>>, + messages_count: Arc<AtomicUsize>, } @@ fn new() -> Self { Self { - should_interrupt: Arc::new(Mutex::new(false)), - interrupt_after_messages: Arc::new(Mutex::new(None)), - messages_count: Arc::new(Mutex::new(0)), + should_interrupt: Arc::new(AtomicBool::new(false)), + interrupt_after_messages: Arc::new(Mutex::new(None)), + messages_count: Arc::new(AtomicUsize::new(0)), } } @@ async fn should_interrupt(&self) -> bool { - let mut count = self.messages_count.lock().await; - *count += 1; + let count = self.messages_count.fetch_add(1, Ordering::Relaxed) + 1; - if let Some(limit) = *self.interrupt_after_messages.lock().await { - if *count >= limit { - *self.should_interrupt.lock().await = true; - } - } + if let Some(limit) = *self.interrupt_after_messages.lock().await { + if count >= limit { + self.should_interrupt.store(true, Ordering::Relaxed); + } + } - *self.should_interrupt.lock().await + self.should_interrupt.load(Ordering::Relaxed) } @@ async fn reset(&self) { - *self.should_interrupt.lock().await = false; - *self.messages_count.lock().await = 0; + self.should_interrupt.store(false, Ordering::Relaxed); + self.messages_count.store(0, Ordering::Relaxed); }Also applies to: 40-61
dash-spv/tests/chainlock_validation_test.rs (4)
1-9: Remove duplicate module documentation.Lines 1-4 and 8-9 contain duplicate module-level documentation. The second documentation block on lines 8-9 should be removed.
//! Integration tests for ChainLock validation flow with masternode engine //! //! NOTE: This test file is currently disabled due to incomplete mock NetworkManager implementation. //! TODO: Re-enable once NetworkManager trait methods are fully implemented. #![cfg(feature = "skip_mock_implementation_incomplete")] -//! Integration tests for ChainLock validation flow with masternode engine - use dash_spv::client::{ClientConfig, DashSpvClient};
239-244: Inconsistent test termination.The test has multiple commented sections followed by an early return. This makes the test effectively a no-op. Consider either properly implementing the test with the available API or marking it with
#[ignore]until the API is ready.- // Process pending ChainLocks (skipped for now due to API changes) - // let chain_state = ChainState::new(); - // Note: storage_mut() is not available in current API - // let storage = client.storage_mut(); - // Skip this test section as it needs to be rewritten for the new client API - return; + // TODO: Complete test implementation once the following APIs are available: + // - storage_mut() for mutable storage access + // - ChainState construction + // - Process pending ChainLocks functionality + + // For now, we can only verify the basic setup succeeded + assert!(client.chainlock_manager().get_stats().total_chain_locks == 0);
341-355: Test assertions without proper setup.The test is checking for ChainLocks at height 0, but the setup code that would add them is commented out. This will likely cause the assertions to fail.
The test assertions expect a ChainLock at height 0, but the code that would add it (lines 331-338) is commented out. This means:
has_chain_lock_at_height(0)will return falseget_chain_lock_by_height(0)will return None, causing unwrap() to panicEither skip these assertions or provide an alternative way to set up the test data.
- // Test cache operations - assert!(chainlock_manager.has_chain_lock_at_height(0)); - - let entry = chainlock_manager.get_chain_lock_by_height(0); - assert!(entry.is_some()); - assert_eq!(entry.unwrap().chain_lock.block_height, 0); - - let entry_by_hash = chainlock_manager.get_chain_lock_by_hash(&genesis.block_hash()); - assert!(entry_by_hash.is_some()); - assert_eq!(entry_by_hash.unwrap().chain_lock.block_height, 0); + // Without the ability to add ChainLocks, we can only test empty state + assert!(!chainlock_manager.has_chain_lock_at_height(0)); + assert!(chainlock_manager.get_chain_lock_by_height(0).is_none()); + assert!(chainlock_manager.get_chain_lock_by_hash(&genesis.block_hash()).is_none());
174-413: Consider consolidating or removing ineffective tests.Based on the learnings that specify "All code must be clippy-clean", these tests with multiple early returns and commented-out code sections don't provide value and may cause maintenance burden. Consider either:
- Properly implementing the tests with the current API
- Consolidating them into a single documented test that lists all pending API requirements
- Removing them entirely until the API stabilizes
Would you like me to help refactor these tests to work with the current API, or create a single comprehensive test that documents all the pending API requirements?
🧹 Nitpick comments (14)
dash-spv/tests/block_download_test.rs (4)
27-27: Remove commented-out wallet import.
Keep the test file lean; commented code adds noise.Apply this diff:
-// use key_wallet::wallet::ManagedWalletInfo;
167-173: Delete long-commented helper (create_test_address).
Dead code; history keeps it if needed later.Apply this diff:
-// fn create_test_address() -> Address { -// use dashcore::{Address, PubkeyHash, ScriptBuf}; -// -// use dashcore_hashes::Hash; -// -// let pubkey_hash = PubkeyHash::from_slice(&[1u8; 20]).unwrap(); -// let script = ScriptBuf::new_p2pkh(&pubkey_hash); -// -// Address::from_script(&script, Network::Testnet).unwrap() -// }
387-387: Remove commented-out address creation.
Consistent with removing the helper above.Apply this diff:
- // let address = create_test_address();
372-375: Remove empty ignored test stub in dash-spv/tests/block_download_test.rs
Thetest_sync_manager_integrationstub is gated underskip_mock_implementation_incompleteand contains no implementation. Drop it to avoid dead code.-#[ignore = "mock implementation incomplete"] -#[tokio::test] -async fn test_sync_manager_integration() {}dash-spv/tests/rollback_test.rs (1)
58-58: Preferdrop(...)overlet _ = ...for ignored results
drop(...)makes the intent explicit, silencesunused_must_use, and avoids pedanticlet_underscore_*lints.Apply to all occurrences (lines 58, 62, 66, 72 of dash-spv/tests/rollback_test.rs):
- let _ = storage.get_tip_height().await?; + drop(storage.get_tip_height().await?); - let _ = storage.load_headers(0..10).await?; + drop(storage.load_headers(0..10).await?); - let _ = storage.get_header(6).await?; + drop(storage.get_header(6).await?); - let _ = storage.get_header_height_by_hash(&hash_of_removed_header).await?; + drop(storage.get_header_height_by_hash(&hash_of_removed_header).await?);Run pedantic clippy locally to confirm no regressions:
cargo clippy -p dash-spv --tests --all-features -D warnings -W clippy::pedanticdash-spv/tests/error_recovery_integration_test.rs (2)
514-520: Channel receiver dropped immediately; returned Sender will error on send.Keep the receiver alive or return a no-op sender implementation to avoid misleading behavior in tests.
fn get_message_sender( &self, ) -> tokio::sync::mpsc::Sender<dashcore::network::message::NetworkMessage> { - let (_tx, _rx) = tokio::sync::mpsc::channel(1); - _tx + let (tx, _rx) = tokio::sync::mpsc::channel(1); + // Store _rx somewhere (e.g., self) or use an unbounded channel just for test stubs. + tx }
6-6: Invert or rename theskip_mock_implementation_incompletegate
Theskip_mock_implementation_incompletefeature (dash-spv/Cargo.toml:71) is currently used as#![cfg(feature = "skip_mock_implementation_incomplete")]which compiles these tests when enabled—opposite of its “skip” name. To align intent, either:
- Rename to a positive flag (e.g.
integration-tests) and gate via#- Or invert the existing gate:
#![cfg(not(feature = "skip_mock_implementation_incomplete"))]dash-spv/tests/instantsend_integration_test.rs (3)
115-116: UseInstantLockValidator::default()(idiomatic, avoids “new” when Default exists).Minor idiomatic cleanup aligned with recent API.
- let validator = dash_spv::validation::InstantLockValidator::new(); + let validator = dash_spv::validation::InstantLockValidator::default(); @@ - let validator = dash_spv::validation::InstantLockValidator::new(); + let validator = dash_spv::validation::InstantLockValidator::default();Also applies to: 170-171
100-102: Consider adding a minimal dummy input tospend_txfor realism.Zero-input txs can trip invariants in downstream validation.
- let spend_tx = create_regular_transaction(vec![], vec![(80_000_000, ScriptBuf::new())]); + let dummy_input = OutPoint { txid: Txid::from_byte_array([1; 32]), vout: 0 }; + let spend_tx = create_regular_transaction(vec![dummy_input], vec![(80_000_000, ScriptBuf::new())]);
1-1: Gate incomplete SPV tests behind a dedicated non-default feature
Replace the#![cfg(feature = "skip_mock_implementation_incomplete")]annotation indash-spv/tests/*.rs(e.g. line 1 of instantsend_integration_test.rs) with a positive, non-default feature—such asintegration-tests—or invert the existing flag (#![cfg(not(feature = "skip_mock_implementation_incomplete"))]) so that these files remain excluded when running Clippy with--all-features.dash-spv/tests/chainlock_validation_test.rs (4)
15-15: Consider removing commented-out imports.Instead of keeping the commented import, consider removing it entirely if the
MasternodeListEngineis no longer needed. If it's intended for future use, add a TODO comment explaining when it will be re-enabled.-// use dashcore::sml::masternode_list_engine::MasternodeListEngine; +// TODO: Re-enable MasternodeListEngine import once API stabilizes +// use dashcore::sml::masternode_list_engine::MasternodeListEngine;
229-237: Clean up commented code blocks.Multiple lines of commented code should either be removed or properly documented with TODOs if they're intended for future implementation.
- // let mock_engine = MasternodeListEngine::default_for_network(Network::Dash); - - // Update the ChainLock manager with the engine let updated = client.update_chainlock_validation().unwrap(); assert!(!updated); // Should be false since we don't have a real engine - // For testing, directly set a mock engine - // let engine_arc = Arc::new(mock_engine); - // client.chainlock_manager().set_masternode_engine(engine_arc); + // TODO: Once the API supports it, test setting a mock masternode engine: + // - Create MasternodeListEngine::default_for_network(Network::Dash) + // - Set it via client.chainlock_manager().set_masternode_engine()
331-338: Improve handling of disabled test sections.The commented code blocks make the test incomplete. Consider using conditional compilation or feature flags to handle API availability.
- // let storage = client.storage(); - // storage.store_header(&genesis, 0).await.unwrap(); - - // Create and process a ChainLock - skip for now as storage access pattern changed - // let chain_lock = create_test_chainlock(0, genesis.block_hash()); - // let chain_state = ChainState::new(); - // Note: storage access pattern has changed in the new client API - // let _ = chainlock_manager.process_chain_lock(chain_lock.clone(), &chain_state, storage).await; + #[cfg(feature = "test_with_storage_access")] + { + let storage = client.storage(); + storage.lock().await.store_header(&genesis, 0).await.unwrap(); + + let chain_lock = create_test_chainlock(0, genesis.block_hash()); + // Process the ChainLock when API is available + }
401-402: Remove unnecessary empty comment.The comment line 402 with just
//serves no purpose.// Create a mock masternode list engine - // let mock_engine = MasternodeListEngine::default_for_network(Network::Dash); - + // TODO: let mock_engine = MasternodeListEngine::default_for_network(Network::Dash);
📜 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 (8)
dash-spv/src/client/block_processor_test.rs(3 hunks)dash-spv/tests/block_download_test.rs(4 hunks)dash-spv/tests/cfheader_gap_test.rs(2 hunks)dash-spv/tests/chainlock_validation_test.rs(12 hunks)dash-spv/tests/error_handling_test.rs(1 hunks)dash-spv/tests/error_recovery_integration_test.rs(2 hunks)dash-spv/tests/instantsend_integration_test.rs(3 hunks)dash-spv/tests/rollback_test.rs(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- dash-spv/tests/error_handling_test.rs
- dash-spv/tests/cfheader_gap_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/src/client/block_processor_test.rs
🧰 Additional context used
📓 Path-based instructions (5)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Place integration tests under the
tests/directory
Files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.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 in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
{dash-spv,rpc-integration-test}/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for network operations
Files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
🧠 Learnings (19)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/tests/rollback_test.rsdash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/tests/error_recovery_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the `tests/` directory
Applied to files:
dash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/integration_real_node_test.rs : Integration tests that connect to a live Dash Core node must gracefully skip when the node at 127.0.0.1:9999 is unavailable
Applied to files:
dash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Applied to files:
dash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Integration tests must live under tests/ (e.g., rpc-integration-test)
Applied to files:
dash-spv/tests/error_recovery_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
Applied to files:
dash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/**/*.rs : Always validate network consistency; never mix mainnet and testnet operations
Applied to files:
dash-spv/tests/error_recovery_integration_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs
Applied to files:
dash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
dash-spv/tests/block_download_test.rsdash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/mnemonic_tests.rs : Place mnemonic generation and validation tests in tests/mnemonic_tests.rs
Applied to files:
dash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Applied to files:
dash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/tests/chainlock_validation_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-06-26T16:01:37.609Z
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.
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
🧬 Code graph analysis (4)
dash-spv/tests/error_recovery_integration_test.rs (1)
dash-spv/src/client/mod.rs (1)
storage(2379-2381)
dash-spv/tests/block_download_test.rs (1)
dash-spv/src/client/mod.rs (3)
network(418-420)network(1219-1222)storage(2379-2381)
dash-spv/tests/chainlock_validation_test.rs (3)
dash-spv/src/client/mod.rs (5)
storage(2379-2381)new(174-237)network(418-420)network(1219-1222)wallet(105-107)dash-spv/src/chain/chainlock_manager.rs (1)
new(53-62)dash-spv/src/storage/disk.rs (1)
new(135-249)
dash-spv/tests/instantsend_integration_test.rs (2)
dash-spv/src/client/mod.rs (1)
wallet(105-107)key-wallet-manager/src/wallet_manager/mod.rs (2)
new(70-76)new(107-114)
⏰ 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). (16)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: Core Components Tests
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (7)
dash-spv/tests/block_download_test.rs (2)
19-20: LGTM: explicit BlockHash import improves readability.
Nothing else to change here.
24-26: LGTM: concise, targeted imports from dash_spv.
Matches current test usages.dash-spv/tests/rollback_test.rs (1)
103-103: Usedrop(...)instead of assigning to_Refactor the three calls in
dash-spv/tests/rollback_test.rs(lines 103, 107, 111) to usedrop(...), preventing Clippy warnings across all configurations:- let _ = storage.get_filter_tip_height().await?; + drop(storage.get_filter_tip_height().await?); - let _ = storage.load_filter_headers(0..10).await?; + drop(storage.load_filter_headers(0..10).await?); - let _ = storage.get_filter_header(4).await?; + drop(storage.get_filter_header(4).await?);Please verify Clippy is clean after these changes.
dash-spv/tests/chainlock_validation_test.rs (4)
12-13: Simplified imports look good.The consolidation of imports to use
dash_spv::storage::DiskStorageManageranddash_spv::types::ValidationModedirectly is cleaner and aligns with the clippy recommendations.
199-199: Good use of.await.unwrap()pattern.The change from synchronous to async initialization with proper await handling aligns with the tokio runtime requirements mentioned in the coding guidelines.
220-220: Proper async client initialization.The updated client initialization properly uses the async constructor with await, following the new API pattern.
257-257: Consistent async storage initialization.All storage manager initializations have been properly updated to use the async constructor. This is consistent across all test functions.
Also applies to: 310-310, 368-368, 384-384
| // Placeholder: test requires API updates; skip for now | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early return pattern needs proper documentation.
The early return with just a comment "Placeholder: test requires API updates; skip for now" doesn't provide enough context. Consider adding more details or using the #[ignore] attribute with a descriptive message instead.
+ // TODO: This test is temporarily disabled pending API updates for:
+ // - Access to pending_chainlocks field (currently private)
+ // - Updated storage access patterns
// Placeholder: test requires API updates; skip for now
return;📝 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.
| // Placeholder: test requires API updates; skip for now | |
| return; | |
| // TODO: This test is temporarily disabled pending API updates for: | |
| // - Access to pending_chainlocks field (currently private) | |
| // - Updated storage access patterns | |
| // Placeholder: test requires API updates; skip for now | |
| return; |
🤖 Prompt for AI Agents
In dash-spv/tests/chainlock_validation_test.rs around lines 179 to 180, the test
contains an early return with only the comment "Placeholder: test requires API
updates; skip for now" which lacks context; replace the early return by either
marking the test with #[ignore] and a one-line comment explaining why (e.g., API
changes pending, reference issue/PR number) or add a doc comment above the test
detailing the required API changes and expected follow-up, and include a TODO
with issue/PR link so future reviewers know when to re-enable and how to address
it.
| // Process pending (skipped for now due to API changes) | ||
| // Skip this test as it needs to be rewritten for the new client API | ||
| return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Another early return without proper test coverage.
Similar to previous tests, this test ends with an early return, making it ineffective.
Consider implementing at least a basic assertion before returning, or combine these placeholder tests into a single test that documents all pending API requirements:
- // Process pending (skipped for now due to API changes)
- // Skip this test as it needs to be rewritten for the new client API
- return;
+ // Verify that ChainLocks were queued (even if we can't access the internal state)
+ // This at least ensures the queue operations don't panic
+ assert!(chainlock_manager.get_stats().total_chain_locks == 0);
+
+ // TODO: Once pending_chainlocks becomes accessible, verify:
+ // - All 3 ChainLocks are in the queue
+ // - They maintain the correct order (heights 100, 200, 300)📝 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.
| // Process pending (skipped for now due to API changes) | |
| // Skip this test as it needs to be rewritten for the new client API | |
| return; | |
| // Verify that ChainLocks were queued (even if we can't access the internal state) | |
| // This at least ensures the queue operations don't panic | |
| assert!(chainlock_manager.get_stats().total_chain_locks == 0); | |
| // TODO: Once pending_chainlocks becomes accessible, verify: | |
| // - All 3 ChainLocks are in the queue | |
| // - They maintain the correct order (heights 100, 200, 300) |
🤖 Prompt for AI Agents
In dash-spv/tests/chainlock_validation_test.rs around lines 295–297, the test
contains an early return that makes it a no-op; replace the early return with a
minimal meaningful assertion or consolidate this and other placeholder tests
into one documented placeholder test: either (a) perform a basic assertion that
validates a small, deterministic behavior (e.g., assert expected error or
assert_true on a constructed value) before returning, or (b) remove the early
return and annotate a single representative test with #[ignore] and a clear TODO
comment describing required API changes so the CI still records it as pending;
ensure the test file compiles and test intent is documented.
| let temp_dir = tempfile::tempdir().unwrap(); | ||
| let storage = Arc::new(RwLock::new(DiskStorageManager::new(storage_path).await.unwrap())); | ||
|
|
There was a problem hiding this comment.
Fix: undefined variable storage_path breaks compilation.
Initialize storage_path from temp_dir before constructing DiskStorageManager.
- let temp_dir = tempfile::tempdir().unwrap();
- let storage = Arc::new(RwLock::new(DiskStorageManager::new(storage_path).await.unwrap()));
+ let temp_dir = tempfile::tempdir().unwrap();
+ let storage_path = temp_dir.path().to_path_buf();
+ let storage = Arc::new(RwLock::new(DiskStorageManager::new(storage_path).await.unwrap()));📝 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 temp_dir = tempfile::tempdir().unwrap(); | |
| let storage = Arc::new(RwLock::new(DiskStorageManager::new(storage_path).await.unwrap())); | |
| let temp_dir = tempfile::tempdir().unwrap(); | |
| let storage_path = temp_dir.path().to_path_buf(); | |
| let storage = Arc::new(RwLock::new(DiskStorageManager::new(storage_path).await.unwrap())); |
🤖 Prompt for AI Agents
In dash-spv/tests/error_recovery_integration_test.rs around lines 125 to 127,
the code uses an undefined variable storage_path when creating
DiskStorageManager; initialize storage_path from the temp_dir created earlier
(e.g., let storage_path = temp_dir.path().to_path_buf();) before calling
DiskStorageManager::new(storage_path).await.unwrap() so the manager receives a
valid PathBuf and temp_dir remains in scope for the test.
| Address, InstantLock, Network, OutPoint, ScriptBuf, Transaction, TxIn, TxOut, Txid, Witness, | ||
| }; | ||
| use dashcore_hashes::{sha256d, Hash}; | ||
| use dashcore_hashes::Hash; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop unused dashcore_hashes::Hash import.
Cleans clippy::unused_imports.
-use dashcore_hashes::Hash;📝 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 dashcore_hashes::Hash; |
🤖 Prompt for AI Agents
In dash-spv/tests/instantsend_integration_test.rs around line 27, the import
dashcore_hashes::Hash is unused; remove that unused import to satisfy
clippy::unused_imports and keep imports minimal.
- Remove unused imports/vars across tests\n- Simplify test scaffolding to avoid dead_code\n- Adjust mempool filter test enums to unit variants\n- Normalize ignored tests to a single #[ignore] and discard unused results explicitly\n\ncargo check -p dash-spv --all-features --lib --bins --tests now runs clean
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
dash-spv/tests/instantsend_integration_test.rs (2)
23-23: Remove unuseduse dash_spv;.The crate root can be referenced directly (e.g.,
dash_spv::...); this import is unnecessary and triggers clippy::unused_imports.-use dash_spv; // keep module path available for validator usage
27-27: Drop unuseddashcore_hashes::Hashimport.Not referenced anywhere in this file; remove to satisfy clippy::unused_imports.
-use dashcore_hashes::Hash;
🧹 Nitpick comments (18)
dash-spv/tests/instantsend_integration_test.rs (4)
19-20: Delete commented-out imports to reduce noise.These commented imports add clutter and provide no value.
-// use std::sync::Arc; -// use tokio::sync::RwLock;
28-29: Remove commented-out key-wallet imports.Dead commented code invites drift and complicates diffs.
-// use key_wallet::wallet::managed_wallet_info::ManagedWalletInfo; -// use key_wallet_manager::wallet_manager::WalletManager;
98-99: Ensure InstantLock has at least one input.Creating
spend_txwith no inputs likely yields an emptyInstantLock.inputs, which may fail validation when this test is enabled. Add a dummy input like you do later forincoming_tx.- // 1. Create a dummy spending transaction (skipped wallet operations due to API changes) - let spend_tx = create_regular_transaction(vec![], vec![(80_000_000, ScriptBuf::new())]); + // 1. Create a dummy spending transaction with a placeholder input so the InstantLock has inputs + let dummy_input = OutPoint { + txid: Txid::from_byte_array([42; 32]), + vout: 0, + }; + let spend_tx = + create_regular_transaction(vec![dummy_input], vec![(80_000_000, ScriptBuf::new())]);
154-156: Track TODOs with an issue and remove commented code.Convert TODOs into a tracked issue reference and delete commented-out calls to keep the test lean.
Want me to open an issue and add the reference here?
dash-spv/tests/cfheader_gap_test.rs (3)
154-154: Prefer instantiating a no-op FilterSyncManager over leaving a placeholderEven if the restart logic isn’t implemented, constructing the manager ensures type paths stay valid and keeps clippy honest on this file.
- // FilterSyncManager instantiation omitted until restart logic is implemented + // Keep type sanity even before restart logic lands + let _filter_sync: FilterSyncManager<MemoryStorageManager, MultiPeerNetworkManager> = + FilterSyncManager::new(&config, Arc::new(Mutex::new(HashSet::new())));
175-259: Remove or move the commented-out MockNetworkManagerA 80+ line commented block will drift. Either delete it (git keeps history) or move a minimal stub into a
tests/support/mock_network.rsbehind#[cfg(test)], then re-enable targeted tests when ready.I can extract a minimal
NetworkManagerstub that compiles under tests and wire it here when you’re ready.
261-261: Trim redundant comment or reference a tracking issueReplace with a TODO including an issue/PR reference for when restart logic is implemented.
- // Network manager omitted until restart logic is implemented + // TODO(dash-spv#<issue>): add mock network once restart logic is implementeddash-spv/tests/rollback_test.rs (2)
18-19: Redundant test-disabling: feature gate + #[ignore].The whole file is gated by the feature; adding #[ignore] is probably unnecessary. Prefer one mechanism to avoid confusion.
Apply if desired:
-#[ignore = "mock implementation incomplete"] +// #[ignore] not needed since this module is behind a feature gate
78-78: Same note as above: feature-gated test also marked #[ignore].Consider removing #[ignore] to reduce redundancy; the feature already excludes it by default.
dash-spv/tests/error_handling_test.rs (1)
58-58: Ack: removed unused setter.Given fields are local to this module, flipping flags directly is fine. If you plan to reuse this mock elsewhere, consider adding minimal setters back to avoid exposing fields.
dash-spv/src/mempool_filter.rs (8)
39-47: Remove redundant HashSet->HashSet collect in constructor
watched_addresses.into_iter().collect()needlessly rebuilds the same set. Move the owned set directly.Self { strategy, recent_send_window, max_transactions, mempool_state, - watched_addresses: watched_addresses.into_iter().collect(), + watched_addresses, network, }
74-83: Avoid unnecessary clone when inserting into local set
addressis owned in this scope; insert it directly into the set.- if let Ok(address) = Address::from_script(&output.script_pubkey, self.network) { - addresses.insert(address.clone()); + if let Ok(address) = Address::from_script(&output.script_pubkey, self.network) { + addresses.insert(address); tracing::trace!("Transaction {} output {} has address: {}", txid, idx, address); }
209-212: Prefer fieldless enum variants for placeholdersUse
Script/Outpoint(fieldless) instead ofScript(())/Outpoint(())for clarity and zero-sized efficiency.enum WatchItem { Address(Address), - // Keep placeholders as unit to avoid dead_code warnings - Script(()), - Outpoint(()), + // Placeholders until wallet-backed script/outpoint watching lands + Script, + Outpoint, }
223-225: Align constructor with fieldless Script variantReturn the fieldless variant.
- fn script(_script: ScriptBuf) -> Self { - WatchItem::Script(()) - } + fn script(_script: ScriptBuf) -> Self { + WatchItem::Script + }
227-229: Align constructor with fieldless Outpoint variant- fn outpoint(_outpoint: OutPoint) -> Self { - WatchItem::Outpoint(()) - } + fn outpoint(_outpoint: OutPoint) -> Self { + WatchItem::Outpoint + }
237-241: Drop unusednetworkparameter; provide Default
MockWallet::new(_network: Network)ignores its arg. Prefernew()and/orDefaultto avoid clippy’s unused parameter lint in tests.- fn new(_network: Network) -> Self { + fn new() -> Self { Self { watched_addresses: HashSet::new(), } }Also add (outside this hunk):
impl Default for MockWallet { fn default() -> Self { Self { watched_addresses: HashSet::new() } } }And update call sites in this file:
- let mut wallet = MockWallet::new(network); + let mut wallet = MockWallet::new();- let mut wallet = MockWallet::new(network); + let mut wallet = MockWallet::new();
247-248: Comment is fine; consider documenting MockWallet’s minimal scopeIf you keep MockWallet intentionally minimal, a brief doc comment explaining it only tracks addresses (not scripts/outpoints) will prevent future confusion.
666-672: Replace tautological assert with a no-op validation
assert!(pruned.is_empty() || !pruned.is_empty())is tautological and may trip clippy. If the goal is “no panic,” simply exercise the function and ensure types line up.- assert!(pruned.is_empty() || !pruned.is_empty()); // Tautology, but shows the test ran + let _ = pruned; // Method executed; detailed pruning verified elsewhere with time control
📜 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 (7)
dash-spv/src/mempool_filter.rs(2 hunks)dash-spv/tests/block_download_test.rs(4 hunks)dash-spv/tests/cfheader_gap_test.rs(4 hunks)dash-spv/tests/error_handling_test.rs(2 hunks)dash-spv/tests/error_recovery_integration_test.rs(3 hunks)dash-spv/tests/instantsend_integration_test.rs(3 hunks)dash-spv/tests/rollback_test.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-spv/tests/error_recovery_integration_test.rs
- dash-spv/tests/block_download_test.rs
🧰 Additional context used
📓 Path-based instructions (6)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Enforce Rust formatting viacargo fmt --checkon all Rust source files
All code must be clippy-clean: runcargo clippy --all-targets --all-features -- -D warnings
Files:
dash-spv/src/mempool_filter.rsdash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.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 in Rust
Use conditional compilation feature flags for optional features (#[cfg(feature = ...)])
Format Rust code with cargo fmt (and enforce via cargo fmt --check)
Run clippy with -D warnings and fix all lints
Adhere to MSRV Rust 1.89 (avoid features requiring newer compiler)
**/*.rs: Format Rust code with rustfmt (per rustfmt.toml); run cargo fmt --all before commits
Lint with clippy; treat warnings as errors in CI
Follow Rust naming: snake_case for functions/variables, UpperCamelCase for types/traits, SCREAMING_SNAKE_CASE for consts
Prefer async using tokio where applicable
Files:
dash-spv/src/mempool_filter.rsdash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/src/**/*.rs: Each crate keeps sources in src/
Avoid unwrap()/expect() in library code; use proper error types (e.g., thiserror)
Place unit tests alongside code with #[cfg(test)]
Files:
dash-spv/src/mempool_filter.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Place integration tests under the
tests/directory
Files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
{dash-spv,rpc-integration-test}/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Provide integration tests for network operations
Files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use proptest for property-based testing where appropriate
**/tests/**/*.rs: Integration tests must live under tests/ (e.g., rpc-integration-test)
Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Mark network-dependent or long-running tests with #[ignore] and run with -- --ignored
Add regression tests for fixes; consider property tests (e.g., proptest) where valuable
Keep test vectors deterministic
Files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
🧠 Learnings (25)
📓 Common learnings
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
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.
📚 Learning: 2025-06-26T16:01:37.609Z
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.
Applied to files:
dash-spv/src/mempool_filter.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/address_tests.rs : Place address generation and validation tests in tests/address_tests.rs
Applied to files:
dash-spv/src/mempool_filter.rsdash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:00:14.154Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.154Z
Learning: Applies to {dash-spv,rpc-integration-test}/tests/**/*.rs : Provide integration tests for network operations
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Keep test vectors in sync with Dash Core releases
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/rollback_test.rsdash-spv/tests/cfheader_gap_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : All code must be clippy-clean: run `cargo clippy --all-targets --all-features -- -D warnings`
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/**/*.rs : Place integration tests under the `tests/` directory
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add a corresponding Rust unit test for each new FFI function
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rsdash-spv/tests/cfheader_gap_test.rs
📚 Learning: 2025-09-01T14:44:53.683Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T14:44:53.683Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., test_parse_address_mainnet) for integration tests
Applied to files:
dash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/error.rs : Define and maintain domain-specific error types in `src/error.rs` (NetworkError, StorageError, SyncError, ValidationError, SpvError) and update them when adding new failure modes
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/cfheader_gap_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/psbt.rs : Place PSBT serialization/deserialization tests in tests/psbt.rs
Applied to files:
dash-spv/tests/error_handling_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/bip32_tests.rs : Organize unit tests so key-derivation correctness lives in tests/bip32_tests.rs
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Provide both `MemoryStorageManager` and `DiskStorageManager` behind the `StorageManager` trait
Applied to files:
dash-spv/tests/error_handling_test.rsdash-spv/tests/rollback_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{bip32.rs,dip9.rs} : Keep Dash-specific derivation paths up to date (e.g., DIP-9) in bip32.rs and dip9.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/**/*.rs : Include integration tests that cover full wallet workflows
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:15:57.225Z
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
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Expose complex Rust types to C as opaque pointers (e.g., FFIDashSpvClient*)
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Functions returning pointers transfer ownership to the caller; functions taking pointers only borrow
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-16T04:14:07.718Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-08-16T04:14:07.718Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Provide corresponding _destroy() functions for all FFI-owned types to enforce explicit memory management
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/**/*.rs : Enforce Rust formatting via `cargo fmt --check` on all Rust source files
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/mnemonic_tests.rs : Place mnemonic generation and validation tests in tests/mnemonic_tests.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-25T17:15:59.361Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-08-25T17:15:59.361Z
Learning: Applies to dash-spv/tests/integration_real_node_test.rs : Integration tests that connect to a live Dash Core node must gracefully skip when the node at 127.0.0.1:9999 is unavailable
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/**/*.rs : Always validate network consistency; never mix mainnet and testnet operations
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/tests/derivation_tests.rs : Place path derivation tests in tests/derivation_tests.rs
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
📚 Learning: 2025-08-29T04:01:18.111Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.111Z
Learning: Applies to key-wallet/src/{managed_account,transaction_checking}/**/*.rs : Update wallet state atomically: add transaction, update UTXOs, recalculate balance, and mark addresses used in a single operation
Applied to files:
dash-spv/tests/instantsend_integration_test.rs
🧬 Code graph analysis (1)
dash-spv/tests/cfheader_gap_test.rs (1)
dash-spv/src/client/mod.rs (3)
network(418-420)network(1219-1222)storage(2379-2381)
⏰ 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). (19)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: RPC Tests (stable, true)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
🔇 Additional comments (8)
dash-spv/tests/instantsend_integration_test.rs (2)
25-26: Imports look good.Consolidated dashcore imports are minimal and used.
33-33: Comment OK.Note about removed helper is fine; no action needed.
dash-spv/tests/cfheader_gap_test.rs (1)
17-17: Imports for dashcore types look gooddash-spv/tests/rollback_test.rs (1)
57-70: Drop must_use return values instead of usinglet _ =
Replace eachlet _ = storage.method().await?;withdrop(storage.method().await?);to avoid clippy::let_underscore_must_use- let _ = storage.get_tip_height().await?; + drop(storage.get_tip_height().await?); - let _ = storage.load_headers(0..10).await?; + drop(storage.load_headers(0..10).await?); - let _ = storage.get_header(6).await?; + drop(storage.get_header(6).await?); - let _ = storage.get_header_height_by_hash(&hash_of_removed_header).await?; + drop(storage.get_header_height_by_hash(&hash_of_removed_header).await?);dash-spv/tests/error_handling_test.rs (3)
20-20: Import cleanup LGTM.Reducing std::time imports to Duration only is correct and keeps the surface minimal.
26-26: dashcore import pruning looks good.Keeping only BlockHash, Network, OutPoint, Txid addresses unused-import lints without losing clarity.
34-34: Storage imports simplified appropriately.Dropping unused MemoryStorageManager/Utxo aligns with the test’s usage.
dash-spv/src/mempool_filter.rs (1)
79-83: ConfirmAddress::from_scriptsignature and adjust pattern matching
The repository search didn’t locate thefrom_scriptdefinition—manually verify whether it returnsOptionorResultand update theif letpatterns indash-spv/src/mempool_filter.rs(lines 79–83 and 147–153) accordingly.
| use dash_spv::{ | ||
| client::ClientConfig, | ||
| error::{NetworkError, NetworkResult}, | ||
| network::{MultiPeerNetworkManager, NetworkManager}, | ||
| network::MultiPeerNetworkManager, | ||
| storage::{MemoryStorageManager, StorageManager}, | ||
| sync::filters::FilterSyncManager, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused StorageManager import to keep clippy -D warnings passing
StorageManager isn’t referenced in this file. This will trigger an unused_import warning in tests.
use dash_spv::{
client::ClientConfig,
network::MultiPeerNetworkManager,
- storage::{MemoryStorageManager, StorageManager},
+ storage::MemoryStorageManager,
sync::filters::FilterSyncManager,
};📝 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 dash_spv::{ | |
| client::ClientConfig, | |
| error::{NetworkError, NetworkResult}, | |
| network::{MultiPeerNetworkManager, NetworkManager}, | |
| network::MultiPeerNetworkManager, | |
| storage::{MemoryStorageManager, StorageManager}, | |
| sync::filters::FilterSyncManager, | |
| }; | |
| use dash_spv::{ | |
| client::ClientConfig, | |
| network::MultiPeerNetworkManager, | |
| storage::MemoryStorageManager, | |
| sync::filters::FilterSyncManager, | |
| }; |
🤖 Prompt for AI Agents
In dash-spv/tests/cfheader_gap_test.rs around lines 11 to 16, the import list
includes StorageManager which is unused; remove StorageManager from the use
statement (i.e., delete ", StorageManager" from the import list) so the test no
longer triggers an unused_import/clippy warning.
| BlockHash, Network, | ||
| }; | ||
| use dashcore::{block::Header as BlockHeader, hash_types::FilterHeader, BlockHash, Network}; | ||
| use dashcore_hashes::Hash; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop unused dashcore_hashes::Hash import
Not used; remove to avoid unused_import warnings in test target.
-use dashcore_hashes::Hash;📝 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 dashcore_hashes::Hash; |
🤖 Prompt for AI Agents
In dash-spv/tests/cfheader_gap_test.rs around line 18, the import use
dashcore_hashes::Hash is unused in the test; remove that import line to
eliminate the unused_import warning (or if needed later, replace with the
actually used item). Ensure no other references to Hash remain in this test file
after removal.
| let _ = storage.get_filter_tip_height().await?; | ||
| // assert_eq!(filter_tip_after_rollback, Some(3)); | ||
|
|
||
| // Verify we can only load filter headers up to height 3 | ||
| let filter_headers_after_rollback = storage.load_filter_headers(0..10).await?; | ||
| let _ = storage.load_filter_headers(0..10).await?; | ||
| // assert_eq!(filter_headers_after_rollback.len(), 4); // heights 0-3 | ||
|
|
||
| // Verify filter header at height 4 is not accessible | ||
| let filter_header_at_4 = storage.get_filter_header(4).await?; | ||
| let _ = storage.get_filter_header(4).await?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use drop(...) instead of let _ = …? to silence must_use cleanly.
This mirrors the earlier hunk and keeps the file clippy-clean under the feature.
Proposed fix:
- let _ = storage.get_filter_tip_height().await?;
+ drop(storage.get_filter_tip_height().await?);
- let _ = storage.load_filter_headers(0..10).await?;
+ drop(storage.load_filter_headers(0..10).await?);
- let _ = storage.get_filter_header(4).await?;
+ drop(storage.get_filter_header(4).await?);📝 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 _ = storage.get_filter_tip_height().await?; | |
| // assert_eq!(filter_tip_after_rollback, Some(3)); | |
| // Verify we can only load filter headers up to height 3 | |
| let filter_headers_after_rollback = storage.load_filter_headers(0..10).await?; | |
| let _ = storage.load_filter_headers(0..10).await?; | |
| // assert_eq!(filter_headers_after_rollback.len(), 4); // heights 0-3 | |
| // Verify filter header at height 4 is not accessible | |
| let filter_header_at_4 = storage.get_filter_header(4).await?; | |
| let _ = storage.get_filter_header(4).await?; | |
| drop(storage.get_filter_tip_height().await?); | |
| // assert_eq!(filter_tip_after_rollback, Some(3)); | |
| // Verify we can only load filter headers up to height 3 | |
| drop(storage.load_filter_headers(0..10).await?); | |
| // assert_eq!(filter_headers_after_rollback.len(), 4); // heights 0-3 | |
| // Verify filter header at height 4 is not accessible | |
| drop(storage.get_filter_header(4).await?); |
🤖 Prompt for AI Agents
In dash-spv/tests/rollback_test.rs around lines 101 to 109, the test currently
uses "let _ = <future>.await?;" to ignore must_use results; replace those with
drop(<future>.await?); for each occurrence (getting filter tip height, loading
filter headers, and getting filter header at height 4) so the code cleanly
silences the value and remains clippy-clean under the feature.
|
merging this in because it's an improvement. |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores