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 17 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.
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.
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)
.