feat(rs-sdk): identity keys query #2806
Conversation
✅ gRPC Query Coverage Report |
WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)packages/**/tests/**📄 CodeRabbit inference engine (AGENTS.md)
Files:
🪛 Biome (2.1.2)packages/rs-sdk/tests/vectors/test_identity_public_keys_specific_read/quorum_pubkey-106-21baa6993291f924461caabc476b24aa053e85963ae5c920082bc94e9c544f44.json[error] 1-1: Missing exponent Expected a digit as the exponent (parse) [error] 1-1: String values must be double quoted. (parse) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-sdk/src/platform/query.rs (1)
187-237: Well-structured query type following existing patterns.The
IdentityKeysQuerystruct and its builder methods follow the established patterns in this file consistently. The documentation is clear and includes a helpful example.Optional: Consider validating that
key_idsis not empty in the constructor to provide earlier feedback:pub fn new(identity_id: Identifier, key_ids: Vec<KeyID>) -> Self { + assert!(!key_ids.is_empty(), "key_ids cannot be empty"); Self { identity_id, key_ids, limit: None, offset: None, } }However, this validation may be intentionally deferred to the backend or a higher layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-sdk/src/platform/query.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-sdk/src/platform/query.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/src/platform/query.rs
🧠 Learnings (1)
📓 Common learnings
Learnt from: thephez
PR: dashpay/platform#2718
File: packages/wasm-sdk/index.html:0-0
Timestamp: 2025-08-05T13:55:39.147Z
Learning: The get_identity_keys_with_proof_info function in the Rust WASM bindings does not support the "search" key request type and lacks the searchPurposeMap parameter. When proof mode is enabled with keyRequestType === 'search', the implementation falls back to the non-proof version (get_identity_keys) to maintain functionality.
🧬 Code graph analysis (1)
packages/rs-sdk/src/platform/query.rs (1)
packages/dapi-grpc/clients/platform/v0/web/platform_pb.d.ts (5)
KeyRequestType(812-837)SpecificKeys(870-884)GetIdentityKeysRequest(937-952)Version(5599-5618)GetIdentityKeysRequestV0(959-991)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (2)
packages/rs-sdk/src/platform/query.rs (2)
26-26: LGTM!The new imports (
SpecificKeysandKeyID) are necessary for theIdentityKeysQueryimplementation and are placed appropriately with related imports.Also applies to: 34-34
239-264: Type compatibility verified—no issues found.The implementation correctly handles type conversion.
KeyIDis au32type alias, andSpecificKeys.key_idsexpectsVec<u32>(from the protobuf definitionrepeated uint32 key_ids). The.into_iter().collect()at line 256 correctly transforms the collection without any type inference issues.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-sdk/tests/fetch/identity.rs (1)
125-125: Consider test naming convention (optional).The coding guidelines suggest naming tests starting with "should …", but this test follows the existing
test_prefix pattern used throughout the file. While consistency with the current codebase is valuable, you may want to verify whether the guidelines apply strictly to Rust tests or are primarily for JS/TS tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/rs-sdk/tests/fetch/identity.rs(2 hunks)packages/rs-sdk/tests/vectors/test_identity_public_keys_specific_read/quorum_pubkey-106-1f0a25d463a2912cd31dd4f91b899a143d6ae990bb9bc89cb34f7c5db8b1b705.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/rs-sdk/tests/vectors/test_identity_public_keys_specific_read/quorum_pubkey-106-1f0a25d463a2912cd31dd4f91b899a143d6ae990bb9bc89cb34f7c5db8b1b705.jsonpackages/rs-sdk/tests/fetch/identity.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-sdk/tests/fetch/identity.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/tests/fetch/identity.rs
packages/**/tests/**/*.{js,ts,jsx,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
packages/**/tests/**/*.{js,ts,jsx,tsx,rs}: Name tests descriptively, starting with “should …”
Unit/integration tests must not perform network calls; mock dependencies instead
Files:
packages/rs-sdk/tests/fetch/identity.rs
🧠 Learnings (1)
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-sdk/tests/fetch/identity.rs
🧬 Code graph analysis (1)
packages/rs-sdk/tests/fetch/identity.rs (2)
packages/rs-drive-proof-verifier/src/proof.rs (2)
identity(509-513)identity(515-519)packages/rs-sdk/src/platform/types/identity.rs (8)
query(40-51)query(62-76)query(87-106)query(110-123)query(127-142)query(148-165)query(169-181)query(185-198)
🪛 Biome (2.1.2)
packages/rs-sdk/tests/vectors/test_identity_public_keys_specific_read/quorum_pubkey-106-1f0a25d463a2912cd31dd4f91b899a143d6ae990bb9bc89cb34f7c5db8b1b705.json
[error] 1-1: String values must be double quoted.
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (dash-sdk) / 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: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Build Docker images (DAPI, dapi, dapi) / Build DAPI image
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (3)
packages/rs-sdk/tests/fetch/identity.rs (2)
2-2: LGTM: Imports are appropriate.The new imports support the specific identity keys query functionality:
IdentityKeysQueryfor building the queryIdentityPublicKeysfor offline test mockingBTreeSetfor efficient membership checkingAll imports are properly used and the
cfgattribute correctly limitsIdentityPublicKeysto offline-testing scenarios.Also applies to: 8-10
126-212: LGTM: Test logic is thorough and correct.The test properly validates the specific key fetch functionality:
- Safely handles edge cases (identities with 1-3+ keys via
take(3))- Validates both presence and correctness of fetched keys
- Ensures no unrequested keys are returned
- Properly configures mocks for offline testing
The validation strategy is comprehensive, checking count, individual key correctness, and absence of unexpected keys.
packages/rs-sdk/tests/vectors/test_identity_public_keys_specific_read/quorum_pubkey-106-1f0a25d463a2912cd31dd4f91b899a143d6ae990bb9bc89cb34f7c5db8b1b705.json (1)
1-1: The file format is intentional and consistent with the project's test vector framework.The script output confirms that files named
quorum_pubkey-*.jsonthroughout the test vectors directory contain raw hex strings without JSON formatting. This is not an error but an established pattern in the codebase. Per the project's documented learnings, test vector.jsonfiles may intentionally not follow standard JSON format—this is acceptable within the testing framework.The file in question follows the same pattern as 80+ other similar test vector files and should not be modified.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/rs-sdk/tests/fetch/identity.rs (1)
123-148: Test setup and key selection logic are sound.The test follows the established patterns in this file and correctly fetches all keys before selecting a subset. The
take(3)safely handles identities with fewer keys.Minor: Redundant assertion (lines 145-148).
Since line 139 already asserts
all_public_keysis not empty, andrequested_key_idsis derived fromall_public_keys.keys(), the assertion on lines 145-148 is guaranteed to pass and can be removed.- assert!( - !requested_key_ids.is_empty(), - "expected to select at least one key id" - ); - let query = IdentityKeysQuery::new(identity_id, requested_key_ids.clone());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/rs-sdk/tests/fetch/identity.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format Rust code with cargo fmt
Run Clippy linter for Rust code
Files:
packages/rs-sdk/tests/fetch/identity.rs
packages/rs-*/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
packages/rs-*/**/*.rs: Use rustfmt defaults for Rust code (format via cargo fmt --all)
Keep Rust code clippy-clean (cargo clippy)
Name Rust modules in snake_case
Use PascalCase for Rust types
Use SCREAMING_SNAKE_CASE for Rust constants
Files:
packages/rs-sdk/tests/fetch/identity.rs
packages/**/tests/**
📄 CodeRabbit inference engine (AGENTS.md)
Place unit and integration tests alongside each package in packages//tests
Files:
packages/rs-sdk/tests/fetch/identity.rs
packages/**/tests/**/*.{js,ts,jsx,tsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
packages/**/tests/**/*.{js,ts,jsx,tsx,rs}: Name tests descriptively, starting with “should …”
Unit/integration tests must not perform network calls; mock dependencies instead
Files:
packages/rs-sdk/tests/fetch/identity.rs
🧠 Learnings (1)
📚 Learning: 2024-10-30T11:19:59.163Z
Learnt from: lklimek
PR: dashpay/platform#2277
File: packages/rs-sdk/tests/fetch/config.rs:233-233
Timestamp: 2024-10-30T11:19:59.163Z
Learning: In the Rust SDK's `rs-sdk/tests` integration tests (e.g., in `packages/rs-sdk/tests/fetch/config.rs`), we cannot create objects during tests because there is no support for object creation in this context. Therefore, hardcoded values for test identities must be used.
Applied to files:
packages/rs-sdk/tests/fetch/identity.rs
🧬 Code graph analysis (1)
packages/rs-sdk/tests/fetch/identity.rs (1)
packages/rs-drive-proof-verifier/src/proof.rs (2)
identity(509-513)identity(515-519)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Rust packages (rs-sdk-ffi) / Tests
- GitHub Check: Rust packages (wasm-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Tests
- GitHub Check: Rust packages (dash-sdk) / Check each feature
- GitHub Check: Rust packages (dash-sdk) / Unused dependencies
- GitHub Check: Rust packages (dash-sdk) / Linting
- GitHub Check: Build Docker images (Drive, drive, drive-abci) / Build Drive image
- GitHub Check: Build JS packages / Build JS
- 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: Rust crates security audit
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios-sim)
- GitHub Check: Build rs-sdk-ffi for iOS targets (aarch64-apple-ios)
- GitHub Check: Swift SDK and Example build (warnings as errors)
🔇 Additional comments (3)
packages/rs-sdk/tests/fetch/identity.rs (3)
2-2: LGTM!The new imports are necessary and properly scoped. The
IdentityKeysQuerysupports the new query functionality, and the conditionalIdentityPublicKeysimport is appropriately cfg-gated for offline testing.Also applies to: 7-10
150-169: LGTM!The query construction and offline-testing mock setup are correct. The expected subset is properly built by mapping requested key IDs to their corresponding values from the full key set, and the mock is appropriately feature-gated.
171-212: Excellent verification logic!The test performs comprehensive validation:
- Verifies count matches requested key IDs (lines 175-179)
- Confirms each fetched key matches the expected key from the full set (lines 183-199)
- Ensures no unrequested keys are included (lines 201-211)
The use of
BTreeSetfor efficient membership checks and properOptionhandling with.and_then(|value| value.as_ref())demonstrate careful implementation.
|
✅ DashSDKFFI.xcframework built for this PR.
SwiftPM (host the zip at a stable URL, then use): .binaryTarget(
name: "DashSDKFFI",
url: "https://your.cdn.example/DashSDKFFI.xcframework.zip",
checksum: "fcc475660d3654951c835eebfad324cfc07d3dec3f9a31b25dfc8dd7bd1291a0"
)Xcode manual integration:
|
3 participants
Issue being fixed or feature implemented
Fetch specific identity keys via Identity ID + Key IDs with optional limit and offset.
What was done?
Created an IdentityKeysQuery struct and implemented
queryon it.How Has This Been Tested?
DET
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Tests