From 2c64cf60a2462eab2850a9e7face3339191e5540 Mon Sep 17 00:00:00 2001 From: Jonathan Fung <121899091+jonfung-dydx@users.noreply.github.com> Date: Tue, 21 Nov 2023 18:08:24 -0800 Subject: [PATCH] Statefully validate stateful orders in PrepareCheckState Replay (#797) * statefully validate stateful orders when replaying * AddPreExistingStatefulOrder does not need block height * fake memclob keeper * fix order removal test --- protocol/mocks/MemClobKeeper.go | 37 +++++++++++++++++++++++ protocol/testutil/memclob/keeper.go | 13 ++++++++ protocol/x/clob/e2e/order_removal_test.go | 17 ----------- protocol/x/clob/keeper/orders.go | 5 ++- protocol/x/clob/keeper/orders_test.go | 3 +- protocol/x/clob/memclob/memclob.go | 6 ++-- protocol/x/clob/types/mem_clob_keeper.go | 10 ++++++ 7 files changed, 66 insertions(+), 25 deletions(-) diff --git a/protocol/mocks/MemClobKeeper.go b/protocol/mocks/MemClobKeeper.go index b472609955..88ffde9f52 100644 --- a/protocol/mocks/MemClobKeeper.go +++ b/protocol/mocks/MemClobKeeper.go @@ -47,6 +47,43 @@ func (_m *MemClobKeeper) AddOrderToOrderbookCollatCheck(ctx types.Context, clobP return r0, r1 } +// AddPreexistingStatefulOrder provides a mock function with given fields: ctx, order, memclob +func (_m *MemClobKeeper) AddPreexistingStatefulOrder(ctx types.Context, order *clobtypes.Order, memclob clobtypes.MemClob) (subaccountstypes.BaseQuantums, clobtypes.OrderStatus, *clobtypes.OffchainUpdates, error) { + ret := _m.Called(ctx, order, memclob) + + var r0 subaccountstypes.BaseQuantums + if rf, ok := ret.Get(0).(func(types.Context, *clobtypes.Order, clobtypes.MemClob) subaccountstypes.BaseQuantums); ok { + r0 = rf(ctx, order, memclob) + } else { + r0 = ret.Get(0).(subaccountstypes.BaseQuantums) + } + + var r1 clobtypes.OrderStatus + if rf, ok := ret.Get(1).(func(types.Context, *clobtypes.Order, clobtypes.MemClob) clobtypes.OrderStatus); ok { + r1 = rf(ctx, order, memclob) + } else { + r1 = ret.Get(1).(clobtypes.OrderStatus) + } + + var r2 *clobtypes.OffchainUpdates + if rf, ok := ret.Get(2).(func(types.Context, *clobtypes.Order, clobtypes.MemClob) *clobtypes.OffchainUpdates); ok { + r2 = rf(ctx, order, memclob) + } else { + if ret.Get(2) != nil { + r2 = ret.Get(2).(*clobtypes.OffchainUpdates) + } + } + + var r3 error + if rf, ok := ret.Get(3).(func(types.Context, *clobtypes.Order, clobtypes.MemClob) error); ok { + r3 = rf(ctx, order, memclob) + } else { + r3 = ret.Error(3) + } + + return r0, r1, r2, r3 +} + // CanDeleverageSubaccount provides a mock function with given fields: ctx, subaccountId func (_m *MemClobKeeper) CanDeleverageSubaccount(ctx types.Context, subaccountId subaccountstypes.SubaccountId) (bool, error) { ret := _m.Called(ctx, subaccountId) diff --git a/protocol/testutil/memclob/keeper.go b/protocol/testutil/memclob/keeper.go index 472374204a..5508cd8edc 100644 --- a/protocol/testutil/memclob/keeper.go +++ b/protocol/testutil/memclob/keeper.go @@ -470,6 +470,19 @@ func (f *FakeMemClobKeeper) OffsetSubaccountPerpetualPosition( panic("This function should not be implemented as FakeMemClobKeeper is getting deprecated (CLOB-175)") } +func (f *FakeMemClobKeeper) AddPreexistingStatefulOrder( + ctx sdk.Context, + order *types.Order, + memclob types.MemClob, +) ( + orderSizeOptimisticallyFilledFromMatchingQuantums satypes.BaseQuantums, + orderStatus types.OrderStatus, + offchainUpdates *types.OffchainUpdates, + err error, +) { + panic("This function should not be implemented as FakeMemClobKeeper is getting deprecated (CLOB-175)") +} + func (f *FakeMemClobKeeper) IsLiquidatable( ctx sdk.Context, subaccountId satypes.SubaccountId, diff --git a/protocol/x/clob/e2e/order_removal_test.go b/protocol/x/clob/e2e/order_removal_test.go index e564ffa71f..b014cdbdb0 100644 --- a/protocol/x/clob/e2e/order_removal_test.go +++ b/protocol/x/clob/e2e/order_removal_test.go @@ -606,23 +606,6 @@ func TestOrderRemoval_Invalid(t *testing.T) { }, expectedErr: "Immediate-or-cancel order is fully filled", }, - "invalid proposal: non fully-filled long-term order cannot be removed with fully filled removal reason": { - subaccounts: []satypes.Subaccount{ - constants.Alice_Num0_100_000USD, - }, - orders: []clobtypes.Order{ - constants.LongTermOrder_Alice_Num0_Id0_Clob0_Buy100_Price10_GTBT15, - }, - msgProposedOperations: &clobtypes.MsgProposedOperations{ - OperationsQueue: []clobtypes.OperationRaw{ - clobtestutils.NewOrderRemovalOperationRaw( - constants.LongTermOrder_Alice_Num0_Id0_Clob0_Buy100_Price10_GTBT15.OrderId, - clobtypes.OrderRemoval_REMOVAL_REASON_FULLY_FILLED, - ), - }, - }, - expectedErr: "Order has remaining size", - }, "invalid proposal: post-only removal reason used for non post-only order": { subaccounts: []satypes.Subaccount{ constants.Alice_Num0_100_000USD, diff --git a/protocol/x/clob/keeper/orders.go b/protocol/x/clob/keeper/orders.go index e1ac84655d..df9c243e0f 100644 --- a/protocol/x/clob/keeper/orders.go +++ b/protocol/x/clob/keeper/orders.go @@ -389,7 +389,6 @@ func (k Keeper) ReplayPlaceOrder( func (k Keeper) AddPreexistingStatefulOrder( ctx sdk.Context, order *types.Order, - blockHeight uint32, memclob types.MemClob, ) ( orderSizeOptimisticallyFilledFromMatchingQuantums satypes.BaseQuantums, @@ -399,7 +398,8 @@ func (k Keeper) AddPreexistingStatefulOrder( ) { order.MustBeStatefulOrder() // Perform stateful validation without checking existing order in state. - err = k.PerformStatefulOrderValidation(ctx, order, blockHeight, true) + // Block height is not used when validating stateful orders, so always pass in zero. + err = k.PerformStatefulOrderValidation(ctx, order, 0, true) if err != nil { return 0, 0, nil, err } @@ -456,7 +456,6 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( _, orderStatus, placeOrderOffchainUpdates, err := k.AddPreexistingStatefulOrder( ctx, &order, - 0, k.MemClob, ) diff --git a/protocol/x/clob/keeper/orders_test.go b/protocol/x/clob/keeper/orders_test.go index c9fd0906ed..a9f77d58af 100644 --- a/protocol/x/clob/keeper/orders_test.go +++ b/protocol/x/clob/keeper/orders_test.go @@ -937,7 +937,6 @@ func TestAddPreexistingStatefulOrder(t *testing.T) { _, orderStatus, _, err := ks.ClobKeeper.AddPreexistingStatefulOrder( ctx.WithIsCheckTx(true), &order, - blockHeight, memClob, ) require.NoError(t, err) @@ -966,7 +965,7 @@ func TestAddPreexistingStatefulOrder(t *testing.T) { orderSizeOptimisticallyFilledFromMatching, orderStatus, _, - err := ks.ClobKeeper.AddPreexistingStatefulOrder(ctx.WithIsCheckTx(true), &tc.order, blockHeight, memClob) + err := ks.ClobKeeper.AddPreexistingStatefulOrder(ctx.WithIsCheckTx(true), &tc.order, memClob) // Verify test expectations. require.ErrorIs(t, err, tc.expectedErr) diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index d8b9b1d5b2..662fc530fb 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -969,11 +969,11 @@ func (m *MemClobPriceTimePriority) ReplayOperations( continue } - // Note that we use `memclob.PlaceOrder` here, this will skip writing the stateful order placement to state. // TODO(DEC-998): Research whether it's fine for two post-only orders to be matched. Currently they are dropped. - _, orderStatus, placeOrderOffchainUpdates, err := m.PlaceOrder( + _, orderStatus, placeOrderOffchainUpdates, err := m.clobKeeper.AddPreexistingStatefulOrder( ctx, - statefulOrderPlacement.Order, + &statefulOrderPlacement.Order, + m, ) placedPreexistingStatefulOrderIds[*orderId] = struct{}{} existingOffchainUpdates = m.GenerateOffchainUpdatesForReplayPlaceOrder( diff --git a/protocol/x/clob/types/mem_clob_keeper.go b/protocol/x/clob/types/mem_clob_keeper.go index 8b43eba3ec..1562a2c33f 100644 --- a/protocol/x/clob/types/mem_clob_keeper.go +++ b/protocol/x/clob/types/mem_clob_keeper.go @@ -57,6 +57,16 @@ type MemClobKeeper interface { offchainUpdates *OffchainUpdates, err error, ) + AddPreexistingStatefulOrder( + ctx sdk.Context, + order *Order, + memclob MemClob, + ) ( + orderSizeOptimisticallyFilledFromMatchingQuantums satypes.BaseQuantums, + orderStatus OrderStatus, + offchainUpdates *OffchainUpdates, + err error, + ) CancelShortTermOrder( ctx sdk.Context, msgCancelOrder *MsgCancelOrder,