allow operator and governance to retrieve from vaults #2290
Conversation
WalkthroughThis pull request introduces a new functionality to the vault management system by adding the ability to retrieve funds from a vault. It includes the implementation of the Changes
Possibly related PRs
Suggested reviewers
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.
Caution
Inline review comments failed to post
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
- proto/dydxprotocol/vault/tx.proto (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs_test.go (1 hunks)
- protocol/x/vault/client/cli/tx.go (2 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault.go (1 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go (1 hunks)
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 233-233: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 236-236: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (30)
protocol/x/vault/keeper/msg_server_retrieve_from_vault.go (5)
14-17: LGTM!The function signature and documentation are clear and accurately describe the purpose of the function.
21-28: LGTM!The authority validation logic is correct. It checks if the authority is either a module authority or the operator, and returns an appropriate error if the authority is invalid.
30-33: LGTM!The vault existence check is correct. It retrieves the vault parameters using the provided
VaultIdand returns an appropriate error if the vault doesn't exist.
35-44: LGTM!The fund transfer logic is implemented correctly:
- It uses the correct method
TransferFundsFromSubaccountToSubaccountto transfer funds.- It correctly specifies the source subaccount using the provided
VaultIdand the destination subaccount as the main vault.- It uses the correct asset type (USDC) for the transfer.
- It correctly retrieves the transfer amount from the
QuoteQuantumsfield of the message.- It propagates any errors returned by the transfer method.
46-47: LGTM!The function correctly returns an empty response on successful execution and propagates any errors encountered during the process.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4)
3-3: LGTM!The import statement correctly includes the new message types
MsgRetrieveFromVaultandMsgRetrieveFromVaultResponserelated to the retrieval functionality.
27-29: LGTM!The
retrieveFromVaultmethod has been correctly added to theMsginterface with the appropriate request and response types. The JSDoc comment provides a clear description of the method's purpose to retrieve funds from a vault to the main vault.
42-42: LGTM!The
retrieveFromVaultmethod is correctly bound in the constructor of theMsgClientImplclass, ensuring it has the rightthiscontext when called.
81-85: LGTM!The
retrieveFromVaultmethod has been implemented correctly in theMsgClientImplclass. It follows the same pattern as the other methods:
- Encodes the request using
MsgRetrieveFromVault.encode(request).finish().- Sends an RPC request using
this.rpc.request("dydxprotocol.vault.Msg", "RetrieveFromVault", data).- Decodes the response using
MsgRetrieveFromVaultResponse.decode(new _m0.Reader(data)).- Returns a Promise that resolves to the decoded response.
The implementation is consistent and handles the request and response correctly.
protocol/x/vault/client/cli/tx.go (1)
197-245: LGTM!The
CmdRetrieveFromVaultfunction introduces a new CLI command that enables retrieving funds from a vault. The implementation follows the existing patterns and conventions used in the codebase, ensuring consistency and maintainability. Here are the key aspects of the implementation:
- The function correctly parses the vault type, vault number, and quantums from the command arguments using appropriate utility functions.
- It constructs a
MsgRetrieveFromVaultmessage with the parsed parameters, adhering to the expected message structure.- The existing transaction broadcasting mechanism is utilized to handle the transaction, either generating or broadcasting it based on the provided flags.
- The necessary transaction flags are added to the command, ensuring proper CLI functionality and user interaction.
Overall, the implementation enhances the functionality of the vault management system by providing a convenient way to interact with vaults through the CLI.
proto/dydxprotocol/vault/tx.proto (3)
37-39: LGTM!The new
RetrieveFromVaultRPC method is properly defined in theMsgservice, following the existing naming conventions and syntax.
159-174: Looks good!The
MsgRetrieveFromVaultmessage is well-defined with appropriate fields and options:
- The
authorityfield with(cosmos.msg.v1.signer)option ensures that only authorized addresses can perform the retrieval.- The
vault_idfield correctly specifies the source vault for the retrieval.- The
quote_quantumsfield uses theSerializableIntcustom type for proper serialization and is marked as non-nullable to enforce a valid value.
176-177: Approved.The
MsgRetrieveFromVaultResponsemessage is defined correctly as an empty message, which is valid for the response type of theRetrieveFromVaultRPC method if no specific response data is needed.protocol/app/msgs/normal_msgs_test.go (1)
154-155: LGTM! The test coverage has been expanded to include the new vault retrieval functionality.The addition of the new message types
MsgRetrieveFromVaultandMsgRetrieveFromVaultResponseto theexpectedMsgsslice ensures that these messages are properly registered and can be processed by the system. This change enhances the test coverage for the vault-related features.protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go (8)
19-253: Comprehensive test suite forRetrieveFromVaultfunctionality.The
TestMsgRetrieveFromVaultfunction provides a thorough set of test cases to validate the behavior of theRetrieveFromVaultmessage under various scenarios. The test cases cover successful retrievals, failures due to insufficient funds, invalid authorities, and attempts to access non-existent vaults.The use of table-driven testing allows for easy maintenance and extension of the test suite. The test function sets up the mock application environment based on the test case parameters, executes the message, and verifies the outcomes against the expected results.
Overall, this test function ensures the robustness and correctness of the
RetrieveFromVaultfunctionality.
36-48: LGTM!This test case correctly verifies a successful retrieval of 50 quote quantums from Vault Clob 0 by the governance authority. The setup ensures that the retrieval amount is within the available balance of the sub vault, and the vault is in the appropriate status for retrieval.
49-61: Looks good!This test case correctly verifies a successful retrieval of all quote quantums (3,500,000) from Vault Clob 1 by the operator authority. The setup ensures that the retrieval amount matches the entire balance of the sub vault, and the vault is in the "close_only" status, allowing for a complete withdrawal.
62-75: Test case looks good!This test case correctly verifies a failure scenario where the requested retrieval amount (27 quote quantums) exceeds the available balance (26 quote quantums) in the sub vault of Vault Clob 0. The setup ensures that the retrieval fails with the expected error message "failed to apply subaccount updates" and that the vault balances remain unchanged.
76-93: Well-crafted test case!This test case correctly verifies a successful retrieval scenario where the sub vault has an open position and the retrieval amount (1,925,000 quote quantums) exactly meets the initial margin requirement. The setup accurately calculates the open notional, equity, and initial margin requirement based on the position size and the initial margin factor (IMF).
The test case ensures that the retrieval succeeds without any errors and that the vault balances are updated accordingly.
94-112: Excellent test case!This test case correctly verifies a failure scenario where the sub vault has an open position and the retrieval amount (1,925,001 quote quantums) would result in undercollateralization. The setup accurately calculates the open notional, equity, and initial margin requirement based on the position size and the initial margin factor (IMF).
The test case ensures that the retrieval fails with the expected error message "NewlyUndercollateralized" and that the vault balances remain unchanged.
113-123: Test case looks good!This test case correctly verifies a failure scenario where the retrieval is attempted from a non-existent vault (Vault Clob 0). The setup ensures that the retrieval fails with the expected error message "ErrVaultParamsNotFound" and that the vault balances remain unchanged.
124-145: Test cases for invalid and empty authority look good!The test cases "Failure - Invalid Authority" and "Failure - Empty Authority" correctly verify failure scenarios where the retrieval is attempted with an invalid authority or an empty authority.
In the "Failure - Invalid Authority" test case, the operator is set to Bob's address, but the retrieval message specifies Alice's address as the authority. This mismatch should result in an "ErrInvalidAuthority" error.
In the "Failure - Empty Authority" test case, the authority field in the retrieval message is left empty, which should also result in an "ErrInvalidAuthority" error.
Both test cases ensure that the retrieval fails with the expected error message and that the vault balances remain unchanged.
protocol/app/msgs/normal_msgs.go (2)
253-253: LGTM!The addition of the
MsgRetrieveFromVaultmessage type to theNormalMsgsDydxCustommap is correct and consistent with the existing code.
254-254: Looks good!The addition of the
MsgRetrieveFromVaultResponsekey-value pair with anilvalue is consistent with the existing code and follows the common practice for response message types.protocol/app/msgs/all_msgs.go (1)
256-257: LGTM!The addition of the new message types
MsgRetrieveFromVaultandMsgRetrieveFromVaultResponsealigns with the PR objective of enabling operators and governance to retrieve from vaults. The entries follow the existing naming conventions in theAllTypeMessagesmap.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (5)
211-219: LGTM!The
MsgRetrieveFromVaultinterface is well-defined and captures the necessary request parameters for a vault retrieval operation.
869-875: LGTM!The
createBaseMsgRetrieveFromVaultfunction correctly initializes aMsgRetrieveFromVaultobject with default values.
878-892: LGTM!The
MsgRetrieveFromVault.encodefunction correctly encodes aMsgRetrieveFromVaultmessage to a binary format using aWriterobject, following the protobuf encoding conventions.
894-922: LGTM!The
MsgRetrieveFromVault.decodefunction correctly decodes a binary-encodedMsgRetrieveFromVaultmessage using aReaderobject, following the protobuf decoding conventions.
924-930: LGTM!The
MsgRetrieveFromVault.fromPartialfunction correctly creates aMsgRetrieveFromVaultmessage from a partial object, handling the field assignments and default values properly.
Comments failed to post (1)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (1)
233-233: Use a type alias instead of an empty interface.
An empty interface is equivalent to
{}. Consider using a type alias instead for better readability and to avoid the empty interface lint error.-export interface MsgRetrieveFromVaultResponse {} +export type MsgRetrieveFromVaultResponse = Record<string, never>;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.export type MsgRetrieveFromVaultResponse = Record<string, never>;Tools
Biome
[error] 233-233: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
- proto/dydxprotocol/vault/tx.proto (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs_test.go (1 hunks)
- protocol/x/vault/client/cli/tx.go (2 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault.go (1 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts
- proto/dydxprotocol/vault/tx.proto
- protocol/app/msgs/all_msgs.go
- protocol/app/msgs/normal_msgs.go
- protocol/app/msgs/normal_msgs_test.go
- protocol/x/vault/client/cli/tx.go
- protocol/x/vault/keeper/msg_server_retrieve_from_vault.go
- protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 233-233: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 236-236: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (1)
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (1)
211-219: LGTM!The
MsgRetrieveFromVaultinterface looks good. The properties are correctly typed and match the naming conventions.
| export interface MsgRetrieveFromVaultResponse {} | ||
| /** MsgRetrieveFromVaultResponse is the Msg/RetrieveFromVault response type. */ | ||
|
|
||
| export interface MsgRetrieveFromVaultResponseSDKType {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
The MsgRetrieveFromVaultResponseSDKType interface is currently empty, which is equivalent to {}. Using an empty interface is not recommended as it can lead to confusion and maintenance issues.
Consider using a type alias instead:
-export interface MsgRetrieveFromVaultResponseSDKType {}
+export type MsgRetrieveFromVaultResponseSDKType = Record<string, never>;This provides a cleaner and more explicit way to represent an empty type.
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.
| export interface MsgRetrieveFromVaultResponseSDKType {} | |
| export type MsgRetrieveFromVaultResponseSDKType = Record<string, never>; |
Tools
Biome
[error] 236-236: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| } | ||
| /** MsgRetrieveFromVaultResponse is the Msg/RetrieveFromVault response type. */ | ||
|
|
||
| export interface MsgRetrieveFromVaultResponse {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
The MsgRetrieveFromVaultResponse interface is currently empty, which is equivalent to {}. Using an empty interface is not recommended as it can lead to confusion and maintenance issues.
Consider using a type alias instead:
-export interface MsgRetrieveFromVaultResponse {}
+export type MsgRetrieveFromVaultResponse = Record<string, never>;This provides a cleaner and more explicit way to represent an empty type.
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.
| export interface MsgRetrieveFromVaultResponse {} | |
| export type MsgRetrieveFromVaultResponse = Record<string, never>; |
Tools
Biome
[error] 233-233: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Changelist
add
MsgRetrieveFromVaultand cli for itTest Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
RetrieveFromVaultcommand and associated message types.Bug Fixes
Tests