-
Notifications
You must be signed in to change notification settings - Fork 183
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
ERC4626 Support #162
Comments
Hey @Joeysantoro love the flag here for ERC-4626. We designed permit2 to support other token types (ERC721/ERC1155) as well but each token standard would have their own canonical permit2. So this permit2 contract is specifically for ERC20 tokens and we plan to launch separate permit2 instances for 721/1155. I'm not as familiar with this EIP so I'll need to look more into it, but I guess the question would be, how different is the approve/transfer methods for ERC-4626? Maybe we can also write a canonical ERC-4626 specific permit2 (similar to ERC721/ERC1155 pattern we've designed) or do you see natural benefit in supporting it at the ERC20 permit2 contract layer? |
@snreynolds thanks for the quick reply! It makes most sense directly in the ERC20 permit2 contract specifically because the ERC-4626 contract directly uses the ERC-20 approval system (all ERC-4626 contracts are themselves ERC-20). Per EIP-4626:
Its easiest to imagine ERC-4626 as a cToken (even though Compound is not compliant, the following discussion assumes it is). Take cDAI for example, the contract has all basic ERC-20 functionality as well as the ability to deposit and withdraw. The ERC-4626 standard allows for approved addresses of a given cDAI holder to withdraw or redeem the cDAI on behalf of the holder. Taking Universal Router where a user holds cDAI and wishes to swap into ETH as an example:
Without the permit2 integration for the withdraw method, step 3 would become: You can see how the withdrawal (and redeem) logic shares the ERC-20 approval system in the solmate example ERC-4626 base: https://github.com/transmissions11/solmate/blob/main/src/mixins/ERC4626.sol#L73-L84 |
+1 Supporting ERC4626 not only provides gas savings but can also provides better price execution. A use could simply unwrap and wrap two different 4626 tokens with the same underlying asset without even having to make a trade incurring slippage and swap fees. Since all ERC4626 must be ERC20 compliant and they are fully-backed wrappers around ERC20 tokens as well I think it makes sense to include in canonical Permit2 contract as well. 4626 already has a lot of support and there will be a ton of volume between ERC20 and 4626 tokens. |
hey @Joeysantoro looping back in here. I think we'd be down to support ERC4626 in this repo but in a different contract similar to how we are doing ERC721 and ERC1155 support which will be implemented this week. If you want to give it a go, essentially created a new Permit2_4626 contract that inherits new AllowanceTransfer and SignatureTransfer contracts that support ERC4626 I am happy to review and get it merged into this repo. We can then discuss integrating those contracts into Universal Router as per the discussion here. cc @hensha256 Also if you want to chat more feel free to shoot me a TG @saraareynolds |
@snreynolds great! Seeing the 721 or 1155 implementation will be helpful as well. If you all are committed that the contracts should be separate, then either the 4626 contract should contain all ERC-20 methods as well, or users of each user of a 4626 contract should be expected to approve the ERC-20 permit2 instance for ERC-20 functionality and ERC-4626 instance for 4626 functionality Both are somewhat clunky for integrating contracts such as the router. The former would have better UX (user only needs to approve in one place, but contracts will need to duplicate all ERC-20 permit2 functions across the ERC20 and ERC4626 instances). The latter would have better devX because the ERC-20 functionality is all in the same contract so it is less duplication. Let me know if this makes sense and what your thoughts are |
[EDIT]: only withdraw and redeem methods in ERC-4626 use the underlying ERC-20 approval, so I edited the below to reflect that. This also simplifies the permit2 integration.
Similar to Uniswap/universal-router#176, I believe ERC-4626 integration directly into permit2 is highly beneficial. The main reasons are:
Given that permit2 is intended to be deployed only once to save user approvals on gas, I'd lobby for ERC-4626 inclusion in the canonical deployment, whereas on universal router it can be added in a later deployment.
The simplest integration would be to only replicate the single-use transferFrom logic in AllowanceTransfer.sol for both of the
withdraw
andredeem
functions. Batch methods and SignatureTransfer could also be added for each, although I'm not sure if the extra complexity is warrantedCan I get a sense for the intended freeze date for the permit2 contract? If the timelines are possible, I would be able to have a tested PR ready before the end of next week.
The text was updated successfully, but these errors were encountered: