Skip equity tier limit check in PlaceShortTermOrder #1318
Conversation
WalkthroughThe recent updates focus on simplifying the order placement process within a trading protocol by adjusting how equity tier limits impact order placements. These changes involve removing validations for subaccount equity tier limits during short-term order placements and modifying related test cases. This streamlines the order placement process by eliminating certain checks for equity tiers, impacting both functionality and testing of order placements and cancellations based on these limits. Changes
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: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/e2e/equity_tier_limit_test.go (8 hunks)
- protocol/x/clob/keeper/orders.go (1 hunks)
- protocol/x/clob/keeper/orders_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/keeper/orders_test.go
Additional Context Used
Path-based Instructions (2)
protocol/x/clob/e2e/equity_tier_limit_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/clob/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.
Additional comments not posted (3)
protocol/x/clob/e2e/equity_tier_limit_test.go (2)
10-10: Ensure that the logic for order expiry, especially in the context of equity tier limits, is still valid and necessary after the removal of equity tier limit checks. If the concept of order expiry is tightly coupled with equity tier limits, consider if adjustments or additional tests are needed to reflect the new system behavior without these checks.
10-10: With the removal of equity tier limit checks, reassess the necessity and accuracy of tests related to order fills in the context of equity tier limits. If these tests are still relevant, ensure they accurately reflect the system's behavior without the equity tier limit checks. Otherwise, consider adjusting or removing these tests to align with the updated system logic.protocol/x/clob/keeper/orders.go (1)
208-213: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-1000]
Ensure all newly introduced or modified functions, especially those related to order processing and validation, are accompanied by corresponding unit tests. This is crucial for verifying the correctness of the changes and maintaining the reliability of the system. Additionally, review the error handling strategies across the file to ensure they are consistent and robust, particularly in functions that interact with external systems or perform critical operations.
Would you like assistance in creating additional unit tests or reviewing error handling strategies?
| return 0, 0, err | ||
| } | ||
|
|
||
| // Validate that adding the order wouldn't exceed subaccount equity tier limits. | ||
| err = k.ValidateSubaccountEquityTierLimitForShortTermOrder(ctx, order) | ||
| if err != nil { | ||
| return 0, 0, err | ||
| } | ||
|
|
||
| // Place the order on the memclob and return the result. | ||
| orderSizeOptimisticallyFilledFromMatchingQuantums, orderStatus, offchainUpdates, err := k.MemClob.PlaceOrder( | ||
| ctx, |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [285-357]
The removal of the equity tier limit validation from the PlaceShortTermOrder function aligns with the PR objectives to optimize performance by eliminating ineffective checks. Ensure that this change has been thoroughly tested, especially in scenarios where equity tier limits might have previously impacted order placements. Additionally, consider documenting this change prominently in the function's comments or in the project's change log to maintain clarity for future maintainers and users.
| @@ -2,6 +2,9 @@ package clob_test | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| "testing" | |||
| "time" | |||
|
|
|||
| "github.com/cometbft/cometbft/types" | |||
| sdk "github.com/cosmos/cosmos-sdk/types" | |||
| "github.com/dydxprotocol/v4-chain/protocol/dtypes" | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Given the widespread references to equity tier limits across various parts of the codebase, including protobuf definitions, configuration files, keeper logic, and end-to-end tests, it is evident that other functionalities or tests do rely on equity tier limits for critical operations. The removal of these checks needs careful consideration to ensure it does not inadvertently affect these interconnected parts of the system.
Analysis chain
Given the PR's objective to remove equity tier limit checks, ensure that the removal of these checks does not inadvertently affect other functionalities or tests that might still rely on the concept of equity tiers for validation or setup. It's crucial to verify that the removal aligns with the overall system design and does not introduce unintended side effects.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that no other functionalities or tests rely on equity tier limits for critical operations.
rg "equityTierLimit" --type go
Length of output: 6576
| @@ -208,12 +208,6 @@ func (k Keeper) PlaceShortTermOrder( | |||
| return 0, 0, err | |||
| } | |||
|
|
|||
| // Validate that adding the order wouldn't exceed subaccount equity tier limits. | |||
| err = k.ValidateSubaccountEquityTierLimitForShortTermOrder(ctx, order) | |||
| if err != nil { | |||
There was a problem hiding this comment.
remove the comment on line 170
There was a problem hiding this comment.
we also technically dont use ValidateSubaccountEquityTierLimitForShortTermOrder anymore maybe add a comment that we removed the sa eq tier check for short term orders on check tx to that fn or just del
There was a problem hiding this comment.
OK I marked it as deprecated.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/equity_tier_limit.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
protocol/x/clob/keeper/equity_tier_limit.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 (1)
protocol/x/clob/keeper/equity_tier_limit.go (1)
98-99: The deprecation notice forValidateSubaccountEquityTierLimitForShortTermOrderis clear and well-documented.However, consider adding guidance for developers on handling short-term orders without equity tier limits, if there's a new mechanism in place.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/equity_tier_limit.go (1 hunks)
- protocol/x/clob/keeper/orders.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/keeper/equity_tier_limit.go
- protocol/x/clob/keeper/orders.go
* Add subaccountNumber to PerpetualPositionResponseObject (#1274) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * [CT-712] send updates for both normal order matches and liquidation (#1280) * Remove volatile market (#1263) * [SKI-21] Bump slinky version to v0.3.1 (#1275) * Bump slinky version * Add cp number method * clean up socks logging (#1285) * [CT-681] fix liquidated side and offsetting side for indexer delevera… (#1284) * [CT-681] fix liquidated side and offsetting side for indexer deleveraging events * fix test * fix test * fix lint * [TRA-105] Add API for parent subaccount perpetual positions (#1282) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Use sample rate with stream destroyed stats. (#1294) * Revert "[CT-708] Indexer track e2e latency (#1237)" (#1292) This reverts commit 60b94df. * Fix swagger generation makefile command / regen swagger docs (#1299) * pull dydx fork to generate swagger properly * remove the print * remove vault constants (#1293) * Remove custom ping message from socks (#1301) * Add subaccountNumber to the OrderResponseObject (#1296) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * sample more metrics (#1304) * [OTE-256] Add upgrade handler to initialize OI during upgrade handler (#1302) * Add upgrade handler to initialize OI during upgrade handler * nits * Fix lib.ErrorLogWithError: properly pass in args (#1306) * fix broken tests (#1312) * Explicitly close websockets on errors (#1290) * Increase the number of allowed connections to 8000 (#1317) * [TRA-104] Add parentSubaccountNumber API for orders (#1313) Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> * Improve Slinky logs to prevent unnecessary logs (#1289) * [SKI-26]: Prevent funding index update with no oracle prices from (#1321) halting indexer * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment * fix lint (#1323) --------- Signed-off-by: Shrenuj Bansal <shrenuj@dydx.exchange> Co-authored-by: shrenujb <98204323+shrenujb@users.noreply.github.com> Co-authored-by: jayy04 <103467857+jayy04@users.noreply.github.com> Co-authored-by: Eric Warehime <eric.warehime@gmail.com> Co-authored-by: vincentwschau <99756290+vincentwschau@users.noreply.github.com> Co-authored-by: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> Co-authored-by: Tian <tian@dydx.exchange> Co-authored-by: Teddy Ding <teddy@dydx.exchange> Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Christopher-Li <Christopher-Li@users.noreply.github.com>
* Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment
|
@Mergifyio backport release/protocol/v4.1.x |
✅ Backports have been created
|
* Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) # Conflicts: # protocol/x/clob/e2e/equity_tier_limit_test.go # protocol/x/clob/keeper/equity_tier_limit.go # protocol/x/clob/keeper/orders.go
…1516) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) # Conflicts: # protocol/x/clob/e2e/equity_tier_limit_test.go # protocol/x/clob/keeper/equity_tier_limit.go # protocol/x/clob/keeper/orders.go * resolve conflict --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
|
https://github.com/Mergifyio backport release/protocol/v5.x |
✅ Backports have been created
|
* Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff)
…1549) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) * Fix lint --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
…1549) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) * Fix lint --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
…1549) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) * Fix lint --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
…1549) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) * Fix lint --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
…1549) * Skip equity tier limit check in PlaceShortTermOrder (#1318) * Skip equity tier limit check in PlaceShortTermOrder * remove tests * Add comment (cherry picked from commit 24790ff) * Fix lint --------- Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com> Co-authored-by: Roy Li <roy@dydx.exchange>
Changelist
These checks are not effective for their original purpose and take 30% of the CheckTx global lock time in testnet loads.
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.