Skip to content

feat(sdk): get identities by non-unique public key hashes (part 1)#2502

Closed
QuantumExplorer wants to merge 3 commits into
v2.0-devfrom
feat/getIdentitiesByPublicKeyHashes1
Closed

feat(sdk): get identities by non-unique public key hashes (part 1)#2502
QuantumExplorer wants to merge 3 commits into
v2.0-devfrom
feat/getIdentitiesByPublicKeyHashes1

Conversation

@QuantumExplorer
Copy link
Copy Markdown
Member

@QuantumExplorer QuantumExplorer commented Mar 17, 2025

Issue being fixed or feature implemented

Our desire is to get some identities by non unique public key hash. The way this works however is that you can only get one at a time, and then if it's not the one you want you must try to get another.

What was done?

Implements methods to get identities by non unique public key hashes.

How Has This Been Tested?

Added unit tests.

Breaking Changes

Non breaking.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
  • I have made corresponding changes to the documentation if needed

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

Summary by CodeRabbit

  • New Features

    • Introduced enhanced identity management capabilities that allow retrieval of identities using non-unique public key hashes with built-in pagination.
    • Added methods for proving identities associated with non-unique public key hashes, enhancing verification processes.
    • Streamlined naming conventions and versioning across identity-related endpoints for greater clarity and consistency.
  • Bug Fixes

    • Resolved inconsistencies in method naming related to unique and non-unique public key hashes.
  • Documentation

    • Updated documentation to reflect new features and naming conventions for identity management.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 17, 2025

Walkthrough

This update introduces new RPC methods and messaging types to support querying identities by non-unique public key hashes. In addition, several query, fetch, proof, and verification modules have been added or refactored. The changes include renaming methods and fields from "public_key_hash" to "unique_public_key_hash" to clarify their behavior, while also incorporating versioning support and error handling across components. These modifications span multiple packages, impacting both gRPC services and underlying drive and platform modules.

Changes

File(s) Change Summary
.../protos/platform/v0/platform.proto Added RPC getIdentityByNonUniquePublicKeyHash with new request/response messages for non-unique public key hash identity queries.
.../rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/(mod.rs, v0/mod.rs) Introduced query methods for non-unique public key hash identity retrieval with version checks and error handling.
.../rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/(mod.rs, v0/mod.rs) Renamed methods from query_identity_by_public_key_hash_v0 to query_identity_by_unique_public_key_hash_v0 to ensure naming consistency.
.../rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/* Added modules for fetching full identity and identity IDs via non-unique public key hash with pagination and versioning.
.../rs-drive/src/drive/identity/fetch/prove/* Introduced modules to prove full identity by non-unique public key hash and updated proof methods to reflect unique naming.
.../rs-drive/src/verify/identity/* Updated verification methods and modules by renaming functions from public_key_hash to unique_public_key_hash and adding flows for non-unique queries.
.../rs-platform-version/src/version/drive* Renamed version fields from identity_by_public_key_hash to identity_by_unique_public_key_hash and added new fields for non-unique public key hash operations.
.../tests/strategy_tests/query.rs, .../rs-drive-proof-verifier/* Modified test cases and proof verification calls to use the new unique public key hash methods.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant QueryService
    participant Platform
    participant Drive
    
    Client->>QueryService: get_identity_by_non_unique_public_key_hash(request)
    QueryService->>Platform: query_identity_by_non_unique_public_key_hash(request)
    Platform->>Drive: fetch_full_identity_by_non_unique_public_key_hash(...)
    Drive-->>Platform: identity data / error
    Platform-->>QueryService: GetIdentityByNonUniquePublicKeyHashResponse
    QueryService-->>Client: Response
Loading

Suggested labels

js-sdk

Suggested reviewers

  • shumkov

Poem

I’m a rabbit on the run,
Hopping through code under the sun.
New queries bloom like carrots fresh and neat,
Unique and non-unique – a coding treat!
With every change my heart skips a beat.
CodeRabbit leads, and I cheer in delight!
🥕🐇 Happy coding under moonlight!

Tip

⚡🧪 Multi-step agentic review comment chat (experimental)
  • We're introducing multi-step agentic chat in review comments. This experimental feature enhances review discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments.
    - To enable this feature, set early_access to true under in the settings.

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (19)
packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs (2)

11-40: Documentation needs to be updated for clarity

The method documentation still refers to "public key hash" rather than "unique public key hash", which is inconsistent with the method name and implementation changes.

 /// Verifies the full identity of a user by their public key hash.
 ///
 /// This function takes a byte slice `proof` and a 20-byte array `public_key_hash` as arguments,
 /// then it verifies the identity of the user with the given public key hash.
 ///
 /// The `proof` should contain the proof of authentication from the user.
 /// The `public_key_hash` should contain the hash of the public key of the user.
 ///
 /// 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 verify
+/// by calling `verify_identity_id_by_unique_public_key_hash()`. It then uses this identity ID to verify
 /// the full identity by calling `verify_full_identity_by_identity_id()`.

43-43: Parameter name should be updated for consistency

The parameter name is still public_key_hash while the method name now specifies "unique" public key hash. Consider updating the parameter name for consistency.

-    pub(super) fn verify_full_identity_by_unique_public_key_hash_v0(
-        proof: &[u8],
-        public_key_hash: [u8; 20],
-        platform_version: &PlatformVersion,
+    pub(super) fn verify_full_identity_by_unique_public_key_hash_v0(
+        proof: &[u8],
+        unique_public_key_hash: [u8; 20],
+        platform_version: &PlatformVersion,
packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs (3)

14-15: Consider a docstring for the v0 submodule.
While versioned modules are useful, it helps future maintainers to see a small docstring explaining the purpose of v0 (e.g., to handle the initial version of the query).


16-31: Clarify decoding failure condition.
Your error message at lines 24-29 indicates a decoding error if the version is missing, but the cause might be more specific (e.g. “missing or invalid version field in request”). Consider refining the error message for more precise debugging.


54-69: Provide usage examples or integration tests for query flow.
This function returns a QueryValidationResult that maps to a response. It would be helpful to include or link to a short snippet or test illustrating the typical usage of this method, especially covering error paths (e.g., invalid public key hash length, version out of bounds).

packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/mod.rs (2)

1-2: Add a brief doc comment for the v0 module.
The presence of a dedicated v0 submodule is clear, but a short description helps maintainers quickly see why versioning logic is separated.


3-12: Document is_proof_subset parameter usage more clearly.
While the high-level doc mentions a subset proof, it’s worth noting precisely what subset logic we’re verifying or any constraints about partial vs. full proofs. This helps a new contributor see how partial or subset proofs are expected to behave.

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-2: Add a quick doc summary to the v0 submodule.
Similar to other versioned submodules, a brief note helps maintainers see that v0 is the initial implementation for fetching identities by non-unique public key hashes.


3-9: Check for concurrency or transaction conflicts.
Though trivial usage is shown, consider whether parallel calls to fetch identities could cause race conditions with the underlying database. Review if the transaction argument or GroveDB concurrency rules require additional documentation.


26-54: Validate result from version handler.
This function properly routes to fetch_full_identity_by_non_unique_public_key_hash_v0. If you plan to add more versions soon, consider logging or tracing the chosen version. That typically helps with debugging version mismatches in production.

packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/mod.rs (1)

1-2: Add a short doc comment for the v0 submodule.
A brief explanation of what is contained in mod v0 can help future contributors quickly grasp its responsibilities.

packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs (1)

105-192: Comprehensive test coverage for invalid, missing, and absence-proof scenarios.
These tests cover critical negative paths comprehensively. Adding an explicit positive test case (with an actual valid public key hash of length 20 that resolves to a found identity) might further strengthen confidence in correctness.

packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)

69-69: Remove or replace debug print statement.
Printing the identity proof in hex can leak sensitive information into logs. Consider removing this or using a more secure logging approach.

-    println!("hex {}", hex::encode(&identity_proof));
+    // TODO: Use a secure logger or remove if not necessary for production
packages/dapi-grpc/protos/platform/v0/platform.proto (2)

36-37: Add a brief documentation comment for consistency.

It's helpful to include a short description above this RPC in the .proto file. This will clarify the purpose and usage of the getIdentityByNonUniquePublicKeyHash method, just like the other RPC methods in this interface.


625-632: Clarify start_after usage and edge cases.

The optional start_after field is useful for pagination. However, it's not entirely clear how the server behaves when start_after isn't provided or if an invalid/unknown identity is passed. Consider adding documentation or constraints to clarify its semantics.

packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/v0/mod.rs (1)

37-67: Validate partial proof usage and limit corner cases.

  1. The method sets limit = Some(1) in the query, returning a single identity. While this works for a single match, be cautious if later requirements expand to multiple results.
  2. For partial proofs (is_proof_subset = true), ensure that upstream logic handles scenario mismatches or incomplete proofs gracefully.
  3. Overall, this function is clear and idiomatic, with consistent error handling.
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)

11-29: Add unit tests for pagination parameter.

The logic for (public_key_hash, after) can introduce pagination-like behavior. Currently, there's no direct mention of test coverage for edge cases (e.g., after identity not found, or no identities). A dedicated test would strengthen reliability.

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: Updated documentation to reflect new parameters

The docstring has been updated to include the new limit parameter, but is missing documentation for the after parameter that was added to the method signature.

Add documentation for the after parameter:

/// * `limit` - An optional limit.
+/// * `after` - An optional identity ID to start after for pagination.
/// * `transaction` - Transaction arguments.
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)

46-55: Implemented pagination logic

The implementation now constructs the appropriate PathQuery based on whether an after parameter is provided, enabling effective pagination. The limit parameter is also correctly applied to the query.

However, there's a potential edge case if very large limits are provided. Consider adding validation or a maximum limit to prevent potential performance issues with extremely large result sets.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6161f75 and 8d99708.

📒 Files selected for processing (42)
  • packages/dapi-grpc/protos/platform/v0/platform.proto (2 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 (1 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/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)
⏰ Context from checks skipped due to timeout of 90000ms (5)
  • 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: JS NPM security audit
  • GitHub Check: Build JS packages / Build JS
  • GitHub Check: Determine changed packages
🔇 Additional comments (66)
packages/rs-drive/src/drive/identity/fetch/prove/mod.rs (1)

4-4: Well-structured module addition

The addition of the new module prove_full_identity_by_non_unique_public_key_hash is properly integrated into the existing module structure. This module supports the new feature for retrieving identities using non-unique public key hashes.

packages/rs-drive/src/drive/identity/mod.rs (1)

43-45: Good module documentation and feature configuration

The new module is well-documented with a clear explanation of its purpose. The feature configuration (#[cfg(any(feature = "server", feature = "verify"))]) follows the established pattern used for other modules in this file, maintaining consistency in the codebase.

packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/v1.rs (1)

70-70: Clear naming improvement for explicit semantics

Renaming the field from identity_by_public_key_hash to identity_by_unique_public_key_hash clarifies the precise behavior of this feature version. This change is part of a broader effort to distinguish between unique and non-unique public key hashes throughout the codebase, making the API more explicit and reducing potential confusion.

packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_ids_by_unique_public_key_hashes/v0/mod.rs (1)

92-92: Consistent method renaming in test code

The update to use verify_identity_ids_by_unique_public_key_hashes instead of the previous method name maintains consistency with the naming convention changes implemented across the codebase. This ensures that test code properly reflects the production code it's testing.

packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/v0/mod.rs (1)

36-36: Appropriate renaming to clarify function purpose

The function has been correctly renamed from verify_identity_id_by_public_key_hash_v0 to verify_identity_id_by_unique_public_key_hash_v0, which better describes its specific purpose of verifying identity IDs using unique public key hashes. This naming change aligns with the feature implementation for supporting non-unique public key hashes.

packages/rs-platform-version/src/version/drive_abci_versions/drive_abci_query_versions/mod.rs (1)

55-55: Consistent field renaming for clarity

The field has been properly renamed from identity_by_public_key_hash to identity_by_unique_public_key_hash in the DriveAbciQueryIdentityVersions struct. This change maintains consistency with the overall approach of clearly distinguishing between unique and non-unique public key hash functionality.

packages/rs-drive-abci/src/query/identity_based_queries/mod.rs (1)

6-7: Proper module structure for new feature implementation

The module has been appropriately restructured by replacing the single identity_by_public_key_hash module with two specialized modules: identity_by_non_unique_public_key_hash and identity_by_unique_public_key_hash. This change directly supports the PR objective of implementing identity retrieval by non-unique public key hashes while maintaining a clear separation of concerns.

packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/v0/mod.rs (2)

40-40: Function name updated for consistency

The function has been renamed from verify_identity_ids_by_public_key_hashes_v0 to verify_identity_ids_by_unique_public_key_hashes_v0, which maintains naming consistency with other related functions in the codebase. This change helps clearly distinguish the function's purpose of working with unique public key hashes.


48-48:

Details

✅ Verification successful

Verify helper method name consistency

The function appears to be calling a helper method identity_ids_by_unique_public_key_hash_query. Make sure that this helper method has also been renamed from its previous version (likely identity_ids_by_public_key_hash_query) for full consistency.


🏁 Script executed:

#!/bin/bash
# Check if the helper method has been properly renamed
grep -r "identity_ids_by_.*public_key_hash_query" packages/rs-drive/src/

Length of output: 721


Helper method name update verified

All references are consistently using identity_ids_by_unique_public_key_hash_query across the affected files:

  • In packages/rs-drive/src/drive/identity/fetch/queries/mod.rs, the helper method is declared with the correct name.
  • Invocations in both packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_ids_by_unique_public_key_hashes/v0/mod.rs and packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/v0/mod.rs use the renamed helper.

No remnants of the old naming were found. Excellent work on ensuring naming consistency!

packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/v1.rs (2)

30-30: LGTM: Adding new fetch method for non-unique public key hash support

The addition of fetch_full_identity_by_non_unique_public_key_hash with version 0 is consistent with the PR objective of implementing support for retrieving identities using non-unique public key hashes.


62-62: LGTM: Adding new prove method for non-unique public key hash support

The addition of prove_full_identity_by_non_unique_public_key_hash with version 0 complements the fetch method and provides the necessary proving capability for the non-unique public key hash feature.

packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/v0/mod.rs (1)

41-47: LGTM: Method renamed for clarity and consistency

The method name has been updated to clarify that it works specifically with unique public key hashes, which is consistent with the broader changes in this PR.

packages/rs-drive/src/drive/identity/identity_and_non_unique_public_key_hash_double_proof.rs (1)

1-21: LGTM: Well-documented struct for non-unique public key hash proofs

The new IdentityAndNonUniquePublicKeyHashDoubleProof struct is well-documented and provides a clear structure for handling the necessary proofs for verifying identities associated with non-unique public key hashes.

packages/rs-drive/src/drive/identity/fetch/fetch_by_public_key_hashes/mod.rs (2)

2-2: LGTM: Adding new module for non-unique public key hash fetch operations

The addition of fetch_full_identity_by_non_unique_public_key_hash module is consistent with the PR objective of implementing support for retrieving identities using non-unique public key hashes.


72-78: LGTM: Updated test to match new method signature

The test has been correctly updated to accommodate the new parameters in the fetch_identity_ids_by_non_unique_public_key_hash method signature and to use platform_version instead of drive_version.

packages/rs-drive-abci/tests/strategy_tests/query.rs (1)

269-274: Method renamed for clarity

The method has been renamed from verify_full_identity_by_public_key_hash to verify_full_identity_by_unique_public_key_hash to explicitly indicate that it works with unique public key hashes. This change is part of the effort to support both unique and non-unique public key hashes as mentioned in the PR title.

packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_unique_public_key_hash/v0/mod.rs (1)

90-95: Consistent method renaming in test

The test has been updated to use the renamed method verify_full_identity_by_unique_public_key_hash instead of verify_full_identity_by_public_key_hash, maintaining consistency with the changes throughout the codebase. The functionality remains the same.

packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/v0/mod.rs (4)

17-76: Method renamed to clarify its purpose

The method query_identity_by_public_key_hash_v0 has been renamed to query_identity_by_unique_public_key_hash_v0 to explicitly indicate that it works with unique public key hashes. This change improves code clarity and is consistent with the broader refactoring to support both unique and non-unique public key hash queries.


94-95: Updated method call in test

The test has been updated to call the renamed method query_identity_by_unique_public_key_hash_v0 instead of the previous name, ensuring that tests continue to pass after the refactoring.


114-115: Updated method call in test

The test has been updated to call the renamed method query_identity_by_unique_public_key_hash_v0 instead of the previous name, ensuring that tests continue to pass after the refactoring.


134-135: Updated method call in test

The test has been updated to call the renamed method query_identity_by_unique_public_key_hash_v0 instead of the previous name, ensuring that tests continue to pass after the refactoring.

packages/rs-drive/src/verify/identity/verify_identity_ids_by_unique_public_key_hashes/mod.rs (1)

39-66: Renamed method and updated references

The method verify_identity_ids_by_public_key_hashes has been renamed to verify_identity_ids_by_unique_public_key_hashes for clarity. All references to this method within the implementation have been updated accordingly, including:

  1. The method signature
  2. The method reference in the match statement
  3. The call to the versioned implementation
  4. The error message for unknown version mismatches

This change is part of the systematic renaming throughout the codebase to distinguish between unique and non-unique public key hash operations.

packages/rs-drive/src/drive/identity/fetch/prove/prove_identity_id_by_unique_public_key_hash/v0/mod.rs (1)

68-74: Method name updated for clarity

The change from verify_identity_id_by_public_key_hash to verify_identity_id_by_unique_public_key_hash properly aligns with the PR's goal of distinguishing between unique and non-unique public key hashes. This renaming maintains consistency with other similar changes across the codebase.

packages/rs-drive-abci/src/query/identity_based_queries/identity_by_unique_public_key_hash/mod.rs (3)

35-35: Identifier renamed for consistent terminology

The path identifier has been renamed from identity_by_public_key_hash to identity_by_unique_public_key_hash, which correctly reflects the feature change to distinguish between unique and non-unique public key hashes.


44-44: Error message updated to match feature name

The error message has been updated to reference identity_by_unique_public_key_hash instead of identity_by_public_key_hash, maintaining consistency with the feature renaming.


55-55: Method call updated to reflect renamed function

The method call has been updated from query_identity_by_public_key_hash_v0 to query_identity_by_unique_public_key_hash_v0, which corresponds to the renamed implementation supporting unique public key hashes.

packages/rs-drive/src/verify/identity/verify_full_identities_by_public_key_hashes/v0/mod.rs (1)

51-51: Method call updated for specificity

The method call has been updated from verify_identity_ids_by_public_key_hashes to verify_identity_ids_by_unique_public_key_hashes, correctly reflecting that this implementation handles unique public key hashes specifically.

packages/rs-platform-version/src/version/mocks/v2_test.rs (1)

203-203: Configuration field renamed for consistency

The field in the platform version configuration has been renamed from identity_by_public_key_hash to identity_by_unique_public_key_hash to maintain consistency with the feature changes across the codebase.

packages/rs-drive-proof-verifier/src/proof.rs (1)

328-328: Updated method name clarifies the intent of the operation.

The method call has been renamed from Drive::verify_full_identity_by_public_key_hash to Drive::verify_full_identity_by_unique_public_key_hash, which makes it explicit that this method specifically works with unique public key hashes.

packages/rs-platform-version/src/version/drive_versions/drive_identity_method_versions/mod.rs (2)

97-97: Added feature for fetching identities by non-unique public key hash.

This new feature version field enables the functionality to fetch full identities using non-unique public key hashes, expanding the platform's identity lookup capabilities.


136-136: Added feature for proving identities by non-unique public key hash.

This new feature version field enables the functionality to prove full identities using non-unique public key hashes, complementing the fetch capability and completing the verification process.

packages/rs-drive/src/verify/identity/verify_full_identity_by_unique_public_key_hash/mod.rs (3)

39-43: Method name now explicitly indicates it works with unique public key hashes.

The method name has been updated to be more specific about the type of public key hash it handles, improving API clarity.


51-55: Updated internal method call for consistency.

The internal versioned method call has been renamed to match the parent method name, maintaining consistency throughout the codebase.


57-57: Updated error message to reflect new method name.

The error message has been updated to use the new method name, ensuring clear and accurate error reporting.

packages/rs-drive/src/verify/identity/verify_identity_id_by_unique_public_key_hash/mod.rs (4)

36-41: Method name now explicitly indicates it works with unique public key hashes.

The method name has been updated to clearly specify that it verifies identity IDs using unique public key hashes.


47-48: Updated version path to match renamed method.

The version lookup path has been updated to reflect the renamed method, ensuring that the correct versioning is applied.


49-54: Updated internal method call for consistency.

The internal versioned method call has been renamed to match the parent method name, maintaining consistency throughout the codebase.


56-56: Updated error message to reflect new method name.

The error message has been updated to use the new method name, ensuring clear and accurate error reporting.

packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/v1.rs (2)

24-25: No issues found with these re-labeled methods.
These additions align with clarifying the identity method names for "unique" public key hashes.


31-32: Validate usage and coverage for new methods.
The newly introduced methods for "non_unique_public_key_hash" appear consistent. Make sure to add or update unit tests and integration tests to cover these new verification pathways.

packages/rs-drive-abci/src/query/service.rs (1)

415-425: Ensure test coverage for new endpoint.
This new endpoint looks good and follows the existing query pattern. Please confirm that there's a corresponding test case in the integration or functional tests to verify correctness.

packages/rs-drive/src/verify/identity/verify_full_identity_by_non_unique_public_key_hash/mod.rs (2)

16-47: Comprehensive documentation.
Your doc comments are thorough and clearly communicate the usage and parameters of this method. Great job providing context and disclaimers about unsupported versions!


53-79: Consider test coverage for version handling and error scenarios.
The version-based dispatch is clearly implemented. Ensure robust testing to simulate unsupported versions and edge cases (e.g., invalid proofs or incorrect public key hash sizes).

packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/mod.rs (1)

1-13:

Details

✅ Verification successful

Ensure consistent naming alignment for imports and usage.
All imports look correct and relevant to this new module’s functionality, but confirm that the dapi_grpc references match the finalized proto definitions for the new “non-unique public key hash” feature to avoid mismatched types or naming discrepancies.

You can run a quick search to confirm references:


🏁 Script executed:

#!/bin/bash
rg -A 3 -B 3 'GetIdentityByNonUniquePublicKeyHashRequest'

Length of output: 11763


Import Naming and Version Consistency Verified

The dapi_grpc imports in this module have been verified against the finalized proto definitions. The use of version aliases (RequestVersion and ResponseVersion) and the base types (GetIdentityByNonUniquePublicKeyHashRequest and GetIdentityByNonUniquePublicKeyHashResponse) is consistent with the versioned implementations (e.g., in the v0 subdirectory) and the proto file definitions.

  • The proto definitions in packages/dapi-grpc/protos/platform/v0/platform.proto confirm that the naming and structure align with the imports.
  • No mismatches or naming discrepancies were found in the search results.
packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/mod.rs (1)

13-66:

Details

❓ Verification inconclusive

Ensure thorough unit coverage for the verification process.
This function elegantly dispatches to verify_identity_id_by_non_unique_public_key_hash_v0 or raises an error if the version is unknown. Please confirm there’s robust test coverage for cases like invalid proof data, mismatched public key hash, or unsupported version scenarios.

Here’s a possible shell snippet to confirm coverage in tests:


🏁 Script executed:

#!/bin/bash
fd 'test_' | xargs rg 'verify_identity_id_by_non_unique_public_key_hash'

Length of output: 70


Test Coverage Verification Needed

The automated test search didn’t return any matches for references to verify_identity_id_by_non_unique_public_key_hash. Please manually verify that robust unit tests exist covering scenarios such as:

  • Invalid proof data.
  • Mismatched public key hash.
  • Unsupported version.

If these cases aren’t explicitly tested, consider adding specific unit tests to ensure complete coverage.

packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/mod.rs (2)

3-10: Imports look well-structured.
No concerns here. The added imports align with the established patterns in this codebase.


11-75:

Details

❓ Verification inconclusive

Implementation appears correct; please confirm coverage.
The version handling logic is consistent, and the function is well-documented. It would be good to verify that after behavior is tested.


🏁 Script executed:

#!/bin/bash
# Searching the codebase for references to confirm usage and test coverage
rg 'prove_full_identity_by_non_unique_public_key_hash'

Length of output: 2452


Version handling appears correct – please review the test coverage for the after parameter

The version delegation and documentation were verified by the grep results, which show that version 0 is consistently used across the codebase. However, it isn’t immediately clear from the current search output whether the specific behavior of the optional after parameter—ensuring that the identity provided in after is excluded and that retrieval starts from the subsequent identity—is explicitly tested.

  • Confirm that tests (unit or integration) verify the behavior when after is provided.
  • Ensure edge cases (e.g., when after is the last identity or when it doesn’t correspond to any valid identity) are covered.
packages/rs-drive/src/drive/identity/fetch/queries/mod.rs (2)

3-6: New references to non-unique public key hash paths.
These additions align well with the existing path structures for identity queries.


236-251: Reuses the function with the same inclusive RangeFrom.
Because this function calls identity_id_by_non_unique_public_key_hash_query, it inherits the same potential issue with RangeFrom.

packages/rs-drive/src/verify/identity/mod.rs (1)

3-4: Module separation for non-unique vs. unique public key hash verification looks good.
This approach cleanly delineates the logic for each type of public key hash.

Also applies to: 10-12

packages/rs-drive-abci/src/query/identity_based_queries/identity_by_non_unique_public_key_hash/v0/mod.rs (2)

28-33: Validate error message aligns with input size.
The error message correctly indicates a 20-byte requirement for the public_key_hash.


47-99: Streamlined branching for proof vs. direct lookup.
This conditional block cleanly handles proof case vs. non-proof case. The approach overall looks correct and maintainable.

packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/v0/mod.rs (1)

56-220: Tests validate multiple identities and pagination.
The provided tests thoroughly demonstrate scenarios with multiple matching identities and sequential lookups. The coverage is appreciated and aligns well with designed pagination logic.

packages/dapi-grpc/protos/platform/v0/platform.proto (1)

634-652: Ensure consistency with existing response patterns.

This response struct mirrors the structure of other "GetIdentityByPublicKeyHash" responses. Everything looks consistent and follows the established pattern.

packages/rs-drive/src/verify/identity/verify_identity_id_by_non_unique_public_key_hash/v0/mod.rs (1)

1-10: Imports and initial setup are clear.

These imports and module structure properly prepare for proof verification. No concerns here.

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: Confirm correct ordering and handle more than one result if needed.

  1. You fetch up to one identity ID by calling fetch_identity_ids_by_non_unique_public_key_hash_operations with a max of 1. If future requirements expand, ensure an error or additional logic is introduced when multiple identity IDs share the same public key hash.
  2. Good use of existing fetch_full_identity method for the final fetch.

Would you like a script to search for all call sites of this method and verify that multiple matches are never expected?

packages/rs-platform-version/src/version/drive_versions/drive_verify_method_versions/mod.rs (2)

38-39: Consistent renaming to clarify identity key types

Field names have been updated from verify_identity_id_by_public_key_hash to verify_identity_id_by_unique_public_key_hash (and similar for the plural version). This renaming clearly distinguishes between unique and non-unique public key hashes, which aligns with the PR objective.


45-46: New verification methods for non-unique public key hashes

These new fields appropriately extend the verification capabilities to support non-unique public key hashes, which is the main focus of this PR. The naming convention is consistent with the renamed fields above.

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-7: Updated imports to support new functionality

The added import for LowLevelDriveOperation is necessary for the new operations-tracking method, and switching to PlatformVersion aligns with the broader versioning approach across the codebase.


24-31: Added pagination support to fetch method

The method signature has been enhanced with pagination parameters (limit and after) and updated to use PlatformVersion instead of DriveVersion, which aligns with the broader refactoring across the platform.


32-46: Updated version matching to use platform version

The matching logic now correctly uses the platform_version hierarchy and passes the new pagination parameters to the implementation. This change maintains backward compatibility through versioning.


55-86: Added operations-tracking variant of the fetch method

A new method fetch_identity_ids_by_non_unique_public_key_hash_operations has been added that follows the same pattern as the original method but with additional tracking of low-level operations. This is a common pattern in this codebase to support fee calculations and other operational needs.

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-12: Updated import to use PlatformVersion

The import has been correctly updated to use PlatformVersion instead of the previous DriveVersion, which aligns with changes in the parent module.


16-23: Updated v0 implementation method signature

The v0 implementation method signature has been updated to include the new pagination parameters and use PlatformVersion, keeping it consistent with the parent method.


35-44: Updated operations method signature

The operations variant of the implementation method has also been updated with the new pagination parameters and to use PlatformVersion, maintaining consistency across the codebase.


61-62: Updated to use platform_version.drive

The code now correctly uses platform_version.drive to access the drive configuration, aligning with the changes in parameter types.

Comment on lines +32 to +52
let feature_version_bounds = &platform_version
.drive_abci
.query
.identity_based_queries
.identity_by_unique_public_key_hash;

let feature_version = match &version {
RequestVersion::V0(_) => 0,
};

if !feature_version_bounds.check_version(feature_version) {
return Ok(QueryValidationResult::new_with_error(
QueryError::UnsupportedQueryVersion(
"identity_by_non_unique_public_key_hash".to_string(),
feature_version_bounds.min_version,
feature_version_bounds.max_version,
platform_version.protocol_version,
feature_version,
),
));
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix mismatch in naming for feature version field.
Here, you reference identity_by_unique_public_key_hash (line 36) while we are in a file specifically for non-unique public key hashes. This can be confusing and could lead to errors if the underlying version bounds differ. Confirm you’re referencing the correct feature.

- .identity_by_unique_public_key_hash;
+ .identity_by_non_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.

Suggested change
let feature_version_bounds = &platform_version
.drive_abci
.query
.identity_based_queries
.identity_by_unique_public_key_hash;
let feature_version = match &version {
RequestVersion::V0(_) => 0,
};
if !feature_version_bounds.check_version(feature_version) {
return Ok(QueryValidationResult::new_with_error(
QueryError::UnsupportedQueryVersion(
"identity_by_non_unique_public_key_hash".to_string(),
feature_version_bounds.min_version,
feature_version_bounds.max_version,
platform_version.protocol_version,
feature_version,
),
));
}
let feature_version_bounds = &platform_version
.drive_abci
.query
.identity_based_queries
- .identity_by_unique_public_key_hash;
+ .identity_by_non_unique_public_key_hash;
let feature_version = match &version {
RequestVersion::V0(_) => 0,
};
if !feature_version_bounds.check_version(feature_version) {
return Ok(QueryValidationResult::new_with_error(
QueryError::UnsupportedQueryVersion(
"identity_by_non_unique_public_key_hash".to_string(),
feature_version_bounds.min_version,
feature_version_bounds.max_version,
platform_version.protocol_version,
feature_version,
),
));
}

Comment on lines +10 to +25
impl Drive {
/// Fetches an identity with all its related information from storage based on a non unique public key hash.
///
/// This function leverages the versioning system to direct the fetch operation to the appropriate handler based on the `DriveVersion` provided.
///
/// # Arguments
///
/// * `public_key_hash` - A non-unique public key hash corresponding to the identity to be fetched.
/// * `after` - An identity ID after which we want to get back our identity. Basically "don't get back this identity, get the next one"
/// * `transaction` - Transaction arguments.
/// * `drive_version` - A reference to the drive version.
///
/// # Returns
///
/// Returns a `Result` containing an `Option` of the `Identity` if it exists, otherwise an `Error` if the fetch operation fails or the version is not supported.
pub fn fetch_full_identity_by_non_unique_public_key_hash(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Synchronize doc comment references.
The docstring references “drive_version” while the function signature expects platform_version. Update the doc lines (lines 13, 20) to reflect platform_version for clarity and consistency.

- /// * `drive_version` - A reference to the drive version.
+ /// * `platform_version` - A reference to the platform version.
📝 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.

Suggested change
impl Drive {
/// Fetches an identity with all its related information from storage based on a non unique public key hash.
///
/// This function leverages the versioning system to direct the fetch operation to the appropriate handler based on the `DriveVersion` provided.
///
/// # Arguments
///
/// * `public_key_hash` - A non-unique public key hash corresponding to the identity to be fetched.
/// * `after` - An identity ID after which we want to get back our identity. Basically "don't get back this identity, get the next one"
/// * `transaction` - Transaction arguments.
/// * `drive_version` - A reference to the drive version.
///
/// # Returns
///
/// Returns a `Result` containing an `Option` of the `Identity` if it exists, otherwise an `Error` if the fetch operation fails or the version is not supported.
pub fn fetch_full_identity_by_non_unique_public_key_hash(
impl Drive {
/// Fetches an identity with all its related information from storage based on a non unique public key hash.
///
/// This function leverages the versioning system to direct the fetch operation to the appropriate handler based on the `DriveVersion` provided.
///
/// # Arguments
///
/// * `public_key_hash` - A non-unique public key hash corresponding to the identity to be fetched.
/// * `after` - An identity ID after which we want to get back our identity. Basically "don't get back this identity, get the next one"
/// * `transaction` - Transaction arguments.
/// * `platform_version` - A reference to the platform version.
///
/// # Returns
///
/// Returns a `Result` containing an `Option` of the `Identity` if it exists, otherwise an `Error` if the fetch operation fails or the version is not supported.
pub fn fetch_full_identity_by_non_unique_public_key_hash(

Comment on lines +96 to +112
/// The query for proving an identity id from a non-unique public key hash.
/// This should be used for absence proofs
pub fn identity_id_by_non_unique_public_key_hash_query(
public_key_hash: [u8; 20],
after: Option<[u8; 32]>,
) -> PathQuery {
let non_unique_key_hashes = non_unique_key_hashes_tree_path_vec();
let mut query = Query::new_single_key(public_key_hash.to_vec());
let sub_query = if let Some(after) = after {
Query::new_single_query_item(QueryItem::RangeFrom(after.to_vec()..))
} else {
Query::new_range_full()
};
query.set_subquery(sub_query);
PathQuery::new(non_unique_key_hashes, SizedQuery::new(query, None, None))
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Possible off-by-one error when skipping the specified identity.
The comments indicate the identity matching after should be excluded, but QueryItem::RangeFrom is inclusive. Consider switching to RangeAfter to ensure the identity isn’t included.

- Query::new_single_query_item(QueryItem::RangeFrom(after.to_vec()..))
+ Query::new_single_query_item(QueryItem::RangeAfter(after.to_vec()..))
📝 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.

Suggested change
/// The query for proving an identity id from a non-unique public key hash.
/// This should be used for absence proofs
pub fn identity_id_by_non_unique_public_key_hash_query(
public_key_hash: [u8; 20],
after: Option<[u8; 32]>,
) -> PathQuery {
let non_unique_key_hashes = non_unique_key_hashes_tree_path_vec();
let mut query = Query::new_single_key(public_key_hash.to_vec());
let sub_query = if let Some(after) = after {
Query::new_single_query_item(QueryItem::RangeFrom(after.to_vec()..))
} else {
Query::new_range_full()
};
query.set_subquery(sub_query);
PathQuery::new(non_unique_key_hashes, SizedQuery::new(query, None, None))
}
/// The query for proving an identity id from a non-unique public key hash.
/// This should be used for absence proofs
pub fn identity_id_by_non_unique_public_key_hash_query(
public_key_hash: [u8; 20],
after: Option<[u8; 32]>,
) -> PathQuery {
let non_unique_key_hashes = non_unique_key_hashes_tree_path_vec();
let mut query = Query::new_single_key(public_key_hash.to_vec());
let sub_query = if let Some(after) = after {
- Query::new_single_query_item(QueryItem::RangeFrom(after.to_vec()..))
+ Query::new_single_query_item(QueryItem::RangeAfter(after.to_vec()..))
} else {
Query::new_range_full()
};
query.set_subquery(sub_query);
PathQuery::new(non_unique_key_hashes, SizedQuery::new(query, None, None))
}

Comment on lines +35 to +45
let start_after = if let Some(start_after) = start_after {
Some(check_validation_result_with_data!(Bytes32::from_vec(
start_after
)
.map(|bytes| bytes.0)
.map_err(|_| QueryError::InvalidArgument(
"public key hash must be 20 bytes long".to_string()
))))
} else {
None
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix misleading error message for start_after validation.
When parsing start_after as a 32-byte value, the error message still references "20 bytes." This can lead to confusion.

Consider applying the following fix:

-            .map_err(|_| QueryError::InvalidArgument(
-                "public key hash must be 20 bytes long".to_string()
-            ))
+            .map_err(|_| QueryError::InvalidArgument(
+                "start_after must be 32 bytes long".to_string()
+            ))
📝 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.

Suggested change
let start_after = if let Some(start_after) = start_after {
Some(check_validation_result_with_data!(Bytes32::from_vec(
start_after
)
.map(|bytes| bytes.0)
.map_err(|_| QueryError::InvalidArgument(
"public key hash must be 20 bytes long".to_string()
))))
} else {
None
};
let start_after = if let Some(start_after) = start_after {
Some(check_validation_result_with_data!(Bytes32::from_vec(
start_after
)
.map(|bytes| bytes.0)
.map_err(|_| QueryError::InvalidArgument(
"start_after must be 32 bytes long".to_string()
))))
} else {
None
};

Comment on lines +28 to +29
Self::identity_id_by_non_unique_public_key_hash_query(public_key_hash, after);
path_query.query.limit = Some(1);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Clarify the reason for limit=1.
Limiting identity retrieval to only one identity may be intentional, but clarifying in docs or naming helps reflect why only the first match is returned for a non-unique key. If multiple identities share the same key, ensure the caller knows subsequent identities will be excluded.

Consider verifying usage of this method across the codebase to confirm only the first identity is required. You can run a search:


🏁 Script executed:

#!/bin/bash
rg -A 5 'prove_full_identity_by_non_unique_public_key_hash'

Length of output: 10735


Clarify the intentional use of limit=1 when fetching identities

The call to

Self::identity_id_by_non_unique_public_key_hash_query(public_key_hash, after);
path_query.query.limit = Some(1);

limits the query result to a single identity. Our codebase-wide search confirms that all usages of prove_full_identity_by_non_unique_public_key_hash consistently rely on returning only the first match—even though multiple identities could potentially share the same public key hash.

To improve clarity and maintainability, please update the documentation and/or inline comments to explain that:

  • The method intentionally returns only one identity despite the public key hash being non-unique.
  • Callers must be aware that if multiple identities exist with the same key, only the first one is returned.

File: packages/rs-drive/src/drive/identity/fetch/prove/prove_full_identity_by_non_unique_public_key_hash/v0/mod.rs (lines 28–29)

@thephez thephez added the dapi-endpoint DAPI endpoint addition or modification label Mar 17, 2025
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
packages/dapi-grpc/protos/platform/v0/platform.proto (2)

625-632: Ensure Request Message Consistency and Clear Pagination Semantics
The new GetIdentityByNonUniquePublicKeyHashRequest message follows the established versioning pattern with its nested V0 message and includes the expected fields:

  • public_key_hash: the key hash to query by,
  • start_after: an optional field to support pagination (returning one result after a previous result), and
  • prove: for requesting a proof.

Please verify that the semantics of start_after align with the intended pagination behavior across the system and consider adding a more detailed comment or documentation reference if necessary.


634-652: Clarify Response Structure and Document the Dual-Proof Approach
The GetIdentityByNonUniquePublicKeyHashResponse message introduces two nested response types via a oneof:

  • IdentityResponse (providing the optional identity bytes), and
  • IdentityProvedResponse which contains:
    • grovedb_identity_public_key_hash_proof (of type Proof), and
    • identity_proof_bytes (an additional optional bytes field, noted as "A hack, we return 2 proofs").

It would be beneficial to clarify this “hack” by either updating the inline comment or by adding documentation that explains why two proofs are returned and how clients should decide which branch to use. This will help ensure the API’s usage is unambiguous for developers consuming these changes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d99708 and 61ad58a.

📒 Files selected for processing (1)
  • packages/dapi-grpc/protos/platform/v0/platform.proto (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (14)
  • GitHub Check: Rust packages (rs-dapi-client) / Formatting
  • GitHub Check: Rust packages (drive-abci) / Linting
  • GitHub Check: Rust packages (drive-abci) / Check each feature
  • GitHub Check: Rust packages (drive-abci) / Unused dependencies
  • GitHub Check: Rust packages (drive-abci) / Formatting
  • GitHub Check: Rust packages (drive) / Unused dependencies
  • GitHub Check: Rust packages (drive) / Linting
  • GitHub Check: Rust packages (drive) / Tests
  • GitHub Check: Rust packages (wasm-dpp) / Formatting
  • GitHub Check: Rust packages (wasm-dpp) / Linting
  • GitHub Check: Rust packages (dpp) / Check each feature
  • GitHub Check: Rust packages (dpp) / Linting
  • GitHub Check: Rust packages (dpp) / Tests
  • GitHub Check: Build JS packages / Build JS

@QuantumExplorer
Copy link
Copy Markdown
Member Author

replaced by #2507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dapi-endpoint DAPI endpoint addition or modification

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants