Read streaming fees from on-chain via set.js #380
Conversation
`yarn start` works now. Had to build set.js locally with SetProtocol PR 76. So this PR will be blocked on upgrading to a release of set.js with that change.
- Add ethers as a dep - Need `resolutions` block to resolve error
Prettier should really handle this for us
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Thanks for the super details comments as usual! :D
I think we definitely want to read the quantities on chain here.
- Set eventually wants to deprecate its APIs and go pretty decentralized
- Set runs a scraper bot in the background to update it's DB with set positions. If we're doing something like issuance/redemption we need to be pretty sure the position's we're reading are accurate.
I added a comment re: fetching streaming fee quantities on-chain. This info probably isn't included in the docs. Sorry about that!
This comment has been minimized.
This comment has been minimized.
rootulp
changed the title
I had to update the contract addresses in .env and .env.prod to get this to work
| "@ethersproject/bignumber": "5.4.1", | ||
| "@ethersproject/hdnode": "5.4.0", | ||
| "@ethersproject/transactions": "5.4.0", | ||
| "@ethersproject/providers": "5.4.5" |
There was a problem hiding this comment.
these resolutions were necessary to resolve a build error. It's possible these can be removed after upgrading existing dependencies.
| @@ -9,7 +9,7 @@ export interface ProductToken { | |||
| image: string | |||
| coingeckoId: string | |||
| tokensetsId: string | |||
| fees: any | undefined | |||
| fees: { streamingFee: string } | undefined | |||
There was a problem hiding this comment.
Proposal for a refactor that is out of scope for this PR:
Modify IndexToken to not be of type ProductToken but instead pick a subset of props (all props excluding fees) b/c all other product tokens have fees. Then we can make this field:
fees: { streamingFee: string }
There was a problem hiding this comment.
I like this idea. There are some other small bits with how it's currently set up I would also like to change. We may need to discuss this soon.
rootulp
changed the title
There was a problem hiding this comment.
only small nits: in many of the files you're importing you're importing '../../constants~' but you don't have to. Can you double check all your files and change anything with a leading ../ to just 'constants' or 'utils' etc?
this looks great otherwise, looking forward to getting this in
There was a problem hiding this comment.
My apologies for taking so long to review these changes. I notice the PR changed from reading token allocations of products on-chain to reading streaming fees on-chain. Is this meant to be a small bite on the way to reading token allocations on-chain?
PR looks mostly good besides the minor import clean up!
| import useWallet from "../../hooks/useWallet" | ||
| import { bedTokenAddress, btc2xfliTokenAddress, dpiTokenAddress, eth2xfliTokenAddress, mviTokenAddress } from "constants/ethContractAddresses" | ||
| import { convertToPercentage } from "../../utils/ethersBigNumber" | ||
| import StreamingFeeContext from "./StreamingFeeContext" |
There was a problem hiding this comment.
Per Modene's comment cleaning up these imports to be absolute + ordering (external packages at the top of the file)
There was a problem hiding this comment.
Can do. With regards to ordering, if we're going to to enforce this manually (i.e. without the help of a linter) then I propose we draft a style guide and link it from the README of this package. I can take a first stab at this with this rule if we feel strongly about it.
On the other hand, I like projects where lint --fix automatically formats the code according to all team style guide code conventions. Prettier / ESLint are a solid step towards this. With regards to import ordering, we could enforce something like https://eslint.org/docs/rules/sort-imports
There was a problem hiding this comment.
I'm all for an auto linter/prettifier!
Yep |
Added to google doc |
Description
Previously all streaming fees were hard-coded in
productTokens.tsexample. This change reads streaming fees for all products from on-chain. This helps us future-proof in-case streaming fees for existing products are modified. This also sets up infrastructure to read other data from set.js which will be helpful for future PRs.Motivation
Google doc
Smoke test for reviewers
yarn start