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
156 changes: 156 additions & 0 deletions eip-4626.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
# 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

Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved
eip: 4626
title: Yield Bearing Vault Standard
description: A standard for yield bearing vaults.
author: Author: Joey Santoro (@joeysantoro), t11s (@transmissions11), Jet Jadeja (@JetJadeja)
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved
discussions-to: <URL>
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved
status: Draft
type: Standards Track
category: ERC
created: 2021-12-22

Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved
## Simple Summary

A standard for yield bearing vaults.

## 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


## Motivation

Yield bearing vaults have a lack of standardization leading to diverse implementation details. Some various examples include lending markets (Compound, Aave, Fuse), aggregators (Yearn, Rari Vaults, Idle), and intrinsically interest bearing tokens (xSushi). This makes integration difficult at the aggregator or plugin layer for protocols which need to conform to many standards. This forces each protocol to implement their own adapters which are error prone and waste development resources.

A standard for yield bearing vaults will allow for a similar cambrian explosion to ERC-20, unlocking access to yield in a variety of applications with little specialized effort from developers.


## Specification

### Methods
lightclient marked this conversation as resolved.
Show resolved Hide resolved

#### 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 😞


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

@davidcallanan davidcallanan Jan 7, 2022

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


MAY represent `_shares` using internal accounting or an ERC-20 token.

If pro-rata shares ownership is implemented, the vault SHOULD implement `balanceOf`, `redeem`, `totalSupply` and `exchangeRate`.


#### 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

@Joeysantoro Joeysantoro Jan 6, 2022

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

@davidcallanan davidcallanan Jan 7, 2022

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

@davidcallanan davidcallanan Jan 7, 2022

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


Withdraws `_value` tokens from the vault and transfers them to `_to`.

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


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


Returns the total amount of underlying tokens held/managed by the vault.


#### balanceOfUnderlying
`function balanceOfUnderlying(address _owner) public view returns (uint256)`

Returns the total amount underlying tokens held in the vault for `_owner`.

#### underlying
`function underlying() public view returns (address)`

Returns the address of the token the vault uses for accounting, depositing, and withdrawing.

SHOULD return a token implementing the ERC-20 standard.


#### totalSupply

`function totalSupply() public view returns (uint256)`

Returns the total number of unredeemed vault shares in circulation.

OPTIONAL - This method is only needed for vaults that implement a pro-rata share mechanism for deposits.

#### balanceOf

`function balanceOf(address _owner) public view returns (uint256)`

Returns the total amount of vault shares the `_owner` currently has.

OPTIONAL - This method is only needed for vaults that implement a pro-rata share mechanism for deposits.

#### redeem
Copy link
Contributor

@JTraversa JTraversa Jan 5, 2022

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's deposit/withdraw in this EIP, it might make sense to retain parallel structure, and replace redeem with withdrawShares.

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...¯\_(ツ)_/¯

Copy link
Contributor

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's redeem, which has similar behavior even in compound:

Screen Shot 2022-01-04 at 9 28 55 PM

Copy link
Contributor

@JTraversa JTraversa Jan 5, 2022

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, only withdraw and withdraw+redeem do the same thing, just one is scaled for shares, it might make sense to associate the two. (in the same way compound associates redeem and redeemUnderlying)

Again, I think redeem sounds nicer, while withdrawShares describes what the method does in context of removing redeemUnderlying in favor of withdraw. Very minor hairs being split.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair points

Very minor hairs being split.

thats t11s for ya ;P


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

Redeems a specific number of `_shares` for underlying tokens and transfers them to `_to`.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

yup


OPTIONAL - This method is only needed for vaults that implement a pro-rata share mechanism for deposits.

#### exchangeRate

`function exchangeRate() public view returns (uint256)`

The amount of underlying tokens one `baseUnit` of vault shares is redeemable for.
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved

e.g. `_shares * exchangeRate() / baseUnit() = _value`.
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved

`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

@Joeysantoro Joeysantoro Jan 5, 2022

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


OPTIONAL - This method is only needed for vaults that implement a pro-rata share mechanism for deposits.

#### baseUnit

`function baseUnit() public view returns(uint256)`

The decimal scalar for vault shares and operations involving `exchangeRate()`.

OPTIONAL - This method is only needed for vaults that implement a pro-rata share mechanism for deposits.
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved


### Events

#### Deposit

MUST be emitted when tokens are deposited into the vault.

`event Deposit(address indexed _from, addres indexed _to, uint256 _value)`
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved

Where `_from` is the user who triggered the deposit and approved `_value` underlying tokens to the vault, and `_to` is the user who is able to withdraw the deposited tokens.


#### Withdraw

MUST be emitted when tokens are withdrawn from the vault by a depositor.

`event Withdraw(address indexed _owner, addres indexed _to, uint256 _value)`
Joeysantoro marked this conversation as resolved.
Show resolved Hide resolved

Where `_from` is the user who triggered the withdrawal and held `_value` underlying tokens in the vault, and `_to` is the user who received the withdrawn tokens.


## Rationale

The vault interface is designed to be optimized for minimal implementation and integration logic while maintaining flexibility for both parties. Details such as accounting and allocation of deposited tokens are intentionally not specified, as vaults are expected to be treated as black boxes on-chain and inspected off-chain before use.

lightclient marked this conversation as resolved.
Show resolved Hide resolved
## Reference Implementation

[Solmate Minimal Implementation](https://github.com/Rari-Capital/solmate/pull/88) - a tokenized vault using the ERC-20 extension with hooks for developers to add logic in deposit and withdraw.

[Rari Vaults](https://github.com/Rari-Capital/vaults/blob/main/src/Vault.sol) are an implementation that is nearly ready for production release. Any discrepancies between the vaults abi and this ERC will be adapted to conform to the ERC before mainnet deployment.

## Security Considerations

This specification has similar security considerations to the ERC-20 interface. Fully permissionless yield aggregators, for example, could fall prey to malicious implementations which only conform to the interface but not the specification.

## Copyright

Copyright and related rights waived via [CC0](https://creativecommons.org/publicdomain/zero/1.0/).