[IND-482]: Send timing fields for orders when possible #768
Conversation
IND-482 Stateful orders are not sending timing fields
When stateful orders are placed and canceled, they send the event to vulcan without any of the timing fields such as |
WalkthroughThe changes primarily revolve around the addition of a new optional field 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 (7)
- indexer/packages/postgres/tests/helpers/constants.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/order-types.ts (1 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (3 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (9 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (3 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (5 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/postgres/src/types/db-model-types.ts
Additional comments: 20
indexer/packages/postgres/__tests__/helpers/constants.ts (1)
- 226-231: The addition of
createdAtHeightfield todefaultOrderGoodTilBlockTimeobject is consistent with the PR description. Ensure that this field is being used correctly in the rest of the codebase.indexer/packages/postgres/src/types/order-types.ts (1)
- 62-68: The addition of the optional
createdAtHeightfield to theOrderCreateObjectinterface is consistent with the PR description. Ensure that all uses of this interface throughout the codebase have been updated to handle this new optional field.indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (3)
64-64: The import of
isStatefulOrderfunction is correct and necessary for the changes in this PR.1080-1080: The
isStatefulOrderfunction is correctly used to determine if an order is stateful.1107-1109: The conditional inclusion of
createdAtHeight,updatedAt, andupdatedAtHeightbased on theisStatefulflag is a good practice to avoid unnecessary data in non-stateful orders.indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts (9)
343-345: The addition of
createdAtHeight,updatedAt, andupdatedAtHeightfields in the test suite is consistent with the changes described in the pull request summary. Ensure that these fields are being correctly populated and used in theOrderRemoveHandlerclass.479-481: Same as above.
616-618: Same as above.
754-756: Same as above.
1036-1038: Same as above.
1150-1152: Same as above.
1271-1273: Same as above.
1408-1410: Same as above.
1540-1542: Same as above.
indexer/services/vulcan/src/handlers/order-remove-handler.ts (4)
13-17: The import of
IsoStringhas been added. Ensure that it is used correctly in the code.272-286: The
createdAtHeight,updatedAt, andupdatedAtHeightfields have been added to thecanceledOrderobject. Ensure that these fields are being populated correctly and that they are being used as intended in the rest of the code.486-502: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [486-534]
The
createdAtHeight,updatedAt, andupdatedAtHeightfields have been added to thecontentsobject. Ensure that these fields are being populated correctly and that they are being used as intended in the rest of the code.
- 572-580: The
createdAtHeight,updatedAt, andupdatedAtHeightfields have been added to thecontentsobject. Ensure that these fields are being populated correctly and that they are being used as intended in the rest of the code.indexer/services/vulcan/src/handlers/order-place-handler.ts (2)
16-20: The import of
IsoStringis new and is used later in the code for type annotation. Ensure that this import is necessary and correctly used.281-288: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [281-315]
The
createdAtHeight,updatedAt, andupdatedAtHeightfields are conditionally added to thecontentsobject. This is done correctly, but ensure that the rest of the codebase can handle these fields being undefined.+ const createdAtHeight: string | undefined = order?.createdAtHeight; + const updatedAt: IsoString | undefined = order?.updatedAt; + const updatedAtHeight: string | undefined = order?.updatedAtHeight; + const contents: SubaccountMessageContents = { + orders: [ + { + id: OrderTable.orderIdToUuid(redisOrder.order!.orderId!), + subaccountId: SubaccountTable.subaccountIdToUuid( + redisOrder.order!.orderId!.subaccountId!, + ), + clientId: redisOrder.order!.orderId!.clientId.toString(), + clobPairId: perpetualMarket.clobPairId, + side: protocolTranslations.protocolOrderSideToOrderSide(redisOrder.order!.side), + size: redisOrder.size, + price: redisOrder.price, + status, + type: protocolTranslations.protocolConditionTypeToOrderType( + redisOrder.order!.conditionType, + ), + timeInForce: apiTranslations.orderTIFToAPITIF(orderTIF), + postOnly: apiTranslations.isOrderTIFPostOnly(orderTIF), + reduceOnly: redisOrder.order!.reduceOnly, + orderFlags: redisOrder.order!.orderId!.orderFlags.toString(), + goodTilBlock: protocolTranslations.getGoodTilBlock(redisOrder.order!) + ?.toString(), + goodTilBlockTime: protocolTranslations.getGoodTilBlockTime(redisOrder.order!), + ticker: redisOrder.ticker, + ...(createdAtHeight && { createdAtHeight }), + ...(updatedAt && { updatedAt }), + ...(updatedAtHeight && { updatedAtHeight }), + clientMetadata: redisOrder.order!.clientMetadata.toString(), + triggerPrice: getTriggerPrice(redisOrder.order!, perpetualMarket), + }, + ], + };
Christopher-Li
force-pushed
the
cl_order_timing
branch
from
44026c4 to
8720e04
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/order-types.ts (1 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (3 hunks)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (9 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (3 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (5 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/postgres/src/types/db-model-types.ts
- indexer/packages/postgres/src/types/order-types.ts
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts
Additional comments: 8
indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
- 281-288: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [281-315]
The code correctly handles the optional fields
createdAtHeight,updatedAt, andupdatedAtHeight. It only includes these fields in thecontentsobject if they are defined.indexer/packages/postgres/__tests__/helpers/constants.ts (1)
- 226-231: The addition of
createdAtHeighttodefaultOrderGoodTilBlockTimeis consistent with the pull request summary. Ensure that this new field is handled correctly in all places where this object is used.indexer/services/vulcan/src/handlers/order-remove-handler.ts (3)
13-16: The addition of
IsoStringimport is fine as it is used later in the code.277-291: The introduction of
canceledOrdervariable to store the result of a database query is a good practice. It helps in reducing the number of database calls if the same data is required multiple times.513-529: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [513-561]
The utilization of
canceledOrderin thecreateSubaccountWebsocketMessageFromRemoveOrderResultmethod to extractcreatedAtHeight,updatedAt, andupdatedAtHeightvalues is correct. It helps in providing more detailed information in the websocket message.indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (3)
65-65: The import of
isStatefulOrderfunction is correct and necessary for the changes in this pull request.1081-1081: The
isStatefulvariable is correctly defined using theisStatefulOrderfunction.1108-1110: The conditional inclusion of
createdAtHeight,updatedAt, andupdatedAtHeightis correctly implemented. Ensure thatdbOrderhas these properties and they are of the correct type.
| } else { | ||
| await runFuncWithTimingStat( | ||
| OrderTable.findById(OrderTable.orderIdToUuid(orderRemove.removedOrderId!)), | ||
| this.generateTimingStatsOptions('find_order'), | ||
| ); |
There was a problem hiding this comment.
Is this call needed? I don't see the results being assigned to a variable.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/vulcan/tests/handlers/order-remove-handler.test.ts (9 hunks)
- indexer/services/vulcan/src/handlers/order-remove-handler.ts (5 hunks)
Additional comments: 13
indexer/services/vulcan/__tests__/handlers/order-remove-handler.test.ts (9)
344-346: The addition of
createdAtHeight,updatedAt, andupdatedAtHeightfields to the object is consistent with the pull request summary. Ensure that these fields are being correctly populated in theremovedOrderobject.481-483: Same as above.
619-621: Same as above.
758-760: Same as above.
1056-1058: Same as above.
1170-1172: Same as above.
1292-1294: Same as above.
1430-1432: Same as above.
1564-1566: Same as above.
indexer/services/vulcan/src/handlers/order-remove-handler.ts (4)
13-17: The import of
IsoStringis new and is used later in the code. Ensure that it is correctly imported and used.277-291: The logic to retrieve
canceledOrderbased on the remaining amount of the order is added. Ensure that the logic is correct and that thecanceledOrderis correctly used later in the code.513-529: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [513-561]
The
canceledOrderis now passed as a parameter increateSubaccountWebsocketMessageFromRemoveOrderResultmethod. Ensure that the method correctly handles the new parameter.
- 553-559: The
createdAtHeight,updatedAt, andupdatedAtHeightfields are added to theSubaccountMessageContentsobject. Ensure that these fields are correctly populated and used.
| } else { | ||
| canceledOrder = await runFuncWithTimingStat( | ||
| OrderTable.findById(OrderTable.orderIdToUuid(orderRemove.removedOrderId!)), | ||
| this.generateTimingStatsOptions('find_order'), | ||
| ); |
There was a problem hiding this comment.
Suggestion: This could probably be optimized by not querying for the order if there isn't an entry in the state filled quantums cache for the order. That would mean no fills for the order, and as it would be a short-term order that means it won't be in the DB. Up to you to add it as a TODO or fix now.
* [IND-482]: Send timing fields for orders when possible * nit and fix test
* [IND-482]: Send timing fields for orders when possible * nit and fix test
* [IND-482]: Send timing fields for orders when possible * nit and fix test
* [IND-482]: Send timing fields for orders when possible * nit and fix test
* [IND-482]: Send timing fields for orders when possible * nit and fix test
* [IND-482]: Send timing fields for orders when possible * nit and fix test
Changelist
Sends
createdAtHeight,updatedAtHeight, andupdatedAtfor stateful order placements and cancellations. When an order is removed, sendupdatedAtHeightandupdatedAtfor all orders andcreatedAtHeightfor stateful ordersTest Plan
Tested with unit tests
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.