From c2fe01826843944cdbe6202d21e4d0198e7f5990 Mon Sep 17 00:00:00 2001 From: Roy Li Date: Tue, 4 Jun 2024 00:28:43 -0400 Subject: [PATCH] Restrict order replacement to only changing size, price, GTT/GTB --- .../x/clob/e2e/reduce_only_orders_test.go | 349 +----------------- protocol/x/clob/e2e/short_term_orders_test.go | 59 +-- protocol/x/clob/memclob/memclob.go | 52 ++- .../clob/memclob/memclob_place_order_test.go | 49 ++- protocol/x/clob/types/errors.go | 3 +- 5 files changed, 87 insertions(+), 425 deletions(-) diff --git a/protocol/x/clob/e2e/reduce_only_orders_test.go b/protocol/x/clob/e2e/reduce_only_orders_test.go index 9513b9aa35..d44d626558 100644 --- a/protocol/x/clob/e2e/reduce_only_orders_test.go +++ b/protocol/x/clob/e2e/reduce_only_orders_test.go @@ -674,6 +674,20 @@ func TestReduceOnlyOrderFailure(t *testing.T) { "", }, }, + "Regular Reduce only order fails because disabled": { + subaccounts: []satypes.Subaccount{ + constants.Alice_Num1_1BTC_Short_100_000USD, + }, + orders: []clobtypes.Order{ + testapp.MustScaleOrder( + constants.Order_Alice_Num1_Id1_Clob0_Sell10_Price15_GTB20_RO, + testapp.DefaultGenesis(), + ), + }, + errorMsg: []string{ + clobtypes.ErrReduceOnlyDisabled.Error(), + }, + }, } for name, tc := range tests { @@ -715,338 +729,3 @@ func TestReduceOnlyOrderFailure(t *testing.T) { }) } } - -func TestReduceOnlyOrderReplacement(t *testing.T) { - tests := map[string]struct { - subaccounts []satypes.Subaccount - firstOrders []clobtypes.Order - secondOrders []clobtypes.Order - secondOrdersErrors []string - - expectedOrderFillAmounts map[uint32]map[clobtypes.OrderId]uint64 - }{ - `A regular order is partially filled. Replacement FOK RO order fails because it is immediate execution.`: { - subaccounts: []satypes.Subaccount{ - { - Id: &constants.Alice_Num1, - AssetPositions: []*satypes.AssetPosition{ - &constants.Usdc_Asset_100_000, - }, - PerpetualPositions: []*satypes.PerpetualPosition{ - { - PerpetualId: 0, - Quantums: dtypes.NewInt(60), // 60 quantums of BTC long - }, - }, - }, - constants.Carl_Num0_100000USD, - }, - - firstOrders: []clobtypes.Order{ - // Regular order on the opposite side of the following replacement RO order. - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20, - // Partial match for the above order. 70 quantums are matched. Thus, current position size - // for Alice is 60 quantums long - 70 quantums = 10 quantums short. - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10, - }, - - secondOrders: []clobtypes.Order{ - // Currently, IOC/FOK replacement orders for orders that are partially filled are not allowed. - // Because the order was partially filled, this results in a ErrInvalidReplacement error. - // If IOC/FOK replacement orders were allowed, this buy RO order should succeed because the - // current position size is 10 quantums short, and buying reduces a short position. - constants.Order_Alice_Num1_Id0_Clob0_Buy110_Price50000_GTB21_FOK_RO, - }, - secondOrdersErrors: []string{ - clobtypes.ErrInvalidReplacement.Error(), - }, - - expectedOrderFillAmounts: map[uint32]map[clobtypes.OrderId]uint64{ - 2: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20.OrderId: 70, - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10.OrderId: 70, - }, - 3: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20.OrderId: 70, - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10.OrderId: 70, - }, - }, - }, - `A regular order is partially filled. Replacement IOC RO order fails because it is immediate execution.`: { - subaccounts: []satypes.Subaccount{ - { - Id: &constants.Alice_Num1, - AssetPositions: []*satypes.AssetPosition{ - &constants.Usdc_Asset_100_000, - }, - PerpetualPositions: []*satypes.PerpetualPosition{ - { - PerpetualId: 0, - Quantums: dtypes.NewInt(60), // 60 quantums of BTC long - }, - }, - }, - constants.Carl_Num0_100000USD, - }, - - firstOrders: []clobtypes.Order{ - // Regular order on the opposite side of the following replacement RO order. - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20, - // Partial match for the above order. 70 quantums are matched. Thus, current position size - // for Alice is 60 quantums long - 70 quantums = 10 quantums short. - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10, - }, - - secondOrders: []clobtypes.Order{ - // Currently, IOC/FOK replacement orders for orders that are partially filled are not allowed. - // Because the order was partially filled, this results in a ErrInvalidReplacement error. - // If IOC/FOK replacement orders were allowed, this buy RO order should succeed because the - // current position size is 10 quantums short, and buying reduces a short position. - constants.Order_Alice_Num1_Id0_Clob0_Buy110_Price50000_GTB21_IOC_RO, - }, - secondOrdersErrors: []string{ - clobtypes.ErrInvalidReplacement.Error(), - }, - - expectedOrderFillAmounts: map[uint32]map[clobtypes.OrderId]uint64{ - 2: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20.OrderId: 70, - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10.OrderId: 70, - }, - 3: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20.OrderId: 70, - constants.Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10.OrderId: 70, - }, - }, - }, - `Position size is long. A regular order is placed but not filled. Replacement Sell FOK RO - reduces current long position size, but fails because it is resized to a smaller size due to - current position size.`: { - subaccounts: []satypes.Subaccount{ - { - Id: &constants.Alice_Num1, - AssetPositions: []*satypes.AssetPosition{ - &constants.Usdc_Asset_100_000, - }, - PerpetualPositions: []*satypes.PerpetualPosition{ - { - PerpetualId: 0, - Quantums: dtypes.NewInt(60), // 60 quantums of BTC long - }, - }, - }, - constants.Carl_Num0_100000USD, - }, - - firstOrders: []clobtypes.Order{ - // Regular order on the opposite side of the following replacement RO order. - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20, - }, - - secondOrders: []clobtypes.Order{ - // Full match for the below order. - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10, - // The original order being replaced has no partial fills. This sell RO order should succeed because the - // current position size is 60 quantums long, and selling reduces a long position. However, - // the RO property resizes the order to the current position size (60) and thus the FOK order - // cannot be fully filled and errors out. - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_FOK_RO, - }, - secondOrdersErrors: []string{ - "", - clobtypes.ErrFokOrderCouldNotBeFullyFilled.Error(), - }, - - expectedOrderFillAmounts: map[uint32]map[clobtypes.OrderId]uint64{ - 2: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20.OrderId: 0, - }, - 3: { - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_FOK_RO.OrderId: 0, - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10.OrderId: 0, - }, - }, - }, - `Position size is long. A regular order is placed but not filled. Replacement Sell FOK RO - reduces current long position size, and succeeds because subaccount has enough position - to not resize the order smaller.`: { - subaccounts: []satypes.Subaccount{ - { - Id: &constants.Alice_Num1, - AssetPositions: []*satypes.AssetPosition{ - &constants.Usdc_Asset_100_000, - }, - PerpetualPositions: []*satypes.PerpetualPosition{ - { - PerpetualId: 0, - Quantums: dtypes.NewInt(110), // 110 quantums of BTC long - }, - }, - }, - constants.Carl_Num0_100000USD, - }, - - firstOrders: []clobtypes.Order{ - // Regular order on the opposite side of the following replacement RO order. - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20, - }, - - secondOrders: []clobtypes.Order{ - // Full match for the below order. - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10, - // The original order being replaced has no partial fills. This sell RO order should succeed because the - // current position size is 110 quantums long, and selling reduces a long position. RO property of the - // order does not resize the order and order succeeds for full amount. - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_FOK_RO, - }, - secondOrdersErrors: []string{ - "", - "", - }, - - expectedOrderFillAmounts: map[uint32]map[clobtypes.OrderId]uint64{ - 2: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20.OrderId: 0, - }, - 3: { - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_FOK_RO.OrderId: 110, - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10.OrderId: 110, - }, - }, - }, - `Position size is long. A regular order is placed but not filled. Replacement Sell IOC RO - reduces current long position size, but is resized to a smaller size due to current position size - and partially filled.`: { - subaccounts: []satypes.Subaccount{ - { - Id: &constants.Alice_Num1, - AssetPositions: []*satypes.AssetPosition{ - &constants.Usdc_Asset_100_000, - }, - PerpetualPositions: []*satypes.PerpetualPosition{ - { - PerpetualId: 0, - Quantums: dtypes.NewInt(60), // 60 quantums of BTC long - }, - }, - }, - constants.Carl_Num0_100000USD, - }, - - firstOrders: []clobtypes.Order{ - // Regular order on the opposite side of the following replacement RO order. - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20, - }, - - secondOrders: []clobtypes.Order{ - // Full match for the below order. - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10, - // The original order being replaced has no partial fills. This sell RO order should succeed because the - // current position size is 60 quantums long, and selling reduces a long position. - // The RO property resizes the order to the current position size (60) and thus the IOC order - // is partially filled. - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_IOC_RO, - }, - secondOrdersErrors: []string{ - "", - "", - }, - - expectedOrderFillAmounts: map[uint32]map[clobtypes.OrderId]uint64{ - 2: { - constants.Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20.OrderId: 0, - }, - 3: { - constants.Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_IOC_RO.OrderId: 60, - constants.Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10.OrderId: 60, - }, - }, - }, - } - - for name, tc := range tests { - t.Run(name, func(t *testing.T) { - tApp := testapp.NewTestAppBuilder(t). - WithGenesisDocFn(func() (genesis types.GenesisDoc) { - genesis = testapp.DefaultGenesis() - if len(tc.subaccounts) > 0 { - testapp.UpdateGenesisDocWithAppStateForModule( - &genesis, - func(genesisState *satypes.GenesisState) { - genesisState.Subaccounts = tc.subaccounts - }, - ) - } - return genesis - }). - WithCrashingAppCheckTxNonDeterminismChecksEnabled(false). - Build() - ctx := tApp.InitChain() - - // place first set of orders. - for _, order := range tc.firstOrders { - for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg( - ctx, - tApp.App, - *clobtypes.NewMsgPlaceOrder(order), - ) { - resp := tApp.CheckTx(checkTx) - require.Conditionf(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) - } - } - // Advance the block to persist matches. - ctx = tApp.AdvanceToBlock(2, testapp.AdvanceToBlockOptions{}) - - // validate order fill amounts. - if orderMap, exists := tc.expectedOrderFillAmounts[2]; exists { - for orderId, expectedFillAmount := range orderMap { - exists, actualFillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) - if expectedFillAmount == 0 { - require.False(t, exists) - } else { - require.True(t, exists) - require.Equal(t, expectedFillAmount, actualFillAmount.ToUint64()) - } - } - } - - // place second set of orders. - for idx, order := range tc.secondOrders { - for _, checkTx := range testapp.MustMakeCheckTxsWithClobMsg( - ctx, - tApp.App, - *clobtypes.NewMsgPlaceOrder(order), - ) { - resp := tApp.CheckTx(checkTx) - - if tc.secondOrdersErrors[idx] == "" { - require.Conditionf(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) - } else { - require.Conditionf(t, resp.IsErr, "Expected CheckTx to error. Response: %+v", resp) - require.Contains( - t, - resp.Log, - tc.secondOrdersErrors[idx], - ) - } - } - } - - // Advance the block to persist matches. - ctx = tApp.AdvanceToBlock(3, testapp.AdvanceToBlockOptions{}) - - // validate order fill amounts. - if orderMap, exists := tc.expectedOrderFillAmounts[3]; exists { - for orderId, expectedFillAmount := range orderMap { - exists, actualFillAmount, _ := tApp.App.ClobKeeper.GetOrderFillAmount(ctx, orderId) - if expectedFillAmount == 0 { - require.False(t, exists) - } else { - require.True(t, exists) - require.Equal(t, expectedFillAmount, actualFillAmount.ToUint64()) - } - } - } - }) - } -} diff --git a/protocol/x/clob/e2e/short_term_orders_test.go b/protocol/x/clob/e2e/short_term_orders_test.go index a3128134cb..62272dfaea 100644 --- a/protocol/x/clob/e2e/short_term_orders_test.go +++ b/protocol/x/clob/e2e/short_term_orders_test.go @@ -696,7 +696,7 @@ func TestShortTermOrderReplacements(t *testing.T) { }, }, }, - "Success: Replace in same block on opposite side": { + "Fail: Replace in same block on opposite side": { blocks: []blockOrdersAndExpectations{ { ordersToPlace: []clobtypes.MsgPlaceOrder{ @@ -704,9 +704,9 @@ func TestShortTermOrderReplacements(t *testing.T) { PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21, }, orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ - PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21.Order.OrderId: { + PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { shouldExistOnMemclob: true, - expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21.Order, + expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order, }, }, }, @@ -840,49 +840,6 @@ func TestShortTermOrderReplacements(t *testing.T) { }, }, }, - "Success: Replacement order swaps side in next block after partial fill": { - blocks: []blockOrdersAndExpectations{ - { - ordersToPlace: []clobtypes.MsgPlaceOrder{ - PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20, - *clobtypes.NewMsgPlaceOrder(testapp.MustScaleOrder( - 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, - }, - 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{ - PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21, - }, - orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ - PlaceOrder_Alice_Num0_Id0_Clob0_Buy7_Price10_GTB21.Order.OrderId: { - shouldExistOnMemclob: true, - expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21.Order, - expectedFillAmount: PlaceOrder_Alice_Num0_Id0_Clob0_Sell6_Price10_GTB21.Order.Quantums / 2, - }, - }, - }, - }, - }, "Success: Replacement order decreases size in next block after partial fill": { blocks: []blockOrdersAndExpectations{ { @@ -1015,7 +972,7 @@ func TestShortTermOrderReplacements(t *testing.T) { }, }, }, - "Success: Replacing order with FOK which does not fully match results in order being removed from the book": { + "Fail: Replacing order with FOK fails": { blocks: []blockOrdersAndExpectations{ { ordersToPlace: []clobtypes.MsgPlaceOrder{ @@ -1024,13 +981,14 @@ func TestShortTermOrderReplacements(t *testing.T) { }, orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { - shouldExistOnMemclob: false, + shouldExistOnMemclob: true, + expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order, }, }, }, }, }, - "Success: Replacing order with IOC which does not fully match results in order being removed from the book": { + "Fail: Replacing order with IOC fails": { blocks: []blockOrdersAndExpectations{ { ordersToPlace: []clobtypes.MsgPlaceOrder{ @@ -1039,7 +997,8 @@ func TestShortTermOrderReplacements(t *testing.T) { }, orderIdsExpectations: map[clobtypes.OrderId]orderIdExpectations{ PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order.OrderId: { - shouldExistOnMemclob: false, + shouldExistOnMemclob: true, + expectedOrder: PlaceOrder_Alice_Num0_Id0_Clob0_Buy6_Price10_GTB20.Order, }, }, }, diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index 72ef7748c9..2ad5310654 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -1334,11 +1334,41 @@ func (m *MemClobPriceTimePriority) PurgeInvalidMemclobState( return existingOffchainUpdates } +// validateReplacement validates that an order can replace another. +// Only size, price, and GTB/GTT can change. GTB/GTT cannot be reduced. +func validateReplacement(existing, new types.Order) error { + if existing.MustCmpReplacementOrder(&new) >= 0 { + return errorsmod.Wrapf( + types.ErrInvalidReplacement, + "New order cannot reduce the GoodTilBlock or GoodTilBlockTime of existing order", + ) + } + if existing.Side != new.Side { + return errorsmod.Wrapf( + types.ErrInvalidReplacement, + "New order cannot change the Side of the existing order", + ) + } + if existing.TimeInForce != new.TimeInForce { + return errorsmod.Wrapf( + types.ErrInvalidReplacement, + "New order cannot change the TimeInForce of the existing order", + ) + } + if existing.ReduceOnly != new.ReduceOnly { + return errorsmod.Wrapf( + types.ErrInvalidReplacement, + "New order cannot change the ReduceOnly field of the existing order", + ) + } + // TODO(DEC-1238): Support stateful order replacements. + return nil +} + // validateNewOrder will perform the following validation against the memclob's in-memory state to ensure the order // can be placed (and if any condition is false, an error will be returned): // - The order is not canceled (with an equal-to-or-greater-than `GoodTilBlock` than the new order). -// - If the order is replacing another order, then the new order's expiration must not be less than the -// existing order's expiration. +// - The order is a valid replacement // // Note that it does not perform collateralization checks since that will be done when matching the order (if the order // overlaps the book) and when adding the order to the book (if the order has remaining size after matching). @@ -1388,17 +1418,15 @@ func (m *MemClobPriceTimePriority) validateNewOrder( existingRestingOrder, restingOrderExists := orderbook.getOrder(orderId) existingMatchedOrder, matchedOrderExists := m.operationsToPropose.MatchedOrderIdToOrder[orderId] - - // If an order with the same `OrderId` already exists on the orderbook (or was already matched), - // then we must validate that the new order's `GoodTilBlock` is greater-in-value than the old order. - // If greater, then it can be placed (replacing the old order if it was resting on the book). - // If equal-or-lesser, then it is dropped. - if restingOrderExists && existingRestingOrder.MustCmpReplacementOrder(&order) >= 0 { - return types.ErrInvalidReplacement + if restingOrderExists { + if err := validateReplacement(existingRestingOrder, order); err != nil { + return err + } } - - if matchedOrderExists && existingMatchedOrder.MustCmpReplacementOrder(&order) >= 0 { - return types.ErrInvalidReplacement + if matchedOrderExists { + if err := validateReplacement(existingMatchedOrder, order); err != nil { + return err + } } // If the order is a reduce-only order, we should ensure that the sign of the order size is the opposite of diff --git a/protocol/x/clob/memclob/memclob_place_order_test.go b/protocol/x/clob/memclob/memclob_place_order_test.go index 3744160f78..17eed53fbf 100644 --- a/protocol/x/clob/memclob/memclob_place_order_test.go +++ b/protocol/x/clob/memclob/memclob_place_order_test.go @@ -75,7 +75,7 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) { expectedToReplaceOrder: false, }, `Can place a new sell order on an orderbook with asks at same price level, and best ask is not updated but total - level quantums is updated`: { + level quantums is updated`: { existingOrders: []types.MatchableOrder{ &constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20, }, @@ -87,7 +87,7 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) { expectedToReplaceOrder: false, }, `Can place a new buy order on an orderbook with bids at same price level, and best bid is not updated but total - level quantums is updated`: { + level quantums is updated`: { existingOrders: []types.MatchableOrder{ &constants.Order_Alice_Num1_Id2_Clob1_Buy67_Price5_GTB20, }, @@ -204,16 +204,15 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) { expectedOrderStatus: types.Success, expectedToReplaceOrder: true, }, - "Replacing an order succeeds if GoodTilBlock is greater than existing order and changes sides": { + "Replacing an order fails if GoodTilBlock is greater than existing order and changes sides": { existingOrders: []types.MatchableOrder{ &constants.Order_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB15, }, order: constants.Order_Alice_Num0_Id0_Clob0_Sell5_Price10_GTB20, - collateralizationCheck: satypes.Success, - expectedOrderStatus: types.Success, - expectedToReplaceOrder: true, + expectedErr: types.ErrInvalidReplacement, + expectedToReplaceOrder: false, }, "Replacing an order succeeds if GoodTilBlock is greater than existing order and changes price": { existingOrders: []types.MatchableOrder{ @@ -254,18 +253,6 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) { order: constants.Order_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB20, - collateralizationCheck: satypes.Success, - expectedOrderStatus: types.Success, - expectedToReplaceOrder: true, - }, - `Replacing an order succeeds and old order is skipped during matching if GoodTilBlock is greater - than existing order and the new replacement order is on the opposite side of the existing order`: { - existingOrders: []types.MatchableOrder{ - &constants.Order_Alice_Num0_Id0_Clob0_Buy5_Price10_GTB15, - }, - - order: constants.Order_Alice_Num0_Id0_Clob0_Sell5_Price10_GTB20, - collateralizationCheck: satypes.Success, expectedOrderStatus: types.Success, expectedToReplaceOrder: true, @@ -327,7 +314,8 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) { // If this is an order replacement and it was successful, we assert that the old order being replaced // is no longer on the book. matchableOrderOrder := matchableOrder.MustGetOrder() - if matchableOrderOrder.OrderId == tc.order.OrderId && tc.order.MustCmpReplacementOrder(&matchableOrderOrder) > 0 { + if tc.expectedToReplaceOrder && matchableOrderOrder.OrderId == tc.order.OrderId && + tc.order.MustCmpReplacementOrder(&matchableOrderOrder) > 0 { if matchableOrderOrder.Subticks != tc.order.Subticks { expectedReplacementOrderPriceChanged = true } @@ -2199,28 +2187,37 @@ func TestPlaceOrder_MatchOrders_PreexistingMatches(t *testing.T) { expectedErr: types.ErrInvalidReplacement, }, - "IOC Taker replaces unfilled non IOC order": { + "IOC Taker cannot replace unfilled non IOC order": { placedMatchableOrders: []types.MatchableOrder{ &constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20, }, + expectedRemainingAsks: []OrderWithRemainingSize{ + { + Order: constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20, + RemainingSize: 10, + }, + }, order: constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB21_IOC, expectedInternalOperations: []types.InternalOperation{}, - expectedOrderStatus: types.ImmediateOrCancelWouldRestOnBook, - expectedToReplaceOrder: true, + expectedErr: types.ErrInvalidReplacement, }, - "FOK Taker replaces unfilled non IOC order": { + "FOK Taker cannot replace unfilled non IOC order": { placedMatchableOrders: []types.MatchableOrder{ &constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20, }, + expectedRemainingAsks: []OrderWithRemainingSize{ + { + Order: constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB20, + RemainingSize: 10, + }, + }, order: constants.Order_Alice_Num1_Id1_Clob1_Sell10_Price15_GTB21_FOK, expectedInternalOperations: []types.InternalOperation{}, - expectedOrderStatus: types.InternalError, - expectedToReplaceOrder: true, - expectedErr: types.ErrFokOrderCouldNotBeFullyFilled, + expectedErr: types.ErrInvalidReplacement, }, } diff --git a/protocol/x/clob/types/errors.go b/protocol/x/clob/types/errors.go index cc7ac4f4b3..bf71b05325 100644 --- a/protocol/x/clob/types/errors.go +++ b/protocol/x/clob/types/errors.go @@ -88,8 +88,7 @@ var ( ErrInvalidReplacement = errorsmod.Register( ModuleName, 20, - "An order with the same `OrderId` already exists for this CLOB with a greater-than-or-equal `GoodTilBlock` "+ - "or Order Hash", + "Replacing an existing order failed", ) ErrClobPairAndPerpetualDoNotMatch = errorsmod.Register( ModuleName,