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

Feat/permit2 makers #82

Draft
wants to merge 2 commits into
base: feat/advancedRouting
Choose a base branch
from
Draft

Conversation

jkrivine
Copy link
Collaborator

@jkrivine jkrivine commented Oct 9, 2023

This PR proposes to allow strats made with the strat lib (either Direct or Forwarder) to use permit2 approval.
Permit2 based approvals for Direct is not implemented and left to the designer's choice (in order not to impose additional gas cost if the strat does not want to use permit2).

  • 112890b introduces the main changes
  • the following commit propagates changes to tests and examples

Comment on lines 283 to +287
ApprovalInfo memory approvalInfo;
uint pulled = router().pull(IERC20(order.olKey.outbound_tkn), owner, amount, true, approvalInfo);
if (data.usePermit2) {
approvalInfo.approvalType = ApprovalType.Permit2Approval;
}
uint pulled = router().pull(IERC20(order.olKey.outbound_tkn), data.owner, amount, true, approvalInfo);
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 something wrong with the current logic. We are only. a partial ApprovalInfo object if we use permit2.

Currently, if we use permit2 with an offer forwarder on pull, this is what the code does :

  1. pushes successfully
  2. uses the ApprovalTransferLib.sol lib to pull
  3. Use the Permit2TransferLib.sol lib function transferTokenFromWithPermit2 that will try to permit our forwarder to pull from the reserve, but our signature is 0x0 as we didn't pass anything
  4. From now it failed but should transfer tokens to forwarder

The only thing that should differ is permit2.transferFrom(...) instead of token.transferFrom(...) if we use permit2.

The approval should have been given in advance by the reserve right ?

Copy link
Contributor

@Louis-Amas Louis-Amas Oct 9, 2023

Choose a reason for hiding this comment

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

Did you test it ?
What I remember implementing was that if approvalInfo.spender is 0x0 we directly call permit2.transferFrom which expect the use approve using permit2 in advance.
So here when Jean is only setting approvalType it means that spender and signature will be 0x0 meaning that we will directly call permit2.transferFrom which will succeed if

  • The user approve ERC20 -> Permit2
  • The user approved the router using Permit2 by using an EOA to approve or a contract calling permit with the user signature

The approval should have been given in advance by the reserve right ?
yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Just assumed, should have looked into permit2 before then. The function sig is very mysterious then that's why, I'll try and run it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the code above assumes the signature has been already given to permit2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants