[CLOB-184] - e2e short term order replacement tests #491
Conversation
CLOB-184 Add more end-to-end tests to cover order replacement edge cases
We should ensure we have good test coverage around order replacements. Some examples of additional test coverage to add would be the following:
Lower priority and can be put in a separate ticket:
|
WalkthroughThe changes primarily focus on enhancing the testing of the trading system. New test cases have been added to verify the functionality of order replacements under various scenarios. Additionally, new order placements with different parameters have been introduced to test different configurations and scenarios for order placement. Changes
TipsChat with CodeRabbit Bot (
|
![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
Files selected for processing (3)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (1 hunks)
- protocol/x/clob/types/message_proposed_operations.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/types/message_proposed_operations.go
Additional comments (Suppressed): 3
protocol/x/clob/e2e/app_test.go (1)
- 53-88: The new hunks introduce three new order placements with varying parameters such as the number of quantums and the side of the order. These orders are created using the
NewMsgPlaceOrderfunction and are based on the default genesis state. The code seems to be logically correct, but it's important to ensure that these new orders are being used correctly in the tests where they are referenced.protocol/x/clob/e2e/short_term_orders_test.go (2)
769-1097: The new test function
TestShortTermOrderReplacementsis comprehensive and covers a variety of scenarios for short-term order replacements. It tests replacing orders within the same block, replacing orders in the next block, handling partial fills, and various failure cases. The structure of the test cases is well organized with clear expectations defined for each case. However, there's no error handling or recovery mechanism if any of the assertions fail. Consider adding cleanup or recovery code to handle such situations.1083-1094: This loop verifies the state of the orders after they've been placed and potentially replaced. It checks whether the order exists on the memclob, whether the order matches the expected order, and whether the fill amount is as expected. These checks are thorough and cover the important aspects of order placement and replacement. However, the error messages from the assertions could be more descriptive to help diagnose failures.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (1 hunks)
Additional comments (Suppressed): 1
protocol/x/clob/e2e/app_test.go (1)
- 86-118: The new hunks introduce three new order placements with varying parameters. The orders are created using the
NewMsgPlaceOrderfunction and are based on the default genesis state. The orders are replacements of a previous order with different parameters such as the number of quantums, the side of the order (buy/sell), and the good-til-block value. Ensure that these changes align with the intended business logic and that the parameters are correctly set for each order.
| )), | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB21, | ||
| }, | ||
| orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ |
There was a problem hiding this comment.
Can we add some expectations that PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20 does not exist anywhere?
Also the 2nd message that was placed should have a fill amount too?
There was a problem hiding this comment.
As for 1) By asserting that PlaceOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB21.Order exists on the memclob, we are simultaneously asserting that PlaceOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB20.Order does not exist. These two orders share an OrderId and all data stored on both the memclob and in state is keyed off of OrderId.
2). Yes the second order should have a fill amount but honestly I feel like it is fine to not assert that here. It makes the test simpler to read by only asserting information relevant to the order being replaced. We have other tests that validate matching
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Files selected for processing (2)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (1 hunks)
Additional comments (Suppressed): 2
protocol/x/clob/e2e/app_test.go (1)
- 86-118: The new hunks introduce three new order placements with varying parameters such as the number of quantums, the side of the order (buy/sell), and the good-til-block value. These orders are created using the
NewMsgPlaceOrderfunction and are based on the default genesis state. The orders are replacements of a previous order with different parameters. The code seems to be logically correct and follows the best practices for creating new orders in the system. However, ensure that these new orders are being used correctly in the tests and that they are producing the expected results.protocol/x/clob/e2e/short_term_orders_test.go (1)
- 610-1023: The new hunk introduces a comprehensive test suite for short term order replacements. It covers a variety of scenarios including successful replacements, failed replacements due to various conditions, and replacements with different order types (FOK, IOC). The tests are well-structured and thorough, covering edge cases and ensuring the correct behavior of the system under different conditions. However, there are a couple of issues that need to be addressed.
| clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: constants.Bob_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: 3, | ||
| Subticks: 10, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| }, | ||
| testapp.DefaultGenesis(), | ||
| )), | ||
| }, | ||
| orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { | ||
| shouldExistOnMemclob: true, | ||
| expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order, | ||
| expectedFillAmount: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.Quantums / 2, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| ordersToPlace: []clobtypes.MsgPlaceOrder{ | ||
| *clobtypes.NewMsgPlaceOrder(MustScaleOrder( | ||
| clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: constants.Alice_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 2, | ||
| Subticks: 10, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| }, | ||
| testapp.DefaultGenesis(), | ||
| )), | ||
| }, | ||
| orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { | ||
| shouldExistOnMemclob: true, | ||
| expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order, | ||
| expectedFillAmount: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.Quantums / 2, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "Success: Replacing order with FOK which does not fully match results in order being removed from the book": { | ||
| blocks: []blockOrdersAndExpectations{ | ||
| { | ||
| ordersToPlace: []clobtypes.MsgPlaceOrder{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20, | ||
| fok_replacement, | ||
| }, | ||
| orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { | ||
| shouldExistOnMemclob: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| "Success: Replacing order with IOC which does not fully match results in order being removed from the book": { | ||
| blocks: []blockOrdersAndExpectations{ | ||
| { | ||
| ordersToPlace: []clobtypes.MsgPlaceOrder{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20, | ||
| ioc_replacement, | ||
| }, | ||
| orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ | ||
| PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { | ||
| shouldExistOnMemclob: false, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| tApp := testapp.NewTestAppBuilder(t).Build() | ||
| ctx := tApp.InitChain() | ||
|
|
||
| for i, block := range tc.blocks { | ||
| for _, order := range block.ordersToPlace { | ||
| for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg(ctx, tApp.App, order) { | ||
| tApp.CheckTx(checkTx) | ||
| } | ||
| } | ||
|
|
||
| for orderId, expectations := range block.orderIdsExpectations { | ||
| order, exists := tApp.App.ClobKeeper.MemClob.GetOrder(ctx, orderId) | ||
| require.Equal(t, expectations.shouldExistOnMemclob, exists) | ||
| if expectations.shouldExistOnMemclob { | ||
| require.Equal(t, expectations.expectedOrder, order) | ||
| } | ||
| _, fillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) | ||
| require.Equal(t, expectations.expectedFillAmount, uint64(fillAmount)) | ||
| } | ||
|
|
||
| ctx = tApp.AdvanceToBlock(uint32(i+2), testapp.AdvanceToBlockOptions{}) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new hunk introduces a comprehensive test suite for short term order replacements. It covers a variety of scenarios including successful replacements, failed replacements due to lower GTB, equal GTB, and attempts to decrease size below partially filled amount. It also tests the behavior of FOK and IOC orders. The tests are well-structured and thorough, covering a wide range of edge cases. However, there is a potential issue with the test setup.
In the test setup, the same order object is being used and modified for different test cases (lines 611-616). This could potentially lead to unexpected side effects if the order object is modified in one test case and then used in another. It would be safer to create a new order object for each test case to ensure isolation between tests.
- order := PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20
- fok_replacement := order
- fok_replacement.Order.GoodTilOneof = &clobtypes.Order_GoodTilBlock{GoodTilBlock: 21}
- fok_replacement.Order.TimeInForce = clobtypes.Order_TIME_IN_FORCE_FILL_OR_KILL
- ioc_replacement := fok_replacement
- ioc_replacement.Order.TimeInForce = clobtypes.Order_TIME_IN_FORCE_IOC
+ order := func() clobtypes.MsgPlaceOrder {
+ o := PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20
+ return o
+ }
+ fok_replacement := func() clobtypes.MsgPlaceOrder {
+ o := order()
+ o.Order.GoodTilOneof = &clobtypes.Order_GoodTilBlock{GoodTilBlock: 21}
+ o.Order.TimeInForce = clobtypes.Order_TIME_IN_FORCE_FILL_OR_KILL
+ return o
+ }
+ ioc_replacement := func() clobtypes.MsgPlaceOrder {
+ o := fok_replacement()
+ o.Order.TimeInForce = clobtypes.Order_TIME_IN_FORCE_IOC
+ return o
+ }
Changelist
Summary by CodeRabbit
TestShortTermOrderReplacementsto validate the behavior of order replacements in different scenarios, including same block replacements, next block replacements, and handling of partial fills.validateShortTermOrderPlacementOperationfunction.