Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
EIP-4626: Tokenized Vault Standard #4626
Changes from 6 commits
dfc160b
90c4fbb
8adc0d9
b9b6eec
636f8e2
e4cb510
39a614e
e6abeeb
f198798
9de5af5
b0381e6
b30ef7a
98d9490
efb8c0a
46b47bf
50fe1da
295f442
8a0ff46
6fb327b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
Depositing everything to another person is rare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for
withdraw
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
As another side note... I really wish people would start writing ERC specifications in YAML per EIP-2069 but that never really took off 😞
There was a problem hiding this comment.
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:
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:
depositToken() public view returns (address)
underlying()
_amount
of deposit tokens, which is worth_value = _shares * exchangeRate()
in value into the vault and grants ownership of the value to_to
_shares
for deposit tokens and transfers them to_to
. The value of the redeemed tokens will be_shares * exchangeRate()
_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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Option 1: If we use the current standard:
Option 2: if we apply the proposed semantic changes here, it can be:
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.
There was a problem hiding this comment.
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
orshares
, that I agree for the sake of completeness, familiarity, and ease of integration.value
underlying, receive someshares
shares
, deposit underlying for somevalue
shares
, receive underlying for somevalue
value
of underlying, by withdrawing someshares
.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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 defaultwithdraw
function to useshares
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 corewithdraw
function if necessary. This is probably better than forcing the function in the interface.There was a problem hiding this comment.
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
anddeposit
features should accept bothvalue
orshares
as inputs.There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Did the depositor really want to keep those 0.02 tokens? Or is the implementation that forces them into something they don't want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GalloDaSballo
Definitions:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, the term "redeem" is perhaps better-fitted than "withdraw".
I agree, there definitely may be an added complexity when supporting
withdraw(underlying)
.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
totalHoldings
and nottotalUnderlying
,underlyingHoldings
, orunderlyingReserves
. I think this is the only instance where "Holdings" is used to refer to underlying.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given compound's
mint
/redeemUnderlying
methods have been replaced by yearn'sdeposit
/withdraw
in this EIP, it might make sense to retain parallel structure, and replaceredeem
withwithdrawShares
.I think this makes the method's use case a bit more clear at a glance, though
redeem
will always have a nice ring to it...¯\_(ツ)_/¯There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but this is not
redeemUnderlying
it'sredeem
, which has similar behavior even in compound:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup its a 1-1 of
redeem
from compound!But if there is no
redeemUnderlying
, onlywithdraw
andwithdraw
+redeem
do the same thing, just one is scaled for shares, it might make sense to associate the two. (in the same way compound associatesredeem
andredeemUnderlying
)Again, I think
redeem
sounds nicer, whilewithdrawShares
describes what the method does in context of removingredeemUnderlying
in favor ofwithdraw
. Very minor hairs being split.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair points
thats t11s for ya ;P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the documentation here is reversed, the return parameter here is rightfully value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 keepingThere was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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