Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add megavault owner shares query #2323

Merged
merged 2 commits into from
Sep 23, 2024
Merged

add megavault owner shares query #2323

merged 2 commits into from
Sep 23, 2024

Conversation

tqin7
Copy link
Contributor

@tqin7 tqin7 commented Sep 23, 2024

Changelist

query for owner shares, locked shares, their proportion of equity, and how much is withdrawable

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Introduced a new method to query all owner shares of a megavault, enhancing data retrieval capabilities.
  • Improvements
    • Updated existing methods to require specific parameters and provide more detailed responses regarding owner shares and equity.
  • Bug Fixes
    • Enhanced error handling for non-existent shares and improved response structures for queries.
  • Tests
    • Expanded test coverage for querying owner shares with new scenarios and request types.

@tqin7 tqin7 requested a review from a team as a code owner September 23, 2024 19:11
Copy link

linear bot commented Sep 23, 2024

Copy link
Contributor

coderabbitai bot commented Sep 23, 2024

Walkthrough

This pull request enhances the querying capabilities for megavault owner shares within the dydxprotocol codebase. It introduces a new method, megavaultAllOwnerShares, to retrieve all owner shares, while updating existing methods to enforce stricter parameter requirements. The query interfaces and corresponding protobuf definitions are modified to include new fields and structures, allowing for more detailed responses.

Changes

File Change Summary
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts - Added method megavaultAllOwnerShares to query all owner shares.
- Updated megavaultOwnerShares to require a QueryMegavaultOwnerSharesRequest parameter.
proto/dydxprotocol/vault/query.proto - Added RPC method MegavaultAllOwnerShares with corresponding request and response messages.
- Updated existing messages to include new fields.

Possibly related PRs

  • add megavault withdrawal info query #2316: This PR adds a new method to the LCDQueryClient class for querying withdrawal information for a megavault, which is directly related to the changes made in the main PR that also involves modifications to the LCDQueryClient class and introduces new methods for querying megavault-related data.

Suggested reviewers

  • vincentwschau

Poem

In the vault where shares do play,
A rabbit hops and leaps all day.
New queries bloom, like flowers bright,
Owner shares now shine with light.
With equity and unlocks galore,
Hooray for changes, we all adore! 🐇✨


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?

Share
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 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
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: 7

Outside diff range and nitpick comments (7)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (2)

63-74: LGTM with a minor suggestion: megavaultOwnerShares method updated correctly

The megavaultOwnerShares method has been updated to require a QueryMegavaultOwnerSharesRequest parameter, which is a good practice for ensuring proper input. The new logic correctly handles the address parameter if provided.

Consider using optional chaining for a more concise check:

if (params?.address != null) {
  options.params.address = params.address;
}

This change would make the code slightly more readable and consistent with modern TypeScript practices.


Line range hint 75-90: LGTM with a minor suggestion: New megavaultAllOwnerShares method added correctly

The new megavaultAllOwnerShares method is well-implemented and follows the existing patterns in the class. It correctly handles pagination parameters and uses the appropriate endpoint and return type.

For consistency with other methods in the class, consider moving the pagination: undefined default value to the method body:

async megavaultAllOwnerShares(params: QueryMegavaultAllOwnerSharesRequest = {}): Promise<QueryMegavaultAllOwnerSharesResponseSDKType> {
  const options: any = {
    params: {}
  };

  if (typeof params?.pagination !== "undefined") {
    setPaginationParams(options, params.pagination);
  }

  const endpoint = `dydxprotocol/vault/megavault/all_owner_shares`;
  return await this.req.get<QueryMegavaultAllOwnerSharesResponseSDKType>(endpoint, options);
}

This change would make the method signature more consistent with others in the class while maintaining the same functionality.

protocol/x/vault/client/cli/query.go (2)

173-177: LGTM: Function updated to query all owner shares.

The changes correctly update the function to query all owner shares instead of a specific owner's shares, which aligns with the PR objective. The request type and query client method have been appropriately updated.

Consider updating the function name from CmdQueryListOwnerShares to CmdQueryListAllOwnerShares to more accurately reflect its new functionality. This would improve code readability and maintain consistency with the new query name.


226-251: LGTM: New function added to query owner shares by address.

The new CmdQueryOwnerShares function is well-implemented and follows the existing pattern for CLI commands. It correctly uses the MegavaultOwnerShares method from the query client and handles input validation and error checking appropriately.

