feat: add Permit2 approval process for UniversalRouter

[0xAlec]

What changed? Why?

• add Permit2 approval flow for V2 API
• in our V2 API, we use Uniswap's UniversalRouter
• this is different than the standard approval process, where you approve ERC-20 tokens for spending against the router directly
• instead, for UniversalRouter ERC-20s are approved against an intermediary Permit2 contract
• this adds an additional step in our approval flow, since we need to approve the UniversalRouter contract to spend the funds approved against Permit2
  • this is typically done as a (gasless) signature - but we implement this as a transaction to allow for batching by smart wallets
• blockchain security recommended us only approve exact amounts to Permit2 (typically the approval is unlimited and only done once - because of the this constraint we need a transaction for each Permit2 approval, making the swap experience 3 separate transactions for EOAs)

read more: https://blog.uniswap.org/permit2-and-universal-router

Notes to reviewers

How has it been tested?
using https://github.com/ilikesymmetry/onchainkit-demo

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
onchainkit-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 6, 2024 7:33pm

[Zizzamia]

Why minor? is this causing a breaking changes?

[0xAlec]

it will be a breaking change for users of V2 API - but it is under experimental flag anyways so developers probably understand there will be breaking changes

i can change this to patch if you think it's more suitable

[Zizzamia]

let's do patch as it is experimental.

[0xAlec]

fixed

[Zizzamia]

Keep alphabetical order.

[0xAlec]

fixed

[Zizzamia]

Why this number? what's the meaning behind.

[0xAlec]

https://docs.uniswap.org/contracts/permit2/reference/allowance-transfer

it's the deadline where the approval is no longer valid - since we're only approving the exact amount fromAmount it shouldn't be a security concern if this isn't granular

[Zizzamia]

I feel like we should either have a comment here, or have a variable that better explain what's for.

This will help future developer undersand what's going on.

[0xAlec]

added a comment with a link to the above

[Zizzamia]

I feel like this code, has a lot of important pieces, and there is only one unit-test checking on this.

I wonder how this could be break down in 2~3 unit-tests. Or even the code break down a bit more.

[0xAlec]

the single unit test checks for all of the code/cases in this conditional

    expect(setPendingTransaction).toHaveBeenCalledTimes(6);
    expect(setPendingTransaction).toHaveBeenCalledWith(true);
    expect(setPendingTransaction).toHaveBeenCalledWith(false);
    expect(sendTransactionAsyncPermit2).toHaveBeenCalledTimes(3);
    expect(waitForTransactionReceipt).toHaveBeenCalledTimes(3);

    expect(setLoading).toHaveBeenCalledTimes(1);
    expect(setLoading).toHaveBeenCalledWith(true);
    expect(onSuccess).toHaveBeenCalledTimes(1);
    expect(onSuccess).toHaveBeenCalledWith({});
    expect(onStart).toHaveBeenCalledTimes(3);
    expect(onStart).toHaveBeenNthCalledWith(1, 'approveTxHash');
    expect(onStart).toHaveBeenNthCalledWith(2, 'permit2TxHash');
    expect(onStart).toHaveBeenNthCalledWith(3, 'txHash');

there can be an argument to decompose into two separate functions - processSwapApprovals and processSwapTransaction, both of which are under processSwap parent function. I can take this action item as part of the Smart Wallet/Transaction component integration.