yearn / yearn-sdk

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

Add Wido Zaps with support for more vaults #306

Open mazurroman opened 1 year ago

mazurroman commented 1 year ago

Description

Add Zap support for more vaults on Yearn.

Wido brings Zap support for:

With Zaps for more vaults to come later.

Related Issue

PR in yearn-finance-v3 https://github.com/yearn/yearn-finance-v3/pull/755

Motivation and Context

Zaps let users deposit any token into a vault instead of just the underlying token. That improves UX and increases deposits and TVL.

Data shows that Zaps contribute to 40% of deposits on Yearn. Having Zap support for all vaults will grow Yearn's overall TVL.

Report: https://twitter.com/widolabs/status/1574762471314915334

How Has This Been Tested?

It was tested locally with the front end running the yalc'ed SDK.

Left to do

Screenshots (if appropriate):

image
xgambitox commented 1 year ago

PR looks good overall :)

We should add a mechanism to dynamically switch precedence between different zap providers. So users of SDK can define the precedence of their choice if a vault is supported on multiple services. An idea could be setting a configuration on the SDK context, so this can be set when the SDK gets instantiated, and have a default if its not provided. Also that same mechanism would be useful to turn on/off a zap service provider (this can be done in separate PR though) for added privacy use cases.

something like:

export interface ContextValue {
  ...
  zaps?: {
    zapInWith: ZapInWith[];
    zapOutWith: ZapOutWith[];
  };
  ...
}

Where the first element in the array takes precedence over the next.

wdyt?

kernelwhisperer commented 1 year ago

PR looks good overall :)

We should add a mechanism to dynamically switch precedence between different zap providers. So users of SDK can define the precedence of their choice if a vault is supported on multiple services. An idea could be setting a configuration on the SDK context, so this can be set when the SDK gets instantiated, and have a default if its not provided. Also that same mechanism would be useful to turn on/off a zap service provider (this can be done in separate PR though) for added privacy use cases.

something like:

export interface ContextValue {
  ...
  zaps?: {
    zapInWith: ZapInWith[];
    zapOutWith: ZapOutWith[];
  };
  ...
}

Where the first element in the array takes precedence over the next.

wdyt?

Makes sense. Can you take a look at c82cc69ca8570f3e86596933bfe74c652c35659e ?

kernelwhisperer commented 1 year ago

With regards to deposit vs _deposit:

I audited _deposit and _withdraw and they correctly call the correct zap functions. E.g.: getDepositContractAddress and populateDepositTransaction are called, both of which are zap aware.

If we were to deprecate/remove deposit and withdraw we could also remove the functions zapInApprovalState, zapInApprovalTransaction, zapOutApprovalState and zapOutApprovalTransaction in both wido.ts and portals.ts, as they would not be used.

xgambitox commented 1 year ago

With regards to deposit vs _deposit:

I audited _deposit and _withdraw and they correctly call the correct zap functions. E.g.: getDepositContractAddress and populateDepositTransaction are called, both of which are zap aware.

If we were to deprecate/remove deposit and withdraw we could also remove the functions zapInApprovalState, zapInApprovalTransaction, zapOutApprovalState and zapOutApprovalTransaction in both wido.ts and portals.ts, as they would not be used.

Ok cool then, lets leave them for now since its done and code looks good, and once we get out of alpha for this version I will deprecate and remove those functions. Thanks for looking into it!