Consider adding a brief comment above the function to describe its purpose and expected input, similar to other functions in this file. For example:

// CmdQueryOwnerShares returns the command to query megavault owner shares for a specific address.
func CmdQueryOwnerShares() *cobra.Command {
    // ... (existing implementation)
}

This would improve code documentation and maintain consistency with other functions in the file.

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (2)

21-24: LGTM: Query interface updated correctly with a minor suggestion.

The changes to the Query interface look good:

  1. The megavaultOwnerShares method now requires a request parameter, improving type safety.
  2. The new megavaultAllOwnerShares method is properly added with clear documentation.

Consider adding a brief comment explaining the difference between megavaultOwnerShares and megavaultAllOwnerShares to clarify their distinct purposes.


73-85: LGTM: QueryClientImpl methods implemented correctly with a minor suggestion.

The changes to the QueryClientImpl class methods look good:

  1. The megavaultOwnerShares method now correctly requires a request parameter.
  2. The new megavaultAllOwnerShares method is implemented consistently with other methods in the class.
  3. Both methods use the correct request and response types, and the RPC calls are properly set up.

Consider adding error handling for the RPC calls. For example, you could wrap the promise.then() calls in a try-catch block to handle any potential errors during decoding or network issues.

protocol/x/vault/keeper/grpc_query_shares.go (1)

49-52: Enhance error handling with more context

When fetching megavaultEquity, an error is returned if the operation fails:

megavaultEquity, err := k.GetMegavaultEquity(ctx)
if err != nil {
    return nil, status.Error(codes.Internal, err.Error())
}

Currently, the error returned to the client might not provide enough context about the failure.

Consider wrapping the error with more descriptive information to aid in debugging:

if err != nil {
    return nil, status.Errorf(codes.Internal, "failed to get megavault equity: %v", err)
}
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ebf8fe4 and 90be117.

