feat: add buys and sells as marks on tv chart #927

tinaszheng · 2024-08-15T21:55:07Z

This PR adds Marks to the TradingView chart for historical buy and sell orders.

Adding supports_marks: true to the datafeed configuration will tell TV to query the new getMarks method
Implemented getMarks
Also did some minor cleanup in our custom css file by combining a bunch of duplicate toggle classnames into singular .toggle and .toggle-active

Testing plan

Create a bunch of orders and close them (or view a market for which you have a bunch of historic orders)
Verify that the buy/sell marks show up on the chart for the correct time interval
Verify that the text in the tooltip shows the correct size and average price

linear · 2024-08-15T21:55:10Z

CT-1055 Show buys/sells

vercel · 2024-08-15T21:55:10Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
v4-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 19, 2024 7:42pm
v4-testnet	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 19, 2024 7:42pm

tinaszheng · 2024-08-15T21:57:30Z

src/lib/tradingView/dydxfeed/utils.ts

+    .toNumber();
+}
+
+function averageFillPrice(fills: SubaccountFills) {


need to consolidate this method with logic from this PR

…-marks

moo-onthelawn

🔥

moo-onthelawn · 2024-08-16T15:23:45Z

src/constants/candles.ts

+  '1': timeUnits.second,
+  '5': 5 * timeUnits.minute,
+  '15': 15 * timeUnits.minute,
+  '30': 30 * timeUnits.minute,


nit, unrelated to this PR specifically, I wonder if it'd be better to convert CandleResolution, RESOLUTION_MAP, and RESOLUTION_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 consider

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

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:

enum InterfaceResolutionString { ONE_MINUTE = '1', FIVE_MINUTES = '5', FIFTEEN_MINUTES = '15', THIRTY_MINUTES = '30', ONE_HOUR = '60', FOUR_HOURS = '240', ONE_DAY = '1D', } /** * @description ResolutionStrings used with TradingView's charting library mapped to time interval per candle in ms */ export const RESOLUTION_TO_INTERVAL_MS: { [resolution in InterfaceResolutionString]: number } = { '1': timeUnits.second, '5': 5 * timeUnits.minute, '15': 15 * timeUnits.minute, '30': 30 * timeUnits.minute, '60': timeUnits.hour, '240': 4 * timeUnits.hour, // '1D': timeUnits.day, };

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 :P

we can alternatively also combine all three maps into one map, so something like

type ResolutionConfig = { candleResolution: CandleResolution, intervalMs: number, defaultRange: number } export const RESOLUTION_MAP: { [resolution: ResolutionString]: ResolutionConfig } = { '1': { candleResolution: CandleResolution.ONE_MINUTE, intervalMs: timeUnits.second, defaultRange: timeUnits.hour }, ... };

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 👍

moo-onthelawn · 2024-08-16T15:24:29Z

src/hooks/tradingView/useChartLines.ts

          } else {
-            orderLineToggle?.classList?.remove('order-lines-active');
+            orderLineToggle?.classList?.remove('toggle-active');


moo-onthelawn · 2024-08-16T15:26:22Z

src/hooks/tradingView/useTradingView.ts

              orderbookCandlesToggleRef.current.setAttribute('title', ohlcBody as string);
            }
+            if (buySellMarksToggleRef) {
+              buySellMarksToggleRef.current = tvWidgetRef.current.createButton();
+              buySellMarksToggleRef.current.innerHTML = `<span>Buys/Sells</span> <div class="toggle"></div>`;


can we localize Buys/Sells? i.e. like ORDER_LINES does it

OMG yes i forgot! i even added it to v4-localization already 😂

moo-onthelawn · 2024-08-16T15:26:38Z

src/hooks/tradingView/useTradingView.ts

+              buySellMarksToggleRef.current.innerHTML = `<span>Buys/Sells</span> <div class="toggle"></div>`;
+              buySellMarksToggleRef.current.setAttribute(
+                'title',
+                'Display historic buys and sells'


moo-onthelawn · 2024-08-16T15:27:02Z

src/lib/__test__/tradingviewUtils.spec.ts

+    const timestampInSeconds = getBarTime(1716091200000, 1723573418524, '1D' as ResolutionString);
+    expect(timestampInSeconds).toBe(1723521600);
+  });
+});


yessss 🔥

moo-onthelawn · 2024-08-16T15:30:38Z

src/hooks/tradingView/useBuySellMarks.ts

+      tvWidget,
+      isChartReady,
+      currentMarketFills,
+      theme.positive,


is the theme.positive dependency for display pref changes? if so, maybe we update it to theme (because I imagine the red shades and green shades can also be diff for diff themes)

moo-onthelawn · 2024-08-16T15:32:13Z

src/hooks/tradingView/useTradingView.ts

@@ -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>`;


nit: I wonder if it's worth factoring out toggleHtml() helper

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!

moo-onthelawn · 2024-08-16T15:37:21Z

src/lib/tradingView/dydxfeed/utils.ts

+    time: getBarTime(barStartMs, fill.createdAtMilliseconds, resolution) ?? 0,
+    minSize: 16,
+    text,
+    labelFontColor: 'white',


could be follow up, but instead of white, can we use one of the text-color-x variables? (to have it better align with themeing)! also nit, but any chance we can also update the border color to better align?

ok unfort theres no exposed way for us to change the border color until we upgrade the library but i can add that to the TODO!

will update the text color based on theme tho :D

yogurtandjam · 2024-08-16T19:36:10Z

src/lib/__test__/tradingviewUtils.spec.ts

@@ -0,0 +1,26 @@
+import { ResolutionString } from 'public/tradingview/charting_library';


love the tests!
i have a preference for test file names matching the file they are testing, to prevent overlap, duplication, or gaps in testing.
i'd recommend creating a __test__ folder in the lib/tradingView/dydxfeed path, what do you think?

yesss for sure

yogurtandjam

nice work! have some q's and thoughts but nothing major and it looks like you've already gotten a stamp :)

yogurtandjam · 2024-08-16T19:37:00Z

src/lib/orders.ts

@@ -140,3 +142,13 @@ export const getHydratedTradingData = <

 export const getTradeType = (orderType: string) =>
  TRADE_TYPES[orderType as KotlinIrEnumValues<typeof AbacusOrderType>];
+
+export const getAverageFillPrice = (fills: SubaccountFills) => {


can we add a quick test for this?

yogurtandjam · 2024-08-16T19:38:41Z

src/lib/tradingView/dydxfeed/index.ts

@@ -96,6 +116,41 @@ export const getDydxDatafeed = (
    setTimeout(() => onSymbolResolvedCallback(symbolInfo), 0);
  },

+  getMarks: async (
+    symbolInfo: LibrarySymbolInfo,
+    from: number,


can we rename these fromSeconds and toSeconds or some other name that describes denomination

yogurtandjam · 2024-08-16T19:42:04Z

src/lib/tradingView/dydxfeed/utils.ts

+    .toNumber();
+}
+
+export const getMarkForOrderFills = (


[q] i noticed we tested getBarTime but i don't see a test for getMarkForOrderFills in the spec file. is this because the overhead to create all the necessary fixtures is too high?

if so, could we make a ticket + comment it in code as a TODO. If we find that there's a lot of instances where we're unable to test due to lack of features it may be worth actively investing time to remediate this

hm so i personally dont think it's that important to add a unit test here since this is just a function that prepares all the little UI components - probably more interesting to add unit tests separately for each piece (like i did for getBarTime)

yogurtandjam

🔥

tinaszheng added 6 commits August 14, 2024 17:48

feat: add marks based on historical buy/sells

917296f

group order fills by id and show one mark per order

5b7b84d

add localized strings

a35df4e

update styling for mark tooltips

31c726f

add tests

b7d25b3

update marks when there are new fills

bacdd99

tinaszheng requested a review from a team as a code owner August 15, 2024 21:55

tinaszheng commented Aug 15, 2024

View reviewed changes

vercel bot deployed to Preview – v4-staging August 15, 2024 21:57 View deployment

tinaszheng requested review from aforaleka, moo-onthelawn and prashanDYDX August 15, 2024 21:58

vercel bot deployed to Preview – v4-testnet August 15, 2024 22:00 View deployment

use theme colors for mark background

508a417

vercel bot deployed to Preview – v4-testnet August 15, 2024 22:35 View deployment

vercel bot deployed to Preview – v4-staging August 15, 2024 22:38 View deployment

Merge remote-tracking branch 'origin/main' into tina/ct-1055-buy-sell…

898e65c

…-marks

vercel bot deployed to Preview – v4-testnet August 16, 2024 15:10 View deployment

vercel bot deployed to Preview – v4-staging August 16, 2024 15:10 View deployment

reuse average price function

3de43bc

vercel bot deployed to Preview – v4-staging August 16, 2024 15:29 View deployment

vercel bot deployed to Preview – v4-testnet August 16, 2024 15:32 View deployment

moo-onthelawn approved these changes Aug 16, 2024

View reviewed changes

add localization

9e8324a

vercel bot deployed to Preview – v4-testnet August 16, 2024 19:28 View deployment

vercel bot deployed to Preview – v4-staging August 16, 2024 19:30 View deployment

yogurtandjam reviewed Aug 16, 2024

View reviewed changes

refresh widget on theme change

dd8677f

vercel bot deployed to Preview – v4-staging August 16, 2024 20:32 View deployment

vercel bot deployed to Preview – v4-testnet August 16, 2024 20:35 View deployment

tinaszheng added 2 commits August 19, 2024 14:27

use primary text color based on theme

7096900

move test, add test for average fill price

fcbc8b2

vercel bot deployed to Preview – v4-staging August 19, 2024 19:42 View deployment

vercel bot deployed to Preview – v4-testnet August 19, 2024 19:42 View deployment

moo-onthelawn approved these changes Aug 19, 2024

View reviewed changes

yogurtandjam approved these changes Aug 19, 2024

View reviewed changes

tinaszheng merged commit b1e5697 into main Aug 19, 2024
8 checks passed

tinaszheng deleted the tina/ct-1055-buy-sell-marks branch August 19, 2024 20:28

yujin512 pushed a commit that referenced this pull request Aug 26, 2024

feat: add buys and sells as marks on tv chart (#927)

083a58f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add buys and sells as marks on tv chart #927

feat: add buys and sells as marks on tv chart #927

tinaszheng commented Aug 15, 2024 •

edited

Loading

linear bot commented Aug 15, 2024

vercel bot commented Aug 15, 2024 •

edited

Loading

tinaszheng Aug 15, 2024

moo-onthelawn left a comment

moo-onthelawn Aug 16, 2024

yogurtandjam Aug 16, 2024

tinaszheng Aug 19, 2024

yogurtandjam Aug 19, 2024

moo-onthelawn Aug 16, 2024

moo-onthelawn Aug 16, 2024

tinaszheng Aug 16, 2024

moo-onthelawn Aug 16, 2024

moo-onthelawn Aug 16, 2024

moo-onthelawn Aug 16, 2024

tinaszheng Aug 16, 2024

moo-onthelawn Aug 16, 2024

tinaszheng Aug 19, 2024

moo-onthelawn Aug 16, 2024

tinaszheng Aug 19, 2024

yogurtandjam Aug 16, 2024

tinaszheng Aug 19, 2024

yogurtandjam left a comment

yogurtandjam Aug 16, 2024

yogurtandjam Aug 16, 2024

yogurtandjam Aug 16, 2024

tinaszheng Aug 19, 2024

yogurtandjam left a comment

		@@ -0,0 +1,26 @@
		import { ResolutionString } from 'public/tradingview/charting_library';

feat: add buys and sells as marks on tv chart #927

feat: add buys and sells as marks on tv chart #927

Conversation

tinaszheng commented Aug 15, 2024 • edited Loading

Testing plan

linear bot commented Aug 15, 2024

vercel bot commented Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

moo-onthelawn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogurtandjam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yogurtandjam left a comment

Choose a reason for hiding this comment

tinaszheng commented Aug 15, 2024 •

edited

Loading

vercel bot commented Aug 15, 2024 •

edited

Loading