feat: Trade history#2087
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Wiz Scan Summary
|
| Scanner | Findings |
|---|---|
 |
23    |
 |
- |
 |
- |
 |
- |
 |
- |
 |
- |
| Total | 23 |
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.
dankim214
left a comment
There was a problem hiding this comment.
looks good!
dont have the trade history to properly test this but going off the screenshot it looks like the we have TradeHistoryList and TradeHistoryTable now, which is being used in what? i dont see <TradeHistoryList /> being rendered
| data.wallet!, | ||
| data.subaccount!, |
There was a problem hiding this comment.
do we need this assertion? i wouldve thought the above typeguards do that for us
There was a problem hiding this comment.
we do for data.wallet, the subaccount assertion is redundant but we do it that way in other areas like orders.ts so i figured id do the same
There was a problem hiding this comment.
hmm totally fine to do in a separate PR (or even just going forward) but generally could we avoid asserting like this unless its an escape hatch situation?
think assertion in this case is only required because we did nested access for wallet. if we do something like
(indexerClient, { wallet, subaccount, isPerpsGeoRestricted}) => {
if (wallet == null || data.subaccount == null || data.isPerpsGeoRestricted) {
return null
}
// From this point on wallet and subaccount will be defined
}
and i dont think how much contextual value the variable data is providing
There was a problem hiding this comment.
100% agree, imo might be worth doing in a separate pr and cleaning up all similar logic at that time
| marketId?: string; | ||
| isLong: boolean; | ||
| isCross: boolean; | ||
| shareType?: 'open' | 'close' | 'liquidated' | 'partialClose' | 'extend' | undefined; |
There was a problem hiding this comment.
nit: can we pull this into an enum
| liquidationPrice: trade.liquidationPrice, | ||
| }; | ||
|
|
||
| const openShareDialog = () => { |
| getCellValue: (row) => row.createdAt, | ||
| label: stringGetter({ key: STRING_KEYS.TIME }), | ||
| renderCell: ({ createdAt }) => ( | ||
| <Output |
There was a problem hiding this comment.
could we use the new DateAgeOutput component (so we could switch between time/date) similar to FillsTable
| getCellValue: (row) => row.marketId, | ||
| label: stringGetter({ key: STRING_KEYS.MARKET }), | ||
| renderCell: ({ marketSummary }) => ( | ||
| <TableCell |
There was a problem hiding this comment.
can use MarketSummaryTableCell
| label: stringGetter({ key: STRING_KEYS.SIDE }), | ||
| renderCell: ({ side }) => | ||
| side != null ? ( | ||
| <$Side side={side}>{side === IndexerOrderSide.BUY ? 'Buy' : 'Sell'}</$Side> |
There was a problem hiding this comment.
can use <OrderSideTag />
There was a problem hiding this comment.
We actually dont even show this column in the ui so im gonna delete it
eb9e37f to
91bb52f
Compare
dankim214
left a comment
There was a problem hiding this comment.
looks good! just couple nits
| liveTrades: IndexerCompositeTradeHistoryObject[] | undefined | ||
| ): SubaccountTrade[] { | ||
| const getTradesById = (data: IndexerCompositeTradeHistoryObject[]) => { | ||
| const tradesWithIds = data.filter((trade) => { |
There was a problem hiding this comment.
nit: (trade): trade is TradeType & { id: string }
can typeguard here so we don't have to assert below
| data.wallet!, | ||
| data.subaccount!, |
There was a problem hiding this comment.
hmm totally fine to do in a separate PR (or even just going forward) but generally could we avoid asserting like this unless its an escape hatch situation?
think assertion in this case is only required because we did nested access for wallet. if we do something like
(indexerClient, { wallet, subaccount, isPerpsGeoRestricted}) => {
if (wallet == null || data.subaccount == null || data.isPerpsGeoRestricted) {
return null
}
// From this point on wallet and subaccount will be defined
}
and i dont think how much contextual value the variable data is providing
| ? 'liquidated' | ||
| : undefined; | ||
|
|
||
| const prevSize = !!trade.prevSize && !!trade.price ? trade.prevSize * trade.price : undefined; |
There was a problem hiding this comment.
gotcha- not to be pedantic, but could we call this something like prevSizeDollar or just something different than prevSize since this one is a dollar amount and not size amount
64bc652 to
1080154
Compare
Changes
Issue
Eng-1839
Screenshots/Recordings (Optional)