refactor: minor masternode list improvements based on #51 review #54
Conversation
|
Warning Rate limit exceeded@shumkov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis pull request updates various parts of the masternode and quorum verification modules by replacing raw byte array representations with a more structured hash type ( Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
shumkov
changed the title
![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)
dash/src/sml/masternode_list/masternode_helpers.rs (1)
81-81: Ensure you're not overlooking differences in keys
If the goal is to compare both the keys and the corresponding values, consider comparing the entire map instead of just the values. Otherwise, two maps with different keys but matching values in the same iteration order could incorrectly pass as equal.Here is a potential fix:
- self.masternodes.values().eq(list.masternodes.values()) + self.masternodes == list.masternodesdash/src/sml/masternode_list/builder.rs (1)
23-37: Good implementation of the new constructor.Fields are properly initialized with
Nonefor the optional merkle roots. One small observation: the parameter order in the method signature (lines 23-28) doesn't match the field order in the struct initialization (lines 29-36). While this doesn't affect functionality, maintaining a consistent order would improve readability.Self { - quorums, block_hash, block_height, masternode_merkle_root: None, llmq_merkle_root: None, masternodes, + quorums, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dash/src/sml/llmq_entry_verification.rs(3 hunks)dash/src/sml/masternode_list/apply_diff.rs(2 hunks)dash/src/sml/masternode_list/builder.rs(1 hunks)dash/src/sml/masternode_list/masternode_helpers.rs(1 hunks)dash/src/sml/masternode_list/merkle_roots.rs(6 hunks)dash/src/sml/masternode_list/mod.rs(1 hunks)dash/src/sml/masternode_list_entry/hash.rs(1 hunks)dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs(2 hunks)dash/src/sml/quorum_entry/qualified_quorum_entry.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
dash/src/sml/masternode_list_entry/hash.rs (1)
Learnt from: QuantumExplorer
PR: dashpay/rust-dashcore#51
File: dash/src/sml/masternode_list_entry/hash.rs:7-12
Timestamp: 2025-02-25T06:19:32.230Z
Learning: The `consensus_encode` method on `MasternodeListEntry` writing to a `Vec` buffer cannot fail, so using `.expect()` is appropriate rather than propagating the error with the `?` operator.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
- GitHub Check: Tests (nightly, true, false, true)
- GitHub Check: Tests (beta, true, false)
- GitHub Check: Tests (stable, true, true, false)
🔇 Additional comments (23)
dash/src/sml/quorum_entry/qualified_quorum_entry.rs (1)
62-62: Use typedBlockHashfor improved clarity
Switching from a raw byte array to a structuredBlockHashis a welcome enhancement, as it promotes type safety and clarity in the code.dash/src/sml/masternode_list/mod.rs (1)
11-15: Builder module introduction looks good
Re-exportingMasternodeListBuilderat the module level provides a clean entry point for users. This refactor nicely supports a more structured approach to constructing masternode lists.dash/src/sml/masternode_list/apply_diff.rs (2)
4-4: Update imports to reflect the builder pattern additionThe import has been updated to include
MasternodeListBuilder, which aligns with the refactoring to use the builder pattern instead of direct instantiation.
79-86: Improved code structure with builder patternThe implementation now uses a builder pattern which provides better encapsulation and flexibility for object construction. This is a good improvement over direct instantiation.
dash/src/sml/masternode_list_entry/hash.rs (3)
1-1: Updated import for consistent hash type usageThe import has been updated to use
sha256dfrom thehashesmodule, which aligns with the PR's goal of using structured hash types instead of raw byte arrays.
7-7: Return type improved to use sha256d::HashChanged the return type from raw byte array
[u8; 32]tosha256d::Hash, enhancing type safety and consistency with other parts of the codebase.
11-11: Simplified hash calculationRemoved unnecessary
to_byte_array()conversion, directly returning the hash. This aligns with the learnings from PR #51 where it was noted that this operation cannot fail, making.expect()appropriate.dash/src/sml/masternode_list_entry/qualified_masternode_list_entry.rs (2)
5-5: Updated import for structured hash typesThe import now includes
sha256dfrom thehashesmodule, supporting the consistent use of structured hash types throughout the codebase.
21-21: Enhanced type safety with sha256d::HashChanged field type from raw byte array
[u8; 32]tosha256d::Hash, improving type safety and consistency. This change aligns with the overall PR objective of using more structured hash types.dash/src/sml/llmq_entry_verification.rs (3)
5-5: Updated import for BlockHash typeAdded import for
BlockHashtype to support the enum variant type change.
16-16: Enhanced type safety in enum variantChanged
UnknownBlockvariant from raw byte array[u8; 32]toBlockHashtype, improving type safety and making the code more expressive and consistent with the rest of the codebase.
30-30: Simplified display formattingUpdated the
Displayimplementation to directly use theblock_hashwithout additional encoding, which is possible because theBlockHashtype implements the necessary display traits.dash/src/sml/masternode_list/builder.rs (5)
1-8: Good import organization.All necessary dependencies are properly imported and well-organized.
9-16: Well-designed builder pattern structure.The
MasternodeListBuilderstruct is properly designed with clear fields that match theMasternodeListrequirements. Using public fields is appropriate for a builder pattern.
19-21: Clear and concise empty constructor.The
emptymethod provides a convenient way to create a builder with default empty collections while reusing thenewmethod implementation.
39-47: Effective method chaining implementation.The
with_merkle_rootsmethod follows the builder pattern correctly by taking ownership ofself, modifying it, and returning it for method chaining.
49-65: Good build method with automatic merkle root calculation.The
buildmethod effectively constructs theMasternodeListand conditionally calculates merkle roots if they weren't explicitly provided. This makes the builder both flexible and robust.dash/src/sml/masternode_list/merkle_roots.rs (6)
23-23: Type-safe hash handling improvement.Changing from raw byte arrays to the specific
sha256d::Hashtype enhances type safety and makes the code more semantically clear.
32-38: Improved hash processing with proper type usage.The hash processing now properly uses the
sha256d::Hashtype's methods likeas_byte_array()andhash()instead of working with raw byte arrays. This provides better type safety and clarity.
129-129: Consistent type conversion using from_raw_hash.Using
from_raw_hashconsistently improves code clarity by making it explicit that you're working with hash types rather than raw byte arrays.
143-143: Consistent type conversion for quorum merkle roots.Similar to the masternode merkle root, using
from_raw_hashhere maintains consistency and type safety.
157-159: Updated return type documentation.The comments and return type have been correctly updated to reflect the change from raw byte arrays to the specific hash type.
183-188: Improved quorum merkle root hash handling.The function now correctly returns and processes
sha256d::Hashtypes, and uses the appropriateto_raw_hash()method when mapping quorum entries.
shumkov
changed the title
github-project-automation
bot
moved this from In review / testing
to Done
in Platform team
* feat: add single node quorum type * chore: update lazy_static * revert: lazy_static version
MasternodeListBuilderSummary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores