Add book depth chart #219

KoalaSat · 2022-08-25T21:18:52Z

Fixes #134

Max Zoom

Lines disappears after over passing Y Axis

Cross-hair

Mobile

Y Axis

const calculateBtc = (order: any): number => {
  const amount = parseInt(order.amount || order.max_amount)
  return amount / order.price
}

X Axis

order.premium

KoalaSat · 2022-08-26T07:29:26Z

frontend/src/components/Charts/DepthChart/index.tsx

@@ -0,0 +1,172 @@
+import React, { useEffect, useState } from "react"


It's a good practice in React to always create index.ts|tsx files, let me know what do you think

KoalaSat · 2022-08-26T07:31:45Z

frontend/src/components/Charts/nivoScheme.ts

@@ -0,0 +1,29 @@
+import { Theme } from "@nivo/core"
+


This is intended to be the default shared theme for future Nivo charts

* Add WebLN support * Fix Variable Typo * Invoice Generation Signed-off-by: KoalaSat <111684255+KoalaSat@users.noreply.github.com> * Code Review * Second CR * Catch cancelations * Final Review Signed-off-by: KoalaSat <111684255+KoalaSat@users.noreply.github.com>

Reckless-Satoshi

This is perfect and really fun to use! Congratulations I am sure Robots will enjoy this feature. I certainly do 😄

The chart theme is not sensitive to a switch between dark and light theme on the RoboSats app. The axes and numbers can only be seen with the dark theme (therefore not visible by default in Tor Browser). The lines themselves would also look prettier on the light theme if they were darker.
I am unsure on how the (-) and (+) buttons are supposed to work. These are zoom-in and zoom-out buttons, but at the moment they do not seem to be specially useful, do you envision them being useful at some point?
Given that the last day premium is located on the center as if it was the chart title, one instantly think that it is also the center of the Depth Chart, and that - and + will move you up and down in the X axis.
I see finally you have left the X axis only in premium units. What happened to the X axes priced in Fiat units? :D Both are potentially very useful and informative. In fact, the fiat unit chosen for display could be the one in this.props.currency, to not leave behind those users who do not price life in USD.

All in all I think the chart might not need to display the last day average premium (or at least, not so prominently). What do you think?

Hypothetical cool things that could be done:

On desktop, hovering the mouse shows a crosshair and the points can be "clicked". I am not sure if this is even possible, since the chart is aggregating the height of many orders... Is there a way we can overlay a small Tooltip with a little info about the order that matches that premium and has the highest volume on mouse hovering (E.g. RobotName, FiatAmount, Currency and Payment Method Icons)? That would be really top!!

Reckless-Satoshi · 2022-08-26T14:22:20Z

frontend/src/components/Charts/nivoScheme.ts

+  axis: {
+    ticks: {
+      text: {
+        fill: "rgb(255, 255, 255)"


Would be great if these variables can be dynamically set to match the colors defined in the MUI theme in App.js . It is provided as context to all children. https://github.com/Reckless-Satoshi/robosats/blob/8eda1dd20fec576726cb1578e263bfb43472be48/frontend/src/components/App.js#L26-L42
This would solve the conflict we have now with the light/dark themes as well as make it future-proof for any possible theme change.

Take a look at MUI Theming and the use of the hook useTheme() https://mui.com/material-ui/customization/theming/

Reckless-Satoshi · 2022-08-26T14:45:04Z

frontend/src/components/BookPage.js

+                <>
+                  <Button variant="contained" color='primary' to='/make/' component={Link}>{t("Make Order")}</Button>
+                  <Button color='inherit' style={{color: '#111111'}} onClick={this.handleClickView}>
+                    { this.state.view == 'depth' ? t("List") : t("Depth") }


What do you think of using "Chart" instead of "Depth"?
We could also add a small icon on this button so it is highlighted. For example:

// Chart Button import BarChartIcon from '@mui/icons-material/BarChart'; // List Button import FormatListBulletedIcon from '@mui/icons-material/FormatListBulleted';

KoalaSat · 2022-08-27T08:26:48Z

I am unsure on how the (-) and (+) buttons are supposed to work. These are zoom-in and zoom-out buttons, but at the moment they do not seem to be specially useful, do you envision them being useful at some point?

I just thought + and - represent the range, but you are right I checked other exchanges and it's clearly a zoom

Given that the last day premium is located on the center as if it was the chart title, one instantly think that it is also the center of the Depth Chart, and that - and + will move you up and down in the X axis.
All in all I think the chart might not need to display the last day average premium (or at least, not so prominently). What do you think?

The point is, that value is the one that determines de exact middle X point on the chart, Y can hide it but I see it useful to easily understand where the chart is located.

On desktop, hovering the mouse shows a crosshair and the points can be "clicked". I am not sure if this is even possible, since the chart is aggregating the height of many orders... Is there a way we can overlay a small Tooltip with a little info about the order that matches that premium and has the highest volume on mouse hovering (E.g. RobotName, FiatAmount, Currency and Payment Method Icons)? That would be really top!!

I think with the magic of Nivo this should be easy peasy

KoalaSat · 2022-08-27T08:31:51Z

I see finally you have left the X axis only in premium units. What happened to the X axes priced in Fiat units? :D Both are potentially very useful and informative. In fact, the fiat unit chosen for display could be the one in this.props.currency, to not leave behind those users who do not price life in USD.

I understood the idea was just show, the premium, no problem I have that piece of code I can recover it

Reckless-Satoshi · 2022-08-27T08:43:29Z

The point is, that value is the one that determines de exact middle X point on the chart, Y can hide it but I see it useful to easily understand where the chart is located.

Ah, I see! Maybe we can instead mark the "last 24h premium" as a dotted vertical line in the chart and label it inside the chart? It would be clearer in my opinion. Centered texts just above a chart usually are the title.

KoalaSat · 2022-08-27T11:55:21Z

The point is, that value is the one that determines de exact middle X point on the chart, Y can hide it but I see it useful to easily understand where the chart is located.

Ah, I see! Maybe we can instead mark the "last 24h premium" as a dotted vertical line in the chart and label it inside the chart? It would be clearer in my opinion. Centered texts just above a chart usually are the title.

That was my original idea but there is no way to make it appear 🥲 the documentation clearly allows it and it had an example but after trying over and over I found no way, I'll give another try with a workaround I don't like but maybe it'll make it work

KoalaSat · 2022-08-27T15:51:28Z

New Version looks cooler

X Axis selector

When the selection is Price, the amount is calculated over the base currency selected (USD if ANY)
The Price middle point is the median of all the converted amounts

Tooltip

As suggested, I included a too-tip per point, the criteria to show one point or another is based on a boronoi map generated by Nivo. Clicking on the chart when a tool-tip is being displayed pushes the user to the order page.

IMPORTANT Because I wanted to reuse the avatar code, I ended up refactoring a little bit the columns arrays on the book orders list, I hope that's fine, there was a couple of unnecessary loops, another unnecessary serialization and a lot of duplicated/unused code. Right now I think it looks cleaner :)

Light/Dark Mode

A function returns one theme or another base on Mui's theme, I decided to move the entire object in case in the future we want to reuse colors.

Vertical middle line

I didn't manage to make it work 😅

KoalaSat · 2022-08-27T15:52:49Z

frontend/src/components/BookPage.js

-              payment_method: order.payment_method,
-              price: order.price,
-              premium: order.premium,
-            })


I consider this serialization unnecessary, we already have an book object with all the information, I see no point on just changing names and confuse future developers

KoalaSat · 2022-08-27T15:53:37Z

frontend/src/components/BookPage.js

-            )} },
-          ]}
+            )} 
+          }]}


In another iteration I see this array reusable for both tables, mobile and desktop

Reckless-Satoshi

This chart is soo much fun to use and look at! To me is already looking ready for merge. I requested a few tiny changes.

A couple of things we also might want to check:

Switching the currency on the drop down menu does change the currency code shown on the chart, but does not change the tick labels of the X axis (They remain in USD)
In mobile, when using Price in the X axis, the ticks of the X label become unreadable as they are too close to each other. Maybe the number of ticks has to be decimated.
When hovering the mouse, for a fraction of a second, the tooltips show below the pointer, then on top. No issue whatsoever, yet when you are looking at tooltips on the bottom of the chart, the bigger size makes the vertical and horizontal scroll bars show. Just a tiny detail that might be too hard to fix to bother though...

(I was able to take this screenshot, but it lasts for a fraction of a second only)

All in all congrats! This is a feature robots will love 🚀

Reckless-Satoshi · 2022-08-28T02:26:48Z

frontend/src/components/Charts/DepthChart/index.tsx

+
+  return bookLoading || !center || enrichedOrders.length < 1 ? (
+    <div style={{display: "flex", justifyContent: "center", paddingTop: 200, height: 420 }}>
+      <CircularProgress />


The chart feels sluggish to load if you switch a few times between List/Chart repeatedly. I believe this is because every single time the chart is loaded, there is a call to "/limits/" that has to be waited.

Do you think we could save /limits/ into the app state and from that point on only update that variable? (same as we do with the book orders)

Good catch! I change it and now it only loads once :)

Reckless-Satoshi · 2022-08-28T02:29:47Z

frontend/src/components/Charts/DepthChart/index.tsx

+                  <PaymentText 
+                    othersText={t("Others")} 
+                    verbose={true} 
+                    size={12} 


I would maybe use size 20 or 22 for better visibility. Any cons to this bigger size?

Reckless-Satoshi · 2022-08-28T02:34:03Z

frontend/src/components/Charts/DepthChart/index.tsx

+          xFormat={(value) => Number(value).toFixed(0)}
+          lineWidth={3}
+          theme={getNivoScheme(theme)}
+          colors={['rgb(136, 252, 102)', 'rgb(255, 108, 57)']}


While orange and green is widely used for depth charts, we could give it a touch by sticking to the RoboSats color scheme. Throughout the UI we are loosely sticking to using purple for "things you click if you want to sell" and blue for "things you click on if you want to buy".

What do you think of this?

colors={[theme.palette.secondary.main,theme.palette.primary.main]}

These are the color references for the default MUI palette https://mui.com/material-ui/customization/palette/
When using these theme variables, the line colors will be brighter when the dark mode is enabled.

KoalaSat · 2022-08-28T10:47:11Z

When hovering the mouse, for a fraction of a second, the tooltips show below the pointer, then on top. No issue whatsoever, yet when you are looking at tooltips on the bottom of the chart, the bigger size makes the vertical and horizontal scroll bars show. Just a tiny detail that might be too hard to fix to bother though...

looks like something on Nivo, it only happens for the first tool tip that loads once the mouse hovers the charts, looks like the default position, which quickly gets the first configuration and changes, I don't see any way to change it from outside the library.

KoalaSat · 2022-08-28T13:22:42Z

Good news, I managed to generate the middle line 😄

I think I already covered all your comments @Reckless-Satoshi

KoalaSat · 2022-08-28T13:26:08Z

BTW this is my proposal to avoid overlapping on mobile view

Reckless-Satoshi · 2022-08-28T18:10:32Z

It's looking great!

I will take a more deep read at the code to understand myself everything that has been introduced before merging. So it might take 1-2 days until this PR is merge.

Please, share a LN invoice for 300K with a long expiration time and from a proxy wallet.

KoalaSat · 2022-08-28T18:13:44Z

I will take a more deep read at the code to understand myself everything that has been introduced before merging. So it might take 1-2 days until this PR is merge.

Take your time, any clarification or modification request will be welcome

KoalaSat · 2022-08-28T18:14:29Z

Please, share a LN invoice for 300K with a long expiration time and from a proxy wallet.

lnbc3m1p3shvrlpp5wtptd958lwqwekgrdctyf5geuzwu6j380wwqcpppqj0dhu2h2xesdq5g9kxy7fqd9h8vmmfvdjscqzpgxqyz5vqsp55mhyf3g3vadlnkcpuuamanlmc2npglwx5kr2dcqjslmdeunar73s9qyyssq3l20e72s82y6nlvfx2h6lp4cwwf64frslk8x928yvsqwyr0y66dkt389xku0tr7z26t8n4qqgqwuamu7phcnjyx4e73xez2u4y255kqqppql8a

KoalaSat · 2022-08-28T18:23:01Z

frontend/src/components/Charts/DepthChart/index.tsx

+  const calculateBtc = (order: Order): number => {
+    const amount = parseInt(order.amount) || order.max_amount
+    return amount / order.price
+  }


This calculus might potentially be included on the back-end response, I can even do it myself if that sounds good, I wasn't sure if there is any politics for including new attributes on the API

It can be added to the API. It is still very fluid.

I was thinking on creating a separate PR after we merge this one so we don't overcharge it, what do you think?

Yes, let's do that on a different PR. Take a look at the model Order and the var order.last_satoshis, it keeps a snapshot of the size of the order in satoshis when it is created. Maybe it can be added as one of the response variable to the Book List API view. Is this enough or do we need it to be realtime ?

It has to be real time, consider everyone creates right now their orders and this night Bitcoin pumps up 20%, the amount of BTC you'll be able to buy tomorrow morning is going to be 20% more than the real quantity.

I found these 2:

https://github.com/Reckless-Satoshi/robosats/blob/eff58dc91d93fe98a6738e0e3898ed2a476d2ebc/api/models.py#L382
https://github.com/Reckless-Satoshi/robosats/blob/eff58dc91d93fe98a6738e0e3898ed2a476d2ebc/api/models.py#L390

but the only logic for t0_satoshis is this:

https://github.com/Reckless-Satoshi/robosats/blob/eff58dc91d93fe98a6738e0e3898ed2a476d2ebc/api/views.py#L146

Looks like there was an original intention to keep the sats on creation and the sats right now, that's cool, but at some point it got lost and last_satoshis was used as t0_satoshis.

I like the original idea, I had a look and there are not too many places where last_satoshis is being used

https://github.com/Reckless-Satoshi/robosats/search?q=last_satoshis

do you believe it's a good idea to try a renaming so we use t0_satoshis everywhere and start calculating last_satoshis on every request?

I think I'll move this conversation to an issue I can assign to myself so it doesn't get lost into this PR

KoalaSat · 2022-08-28T18:57:26Z

frontend/src/components/Charts/DepthChart/index.tsx

+          data={series} 
+          enableArea={true}
+          useMesh={true}
+          animate={false}


In general, the animation feature is cool, but on this case the X axis scale changes so drastically (from premium to price) that it really looks odd.

Reckless-Satoshi · 2022-08-28T21:19:06Z

Please, share a LN invoice for 300K with a long expiration time and from a proxy wallet.

lnbc3m1p3shvrlpp5wtptd958lwqwekgrdctyf5geuzwu6j380wwqcpppqj0dhu2h2xesdq5g9kxy7fqd9h8vmmfvdjscqzpgxqyz5vqsp55mhyf3g3vadlnkcpuuamanlmc2npglwx5kr2dcqjslmdeunar73s9qyyssq3l20e72s82y6nlvfx2h6lp4cwwf64frslk8x928yvsqwyr0y66dkt389xku0tr7z26t8n4qqgqwuamu7phcnjyx4e73xez2u4y255kqqppql8a

3037303861373334363436653765376562316364343532383730623833633163

(Funny, Alby wallet preimage HEX is always made only with decimal numbers!)

KoalaSat · 2022-08-28T21:26:04Z

Please, share a LN invoice for 300K with a long expiration time and from a proxy wallet.

lnbc3m1p3shvrlpp5wtptd958lwqwekgrdctyf5geuzwu6j380wwqcpppqj0dhu2h2xesdq5g9kxy7fqd9h8vmmfvdjscqzpgxqyz5vqsp55mhyf3g3vadlnkcpuuamanlmc2npglwx5kr2dcqjslmdeunar73s9qyyssq3l20e72s82y6nlvfx2h6lp4cwwf64frslk8x928yvsqwyr0y66dkt389xku0tr7z26t8n4qqgqwuamu7phcnjyx4e73xez2u4y255kqqppql8a

3037303861373334363436653765376562316364343532383730623833633163

(Funny, Alby wallet preimage HEX is always made only with decimal numbers!)

I was also wondering what were the odds the first time 😄

* Amount X Axis, Avatars and refactor * Theme and performance improvements * Remove duplicated tooltips * Code Review * Marker Theme color * Missing end lines Signed-off-by: KoalaSat <111684255+KoalaSat@users.noreply.github.com>

Reckless-Satoshi · 2022-09-02T05:35:21Z

frontend/src/utils/prettyNumbers.test.ts

+describe("amountToString", () => {
+  test("pn()", () => {
+    [
+      {input: null, output: undefined},


@KoalaSat just a heads up. After merging this PR the frontend build GH action was failing because it did not pass this test. I amended this test in the main branch.

KoalaSat commented Aug 26, 2022

View reviewed changes

KoalaSat marked this pull request as ready for review August 26, 2022 07:31

KoalaSat force-pushed the add-book-depth-chart branch 4 times, most recently from 5c1ef91 to 496f6fc Compare August 26, 2022 08:49

KoalaSat force-pushed the add-book-depth-chart branch from 496f6fc to 656328e Compare August 26, 2022 08:50

Reckless-Satoshi self-requested a review August 26, 2022 13:58

Reckless-Satoshi reviewed Aug 26, 2022

View reviewed changes

Reckless-Satoshi force-pushed the main branch 4 times, most recently from b6087ab to 7fb2767 Compare August 26, 2022 17:58

Amount X Axis, Avatars and refactor

cbe6f75

KoalaSat force-pushed the add-book-depth-chart branch from da2abaa to 19c04c1 Compare August 27, 2022 15:44

KoalaSat commented Aug 27, 2022

View reviewed changes

Theme and performance improvements

bf054de

KoalaSat force-pushed the add-book-depth-chart branch from 19c04c1 to bf054de Compare August 27, 2022 15:59

Remove duplicated tooltips

cbfccf5

Reckless-Satoshi requested changes Aug 28, 2022

View reviewed changes

Code Review

1b6ea99

KoalaSat force-pushed the add-book-depth-chart branch from ef252f9 to 1b6ea99 Compare August 28, 2022 13:25

KoalaSat requested a review from Reckless-Satoshi August 28, 2022 13:27

KoalaSat added 2 commits August 28, 2022 15:49

Marker Theme color

3168a94

Missing end lines

ec6a74c

KoalaSat commented Aug 28, 2022

View reviewed changes

Reckless-Satoshi approved these changes Aug 28, 2022

View reviewed changes

Reckless-Satoshi force-pushed the main branch 4 times, most recently from ff3e913 to e613cd9 Compare August 28, 2022 22:23

Reckless-Satoshi merged commit 458f5ac into RoboSats:main Aug 30, 2022

KoalaSat mentioned this pull request Sep 1, 2022

Move the most recent value in sats of an order calculation to the backend #224

Closed

Reckless-Satoshi reviewed Sep 2, 2022

View reviewed changes

Reckless-Satoshi mentioned this pull request Sep 8, 2022

Fix dependencies #231

Merged

KoalaSat deleted the add-book-depth-chart branch September 25, 2022 12:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add book depth chart #219

Add book depth chart #219

KoalaSat commented Aug 25, 2022 •

edited

KoalaSat Aug 26, 2022 •

edited

KoalaSat Aug 26, 2022

Reckless-Satoshi left a comment •

edited

Reckless-Satoshi Aug 26, 2022

Reckless-Satoshi Aug 26, 2022

KoalaSat commented Aug 27, 2022

KoalaSat commented Aug 27, 2022

Reckless-Satoshi commented Aug 27, 2022 •

edited

KoalaSat commented Aug 27, 2022 •

edited

KoalaSat commented Aug 27, 2022 •

edited

KoalaSat Aug 27, 2022

KoalaSat Aug 27, 2022 •

edited

Reckless-Satoshi left a comment

Reckless-Satoshi Aug 28, 2022

KoalaSat Aug 28, 2022

Reckless-Satoshi Aug 28, 2022

Reckless-Satoshi Aug 28, 2022

KoalaSat commented Aug 28, 2022

KoalaSat commented Aug 28, 2022 •

edited

KoalaSat commented Aug 28, 2022 •

edited

Reckless-Satoshi commented Aug 28, 2022 •

edited

KoalaSat commented Aug 28, 2022

KoalaSat commented Aug 28, 2022

KoalaSat Aug 28, 2022 •

edited

Reckless-Satoshi Aug 28, 2022

KoalaSat Aug 28, 2022

Reckless-Satoshi Aug 30, 2022 •

edited

KoalaSat Aug 30, 2022 •

edited

KoalaSat Aug 31, 2022 •

edited

KoalaSat Sep 1, 2022

KoalaSat Aug 28, 2022

Reckless-Satoshi commented Aug 28, 2022

KoalaSat commented Aug 28, 2022

Reckless-Satoshi Sep 2, 2022

		@@ -0,0 +1,172 @@
		import React, { useEffect, useState } from "react"

Add book depth chart #219

Add book depth chart #219

Conversation

KoalaSat commented Aug 25, 2022 • edited

Max Zoom

Cross-hair

Mobile

Y Axis

X Axis

KoalaSat Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reckless-Satoshi left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KoalaSat commented Aug 27, 2022

KoalaSat commented Aug 27, 2022

Reckless-Satoshi commented Aug 27, 2022 • edited

KoalaSat commented Aug 27, 2022 • edited

KoalaSat commented Aug 27, 2022 • edited

X Axis selector

Tooltip

Light/Dark Mode

Vertical middle line

Choose a reason for hiding this comment

KoalaSat Aug 27, 2022 • edited

Choose a reason for hiding this comment

Reckless-Satoshi 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

KoalaSat commented Aug 28, 2022

KoalaSat commented Aug 28, 2022 • edited

KoalaSat commented Aug 28, 2022 • edited

Reckless-Satoshi commented Aug 28, 2022 • edited

KoalaSat commented Aug 28, 2022

KoalaSat commented Aug 28, 2022

KoalaSat Aug 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reckless-Satoshi Aug 30, 2022 • edited

Choose a reason for hiding this comment

KoalaSat Aug 30, 2022 • edited

Choose a reason for hiding this comment

KoalaSat Aug 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reckless-Satoshi commented Aug 28, 2022

KoalaSat commented Aug 28, 2022

Choose a reason for hiding this comment

KoalaSat commented Aug 25, 2022 •

edited

KoalaSat Aug 26, 2022 •

edited

Reckless-Satoshi left a comment •

edited

Reckless-Satoshi commented Aug 27, 2022 •

edited

KoalaSat commented Aug 27, 2022 •

edited

KoalaSat commented Aug 27, 2022 •

edited

KoalaSat Aug 27, 2022 •

edited

KoalaSat commented Aug 28, 2022 •

edited

KoalaSat commented Aug 28, 2022 •

edited

Reckless-Satoshi commented Aug 28, 2022 •

edited

KoalaSat Aug 28, 2022 •

edited

Reckless-Satoshi Aug 30, 2022 •

edited

KoalaSat Aug 30, 2022 •

edited

KoalaSat Aug 31, 2022 •

edited