fix(sdk)!: wrong 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.
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.
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:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/rs-drive-proof-verifier/src/types.rs (2)
54-61: Clarify the documentation regarding order preservationThe documentation states that the data structure preserves the order of object insertion but also mentions that the actual order depends on Dash Drive and is "not always guaranteed to be correct." This could be confusing for users.
Consider clarifying under what conditions the order is preserved and when it might not be reliable. This will help users understand how to use
RetrievedObjectseffectively.
Line range hint
578-610: UpdateContenders::to_contendersreturn type to useIndexMapWhile other types have been updated to use
IndexMap, the methodContenders::to_contendersstill returns aBTreeMap. For consistency and to preserve insertion order, consider changing the return type toIndexMap<Identifier, Contender>.Apply this diff to update the return type:
fn to_contenders( &self, document_type: &DocumentType, platform_version: &PlatformVersion, ) -> Result<IndexMap<Identifier, Contender>, crate::Error> { self.contenders .iter() .map(|(id, v)| { let contender = v.try_to_contender(document_type.as_ref(), platform_version)?; Ok((*id, contender)) }) - .collect::<Result<BTreeMap<Identifier, Contender>, dpp::ProtocolError>>() + .collect::<Result<IndexMap<Identifier, Contender>, dpp::ProtocolError>>() .map_err(Into::into) }This change will align the method with the updated data structures and maintain order consistency throughout the codebase.
packages/rs-drive-proof-verifier/src/proof.rs (1)
835-845: Simplify Error Handling in Mapping overcontractsConsider simplifying the error handling within the
mapclosure for better readability by moving the error mapping outside the closure.Here's how you can refactor the code:
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(), - } - }) + Identifier::from_bytes(&k).map(|id| (id, v)) }) - .collect::<Result<DataContracts, Error>>()? + .collect::<Result<DataContracts, _>>() + .map_err(|e| Error::ResultEncodingError { + error: e.to_string(), + })? .into_option();
📜 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 (3)
- 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 (5 hunks)
🧰 Additional context used
📓 Learnings (2)
packages/rs-drive-proof-verifier/Cargo.toml (2)
Learnt from: lklimek PR: dashpay/platform#2207 File: packages/rs-drive-proof-verifier/Cargo.toml:46-46 Timestamp: 2024-10-04T10:07:23.864Z Learning: Usages of `BTreeMap` in crates that are out of scope of a change should not be flagged for replacement.Learnt from: lklimek PR: dashpay/platform#2207 File: packages/rs-drive-proof-verifier/Cargo.toml:46-46 Timestamp: 2024-10-09T00:22:57.778Z Learning: Usages of `BTreeMap` in crates that are out of scope of a change should not be flagged for replacement.packages/rs-drive-proof-verifier/src/proof.rs (2)
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.Learnt from: lklimek PR: dashpay/platform#2207 File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664 Timestamp: 2024-10-09T00:22:57.778Z 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 (10)
packages/rs-drive-proof-verifier/Cargo.toml (3)
Line range hint
3-3: Verify the version bumpThe package version has been updated from 1.4.0 to 1.4.1. However, the PR objectives mention breaking changes. Consider if a major version bump (to 2.0.0) might be more appropriate to reflect the breaking changes introduced by switching from BTreeMap to IndexMap/IndexSet.
18-18: LGTM: New feature additionThe addition of the "indexmap/serde" feature to the "mocks" feature group is appropriate. This aligns with the PR objective of changing data structures to IndexMap/IndexSet and ensures proper serialization support.
46-46: LGTM: New dependency additionThe addition of the indexmap dependency (version 2.6.0) is appropriate and necessary for implementing IndexMap/IndexSet as per the PR objectives. Using the latest stable version is good practice.
Regarding the past comments about remaining BTreeMap usages:
As per the learnings from previous discussions, usages of
BTreeMapin crates that are out of scope of this change should not be flagged for replacement. Therefore, we can safely ignore those instances in this review.packages/rs-drive-proof-verifier/src/proof.rs (7)
46-46: ImportingIndexMapfor Order PreservationThe addition of
IndexMapis appropriate to maintain insertion order in collections, ensuring consistent ordering of elements where necessary.
900-904: Preserving Order withIndexMapinDataContractHistoryUsing
IndexMap::from_iterensures that the history of data contracts retains the order, which could be significant for historical data analysis.
987-993: Proper Handling of Single Epoch ExpectationThe code correctly checks if exactly one epoch is returned and provides a meaningful error if not. This ensures that the function behaves as expected when a single epoch is required.
1056-1056: Collecting Epoch Information EfficientlyCollecting into
ExtendedEpochInfosis efficient and maintains consistency with the expected data structure.
1203-1207: Efficient Conversion toElementsInitializing
Elementsusingfrom_iteris an effective way to construct the collection from the retrieved objects.
1639-1657: Safe Use of.expect()Due to Prior ChecksThe use of
.expect("is_empty() must detect empty map")is safe here because the match arms ensure that the map is not empty before this line is executed.
1881-1885: ImplementingLengthTrait forIndexMapThe implementation of the
Lengthtrait forIndexMap<K, Option<T>>correctly counts the number of elements that areSome, aligning with the existing implementations for other collections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/rs-drive-proof-verifier/src/types.rs (3)
54-62: Clarify the order preservation guarantee in the documentationThe documentation for
RetrievedObjectscontains a slight inconsistency:
- Line 58-59 states that the order "is not always guaranteed to be correct."
- Line 62 mentions that the structure "preserves order of objects insertion."
To avoid confusion, consider rephrasing to clarify that while the insertion order is preserved, it may not always reflect the logical or expected order of the objects. This will help users understand the behavior more accurately.
70-88: Approve the new RetrievedValues type and suggest minor documentation improvementThe introduction of
RetrievedValuesas a replacement forRetrievedIntegerValueis well-implemented and documented. The use ofIndexMapis consistent with the overall changes in the file.Suggestion for improvement:
Consider adding a brief example in the documentation to illustrate how users can sort the objects if needed, as mentioned in the comment. This would provide immediate guidance for developers working with this type.
Line range hint
1-624: Summary of changes and recommendationsThe changes in this file primarily involve transitioning from
BTreeMaptoIndexMapfor various data structures. This transition preserves insertion order but may have different performance characteristics and impacts on code that relies on specific ordering.Key recommendations:
- Conduct thorough performance testing, especially for large datasets, to ensure no negative impacts from the
IndexMapusage.- Update all relevant documentation to reflect the new ordering behavior and performance characteristics.
- Review and update any code that may have been relying on the specific ordering provided by
BTreeMap.- Consider adding more detailed comments or examples in the documentation to guide users on how to work with the new
IndexMap-based structures, especially regarding sorting and performance considerations.- Ensure that the changelog or release notes clearly communicate these changes, as they may be breaking changes for some users.
Overall, the changes are well-implemented and consistent, but careful verification and documentation updates are crucial to ensure a smooth transition for all users of this module.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-drive-proof-verifier/src/proof.rs (13 hunks)
- packages/rs-drive-proof-verifier/src/types.rs (6 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/rs-drive-proof-verifier/src/proof.rs (2)
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.Learnt from: lklimek PR: dashpay/platform#2207 File: packages/rs-drive-proof-verifier/src/proof.rs:1646-1664 Timestamp: 2024-10-09T00:22:57.778Z 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 (12)
packages/rs-drive-proof-verifier/src/types.rs (5)
34-35: Acknowledge the breaking change and ensure proper documentationThe addition of
IndexMapto the public API is a significant change. The existing comment correctly identifies this as a breaking change. To complement this:
- Ensure that the module's documentation clearly explains the rationale behind using
IndexMapand its implications.- Update any relevant API documentation to reflect this change.
- Consider adding a note in the changelog or release notes about this change.
68-68: Acknowledge the change to IndexMap and suggest performance testingThe change from
BTreeMaptoIndexMapforRetrievedObjectsis significant. The existing comment correctly identifies the implications of this change. To further address this:
- Conduct thorough performance testing, especially with large datasets, to ensure that this change doesn't negatively impact the system's overall performance.
- Update any documentation or comments that might reference the previous ordering behavior of
BTreeMap.- Consider adding a note in the code comments about the performance characteristics of
IndexMapvsBTreeMapfor future maintainers.To assist with performance testing, you can use the following script to identify usage of
RetrievedObjectsin the codebase:#!/bin/bash # Description: Find usage of RetrievedObjects in the codebase # Search for RetrievedObjects usage echo "Searching for RetrievedObjects usage:" rg --type rust "RetrievedObjects" -C 2
255-257: Approve the new From implementation for ContestedResourceThe addition of the
From<ContestedResource> for Valueimplementation is a good improvement. It provides a convenient and idiomatic way to convertContestedResourcetoValue, which can simplify code that works with these types.
Line range hint
575-607: Approve changes to ProposerBlockCounts and suggest verificationThe changes to
ProposerBlockCountsand itsFromIteratorimplementations are consistent with the overall transition fromBTreeMaptoIndexMapin this file. The core logic remains the same, which is good for maintaining consistency.However, given that this change affects the ordering behavior and potentially the performance characteristics, it's important to verify its impact.
Please run the following script to identify usage of
ProposerBlockCountsin the codebase:Review the results to ensure that:
- Any code using
ProposerBlockCountsis updated if necessary to account for the new ordering behavior.- Performance-critical sections that use
ProposerBlockCountsare tested to ensure they're not negatively impacted by the change toIndexMap.
93-93: Approve the change to DataContractHistory and suggest impact verificationThe change of
DataContractHistoryto useRetrievedValuesis consistent with the other changes in this file. This change preserves insertion order but may affect code that relies on the previous ordering behavior ofBTreeMap.To ensure this change doesn't introduce unexpected behavior, please run the following script to identify usage of
DataContractHistoryin the codebase:Review the results to ensure that any code using
DataContractHistoryis updated if necessary to account for the new ordering behavior.✅ Verification successful
DataContractHistory Type Change Approved
The change of
DataContractHistoryto useRetrievedValueshas been verified. No usages that rely on the previousBTreeMapordering were found, ensuring no impact on the existing codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usage of DataContractHistory in the codebase # Search for DataContractHistory usage echo "Searching for DataContractHistory usage:" rg --type rust "DataContractHistory" -C 2Length of output: 10083
Script:
#!/bin/bash # Description: Find usage of DataContractHistory in the codebase # Search for DataContractHistory usage echo "Searching for DataContractHistory usage:" rg --type rust "DataContractHistory" -C 2Length of output: 24412
Script:
#!/bin/bash # Description: Comprehensive search for DataContractHistory usage in the codebase # Using ast-grep to find type declarations and implementations involving DataContractHistory echo "Searching for DataContractHistory type declarations and implementations:" ast-grep --lang rust --pattern 'pub type DataContractHistory = $_;' . echo "Searching for implementations involving DataContractHistory:" ast-grep --lang rust --pattern 'impl $_ for DataContractHistory $_' .Length of output: 890
packages/rs-drive-proof-verifier/src/proof.rs (7)
46-46: LGTM: IndexMap addition and Length trait implementationThe addition of IndexMap and its Length trait implementation are consistent with the changes mentioned in the summary. The implementation follows the same pattern as BTreeMap, which is good for maintaining consistency in the codebase.
Also applies to: 1905-1912
835-850: Improved handling of GetDataContractsRequest resultsThe changes in this section enhance the processing of contracts:
- It now uses
IndexMap::from_iter, which is consistent with the move towards using IndexMap for better collection handling.- The empty check
contracts.is_empty()ensures that None is returned when no contracts are found, improving the clarity of the API response.These changes improve both the consistency and the clarity of the code.
905-909: Enhanced GetDataContractHistoryRequest result handlingThe change to wrap the contract history result in
IndexMap::from_iteris a good improvement:
- It's consistent with the overall shift towards using IndexMap for better collection handling.
- Using IndexMap preserves the order of history entries, which is crucial for maintaining the chronological order of contract changes.
This change enhances both the consistency of the codebase and the functionality of the contract history feature.
Line range hint
992-1003: Improved error handling in GetEpochsInfoRequestThe changes in this section significantly enhance the robustness of the epoch info retrieval:
- It now checks for the exact number of epochs returned, throwing an error if more than one epoch is found. This ensures data integrity and prevents silent errors.
- The handling of the no-epochs-found case is now explicit, returning None in this scenario.
- The use of
into_iter().next()andand_thenprovides a concise and idiomatic way to extract the single epoch if it exists.These changes improve both the error handling and the clarity of the code, making it more reliable and maintainable.
1208-1208: Consistent use of from_iter in GetPathElementsRequestThe change to use
Elements::from_iterinstead of directly collecting into Elements is a good improvement:
- It's consistent with the overall pattern of using more flexible collection methods throughout the codebase.
- This approach potentially allows for better performance or a different internal representation of Elements, providing more flexibility for future optimizations.
This change enhances the consistency of the codebase and leaves room for future improvements in the Elements type.
1643-1662: Enhanced vote processing in GetContestedResourceIdentityVotesRequestThe changes in this section significantly improve the robustness of vote processing:
- It now handles multiple scenarios explicitly: multiple votes (error), no votes (return None), and exactly one vote (process it).
- The error messages are clear and informative, which will aid in debugging and troubleshooting.
- The use of
expecton line 1661 is safe due to the prioris_empty()check, ensuring no panics will occur.These changes greatly enhance the error handling and edge case management of the vote processing logic, making the code more reliable and easier to maintain.
1856-1857: Improved flexibility with new Length trait methodThe addition of the
countmethod to the Length trait implementations is a valuable enhancement:
- It provides more flexibility in how the length of collections is calculated and used, allowing for distinguishing between the total number of elements and the number of non-None elements.
- The implementation is consistent across different types (Option, Vec, BTreeMap, IndexMap), which maintains code coherence and predictability.
- This change can be particularly useful in scenarios where the presence of None values is as significant as non-None values.
These additions improve the expressiveness of the Length trait and provide more options for working with collections in the codebase.
Also applies to: 1867-1872, 1879-1882, 1889-1892, 1899-1902, 1910-1912
shumkov
changed the title
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
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