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

Integrate Tokens table data from the subgraph #98

Merged
merged 49 commits into from
Jun 10, 2022

Conversation

ramirotw
Copy link
Contributor

@ramirotw ramirotw commented May 20, 2022

Summary

image

@github-actions
Copy link

@ramirotw ramirotw requested a review from elena-zh May 20, 2022 13:26
@ramirotw
Copy link
Contributor Author

@elena-zh FYI the Gnosis Chain tokens data is broken. @GabrielCamba pushed the updated changes for the mainnet and gc subgraphs but only the gc subgraph finished indexing, I need to push the updates to fix the queries but I'll do it once the mainnet subgraph is done.

@elena-zh
Copy link

@ramirotw , I have left a few notes in the #93 (comment) PR, so the issues I found there are also related to the current PR.

In addition, I have noticed that some data cannot be loaded here
image

And for 1 token I noticed '-infinity%' for rETH
image
So it would be nice to change the percentage to '0' of '-' (dash) when it shows 'infinity'

Then, I noticed that 0 price can have different amount of decimals (Rinkeby)
image

Thanks!

src/components/token/TokenTable/index.tsx Outdated Show resolved Hide resolved
src/components/token/TokenTable/index.tsx Outdated Show resolved Hide resolved
@ramirotw
Copy link
Contributor Author

@alfetopito we have a problem:

image

Context: With the introduction of the ipfs-only-hash lib in cowprotocol/cow-sdk@0708de8#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R34 there's now an incompatibility of node versions as a dependency of ipfs-only-hash expects node to be >= 14. I have tested everything locally and the only thing I needed to bump because it was causing issues was favicons-webpack-plugin, the rest worked fine. Maybe this PR is a good moment to test moving to node 16.

@alfetopito
Copy link
Collaborator

@alfetopito we have a problem:

image

Context: With the introduction of the ipfs-only-hash lib in cowprotocol/cow-sdk@0708de8#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R34 there's now an incompatibility of node versions as a dependency of ipfs-only-hash expects node to be >= 14. I have tested everything locally and the only thing I needed to bump because it was causing issues was favicons-webpack-plugin, the rest worked fine. Maybe this PR is a good moment to test moving to node 16.

Indeed, I see no reason to stay on node 14.
Likely it made sense when it was introduced, but shouldn't be needed to stay on it anymore

@ramirotw
Copy link
Contributor Author

ramirotw commented Jun 8, 2022

Thanks @elena-zh for the feedback!

The dash is the absence of data from the subgraph.

2. I wonder what's the difference between '0%' and '-' in the table's values?
image

Same as above, the dash is the absence of data. The chart uses 7 data points max, but if we have less data points than that, we still show the chart, hence different charts with different sizes.

6. Again, what is the difference in the 'dash' and in the horizontal graph if everywhere 0-s are displayed? Also, I see graphs have a different size
difference

@ramirotw
Copy link
Contributor Author

ramirotw commented Jun 9, 2022

@elena-zh please take a look, all but the Rinkeby stuff should be good now.

@henrypalacios
Copy link
Contributor

@ramirotw removing the row selector was intentional?

Selection_575

@ramirotw
Copy link
Contributor Author

ramirotw commented Jun 9, 2022

@ramirotw removing the row selector was intentional?

Selection_575

no, probably when fixing something related to the pagination it got hidden/removed. Good catch

@elena-zh
Copy link

elena-zh commented Jun 9, 2022

Hey @ramirotw , the recent changes look great to me!
I have created #115 issue for the 8th point related to Rinkeby network.

However, I think we need to fix the 9th issue with no token name and no token icon for a native currency, as users are able to but ETH/XDAI in CowSwap.
image

A new tiny issue is to write 'Volume' from a lower-case letter
image

And might be a nitpick, but it would be nice to scroll to the table top when flip pages in a mobile view.

Also, it would great to address @henrypalacios 's issue reported above.

Thanks!

@elena-zh
Copy link

Hey @ramirotw , great job!
I have created a separate enhancement for the scroll to top action in a mobile view cowprotocol/cowswap#3729 .

Also, it would be great to fix 'Total volume' column name in a desktop view ('volume' should start with a lowercase letter)
image

@elena-zh
Copy link

I have also found an issue that is related to the difference in data in BARN and PROD APIs. I assume, end users can face it, so I reported it: #118
@ramirotw , @alfetopito , please review it and let me know if we need to fix or close it. Thanks

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Approved now!

@ramirotw ramirotw merged commit 68113df into 35-epic-home-page Jun 10, 2022
@ramirotw ramirotw deleted the 39/tokens-table-subgraph branch June 10, 2022 15:08
@henrypalacios henrypalacios mentioned this pull request Jun 23, 2022
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.

6 participants