Show token prices #1382

mudrila · 2024-02-28T00:06:21Z

Description

This PR implements fetching coin prices from CoinGecko.
The implementation is as dumb as you could only imagine. Probably we could save some requests to putting returned value of useFormatCoins into global context - but that might lead us into unnecessary implementation over-engineering (given the fact that we could be migrating to the Safe API Kit once they'll release pricing service anyway).

Notes

Issue

Closes #1362

Testing

Visit some mainnet DAO with bunch of assets
Verify that prices for token and total USD is calculated properly

Screenshots (if applicable)

netlify · 2024-02-28T00:06:37Z

✅ Deploy Preview for fractal-framework-dev ready!

Name	Link
🔨 Latest commit	`daf34df`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-framework-dev/deploys/65ea2aa7e5b72e0008590138
😎 Deploy Preview	https://deploy-preview-1382.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Da-Colon · 2024-02-28T14:54:37Z

Not quite working. Went to Decent DAO on mainnet and it is showing only $9,708.66

adamgall · 2024-02-28T15:34:23Z

Seeing that this implementation contained a CoinGecko API key surprised me, frankly.

I suppose it's been a minute (~2 years) since I last implemented a CoinGecko powered pricing mechanism in a frontend app. Back then, the API was keyless -- totally open without any need for an API key, and rate limiting was simply enforced via IP address. Even still, those limits were quite generous. I believe they allowed something like up to 30 requests per minute. As long as I wrote some smart code to minimize API requests, there was no way any one user was going to hit those limits unless they refreshed the page over and over and over.

I see that this isn't the case any longer, and I wonder if the implementation in this PR as it currently stands will work for us.

Here are the problems I see:

Keyless API full deprecation
- According to the official CoinGecko Reddit account, the open keyless API should be fully deprecated as of right now.
- In practice this isn't the case as far as I can tell. See: https://api.coingecko.com/api/v3/simple/price?ids=ethereum&vs_currencies=usd
- There are still IP-based rate limits with this keyless API. Hit refresh a few times on that above API call and you'll hit it pretty quickly. It's certainly not the "30 per minute" that it used to be.
- I think it'd be foolish to rely on this open API (without needing a key) being available indefinitely.
Free API key limits
- They have a free API tier, which includes a key that rate limits at 30 calls per minute.
- This sounds good initially, but important to note that the limits are enforced at the API key, not the IP address. So if we bundle this API key in the app, and all Fractal users are relying on this same API key, there's no way in heck this will work for us.
Paid tier
- So now we're looking at CoinGecko's paid tiers. The cheapest tier still has a rate limit of 500 calls per minute, and I don't think that would hold up either during periods of a couple of DAOs with a couple of proposals active at the same time.
- If there are a few dozen users hitting the site at the same time, I could quickly see those per-minute rates being hit, because each site load is going to hit a handful of endpoints.
- Even still, it's just a bad pattern to include this API key in the frontend codebase. Anyone can sniff it and start using it. CoinGecko doesn't allow URL Referral enforcements either.
- CoinGecko strongly recommends we store this key on a backend server, and proxy requests through it. (See the Notes section on this page).

So where does that leave us? I'm not sure.

Are there any other open pricing APIs we could use?
If not
- Do we simply start paying for a high-volume CoinGecko API tier, stick that API key in our frontend bundle, and hope for the best?
- Or do we build a backend service (probably a Netlify function cause why not that seems easy enough) to proxy calls through, and importantly build some persistence into that service so we don't need to hammer CoinGecko's API.

mudrila · 2024-02-28T16:21:01Z

Not quite working. Went to Decent DAO on mainnet and it is showing only $9,708.66

Yeah, that's because I haven't added API key to the secrets yet. I was a bit concerned about same points that @adamgall raised, so just wanted to discuss approach first. I'm leaning towards proxying request through our own Netlify API function.

mudrila · 2024-02-28T16:29:48Z

So where does that leave us? I'm not sure.

Are there any other open pricing APIs we could use?
If not
Do we simply start paying for a high-volume CoinGecko API tier, stick that API key in our frontend bundle, and hope for the best?
Or do we build a backend service (probably a Netlify function cause why not that seems easy enough) to proxy calls through, and importantly build some persistence into that service so we don't need to hammer CoinGecko's API.

After quick research - it seems like other popular pricing APIs either having pretty much same limitations (like https://pro.coinmarketcap.com/api/pricing ) or way less user friendly (like https://www.coinapi.io/market-data-api - here you need to execute 1 request per asset and their free plan is just 100 requests per day).

I'm rather leaning towards implementing Netlify function and proxying call through it, hiding API key from FE. This request is executed primarily from Treasury page, which I wouldn't expect to be seen much. And for fetching the total for displaying on DAO Dashboard we could simply cache this value for some reasonable time (like 1h or even 1d and then adding a tooltip saying that this value updated once per day) on our side (not FE but function side), this will help reducing the cost.

…oken prices through Netlify blob

.env

netlify/functions/tokensPrices.mts

src/providers/App/hooks/usePriceAPI.ts

netlify/functions/tokensPrices.mts

…ing tokensStringParam once more, move const now down closer to the try...catch block of validating/setting cache

- Don't throw error when query param is not a valid address, just skip it. - Don't send "ethereum" string as a token to CG token API endpoint. - Return ethereum price in response if all data can be found in cache. - Fix expiration conditionals - Handle case when CoinGecko responds to an address but there's no usd price - Add entries to blob store (with long expiration) for token addresses that CoinGecko doesn't track, to avoid constantly asking about these tokens

adamgall · 2024-03-07T06:22:53Z

I've just pushed up a pretty major refactor. Some things to note:

For the devs:
- As @mudrila noted, I had good success (better than him it sounds like!) testing locally via a simple npx netlify-cli dev
- Need two new local environment variables:
  - TOKEN_PRICE_VALID_CACHE_MINUTES="5" # or however long you want
  - TOKEN_PRICE_INVALID_CACHE_MINUTES="10" # or however long you want
  - Make sure you have a local COINGECKO_API_KEY env var set. Sign up for a free one.
Heavily optimized to not bombard CoinGecko
- Caches valid prices in blob storage (see TOKEN_PRICE_VALID_CACHE_MINUTES)
- Don't throw error when query param is not a valid address, just skip it.
- Don't send "ethereum" string as a token to CG token API endpoint.
- Return ethereum and token price in response immediately if all data can be found in cache.
- Add entries to blob store (with long expiration, see TOKEN_PRICE_INVALID_CACHE_MINUTES) for token addresses that CoinGecko doesn't track, or doesn't have USD price for, to avoid constantly asking about these tokens
- Netlify function will always return response consisting of requested token addresses, even if all of the prices are "0" (in worst case)
Frontend Treasury page could use some more optimizing for when CoinGecko fails / hits rate limits. The backend currently works well and as expected (by returning cached data, even if it might be stale), but frontend throws an error toast. I think maybe it shouldn't throw an error toast.

adamgall

One more thing I just thought of -- we should namespace these blob keys to include the network. That way when we start more seriously supporting other production networks we won't have token address conflicts.

netlify/functions/tokenPrices.mts

src/providers/App/hooks/usePriceAPI.ts

adamgall

Just one thing that seems off to me.

netlify/functions/tokenPrices.mts

adamgall

I'm testing this locally, and it's not working. I have a value of "5" (minutes) set in my env var, but putting in a console.log right before the fetch to coingecko shows that it's making that request every time the frontend calls the API (every ~20 seconds).

adamgall · 2024-03-07T20:19:22Z

netlify/functions/tokenPrices.mts

+
+  // Let's pull out all of the expired addresses from our cache.
+  const expiredCachedTokenAddresses = cachedTokenPrices
+    .filter(tokenPrice => tokenPrice.metadata.fetched + config.cacheTime < config.now)


Perhaps we should be converting this cacheTime from "minutes" to "seconds" heh

Maybe we should just specify the env var value in miliseconds? config.now gives milliseconds and I feel like it's more appropriate to not convert minutes to milliseconds here. WDYT?

config.now gives seconds (https://github.com/decent-dao/fractal-interface/pull/1382/files#diff-284b3930e569c4259e85b685d36c139226ef2a210eaeb25f614b752fd1b2e910R197)

We did name the env var TOKEN_PRICE_CACHE_INTERVAL_MINUTES, which specifies that the value should be MINUTES. Minutes are a lot easier to reason about than milliseconds, when setting configs in an env var.

I think the cleaner solution is to convert the value to seconds when setting the config property here: https://github.com/decent-dao/fractal-interface/pull/1382/files#diff-284b3930e569c4259e85b685d36c139226ef2a210eaeb25f614b752fd1b2e910R198

const cacheTime = parseInt(process.env.TOKEN_PRICE_CACHE_INTERVAL_MINUTES) * 60

My TL;DR is that:

I'd like the user-facing number to be denominated in "minutes" (because that seems like an appropriate resolution for setting things like a cache expiration).

I'd like the code and data to be denominated in seconds (because we're using Unix timestamps here and those are in seconds).

@mudrila i just pushed up the fix, no worries

…ing with seconds

adamgall

Let's go!

…-token-prices

mudrila added 2 commits February 28, 2024 00:56

Implement fetching coin prices from CoinGecko

0b03759

Leverage useFormatCoins for showing treasury total on DAO dashboard page

2942280

mudrila added bug Something isn't working enhancement New feature or request labels Feb 28, 2024

mudrila requested review from adamgall and Da-Colon February 28, 2024 00:06

mudrila self-assigned this Feb 28, 2024

empty commit

22b4d13

Merge branch 'develop' into fix/show-token-prices

324edbe

adamgall mentioned this pull request Feb 29, 2024

Design and Build Pricing Service #1388

Closed

mudrila added 2 commits March 4, 2024 17:49

Add Netlify function tokenPrices, route request to CoinGecko, cache t…

e1f77e1

…oken prices through Netlify blob

Refactor and cleanup of storing/getting prices

2d6de29

mudrila commented Mar 4, 2024

View reviewed changes

.env Show resolved Hide resolved

netlify/functions/tokensPrices.mts Outdated Show resolved Hide resolved

netlify/functions/tokensPrices.mts Outdated Show resolved Hide resolved

src/providers/App/hooks/usePriceAPI.ts Outdated Show resolved Hide resolved

mudrila added 5 commits March 4, 2024 18:10

Fix returningprice for ETH

ea7aa8f

Try fixing token fiat balance overflow

c5b477a

Use ethers.constants.MaxUint256 instead of Number max value

88f356a

Use smaller multiplicator

3e5f170

Add math.round

59dd067

adamgall added maintenance Keep the lights on and removed bug Something isn't working enhancement New feature or request labels Mar 4, 2024

Merge branch 'develop' into fix/show-token-prices

e5afb36

adamgall removed the maintenance Keep the lights on label Mar 4, 2024

adamgall requested changes Mar 5, 2024

View reviewed changes

adamgall requested a review from tomstuart123 March 5, 2024 00:21

mudrila and others added 5 commits March 7, 2024 00:28

Trying changing possibleCachedTokenPrices filtering null values

2c92f7b

Fix filtering logic of uncached token prices

873df06

Use rawTokenAddresses for allUncachedTokenAddresses instead of splitt…

15e1564

…ing tokensStringParam once more, move const now down closer to the try...catch block of validating/setting cache

Refactor to compartmentalize code

b207d66

tomstuart123 approved these changes Mar 7, 2024

View reviewed changes

adamgall requested changes Mar 7, 2024

View reviewed changes

mudrila added 2 commits March 7, 2024 15:24

Split stores per network

e70037d

Add network param to the tokenPrices function

91c0326

mudrila requested a review from adamgall March 7, 2024 14:41

adamgall requested changes Mar 7, 2024

View reviewed changes

netlify/functions/tokenPrices.mts Outdated Show resolved Hide resolved

adamgall requested changes Mar 7, 2024

View reviewed changes

netlify/functions/tokenPrices.mts Outdated Show resolved Hide resolved

adamgall requested changes Mar 7, 2024

View reviewed changes

src/providers/App/hooks/usePriceAPI.ts Outdated Show resolved Hide resolved

mudrila added 2 commits March 7, 2024 20:09

Fix token address chain namespacing

2248280

Swap expiration with time of fetching

621771c

mudrila requested a review from adamgall March 7, 2024 19:48

Cleanup comments

96420f8

adamgall reviewed Mar 7, 2024

View reviewed changes

netlify/functions/tokenPrices.mts Outdated Show resolved Hide resolved

mudrila added 2 commits March 7, 2024 21:00

Reverse adding cacheTime

dfaa285

Fix cached token addresses filtering

b5a93f5

mudrila requested a review from adamgall March 7, 2024 20:02

adamgall requested changes Mar 7, 2024

View reviewed changes

Convert minutes to seconds, specify in variable names that we're deal…

2f85687

…ing with seconds

adamgall approved these changes Mar 7, 2024

View reviewed changes

adamgall added 2 commits March 7, 2024 15:55

Merge commit '11303a767df67e67e8007ca20e07894b268e3269' into fix/show…

7720d94

…-token-prices

Prettier updates

daf34df

adamgall merged commit d4b6591 into develop Mar 7, 2024
7 checks passed

adamgall deleted the fix/show-token-prices branch March 7, 2024 21:14

adamgall mentioned this pull request Mar 7, 2024

Production push #1420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show token prices #1382

Show token prices #1382

mudrila commented Feb 28, 2024

netlify bot commented Feb 28, 2024 •

edited

Loading

Da-Colon commented Feb 28, 2024

adamgall commented Feb 28, 2024 •

edited

Loading

mudrila commented Feb 28, 2024

mudrila commented Feb 28, 2024

adamgall commented Mar 7, 2024 •

edited

Loading

adamgall left a comment •

edited

Loading

adamgall left a comment

adamgall left a comment

adamgall Mar 7, 2024

mudrila Mar 7, 2024

adamgall Mar 7, 2024 •

edited

Loading

adamgall Mar 7, 2024

adamgall left a comment

Show token prices #1382

Show token prices #1382

Conversation

mudrila commented Feb 28, 2024

Description

Notes

Issue

Testing

Screenshots (if applicable)

netlify bot commented Feb 28, 2024 • edited Loading

✅ Deploy Preview for fractal-framework-dev ready!

Da-Colon commented Feb 28, 2024

adamgall commented Feb 28, 2024 • edited Loading

mudrila commented Feb 28, 2024

mudrila commented Feb 28, 2024

adamgall commented Mar 7, 2024 • edited Loading

adamgall left a comment • edited Loading

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

adamgall Mar 7, 2024

Choose a reason for hiding this comment

mudrila Mar 7, 2024

Choose a reason for hiding this comment

adamgall Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

adamgall Mar 7, 2024

Choose a reason for hiding this comment

adamgall left a comment

Choose a reason for hiding this comment

netlify bot commented Feb 28, 2024 •

edited

Loading

adamgall commented Feb 28, 2024 •

edited

Loading

adamgall commented Mar 7, 2024 •

edited

Loading

adamgall left a comment •

edited

Loading

adamgall Mar 7, 2024 •

edited

Loading