fix(sdk): small sdk improvements and fixes for v1.4 #2200
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the 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: 1
🧹 Outside diff range and nitpick comments (8)
packages/withdrawals-contract/src/lib.rs (2)
48-59: LGTM: Implementation of fmt::Display for WithdrawalStatus.The implementation is correct and improves the usability of the
WithdrawalStatusenum by providing human-readable string representations. All enum variants are covered, ensuring exhaustiveness.For consistency with the enum variant names, consider using uppercase strings in the
matcharms:impl fmt::Display for WithdrawalStatus { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let status_str = match self { - WithdrawalStatus::QUEUED => "Queued", - WithdrawalStatus::POOLED => "Pooled", - WithdrawalStatus::BROADCASTED => "Broadcasted", - WithdrawalStatus::COMPLETE => "Complete", - WithdrawalStatus::EXPIRED => "Expired", + WithdrawalStatus::QUEUED => "QUEUED", + WithdrawalStatus::POOLED => "POOLED", + WithdrawalStatus::BROADCASTED => "BROADCASTED", + WithdrawalStatus::COMPLETE => "COMPLETE", + WithdrawalStatus::EXPIRED => "EXPIRED", }; write!(f, "{}", status_str) } }This change would maintain consistency with the enum variant names and potentially make it easier to match string representations back to enum variants if needed.
7-7: Summary: Improved WithdrawalStatus enum usability.The changes in this file are minimal and focused, aligning well with the PR objectives of making small SDK improvements. The implementation of
fmt::DisplayforWithdrawalStatusenhances the usability of the enum by providing human-readable string representations. This change does not introduce any breaking changes and should improve the developer experience when working with withdrawal statuses.Consider adding unit tests for the new
fmt::Displayimplementation to ensure correct functionality and prevent regressions in future updates.Also applies to: 48-59
packages/rs-dpp/src/core_types/validator_set/mod.rs (1)
38-44: LGTM! Consider future-proofing the implementation.The
Displayimplementation forValidatorSetis correct and follows Rust's idiomatic patterns. It properly delegates the formatting to the containedValidatorSetV0instance.For future-proofing, consider adding a catch-all arm to the match expression. This will ensure that the code continues to compile if new variants are added to the
ValidatorSetenum in the future. Here's a suggested improvement:impl Display for ValidatorSet { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { ValidatorSet::V0(v0) => write!(f, "{}", v0), _ => write!(f, "Unknown ValidatorSet variant"), } } }This change will make the code more resilient to future updates of the
ValidatorSetenum.packages/rs-sdk/src/mock/requests.rs (1)
170-191: LGTM! Consider using the globalBINCODE_CONFIGfor consistency.The implementation of
MockResponseforElementlooks good and follows the pattern established for other types in this file. It correctly implements bothmock_serializeandmock_deserializemethods usingbincode.For consistency with other implementations in this file, consider using the global
BINCODE_CONFIGinstead of creating a new standard configuration in each method. Here's a suggested modification:impl MockResponse for Element { fn mock_serialize(&self, _sdk: &MockDashPlatformSdk) -> Vec<u8> { - // Create a bincode configuration - let config = standard(); - - // Serialize using the specified configuration - bincode::encode_to_vec(self, config).expect("Failed to serialize Element") + bincode::encode_to_vec(self, BINCODE_CONFIG).expect("Failed to serialize Element") } fn mock_deserialize(_sdk: &MockDashPlatformSdk, buf: &[u8]) -> Self where Self: Sized, { - // Create a bincode configuration - let config = standard(); - - // Deserialize using the specified configuration - bincode::decode_from_slice(buf, config) + bincode::decode_from_slice(buf, BINCODE_CONFIG) .expect("Failed to deserialize Element") .0 } }This change would make the implementation more consistent with other
MockResponseimplementations in the file and reduce redundancy.packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (1)
40-67: LGTM: Well-implemented Display trait with a minor suggestion.The
Displayimplementation forValidatorSetV0is well-structured and provides a clear, human-readable representation of the struct. It correctly handles all fields, including the optionalquorum_index.For consistency, consider using the
to_string()method forquorum_hashandthreshold_public_keyinstead ofhex::encode(), similar to how it's done in theDebugimplementation. This would make the code more uniform across different trait implementations. For example:write!( f, "ValidatorSet {{ quorum_hash: {}, // ... other fields ... threshold_public_key: {} }}", self.quorum_hash.to_string(), // ... other fields ... self.threshold_public_key.to_string() )This change would maintain consistency with the
Debugimplementation while still providing a readable output.packages/rs-sdk/src/platform/query.rs (1)
626-642: Implementation looks good, with a minor suggestion for improvement.The new implementation of
QueryforNoParamQueryis correct and follows the established patterns in the file. However, consider replacing theunimplemented!macro with a properErrorreturn for better error handling whenproveis true.Consider replacing the
unimplemented!macro with anErrorreturn:fn query(self, prove: bool) -> Result<GetCurrentQuorumsInfoRequest, Error> { if prove { - unimplemented!( - "query with proof are not supported yet for GetCurrentQuorumsInfoRequest" - ); + return Err(Error::Generic("Query with proof is not supported yet for GetCurrentQuorumsInfoRequest".into())); } let request: GetCurrentQuorumsInfoRequest = GetCurrentQuorumsInfoRequest { version: Some(get_current_quorums_info_request::Version::V0( GetCurrentQuorumsInfoRequestV0 {}, )), }; Ok(request) }This change would provide a more graceful error handling approach and avoid potential panics in production code.
packages/rs-sdk/src/platform/fetch_many.rs (2)
19-20: Missing documentation for the new request typeGetPathElementsRequest.The newly added
GetPathElementsRequestin the import statements lacks accompanying documentation. Providing documentation will enhance code readability and help other developers understand its purpose.
425-431: Add documentation for the newFetchManyimplementation forElement.The implementation of
FetchMany<Key, Elements> for Elementlacks documentation. To maintain consistency with other trait implementations and assist developers in understanding how to use this functionality, please add comprehensive documentation, including the purpose, usage examples, and supported query types.Here's a suggested addition:
/// Fetch multiple elements from Platform. /// /// Returns [Elements](drive_proof_verifier::types::Elements) indexed by their [Key](drive::grovedb::query_result_type::Key). /// /// ## Supported query types /// /// * [KeysInPath] - Specifies the path and keys from which to fetch elements. /// /// ## Example /// /// ```rust /// use dash_sdk::platform::{FetchMany, KeysInPath}; /// use dash_sdk::Sdk; /// use drive::grovedb::Element; /// /// #[tokio::main] /// async fn main() -> Result<(), dash_sdk::error::Error> { /// let sdk = Sdk::new(); /// let path = vec!["some", "path"]; /// let keys = vec![b"key1".to_vec(), b"key2".to_vec()]; /// let query = KeysInPath::new(path, keys); /// let elements = Element::fetch_many(&sdk, query).await?; /// Ok(()) /// } /// ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- packages/rs-dpp/src/core_types/validator/v0/mod.rs (1 hunks)
- packages/rs-dpp/src/core_types/validator_set/mod.rs (2 hunks)
- packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (2 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (1 hunks)
- packages/rs-sdk/src/mock/requests.rs (3 hunks)
- packages/rs-sdk/src/platform/fetch_many.rs (3 hunks)
- packages/rs-sdk/src/platform/query.rs (2 hunks)
- packages/withdrawals-contract/src/lib.rs (2 hunks)
🔇 Additional comments (10)
packages/withdrawals-contract/src/lib.rs (1)
7-7: LGTM: Import statement for fmt module.The import statement for
std::fmtis correctly added and necessary for implementing thefmt::Displaytrait.packages/rs-dpp/src/core_types/validator_set/mod.rs (1)
Line range hint
1-45: Summary: Display trait implementation enhances ValidatorSet usabilityThe addition of the
Displaytrait implementation forValidatorSetis a positive change that improves the enum's usability by providing a string representation. This implementation is consistent with the existing code structure and doesn't introduce any breaking changes or affect the existing functionality.The change aligns well with the PR objectives of implementing minor improvements to the SDK. It enhances the developer experience by allowing easier debugging and logging of
ValidatorSetinstances.packages/rs-dpp/src/core_types/validator/v0/mod.rs (3)
2-3: LGTM: Import statements reorganized.The changes to the import statements improve the organization and flexibility of the code. Importing the whole
fmtmodule and adding theDisplaytrait to the specific imports is a good practice that allows for easier expansion of formatting capabilities in the future.
Line range hint
117-127: LGTM: Improved Debug implementation for ValidatorV0.The changes to the Debug implementation for ValidatorV0 enhance the readability of the debug output. Using
to_string()for thepro_tx_hashfield provides a more human-readable representation. The implementation follows Rust's best practices for the Debug trait.
Line range hint
1-227: Overall assessment: Changes improve code quality.The modifications in this file, including the reorganization of import statements and the enhancement of the Debug implementation for ValidatorV0, contribute to better code organization and improved debug output readability. These changes align well with the PR objectives of implementing small SDK improvements.
packages/rs-dpp/src/core_types/validator_set/v0/mod.rs (3)
13-13: LGTM: Necessary import for new functionality.The addition of
use itertools::Itertools;is appropriate for the newDisplayimplementation that uses thejoinmethod from this crate.
17-18: LGTM: Appropriate imports for Display trait implementation.The addition of
use std::fmt;and the inclusion ofDisplayin the existing fmt import are correct and necessary for implementing theDisplaytrait.
Line range hint
1-268: Summary: Good improvements with minor suggestions.The changes to this file, primarily the addition of the
Displaytrait implementation forValidatorSetV0, are well-implemented and enhance the struct's usability. The new implementation provides a clear, human-readable representation of the struct's data.The necessary imports have been correctly added, and the code is generally well-structured. A minor suggestion was made for consistency in string conversion methods between the
DisplayandDebugimplementations.Overall, these changes represent a positive improvement to the codebase.
packages/rs-sdk/src/platform/query.rs (1)
11-11: LGTM: New import added correctly.The new import for
GetCurrentQuorumsInfoRequestV0is correctly placed and consistent with the existing code style.packages/rs-sdk/src/platform/fetch_many.rs (1)
34-35: Verify the necessity of importingPath.The import of
Pathfromdrive::grovedb::query_result_typeis added, but there doesn't appear to be any usage ofPathwithin this file. Please verify ifPathis required, and remove it if it's unused to keep the codebase clean.Run the following script to check for usages of
Path:✅ Verification successful
Remove Unused
PathImportThe
Pathimport inpackages/rs-sdk/src/platform/fetch_many.rsis not used and can be safely removed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if `Path` is used in the codebase. # Search for `Path` usage in Rust files within the `packages/rs-sdk` directory. rg --type rust '\bPath\b' packages/rs-sdk/Length of output: 382
| @@ -231,7 +231,7 @@ impl FromUnproved<platform::GetCurrentQuorumsInfoRequest> for CurrentQuorumsInfo | |||
| pro_tx_hash, | |||
| public_key: None, // Assuming it's not provided here | |||
| node_ip: member.node_ip, | |||
| node_id: PubkeyHash::from_slice(&[0; 32]).unwrap(), // Placeholder, since not provided | |||
| node_id: PubkeyHash::from_slice(&[0; 20]).expect("expected to make pub key hash from 20 byte empty array"), // Placeholder, since not provided | |||
There was a problem hiding this comment.
Handle missing node_id appropriately
Assigning a placeholder node_id with a zeroed array may lead to incorrect behavior or security issues. Instead of using a placeholder, consider implementing error handling or default logic to manage cases where node_id is not provided. This ensures the system remains robust and secure when encountering incomplete data.
Would you like assistance in implementing proper error handling for the missing node_id?
There was a problem hiding this comment.
No this is what I want to do.
There was a problem hiding this comment.
@QuantumExplorer, understood. If you have any further questions or need assistance in the future, feel free to let me know.
(ノ_・。)
✏️ Learnings added
Learnt from: QuantumExplorer
PR: dashpay/platform#2200
File: packages/rs-drive-proof-verifier/src/unproved.rs:234-234
Timestamp: 2024-10-03T05:17:52.320Z
Learning: When `node_id` is not provided, assigning a placeholder `node_id` with a zeroed array is acceptable.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Issue being fixed or feature implemented
This PR introduces the implementation of Display traits for several types and improves mock serialization and deserialization. Additionally, it addresses a panic issue related to the PubkeyHash length mismatch and improves support for fetching multiple elements and queries.
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
ValidatorV0andValidatorSetV0with improved string formatting.GetPathElementsRequestto support fetching multiple elements in theFetchManytrait.Displayimplementations forWithdrawalStatus, providing user-friendly string representations.Bug Fixes
maybe_from_unproved_with_metadatamethod forCurrentQuorumsInfo.Documentation