[TRA-111] add vault query #1273
Conversation
|
Warning Rate Limit Exceeded@tqin7 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 51 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update introduces a new functionality for querying specific vaults by type and number across several components of the dYdX protocol. It encompasses adding necessary imports, defining new message types for the request and response, and implementing the query logic in both the command-line interface and the keeper package. Additionally, tests are added to ensure the new querying capability behaves as expected, verifying the retrieval of detailed vault information including ownership shares and vault metrics. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/vault/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (4)
- proto/dydxprotocol/vault/query.proto (3 hunks)
- protocol/x/vault/client/cli/query.go (3 hunks)
- protocol/x/vault/keeper/grpc_query_vault.go (1 hunks)
- protocol/x/vault/keeper/grpc_query_vault_test.go (1 hunks)
Path instructions used (4)
proto/dydxprotocol/vault/query.proto (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/client/cli/query.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/grpc_query_vault.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/grpc_query_vault_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments (6)
proto/dydxprotocol/vault/query.proto (3)
- 6-8: The addition of imports for
dydxprotocol/subaccounts/subaccount.protoanddydxprotocol/vault/vault.protois appropriate for the new functionality being introduced. These imports are necessary for the new message types and RPC method that facilitate querying vaults by type and number.- 18-21: The new RPC method
Vaultwithin theQueryservice is well-defined, with a clear HTTP GET path specified. This method allows querying a vault by its type and number, which aligns with the PR's objective to enhance querying capabilities. Ensure that the specified path/dydxprotocol/v4/vault/vaults/{type}/{number}is correctly implemented in the server-side routing to handle these requests.Verification successful
The verification process successfully identified the specified HTTP GET path
/dydxprotocol/v4/vault/vaults/{type}/{number}within theproto/dydxprotocol/vault/query.protofile, as intended in the review comment. This confirms that the new RPC methodVaultis correctly defined with the intended path in the proto file. No issues were found regarding the definition of the RPC method and its path in the server-side routing.* 32-52: The new message types `QueryVaultRequest`, `QueryVaultResponse`, and `OwnerShares` are well-defined and align with the functionality required for querying vaults. These message types are crucial for the request-response cycle of the new `Vault` RPC method. It's important to ensure that these message types are used consistently across the implementation, particularly in the keeper and CLI command implementations.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the server-side routing for the new RPC method rg --type proto "/dydxprotocol/v4/vault/vaults/{type}/{number}"Length of output: 224
Script:
#!/bin/bash # Verify the server-side routing for the new RPC method with corrected regex pattern rg --type proto '/dydxprotocol/v4/vault/vaults/\{type\}/\{number\}'Length of output: 99
Script:
#!/bin/bash # Verify the server-side routing for the new RPC method without using the --type filter and specifying .proto extension rg '/dydxprotocol/v4/vault/vaults/\{type\}/\{number\}' --glob '*.proto'Length of output: 194
protocol/x/vault/client/cli/query.go (1)
- 55-98: The implementation of
CmdQueryVault()is comprehensive, covering the parsing of command-line arguments for vault type and number, and the querying logic to interact with the protocol's querying service. A few points to consider:
- The switch statement for parsing the vault type currently only supports the "clob" type. If the protocol plans to support more vault types in the future, consider making this parsing more extensible.
- Error messages are clear and informative, which is good practice for CLI tools.
- The use of
strconv.ParseUintfor parsing the vault number is appropriate. Ensure that the vault number's range is within acceptable limits for the protocol.Overall, the command is well-implemented and aligns with the PR's objectives.
protocol/x/vault/keeper/grpc_query_vault.go (1)
- 17-77: The
Vaultfunction in the keeper package is well-structured and covers several key aspects of vault querying, including total shares, owner shares, vault equity, and vault inventory. Points to consider:
- The error handling is comprehensive, with clear error messages for different failure scenarios.
- The use of
storetypes.KVStorePrefixIteratorfor iterating over owner shares is appropriate. Ensure that the key format used for owner shares is consistent across the protocol.- Sorting owner shares by the number of shares is a useful feature for clients. Consider documenting this behavior in the API documentation.
- The retrieval of vault inventory involves casting to a specific
ClobPairmetadata type. Ensure that this casting is safe and that theClobPairstructure is guaranteed to have the expected metadata format.Overall, the function is well-implemented and aligns with the PR's objectives.
protocol/x/vault/keeper/grpc_query_vault_test.go (1)
- 17-156: The test cases for the
Vaultfunction are comprehensive and cover various scenarios, including successful querying, querying a non-existent vault, and handling a nil request. Points to consider:
- The setup for each test case is thorough, including setting total shares and owner shares. This ensures that the function is tested under realistic conditions.
- The use of
require.NoErrorandrequire.ErrorContainsfor assertions is appropriate and ensures that the function's error handling is correctly implemented.- The assertions for the expected response in successful cases are detailed, checking each field of the
QueryVaultResponse. This thoroughness is commendable.Overall, the test cases are well-implemented and provide good coverage for the new querying functionality.
proto/dydxprotocol/vault/query.proto
Outdated
| uint64 equity = 3; | ||
| uint64 inventory = 4; | ||
| uint64 total_shares = 5; | ||
| repeated OwnerShares all_owner_shares = 6 [ (gogoproto.nullable) = false ]; |
There was a problem hiding this comment.
nit: Might be better to have a separate paginated query for this considering there may be many owners.
There was a problem hiding this comment.
makes sense. created a ticket to add a separate query for owner shares. let me remove owner shares for now
There was a problem hiding this comment.
Non-blocking, I see that the owner shares are stored as only integers, and not a struct with both the owner + the shares so it won't be as simple as using query.Paginate on the store.
There was a problem hiding this comment.
Yep will add a separate query for owner shares in phase 2
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/vault/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (7)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (2 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (3 hunks)
- proto/dydxprotocol/vault/query.proto (3 hunks)
- protocol/x/vault/client/cli/query.go (3 hunks)
- protocol/x/vault/keeper/grpc_query_vault.go (1 hunks)
- protocol/x/vault/keeper/grpc_query_vault_test.go (1 hunks)
Path instructions used (3)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Files skipped from review as they are similar to previous changes (4)
- proto/dydxprotocol/vault/query.proto
- protocol/x/vault/client/cli/query.go
- protocol/x/vault/keeper/grpc_query_vault.go
- protocol/x/vault/keeper/grpc_query_vault_test.go
Additional comments (7)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (1)
- 25-27: The implementation of the
vaultmethod in theLCDQueryClientclass looks good. It correctly constructs the endpoint URL dynamically and uses thereq.getmethod to perform the query.However, consider adding a comment about error handling practices for this method, especially if there are specific expectations for how errors should be managed by the calling code or if there's any fallback mechanism within
req.get.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (3)
- 10-12: The addition of the
vaultmethod to theQueryinterface is correctly implemented, with appropriate request and response types specified.- 29-33: The implementation of the
vaultmethod in theQueryClientImplclass follows the established pattern for RPC calls, correctly usingencodeanddecodefor handling the request and response.Consider adding a comment about error handling practices for this method, especially regarding how errors from the RPC request are expected to be managed.
- 44-45: The update to the
createRpcQueryExtensionfunction to include thevaultmethod is correctly implemented, aligning with the design of extending the querying capabilities of the protocol.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (3)
- 1-3: The new imports for
VaultType,VaultId, andSubaccountIdare correctly referenced and necessary for the types used in the updated and new interfaces.- 22-65: The updates to the
QueryVaultRequestandQueryVaultResponseinterfaces, along with the addition of theOwnerSharesinterface, are correctly implemented. These changes are logically structured and use appropriate types for the new fields.Consider adding comments to the new fields in these interfaces for better clarity and documentation, especially for fields like
equity,inventory, andallOwnerSharesin theQueryVaultResponseinterface.
- 146-348: The encoding and decoding functions for
QueryVaultRequest,QueryVaultResponse, andOwnerSharesare correctly implemented, following the established pattern for handling protobuf data. They correctly manage the new fields and are consistent with the usage of protobufjs.
Changelist
add vault query
Test Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.