add megavault withdrawal info query #2316
Conversation
WalkthroughThe pull request introduces a new method for querying withdrawal information related to megavaults across multiple files. This includes the addition of 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.
Caution
Inline review comments failed to post
Actionable comments posted: 8
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/withdraw.go (1)
154-155: Typo in comment: Adjust wording for clarityConsider rephrasing the comment for clarity:
"Note that in the function below, quote quantums redeemed from each sub vault are transferred to the main vault."
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (1)
207-234: Clarify the calculation in the comment forexpectedQuoteQuantumsThe comment explaining how to calculate withdrawal slippage may be a bit unclear. To enhance readability and comprehension, consider rephrasing it.
Suggested rephrased comment:
/** * Number of quote quantums expected to be redeemed. * Withdrawal slippage can be calculated by comparing * `expected_quote_quantums` with - * `megavault_equity * shares_to_withdraw / total_shares` + * `(megavaultEquity * sharesToWithdraw) / totalShares` */This aligns the variable names in the equation with the field names in the interface and improves mathematical clarity.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/vault/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/vault/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
Files selected for processing (8)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (5 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (3 hunks)
- proto/dydxprotocol/vault/query.proto (2 hunks)
- protocol/x/vault/client/cli/query.go (3 hunks)
- protocol/x/vault/keeper/grpc_query_withdrawal_info.go (1 hunks)
- protocol/x/vault/keeper/grpc_query_withdrawal_info_test.go (1 hunks)
- protocol/x/vault/keeper/withdraw.go (4 hunks)
Additional comments not posted (15)
protocol/x/vault/keeper/grpc_query_withdrawal_info.go (1)
14-40: LGTM!The
MegavaultWithdrawalInfofunction is implemented correctly:
- It handles the nil request case by returning an appropriate error.
- It unwraps the SDK context correctly before using it.
- It calls
RedeemFromMainAndSubVaultswith the correct arguments and handles the returned values and error appropriately.- It constructs and returns the response correctly with all the required fields.
The function logic is sound, and the implementation is accurate.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.lcd.ts (3)
3-3: LGTM!The import statement is correctly updated to include the new types
QueryMegavaultWithdrawalInfoRequestandQueryMegavaultWithdrawalInfoResponseSDKType, which are likely used in the implementation of the newmegavaultWithdrawalInfomethod.
19-19: LGTM!The constructor is correctly updated to bind the new
megavaultWithdrawalInfomethod, ensuring that thethiscontext is correctly set when the method is called.
83-97: LGTM!The new
megavaultWithdrawalInfomethod is implemented correctly:
- The method is correctly typed with the appropriate input and output types.
- The endpoint URL is correctly constructed.
- The options object is correctly populated with the
shares_to_withdrawparameter if it is defined in the input.- The HTTP GET request is correctly made with the constructed endpoint and options.
The method provides a way to query the expected outcomes of a withdrawal in terms of quote quantums and slippage metrics, which aligns with the PR objectives of enhancing the functionality related to megavault withdrawals by providing more detailed information on expected outcomes.
protocol/x/vault/client/cli/query.go (1)
191-223: LGTM!The
CmdQueryMegavaultWithdrawalInfofunction is implemented correctly:
- It parses the command line argument into an unsigned 64-bit integer correctly.
- It constructs the request object by wrapping the parsed integer in a
NumSharesobject, which matches the expected request format.- It handles errors appropriately by returning them to the caller.
- It prints the response using the client context, which is consistent with other CLI commands.
- The function is well-structured and follows the existing patterns for similar CLI commands.
Great job!
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.rpc.Query.ts (5)
4-4: LGTM!The import statement is correctly adding the necessary types for the megavault withdrawal info query.
25-27: LGTM!The
megavaultWithdrawalInfomethod is correctly added to theQueryinterface, allowing clients to query withdrawal information for megavaults. The method signature uses the appropriate types and is consistent with the existing methods.
40-40: LGTM!The
megavaultWithdrawalInfomethod is correctly bound to the class instance in the constructor, ensuring that it maintains the correct context when called. The binding is consistent with the existing method bindings.
83-87: LGTM!The implementation of the
megavaultWithdrawalInfomethod in theQueryClientImplclass is correct and follows the same pattern as the existing methods. The request is properly encoded, sent via RPC to the correct service and method, and the response is correctly decoded into the expected type.
116-119: LGTM!The
megavaultWithdrawalInfomethod is correctly added to the object returned by thecreateRpcQueryExtensionfunction, allowing it to be called through the RPC query extension. The method properly calls the corresponding method of thequeryServiceinstance, passing therequestparameter. The addition is consistent with the existing methods in the returned object.proto/dydxprotocol/vault/query.proto (3)
42-47: LGTM!The new
MegavaultWithdrawalInfoRPC method in theQueryservice follows the naming conventions, has descriptive request/response message names, and is annotated correctly with thegoogle.api.httpoption. The functionality aligns with the PR objective.
128-134: Looks good!The
QueryMegavaultWithdrawalInfoRequestmessage is defined correctly with a descriptive name and a non-nullableshares_to_withdrawfield of typeNumSharesto capture the required input for the query.
136-158: Excellent work!The
QueryMegavaultWithdrawalInfoResponsemessage is well-defined with descriptive field names, appropriate data types, and non-nullable annotations. The custom serialization type used forexpected_quote_quantumsandmegavault_equityfields ensures proper serialization. The comments provide clear explanations of each field's purpose and usage.protocol/x/vault/keeper/withdraw.go (1)
262-381: Verify consistency in error handling and impact on redemptionIn the loop iterating over sub vaults, errors encountered during processing (e.g., retrieving vault data or calculations) are logged (when
txModeistrue), and the function continues to the next vault. This behavior could lead to some vaults being skipped, potentially affecting the totalredeemedQuoteQuantumsandmegavaultEquity. Please verify whether skipping vaults on error aligns with the intended redemption process, and consider whether the function should return an error to ensure data consistency.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (1)
186-187: Ensure optional fields are handled consistentlyThe fields
sharesToWithdrawin bothQueryMegavaultWithdrawalInfoRequestandQueryMegavaultWithdrawalInfoResponseare marked as optional (?). Verify whether these fields are truly optional or if they are required for the functionality. If they are required, consider removing the optional marker to enforce input validation.Also applies to: 205-206
Comments failed to post (8)
protocol/x/vault/keeper/grpc_query_withdrawal_info_test.go (3)
129-225: Refactor Genesis State Initialization to Improve Maintainability
The genesis state initialization code is repeated multiple times for different modules within the test function. Consider extracting this setup into separate helper functions or methods to reduce code duplication and enhance readability. This refactoring will make the tests easier to maintain and update in the future.
122-122: Use Error Type Comparison for Robust Error Checking
Currently, the test stores the expected error as a string and uses
require.ErrorContains(t, err, tc.expectedErr)for comparison. This approach relies on matching error messages, which can be brittle. Instead, consider storingexpectedErras anerrortype and usingrequire.True(t, errors.Is(err, tc.expectedErr))to compare error types directly. This method is more robust and less susceptible to issues if error messages change.Also applies to: 227-229
230-231: Ensure Accurate Comparison of
big.IntFields in AssertionsWhen asserting equality with
require.Equal(t, tc.res, response), thebig.Intfields within the structs may not be compared correctly due to pointer semantics in thebig.Inttype. This can lead to tests passing or failing incorrectly. Consider implementing a custom comparison function that checks the numerical values of thebig.Intfields usingCmp, or userequire.EqualValuesif suitable. This will ensure that the actual numerical values are compared accurately.protocol/x/vault/keeper/withdraw.go (1)
225-228: Consider renaming
txModeparameter for clarityThe
txModeparameter determines whether logging is enabled and whether quote quantums redeemed from each sub vault are transferred to the main vault. To improve readability, consider renamingtxModetoenableTransfersorisTransactionModeto better reflect its purpose.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/query.ts (4)
933-943: Update encoding and decoding logic for changed data types
With the proposed change to use
NumSharesforexpectedQuoteQuantumsandmegavaultEquity, the encoding and decoding logic needs to be updated accordingly.Apply the following diffs to the
encodemethod:export const QueryMegavaultWithdrawalInfoResponse = { encode(message: QueryMegavaultWithdrawalInfoResponse, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer { if (message.sharesToWithdraw !== undefined) { NumShares.encode(message.sharesToWithdraw, writer.uint32(10).fork()).ldelim(); } - if (message.expectedQuoteQuantums.length !== 0) { - writer.uint32(18).bytes(message.expectedQuoteQuantums); + if (message.expectedQuoteQuantums !== undefined) { + NumShares.encode(message.expectedQuoteQuantums, writer.uint32(18).fork()).ldelim(); } - if (message.megavaultEquity.length !== 0) { - writer.uint32(26).bytes(message.megavaultEquity); + if (message.megavaultEquity !== undefined) { + NumShares.encode(message.megavaultEquity, writer.uint32(26).fork()).ldelim(); } if (message.totalShares !== undefined) { NumShares.encode(message.totalShares, writer.uint32(34).fork()).ldelim(); } return writer; },And to the
decodemethod:while (reader.pos < end) { const tag = reader.uint32(); switch (tag >>> 3) { case 1: message.sharesToWithdraw = NumShares.decode(reader, reader.uint32()); break; - case 2: - message.expectedQuoteQuantums = reader.bytes(); + case 2: + message.expectedQuoteQuantums = NumShares.decode(reader, reader.uint32()); break; - case 3: - message.megavaultEquity = reader.bytes(); + case 3: + message.megavaultEquity = NumShares.decode(reader, reader.uint32()); break; case 4: message.totalShares = NumShares.decode(reader, reader.uint32()); break; default: reader.skipType(tag & 7); break; } }Also, update the
fromPartialmethod accordingly:fromPartial(object: DeepPartial<QueryMegavaultWithdrawalInfoResponse>): QueryMegavaultWithdrawalInfoResponse { const message = createBaseQueryMegavaultWithdrawalInfoResponse(); message.sharesToWithdraw = object.sharesToWithdraw !== undefined && object.sharesToWithdraw !== null ? NumShares.fromPartial(object.sharesToWithdraw) : undefined; - message.expectedQuoteQuantums = object.expectedQuoteQuantums ?? new Uint8Array(); - message.megavaultEquity = object.megavaultEquity ?? new Uint8Array(); + message.expectedQuoteQuantums = object.expectedQuoteQuantums !== undefined && object.expectedQuoteQuantums !== null ? NumShares.fromPartial(object.expectedQuoteQuantums) : undefined; + message.megavaultEquity = object.megavaultEquity !== undefined && object.megavaultEquity !== null ? NumShares.fromPartial(object.megavaultEquity) : undefined; message.totalShares = object.totalShares !== undefined && object.totalShares !== null ? NumShares.fromPartial(object.totalShares) : undefined; return message; }Also applies to: 949-971
195-196: Maintain consistency in SDK type interface field naming
In the
QueryMegavaultWithdrawalInfoRequestSDKTypeandQueryMegavaultWithdrawalInfoResponseSDKTypeinterfaces, the field names use underscores (e.g.,shares_to_withdraw). Ensure that this naming convention aligns with the rest of the codebase and complies with the project's style guidelines for SDK type interfaces.Also applies to: 228-229
207-211: Typo in comment: Correct 'Withdrawl' to 'Withdrawal'
There's a typo in the comment on line 208~:
- Incorrect spelling: "Withdrawl"
- Correct spelling: "Withdrawal"
Apply this diff to correct the typo:
- * Withdrawl slippage can be calculated by comparing + * Withdrawal slippage can be calculated by comparingCommittable 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.* Number of quote quantums above `shares` are expected to redeem. * Withdrawal slippage can be calculated by comparing * `expected_quote_quantums` with * `megavault_equity * shares_to_withdraw / total_shares` */
205-220: Inconsistent data types for numerical fields in
QueryMegavaultWithdrawalInfoResponseIn the
QueryMegavaultWithdrawalInfoResponseinterface, the fieldsexpectedQuoteQuantums(line 213~) andmegavaultEquity(line 216~) are typed asUint8Array, whereassharesToWithdraw(line 205~) andtotalShares(line 219~) are of typeNumShares. SinceexpectedQuoteQuantumsandmegavaultEquityrepresent numerical amounts, consider usingNumSharesfor consistency and clarity.Apply this diff to update the data types:
export interface QueryMegavaultWithdrawalInfoResponse { /** Number of shares to withdraw. */ sharesToWithdraw?: NumShares; /** * Number of quote quantums expected to redeem. * Withdrawal slippage can be calculated by comparing * `expected_quote_quantums` with * `megavault_equity * shares_to_withdraw / total_shares` */ - expectedQuoteQuantums: Uint8Array; + expectedQuoteQuantums?: NumShares; /** Equity of megavault (in quote quantums). */ - megavaultEquity: Uint8Array; + megavaultEquity?: NumShares; /** Total shares in megavault. */ totalShares?: NumShares; }Ensure corresponding changes are made in:
- The SDK type interface
QueryMegavaultWithdrawalInfoResponseSDKType(lines 226~ to 243~).- The
encode,decode, andfromPartialmethods (lines 928~ to 990~).Committable suggestion was skipped due to low confidence.
protocol/x/vault/keeper/withdraw.go
Outdated
| func (k Keeper) RedeemFromMainAndSubVaults( | ||
| ctx sdk.Context, | ||
| shares *big.Int, | ||
| txMode bool, |
There was a problem hiding this comment.
nit: Might be better to call this something like simulate.
| megavaultEquity, err = k.GetSubaccountEquity(ctx, types.MegavaultMainSubaccount) | ||
| if err != nil { | ||
| if txMode { | ||
| log.ErrorLogWithError(ctx, "Megavault withdrawal: failed to get megavault main vault equity", err) |
There was a problem hiding this comment.
nit: Perhaps better to log at level debug if it's a query and log at level error if it's in a transaction?
| redeemedQuoteQuantums, megavaultEquity, totalShares, err := k.RedeemFromMainAndSubVaults( | ||
| ctx, | ||
| req.SharesToWithdraw.NumShares.BigInt(), | ||
| false, |
There was a problem hiding this comment.
nit: Comment on what this variable is (txMode or simulate) as it's quite important that it's set to false.
There was a problem hiding this comment.
queries do execute on their own branched context so it's ok even if this is set to true, but yeah will add a comment
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)
153-156: LGTM! Consider improving variable naming for clarity.The refactoring to use
RedeemFromMainAndSubVaultssimplifies the withdrawal process and improves code maintainability.Consider renaming
redeemedQuoteQuantumstoredeemedQuoteQuantumsFromMainVaultfor clarity, as it's later used as a base value for the total redeemed quantums from all vaults.
Line range hint
172-195: LGTM! Consider enhancing error handling.The changes improve the withdrawal process by ensuring all funds are collected before transferring to the destination subaccount. The error logging enhancements are beneficial for debugging.
Consider wrapping the error returned from
ProcessTransferto provide more context:if err != nil { return nil, errorsmod.Wrapf(err, "failed to transfer %d quantums from main vault to subaccount %s", redeemedQuoteQuantums.Uint64(), toSubaccount) }This would provide more detailed information in case of failure, potentially aiding in debugging and error resolution.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/vault/keeper/grpc_query_withdrawal_info.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/grpc_query_withdrawal_info.go
Additional comments not posted (1)
protocol/x/vault/keeper/withdraw.go (1)
225-426: LGTM! Well-structured and comprehensive implementation.The new
RedeemFromMainAndSubVaultsfunction effectively centralizes the redemption logic for both main and sub vaults, improving code maintainability and consistency. The implementation handles both simulation and real transaction scenarios well, with appropriate error handling and logging.
- Consider extracting the vault iteration logic into a separate helper function to improve readability and reduce the overall function size.
- The error messages for simulation and real transaction scenarios are very similar. Consider creating constants for these messages to ensure consistency and ease of maintenance.
To ensure that this new function is correctly integrated and doesn't introduce any regressions, please run the following verification script:
This script will help verify the correct integration of the new function and identify any potential issues or remaining tasks related to vault withdrawals.
Verification successful
Approved! The verification confirms the changes are correctly implemented.
The usage of
RedeemFromMainAndSubVaultsandGetVaultWithdrawalSlippagehas been properly integrated within the codebase. No issues or outstanding tasks were detected during the verification process.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of RedeemFromMainAndSubVaults and related functions # Test 1: Check if RedeemFromMainAndSubVaults is only called from WithdrawFromMegavault echo "Test 1: Verifying RedeemFromMainAndSubVaults usage" rg --type go "RedeemFromMainAndSubVaults" -C 5 # Test 2: Ensure that GetVaultWithdrawalSlippage is used correctly echo "Test 2: Verifying GetVaultWithdrawalSlippage usage" rg --type go "GetVaultWithdrawalSlippage" -C 5 # Test 3: Check for any TODOs or FIXMEs related to vault withdrawal echo "Test 3: Checking for TODOs or FIXMEs" rg --type go "TODO|FIXME" -C 5 | grep -i "vault.*withdraw"Length of output: 7105
Script:
#!/bin/bash # Description: Enhanced verification of GetVaultWithdrawalSlippage usage echo "Test 1: Verifying RedeemFromMainAndSubVaults usage" rg --type go "RedeemFromMainAndSubVaults" -C 5 echo "Test 2: Verifying GetVaultWithdrawalSlippage usage with broader pattern" rg --type go "GetVaultWithdrawalSlippage" -C 5 || echo "No results found for GetVaultWithdrawalSlippage" echo "Test 3: Searching for indirect usages or aliases of GetVaultWithdrawalSlippage" rg --type go "Slippage" -C 5 | grep "GetVaultWithdrawalSlippage" echo "Test 4: Checking for any overridden or wrapped instances of GetVaultWithdrawalSlippage" rg --type go "func.*GetVaultWithdrawalSlippage" -C 5Length of output: 9692
Changelist
query how many quote quantums are expected from a withdrawal and slippage
Test Plan
unit tests
localnet testing
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
Refactor