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

EIP-4626: Tokenized Vault Standard #4626

Merged
merged 19 commits into from
Jan 8, 2022
Merged

Conversation

Joeysantoro
Copy link
Contributor

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

@Joeysantoro Joeysantoro changed the title Create eip-4626.md Yield Bearing Vault Standard Jan 4, 2022
@eth-bot
Copy link
Collaborator

eth-bot commented Jan 4, 2022

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):


(fail) eip-4626.md

classification
newEIPFile
  • File with name EIPS/eip-4626.md is new and new files must be reviewed
  • This PR requires review from one of [@lightclient, @axic]

eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated
Comment on lines 1 to 2
# Yield Bearing Vault Standard

Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow the template found at https://raw.githubusercontent.com/ethereum/EIPs/master/eip-template.md

The formatting on this header is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Co-authored-by: Micah Zoltu <micah@zoltu.net>
eip-4626.md Outdated Show resolved Hide resolved
@Joeysantoro Joeysantoro changed the title Yield Bearing Vault Standard EIP-4626: Yield Bearing Vault Standard Jan 4, 2022
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
Copy link
Contributor

@wschwab wschwab left a comment

Choose a reason for hiding this comment

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

I only had time for a quick passthrough rn, so I left some comments on some stuff that stuck out to me, mainly just technical stuff about how EIPs need to be structured. Don't get disheartened - especially the first time submitting an EIP it's hard to keep track of all the stuff that needs to be there and how it needs to be formatted. It might take a couple of rounds of iterations before it gets merged in as a Draft, but this looks like a good candidate imho.

eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
Co-authored-by: William Schwab <31592931+wschwab@users.noreply.github.com>
Co-authored-by: William Schwab <31592931+wschwab@users.noreply.github.com>
eip-4626.md Outdated


#### totalHoldings
`function totalHoldings() public view returns (uint256)`

Choose a reason for hiding this comment

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

There ought to be some documentation here or elsewhere about whether this view should rely on external state. One
can imagine a naiive implementer making totalHoldings dependent on some kind of external oracle without realizing the security implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interface is unopinionated in the same way totalSupply is unopinionated for ERC20. We expect most implementations to use a shares based model. It will always be up to the integration layer to control for "safe" vaults based on their own security model

Copy link
Contributor

Choose a reason for hiding this comment

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

Why totalHoldings and not totalUnderlying, underlyingHoldings, or underlyingReserves. I think this is the only instance where "Holdings" is used to refer to underlying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like totalUnderlying as well. @transmissions11 @JetJadeja wdyt?

eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
@transmissions11
Copy link
Contributor

transmissions11 commented Jan 4, 2022

we need to be more clear about the decimal scalars for each of these functions. like exchangeRate is scaled by decimals() but totalHoldings() scaled by underlying().decimals()

eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated

e.g. `_shares * exchangeRate() / baseUnit() = _value`.

`exchangeRate() * totalSupply()` MUST equal `totalHoldings()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to divide by the right scalar cuz totalHoldings decimals is scaled by underlying decimals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great call, should we remove this MUST condition or add the decimals scalar? Leaning former

Copy link
Contributor

Choose a reason for hiding this comment

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

im not too opinionated either way tbh, just want to make sure we clarify the decimals one way or another

Copy link
Contributor

Choose a reason for hiding this comment

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

i do think exchangeRate() * totalSupply() / SCALAR_HERE = totalHoldings() is a good invariant tho, leaning towards keeping

Copy link
Contributor

Choose a reason for hiding this comment

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

It's easier and more general to use a calculateShares(uint256 underlyingAmount) returns (uint256 shares amount). No need to think about scalars, and supports non-linear exchange rates with precision.

Likewise, you should have the symmetrical calculateUnderlying(uint256 sharesAmount) returns (uint256 underlyingAmount).

This pattern has been used in the yield oracles with great success, and partially in the lido staked ether contract where you have also symmetrical functions for asset conversions.

Copy link
Contributor

@alcueca alcueca Jan 5, 2022

Choose a reason for hiding this comment

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

To give another example, if a vault is implemented with a fee on deposits or withdrawals, it won't be possible for an integrating contract to know the deposit or withdrawal output, without specific knowledge of those fees.

By putting any conversion logic behind the calculate functions, vaults with non-linear math or fees can be integrated seamlessly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion as part of the OPTIONAL interface instead of or in addition to exchangeRate(). As an integrating contract, the only time I'd want to use exchangeRate() would be to calculate one of these two conversions.

Copy link
Contributor

@alcueca alcueca Jan 5, 2022

Choose a reason for hiding this comment

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

I would suggest this as instead exchangeRate. From the point of an integrating contract, you are not usually interested in the exchange rate itself, but in a specific conversion of a for b.

If you would still be interested in the exchange rate for a given use case, you can obtain it by passing one full unit as the amount parameter.

Furthermore, using two parameterized functions removes the constraint on how tokens are converted for shares, which the EIP forces to a linear, no-fee formula (otherwise exchangeRate is true only for one full unit).

If still unconvinced, I'd like to hear the arguments in favour of a exchangeRate variable over two symmetrical functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm writing a generic ERC4626 router and all of your advice has become very appropriate as I encounter some of the nuances of the implementation details between vaults of different types

eip-4626.md Outdated Show resolved Hide resolved
Co-authored-by: t11s <greenfeatherhelp@gmail.com>
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
@alcueca
Copy link
Contributor

alcueca commented Jan 5, 2022

deposit, withdraw and redeem might seem like familiar options for function names, but there is a gap.

  • deposit: User chooses an amount of underlying. Underlying goes into the vault. User mints shares (or some kind of receipt).
  • withdraw: User chooses an amount of underlying. Underlying gets out from the vault. User burns shares (or some kind of receipt).
  • redeem: User chooses an amount of shares (or some kind of receipt). Underlying gets out from the vault. User burns shares (or receipt).

There is a function missing:

  • x: User chooses an amount of shares (or some kind of receipt). Underlying goes into the vault. User mints shares (or receipt).

If this sounds a bit strange, it is easier to see it by comparing to a fully-fledged AMM. In Pool.sol from YieldSpace, there are four functions, which can be mapped onto a generic yield-bearing vault:

  • sellBase: Choose an exact amount of asset A to give away for an undetermined amount of asset B
  • buyBase: Choose an exact amount of asset A to receive in exchange for an undetermined amount of asset B
  • sellFYToken: Choose an exact amount of asset B to give away for an undetermined amount of asset A
  • buyFYToken: Choose an exact amount of asset B to receive in exchange for an undetermined amount of asset A

Similar functionality is present in other AMMs like Uniswap, usually merged into one sell and one buy functions because both assets in a regular AMM pair are identical.

In the case of a generic Yield-Bearing Vault, it might help to organize the asset transfer functions in the same way, to ensure that the features provide complete and consistent coverage of use cases.

eip-4626.md Outdated

## Abstract

The following standard allows for the implementation of a standard API for yield bearing vaults within smart contracts. This standard provides basic functionality for depositing and withdrawing tokens and reading balances with an optional extension for tokenized vaults using ERC-20.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the ERC20 tokenized vault optional?

I understand that there might be an interest to describe the Yield-Bearing Vault specification separately, to be able to combine it later on with ERC-20, ERC-721, ERC-1155, or some other custom tokenization standard.

However, the EIP does a halfway job of dissociating itself from ERC-20, stating it as optional, which turns it confusing.

I would suggest to either remove the optionality of tokenized vaults, and specify a pure Yield-Bearing Vault EIP, or make ERC-4626 Vaults to inherit from ERC-20 as a requirement. Either case would make the EIP clearer and more robust.

My preference would be to make ERC-4626 Yield-Bearing Vaults to compulsorily inherit from ERC-20, and to provide a complete standard that also matches closely the existing implementations. I think it will be quite hard to specify a Yield-Bearing Vault with no relationship to any tokenizing standard, yet with enough meaning to be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

i am ok with enforcing tokenization if consensus is there but we strayed away from specifying it cuz there are many different approaches to doing it beyond a simple non-rebasing erc20 and we want the core deposit and withdraw logic to be adopted widely

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you can require the vault to be an ERC20 in which balances represent ownership of the vault (shares), without specifying how shares are allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In favor, will enforce ERC20 compatibility

eip-4626.md Outdated

#### withdraw

`function withdraw(address _to, uint256 _value) public returns (uint256 _shares)`

Choose a reason for hiding this comment

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

I love the idea of standardizing Vaults.
I respect your work with Rari and Fei.

But every single Vault Product in DeFi uses shares to account for withdraw, this recommended standard is invalidating every other vault code.

Additionally some / most shares end up being a token of their own and people think of them that way, (bDigg, bveCVX, yveCRV, etc...)

I recommend having withdraw use the number of shares.

For context:
Yearn V2:
https://github.com/yearn/yearn-vaults/blob/67e7ddda69c24f6e85a56338a3519a461b20d107/contracts/Vault.vy#L1025

Yearn V1:
https://github.com/yearn/yearn-protocol/blob/7b7d4042f87d6e854b00de9228c01f6f587cf0c0/contracts/vaults/yVault.sol#L101

Badger Vaults:
https://github.com/Badger-Finance/badger-sett-1.5

Mushroom Finance:
https://etherscan.io/address/0x0c0291f4c12f04da8b4139996c720a89d28ca069#code#L887

Inverse Finance:
https://github.com/InverseFinance/inverse-protocol/blob/55ff82d2e2af075832555fa3e933f73410845e43/contracts/vault/Vault.sol#L53

The only notable example that doesn't follow this pattern is Idle Finance, which has a unique interface:
https://github.com/Idle-Labs/idle-contracts/blob/develop/contracts/IdleTokenV3_1.sol

As an addendum, to avoid further invalidating already existing projects, there could be multiple overloaded withdraw signatures similarly to how Yearn V2 did which would allow most projects to comply to the standard without changing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am open to reordering or making redeem required which is the shares based model. I do like the symmetry of deposit/withdraw both using the same underlying token accounting and having redeem or withdrawShares use the shares model. I'm also comfortable having withdraw be the shares and withdrawUnderlying be for underlying. Combined with @alcueca's feedback, I believe the standard can be somewhat more opinionated on some of these details.

Wdyt @transmissions11?

Choose a reason for hiding this comment

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

Agree on this, I think the norm of most protocols is to use value for depositing & shares for withdrawing, so making the default withdraw function to use shares will minimise unexpected surprises to devs.
Since the interface is for a vault, having an additional withdrawValue is maybe necessary? But I think the devs can easily do some math and use the core withdraw function if necessary. This is probably better than forcing the function in the interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please, see above. I think that both the withdraw and deposit features should accept both value or shares as inputs.

Choose a reason for hiding this comment

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

Happy to agree to disagree, I'm not making any statement about what I think the code should look like.

The code that is being used in production by the majority of protocols looks like the following
function deposit(uint256 amount)
function withdraw(uint256 shares)

Happy to go into detail as to why that is the case, but this interface has survived over hundreds of forks on all EVM compatible chains, and as such shows wisdom in its simplicity

Copy link

Choose a reason for hiding this comment

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

All of the cToken-based protocols (Compound, Rari, Cream, etc) use mint(uint amount), redeem(uint shares), redeemUnderlying(uint amount) so I think whichever way is chosen there's going to be a group of existing protocols that aren't natively compatible.

Choose a reason for hiding this comment

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

withdraw standing for shares is flat out illogical. you don't "withdraw" a check at the bank, or "withdraw" a winning lottery ticket. you redeem them. you withdraw $100

You can't withdraw to someone else either, the bank analogy falls flat

Choose a reason for hiding this comment

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

withdraw standing for shares is flat out illogical. you don't "withdraw" a check at the bank, or "withdraw" a winning lottery ticket. you redeem them. you withdraw $100

As for withdrawing $100 dollars, the reason why fork over fork never killed the withdraw(shares) is the level of complexity or close to impossibility to withdraw all shares.

Illustrated by literally the first withdraw I found on Rari's smart contract

https://etherscan.io/tx/0xcada09b35d35aaee76fdaf2a00b8f275b07ced7eb8c80cf48cc5dac42c4832ea

Screenshot 2022-01-07 at 15 40 02

Screenshot 2022-01-07 at 15 40 14

Did the depositor really want to keep those 0.02 tokens? Or is the implementation that forces them into something they don't want?

Copy link

Choose a reason for hiding this comment

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

You can't withdraw to someone else either, the bank analogy falls flat

@GalloDaSballo

Definitions:

(Google) withdraw: remove or take away (something) from a particular place or position
(Merriam Webster) withdraw: to remove (money) from a place of deposit

It doesn't seem to say anything about where the money ends up (the destination), so I think you can withdraw into someone else's account. On apps like Binance (a crypto exchange), you just withdraw to some arbitrary wallet address, Binance doesn't care who owns the destination address. If anything, withdraw is commonly used to refer to a separate destination than your own.

But, most importantly, withdrawals have always historically referred to the actual asset (the underlying asset). I agree that redeem would make more sense here (when it's in terms of the wrapper token/shares), but I can understand your point of view since many protocols have used the term withdraw here (IMO) incorrectly.

Copy link

Choose a reason for hiding this comment

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

the reason why fork over fork never killed the withdraw(shares) is the level of complexity or close to impossibility to withdraw all shares

Regardless, the term "redeem" is perhaps better-fitted than "withdraw".

I agree, there definitely may be an added complexity when supporting withdraw(underlying).

eip-4626.md Outdated
Comment on lines 28 to 34
#### deposit

`function deposit(address _to, uint256 _value) public returns (uint256 _shares)`

Deposits `_value` tokens into the vault and grants ownership of them to `_to`.

MAY return a pro-rata ownership `_shares` value corresponding `_value`, if not MUST return `0`.
Copy link

@mrenoon mrenoon Jan 6, 2022

Choose a reason for hiding this comment

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

As it stands now, the inputs to an EIP-4626 pool must be the underlying, which is the unit for valuing the pool and the user's position.

I would like to make a case to allow the inputs to be something else, called the deposit token. There are a few cases where the deposit token could be different from the underlying:

  1. A vault could take in aUSDC instead of USDC, due to certain design choices. The underlying unit of accounting here, is still USDC
  2. A wrapper contract could be made to take in gOHM or xSUSHI, use OHM/SUSHI as the underlying unit of accounting, and essentially produce a wrapped_gOhm that adheres to EIP-4626
  3. A vault could take in a UniswapV2 LP token, while choosing to use liquidity (sqrt(token1Amount*token2Amount)) as the underlying unit of accounting.

To allow for these use cases, we just need to make one additional view function and three semantic changes:

  • Additional function:
    • depositToken() public view returns (address)
    • Meaning: the token for depositing and withdrawing, which could be the same as underlying()
  • Semantic change for depositing:
    • Rename variables from
      function deposit(address _to, uint256 _value) public returns (uint256 _shares)
      
      to
      function deposit(address _to, uint256 _amount) public returns (uint256 _shares)
      
    • Meaning: deposits _amount of deposit tokens, which is worth _value = _shares * exchangeRate() in value into the vault and grants ownership of the value to _to
  • Semantic change for redeeming (shares):
    • Rename variables from
      function redeem(address _to, uint256 _shares) public returns (uint256 _value)
      
      to
      function redeem(address _to, uint256 _shares) public returns (uint256 _amount)
      
    • Meaning: Redeems a specific number of _shares for deposit tokens and transfers them to _to. The value of the redeemed tokens will be _shares * exchangeRate()
  • Semantic change for withdrawing (deposit tokens):
    • Rename
      function withdraw(address _to, uint256 _value) public returns (uint256 _shares)
      
      to
      function withdraw(address _to, uint256 _amount) public returns (uint256 _shares)
      
    • Meaning: withdraws _amout deposit tokens from the vault and transfers them to _to.

In short: These semantic changes do not alter how things currently work but will make the EIP more general and future-proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need for this, as what the vault does with the deposited tokens internally is completely irrelevant to this ERC interface.

underlying signifies what comes in and out of the vault and what totalHoldings() and such is measured in, we do not specify what happens to deposited underlying tokens. you can impl the vaults you proposed without additional metadata.

Copy link

@mrenoon mrenoon Jan 6, 2022

Choose a reason for hiding this comment

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

Let's say we want to implement this vault A:

  • User deposits cUSDC
  • The vault claims COMP over time and convert to more cUSDC

Option 1: If we use the current standard:

  • underlying() = cUSDC
  • exchangeRate() = rate of how much more cUSDC a user will get, due to COMP
  • balanceOfUnderlying(user) = cUSDC balance

Option 2: if we apply the proposed semantic changes here, it can be:

  • underlying() = USDC
  • exchangeRate() = rate of how much more USDC user is getting, from both Compound's rate and claiming COMP in this vault
  • balanceOfUnderlying(user) = USDC-equivalent balance of the user

If we take the perspective of let's say a UI to show the details of this vault, it makes more sense to show the user balance in USDC using balanceOfUnderlying(), and easier to compare this vault with other vaults with the same USDC underlying.
Another example is from the perspective of a protocol to tokenise the yield of this vault with respect to USDC, where Option 2 makes a lot more sense as well.

Copy link
Contributor

@alcueca alcueca Jan 6, 2022

Choose a reason for hiding this comment

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

I think that explicitly specifying a multi-layer approach adds complexity to the standard, which should be kept as simple as possible. Not all existing vaults will be immediately compliant and that must be accepted.

As such, I don't think that a depositToken() method should be added.

However

On specifying inputs based on value or shares, that I agree for the sake of completeness, familiarity, and ease of integration.

  1. Deposit exact value underlying, receive some shares
  2. Receive exact shares, deposit underlying for some value
  3. Withdraw exact shares, receive underlying for some value
  4. Receive exact value of underlying, by withdrawing some shares.

This follows the pattern seen in AMMs that also deal with two tokens (in this case the underlying and the vault tokens).

Externally, the vault does the same job of exchanging between the two, and it is irrelevant the internal mechanism used to determine the exchange rate.

I'm not arguing to provide an interface identical to any existing AMM, but a familiar set of methods, proven to work in an equivalent use case, would definitely help with integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it makes a lot of sense to add a "mint" method which does 3. Withdraw exact shares, receive underlying for some value @transmissions11 @JetJadeja wdyt?

Copy link

Choose a reason for hiding this comment

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

@mrenoon Perhaps whatever wants the USDC value can go through two interfaces. First determine SHARES/aUSDC value via ERC-4626 and then determine aUSDC/USDC via say a DEX or another ERC-4626. This is kind of composing two "valuation" interfaces which have "calculate" functions. Could that be standardized independent of the rest of EIP-4626? Both ERC-4626 and a DEX could implement this "valuation" interface. Then the logic which composes two valuation interfaces could be wrapped up into its own valuation interface, achieving your goal of going through this one interface which calculates the SHARES/USDC valuation directly.

Edit: Alternatively, one could wrap an ERC-4626 using aUSDC as its underlying into another ERC-4626 which behaves "as if" USDC is the underlying. This implementation may go through two other ERC-4626s or similar to above, may go through a DEX.

Side note: You could nearly argue that a DEX could implement ERC-4626 (who says it needs to be a new shares token? One token in the pair acts as an investment into the pool where one expects the number of the other token to increase over time).

@Joeysantoro Joeysantoro changed the title EIP-4626: Yield Bearing Vault Standard EIP-4626: Tokenized Vault Standard Jan 7, 2022
and withdrawFrom, redeemFrom, events
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated

#### deposit

`function deposit(address _to, uint256 _value) public returns (uint256 _shares)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Any way that address to send shares to can be the second parameter? In yearn vaults, we have them in this order because in decreasing order of usage:

  • deposit everything for yourself
  • deposit a specific amount for yourself
  • deposit a specific amount and give to someone else

Depositing everything to another person is rare

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for withdraw

Copy link
Contributor

Choose a reason for hiding this comment

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

cool with me, makes sense in languages (vyper) that have optional args

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a decent preference for the other order given it follows ERC20 spec. Wouldn't yearn need to still make adapters for the remaining functions anyway? What is the gain by switching if its only limited backward compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note, the likelihood any protocol makes an adapter is quite low, I didn't make the suggestion for backwards compatibility reasons.

Anyways, I don't think ERC20 plays a huge role here in this spec in terms of driving the interface of these operations. Merely consider that the addition of override functions might be a good way to reduce gas costs for common operations, if the ordering is correct.

So adding:

function deposit() public returns (uint256 _shares) // Deposit all to self
function deposit(uint256 _value) public returns (uint256 _shares) // Deposit some to self
function deposit(uint256 _value, address _to) public returns (uint256 _shares) // Deposit some to other

As another side note... I really wish people would start writing ERC specifications in YAML per EIP-2069 but that never really took off 😞

eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
eip-4626.md Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

Can the discussions here please be moved to the discussion thread? https://ethereum-magicians.org/t/eip-4626-yield-bearing-vault-standard/7900

@lightclient lightclient closed this Jan 8, 2022
@lightclient lightclient reopened this Jan 8, 2022
@lightclient lightclient merged commit e86b7fe into ethereum:master Jan 8, 2022
@aallamaa
Copy link

aallamaa commented Jan 8, 2022

Would it be possible to add a deposit with permit (ERC2612 permit) function “depositWithPermit” in the standard ?

@MicahZoltu
Copy link
Contributor

Please use the discussions-to URL at the top of the EIP to discuss this.

PhABC pushed a commit to PhABC/EIPs that referenced this pull request Jan 25, 2022
* Create eip-4626.md

* Update eip-4626.md

* Update eip-4626.md

Co-authored-by: Micah Zoltu <micah@zoltu.net>

* Update eip-4626.md

* Update eip-4626.md

* Apply suggestions from code review

* Update eip-4626.md

Co-authored-by: William Schwab <31592931+wschwab@users.noreply.github.com>

* Update eip-4626.md

Co-authored-by: William Schwab <31592931+wschwab@users.noreply.github.com>

* Update eip-4626.md

Co-authored-by: t11s <greenfeatherhelp@gmail.com>

* Update eip-4626.md

Co-authored-by: t11s <greenfeatherhelp@gmail.com>

* Update eip-4626.md

Co-authored-by: t11s <greenfeatherhelp@gmail.com>

* Update eip-4626.md

Co-authored-by: t11s <greenfeatherhelp@gmail.com>

* Update eip-4626.md

Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>

* Update eip-4626.md

* Backward compatibility

and withdrawFrom, redeemFrom, events

* Fixes

* Update eip-4626.md

* Apply suggestions from code review

* move eip into eips folder

Co-authored-by: Micah Zoltu <micah@zoltu.net>
Co-authored-by: William Schwab <31592931+wschwab@users.noreply.github.com>
Co-authored-by: t11s <greenfeatherhelp@gmail.com>
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Co-authored-by: lightclient@protonmail.com <lightclient@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet