feat(platform): get current quorum info #2168
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new RPC method Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (19)
packages/rs-dpp/src/core_types/mod.rs (1)
1-2: Consider adding module-level documentation.While the module names are self-explanatory, it would be beneficial to add brief documentation comments explaining the purpose and contents of each module. This can help developers quickly understand the role of these modules within the larger system.
Consider adding documentation like this:
/// Module containing types and functionality related to individual validators. pub mod validator; /// Module for managing sets of validators and related operations. pub mod validator_set;packages/rs-drive-proof-verifier/src/lib.rs (1)
17-17: LGTM! Consider adding documentation for the new module.The addition of the
unprovedmodule aligns with the PR objectives and is correctly implemented. The module is properly declared as public, allowing it to be used by other parts of the codebase or external users of this library.To improve code clarity and maintain consistency with other modules in this file, consider adding a brief documentation comment explaining the purpose of the
unprovedmodule. For example:/// Module for handling unproved data retrieval and processing pub mod unproved;packages/rs-sdk/src/platform.rs (1)
Line range hint
3-7: Note the TODO for future SDK development.There's an existing TODO comment at the beginning of the file that outlines plans for future SDK development. While not directly related to the current change, it's worth keeping in mind for future work:
- A proper user-facing API should be defined to hide transport and serialization details.
- The current re-exports of proto-generated types are temporary and may be replaced in the future.
Consider creating a GitHub issue to track this future work, ensuring it's not overlooked in subsequent development cycles.
packages/rs-dpp/src/lib.rs (1)
Line range hint
1-92: Consider grouping related modules for better organizationThe file structure is generally well-organized, but there's an opportunity to improve readability and maintainability by grouping related modules together. For example:
- Core functionality modules (e.g.,
data_contract,document,identity)- Utility and helper modules (e.g.,
util,serialization)- Feature-specific modules (e.g.,
state_transition,dash_platform_protocol)- Cryptography-related modules (e.g.,
signing,bls)This grouping could be achieved through comments or by reordering the module declarations. It would make it easier for developers to navigate the codebase and understand the relationships between different modules.
packages/rs-dpp/Cargo.toml (1)
198-199: LGTM: New features added toabciand declaredThe addition of
core-types-serializationandcore-types-serde-conversionto theabcilist and their declaration as empty arrays at the end of the file is consistent with the project structure.Consider adding a brief comment or documentation for these new features to explain their intended use and why they're currently empty. This will help other developers understand the purpose of these features and their future implementation plans.
Also applies to: 272-273
packages/rs-dapi-client/src/transport/grpc.rs (1)
379-387: LGTM! Consider adding a brief comment for clarity.The implementation of
get_current_quorums_infois consistent with other gRPC method implementations in the file and aligns with the PR objectives. It correctly uses theimpl_transport_request_grpc!macro with appropriate request and response types.Consider adding a brief comment above the implementation to describe the purpose of this method, similar to other implementations in the file. For example:
// Retrieves the current quorums information from the platform impl_transport_request_grpc!( // ... (rest of the implementation) );packages/rs-drive-abci/src/query/service.rs (1)
601-611: LGTM: New method implemented correctlyThe
get_current_quorums_infomethod has been implemented consistently with other query methods in this service. It correctly uses thehandle_blocking_querymethod to manage potentially time-consuming operations.Consider updating the endpoint name in the
handle_blocking_querycall from "query_current_quorums_info" to "get_current_quorums_info" for consistency with the method name and other endpoint names in this file.- "query_current_quorums_info", + "get_current_quorums_info",packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (2)
348-357: LGTM! Consider usingsort_unstable_byfor potential performance improvement.The implementation correctly collects and sorts the validator sets by core height in descending order. It's well-structured and achieves its purpose effectively.
For a potential performance improvement, consider using
sort_unstable_byinstead ofsort_by.sort_unstable_byis generally faster but doesn't preserve the order of equal elements. If the order of equal elements is not important in this context, you can apply this change:- validator_sets.sort_by(|a, b| b.core_height().cmp(&a.core_height())); + validator_sets.sort_unstable_by(|a, b| b.core_height().cmp(&a.core_height()));
Line range hint
359-372: LGTM! Consider adding a debug assertion for position overflow.The implementation correctly uses the new sorting method and finds the position of the current validator set. It's well-structured and achieves its purpose effectively.
To enhance robustness, consider adding a debug assertion to check for potential overflow when converting from
usizetou16. This won't affect release builds but can help catch issues during development:validator_sets .iter() .position(|&validator_set| validator_set.quorum_hash() == ¤t_quorum_hash) - .map(|position| position as u16) // Convert position to u16 + .map(|position| { + debug_assert!(position <= u16::MAX as usize, "Validator set position overflow"); + position as u16 + })packages/rs-drive-abci/src/platform_types/validator_set/mod.rs (4)
4-4: Consider more selective re-export to reduce namespace pollutionUsing
pub use dpp::core_types::validator_set::*;re-exports all items from the module, which may introduce unnecessary items into your module's public API. Consider re-exporting only the necessary types or functions to maintain a clean and intentional public interface.
6-7: Enhance module documentation for clarityThe comment
/// v0is minimal. Consider adding more detailed documentation to describe the purpose and functionality of thev0module to improve code readability and maintainability.
9-12: Add documentation for trait and methodsAdding documentation comments to the
ValidatorSetExttrait and its methodsto_updateandto_update_ownedwill enhance code clarity and help other developers understand their purpose and usage.
15-24: Consider adding documentation to trait implementationsAdding doc comments to the implementation of
ValidatorSetExtforValidatorSetand its methodsto_updateandto_update_ownedcan improve code clarity and assist other developers in understanding the functionality and usage of these methods.packages/rs-sdk/src/platform/fetch_unproved.rs (2)
65-65: Avoid unnecessary cloning of the requestThe request is cloned before execution, but since it's not used elsewhere after this point, cloning may be unnecessary. Removing the clone can improve performance by reducing overhead.
Proposed change:
- let response = request.clone().execute(sdk, settings).await?; + let response = request.execute(sdk, settings).await?;Ensure that the
executemethod does not consume or modifyrequestin a way that affects subsequent code.
53-74: Add unit tests forfetch_unproved_with_settingsIncluding unit tests for the
fetch_unproved_with_settingsmethod will ensure that it behaves correctly under various conditions and will help catch regressions early.Would you like assistance in creating unit tests for this method? I can help set up test scenarios and examples.
packages/rs-drive-abci/src/query/system/current_quorums_info/v0/mod.rs (1)
16-16: Clarify Unused Request ParameterThe request parameter
GetCurrentQuorumsInfoRequestV0 {}is unused within the function body. For clarity and convention, consider renaming it to_requestto indicate that it's intentionally unused.Apply this diff to rename the parameter:
- GetCurrentQuorumsInfoRequestV0 {}: GetCurrentQuorumsInfoRequestV0, + _request: GetCurrentQuorumsInfoRequestV0,packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (2)
Line range hint
157-202: Refactor to eliminate code duplication into_updatemethodsThe
to_updateandto_update_ownedmethods contain nearly identical logic, differing mainly in how they iterate over thememberscollection (iter()vs.into_values()). This duplication increases maintenance overhead and potential for inconsistencies.Consider abstracting the shared logic into a generic function or helper method that can handle both references and owned values. This refactoring will improve code maintainability and reduce redundancy.
Also applies to: 204-248
Line range hint
269-289: Simplify error handling intry_from_quorum_info_resultThe use of
filter_mapwith mixedOptionandResulttypes can make error handling confusing and less readable. ReturningSome(Err(e))withinfilter_mapblends the two layers unnecessarily.Refactor the closure to use
mapinstead offilter_map, allowing for clearer separation of errors and option handling. Alternatively, collect the results into an intermediateResult<Vec<_>, Error>before constructing theBTreeMap, enhancing clarity and error propagation.packages/rs-drive-proof-verifier/src/types.rs (1)
383-396: Simplify and Conciseness in Documentation CommentsThe documentation for
CurrentQuorumsInfois extensive but may be overly verbose and include redundant information. Consider simplifying the comments to focus on key details, enhancing readability and maintainability.For example, the struct-level comment can be condensed:
/// Represents the current state of quorums in the platform. -/// -/// This struct holds various information related to the current quorums, -/// including the list of quorum hashes, the current active quorum hash, -/// and details about the validators and their statuses. /// -/// # Fields -/// -/// - `quorum_hashes`: A list of 32-byte hashes representing the active quorums. -/// - `current_quorum_hash`: A 32-byte hash identifying the currently active quorum. -/// - `validator_sets`: A collection of [`ValidatorSet`] structs for different quorums. -/// - `last_block_proposer`: The ProTxHash of the last block proposer. -/// - `last_platform_block_height`: The height of the most recent platform block. -/// - `last_core_block_height`: The height of the most recent core blockchain block. +/// Contains information about active quorums and validator sets, including hashes, validator details, +/// and the last block proposer information.Similarly, ensure field comments are concise and consistently formatted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (28)
- packages/dapi-grpc/protos/platform/v0/platform.proto (2 hunks)
- packages/rs-dapi-client/src/transport/grpc.rs (1 hunks)
- packages/rs-dpp/Cargo.toml (5 hunks)
- packages/rs-dpp/src/core_types/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator_set/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/lib.rs (1 hunks)
- packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs (2 hunks)
- packages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/v0/mod.rs (4 hunks)
- packages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/v1/mod.rs (4 hunks)
- packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/validator/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/validator/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/validator_set/mod.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (9 hunks)
- packages/rs-drive-abci/src/query/service.rs (2 hunks)
- packages/rs-drive-abci/src/query/system/current_quorums_info/mod.rs (1 hunks)
- packages/rs-drive-abci/src/query/system/current_quorums_info/v0/mod.rs (1 hunks)
- packages/rs-drive-abci/src/query/system/mod.rs (1 hunks)
- packages/rs-drive-proof-verifier/Cargo.toml (1 hunks)
- packages/rs-drive-proof-verifier/src/lib.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (2 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (1 hunks)
- packages/rs-sdk/src/platform.rs (1 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Rust packages (wasm-dpp) / Linting
packages/rs-dpp/src/core_types/validator/v0/mod.rs
[warning] 16-16: unused import:
dashcore::hashes::Hash
warning: unused import:dashcore::hashes::Hash
--> packages/rs-dpp/src/core_types/validator/v0/mod.rs:16:5
|
16 | use dashcore::hashes::Hash;
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-dpp/src/core_types/validator_set/v0/mod.rs
[failure] 105-105: cannot find trait
BorrowDecodein this scope
error[E0405]: cannot find traitBorrowDecodein this scope
--> packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:105:11
|
105 | impl<'de> BorrowDecode<'de> for ValidatorSetV0 {
| ^^^^^^^^^^^^ not found in this scope
|
help: consider importing one of these items
|
1 + use bincode::BorrowDecode;
|
1 + use crate::platform_serialization::BorrowDecode;
|
1 + use platform_serialization::BorrowDecode;
|
[failure] 106-106: cannot find trait
Decoderin this scope
error[E0405]: cannot find traitDecoderin this scope
--> packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:106:25
|
106 | fn borrow_decode<D: Decoder>(decoder: &mut D) -> Result<Self, bincode::error::DecodeError> {
| ^^^^^^^ not found in this scope
|
help: consider importing one of these items
|
1 + use bincode::de::Decoder;
|
1 + use crate::platform_serialization::de::Decoder;
|
1 + use platform_serialization::de::Decoder;
|
🪛 GitHub Check: Rust packages (drive) / Linting
packages/rs-dpp/src/core_types/validator/v0/mod.rs
[warning] 16-16: unused import:
dashcore::hashes::Hash
warning: unused import:dashcore::hashes::Hash
--> packages/rs-dpp/src/core_types/validator/v0/mod.rs:16:5
|
16 | use dashcore::hashes::Hash;
| ^^^^^^^^^^^^^^^^^^^^^^
|
= note:#[warn(unused_imports)]on by defaultpackages/rs-dpp/src/core_types/validator_set/v0/mod.rs
[failure] 105-105: cannot find trait
BorrowDecodein this scope
error[E0405]: cannot find traitBorrowDecodein this scope
--> packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:105:11
|
105 | impl<'de> BorrowDecode<'de> for ValidatorSetV0 {
| ^^^^^^^^^^^^ not found in this scope
|
help: consider importing one of these items
|
1 + use bincode::BorrowDecode;
|
1 + use crate::platform_serialization::BorrowDecode;
|
1 + use platform_serialization::BorrowDecode;
|
[failure] 106-106: cannot find trait
Decoderin this scope
error[E0405]: cannot find traitDecoderin this scope
--> packages/rs-dpp/src/core_types/validator_set/v0/mod.rs:106:25
|
106 | fn borrow_decode<D: Decoder>(decoder: &mut D) -> Result<Self, bincode::error::DecodeError> {
| ^^^^^^^ not found in this scope
|
help: consider importing one of these items
|
1 + use bincode::de::Decoder;
|
1 + use crate::platform_serialization::de::Decoder;
|
1 + use platform_serialization::de::Decoder;
|
🔇 Additional comments (31)
packages/rs-dpp/src/core_types/mod.rs (1)
1-2: LGTM! Changes align well with PR objectives.The addition of
validatorandvalidator_setmodules aligns perfectly with the PR objective of moving Validator Types and Validator Set to the Decentralized Payment Protocol (DPP). This structure promotes better organization and separation of concerns in the codebase.packages/rs-drive-abci/src/platform_types/validator/mod.rs (1)
1-3: Consider the implications of restructuring validator typesThe removal of the
Validatorenum and its associated implementations, coupled with the direct re-export fromdpp::core_types::validator, represents a significant change in how validator types are handled. This simplification aligns with the PR objectives of moving Validator Types to DPP, but it has several implications:
- It removes the version-specific implementation layer, which might impact code that relied on the
Validatorenum.- The direct re-export suggests a move towards using core types, which can improve consistency but may require updates in other parts of the codebase.
- The presence of the
v0module (line 1) seems inconsistent with the removal of version-specific implementations.To ensure this change doesn't introduce breaking changes or leave unused code:
Please review the results of these checks and consider:
- Updating any code that might still be using the old
Validatorenum.- Removing the
v0module if it's no longer necessary.- Ensuring that all parts of the codebase are updated to use the new validator types from
dpp::core_types::validator.packages/rs-sdk/src/platform.rs (1)
14-14: LGTM! New module added as expected.The addition of the
fetch_unprovedmodule aligns with the PR objectives to enhance the SDK's capability for obtaining unproved data. The placement is consistent with the existing structure.Let's verify the usage of this new module and check if any documentation updates are needed:
Please review the results of this script to ensure proper usage and documentation of the new module.
packages/rs-drive-proof-verifier/Cargo.toml (1)
31-31: LGTM. Please verify the necessity and impact of the new feature.The addition of the "core-types-serialization" feature to the
dppdependency aligns with the PR objectives, particularly the movement of Validator Set and Validator Types to DPP.Could you please confirm:
- Is this feature necessary for handling the Validator Set and Validator Types in DPP?
- Are there any other modules in the project that might need this feature as well?
To help verify the impact, you can run the following script:
This will help ensure that the feature is appropriately added where needed and that we haven't missed any other modules that might benefit from it.
✅ Verification successful
Verified the addition of the "core-types-serialization" feature across relevant modules. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other potential usages of the new feature across the project # Search for potential usages of core types serialization in Rust files echo "Searching for potential usages of core types serialization:" rg --type rust -i "core.*types.*serialization" -g '!target/' # Check other Cargo.toml files for the dpp dependency echo "Checking other Cargo.toml files for the dpp dependency:" rg --type toml 'dpp.*=.*path.*=.*"../rs-dpp"' -g 'Cargo.toml'Length of output: 2737
packages/rs-dpp/src/lib.rs (1)
56-56: New modulecore_typesaddedThe addition of the
core_typesmodule aligns with the PR objectives of moving Validator Set and Validator Types to the Decentralized Payment Protocol (DPP). This new module likely contains these core types, which is a good step towards better organization and modularity.To ensure the
core_typesmodule is properly implemented and contains the expected types, let's verify its contents:packages/rs-drive-abci/src/execution/engine/initialization/init_chain/v0/mod.rs (3)
13-13: LGTM: New import for ValidatorSetExtThe addition of
ValidatorSetExtimport is appropriate and aligns with the changes in theinit_chain_v0method. This import likely provides extended functionality for working with validator sets.
109-109: LGTM: Simplified ValidatorSetUpdate creationThe change from
ValidatorSetUpdate::new(validator_set_inner.1)tovalidator_set_inner.1.to_update()is a good improvement. It leverages theValidatorSetExttrait to provide a more idiomatic and potentially more efficient way to create aValidatorSetUpdate. This change:
- Simplifies the code, making it more readable.
- Standardizes the conversion process from a validator set to an update.
- Potentially improves performance by using a specialized method.
The functionality remains the same, but the implementation is more elegant.
13-13: Summary: Improved validator set handlingThe changes in this file enhance the handling of validator sets in the initialization process:
- The addition of
ValidatorSetExtimport provides extended functionality for validator sets.- The simplification of
ValidatorSetUpdatecreation in theinit_chain_v0method improves code readability and potentially performance.These changes align well with the PR objectives of improving quorum information retrieval and streamlining validator set management. The modifications are minimal but impactful, contributing to better code organization and potentially more efficient quorum handling.
Also applies to: 109-109
packages/rs-dpp/Cargo.toml (4)
92-93: LGTM: New features added toall_featuresThe addition of
core-types-serializationandcore-types-serde-conversionto theall_featureslist is consistent with the project structure and appears to be a valid enhancement.
136-137: LGTM: New features added todash-sdk-featuresThe addition of
core-types-serializationandcore-types-serde-conversionto thedash-sdk-featureslist is consistent with the project structure and appears to be a valid enhancement for the SDK.
159-160: LGTM: New features added toall_features_without_clientThe addition of
core-types-serializationandcore-types-serde-conversionto theall_features_without_clientlist is consistent with the project structure and appears to be a valid enhancement for non-client features.
92-93: Summary: New serialization features addedThe changes introduce two new features,
core-types-serializationandcore-types-serde-conversion, across multiple feature sets in the project. These additions are consistent and well-integrated into the existing structure.Key points:
- The new features are added to
all_features,dash-sdk-features,all_features_without_client, andabci.- Empty arrays for these features are declared at the end of the file, allowing for future expansion.
These changes appear to be laying the groundwork for enhanced serialization capabilities in the project, which aligns with the PR objectives of improving data handling and streamlining integration within the platform.
To ensure these changes don't introduce any conflicts or issues, please run the following verification script:
Also applies to: 136-137, 159-160, 198-199, 272-273
✅ Verification successful
Verification Successful: New serialization features are correctly integrated
The executed verification scripts confirm that:
- Presence in All Locations: Both
core-types-serializationandcore-types-serde-conversionare present in all expected sections ofCargo.toml.- Proper Declarations: The new features are correctly declared as empty arrays, allowing for future enhancements.
- No Conflicts or Duplicates: There are no duplicate entries or conflicts related to the new features.
These results indicate that the new serialization features have been seamlessly integrated without introducing any issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the new features are correctly added and don't conflict with existing ones # Test 1: Check if the new features are present in all expected locations echo "Checking for new features in all expected locations:" grep -n "core-types-serialization" packages/rs-dpp/Cargo.toml grep -n "core-types-serde-conversion" packages/rs-dpp/Cargo.toml # Test 2: Verify that the new features are declared as empty arrays echo "Verifying new feature declarations:" tail -n 5 packages/rs-dpp/Cargo.toml | grep -E "core-types-(serialization|serde-conversion) = \[\]" # Test 3: Check for any potential conflicts or duplicates echo "Checking for potential conflicts or duplicates:" grep -n "core-types-" packages/rs-dpp/Cargo.toml | sort | uniq -c # Note: If any of these tests fail, it might indicate an issue with the changes.Length of output: 1381
packages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/v0/mod.rs (5)
14-14: LGTM: New import aligns with code changes.The addition of
ValidatorSetExtimport is consistent with the usage of theto_update()method in the code changes below. This suggests a refactoring to use extension methods forValidatorSet, which can improve code organization and reusability.
148-148: LGTM: Improved method naming for validator set update.The change from (likely)
into()toto_update()improves code readability and self-documentation. The new method name clearly indicates its purpose of creating a validator set update.
167-167: LGTM: Consistent use of new method across code paths.The change to
to_update()is consistently applied here, maintaining uniformity in how validator set updates are created across different scenarios in the code.
189-189: LGTM: Consistent refactoring in non-rotation scenario.The change to
to_update()is appropriately applied in the scenario where a validator set update occurs without rotation. This maintains consistency with the overall refactoring and improves code clarity.
Line range hint
24-199: Consider further refactoring and ensure comprehensive testing.The changes consistently improve the code by using the more descriptive
to_update()method. However, thevalidator_set_update_v0function remains quite complex, handling multiple scenarios for validator set updates and rotations.Consider the following suggestions:
- Break down the function into smaller, more focused functions to improve readability and maintainability.
- Add or update unit tests to ensure the new
to_update()method works correctly in all scenarios, including edge cases.- Consider adding debug logging for the
to_update()method calls to aid in troubleshooting.To verify the impact and coverage of these changes, you can run the following script:
packages/rs-drive-abci/src/execution/platform_events/block_end/validator_set_update/v1/mod.rs (5)
163-163: LGTM. Consider updating documentation if needed.The change from
new_validator_set.into()tonew_validator_set.to_update()is an improvement in terms of explicitness and likely type safety. This is consistent with the newly importedValidatorSetExttrait.If there's any existing documentation for this method or the
ValidatorSettype, consider updating it to reflect this change in the conversion method.
Line range hint
1-218: Summary: Consistent improvement in ValidatorSet conversionThe changes in this file consistently replace the use of
.into()with.to_update()forValidatorSetconversions. This modification, along with the addition of theValidatorSetExtimport, suggests a more explicit and potentially safer approach to convertingValidatorSettoValidatorSetUpdate.These changes align well with the PR objectives of enhancing the platform's functionality for retrieving quorum information. They appear to be part of a larger refactoring effort to improve the handling of validator sets.
Key points:
- The new
to_update()method is likely more explicit about the conversion operation.- The changes are applied consistently throughout the file.
- The modifications may improve code readability and type safety.
To ensure the changes have been applied correctly across the entire codebase and haven't introduced any issues, please run the verification scripts provided in the previous comments.
14-14: LGTM. Verify usage of the new import.The addition of
ValidatorSetExtimport is appropriate given the changes in the conversion method. This likely introduces new functionality or extension methods forValidatorSet.To ensure this import is used correctly throughout the codebase, run the following script:
#!/bin/bash # Description: Verify the usage of ValidatorSetExt in the codebase # Test: Search for ValidatorSetExt usage rg --type rust "ValidatorSetExt" # Test: Check if there are any remaining .into() calls on ValidatorSet that should be replaced rg --type rust "ValidatorSet.*\.into\(\)"
207-207: LGTM. Verify overall impact of the changes.The change from
current_validator_set.into()tocurrent_validator_set.to_update()is consistent with the previous changes and completes the update throughout this file.To ensure the changes have been applied correctly and to verify their impact, run the following script:
#!/bin/bash # Description: Verify the impact of ValidatorSet conversion changes # Test: Check for any remaining .into() calls on ValidatorSet rg --type rust "ValidatorSet.*\.into\(\)" # Test: Confirm usage of .to_update() method rg --type rust "ValidatorSet.*\.to_update\(\)" # Test: Check for any compiler errors or warnings related to these changes cargo check --all-targets cargo clippy --all-targets -- -D warningsThis script will help ensure that all instances have been updated, and that the changes haven't introduced any new compiler errors or warnings.
185-185: LGTM. Verify all instances are updated.The change from
new_validator_set.into()tonew_validator_set.to_update()is consistent with the previous change and improves code clarity.To ensure all instances of this conversion have been updated throughout the codebase, run the following script:
packages/rs-drive-abci/src/query/service.rs (2)
18-24: LGTM: Import statements updated correctlyThe import statement has been appropriately updated to include
GetCurrentQuorumsInfoRequestandGetCurrentQuorumsInfoResponse. This change is consistent with the new method being implemented and follows the existing import structure.
Line range hint
1-624: Summary: New query method added successfullyThe changes in this file successfully introduce a new
get_current_quorums_infomethod to thePlatformServiceimplementation. The implementation follows the existing patterns in the file, using thehandle_blocking_querymethod for consistent error handling and metrics tracking. The new method appears to be well-integrated into the existing service structure.To ensure the new method is properly integrated, let's verify its usage across the codebase:
✅ Verification successful
To ensure the
query_current_quorums_infofunction is properly implemented, let's perform additional checks:
Verification Successful:
query_current_quorums_infois properly implementedThe
query_current_quorums_infomethod and its versioned implementationquery_current_quorums_info_v0are correctly defined in the codebase. The integration aligns with existing patterns, and no missing implementations were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new get_current_quorums_info method # Search for potential calls to the new method rg --type rust 'get_current_quorums_info' # Search for the implementation of the query_current_quorums_info function ast-grep --lang rust --pattern 'fn query_current_quorums_info'Length of output: 1763
Script:
# Description: Search for implementations of `query_current_quorums_info` with any variations # Search for function implementations that might match `query_current_quorums_info` with different signatures or in different modules rg --type rust 'fn\s+query_current_quorums_info' --glob '**/*.rs' # Additionally, search for trait implementations that might include `query_current_quorums_info` rg --type rust 'query_current_quorums_info' --context 3 # Check for any pending branches or commits that might contain the implementation git branch --all git log --all --grep='query_current_quorums_info'Length of output: 3385
packages/rs-drive-abci/src/platform_types/platform_state/v0/mod.rs (1)
Line range hint
348-372: Overall, the changes look good and align with the PR objectives.The introduction of the
validator_sets_sorted_by_core_height_by_most_recentmethod and the update tocurrent_validator_set_position_in_list_by_most_recentenhance the platform's ability to manage and retrieve quorum information efficiently. The implementations are correct, and the code is well-structured.Minor suggestions for optimization and error handling have been provided, which could further improve the robustness and performance of the code. These changes contribute positively to the overall functionality of the platform state management.
packages/rs-drive-abci/src/platform_types/validator_set/mod.rs (1)
1-2: Imports are appropriate and necessaryThe added imports bring in required traits and types for
ValidatorSetoperations.packages/rs-sdk/src/platform/fetch_unproved.rs (2)
1-9: Imports are appropriate and necessaryAll imported modules are utilized within the file, ensuring no redundant imports. This promotes maintainability and clarity.
10-47: Well-definedFetchUnprovedtrait with clear documentationThe
FetchUnprovedtrait is properly defined with comprehensive documentation for each method. The use ofasync_traiteffectively allows asynchronous methods within the trait, aligning with best practices.packages/rs-dpp/src/core_types/validator_set/mod.rs (1)
1-111: Well-structured implementation ofValidatorSetwith versioning supportThe code introduces a
ValidatorSetenum to manage different versions of validator sets, currently with a single variantV0. The implementation correctly delegates getter and setter methods to the underlyingValidatorSetV0struct, ensuring that the code is extendable for future versions.Conditional compilation features are appropriately used to include serialization and deserialization capabilities when the respective features are enabled. This approach maintains modularity and allows for flexibility based on the build configuration.
packages/rs-dpp/src/core_types/validator/v0/mod.rs (1)
44-78: Verify consistency betweenencodeanddecodeimplementationsEnsure that the order of fields and their serialization in the
encodeanddecodemethods match exactly. Any inconsistency can lead to serialization/deserialization errors that are hard to detect.Also applies to: 81-128
packages/dapi-grpc/protos/platform/v0/platform.proto (1)
1190-1194: Empty 'GetCurrentQuorumsInfoRequestV0' MessageThe
GetCurrentQuorumsInfoRequestV0message is currently empty. If no request parameters are needed at this time, this is acceptable and allows for future expansion. Ensure this aligns with the intended API design.
| use crate::platform_types::validator_set::v0::{ | ||
| ValidatorSetMethodsV0, ValidatorSetV0, ValidatorSetV0Getters, | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Unnecessary Import: ValidatorSetMethodsV0 is not used in the file.
The imported ValidatorSetMethodsV0 in packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs is not utilized within the file. Consider removing it to clean up the codebase.
- File:
packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs- Lines: 10-12
🔗 Analysis chain
LGTM. Please clarify the usage of ValidatorSetMethodsV0.
The addition of ValidatorSetMethodsV0 to the import statement is in line with the PR objectives of moving Validator Set to DPP. However, I don't see any explicit use of this import in the current file. Could you please clarify where and how ValidatorSetMethodsV0 is being used, or if it's added preemptively for future use?
To verify the usage of ValidatorSetMethodsV0, please run the following command:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg "ValidatorSetMethodsV0" packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_quorum_info/v0/mod.rs
Length of output: 193
packages/rs-drive-abci/src/query/system/current_quorums_info/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/query/system/current_quorums_info/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
packages/rs-drive-abci/src/query/system/path_elements/mod.rs (1)
23-23: Approve the error message update with a minor suggestion.The change from "could not decode epoch info request" to "could not decode path elements" is an improvement as it more accurately reflects the context of the
query_path_elementsmethod. This update enhances error clarity without affecting functionality.Consider further improving the error message to be more specific about the missing
version:- QueryError::DecodingError("could not decode path elements".to_string()), + QueryError::DecodingError("missing version in GetPathElementsRequest".to_string()),This change would provide even more precise information about the nature of the decoding error.
packages/rs-drive-abci/src/query/system/total_credits_in_platform/mod.rs (1)
25-25: Approved: Error message update improves clarityThe change in the error message from "could not decode epoch info request" to "could not decode total credits in platform" is an improvement. It accurately reflects the context of the
query_total_credits_in_platformmethod, enhancing clarity for developers.Consider adding more specificity to the error message. For example:
QueryError::DecodingError(format!("Failed to decode version in GetTotalCreditsInPlatformRequest: {:?}", version))This would provide even more context for debugging purposes.
packages/rs-drive-proof-verifier/src/unproved.rs (2)
143-150: Consider enhancing error handling for metadata extractionWhile the current implementation is functional, consider providing more context in the error message when metadata is missing. This can help in debugging and troubleshooting.
Here's a suggested improvement:
let metadata = match &response.version { Some(platform::get_current_quorums_info_response::Version::V0(ref v0)) => { v0.metadata.clone() } None => None, } -.ok_or(Error::EmptyResponseMetadata)?; +.ok_or(Error::EmptyResponseMetadata { + context: "Missing metadata in GetCurrentQuorumsInfoResponse".to_string(), +})?;This change assumes that you have an
EmptyResponseMetadatavariant in yourErrorenum that accepts a context string. If not, consider adding such a variant to provide more detailed error information.
227-232: Enhance error handling forBlsPublicKeyparsingThe current error handling for
BlsPublicKeyparsing could be more informative. Consider including the actual error message from the parsing attempt in yourProtocolError.Here's a suggested improvement:
threshold_public_key: BlsPublicKey::from_bytes( &vs.threshold_public_key, ) -.map_err(|_| Error::ProtocolError { - error: "Invalid BlsPublicKey format".to_string(), +.map_err(|e| Error::ProtocolError { + error: format!("Invalid BlsPublicKey format: {}", e), })?,This change will provide more context about why the
BlsPublicKeyparsing failed, which can be helpful for debugging.packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (4)
18-29: LGTM: New trait improves abstraction. Consider adding documentation.The introduction of the
ValidatorSetMethodsV0trait is a good design choice, encapsulating core functionality for validator set operations. This abstraction allows for potential implementation by different types, enhancing flexibility and maintainability.Consider adding documentation comments for the trait and its methods to improve code clarity and maintainability. For example:
/// Trait defining core operations for validator sets. pub(crate) trait ValidatorSetMethodsV0 { /// Computes the difference between two validator sets and returns a ValidatorSetUpdate. fn update_difference(&self, rhs: &ValidatorSetV0) -> Result<ValidatorSetUpdate, Error>; /// Converts the validator set to a ValidatorSetUpdate. fn to_update(&self) -> ValidatorSetUpdate; /// Converts the validator set to a ValidatorSetUpdate, consuming self. fn to_update_owned(self) -> ValidatorSetUpdate; /// Attempts to create a quorum from the given QuorumInfoResult and PlatformState. fn try_from_quorum_info_result( value: QuorumInfoResult, state: &PlatformState, ) -> Result<ValidatorSetV0, Error>; }
Line range hint
35-154: LGTM: Improved validation and streamlined logic. Consider minor optimization.The updated
update_differencemethod provides more thorough validation of the validator sets being compared, which enhances the robustness of the code. The streamlined logic for generatingValidatorSetUpdateimproves readability and maintainability.Consider using
Iterator::filter_mapinstead offilterfollowed bymapto slightly optimize the code and improve readability. For example:let validator_updates = self .members .iter() .filter_map(|(pro_tx_hash, old_validator_state)| { rhs.members.get(pro_tx_hash).and_then(|new_validator_state| { if new_validator_state != old_validator_state { // ... existing logic for creating ValidatorUpdate ... } else { None } }) }) .collect::<Vec<abci::ValidatorUpdate>>(); Ok(ValidatorSetUpdate { validator_updates, threshold_public_key: Some(crypto::PublicKey { sum: Some(Bls12381(self.threshold_public_key.to_bytes().to_vec())), }), quorum_hash: self.quorum_hash.to_byte_array().to_vec(), })This change would eliminate the need for the
Resultwrapping and unwrapping, simplifying the code structure.
Line range hint
204-249: LGTM: Efficient consuming update method added. Consider minor optimization.The new
to_update_ownedmethod is a valuable addition, providing an efficient way to create aValidatorSetUpdateby consumingself. This can be particularly useful in scenarios where theValidatorSetV0is no longer needed after creating the update.Consider using
Iterator::filter_mapinstead offilter_mapfollowed bymapto slightly optimize the code and improve readability. For example:ValidatorSetUpdate { validator_updates: validator_set .into_values() .filter_map(|validator| { let ValidatorV0 { pro_tx_hash, public_key, node_ip, node_id, platform_p2p_port, is_banned, .. } = validator; if is_banned { return None; } let node_address = format!( "tcp://{}@{}:{}", hex::encode(node_id.to_byte_array()), node_ip, platform_p2p_port ); Some(abci::ValidatorUpdate { pub_key: public_key.map(|public_key| crypto::PublicKey { sum: Some(Bls12381(public_key.to_bytes().to_vec())), }), power: 100, pro_tx_hash: pro_tx_hash.to_byte_array().to_vec(), node_address, }) }) .collect(), threshold_public_key: Some(crypto::PublicKey { sum: Some(Bls12381(threshold_public_key.to_bytes().to_vec())), }), quorum_hash: quorum_hash.to_byte_array().to_vec(), }This change would eliminate the need for the separate
ifstatement andreturn, simplifying the code structure.
Line range hint
253-308: LGTM: Correct implementation with good error handling. Consider minor improvement.The
try_from_quorum_info_resultmethod correctly handles the conversion fromQuorumInfoResulttoValidatorSetV0, with appropriate error handling using the?operator for propagating errors.Consider using
map_errto provide more context to the error when converting theBlsPublicKey. This can help in debugging and error tracing. For example:let threshold_public_key = BlsPublicKey::from_bytes(quorum_public_key.as_slice()) .map_err(|e| ExecutionError::BlsErrorFromDashCoreResponse(e) .context("Failed to convert quorum public key"))?;This change would provide more context about where the error occurred, which can be helpful in complex systems.
packages/rs-platform-version/src/version/v1.rs (1)
984-988: LGTM! Consider adding a comment for clarity.The addition of
current_quorums_infoto the system query versions is consistent with the PR objectives and follows the existing structure. This new field will enable querying current quorum information, which is a valuable addition to the platform's capabilities.Consider adding a brief comment above this field to explain its purpose and how it relates to the new RPC for retrieving current quorum information. This would enhance code readability and maintainability.
packages/rs-platform-version/src/version/v4.rs (1)
986-990: LGTM. Consider updating documentation.The addition of the
current_quorums_infofield toDriveAbciQuerySystemVersionsis well-structured and consistent with the existing code. This new field likely enables querying current quorum information, which aligns with the PR objectives.Consider updating any relevant documentation to reflect this new querying capability for current quorum information.
packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
985-989: LGTM! Consider minor formatting adjustment.The addition of
current_quorums_infois consistent with the PR objectives and follows the existing pattern for introducing new features. This change enables querying current quorum information, which is a valuable addition to the system's capabilities.For consistency with the surrounding code, consider adjusting the indentation of the new field to match the others:
current_quorums_info: FeatureVersionBounds { - min_version: 0, - max_version: 0, - default_current_version: 0, - }, + min_version: 0, + max_version: 0, + default_current_version: 0, + },packages/rs-platform-version/src/version/mocks/v3_test.rs (1)
985-989: LGTM! Consider grouping related fields.The addition of
current_quorums_infois consistent with the PR objectives and follows the existing structure. Good job!Consider grouping this new field with other related quorum or system info fields if any exist in this struct. This could improve code readability and organization.
packages/rs-platform-version/src/version/v3.rs (1)
991-995: LGTM. Consider adding documentation for the new field.The addition of the
current_quorums_infofield toDriveAbciQuerySystemVersionsis appropriate and consistent with the structure's existing fields. This new field likely introduces the ability to query current quorum information, which aligns with the PR objectives.Consider adding a brief comment above this field to explain its purpose and any specific behavior related to quorum information querying.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- packages/dapi-grpc/protos/platform/v0/platform.proto (2 hunks)
- packages/rs-dpp/Cargo.toml (5 hunks)
- packages/rs-dpp/src/core_types/validator/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/lib.rs (1 hunks)
- packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (8 hunks)
- packages/rs-drive-abci/src/query/system/current_quorums_info/mod.rs (1 hunks)
- packages/rs-drive-abci/src/query/system/path_elements/mod.rs (1 hunks)
- packages/rs-drive-abci/src/query/system/total_credits_in_platform/mod.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/lib.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (2 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (1 hunks)
- packages/rs-platform-version/src/version/drive_abci_versions.rs (1 hunks)
- packages/rs-platform-version/src/version/mocks/v2_test.rs (1 hunks)
- packages/rs-platform-version/src/version/mocks/v3_test.rs (1 hunks)
- packages/rs-platform-version/src/version/v1.rs (1 hunks)
- packages/rs-platform-version/src/version/v2.rs (1 hunks)
- packages/rs-platform-version/src/version/v3.rs (1 hunks)
- packages/rs-platform-version/src/version/v4.rs (1 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/dapi-grpc/protos/platform/v0/platform.proto
- packages/rs-dpp/Cargo.toml
- packages/rs-dpp/src/core_types/validator/v0/mod.rs
- packages/rs-dpp/src/core_types/validator_set/v0/mod.rs
- packages/rs-dpp/src/lib.rs
- packages/rs-drive-abci/src/query/system/current_quorums_info/mod.rs
- packages/rs-drive-proof-verifier/src/lib.rs
- packages/rs-drive-proof-verifier/src/types.rs
- packages/rs-sdk/src/platform/fetch_unproved.rs
🔇 Additional comments (7)
packages/rs-drive-proof-verifier/src/unproved.rs (3)
1-124: LGTM: Well-structured trait definition with clear documentationThe imports and trait definition for
FromUnprovedare well-organized and properly documented. The trait provides a clear interface for parsing unproved responses, which enhances code readability and maintainability.
237-251: LGTM: Proper struct creation and returnThe creation of the
CurrentQuorumsInfostruct and its return along with metadata is implemented correctly. All required fields are properly populated with parsed values, and the use of metadata for certain fields is appropriate.
1-251: Overall assessment: Solid implementation with room for improvementsThe
unproved.rsfile introduces a well-structuredFromUnprovedtrait and its implementation forCurrentQuorumsInfo. The parsing logic for quorum information is comprehensive and handles complex nested structures effectively. However, there are a few areas where the code could be improved:
- Error handling could be more informative in some places, particularly for metadata extraction and
BlsPublicKeyparsing.- There's some duplication in hash validation logic that could be refactored into a helper function.
- The use of a placeholder value for
node_idshould be reconsidered to avoid potential issues.Addressing these points will enhance the robustness and maintainability of the code. Overall, the implementation is solid and provides a good foundation for handling unproved quorum information.
packages/rs-drive-abci/src/platform_types/validator_set/v0/mod.rs (2)
5-12: LGTM: Import changes align with new structure.The changes in imports reflect the restructuring of the code, particularly the introduction of the new
ValidatorSetMethodsV0trait and the shift towards using types from thedppcrate. This aligns well with the new trait-based approach and suggests improved modularity.
Line range hint
157-202: LGTM: Efficient non-consuming update method added.The new
to_updatemethod is a valuable addition, providing an efficient way to create aValidatorSetUpdatewithout consumingself. This allows for more flexible usage in different contexts.packages/rs-platform-version/src/version/v2.rs (1)
984-988: LGTM. Please provide more context about thecurrent_quorums_infofeature.The addition of the
current_quorums_infofield to thePlatformVersionstruct is implemented correctly. TheFeatureVersionBoundsare initialized appropriately with all versions set to 0, which is consistent with introducing a new feature.Could you provide more information about the purpose and functionality of the
current_quorums_infofeature? This context would be helpful for understanding its role in the platform version configuration.packages/rs-platform-version/src/version/mocks/v3_test.rs (1)
Line range hint
1-1089: Consider adding documentation and clarify version numbers.While the changes look good, I have a couple of suggestions to improve the overall code quality:
This large constant structure could benefit from some documentation explaining its purpose and how it's used in tests. Consider adding a doc comment at the beginning of the constant.
Most version numbers, including the newly added
current_quorums_info, are set to 0. While this might be intentional for a test mock, it would be helpful to confirm if this is the desired behavior or if some of these should have different values.Example documentation:
/// TEST_PLATFORM_V3 represents a mock PlatformVersion used for testing. /// It includes various feature versions and configuration settings. /// Note: Most version numbers are set to 0 for testing purposes. pub const TEST_PLATFORM_V3: PlatformVersion = ...Could you clarify if setting all version numbers to 0 is intentional for this test mock? If not, which ones should have different values?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h (1)
528-534: New methods added to Platform protocol for backwards compatibility.The addition of
getCurrentQuorumsInfoWithRequest:handler:andRPCTogetCurrentQuorumsInfoWithRequest:handler:methods to the Platform protocol maintains consistency with other RPC methods in this deprecated API set. This ensures backwards compatibility while encouraging the use of the newer Platform2 protocol.However, it's worth noting that these methods lack the deprecation warning comments present for some other methods in this protocol.
Consider adding deprecation warning comments to these new methods, similar to other methods in this protocol, to encourage users to adopt the newer Platform2 protocol. For example:
/** * This method belongs to a set of APIs that have been deprecated. Using the v2 API is recommended. */ - (void)getCurrentQuorumsInfoWithRequest:(GetCurrentQuorumsInfoRequest *)request handler:(void(^)(GetCurrentQuorumsInfoResponse *_Nullable response, NSError *_Nullable error))handler; /** * This method belongs to a set of APIs that have been deprecated. Using the v2 API is recommended. */ - (GRPCProtoCall *)RPCTogetCurrentQuorumsInfoWithRequest:(GetCurrentQuorumsInfoRequest *)request handler:(void(^)(GetCurrentQuorumsInfoResponse *_Nullable response, NSError *_Nullable error))handler;packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js (2)
292-299: LGTM! Consider adding a comment for consistency.The addition of the
getCurrentQuorumsInfomethod to thePlatformservice is well-structured and follows the established pattern. It correctly defines a unary RPC with appropriate request and response types.For consistency with other methods in this file, consider adding a brief comment describing the purpose of this method. For example:
// Get information about current quorums Platform.getCurrentQuorumsInfo = { // ... (rest of the code remains the same) };
1269-1298: LGTM! Consider refactoring for improved code reusability.The implementation of
getCurrentQuorumsInfoin thePlatformClientprototype is well-structured and consistent with other methods in the class. It correctly handles the gRPC unary call, manages callbacks, and provides error handling.To improve code reusability and reduce duplication, consider extracting the common pattern of these methods into a higher-order function. This would make the code more maintainable and less prone to errors when adding new methods. Here's a suggested refactor:
function createUnaryMethod(methodName) { return function(requestMessage, metadata, callback) { if (arguments.length === 2) { callback = arguments[1]; } var client = grpc.unary(Platform[methodName], { request: requestMessage, host: this.serviceHost, metadata: metadata, transport: this.options.transport, debug: this.options.debug, onEnd: function (response) { if (callback) { if (response.status !== grpc.Code.OK) { var err = new Error(response.statusMessage); err.code = response.status; err.metadata = response.trailers; callback(err, null); } else { callback(null, response.message); } } } }); return { cancel: function () { callback = null; client.close(); } }; }; } // Then, you can define methods like this: PlatformClient.prototype.getCurrentQuorumsInfo = createUnaryMethod('getCurrentQuorumsInfo');This refactoring would significantly reduce code duplication across all similar methods in the
PlatformClientprototype.packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py (2)
373-377: Add a docstring to thegetCurrentQuorumsInfomethod.While the implementation is correct, it's recommended to add a docstring to maintain consistency with other methods and improve code documentation.
Consider adding a docstring like this:
def getCurrentQuorumsInfo(self, request, context): """Get current quorums information. Args: request (GetCurrentQuorumsInfoRequest): The request object. context (grpc.ServicerContext): The gRPC service context. Raises: NotImplementedError: This method is not implemented. """ context.set_code(grpc.StatusCode.UNIMPLEMENTED) context.set_details('Method not implemented!') raise NotImplementedError('Method not implemented!')
1079-1094: Add a docstring to the staticgetCurrentQuorumsInfomethod.The implementation of the static method is correct and consistent with other methods in the class. However, a docstring should be added to improve code documentation and maintain consistency.
Consider adding a docstring like this:
@staticmethod def getCurrentQuorumsInfo(request, target, options=(), channel_credentials=None, call_credentials=None, insecure=False, compression=None, wait_for_ready=None, timeout=None, metadata=None): """ Retrieve current quorums information. This method is used to make a unary-unary RPC call to get the current quorums information. Args: request (GetCurrentQuorumsInfoRequest): The request message. target (str): The target server address. options (tuple): Additional options for the call. channel_credentials (grpc.ChannelCredentials): The credentials to use for the channel. call_credentials (grpc.CallCredentials): The credentials to use for the call. insecure (bool): Whether to use an insecure channel. compression (grpc.Compression): The compression method to use. wait_for_ready (bool): Whether to wait for the server to be ready. timeout (float): The call timeout in seconds. metadata (tuple): Additional metadata for the call. Returns: A future that will eventually contain the response message. """ # ... (existing implementation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java (9 hunks)
- packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js (3 hunks)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (2 hunks)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m (2 hunks)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h (3 hunks)
- packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m (1 hunks)
- packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py (4 hunks)
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (1 hunks)
- packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (3 hunks)
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts (3 hunks)
- packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.js (2 hunks)
🧰 Additional context used
🪛 Biome
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts
[error] 5853-5854: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (30)
packages/dapi-grpc/clients/platform/v0/web/platform_pb_service.d.ts (4)
286-293: New type definition looks good.The new type
PlatformgetCurrentQuorumsInfois correctly defined and follows the existing pattern for other RPC methods in this file. It properly specifies the method name, service, request/response stream types, and request/response message types.
328-328: Addition to Platform class is correct.The new static readonly property
getCurrentQuorumsInfohas been added to thePlatformclass, following the same pattern as other RPC methods. This addition is consistent with the new type definition.
642-650: New method signatures in PlatformClient class are properly implemented.Two new method signatures for
getCurrentQuorumsInfohave been added to thePlatformClientclass:
- One that accepts metadata as a parameter.
- One that doesn't require metadata.
Both signatures follow the existing pattern in the file and are correctly typed with the appropriate request and response message types.
286-293: Summary: New getCurrentQuorumsInfo RPC method successfully implemented.The changes in this file successfully implement the new
getCurrentQuorumsInfoRPC method as part of the feature to retrieve current quorum information. The additions include:
- A new type definition
PlatformgetCurrentQuorumsInfo.- An update to the
Platformclass with a new static readonly property.- Two new method signatures in the
PlatformClientclass.All changes follow the existing patterns in the file, maintaining consistency with other RPC methods. The implementation appears to be correct and complete, with no apparent issues or inconsistencies.
Also applies to: 328-328, 642-650
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.h (3)
29-30: New class declarations added for quorum information.The addition of
GetCurrentQuorumsInfoRequestandGetCurrentQuorumsInfoResponseclasses is consistent with the PR objective of introducing a new feature to retrieve current quorum information. These classes will likely be used to encapsulate the request and response data for the new RPC method.
249-251: New RPC method added to Platform2 protocol for retrieving current quorum information.The addition of the
getCurrentQuorumsInfoWithMessage:responseHandler:callOptions:method to the Platform2 protocol is in line with the PR objective. This method follows the established pattern for other RPC methods in the protocol, taking a message, response handler, and call options as parameters.
29-30: Summary of changes: New RPC method for retrieving current quorum information.The changes in this file successfully implement the new feature for retrieving current quorum information. The additions include:
- New class declarations for request and response objects.
- A new RPC method in the Platform2 protocol.
- Corresponding methods in the deprecated Platform protocol for backwards compatibility.
These changes are consistent with the existing code structure and follow the established patterns for RPC method declarations in this file. The implementation aligns well with the PR objectives and should integrate smoothly with the existing codebase.
Also applies to: 249-251, 528-534
packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbrpc.m (1)
758-776: LGTM! New method implementation is consistent with existing patterns.The new
getCurrentQuorumsInfomethod is well-implemented and follows the established pattern in thePlatformclass. It correctly includes three variations:
- A simple method to start the RPC call
- A method to create and return a not-yet-started RPC object
- A method to create and return a unary proto call
The implementation uses appropriate types (
GetCurrentQuorumsInfoRequestandGetCurrentQuorumsInfoResponse) and follows the correct naming conventions. The use ofGRPCProtoCallandGRPCUnaryProtoCallis consistent with gRPC best practices.packages/dapi-grpc/clients/platform/v0/python/platform_pb2_grpc.py (3)
172-176: LGTM: New methodgetCurrentQuorumsInfoadded toPlatformStub.The implementation follows the established pattern for other methods in the class. It correctly sets up the gRPC channel, and handles serialization and deserialization appropriately.
537-541: LGTM:getCurrentQuorumsInfocorrectly added toadd_PlatformServicer_to_server.The new method is properly integrated into the
rpc_method_handlersdictionary, maintaining consistency with other entries. Serialization and deserialization are handled correctly.
Line range hint
1-1094: Summary: NewgetCurrentQuorumsInfomethod successfully integrated.The changes effectively introduce the new
getCurrentQuorumsInfofunctionality to the gRPC service. The implementation is consistent with existing code patterns and correctly handles serialization, deserialization, and gRPC channel setup. Minor improvements in documentation (adding docstrings) were suggested to enhance code readability and maintain consistency with best practices.packages/dapi-grpc/clients/platform/v0/java/org/dash/platform/dapi/v0/PlatformGrpc.java (4)
979-1008: Method descriptor forgetCurrentQuorumsInfolooks good.The implementation of the method descriptor for
getCurrentQuorumsInfois correct and consistent with other method descriptors in the file. It properly sets up the request and response types, method type, full method name, and marshalling for request and response objects.
1290-1295: Implementations ofgetCurrentQuorumsInfoin service and client stubs are correct.The
getCurrentQuorumsInfomethod has been properly implemented in:
PlatformImplBaseclassPlatformStubclassPlatformBlockingStubclassPlatformFutureStubclassAll implementations use the correct request and response types, have matching method signatures, and use proper call options and channel methods.
Also applies to: 1804-1810, 2059-2064, 2344-2350
2384-2384: Method ID forgetCurrentQuorumsInfois correctly assigned.The method ID for
getCurrentQuorumsInfois properly defined with the value 31, which appears to be the next available ID in the sequence. This assignment is consistent with the pattern used for other methods in the file.
2527-2530:getCurrentQuorumsInfois correctly included in the service definition.The
getCurrentQuorumsInfomethod has been properly integrated into the service definition:
- It's correctly added to the
MethodHandlersswitch statement with the right method ID.- The method is included in the service descriptor builder.
These additions ensure that the new method is fully integrated into the gRPC service.
Also applies to: 2623-2623
packages/dapi-grpc/clients/platform/v0/nodejs/platform_protoc.js (3)
78-85: New exports for GetCurrentQuorumsInfo messagesThe code correctly exports the new message types for
GetCurrentQuorumsInfoRequestandGetCurrentQuorumsInfoResponse, along with their nested classes. This ensures that these messages are available for use throughout the application.
4133-4258: Definition of GetCurrentQuorumsInfoRequest and related message classesThe code properly defines constructors and prototype inheritance for the new message types:
GetCurrentQuorumsInfoRequestGetCurrentQuorumsInfoRequest.GetCurrentQuorumsInfoRequestV0GetCurrentQuorumsInfoResponseGetCurrentQuorumsInfoResponse.ValidatorV0GetCurrentQuorumsInfoResponse.ValidatorSetV0GetCurrentQuorumsInfoResponse.GetCurrentQuorumsInfoResponseV0This setup is essential for correct message serialization and deserialization in the gRPC client.
44168-45523: Implementation of serialization and deserialization methodsThe code implements serialization and deserialization methods for the newly added messages. By defining methods like
serializeBinary,deserializeBinary, andtoObject, the messages can be accurately converted to and from the protobuf wire format. This ensures seamless communication over the network.packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.h (2)
57-60: New Class Declarations Added CorrectlyThe new class declarations for
GetCurrentQuorumsInfoRequest_GetCurrentQuorumsInfoRequestV0,GetCurrentQuorumsInfoResponse_GetCurrentQuorumsInfoResponseV0,GetCurrentQuorumsInfoResponse_ValidatorSetV0, andGetCurrentQuorumsInfoResponse_ValidatorV0are appropriately added and follow the existing naming conventions.
4840-4963: Implementation of New RPC Methods Aligns with Codebase StandardsThe addition of the
GetCurrentQuorumsInfoRequestandGetCurrentQuorumsInfoResponseclasses, along with their associated nested classes and enums, is well-structured and consistent with the codebase's existing patterns. The properties and methods are accurately defined, and the implementation adheres to Objective-C conventions.packages/dapi-grpc/clients/platform/v0/objective-c/Platform.pbobjc.m (7)
67-72: New class declarations are correctly addedThe class declarations for the new classes related to quorum information are added properly and follow the naming conventions:
GetCurrentQuorumsInfoRequestGetCurrentQuorumsInfoRequest_GetCurrentQuorumsInfoRequestV0GetCurrentQuorumsInfoResponseGetCurrentQuorumsInfoResponse_GetCurrentQuorumsInfoResponseV0GetCurrentQuorumsInfoResponse_ValidatorSetV0GetCurrentQuorumsInfoResponse_ValidatorV0
12446-12497: Implementation ofGetCurrentQuorumsInfoRequestis correctly definedThe
GetCurrentQuorumsInfoRequestclass is implemented correctly with dynamic properties and the descriptor method. The oneof fieldversionis set up appropriately, and the storage structure is properly defined.
12498-12533: Implementation ofGetCurrentQuorumsInfoRequest_GetCurrentQuorumsInfoRequestV0is correctly definedThe subclass
GetCurrentQuorumsInfoRequest_GetCurrentQuorumsInfoRequestV0is implemented properly with an empty descriptor, as there are no fields defined. The class setup is correct and follows the required structure.
12536-12593: Implementation ofGetCurrentQuorumsInfoResponseis correctly definedThe
GetCurrentQuorumsInfoResponseclass is implemented correctly with dynamic properties and the descriptor method. The oneof fieldversionis set up appropriately, matching the request class structure.
12594-12658: Implementation ofGetCurrentQuorumsInfoResponse_ValidatorV0is correctly definedThe
GetCurrentQuorumsInfoResponse_ValidatorV0class is implemented properly with dynamic properties forproTxHash,nodeIp, andisBanned. The storage structure and field descriptors are correctly set up, ensuring proper memory management.
12660-12737: Implementation ofGetCurrentQuorumsInfoResponse_ValidatorSetV0is correctly definedThe
GetCurrentQuorumsInfoResponse_ValidatorSetV0class is implemented correctly with dynamic properties forquorumHash,coreHeight,membersArray, andthresholdPublicKey. The storage structure and field descriptors are accurately defined.
12739-12828: Implementation ofGetCurrentQuorumsInfoResponse_GetCurrentQuorumsInfoResponseV0is correctly definedThe
GetCurrentQuorumsInfoResponse_GetCurrentQuorumsInfoResponseV0class is implemented properly with dynamic properties forquorumHashesArray,currentQuorumHash,validatorSetsArray,lastBlockProposer, andmetadata. The storage structure and field descriptors are appropriately set up, ensuring accurate data handling.packages/dapi-grpc/clients/platform/v0/web/platform_pb.js (3)
78-85: New export symbols for 'GetCurrentQuorumsInfo' messages added.The new message types
GetCurrentQuorumsInfoRequest,GetCurrentQuorumsInfoResponse, and their nested versions are correctly exported.
4133-4258: Definition of 'GetCurrentQuorumsInfoRequest' message classes.The classes for
GetCurrentQuorumsInfoRequestand its nestedGetCurrentQuorumsInfoRequestV0are properly defined, including constructors and inheritance fromjspb.Message.
44168-45524: Implementation of serialization and deserialization methods.The serialization, deserialization, and
toObjectmethods forGetCurrentQuorumsInfoRequest,GetCurrentQuorumsInfoResponse, and their nested message types are correctly implemented following Protocol Buffers JavaScript conventions.
| export type AsObject = { | ||
| } |
There was a problem hiding this comment.
Avoid using '{}' as a type; explicitly define the object shape.
The AsObject type is currently defined as an empty object {}. Using {} as a type is discouraged because it matches any non-nullable value, which may not be the intended behavior. It's better to explicitly define the object shape or omit the AsObject type if it has no fields.
Apply this diff to remove the empty AsObject type:
export namespace GetCurrentQuorumsInfoRequestV0 {
- export type AsObject = {
- }
}📝 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.
| export type AsObject = { | |
| } | |
| export namespace GetCurrentQuorumsInfoRequestV0 { | |
| } |
🧰 Tools
🪛 Biome
[error] 5853-5854: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| /// * `Ok(Some(Self))` when object is found. | ||
| /// * `Ok(None)` when object is not found. | ||
| /// * [`Err(Error)`](Error) when an error occurs. | ||
| async fn fetch_unproved_with_settings( |
There was a problem hiding this comment.
Can we have a default implementation here, in a similar way to Fetch?
There was a problem hiding this comment.
Let's just do that in another PR.
Issue being fixed or feature implemented
We wanted to be able to get the current quorum info from the state.
What was done?
Added a new RPC.
Moved Validator Set and Validator Types to DPP.
Added the ability to get unproved data through sdk.
How Has This Been Tested?
Main tests.
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores