I rewrote the parameters and the text referring to them in most functions to fix these consistency issues. I’m inclined to change previewWithdraw and previewRedeem as suggested.
1 Like
In last changes (https://github.com/ethereum/EIPs/commit/6c0b89e7838d43042bcb0e2bb72b573591549b74) in Withdraw sender has been renamed to caller. So now we will log not owner as you answered me and is it ok to change owner to caller in event to log? Do we need log both owner and caller?
previewWithdrawshould have anownerparameter like thewithdrawfunction does. The function caller can be different to the owner of the shares for withdrawals.
The preview* functions don’t take into consideration limits imposed on the owner or caller. If integrators want to call a preview* with those limits into consideration, they should call the appropriate max* function first.
The event fields needed a bit of further thinking, indeed. Proposal.
Deposit(caller, receiver, assets, shares) → Deposit(caller, owner, assets, shares)
Withdraw(caller, receiver, assets, shares) → Withdraw(caller, receiver, owner, assets, shares)
Hi there 
I’m late to the review and new to the whole process. This seems like a pretty important EIP and is directly relevant to something I’ve been working on. I’ll try to review the comments and the EIP and provide timely feedback if I have anything useful to say about it.
I saw this was about to be moved to “Last Call” and wanted to introduce myself and let you know I may have some late feedback 
Quick question: The first post here specifically refers to Compound, Aave, Fuse, Yearn, Rari Vaults, Idle, and xSushi. Are people from those protocols aware of this EIP and have they provided their feedback? A standard like this could impact them and others, e.g. Bancor, Balancer, not mentioned. I’m wondering if enough eyes are on this since it seems to have been introduced not long ago on Jan 3.
1 Like
The EIP states that the “preview” functions should not revert. This leaves me wondering what to do with absurd input:
- what should
previewDepositandpreviewWithdrawreturn is the “expected” value doesn’t fit into a uint256 ?- this could happen if each asset unit is represented by multiple shares units, and the previewDeposit is called with type(uint256).max as an input.
- what should
previewWithdrawandpreviewRedeemreturn is the input is higher thentotalAssets/totalSupply?
Has anyone worked on an ERC4626 implementation based on bounding curve, I’d be curious to see that.
2 Likes
I am working on something that can be thought of as a “Vault + Bonding Curve” which is why this EIP got my attention. My priority is to work out my own thing (without bias) and then I hope to compare what I come up with to this EIP. At that point I hope to have something useful to say about this proposal, but I’m not there yet obviously 
Btw, I heard about this EIP from an ETH Denver session (link to the exact timestamp the comment is made):
https://www.youtube.com/watch?v=wPXvmjZxjj8&t=879s
where Jai Bhavnani says,
“Joey, who’s in the crowd here, even wrote a new token standard ERC4626 that makes it so that we literally own the standard that our fuse pools are using.”
I hadn’t heard of EIP4626 until then and found it a little odd that a protocol would claim to own the standard and use it as a marketing tool 
1 Like
previewFunctions should take already validated input, at least that was my working assumptions when I experimented with this EIP twice. There is no point to burden calculation logic with validation logic if those can be kept separate. This would also break interoperability of this standard as expectations about what happens in shares calculations wouldn’t be kept. I also don’t see a scenario in which previews get input higher than total assets managed by the Vault and somehow are supposed to return correct value. Let’s keep in mind that Vaults are purely accounting and I think this EIP also tries to standardize this notion.
As for current EIP doc. I would stress more that EIP in current form works ONLY for single underlying token. And multiple underlying Vaults should propose there own extension. This is something I’ve ran into while doing second small experiment with building Stablecoin. Share accounting will break or will be unpredictable from the perspective of 3rd party if multiple underlying tokens are used.
Here’s a code for 4626-Stablecoin (based on USM which I think is in part work of @albertocuestacanada). A lot of concessions were made, but I only wanted to see if it’s feasible to also build stable currency of this standard without breaking out of it (yes, can be done easily) :
I know this is in last call, but a few comments.
Agreeing with this comment, we should remove the requirement of EIP-2612 on ERC-4626: Tokenized Vaults, to only require EIP-20.
- name: withdraw
type: function
stateMutability: nonpayable
inputs:
- name: assets
type: uint256
- name: receiver
type: address
- name: owner
type: address
outputs:
- name: shares
type: uint256
For withdraw and redeem, including an owner param introduces an easy pitfall to fall into: users redeeming/withdrawing shares they don’t own. The common scenario with vaults is requiring msg.sender to be the owner of the shares. This standard makes it pretty easy for whoever who intends to implement the most common scenario to forget the require(msg.sender == owner) and introduce a devastating vulnerability.
While exposing owner provides extra flexibility, couldn’t that signature be exposed as a signature outside of the standard, if the uncommon scenario is desired, and only expose redeem(uint256 assets, address receiver)? In addition, a redeem on behalf of can be easily implemented by an integrating contract by first having the user transfer the shares to the integrating contract, and the integrating contract calling redeem(userShares, userRedeemingOnBehalfOf), sending the assets to the user as the recipient.
Thanks for the additional feedback!
Its optional and a super useful standard, I’m inclined to keep it as-is.
This is only possible if the spec isn’t followed. Imo the feature saves a ton of gas on a useful interaction and it shares the same approval system.
We’ve had representatives from Aave, Alchemix, Maple, Tracer, Paradigm, and many other teams comment on and potentially integrate the spec. The Authors span Fei, Rari, Yield, and Yearn and have been discussing the spec at length across an extremely diverse set of use cases and nuances.
Probably a mis-speak. Obviously no team owns the spec. What he probably meant and how I discuss the spec is that the Tribe DAO will be building a TON of its infrastructure around the standard (Turbo, Vaults, Fuse plugins, fixed income, etc) and that its adoption will further the interoperability with Tribe products.
Bingo, multiple underlyings + alternate reward tokens are left out of scope as the standard is already covering quite a wide array of use cases.
1 Like
I’m not sure I understand what you mean by This is only possible if the spec isn’t followed. I don’t see anywhere in the 4626 spec protecting against unwanted redemptions-on-behalf of. I didn’t see the solmate implementation linked on the EIP site until now (solmate/ERC4626.sol at fd67739ec00b8441605f6dcf04d83458884fa3d0 · Rari-Capital/solmate · GitHub) and the approval scheme of checking the msg.sender’s allowance for from (in the new spec owner) makes a lot of sense, so maybe you meant the ERC20 spec not being followed, but maybe a note about making sure approvals are accounted for correctly in redeem/withdraw in the 4626 spec can help guide users to make sure they implement redeem/withdraw correctly?
Despite the above, I definitely do see the point about gas savings, and can see it enabling smoother implementations, so I think it makes sense to leave the interface as is.
Is anyone working on an extension that covers multiple underlyings? I think this standard could also be used for LP positions, and projects like GUNI (see: Sorbet Finance - Automation for Uniswap, Quickswap and PancakeSwap)
2 Likes
The owner parameter works the same as transferFrom, implementors should use similar logic from there to fulfill this feature.
You would need the concept of a price or exchange rate here for multiple assets to work. We’ve also heard of people ask for one underlying asset, and a second redemption asset for farming, but that use cases is also more complex and should be handled separately if desired to have a formal spec around it.
In over a year, I’ve heard of many people ask for this and not a single version of it was built out, so I’m inclined to think no one actually needs this.
I feel like supporting multiple shares and assets would open up a lot more use cases for this standard. I’m making a stablecoin similar to shortdoom’s and while it is possible to use this standard, it doesn’t cover the full interface of the contract (shortdoom had to add additional methods for a second share token), which is less than ideal and IMO makes it not worth conforming to the standard.
This standard has been finalized, explicitly with the intent of only working with single asset vaults.
You are definitely welcome to take the design of this ERC and try to expand on it separately for a multi-asset or pooled asset use case, or a number of other use cases that might deviate from the standard, like separate base and reward assets.
There are numerous ideas we have heard on how to expand this ERC, but we found it difficult to support all of these in practice, and found them somewhat underutilized in real use cases. Perhaps if a separate standard or standards existed, that would change.
2 Likes
Why don’t this ERC utilize EIP-2612 for a managed asset? I would prefer to have depositWithPermit and mintWithPermit with additional argument bytes memory permitData doing smth like this: solidity-utils/Permitable.sol at master · 1inch/solidity-utils · GitHub
Have you seen the router on github at fei-protocol/ERC4626 (for some reason the forum isn’t allowing me to post the link)
It could be used to deposit with permit on any vault
Checkout a wonderful explainer of tokenized vault Ethereum standard by @JetJadeja on PEEPanEIP
