Skip to content
This repository was archived by the owner on Jul 3, 2023. It is now read-only.

Better apy polling #65#66

Open
blockcontractor wants to merge 8 commits intobaofinance:mainfrom
blockcontractor:better-apy-polling-#65
Open

Better apy polling #65#66
blockcontractor wants to merge 8 commits intobaofinance:mainfrom
blockcontractor:better-apy-polling-#65

Conversation

@blockcontractor
Copy link
Copy Markdown

@blockcontractor blockcontractor commented Apr 1, 2021

Closes #65

### Approach

Display APY for pairs that do not use ETH.

  • For non ETH pairs, add a denominatorAddresses field in constants.js that points to the denominator token adddresses. This is optional, and if no denominator contract is set, we assume the denominator is ETH.
  • Additionally add denominator symbol (not used, but added due to Farms some commented code on the farms card that was showing amount of ETH in the pool)
  • Update useAllStakedValue to return denominatorAmount, denominatorWethEquivalent, and tokenPriceInDenominator
  • Update getTotalLPWethValue function:
    • now works with NON-LP token deposits (i.e. the xSushi deposit farm)
      • We determine if the farm is a 'token farm' by checking if the token contract is the same as the lp token contract.
      • For token farms, we simply use the balance of the token in the masterchef contract for calculations.
    • now works with non ETH denominators
      • if denominatorContract is set, we additionally fetch the value of the denominator in WETH, and the number of decimals it has, by calling getDenominatorWethValueAndDecimals. This function makes requests to sushiswap to get the token details, and the current midprice of the denomninator/WETH pair. These values are then used to calcualte the ETH worth of the denominator for use in the APY calcs.

Reduce the number of calls used to show APY data.

  • Create an addCallsToBatch utility function that accepts a web3 batch request object and an array of contract calls, and returns a promise that will resolve with the result, without executing the batch (i.e. the requests won't actually be called by the function).
  • Use this in getTotalLPWethValue to batch all calls there.

Additional Notes

ethers was added to dependencies due to it being a requirement for the sushiswap SDK. It may be preferable to add API keys instead of using the default ones. This is relatively straightforward but not 100% necessary (I've not had any issues with the defaults). Details here.

@blockcontractor
Copy link
Copy Markdown
Author

@thebaoman (or anyone else who may be able to advise here - please feel free to tag them)...

A few questions:

  1. I've just spent some time refactoring the getTotalLPWethValue utility function to use web3.batchRequest (sidenote: the documentation is pretty useless...!). This seems to be working (i.e., the ETH denominated APYs are all calculated as before), but I am unsure if this has actually done anything. I had assumed making a batch request would reduce the number of http requests shown in my Metamask's background page, but actually it's pretty much the same. The original issue states:

    They also hit the RPC node once per farm.

    Am I barking up the wrong tree in my approach to fixing this? How can I measure the number of times the RPC node is hit so I know if my approach is working?

  2. The useEffect dependencies for fetchAllStakedValue currently include the block. This is currently resulting in fetchAllStakedValue being called very frequently. I think the memoization for the useCallback is not working at present as I think this should be preventing it being called unless one of the deps changes, but a simple console log suggests that's possibly not the case.

  3. Not really a question, more of a statement, but the type definitions are a bit ropey at the moment, possibly due to having quite a few .js files mixed into the project. It seems we're quite tolerant of any types at the moment - is there some sort of guide/convention for this, or is it (as I assume) "type what you can" for now?

Apologies for the newbie questions - as I stated when I applied I'm relatively new with the web3 bits. Hopefully you can bear with me!

I've joined the discord (@blockcontractor#7712) - not likely to be on much over the easter weekend though.

@blockcontractor
Copy link
Copy Markdown
Author

As an update: whilst waiting for input on the batch request question tonight, I worked on non-WETH denominated pairs. Haven't pushed as it's still a WIP.

Looking very achievable (nearly there) for mainnet, but haven't considered xDai (or others) properly yet so may need to rethink. Thought I'd get mainnet down though to ensure my approach is sound.

As mentioned before I'll probably not get a chance to work on this again until after easter weekend (i.e. not until tues 6th).

@blockcontractor
Copy link
Copy Markdown
Author

Managed to spend some time today on this - APYs now show for all pairs, using the sushiswap sdk.

A couple of notes (partially to myself!)

  • I have currently added ethers dependencies - I am planning/aiming to remove these by supplying the web3 provider to the sushiswap SDK.
  • Since I've started this I've realised it may be better to use the chainlink pricing oracles to calculate the price of non eth pairs instead of the sushiswap sdk (I assume this is what the baomasterfamer contract is doing?). Would this be preferable? If so, how should I get the xSushi price which chainlink doesn't have an oracle for?

@jondwillis
Copy link
Copy Markdown
Contributor

jondwillis commented Apr 6, 2021

@blockcontractor https://github.com/baofinance/baoswap-ui/pull/42 some relevant work there. you'll have to do cross-chain lookups for xdai and use the reserve ratio on the SLPs for some of the pairs

happy to discuss here or on discord, DM'd you there.

@blockcontractor blockcontractor marked this pull request as ready for review April 7, 2021 20:54
@zashton
Copy link
Copy Markdown
Contributor

zashton commented Apr 8, 2021

As an update: whilst waiting for input on the batch request question tonight, I worked on non-WETH denominated pairs. Haven't pushed as it's still a WIP.

Looking very achievable (nearly there) for mainnet, but haven't considered xDai (or others) properly yet so may need to rethink. Thought I'd get mainnet down though to ensure my approach is sound.

As mentioned before I'll probably not get a chance to work on this again until after easter weekend (i.e. not until tues 6th).

I believe the issue is pushing too many request within a short span of time, causing some sort of throttling problem. You probably can fix it if you can sub-batch and space the requests out with some time delay

@blockcontractor

@blockcontractor
Copy link
Copy Markdown
Author

@zashton - I don't actually experience the issue - how does it manifest / how do you reproduce it?

All of the APYs now show for me.

Surely batching requests has reduced the number?

@rocky-baoboa rocky-baoboa self-requested a review April 8, 2021 14:33
@blockcontractor
Copy link
Copy Markdown
Author

@rocky-baoboa @thebaoman appreciate you're busy but some feedback would be useful when you get a chance.

@blockcontractor
Copy link
Copy Markdown
Author

@rocky-baoboa bump

@rocky-baoboa
Copy link
Copy Markdown
Contributor

Thanks for bumping @blockcontractor apologies this has sat for so long.

To answer some of your questions.

  1. That is how one would expect batchRequest to work if the underlying provider supports it. But I don't believe this is actually the case. From some digging it looks like it's entirely based on whether or not the underlying provider implements it. It doesn't appear that the http provider actually includes a mechanism to batch requests in the matter you'd expect. Instead they seem to be just a mechanism to group requests and organize them in code. But @thebaoman or @zashton might have some more insight on that.

I believe you'll actually have to implement your own batch/delay mechanism so we can query with one batch, wait some amount of time, and then query again.

With APY optimizing we've seen a few times where it seems to work locally when not doing this batch/delay method, but not in staging.

  1. This is very likely. The original source we forked had a few bugs like this and were exacerbated by our numerous farms. If you could log an issue for this that would be great.

  2. The way we've been going about it is to add the types as we make changes. There isn't a larger move to type everything just yet - but it is something that we need to get to.


I'm unsure if these changes will actually reduce the overall number of HTTP calls being made, but only because I'm unsure about the batch method itself. I'll do some more testing on this branch and hopefully have some answers for you.

@blockcontractor
Copy link
Copy Markdown
Author

blockcontractor commented May 26, 2021

Hi @rocky-baoboa & @thebaoman,

I'm looking for this to get merged and for a payout on the bounty.

I've done the work requested and without further, concrete steps to reproduce the issue and demonstrate that my changes do not solve it, I feel this passes both of the points in both the original brief and the work plan I submitted, and was approved based upon.

If the issue is no-longer relevant due to changes in the product, I still feel it is fair to pay the bounty, as the work was submitted two months ago.

Thanks.

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.

Gitcoin Grant: Better APY Polling ($2k)

4 participants