disable deactivating vaults with positive equity #2301
Conversation
WalkthroughThe changes introduce a validation check in the Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Additional comments not posted (6)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/params_test.go (2)
155-190: Simplify Genesis State InitializationThe genesis state setup for both the
vaultandsubaccountsmodules involves conditional checks and updates. Consider refactoring this code to reduce complexity and improve maintainability, perhaps by extracting common logic into helper functions.
Line range hint
301-305: Add Validation for Empty Operator AddressSetting the operator address to an empty string should fail with a validation error. Consider adding explicit validation for the operator address to ensure it's a valid non-empty string.
Apply this diff to include address validation:
// Set invalid operator and get. invalidParams := vaulttypes.OperatorParams{ Operator: "", } err = k.SetOperatorParams(ctx, invalidParams) require.Error(t, err) +// Verify that the error is due to an empty operator address. +require.Equal(t, "operator address cannot be empty", err.Error()) require.Equal(t, newParams, k.GetOperatorParams(ctx))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/vault/keeper/params.go (1 hunks)
- protocol/x/vault/keeper/params_test.go (10 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
Additional comments not posted (8)
protocol/x/vault/types/errors.go (1)
148-152: LGTM!The introduction of the
ErrVaultDeactivationerror variable enhances the error handling capabilities of the vault module by providing a specific error condition related to vault deactivation. The error message clearly conveys the condition under which the error is triggered, i.e., when attempting to deactivate a vault with non-zero USDC balance.The unique error code (29) allows for easy identification and handling of this specific error condition in the codebase. The error is registered under the appropriate
ModuleNamenamespace, ensuring proper organization and scoping of errors within the module.Overall, this addition improves the clarity and robustness of the code's logic regarding vault management, making it explicit that vaults can only be deactivated when their USDC balance is zero.
protocol/x/vault/keeper/params.go (1)
71-76: LGTM! The added validation check enhances the vault deactivation logic.The new validation check in the
SetVaultParamsfunction prevents the deactivation of a vault if it still has a non-zero USDC position. This is a valuable safeguard that ensures vaults with active positions cannot be deactivated prematurely, maintaining the integrity of the vault system.Returning the custom error
types.ErrVaultDeactivationprovides a clear indication of the specific reason for the validation failure, making it easier to handle and troubleshoot issues related to vault deactivation.Overall, this change enhances the robustness and reliability of the vault management process.
protocol/x/vault/keeper/params_test.go (6)
17-19: Consistent Renaming of Imported TypesThe aliases
satypesandvaulttypesare correctly used for the subaccounts and vault types packages, respectively. This enhances code readability by clearly indicating which module the types are associated with.
98-113: Correct Handling of Vault Deactivation Without PositionsThe test case
"Success - Deactivate a vault with no quote quantums"appropriately checks that a vault without any quote quantums can be deactivated successfully. This ensures that vaults with no active positions can be cleanly deactivated.
202-208: Ensure Vault Params Remain Unchanged After ErrorAfter an error occurs in
SetVaultParams, the test correctly verifies that the existing vault parameters remain unchanged. This is important to maintain data integrity when an operation fails.
265-265: Default Quoting Params are Correctly RetrievedThe assertion
require.Equal(t, vaulttypes.DefaultQuotingParams(), p)confirms that when no quoting parameters are set for a vault, the default parameters are used. This ensures that vaults have valid quoting parameters even when custom ones are not defined.
286-290: Verification of Default Operator ParametersThe test confirms that the operator defaults to the governance module account at genesis, which is essential for initializing the system with proper permissions.
6-6: Verify the Usage of CometBFT PackageThe import statement
"github.com/cometbft/cometbft/types"introduces thetypespackage from CometBFT. Ensure that this package is compatible with the rest of your codebase and that there are no version conflicts or deprecation issues.Run the following script to check for consistent usage of CometBFT types:
Verification successful
CometBFT Package Usage Verified
The import of
"github.com/cometbft/cometbft/types"inprotocol/x/vault/keeper/params_test.gois consistent with its usage across the codebase. This package is widely used in both test and production code across various modules, indicating it's an integral part of the project. No inconsistencies or potential conflicts were identified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all Go files importing CometBFT types. rg --type go --glob '*.go' '"github.com/cometbft/cometbft/types"'Length of output: 4950
| "Failure - Deactivate a vault with non-zero quote quantums": { | ||
| vaultId: constants.Vault_Clob1, | ||
| existingVaultParams: &constants.VaultParams, | ||
| assetQuoteQuantums: 1, | ||
| vaultParams: &vaulttypes.VaultParams{ | ||
| Status: vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED, | ||
| }, | ||
| expectedIndexerEvents: []*indexerevents.UpsertVaultEventV1{ | ||
| { | ||
| Address: constants.Vault_Clob1.ToModuleAccountAddress(), | ||
| ClobPairId: constants.Vault_Clob1.Number, | ||
| Status: v1.VaultStatusToIndexerVaultStatus( | ||
| vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED, | ||
| ), | ||
| }, | ||
| }, | ||
| expectedErr: vaulttypes.ErrVaultDeactivation, |
There was a problem hiding this comment.
Adjust Expected Indexer Events for Failed Operations
In the test case "Failure - Deactivate a vault with non-zero quote quantums", even though an error is expected due to existing quote quantums, expectedIndexerEvents are still populated. Since the operation fails, no indexer events should be emitted.
Apply this diff to remove the expectedIndexerEvents from the failure test case:
"Failure - Deactivate a vault with non-zero quote quantums": {
vaultId: constants.Vault_Clob1,
existingVaultParams: &constants.VaultParams,
assetQuoteQuantums: 1,
vaultParams: &vaulttypes.VaultParams{
Status: vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED,
},
- expectedIndexerEvents: []*indexerevents.UpsertVaultEventV1{
- {
- Address: constants.Vault_Clob1.ToModuleAccountAddress(),
- ClobPairId: constants.Vault_Clob1.Number,
- Status: v1.VaultStatusToIndexerVaultStatus(
- vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED,
- ),
- },
- },
expectedErr: vaulttypes.ErrVaultDeactivation,
},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.
| "Failure - Deactivate a vault with non-zero quote quantums": { | |
| vaultId: constants.Vault_Clob1, | |
| existingVaultParams: &constants.VaultParams, | |
| assetQuoteQuantums: 1, | |
| vaultParams: &vaulttypes.VaultParams{ | |
| Status: vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED, | |
| }, | |
| expectedIndexerEvents: []*indexerevents.UpsertVaultEventV1{ | |
| { | |
| Address: constants.Vault_Clob1.ToModuleAccountAddress(), | |
| ClobPairId: constants.Vault_Clob1.Number, | |
| Status: v1.VaultStatusToIndexerVaultStatus( | |
| vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED, | |
| ), | |
| }, | |
| }, | |
| expectedErr: vaulttypes.ErrVaultDeactivation, | |
| "Failure - Deactivate a vault with non-zero quote quantums": { | |
| vaultId: constants.Vault_Clob1, | |
| existingVaultParams: &constants.VaultParams, | |
| assetQuoteQuantums: 1, | |
| vaultParams: &vaulttypes.VaultParams{ | |
| Status: vaulttypes.VaultStatus_VAULT_STATUS_DEACTIVATED, | |
| }, | |
| expectedErr: vaulttypes.ErrVaultDeactivation, |
tqin7
changed the title
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests