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

[CT-875] Simplify indexer messages for short term order replacement #1593

Merged
merged 18 commits into from
Jun 3, 2024
Merged
Changes from 8 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
14 changes: 0 additions & 14 deletions indexer/services/vulcan/src/handlers/order-place-handler.ts
Original file line number Diff line number Diff line change
@@ -157,20 +157,6 @@ export class OrderPlaceHandler extends Handler {
};
sendMessageWrapper(subaccountMessage, KafkaTopics.TO_WEBSOCKETS_SUBACCOUNTS);
}

// TODO(IND-68): Remove once order replacement flow in V4 protocol removes the old order and
// places the updated order.
if (updatedQuantums !== undefined) {
const orderbookMessage: Message = {
value: this.createOrderbookWebsocketMessage(
placeOrderResult.oldOrder!,
perpetualMarket,
updatedQuantums,
),
headers,
};
sendMessageWrapper(orderbookMessage, KafkaTopics.TO_WEBSOCKETS_ORDERBOOKS);
}
Copy link
Contributor Author

@chenyaoy chenyaoy May 30, 2024

Choose a reason for hiding this comment

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

Removing because this is sending a websocket message to remove the unfilled amount of the original order from the orderbook price level. This is not expected to be called currently (before my changes) because we expect order removal to always happen before order place so the updatedQuantums should always be undefined (because an existing order in the Orders Cache should have already been removed at this point)

On order replacement there are 2 cases:

  1. If the price is different, we expect an order removal message first so the original order shouldn't exist at this point, so we don't need this here
  2. When the price is the same, we shouldn't remove the unfilled amount of the original order from the orderbook and then re-add the replacement order size to the orderbook via the Order Update message because this is 2 websocket messages and will cause flickering. We should just wait for the update message we expect after this order place to update the orderbook price-level size

I am concerned that the data sent through socks will not match the data in OpenOrdersCache and OrderbookLevelsCache for the short amount of time between this handler and when the Order Update Handler finishes running, I'm not sure if this is okay or not.

}

/**
33 changes: 19 additions & 14 deletions protocol/x/clob/memclob/memclob.go
Original file line number Diff line number Diff line change
@@ -495,23 +495,28 @@ func (m *MemClobPriceTimePriority) PlaceOrder(
}

if m.generateOffchainUpdates {
// If this is a replacement order, then ensure we send the appropriate replacement message.
// Send an order place message.
// If existing order ID is found and the price of the existing order is different from the new order,
// create an order removal message first so we can remove the original price level from the orderbook.
orderId := order.OrderId
if _, found := orderbook.getOrder(orderId); found {
if message, success := off_chain_updates.CreateOrderReplaceMessage(
ctx,
order,
); success {
offchainUpdates.AddReplaceMessage(orderId, message)
}
} else {
if message, success := off_chain_updates.CreateOrderPlaceMessage(
ctx,
order,
); success {
offchainUpdates.AddPlaceMessage(order.OrderId, message)
if existingOrder, found := orderbook.getOrder(orderId); found {
if order.Subticks != existingOrder.Subticks {
if message, success := off_chain_updates.CreateOrderRemoveMessageWithReason(
ctx,
orderId,
indexersharedtypes.OrderRemovalReason_ORDER_REMOVAL_REASON_REPLACED,
ocutypes.OrderRemoveV1_ORDER_REMOVAL_STATUS_BEST_EFFORT_CANCELED,
); success {
offchainUpdates.AddRemoveMessage(orderId, message)
}
}
}
if message, success := off_chain_updates.CreateOrderPlaceMessage(
ctx,
order,
); success {
offchainUpdates.AddPlaceMessage(order.OrderId, message)
}
}

// Attempt to match the order against the orderbook.
Original file line number Diff line number Diff line change
@@ -1385,6 +1385,7 @@ func TestPlaceOrder_ReduceOnly(t *testing.T) {
tc.expectedCancelledReduceOnlyOrders,
// TODO(IND-261): Add tests for replaced reduce-only orders.
false,
false,
)
})
}
@@ -1668,6 +1669,7 @@ func TestPlaceOrder_LongTermReduceOnlyRemovals(t *testing.T) {
tc.expectedNewMatches,
tc.expectedCancelledReduceOnlyOrders,
false,
false,
)
})
}
12 changes: 10 additions & 2 deletions protocol/x/clob/memclob/memclob_place_order_test.go
Original file line number Diff line number Diff line change
@@ -349,6 +349,7 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) {
ordersOnBook = append(ordersOnBook, &order)
}

expectedReplacementOrderPriceChanged := false
for _, matchableOrder := range ordersOnBook {
// Note we assume these are regular orders since liquidation orders cannot rest on
// the book.
@@ -357,6 +358,9 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) {
// is no longer on the book.
matchableOrderOrder := matchableOrder.MustGetOrder()
if matchableOrderOrder.OrderId == tc.order.OrderId && tc.order.MustCmpReplacementOrder(&matchableOrderOrder) > 0 {
if matchableOrderOrder.Subticks != tc.order.Subticks {
expectedReplacementOrderPriceChanged = true
}
continue
}

@@ -408,6 +412,7 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) {
[]expectedMatch{},
[]types.OrderId{},
tc.expectedToReplaceOrder,
expectedReplacementOrderPriceChanged,
)
})
}
@@ -2245,8 +2250,7 @@ func TestPlaceOrder_MatchOrders_PreexistingMatches(t *testing.T) {

expectedOrderStatus: types.InternalError,
expectedToReplaceOrder: true,

expectedErr: types.ErrFokOrderCouldNotBeFullyFilled,
expectedErr: types.ErrFokOrderCouldNotBeFullyFilled,
},
}

@@ -2305,6 +2309,7 @@ func TestPlaceOrder_MatchOrders_PreexistingMatches(t *testing.T) {
tc.expectedNewMatches,
[]types.OrderId{},
tc.expectedToReplaceOrder,
false,
)
})
}
@@ -3281,6 +3286,7 @@ func TestPlaceOrder_PostOnly(t *testing.T) {
[]expectedMatch{},
[]types.OrderId{},
false,
false,
)
})
}
@@ -3424,6 +3430,7 @@ func TestPlaceOrder_ImmediateOrCancel(t *testing.T) {
tc.expectedCollatCheck,
[]types.OrderId{},
false,
false,
)
})
}
@@ -4098,6 +4105,7 @@ func TestPlaceOrder_FillOrKill(t *testing.T) {
expectedMatches,
[]types.OrderId{},
false,
false,
)
})
}
36 changes: 25 additions & 11 deletions protocol/x/clob/memclob/memclob_test_util.go
Original file line number Diff line number Diff line change
@@ -1312,30 +1312,44 @@ func assertPlaceOrderOffchainMessages(
expectedNewMatches []expectedMatch,
expectedCancelledReduceOnlyOrders []types.OrderId,
expectedToReplaceOrder bool,
expectedReplacementOrderPriceChanged bool,
) {
actualOffchainMessages := offchainUpdates.GetMessages()
expectedOffchainMessages := []msgsender.Message{}
seenCancelledReduceOnlyOrders := mapset.NewSet[types.OrderId]()

// If there are no errors expected, an order place message should be sent.
if expectedErr == nil || doesErrorProduceOffchainMessages(expectedErr) {
var updateMessage msgsender.Message
if expectedToReplaceOrder {
updateMessage = off_chain_updates.MustCreateOrderReplaceMessage(
ctx,
order,
)
if expectedReplacementOrderPriceChanged {
removeMessage := off_chain_updates.MustCreateOrderRemoveMessageWithReason(
ctx,
order.OrderId,
indexershared.OrderRemovalReason_ORDER_REMOVAL_REASON_REPLACED,
ocutypes.OrderRemoveV1_ORDER_REMOVAL_STATUS_BEST_EFFORT_CANCELED,
)
placeMessage := off_chain_updates.MustCreateOrderPlaceMessage(
ctx,
order,
)
expectedOffchainMessages = append(
expectedOffchainMessages,
removeMessage,
placeMessage,
)
require.Equal(t, expectedOffchainMessages, actualOffchainMessages[:len(expectedOffchainMessages)])
}
} else {
updateMessage = off_chain_updates.MustCreateOrderPlaceMessage(
placeMessage := off_chain_updates.MustCreateOrderPlaceMessage(
ctx,
order,
)
expectedOffchainMessages = append(
expectedOffchainMessages,
placeMessage,
)
require.Equal(t, expectedOffchainMessages, actualOffchainMessages[:len(expectedOffchainMessages)])
}
expectedOffchainMessages = append(
expectedOffchainMessages,
updateMessage,
)
require.Equal(t, expectedOffchainMessages, actualOffchainMessages[:len(expectedOffchainMessages)])
}

// Reduce-only order removals are sent before updates if the maker orders are removed during