-
Notifications
You must be signed in to change notification settings - Fork 540
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
feat: add ERC4626 Tokenized Vault hook #452
base: main
Are you sure you want to change the base?
Conversation
Ideally ERC4626Hook should be defined as an abstract contract, but I wasn't sure what's the desired convention for testing an abstract contract in this repo. |
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.
Overall this PR looks interesting and combining a hook with ERC4626 opens many interesting use cases. However, could you explain the exact use case for when this hook would be used?
@partylikeits1983 Enabling trading between vault <-> asset with no Uniswap pool slippage or pool liquidity, limited only by the constraints that exist in |
Why should it be abstract? Typical convention for testing abstract contracts is to make simple test implementations of virtuals and test those! |
@marktoda I was torn whether it should be abstract or not from a nitpick standpoint, as ERC4626 is a generic standard rather than a specific implementation like wstETH. That being said, it doesn't contain any virtual functions and any specific contract implementations extending it would only utilize the constructor instantiation, so perhaps it's fine as is. |
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.
LGTM
Related Issue
Which issue does this pull request resolve?
Description of changes
Adds a hook for wrapping/unwrapping assets to/from an ERC4626 vault, inspired by #451.