refactor: clippy warnings resolution #125
Conversation
- Remove always-true assertion in test_get_quorum_info() - Fixes absurd_extreme_comparisons clippy error - Integration test now passes clippy with warnings only
- Remove redundant field names in struct initialization - Remove unused imports (Script, RawTx, BsFields) - Elide unnecessary explicit lifetimes in trait implementations - Remove needless borrowing in function calls - Remove redundant Ok(?) patterns in Result handling - Fix .into_iter() vs .iter() usage on references - Replace .nth(0) with .next() for better readability - Use std::slice::from_ref instead of cloning for single elements - Remove unnecessary reference taking in comparisons - Remove unnecessary let bindings for unit values - Remove identity mapping in iterators - Use !is_empty() instead of != "" for clarity - Remove unnecessary return statements at end of functions This reduces total clippy warnings from ~600+ to ~584 across all crates
- Convert single-binding match to let statement in test_get_protx_info - Remove unnecessary .to_string() calls in format! and trace! macros - Replace .to_vec().into_iter().map() with .iter().cloned().map() for better performance All safe clippy warning fixes have been completed across non-strict crates
- Remove redundant closure in get_transaction_are_locked - Fix useless type conversion (remove .into() on string literal) - Change &Vec to &[T] for better performance in function signature - Remove unnecessary reference taking in pattern matching - Fix remaining needless borrows in node management functions - Verified remaining "redundant imports" are actually used (false positives) Further reduces clippy warnings across the codebase
WalkthroughThis PR removes lifetimes from RawTx impls, adds RawTx for String, updates RpcApi methods to call self.call directly and adjust iter/slice usage, changes a method signature to take a slice, refactors tests for references/slices and logging, and tweaks a serialization match binding. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as RpcApi impl
participant Transport as HTTP/JSON-RPC
participant Node as RPC Server
rect rgb(230,245,255)
note over Client: Updated pattern: direct return of self.call(...)
Client->>Transport: call(method, params)
Transport->>Node: JSON-RPC request
Node-->>Transport: JSON-RPC response
Transport-->>Client: Result<T>
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rpc-client/src/client.rs (1)
527-532: Use numeric serialization for asset unlock indices.RPC expects numeric integers for the
indicesparameter; replace the manual string mapping withinto_json(indices)?to emit JSON numbers:- let indices_json = indices - .iter() - .map(|index| into_json(index.to_string())) - .collect::<Result<Vec<Value>>>()?; - let args = [indices_json.into(), opt_into_json(height)?]; + let args = [into_json(indices)?, opt_into_json(height)?];
🧹 Nitpick comments (2)
rpc-client/src/client.rs (2)
513-518: Simplify JSON args construction.You can let serde build the array directly and avoid the map/collect.
Apply:
- let transaction_ids_json = tx_ids.iter().map(into_json).collect::<Result<Vec<Value>>>()?; - let args = [transaction_ids_json.into()]; + let args = [into_json(tx_ids)?];
1112-1112: Make fee-estimate error check resilient.Exact-string match is brittle across Core versions.
Apply:
- if errors == ["Insufficient data or no feerate found"] { + if errors.iter().any(|e| e.contains("Insufficient data") || e.contains("no feerate")) {
📜 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 (3)
rpc-client/src/client.rs(16 hunks)rpc-integration-test/src/main.rs(26 hunks)rpc-json/src/lib.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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)
Files:
rpc-json/src/lib.rsrpc-integration-test/src/main.rsrpc-client/src/client.rs
🧠 Learnings (14)
📓 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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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:00:14.140Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-29T04:00:14.140Z
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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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/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/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:
rpc-integration-test/src/main.rs
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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
📚 Learning: 2025-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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-08-29T04:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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:01:18.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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.082Z
Learnt from: CR
PR: dashpay/rust-dashcore#0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-08-29T04:01:18.082Z
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
🧬 Code graph analysis (1)
rpc-client/src/client.rs (1)
rpc-client/src/queryable.rs (4)
query(22-22)query(28-33)query(39-44)query(50-52)
⏰ 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: RPC Tests (stable, true)
- GitHub Check: Strict Warnings and Clippy Checks
- GitHub Check: Core Components Tests
- GitHub Check: SPV Components Tests
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (dash_deserialize_amount)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
🔇 Additional comments (29)
rpc-json/src/lib.rs (1)
1057-1068: Match binding cleanup is correct.Binding
Address(addr)removes an unnecessary ref without changing semantics. LGTM.rpc-client/src/client.rs (24)
117-145: Lifetime on return slice makes intent explicit.Signature and logic read cleanly. LGTM.
161-183: RawTx impls without explicit lifetimes look good.Covers &Transaction, &[u8], &Vec<u8], &str. Encodings are correct. LGTM.
229-229: Remove needless Ok-wrapping — good.Directly returning
T::query(self, id)is cleaner. LGTM.
327-327: Returnself.callresult directly.Concise and correct. LGTM.
369-372: Field-init shorthand in get_block_template is fine.No behavior change. LGTM.
501-502: Direct call in get_balances.Simpler, same behavior. LGTM.
780-782: Vectorizing RawTx hexes — good.
iter().cloned()ensuresraw_hex()ownership. LGTM.
894-905: addnode subcommands via second arg — good.Clearer than distinct RPCs. LGTM.
908-909: disconnectnode arg handling — good.Matches RPC signature. LGTM.
931-931: getnodeaddresses defaulting — good.
unwrap_or(1)matches Core default. LGTM.
1695-1695: Useas_deref()when building request.Neat and idiomatic. LGTM.
1741-1741: Use string literal in test URL.Removes needless allocation. LGTM.
703-703: Use of returned blocks — fine.Keeps the test deterministic. LGTM.
733-733: Conditional get_block_filter — fine.Version gate is correct. LGTM.
829-829: Prefernext()overnth(0).Idiomatic and clearer. LGTM.
838-840: Usestd::slice::from_reffor single-item slices.Eliminates temp vecs. LGTM.
891-893: Samefrom_refimprovement.LGTM.
933-933: Samefrom_refimprovement.LGTM.
972-978: Consistentfrom_refusage — good.
1001-1006: Consistentfrom_refusage — good.
1028-1034: Consistentfrom_refusage — good.
1125-1125: Check return value, don’t discard.Direct
unwrap()is fine in tests. LGTM.
1194-1194: Use HashSet for wallet existence — good.Efficient membership checks. LGTM.
1245-1248: Assertions filter out test/faucet wallets — good.Minimizes false positives. LGTM.
rpc-integration-test/src/main.rs (4)
19-19: Importjsondirectly.Reduces verbosity in tests. LGTM.
204-207: URL formatting with &'static str names — good.Avoids allocations and keeps constants in one place. LGTM.
228-238: Log messages switched to constants — good.Cleaner formatting; avoids
.to_string(). LGTM.Also applies to: 262-262
409-410: Add asset-unlock-statuses test call — good.Exercising the new client method is valuable. LGTM.
| let outputs: Vec<_> = | ||
| outputs.iter().map(|o| serde_json::to_value(JsonOutPoint::from(*o)).unwrap()).collect(); | ||
| self.call("lockunspent", &[false.into(), outputs.into()]) |
There was a problem hiding this comment.
Avoid unwrap in JSON conversion.
serde_json::to_value can fail; propagate errors instead of panicking.
Apply to both functions:
- let outputs: Vec<_> =
- outputs.iter().map(|o| serde_json::to_value(JsonOutPoint::from(*o)).unwrap()).collect();
+ let outputs: Vec<Value> = outputs
+ .iter()
+ .map(|o| into_json(JsonOutPoint::from(*o)))
+ .collect::<Result<_>>()?;Also applies to: 684-686
🤖 Prompt for AI Agents
In rpc-client/src/client.rs around lines 678-680 and 684-686, the code uses
serde_json::to_value(...).unwrap() which can panic; change these functions to
propagate serialization errors instead of unwrapping: replace unwrap with the ?
operator (or map_err to convert serde_json::Error into the function's
Error/Result type) so the function returns a Result and any to_value failures
are returned to the caller; ensure the call to self.call still returns the
appropriate Result by constructing/returning an Err when serialization fails.
Summary by CodeRabbit
New Features
Refactor
Tests