Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial PrepareCheckState audit #695

Merged
merged 5 commits into from
Oct 25, 2023
Merged
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions protocol/x/clob/keeper/liquidations.go
Original file line number Diff line number Diff line change
@@ -271,6 +271,9 @@ func (k Keeper) PlacePerpetualLiquidation(
ctx,
liquidationOrder,
)
if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we were previously ignoring this error and re-assigning it below

return 0, 0, err
}

// TODO(DEC-1323): Potentially allow liquidating the same perpetual + subaccount
// multiple times in a block.
11 changes: 11 additions & 0 deletions protocol/x/clob/keeper/orders.go
Original file line number Diff line number Diff line change
@@ -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 triggered state, since it
// can't have been canceled.
if orderId.IsConditionalOrder() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional orders should always exist in state, so error logging here if they don't exist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also might be good to assert IsConditionalOrderTriggered is True. Since the conditional order has to be triggered at this point in time. It is possible to read out from untriggered state inGetLongTermOrderPlacement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ should definitely assert that the conditional order is triggered

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validation is performed in PlaceConditionalOrdersTriggeredInLastBlock so I'm going to skip this for now if that's fine! Let me know if anyone disagrees.

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
23 changes: 16 additions & 7 deletions protocol/x/clob/memclob/memclob.go
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check was previously the exact same as the above check, which was an oversight

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be necessary since all of these operations should be short-term orders

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,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, return early.
// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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 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. For short-term orders, their fill amounts are stored past expiration,
// so this return is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, lmk what you think!

if !exists {
if orderId.IsShortTermOrder() {
m.clobKeeper.Logger(ctx).Error(
"RemoveOrderIfFilled: filled Short-Term order ID has no fill amount",
"orderId",
orderId,
)
}
return
}