fix(hashes): improve unsafe block warnings and macro cleanup #84
Conversation
WalkthroughThe updates focus on code clarity and safety improvements. Unused imports are removed, unsafe operations are explicitly scoped within macros and functions, and macro variable names are made more descriptive. No logic, control flow, or public API behavior is changed, but macro and function parameter names are updated for consistency. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: profiles for the non root package will be ignored, specify profiles at the workspace root: Caused by: Caused by: Caused by: ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
DCG-Claude
force-pushed
the
feat/hashes-module-updates
branch
from
8accee0 to
4bebfa2
Compare
- Fix unsafe block warnings in siphash24.rs by properly wrapping unsafe operations - Improve hash utility functions with better variable naming ($generator instead of $gen) - Remove unused import in internal_macros.rs for bincode feature These changes address compiler warnings about unsafe operations and improve code clarity without changing functionality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
DCG-Claude
force-pushed
the
feat/hashes-module-updates
branch
from
4bebfa2 to
2a1cd37
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (2)
hashes/src/util.rs (1)
591-592: Minor nit: parameter overshadowing the type name
generator: &mut SchemaGeneratorshadows the importedschemars::r#gen::SchemaGeneratortype alias, which is harmless but can be visually confusing when reading the function body. If you want to avoid that cognitive load, consider a more specific variable name (schema_gen?)—totally optional.hashes/src/siphash24.rs (1)
194-206: Update safety comment to reflect current call pattern
load_int_le!is still marked “Unsafe because: unchecked indexing…”, yet every call site (e.g. here) is now safe-context thanks to the macro’s internalunsafeblock. Consider re-wording the doc comment to clarify that use of the macro is safe provided the debug-assert pre-condition holds, and the unsafety is fully encapsulated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hashes/src/internal_macros.rs(0 hunks)hashes/src/siphash24.rs(3 hunks)hashes/src/util.rs(5 hunks)
💤 Files with no reviewable changes (1)
- hashes/src/internal_macros.rs
🧰 Additional context used
🧠 Learnings (1)
hashes/src/siphash24.rs (4)
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#56
File: dash/src/sml/masternode_list_engine/message_request_verification.rs:91-91
Timestamp: 2025-02-27T05:39:16.767Z
Learning: In the Dash Rust codebase, unwrap() on try_into() for byte slices from hash types like QuorumSigningRequestId is safe because hash types guarantee fixed-size byte arrays (32 bytes), ensuring the slice operations and conversions will never fail.
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.
Learnt from: DCG-Claude
PR: dashpay/rust-dashcore#0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#51
File: dash/src/sml/masternode_list/scores_for_quorum.rs:50-73
Timestamp: 2025-02-25T06:13:52.858Z
Learning: ScoreHash is a cryptographic hash in the rust-dashcore library, and therefore does not need collision handling when used as a key in collections like BTreeMap due to the extremely low probability of collisions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_amount)
🔇 Additional comments (4)
hashes/src/util.rs (2)
21-22: Clearer macro parameter name – nice cleanupRenaming
$gento$generatorimproves readability and avoids the common shorthand that can collide with unrelated identifiers in macro-heavy codebases.
66-74: Consistent rename propagated to borrow-slice helpersThe same
$generatorrename is correctly mirrored inborrow_slice_impl!; no functional impact and keeps generics consistent across macros.hashes/src/siphash24.rs (2)
76-82: Unsafe copy now properly wrappedAdding the explicit
unsafeblock aroundptr::copy_nonoverlappingscopes unsafety precisely and eliminates the “operation requires unsafe” lint. 👍
274-275: Redundant but requiredunsafeinside anunsafe fnEven inside an
unsafe fn, Rust still demands anunsafeblock forget_unchecked; the added block is correct and silences the warning without altering semantics.
2 participants
Summary
Changes Made
ptr::copy_nonoverlappingcall in explicit unsafe block and fixed unsafeget_uncheckedusage$gento$generatorfor better clarity and to avoid potential conflictsuse crate::Hash;import in bincode feature blockTest Plan
These changes address compiler warnings about unsafe operations and improve code clarity without changing functionality. The changes are minimal (~38 lines) and only touch unsafe block handling and macro parameter naming.
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Chores