[TRA-220] add activation threshold on vaults #1344
Conversation
WalkthroughThe recent updates aim to improve vault order processing by introducing a new parameter, Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional Context UsedPath-based Instructions (1)
Additional comments not posted (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/app/testdata/default_genesis_state.jsonis excluded by!**/*.jsonprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by!**/*.jsonprotocol/x/vault/types/params.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
Files selected for processing (10)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (6 hunks)
- proto/dydxprotocol/vault/params.proto (2 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/vault/keeper/msg_server_update_params_test.go (2 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (10 hunks)
- protocol/x/vault/keeper/params_test.go (2 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/params.go (2 hunks)
- protocol/x/vault/types/params_test.go (2 hunks)
Additional Context Used
Path-based Instructions (10)
proto/dydxprotocol/vault/params.proto (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/types/errors.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/types/params.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/params_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/msg_server_update_params_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/types/params_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/testutil/constants/genesis.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (22)
proto/dydxprotocol/vault/params.proto (2)
4-4: LGTM!
31-38: LGTM!protocol/x/vault/types/errors.go (1)
68-72: LGTM!protocol/x/vault/types/params.go (2)
12-18: LGTM!
40-43: LGTM!protocol/x/vault/keeper/params_test.go (2)
6-6: LGTM!
23-43: LGTM!protocol/x/vault/keeper/msg_server_update_params_test.go (2)
6-6: LGTM!
41-47: LGTM!protocol/x/vault/types/params_test.go (2)
6-6: LGTM!
24-80: LGTM!indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (2)
30-36: LGTM!
81-82: LGTM!protocol/x/vault/keeper/orders.go (2)
9-13: LGTM!
39-53: LGTM!protocol/x/vault/keeper/orders_test.go (6)
68-108: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-105]
The changes from lines 27 to 105 introduce the
activationThresholdQuoteQuantumsfield in test cases, which is crucial for testing the new activation threshold feature for vaults. Ensure that the test cases cover a comprehensive range of scenarios, including edge cases where the asset quantums are exactly equal to the activation threshold. This will help in thoroughly validating the feature's behavior under various conditions.
125-142: The setup for initializing the vault's parameters in the genesis state, specifically theactivationThresholdQuoteQuantums, is correctly implemented. This setup is essential for ensuring that the test environment accurately reflects the intended system state with the new activation threshold feature. It's good practice to ensure that all relevant parameters are initialized to their expected values before running the tests.
164-164: The conditional logic from lines 164 onwards, which determines whether to place vault CLOB orders based on the activation threshold, is a critical part of the test. It accurately reflects the intended behavior of the system with the new feature. However, consider adding explicit test assertions to verify that vaults below the activation threshold do not have their orders placed. This would make the tests more robust by explicitly checking for the absence of orders in scenarios where the activation threshold is not met.
187-187: The verification logic for expected orders, starting from line 187, is well-structured and ensures that only orders from vaults meeting the activation threshold are processed. This aligns with the feature's goal of optimizing system efficiency by preventing unnecessary activation of vaults. To further enhance the test's clarity and maintainability, consider abstracting some of the repetitive logic into helper functions or using table-driven tests to manage the various scenarios more concisely.
365-377: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [309-448]
The parameter setup for the
TestGetVaultClobOrdersfunction, specifically thevaultParamsstruct, is comprehensive and covers a wide range of scenarios to ensure thorough testing of the new activation threshold feature. It's important to maintain this level of detail in test setups to capture the full spectrum of system behavior under different configurations. Additionally, consider including comments or documentation that explain the rationale behind the chosen parameter values, especially for complex calculations or non-obvious choices. This will aid in understanding the test's intent and make it easier for future maintainers to modify or extend the tests as needed.
442-448: In the test case "Success - Get orders from Vault for Clob Pair 1, No Orders due to Zero Order Size", the scenario where the order size is effectively zero due to a very small vault asset quote quantum is well thought out. It's a good practice to include such edge cases in tests to ensure the system behaves as expected under extreme conditions. However, ensure that similar edge cases are consistently tested across all relevant functions to maintain comprehensive test coverage.protocol/testutil/constants/genesis.go (1)
1502-1503: The addition of theactivation_threshold_quote_quantumsfield with the value"1000000000"correctly implements the PR's objective.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/vault/types/params.pb.gois excluded by!**/*.pb.go
Files selected for processing (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts (6 hunks)
- proto/dydxprotocol/vault/params.proto (2 hunks)
- protocol/client/docs/swagger-ui/swagger.yaml (8 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/params.ts
- proto/dydxprotocol/vault/params.proto
Additional Context Used
Path-based Instructions (1)
protocol/client/docs/swagger-ui/swagger.yaml (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
| // positions must have to activate, i.e. if a vault has no perpetual positions | ||
| // and has strictly less than this amount of quote asset, it will not | ||
| // activate. | ||
| bytes activation_threshold_quote_quantums = 7 [ |
There was a problem hiding this comment.
Nit: consider using uint64 why is sufficient for all realistic threshold here (uint64 max = 18 trillion USDC)
There was a problem hiding this comment.
i agreed that uint64 is probably sufficient but a deposit to vault is represented as a serializable int already and wanted to keep it consistent
| // positions must have to activate, i.e. if a vault has no perpetual positions | ||
| // and has strictly less than this amount of quote asset, it will not | ||
| // activate. | ||
| bytes activation_threshold_quote_quantums = 7 [ |
There was a problem hiding this comment.
Did we ever create upgrade handler for x/vault? If so do we need to initialize this value as well?
There was a problem hiding this comment.
yep upgrade handler is created for x/vault. this value should be automatically initialized as the upgrade handler calls DefaultParams() to get params to set
protocol/x/vault/keeper/orders.go
Outdated
| @@ -34,6 +36,22 @@ func (k Keeper) RefreshAllVaultOrders(ctx sdk.Context) { | |||
| continue | |||
| } | |||
|
|
|||
| // Skip if vault has no perpetual positions and strictly less than `activation_threshold_quote_quantums` USDC. | |||
| params := k.GetParams(ctx) | |||
There was a problem hiding this comment.
We don't need to query this params for each vaults. Should just query once at the top
protocol/x/vault/keeper/orders.go
Outdated
| params := k.GetParams(ctx) | ||
| vault := k.subaccountsKeeper.GetSubaccount(ctx, *vaultId.ToSubaccountId()) | ||
| if vault.PerpetualPositions == nil || len(vault.PerpetualPositions) == 0 { | ||
| usdcQuoteQuantums := dtypes.NewInt(0) |
There was a problem hiding this comment.
Use subaccount.GetUsdcPosition() instead
|
@Mergifyio backport release/protocol/v5.x |
✅ Backports have been created
|
* add activation threshold on vaults * fix proto format * address comments (cherry picked from commit f652898) # Conflicts: # protocol/client/docs/statik/statik.go
Changelist
do not activate a vault if it has no perp position and has less than some threshold of quote quantums
default value is 1000 USDC
Test Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor
Chores