yearn / yearn-sdk

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

feat: Custom Zap Deposit Simulations on SDK [WEB-1011] #280

Closed karelianpie closed 2 years ago

karelianpie commented 2 years ago

Description

Simulations should support any custom zap transaction

Related Issue

https://linear.app/yearn/issue/WEB-1011

Motivation and Context

Users know expected transaction outcome for any type of transaction

How Has This Been Tested?

Yalc'ed the SDK and did some FE simulations

Screenshots (if appropriate):

ETH simulation working as expected

Screen Shot 2022-04-26 at 3 10 44 pm
linear[bot] commented 2 years ago
WEB-1011 Custom Zap Simulations on SDK

### Elevator Pitch: **Simulations should support any custom zap transaction** ### Customer Value: **Users know expected transaction outcome for any type of transaction** ### Agreed upon Acceptance Criteria (definition of Done): * `simulation.deposit()` routes token correctly to a vault via native or zap in contract. * `simulation.withdraw()` routes token correctly from a vault via native or zap out contract. * If simulation requires token approval before tx execution, execute an approve tx to max in fork. * If vault does not support deposits/withdrawals from/to a given token, throw error. ### Design Approach: Generally, we want vault.deposit execute almost identical payload than simulation.deposit, since intention of simulations is to run catch error for users before they happen, and return the expected outcome. Use as much abstractions as possible, since in the future we might need to simulate other type of transactions, like migrations, staking, etc. ### Refactoring Impact: Might be good idea to refactor vault.deposit and vault.withdraw methods, and extract a the logic that creates the the populated transaction payload, so this can be used by simulations. ### Threat Modeling: Should simulations also go through allowlist validation? ### Design review: xgambitox

github-actions[bot] commented 2 years ago

size-limit report 📦

Path Size
dist/sdk.cjs.production.min.js 46.23 KB (+3.14% 🔺)
dist/sdk.esm.js 46.61 KB (+3.12% 🔺)
xgambitox commented 2 years ago

Getting this error testing zap-in with ETH image

xgambitox commented 2 years ago

also error on a direct deposit image

karelianpie commented 2 years ago

Getting this error testing zap-in with ETH

The issue here is that the cached vaults don't have the new zapInWith and zapOutWith props, because when we call vaults.get() we just returned the cached values and don't call the vaults.getDynamic() where the new Zap props are being added. Without cache it works as expected:

Screen Shot 2022-04-28 at 3 11 12 pm
karelianpie commented 2 years ago

also error on a direct deposit

With this one, the issue was that we were approving before creating the fork in tenderly, so we did not have permission to move the token in the first place. That was fixed with https://github.com/yearn/yearn-sdk/pull/280/commits/c10198a2f0a27339b04ba78f41367973f35ae9ae and it's working as expected now:

Screen Shot 2022-04-28 at 3 13 32 pm