Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Integrate the cow-sdk into the Home Page widgets #77

Merged

Conversation

ramirotw
Copy link
Contributor

@ramirotw ramirotw commented May 2, 2022

Summary

Closes #40

This integrates the Volume chart and the Summary cards to consume data provided by the cow-subgraph through the cow-sdk

@github-actions
Copy link

github-actions bot commented May 2, 2022

Copy link
Contributor

@alongoni alongoni left a comment

Choose a reason for hiding this comment

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

Works fine!
The line on this graph should be green.
image

@ramirotw ramirotw requested a review from alfetopito May 2, 2022 15:16
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

It's looking awesome!

Added a few comments

Comment on lines +189 to +192
return format(fromUnixTime(crossHairData.time as UTCTimestamp), 'MMM d HH:mm, yyyy')
}

return format(fromUnixTime(crossHairData.time as UTCTimestamp), 'MMM d, yyyy')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these date formats locale aware?

@elena-zh
Copy link

elena-zh commented May 3, 2022

Hey @ramirotw , great job!!
However, some issues/questions:

  1. The chart shows 0 for all periods in Rinkeby, plus no stats for 24h Fees:
    image

  2. Just like an enhancement: I think, it would be nice to shoe the chart in grey if it has no progress in comparison with the previous period (0%)

  3. I noticed, that Last batch card can navigate to a not existing batch ID for the current environment. I can assume, that the last batch run for the prod API, and Barn API can't show it. (see the video: https://watch.screencastify.com/v/KI1T3g7Xm2nbH3785XR7
    So, can we separate somehow volume chart/cars info for these environments and show an appropriate stats for each of them?

  4. I noticed that the volume chart is incorreclty colored in the Mainnet for several periods:

  • 1 week: it is in green, it shows +75%, but the chart itself goes down in the end
    1 week in red

  • 1 month: the same
    month

Thanks

@ramirotw
Copy link
Contributor Author

ramirotw commented May 3, 2022

  1. The chart shows 0 for all periods in Rinkeby, plus no stats for 24h Fees:
    image

This is because there are no price references for tokens on Rinkeby, so it will always be 0.

2. Just like an enhancement: I think, it would be nice to shoe the chart in grey if it has no progress in comparison with the previous period (0%)

I'll point something below about this so we all are on the same page.

3. I noticed, that Last batch card can navigate to a not existing batch ID for the current environment. I can assume, that the last batch run for the prod API, and Barn API can't show it. (see the video: https://watch.screencastify.com/v/KI1T3g7Xm2nbH3785XR7
So, can we separate somehow volume chart/cars info for these environments and show an appropriate stats for each of them?

As we discussed on the call, lets wait for the backend to have an answer about this and in the case we need to merge the responses on the client we can create a ticket for it.

4. I noticed that the volume chart is incorreclty colored in the Mainnet for several periods:

  • 1 week: it is in green, it shows +75%, but the chart itself goes down in the end
    1 week in red
  • 1 month: the same
    month

This is something that is bringing some confusion and I want be sure we all understand how it's supposed to work. As it is implemented right now we have 3 different values:

image

  1. Current period average
  2. Diff in % from previous period
  3. First volume value from current period
  4. Last volume value from current period

Now based on those values we have colors applied to labels and the chart itself:

  • The label for the % diff from previous period is green when it's >0 and red when <0
  • The chart should (now it's not) be green if the last chart value (4) is bigger than the first value (3)
  • The chart should be red if the last chart value (4) is smaller than the first value (3)

So here we could have something that is confusing at first glance: we can have a green % value from the previous period (2) and a red chart because the trend was downside.

Does that makes sense? @elena-zh @alfetopito @henrypalacios @alongoni

@alfetopito
Copy link
Collaborator

Thanks for the explanation Ramiro.

I'm not that well versed into this kind of graph, but if I'm not mistaken I've seen a graph where the color is applied to the sections of the graph above/below the 3 opening line.

So anything above 3 is green, anything below that is red.

Again, not a strong opinion, just throwing out ideas. Not even sure this graph lib is capable of doing that.

@elena-zh
Copy link

elena-zh commented May 4, 2022

@ramirotw , thank you for the explanation!
Frankly, I thought that Diff in % from previous period is related to the deviation for the chart volume (deference between 3 and 4), so it was confusing to me to see different colors.
So in my understanding it was this way:

  1. Accumulated volume for a period (1 week, as an example)
  2. Deviation % in comparison with the 1st value in the period
  3. Start value (volume) of the period.
  • Then the chart shows an accumulated volume of trades within a period, and goes up or down
  1. Last value, that is equal to the 1.
  • the graph is in red when 2 is negative
  • the graph is in green, when 2 is positive

However, I do not have a strong opinion about it.

Btw, if we read the graph values so differently, maybe we could add some kind of tooltips to each value that will explain what this or that value means? 🧐
image

@ramirotw
Copy link
Contributor Author

ramirotw commented May 4, 2022

@ramirotw , thank you for the explanation! Frankly, I thought that Diff in % from previous period is related to the deviation for the chart volume (deference between 3 and 4), so it was confusing to me to see different colors. So in my understanding it was this way:

  1. Accumulated volume for a period (1 week, as an example)
  2. Deviation % in comparison with the 1st value in the period
  3. Start value (volume) of the period.
  • Then the chart shows an accumulated volume of trades within a period, and goes up or down
  1. Last value, that is equal to the 1.
  • the graph is in red when 2 is negative
  • the graph is in green, when 2 is positive

However, I do not have a strong opinion about it.

Btw, if we read the graph values so differently, maybe we could add some kind of tooltips to each value that will explain what this or that value means? 🧐 image

Thanks Elena!

So I see two different ways of looking at the chart:

  1. Accumulated volume or average. Here I'm more inclined towards accumulated volume.
  2. As you said, it's the deviation in %

3 and 4: Whether the chart should display accumulated volume or not. Here I'm inclined towards not accumulating the volume. In the Cow Protocol Dune's dashboard https://dune.com/gnosis.protocol/Gnosis-Protocol-V2 we have two different charts to show daily volume:

  • This is the current chart behavior

image

  • Accumulated as you suggested

image

@anxolin what are your thoughts?

@ramirotw
Copy link
Contributor Author

ramirotw commented May 4, 2022

From today's call:

Volume chart:

  1. Show the accumulated volume as the main label instead of the average as it is right now
  2. Update the current CoW Protocol volume to reflect the selected period, i.e. Daily volume, Monthly volume, etc
  3. Add a Total Volume card
  4. Add a tooltip with the selected volume and date values
  5. Make the volume chart bigger so the tooltip is correctly displayed. Make it occupy the whole cards width
  6. Remove the green/red chart colors which causes confusion. Use a neutral color, the main orange is a good option
  7. Remove the current x-axis at the current volume value
  8. Add the x-axis to the hover state, keep the y-axis too

Last Batch card:

  1. We will need to implement the logic to handle the cases where the batches ids from the subgraph don't exist on the Barn API, either by performing redirects or by merging the API responses. Will create a ticket for this and define it better.

@anxolin @alfetopito @elena-zh

@ramirotw
Copy link
Contributor Author

ramirotw commented May 4, 2022

  1. Show the accumulated volume as the main label instead of the average as it is right now

Since this is the sdk integration PR I'll only update the main value to show the accumulated volume. The remaining bullets will be worked on different PRs so they are more focused.

@elena-zh
Copy link

elena-zh commented May 5, 2022

@ramirotw , will you create separate tasks for all the rest points?
Otherwise I can create a 1 separate task for all of them

@ramirotw
Copy link
Contributor Author

ramirotw commented May 5, 2022

@ramirotw , will you create separate tasks for all the rest points?
Otherwise I can create a 1 separate task for all of them

Go ahead @elena-zh and create 1 for everything but the Last Batch card issue that I need to write a description

@ramirotw ramirotw merged commit 4e2b5ae into 35-epic-home-page May 5, 2022
@ramirotw ramirotw deleted the ramirotw/issue-40-integrate-sdk-explorer branch May 5, 2022 15:17
@elena-zh elena-zh mentioned this pull request May 6, 2022
8 tasks
@elena-zh
Copy link

elena-zh commented May 6, 2022

@ramirotw , here is the new task #87

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants