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
Show file tree
Hide file tree
Changes from 12 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
Original file line number Diff line number Diff line change
Expand Up @@ -641,12 +641,6 @@ describe('order-place-handler', () => {
testConstants.defaultPerpetualMarket,
APIOrderStatusEnum.BEST_EFFORT_OPENED,
expectSubaccountMessage,
expectOrderBookUpdate
? OrderbookMessage.fromPartial({
contents: JSON.stringify(orderbookContents),
clobPairId: testConstants.defaultPerpetualMarket.clobPairId,
version: ORDERBOOKS_WEBSOCKET_MESSAGE_VERSION,
}) : undefined,
);
expectStats(true);
});
Expand Down
16 changes: 1 addition & 15 deletions indexer/services/vulcan/src/handlers/order-place-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class OrderPlaceHandler extends Handler {

// TODO(IND-68): Remove once order replacement flow in V4 protocol removes the old order and
// places the updated order.
const updatedQuantums: number | undefined = await this.updatePriceLevel(
await this.updatePriceLevel(
placeOrderResult,
perpetualMarket,
update,
Expand Down Expand Up @@ -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.

}

/**
Expand Down
33 changes: 19 additions & 14 deletions protocol/x/clob/memclob/memclob.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1385,6 +1385,7 @@ func TestPlaceOrder_ReduceOnly(t *testing.T) {
tc.expectedCancelledReduceOnlyOrders,
// TODO(IND-261): Add tests for replaced reduce-only orders.
false,
false,
)
})
}
Expand Down Expand Up @@ -1668,6 +1669,7 @@ func TestPlaceOrder_LongTermReduceOnlyRemovals(t *testing.T) {
tc.expectedNewMatches,
tc.expectedCancelledReduceOnlyOrders,
false,
false,
)
})
}
Expand Down
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
Expand Up @@ -319,6 +319,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.
Expand All @@ -327,6 +328,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
}

Expand Down Expand Up @@ -378,6 +382,7 @@ func TestPlaceOrder_AddOrderToOrderbook(t *testing.T) {
[]expectedMatch{},
[]types.OrderId{},
tc.expectedToReplaceOrder,
expectedReplacementOrderPriceChanged,
)
})
}
Expand Down Expand Up @@ -2215,8 +2220,7 @@ func TestPlaceOrder_MatchOrders_PreexistingMatches(t *testing.T) {

expectedOrderStatus: types.InternalError,
expectedToReplaceOrder: true,

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

Expand Down Expand Up @@ -2275,6 +2279,7 @@ func TestPlaceOrder_MatchOrders_PreexistingMatches(t *testing.T) {
tc.expectedNewMatches,
[]types.OrderId{},
tc.expectedToReplaceOrder,
false,
)
})
}
Expand Down Expand Up @@ -3246,6 +3251,7 @@ func TestPlaceOrder_PostOnly(t *testing.T) {
[]expectedMatch{},
[]types.OrderId{},
false,
false,
)
})
}
Expand Down Expand Up @@ -3389,6 +3395,7 @@ func TestPlaceOrder_ImmediateOrCancel(t *testing.T) {
tc.expectedCollatCheck,
[]types.OrderId{},
false,
false,
)
})
}
Expand Down Expand Up @@ -4063,6 +4070,7 @@ func TestPlaceOrder_FillOrKill(t *testing.T) {
expectedMatches,
[]types.OrderId{},
false,
false,
)
})
}
Expand Down
29 changes: 18 additions & 11 deletions protocol/x/clob/memclob/memclob_test_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -1292,28 +1292,35 @@ 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,
)
} else {
updateMessage = off_chain_updates.MustCreateOrderPlaceMessage(
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,
)
expectedOffchainMessages = append(
expectedOffchainMessages,
removeMessage,
)
}
}
placeMessage := off_chain_updates.MustCreateOrderPlaceMessage(
ctx,
order,
)
expectedOffchainMessages = append(
expectedOffchainMessages,
updateMessage,
placeMessage,
)
require.Equal(t, expectedOffchainMessages, actualOffchainMessages[:len(expectedOffchainMessages)])
}
Expand Down
Loading