imp(discovery): clippy + cleanup#591
Conversation
2a2ab5d to
bc84a04
Compare
There was a problem hiding this comment.
Pull Request Overview
The PR cleans up code and enables stricter Clippy lints across multiple crates, modernizing syntax and improving performance.
- Removes unnecessary
clone()calls and replaces verbosematchblocks withlet-elsepatterns. - Optimizes Redis lookups in
NodeStoreusing pipelining. - Configures workspace-level Clippy lint settings and refactors API routes for fewer allocations.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/shared/src/web3/contracts/core/contract.rs | Remove redundant clone() when parsing ABI JSON |
| crates/shared/src/security/auth_signature_middleware.rs | Simplify error handling with let-else for signature recovery |
| crates/shared/src/p2p/client.rs | Refactor signature parsing and message signing error propagation |
| crates/orchestrator/src/store/core/context.rs | Eliminate unnecessary clone() when initializing metrics store |
| crates/discovery/src/store/node_store.rs | Change String parameters to &str and optimize count_active_nodes_by_ip with Redis pipelining |
| crates/discovery/src/main.rs | Simplify URL parsing with let-else and improve panic messages for contract build |
| crates/discovery/src/location_service.rs | Replace expect with unwrap_or_else for HTTP client construction |
| crates/discovery/src/chainsync/sync.rs | Convert run method from async to sync to streamline invocation |
| crates/discovery/src/api/server.rs | Refactor health check logic to use if let instead of match |
| crates/discovery/src/api/routes/node.rs | Reduce unnecessary cloning and streamline node registration flow |
| crates/discovery/src/api/routes/get_nodes.rs | Improve node filtering algorithm and modernize error handling patterns |
| crates/discovery/Cargo.toml | Add workspace lint configuration for Clippy |
| Cargo.toml | Define workspace-level Clippy lint settings |
| let pool_contract_id: U256 = match pool_id.parse::<U256>() { | ||
| Ok(id) => id, | ||
| Err(_) => { | ||
| return HttpResponse::BadRequest() | ||
| .json(ApiResponse::new(false, "Invalid pool ID format")); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
[nitpick] You parse pool_id twice (once to U256 and again to u32). Consider parsing once to u32 and converting to U256 using U256::from(parsed_id) to avoid duplication and improve readability.
| let pool_contract_id: U256 = match pool_id.parse::<U256>() { | |
| Ok(id) => id, | |
| Err(_) => { | |
| return HttpResponse::BadRequest() | |
| .json(ApiResponse::new(false, "Invalid pool ID format")); | |
| } | |
| }; |
| } | ||
| }; | ||
|
|
||
| let pool_id: u32 = match pool_id.parse() { |
There was a problem hiding this comment.
[nitpick] Shadowing the pool_id variable (string → u32) may be confusing. Consider using a distinct name like parsed_pool_id for clarity.
| let pool_id: u32 = match pool_id.parse() { | |
| let parsed_pool_id: u32 = match pool_id.parse() { |
| .with_stake_manager() | ||
| .build() | ||
| .unwrap(); | ||
| .unwrap_or_else(|_| panic!("Failed to build contracts")); |
There was a problem hiding this comment.
[nitpick] Include the original error in the panic message to aid debugging, e.g., unwrap_or_else(|e| panic!("Failed to build contracts: {e:?}")).
| .unwrap_or_else(|_| panic!("Failed to build contracts")); | |
| .unwrap_or_else(|e| panic!("Failed to build contracts: {e:?}")); |
No description provided.