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

[TRA-625] Add upgrade handler for migrating vault params. #2304

Merged
merged 10 commits into from
Sep 20, 2024

Conversation

vincentwschau
Copy link
Contributor

@vincentwschau vincentwschau commented Sep 20, 2024

Changelist

Add upgrade handler for vault quoting parameters to vault params.
For vaults with no quoting params, set quoting parameters to nil.
All vaults are initialized to QUOTING status.

Misc:

  • added new query to get vault params to get around issue with unmarshalling SerializableInt type in container tests
  • moved all deprecated state related functions / keys into a separate file

Test Plan

Container test for both a vault with no quoting params, and one with quoting params.

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

Release Notes

  • New Features

    • Introduced a new method to query specific vault parameters, enhancing user access to detailed vault information.
    • Added functionality for querying vault parameters through gRPC, improving integration capabilities.
    • Enhanced the LCDQueryClient to support vault parameter queries.
  • Bug Fixes

    • Enhanced error handling for vault parameter queries, ensuring clearer feedback for non-existent vaults.
  • Documentation

    • Updated API documentation to reflect new vault parameter query capabilities and usage examples.
  • Tests

    • Added comprehensive tests for the new vault parameters query functionality to ensure reliability and correctness.
  • Chores

    • Removed deprecated methods to streamline the vault management process and improve security.

@vincentwschau vincentwschau requested a review from a team as a code owner September 20, 2024 01:33
Copy link

linear bot commented Sep 20, 2024

Copy link
Contributor

coderabbitai bot commented Sep 20, 2024

Walkthrough

The pull request introduces enhancements to the vault management system within the protocol. A new RPC method VaultParams is added to query vault parameters, along with a migration function for quoting parameters. The upgrade handling process is modified to include the VaultKeeper, and deprecated functions related to parameter management are removed. Additionally, unit tests are added to ensure the correctness of the new features, enhancing the overall functionality and reliability of vault operations.

Changes

File Path Change Summary
proto/dydxprotocol/vault/query.proto Added rpc VaultParams method, QueryVaultParamsRequest, and QueryVaultParamsResponse messages for querying vault parameters.
protocol/app/upgrades.go Modified setupUpgradeHandlers to include app.VaultKeeper as a parameter, enhancing upgrade handling related to vault management.
protocol/app/upgrades/v7.0.0/upgrade.go Introduced migrateVaultQuotingParamsToVaultParams function for migrating quoting parameters to a new structure and modified CreateUpgradeHandler to include vaultKeeper.
protocol/x/vault/keeper/deprecated_state.go Introduced functions for managing deprecated state data during upgrades, focusing on quoting parameters and legacy data structures.
protocol/x/vault/keeper/grpc_query_vault_params.go Defined VaultParams method in Keeper for querying vault parameters via gRPC, ensuring proper error handling.
protocol/x/vault/keeper/grpc_query_vault_params_test.go Added unit tests for VaultParams query method, covering various scenarios and ensuring robust error handling.
protocol/x/vault/keeper/params.go Removed UnsafeGetParams and UnsafeDeleteParams functions to eliminate unsafe operations in parameter management.
protocol/x/vault/types/types.go Modified VaultKeeper interface methods for managing total shares, simplifying method signatures and return types.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts Added interfaces for QueryVaultParamsRequest and QueryVaultParamsResponse, including encoding and decoding methods.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts Introduced vaultParams method to LCDQueryClient for querying vault parameters.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts Added vaultParams method to Query interface and QueryClientImpl for gRPC querying of vault parameters.

Possibly related PRs

🐇 In the vaults where secrets lie,
New parameters flutter and fly.
With upgrades and queries, oh what a sight,
Our vaults now shine, oh so bright!
Migrate the old, embrace the new,
Hopping along, we’ll see this through! 🌟


Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 4e2bbfe and 98bd055.

Files selected for processing (2)
  • protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2 hunks)
  • protocol/mocks/VaultKeeper.go (3 hunks)
Additional comments not posted (6)
protocol/app/upgrades/v7.0.0/upgrade_container_test.go (3)

50-51: LGTM!

The addition of the postUpgradeVaultParamsCheck function call to perform post-upgrade checks related to vault parameters is a valid and necessary step.


53-71: Great job with the post-upgrade checks for vault parameters!

The postUpgradeVaultParamsCheck function correctly validates the two scenarios related to the migration of vault parameters:

  1. A vault with quoting parameters is successfully migrated and the quoting parameters are migrated to the vault parameters.
  2. A vault without quoting parameters is successfully migrated and the quoting parameters are not set in the migrated vault parameters.

The use of the checkVaultParams helper function is a good practice to avoid code duplication. The expected quoting parameters and vault status are correctly defined for each scenario.

Overall, this is a well-implemented post-upgrade check.


73-97: The checkVaultParams helper function is well-implemented!

The checkVaultParams function is a useful helper function that encapsulates the logic for querying and validating vault parameters. It takes the necessary parameters, such as the node, testing object, vault number, expected status, and expected quoting parameters, to perform the checks.

The function correctly queries the vault parameters using the containertest.Query function and the vaulttypes.QueryClient.VaultParams RPC method. It then unmarshals the response into a vaulttypes.QueryVaultParamsResponse object and checks that the actual vault status and quoting parameters match the expected values.

The function is well-structured, easy to understand, and promotes code reusability by avoiding duplication in the postUpgradeVaultParamsCheck function.

Great job with this helper function!

protocol/mocks/VaultKeeper.go (3)

39-54: Verify the updated function signature across the codebase.

The changes to the GetTotalShares function signature look good as they simplify the interface. However, ensure that all callers of this function have been updated to match the new signature.

Run the following script to verify the function usage:

Verification successful

GetTotalShares function signature update verified successfully

The comprehensive search of the codebase confirms that the GetTotalShares function signature has been consistently updated across all relevant files. All occurrences now use the new signature with only the ctx parameter, and there are no remaining references to vaultId in relation to this function. The changes have been properly implemented, including in the mock implementation and various usages throughout the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `GetTotalShares` match the new signature.

# Test: Search for the function usage. Expect: No occurrences of the old signature.
rg --type go -A 5 $'GetTotalShares\(ctx, vaultId\)'

Length of output: 52


Script:

#!/bin/bash
# Description: Perform a comprehensive search for GetTotalShares usage

# Search for any usage of GetTotalShares function
echo "Searching for GetTotalShares usage:"
rg --type go -A 5 "GetTotalShares\("

# Search for any remaining references to vaultId near GetTotalShares
echo "Searching for vaultId references near GetTotalShares:"
rg --type go -A 5 -B 5 "GetTotalShares.*vaultId"

Length of output: 6374


117-144: Verify the updated function signature across the codebase.

The changes to the MintShares function signature look good as they provide more detailed information about the operation's outcome. However, ensure that all callers of this function have been updated to:

  1. Match the new signature.
  2. Handle the additional *big.Int return value.

Run the following script to verify the function usage:

Verification successful

Changes to MintShares function have been correctly implemented.

The MintShares function signature has been successfully updated throughout the codebase. All calls to this function now use the new signature (ctx, owner, quantumsToDeposit), and there are no remaining instances of the old signature that include the vaultId parameter.

Key points:

  • The mock file (protocol/mocks/VaultKeeper.go) has been updated with the new function signature.
  • The function is called correctly in deposit.go and deposit_test.go.
  • No occurrences of the old function signature were found in the codebase.

The changes have been implemented consistently, and no further updates are required for this function.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `MintShares` match the new signature and handle the return values.

# Test: Search for the function usage. Expect: No occurrences of the old signature or mishandled return values.
rg --type go -A 10 $'MintShares\(ctx, vaultId, owner, quantumsToDeposit\)'

Length of output: 75


Script:

#!/bin/bash
# Description: Verify the usage of MintShares function across the codebase

# Search for any occurrence of MintShares in Go files
echo "Occurrences of MintShares in Go files:"
rg --type go -C 5 "MintShares"

echo "\nPotential missed updates (occurrences of 'vaultId' near 'MintShares'):"
rg --type go -C 5 -e "MintShares" -e "vaultId" | rg --multiline "(?s)MintShares.*vaultId|vaultId.*MintShares"

Length of output: 176839


Line range hint 188-203: Verify the updated function signature across the codebase.

The changes to the SetTotalShares function signature look good as they streamline the interface. However, ensure that all callers of this function have been updated to match the new signature.

Run the following script to verify the function usage:

Verification successful

SetTotalShares signature update verified successfully.

The changes to the SetTotalShares function have been correctly implemented across the codebase:

  1. The VaultKeeper interface in protocol/x/vault/types/types.go defines the method with the new signature.
  2. The implementation in protocol/x/vault/keeper/shares.go matches the interface.
  3. All usage of SetTotalShares across the codebase has been updated to use the new signature.

No further action is required.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `SetTotalShares` match the new signature.

# Test: Search for the function usage. Expect: No occurrences of the old signature.
rg --type go -A 5 $'SetTotalShares\(ctx, vaultId, totalShares\)'

Length of output: 65


Script:

#!/bin/bash
# Description: Comprehensive verification of SetTotalShares usage and definition

# Search for any usage of SetTotalShares
echo "Searching for SetTotalShares usage:"
rg --type go -A 3 "SetTotalShares\("

# Look for the VaultKeeper interface definition
echo -e "\nSearching for VaultKeeper interface definition:"
rg --type go -A 10 "type VaultKeeper interface"

# Search for TODO comments related to SetTotalShares
echo -e "\nSearching for TODO comments related to SetTotalShares:"
rg --type go "TODO.*SetTotalShares"

Length of output: 4905


Script:

#!/bin/bash
# Description: Final verification of SetTotalShares in VaultKeeper interface and old signature usage

# Search for SetTotalShares within VaultKeeper interface
echo "Searching for SetTotalShares in VaultKeeper interface:"
rg --type go -A 3 "type VaultKeeper interface" -p protocol/x/vault/types/types.go | rg -A 20 "SetTotalShares"

# Double-check for any remaining instances of the old signature
echo -e "\nDouble-checking for old signature usage:"
rg --type go "SetTotalShares\([^)]*vaultId[^)]*\)"

Length of output: 378


Script:

#!/bin/bash
# Description: Verify VaultKeeper interface and implementation for SetTotalShares

# Search for the entire VaultKeeper interface
echo "Searching for the entire VaultKeeper interface:"
rg --type go -A 50 "type VaultKeeper interface" protocol/x/vault/types/types.go

# Search for SetTotalShares method in VaultKeeper implementation
echo -e "\nSearching for SetTotalShares method in VaultKeeper implementation:"
rg --type go -A 5 "func \(k .*Keeper\) SetTotalShares\("

Length of output: 1505


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

Outside diff range and nitpick comments (1)
protocol/app/upgrades/v7.0.0/upgrade_container_test.go (1)

40-40: Consider removing the empty preUpgradeSetups function if not needed

The function preUpgradeSetups is currently empty. If there are no setup steps required before the upgrade, consider removing this function to keep the code clean. If it's intended as a placeholder for future setup logic, you might add a comment to indicate that.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cee80fc and b65edc5.

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 (13)
  • 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 (5 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (2 hunks)
  • proto/dydxprotocol/vault/query.proto (2 hunks)
  • protocol/app/upgrades.go (1 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade.go (3 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2 hunks)
  • protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
  • protocol/x/vault/keeper/deprecated_state.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params_test.go (1 hunks)
  • protocol/x/vault/keeper/params.go (0 hunks)
  • protocol/x/vault/types/types.go (1 hunks)
Files not reviewed due to no reviewable changes (1)
  • protocol/x/vault/keeper/params.go
Additional comments not posted (28)
protocol/x/vault/keeper/grpc_query_vault_params.go (1)

13-37: LGTM!

The VaultParams function follows a clear and logical flow:

  1. It validates the input request.
  2. It extracts the vault ID from the request.
  3. It retrieves the vault parameters using the vault ID.
  4. It handles the "not found" case explicitly.
  5. It returns a structured response or an error.

The function is well-structured and adheres to best practices. The code changes look good to me.

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

13-86: Excellent test coverage!

The TestVaultParamsQuery function provides comprehensive test coverage for the VaultParams query functionality. It covers various scenarios, including successful queries and error conditions, using a well-structured table-driven approach.

The test initializes a new test application context for each scenario, sets up the vault parameters, invokes the VaultParams method, and validates the results against the expected outcomes. This approach ensures the reliability of the vault query functionality and provides a clear framework for future tests and modifications.

Great job on writing thorough and well-organized tests!

protocol/app/upgrades.go (1)

34-34: LGTM!

The addition of app.VaultKeeper as a parameter to the v7_0_0.CreateUpgradeHandler function call aligns with the PR objective of introducing an upgrade handler for migrating vault parameters. Passing the VaultKeeper to the upgrade handler will allow it to access the vault state and perform the necessary vault-related operations during the upgrade.

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

3-3: LGTM!

The import statement is correct and aligns with the PR objective of adding an upgrade handler for vault quoting parameters.


Line range hint 18-81: Implementation looks good and aligns with the PR objective!

The new vaultParams method in the LCDQueryClient class is implemented correctly and enhances the functionality by enabling queries for individual vault parameters. This aligns perfectly with the PR objective of adding an upgrade handler for vault quoting parameters.

Some key observations:

  • The method constructs the endpoint URL correctly, incorporating the vault type and number for targeted queries.
  • It utilizes the new QueryVaultParamsRequest and QueryVaultParamsResponseSDKType types appropriately.
  • The constructor update ensures that the method can be called correctly within the class context.

Overall, the implementation follows the existing pattern of other query methods in the class and integrates well with the codebase.

protocol/x/vault/keeper/deprecated_state.go (6)

27-39: LGTM!

The function correctly retrieves the vault parameters as they existed in version 5.x, which is necessary for the v6.x upgrade handler. The deprecation comment provides clear guidance on its intended use.


41-49: LGTM!

The function correctly deletes the vault parameters as they existed in version 5.x, which is necessary for the v6.x upgrade handler. The deprecation comment provides clear guidance on its intended use.


53-70: LGTM!

The function correctly sets the quoting parameters for a given vault in the state, which is necessary for the v7.x upgrade handler. It performs the necessary validation and marshaling of the quotingParams struct before setting it in the store. The deprecation comment provides clear guidance on its intended use.


72-91: LGTM!

The function correctly retrieves the quoting parameters for a given vault from the state, which is necessary for the v7.x upgrade handler. It handles the case when the quoting parameters do not exist for the given vault and unmarshals the retrieved bytes into the quotingParams struct when they exist. The deprecation comment provides clear guidance on its intended use.


93-106: LGTM!

The function correctly deletes the quoting parameters for a given vault from the state, which is necessary for the v7.x upgrade handler. It handles the case when the quoting parameters do not exist for the given vault and deletes them from the store when they exist. The deprecation comment provides clear guidance on its intended use.


108-124: LGTM!

The function correctly retrieves all vault IDs from the state using the deprecated total shares data. It iterates through the total shares store, extracts the vault ID from each key using the GetVaultIdFromStateKey function, and appends it to the vaultIds slice. The deprecation comment provides clear guidance on its intended use during the upgrade process.

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

38-41: LGTM!

The new VaultParams RPC method follows the established conventions and patterns. The HTTP GET endpoint is well-defined with appropriate path parameters.


110-115: LGTM!

The QueryVaultParamsRequest message type is defined correctly with appropriate field types, names, and numbers. It aligns well with the VaultParams RPC method.


117-121: LGTM!

The QueryVaultParamsResponse message type is defined correctly with appropriate field types, names, numbers, and non-nullable options. It aligns well with the VaultParams RPC method and ensures the presence of the required fields in the response.

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

4-4: LGTM!

The import statement for QueryVaultParamsRequest and QueryVaultParamsResponse is correct and consistent with the changes described in the PR summary.


22-24: LGTM!

The addition of the vaultParams method to the Query interface is correct and consistent with the changes described in the PR summary. The method signature accurately reflects the intent to query vault parameters for a specific vault.


36-36: LGTM!

Binding the vaultParams method in the constructor of the QueryClientImpl class is correct and consistent with how other methods are bound. This ensures that the method can be called with the correct this context.


73-77: LGTM!

The implementation of the vaultParams method in the QueryClientImpl class is correct and follows the same pattern as other methods in the class. The RPC request is made to the correct endpoint with the appropriate method name, and the request and response are correctly encoded and decoded using the corresponding types.


102-105: LGTM!

Adding the vaultParams method to the object returned by the createRpcQueryExtension function is correct and consistent with how other methods are added. The method correctly delegates the call to the queryService instance, making it available to clients that use the RPC query extension.

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

156-179: LGTM!

The new interfaces QueryVaultParamsRequest and QueryVaultParamsResponse are well-defined with clear field names and types. The corresponding SDK types are correctly mapped to the interface fields.


697-806: LGTM!

The encoding, decoding, and fromPartial methods for the new interfaces QueryVaultParamsRequest and QueryVaultParamsResponse are implemented correctly. The methods handle the optional fields in the response interface appropriately, and the field types are correctly encoded and decoded using the corresponding methods.

protocol/testing/containertest/preupgrade_genesis.json (1)

4391-4400: Looks good! The new quoting_params fields enable granular configuration of quoting behavior for vaults.

The added parameters allow tuning key aspects of quoting such as:

  • Order book depth
  • Bid-ask spreads
  • Order sizing
  • Order expiration time
  • Activation threshold

This provides greater flexibility to optimize quoting strategies on a per-vault basis. The field names are clear and the included parameters align with the stated objectives of the upgrade handler.

protocol/app/upgrades/v7.0.0/upgrade.go (3)

13-14: Imported necessary vault modules

The addition of vaultkeeper and vaulttypes imports is appropriate for the migration functions.


61-62: Migration function correctly integrated into upgrade handler

The call to migrateVaultQuotingParamsToVaultParams ensures that the migration is executed during the upgrade process.


52-52: Ensure all invocations of CreateUpgradeHandler are updated

The function signature of CreateUpgradeHandler has changed with the addition of the vaultKeeper parameter. Make sure that all calls to this function are updated to include the new parameter to prevent compilation errors.

Run the following script to find all usages of CreateUpgradeHandler and verify they include the new vaultKeeper parameter:

Verification successful

Verification successful: CreateUpgradeHandler updated correctly

The CreateUpgradeHandler function in protocol/app/upgrades/v7.0.0/upgrade.go has been correctly updated to include the vaultKeeper parameter. Additionally, the invocation of this function in protocol/app/upgrades.go has been updated to include the app.VaultKeeper argument. These changes address the concern raised in the original review comment.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to `CreateUpgradeHandler` and check if they have been updated.

# Expected result: All calls should include `vaultKeeper` as a parameter.
rg --type go 'CreateUpgradeHandler\(' -A 2

Length of output: 1635


Script:

#!/bin/bash
# Search for the full parameter list of CreateUpgradeHandler in v7.0.0/upgrade.go
echo "CreateUpgradeHandler in v7.0.0/upgrade.go:"
rg --type go 'func CreateUpgradeHandler\(' protocol/app/upgrades/v7.0.0/upgrade.go -A 10

# Search for the full invocation of v7_0_0.CreateUpgradeHandler in upgrades.go
echo -e "\nCreateUpgradeHandler invocation in upgrades.go:"
rg --type go 'v7_0_0\.CreateUpgradeHandler\(' protocol/app/upgrades.go -A 10

Length of output: 1127

protocol/app/upgrades/v7.0.0/upgrade_container_test.go (3)

6-7: Import statement for proto package is appropriate

The import of "github.com/cosmos/gogoproto/proto" is necessary for unmarshaling protocol buffer messages in the code.


9-12: Import statements are appropriate and necessary

The added import statements are required for the functionality implemented in the test, including references to vault types, constants, and test utilities.


48-49: Approved: Added post-upgrade check function call

The call to postUpgradeVaultParamsCheck(node, t) integrates the new post-upgrade checks into the test flow appropriately.

@@ -31,19 +31,16 @@ type VaultKeeper interface {
// Shares.
GetTotalShares(
ctx sdk.Context,
vaultId VaultId,
) (val NumShares, exists bool)
) (val NumShares)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Update mock implementation of GetTotalShares

The GetTotalShares method signature change has been successfully implemented across the main codebase. However, the mock implementation in protocol/mocks/VaultKeeper.go still uses the old signature:

func (_m *VaultKeeper) GetTotalShares(ctx types.Context, vaultId vaulttypes.VaultId) (vaulttypes.NumShares, bool)

This mock needs to be updated to match the new signature:

func (_m *VaultKeeper) GetTotalShares(ctx types.Context) vaulttypes.NumShares

Ensure that any tests using this mock are also updated to reflect the new signature and return value.

Analysis chain

Verify the impact of the method signature change on the callers.

The GetTotalShares method signature has been modified to remove the vaultId parameter and the boolean return value. Ensure that all callers of this method have been updated to match the new signature and handle the absence of the boolean return value correctly.

Run the following script to verify the method usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all method calls to `GetTotalShares` match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'GetTotalShares'

Length of output: 3034


Script:

#!/bin/bash
# Description: Verify all method calls to `GetTotalShares` match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
if rg --type go -A 5 'GetTotalShares'; then
    echo "Found occurrences of GetTotalShares"
else
    echo "No occurrences of GetTotalShares found in the codebase"
fi

# Additional search to find any potential references to vaultId in relation to GetTotalShares
echo "Searching for potential old usage patterns:"
rg --type go 'GetTotalShares.*vaultId'

Length of output: 7077

owner string,
quantumsToDeposit *big.Int,
) error
) (*big.Int, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Update mock implementation of MintShares in VaultKeeper.go

The MintShares method signature has been updated in the main codebase, but the mock implementation in protocol/mocks/VaultKeeper.go still uses the old signature. This inconsistency needs to be addressed:

  • Current mock signature: MintShares(ctx types.Context, vaultId vaulttypes.VaultId, owner string, quantumsToDeposit *big.Int) error
  • Expected signature: MintShares(ctx types.Context, owner string, quantumsToDeposit *big.Int) (*big.Int, error)

Please update the mock implementation to match the new signature. Additionally:

  • Review and update any tests that use this mock to ensure they're compatible with the new signature.
  • Regenerate the mock if you're using a mocking framework that automates this process.
Analysis chain

Verify the impact of the method signature change on the callers.

The MintShares method signature has been modified to remove the vaultId parameter and return the amount of shares minted (*big.Int) alongside any potential error. Ensure that all callers of this method have been updated to match the new signature and handle the additional return value correctly.

Run the following script to verify the method usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all method calls to `MintShares` match the new signature.

# Test: Search for the method usage. Expect: Only occurrences of the new signature.
rg --type go -A 5 $'MintShares'

Length of output: 3034


Script:

#!/bin/bash
# Description: Search for any remaining occurrences of the old MintShares signature

# Search for MintShares with vaultId parameter
rg --type go -A 5 'MintShares\s*\(\s*ctx\s*[^,]*,\s*vaultId'

# Search for MintShares that don't return *big.Int
rg --type go -A 5 'MintShares\s*\([^)]*\)\s*error'

Length of output: 1005

Comment on lines 28 to 46
func migrateVaultQuotingParamsToVaultParams(ctx sdk.Context, k vaultkeeper.Keeper) {
vaultIds := k.UnsafeGetAllVaultIds(ctx)
ctx.Logger().Info(fmt.Sprintf("Migrating quoting parameters of %d vaults", len(vaultIds)))
for _, vaultId := range vaultIds {
quotingParams, exists := k.UnsafeGetQuotingParams(ctx, vaultId)
vaultParams := vaulttypes.VaultParams{
Status: vaulttypes.VaultStatus_VAULT_STATUS_QUOTING,
}
if exists {
vaultParams.QuotingParams = &quotingParams
}
k.SetVaultParams(ctx, vaultId, vaultParams)
k.UnsafeDeleteQuotingParams(ctx, vaultId)
ctx.Logger().Info(fmt.Sprintf(
"Successfully migrated vault %+v",
vaultId,
))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential issue: Setting all vault statuses to VAULT_STATUS_QUOTING

In the migrateVaultQuotingParamsToVaultParams function, all vaults are assigned the status VAULT_STATUS_QUOTING, regardless of their current status or whether they have existing quoting parameters. This could overwrite existing statuses that should be preserved. Please verify that it is appropriate to set all vaults to VAULT_STATUS_QUOTING, or modify the migration logic to retain existing statuses where necessary.

Comment on lines 51 to 100
func postUpgradeVaultParamsCheck(node *containertest.Node, t *testing.T) {
// Check that a vault with quoting params is successfully migrated and the quoting params are
// successfully migrated to the vault params.
resp, err := containertest.Query(
node,
vaulttypes.NewQueryClient,
vaulttypes.QueryClient.VaultParams,
&vaulttypes.QueryVaultParamsRequest{
Type: vaulttypes.VaultType_VAULT_TYPE_CLOB,
Number: 0,
},
)
require.NoError(t, err)
require.NotNil(t, resp)
vaultParamsResp := vaulttypes.QueryVaultParamsResponse{}
err = proto.UnmarshalText(resp.String(), &vaultParamsResp)
require.NoError(t, err)

expectedQuotingParams := &vaulttypes.QuotingParams{
Layers: 3,
SpreadMinPpm: 1500,
SpreadBufferPpm: 500,
SkewFactorPpm: 1000000,
OrderSizePctPpm: 50000,
OrderExpirationSeconds: 30,
ActivationThresholdQuoteQuantums: dtypes.NewIntFromUint64(500_000_000),
}

require.Equal(t, vaulttypes.VaultStatus_VAULT_STATUS_QUOTING, vaultParamsResp.VaultParams.Status)
require.Equal(t, expectedQuotingParams, vaultParamsResp.VaultParams.QuotingParams)

// Check that a vault without quoting params is successfully migrated and the quoting params are
// not set in the migrated vault params.
resp, err = containertest.Query(
node,
vaulttypes.NewQueryClient,
vaulttypes.QueryClient.VaultParams,
&vaulttypes.QueryVaultParamsRequest{
Type: vaulttypes.VaultType_VAULT_TYPE_CLOB,
Number: 1,
},
)
require.NoError(t, err)
require.NotNil(t, resp)
vaultParamsResp = vaulttypes.QueryVaultParamsResponse{}
err = proto.UnmarshalText(resp.String(), &vaultParamsResp)
require.NoError(t, err)

require.Equal(t, vaulttypes.VaultStatus_VAULT_STATUS_QUOTING, vaultParamsResp.VaultParams.Status)
require.Equal(t, (*vaulttypes.QuotingParams)(nil), vaultParamsResp.VaultParams.QuotingParams)
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 20, 2024

Choose a reason for hiding this comment

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

Refactor postUpgradeVaultParamsCheck to reduce code duplication

The function postUpgradeVaultParamsCheck contains duplicated code for querying and validating vault parameters for two vaults. Consider abstracting the common logic into a helper function that accepts parameters such as the vault number and expected quoting parameters. This will improve maintainability and readability.

Here's how you might refactor the function:

func checkVaultParams(node *containertest.Node, t *testing.T, vaultNumber uint64, expectedStatus vaulttypes.VaultStatus, expectedQuotingParams *vaulttypes.QuotingParams) {
    resp, err := containertest.Query(
        node,
        vaulttypes.NewQueryClient,
        vaulttypes.QueryClient.VaultParams,
        &vaulttypes.QueryVaultParamsRequest{
            Type:   vaulttypes.VaultType_VAULT_TYPE_CLOB,
            Number: vaultNumber,
        },
    )
    require.NoError(t, err)
    require.NotNil(t, resp)

    vaultParamsResp := vaulttypes.QueryVaultParamsResponse{}
    err = proto.UnmarshalText(resp.String(), &vaultParamsResp)
    require.NoError(t, err)

    require.Equal(t, expectedStatus, vaultParamsResp.VaultParams.Status)
    require.Equal(t, expectedQuotingParams, vaultParamsResp.VaultParams.QuotingParams)
}

Then, update postUpgradeVaultParamsCheck to use this helper function:

func postUpgradeVaultParamsCheck(node *containertest.Node, t *testing.T) {
    // Check vault with quoting params
    expectedQuotingParams := &vaulttypes.QuotingParams{
        Layers:                           3,
        SpreadMinPpm:                     1500,
        SpreadBufferPpm:                  500,
        SkewFactorPpm:                    1000000,
        OrderSizePctPpm:                  50000,
        OrderExpirationSeconds:           30,
        ActivationThresholdQuoteQuantums: dtypes.NewIntFromUint64(500_000_000),
    }

    checkVaultParams(node, t, 0, vaulttypes.VaultStatus_VAULT_STATUS_QUOTING, expectedQuotingParams)

    // Check vault without quoting params
    checkVaultParams(node, t, 1, vaulttypes.VaultStatus_VAULT_STATUS_QUOTING, nil)
}

This refactoring reduces redundancy and makes the code cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vincentwschau, I'm glad to hear that my suggestion was useful!

(_/)
(❁ᴗ͈ˬᴗ͈)っ

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

Outside diff range and nitpick comments (3)
protocol/x/vault/keeper/deprecated_state.go (3)

28-30: Clarify deprecation notes for UnsafeGetParams.

The comments mention usage for the v6.x upgrade handler while referencing v5.x state. To avoid confusion, clarify that this function accesses v5.x state for use in the v6.x upgrade.


14-23: Remove redundant comments and consolidate deprecation notes.

The comments for the deprecated state keys are repeated and could be consolidated for clarity. This enhances readability and maintains a clean codebase.

Apply this diff to consolidate the comments:

-// Deprecated: For use by the v7.x upgrade handler
-// QuotingParamsKeyPrefix is the prefix to retrieve all QuotingParams.
-// QuotingParams store: vaultId VaultId -> QuotingParams.
+// Deprecated: For use by the v7.x upgrade handler.
+// QuotingParamsKeyPrefix is the prefix to retrieve all QuotingParams.
 // QuotingParams store: vaultId VaultId -> QuotingParams.
 QuotingParamsKeyPrefix = "QuotingParams:"

-// Deprecated: For use by the v7.x upgrade handler
-// TotalSharesKeyPrefix is the prefix to retrieve all TotalShares.
+// TotalSharesKeyPrefix is the prefix to retrieve all TotalShares.
 TotalSharesKeyPrefix = "TotalShares:"

47-49: Optional: Check for key existence before deletion in UnsafeDeleteParams.

Currently, UnsafeDeleteParams deletes the Params key without checking if it exists. While deleting a non-existent key is generally safe, adding a check could make the code more robust.

Consider adding:

if store.Has([]byte("Params")) {
    store.Delete([]byte("Params"))
}
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cee80fc and b65edc5.

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 (13)
  • 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 (5 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (2 hunks)
  • proto/dydxprotocol/vault/query.proto (2 hunks)
  • protocol/app/upgrades.go (1 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade.go (3 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2 hunks)
  • protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
  • protocol/x/vault/keeper/deprecated_state.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params_test.go (1 hunks)
  • protocol/x/vault/keeper/params.go (0 hunks)
  • protocol/x/vault/types/types.go (1 hunks)
Files not reviewed due to no reviewable changes (1)
  • protocol/x/vault/keeper/params.go
Additional comments not posted (28)
protocol/x/vault/types/types.go (2)

34-34: Approve the change to GetTotalShares method signature.

The removal of the vaultId parameter and the exists return value aligns with the goal of the upgrade handler to manage vault quoting parameters consistently across vaults.

Note that this change could potentially break existing code that relies on the vaultId parameter or the exists return value. Ensure that all affected code has been updated accordingly.


43-43: Approve the change to MintShares method signature.

The removal of the vaultId parameter aligns with the goal of the upgrade handler to manage vault quoting parameters consistently across vaults.

Returning a pointer to a big.Int in addition to the error is an improvement that provides more information about the minted shares to the caller.

Note that this change could potentially break existing code that relies on the vaultId parameter or expects only an error return value. Ensure that all affected code has been updated to handle the new return value.

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

13-37: LGTM!

The VaultParams function is well-structured and aligns with the PR objectives. It performs the necessary validations, error handling, and retrieves the vault parameters based on the provided vault ID. The function enhances the querying capabilities of the vault management system.

Some key observations:

  • The function checks for a valid request and returns an appropriate error if the request is nil.
  • It unwraps the SDK context and constructs a VaultId using the provided type and number from the request.
  • It retrieves the vault parameters associated with the constructed VaultId and returns a not-found error if the parameters do not exist.
  • If the parameters are successfully retrieved, it constructs and returns a QueryVaultParamsResponse containing the vault ID and the corresponding vault parameters.

Overall, the implementation is robust and facilitates the querying of vault parameters in a structured manner.

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

13-86: Excellent test coverage!

The TestVaultParamsQuery function is well-structured and provides comprehensive test coverage for the VaultParams query method. The test cases cover various scenarios, including successful queries, handling of specific vault statuses, and error conditions.

The use of a map structure to define test cases is a good practice as it makes the tests organized and scalable. The assertions using the require package are clear and concise, providing immediate feedback on test failures.

Overall, this test function enhances the robustness of the vault module by ensuring the query functionality behaves as expected under different conditions.

protocol/app/upgrades.go (1)

34-34: LGTM! The code change aligns with the PR objectives.

The addition of app.VaultKeeper as a parameter to the v7_0_0.CreateUpgradeHandler function call is consistent with the PR's goal of introducing an upgrade handler for migrating vault parameters. By providing the VaultKeeper to the upgrade handler, it enables the necessary functionality for managing vault parameters during the upgrade process.

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

3-3: LGTM!

The import statement is correctly adding the new types QueryVaultParamsRequest and QueryVaultParamsResponseSDKType related to the vault params query functionality.


Line range hint 18-81: Implementation looks good!

The vaultParams method is implemented correctly:

  • The method signature is consistent with the imported types.
  • The endpoint URL is constructed properly using the type and number properties from the request.
  • The method is bound correctly in the constructor.
  • The method is documented appropriately.

The implementation adds the capability to query params of a specific vault and is consistent with the existing codebase. No potential issues identified.

protocol/app/upgrades/v7.0.0/upgrade_container_test.go (4)

6-7: LGTM!

The import statement for the github.com/cosmos/gogoproto/proto package is valid and necessary for using protocol buffers in the code.


40-49: LGTM!

The changes to the postUpgradeChecks function to include a call to postUpgradeVaultParamsCheck are logically correct and follow the expected pattern for upgrade tests. The empty preUpgradeSetups and preUpgradeChecks functions serve as placeholders for adding pre-upgrade logic if needed in the future.


51-80: LGTM!

The postUpgradeVaultParamsCheck function is well-structured and follows a clear logic flow. It correctly queries the vault parameters, unmarshals the query response, and compares the actual quoting parameters with the expected quoting parameters to ensure the correctness of the migration. The code logic and syntax are sound.


82-100: LGTM!

The code segment for checking vaults without quoting parameters follows a similar logic flow as the previous segment and is implemented correctly. It queries the vault parameters, unmarshals the query response, and verifies that the quoting parameters are set to nil. The code logic and syntax are sound.

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

38-41: LGTM!

The new VaultParams RPC method in the Query service looks good:

  • It allows querying vault parameters by providing the vault type and number.
  • The HTTP GET annotation follows the RESTful principles for querying resources.
  • The endpoint URL is structured correctly, including the vault type and number as path parameters.

110-121: Looks good!

The new QueryVaultParamsRequest and QueryVaultParamsResponse message types are well-defined:

  • QueryVaultParamsRequest includes the necessary fields (VaultType and number) to query vault parameters.
  • QueryVaultParamsResponse returns the queried VaultId and VaultParams.
  • The fields in both messages are defined with the correct types.
  • The (gogoproto.nullable) = false option ensures the fields in QueryVaultParamsResponse are always present.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (5)

4-4: LGTM!

The import statement has been correctly updated to include the necessary request and response types for the new vaultParams method.


22-24: LGTM!

The vaultParams method has been correctly added to the Query interface with the appropriate request and response types.


36-36: LGTM!

The vaultParams method has been correctly bound to the class context in the constructor of the QueryClientImpl class.


73-77: LGTM!

The vaultParams method has been correctly implemented in the QueryClientImpl class, following the same pattern as the other methods and correctly encoding and decoding the request and response.


102-105: LGTM!

The vaultParams method has been correctly added to the object returned by the createRpcQueryExtension function, allowing it to be accessed through the RPC query extension.

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

156-161: LGTM!

The QueryVaultParamsRequest interface is defined correctly with appropriate property types.


162-167: LGTM!

The QueryVaultParamsRequestSDKType interface is defined correctly with appropriate property types.


168-173: LGTM!

The QueryVaultParamsResponse interface is defined correctly with appropriate optional property types.


174-179: LGTM!

The QueryVaultParamsResponseSDKType interface is defined correctly with appropriate optional property types.

protocol/testing/containertest/preupgrade_genesis.json (1)

4391-4400: New "quoting_params" section added to define the quoting strategy for the vault.

The new "quoting_params" section includes several parameters that collectively define the quoting behavior for the vault, such as:

  • The depth of the order book to consider
  • The minimum spread and additional buffer between bid and ask prices
  • The skew factor to adjust the order book
  • The size and expiration time of each order
  • The minimum quote size required to activate the strategy

These parameters allow for customization of the quoting strategy based on the desired behavior and risk profile.

Verify that the chosen parameter values are appropriate for the intended use case and align with the overall risk management strategy. Consider running simulations or backtests to validate the performance and risk characteristics of the defined quoting strategy.

protocol/app/upgrades/v7.0.0/upgrade.go (4)

13-14: Imports for vaultkeeper and vaulttypes added correctly

The necessary imports for vaultkeeper and vaulttypes have been appropriately added to support the new migration functionality.


61-63: Migration function invoked appropriately in upgrade handler

The migrateVaultQuotingParamsToVaultParams function is correctly called within CreateUpgradeHandler, ensuring that vault parameters are migrated as part of the upgrade process.


28-46: Verify the use of Unsafe methods in migration

The function migrateVaultQuotingParamsToVaultParams utilizes UnsafeGetAllVaultIds, UnsafeGetQuotingParams, and UnsafeDeleteQuotingParams. The use of Unsafe methods may bypass certain safety checks or validations. Please confirm that using these methods is appropriate in this context. If safer alternatives exist, consider using them to prevent potential issues during the migration process.

Additionally, consider handling any potential errors or exceptions that might occur during the migration to ensure robustness.


47-52: Ensure all invocations of CreateUpgradeHandler are updated

The function signature of CreateUpgradeHandler now includes the additional parameter vaultKeeper vaultkeeper.Keeper. Please verify that all calls to this function across the codebase have been updated to include the new parameter to avoid any compilation or runtime errors.

Run the following script to identify all invocations of CreateUpgradeHandler and check for the updated parameter:

Verification successful

To gather the necessary information for a comprehensive verification, I'll generate two scripts to examine the relevant files in more detail.


Changes to CreateUpgradeHandler have been correctly implemented

The CreateUpgradeHandler function in protocol/app/upgrades/v7.0.0/upgrade.go has been updated to include the vaultKeeper vaultkeeper.Keeper parameter. Additionally, the invocation of this function in protocol/app/upgrades.go has been correctly modified to pass the app.VaultKeeper argument.

  • The function definition in v7.0.0/upgrade.go includes the new parameter and uses it within the upgrade handler.
  • The invocation in upgrades.go passes all required arguments, including app.VaultKeeper.

No further changes are necessary for other versions of CreateUpgradeHandler as this update is specific to v7.0.0.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all calls to `CreateUpgradeHandler` and verify they include `vaultKeeper`.

# Test: Search for invocations of `CreateUpgradeHandler`. Expect: All include the `vaultKeeper` parameter.
rg --type go --no-heading -A2 'CreateUpgradeHandler\('

Length of output: 1647


Script:

#!/bin/bash
# Description: View the full content of protocol/app/upgrades.go, focusing on the CreateUpgradeHandler invocation

echo "Content of protocol/app/upgrades.go:"
sed -n '/CreateUpgradeHandler/,/)/p' protocol/app/upgrades.go

echo -e "\nContent of protocol/app/upgrades/v7.0.0/upgrade.go:"
sed -n '/func CreateUpgradeHandler/,/^}/p' protocol/app/upgrades/v7.0.0/upgrade.go

Length of output: 1188

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

85-87: Consistent error handling when quoting params are missing.

In UnsafeGetQuotingParams, when the quoting parameters for a vault do not exist, the function returns false without logging or additional context. Ensure that the callers of this function handle the exists flag appropriately to prevent unexpected behavior.

Comment on lines +117 to +119
vaultId, err := types.GetVaultIdFromStateKey(totalSharesIterator.Key())
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid using panic for error handling in UnsafeGetAllVaultIds.

Using panic for error handling can be risky during an upgrade process. It's better to handle the error gracefully or return it to the caller for proper handling.

Apply this diff to handle the error without panicking:

 for ; totalSharesIterator.Valid(); totalSharesIterator.Next() {
 	vaultId, err := types.GetVaultIdFromStateKey(totalSharesIterator.Key())
 	if err != nil {
-		panic(err)
+		// Log the error and continue processing or return the error
+		return nil
 	}
 	vaultIds = append(vaultIds, *vaultId)
 }

Consider modifying the function signature to return an error:

func (k Keeper) UnsafeGetAllVaultIds(ctx sdk.Context) ([]types.VaultId, error) {
    // function body
}

Comment on lines 93 to 95
// Deprecated: Only used to set quoting params as they were in v6.x.
// UnsafeDeleteQuotingParams deletes quoting parameters for a given vault from state.
// Used for v7.x upgrade handler
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct the function comment for UnsafeDeleteQuotingParams.

The comment incorrectly states that the function is used to set quoting params, but it actually deletes them. Updating the comment improves code readability and avoids confusion.

Apply this diff to fix the comment:

-// Deprecated: Only used to set quoting params as they were in v6.x.
+// Deprecated: Only used to delete quoting params as they were in v6.x.
 // UnsafeDeleteQuotingParams deletes quoting parameters for a given vault from state.
 // Used for v7.x upgrade handler
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
// Deprecated: Only used to set quoting params as they were in v6.x.
// UnsafeDeleteQuotingParams deletes quoting parameters for a given vault from state.
// Used for v7.x upgrade handler
// Deprecated: Only used to delete quoting params as they were in v6.x.
// UnsafeDeleteQuotingParams deletes quoting parameters for a given vault from state.
// Used for v7.x upgrade handler

) {
store := ctx.KVStore(k.storeKey)
b := store.Get([]byte("Params"))
k.cdc.MustUnmarshal(b, &params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Handle potential nil value before unmarshaling Params.

In UnsafeGetParams, the code calls k.cdc.MustUnmarshal(b, &params) without checking if b is nil. If the Params key does not exist, store.Get will return nil, and MustUnmarshal will panic. To prevent a potential panic, add a check for nil before unmarshaling.

Apply this diff to fix the issue:

 func (k Keeper) UnsafeGetParams(
 	ctx sdk.Context,
 ) (
 	params types.QuotingParams,
 ) {
 	store := ctx.KVStore(k.storeKey)
 	b := store.Get([]byte("Params"))
+	if b == nil {
+		return params
+	}
 	k.cdc.MustUnmarshal(b, &params)
 	return params
 }
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
k.cdc.MustUnmarshal(b, &params)
func (k Keeper) UnsafeGetParams(
ctx sdk.Context,
) (
params types.QuotingParams,
) {
store := ctx.KVStore(k.storeKey)
b := store.Get([]byte("Params"))
if b == nil {
return params
}
k.cdc.MustUnmarshal(b, &params)
return params
}

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

Outside diff range and nitpick comments (2)
protocol/app/upgrades/v7.0.0/upgrade.go (2)

29-45: Adopt Go naming conventions for 'ID' in variable names

In Go, it's conventional to capitalize 'ID' in identifiers for consistency and readability. Consider renaming vaultIds to vaultIDs and vaultId to vaultID.

Apply this diff to update the variable names:

 func migrateVaultQuotingParamsToVaultParams(ctx sdk.Context, k vaultkeeper.Keeper) {
-	vaultIds := k.UnsafeGetAllVaultIds(ctx)
+	vaultIDs := k.UnsafeGetAllVaultIDs(ctx)

-	ctx.Logger().Info(fmt.Sprintf("Migrating quoting parameters of %d vaults", len(vaultIds)))
+	ctx.Logger().Info(fmt.Sprintf("Migrating quoting parameters of %d vaults", len(vaultIDs)))

-	for _, vaultId := range vaultIds {
+	for _, vaultID := range vaultIDs {

-		quotingParams, exists := k.UnsafeGetQuotingParams(ctx, vaultId)
+		quotingParams, exists := k.UnsafeGetQuotingParams(ctx, vaultID)

 		vaultParams := vaulttypes.VaultParams{
 			Status: vaulttypes.VaultStatus_VAULT_STATUS_QUOTING,
 		}
 		if exists {
 			vaultParams.QuotingParams = &quotingParams
 		}
-		k.SetVaultParams(ctx, vaultId, vaultParams)
-		k.UnsafeDeleteQuotingParams(ctx, vaultId)
+		k.SetVaultParams(ctx, vaultID, vaultParams)
+		k.UnsafeDeleteQuotingParams(ctx, vaultID)
 		ctx.Logger().Info(fmt.Sprintf(
-			"Successfully migrated vault %+v",
-			vaultId,
+			"Successfully migrated vault %v",
+			vaultID,
 		))
 	}
 }

42-44: Use appropriate format specifier in logging

When logging vaultID, using %v is sufficient since it's a basic type (e.g., integer or string). The %+v specifier is typically used for structs to display field names and values. Consider changing %+v to %v for clarity.

Apply this diff to update the format specifier:

 		ctx.Logger().Info(fmt.Sprintf(
-			"Successfully migrated vault %+v",
+			"Successfully migrated vault %v",
 			vaultID,
 		))
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between cee80fc and 3780a04.

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 (10)
  • proto/dydxprotocol/vault/query.proto (2 hunks)
  • protocol/app/upgrades.go (1 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade.go (3 hunks)
  • protocol/app/upgrades/v7.0.0/upgrade_container_test.go (2 hunks)
  • protocol/testing/containertest/preupgrade_genesis.json (1 hunks)
  • protocol/x/vault/keeper/deprecated_state.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params.go (1 hunks)
  • protocol/x/vault/keeper/grpc_query_vault_params_test.go (1 hunks)
  • protocol/x/vault/keeper/params.go (0 hunks)
  • protocol/x/vault/types/types.go (1 hunks)
Files not reviewed due to no reviewable changes (1)
  • protocol/x/vault/keeper/params.go
Additional comments not posted (22)
protocol/x/vault/types/types.go (3)

34-34: Verify the impact of removing the vaultId parameter.

The removal of the vaultId parameter suggests that the function now returns the total shares across all vaults. Please ensure that this change aligns with the intended functionality and doesn't break any existing code that relies on retrieving the total shares for a specific vault.

Run the following script to verify the usage of GetTotalShares:

#!/bin/bash
# Description: Verify the usage of `GetTotalShares` and ensure that the removal of the `vaultId` parameter doesn't break existing code.

# Test: Search for the function usage. Expect: No occurrences of `GetTotalShares` with a `vaultId` argument.
rg --type go -A 5 $'GetTotalShares'

40-43: Verify the impact of removing the vaultId parameter and changing the return type.

The removal of the vaultId parameter suggests that the function now mints shares across all vaults. Please ensure that this change aligns with the intended functionality and doesn't break any existing code that relies on minting shares for a specific vault.

Additionally, the change in the return type to return a pointer to big.Int along with an error provides more flexibility in handling the minted shares and any errors that may occur during the minting process. Please ensure that the callers of this function are updated to handle the new return type appropriately.

Run the following script to verify the usage of MintShares:

#!/bin/bash
# Description: Verify the usage of `MintShares` and ensure that the removal of the `vaultId` parameter and the change in the return type don't break existing code.

# Test: Search for the function usage. Expect: No occurrences of `MintShares` with a `vaultId` argument, and the callers should handle the new return type.
rg --type go -A 5 $'MintShares'

36-39: Verify the impact of removing the vaultId parameter.

The removal of the vaultId parameter suggests that the function now sets the total shares across all vaults. Please ensure that this change aligns with the intended functionality and doesn't break any existing code that relies on setting the total shares for a specific vault.

Run the following script to verify the usage of SetTotalShares:

Verification successful

To gather more information about the current implementation and usage of SetTotalShares, let's run some additional searches.


Based on the analysis of the provided information, I can now generate the final response regarding the removal of the vaultId parameter from the SetTotalShares function.

Removal of vaultId parameter is part of intentional architectural change

The removal of the vaultId parameter from SetTotalShares is consistent with a larger architectural shift in the vault system. The codebase has moved from managing multiple individual vaults to a single "megavault" system. This change simplifies share management and aligns with the new megavault concept.

However, there are a few points that need attention:

  • The mock VaultKeeper in protocol/mocks/VaultKeeper.go still includes a SetTotalShares method with a vaultId parameter. This should be updated to match the current implementation.
  • Any remaining test files using the old SetTotalShares signature should be reviewed and updated.
  • Consider updating documentation to explain the shift to the megavault system and its implications for developers and users.

These changes ensure consistency across the codebase and prevent potential confusion or errors in the future.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `SetTotalShares` and ensure that the removal of the `vaultId` parameter doesn't break existing code.

# Test: Search for the function usage. Expect: No occurrences of `SetTotalShares` with a `vaultId` argument.
rg --type go -A 5 $'SetTotalShares'

Length of output: 2940


Script:

#!/bin/bash
# Description: Verify the existence of SetTotalShares and examine recent changes

# Search for SetTotalShares
echo "Searching for SetTotalShares:"
rg --type go -A 5 'SetTotalShares'

# Check git history for recent changes to SetTotalShares
echo -e "\nChecking git history for SetTotalShares:"
git log -p -S 'SetTotalShares' -- '*.go'

# Examine MintShares function
echo -e "\nExamining MintShares function:"
ast-grep --lang go --pattern 'func $_ MintShares($_) $_ {
  $$$
}'

Length of output: 140053


Script:

#!/bin/bash
# Description: Verify the current implementation and usage of SetTotalShares

# Check the current implementation of SetTotalShares
echo "Current implementation of SetTotalShares:"
rg --type go -A 10 'func.*SetTotalShares'

# Look for any remaining uses of SetTotalShares with a vaultId parameter
echo -e "\nUsages of SetTotalShares with vaultId parameter:"
rg --type go 'SetTotalShares\s*\(\s*ctx.*vaultId'

# Search for comments or documentation about this change
echo -e "\nComments or documentation about the change:"
rg --type go -i "megavault|global vault|removed vaultId"

# Look for new functions that might have replaced vault-specific functionality
echo -e "\nPossible new vault-related functions:"
rg --type go 'func.*Vault'

Length of output: 111896

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

13-37: LGTM!

The new VaultParams function provides a clean and structured way to query vault parameters via gRPC. The implementation handles errors appropriately for nil requests and non-existent vaults. This addition enhances the gRPC interface for vault management and can be beneficial for external systems interacting with the vault management system.

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

13-86: Excellent test coverage!

The TestVaultParamsQuery function provides comprehensive test coverage for the VaultParams query method. The test cases cover various scenarios, including successful queries, handling of specific vault statuses, and error cases. The use of table-driven tests and the testify package for assertions makes the tests readable and maintainable.

The test setup and assertions are clear and concise, and the use of constants from the testutil/constants package improves readability and maintainability. The error messages in the test cases provide clear context for the expected errors.

Overall, this test function ensures the reliability and correctness of the vault querying functionality. Great job!

protocol/app/upgrades.go (1)

34-34: LGTM!

The inclusion of app.VaultKeeper as a parameter to the v7_0_0.CreateUpgradeHandler function call is a valid enhancement to the upgrade handling process. It integrates the VaultKeeper component, potentially expanding the capabilities of the upgrade handler to manage vault-related functionalities or data during application upgrades.

This change does not alter the existing logic or control flow significantly but could be critical for maintaining state or ensuring consistency of vault-related data during upgrades.

protocol/app/upgrades/v7.0.0/upgrade_container_test.go (3)

51-100: LGTM!

The postUpgradeVaultParamsCheck function is well-structured and thoroughly tests the migration of vault parameters during an upgrade. It covers two important test cases:

  1. A vault with quoting parameters, where it verifies that the expected quoting parameters are correctly migrated and the vault status is set to VAULT_STATUS_QUOTING.
  2. A vault without quoting parameters, where it checks that the vault status is VAULT_STATUS_QUOTING and the quoting parameters are nil.

The function uses containertest.Query to query vault parameters and require.Equal to assert the expected values, which are good practices for integration tests in Go.

The code is also well-documented with comments explaining the purpose of each test case.


40-40: Skipped reviewing the empty function.

The preUpgradeSetups function is empty, so there is nothing to review at the moment. It seems to be a placeholder for future implementation of setup tasks before an upgrade.


45-49: LGTM!

The postUpgradeChecks function serves as a wrapper to call postUpgradeVaultParamsCheck, which performs the necessary post-upgrade checks related to vault parameters. This is a good practice to keep the test function focused and modular.

protocol/x/vault/keeper/deprecated_state.go (6)

27-39: LGTM!

The function correctly retrieves the vault parameters from the v5.x state for use in the v6.x upgrade handler. The deprecation comment is appropriate.


41-49: LGTM!

The function correctly deletes the vault parameters from the v5.x state for use in the v6.x upgrade handler. The deprecation comment is appropriate.


53-70: LGTM!

The function correctly sets the quoting parameters for a given vault in the v6.x state for use in the v7.x upgrade handler. The deprecation comment is appropriate, and the function includes necessary validation and error handling.


72-91: LGTM!

The function correctly retrieves the quoting parameters for a given vault from the v6.x state for use in the v7.x upgrade handler. The deprecation comment is appropriate, and the function handles the case when quoting parameters are not found.


93-106: LGTM!

The function correctly deletes the quoting parameters for a given vault from the v6.x state for use in the v7.x upgrade handler. The deprecation comment is appropriate, and the function handles the case when quoting parameters are not found.


108-124: LGTM!

The function correctly retrieves all vault IDs from the v6.x state using the deprecated total shares state. The deprecation comment is appropriate, and the function handles the extraction of vault IDs from the state keys.

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

38-41: LGTM!

The new RPC method VaultParams is well-defined with clear request and response types. The HTTP mapping follows the standard convention and allows querying vault params by type and number.


110-121: Looks good!

The new message types QueryVaultParamsRequest and QueryVaultParamsResponse are well-structured to support the VaultParams RPC method. The fields are clearly defined with appropriate types and options.

protocol/testing/containertest/preupgrade_genesis.json (1)

4391-4400: LGTM!

The addition of the quoting_params object within the vaults array is consistent with the PR objective of introducing an upgrade handler for migrating vault quoting parameters. The parameters within the object seem reasonable and are likely aimed at enhancing the precision and control over trading operations.

protocol/app/upgrades/v7.0.0/upgrade.go (4)

13-14: Add necessary vault imports

The addition of vaultkeeper and vaulttypes imports is appropriate for the migration functionality.


28-46: Migration function effectively migrates vault quoting parameters

The migrateVaultQuotingParamsToVaultParams function correctly migrates quoting parameters to the new VaultParams structure. It handles both the existence and absence of quoting parameters appropriately.


52-52: Update function signature to include vaultKeeper

The CreateUpgradeHandler function signature has been correctly updated to include vaultKeeper, allowing access to vault data during the upgrade process.


61-62: Integrate migration function into the upgrade handler

Incorporating migrateVaultQuotingParamsToVaultParams within the upgrade handler ensures that vault quoting parameters are migrated seamlessly during the upgrade.

@vincentwschau vincentwschau merged commit 18f76cc into main Sep 20, 2024
39 checks passed
@vincentwschau vincentwschau deleted the vincentc/tra-625-upgrade-quoting-params branch September 20, 2024 17:26
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