[CLOB-1058] Allow FOK/IOC Reduce Only Orders #919
Conversation
WalkthroughThe overall change enhances the validation process for reduce-only orders in a trading protocol. It introduces a new error for when a non-reduce only order with the same ID is present, preventing the placement of reduce-only orders in such cases. Additionally, the validation logic now checks that reduce-only orders require immediate execution, refining the conditions under which these orders are accepted or rejected. 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/errors.go (2 hunks)
- protocol/x/clob/types/message_place_order.go (1 hunks)
Additional comments: 4
protocol/x/clob/types/message_place_order.go (1)
- 77-78: The updated condition in
ValidateBasiccorrectly enforces that reduce-only orders must be short term IOC or FOK orders. Ensure that corresponding unit tests are added to validate this new behavior.protocol/x/clob/types/errors.go (2)
204-208: The addition of
ErrReduceOnlyOrderReplacementwith code 44 is consistent with the PR's objectives to handle validation scenarios for reduce-only orders. The error message is clear and specific.501-501: The updated message for
ErrReduceOnlyDisabledadds clarity for non-FOK/IOC orders, aligning with the PR's objectives to enhance error messaging.protocol/x/clob/memclob/memclob.go (1)
- 1352-1368: The addition of the reduce-only order validation logic in the
validateNewOrdermethod is a critical update to ensure that reduce-only orders do not replace existing non-reduce only orders. This is a key part of the PR's objectives to handle reduce-only orders correctly. The code correctly checks for the existence of a non-reduce only order with the same ID and rejects the new reduce-only order if found. This prevents a scenario where a reduce-only order could inadvertently increase a position, which would be against the intended behavior of such orders.
| // If the order is a reduce-only order, we should ensure the full size of the order does not increase the | ||
| // subaccount's current position size. Note that we intentionally do not validate that the reduce-only order | ||
| // does not change the subaccount's position _side_, and that will be validated if the order is matched. | ||
| // TODO(DEC-1228): use `MustValidateReduceOnlyOrder` and move this to `PerformStatefulOrderValidation`. | ||
| if order.IsReduceOnly() { | ||
| // If there exists an non-reduce only order with the same order id | ||
| // cancel the incoming reduce only order by returning an error. | ||
| if restingOrderExists && !existingRestingOrder.IsReduceOnly() { | ||
| return types.ErrReduceOnlyOrderReplacement | ||
| } | ||
|
|
||
| existingPositionSize := m.clobKeeper.GetStatePosition(ctx, orderId.SubaccountId, order.GetClobPairId()) | ||
| orderSize := order.GetBigQuantums() | ||
|
|
There was a problem hiding this comment.
The changes to the validateNewOrder method include additional checks for reduce-only orders. It now ensures that if an order is marked as reduce-only, it cannot replace an existing non-reduce only order with the same order ID. This logic is sound and aligns with the PR objectives to prevent reduce-only orders from increasing a position. However, the check for the order's position side change due to the matched amount (lines 1366-1368) seems to be missing the actual comparison logic. The code comments suggest that there should be a comparison to ensure the reduce-only order does not change the subaccount's position side, but the code to perform this check is not present.
- // If the reduce-only order is not on the opposite side of the existing position size,
- // cancel the order by returning an error.
- if orderSize.Sign()*existingPositionSize.Sign() != -1 {
- return types.ErrReduceOnlyWouldIncreasePositionSize
- }Committable 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.
| // If the order is a reduce-only order, we should ensure the full size of the order does not increase the | |
| // subaccount's current position size. Note that we intentionally do not validate that the reduce-only order | |
| // does not change the subaccount's position _side_, and that will be validated if the order is matched. | |
| // TODO(DEC-1228): use `MustValidateReduceOnlyOrder` and move this to `PerformStatefulOrderValidation`. | |
| if order.IsReduceOnly() { | |
| // If there exists an non-reduce only order with the same order id | |
| // cancel the incoming reduce only order by returning an error. | |
| if restingOrderExists && !existingRestingOrder.IsReduceOnly() { | |
| return types.ErrReduceOnlyOrderReplacement | |
| } | |
| existingPositionSize := m.clobKeeper.GetStatePosition(ctx, orderId.SubaccountId, order.GetClobPairId()) | |
| orderSize := order.GetBigQuantums() | |
| // If the order is a reduce-only order, we should ensure the full size of the order does not increase the | |
| // subaccount's current position size. Note that we intentionally do not validate that the reduce-only order | |
| // does not change the subaccount's position _side_, and that will be validated if the order is matched. | |
| // TODO(DEC-1228): use `MustValidateReduceOnlyOrder` and move this to `PerformStatefulOrderValidation`. | |
| if order.IsReduceOnly() { | |
| // If there exists an non-reduce only order with the same order id | |
| // cancel the incoming reduce only order by returning an error. | |
| if restingOrderExists && !existingRestingOrder.IsReduceOnly() { | |
| return types.ErrReduceOnlyOrderReplacement | |
| } | |
| existingPositionSize := m.clobKeeper.GetStatePosition(ctx, orderId.SubaccountId, order.GetClobPairId()) | |
| orderSize := order.GetBigQuantums() |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/testutil/constants/orders.go (2 hunks)
- protocol/testutil/constants/subaccounts.go (1 hunks)
- protocol/x/clob/e2e/reduce_only_orders_test.go (1 hunks)
- protocol/x/clob/types/message_place_order_test.go (2 hunks)
Additional comments: 12
protocol/x/clob/types/message_place_order_test.go (4)
236-247: The test case "short-term FOK reduce-only success" seems to be testing the successful validation of a short-term FOK reduce-only order. Ensure that the test setup correctly reflects the conditions under which a reduce-only FOK order should be considered valid.
255-270: The test case "short-term IOC reduce-only success" is intended to validate the successful placement of a short-term IOC reduce-only order. It is important to confirm that the test accurately represents the expected behavior of the system when handling IOC reduce-only orders.
274-292: The test case "long-term order reduce-only disabled" should verify that long-term orders cannot be marked as reduce-only. The test setup must ensure that the correct error is returned when such an order is placed.
294-313: The test case "long-term reduce-only disabled" appears to be testing the scenario where a conditional long-term order is incorrectly marked as reduce-only. The test should ensure that the system correctly identifies and rejects such orders.
protocol/x/clob/e2e/reduce_only_orders_test.go (4)
- 15-83: The test case "IOC Reduce only order partially matches short term order same block, maker order fully filled" is designed to test the behavior of an IOC reduce-only order in a scenario where it partially matches with another order. Ensure that the test accurately simulates the matching process and the expected outcomes for both the maker and taker orders.
Verification successful
I will attempt a different strategy to locate the matching logic for IOC reduce-only orders by searching for keywords related to order matching.
To verify the test case logic, I will look for a specific test file that might contain the logic for placing and matching orders, particularly for IOC reduce-only orders. I will focus on
memclob_place_order_reduce_only_test.goas it seems most relevant to the review comment.
I will verify the existence of the file
memclob_place_order_reduce_only_test.goand ensure the search command is correctly formatted.
The provided file
memclob_place_order_reduce_only_test.gocontains detailed test cases for placing reduce-only orders. These tests cover various scenarios, including the placement of reduce-only orders that are expected to be canceled, added to the order book, or matched against existing orders. The tests also check for the correct handling of orders when they are expected to be resized or canceled due to changes in position size or collateralization checks.The test cases are comprehensive and appear to be designed to ensure that reduce-only orders behave as expected in different scenarios, including those involving IOC orders. The tests include checks for expected errors, order status, filled sizes, pending matches, and the state of the order book after order placement.
Based on the detailed test cases provided in the file, it seems that the behavior of IOC reduce-only orders and their matching logic is being thoroughly tested, which aligns with the PR objectives and the review comment.
* 84-143: The test case "IOC Reduce only order partially matches short term order second block, maker order fully filled" should ensure that the system correctly handles the matching of IOC reduce-only orders in subsequent blocks. It is crucial that the test reflects the expected state changes after the matching process.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the matching logic for IOC reduce-only orders is correctly implemented. ast-grep --lang go --pattern $'func ($_ *ClobKeeper) MatchOrder($_, $_) $_ { $$$ if $_ && $_ { $$$ } $$$ }'Length of output: 120
Script:
#!/bin/bash # Search for the matching logic related to IOC reduce-only orders using keywords. rg "MatchOrder" --type=goLength of output: 18075
Script:
#!/bin/bash # Look for the test implementation related to placing and matching IOC reduce-only orders. rg "memclob_place_order_reduce_only_test.go" --type=go -A 30 -B 30Length of output: 66
Script:
#!/bin/bash # Verify the existence of the file and list its contents. fd "memclob_place_order_reduce_only_test.go" --type f --exec cat {}Length of output: 64017
144-202: The test case "IOC Reduce only order partially matches short term order second block, maker order partially filled" tests the scenario where an IOC reduce-only order partially fills a maker order in a subsequent block. The test should validate that the system correctly updates the order book and the subaccount positions.
286-349: The test case "Reduce only order fails to replace non-reduce only order" is critical as it tests the system's ability to prevent a reduce-only order from replacing a non-reduce-only order. The test should ensure that the correct error is returned in this scenario.
Verification successful
The verification process has provided the following information:
The
ast-grepcommand did not find thePlaceOrderfunction within theClobKeeperstruct. This could be due to the pattern not being accurate or the function not existing with the expected signature.The
rgcommand found references toErrReduceOnlyOrderReplacementinerrors.go, which confirms the existence of the error code 44 for the scenario where a reduce-only order attempts to replace a non-reduce only order. Additionally, the same error is referenced inmemclob.gowithin a conditional statement that checks if an order is reduce-only and if there exists a non-reduce only order with the same order ID, in which case the error is returned.The
rgcommand also confirmed the presence of the test case "Reduce only order fails to replace non-reduce only order" inreduce_only_orders_test.go. The test case includes a scenario where a non-reduce only order is placed, followed by an attempt to place a reduce-only order with the same order ID, which should fail and return theErrReduceOnlyOrderReplacementerror.Based on the information gathered, the review comment is consistent with the codebase. The test case is designed to verify that a reduce-only order cannot replace a non-reduce only order and that the correct error is returned when this scenario occurs.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that reduce-only orders cannot replace non-reduce-only orders and that the correct error is returned. ast-grep --lang go --pattern $'func ($_ *ClobKeeper) PlaceOrder($_) $_ { $$$ if $_ && $_ { return ErrReduceOnlyOrderReplacement } $$$ }'Length of output: 149
Script:
#!/bin/bash # First, let's find the PlaceOrder function within the ClobKeeper struct to see if it exists. ast-grep --lang go --pattern $'func ($_ *ClobKeeper) PlaceOrder($_) $_ { $$$ }'Length of output: 82
Script:
#!/bin/bash # Next, let's search for the ErrReduceOnlyOrderReplacement error within the PlaceOrder function to confirm its usage. rg "ErrReduceOnlyOrderReplacement" --context 5Length of output: 2388
Script:
#!/bin/bash # Lastly, let's confirm that the test case "Reduce only order fails to replace non-reduce only order" is designed correctly. # We will look for the test case by name and check the associated logic. rg "Reduce only order fails to replace non-reduce only order" --context 5Length of output: 909
protocol/testutil/constants/subaccounts.go (1)
- 38-61: The addition of
Alice_Num1_1BTC_Short_100_000USDandAlice_Num1_1BTC_Long_500_000USDsubaccounts seems consistent with the existing pattern of subaccount declarations. Ensure that thePerpetualIdandQuantumsvalues align with the domain model and business logic for representing short and long positions in BTC.protocol/testutil/constants/orders.go (3)
606-611: The addition of
Order_Carl_Num0_Id0_Clob0_Buy10_Price500000_GTB20seems consistent with the PR objectives, introducing a new order with a largeSubticksvalue. Ensure that theSubticksvalue aligns with the system's expected range and precision.1134-1139: The new order
Order_Alice_Num1_Id1_Clob1_Buy10_Price15_GTB20_FOK_ROcorrectly setsTimeInForcetoFILL_OR_KILLandReduceOnlytotrue, aligning with the PR's goal to support FOK/IOC reduce-only orders.1162-1167: The order
Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20_IOC_ROintroduces an IOC reduce-only order. Verify that the logic for handling IOC orders withReduceOnlyset totrueis correctly implemented in the order processing system.Verification successful
The search for "ReduceOnly" within Go files has produced a significant amount of output, indicating that the logic for handling reduce-only orders is indeed implemented and spread across various files in the codebase. Notably, there are references to functions and methods such as
IsReduceOnly, error types likeErrReduceOnlyOrderReplacement, and tests that check the behavior of reduce-only orders.Based on the output, here are some key points:
- The
IsReduceOnlymethod is defined inorder.goandliquidation_order.go, which likely checks if an order is marked as reduce-only.- Error handling for reduce-only orders is present, with errors such as
ErrReduceOnlyOrderReplacement,ErrReduceOnlyWouldIncreasePositionSize, andErrReduceOnlyWouldChangePositionSidedefined inerrors.go.- The
message_place_order.gofile contains logic that checks if an order is reduce-only and requires immediate execution, which aligns with the PR objectives.- There are tests in files like
memclob_place_order_reduce_only_test.goandreduce_only_orders_test.gothat seem to test the behavior of reduce-only orders, including scenarios where errors are expected.Given this information, it appears that the logic for handling IOC reduce-only orders is implemented in the order processing system, and there are mechanisms in place to validate and test these orders.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the handling of IOC reduce-only orders in the order processing logic. rg --type go "func .*processOrder" --exec cat {} \; | grep -C 5 "ReduceOnly"Length of output: 488
Script:
#!/bin/bash # Search for the handling of IOC reduce-only orders in the order processing logic within Go files. rg --type go "func .*processOrder.*ReduceOnly" -C 5Length of output: 53
Script:
#!/bin/bash # First, find functions that might process orders. rg --type go "func .*processOrder" -lLength of output: 39
Script:
#!/bin/bash # Search for the term "ReduceOnly" within Go files to locate relevant order processing logic. rg --type go "ReduceOnly"Length of output: 24700
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestReduceOnlyOrders(t *testing.T) { |
There was a problem hiding this comment.
can we add tests for FOK as well?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/reduce_only_orders_test.go (1 hunks)
Additional comments: 1
protocol/x/clob/e2e/reduce_only_orders_test.go (1)
- 286-389: The
TestReduceOnlyOrderFailurefunction correctly tests various failure scenarios for reduce-only orders. It's important to ensure that these tests cover all new validation logic introduced in the PR, including the new error conditions.
| }, | ||
| expectedOrderFillAmount: map[clobtypes.OrderId]uint64{ | ||
| constants.Order_Carl_Num0_Id0_Clob0_Buy10_Price500000_GTB20.OrderId: 100, | ||
| }, | ||
| expectedSubaccounts: []satypes.Subaccount{ | ||
| { | ||
| Id: &constants.Carl_Num0, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| { | ||
| AssetId: 0, | ||
| Quantums: dtypes.NewInt(95_000_550_000), | ||
| }, | ||
| }, | ||
| PerpetualPositions: []*satypes.PerpetualPosition{ | ||
| { | ||
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(100), | ||
| FundingIndex: dtypes.NewInt(0), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Id: &constants.Alice_Num1, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| { | ||
| AssetId: 0, | ||
| Quantums: dtypes.NewInt(504_997_500_000), | ||
| }, | ||
| }, | ||
| PerpetualPositions: []*satypes.PerpetualPosition{ | ||
| { | ||
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(99_999_900), | ||
| FundingIndex: dtypes.NewInt(0), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "IOC Reduce only order partially matches short term order second block, maker order partially filled": { | ||
| subaccounts: []satypes.Subaccount{ | ||
| constants.Carl_Num0_100000USD, | ||
| constants.Alice_Num1_1BTC_Long_500_000USD, | ||
| }, | ||
| ordersForFirstBlock: []clobtypes.Order{ | ||
| MustScaleOrder( | ||
| constants.Order_Carl_Num0_Id0_Clob0_Buy80_Price500000_GTB20, | ||
| testapp.DefaultGenesis(), | ||
| ), | ||
| }, | ||
| ordersForSecondBlock: []clobtypes.Order{ | ||
| MustScaleOrder( | ||
| constants.Order_Alice_Num1_Id1_Clob0_Sell15_Price500000_GTB20_IOC_RO, | ||
| testapp.DefaultGenesis(), | ||
| ), | ||
| }, | ||
|
|
||
| expectedOrderOnMemClob: map[clobtypes.OrderId]bool{ | ||
| constants.Order_Carl_Num0_Id0_Clob0_Buy80_Price500000_GTB20.OrderId: true, | ||
| constants.Order_Alice_Num1_Id1_Clob0_Sell15_Price500000_GTB20_IOC_RO.OrderId: false, | ||
| }, | ||
| expectedOrderFillAmount: map[clobtypes.OrderId]uint64{ | ||
| constants.Order_Carl_Num0_Id0_Clob0_Buy80_Price500000_GTB20.OrderId: 150, | ||
| }, | ||
| expectedSubaccounts: []satypes.Subaccount{ | ||
| { | ||
| Id: &constants.Carl_Num0, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| { | ||
| AssetId: 0, | ||
| Quantums: dtypes.NewInt(9_250_0825_000), | ||
| }, | ||
| }, | ||
| PerpetualPositions: []*satypes.PerpetualPosition{ | ||
| { | ||
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(150), | ||
| FundingIndex: dtypes.NewInt(0), | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| Id: &constants.Alice_Num1, | ||
| AssetPositions: []*satypes.AssetPosition{ | ||
| { | ||
| AssetId: 0, | ||
| Quantums: dtypes.NewInt(507_496_250_000), | ||
| }, | ||
| }, | ||
| PerpetualPositions: []*satypes.PerpetualPosition{ | ||
| { | ||
| PerpetualId: 0, | ||
| Quantums: dtypes.NewInt(99_999_850), | ||
| FundingIndex: dtypes.NewInt(0), | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
The test cases within TestReduceOnlyOrders seem comprehensive, covering various scenarios for reduce-only orders. However, the previous comment by jakob-dydx suggests adding tests for FOK as well. It's not clear if FOK scenarios are included in the current test cases. If not, this should be addressed.
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/e2e/reduce_only_orders_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/e2e/reduce_only_orders_test.go
| // If there exists an non-reduce only order with the same order id | ||
| // cancel the incoming reduce only order by returning an error. | ||
| if restingOrderExists && !existingRestingOrder.IsReduceOnly() { | ||
| return types.ErrReduceOnlyOrderReplacement | ||
| } |
There was a problem hiding this comment.
why is this logic necessary?
There was a problem hiding this comment.
Started discussion in #p-reduce-only-ioc-fok
| } | ||
| } | ||
|
|
||
| func TestReduceOnlyOrderFailure(t *testing.T) { |
There was a problem hiding this comment.
can we add conditional order tests + long-term order tests for reduce-only?
| @@ -250,6 +251,66 @@ func TestMsgPlaceOrder_ValidateBasic(t *testing.T) { | |||
| ReduceOnly: true, | |||
| }, | |||
| }, | |||
| }, | |||
| "short-term IOC reduce-only success": { | |||
There was a problem hiding this comment.
add tests for conditional orders?
2 very small edits to allow FOK IOC reduce only errors, add validation for no replacement RO orders. E2e tests as well