[IND-402] Cache and send order updates for stateful orders. #683
Conversation
WalkthroughThe changes primarily focus on handling stateful order updates in the system. They introduce caching for stateful order updates, add a mechanism to remove old cached order updates, and modify existing handlers and tasks to accommodate these changes. The changes also include additional test cases to verify the new functionality and configuration parameters to control the behavior. Changes
TipsChat with CodeRabbit Bot (
|
IND-402 Fix Stateful order placement with no fill
Stateful orders that are not filled do not get added to the order book. Issue: Potential Solution:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/services/ender/tests/handlers/stateful-order/stateful-order-placement-handler.test.ts (5 hunks)
- indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (2 hunks)
- indexer/services/roundtable/tests/tasks/remove-old-order-updates.test.ts (1 hunks)
- indexer/services/roundtable/src/config.ts (1 hunks)
- indexer/services/roundtable/src/tasks/remove-old-order-updates.ts (1 hunks)
- indexer/services/vulcan/tests/handlers/order-update-handler.test.ts (4 hunks)
- indexer/services/vulcan/src/handlers/order-update-handler.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- indexer/services/roundtable/tests/tasks/remove-old-order-updates.test.ts
- indexer/services/roundtable/src/tasks/remove-old-order-updates.ts
Additional comments (Suppressed): 16
indexer/services/roundtable/src/config.ts (1)
- 123-124: The addition of
OLD_CACHED_ORDER_UPDATES_WINDOW_MSconfiguration parameter is appropriate for managing the time window for removing old cached order updates. Ensure that the default value of 30 seconds is suitable for your application's needs and that it's properly documented for future reference.indexer/services/ender/__tests__/handlers/stateful-order/stateful-order-placement-handler.test.ts (6)
22-22: The import of
OrderUpdateV1is new and seems to be used for handling cached order updates. Ensure that this import is used correctly throughout the code.49-51: New imports from
@dydxprotocol-indexer/redisare introduced. These are used for handling the caching of stateful order updates. Make sure these are used correctly and the necessary dependencies are installed.72-72: The addition of
await redis.deleteAllAsync(redisClient);is to clear the Redis cache after each test. This is a good practice to ensure that each test starts with a clean state.147-172: The test cases have been expanded to include scenarios where there is a cached order update. This is a good addition as it tests the new functionality of handling cached order updates.
178-188: The new code checks if there is a cached order update and if so, it adds it to the
StatefulOrderUpdatesCache. This is a good addition as it tests the new functionality of handling cached order updates.228-245: The new code checks if there was a cached order update and if so, it verifies that the cache is now empty and that the cached order update was sent to the Kafka producer. This is a good addition as it tests the new functionality of handling cached order updates.
indexer/services/ender/src/handlers/stateful-order/stateful-order-placement-handler.ts (2)
9-9: The
StatefulOrderUpdatesCachefrom@dydxprotocol-indexer/redisis imported. This is a new addition to the code and is used to store and retrieve cached order updates for stateful orders.66-94: The logic for handling stateful order placement events has been updated. After generating the off-chain update for the order placement status, the code now retrieves any pending order update from the
StatefulOrderUpdatesCacheand generates an off-chain update for it. This ensures that any order updates that were received before the order placement are included in the Kafka events. This is a significant change and should be thoroughly tested to ensure it works as expected.- return [ - this.generateConsolidatedVulcanKafkaEvent( - getOrderIdHash(order.orderId!), - offChainUpdate, - ), - ]; + const kafakEvents: ConsolidatedKafkaEvent[] = []; + kafakEvents.push(this.generateConsolidatedVulcanKafkaEvent( + getOrderIdHash(order.orderId!), + offChainUpdate, + )); + + const pendingOrderUpdate: OrderUpdateV1 | undefined = await StatefulOrderUpdatesCache + .removeStatefulOrderUpdate( + OrderTable.orderIdToUuid(order.orderId!), + Date.now(), + redisClient, + ); + if (pendingOrderUpdate !== undefined) { + kafakEvents.push(this.generateConsolidatedVulcanKafkaEvent( + getOrderIdHash(order.orderId!), + OffChainUpdateV1.fromPartial({ + orderUpdate: pendingOrderUpdate, + }), + )); + } + + return kafakEvents;indexer/services/vulcan/__tests__/handlers/order-update-handler.test.ts (4)
7-7: The
StatefulOrderUpdatesCachemodule is imported. This module is used to cache order updates for stateful orders.37-37: The
OrderUpdateV1type is imported. This type is used to represent order updates in the new test case.469-469: The test case description has been updated to specify that it applies to short-term orders. This change reflects the new logic in the
OrderUpdateHandlermodule, which handles short-term orders and stateful orders differently.489-521: A new test case has been added to verify the behavior when an order update is received for a stateful order that does not exist in the order cache. The test case checks that the order update is added to the
StatefulOrderUpdatesCache, and that no other actions are taken (such as updating theOrderbookLevelsCacheor sending WebSocket messages). The test case also checks that an informational log message is generated, and that a metric is incremented with the order flags of the order update.+ it('adds order update to stateful order update cache if stateful order not found', async () => { + synchronizeWrapBackgroundTask(wrapBackgroundTask); + const producerSendSpy: jest.SpyInstance = jest.spyOn(producer, 'send').mockReturnThis(); + const statefulOrderUpdate: redisTestConstants.OffChainUpdateOrderUpdateUpdateMessage = { + ...redisTestConstants.orderUpdate, + orderUpdate: { + ...redisTestConstants.orderUpdate.orderUpdate, + orderId: redisTestConstants.defaultOrderIdGoodTilBlockTime, + }, + }; + await handleOrderUpdate(statefulOrderUpdate); + + const cachedOrderUpdate: OrderUpdateV1 | undefined = await StatefulOrderUpdatesCache + .removeStatefulOrderUpdate( + redisTestConstants.defaultOrderUuidGoodTilBlockTime, + Date.now(), + client, + ); + expect(cachedOrderUpdate).toBeDefined(); + expect(cachedOrderUpdate).toEqual(statefulOrderUpdate.orderUpdate); + + expect(OrderbookLevelsCache.updatePriceLevel).not.toHaveBeenCalled(); + expect(logger.info).toHaveBeenCalledWith(expect.objectContaining({ + at: 'OrderUpdateHandler#handle', + message: expect.stringMatching('Received order update for order that does not exist, order id '), + })); + expectWebsocketMessagesNotSent(producerSendSpy); + expect(stats.increment).toHaveBeenCalledWith( + 'vulcan.order_update_order_does_not_exist', + 1, + { + orderFlags: String(statefulOrderUpdate.orderUpdate.orderId!.orderFlags), + }, + ); + });indexer/services/vulcan/src/handlers/order-update-handler.ts (3)
11-18: The new hunk imports
OrderTableandStatefulOrderUpdatesCachefrom@dydxprotocol-indexer/postgresand@dydxprotocol-indexer/redisrespectively. It also importsisStatefulOrderfrom@dydxprotocol-indexer/v4-proto-parser. These new imports are used in the updated logic for handling order updates.80-108: The new hunk adds a check to see if the order update was for a stateful order when the order does not exist in the cache. If it is a stateful order, the order update is added to a cache of order updates for stateful orders. This allows the order update to be re-sent after the on-chain event for the stateful order placement is processed. The order id is converted to a UUID before being added to the cache. The current time is used as the timestamp for the cache entry. The Redis client is passed to the function that adds the order update to the cache. The order flags are logged and used as a tag for a stats counter increment.
80-108: Ensure that the
StatefulOrderUpdatesCache.addStatefulOrderUpdatefunction properly handles any errors that may occur during the operation. This is important to prevent the application from crashing or entering an inconsistent state due to an unhandled exception.
| /** | ||
| * This task removes any old cached stateful orer updates from the StatefulOrderUpdates cache | ||
| */ | ||
| export default async function runTask(): Promise<void> { |
There was a problem hiding this comment.
are you going to create a followup PR for index.ts changes?
There was a problem hiding this comment.
Good point, I missed adding the code to run the task. Will add.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/roundtable/src/index.ts
Additional comments (Suppressed): 3
indexer/services/roundtable/src/config.ts (3)
44-44: The new configuration parameter
LOOPS_ENABLED_REMOVE_OLD_ORDER_UPDATESis introduced to control whether the task for removing old cached order updates is enabled or not. Ensure that this configuration is properly documented and the implications of enabling/disabling this task are clearly communicated to the users.74-76: The new configuration parameter
LOOPS_INTERVAL_MS_REMOVE_OLD_ORDER_UPDATESis introduced to control the interval at which the task for removing old cached order updates is run. Ensure that this interval is set to a reasonable value that balances the need for timely removal of old cached order updates and the potential impact on system performance.126-128: The new configuration parameter
OLD_CACHED_ORDER_UPDATES_WINDOW_MSis introduced to control the time window for removing old cached order updates. Ensure that this time window is set to a reasonable value that balances the need for timely removal of old cached order updates and the potential impact on system performance.
vincentwschau
deleted the
vincentc/ind-402-send-cached-stateful-order-updates
branch
Changelist
Fixes bug where stateful orders that are not filled are not updated with the total filled amount after placing.
The bug occurs as the
OrderUpdatefor stateful orders are sent before theOrderPlacemessage is sent. As such theOrderUpdateis discarded.The fix proposed in this PR is to cached the
OrderUpdateand re-send it when theOrderPlaceis received for the stateful order.Use the added
StatefulOrderUpdatesCacheto store and send cached order updates for stateful orders.Updates the
OrderUpdateHandlerinvulcanto cache order updates for stateful orders if the order update does not match any order in the order cache.Updates the handler for stateful order placements in
enderto re-send any cached order updates for the stateful order after sending theOrderPlacemessage.Updates
roundtableto add a task to remove any old cachedOrderUpdates.Will not be merged until the base PR is merged.
Test Plan
Added unit tests.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.