From b8496ea824cc182d248c08a3eec9cea8912dfc48 Mon Sep 17 00:00:00 2001 From: Lucas Date: Tue, 24 Oct 2023 15:47:28 -0400 Subject: [PATCH 1/5] Initial PrepareCheckState audit --- protocol/x/clob/keeper/orders.go | 11 +++++++++++ protocol/x/clob/memclob/memclob.go | 20 +++++++++++++------- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/protocol/x/clob/keeper/orders.go b/protocol/x/clob/keeper/orders.go index 52473ea9f7..55ee9ee6be 100644 --- a/protocol/x/clob/keeper/orders.go +++ b/protocol/x/clob/keeper/orders.go @@ -454,6 +454,17 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( orderPlacement, exists := k.GetLongTermOrderPlacement(ctx, orderId) if !exists { + // Error log if this is a conditional orders and it doesn't exist in state, since it + // can't have been canceled. + if orderId.IsConditionalOrder() { + k.Logger(ctx).Error( + fmt.Sprintf( + "PlaceStatefulOrdersFromLastBlock: Triggered conditional Order ID %+v doesn't exist in state", + orderId, + ), + ) + } + // Order does not exist in state and therefore should not be placed. This likely // indicates that the order was cancelled. continue diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index 50a30efb6c..ba5ff5f2ac 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -1004,7 +1004,7 @@ func (m *MemClobPriceTimePriority) ReplayOperations( } // Log an error if there are two Order Removals for the same OrderId - if _, found := placedPreexistingStatefulOrderIds[orderId]; found { + if _, found := placedOrderRemovalOrderIds[orderId]; found { m.clobKeeper.Logger(ctx).Error( "ReplayOperations: OrderRemoval operation for order which was already removed", metrics.OrderId, orderId, @@ -1132,12 +1132,12 @@ func (m *MemClobPriceTimePriority) RemoveAndClearOperationsQueue( otpOrderHash := operation.GetShortTermOrderPlacement().Order.GetOrderHash() // If the order exists in the book, remove it. - // Else if the order is a Short-Term order, since it's no longer on the book or operations - // queue we should remove the order hash from `ShortTermOrderTxBytes`. + // Else, since the Short-Term order is no longer on the book or operations queue we + // should remove the order hash from `ShortTermOrderTxBytes`. existingOrder, found := m.openOrders.getOrder(ctx, otpOrderId) if found && existingOrder.GetOrderHash() == otpOrderHash { m.mustRemoveOrder(ctx, otpOrderId) - } else if otpOrderId.IsShortTermOrder() { + } else { order := operation.GetShortTermOrderPlacement().Order m.operationsToPropose.RemoveShortTermOrderTxBytes(order) } @@ -1165,7 +1165,7 @@ func (m *MemClobPriceTimePriority) RemoveAndClearOperationsQueue( // - Forcefully removed stateful orders. func (m *MemClobPriceTimePriority) PurgeInvalidMemclobState( ctx sdk.Context, - fullyFilledOrderIds []types.OrderId, + filledOrderIds []types.OrderId, expiredStatefulOrderIds []types.OrderId, canceledStatefulOrderIds []types.OrderId, removedStatefulOrderIds []types.OrderId, @@ -1183,7 +1183,7 @@ func (m *MemClobPriceTimePriority) PurgeInvalidMemclobState( blockHeight := lib.MustConvertIntegerToUint32(ctx.BlockHeight()) // Remove all fully-filled order IDs from the memclob if they exist. - for _, orderId := range fullyFilledOrderIds { + for _, orderId := range filledOrderIds { m.RemoveOrderIfFilled(ctx, orderId) } @@ -2003,8 +2003,14 @@ func (m *MemClobPriceTimePriority) RemoveOrderIfFilled( // Get current fill amount for this order. exists, orderStateFillAmount, _ := m.clobKeeper.GetOrderFillAmount(ctx, orderId) - // If there is no fill amount for this order, return early. + // If there is no fill amount for this order, error log and return early. Note this is an + // error since this function should only be called for orders that were partially or fully filled. if !exists { + m.clobKeeper.Logger(ctx).Error( + "RemoveOrderIfFilled: order ID that should be partially or fully filled has no fill amount", + "orderId", + orderId, + ) return } From b225ed8f2677939feca0f611cdebb2a77f3b18db Mon Sep 17 00:00:00 2001 From: Lucas Date: Tue, 24 Oct 2023 17:29:38 -0400 Subject: [PATCH 2/5] Panic if PlacePerpetualLiquidation returns an error --- protocol/x/clob/keeper/liquidations.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/protocol/x/clob/keeper/liquidations.go b/protocol/x/clob/keeper/liquidations.go index 96cb0c6a6f..4fe10fadd8 100644 --- a/protocol/x/clob/keeper/liquidations.go +++ b/protocol/x/clob/keeper/liquidations.go @@ -271,6 +271,9 @@ func (k Keeper) PlacePerpetualLiquidation( ctx, liquidationOrder, ) + if err != nil { + return err + } // TODO(DEC-1323): Potentially allow liquidating the same perpetual + subaccount // multiple times in a block. From 49fea6d90ac55fc2b6ae792390c9539565e744f7 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 25 Oct 2023 08:27:29 -0400 Subject: [PATCH 3/5] Address PR comments --- protocol/x/clob/keeper/orders.go | 2 +- protocol/x/clob/memclob/memclob.go | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/protocol/x/clob/keeper/orders.go b/protocol/x/clob/keeper/orders.go index 55ee9ee6be..d942f797f3 100644 --- a/protocol/x/clob/keeper/orders.go +++ b/protocol/x/clob/keeper/orders.go @@ -454,7 +454,7 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( orderPlacement, exists := k.GetLongTermOrderPlacement(ctx, orderId) if !exists { - // Error log if this is a conditional orders and it doesn't exist in state, since it + // Error log if this is a conditional orders and it doesn't exist in triggered state, since it // can't have been canceled. if orderId.IsConditionalOrder() { k.Logger(ctx).Error( diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index ba5ff5f2ac..800222a565 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -2003,14 +2003,17 @@ func (m *MemClobPriceTimePriority) RemoveOrderIfFilled( // Get current fill amount for this order. exists, orderStateFillAmount, _ := m.clobKeeper.GetOrderFillAmount(ctx, orderId) - // If there is no fill amount for this order, error log and return early. Note this is an - // error since this function should only be called for orders that were partially or fully filled. + // If there is no fill amount for this order, return early. Note this is only valid if the + // order is a stateful order that was fully-filled or partially-filled and expired / canceled / + // removed in the last block. if !exists { - m.clobKeeper.Logger(ctx).Error( - "RemoveOrderIfFilled: order ID that should be partially or fully filled has no fill amount", - "orderId", - orderId, - ) + if orderId.IsShortTermOrder() { + m.clobKeeper.Logger(ctx).Error( + "RemoveOrderIfFilled: filled Short-Term order ID has no fill amount", + "orderId", + orderId, + ) + } return } From 92dcd43b3377c068f23f4cb649e85be3bd8e68a0 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 25 Oct 2023 08:32:40 -0400 Subject: [PATCH 4/5] Fix compilation error --- protocol/x/clob/keeper/liquidations.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/protocol/x/clob/keeper/liquidations.go b/protocol/x/clob/keeper/liquidations.go index 4fe10fadd8..eb5d4190b8 100644 --- a/protocol/x/clob/keeper/liquidations.go +++ b/protocol/x/clob/keeper/liquidations.go @@ -272,7 +272,7 @@ func (k Keeper) PlacePerpetualLiquidation( liquidationOrder, ) if err != nil { - return err + return 0, 0, err } // TODO(DEC-1323): Potentially allow liquidating the same perpetual + subaccount From 0e2cd1df93cc9a621115bdb04988e9baed824d10 Mon Sep 17 00:00:00 2001 From: Lucas Date: Wed, 25 Oct 2023 13:22:26 -0400 Subject: [PATCH 5/5] Update error log comment --- protocol/x/clob/memclob/memclob.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/protocol/x/clob/memclob/memclob.go b/protocol/x/clob/memclob/memclob.go index 800222a565..ece03e7724 100644 --- a/protocol/x/clob/memclob/memclob.go +++ b/protocol/x/clob/memclob/memclob.go @@ -2005,7 +2005,8 @@ func (m *MemClobPriceTimePriority) RemoveOrderIfFilled( // If there is no fill amount for this order, return early. Note this is only valid if the // order is a stateful order that was fully-filled or partially-filled and expired / canceled / - // removed in the last block. + // removed in the last block. This is because Short-Term orders have their fill amounts + // stored past expiration, so the fill amount should exist in state immediately after being filled. if !exists { if orderId.IsShortTermOrder() { m.clobKeeper.Logger(ctx).Error(