Files ignored due to path filters (2)
  • protocol/x/vault/types/query.pb.go is excluded by !**/*.pb.go
  • protocol/x/vault/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
Files selected for processing (7)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (5 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (7 hunks)
  • proto/dydxprotocol/vault/query.proto (3 hunks)
  • protocol/x/vault/client/cli/query.go (3 hunks)
  • protocol/x/vault/keeper/grpc_query_shares.go (3 hunks)
  • protocol/x/vault/keeper/grpc_query_shares_test.go (5 hunks)
Additional context used
buf
proto/dydxprotocol/vault/query.proto

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

Additional comments not posted (17)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (3)

3-3: LGTM: Import statement updated correctly

The import statement has been properly updated to include all the necessary types for the new and modified methods. This change is consistent with the modifications made in the file.


18-18: LGTM: New method binding added correctly

The megavaultAllOwnerShares method is properly bound in the constructor, following the existing pattern for method bindings. This change is consistent with the addition of the new method in the class.


Line range hint 1-90: Overall assessment: Well-implemented changes with minor suggestions

The changes in this file successfully implement the new megavaultAllOwnerShares query and update the megavaultOwnerShares query as described in the PR objectives. The code quality is good, and the changes are consistent with the existing codebase.

A few minor suggestions have been made for improved readability and consistency, but these are not critical issues. The new functionality appears to be correctly implemented and follows the established patterns in the LCDQueryClient class.

protocol/x/vault/client/cli/query.go (2)

33-33: LGTM: New command added correctly.

The addition of CmdQueryOwnerShares() to the command list is consistent with the existing pattern and aligns with the PR objective of adding a new query for megavault owner shares.


Line range hint 1-251: Overall assessment: Changes are well-implemented and align with PR objectives.

The modifications to this file successfully introduce the new query for megavault owner shares while maintaining consistency with existing code patterns. The changes enhance the CLI's querying capabilities as intended. Minor suggestions for improvements have been made to further increase code clarity and consistency.

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (4)

4-4: LGTM: Import statement updated correctly.

The import statement has been properly updated to include the new types QueryMegavaultAllOwnerSharesRequest and QueryMegavaultAllOwnerSharesResponse. This change is consistent with the addition of the new megavaultAllOwnerShares method.


42-42: LGTM: Constructor updated correctly.

The megavaultAllOwnerShares method is properly bound in the QueryClientImpl constructor. This ensures that the new method will work correctly when called on an instance of QueryClientImpl.


120-126: LGTM: createRpcQueryExtension function updated correctly.

The changes to the createRpcQueryExtension function are consistent with the updates to the Query interface and QueryClientImpl class:

  1. The megavaultOwnerShares method now correctly requires a request parameter.
  2. The new megavaultAllOwnerShares method is properly added and implemented.

These changes ensure that the RPC query extension correctly exposes the updated and new methods.


Line range hint 1-137: Overall assessment: Well-implemented changes with minor suggestions for improvement.

The changes in this file successfully introduce the new megavaultAllOwnerShares query and update the megavaultOwnerShares query. The modifications are consistent across the Query interface, QueryClientImpl class, and createRpcQueryExtension function. These changes enhance the querying capabilities for megavault owner shares.

Key points:

  1. New megavaultAllOwnerShares method added to query all owner shares.
  2. megavaultOwnerShares method updated to require a request parameter, improving type safety.
  3. Proper implementation and binding of new and updated methods in QueryClientImpl.
  4. Consistent updates in the createRpcQueryExtension function.

Suggestions for further improvement:

  1. Add brief comments to clarify the distinction between megavaultOwnerShares and megavaultAllOwnerShares.
  2. Consider implementing error handling for RPC calls in the QueryClientImpl methods.

These changes align well with the PR objectives of introducing a new query related to megavault owner shares.

protocol/x/vault/keeper/grpc_query_shares.go (2)

55-62: Verify mathematical correctness of equity calculations

The calculations for ownerEquity and ownerWithdrawableEquity involve multiple big integer operations. It's important to verify that these computations accurately reflect the intended financial logic.

Consider writing unit tests to cover scenarios with various share amounts, including edge cases like zero shares and maximum possible shares. This will help ensure that the calculations are correct and handle all possible inputs appropriately.


Line range hint 72-101: Well-implemented MegavaultAllOwnerShares method

The MegavaultAllOwnerShares function is effectively implemented, handling pagination correctly and ensuring robust error handling during the iteration over the store.

proto/dydxprotocol/vault/query.proto (1)

39-44: LGTM: Addition of new RPC method 'MegavaultAllOwnerShares'

The new RPC method MegavaultAllOwnerShares is correctly defined with appropriate request and response messages, and the HTTP GET endpoint is properly specified.

protocol/x/vault/keeper/grpc_query_shares_test.go (2)

Line range hint 17-98: Test function TestMegavaultAllOwnerShares updated correctly with comprehensive test cases

The updated function TestMegavaultAllOwnerShares correctly reflects the new request type and includes thorough test cases for different scenarios, such as success with owners, success with no owners, and error handling for nil requests.


100-265: New test function TestMegavaultOwnerShares adds comprehensive coverage

The newly added TestMegavaultOwnerShares function provides extensive test cases covering various scenarios, including:

  • Success cases with zero, one, and two share unlocks.
  • Error cases for non-existent owners and nil requests.

This enhances the robustness of the testing suite by ensuring the MegavaultOwnerShares query behaves as expected under different conditions.

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (3)

144-160: Enhancement: Added detailed fields to QueryMegavaultOwnerSharesResponse.

The addition of fields like address, shares, shareUnlocks, equity, and withdrawableEquity enriches the response data, providing more comprehensive information about the owner's shares in the megavault.


186-201: Introduction of QueryMegavaultAllOwnerSharesRequest and Response interfaces.

The new interfaces facilitate querying all owner shares with pagination support, enhancing the querying capabilities for the megavault.


128-128: Update usages to include the required address field in QueryMegavaultOwnerSharesRequest.

The QueryMegavaultOwnerSharesRequest interface now requires an address field. Please ensure that all code instances creating this request provide the address parameter, replacing any prior usage of pagination.

Run the following script to identify any instances that may need updating:

Also applies to: 136-136

protocol/x/vault/keeper/grpc_query_shares.go Show resolved Hide resolved
protocol/x/vault/keeper/grpc_query_shares.go Show resolved Hide resolved
Comment on lines +43 to +55
shareUnlocks, exists := k.GetOwnerShareUnlocks(ctx, req.Address)
totalLockedShares := big.NewInt(0)
if exists {
totalLockedShares = shareUnlocks.GetTotalLockedShares()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential nil dereference of shareUnlocks

At lines 43-47, if exists is false, shareUnlocks will be uninitialized or nil. However, at line 66, shareUnlocks.ShareUnlocks is accessed without checking if shareUnlocks is nil, which could lead to a nil pointer dereference.

To prevent a possible runtime panic, ensure that shareUnlocks is properly initialized when it does not exist. You can modify the code as follows:

 ownerShares, exists := k.GetOwnerShares(ctx, req.Address)
 if !exists {
     return nil, status.Error(codes.NotFound, "owner not found")
 }

+shareUnlocks := &types.OwnerShareUnlocks{}
 totalLockedShares := big.NewInt(0)
 if unlocksExist {
     shareUnlocks, unlocksExist = k.GetOwnerShareUnlocks(ctx, req.Address)
     totalLockedShares = shareUnlocks.GetTotalLockedShares()
 }

Alternatively, check for nil before accessing shareUnlocks.ShareUnlocks when constructing the response.

Committable suggestion was skipped due to low confidence.

@@ -1,9 +1,10 @@
syntax = "proto3";
package dydxprotocol.vault;

import "cosmos_proto/cosmos.proto";
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix missing import 'cosmos_proto/cosmos.proto'

The imported file cosmos_proto/cosmos.proto does not exist, causing compilation errors. Please verify the import path or ensure that the file is included in your project dependencies.

If the path is incorrect, update it to point to the correct location of cosmos.proto. Otherwise, include the necessary dependency in your build configuration.

Tools
buf

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

proto/dydxprotocol/vault/query.proto Show resolved Hide resolved
proto/dydxprotocol/vault/query.proto Show resolved Hide resolved
proto/dydxprotocol/vault/query.proto Outdated Show resolved Hide resolved
Copy link
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: 1

Outside diff range and nitpick comments (2)
proto/dydxprotocol/vault/query.proto (1)

121-140: LGTM: Enhanced QueryMegavaultOwnerSharesResponse with detailed owner information

The changes to QueryMegavaultOwnerSharesResponse significantly improve the amount of information provided about an owner's shares and equity in the megavault. The new fields (address, shares, share_unlocks, equity, and withdrawable_equity) offer a comprehensive view of an owner's position.

The use of appropriate gogoproto options and clear comments for each field is commendable.

Consider adding units to the comments for equity and withdrawable_equity fields for extra clarity:

- // Owner equity in megavault (in quote quantums).
+ // Owner equity in megavault (in quote quantums, smallest unit of the quote asset).
- // Equity that owner can withdraw in quote quantums (as one cannot
- // withdraw locked shares).
+ // Equity that owner can withdraw (in quote quantums, smallest unit of the quote asset).
+ // This excludes locked shares which cannot be withdrawn.
protocol/x/vault/keeper/grpc_query_shares_test.go (1)

100-274: LGTM: Comprehensive test cases for MegavaultOwnerShares query with a minor suggestion

The TestMegavaultOwnerShares function provides an extensive set of test cases for the MegavaultOwnerShares query. It covers various scenarios including zero unlocks, single unlock, multiple unlocks, and error cases. The equity calculations and test setup, including the initialization of the megavault main vault, are well-implemented.

Consider adding a test case for when the total shares are zero to ensure the function handles potential division by zero errors gracefully. This edge case could occur in real-world scenarios and it's important to verify the behavior.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 90be117 and 22dcc7f.

Files ignored due to path filters (2)
  • protocol/x/vault/types/query.pb.go is excluded by !**/*.pb.go
  • protocol/x/vault/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
Files selected for processing (7)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (5 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (7 hunks)
  • proto/dydxprotocol/vault/query.proto (3 hunks)
  • protocol/x/vault/client/cli/query.go (3 hunks)
  • protocol/x/vault/keeper/grpc_query_shares.go (3 hunks)
  • protocol/x/vault/keeper/grpc_query_shares_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (4)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts
  • protocol/x/vault/client/cli/query.go
  • protocol/x/vault/keeper/grpc_query_shares.go
Additional context used
buf
proto/dydxprotocol/vault/query.proto

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

Additional comments not posted (15)
proto/dydxprotocol/vault/query.proto (5)

39-44: LGTM: New RPC method for querying all megavault owner shares

The addition of the MegavaultAllOwnerShares RPC method to the Query service is well-implemented. It provides a way to query all owner shares of the megavault, which enhances the functionality of the vault system.

The method signature, request/response types, and HTTP GET endpoint are all correctly defined.


115-115: LGTM: Address field added to QueryMegavaultOwnerSharesRequest

The addition of the address field to QueryMegavaultOwnerSharesRequest is a good improvement. It allows the query to be more specific by providing the owner's address.

The use of (cosmos_proto.scalar) = "cosmos.AddressString" is correct for Cosmos SDK address types, ensuring proper serialization and validation of the address field.


142-146: LGTM: New request message for querying all megavault owner shares

The addition of QueryMegavaultAllOwnerSharesRequest is well-structured and appropriate for the new RPC method.

The inclusion of a pagination field (of type cosmos.base.query.v1beta1.PageRequest) is a good practice, allowing for efficient querying of potentially large sets of owner shares.


Line range hint 1-193: Summary: Comprehensive enhancements to megavault querying capabilities

The changes in this file significantly improve the querying capabilities for megavault owner shares. Here's a summary of the key enhancements:

  1. Added a new RPC method MegavaultAllOwnerShares to query all owner shares.
  2. Enhanced QueryMegavaultOwnerSharesRequest and QueryMegavaultOwnerSharesResponse with more detailed information.
  3. Introduced new message types for querying all owner shares with pagination support.

These changes provide a more comprehensive and flexible way to query megavault information, which should improve the overall functionality of the vault system. The additions are well-structured, consistent with existing code, and follow protobuf best practices.

To ensure all changes are properly integrated, please verify the following:

  1. The OwnerShare message type is correctly defined or imported.
  2. All new RPC methods are properly implemented in the corresponding Go code.
  3. The changes are reflected in any client-side code that interacts with these RPCs.

Run the following command to check for any undefined message types:

This will help locate the definition of the OwnerShare message type if it exists in your project.


4-5: Verify the existence of 'cosmos_proto/cosmos.proto' in your project dependencies

The import of cosmos_proto/cosmos.proto is necessary for using cosmos-related scalar types, and the pagination import is needed for the new pagination-related fields. However, the static analysis tool indicates that the cosmos_proto/cosmos.proto file might be missing.

Please ensure that the cosmos_proto/cosmos.proto file is included in your project dependencies. If it's not present, you may need to add it to your build configuration or update your dependency management.

Run the following command to check if the file exists in your project:

If the file is not found, you may need to update your project's dependencies or build configuration.

Tools
buf

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

protocol/x/vault/keeper/grpc_query_shares_test.go (1)

Line range hint 17-98: LGTM: Comprehensive test cases for MegavaultAllOwnerShares query

The TestMegavaultAllOwnerShares function provides a thorough set of test cases for the MegavaultAllOwnerShares query. It covers successful scenarios with multiple owners, cases with no owners, and error handling for nil requests. The test setup and assertions are well-structured and comprehensive.

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (9)

3-3: Added necessary imports from './share'

The new imports NumShares, ShareUnlock, and OwnerShare are appropriately added for use in the updated interfaces.


128-128: Replaced 'pagination' with required 'address' field in QueryMegavaultOwnerSharesRequest

The change to include a required address field enhances the specificity of the request. Ensure that all callers of this interface are updated accordingly.

Also applies to: 136-136


145-160: Expanded QueryMegavaultOwnerSharesResponse with detailed owner share information

The addition of address, shares, shareUnlocks, equity, and withdrawableEquity fields provides comprehensive data about the owner's shares and equity in the megavault.

Also applies to: 168-184


186-193: Introduced QueryMegavaultAllOwnerSharesRequest for querying all owner shares

The new interface with optional pagination supports retrieval of all owner shares, facilitating broader queries.

Also applies to: 199-201


207-216: Added QueryMegavaultAllOwnerSharesResponse to handle responses for all owner shares

Including ownerShares array and optional pagination aligns with the request interface and enhances response handling.

Also applies to: 216-217


Line range hint 729-766: Implemented encode/decode functions for QueryMegavaultOwnerSharesRequest

The address field is correctly encoded and decoded, ensuring proper serialization of the request.


774-855: Implemented encode/decode functions for QueryMegavaultOwnerSharesResponse

All new fields are correctly handled in encode, decode, and fromPartial methods with appropriate field numbers and initializations.


856-899: Implemented QueryMegavaultAllOwnerSharesRequest encode/decode functions

The pagination field is properly managed, allowing for correct serialization of requests.


Line range hint 900-949: Implemented QueryMegavaultAllOwnerSharesResponse encode/decode functions

The encoding and decoding of ownerShares and pagination ensure accurate response serialization.

// QueryMegavaultAllOwnerSharesRequest is a request type for the
// MegavaultAllOwnerShares RPC method.
message QueryMegavaultAllOwnerSharesRequest {
cosmos.base.query.v1beta1.PageRequest pagination = 1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh this was the old MegavaultOwnerSharesRequest and it lists out all owner shares

@@ -35,6 +36,12 @@ service Query {
returns (QueryMegavaultOwnerSharesResponse) {
option (google.api.http).get = "/dydxprotocol/vault/megavault/owner_shares";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
option (google.api.http).get = "/dydxprotocol/vault/megavault/owner_shares";
option (google.api.http).get = "/dydxprotocol/vault/megavault/owner_shares/{address}";

Copy link
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: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 22dcc7f and 1ee2b84.

Files ignored due to path filters (2)
  • protocol/x/vault/types/query.pb.go is excluded by !**/*.pb.go
  • protocol/x/vault/types/query.pb.gw.go is excluded by !**/*.pb.gw.go
<details>
Files selected for processing (2)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (4 hunks)
  • proto/dydxprotocol/vault/query.proto (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts
Additional context used
buf
proto/dydxprotocol/vault/query.proto

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

Additional comments not posted (6)
proto/dydxprotocol/vault/query.proto (6)

5-5: LGTM: Pagination import added

The addition of the import for pagination is correct and necessary for the new pagination-related fields in the query messages.


37-44: LGTM: New query method and updated HTTP option

The changes to the Query service are well-implemented:

  1. The updated HTTP GET option for MegavaultOwnerShares now includes the address parameter, allowing for more specific queries.
  2. The new MegavaultAllOwnerShares method has been added, which aligns with the PR objective of querying all megavault owner shares.

These changes enhance the querying capabilities for megavault owner shares as intended.


116-116: LGTM: Address field added to QueryMegavaultOwnerSharesRequest

The addition of the address field with the (cosmos_proto.scalar) option is correct and aligns with the updated query functionality.

Note: The effectiveness of this change depends on resolving the cosmos_proto/cosmos.proto import issue mentioned earlier.


122-141: LGTM: Expanded QueryMegavaultOwnerSharesResponse

The expansion of the QueryMegavaultOwnerSharesResponse message is well-implemented:

  1. It now includes detailed information about the owner's shares, equity, and withdrawable equity.
  2. The use of (gogoproto.nullable) = false ensures that these fields are always present in the response.
  3. The custom types for equity fields use the appropriate serializable integer type.

These changes provide a comprehensive response for megavault owner share queries, which aligns with the PR objectives.


145-147: LGTM: New QueryMegavaultAllOwnerSharesRequest message

The new QueryMegavaultAllOwnerSharesRequest message is well-defined:

  1. It includes a pagination field, which is essential for handling large datasets of owner shares.
  2. The use of the imported pagination request type is correct.

This message type properly supports the new MegavaultAllOwnerShares RPC method.


4-4: Verify the import path for 'cosmos_proto/cosmos.proto'

The static analysis tool indicates that the file "cosmos_proto/cosmos.proto" does not exist. This import is crucial for the (cosmos_proto.scalar) option used in the message definitions.

Please run the following script to check if the file exists in the project or its dependencies:

If the file is not found, ensure that the correct dependency is included in your project or update the import path accordingly.

Verification successful

Import path for 'cosmos_proto/cosmos.proto' verified.

The file "cosmos_proto/cosmos.proto" exists in the project, ensuring that the import statement is correct. The initial concern regarding the missing import has been resolved.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if cosmos_proto/cosmos.proto exists
if fd -t f "cosmos_proto/cosmos.proto"; then
    echo "File found"
else
    echo "File not found"
    echo "Searching for similar files:"
    fd -t f "cosmos.*proto"
fi

Length of output: 66

Tools
buf

4-4: import "cosmos_proto/cosmos.proto": file does not exist

(COMPILE)

proto/dydxprotocol/vault/query.proto Show resolved Hide resolved
@tqin7 tqin7 merged commit 0601feb into main Sep 23, 2024
37 of 38 checks passed
@tqin7 tqin7 deleted the tq/tra-628 branch September 23, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants