fix(sdk)!: preserve order of objects returned by drive #2207
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested reviewers
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
packages/rs-sdk/tests/fetch/mock_fetch_many.rs (1)
34-34: LGTM: ConsistentDocumentsusage.The replacement of
BTreeMapwithDocumentsis consistent with the previous change and aligns with the PR objective.For consistency, consider refactoring both
Documents::fromcalls into a helper function:fn create_documents(doc: Document) -> Documents { Documents::from([(doc.id(), Some(doc))]) } // Usage: let expected = create_documents(expected_doc.clone()); let not_expected = create_documents(not_expected_doc);This would reduce duplication and make future changes easier to manage.
packages/rs-drive-proof-verifier/src/provider.rs (1)
Line range hint
131-235: LGTM: Comprehensive MockContextProvider implementation with a minor suggestion.The
MockContextProvideris well-implemented and provides useful functionality for testing:
- It allows reading quorum keys and data contracts from files.
- Proper error handling is implemented for file operations and configuration issues.
- The
get_platform_activation_heightmethod returns a hardcoded value for the Regtest network.Consider adding a configurable platform activation height instead of hardcoding it. This would make the mock provider more flexible for different testing scenarios. For example:
pub struct MockContextProvider { quorum_keys_dir: Option<std::path::PathBuf>, platform_activation_height: CoreBlockHeight, } impl MockContextProvider { // ... existing methods ... pub fn set_platform_activation_height(&mut self, height: CoreBlockHeight) { self.platform_activation_height = height; } } impl ContextProvider for MockContextProvider { // ... existing methods ... fn get_platform_activation_height(&self) -> Result<CoreBlockHeight, ContextProviderError> { Ok(self.platform_activation_height) } }This change would allow testers to set custom activation heights for different test cases.
packages/rs-sdk/src/mock/sdk.rs (1)
333-349: LGTM: Improved type safety and constraintsThe changes to the
expect_fetch_manymethod signature enhance type safety and clarity:
- The generic parameter
Ris now more explicitly defined with necessary trait bounds.- The addition of
FromProof,MockResponse,Sync,Send, andDefaulttrait bounds ensures thatRhas all required capabilities.These changes make the method more robust and less prone to runtime errors due to missing implementations.
Consider adding a brief comment explaining the purpose of each trait bound for
R, especiallyFromProof, to improve code maintainability.packages/rs-drive-proof-verifier/src/types.rs (1)
34-35: LGTM! Consider adding documentation for IndexMap usage.The introduction of
IndexMapaligns with the PR objective of preserving the order of objects returned by drive. This change allows other modules to use this data structure directly.Consider adding a brief comment explaining why
IndexMapis being used and its benefits overBTreeMapin this context. This will help future developers understand the rationale behind this change.packages/rs-sdk/tests/fetch/contested_resource_vote_state.rs (1)
279-279: Update test case description for consistencyThe test case description still references
"limit std::u16::MAX"while the code has been updated to useu16::MAX. For consistency and clarity, consider updating the description to"limit u16::MAX".Apply this diff to update the test case description:
#[test_case(|q| q.limit = Some(u16::MAX), Err("limit 65535 out of bounds of [1, 100]"); "limit std::u16::MAX")] +#[test_case(|q| q.limit = Some(u16::MAX), Err("limit 65535 out of bounds of [1, 100]"); "limit u16::MAX")]packages/rs-sdk/tests/fetch/contested_resource.rs (2)
Line range hint
377-377: Correct the spelling of 'prerequisites'The function
check_mn_voting_prerequisitiescontains a typographical error in its name. The correct spelling isprerequisites.Apply this diff to correct the function name:
-pub async fn check_mn_voting_prerequisities(cfg: &Config) -> Result<(), Vec<String>> { +pub async fn check_mn_voting_prerequisites(cfg: &Config) -> Result<(), Vec<String>> {Remember to update all references to this function accordingly.
Line range hint
44-44: Update references to the renamed functionAfter renaming
check_mn_voting_prerequisitiestocheck_mn_voting_prerequisites, ensure all calls to this function are updated to match the new name.Apply this diff to update function calls:
- check_mn_voting_prerequisities(&cfg) + check_mn_voting_prerequisites(&cfg)Please apply this change wherever the function is called.
Also applies to: 61-61, 383-383
📜 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 (15)
- packages/rs-drive-proof-verifier/Cargo.toml (1 hunks)
- packages/rs-drive-proof-verifier/src/proof.rs (7 hunks)
- packages/rs-drive-proof-verifier/src/provider.rs (1 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (2 hunks)
- packages/rs-sdk/src/mock/requests.rs (4 hunks)
- packages/rs-sdk/src/mock/sdk.rs (2 hunks)
- packages/rs-sdk/src/platform/block_info_from_metadata.rs (1 hunks)
- packages/rs-sdk/src/platform/fetch_many.rs (2 hunks)
- packages/rs-sdk/tests/fetch/config.rs (1 hunks)
- packages/rs-sdk/tests/fetch/contested_resource.rs (1 hunks)
- packages/rs-sdk/tests/fetch/contested_resource_vote_state.rs (1 hunks)
- packages/rs-sdk/tests/fetch/document.rs (1 hunks)
- packages/rs-sdk/tests/fetch/epoch.rs (3 hunks)
- packages/rs-sdk/tests/fetch/mock_fetch.rs (1 hunks)
- packages/rs-sdk/tests/fetch/mock_fetch_many.rs (2 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/rs-sdk/src/platform/block_info_from_metadata.rs
- packages/rs-sdk/tests/fetch/config.rs
- packages/rs-sdk/tests/fetch/mock_fetch.rs
🔇 Additional comments (25)
packages/rs-sdk/tests/fetch/mock_fetch_many.rs (3)
15-15: LGTM: New import aligns with PR objectives.The addition of
Documentsfromdrive_proof_verifier::typesis consistent with the PR's goal of changing data structures to preserve object order.
Line range hint
1-62: Summary: Changes successfully implement PR objectives.The modifications in this file successfully replace
BTreeMapwithDocuments, which should preserve the order of objects returned by the drive. The test logic remains intact, ensuring that the new implementation is properly tested. These changes align well with the PR objectives.A few suggestions for improvement:
- Consider adding a comment explaining why
Documentsis used instead ofBTreeMapfor future maintainers.- Update the test documentation to reflect the importance of order preservation, if applicable.
- If not done already, ensure that similar changes are applied consistently across the codebase.
Overall, the changes look good and achieve the intended goal.
28-28: LGTM:Documentsusage preserves object order.The replacement of
BTreeMapwithDocumentsaligns with the PR objective. This change should preserve the order of objects returned by the drive.Let's verify that
Documentsis used consistently throughout the codebase:✅ Verification successful
To ensure that the usage of
Documents::fromis consistent throughout the codebase and aligns with the changes made to preserve object order, we need to search for other occurrences. Let's run a script to find all instances ofDocuments::from.
To ensure we have a complete picture of how the Documents struct is used throughout the codebase, let's search for its general usage. This will help us confirm that all interactions with the Documents struct are consistent with the changes made to preserve object order.
Based on the comprehensive analysis of the codebase, I can confirm that the changes to use
Documents::from()instead ofBTreeMaphave been implemented consistently and correctly. The usage of theDocumentsstruct aligns with the goal of preserving object order, and there are no instances ofBTreeMapbeing used directly with document-related operations.LGTM: The changes to use
Documents::from()have been applied consistently throughout the codebase.The implementation preserves object order as intended, and the usage is consistent across various parts of the project, including tests, query handling, and core functionality. This change should effectively address the issue of preserving object order in document-related operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of Documents instead of BTreeMap # Test 1: Search for remaining BTreeMap usages echo "Searching for remaining BTreeMap usages:" rg --type rust 'BTreeMap<' -C 3 # Test 2: Verify Documents usage echo "Verifying Documents usage:" rg --type rust 'Documents::from' -C 3Length of output: 418190
Script:
#!/bin/bash # Search for all occurrences of Documents::from in the codebase echo "Searching for Documents::from usage:" rg --type rust "Documents::from" -C 3Length of output: 1311
Script:
#!/bin/bash # Search for all occurrences of the Documents struct in the codebase echo "Searching for Documents struct usage:" rg --type rust "\bDocuments\b" -C 3Length of output: 96068
packages/rs-sdk/tests/fetch/epoch.rs (4)
16-16: LGTM: Import statement for ExtendedEpochInfos added.The new import statement for
ExtendedEpochInfosis correctly added and aligns with the PR objective of changing the data structure for returning objects.
91-93: LGTM: test_epoch_list function updated to use ExtendedEpochInfos.The
test_epoch_listfunction has been correctly updated to use the newExtendedEpochInfostype. Thefetch_manymethod is now used directly onExtendedEpochInfo, which is consistent with the PR objective of changing the data structure for returning objects.
Line range hint
1-180: Verify changes in test_epoch_list_limit function.The AI summary mentions changes to the
test_epoch_list_limitfunction, specifically updating theepochsvariable type and thefetch_manycall. However, these changes are not visible in the provided code snippet. Please verify if these changes have been implemented correctly.Run the following script to check the implementation of
test_epoch_list_limit:#!/bin/bash # Description: Verify changes in test_epoch_list_limit function # Test: Check the implementation of test_epoch_list_limit rg --type rust -A 20 'async fn test_epoch_list_limit' packages/rs-sdk/tests/fetch/epoch.rs
37-37: LGTM: Function signature updated to use ExtendedEpochInfos.The
assert_epochsfunction signature has been correctly updated to use the newExtendedEpochInfostype, which is consistent with the PR objective.Please ensure that the function body has been adjusted to work correctly with the new
ExtendedEpochInfostype. Run the following script to verify the usage ofExtendedEpochInfoswithin theassert_epochsfunction:packages/rs-sdk/tests/fetch/document.rs (1)
Line range hint
214-218: Query update addresses PLAN-653, but test remains ignored.The addition of the
with_whereclause appears to address the bug PLAN-653 related to processing aValue::Text("l")string. However, the test is still marked as ignored.Could you please clarify:
- Is this change expected to resolve PLAN-653 completely?
- If so, can we remove the
#[ignore]attribute and enable the test?- If not, what additional steps are needed to fully resolve the issue?
To verify the current state of this issue, please run:
#!/bin/bash # Description: Check for other occurrences of PLAN-653 in the codebase # Expect: Information about the current state of the PLAN-653 issue rg --type rust 'PLAN-653' -C 3packages/rs-drive-proof-verifier/src/provider.rs (4)
50-50: LGTM: Improved documentation clarity.The added line in the documentation for
get_data_contractmethod provides valuable information about the use ofArc. This improvement helps developers understand the rationale behind the return type.
Line range hint
87-107: LGTM: Improved thread safety in Mutex implementation.The changes in the
ContextProviderimplementation forstd::sync::Mutex<T>correctly address thread safety concerns:
- Each method now acquires a lock before delegating to the inner
ContextProvider.- The use of
expect("lock poisoned")is appropriate for handling potential poisoned locks.- The implementation correctly delegates to the inner methods after acquiring the lock.
These changes ensure that the
Mutex<T>wrapper properly provides thread-safe access to the underlyingContextProvider.
Line range hint
110-129: LGTM: Well-designed DataContractProvider trait and implementation.The addition of the
DataContractProvidertrait and its implementation for types implementingContextProvideris a good design decision:
- It provides a clean interface for obtaining a
ContractLookupFn.- The implementation correctly wraps the
get_data_contractmethod into aContractLookupFn.- Error handling is properly implemented, mapping
ContextProviderErrortodrive::error::Error.This abstraction will make it easier to use context providers with Drive proof verification functions.
Line range hint
237-242: LGTM: Useful AsRef implementation for Arc.The addition of the
AsRefimplementation forArc<T>whereT: ContextProvideris a good improvement:
- It allows
Arc<T>to be treated as a reference toContextProvider.- The implementation correctly uses
deref()to access the innerContextProvider.- This addition improves the ergonomics of working with
Arc-wrapped context providers.This implementation will make it easier to use
Arc-wrapped context providers in various contexts.packages/rs-sdk/src/mock/sdk.rs (2)
307-308: LGTM: Improved documentation clarityThe updated documentation for the generic parameter
Onow explicitly states thatVec<O>must implementMockResponse. This clarification enhances the understanding of the method's requirements and is beneficial for developers using this API.
Line range hint
1-1: Documentation consistency improvement notedThe AI summary mentions that the documentation for the
expect_fetchmethod has been updated for clarity. While specific line numbers are not provided, maintaining consistent and clear documentation across related methods is a positive change.To confirm the changes to the
expect_fetchmethod documentation, please run the following command:packages/rs-sdk/src/platform/fetch_many.rs (3)
241-243: LGTM! Verify theDocumentstype usage.The change from
Option<BTreeMap<K,Document>>toDocumentsimproves type specificity and aligns with the mentioned changes in the AI-generated summary. This should provide a more appropriate representation of fetched documents.To ensure the
Documentstype is used consistently, run the following command:#!/bin/bash # Description: Check for usage of the Documents type in the codebase. rg --type rust -n 'Documents' packages/rs-sdk/src/
Line range hint
1-445: Overall changes look good and align with the PR objectives.The modifications in this file primarily focus on import adjustments and refining the
FetchManytrait implementation forDocument. These changes appear to be part of a broader effort to improve type specificity and remove unused imports. The switch fromBTreeMaptoDocumentsin theDocumentimplementation aligns with the PR objective of preserving the order of objects returned by the drive.While the changes seem to maintain overall functionality, it's important to ensure that:
- The new
Documentstype is used consistently across the codebase.- The removal of the
Pathimport and other import adjustments don't introduce any unintended side effects.- The order preservation mentioned in the PR objectives is indeed achieved with these changes.
To ensure the changes meet the PR objectives, particularly regarding order preservation, consider running integration tests that verify the order of returned objects:
34-34: LGTM! Consider verifying unused imports.The changes in the import statements look good. The removal of
Pathand the adjustments in thedrive_proof_verifier::typesimports suggest a cleanup or update in the types being used.To ensure all imports are necessary, run the following command:
Also applies to: 38-40
packages/rs-drive-proof-verifier/src/types.rs (1)
62-62: LGTM! Consider documenting performance implications.The change from
BTreeMaptoIndexMapforRetrievedObjectstype successfully addresses the PR objective of preserving the order of objects returned by drive.While this change achieves the desired functionality, consider the following:
Document the performance implications of using
IndexMapvsBTreeMapin the type's documentation. This will help users of this type understand any potential trade-offs.Ensure that all code relying on
RetrievedObjectsis updated to account for the change fromBTreeMaptoIndexMap, as some operations may have different time complexities.If order preservation is not always necessary, consider adding a separate type (e.g.,
UnorderedRetrievedObjects) that still usesBTreeMapfor scenarios where order doesn't matter and the performance characteristics ofBTreeMapare preferred.To ensure this change doesn't introduce any issues, let's verify the usage of
RetrievedObjectsacross the codebase:✅ Verification successful
Change verified and approved. The modification of the
RetrievedObjectstype fromBTreeMaptoIndexMapis correctly localized and does not affect other parts of the codebase. Ensure that performance implications are documented as previously suggested.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for RetrievedObjects usage echo "Searching for RetrievedObjects usage:" rg --type rust "RetrievedObjects" -C 3 # Search for potential BTreeMap to IndexMap conversions echo "\nSearching for potential BTreeMap to IndexMap conversions:" rg --type rust "BTreeMap<.+, Option<.+>>" -C 3Length of output: 42867
packages/rs-sdk/src/mock/requests.rs (2)
6-6: Appropriate aliasing ofHashasCoreHash.Aliasing
Hashfromdashcore::hashestoCoreHashhelps prevent conflicts withstd::hash::Hashand improves code clarity. Good job on making this change.
258-258: Verify the correctness ofCoreHashusage inProTxHashdeserialization.In the
mock_deserializemethod forProTxHash, you're creating aProTxHashusingCoreHash::from_byte_array(data). Ensure thatCoreHash::from_byte_arraycorrectly constructs the hash and that this change doesn't affect the integrity ofProTxHash.Consider running the following script to check for any potential issues:
This will help confirm that the method is used appropriately elsewhere and maintain consistency.
✅ Verification successful
Verified: The usage of
CoreHash::from_byte_arrayinProTxHashdeserialization is correct and maintains the integrity ofProTxHash. No inconsistent usages were found in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `CoreHash::from_byte_array` is used correctly and consistently. # Search for all usages of `CoreHash::from_byte_array` in the codebase. rg --type rust 'CoreHash::from_byte_array' -A 2 -B 2Length of output: 444
packages/rs-sdk/tests/fetch/contested_resource.rs (1)
281-281: Consider adjusting the limit in the test caseUsing
u16::MAXas the limit might be excessive and could impact test performance. If the intent is to test the maximum allowed value, ensure that the system can handle such large inputs efficiently.If you want to confirm whether using
u16::MAXaffects test performance, you can run the following script:Review the execution time and consider using a smaller limit if necessary.
✅ Verification successful
The use of
u16::MAXin the test case is consistent with other test scenarios across the codebase and does not impact performance.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Measure execution time of the test case with u16::MAX limit # Command: Run the specific test case and measure time time cargo test contested_resources_limit_PLAN_656 -- --nocaptureLength of output: 177
Script:
#!/bin/bash # Description: Search for test cases using u16::MAX to evaluate their usage and potential performance implications # Use ripgrep to find occurrences of u16::MAX in Rust test files rg "u16::MAX" --type rust --context 2 --prettyLength of output: 6699
packages/rs-drive-proof-verifier/src/proof.rs (4)
46-46: Approved: ImportingIndexMapThe addition of
use indexmap::IndexMap;is appropriate for maintaining the insertion order of collections, which aligns with the PR's objective to preserve the order of objects returned by the drive.
1206-1210: Approved: Conversion toElementsThe conversion using
Elements::from_iter(objects)is appropriate and takes advantage of theFromIteratortrait implementation forElements.
1884-1888: Approved: ImplementingLengthforIndexMapThe implementation of the
Lengthtrait forIndexMap<K, Option<T>>correctly counts the number of non-Noneelements, ensuring consistent behavior across different collection types.
990-996:⚠️ Potential issueVerify handling of multiple epochs
In the
maybe_from_proof_with_metadataimplementation forExtendedEpochInfo, the code checks if the length ofepochs.0is not equal to 1 and returns an error if so. If multiple epochs are not expected, this is appropriate. However, if there is a possibility of receiving multiple epochs, consider adjusting the logic to handle them correctly.Please verify whether the API guarantees that only one epoch should be returned. If multiple epochs can be valid, the code should be updated to handle multiple entries appropriately.
packages/rs-sdk/src/mock/requests.rs
Outdated
| Self: Sized, | ||
| { | ||
| let (data, _): (IndexMap<Vec<u8>, Vec<u8>>, _) = | ||
| bincode::serde::decode_from_slice(buf, BINCODE_CONFIG).expect("decode BTreeMap"); |
There was a problem hiding this comment.
Correct error messages to reflect IndexMap instead of BTreeMap.
In the mock_deserialize and mock_serialize methods for IndexMap<K, V>, the expect messages still reference "decode BTreeMap" and "encode BTreeMap". Updating these messages will improve clarity during debugging.
Here is the suggested fix:
fn mock_deserialize(sdk: &MockDashPlatformSdk, buf: &[u8]) -> Self
where
Self: Sized,
{
let (data, _): (IndexMap<Vec<u8>, Vec<u8>>, _) =
- bincode::serde::decode_from_slice(buf, BINCODE_CONFIG).expect("decode BTreeMap");
+ bincode::serde::decode_from_slice(buf, BINCODE_CONFIG).expect("decode IndexMap");
data.into_iter()
.map(|(k, v)| (K::mock_deserialize(sdk, &k), V::mock_deserialize(sdk, &v)))
.collect()
}
fn mock_serialize(&self, sdk: &MockDashPlatformSdk) -> Vec<u8> {
let data: IndexMap<Vec<u8>, Vec<u8>> = self
.iter()
.map(|(k, v)| (k.mock_serialize(sdk), v.mock_serialize(sdk)))
.collect();
- bincode::serde::encode_to_vec(data, BINCODE_CONFIG).expect("encode BTreeMap")
+ bincode::serde::encode_to_vec(data, BINCODE_CONFIG).expect("encode IndexMap")
}Also applies to: 137-137
There was a problem hiding this comment.
@lklimek Great, thanks for addressing the issue!
((
( ^.^)
o_(")(")
🧠 Learnings used
Learnt from: lklimek
PR: dashpay/platform#2207
File: packages/rs-sdk/src/mock/requests.rs:0-0
Timestamp: 2024-10-04T14:09:58.852Z
Learning: In Rust, when serializing `IndexMap<K, V>` with bincode, use `bincode::serde::encode_to_vec` and `bincode::serde::decode_from_slice` because `IndexMap` does not implement `bincode::Encode`.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
packages/rs-sdk/src/mock/requests.rs
Outdated
| impl<K: Hash + Eq + MockResponse, V: MockResponse> MockResponse for IndexMap<K, V> { | ||
| fn mock_deserialize(sdk: &MockDashPlatformSdk, buf: &[u8]) -> Self | ||
| where | ||
| Self: Sized, | ||
| { | ||
| let (data, _): (IndexMap<Vec<u8>, Vec<u8>>, _) = | ||
| bincode::serde::decode_from_slice(buf, BINCODE_CONFIG).expect("decode BTreeMap"); | ||
|
|
||
| data.into_iter() | ||
| .map(|(k, v)| (K::mock_deserialize(sdk, &k), V::mock_deserialize(sdk, &v))) | ||
| .collect() | ||
| } | ||
|
|
||
| fn mock_serialize(&self, sdk: &MockDashPlatformSdk) -> Vec<u8> { | ||
| let data: IndexMap<Vec<u8>, Vec<u8>> = self | ||
| .iter() | ||
| .map(|(k, v)| (k.mock_serialize(sdk), v.mock_serialize(sdk))) | ||
| .collect(); | ||
|
|
||
| bincode::serde::encode_to_vec(data, BINCODE_CONFIG).expect("encode BTreeMap") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize serialization methods for consistency.
In the implementation of MockResponse for IndexMap<K, V>, you're using bincode::serde::encode_to_vec and decode_from_slice, whereas for BTreeMap<K, V> and other types, you use bincode::encode_to_vec and decode_from_slice without the serde module. For consistency and maintainability, consider using the same serialization methods across similar implementations unless there's a specific reason to use the serde variants.
If possible, adjust the IndexMap implementation to use:
bincode::encode_to_vec(...)and
bincode::decode_from_slice(...)to match the other implementations.
There was a problem hiding this comment.
IndexMap does not implement bincode::Encode, so we cannot use non-serde functions there.
There was a problem hiding this comment.
@lklimek, thank you for the clarification. Since IndexMap does not implement bincode::Encode, it's necessary to use the bincode::serde serialization functions. That makes sense.
✏️ Learnings added
Learnt from: lklimek
PR: dashpay/platform#2207
File: packages/rs-sdk/src/mock/requests.rs:0-0
Timestamp: 2024-10-04T14:09:58.673Z
Learning: In Rust, when serializing `IndexMap<K, V>` with bincode, use `bincode::serde::encode_to_vec` and `bincode::serde::decode_from_slice` because `IndexMap` does not implement `bincode::Encode`.
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.
| let maybe_contracts: Option<DataContracts> = if !contracts.is_empty() { | ||
| let contracts: DataContracts = contracts | ||
| .into_iter() | ||
| .try_fold(DataContracts::new(), |mut acc, (k, v)| { | ||
| Identifier::from_bytes(&k).map(|id| { | ||
| acc.insert(id, v); | ||
| acc | ||
| }) | ||
| .map_err(|e| Error::ResultEncodingError { | ||
| error: e.to_string(), | ||
| })?; | ||
| }) | ||
| .map_err(|e| Error::ResultEncodingError { | ||
| error: e.to_string(), | ||
| })?; | ||
|
|
||
| Some(contracts) | ||
| } else { | ||
| None | ||
| }; | ||
| Some(contracts) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the construction of maybe_contracts
The current implementation of constructing maybe_contracts can be simplified for better readability and efficiency. Instead of using try_fold, consider using map and collect to handle the transformation and error propagation more succinctly.
Here's a possible refactoring:
-let maybe_contracts: Option<DataContracts> = if !contracts.is_empty() {
- let contracts: DataContracts = contracts
- .into_iter()
- .try_fold(DataContracts::new(), |mut acc, (k, v)| {
- Identifier::from_bytes(&k).map(|id| {
- acc.insert(id, v);
- acc
- })
- })
- .map_err(|e| Error::ResultEncodingError {
- error: e.to_string(),
- })?;
- Some(contracts)
-} else {
- None
-};
+let maybe_contracts = contracts
+ .into_iter()
+ .map(|(k, v)| {
+ Identifier::from_bytes(&k)
+ .map(|id| (id, v))
+ .map_err(|e| Error::ResultEncodingError {
+ error: e.to_string(),
+ })
+ })
+ .collect::<Result<DataContracts, Error>>()
+ .map(Some)?;This approach eliminates the need for manual emptiness checks and leverages collect to build the DataContracts directly while handling errors efficiently.
📝 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.
| let maybe_contracts: Option<DataContracts> = if !contracts.is_empty() { | |
| let contracts: DataContracts = contracts | |
| .into_iter() | |
| .try_fold(DataContracts::new(), |mut acc, (k, v)| { | |
| Identifier::from_bytes(&k).map(|id| { | |
| acc.insert(id, v); | |
| acc | |
| }) | |
| .map_err(|e| Error::ResultEncodingError { | |
| error: e.to_string(), | |
| })?; | |
| }) | |
| .map_err(|e| Error::ResultEncodingError { | |
| error: e.to_string(), | |
| })?; | |
| Some(contracts) | |
| } else { | |
| None | |
| }; | |
| Some(contracts) | |
| } else { | |
| None | |
| }; | |
| let maybe_contracts = contracts | |
| .into_iter() | |
| .map(|(k, v)| { | |
| Identifier::from_bytes(&k) | |
| .map(|id| (id, v)) | |
| .map_err(|e| Error::ResultEncodingError { | |
| error: e.to_string(), | |
| }) | |
| }) | |
| .collect::<Result<DataContracts, Error>>() | |
| .map(Some)?; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
packages/rs-sdk/tests/fetch/data_contract.rs (1)
148-153: Improved error handling and result access. Consider further refinement.The changes enhance the test's robustness and readability:
- Using
expectprovides more informative error messages.- Explicit checks for request success and data contract existence improve error handling.
- Using
into_iter().next()is more idiomatic thanpop_first().These improvements are welcome. However, we can further refine the code for better error handling and readability.
Consider restructuring the code to handle each potential failure case separately:
let result = DataContractHistory::fetch(&sdk, LimitQuery::from((id, 10))) .await .expect("request should succeed"); let contract = result .expect("data contract should exist") .into_iter() .next() .expect("at least one data contract should be returned"); assert_eq!(contract.id(), id);This structure provides clearer error messages for each potential failure point.
packages/rs-drive-proof-verifier/src/types.rs (1)
Line range hint
1-607: Major refactoring: BTreeMap replaced with IndexMap throughout the moduleThis change represents a significant refactoring of the
types.rsmodule, replacingBTreeMapwithIndexMapin multiple type definitions and implementations. This shift affects the ordering behavior, performance characteristics, and potentially the memory usage of various data structures in the module.To ensure a smooth transition and maintain system integrity, consider the following comprehensive strategy:
- Conduct a thorough impact analysis across the entire codebase to identify all affected areas.
- Update all dependent code to account for the new ordering behavior of IndexMap.
- Perform extensive performance testing, especially for large datasets, to ensure that the change doesn't negatively impact system performance.
- Update all relevant documentation, including inline comments, method descriptions, and any external documentation referring to these types.
- Review and update any serialization/deserialization logic that may be affected by this change.
- Consider creating a migration guide for users of your library to help them adapt to these changes.
- Update the crate's version number according to semantic versioning principles, as this is a breaking change.
- Add comprehensive notes about these changes to the changelog or release notes.
Given the scope of these changes, it may be beneficial to implement them in stages, possibly behind feature flags, to allow for easier testing and gradual adoption.
packages/rs-drive-proof-verifier/src/proof.rs (1)
835-845: Approved: Changed from BTreeMap to IndexMap for preserving insertion orderThe modification from
BTreeMaptoIndexMapformaybe_contractspreserves the insertion order of elements, which can be beneficial in certain scenarios. However, it's worth noting thatIndexMaphas slightly different performance characteristics compared toBTreeMap:
IndexMapgenerally offers faster iteration and faster average-case insertion and removal.BTreeMapprovides guaranteed O(log n) worst-case performance for insertions and removals, whichIndexMapdoesn't.Ensure that this change aligns with the performance requirements of your use case.
packages/rs-sdk/src/mock/requests.rs (1)
118-139: Add Comment Explaining Use ofserdefor SerializationSince
IndexMapdoes not implementbincode::Encode, you've correctly usedbincode::serde::encode_to_vecandbincode::serde::decode_from_slicefor serialization and deserialization. Adding a brief comment explaining this exception will enhance maintainability and assist future developers in understanding the rationale behind usingserdefunctions here.packages/rs-drive-proof-verifier/src/unproved.rs (4)
Line range hint
221-230: ReplaceBTreeMapwithIndexMapto Preserve Insertion OrderThe current implementation collects validators into a
BTreeMap, which sorts the keys and does not preserve the insertion order. Since the PR aims to preserve the order of objects returned by the drive, consider usingIndexMapinstead.IndexMapmaintains the insertion order of the elements.Apply this diff to replace
BTreeMapwithIndexMap:-use std::collections::BTreeMap; +use indexmap::IndexMap; ... -.collect::<Result<BTreeMap<ProTxHash, ValidatorV0>, Error>>()?; +.collect::<Result<IndexMap<ProTxHash, ValidatorV0>, Error>>()?;
Line range hint
226-230: Avoid Usingexpectto Prevent Potential PanicsIn the code,
expectis used when creatingnode_id:node_id: PubkeyHash::from_slice(&[0; 20]).expect("expected to make pub key hash from 20 byte empty array"), // Placeholder, since not providedUsing
expectcan cause the application to panic if the condition is not met. It's better to handle the potential error and return an appropriate error instead of panicking.Apply this diff to handle the error properly:
-node_id: PubkeyHash::from_slice(&[0; 20]).expect("expected to make pub key hash from 20 byte empty array"), // Placeholder, since not provided +node_id: PubkeyHash::from_slice(&[0; 20]).map_err(|_| Error::ProtocolError { + error: "Invalid node_id format".to_string(), +})?,
Line range hint
195-217: Refactor Repeated Length Checks into a Helper FunctionThere are multiple instances where you check the length of byte arrays and copy them if the length is correct. Examples include checking
current_quorum_hashandlast_block_proposer. To reduce code duplication and improve maintainability, consider creating a helper function for these operations.Here's an example of how you might implement the helper function:
fn validate_and_copy_hash(input: &[u8], field_name: &str) -> Result<[u8; 32], Error> { if input.len() != 32 { return Err(Error::ProtocolError { error: format!("Invalid {} length", field_name), }); } let mut output = [0u8; 32]; output.copy_from_slice(input); Ok(output) }You can then use it as follows:
let current_quorum_hash = validate_and_copy_hash(&v0.current_quorum_hash, "current_quorum_hash")?; let last_block_proposer = validate_and_copy_hash(&v0.last_block_proposer, "last_block_proposer")?;
Line range hint
279-281:#[async_trait]Attribute May Be UnnecessaryThe
#[async_trait]attribute is used when implementing asynchronous functions in traits or their implementations. Since theFromUnprovedtrait implementation forEvoNodeStatusdoes not contain any asynchronous functions, the#[async_trait]attribute may not be necessary and can be removed for clarity.
📜 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 (7)
- packages/rs-drive-proof-verifier/Cargo.toml (2 hunks)
- packages/rs-drive-proof-verifier/src/proof.rs (8 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (7 hunks)
- packages/rs-drive-proof-verifier/src/unproved.rs (1 hunks)
- packages/rs-sdk/src/mock/requests.rs (4 hunks)
- packages/rs-sdk/tests/fetch/contested_resource.rs (1 hunks)
- packages/rs-sdk/tests/fetch/data_contract.rs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/rs-drive-proof-verifier/Cargo.toml
- packages/rs-sdk/tests/fetch/contested_resource.rs
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-proof-verifier/src/proof.rs (1)
Learnt from: lklimek PR: dashpay/platform#2207 File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664 Timestamp: 2024-10-04T14:16:05.798Z Learning: In the implementation of `FromProof<platform::GetContestedResourceIdentityVotesRequest>` in `packages/rs-drive-proof-verifier/src/proof.rs`, when matching `maybe_votes`, using `.expect()` on `v.into_iter().next()` is acceptable because the prior match arm `Some(v) if v.is_empty()` ensures that the map is not empty, preventing a panic.
🔇 Additional comments (7)
packages/rs-drive-proof-verifier/src/proof.rs (5)
46-46: New import addedThe
IndexMaptype is now imported from theindexmapcrate. This suggests that the code will be usingIndexMapin place of other map types in some parts of the implementation.
900-904: Approved: Consistent use of IndexMap for DataContractHistoryThe change from
BTreeMaptoIndexMapforDataContractHistoryis consistent with the previous modification. This change preserves the insertion order of elements in the contract history, which can be important for maintaining the chronological order of contract versions.This modification aligns well with the earlier changes and provides a consistent approach to handling ordered data structures throughout the codebase.
Line range hint
987-997: Approved: Simplified logic and consistent use of IndexMapThe changes in this segment improve the code in two ways:
- Consistent use of
IndexMap: This aligns with previous modifications and maintains the order of elements.- Simplified logic: The new implementation provides a more straightforward way of handling the result, making the code easier to read and maintain.
The use of
into_iter().next()is safe here because the previous check ensures thateis not empty. This change reduces unnecessary allocations and improves code clarity.
1056-1056: Approved: Consistent use of IndexMap for ExtendedEpochInfosThe change from
BTreeMaptoIndexMapforExtendedEpochInfosis consistent with the previous modifications in the file. This change ensures that the order of epoch infos is preserved, which can be important for maintaining the chronological sequence of epochs.This modification aligns well with the earlier changes and provides a consistent approach to handling ordered data structures throughout the codebase.
1881-1885: Approved: Added Length trait implementation for IndexMapThe addition of the
Lengthtrait implementation forIndexMapis a necessary and valuable change. This implementation ensures thatIndexMapcan be used consistently with other collection types that implement theLengthtrait.The implementation correctly counts the number of non-None elements in the
IndexMap, which is consistent with the behavior of otherLengthimplementations in the codebase. This addition supports the broader changes made to useIndexMapthroughout the file.packages/rs-sdk/src/mock/requests.rs (2)
6-6: Clarify Import by RenamingHashtoCoreHashRenaming
HashtoCoreHashin the import statement improves code clarity by distinguishing it from otherHashtypes, thereby preventing potential naming conflicts.
258-258: Ensure Consistent Use ofCoreHashin DeserializationThe update to use
CoreHash::from_byte_array(data)inmock_deserializealigns with the renamed import and ensures the correct hash type is utilized during deserialization.
| @@ -73,12 +75,12 @@ pub type RetrievedObjects<K, O> = BTreeMap<K, Option<O>>; | |||
| /// | |||
| /// * `K`: The type of the keys in the map. | |||
| /// * `I`: The type of the integer in the map. | |||
| pub type RetrievedIntegerValue<K, I> = BTreeMap<K, I>; | |||
| pub type RetrievedIntegerValue<K, I> = IndexMap<K, I>; | |||
There was a problem hiding this comment.
Behavior change: RetrievedIntegerValue now uses IndexMap
The RetrievedIntegerValue type alias has been changed from BTreeMap to IndexMap. This change affects the ordering behavior of the stored integer values.
Actions required:
- Update all code that relies on the ordering of
RetrievedIntegerValueto account for the new insertion-order preservation. - Conduct thorough testing to ensure that this change doesn't introduce any bugs in the existing codebase.
- Update the documentation for
RetrievedIntegerValueto reflect the new ordering behavior.
|
|
||
| /// History of a data contract. | ||
| /// | ||
| /// Contains a map of data contract revisions to data contracts. | ||
| pub type DataContractHistory = BTreeMap<u64, DataContract>; | ||
| pub type DataContractHistory = IndexMap<u64, DataContract>; |
There was a problem hiding this comment.
Behavioral change: DataContractHistory now uses IndexMap
The DataContractHistory type alias has been changed from BTreeMap to IndexMap. This change affects how data contract history entries are ordered and accessed.
Consider the following:
- Evaluate the impact of this change on any code that relies on the ordering of data contract history entries.
- Assess the performance implications, especially for large histories, as IndexMap may have different memory usage and access patterns compared to BTreeMap.
- Update documentation to reflect the new behavior of
DataContractHistory. - Ensure that this change doesn't break any assumptions in the contract versioning logic.
| @@ -448,7 +450,7 @@ impl FromIterator<(u64, Vec<VotePoll>)> for VotePollsGroupedByTimestamp { | |||
| // collect all vote polls for the same timestamp into a single vector | |||
| let data = iter | |||
| .into_iter() | |||
| .fold(BTreeMap::new(), |mut acc, (timestamp, vote_poll)| { | |||
| .fold(IndexMap::new(), |mut acc, (timestamp, vote_poll)| { | |||
There was a problem hiding this comment.
Behavior change: VotePollsGroupedByTimestamp now uses IndexMap internally
The VotePollsGroupedByTimestamp::from_iter implementation now uses IndexMap instead of BTreeMap. This change affects the internal ordering of vote polls.
Consider the following:
- Review and update any code that relies on the ordering of vote polls within
VotePollsGroupedByTimestamp. - Conduct thorough testing to ensure that this change doesn't introduce any bugs or unexpected behavior in the voting system.
- Update the documentation for
VotePollsGroupedByTimestampto reflect the new ordering behavior. - Assess the performance impact of this change, especially for large numbers of vote polls.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rs-drive-proof-verifier/src/types.rs (2)
54-61: Clarify order preservation limitations in documentationThe documentation mentions that this data structure preserves the order of object insertion but notes that the order of objects returned by Dash Drive is not always guaranteed to be correct. To prevent confusion, consider clarifying these limitations and specifying how users should handle potential discrepancies in order.
58-58: Add missing article 'the' and correct wording in documentationThe phrase "preserves order of objects insertion" should be "preserves the order of object insertion" for grammatical correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-proof-verifier/src/types.rs (5 hunks)
- packages/rs-sdk/src/mock/requests.rs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/src/mock/requests.rs
🔇 Additional comments (2)
packages/rs-drive-proof-verifier/src/types.rs (2)
601-603:⚠️ Potential issueVerify the correctness of
ProTxHashtoIdentifierconversionThe conversion of
ProTxHashtoIdentifierusingIdentifier::from(pro_tx_hash.to_byte_array())may not handle all edge cases correctly. Please ensure that this conversion is accurate and consistent with howIdentifierinstances are typically created fromProTxHash. Consider whether additional processing or validation is needed.Run the following script to check for consistent usage of
ProTxHashtoIdentifierconversions:#!/bin/bash # Description: Search for instances where `ProTxHash` is converted to `Identifier` and verify usage. # Find all occurrences in Rust files where `ProTxHash` is converted to `Identifier` rg --type rust "Identifier::from\(\s*pro_tx_hash\.to_byte_array\(\)\s*\)" -A 2
90-90: Ensure all references toRetrievedIntegerValueare updatedThe type alias
RetrievedIntegerValuehas been renamed toRetrievedValues. Please verify that all references toRetrievedIntegerValuehave been updated throughout the codebase to maintain consistency and prevent compilation errors.Run the following script to check for any remaining references:
Issue being fixed or feature implemented
Rust Dash SDK does not preserve order of items returned by Drive.
What was done?
Replaced BTreeMap with IndexMap.
How Has This Been Tested?
GHA
Breaking Changes
In Rust SDK, returned objects are IndexMap/IndexSet instead of previous BTreeMap.
Affected types
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
IndexMapfor improved collection handling in various components.MockResponsetrait to support serialization/deserialization forIndexMap.MockContextProviderfor better data contract management and quorum key retrieval.Bug Fixes
Documentation
Tests