Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Wido Zaps with support for more vaults #306

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

mazurroman
Copy link

@mazurroman mazurroman commented Sep 30, 2022

Description

Add Zap support for more vaults on Yearn.

Wido brings Zap support for:

  • yvCurve-ibAUD-USDC
  • yvCurve-ibGBP-USDC
  • yvCurve-ibCHF-USDC
  • yvCurve-ibJPY-USDC
  • yvCurve-ibKRW-USDC
  • yvCurve-ibBTC

With Zaps for more vaults to come later.

Related Issue

PR in yearn-finance-v3 yearn/yearn-finance-v3#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

  • [Bug] Some zaps show expected slippage of 100.0% because priceUSDC is missing in supported tokens
  • Supported Vaults — make an API call instead of hardcoding
  • Unit tests
  • Show “Approve” not “Sign” on Wido Zaps

Screenshots (if appropriate):

image

kernelwhisperer and others added 2 commits October 3, 2022 14:49
* Add Wido Zaps part 1

* Add wido zaps part 2

* Update src/interfaces/simulation.ts

Co-authored-by: Kunal Jain <[email protected]>

* Address PR feedback

* Hoist wido before portals

* Update supported vaults & caches

* Fix populate tx error

* Disable allowlist

* Address some TODOs

Co-authored-by: Kunal Jain <[email protected]>
@xgambitox
Copy link
Contributor

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
Copy link

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 c82cc69 ?

@kernelwhisperer
Copy link

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.

@mazurroman mazurroman marked this pull request as ready for review October 7, 2022 14:19
Copy link
Contributor

@xgambitox xgambitox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments :)

package.json Outdated Show resolved Hide resolved
src/common.ts Outdated Show resolved Hide resolved
src/interfaces/token.ts Outdated Show resolved Hide resolved
src/interfaces/token.ts Outdated Show resolved Hide resolved
src/interfaces/token.ts Outdated Show resolved Hide resolved
src/interfaces/vault.ts Outdated Show resolved Hide resolved
src/services/wido.ts Outdated Show resolved Hide resolved
@xgambitox
Copy link
Contributor

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!

xgambitox
xgambitox previously approved these changes Oct 27, 2022
xgambitox
xgambitox previously approved these changes Oct 28, 2022
@xgambitox xgambitox added the hold Hold pull request label Dec 14, 2022
Resolved Conflicts:
	src/common.ts
	src/interfaces/vault.ts
	src/yearn.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Hold pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants