feat(sdk): get node status #2139
Conversation
pauldelucia
requested review from
shumkov and
QuantumExplorer
as code owners
WalkthroughThe changes involve multiple updates across several files in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 7
🧹 Outside diff range and nitpick comments (10)
packages/rs-dpp/src/node/mod.rs (1)
1-1: LGTM! Consider adding a brief module-level documentation comment.The addition of the
statusmodule aligns well with the PR objectives and follows good Rust practices for code organization. This will provide a clear location for the newEvonodeStatusstructure and related functionality.Consider adding a brief module-level documentation comment to provide context about the purpose of this module. For example:
/// Module for managing node status information. pub mod status;This would enhance code readability and make it easier for other developers to understand the purpose of this module at a glance.
packages/rs-dpp/src/node/status/v0/mod.rs (2)
4-11: LGTM: Well-structured and documentedEvonodeStatusV0struct.The
EvonodeStatusV0struct is well-defined with appropriate derive macros and clear field names. The documentation provides a good overview of its purpose.Consider adding
#[serde(rename_all = "camelCase")]attribute to the struct for consistent JSON serialization if this struct is used in API responses. This would ensure that the field names in JSON match common JavaScript naming conventions./// Information about the status of an Evonode #[derive(Clone, Debug, PartialEq, Encode, Decode, Serialize, Deserialize)] +#[serde(rename_all = "camelCase")] pub struct EvonodeStatusV0 { // ... existing fields ... }
13-20: LGTM: Well-definedEvonodeStatusV0Getterstrait.The
EvonodeStatusV0Getterstrait is correctly defined with appropriate method signatures corresponding to the struct fields. The documentation provides a clear explanation of its purpose.Consider using associated types or generics in the trait to make it more flexible. This would allow for different implementations to return different types, which could be useful if you need to support different versions or representations of the data in the future.
Here's an example of how you could refactor the trait:
pub trait EvonodeStatusV0Getters { type ProTxHash: AsRef<str>; type BlockHeight: Into<u64>; /// Returns the Evonode proTxHash fn pro_tx_hash(&self) -> Self::ProTxHash; /// Returns the Evonode's latest stored block height fn latest_block_height(&self) -> Self::BlockHeight; }This change would allow implementors to use types that can be converted to the required
Stringandu64, providing more flexibility while maintaining type safety.packages/rs-dpp/src/node/status/mod.rs (2)
9-23: LGTM:EvonodeStatusenum is well-defined with appropriate derive macros.The
EvonodeStatusenum is correctly defined with a single variantV0, suggesting good design for future extensibility. The derive macros cover all necessary traits for serialization, deserialization, and common operations.Consider expanding the documentation comment to include a brief explanation of why the enum is used (e.g., for versioning) and what
EvonodeStatusV0represents. This would enhance the clarity for future developers working with this code.- /// Information about the status of an Evonode + /// Information about the status of an Evonode. + /// + /// This enum is used for versioning the Evonode status structure. + /// Currently, it only contains V0, which represents the initial version + /// of the Evonode status information.
25-39: LGTM:EvonodeStatusV0Getterstrait implementation is correct.The implementation of
EvonodeStatusV0GettersforEvonodeStatusis well-structured and correctly handles the single variant case. The methodspro_tx_hashandlatest_block_heightprovide access to the underlyingEvonodeStatusV0data.Consider the following suggestions for improvement:
- Performance optimization: The
pro_tx_hashmethod currently clones the string. If this method is called frequently, it might be worth considering returning a reference instead:- fn pro_tx_hash(&self) -> String { - match self { - EvonodeStatus::V0(v0) => v0.pro_tx_hash.clone(), - } - } + fn pro_tx_hash(&self) -> &str { + match self { + EvonodeStatus::V0(v0) => &v0.pro_tx_hash, + } + }
- Documentation: Enhance the method documentation to include more details about the return values:
- /// Returns the Evonode Identifier + /// Returns the Evonode Identifier (ProTxHash) + /// + /// This identifier uniquely identifies the Evonode in the network. - /// Returns the Evonode's latest stored block height + /// Returns the Evonode's latest stored block height + /// + /// This represents the height of the most recent block that the Evonode has processed and stored.These changes would improve performance for frequent calls and provide more context for developers using these methods.
packages/rs-sdk/README.md (1)
Line range hint
100-126: LGTM! Minor suggestion for improved clarity.The changes in this section look good and align with the PR objectives. The updates to terminology (FetchAny → FetchMany) and the protobuf messages path are consistent and appear to be correct.
Consider adding a brief explanation of the difference between
FetchandFetchManyfor developers who might be new to the SDK. This could be added after line 102, for example:- `Fetch`: Used for retrieving a single object. - `FetchMany`: Used for retrieving multiple objects in a single request.This addition would provide more context and clarity for developers implementing these traits.
packages/rs-sdk/src/platform/fetch.rs (1)
285-288: LGTM: Fetch implementation for EvonodeStatusThe implementation of
FetchforEvonodeStatusis correct and consistent with other implementations in the file. It properly associatesEvonodeStatuswithGetStatusRequest, aligning with the PR objective.Consider adding a brief doc comment above this implementation to explain its purpose, similar to other implementations in this file. For example:
/// Implements `Fetch` for `EvonodeStatus` to allow fetching node status information. impl Fetch for EvonodeStatus { type Request = platform_proto::GetStatusRequest; }packages/rs-dapi-client/src/transport/grpc.rs (1)
415-423: LGTM! Consider adding a brief comment for clarity.The implementation of
GetStatusRequestis correct and consistent with other gRPC request implementations in this file. It properly utilizes theplatform_protomodule and thePlatformGrpcClient, aligning with the PR objective of implementing the newgetStatusdapi-grpc call.Consider adding a brief comment above the implementation to describe the purpose of the
getStatuscall, similar to comments present for some other implementations in this file. This would enhance code readability and maintainability.+// Retrieves the status of the node impl_transport_request_grpc!( platform_proto::GetStatusRequest, platform_proto::GetStatusResponse, PlatformGrpcClient, RequestSettings::default(), get_status );packages/rs-sdk/src/platform/query.rs (1)
650-660: LGTM: New Query implementation for GetStatusRequest added.The new implementation of
Query<GetStatusRequest>for the unit type()is consistent with other implementations in the file and correctly creates a newGetStatusRequestwith the appropriate version.However, consider handling the
proveparameter consistently with other implementations in this file. Most other implementations check ifproveis true and throw an "unimplemented" error if it's false. You might want to add this check for consistency, or document why this implementation differs.Consider adding the following check at the beginning of the
querymethod:impl Query<GetStatusRequest> for () { fn query(self, _prove: bool) -> Result<GetStatusRequest, Error> { - // ignore proof + if !_prove { + unimplemented!("queries without proofs are not supported yet"); + } let request: GetStatusRequest = GetStatusRequest { version: Some(get_status_request::Version::V0(GetStatusRequestV0 {})), }; Ok(request) } }packages/rs-drive-proof-verifier/src/proof.rs (1)
1761-1762: Use a specific error variant for missing response versionWhen handling the absence of a version in the response, the code returns a
ProtocolError::Genericerror:return Err(ProtocolError::Generic("No version in the response".to_string()).into())For clearer error handling and better maintainability, consider defining and using a more specific error variant, such as
Error::EmptyVersion, instead ofProtocolError::Generic. This will make it easier to identify and handle this specific error case in the future.Apply this diff to introduce a new error variant and use it:
+// Define a new error variant +#[derive(Debug)] +pub enum Error { + EmptyVersion, + // ... existing variants +} // In the match statement match response.version { Some(version) => version, None => { - return Err(ProtocolError::Generic("No version in the response".to_string()).into()) + return Err(Error::EmptyVersion) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- packages/rs-dapi-client/src/address_list.rs (2 hunks)
- packages/rs-dapi-client/src/dapi_client.rs (1 hunks)
- packages/rs-dapi-client/src/transport/grpc.rs (1 hunks)
- packages/rs-dpp/src/lib.rs (1 hunks)
- packages/rs-dpp/src/node/mod.rs (1 hunks)
- packages/rs-dpp/src/node/status/mod.rs (1 hunks)
- packages/rs-dpp/src/node/status/v0/mod.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/proof.rs (3 hunks)
- packages/rs-sdk/README.md (3 hunks)
- packages/rs-sdk/src/mock/requests.rs (2 hunks)
- packages/rs-sdk/src/platform/fetch.rs (2 hunks)
- packages/rs-sdk/src/platform/query.rs (3 hunks)
- packages/rs-sdk/src/sdk.rs (1 hunks)
🧰 Additional context used
LanguageTool
packages/rs-sdk/README.md
[uncategorized] ~126-~126: Possible missing article found.
Context: ...gRetrievedObjectsthat will store collection of returned objects, indexed by some k...(AI_HYDRA_LEO_MISSING_A)
🔇 Additional comments not posted (14)
packages/rs-dpp/src/node/status/v0/mod.rs (2)
1-2: LGTM: Imports are appropriate and necessary.The imports from
bincodeandserdeare correctly used for the struct's derive macros, enabling serialization and deserialization capabilities.
1-20: Overall: Well-implemented new module for Evonode status.This new module successfully introduces the
EvonodeStatusV0struct andEvonodeStatusV0Getterstrait, aligning with the PR objectives. The code is well-structured, properly documented, and follows Rust best practices. The use ofbincodeandserdefor serialization is appropriate for the intended use case.A few minor suggestions were made to potentially improve the code:
- Consider adding
#[serde(rename_all = "camelCase")]to the struct for consistent JSON serialization.- The trait could be made more flexible by using associated types or generics.
These suggestions are optional and do not detract from the overall quality and correctness of the implementation.
packages/rs-dpp/src/node/status/mod.rs (2)
1-7: LGTM: Module declaration and imports look appropriate.The module declaration and imports are well-organized and include all necessary components for the implementation of
EvonodeStatus. The use of custom serialization traits (PlatformSerialize,PlatformDeserialize) is noted and appears to be in line with the project's serialization requirements.
1-39: Overall implementation is well-structured and aligns with PR objectives.The introduction of the
EvonodeStatusenum and its associated functionality in this file is well-implemented and aligns with the PR objectives. Key points:
- The code structure allows for future versioning of the Evonode status.
- Appropriate use of traits and derive macros for serialization and common operations.
- The implementation of
EvonodeStatusV0Gettersprovides a clean interface for accessing Evonode status information.The code follows Rust best practices and is well-organized. With the minor suggestions provided in previous comments, this implementation provides a solid foundation for handling Evonode status information in the SDK.
packages/rs-dapi-client/src/address_list.rs (1)
88-89: Consider the implications of derivingCloneforAddressListWhile deriving
CloneforAddressListprovides flexibility in usage, it's important to consider the potential performance implications, especially if large instances are frequently cloned.As per a previous comment from shumkov, it might be preferable to return references instead of encouraging cloning. This approach could be more efficient, especially for large address lists.
To assess the impact, let's check if there are any places in the codebase where
AddressListis being cloned:Consider documenting the rationale for adding
Cloneand any guidelines for its usage to prevent potential misuse.packages/rs-sdk/src/mock/requests.rs (3)
9-9: LGTM: New import forEvonodeStatusadded.The import statement for
EvonodeStatusfrom thedpp::node::statusmodule is correctly placed and necessary for the new implementation ofMockResponseforEvonodeStatus.
Line range hint
1-247: Summary: Changes enhance mock response capabilities.The changes in this file successfully add support for
EvonodeStatusin mock responses. The new import and implementation are consistent with existing patterns in the file and align with the PR objectives. These changes will allow for better testing and mocking of the newgetStatusfunctionality in the SDK.
247-247: LGTM:MockResponseimplementation added forEvonodeStatus.The implementation of
MockResponseforEvonodeStatususing theimpl_mock_response!macro is consistent with other implementations in the file and allowsEvonodeStatusto be used in mock expectation responses. This change aligns with the PR objectives of utilizing the newgetStatusdapi-grpc call within the rs-sdk.To ensure the new
EvonodeStatusmock is being utilized correctly, let's verify its usage in the codebase:✅ Verification successful
Let's update the verification scripts to ensure we capture all instances of
EvonodeStatususage in the codebase.
Verified:
MockResponseimplementation forEvonodeStatusis correctly added and utilized.The
impl_mock_response!(EvonodeStatus);inpackages/rs-sdk/src/mock/requests.rsappropriately integratesEvonodeStatusinto mock expectation responses. No additional usages or TODOs related toEvonodeStatusmocking were found, ensuring the implementation is correctly scoped and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of EvonodeStatus in mock responses # Test 1: Check for imports of EvonodeStatus in test files echo "Checking for imports of EvonodeStatus in test files:" rg --type rust 'use .*EvonodeStatus' -g 'tests/**/*.rs' # Test 2: Look for usage of EvonodeStatus in mock setup echo "Checking for usage of EvonodeStatus in mock setup:" rg --type rust 'mock.*EvonodeStatus' -g 'tests/**/*.rs' # Test 3: Verify if there are any TODO comments related to EvonodeStatus mocking echo "Checking for TODO comments related to EvonodeStatus mocking:" rg --type rust 'TODO.*EvonodeStatus.*mock' -g 'tests/**/*.rs'Length of output: 983
Script:
#!/bin/bash # Description: Verify the usage of EvonodeStatus in mock responses without restrictive path filters # Test 1: Check for imports of EvonodeStatus in all Rust files echo "Checking for imports of EvonodeStatus in Rust files:" rg --type rust 'use .*EvonodeStatus' # Test 2: Look for usage of EvonodeStatus in mock setup across all Rust files echo "Checking for usage of EvonodeStatus in mock setup:" rg --type rust 'mock.*EvonodeStatus' # Test 3: Verify if there are any TODO comments related to EvonodeStatus mocking in all Rust files echo "Checking for TODO comments related to EvonodeStatus mocking:" rg --type rust 'TODO.*EvonodeStatus.*mock'Length of output: 816
packages/rs-dapi-client/src/dapi_client.rs (1)
115-119: LGTM! The new method aligns with best practices and previous feedback.The
address_listmethod provides a clean way to access theaddress_listwithout unnecessary cloning. This implementation addresses the previous review comment about avoiding cloning on every call. It's a good practice to return a reference, allowing the user to decide whether they need to clone or not.packages/rs-sdk/src/platform/fetch.rs (2)
16-17: LGTM: Import modification for EvonodeStatusThe addition of
node::status::EvonodeStatusto the imports from thedppcrate is consistent with the PR objective. It's correctly placed within the existing import group and doesn't conflict with other imports.
16-17: Summary: Successful integration of EvonodeStatusThe changes in this file successfully integrate the new
EvonodeStatustype into the existingFetchframework. The modifications are minimal, focused, and follow established patterns, which reduces the risk of introducing bugs. These changes effectively support the PR objective of utilizing the newgetStatusdapi-grpc call within the rs-sdk.Key points:
- Appropriate import of
EvonodeStatusfrom thedppcrate.- Correct implementation of
FetchforEvonodeStatus.The changes look good and are ready for merging, pending any minor documentation improvements suggested earlier.
Also applies to: 285-288
packages/rs-dapi-client/src/transport/grpc.rs (1)
Line range hint
1-423: Overall, the changes enhance DAPI client capabilities without introducing breaking changes.The addition of the
GetStatusRequestimplementation successfully integrates the newgetStatusdapi-grpc call into the rs-sdk. This change:
- Follows established patterns in the file.
- Doesn't modify any existing code.
- Enhances the DAPI client's functionality.
The implementation aligns well with the PR objectives and maintains the code's consistency and structure.
packages/rs-sdk/src/platform/query.rs (1)
12-12: LGTM: New import added correctly.The new import for
GetStatusRequestV0is correctly placed and consistent with other imports from the same module. This import is necessary for the new implementation ofQueryforGetStatusRequest.packages/rs-drive-proof-verifier/src/proof.rs (1)
1767-1769: Handle missingpro_tx_hashmore explicitlyIn the extraction of
pro_tx_hashfrom the response:let pro_tx_hash = match v0.node { Some(node) => node.pro_tx_hash, None => None, };If
v0.nodeornode.pro_tx_hashisNone, the code proceeds without thepro_tx_hash. Verify whether this is the desired behavior. If thepro_tx_hashis essential for constructing theEvonodeStatus, consider returning an error or providing a default value with appropriate documentation.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
packages/rs-sdk/src/platform/types.rs (1)
3-3: LGTM! Consider adding a brief comment for the new module.The addition of the
evonodemodule is consistent with the existing structure and naming conventions. It's correctly declared as public and placed in alphabetical order among other module declarations.To maintain consistency with the file-level documentation, consider adding a brief inline comment explaining the purpose of the
evonodemodule, similar to:/// Type-specific implementation for Evonode-related functionality pub mod evonode;packages/rs-sdk/src/platform.rs (1)
35-35: Consider adding documentation for the newFetchUnprovedfunctionality.While the addition of
FetchUnprovedis consistent with the existing structure, it would be beneficial to include documentation explaining its purpose and usage.Consider adding a brief comment or documentation string above the
fetch_unprovedmodule declaration to explain its functionality and how it differs from the existingFetchandFetchManyoperations.packages/rs-sdk/src/platform/types/evonode.rs (2)
84-85: Remove unnecessarydropcalls forclientandpool.In Rust, variables are automatically dropped when they go out of scope. Explicitly calling
drop(client);anddrop(pool);is unnecessary unless you need to release resources immediately before the end of the scope. Since there is no specific reason to drop them early in this context, you can remove these calls for cleaner code.Apply this diff to remove the explicit
dropstatements:84 - drop(client); 85 - drop(pool);
31-32: Consider derivingSerializeandDeserializeunconditionally if needed.Currently,
SerializeandDeserializeare derived only when the "mocks" feature is enabled. If serialization and deserialization might be useful outside of mocks—for example, for caching or logging—you might consider deriving them unconditionally.31 -#[cfg_attr(feature = "mocks", derive(Serialize, Deserialize))] +#[derive(Serialize, Deserialize)]packages/rs-drive-proof-verifier/src/unproved.rs (1)
278-278: Unnecessary use of#[async_trait]attributeThe
#[async_trait]attribute is applied, butmaybe_from_unproved_with_metadatais a synchronous function. Since there are no asynchronous functions in this implementation, the attribute can be removed to keep the code clean.Apply this diff to remove the unnecessary attribute:
-#[async_trait]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
- packages/rs-dapi-client/src/lib.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/proof.rs (4 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (3 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (2 hunks)
- packages/rs-sdk/Cargo.toml (3 hunks)
- packages/rs-sdk/src/lib.rs (1 hunks)
- packages/rs-sdk/src/mock.rs (1 hunks)
- packages/rs-sdk/src/mock/requests.rs (2 hunks)
- packages/rs-sdk/src/platform.rs (1 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (3 hunks)
- packages/rs-sdk/src/platform/query.rs (3 hunks)
- packages/rs-sdk/src/platform/types.rs (1 hunks)
- packages/rs-sdk/src/platform/types/evonode.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-sdk/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive-proof-verifier/src/proof.rs
- packages/rs-sdk/src/platform/query.rs
🔇 Additional comments (21)
packages/rs-sdk/src/platform.rs (2)
Line range hint
3-7: Acknowledge the TODO comment and its importance.The TODO comment provides valuable context for the current implementation and future plans for the SDK. It's crucial to keep this comment updated as the SDK evolves.
As the SDK development progresses, please ensure that this comment remains accurate and reflects the current state and plans for the platform DAPI requests.
35-35: New exportFetchUnprovedlooks good.The addition of
FetchUnprovedis consistent with the existing pattern of exports in this file. It appears to introduce a new functionality for fetching data without proof.To ensure the completeness of this change, please verify the implementation of the
fetch_unprovedmodule:✅ Verification successful
FetchUnprovedExport Verified Successfully.The
fetch_unprovedmodule and theFetchUnprovedtrait are present as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the fetch_unproved module # Test: Check if the fetch_unproved module exists and contains the FetchUnproved struct or trait rg --type rust -e "mod fetch_unproved" -e "struct FetchUnproved" -e "trait FetchUnproved" packages/rs-sdk/srcLength of output: 234
packages/rs-sdk/src/mock.rs (3)
41-43: Summary: Enhancements to mocking support look good.The changes in this file appropriately extend the mocking capabilities of the Dash SDK:
- The crate-level export of
BINCODE_CONFIGallows for consistent serialization configuration within the crate when mocks are enabled.- The public export of
MockDashPlatformSdkprovides users with access to the mock SDK implementation when the "mocks" feature is enabled.Both additions are conditionally compiled, ensuring they don't affect the crate's behavior when mocks are disabled. This approach maintains backwards compatibility while enhancing the mocking functionality.
These changes align well with the module's purpose and seem to be a positive addition to the mocking support.
42-43: LGTM. Verify MockDashPlatformSdk implementation and usage.The public export of
MockDashPlatformSdkis appropriate for the mocking functionality. It makes the mock SDK publicly accessible when the "mocks" feature is enabled, which is consistent with the module's purpose.To ensure this addition is properly implemented and utilized, let's verify its implementation and usage:
#!/bin/bash # Description: Verify the implementation and usage of MockDashPlatformSdk # Test 1: Check the implementation of MockDashPlatformSdk ast-grep --lang rust --pattern $'struct MockDashPlatformSdk { $$$ }' packages/rs-sdk/src/sdk.rs # Test 2: Search for MockDashPlatformSdk usage in tests rg --type rust 'MockDashPlatformSdk' packages/rs-sdk/tests
41-41: LGTM. Verify usage of BINCODE_CONFIG.The addition of this crate-level export for
BINCODE_CONFIGlooks good. It enhances the mock functionality by making the configuration available within the crate when mocks are enabled.To ensure this addition is properly utilized, let's verify its usage:
packages/rs-dapi-client/src/lib.rs (1)
17-17: LGTM. Consider verifying documentation forConnectionPool.The addition of
ConnectionPoolto the public exports aligns with the PR objective and is consistent with the module's structure. This change enhances the module's interface by makingConnectionPoolavailable for external use.To ensure comprehensive documentation, please run the following script:
If the
ConnectionPoolor its public methods lack documentation, consider adding it to improve the crate's usability.✅ Verification successful
Documentation for
ConnectionPoolis adequately provided.The
ConnectionPoolstruct and its public methods (new,get) are properly documented, enhancing the crate's usability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for documentation of ConnectionPool # Test: Search for ConnectionPool documentation rg --type rust -A 5 'pub struct ConnectionPool' packages/rs-dapi-client/src/connection_pool.rs # Test: Verify if there are any public methods or associated functions rg --type rust 'impl ConnectionPool' -A 10 packages/rs-dapi-client/src/connection_pool.rsLength of output: 1135
packages/rs-sdk/Cargo.toml (1)
8-8: New dependency added: bincodeThe addition of the
bincodedependency looks good. It's correctly specified with a version, feature, and marked as optional. However, there are a couple of points to consider:
- The version "2.0.0-rc.3" is a release candidate. While it's fine to use for development, consider if a stable version would be more appropriate for production use.
- The
serdefeature is enabled, which aligns well with the serialization needs often associated with bincode usage.To ensure this dependency is used in the codebase, let's run a quick check:
✅ Verification successful
'bincode' dependency is correctly utilized in the codebase
The
bincodedependency is actively used across multiple modules, validating its addition to theCargo.toml. However, since version "2.0.0-rc.3" is a release candidate, consider using a stable release for better reliability in production environments.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for bincode usage in Rust files rg --type rust 'use.*bincode' || echo "No direct usage of bincode found in Rust files."Length of output: 37398
packages/rs-sdk/src/mock/requests.rs (4)
18-21: LGTM: New imports align with PR objectives.The addition of
CurrentQuorumsInfoandEvonodeStatusto the imports is consistent with the PR's goal of implementing support for the newgetStatusdapi-grpc call. These imports are necessary for the mock response implementations added later in the file.
247-248: LGTM: New mock response implementations added.The addition of mock response implementations for
EvonodeStatusandCurrentQuorumsInfois consistent with the PR objectives. The use of theimpl_mock_response!macro ensures these new implementations follow the same pattern as existing ones, maintaining consistency in the codebase.
Line range hint
1-248: Overall, the changes look good and align with the PR objectives.The modifications to this file are well-structured and consistent with the existing codebase. The new imports, visibility change, and mock response implementations all contribute to supporting the new
getStatusdapi-grpc call as intended. The use of existing patterns (like theimpl_mock_response!macro) for new implementations is commendable.A minor point to verify is the necessity of the
BINCODE_CONFIGvisibility change, but otherwise, the changes appear to be complete and correct.
25-25: Visibility change looks good, but please verify necessity.The change of
BINCODE_CONFIGvisibility topub(crate)is appropriate as it maintains encapsulation while allowing access within the crate. However, it's important to ensure this change is necessary.Could you please confirm that this visibility change is required by other modules within the crate? If so, could you provide a brief explanation of where it's used?
✅ Verification successful
Visibility change approved.
The
BINCODE_CONFIGvisibility change topub(crate)is appropriate as all usages are confined within thers-sdkcrate. External crates are not affected by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of BINCODE_CONFIG outside this file rg --type rust -g '!**/mock/requests.rs' 'BINCODE_CONFIG'Length of output: 1392
packages/rs-sdk/src/platform/fetch_unproved.rs (7)
2-5: Necessary imports added correctlyThe additional imports of
MockResponse,ResponseMetadata, andEvonodeStatusare necessary for the updated functionality and are correctly specified.
10-10: ImportingQueryfor flexible queryingThe import of
Queryenables the use of the genericqueryparameter in the methods, enhancing query flexibility.
15-15: AddedMockResponseto trait boundsIncluding
MockResponsein the trait bounds ensures that implementing types can provide mock responses, which is beneficial for testing and development purposes.
30-43: Updatedfetch_unprovedmethod for flexible queryingThe method signature of
fetch_unprovedhas been updated to accept a generic parameterQimplementingQuery, along with aqueryparameter. This change allows for more flexible and reusable querying capabilities. The method correctly delegates tofetch_unproved_with_settingswith default settings and properly handles the return value.
57-79: Enhancedfetch_unproved_with_settingsmethodThe
fetch_unproved_with_settingsmethod now accepts aqueryparameter and returns a tuple containing the optional object andResponseMetadata. This provides additional information about the response, which can be valuable for clients. The default implementation correctly constructs the request from the query, executes it, and parses the response along with metadata.
83-85: ImplementedFetchUnprovedforCurrentQuorumsInfoThe implementation correctly specifies
GetCurrentQuorumsInfoRequestas the associated request type forCurrentQuorumsInfo, aligning with the structure of the trait.
87-89: ImplementedFetchUnprovedforEvonodeStatusThe implementation appropriately defines
GetStatusRequestas the associated request type forEvonodeStatus, supporting the newgetStatusdapi-grpc call as per the PR objectives.packages/rs-sdk/src/platform/types/evonode.rs (1)
74-76: Ensure timeout settings are correctly applied.The condition
if !settings.timeout.is_zero()checks if the timeout is set. Make sure thatgrpc_request.set_timeout(settings.timeout);correctly applies the timeout to the gRPC request. Verify that theset_timeoutmethod behaves as expected with the provided timeout value.To confirm that the timeout is applied, you may run the following script:
packages/rs-drive-proof-verifier/src/types.rs (2)
8-8: ImportingGetStatusResponseis appropriateThe import of
GetStatusResponseis necessary for the implementation ofTryFrom<GetStatusResponse>forEvonodeStatus.
29-29: Importinghex::ToHexfor hexadecimal encodingImporting
hex::ToHexis appropriate for encoding thepro_tx_hashto a hexadecimal string.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/rs-sdk/tests/fetch/mod.rs (1)
Line range hint
1-1: Consider creating an issue for the TODO comment.There's a TODO comment at the beginning of the file suggesting to rename the test package. While this is unrelated to the current changes, it might be beneficial to create an issue to track this task for future improvements.
Would you like me to create a GitHub issue to track the task of renaming the test package?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/rs-sdk/src/platform/types/evonode.rs (1 hunks)
- packages/rs-sdk/tests/fetch/evonode.rs (1 hunks)
- packages/rs-sdk/tests/fetch/mod.rs (1 hunks)
🔇 Additional comments (7)
packages/rs-sdk/tests/fetch/mod.rs (2)
20-20: LGTM: Newevonodemodule added successfully.The addition of the
evonodemodule aligns with the PR objectives to implement functionality related to the newgetStatusdapi-grpc call within the rs-sdk. The module is correctly placed alongside other related modules.
Line range hint
2-7: Verify the necessity of compile-time feature checks.The file includes compile-time feature checks for
mocks,network-testing, andoffline-testing. While these are unrelated to the current changes, it might be worth verifying if these checks are still necessary and up-to-date.✅ Verification successful
Compile-time feature checks are necessary and up-to-date.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the feature flags mentioned in the compile-time checks are still used in the project. # Test 1: Check for the usage of 'mocks' feature echo "Checking usage of 'mocks' feature:" rg --type rust 'feature\s*=\s*"mocks"' -C 3 # Test 2: Check for the usage of 'network-testing' feature echo "Checking usage of 'network-testing' feature:" rg --type rust 'feature\s*=\s*"network-testing"' -C 3 # Test 3: Check for the usage of 'offline-testing' feature echo "Checking usage of 'offline-testing' feature:" rg --type rust 'feature\s*=\s*"offline-testing"' -C 3 # Expected results: The features should be used in other parts of the project. # If any of these searches return no results, it might indicate that the feature is no longer used and the check could potentially be removed.Length of output: 71859
packages/rs-sdk/tests/fetch/evonode.rs (2)
1-7: LGTM: Imports and file-level comment are appropriate.The file-level comment clearly describes the purpose of the tests, and the imports are relevant to the functionality being tested. Good job on keeping the imports concise and focused.
1-46: Overall, well-structured tests with room for enhancement.The
evonode.rsfile contains two well-designed test functions that cover both positive and negative scenarios for Evo node status functionality. This approach to testing is commendable.The suggested improvements for both
test_evonode_statusandtest_evonode_status_refusedfunctions will significantly enhance the robustness, reliability, and informativeness of these tests. Implementing these changes will:
- Improve error handling and prevent premature test termination.
- Add more specific assertions to validate the correctness of fetched data.
- Introduce timeouts to prevent tests from hanging indefinitely.
- Provide more detailed error checking for negative test cases.
These enhancements will lead to more dependable and maintainable tests, which are crucial for ensuring the reliability of the Evo node status functionality in the SDK.
packages/rs-sdk/src/platform/types/evonode.rs (3)
1-32: LGTM: Imports and documentation are well-structured.The imports are appropriate for the functionality being implemented. The module-level documentation clearly explains the purpose of
EvoNode, and the provided example demonstrates its usage effectively.
33-42: LGTM:EvoNodestruct and implementation are well-defined.The
EvoNodestruct is appropriately defined as a tuple struct containing anAddress. The derive macros forDebugandClone, along with the conditional derive forSerializeandDeserialize, are suitable for the struct's purpose. Thenewmethod provides a clear way to create anEvoNodeinstance, addressing the issue raised in the past review comment.
44-53: LGTM:Mockableimplementation forEvoNodeis correct.The
Mockableimplementation, conditional on the "mocks" feature, correctly implements themock_deserializemethod usingbincode. The error handling is appropriate, returningNoneif deserialization fails.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-proof-verifier/src/types.rs (3 hunks)
- packages/rs-sdk/tests/fetch/evonode.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/tests/fetch/evonode.rs
🔇 Additional comments (2)
packages/rs-drive-proof-verifier/src/types.rs (2)
620-632: LGTM: Well-definedEvonodeStatusstructThe
EvonodeStatusstruct is well-defined with appropriate fields, derives, and documentation. The use of[u8; ProTxHash::LEN]forpro_tx_hashensures type safety and efficiency.
634-668: LGTM: RobustTryFrom<GetStatusResponse>implementationThe implementation is thorough, with good error handling and data extraction. The validation of
pro_tx_hashlength is a nice security touch.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| /// Status of a network node. | ||
| /// | ||
| /// Some fields may be `None` if the information is not available, eg. when the node is not fully functional. | ||
| pub struct EvonodeStatus { |
There was a problem hiding this comment.
Let's have it nested as proto otherwise it's too messy
| // We need to delegate FromUnproved for the impl FetchUnproved for EvonodeStatus. | ||
| #[async_trait::async_trait] | ||
| impl FromUnproved<EvoNode> for EvonodeStatus { | ||
| type Request = EvoNode; |
| "get_status" | ||
| } | ||
|
|
||
| fn execute_transport<'c>( |
There was a problem hiding this comment.
We create client every request?
There was a problem hiding this comment.
Yes.
This query (getStatus) goes to exactly the node that is specified by EvoNode.0.
It means that:
- this node doesn't have to be configured as one of Sdk endpoints,
- this request will most likely go to each target only once, so caching clients will not add much value.
To ensure this is not an issue, I created 1 million clients in a loop like:
let start = time::Instant::now();
for i in 0..1_000_000 {
Self::Client::with_uri_and_settings(uri.clone(), settings, &pool);
}
let elapsed = start.elapsed();
tracing::info!("1 mln client creations took: {:?}", elapsed);
Result:
INFO dash_sdk::platform::types::evonode: 1 mln client creations took: 1.718250235s
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/rs-sdk/src/platform/fetch_unproved.rs (3)
17-17: LGTM: Improved flexibility in theFetchUnprovedtrait.The changes to the
FetchUnprovedtrait enhance its flexibility by introducing a generic query parameter and providing a default implementation. The use ofOption<Self>in the return type is a good practice for handling cases where no object is found.Consider adding a brief comment explaining the purpose of the
Q: Querygeneric parameter for better documentation.Also applies to: 33-46
61-84: LGTM: Improvedfetch_unproved_with_settingsmethod.The updates to the
fetch_unproved_with_settingsmethod are well-implemented and consistent with the changes to thefetch_unprovedmethod. The default implementation is clear and concise, providing a good template for specific implementations.Consider adding error handling for the case where
maybe_from_unproved_with_metadatareturnsNone. This could involve mapping theNonecase to an appropriate error type.
95-117: LGTM: Well-implementedFromUnprovedforEvoNodeStatus.The new implementation of
FromUnproved<EvoNode>forEvoNodeStatusis well-structured and uses an effective delegation pattern. This approach promotes code reuse and maintains consistency with existing implementations.Consider adding a brief comment explaining why
EvoNodeis used as theRequesttype here, as it might not be immediately obvious to readers unfamiliar with the codebase.packages/rs-drive-proof-verifier/src/types/evonode_status.rs (2)
174-174: Improve clarity of documentation commentConsider rephrasing the comment for better readability:
-/// Identifier of chain the node is member of. +/// Identifier of the chain the node is a member of.
207-304: Consider adding unit tests for theTryFrom<GetStatusResponse>implementationTo ensure the correctness of the conversion from
GetStatusResponsetoEvoNodeStatus, consider adding unit tests that cover various scenarios, including cases with missing orNonefields and malformed data. This will help validate the robustness of the parsing logic.Would you like me to help generate unit tests for this implementation or open a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/rs-drive-proof-verifier/src/types.rs (4 hunks)
- packages/rs-drive-proof-verifier/src/types/evonode_status.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (2 hunks)
- packages/rs-sdk/src/mock/requests.rs (2 hunks)
- packages/rs-sdk/src/platform/fetch_unproved.rs (2 hunks)
- packages/rs-sdk/src/platform/types/evonode.rs (1 hunks)
- packages/rs-sdk/src/sdk.rs (1 hunks)
- packages/rs-sdk/tests/fetch/evonode.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/rs-drive-proof-verifier/src/types.rs
- packages/rs-drive-proof-verifier/src/unproved.rs
- packages/rs-sdk/src/mock/requests.rs
- packages/rs-sdk/src/platform/types/evonode.rs
- packages/rs-sdk/src/sdk.rs
- packages/rs-sdk/tests/fetch/evonode.rs
🔇 Additional comments (3)
packages/rs-sdk/src/platform/fetch_unproved.rs (2)
1-1: LGTM: New imports are consistent with the changes.The added imports support the new functionality in the
FetchUnprovedtrait and its implementations. They are correctly placed and organized.Also applies to: 3-3, 5-9
87-93: LGTM: New implementations forFetchUnprovedtrait.The new implementations for
CurrentQuorumsInfoandEvoNodeStatusare correctly defined, specifying the requiredRequestassociated type. These minimal implementations leverage the default methods provided by the trait.packages/rs-drive-proof-verifier/src/types/evonode_status.rs (1)
236-242: Verify that default values are appropriate whenv0.nodeis missingIn the
TryFromimplementation, ifv0.nodeisNone, theNodestruct defaults to itsDefaultimplementation usingunwrap_or_default(). Please verify whether defaulting to emptyNodeinformation is acceptable, or if it should result in an error to prevent unintended behavior. This consideration also applies to other fields likechain,network,state_sync, andtime.To confirm whether using default values is appropriate, check how
EvoNodeStatusand its fields are utilized elsewhere in the codebase:This script will help identify whether the code assumes the presence of certain data in these fields.
Issue being fixed or feature implemented
We want to be able to display status of individual nodes in TUI.
What was done?
EvoNodestruct)Sdk::address_list()andimpl IntoIterator for AddressListto expose current address list to the users.How Has This Been Tested?
Added unit tests.
Integrated into TUI in dashpay/platform-tui#85
Breaking Changes
Changed FromUnproved and FetchUnproved traits (not yet released, so we don't consider it a breaking change).
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
AddressListwith cloning and iteration capabilities.DapiClient.getStatusrequests.nodeandstatusadded to enhance modularity.GetStatusRequestintoEvonodeStatus.EvoNodestruct for querying the status of a network node.evonodeto organize related functionalities.FetchUnprovedtrait for improved querying and metadata handling.EvonodeStatusin theFetchtrait.Bug Fixes
Documentation
Chores
EvonodeStatusin mock requests.