Read set component quantities from on-chain #422
Conversation
This comment has been minimized.
This comment has been minimized.
I'm looking into doing this for a lot of other things as well. I think we should combine the token calls by categories, not by token. Will require other sizeable changes, but will be cleaner and help get lessen the provider shelf in |
While working on IndexCoop/index-ui#422 I would've liked to see JSDoc comments for `Position`. I found docs on https://docs.tokensets.com/contracts/core-contracts/set-token#getpositions so I added them inline so devs can't see them while using types from this package. If reviewers approve of a change like this, I can add doc comments to other types in this package. I would fetch docs from https://docs.tokensets.com/contracts but open to other ideas.
+1 to consolidating providers |
|
We should continue moving towards deprecating the Tokensets API. Check my understanding below, but I don't think we need to keep the
For reference, I'm basically driving us towards having comparable functionality displayed here: |
|
Based on that super helpful context, I'll take a stab at ripping out the |
|
Like @controtie said, it seems possible to entirely remove We'll need an alternative source for these fields: c964c8c#diff-75a9444fe9507131dc7438eb5eb9dc75dc9dfd712c8dc86c905142471030043eR45-R48 |
good, kill as many of those avalanching providers as we can |
|
@0xModene or @controtie do ya'll have insight on how If we got this route, I think we want to make a batch CoinGecko request to make this performant. |
This data is all on chain, and I believe it's just simple math @ncitron may have a better idea |
|
Fetching the amount of an underlying component per set token is relatively easy. This can be done through a call to |
This is a V0 of reading DPI index components from on-chain using set.js There is room for a refactor here. Instead of plumbing two types of components (`IndexComponents` and `SetComponents`) into child React components, we can collapse these two objects at the provider / data fetching layer. This will enable us to unit test the data merging logic which is the sketchiest part about this PR. We need to fallback to the existing `IndexComponents` fetched via Set API because it contains other component metadata (e.g. image, symbol, dailyPercentChange, etc.) which is used in the table. Additionally we want to fallback to the existing `IndexComponents` because the set.js request will only work after a user has connected their wallet. We want the DPI page to still contain index component information if a user hasn't connected their wallet yet.
The allocation table looks broken with this change
rootulp
force-pushed
the
rp/index-components
branch
from
c964c8c
to
dd89270
Compare
Fetched component price from CoinGecko
Switch to CoinGecko token list because 1inch list doesn't contain WHALE or WAXE (two recent additions to MVI).
I'm getting HTTP 429's from CoinGecko so this is tough to manually test
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
CoinGecko API is difficult to use. It doesn't appear possible to fetch all components of DPI in the same request b/c token contract address is a query string parameter and fetching 18+ tokens in query string overflows the max length of a URL.
eth2xfli and btc2xfli are still broken
| .catch(e => console.error(e)) | ||
| } | ||
|
|
||
| // HACKHACK is there a safer way to fetch decimal places for these components? |
There was a problem hiding this comment.
Do reviewers know of a safer way to determine this? I naively assumed all component quantities would have 18 significant figures but I had to divide by 4, 6, 8, or 18 to get the component quantities for products like Eth2xFli to match up with what is visible on https://www.tokensets.com/explore
There was a problem hiding this comment.
That's actually a read only function on the contract called decimals() with no params, returns a BN.
This will need to be checked for sure. MVI has a couple of tokens with different significant digits (WHALE comes to mind)
There was a problem hiding this comment.
yep WHALE and WAXE are definitely oddities so I special cased them in this function. Thanks for sharing the decimals() function. I manually verified the tokens that I special cased in the function and they all line up.
Considering this PR diff is huge, I'm inclined to merge this as-is and tackle reading decimals() from on-chain for all token contracts in a fast-follow PR if that's okay with reviewers.
There was a problem hiding this comment.
This data should be available in the tokenList response you're receiving.
There was a problem hiding this comment.
That makes this way easier than I expected. Will fix in this PR
|
@0xModene @controtie ready for review. Sorry for my delay on this PR. |
|
@rootulp you mention batching requests correctly, and theres a TON of that we could do across the app. We may need to look more into that in another story |
|
Sounds good. During development, I noticed that we were performing two network requests per product to fetch market chart data. This seems potentially wasteful. E.g.
Upon further inspection, it looks like the
We likely want the hourly data if we continue supporting the one day and one week options on the market chart.
@0xModene are there other non-batched CoinGecko requests you are interested in? Down to tackle in a separate GH issue |
|
@rootulp actually those are coming for different reasons. hourly data can be had up to 90d, but not beyond through CG. The second call is got the 12m+ data charts |
There was a problem hiding this comment.
Hey this is a really great first draft @rootulp !
Left some comments re: pulling some data but overall it's really easy to read for something so complicated.
One more ask would be putting in a placeholder text when the user's wallet isn't connected under asset allocation.
Maybe something like Connect Your Metamask under the allocations section?
| @@ -22,6 +21,7 @@ import { | |||
| ProductToken, | |||
| } from 'constants/productTokens' | |||
| import BigNumber from 'utils/bignumber' | |||
| import { SetComponent } from "../../contexts/SetComponents/SetComponent" | |||
| .catch(e => console.error(e)) | ||
| } | ||
|
|
||
| // HACKHACK is there a safer way to fetch decimal places for these components? |
There was a problem hiding this comment.
This data should be available in the tokenList response you're receiving.
|
@controtie I'll look today for a way to lint external/internal imports |
This comment has been minimized.
This comment has been minimized.
src/App.tsx
Outdated
| render={() => (window.location.href = discordLink)} | ||
| /> | ||
| <Route exact path='/discord' | ||
| <Route |
There was a problem hiding this comment.
FYI I don't think Prettier actually runs on this codebase b/c this diff was applied when I used VSCode CMD + P > Format Document With... > Prettier
I've seen a handful of instances where double quotes are used when single quotes are specified:
https://github.com/SetProtocol/index-ui/blob/master/.prettierrc.json#L3
We should auto-apply Prettier code-base wide. I don't think the pre-commit hooks are working as expected
| @@ -3,12 +3,25 @@ import { createContext } from 'react' | |||
| interface PricesContextValues { | |||
| indexPrice?: string | |||
| ethereumPrice?: string | |||
| dpiPrice: number | |||
There was a problem hiding this comment.
noting here, but unrelated to PR. Let's make another story that turns these all to numbers to match your new values
Label on the button got changed in IndexCoop#505
|
The last commit changes copy from "Unlock" to "Connect" to match the button in the top right
|
|
I'm having trouble getting the set allocations to show up when connecting my ledger. I'm not an expert on our ledger integration. This shows up in my google search: Since this change sort of coincides with the v2 deployment of the website (strongly encouraging users to login with the majority of non-logged in info displayed in the wrapper marketing application) I'd like to point this to the v2-website-master branch and merge it now while we investigate this issue in parallel |
* Read set component quantities from on-chain (#422) * Read set component quantities from on-chain This is a V0 of reading DPI index components from on-chain using set.js There is room for a refactor here. Instead of plumbing two types of components (`IndexComponents` and `SetComponents`) into child React components, we can collapse these two objects at the provider / data fetching layer. This will enable us to unit test the data merging logic which is the sketchiest part about this PR. We need to fallback to the existing `IndexComponents` fetched via Set API because it contains other component metadata (e.g. image, symbol, dailyPercentChange, etc.) which is used in the table. Additionally we want to fallback to the existing `IndexComponents` because the set.js request will only work after a user has connected their wallet. We want the DPI page to still contain index component information if a user hasn't connected their wallet yet. * Fetch tokenList data and remove IndexComponents The allocation table looks broken with this change * Display allocation per component Fetched component price from CoinGecko * Remove percent daily change * Fetch MVI components Switch to CoinGecko token list because 1inch list doesn't contain WHALE or WAXE (two recent additions to MVI). * [WIP] FLI and Data products I'm getting HTTP 429's from CoinGecko so this is tough to manually test * Sort positions based on percent of set * Remove old component providers and hooks * WIP: Attempt to batch requests to CoinGecko CoinGecko API is difficult to use. It doesn't appear possible to fetch all components of DPI in the same request b/c token contract address is a query string parameter and fetching 18+ tokens in query string overflows the max length of a URL. * WIP batch fetch token prices from CoinGecko * Fetch MVI component prices * Fetch all product component prices eth2xfli and btc2xfli are still broken * Code cleanup from reviewing draft PR * Fix NaNs! * Address @controtie comments * Format document with Prettier We should auto-apply Prettier code-base wide. I don't think the pre-commit hooks are working as expected * Unlock -> Connect Label on the button got changed in #505 * deprecate unused navbar links * remove home from navbar * separate product dropdowns * remove footer * remove home page * add index as home placeholder * remove unused pages in mobile navbar * remove unused views * remove unused view imports from home page * remove news api * remove unused api + hooks * remove unused tests * remove footer tests * replace homepage with index to dpi * [WIP] V2 Launch Polish (#512) * remove product descriptions * remove index token description * remove how to buy link * remove scrollbars on navbar dropdown * remove airdrop functionality * remove unused airdrop test Co-authored-by: Rootul Patel <rootulp@gmail.com> Co-authored-by: 0xModene <7647623+0xModene@users.noreply.github.com>
Type of Change
Summary of Changes
The goal was to replace requests to the Set backend by reading product underlying tokens and their allocations from on-chain. See google doc for more context.
I observed sporadic 429s from CoinGecko during development. I think a paid tier CoinGecko API key would prevent indexcoop.com from observing similar throttles in production. I haven't observed these throttles since batching requests correctly so this may not strictly be necessary.
Test Data or Screenshots
Note: I removed the 24hr change column