[IND-486] update trades api with fill type #779
Conversation
# Walkthrough
The changes primarily revolve around the replacement of the `liquidation` and `deleveraging` properties with a single `type` property in various interfaces and objects across the codebase. This `type` property is of `FillType` and can take values such as "LIQUIDATION", "MARKET", or "LIMIT". The changes also involve updates to import statements, function signatures, and test cases to accommodate this new property.
# Changes
| File(s) | Change Summary |
| --- | --- |
| `.../kafka/src/constants.ts` | Updated `TRADES_WEBSOCKET_MESSAGE_VERSION` from '1.1.0' to '1.2.0'. |
| `.../postgres/src/types/websocket-message-types.ts`<br>`.../comlink/public/websocket-documentation.md`<br>`.../ender/src/handlers/order-fills/abstract-order-fill-handler.ts` | Replaced `liquidation` and `deleveraging` properties with `type` property. |
| `.../comlink/public/api-documentation.md` | Added "type" field of type "MARKET" to the data structure. |
| `.../comlink/src/request-helpers/request-transformer.ts`<br>`.../comlink/src/types.ts`<br>`.../ender/src/helpers/kafka-helper.ts` | Added or modified "type" property in various functions and interfaces. |
| `.../ender/__tests__/helpers/constants.ts`<br>`.../ender/__tests__/lib/kafka-publisher.test.ts`<br>`.../ender/__tests__/helpers/indexer-proto-helpers.ts` | Updated test cases and imports to accommodate the new `type` property. |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 ignored due to filter (1)
- indexer/services/comlink/public/swagger.json
Files selected for processing (10)
- indexer/packages/kafka/src/constants.ts (1 hunks)
- indexer/packages/postgres/src/types/websocket-message-types.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (4 hunks)
- indexer/services/comlink/public/websocket-documentation.md (2 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (1 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
- indexer/services/ender/tests/helpers/constants.ts (3 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (1 hunks)
- indexer/services/ender/tests/lib/kafka-publisher.test.ts (4 hunks)
- indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/kafka/src/constants.ts
Additional comments: 14
indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (1)
- 428-434: The
typeproperty is now required in the trade response object. Ensure that all consumers of this API are updated to handle this new property.indexer/services/comlink/src/types.ts (1)
- 174-180: The new
typeproperty ofFillTypeis added to theTradeResponseObjectinterface. Ensure that all instances whereTradeResponseObjectis used are updated to handle this new property.indexer/services/ender/__tests__/helpers/constants.ts (1)
- 325-329: The
typeproperty has been added to thedefaultTradeContentobject. Ensure that all instances where this object is used have been updated to handle this new property.indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
- 177-183: The new
typeproperty is correctly added to theTradeResponseObject. Ensure that all calls to this function throughout the codebase have been updated to handle the newtypeproperty.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (1)
- 807-813: The
typeproperty of theTradeMessageContentsobject is being set totakerFill!.type. Ensure that thetypeproperty is correctly derived from thetakerFillobject and that it accurately represents the type of the fill (e.g., 'maker', 'taker', 'liquidation', 'deleveraging'). If thetypeproperty is not correctly derived, it could lead to incorrect information being sent in the Kafka message.indexer/packages/postgres/src/types/websocket-message-types.ts (1)
- 175-179: The
typeproperty is now required in theTradeContentinterface. Ensure that all instances ofTradeContentin the codebase are updated to include this property.indexer/services/ender/__tests__/lib/kafka-publisher.test.ts (2)
19-25: The import statements have been reorganized for better readability and maintainability. This is a good practice as it helps in identifying the dependencies of the module at a glance.
436-440: The
liquidationanddeleveragingproperties have been replaced withtype: FillType.LIMITin theTradeContentobjects. Ensure that this change is reflected in all places where these objects are used and that thetypeproperty is being correctly set based on the logic of your application.indexer/services/comlink/public/websocket-documentation.md (2)
717-717: The
typeproperty has been added to theTradeContentinterface. Ensure that all instances ofTradeContentin the codebase have been updated to include this property.739-763: The
typeproperty has been added to the trade objects in the example. Ensure that the logic for determining thetypeproperty is correctly implemented in the codebase.indexer/services/comlink/public/api-documentation.md (4)
1642-1642: The addition of the "type" field is consistent with the changes described in the pull request summary. Ensure that all references to this data structure in the codebase have been updated to include the "type" field.
3066-3066: The addition of the "type" field is consistent with the changes described in the pull request summary. Ensure that all references to this data structure in the codebase have been updated to include the "type" field.
3081-3081: The addition of the "type" field is consistent with the changes described in the pull request summary. Ensure that all references to this data structure in the codebase have been updated to include the "type" field.
3100-3100: The addition of the "type" field is consistent with the changes described in the pull request summary. Ensure that all references to this data structure in the codebase have been updated to include the "type" field.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (2 hunks)
- indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (2 hunks)
- indexer/services/ender/src/helpers/kafka-helper.ts (2 hunks)
Additional comments: 3
indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (1)
- 426-429: Ensure that all references to the
typeproperty in theTradeMessageContentsobject have been updated throughout the codebase to reflect the newFillTypeenum values.indexer/services/ender/src/helpers/kafka-helper.ts (2)
2-30: The import list has been updated to include new types and remove unused ones. Ensure that these changes are reflected across the codebase.
273-274: The
generateOraclePriceContentsfunction has been updated. Ensure that the changes are reflected in all calls to this function.
| price: takerFill!.price, | ||
| side: takerFill!.side.toString(), | ||
| createdAt: takerFill!.createdAt, | ||
| liquidation: isLiquidation(takerFill!), | ||
| deleveraging: isDeleveraging(takerFill!), | ||
| type: takerFill!.type, | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
The type property of the takerFill object is directly assigned to the type property of the trade object in the contents variable. This could potentially lead to incorrect data if the type property of the takerFill object does not match the expected values for the type property of the trade object. It would be safer to map the type property of the takerFill object to the corresponding value for the type property of the trade object.
- type: takerFill!.type,
+ type: mapFillTypeToTradeType(takerFill!.type),Please ensure that the mapFillTypeToTradeType function is implemented to correctly map the type property of the takerFill object to the corresponding value for the type property of the trade object.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| price: takerFill!.price, | |
| side: takerFill!.side.toString(), | |
| createdAt: takerFill!.createdAt, | |
| liquidation: isLiquidation(takerFill!), | |
| deleveraging: isDeleveraging(takerFill!), | |
| type: takerFill!.type, | |
| }, | |
| ], | |
| }; | |
| price: takerFill!.price, | |
| side: takerFill!.side.toString(), | |
| createdAt: takerFill!.createdAt, | |
| type: mapFillTypeToTradeType(takerFill!.type), | |
| }, | |
| ], | |
| }; |
There was a problem hiding this comment.
I am OK with using FillType directly here. It wouldn't hurt to create a new TradeType and do the mapping, but it won't make any difference to FE at this moment.
There was a problem hiding this comment.
@johnqh Understood. If you're confident that the FillType values will always align with the expected TradeType values, then using FillType directly should be fine. However, if there's any chance that these could diverge in the future, implementing a mapping function like mapFillTypeToTradeType could provide an extra layer of safety. But for now, we can proceed as is. Thank you for your input.
| @@ -1639,6 +1639,7 @@ fetch('https://indexer.v4testnet.dydx.exchange/v4/trades/perpetualMarket/{ticker | |||
| "side": "BUY", | |||
| "size": "string", | |||
| "price": "string", | |||
| "type": "MARKET", | |||
There was a problem hiding this comment.
I think the FillType from Indexer will never be "MARKET". FE has to use clientMetadata to derive "MARKET".
| price: takerFill!.price, | ||
| side: takerFill!.side.toString(), | ||
| createdAt: takerFill!.createdAt, | ||
| liquidation: isLiquidation(takerFill!), | ||
| deleveraging: isDeleveraging(takerFill!), | ||
| type: takerFill!.type, | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
I am OK with using FillType directly here. It wouldn't hurt to create a new TradeType and do the mapping, but it won't make any difference to FE at this moment.
| @@ -2,6 +2,6 @@ export const TO_ENDER_TOPIC: string = 'to-ender'; | |||
|
|
|||
| export const ORDERBOOKS_WEBSOCKET_MESSAGE_VERSION: string = '1.0.0'; | |||
| export const SUBACCOUNTS_WEBSOCKET_MESSAGE_VERSION: string = '2.4.0'; | |||
| export const TRADES_WEBSOCKET_MESSAGE_VERSION: string = '1.1.0'; | |||
| export const TRADES_WEBSOCKET_MESSAGE_VERSION: string = '1.2.0'; | |||
There was a problem hiding this comment.
Major version change? Some properties were removed and replaced with a different type of property.
There was a problem hiding this comment.
FE (Abacus) can handle the mapping from liquidation to FillType.LIQUIDATED. Not sure if any MM depends on this.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/kafka/src/constants.ts (1 hunks)
- indexer/services/comlink/public/api-documentation.md (4 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/kafka/src/constants.ts
Additional comments: 4
indexer/services/comlink/public/api-documentation.md (4)
1642-1642: The new "type" field is added to the data structure. Ensure that all parts of the codebase that use this structure are updated to handle the new field.
3066-3066: The new "type" field is added to the data structure. Ensure that all parts of the codebase that use this structure are updated to handle the new field.
3081-3081: The new "type" field is added to the data structure. Ensure that all parts of the codebase that use this structure are updated to handle the new field.
3100-3100: The new "type" field is added to the data structure. Ensure that all parts of the codebase that use this structure are updated to handle the new field.
| @@ -3094,6 +3097,7 @@ or | |||
| "side": "BUY", | |||
| "size": "string", | |||
| "price": "string", | |||
| "type": "LIMIT", | |||
There was a problem hiding this comment.
are you manually editing this? It should be autogenerated when comlink is built
There was a problem hiding this comment.
ah yeah, cc @johnqh
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/comlink/public/api-documentation.md (4 hunks)
Additional comments: 4
indexer/services/comlink/public/api-documentation.md (4)
1642-1642: The addition of the "type" field to the data structure is consistent with the PR description. However, as per the previous comment by johnqh, the "type" field from the Indexer will never be "MARKET". This needs to be verified.
3066-3066: The addition of the "type" field to the data structure is consistent with the PR description. However, as per the previous comment by johnqh, the "type" field from the Indexer will never be "MARKET". This needs to be verified.
3081-3081: The addition of the "type" field to the data structure is consistent with the PR description. However, as per the previous comment by johnqh, the "type" field from the Indexer will never be "MARKET". This needs to be verified.
3100-3100: The addition of the "type" field to the data structure is consistent with the PR description. However, as per the previous comment by johnqh, the "type" field from the Indexer will never be "MARKET". This needs to be verified.
Changelist
consolidate
/v4/tradesAPI endpoint response and websocket message.Test Plan
unit tested.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.