feat(sdk): get identities by non-unique public key hashes #2491
Conversation
WalkthroughThe changes add support for handling identity queries based on non-unique public key hashes. New RPC methods, request and response types, and query modules have been implemented across the system. The gRPC client and SDK were updated to support the new request, and corresponding fetching, proving, and verification logic were added or renamed in backend modules. Versioning structures were also updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK
participant QueryService
participant Platform
participant Drive
Client->>SDK: Send non-unique public key hash query
SDK->>QueryService: get_identity_by_non_unique_public_key_hash(request)
QueryService->>Platform: query_identity_by_non_unique_public_key_hash(request)
Platform->>Drive: fetch or prove identity based on non-unique public key hash
Drive-->>Platform: Return identity data or proof
Platform-->>QueryService: Build response with identity info
QueryService-->>SDK: Return response
SDK-->>Client: Deliver response
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🪧 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: 4
🧹 Nitpick comments (13)
packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs (1)
41-41: Function name updated but documentation needs to be alignedThe function name has been updated to include "unique" in the name, which clarifies the function's purpose. However, the function documentation still refers to the old naming convention without the "unique" qualifier.
Consider updating the documentation comments in lines 12-39 to reflect the new function name. For example, line 20-21 should be updated from:
/// The function first verifies the identity ID associated with the given public key hash /// by calling `verify_identity_id_by_public_key_hash()`. It then uses this identity ID to verifyto:
/// The function first verifies the identity ID associated with the given unique public key hash /// by calling `verify_identity_id_by_unique_public_key_hash()`. It then uses this identity ID to verifypackages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)
69-69: Remove or replace the debug print
Theprintln!()call may be leftover. Consider using structured logging or removing it.- println!("hex {}", hex::encode(&identity_proof)); + // Consider using a logging macro or removing if unneededpackages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/mod.rs (1)
38-64: Consider validating the proof’s content or length before dispatch.While version-based dispatch is a sound approach, adding simple checks (e.g., ensuring the proof slice is not empty) can catch incorrect usage earlier. Stating assumptions and preconditions explicitly at the start of this method can help strengthen defensive programming practices.
packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/mod.rs (1)
54-78: Add a preflight check for the proof’s validity.Before calling the version-specific handler, consider verifying that the provided proof is structurally valid (e.g., correct fields are present). Guarding at this abstraction level might prevent deeper code paths from being triggered with invalid data, reducing debugging overhead.
packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/mod.rs (1)
49-75: Consider enhancing error reports with context on missing identities.When generating a proof for a public key hash that does not map to any identity, consider providing an explicit error or logging detail. This could inform callers or log systems where the gap is (e.g., “No identity found for provided public key hash”), expediting troubleshooting.
packages/rs-drive/src/drive/identity/fetch/queries/mod.rs (1)
94-109: Consider bounding or verifying the result set.While this method generates a
PathQueryfor a non-unique public key hash, it usesnew_range_full()whenafteris not supplied. This could potentially retrieve multiple identities sharing the same hash. If only one result is expected, consider adding an explicit limit or verifying that no unexpected multiple matches occur.packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs (1)
1-71: Revisit version bounds reference for non-unique queries.Lines 37-38 reference
identity_by_unique_public_key_hashwhen checking version bounds, yet this file implements querying by a non-unique hash. If there’s a separate version bound for non-unique features, it might be preferable to reference that for clarity. Otherwise, if it’s intentional to reuse the same bounds, consider adding a clarifying comment.packages/dapi-grpc/protos/platform/v0/platform.proto (2)
625-632: Document the expected input size.
While the protobuf definition forpublic_key_hashis valid, consider adding a clarifying comment that it is expected to be 20 bytes to prevent confusion.
634-652: Consider improving proof handling.
Usingidentity_proof_bytesin addition togrovedb_identity_public_key_hash_proofis labeled a “hack.” Consider consolidating them or more clearly naming this field to avoid confusion in future maintenance.packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs (1)
105-193: Add more negative test coverage forstart_after.
The existing tests thoroughly check invalid key-hash lengths and missing entries. Consider adding a test case to confirm the error behavior when an invalidstart_afterlength is provided, ensuring full coverage of this field.packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)
30-52: Check limit usage for multiple matching identities.
The code setslimitto 1, which discards additional identities. Confirm that ignoring the possibility of multiple matches is desired. If it’s not, consider returning all matching identities.packages/rs-drive-proof-verifier/src/proof.rs (1)
351-442: New proof handling for non-unique public key hashes.
This implementation follows the established pattern for identity proofs, adapted for non-unique key hashes. Verify:
- Whether multiple identities sharing the same key hash are handled (beyond the first match).
- That the
afterparameter is tested, especially for edge cases with and without an initial offset.packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_identity_ids_by_non_unique_public_key_hash/mod.rs (1)
17-19: Doc comments updated for new parameters.
The mention oflimitandplatform_versionis helpful. Consider adding a brief note clarifying howafteris used for pagination or skipping.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
packages/dapi-grpc/build.rs(3 hunks)packages/dapi-grpc/protos/platform/v0/platform.proto(2 hunks)packages/rs-dapi-client/src/transport/grpc.rs(1 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/mod.rs(3 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/v0/mod.rs(4 hunks)packages/rs-drive-abci/src/query/identity_based_queries/mod.rs(1 hunks)packages/rs-drive-abci/src/query/service.rs(2 hunks)packages/rs-drive-abci/tests/strategy_tests/query.rs(1 hunks)packages/rs-drive-proof-verifier/src/proof.rs(4 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_full_identity_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_full_identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_identity_ids_by_non_unique_public_key_hash/mod.rs(4 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_identity_ids_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs(2 hunks)packages/rs-drive/src/drive/identity/fetch/prove/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_id_by_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_ids_by_unique_public_key_hashes/v0/mod.rs(1 hunks)packages/rs-drive/src/drive/identity/fetch/queries/mod.rs(3 hunks)packages/rs-drive/src/drive/identity/identity_and_non_unique_public_key_hash_double_proof.rs(1 hunks)packages/rs-drive/src/drive/identity/mod.rs(1 hunks)packages/rs-drive/src/drive/mod.rs(2 hunks)packages/rs-drive/src/verify/identity/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_full_identities_by_public_key_hashes/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/mod.rs(2 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/mod.rs(2 hunks)packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/mod.rs(2 hunks)packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/v0/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/mod.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/v1.rs(2 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs(1 hunks)packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs(1 hunks)packages/rs-platform-version/src/version/mocks/v2_test.rs(1 hunks)packages/rs-sdk/src/platform/types/identity.rs(3 hunks)packages/rs-sdk/tests/fetch/identity.rs(1 hunks)
🔇 Additional comments (86)
packages/rs-drive/src/drive/identity/fetch/prove/mod.rs (1)
4-4: Appropriate module addition for non-unique public key hash functionalityThe new module
prove_full_identity_by_non_unique_public_key_hashis properly integrated into the existing module structure and aligns with the PR goal of retrieving identities using public key hashes.packages/rs-drive/src/drive/identity/mod.rs (1)
43-45: Module addition follows proper conventionsThe new module for handling non-unique public key hash proofs is correctly added with proper conditionals (
#[cfg(any(feature = "server", feature = "verify"))]) and includes clear documentation describing its purpose.packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_id_by_unique_public_key_hash/v0/mod.rs (1)
68-68: Method name update reflects specificity for unique public key hashesChanging the method call to
verify_identity_id_by_unique_public_key_hashproperly reflects the specific functionality of working with unique public key hashes. This aligns with adding support for non-unique public key hashes elsewhere in the codebase.packages/rs-drive/src/drive/identity/identity_and_non_unique_public_key_hash_double_proof.rs (1)
1-21: Well-structured and documented proof structThe new
IdentityAndNonUniquePublicKeyHashDoubleProofstruct is well-designed with:
- Clear documentation explaining its purpose and fields
- Appropriate field types (optional identity proof and required identity-to-hash proof)
- Proper member visibility (
pub)This struct provides the necessary data structure to support identity verification using non-unique public key hashes.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs (1)
70-70: Field renamed to clarify operation on unique public key hashesThe field has been appropriately renamed from
identity_by_public_key_hashtoidentity_by_unique_public_key_hashto explicitly indicate that this operates on unique public key hashes. This change maintains consistency with other renaming operations across the codebase as part of the implementation for handling non-unique public key hashes.packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/v0/mod.rs (2)
40-40: Function renamed to clarify operation on unique public key hashesThe function has been appropriately renamed from
verify_identity_ids_by_public_key_hashes_v0toverify_identity_ids_by_unique_public_key_hashes_v0to explicitly indicate that this verifies identity IDs based on unique public key hashes. This rename is consistent with the PR's purpose of adding support for handling non-unique public key hashes.
40-96:❓ Verification inconclusive
Verify function call sites are updated
The function rename needs to be reflected at all call sites to prevent compilation errors. Make sure all references to this function have been updated throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to the old function name rg "verify_identity_ids_by_public_key_hashes_v0" --type rustLength of output: 60
Action Required: Manual Verification of Function Call Sites
The recent search for references to the outdated function name (
verify_identity_ids_by_public_key_hashes_v0) returned no matches, which indicates that the renaming appears to be correctly applied. However, since the search output was empty—potentially due to low-quality inference—please manually verify that all call sites throughout the codebase are using the updated name (verify_identity_ids_by_unique_public_key_hashes_v0). Specifically:
- Confirm that no lingering references to the old function name exist.
- Ensure that all call sites have been updated and that the code compiles without issues.
packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs (1)
55-55: Field renamed to clarify operation on unique public key hashesThe field has been appropriately renamed from
identity_by_public_key_hashtoidentity_by_unique_public_key_hashto maintain consistency with the changes in other files. This rename is part of the broader effort to distinguish between unique and non-unique public key hash operations.packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/v0/mod.rs (3)
36-36: Function renamed to clarify operation on unique public key hashesThe function has been appropriately renamed from
verify_identity_id_by_public_key_hash_v0toverify_identity_id_by_unique_public_key_hash_v0to indicate that it verifies a single identity ID based on a unique public key hash. This change is consistent with the PR's focus on supporting operations with non-unique public key hashes.
42-42:✅ Verification successful
Verify related query function is updated
This line calls
identity_id_by_unique_public_key_hash_query, which appears to be the renamed version of a previous function. Make sure this query function is properly implemented and that its name has been updated throughout the codebase.
🏁 Script executed:
#!/bin/bash # Find the implementation of the query function rg "identity_id_by_unique_public_key_hash_query" --type rust -A 5Length of output: 4843
Query Function Naming Verified
I confirmed that the function
identity_id_by_unique_public_key_hash_queryis consistently updated and referenced throughout the codebase. Its implementation inpackages/rs-drive/src/drive/identity/fetch/queries/mod.rsis correctly invoked in all related modules, including the verify and prove components. No further modifications are needed.
36-90:✅ Verification successful
Verify function call sites are updated
The function rename needs to be reflected at all call sites to prevent compilation errors. Make sure all references to this function have been updated throughout the codebase.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to the old function name rg "verify_identity_id_by_public_key_hash_v0" --type rustLength of output: 57
Below is an updated review comment. To further ensure the call sites are correct, please run an additional check for the new function name as well:
Ensure all call sites use
verify_identity_id_by_unique_public_key_hash_v0Our earlier search for the old name (
verify_identity_id_by_public_key_hash_v0) returned no matches, suggesting that the deprecated name is no longer used. However, since the command did not provide output, please manually verify (or run the new script above) that all references have been correctly updated to call the new function. This will help prevent any potential compilation issues.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining references to the updated function name rg "verify_identity_id_by_unique_public_key_hash_v0" --type rustLength of output: 508
Ensure all references to the renamed function are correct
The investigation confirms that no references to the old function name (
verify_identity_id_by_public_key_hash_v0) remain. The search shows that the updated name (verify_identity_id_by_unique_public_key_hash_v0) is correctly used both in the call sites and in its definition:
packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/mod.rscalls the function.packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/v0/mod.rscontains the function definition.There are no outdated usages, so the renaming appears to be applied consistently across the codebase.
packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs (2)
2-2: Good addition of the new module for non-unique public key hash functionality.The addition of this module aligns well with the broader changes to support identity fetching by non-unique public key hashes across the system.
74-77: Parameters correctly updated to match new function signature.The function call has been properly updated to include the new optional parameters and now uses
platform_versioninstead ofdrive_version, maintaining consistency with other similar functions in the codebase.packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_unique_public_key_hash/v0/mod.rs (1)
90-90: Improved method naming for clarity and consistency.Renaming from
verify_full_identity_by_public_key_hashtoverify_full_identity_by_unique_public_key_hashprovides better clarity about the method's purpose and maintains consistency with the naming convention used throughout the codebase.packages/rs-platform-version/src/version/mocks/v2_test.rs (1)
203-203: Field name updated for better precision and consistency.Renaming from
identity_by_public_key_hashtoidentity_by_unique_public_key_hashin theDriveAbciQueryIdentityVersionsstructure makes the field name more precise and consistent with other similar changes in the codebase. This improves code clarity by explicitly indicating that this field relates to unique public key hashes.packages/rs-drive-abci/tests/strategy_tests/query.rs (1)
269-269: Method name updated for clarity and consistency.The method call has been properly renamed from
verify_full_identity_by_public_key_hashtoverify_full_identity_by_unique_public_key_hash, maintaining consistency with similar naming updates throughout the codebase. This change improves code clarity by explicitly indicating the method is for verifying identities with unique public key hashes.packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs (1)
46-51: Function call updated to match new naming conventionThe function call has been correctly updated to use the new naming convention with "unique" qualifier, maintaining consistency with the function name change above.
packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/v1.rs (2)
30-30: Added new field for fetching identity by non-unique public key hashThe addition of this field is consistent with the PR's objective to implement functionality for retrieving identities using public key hashes.
62-62: Added new field for proving identity by non-unique public key hashThe addition of this field complements the fetch functionality by providing proof capabilities for non-unique public key hashes.
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/mod.rs (4)
35-35: Path updated to use new naming conventionThe path has been correctly updated to reflect the new naming convention.
44-45: Error message updated to use new naming conventionThe error message has been correctly updated to match the new feature name.
55-59: Function call updated to use new naming conventionThe function call has been correctly updated to reference the function with the "unique" qualifier.
6-10:❓ Verification inconclusive
Import statements still use old naming convention
While the function and feature path references have been updated to include "unique", the import statements still use the old naming convention without the "unique" qualifier.
This suggests that either:
- The gRPC proto definitions haven't been updated, or
- The generated code hasn't been regenerated after proto updates.
This inconsistency could lead to runtime errors or confusion for developers. Either update the import statements to match the new naming convention or ensure that the proto definitions are correctly aligned with the implementation.
🏁 Script executed:
#!/bin/bash # Check if the proto files have been updated to match the new naming convention # Look for both old and new proto definitions echo "Checking for old proto definitions:" rg -A 2 "GetIdentityByPublicKeyHashRequest" packages/dapi-grpc/protos/ echo "Checking for new proto definitions:" rg -A 2 "GetIdentityByUniquePublicKeyHashRequest" packages/dapi-grpc/protos/Length of output: 1372
Action Required: Align Naming Conventions Between Code and Proto Definitions
It appears that the file
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/mod.rs
still imports identifiers using the old naming (e.g.,get_identity_by_public_key_hash_request::Version) even though the surrounding code and file/directory naming imply a move toward a “unique” naming scheme.
Findings:
- The grep for
GetIdentityByPublicKeyHashRequestinpackages/dapi-grpc/protos/shows that the proto definitions remain using the old naming convention.- No proto definitions were found for
GetIdentityByUniquePublicKeyHashRequest, which confirms that the gRPC proto definitions have not been updated to reflect the new naming.Recommendation:
- Either: Update the gRPC proto definitions (and regenerate the associated code) to include the “unique” qualifier so that the imports and feature naming are aligned.
- Or: Revisit the naming in the implementation (and possibly the file/directory names) so that they remain consistent with the current proto definitions and generated code.
Please verify the intended design in this part of the codebase and adjust accordingly.
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/v0/mod.rs (4)
17-17: Function renamed to clarify uniqueness constraintThe function has been renamed from
query_identity_by_public_key_hash_v0toquery_identity_by_unique_public_key_hash_v0to explicitly indicate that it works with unique public key hashes. This change improves clarity and is part of the broader feature implementation.
94-95: Updated function call to match renamed methodThe test code has been updated to use the renamed function, maintaining consistency throughout the codebase.
114-115: Updated function call to match renamed methodThe test code has been updated to use the renamed function, maintaining consistency throughout the codebase.
134-135: Updated function call to match renamed methodThe test code has been updated to use the renamed function, maintaining consistency throughout the codebase.
packages/rs-drive-abci/src/query/identity_based_queries/mod.rs (1)
6-7: Added modules for specialized public key hash handlingThe single module
identity_by_public_key_hashhas been replaced with two specialized modules:
identity_by_non_unique_public_key_hashidentity_by_unique_public_key_hashThis change supports the new feature for retrieving identities by non-unique public key hashes, providing a clear separation of concerns.
packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/mod.rs (4)
39-40: Method renamed to clarify uniqueness constraintThe method has been renamed from
verify_identity_ids_by_public_key_hashestoverify_identity_ids_by_unique_public_key_hashesto explicitly indicate that it works with unique public key hashes. This improves clarity and maintains consistency with other renamed methods.
52-53: Updated internal method referenceInternal reference to the method has been updated to match the renamed method.
54-55: Updated versioned method callThe versioned method call has been updated to match the renamed method.
61-62: Updated error message to match renamed methodThe error message has been updated to reflect the renamed method, maintaining consistency in error reporting.
packages/rs-dapi-client/src/transport/grpc.rs (1)
493-500: Added support for retrieving identities by non-unique public key hashThis addition implements the
TransportRequesttrait for the newGetIdentityByNonUniquePublicKeyHashRequestand corresponding response types, enabling the gRPC client to handle requests for identity retrieval based on non-unique public key hashes. This is a key component of the new feature.The implementation follows the established pattern used for other similar requests in the codebase.
packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/mod.rs (4)
36-41: Function renamed for enhanced clarity.The function has been renamed from
verify_identity_id_by_public_key_hashtoverify_identity_id_by_unique_public_key_hashto explicitly indicate that it works with unique public key hashes. This naming change improves code clarity by distinguishing it from operations that work with non-unique public key hashes.
47-48: Method path updated to reflect new function name.The method path in the platform version check has been correctly updated to use the new function name.
49-54: Function call correctly updated.The internal function call has been properly updated to use the new v0 function name.
56-57: Error message updated for consistency.The error message has been updated to reflect the new method name, which maintains consistency throughout the codebase.
packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_ids_by_unique_public_key_hashes/v0/mod.rs (1)
92-97: Updated function call to use renamed method.The call to
verify_identity_ids_by_public_key_hasheshas been correctly updated toverify_identity_ids_by_unique_public_key_hashes, maintaining consistency with the renaming pattern across the codebase.packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/mod.rs (3)
39-43: Function renamed for enhanced clarity.The function has been renamed from
verify_full_identity_by_public_key_hashtoverify_full_identity_by_unique_public_key_hashto explicitly indicate that it works with unique public key hashes.
51-55: Internal function call correctly updated.The internal function call has been properly updated to the new v0 function name.
57-58: Error message updated for consistency.The error message has been updated to reflect the new method name, maintaining consistency throughout the codebase.
packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/mod.rs (2)
98-99: Added support for fetching identities by non-unique public key hash.A new field
fetch_full_identity_by_non_unique_public_key_hashhas been added to support fetching full identities using non-unique public key hashes. This enhances the system's capabilities for handling different types of public key hashes.
137-138: Added support for proving identities by non-unique public key hash.A new field
prove_full_identity_by_non_unique_public_key_hashhas been added to support proving full identities using non-unique public key hashes. This complements the fetching capability and completes the feature implementation.packages/dapi-grpc/build.rs (3)
66-66: Updated VERSIONED_REQUESTS array size properlyThe array size in the constant definition has been correctly increased from 34 to 35 to accommodate the new request type.
78-78: New request type added successfullyThe addition of
GetIdentityByNonUniquePublicKeyHashRequestto the VERSIONED_REQUESTS array properly registers this request type for version tracking.
108-110: Helpful documentation added for custom proof handlingGood clarification about why
GetIdentityByNonUniquePublicKeyHashResponseis excluded from versioned responses. The comment clearly explains it requires custom proof handling.packages/rs-drive-abci/src/query/service.rs (2)
31-31: Correctly imported new request and response typesThe necessary types for the new functionality have been properly imported.
415-425: Well-implemented new query methodThe implementation of
get_identity_by_non_unique_public_key_hashfollows the established pattern in the codebase:
- Using the same method signature as other similar methods
- Properly utilizing the
handle_blocking_queryhelper- Passing the correct method reference and logging identifier
The method will integrate seamlessly with the existing query framework.
packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_full_identity_by_non_unique_public_key_hash/mod.rs (3)
1-9: Good module organization and importsThe module structure and imports follow the project's conventions, including proper error handling and necessary dependencies.
10-24: Well-documented functionThe function includes comprehensive documentation with:
- A clear description of its purpose
- Explanation of version dispatching
- Detailed parameter descriptions
- Return value documentation
This level of documentation helps maintainability and follows project standards.
25-52: Solid implementation with proper version handlingThe function properly:
- Accepts appropriate parameters (public key hash, pagination options, transaction, platform version)
- Uses the version dispatching pattern consistently with other similar functions
- Handles unknown versions with descriptive error messages
- Returns the expected result type (Option)
The implementation is consistent with other version-aware methods in the codebase.
packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs (2)
24-25: Successfully renamed methods for clarityThe method names have been updated to clearly indicate they work with unique public key hashes:
verify_identity_id_by_public_key_hash→verify_identity_id_by_unique_public_key_hashverify_identity_ids_by_public_key_hashes→verify_identity_ids_by_unique_public_key_hashesThis renaming enhances API clarity by being explicit about the uniqueness constraint.
31-32: Added new verification methods for non-unique keysThe two new verification methods for non-unique public key hashes complete the feature implementation:
verify_full_identity_by_non_unique_public_key_hashverify_identity_id_by_non_unique_public_key_hashThese methods are initialized with version 0, consistent with other methods in this structure.
packages/rs-drive/src/drive/mod.rs (2)
264-269: Enablement under 'server' or 'verify' features looks correct
These lines appropriately widen the compilation scope to both "server" and "verify" features. The logic returning a simple static path appears fine.
281-287: Similar enablement for sub-tree path
No issues apparent with this added scope. The function signature and path construction are consistent with established patterns.packages/rs-drive/src/verify/identity/verify_full_identities_by_public_key_hashes/v0/mod.rs (1)
51-56: Clarify whether public key hashes are unique or non-unique
The function is namedverify_full_identities_by_public_key_hashes_v0, yet it callsverify_identity_ids_by_unique_public_key_hashes. Verify that strictly unique key hashes are intended.packages/rs-sdk/src/platform/types/identity.rs (3)
12-12: New import for non-unique hash requests
Import statement is straightforward and correct.
35-36: Added enum variant for non-unique public key hash
Expanding the delegate enum withGetIdentityByNonUniquePublicKeyHashvariants is consistent with the rest of the code.
79-108: Check keyhash length
This struct uses a 32-byte key hash, while other areas reference 20-byte hashes. If 32 bytes is intentional for non-unique keys, clarify this difference in documentation.packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)
14-50: Well-documented approach
The inline documentation is thorough and understandable, explaining proof usage and error handling. Good job.packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/mod.rs (1)
13-37: Great documentation and parameter descriptions.The doc comments provide a clear overview of the method’s purpose and usage. Including the meaning of each parameter, potential errors, and version handling is excellent for maintainability and clarity.
packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/mod.rs (1)
16-53: Good thorough documentation.The descriptive comments unify expectations about the parameters, return values, and version constraints, promoting clarity for future maintainers and users of this API.
packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/mod.rs (1)
11-48: Well-structured doc comments.The explanation of arguments and the expected proof content improves understanding. This clarity will help consumers interact accurately with the function and handle the returned proof appropriately.
packages/rs-drive/src/drive/identity/fetch/queries/mod.rs (2)
3-3: Confirm consistent usage of imported function.The newly added import
non_unique_key_hashes_tree_path_veclooks correctly referenced later in the file. No immediate concerns.
234-249: Ensure identity match consistency.The merged query returns the full identity and also queries by non-unique public key hash. In scenarios where multiple identities share the hash, consider verifying that the returned
identity_idmatches the expected single identity in downstream logic.Would you like to confirm downstream usage to ensure that mismatched results are handled properly?
packages/rs-drive/src/verify/identity/mod.rs (2)
3-4: Modules renamed to differentiate between unique and non-unique.The addition of
verify_full_identity_by_non_unique_public_key_hashandverify_full_identity_by_unique_public_key_hashappears consistent with broader naming.
10-12: Ensure correct separation of functionality.Declaring separate modules for
verify_identity_id_by_non_unique_public_key_hash,verify_identity_id_by_unique_public_key_hash, andverify_identity_ids_by_unique_public_key_hashesclarifies different verification pathways. No issues noted.packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/v0/mod.rs (1)
1-70: Confirm single-result limit is intended.This verification function sets a limit of one result for a non-unique key hash. Since multiple identities can theoretically share the same non-unique hash, confirm whether returning only the first match is desired.
Would you like a verification script or inline check to confirm that no downstream logic relies on multiple matching identities for the same hash?
packages/dapi-grpc/protos/platform/v0/platform.proto (1)
36-37: New RPC definition looks consistent.
The addition ofgetIdentityByNonUniquePublicKeyHashaligns with the existing pattern for identity-based RPC methods and follows naming conventions.packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs (2)
1-16: Imports appear well-organized.
There are no immediate issues with these imports, and they seem consistent with the rest of the module’s usage.
17-46: Overall function structure is clear.
This function logically handles bothpublic_key_hashandstart_afterchecks, sets up either a proof or a fetched identity, then wraps the result in aQueryValidationResult. The approach cleanly separates proof vs. non-proof requests.packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/v0/mod.rs (2)
9-55: Method implements proof generation correctly.
The function correctly retrieves the first identity matching the provided non-unique key, builds proofs, and handles the no-result scenario by returningNone. This design is straightforward and aligns with the rest of the Drive module.
56-220: Tests are robust and demonstrate correct functionality.
Both tests confirm behavior with multiple identities sharing a key and confirm proof correctness. This helps ensure the method behaves as expected in various usage scenarios.packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)
13-28: Return behavior may be partial if multiple identities share the key hash.
This function fetches only one identity (the first match) for a potentially non-unique public key hash. Verify that returning a single identity is intentional rather than returning all associated identities.packages/rs-drive-proof-verifier/src/proof.rs (3)
13-14: Imported requests appear consistent.
Addingget_identity_by_non_unique_public_key_hash_requestaligns with the new functionality.
36-36: New import for double-proof structure.
Bringing inIdentityAndNonUniquePublicKeyHashDoubleProofsupports the newly introduced verification logic.
330-344: Ensure unique key hash verification call is correct.
The code callsverify_full_identity_by_unique_public_key_hash. Confirm this aligns with the updated approach for unique key hashes and that all relevant usage sites are updated accordingly.packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_identity_ids_by_non_unique_public_key_hash/mod.rs (4)
5-5: New import for Drive operations.
Usage ofLowLevelDriveOperationis aligned with the rest of the module.
7-7: Platform version import is correct.
Inclusion ofplatform_versionensures consistent version-specific logic in this file.
27-33: Extended arguments for additional query options.
Introducinglimitandafterexpands functionality for partial or paginated fetches. Confirm the calling code handles theseOptionparameters safely.
55-85: Operations-based variant parallels the main fetch method.
Mirrors the same version-check logic while allowing collection of drive operations. If this becomes more widely used, ensure future version handling is in place.packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/fetch_identity_ids_by_non_unique_public_key_hash/v0/mod.rs (4)
11-11: Appropriate platform version integrationThe dependency on
PlatformVersionhas been properly added and the parameter change fromdrive_versiontoplatform_versionhas been consistently applied throughout the file. This is a good practice for maintaining version compatibility across the system.Also applies to: 22-22, 31-31
19-20: Well-implemented pagination parametersThe addition of
limitandafterparameters provides essential pagination functionality for handling potentially large result sets. These parameters are consistently passed through from the public method to the operations method.Also applies to: 27-28, 39-40
46-55: Properly implemented pagination logicThe pagination logic correctly handles the
afterparameter by usingQueryItem::RangeAfterwhen specified, otherwise defaulting toQueryItem::RangeFull. The limit is also appropriately applied to the query. This implementation follows best practices for efficient database querying with pagination.
61-61: Consistent version handlingThe code correctly uses the drive component from the platform version, ensuring that the appropriate version-specific functionality is used for database operations.
packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs (1)
45-46:❓ Verification inconclusive
Well-structured addition of non-unique public key hash verification methods
The addition of methods for handling non-unique public key hashes is well-structured and follows the established pattern in the codebase. These new feature version fields align with the pagination-enabled fetch methods implemented in the first file.
Please ensure that these new methods are properly initialized in the version configuration to avoid potential null reference issues.
🏁 Script executed:
#!/bin/bash # Check how these new feature versions are initialized in the platform version configurations rg -A 5 "DriveVerifyIdentityMethodVersions" --type rust | grep -E "verify_full_identity_by_non_unique_public_key_hash|verify_identity_id_by_non_unique_public_key_hash"Length of output: 167
ACTION REQUIRED: Manually Verify Feature Version Initialization
The new fields for handling non-unique public key hash verification are well-structured and align with the codebase’s established patterns. However, an automated search for their initialization in the platform version configurations did not return any results. Please manually verify that both
verify_full_identity_by_non_unique_public_key_hash verify_identity_id_by_non_unique_public_key_hashare properly initialized in all relevant version configuration files to avoid any potential null reference issues.
| .verify_full_identity_by_public_key_hash | ||
| { |
There was a problem hiding this comment.
Inconsistency between renamed function and method path.
While the function has been renamed to verify_full_identity_by_unique_public_key_hash, the method path still references the old name verify_full_identity_by_public_key_hash. This inconsistency could lead to runtime errors or unexpected behavior.
match platform_version
.drive
.methods
.verify
.identity
- .verify_full_identity_by_public_key_hash
+ .verify_full_identity_by_unique_public_key_hash
{📝 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.
| .verify_full_identity_by_public_key_hash | |
| { | |
| match platform_version | |
| .drive | |
| .methods | |
| .verify | |
| .identity | |
| .verify_full_identity_by_unique_public_key_hash | |
| { |
There was a problem hiding this comment.
@QuantumExplorer or @shumkov , is it safe to rename field in DriveVerifyIdentityMethodVersions ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
...drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs
Outdated
Show resolved
Hide resolved
packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs
Show resolved
Hide resolved
lklimek
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs (1)
105-219: Add a positive test case to validate a successful identity retrieval.Your current tests cover invalid inputs, identity not found, and proof absence scenarios, but do not confirm the behavior when an actual identity exists and is successfully retrieved. Adding this test would strengthen test coverage and help ensure correct functionality for normal (non-error) use cases.
packages/rs-sdk/src/platform/types/identity.rs (2)
79-84: Add explanatory documentation for the new struct.While the struct name is self-descriptive, adding doc comments would help clarify use cases and expected behaviors, especially regarding the difference from unique public key hash queries.
88-90: Handle scenarios where proofs are not required or document rationale for the unimplemented state.Currently, the code calls
unimplemented!ifprove == false. If future scenarios need queries without proofs, consider implementing partial handling or returning a descriptive error/early exit message to guide integrators.packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)
27-28: Consider unifying naming of the 'after' parameter with 'start_after'.Throughout the codebase, similar functionality refers to this parameter as
start_after. Consistent naming helps reduce confusion and align with the rest of the query logic.packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs (1)
30-31: Align the naming to reflect non-unique public key hash usage.The TODO highlights a potential mismatch in naming. Consider renaming or adding a new field instead of reusing “identity_by_unique_public_key_hash” to avoid confusion and improve clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/dapi-grpc/build.rs(3 hunks)packages/rs-dapi-client/src/transport/grpc.rs(1 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs(1 hunks)packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive-proof-verifier/src/proof.rs(28 hunks)packages/rs-drive-verify-c-binding/src/lib.rs(3 hunks)packages/rs-drive/Cargo.toml(2 hunks)packages/rs-drive/src/drive/identity/fetch/queries/mod.rs(3 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs(1 hunks)packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs(2 hunks)packages/rs-platform-version/Cargo.toml(1 hunks)packages/rs-sdk/src/error.rs(1 hunks)packages/rs-sdk/src/platform/query.rs(1 hunks)packages/rs-sdk/src/platform/tokens/token_status.rs(1 hunks)packages/rs-sdk/src/platform/types/identity.rs(3 hunks)packages/rs-sdk/tests/fetch/identity.rs(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/rs-sdk/src/platform/tokens/token_status.rs
- packages/rs-sdk/src/error.rs
- packages/rs-sdk/src/platform/query.rs
- packages/rs-platform-version/Cargo.toml
- packages/rs-drive/Cargo.toml
- packages/rs-drive-verify-c-binding/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/rs-dapi-client/src/transport/grpc.rs
- packages/rs-sdk/tests/fetch/identity.rs
- packages/dapi-grpc/build.rs
- packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs
- packages/rs-drive/src/drive/identity/fetch/queries/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Tests
- GitHub Check: Rust packages (rs-dapi-client) / Check each feature
- GitHub Check: Rust packages (rs-dapi-client) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Rust packages (drive-abci) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (drive-abci) / Linting
- GitHub Check: Rust packages (drive-abci) / Check each feature
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (7)
packages/rs-drive-proof-verifier/src/proof.rs (7)
18-21: Good organization of importsThe expanded import group nicely contains all the related GRPC request and response types, including the new non-unique public key hash related types. The organization makes the code more maintainable.
42-42: Appropriate import for new proof typeThe
IdentityAndNonUniquePublicKeyHashDoubleProofimport matches the functionality being added for handling non-unique public key hash verification.
329-334: Method rename properly distinguishes between unique and non-unique keysThe implementation now uses
verify_full_identity_by_unique_public_key_hashinstead ofverify_full_identity_by_public_key_hash, which provides a clear separation between unique and non-unique public key hash verification flows.
342-432: Well-structured implementation for non-unique public key hash verificationThe implementation for retrieving identities by non-unique public key hash is comprehensive and properly structured. It handles:
- Different proof structure with appropriate error handling
- Extraction and validation of public key hash and optional start_after parameter
- Construction of a specialized
IdentityAndNonUniquePublicKeyHashDoubleProof- Proper verification flow with both Drive and Tenderdash proofs
A few observations:
- The code includes a commented line (359) that explains the different proof structure, which is helpful for maintainers
- Error handling is detailed with specific messages for different failure cases
- The implementation appropriately handles the pagination parameter (start_after) for scenarios where multiple identities match the same public key hash
379-397: Good parameter validation with clear error messagesThe code thoroughly validates input parameters with descriptive error messages:
- Public key hash length validation
- Optional start_after parameter validation
This ensures proper input sanitization before processing the request.
401-408: Well-structured conversion to specialized proof typeThe code properly constructs the
IdentityAndNonUniquePublicKeyHashDoubleProoffrom the response, handling the different structure of proofs for non-unique public key hashes.
418-426: Comprehensive error mappingThe error handling uses pattern matching to specifically handle GroveDB errors with detailed context (including proof bytes, height, and time), while letting other error types pass through the standard conversion mechanism.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-drive/src/drive/identity/fetch/queries/mod.rs (1)
235-250: Enhance documentation for clarity on non-unique key behaviorWhile the method follows the same pattern as
full_identity_with_public_key_hash_query, the documentation could be improved to better explain how it handles non-uniqueness - specifically whether it returns multiple identity matches or just one, and how theafterparameter affects pagination through results.Consider updating the documentation to:
- /// This query gets the full identity and the public key hash + /// This query gets the full identity and the non-unique public key hash + /// + /// For non-unique public key hashes, multiple identities may share the same hash. + /// The `after` parameter can be used for pagination through the results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive/src/drive/identity/fetch/queries/mod.rs(3 hunks)packages/rs-sdk/tests/fetch/identity.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Rust packages (dpp) / Tests
- GitHub Check: Rust packages (dpp) / Linting
- GitHub Check: Rust packages (dpp) / Formatting
- GitHub Check: Rust packages (dpp) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dapi-grpc) / Tests
- GitHub Check: Rust packages (dapi-grpc) / Linting
- GitHub Check: Rust packages (dapi-grpc) / Check each feature
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build JS packages / Build JS
🔇 Additional comments (2)
packages/rs-sdk/tests/fetch/identity.rs (1)
120-177: Test implementation now correctly tests the new functionalityThe test now properly validates fetching identities by non-unique public key hash, which is an improvement over the previous implementation. It correctly loops through each public key of an identity, fetches associated identities using the non-unique public key hash, and verifies that at least one identity is found.
Some suggestions for further improvement:
- Verify that the original identity is among the fetched results
- Consider adding a more descriptive comment explaining what makes public keys "non-unique"
- If possible, consider implementing a method to fetch all matching identities at once rather than using pagination in the test
let mut count = 0; + let mut found_original_identity = false; while let Some(found) = Identity::fetch(&sdk, query) .await .expect("fetch identities by non-unique key hash") { count += 1; + if found.id() == identity.id() { + found_original_identity = true; + } tracing::debug!( ?found, ?key_hash, ?count, "fetched identities by non-unique public key hash" ); query = NonUniquePublicKeyHashQuery { key_hash, after: Some(*found.id().as_bytes()), }; } assert_ne!( count, 0, "no identities found by non-unique public key hash" ); + assert!( + found_original_identity, + "original identity should be found among results" + );packages/rs-drive/src/drive/identity/fetch/queries/mod.rs (1)
94-110: Address the TODO comment on line 105The TODO comment "why not limit 1?" suggests an unresolved design question about query limiting. For non-unique public key hashes, it's important to consider whether all matching identities should be returned (current implementation) or just the first one (which would justify setting limit to 1).
The decision impacts performance and behavior. Please clarify the intended behavior - do you expect multiple identity IDs to be returned from a single non-unique public key hash, or should only the first result be returned?
Additionally, consider documenting the
afterparameter more clearly to explain its pagination purpose and expected value format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-sdk/tests/fetch/identity.rs (1)
120-181: Test implementation has been significantly improvedThe test function now correctly tests the new functionality for fetching identities by non-unique public key hash. The implementation:
- Fetches an existing identity
- Filters for non-unique public keys
- Tests each non-unique key by fetching identities associated with it
- Properly implements pagination with the
afterparameter- Verifies that at least one identity is returned for each non-unique key
This addresses the previous feedback and provides good test coverage for the new feature.
One suggestion for future improvement: consider verifying that the original identity is included in the results, which would provide additional validation of the functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/test/tokens.rs(1 hunks)packages/rs-sdk/tests/fetch/identity.rs(2 hunks)
🔇 Additional comments (1)
packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/test/tokens.rs (1)
111-114: Well implemented addition of non-unique keys for identity testingThe changes properly implement test data generation for the new feature of retrieving identities by non-unique public key hashes. By creating a single
non_unique_keywith a fixed seed and adding it to all identities, you've ensured that multiple identities will share the same public key hash, which is exactly what's needed to test the new query functionality.The code is well-commented, making the intent clear. This approach creates consistent test data that can be reliably used across test runs due to the fixed seed.
Also applies to: 122-124
lklimek
changed the base branch from
v2.0-dev
to
feat/getIdentitiesByPublicKeyHashes1
lklimek
changed the base branch from
feat/getIdentitiesByPublicKeyHashes1
to
v2.0-dev
|
replaced by #2507 |
2 participants
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit