search

yearn / yearn-sdk

🦧 SDK for the yearn.finance platform. WIP
https://npm.im/@yfi/sdk
MIT License
53 stars 56 forks source link

fix: Flatten the cache before filtering #277

Closed karelianpie closed 2 years ago

karelianpie commented 2 years ago

Description

The endpoint https://cache.yearn.finance/v1/chains/1/vaults/getDynamic is returning an array of arrays, however we are filtering the addresses as if it was a flat array. Wondering if anyone has some context on why we are grouping this array into chunks of 20 and if we should get a flat array from that endpoint?

It's working for https://cache.yearn.finance/v1/chains/1/vaults/get because it's a flat array and both are using exactly the same filtering, but unfortunately that only works for the flatten one:

cached.filter((vault) => addresses.includes(vault.address));

Related Issue

Fixes https://github.com/yearn/yearn-sdk/issues/274

How Has This Been Tested?

Tested by running the sdk/fe locally

Screenshots (if appropriate):

Screen Shot 2022-04-09 at 10 47 22 am Screen Shot 2022-04-09 at 10 52 56 am
github-actions[bot] commented 2 years ago

size-limit report 📦

Path Size
dist/sdk.cjs.production.min.js 44.29 KB (+0.01% 🔺)
dist/sdk.esm.js 44.66 KB (+0.01% 🔺)
jstashh commented 2 years ago

Thanks @karelianpie this makes sense as to the reason why we're having the issue. The reason why we are 'paginating' the results is because if we attempted to fetch all of the vaults at once then the call would run out of gas, instead we split it into multiple separate calls, and aggregate the result. A lot of the reason why it runs out of gas is because it's fetching prices from the oracle, which can be very gas intensive - https://github.com/yearn/yearn-lens/tree/master/contracts/Oracle

I think this PR would fix it, but I think it'd be better to fix it at this line here - https://github.com/yearn/yearn-caching-api/blob/master/jobs/vaults.mjs#L29. Perhaps by using concat rather than push. This will let us leave the implementation in the sdk the same, consistent with other reads from the cache, and it'll be quicker for us to deploy the fix. Would you be happy making that change?

karelianpie commented 2 years ago

Thanks @karelianpie this makes sense as to the reason why we're having the issue. The reason why we are 'paginating' the results is because if we attempted to fetch all of the vaults at once then the call would run out of gas, instead we split it into multiple separate calls, and aggregate the result. A lot of the reason why it runs out of gas is because it's fetching prices from the oracle, which can be very gas intensive - https://github.com/yearn/yearn-lens/tree/master/contracts/Oracle

I think this PR would fix it, but I think it'd be better to fix it at this line here - https://github.com/yearn/yearn-caching-api/blob/master/jobs/vaults.mjs#L29. Perhaps by using concat rather than push. This will let us leave the implementation in the sdk the same, consistent with other reads from the cache, and it'll be quicker for us to deploy the fix. Would you be happy making that change?

Sounds good, done with https://github.com/yearn/yearn-caching-api/pull/53

I think that repo could do with some tests too at some point