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.

Sign up for GitHub

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

[IND-486] update trades api with fill type #779

Merged
merged 4 commits into from
Nov 9, 2023
Merged

[IND-486] update trades api with fill type #779

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3351d10
update trades api with fill type
dydxwill Nov 9, 2023
ea2117e
lint
dydxwill Nov 9, 2023
7b74826
address cmts
dydxwill Nov 9, 2023
830e352
revert
dydxwill Nov 9, 2023
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
2 changes: 1 addition & 1 deletion indexer/packages/kafka/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major version change? Some properties were removed and replaced with a different type of property.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FE (Abacus) can handle the mapping from liquidation to FillType.LIQUIDATED. Not sure if any MM depends on this.

export const MARKETS_WEBSOCKET_MESSAGE_VERSION: string = '1.0.0';
export const CANDLES_WEBSOCKET_MESSAGE_VERSION: string = '1.0.0';
3 changes: 1 addition & 2 deletions indexer/packages/postgres/src/types/websocket-message-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ export interface TradeContent {
price: string,
side: string,
createdAt: IsoString,
liquidation: boolean,
deleveraging: boolean,
type: FillType,
}

/* ------- MarketMessageContents ------- */
Expand Down
4 changes: 4 additions & 0 deletions indexer/services/comlink/public/api-documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,7 @@ fetch('https://indexer.v4testnet.dydx.exchange/v4/trades/perpetualMarket/{ticker
"side": "BUY",
"size": "string",
"price": "string",
"type": "MARKET",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the FillType from Indexer will never be "MARKET". FE has to use clientMetadata to derive "MARKET".

"createdAt": "string",
"createdAtHeight": "string"
}
Expand Down Expand Up @@ -3062,6 +3063,7 @@ or
"side": "BUY",
"size": "string",
"price": "string",
"type": "MARKET",
"createdAt": "string",
"createdAtHeight": "string"
}
Expand All @@ -3076,6 +3078,7 @@ or
|side|[OrderSide](#schemaorderside)|true|none|none|
|size|string|true|none|none|
|price|string|true|none|none|
|type|[FillType](#schemafilltype)|true|none|none|
|createdAt|[IsoString](#schemaisostring)|true|none|none|
|createdAtHeight|string|true|none|none|

Expand All @@ -3094,6 +3097,7 @@ or
"side": "BUY",
"size": "string",
"price": "string",
"type": "MARKET",
"createdAt": "string",
"createdAtHeight": "string"
}
Expand Down
4 changes: 4 additions & 0 deletions indexer/services/comlink/public/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -859,6 +859,9 @@
"price": {
"type": "string"
},
"type": {
"$ref": "#/components/schemas/FillType"
},
"createdAt": {
"$ref": "#/components/schemas/IsoString"
},
Expand All @@ -871,6 +874,7 @@
"side",
"size",
"price",
"type",
"createdAt",
"createdAtHeight"
],
Expand Down
10 changes: 5 additions & 5 deletions indexer/services/comlink/public/websocket-documentation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,7 @@ interface TradeContent {
price: string,
side: string,
createdAt: IsoString,
liquidation: boolean,
type: FillType,
}
```

Expand All @@ -736,31 +736,31 @@ interface TradeContent {
"price": "27839",
"side": "BUY",
"createdAt": "2023-04-04T00:29:19.353Z",
"liquidation": false
"type": "LIQUIDATION"
},
{
"id": "38e64479-af09-5417-a795-195f83879156",
"size": "0.000000004",
"price": "27839",
"side": "BUY",
"createdAt": "2023-04-04T00:29:19.353Z",
"liquidation": false
"type": "LIQUIDATION"
},
{
"id": "d310c32c-f066-5ba8-a97d-10a29d9a6c84",
"size": "0.000000005",
"price": "27837",
"side": "SELL",
"createdAt": "2023-04-04T00:29:19.353Z",
"liquidation": false
"type": "MARKET"
},
{
"id": "dd1088b5-5cab-518f-a59c-4d5f735ab861",
"size": "0.000118502",
"price": "27837",
"side": "SELL",
"createdAt": "2023-04-04T00:29:19.353Z",
"liquidation": false
"type": "LIMIT"
},
],
},
Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/src/request-helpers/request-transformer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ export function fillToTradeResponseObject(
side: fill.side,
size: fill.size,
price: fill.price,
type: fill.type,
createdAt: fill.createdAt,
createdAtHeight: fill.createdAtHeight,
};
Expand Down
1 change: 1 addition & 0 deletions indexer/services/comlink/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ export interface TradeResponseObject {
side: OrderSide,
size: string,
price: string,
type: FillType,
createdAt: IsoString,
createdAtHeight: string,
}
Expand Down
36 changes: 18 additions & 18 deletions indexer/services/ender/__tests__/helpers/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,36 @@
import { SUBACCOUNTS_WEBSOCKET_MESSAGE_VERSION } from '@dydxprotocol-indexer/kafka';
import { testConstants, TradeContent } from '@dydxprotocol-indexer/postgres';
import { FillType, testConstants, TradeContent } from '@dydxprotocol-indexer/postgres';
import {
bigIntToBytes,
ORDER_FLAG_CONDITIONAL,
ORDER_FLAG_LONG_TERM,
ORDER_FLAG_SHORT_TERM,
ORDER_FLAG_CONDITIONAL,
} from '@dydxprotocol-indexer/v4-proto-parser';
import {
AssetCreateEventV1,
ClobPairStatus,
DeleveragingEventV1,
FundingEventV1_Type,
LiquidationOrderV1,
MarketBaseEventV1,
MarketEventV1,
IndexerOrder,
IndexerOrder_ConditionType,
IndexerOrder_Side,
IndexerOrder_TimeInForce,
OrderFillEventV1,
IndexerOrderId,
StatefulOrderEventV1,
IndexerSubaccountId,
LiquidationOrderV1,
LiquidityTierUpsertEventV1,
MarketBaseEventV1,
MarketEventV1,
OrderFillEventV1,
OrderRemovalReason,
PerpetualMarketCreateEventV1,
StatefulOrderEventV1,
SubaccountMessage,
SubaccountUpdateEventV1,
Timestamp,
TransferEventV1,
IndexerOrder_ConditionType,
OrderRemovalReason,
AssetCreateEventV1,
PerpetualMarketCreateEventV1,
ClobPairStatus,
LiquidityTierUpsertEventV1,
UpdatePerpetualEventV1,
UpdateClobPairEventV1,
DeleveragingEventV1,
UpdatePerpetualEventV1,
} from '@dydxprotocol-indexer/v4-protos';
import Long from 'long';
import { DateTime } from 'luxon';
Expand All @@ -40,7 +40,8 @@ import { SubaccountUpdate } from '../../src/lib/translated-types';
import {
ConsolidatedKafkaEvent,
FundingEventMessage,
OrderFillEventWithLiquidation, OrderFillEventWithOrder,
OrderFillEventWithLiquidation,
OrderFillEventWithOrder,
SingleTradeMessage,
} from '../../src/lib/types';
import { contentToSingleTradeMessage, createConsolidatedKafkaEventFromTrade } from './kafka-publisher-helpers';
Expand Down Expand Up @@ -324,8 +325,7 @@ export const defaultTradeContent: TradeContent = {
price: '10000',
side: 'BUY',
createdAt: 'createdAt',
liquidation: true,
deleveraging: false,
type: FillType.MARKET,
};
export const defaultTradeMessage: SingleTradeMessage = contentToSingleTradeMessage(
defaultTradeContent,
Expand Down
3 changes: 1 addition & 2 deletions indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,8 +807,7 @@ export async function expectDefaultTradeKafkaMessageFromTakerFillId(
price: takerFill!.price,
side: takerFill!.side.toString(),
createdAt: takerFill!.createdAt,
liquidation: isLiquidation(takerFill!),
deleveraging: isDeleveraging(takerFill!),
type: takerFill!.type,
},
],
};
Comment on lines 805 to 811
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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),
},
],
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Expand Down
15 changes: 4 additions & 11 deletions indexer/services/ender/__tests__/lib/kafka-publisher.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,7 @@ import {
import { IndexerSubaccountId, SubaccountMessage, TradeMessage } from '@dydxprotocol-indexer/v4-protos';
import Big from 'big.js';
import _ from 'lodash';
import {
AnnotatedSubaccountMessage,
ConsolidatedKafkaEvent,
SingleTradeMessage,
} from '../../src/lib/types';
import { AnnotatedSubaccountMessage, ConsolidatedKafkaEvent, SingleTradeMessage } from '../../src/lib/types';

import { KafkaPublisher } from '../../src/lib/kafka-publisher';
import {
Expand Down Expand Up @@ -413,8 +409,7 @@ describe('kafka-publisher', () => {
price: '10000',
side: 'side',
createdAt: 'today',
liquidation: false,
deleveraging: false,
type: FillType.LIMIT,
};
const singleTrade1: SingleTradeMessage = contentToSingleTradeMessage(
tradeContent1,
Expand All @@ -427,8 +422,7 @@ describe('kafka-publisher', () => {
price: '12000',
side: 'side',
createdAt: 'today',
liquidation: false,
deleveraging: false,
type: FillType.LIMIT,
};
const singleTrade2: SingleTradeMessage = contentToSingleTradeMessage(
tradeContent2,
Expand All @@ -442,8 +436,7 @@ describe('kafka-publisher', () => {
price: '1000',
side: 'side',
createdAt: 'today',
liquidation: false,
deleveraging: false,
type: FillType.LIMIT,
};
const singleTrade3: SingleTradeMessage = contentToSingleTradeMessage(
tradeContent3,
Expand Down
3 changes: 1 addition & 2 deletions indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,7 @@ export abstract class AbstractOrderFillHandler<T> extends Handler<T> {
price: fill.price,
side: fill.side.toString(),
createdAt: fill.createdAt,
liquidation: isLiquidation(fill),
deleveraging: isDeleveraging(fill),
type: fill.type,
},
],
};
Expand Down
Loading