[CT-619] deprecate pessimistic add-to-book collateralization check #1107
Conversation
WalkthroughThe recent updates primarily focus on simplifying the collateral check process within the order handling mechanism of a blockchain protocol. The changes involve the removal of complex oracle price calculations in favor of using order subticks for collateral price checks, and adjustments in test cases to align with the new collateralization logic. This streamlining effort aims to enhance the efficiency and clarity of order placement and collateral verification. 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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/orders.go (4 hunks)
- protocol/x/clob/keeper/orders_test.go (9 hunks)
Additional comments: 6
protocol/x/clob/keeper/orders.go (1)
- 984-984: The removal of the
getPessimisticCollateralCheckPricefunction and the direct use ofopenOrder.SubticksforcollatCheckPriceSubtickssimplifies the collateralization check process. This change aligns with the PR's objective to streamline the process by avoiding complex calculations and oracle price dependencies. However, ensure that this simplification does not compromise the accuracy or integrity of the collateralization checks, especially in edge cases where market conditions are volatile.protocol/x/clob/keeper/orders_test.go (5)
- 9-9: The import of
dtypesfromgithub.com/dydxprotocol/v4-chain/protocol/dtypesis added. Ensure that this package is used within the test file for data types that are specific to the protocol.- 516-525: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [519-535]
The test cases
Subaccount cannot place maker buy order for 1 BTC at 5 subticks with 0 collateralandSubaccount cannot place maker sell order for 1 BTC at 500,000 with 0 collateralare correctly set up to test the system's behavior under specific conditions. It's good to see thorough testing of edge cases, such as attempting to place orders with insufficient collateral.
- 586-597: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [537-625]
The grouped tests for deprecating pessimistic value collateralization checks for both BUY and SELL orders are well-structured. These tests effectively validate the system's updated behavior in handling order collateralization checks, ensuring that the deprecated pessimistic checks are no longer applied and that the new logic functions as expected.
- 703-719: The initialization of the equity tier limit using
InitializeEquityTierLimitwith hardcoded values forUsdTncRequiredandLimitis correctly implemented. However, ensure that these values align with the protocol's requirements and that they are appropriately tested to cover various scenarios that might affect order placement and collateralization checks.- 703-720: The configuration for equity tier limits is set up correctly. It's important to ensure that the values used here (
UsdTncRequiredandLimit) are consistent with the protocol's specifications and that they are tested under different scenarios to validate their impact on order placement and processing.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/orders_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/orders_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/msg_server_place_order_test.go (3 hunks)
Additional comments: 3
protocol/x/clob/keeper/msg_server_place_order_test.go (3)
- 9-9: The addition of the
dtypesimport fromgithub.com/dydxprotocol/v4-chain/protocol/dtypesis necessary for the new test setup involving equity tier limits. This change is straightforward and correctly implemented.- 52-58: The addition of a new test case for an error related to the equity tier check is a good practice. It ensures that the system behaves as expected when an order would exceed the maximum open orders equity tier limit. This test case is well-structured and aligns with the existing testing patterns in the file.
- 137-156: The initialization of equity tier limits in the test setup is crucial for testing the new collateralization check logic. This setup ensures that the tests reflect the real-world scenario where equity tier limits are defined and enforced. The configuration of equity tier limits is correctly implemented, using the
SetSubaccountmethod for a test subaccount and theInitializeEquityTierLimitmethod with appropriate parameters. This change enhances the test's relevance and coverage.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2e/gov/add_new_market_test.go (1 hunks)
Additional comments: 1
protocol/testing/e2e/gov/add_new_market_test.go (1)
- 30-31: The update to
ExpectedPlaceOrderCheckTxResponseLogreflects a change in the expected response log for order submission when the oracle price is zero. This change is consistent with the PR's objective to simplify the collateralization check process. However, ensure that this new expected log accurately represents the system's behavior after the deprecation of the pessimistic collateralization check. If this log message is generated from a specific error or exception handling path, verify that the logic leading to this response has been appropriately updated to align with the new collateralization check method.
| @@ -128,6 +134,27 @@ func TestPlaceOrder_Error(t *testing.T) { | |||
| ) | |||
| require.NoError(t, err) | |||
|
|
|||
| ks.SubaccountsKeeper.SetSubaccount(ks.Ctx, constants.Alice_Num0_10_000USD) | |||
There was a problem hiding this comment.
give alice some money so that she fails b/c collat check and not equity tier
| existingOrders: []types.Order{}, | ||
| feeParams: constants.PerpetualFeeParamsNoFee, | ||
| order: constants.Order_Carl_Num0_Id0_Clob0_Buy1BTC_Price5subticks_GTB10, | ||
| expectedErr: types.ErrOrderWouldExceedMaxOpenOrdersEquityTierLimit, |
There was a problem hiding this comment.
this and the test below now fails because of equity tier limit and not collat check
protocol/x/clob/keeper/orders.go
Outdated
| @@ -961,7 +960,6 @@ func (k Keeper) AddOrderToOrderbookCollatCheck( | |||
| pendingUpdates := types.NewPendingUpdates() | |||
|
|
|||
| // Retrieve the associated `PerpetualId` for the `ClobPair`. | |||
| oraclePriceSubticksRat := k.GetOraclePriceSubticksRat(ctx, clobPair) | |||
| perpetualId := clobPair.MustGetPerpetualId() | |||
|
|
|||
| // TODO(DEC-1713): Complete as many calculations from getPessimisticCollateralCheckPrice as possible here | |||
There was a problem hiding this comment.
this ticket isn't relevant anymore right? since we don't use pessimistic oracle price
Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com>
Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/keeper/orders_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/orders_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/orders.go (4 hunks)
- protocol/x/clob/keeper/orders_test.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/orders_test.go
Additional comments: 1
protocol/x/clob/keeper/orders.go (1)
- 981-981: The change to directly use
openOrder.Subticksfor the collateral check price determination simplifies the process by removing the need for complex oracle price calculations. This aligns with the PR's objective to deprecate the pessimistic add-to-book collateralization check. However, it's crucial to ensure that this simplification does not compromise the accuracy or security of the collateralization checks. The direct use of subticks should be thoroughly tested to confirm it provides a reliable basis for these checks.
…1107) * [CT-619] deprecate pessimistic add-to-book collateralization check * fix lint * fix e2e test * fix test * Update protocol/x/clob/keeper/orders_test.go Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com> * Update protocol/x/clob/keeper/orders_test.go Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com> * remove comments --------- Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com>
…1107) (#1377) * [CT-619] deprecate pessimistic add-to-book collateralization check * fix lint * fix e2e test * fix test * Update protocol/x/clob/keeper/orders_test.go * Update protocol/x/clob/keeper/orders_test.go * remove comments --------- Co-authored-by: Lukasz Cwik <126621805+lcwik@users.noreply.github.com>
Changelist
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.