-
Notifications
You must be signed in to change notification settings - Fork 74
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.
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
feat: add buys and sells as marks on tv chart #927
Changes from all commits
917296f
5b7b84d
a35df4e
31c726f
b7d25b3
bacdd99
508a417
898e65c
3de43bc
9e8324a
dd8677f
7096900
fcbc8b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { Dispatch, SetStateAction, useEffect } from 'react'; | ||
|
||
import { TvWidget } from '@/constants/tvchart'; | ||
|
||
import { getMarketFills } from '@/state/accountSelectors'; | ||
import { useAppSelector } from '@/state/appTypes'; | ||
import { getCurrentMarketId } from '@/state/perpetualsSelectors'; | ||
|
||
import { useAppThemeAndColorModeContext } from '../useAppThemeAndColorMode'; | ||
|
||
/** | ||
* @description Hook to handle marks for historic buys and sells on the TV chart | ||
*/ | ||
export function useBuySellMarks({ | ||
buySellMarksToggle, | ||
buySellMarksToggleOn, | ||
setBuySellMarksToggleOn, | ||
tvWidget, | ||
isChartReady, | ||
}: { | ||
buySellMarksToggle: HTMLElement | null; | ||
buySellMarksToggleOn: boolean; | ||
setBuySellMarksToggleOn: Dispatch<SetStateAction<boolean>>; | ||
tvWidget: TvWidget | null; | ||
isChartReady: boolean; | ||
}) { | ||
const marketId = useAppSelector(getCurrentMarketId); | ||
const fills = useAppSelector(getMarketFills); | ||
const currentMarketFills = marketId ? fills[marketId] : undefined; | ||
|
||
const theme = useAppThemeAndColorModeContext(); | ||
|
||
useEffect(() => { | ||
// Initialize onClick for Buys/Sells toggle | ||
if (isChartReady && buySellMarksToggle) { | ||
buySellMarksToggle.onclick = () => setBuySellMarksToggleOn((prev) => !prev); | ||
} | ||
}, [isChartReady, buySellMarksToggle, setBuySellMarksToggleOn]); | ||
|
||
useEffect( | ||
// Update marks on toggle and on new fills and on display preference changes | ||
() => { | ||
if (!isChartReady || !tvWidget) return; | ||
|
||
tvWidget.onChartReady(() => { | ||
tvWidget.headerReady().then(() => { | ||
if (buySellMarksToggleOn) { | ||
buySellMarksToggle?.classList?.add('toggle-active'); | ||
tvWidget.activeChart().refreshMarks(); | ||
} else { | ||
buySellMarksToggle?.classList?.remove('toggle-active'); | ||
tvWidget.activeChart().clearMarks(); | ||
} | ||
}); | ||
}); | ||
}, | ||
[buySellMarksToggleOn, buySellMarksToggle, tvWidget, isChartReady, currentMarketFills, theme] | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -275,9 +275,9 @@ export const useChartLines = ({ | |
if (isChartReady) { | ||
runOnChartReady(() => { | ||
if (orderLinesToggleOn) { | ||
orderLineToggle?.classList?.add('order-lines-active'); | ||
orderLineToggle?.classList?.add('toggle-active'); | ||
} else { | ||
orderLineToggle?.classList?.remove('order-lines-active'); | ||
orderLineToggle?.classList?.remove('toggle-active'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ import { getSavedResolution, getWidgetOptions, getWidgetOverrides } from '@/lib/ | |
import { useDydxClient } from '../useDydxClient'; | ||
import { useEnvFeatures } from '../useEnvFeatures'; | ||
import { useLocalStorage } from '../useLocalStorage'; | ||
import { useLocaleSeparators } from '../useLocaleSeparators'; | ||
import { useAllStatsigGateValues } from '../useStatsig'; | ||
import { useStringGetter } from '../useStringGetter'; | ||
import { useURLConfigs } from '../useURLConfigs'; | ||
|
@@ -40,18 +41,21 @@ export const useTradingView = ({ | |
orderLineToggleRef, | ||
orderbookCandlesToggleRef, | ||
orderbookCandlesToggleOn, | ||
buySellMarksToggleRef, | ||
setIsChartReady, | ||
}: { | ||
tvWidgetRef: React.MutableRefObject<TvWidget | null>; | ||
orderLineToggleRef: React.MutableRefObject<HTMLElement | null>; | ||
orderbookCandlesToggleRef: React.MutableRefObject<HTMLElement | null>; | ||
orderbookCandlesToggleOn: boolean; | ||
buySellMarksToggleRef: React.MutableRefObject<HTMLElement | null>; | ||
setIsChartReady: React.Dispatch<React.SetStateAction<boolean>>; | ||
}) => { | ||
const stringGetter = useStringGetter(); | ||
const urlConfigs = useURLConfigs(); | ||
const featureFlags = useAllStatsigGateValues(); | ||
const { isOhlcEnabled } = useEnvFeatures(); | ||
const { group, decimal } = useLocaleSeparators(); | ||
|
||
const appTheme = useAppSelector(getAppTheme); | ||
const appColorMode = useAppSelector(getAppColorMode); | ||
|
@@ -100,7 +104,10 @@ export const useTradingView = ({ | |
store, | ||
getCandlesForDatafeed, | ||
initialPriceScale, | ||
orderbookCandlesToggleOn | ||
orderbookCandlesToggleOn, | ||
{ decimal, group }, | ||
selectedLocale, | ||
stringGetter | ||
), | ||
interval: (savedResolution ?? DEFAULT_RESOLUTION) as ResolutionString, | ||
locale: SUPPORTED_LOCALE_BASE_TAGS[selectedLocale] as LanguageCode, | ||
|
@@ -119,7 +126,7 @@ export const useTradingView = ({ | |
orderLineToggleRef.current = tvWidgetRef.current.createButton(); | ||
orderLineToggleRef.current.innerHTML = `<span>${stringGetter({ | ||
key: STRING_KEYS.ORDER_LINES, | ||
})}</span> <div class="displayOrdersButton-toggle"></div>`; | ||
})}</span> <div class="toggle"></div>`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I wonder if it's worth factoring out There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think as a followup im going to investigate just combining a lot of the logic here for the three settings that we have :P so combining the html here can be part of that! |
||
orderLineToggleRef.current.setAttribute( | ||
'title', | ||
stringGetter({ key: STRING_KEYS.ORDER_LINES_TOOLTIP }) | ||
|
@@ -135,9 +142,17 @@ export const useTradingView = ({ | |
}); | ||
|
||
orderbookCandlesToggleRef.current = tvWidgetRef.current.createButton(); | ||
orderbookCandlesToggleRef.current.innerHTML = `<span>${`${ohlcTitle}*`}</span> <div class="ohlcButton-toggle"></div>`; | ||
orderbookCandlesToggleRef.current.innerHTML = `<span>${`${ohlcTitle}*`}</span> <div class="toggle"></div>`; | ||
orderbookCandlesToggleRef.current.setAttribute('title', ohlcBody as string); | ||
} | ||
if (buySellMarksToggleRef) { | ||
buySellMarksToggleRef.current = tvWidgetRef.current.createButton(); | ||
buySellMarksToggleRef.current.innerHTML = `<span>${stringGetter({ key: STRING_KEYS.BUYS_SELLS_TOGGLE })}</span> <div class="toggle"></div>`; | ||
buySellMarksToggleRef.current.setAttribute( | ||
'title', | ||
stringGetter({ key: STRING_KEYS.BUYS_SELLS_TOGGLE_TOOLTIP }) | ||
); | ||
} | ||
} | ||
}); | ||
|
||
|
@@ -154,6 +169,8 @@ export const useTradingView = ({ | |
orderLineToggleRef.current = null; | ||
orderbookCandlesToggleRef.current?.remove(); | ||
orderbookCandlesToggleRef.current = null; | ||
buySellMarksToggleRef.current?.remove(); | ||
buySellMarksToggleRef.current = null; | ||
tvWidgetRef.current?.remove(); | ||
tvWidgetRef.current = null; | ||
setIsChartReady(false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
import BigNumber from 'bignumber.js'; | ||
import { describe, expect, it } from 'vitest'; | ||
|
||
import { SubaccountFill } from '@/constants/abacus'; | ||
|
||
import { getAverageFillPrice } from '../orders'; | ||
|
||
// TODO: add real SubaccountFill fixtures here, but `getAverageFillPrice` only uses 'size' and 'price' for now | ||
const mockFill1 = { size: 1, price: 2 } as SubaccountFill; | ||
const mockFill2 = { size: 0.5, price: 1 } as SubaccountFill; | ||
|
||
const fraction = (num: number, denom: number) => { | ||
return BigNumber(num).div(BigNumber(denom)); | ||
}; | ||
|
||
describe('getAverageFillPrice', () => { | ||
it('doesnt error on empty arrays', () => { | ||
expect(getAverageFillPrice([])).toBeNull(); | ||
}); | ||
|
||
it('calculates single fill averages', () => { | ||
expect(getAverageFillPrice([mockFill1])).toEqual(BigNumber(2)); | ||
}); | ||
|
||
it('calculates averages', () => { | ||
expect(getAverageFillPrice([mockFill1, mockFill2])).toEqual(fraction(5, 3)); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import { OrderSide } from '@dydxprotocol/v4-client-js'; | ||
import BigNumber from 'bignumber.js'; | ||
|
||
import { | ||
AbacusOrderStatus, | ||
AbacusOrderType, | ||
AbacusOrderTypes, | ||
KotlinIrEnumValues, | ||
Nullable, | ||
SubaccountFills, | ||
TRADE_TYPES, | ||
type Asset, | ||
type OrderStatus, | ||
|
@@ -140,3 +142,13 @@ export const getHydratedTradingData = < | |
|
||
export const getTradeType = (orderType: string) => | ||
TRADE_TYPES[orderType as KotlinIrEnumValues<typeof AbacusOrderType>]; | ||
|
||
export const getAverageFillPrice = (fills: SubaccountFills) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a quick test for this? |
||
let total = BigNumber(0); | ||
let totalSize = BigNumber(0); | ||
fills.forEach((fill) => { | ||
total = total.plus(BigNumber(fill.price).times(fill.size)); | ||
totalSize = totalSize.plus(fill.size); | ||
}); | ||
return totalSize.gt(0) ? total.div(totalSize) : null; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { ResolutionString } from 'public/tradingview/charting_library'; | ||
import { describe, expect, it } from 'vitest'; | ||
|
||
import { getBarTime } from '../utils'; | ||
|
||
describe('getBarTime', () => { | ||
it('should return the correct value when times start at 0', () => { | ||
const beginningOfChart = getBarTime(0, 0, '1' as ResolutionString); | ||
expect(beginningOfChart).toBe(0); | ||
|
||
const middleOfChart = getBarTime(0, 10001, '1' as ResolutionString); | ||
expect(middleOfChart).toBe(10); | ||
}); | ||
|
||
it('should return the correct value when times dont start at 0', () => { | ||
// Intervals here look like 100, 1100, ... 9100, 10100, .etc | ||
// Should resolve to 9100ms bucket which is 9s | ||
const nonZeroStart = getBarTime(100, 10001, '1' as ResolutionString); | ||
expect(nonZeroStart).toBe(9); | ||
}); | ||
|
||
it('should return correct value with real timestamps', () => { | ||
const timestampInSeconds = getBarTime(1716091200000, 1723573418524, '1D' as ResolutionString); | ||
expect(timestampInSeconds).toBe(1723521600); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, unrelated to this PR specifically, I wonder if it'd be better to convert
CandleResolution
,RESOLUTION_MAP
, andRESOLUTION_CHART_CONFIGS
to functions where we swtich case on the keys (that we could convert to an enum) just to make sure we catch all the cases. But honestly I don't imagine this code will change too much so just a light considerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i like all of these as enums! nice way to get free typing so people don't put in something crazy (unless resolutionStrings already gives us that? that type has a few layers of abstraction to it so it's not immediately clear what it refers to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh yeah, there's actually a way we can do that with just typescript by converting the library's
ResolutionString
(which is unfort just a generic string type) to an enum with the same values and changing the type of these maps. for example if you do:you'll see that this gives a typescript error because the key
1D
is missing. we can address this separately if we'd like though :Pwe can alternatively also combine all three maps into one map, so something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeahhh that's very much in line with what i was envisioning. i dont think either have to be done in this PR but good to keep in mind for a cleanup task 👍