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: add permit2 entrypoints to the periphery #782

Merged

Conversation

bmzig
Copy link
Contributor

@bmzig bmzig commented Nov 29, 2024

This adds two more functions to the spoke pool periphery contract: depositWithPermit2 and swapAndBridgeWithPermit2. Both use the signature transfer logic in permit2.

There is an assumption that the amount to spend on the permit2 signature is equal to the amount to swap (for a swap and bridge tx) or the amount to deposit (for a deposit tx). I believe that this assumption is correct, since a user should not give the periphery an allowance, so it should only ever transferFrom the amount specified in the permit details anyways.

@nicholaspai nicholaspai requested a review from dohaki November 29, 2024 22:20
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

This looks good. Can you add a forge unit test for swapAndBridgeWithPermit2?

@bmzig bmzig requested a review from nicholaspai December 3, 2024 18:04
Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

This looks right to me, just have a couple q's

_swapToken.forceApprove(exchange, swapTokenAmount);
// The exchange will either receive funds from this contract via a direct transfer, an approval to spend funds on this contract, or via an
// EIP1271 permit2 signature.
if (_transferType == PeripherySigningLib.TransferType.Approval)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add unit tests for these three TransferType flows? I also think we should move TransferType into this contract as its not really a "signing" related struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are here: e3cfb18, moving the type definitions is here 1927810

Copy link
Member

@nicholaspai nicholaspai left a comment

Choose a reason for hiding this comment

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

let's just get unit tests in especially for any behavior in _swapAndBridge. I'm more ok with not having unit tests yet for the external permit/auth/permit2 logic yet but I want to make sure the swapAndBridge() and deposit() functions work well

bmzig added 2 commits December 5, 2024 12:35
Signed-off-by: bennett <[email protected]>
Signed-off-by: bennett <[email protected]>
@nicholaspai nicholaspai merged commit 022a8ec into spokepool-periphery-multiple-exchanges Dec 6, 2024
7 of 9 checks passed
@nicholaspai nicholaspai deleted the bz/permit2Periphery branch December 6, 2024 23:33
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