include client ids in vault query #2328
Conversation
WalkthroughThe changes introduce a new property, Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/grpc_query_vault_test.go (2)
50-50: LGTM: clientIds added to "Success" test caseThe addition of
clientIds: []uint32{0, 1, 2, 3}to the "Success" test case is appropriate for testing the new functionality.Consider adding a comment explaining the significance of these specific client ID values, or if they are arbitrary, mention that in the comment. This would improve the test's readability and maintainability.
78-78: LGTM: clientIds added to "negative inventory and equity" test caseThe addition of
clientIds: []uint32{77, 88, 99}to this test case is appropriate. It tests the system with a different set of client IDs, which is good for ensuring robustness.Consider adding a comment explaining why these specific client ID values were chosen, or if they are arbitrary, mention that in the comment. This would improve the test's readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (6 hunks)
- proto/dydxprotocol/vault/query.proto (1 hunks)
- protocol/x/vault/keeper/grpc_query_vault.go (1 hunks)
- protocol/x/vault/keeper/grpc_query_vault_test.go (7 hunks)
Additional comments not posted (12)
protocol/x/vault/keeper/grpc_query_vault.go (1)
53-58: LGTM! Consider error handling and test updates.The addition of
MostRecentClientIdsaligns well with the PR objective of exposing client IDs in the vault query. The implementation looks correct and doesn't introduce breaking changes to the existing structure.However, I have a couple of suggestions:
Verify the error handling for
k.GetMostRecentClientIds(ctx, vaultId). If this method can potentially return an error, consider how to handle it appropriately.Update the relevant unit tests to cover the new
MostRecentClientIdsfield in theQueryVaultResponse.To ensure proper test coverage, let's check the existing tests:
proto/dydxprotocol/vault/query.proto (1)
92-92: LGTM! Verify related code updates.The addition of the
most_recent_client_idsfield to theQueryVaultResponsemessage is appropriate and aligns with the PR objective. The field type and numbering are correct, and it maintains backward compatibility.To ensure complete implementation, please verify that the following updates have been made:
- The server-side implementation populates this field correctly.
- Client-side code can handle and utilize this new field.
- Any relevant documentation has been updated to reflect this addition.
Run the following script to check for related code updates:
protocol/x/vault/keeper/grpc_query_vault_test.go (4)
31-32: LGTM: Addition of clientIds fieldThe addition of the
clientIds []uint32field to the test case struct aligns with the PR objective of exposing client IDs in vault queries. The field type is appropriate for storing client IDs.
65-65: LGTM: Empty clientIds added to "close only vault status" test caseThe addition of an empty
clientIdsarray ([]uint32{}) to this test case is appropriate. It ensures that the system can handle vaults with no associated client IDs, which is an important edge case to test.
Line range hint
1-311: Summary: Changes align with PR objectives and improve test coverageThe modifications to this test file effectively incorporate the new
clientIdsfield into various test cases, covering scenarios with both empty and non-empty client ID arrays. The expected response structure has been updated to includeMostRecentClientIds, ensuring that the new functionality is properly tested.These changes align well with the PR objective of exposing client IDs in vault queries. The test structure remains consistent with the existing code, maintaining readability and maintainability.
Consider addressing the minor suggestions for improved documentation of test data to enhance the test file's clarity and maintainability.
172-177: LGTM: MostRecentClientIds added to expected responseThe addition of
MostRecentClientIds: k.GetMostRecentClientIds(ctx, tc.vaultId)to the expected response structure is appropriate. It ensures that the test verifies the inclusion of the most recent client IDs in the vault query response, which aligns with the PR objective.To ensure the correctness of the
GetMostRecentClientIdsfunction, let's verify its implementation:Verification successful
LGTM: MostRecentClientIds added to expected response
The addition of
MostRecentClientIds: k.GetMostRecentClientIds(ctx, tc.vaultId)to the expected response structure is appropriately verified by existing tests that cover theGetMostRecentClientIdsfunction. This ensures that the inclusion of the most recent client IDs in the vault query response aligns with the PR objectives and is properly tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of GetMostRecentClientIds function # Test: Search for the GetMostRecentClientIds function definition rg --type go -A 10 'func.*GetMostRecentClientIds'Length of output: 2920
Script:
#!/bin/bash # Description: Search for tests covering GetMostRecentClientIds function rg --type go --glob '*_test.go' 'GetMostRecentClientIds'Length of output: 972
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (6)
54-54: New property added to QueryVaultResponse interfaceThe
mostRecentClientIdsproperty has been added to bothQueryVaultResponseandQueryVaultResponseSDKTypeinterfaces. This change aligns with the PR objective of exposing client IDs in the vault query.Also applies to: 64-64
471-472: Initialization of mostRecentClientIds in createBaseQueryVaultResponseThe
mostRecentClientIdsproperty is correctly initialized as an empty array in thecreateBaseQueryVaultResponsefunction. This ensures that the property is always defined, even if no client IDs are present.
498-504: Encoding of mostRecentClientIds in QueryVaultResponse.encodeThe encoding logic for
mostRecentClientIdshas been implemented correctly. It uses a forked writer to efficiently encode the array of client IDs.
537-549: Decoding of mostRecentClientIds in QueryVaultResponse.decodeThe decoding logic for
mostRecentClientIdshas been implemented correctly. It handles both packed and unpacked encoding formats, which is a good practice for backwards compatibility.
566-566: Partial object creation for mostRecentClientIds in QueryVaultResponse.fromPartialThe
fromPartialmethod correctly handles themostRecentClientIdsproperty, creating a new array with the provided values or an empty array if the property is undefined.
54-54: Consider documentation and backwards compatibilityThe addition of
mostRecentClientIdsto theQueryVaultResponsehas been implemented correctly across all relevant parts of the code. However, consider the following:
- Documentation: Ensure that any API documentation is updated to reflect this new property.
- Backwards Compatibility: Verify that existing clients querying vault information can handle the presence of this new field without issues.
- Usage: Consider adding comments or updating existing documentation to explain the purpose and expected content of
mostRecentClientIds.To check for potential documentation updates, you can run:
To verify backwards compatibility, ensure that existing query handlers and clients are tested with this new response format.
Also applies to: 64-64, 471-472, 498-504, 537-549, 566-566
Changelist
expose client ids in vault query so we more easily know a vault's orders
Test Plan
unit tests
localnet testing
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
MostRecentClientIds, in the vault query response, allowing retrieval of the most recent client IDs associated with a vault.Bug Fixes
Tests
MostRecentClientIdsin the vault query responses, ensuring comprehensive coverage of various scenarios.