[IND-450]: Prevent orders transitioning from canceled to best effort canceled #753
Conversation
IND-450 BUG: Orders can transition from CANCELED -> BEST_EFFORT_CANCELED
This can happen in this case: More context on specific orders in testnet-4 where this happened: https://dydx-team.slack.com/archives/C05S4CTSQ74/p1697638332790579 |
WalkthroughThe changes primarily revolve around the handling of canceled orders in the codebase. A new enum Changes
TipsChat with CodeRabbit Bot (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- indexer/packages/redis/tests/caches/canceled-orders-cache.test.ts (3 hunks)
- indexer/packages/redis/src/caches/canceled-orders-cache.ts (4 hunks)
- indexer/packages/redis/src/types.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
- indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (5 hunks)
- indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (2 hunks)
- indexer/services/ender/src/handlers/order-fills/order-handler.ts (4 hunks)
- indexer/services/ender/src/scripts/dydx_get_order_status.sql (1 hunks)
- indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (4 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (4 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (17 hunks)
- indexer/services/vulcan/tests/helpers/helpers.ts (2 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (1 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (2 hunks)
Files skipped from review due to trivial changes (4)
- indexer/packages/redis/src/types.ts
- indexer/services/ender/src/scripts/dydx_get_order_status.sql
- indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts
Additional comments: 31
indexer/services/ender/src/handlers/order-fills/liquidation-handler.ts (2)
16-19: The import statement for
CanceledOrderStatusis added correctly.210-214: The
upsertOrderFromEventmethod is updated to use theCanceledOrderStatus.NOT_CANCELEDenum value instead offalse. This change is consistent with the PR description and should work as expected, assuming that theupsertOrderFromEventmethod has been updated to handle the new enum type.indexer/packages/redis/__tests__/caches/canceled-orders-cache.test.ts (3)
4-10: The import statements have been updated to reflect the changes in the
canceled-orders-cachemodule. The functionremoveOrderFromCachehas been renamed toremoveOrderFromCaches, and the new functionsgetOrderCanceledStatusandaddBestEffortCanceledOrderIdhave been added. The import statement forCanceledOrderStatushas also been added.43-52: The test case 'successfully removes canceled order' has been updated to use the new function
removeOrderFromCaches. Ensure that the function behaves as expected and the test case passes.67-83: New test cases have been added to test the
getOrderCanceledStatusfunction. These test cases check if the function correctly returnsCANCELED,BEST_EFFORT_CANCELED, andNOT_CANCELEDstatuses. Ensure that these test cases cover all possible scenarios and pass.indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (4)
27-30: The new import statement for
CanceledOrderStatusfrom@dydxprotocol-indexer/redisis introduced. Ensure that theCanceledOrderStatusenum is correctly defined in the@dydxprotocol-indexer/redismodule.293-304: The
upsertOrderFromEventmethod's signature is modified to replace theisCanceledparameter withcanceledOrderStatusof typeCanceledOrderStatus. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.310-316: The
getOrderStatusmethod's signature is modified to replace theisCanceledparameter withcanceledOrderStatusof typeCanceledOrderStatus. The logic within thegetOrderStatusmethod is also updated to use thecanceledOrderStatusparameter instead of theisCanceledparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.367-383: The logic within the
getOrderStatusmethod is updated to handle the newCanceledOrderStatusenum. Ensure that the logic correctly handles all possible values ofCanceledOrderStatus.indexer/services/vulcan/__tests__/helpers/helpers.ts (2)
5-11: The new import
CanceledOrderStatusis used in the new functionexpectCanceledOrderStatus. Ensure that this enum is correctly defined and exported in the@dydxprotocol-indexer/redismodule.84-91: The new function
expectCanceledOrderStatusretrieves the canceled order status for a given order ID from theCanceledOrdersCacheand checks if it matches the expected status. This function will be useful in unit tests to verify the correct status of canceled orders.+ export async function expectCanceledOrderStatus( + orderId: string, + canceledOrderStatus: CanceledOrderStatus, + ) { + expect(await CanceledOrdersCache.getOrderCanceledStatus(orderId, redisClient)).toEqual( + canceledOrderStatus, + ); + }indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
- 364-370: The method
removeOrderFromCanceledOrdersCachehas been renamed toremoveOrderFromCaches. Ensure that all calls to this method throughout the codebase have been updated to match the new name.- protected async removeOrderFromCanceledOrdersCache( + protected async removeOrderFromCaches(indexer/packages/redis/src/caches/canceled-orders-cache.ts (7)
1-11: The introduction of a new cache key
BEST_EFFORT_CANCELED_ORDERS_CACHE_KEYand the new enumCanceledOrderStatusis a good approach to handle different cancellation statuses. This change is consistent with the PR summary.26-37: The
isOrderCanceledfunction now checks bothCANCELED_ORDERS_CACHE_KEYandBEST_EFFORT_CANCELED_ORDERS_CACHE_KEYto determine if an order is canceled. This change is consistent with the PR summary.39-60: The new function
getOrderCanceledStatusreturns the cancellation status of an order based on the two cache keys. This function is a good addition as it provides more detailed information about the cancellation status of an order.62-70: The function
removeOrderFromCachesnow removes the order from both cache keys. This change is consistent with the PR summary.79-85: The new function
addBestEffortCanceledOrderIdadds the order ID to theBEST_EFFORT_CANCELED_ORDERS_CACHE_KEY. This function is a good addition as it provides a way to add an order to the best effort canceled orders cache.95-100: The function
addCanceledOrderIdnow uses the new functionaddOrderIdtoCacheto add the order ID to theCANCELED_ORDERS_CACHE_KEY. This change is consistent with the PR summary.95-116: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [109-139]
The new function
addOrderIdtoCacheadds the order ID to the specified cache key. This function is a good addition as it provides a way to add an order to a specified cache key.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (2)
1475-1513: The test cases are well-structured and cover a variety of scenarios. This is a good practice as it ensures that the code is robust and can handle different situations.
1563-1567: The code correctly checks the
isBestEffortCanceledflag and calls the appropriate function based on its value. This is a good practice as it ensures that the correct function is called based on the order status.indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts (3)
42-42: The import of
CanceledOrderStatusis added to reflect the changes in the codebase where theisOrderCanceledvariable and function are replaced withcanceledOrderStatusandgetOrderCanceledStatusrespectively.62-62: The function
expectCanceledOrdersCacheFoundis replaced withexpectCanceledOrderStatusto align with the changes in the codebase.1875-1875: The expected order status in these test cases is changed to reflect the new
CanceledOrderStatusenum values. This is a necessary change to ensure the tests are aligned with the updated codebase.indexer/services/vulcan/src/handlers/order-remove-handler.ts (2)
300-306: The
addOrderToCanceledOrdersCachemethod now accepts anorderRemoveparameter of typeOrderRemoveV1instead oforderIdas a string. It checks theremovalStatusof theorderRemoveobject and calls different methods from theCanceledOrdersCacheclass based on the status. This change allows for handling different removal statuses of orders in the cache.338-363: The
addOrderToCanceledOrdersCachemethod now checks theremovalStatusof theorderRemoveobject and calls different methods from theCanceledOrdersCacheclass based on the status. This change allows for handling different removal statuses of orders in the cache.- protected async addOrderToCanceledOrdersCache( - orderId: string, - blockHeight: number, - ): Promise<void> { - await runFuncWithTimingStat( - CanceledOrdersCache.addCanceledOrderId(orderId, blockHeight, redisClient), - this.generateTimingStatsOptions('add_order_to_canceled_order_cache'), - ); - } + protected async addOrderToCanceledOrdersCache( + orderRemove: OrderRemoveV1, + timestampMs: number, + ): Promise<void> { + const orderId: string = OrderTable.orderIdToUuid(orderRemove.removedOrderId!); + + if ( + orderRemove.removalStatus === + OrderRemoveV1_OrderRemovalStatus.ORDER_REMOVAL_STATUS_BEST_EFFORT_CANCELED + ) { + await runFuncWithTimingStat( + CanceledOrdersCache.addBestEffortCanceledOrderId(orderId, timestampMs, redisClient), + this.generateTimingStatsOptions('add_order_to_canceled_order_cache'), + ); + } else if ( + orderRemove.removalStatus === + OrderRemoveV1_OrderRemovalStatus.ORDER_REMOVAL_STATUS_CANCELED + ) { + await runFuncWithTimingStat( + CanceledOrdersCache.addCanceledOrderId(orderId, timestampMs, redisClient), + this.generateTimingStatsOptions('add_order_to_canceled_order_cache'), + ); + } + }indexer/services/ender/src/handlers/order-fills/order-handler.ts (5)
16-22: The import statement has been updated to include
CanceledOrderStatusfrom the@dydxprotocol-indexer/redismodule. Ensure that the module is correctly installed and imported.75-82: The
getOrderCanceledStatusfunction is being called withorderUuidandredisClientas arguments. Ensure thatorderUuidandredisClientare correctly initialized and have the expected values.93-97: The
dydx_order_fill_handler_per_orderfunction is being called withcanceledOrderStatusas an argument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.181-188: The
getOrderCanceledStatusfunction is being called withorderUuidandredisClientas arguments. Ensure thatorderUuidandredisClientare correctly initialized and have the expected values.192-197: The
upsertOrderFromEventfunction is being called withcanceledOrderStatusas an argument. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (2 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/redis/src/caches/canceled-orders-cache.ts (4 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
Additional comments: 11
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (4)
710-714: The ternary operator is used to set the status of the order. Ensure that the
isOrderCanceledvariable is correctly set before this point.1483-1507: The test cases are well-structured and cover different scenarios. However, ensure that the expected outcomes are correctly defined for each case.
1557-1561: The code correctly checks the status of the order and calls the appropriate function from the
CanceledOrdersCacheclass. Ensure that these functions are implemented correctly and handle all edge cases.1610-1613: The test assertions are correct. Ensure that the
makerOrderandtakerOrderobjects are correctly set before this point.indexer/packages/redis/src/caches/canceled-orders-cache.ts (7)
1-11: The introduction of a new cache key
BEST_EFFORT_CANCELED_ORDERS_CACHE_KEYand the enumCanceledOrderStatusis a good approach to handle different cancellation statuses. This will improve the readability and maintainability of the code.26-37: The function
isOrderCancelednow checks both theCANCELED_ORDERS_CACHE_KEYand theBEST_EFFORT_CANCELED_ORDERS_CACHE_KEYfor the order's cancellation status. This is a good approach to handle different cancellation statuses.39-60: The function
getOrderCanceledStatusis a good addition. It returns the cancellation status of an order based on the two cache keys. This will improve the readability and maintainability of the code.62-70: The function
removeOrderFromCachesnow removes the order from both cache keys. This is a good approach to handle different cancellation statuses.79-85: The function
addBestEffortCanceledOrderIdis a good addition. It specifically adds the order to theBEST_EFFORT_CANCELED_ORDERS_CACHE_KEY. This will improve the readability and maintainability of the code.95-100: The function
addCanceledOrderIdnow callsaddOrderIdtoCachewith theCANCELED_ORDERS_CACHE_KEY. This is a good approach to handle different cancellation statuses.95-116: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [109-139]
The function
addOrderIdtoCachenow accepts acacheKeyparameter to specify the cache key to add the order to. This is a good approach to handle different cancellation statuses.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
Additional comments: 4
indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (4)
711-711: The change from
OrderStatus.BEST_EFFORT_CANCELEDtoOrderStatus.CANCELEDis noted. Ensure that this change is reflected everywhere in the codebase whereOrderStatus.BEST_EFFORT_CANCELEDwas previously used.1483-1514: The addition of new test cases for "post-only canceled" orders is a good practice. It helps to ensure that the system behaves as expected under different conditions.
1557-1561: The conditional handling of
OrderStatus.BEST_EFFORT_CANCELEDandOrderStatus.CANCELEDis clear and straightforward. However, ensure that theaddBestEffortCanceledOrderIdandaddCanceledOrderIdmethods handle any potential errors and exceptions.1610-1610: The test case checks if the status of the maker order is equal to the expected status. This is a good practice as it ensures that the status of the order is updated correctly.
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
…canceled (#753) * [IND-450]: Prevent orders transitioning from canceled to best effort canceled * fix liquidations * nits * lint
Changelist
BEST_EFFORT_CANCELEDandCANCELEDremove-order-handler.tsto update CanceledOrdersCache with statusabstract-order-fill-handler.tsto update short term GTT and short term post-only orders to the correct statusabstract-order-fill-handler.tsto never set stateful orders toBEST_EFFORT_CANCELEDbecause stateful orders are never added to the CanceledOrdersCache in vulcan, and because we never sendBEST_EFFORT_CANCELEDfor stateful ordersTest Plan
Tested in unit tests, verified previous behavior in dev
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.