add ability to allocate to vaults #2277
Conversation
WalkthroughThe changes introduce a new functionality for vault management, specifically the ability to allocate funds from a main vault to a specific vault through the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 0
Outside diff range and nitpick comments (4)
protocol/x/vault/keeper/msg_server_allocate_to_vault.go (1)
14-42: LGTM!The
AllocateToVaultfunction looks good and aligns with the PR objectives of enhancing the functionality of the system by allowing operators and governance to allocate funds from a main vault to sub vaults. The function performs the necessary authority validation and transfers funds from the main vault to the specified vault using theTransferFundsFromSubaccountToSubaccountmethod. The error handling is clear and appropriate.Consider adding a log statement to record the successful allocation of funds. This can be helpful for auditing and debugging purposes. For example:
ctx.Logger().Info("Allocated funds to vault", "vault_id", msg.VaultId, "amount", msg.QuoteQuantums)proto/dydxprotocol/vault/tx.proto (1)
153-154: Empty response message, consider adding fields if needed.The
MsgAllocateToVaultResponsemessage is currently empty, which suggests that no specific response data is required for theAllocateToVaultmethod. This is acceptable if no additional information needs to be returned.However, consider if it would be beneficial to include any fields in the response, such as the updated vault balances or a success status, to provide more information to the client.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2)
203-205: Consider using a type alias for an empty interface.The static analysis tool suggests using a type alias instead of an empty interface for
MsgAllocateToVaultResponse. While an empty interface is valid, using a type alias can improve code clarity and maintainability.-export interface MsgAllocateToVaultResponse {} +export type MsgAllocateToVaultResponse = Record<string, never>;Tools
Biome
[error] 205-205: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
206-208: Consider using a type alias for an empty interface.Similar to the
MsgAllocateToVaultResponseinterface, the static analysis tool suggests using a type alias instead of an empty interface forMsgAllocateToVaultResponseSDKType. Consider making the same change for consistency and clarity.-export interface MsgAllocateToVaultResponseSDKType {} +export type MsgAllocateToVaultResponseSDKType = Record<string, never>;Tools
Biome
[error] 208-208: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
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/x/vault/keeper/msg_server_allocate_to_vault.go (1 hunks)
- protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go (1 hunks)
- protocol/x/vault/keeper/msg_server_set_vault_params.go (1 hunks)
- protocol/x/vault/keeper/msg_server_set_vault_params_test.go (2 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (2 hunks)
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 205-205: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 208-208: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (14)
protocol/x/vault/keeper/msg_server_set_vault_params.go (1)
23-23: LGTM!The change from
govtypes.ErrInvalidSignertotypes.ErrInvalidAuthorityenhances the clarity and specificity of error reporting in theSetVaultParamsfunction. The new error type aligns better with the context of vault authority validation.protocol/x/vault/types/expected_keepers.go (1)
83-89: LGTM!The new method
TransferFundsFromSubaccountToSubaccountis a great addition to theSubaccountsKeeperinterface. It enhances the functionality by allowing the transfer of funds between subaccounts, which aligns with the PR objective of improving the allocation capabilities within the vault system.The method signature is clear and well-defined:
- The naming is descriptive and conveys the purpose of the method.
- The parameters are well-typed and cover all the necessary information for the fund transfer.
- The error return type allows for proper error handling.
Overall, the method declaration looks good and is unlikely to cause any issues. Great job!
protocol/x/vault/types/errors.go (1)
129-133: LGTM!The new error variable
ErrInvalidAuthorityis declared correctly with a unique error code and a clear error message. This addition enhances the error handling capabilities of the module by providing a specific error case related to authority validation, which could be crucial for maintaining security and proper access control within the system.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4)
3-3: LGTM!The import statement is correct and necessary for the subsequent code changes.
24-26: LGTM!The
allocateToVaultmethod is correctly defined in theMsginterface, following the established naming convention and type patterns.
38-38: LGTM!Binding the
allocateToVaultmethod to the class context in the constructor is necessary and consistent with the existing code.
71-75: LGTM!The implementation of the
allocateToVaultmethod inMsgClientImplis correct and follows the established pattern for encoding requests, making RPC calls, and decoding responses.protocol/x/vault/keeper/msg_server_set_vault_params_test.go (2)
58-58: LGTM!The change to use
vaulttypes.ErrInvalidAuthority.Error()for the expected error message is a good improvement. It ensures consistency between the expected and actual error messages, making the test more reliable and maintainable.
67-67: Looks good!Using
vaulttypes.ErrInvalidAuthority.Error()for the expected error message is a positive change. It keeps the test in sync with the actual error type, improving the test's reliability and making it easier to maintain.proto/dydxprotocol/vault/tx.proto (2)
34-36: LGTM!The new RPC method
AllocateToVaultis well-defined and aligns with the PR objective of enabling fund allocation from the main vault to a specified vault. The method signature follows the existing naming convention.
135-151: Looks good!The
MsgAllocateToVaultmessage is well-structured and includes the necessary fields for performing fund allocation to a specified vault. The field types and annotations are appropriate, and the use of theauthorityfield ensures that only authorized addresses can perform the allocation. The custom serializable type forquote_quantumsis a good choice for representing the allocation amount.protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go (1)
19-184: Comprehensive test suite forAllocateToVaultmessage handling.The
TestMsgAllocateToVaultfunction provides a thorough and well-structured test suite for theAllocateToVaultmessage handling logic. It covers various scenarios, including:
- Success cases with different authorities (governance and operator) and varying allocation amounts.
- Failure cases such as insufficient quantums, invalid authority, and empty authority.
The test cases ensure that the message handling behaves correctly in terms of updating vault balances and returning appropriate errors. The use of descriptive names for each test case enhances readability and maintainability.
The test function follows a clear pattern of initializing a test application with custom genesis state, sending the
AllocateToVaultmessage, and verifying the expected outcomes based on the success or failure of the operation.Overall, this test function provides excellent coverage and helps ensure the correctness of the
AllocateToVaultmessage handling implementation.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2)
181-191: LGTM!The
MsgAllocateToVaultinterface is well-defined and captures the necessary properties for the allocation request. The optionalvaultIdproperty provides flexibility in specifying the target vault.
192-202: Looks good!The
MsgAllocateToVaultSDKTypeinterface correctly mirrors theMsgAllocateToVaultinterface using SDK-compatible types. This ensures compatibility with SDK implementations.
tqin7
force-pushed
the
tq/tra-610
branch
2 times, most recently
from
d106409 to
18056a0
Compare
| ) | ||
|
|
||
| // AllocateToVault allocates funds from main vault to a vault. | ||
| func (k msgServer) AllocateToVault( |
There was a problem hiding this comment.
What happens if the vault doesn't exist in state?
There was a problem hiding this comment.
i was thinking that the operator needs to separately set vault params to have a vault exist in state
a smoother experience is probably to set vault params with stand_by status here during allocation if the allocated-to vault doesn't exist
There was a problem hiding this comment.
i was thinking that the operator needs to separately set vault params to have a vault exist in state
Is this validated anywhere? What happens if the operator doesn't make the vault exist?
There was a problem hiding this comment.
not validated so I'd vote to set vault params with stand_by status during allocation. wdyt?
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 (15)
- 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/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testing/genesis.sh (2 hunks)
- protocol/x/vault/client/cli/tx.go (2 hunks)
- protocol/x/vault/keeper/msg_server_allocate_to_vault.go (1 hunks)
- protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go (1 hunks)
- protocol/x/vault/keeper/msg_server_set_vault_params.go (1 hunks)
- protocol/x/vault/keeper/msg_server_set_vault_params_test.go (2 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (2 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/x/vault/keeper/msg_server_allocate_to_vault.go
- protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go
- protocol/x/vault/keeper/msg_server_set_vault_params.go
- protocol/x/vault/keeper/msg_server_set_vault_params_test.go
- protocol/x/vault/types/errors.go
- protocol/x/vault/types/expected_keepers.go
Additional context used
Biome
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
[error] 205-205: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
[error] 208-208: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Additional comments not posted (10)
protocol/x/vault/client/cli/tx.go (2)
31-31: LGTM!Adding the new
CmdAllocateToVaultcommand to the list of subcommands is the correct way to extend the CLI functionality.
146-194: Implementation ofCmdAllocateToVaultlooks good!The
CmdAllocateToVaultcommand is implemented correctly:
- It parses the vault type, vault number, and quantums from the provided arguments and handles parsing errors appropriately.
- It creates the
MsgAllocateToVaultmessage using the parsed values and the provided authority.- It broadcasts or generates the transaction using the standard
tx.GenerateOrBroadcastTxCLIfunction and adds the necessary transaction flags.The command follows the standard pattern for implementing CLI commands in the codebase and enhances the functionality by allowing users to allocate funds to vaults.
protocol/app/msgs/normal_msgs_test.go (1)
150-151: LGTM!The addition of the new message types
"/dydxprotocol.vault.MsgAllocateToVault"and"/dydxprotocol.vault.MsgAllocateToVaultResponse"to theTestNormalMsgs_Keyfunction is consistent with the purpose of the test case and enhances the test coverage for vault-related operations.protocol/app/msgs/normal_msgs.go (1)
249-250: LGTM!The addition of the
MsgAllocateToVaultandMsgAllocateToVaultResponseentries to theNormalMsgsDydxCustommap is correct and consistent with the provided list of alterations.The
MsgAllocateToVaultstruct is properly mapped to enable serialization and deserialization of the message, while thenilvalue for the response indicates that no complex data is returned.These changes enable the handling of the new message type within the protocol, allowing for the allocation of funds from a main vault to sub vaults as described in the PR objectives.
protocol/app/msgs/all_msgs.go (1)
252-253: LGTM!The new message types
MsgAllocateToVaultandMsgAllocateToVaultResponseare added consistently with the existing message declarations in theAllTypeMessagesmap. The naming convention is followed, and the additions suggest an expansion of the vault management functionality within the protocol.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2)
181-191: LGTM!The
MsgAllocateToVaultinterface is well-defined with clear and appropriate properties to enable vault allocation functionality.
192-202: LGTM!The
MsgAllocateToVaultSDKTypeinterface correctly mirrors theMsgAllocateToVaultinterface using SDK types for compatibility.protocol/scripts/genesis/sample_pregenesis.json (1)
3925-3940: New subaccount structure added to the genesis configuration.The changes introduce a new subaccount object within the
subaccountsarray. The subaccount is defined with the following key components:
asset_positions: An array specifying the assets held by the subaccount, each identified by anasset_id,index, andquantumsvalue.id: An object uniquely identifying the subaccount, including anumberand anownerfield.margin_enabled: A boolean indicating whether margin trading is permitted for the subaccount.Consider adding any necessary validation or constraints for the subaccount properties to ensure data integrity and consistency within the genesis state.
To verify the subaccount structure and its usage within the codebase, you can run the following script:
This script searches for:
- Usage of the subaccount structure within the codebase, excluding test, mock, and simulation files.
- Existence of any validation functions specifically for subaccount properties.
If the script yields relevant results, it indicates that the subaccount structure is being used consistently and that there might be validation functions in place. If no results are found, it suggests that additional validation or usage of the subaccount structure may be needed.
Verification successful
Subaccount properties are verified to be in use
The properties mentioned in your review comment (AssetPositions, SubaccountId, and MarginEnabled) are indeed used extensively throughout the codebase. These properties are part of the Subaccount struct and are utilized in various critical parts of the system, including:
- Order processing and management
- Liquidations and deleveraging
- Transfers between subaccounts
- Streaming updates and event generation
The widespread use of these properties across different modules confirms their importance in the system's functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the subaccount structure is used consistently in the codebase echo "Searching for subaccount structure usage..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 $'subaccounts:\n.*asset_positions:.*\n.*id:.*\n.*margin_enabled:' # Check if there are any validation functions for the subaccount properties echo "Searching for subaccount validation functions..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 $'func.*Validate.*Subaccount'Length of output: 2348
Script:
#!/bin/bash # Check if the subaccount structure is used consistently in the codebase echo "Searching for subaccount structure usage..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -U -A 5 'subaccounts:.*\n.*asset_positions:.*\n.*id:.*\n.*margin_enabled:' # Check if there are any validation functions for the subaccount properties echo "Searching for subaccount validation functions..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 'func.*Validate.*Subaccount' # Search for more specific subaccount validation functions echo "Searching for specific subaccount property validation..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 'func.*Validate.*\b(AssetPositions|SubaccountId|MarginEnabled)\b'Length of output: 2439
Script:
#!/bin/bash # Search for a more general subaccount structure pattern echo "Searching for general subaccount structure usage..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -U -A 10 'type.*Subaccount.*struct' # Search for functions dealing with subaccount creation or modification echo "Searching for subaccount creation or modification functions..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 'func.*(Create|Update|Modify).*Subaccount' # Search for any mentions of subaccount properties echo "Searching for subaccount property mentions..." rg --type go --glob '!*_test.go' --glob '!*mock*.go' --glob '!*simulation*.go' -A 5 '\b(AssetPositions|SubaccountId|MarginEnabled)\b'Length of output: 503637
protocol/testing/genesis.sh (2)
25-26: LGTM!The new variables for the megavault main vault account address and initial balance are declared appropriately.
1829-1832: Looks good!The changes to add the megavault main vault account to the subaccounts module in the
edit_genesisfunction are implemented correctly:
- The
add_subaccountfunction is called with the appropriate address and initial balance.- The
acct_idxis incremented to maintain the correct account index.- The
total_accounts_quote_balanceis updated to include the megavault main vault account's balance.The changes follow the existing pattern and maintain consistency with the rest of the function.
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | ||
|
|
||
| export interface MsgAllocateToVaultResponseSDKType {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
Similar to MsgAllocateToVaultResponse, consider using a type alias instead of an empty interface for clarity:
-export interface MsgAllocateToVaultResponseSDKType {}
+export type MsgAllocateToVaultResponseSDKType = Record<string, never>;This change will also resolve the lint error.
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.
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | |
| export interface MsgAllocateToVaultResponseSDKType {} | |
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | |
| export type MsgAllocateToVaultResponseSDKType = Record<string, never>; |
Tools
Biome
[error] 208-208: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | ||
|
|
||
| export interface MsgAllocateToVaultResponse {} |
There was a problem hiding this comment.
Use a type alias instead of an empty interface.
An empty interface is equivalent to {} and can be confusing. Consider using a type alias for clarity:
-export interface MsgAllocateToVaultResponse {}
+export type MsgAllocateToVaultResponse = Record<string, never>;This change will also resolve the lint error.
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.
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | |
| export interface MsgAllocateToVaultResponse {} | |
| /** MsgAllocateToVaultResponse is the Msg/AllocateToVault response type. */ | |
| export type MsgAllocateToVaultResponse = Record<string, never>; |
Tools
Biome
[error] 205-205: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
Changelist
operator and governance have the ability to allocate funds from main vault to sub vaults
Test Plan
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
allocateToVaultmethod for fund allocation between vaults.AllocateToVaultto facilitate fund transfers.Bug Fixes
Tests
AllocateToVaultfunctionality, covering various allocation scenarios.Documentation
sample_pregenesis.jsonwith a defined subaccount structure.