Erc4626 and ERC20 in Vault code #30
Conversation
chore: bump ape to 0.3, fix errors + install instructions
* feat: add tests (todo) * chore: bump ape to 0.3, fix errors + install instructions * fix: fix sol lint * test: add strategy management tests Co-authored-by: anticlimactic <9568756+anticlimactic@users.noreply.github.com>
* fix: use vyper interface for erc20detailed * feat: add simple liquid strategy mock * feat: add locked strategy * feat: add lossy strategy * chore: fix lint * chore: remove unused line * refactor: refactor strategy mocks * feat: add debt update and the corresponding tests * fix: assets and decimals should be immutable * feat: implement strategy report + tests * chore: install vyper on ci Co-authored-by: panda <87183122+pandadefi@users.noreply.github.com>
* feat: add fee manager + tests * feat: add distribute + tests
| return self._amountForShares(shares) | ||
|
|
||
| @external | ||
| def deposit(assets: uint256, receiver: address) -> uint256: |
There was a problem hiding this comment.
doesn't implementing 4626 in here mean that we now only have one deposit interface? this means we can't have separate deposit functions for different integrators, and that they will all have to integrate through 4626 (and will just have to use a default withdrawal queue). is this desirable?
There was a problem hiding this comment.
it doesn't mean that imo. We can overload deposit so it's compliant with ERC4626 and have other more flexible ways of withdrawing. Wdyt?
There was a problem hiding this comment.
i don't think vyper has the same overloading features that solidity has:
vyper has a more limited overloading where you can set default values for variables. in this case we would have to set a default value for the queue of strategies to withdraw.
i think the main challenge of having ERC4626 joined with the vault code is that if we have a periphery contract that returns a default queue, then the vault will now depend on that contract. if our goal is to to have the vault be immutable with as few external dependencies as possible, then this is probably not the right way to go, unless you want to manage the default queue within the vault as well. in which case we can just use the overloading described above with a vault-managed default strategy queue.
There was a problem hiding this comment.
I agree having a separated withdrawal queue may not be the best approach.
I think we should overload withdraw function and have a version that allows the sender to specify which strategies to withdraw from.
def withdraw(_assets: uint256, _receiver: address, _owner: address, _strategiesToWithdraw: DynArray[address, 10] = []) -> uint256
so that the custom and the ERC4626 compliant withdraw (and redeem) functions are available.
For withdrawal queue, we have 3 options for simple ERC4626 compliance:
- assume ERC4626 can only withdraw from funds in the vault (this is simpler, lower gas and
- implement a simple withdrawal queue with low gas, high liquidity set of strategies to withdraw from
- implement a simple withdrawal queue with all strategies like in V2
Wdyt? @pandadefi @anticlimactic
There was a problem hiding this comment.
I think the first and second approaches are viable.
There was a problem hiding this comment.
implement a simple withdrawal queue with low gas, high liquidity set of strategies to withdraw from
how would this work? who would choose the strategies that we are withdrawing from? (i.e. will it be done in a centralized way or decentralized way?) i'm mindful of the direction we are heading in so want to make sure we're being consistent on our path to being more decentralized.
i don't think option 1 is a good idea. option 2 or option 3 are better. and the reason why is because option 1 would make using the vault via erc4626 too restrictive (i.e. it would not be feature-complete, if you compare it to using the vault through a custom interface). if we go with opt 1 (and to some degree opt 2), the vault becomes less composable. consider generalized lending protocols that may want to deposit and withdraw from our vault -- if not all strategies are included, it may be impossible for them withdraw without some intervention. more likely, they would have to build a custom adapter, which would defeat the purpose of implementing erc4626 in the first place.
There was a problem hiding this comment.
if we go with opt 1 (and to some degree opt 2), the vault becomes less composable. consider generalized lending protocols that may want to deposit and withdraw from our vault
I do not agree the erc4626 standard is made with the idea that not everything is withdrawable. You should use maxWithdraw when integrating with 4626 to know what can be withdrawn.
Even with option 3, you might not be able to withdraw everything if some strategies are locking funds.
I would even say options that are gas-intensive are worse and should be avoided if we want others to build on top.
There was a problem hiding this comment.
i do not believe our opinions conflict -- i just think there should be a way for people to withdraw from the vault, through erc4626, without having to depend on strategists freeing funds (assuming funds are withdrawable). this would make option 2 the most workable solution. option 1 doesn't allow for this (as having totalIdle always available means we are not being capital efficient). and option 3 is gas intensive.
|
this is ready for you to implement tests as described in #43 please, merge here and we will merge against master afterwards |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #36
Checklist