add megavault withdrawal msg #2288
Conversation
WalkthroughThe changes introduce a new feature allowing users to withdraw shares from a megavault to a specified subaccount. This includes the addition of new message types, methods, and interfaces to handle withdrawal requests and responses. The modifications span across multiple files, enhancing the vault's functionality and error handling related to withdrawals, while also updating tests to ensure the robustness of the new feature. Changes
Possibly related PRs
Suggested reviewers
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (4)
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
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 (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (3 hunks)
- proto/dydxprotocol/vault/tx.proto (2 hunks)
- protocol/app/module/interface_registry.go (1 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/testutil/encoding/utils.go (1 hunks)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault.go (1 hunks)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go (1 hunks)
- protocol/x/vault/keeper/orders.go (1 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/withdraw.go (4 hunks)
- protocol/x/vault/keeper/withdraw_test.go (15 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
Additional comments not posted (49)
protocol/x/vault/keeper/msg_server_withdraw_from_megavault.go (1)
12-31: LGTM!The
WithdrawFromMegavaultfunction is implemented correctly and aligns with the PR objective. It enhances the functionality of the vault system by allowing users to withdraw assets from a megavault, thereby improving the overall usability and flexibility of the protocol.The function correctly:
- Unwraps the SDK context.
- Calls the keeper's
WithdrawFromMegavaultmethod with the provided parameters.- Constructs and returns an appropriate response based on the success or failure of the withdrawal operation.
- Handles errors by returning them to the caller.
Great job!
protocol/x/vault/types/errors.go (2)
138-142: LGTM!The new error variable
ErrInsufficientWithdrawableSharesis declared correctly using theerrorsmod.Registerfunction. The error code is unique, and the error message accurately describes the error condition related to insufficient withdrawable shares.This addition enhances the error handling capabilities of the module by providing specific feedback to the caller.
143-147: LGTM!The new error variable
ErrInsufficientRedeemedQuoteQuantumsis declared correctly using theerrorsmod.Registerfunction. The error code is unique, and the error message accurately describes the error condition related to insufficient redeemed quote quantums.This addition enhances the error handling capabilities of the module by providing specific feedback to the caller.
protocol/testutil/encoding/utils.go (1)
96-96: LGTM!The addition of
&vaulttypes.MsgWithdrawFromMegavault{}to themsgInterfacesToRegisterslice is a valuable enhancement to the test encoding configuration. It enables more comprehensive testing scenarios involving both deposits and withdrawals from the megavault, improving the overall utility of the encoding configuration for testing purposes.The change is consistent with the existing pattern and does not introduce any breaking changes or compatibility issues.
protocol/app/module/interface_registry.go (1)
98-100: LGTM!The addition of the
dydxprotocol.vault.MsgWithdrawFromMegavaultentry in theNewInterfaceRegistryfunction is consistent with the existing code structure and patterns. It enables the handling of withdrawal messages from the Megavault, expanding the interface registry's functionality.The usage of the
getLegacyMsgSignerFnfunction with thesubaccount_idandownerparameters aligns with the other entries, indicating that the message type has a similar structure and signer requirements.Overall, the change is well-integrated and unlikely to introduce any issues or inconsistencies.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4)
3-3: LGTM!The import statement has been correctly updated to include the necessary types for the new
withdrawFromMegavaultmethod.
9-11: LGTM!The new
withdrawFromMegavaultmethod has been correctly defined in theMsginterface with the appropriate request and response types. The method documentation provides a clear description of its purpose.
37-37: LGTM!The
withdrawFromMegavaultmethod has been correctly bound to the class instance in the constructor, ensuring that it can be called correctly.
51-55: LGTM!The
withdrawFromMegavaultmethod has been correctly implemented in theMsgClientImplclass. The method follows the same pattern as the other methods in the class, correctly encoding the request, sending it to the RPC server, and decoding the response.proto/dydxprotocol/vault/tx.proto (3)
20-22: LGTM!The addition of the
WithdrawFromMegavaultRPC method to theMsgservice looks good. It follows the standard RPC definition format and expands the functionality to support megavault withdrawals.
68-89: Approve theMsgWithdrawFromMegavaultmessage definition.The
MsgWithdrawFromMegavaultmessage is well-defined and includes the necessary fields for the megavault withdrawal operation:
subaccount_id: Identifies the subaccount to credit the withdrawn funds.shares: Specifies the number of shares to withdraw.min_quote_quantums: Sets a minimum threshold for the withdrawn shares in terms of quote quantums, providing a safety check.Additionally, the
(cosmos.msg.v1.signer)option enforces that only the subaccount owner can initiate the withdrawal, enhancing security.
91-100: LGTM!The
MsgWithdrawFromMegavaultResponsemessage is well-defined and includes thequote_quantumsfield, which represents the number of quote quantums redeemed from the withdrawal. This information is valuable for clients to confirm the withdrawal outcome and reconcile balances.protocol/x/vault/keeper/vault.go (1)
66-66: LGTM!The added comment provides valuable information about the
equityvalue and clearly communicates the caller's responsibility to handle different scenarios, including negativeequityvalues. This improves the function's documentation and sets the right expectations for the caller.protocol/app/msgs/normal_msgs_test.go (1)
156-157: LGTM!The addition of the new message types for megavault withdrawal enhances the test coverage and aligns with the existing naming convention. Good job!
protocol/x/vault/keeper/withdraw.go (2)
Line range hint
18-117: LGTM! The refined slippage calculation enhances the accuracy of the withdrawal process.The changes to the
GetVaultWithdrawalSlippagefunction introduce several improvements:
- The additional parameters (
totalShares,leverage,perpetual, andmarketParam) allow for a more nuanced calculation of slippage based on the current market conditions and vault parameters.- The refined formula for estimated slippage provides a more accurate estimation of the slippage incurred during partial withdrawals by considering factors such as posterior leverage, skew factor, and spread.
- The function handles edge cases, such as zero leverage and 100% withdrawals, appropriately.
These enhancements contribute to a more precise and reliable calculation of slippage during the withdrawal process.
119-282: Excellent addition! TheWithdrawFromMegavaultfunction provides a robust and comprehensive mechanism for withdrawals.The new
WithdrawFromMegavaultfunction introduces a well-structured and thorough process for withdrawing funds from a megavault to a specified subaccount. The function encompasses several key steps:
- Checking the owner's share availability to ensure sufficient unlocked shares for withdrawal.
- Redeeming equity from the main vault based on the proportion of shares being withdrawn.
- Iterating through sub vaults to calculate slippage and transfer the appropriate amount of funds from each sub vault to the main vault.
- Handling the transfer of redeemed funds from the main vault to the destination subaccount.
- Decrementing the total and owner shares accordingly.
The function includes robust error handling for various failure scenarios, such as insufficient shares, failed transfers, and insufficient redeemed quantums. This ensures the integrity and reliability of the withdrawal process.
Overall, the
WithdrawFromMegavaultfunction is a valuable addition that enhances the functionality and security of the megavault withdrawal process.protocol/app/msgs/normal_msgs.go (1)
249-256: LGTM!The addition of the new message types for megavault withdrawal operations is consistent with the existing pattern and expands the vault-related functionality as intended.
protocol/x/vault/keeper/orders.go (1)
173-173: LGTM! The changes enhance performance and reliability.
- Passing
perpetualandmarketPriceby reference is an optimization that avoids unnecessary copies of large data structures.- The new check for positive
equityensures that the function only proceeds with the calculation if theequityis valid. This check enhances the robustness of the function.- Returning a specific error type
types.ErrNonPositiveEquitywhenequityis non-positive is a good practice. It allows the caller to handle this specific error case differently if needed.Also applies to: 177-182
protocol/x/vault/keeper/withdraw_test.go (19)
Line range hint
33-49: LGTM!The changes to the test case struct fields look good. The
leveragefield is now a*big.Rattype, which simplifies the test case structure by focusing on the leverage directly.
56-63: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
78-85: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
101-108: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
123-130: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
146-153: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
169-176: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
192-200: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case also verifies that the slippage is the same for positive and negative leverage values with the same absolute value.
216-223: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly.
238-245: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case also verifies the slippage when withdrawing 100% of the shares.
250-257: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case verifies the slippage when withdrawing a small amount of shares from a large total share pool.
273-280: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case verifies the slippage when withdrawing a significant portion of the total shares.
297-304: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case verifies the slippage when withdrawing 50% of the total shares.
321-328: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the expected slippage calculation is updated accordingly. The test case verifies the slippage when withdrawing a very high percentage (99.9999%) of the total shares.
344-352: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the test case expects an error when the vault is not found.
355-363: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the test case expects an error when the shares to withdraw is negative.
366-374: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the test case expects an error when the shares to withdraw is zero.
377-385: LGTM!The changes to the test case look good. The
leveragefield is set correctly, and the test case expects an error when the shares to withdraw is greater than the total shares.
441-449: LGTM!The changes to the
GetVaultWithdrawalSlippagefunction call look good. Theleverageparameter is now passed instead ofassetQuoteQuantumsandpositionBaseQuantums, which aligns with the changes made to the function signature.protocol/app/msgs/all_msgs.go (1)
265-266: Approve the addition of the new message types for withdrawing from the megavault.The addition of
MsgWithdrawFromMegavaultandMsgWithdrawFromMegavaultResponsemessage types under thedydxprotocol.vaultnamespace is a valid change to support the new functionality of withdrawing assets from the megavault.To ensure the new message types are properly handled, run the following script:
The script performs the following verifications:
- Checks if the message handler
WithdrawFromMegavaultexists for processingMsgWithdrawFromMegavault.- Checks if the message handler is registered in the message server.
- Checks if the proto definitions exist for the new message types.
If the script produces relevant results, it confirms that the new message types are properly integrated into the codebase. If not, further investigation and fixes may be required.
Verification successful
Confirm proper integration of new megavault withdrawal message types
The verification process has confirmed that the new message types for withdrawing from the megavault are correctly integrated into the codebase:
- The
WithdrawFromMegavaultfunction is properly implemented in the message server (protocol/x/vault/keeper/msg_server_withdraw_from_megavault.go).- A dedicated test file (protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go) exists, containing the
TestMsgWithdrawFromMegavaultfunction to ensure the new functionality is thoroughly tested.- The vault module has comprehensive test coverage, indicating good development practices and helping to ensure the reliability of the new features.
These findings confirm that the addition of
MsgWithdrawFromMegavaultandMsgWithdrawFromMegavaultResponsemessage types is well-implemented and properly tested within the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the message handler for MsgWithdrawFromMegavault exists ast-grep --lang go --pattern $'func (k Keeper) WithdrawFromMegavault(goCtx sdk.Context, msg *types.MsgWithdrawFromMegavault) (*types.MsgWithdrawFromMegavaultResponse, error) { $$$ }' # Check if the message handler is registered in the message server rg --type go -A 5 $'&types.MsgWithdrawFromMegavault' # Check if the proto definitions exist for the new message types fd --extension proto --exec rg $'MsgWithdrawFromMegavault|MsgWithdrawFromMegavaultResponse'Length of output: 1041
Script:
#!/bin/bash # Check for any function that handles MsgWithdrawFromMegavault ast-grep --lang go --pattern 'func $_($_) $_($_ *types.MsgWithdrawFromMegavault) ($_, error) { $$$ }' # Check the content of msg_server_withdraw_from_megavault.go cat protocol/x/vault/keeper/msg_server_withdraw_from_megavault.go # Check if the message handler is registered in the message server rg --type go 'server.RegisterMsgService\(' -A 10Length of output: 1520
Script:
#!/bin/bash # Check for the setup of the msgServer struct rg --type go 'type msgServer struct' # Look for the registration of the vault module's message server rg --type go 'RegisterMsgServer\(' -A 5 # Check for tests related to WithdrawFromMegavault rg --type go 'func TestWithdrawFromMegavault' -A 5Length of output: 20166
Script:
#!/bin/bash # Check if WithdrawFromMegavault is handled in the message server rg --type go 'func \(k msgServer\) WithdrawFromMegavault' -A 5 # Look for any tests related to withdrawing from megavault rg --type go 'func Test.*Withdraw.*Megavault' -A 5 # Check for any tests in the vault keeper test files rg --type go 'func Test' protocol/x/vault/keeper/*_test.go -A 5Length of output: 18423
protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go (5)
23-33: LGTM!The
VaultSetupstruct is well-defined and encapsulates all the necessary parameters for setting up a vault in the test cases. It provides a clear and organized structure for configuring vaults and their associated entities.
35-281: Excellent test coverage!The
TestMsgWithdrawFromMegavaultfunction and its associated test cases provide comprehensive coverage for theMsgWithdrawFromMegavaultfunctionality. The test cases are well-structured, clearly defined, and cover a wide range of scenarios, including successful and failed withdrawals, different vault configurations, and varying share amounts.The inclusion of edge cases and boundary conditions ensures that the functionality is thoroughly tested and robust. Great work on creating a comprehensive test suite!
283-511: Robust test case execution logic!The test case execution logic is well-implemented and follows a clear and structured flow. The custom genesis document function allows for flexible initialization of the application state based on the test case parameters, ensuring that each test case has the required setup.
The
CheckTxandDeliverTxphases are properly invoked, and the results are validated against the expected outcomes, ensuring end-to-end testing of the functionality. The test case execution logic covers all the necessary steps, from message construction to state validation.Great job on creating a robust and comprehensive test case execution logic!
286-401: Flexible and modular genesis document initialization!The custom genesis document function provides a flexible and modular way to initialize the application state for each test case. It allows for fine-grained control over the initial state of various modules and entities, such as subaccounts, vaults, prices, perpetuals, and clob pairs.
The function is well-structured and organized, with separate sections for each module's initialization. The initialization logic is based on the test case parameters, ensuring that each test case has the required setup.
This approach enhances the maintainability and extensibility of the test suite, as new modules or entities can be easily added or modified without affecting the existing code.
Great work on creating a flexible and modular genesis document initialization function!
482-509: Comprehensive equity validation logic!The equity validation logic is a crucial part of the test suite, ensuring the correctness of the withdrawal functionality. It verifies that the equity of the owner, megavault, and sub-vaults is updated correctly based on the withdrawal amount and the test case parameters.
The validation logic covers both successful and failed withdrawal scenarios, providing comprehensive testing. The comparisons are made using the
require.Equalassertions, which provide clear error messages in case of mismatches, making it easier to identify and debug any issues.This validation logic gives confidence that the withdrawal functionality is working as expected and that the equity values are being updated correctly.
Great job on implementing a comprehensive equity validation logic!
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (6)
48-60: LGTM!The
MsgWithdrawFromMegavaultinterface is well-defined with appropriate properties and types to represent the request for withdrawing shares from a megavault to a subaccount.
84-87: LGTM!The
MsgWithdrawFromMegavaultResponseinterface is well-defined with an appropriate property and type to represent the response for withdrawing shares from a megavault to a subaccount.
364-370: LGTM!The
createBaseMsgWithdrawFromMegavaultfunction correctly creates a baseMsgWithdrawFromMegavaultobject with appropriate default values for the properties.
373-387: LGTM!The
MsgWithdrawFromMegavault.encodefunction correctly encodes aMsgWithdrawFromMegavaultmessage by checking for defined properties and encoding them using the appropriate methods.
389-417: LGTM!The
MsgWithdrawFromMegavault.decodefunction correctly decodes aMsgWithdrawFromMegavaultmessage by iterating over the reader, identifying the tags, and decoding the corresponding properties using the appropriate methods.
419-425: LGTM!The
MsgWithdrawFromMegavault.fromPartialfunction correctly creates aMsgWithdrawFromMegavaultmessage from a plain object by assigning the properties from theobjectparameter and handling undefined and null values appropriately.
| perpetual *perptypes.Perpetual, | ||
| marketPrice *pricestypes.MarketPrice, |
There was a problem hiding this comment.
Tip
Codebase Verification
Add nil pointer checks to GetVaultLeverageAndEquity function
The verification process has revealed two important points:
-
The
GetVaultLeverageAndEquityfunction does not explicitly check for nil pointers, which could lead to runtime panics if nil values are passed forperpetualormarketPrice. -
All callers of the function have been correctly updated to pass pointers instead of values, matching the new function signature.
To improve the robustness of the code:
- Add nil checks for both
perpetualandmarketPriceparameters at the beginning of theGetVaultLeverageAndEquityfunction. - Review the function implementation to ensure safe usage of these pointer parameters throughout the function body.
Example of nil checks to add:
func (k Keeper) GetVaultLeverageAndEquity(
ctx sdk.Context,
vaultId types.VaultId,
perpetual *perptypes.Perpetual,
marketPrice *pricestypes.MarketPrice,
) (
leverage *big.Rat,
equity *big.Int,
err error,
) {
if perpetual == nil {
return nil, nil, errors.New("perpetual cannot be nil")
}
if marketPrice == nil {
return nil, nil, errors.New("marketPrice cannot be nil")
}
// ... rest of the function implementation
}Analysis chain
Verify the handling of pointer parameters and ensure caller updates.
The parameters for perpetual and marketPrice have been changed from value types to pointer types. This change introduces the following considerations:
-
Verify that the function correctly handles the new pointer parameters to avoid potential nil pointer dereferences. Ensure that appropriate nil checks are in place before accessing the pointed-to values.
-
Ensure that all callers of this function have been updated to pass pointers instead of values to match the new function signature.
To verify the handling of pointer parameters and the impact on callers, run the following script:
The first ast-grep command searches for nil checks on the perpetual and marketPrice parameters within the GetVaultLeverageAndEquity function. If no results are found, it indicates that the function might not be handling nil pointers correctly.
The second rg command searches for all occurrences of GetVaultLeverageAndEquity in Go files and includes five lines of context after each match. Review the results to ensure that all callers are passing pointers instead of values to match the updated function signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the function correctly handles nil pointers
ast-grep --lang go --pattern $'func (k Keeper) GetVaultLeverageAndEquity(
ctx sdk.Context,
vaultId types.VaultId,
perpetual *perptypes.Perpetual,
marketPrice *pricestypes.MarketPrice,
) (
leverage *big.Rat,
equity *big.Int,
err error,
) {
$$$
if perpetual == nil {
$$$
}
$$$
if marketPrice == nil {
$$$
}
$$$
}'
# Check if all callers have been updated to pass pointers
rg --type go -A 5 $'GetVaultLeverageAndEquity'
Length of output: 1940
protocol/x/vault/keeper/withdraw.go
Outdated
|
|
||
| _, perpetual, marketParam, marketPrice, err := k.GetVaultClobPerpAndMarket(ctx, *vaultId) | ||
| if err != nil { | ||
| log.ErrorLogWithError(ctx, "Failed to get perpetual and market", err, "Vault ID", vaultId) |
There was a problem hiding this comment.
nit: These errors maybe should also surface that they are happening during withdrawal?
| continue | ||
| } | ||
|
|
||
| slippage, err := k.GetVaultWithdrawalSlippage( |
There was a problem hiding this comment.
nit: Possibly add metric emission of slippage per vault at withdrawals for analytics?
There was a problem hiding this comment.
yeah definitely a good idea to monitor slippage. however, we need to know both "percentage of withdrawal" and "slippage"
let me think more about how to monitor slippage efficiently
There was a problem hiding this comment.
this is what i'm thinking: once queries are more built out, chameleon can monitor "how much slippage to withdraw x% from megavault"
i think monitoring slippage per vault is a bit too granular at the moment. what are your thoughts
There was a problem hiding this comment.
Yea, probably. More wondering how we catch withdrawals with high slippage.
| // Owner shares after withdrawal. | ||
| expectedOwnerShares uint64 | ||
| }{ | ||
| "Success: Withdraw some unlocked shares (5% of total), No sub-vaults, Redeemed quantums = Min quantums": { |
There was a problem hiding this comment.
nit: Test case where withdrawing > unlocked shares = failure?
protocol/x/vault/keeper/withdraw.go
Outdated
| } | ||
|
|
||
| // 4. Return error if less than min quote quantums are redeemed. | ||
| if redeemedQuoteQuantums.Cmp(minQuoteQuantums) < 0 { |
There was a problem hiding this comment.
nit: Emit some event for withdrawals (tendermint events)?
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/vault/keeper/withdraw.go (1)
178-196: Consider adding a metric to monitor the slippage per vault at withdrawals.It would be valuable to track the slippage incurred at each vault during withdrawals. This could help identify any unusual behavior or optimize the withdrawal process.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/x/vault/keeper/deposit.go (1 hunks)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go (1 hunks)
- protocol/x/vault/keeper/withdraw.go (4 hunks)
- protocol/x/vault/types/events.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go
Additional comments not posted (6)
protocol/x/vault/types/events.go (2)
23-35: LGTM!The function correctly constructs a new
deposit_to_megavaultevent with the relevant attributes. The logic is sound, and the implementation is accurate.
37-51: LGTM!The function correctly constructs a new
withdraw_from_megavaultevent with the relevant attributes. The logic is sound, and the implementation is accurate.protocol/x/vault/keeper/deposit.go (1)
47-53: LGTM!The event emission for the deposit operation is implemented correctly:
- The event is emitted at the right place, i.e., after the successful transfer.
- The event captures all the relevant details about the deposit transaction.
- The event name and fields are named appropriately and convey the necessary information.
- The event is created using the
NewDepositToMegavaultEventfunction from thetypespackage, which ensures consistency and type safety.This change enhances the functionality by providing a mechanism to notify other components of the system about the deposit event, improving traceability and potentially enabling further actions based on this event.
protocol/x/vault/keeper/withdraw.go (3)
119-287: Excellent work! TheWithdrawFromMegavaultfunction provides a robust and secure way to withdraw from a megavault to a subaccount.The function is well-structured and includes all the necessary steps to perform a withdrawal. The error handling and logging are thorough and will help with debugging and monitoring.
Line range hint
18-118: LGTM! The changes improve the accuracy and maintainability of the slippage calculation.The modifications to the function signature and the use of additional parameters directly in the calculation make the function more self-contained and accurate. The logic is well-structured and easy to follow.
Verify that all calls to
GetVaultWithdrawalSlippagehave been updated to provide the new parameters:Verification successful
Verification successful: All calls to
GetVaultWithdrawalSlippagehave been updated correctly.The function call in
protocol/x/vault/keeper/withdraw.gohas been properly updated to include all the new parameters:slippage, err := k.GetVaultWithdrawalSlippage( ctx, *vaultId, sharesToWithdraw, totalShares, leverage, &perpetual, &marketParam, )This, combined with our previous observations from the test file, confirms that all usages of the function have been correctly updated to match the new signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetVaultWithdrawalSlippage` provide the new parameters. # Test: Search for the function usage. Expect: Only occurrences with the new signature. rg --type go -A 5 $'GetVaultWithdrawalSlippage'Length of output: 2293
Script:
#!/bin/bash # Description: Verify the complete function call to GetVaultWithdrawalSlippage in withdraw.go # Display 15 lines of context after the function call rg --type go -A 15 'slippage, err := k.GetVaultWithdrawalSlippage\(' protocol/x/vault/keeper/withdraw.goLength of output: 422
119-287: Verify the error handling and logging.The function interacts with several other modules, so it's crucial to ensure that any errors returned by these modules are properly handled and logged.
Verify the error handling and logging:
Verification successful
Error handling and logging verified successfully.
The
WithdrawFromMegavaultfunction demonstrates consistent and appropriate error handling throughout its implementation. Critical errors that should halt the process are returned immediately, while less critical errors are logged to allow continued execution where possible. This approach balances robustness with the ability to process as much as possible in complex operations.Key points:
- Proper use of
log.ErrorLogWithErrorfor logging errors- Immediate returns for critical errors (e.g.,
GetSubaccountEquity, final transfer)- Logged errors with continued execution for non-critical operations (e.g., processing individual vaults)
- Correct construction and return of the
ErrInsufficientRedeemedQuoteQuantumserrorScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the error handling and logging in `WithdrawFromMegavault`. # Test 1: Check if errors returned by `GetOwnerShares` are handled. rg --type go -A 5 $'GetOwnerShares' # Test 2: Check if errors returned by `GetSubaccountEquity` are handled and logged. rg --type go -A 10 $'GetSubaccountEquity' # Test 3: Check if errors returned by `GetVaultClobPerpAndMarket` are handled and logged. rg --type go -A 10 $'GetVaultClobPerpAndMarket' # Test 4: Check if errors returned by `GetVaultLeverageAndEquity` are handled and logged. rg --type go -A 10 $'GetVaultLeverageAndEquity' # Test 5: Check if errors returned by `GetVaultWithdrawalSlippage` are handled and logged. rg --type go -A 10 $'GetVaultWithdrawalSlippage' # Test 6: Check if errors returned by `TransferFundsFromSubaccountToSubaccount` are handled and logged. rg --type go -A 15 $'TransferFundsFromSubaccountToSubaccount' # Test 7: Check if the "insufficient redeemed quote quantums" error is constructed and returned correctly. rg --type go -A 10 $'ErrInsufficientRedeemedQuoteQuantums'Length of output: 39511
| } | ||
|
|
||
| // NewWithdrawFromMegavaultEvent constructs a new withdraw_from_megavault sdk.Event. | ||
| func NewWithdrawFromMegavaultEvent( |
There was a problem hiding this comment.
non-blocking, could add in total equity of the megavault in the event, so that on it's own we can calculate the aggregate slippage of the withdraw.
protocol/x/vault/keeper/withdraw.go
Outdated
| for ; vaultParamsIterator.Valid(); vaultParamsIterator.Next() { | ||
| vaultId, err := types.GetVaultIdFromStateKey(vaultParamsIterator.Key()) | ||
| if err != nil { | ||
| log.ErrorLogWithError(ctx, "Megavault withdrawal: failed to get vault ID from state key", err) |
There was a problem hiding this comment.
nit: In the log message note that withdrawing from this vault is being skipped?
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
protocol/x/vault/keeper/msg_server_allocate_to_vault.go (2)
Line range hint
37-52: Consider extracting the vault initialization logic into a separate function.To improve code readability and reusability, consider extracting the vault initialization logic (setting vault params and adding to address store) into a separate function, e.g.,
InitializeVault(ctx, vaultID). This will make theAllocateToVaultfunction more concise and allow the initialization logic to be reused in other places if needed.Here's how you can refactor the code:
func (k msgServer) InitializeVault(ctx sdk.Context, vaultID types.VaultId) error { err := k.Keeper.SetVaultParams( ctx, vaultID, types.VaultParams{ Status: types.VaultStatus_VAULT_STATUS_STAND_BY, }, ) if err != nil { return err } k.Keeper.AddVaultToAddressStore(ctx, vaultID) return nil } func (k msgServer) AllocateToVault( goCtx context.Context, msg *types.MsgAllocateToVault, ) (*types.MsgAllocateToVaultResponse, error) { // ... _, exists = k.Keeper.GetVaultParams(ctx, msg.VaultId) if !exists { if err := k.InitializeVault(ctx, msg.VaultId); err != nil { return nil, err } } // ... }
Line range hint
1-73: Add a log message for the successful allocation.To improve observability and debugging, consider adding a log message using the
ctx.Logger().Info()function after a successful allocation to the vault. This will help in tracking and monitoring the allocations.Here's how you can add the log message:
// ... // Transfer from main vault to the specified vault. if err := k.Keeper.subaccountsKeeper.TransferFundsFromSubaccountToSubaccount( ctx, types.MegavaultMainSubaccount, *msg.VaultId.ToSubaccountId(), assetstypes.AssetUsdc.Id, msg.QuoteQuantums.BigInt(), ); err != nil { return nil, err } ctx.Logger().Info("Allocated funds to vault", "vault_id", msg.VaultId, "amount", msg.QuoteQuantums) return &types.MsgAllocateToVaultResponse{}, nilprotocol/x/vault/keeper/withdraw.go (1)
109-109: Nitpick: Consider renamingvault.SpreadPpmtovault.GetSpreadPpmfor clarity.To improve readability and maintain consistency with other function names, consider renaming
vault.SpreadPpmtovault.GetSpreadPpm.Apply this diff to rename the function:
- vault.SpreadPpm("ingParams, marketParam), + vault.GetSpreadPpm("ingParams, marketParam),
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/vault/keeper/msg_server_allocate_to_vault.go (2 hunks)
- protocol/x/vault/keeper/withdraw.go (4 hunks)
- protocol/x/vault/types/events.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/types/events.go
Additional comments not posted (4)
protocol/x/vault/keeper/msg_server_allocate_to_vault.go (1)
Line range hint
37-52: Verify the impact of adding a new vault to the address store.The changes look good in terms of initializing the vault parameters and adding the vault to the address store when it doesn't exist. However, please ensure that adding a new vault to the address store in this scenario does not have any unintended side effects or break any assumptions in other parts of the codebase that rely on the address store.
To verify the impact, run the following script:
Verification successful
Verification successful: Adding new vault to address store has no apparent side effects
The changes to initialize vault parameters and add the vault to the address store when it doesn't exist are consistent with existing usage patterns in the codebase. There's no evidence of code making assumptions about vaults in the address store that could be violated by this new behavior. The impact of adding a new vault to the address store in this scenario appears to be minimal and aligned with the intended functionality.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # - Verify if adding a new vault to the address store has any unintended side effects. # - Check if other parts of the codebase rely on the address store and if their assumptions still hold. # Test 1: Search for usages of the `AddVaultToAddressStore` function. # Expect: Only legitimate usages that align with the new behavior. rg --type go -A 5 $'AddVaultToAddressStore' # Test 2: Search for code that reads from the address store and makes assumptions about the stored vaults. # Expect: No code that assumes vaults in the address store must have a specific status or properties. rg --type go -A 10 $'GetVaultFromAddressStore|GetVaultsFromAddressStore'Length of output: 4173
protocol/x/vault/keeper/withdraw.go (3)
119-306: LGTM! TheWithdrawFromMegavaultfunction is well-structured and handles errors appropriately.The new
WithdrawFromMegavaultfunction is implemented correctly and follows best practices:
- It performs necessary checks, such as verifying the owner's available shares for withdrawal.
- It handles the withdrawal process step-by-step, including redeeming equity from the main vault, iterating through sub vaults to compute slippage and execute fund transfers, and managing the transfer of funds to the designated subaccount.
- It includes robust error handling, logging errors and skipping vaults when necessary.
- It emits an event upon successful withdrawal.
Great job on this implementation!
247-247: As mentioned by @vincentwschau in a previous review:nit: Emit some event for withdrawals (tendermint events)?
This has been addressed by emitting the
WithdrawFromMegavaultEventevent on lines 295-303.
Line range hint
18-47: Verify the function signature change in the codebase.The function signature has been updated to include additional parameters:
totalShares,leverage,perpetual, andmarketParam. This change is beneficial as it allows for a more accurate calculation of slippage by directly utilizing themarketParaminstead of relying on an intermediate variable.However, ensure that all calls to
GetVaultWithdrawalSlippagehave been updated to match the new signature throughout the codebase.Run the following script to verify the function usage:
Verification successful
To ensure we haven't missed any potential uses of the function, let's perform one more comprehensive search:
Function signature change verified successfully
The comprehensive search confirms that the
GetVaultWithdrawalSlippagefunction signature has been consistently updated throughout the codebase. All occurrences of the function, including its definition, calls, and test cases, are using the new signature with the additional parameters (totalShares, leverage, perpetual, marketParam).No instances of the old signature or partially updated calls were found, indicating a thorough and consistent implementation of the change.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetVaultWithdrawalSlippage` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'GetVaultWithdrawalSlippage'Length of output: 2293
Script:
#!/bin/bash # Description: Comprehensive search for any uses of GetVaultWithdrawalSlippage # Search for any occurrence of GetVaultWithdrawalSlippage, including comments and string literals rg --type go -i "GetVaultWithdrawalSlippage" # Search for any occurrence of just "WithdrawalSlippage" to catch any potential renamed or similarly named functions rg --type go -i "WithdrawalSlippage"Length of output: 1039
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/withdraw.go (2)
301-309: Consider emitting Tendermint events for withdrawals.In addition to the custom event, consider emitting Tendermint events for withdrawals. This can help with indexing and querying withdrawal transactions.
ctx.EventManager().EmitEvent( sdk.NewEvent( types.EventTypeWithdrawFromMegavault, sdk.NewAttribute(types.AttributeKeyOwner, toSubaccount.Owner), sdk.NewAttribute(types.AttributeKeySharesWithdrawn, sharesToWithdraw.String()), sdk.NewAttribute(types.AttributeKeyTotalShares, totalShares.String()), sdk.NewAttribute(types.AttributeKeyMegavaultEquity, megavaultEquity.String()), sdk.NewAttribute(types.AttributeKeyRedeemedQuoteQuantums, redeemedQuoteQuantums.String()), ), )
174-175: Consider logging when skipping a vault.When an error occurs while processing a vault and it is skipped, consider adding a log message to indicate that the vault is being skipped. This can help with debugging and monitoring.
log.InfoLog( ctx, "Megavault withdrawal: Skipping vault due to error", "Vault ID", vaultId, )
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go (1 hunks)
- protocol/x/vault/keeper/withdraw.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/msg_server_withdraw_from_megavault_test.go
Additional comments not posted (4)
protocol/x/vault/keeper/withdraw.go (4)
202-220: ****nit: Possibly add metric emission of slippage per vault at withdrawals for analytics?
228-234: ****question, why not just transfer from the vault to the destination subaccount rather than all to the main vault first, then the subaccount.
Might be minor, but would reduce gas usage.
Non-blocking: On this note, simply calculating the quote quantums to redeem and then bulk transferring if redeemed > min would be slightly more efficient.
Line range hint
18-47: Verify the new parameters are used correctly in the slippage calculation.The function signature has been updated to include additional parameters:
totalShares,leverage,perpetual, andmarketParam. Ensure that these parameters are being used correctly in the slippage calculation logic.Verification successful
New parameters are correctly used in the slippage calculation
The verification process confirms that the new parameters (
totalShares,leverage,perpetual, andmarketParam) are indeed used correctly in theGetVaultWithdrawalSlippagefunction:
totalSharesis used for input validation and in calculations for posterior leverage and estimated slippage.leverageis used for input validation, simple slippage calculation, and in calculations for posterior leverage and estimated slippage.perpetualis used to retrieve the liquidity tier for the simple slippage calculation.marketParamis used in the spread calculation for estimated slippage.The implementation aligns with the expected usage of these parameters in the slippage calculation logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new parameters in the slippage calculation. # Test 1: Search for the usage of `totalShares`. Expect: Used in the calculation of `posteriorLeverage`. rg --type go -A 5 $'totalShares\b' # Test 2: Search for the usage of `leverage`. Expect: Used in the calculation of `posteriorLeverage` and `estimatedSlippage`. rg --type go -A 5 $'leverage\b' # Test 3: Search for the usage of `perpetual`. Expect: Used to retrieve the liquidity tier for calculating `simpleSlippage`. rg --type go -A 5 $'perpetual\b' # Test 4: Search for the usage of `marketParam`. Expect: Used in the calculation of `estimatedSlippage`. rg --type go -A 5 $'marketParam\b'Length of output: 142
Script:
#!/bin/bash # Verify file existence and content # Find the withdraw.go file fd withdraw.go # If found, display its content fd withdraw.go -x cat {} # If cat works, try searching with grep instead of rg fd withdraw.go -x grep -n "totalShares" {} fd withdraw.go -x grep -n "leverage" {} fd withdraw.go -x grep -n "perpetual" {} fd withdraw.go -x grep -n "marketParam" {}Length of output: 13385
119-312: Ensure proper error handling and logging throughout the withdrawal process.The new
WithdrawFromMegavaultfunction performs several critical steps, including share verification, equity redemption, slippage calculation, and fund transfers. Verify that appropriate error handling and logging are in place to handle potential failures at each step.Verification successful
Error handling and logging in the withdrawal process are generally well-implemented, with one minor inconsistency.
The WithdrawFromMegavault function demonstrates robust error handling and logging practices throughout most of its implementation. Errors are properly wrapped with context-specific error types, and appropriate logging is in place. The sub vault redemption process shows good error management, logging errors with context and skipping problematic vaults.
However, there is one instance of inconsistent error handling:
if err != nil { log.ErrorLogWithError(ctx, "Megavault withdrawal: failed to get megavault main vault equity", err) return nil, err }Consider wrapping this error for consistency with the rest of the function, e.g.:
if err != nil { log.ErrorLogWithError(ctx, "Megavault withdrawal: failed to get megavault main vault equity", err) return nil, errorsmod.Wrap(err, "failed to get megavault main vault equity") }This minor adjustment would improve the overall consistency of error handling in the function.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling and logging in the withdrawal process. # Test 1: Search for error handling when checking shares to withdraw. Expect: Relevant error handling and logging. rg --type go -A 10 $'sharesToWithdraw.Sign\(\) <= 0' # Test 2: Search for error handling when redeeming from the main vault. Expect: Relevant error handling and logging. rg --type go -A 10 $'redeemedQuoteQuantums = new\(big.Int\).Set\(megavaultEquity\)' # Test 3: Search for error handling when redeeming from sub vaults. Expect: Relevant error handling and logging for each potential failure point (getting vault ID, perpetual, market, leverage, equity, slippage, and fund transfer). rg --type go -A 10 $'for ; vaultParamsIterator.Valid\(\); vaultParamsIterator.Next\(\) {' # Test 4: Search for error handling when transferring funds to the destination subaccount. Expect: Relevant error handling and logging. rg --type go -A 10 $'err = k.subaccountsKeeper.TransferFundsFromSubaccountToSubaccount\('Length of output: 4857
Script:
#!/bin/bash # Description: Verify error handling and logging in the withdrawal process, focusing on sub vault redemption and consistency in error wrapping. # Test 1: Search for error handling when redeeming from sub vaults. rg --type go -A 20 'vaultParamsIterator := k.getVaultParamsIterator\(ctx\)' # Test 2: Search for direct error returns in WithdrawFromMegavault function. rg --type go -C 5 'return nil, err' protocol/x/vault/keeper/withdraw.goLength of output: 10610
Changelist
add megavault withdrawal msg
GetVaultWithdrawalSlippageis modified to be more statelessTest Plan
unit + integration test
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
Chores