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.

Sign up for GitHub

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

Jump to bottom

feat: add Permit2 approval process for UniversalRouter #980

Merged
merged 10 commits into from
Aug 6, 2024
Merged

feat: add Permit2 approval process for UniversalRouter #980

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
05e4763
permit2
0xAlec Aug 5, 2024
92c304e
better comments
0xAlec Aug 6, 2024
7b6fa13
test coverage
0xAlec Aug 6, 2024
c79a450
add `onStart` arguments to test
0xAlec Aug 6, 2024
ab404d8
comment
0xAlec Aug 6, 2024
69d2ff3
yarn check
0xAlec Aug 6, 2024
dc5ef93
changeset
0xAlec Aug 6, 2024
7f77855
sort constants alphabetically
0xAlec Aug 6, 2024
94d9a44
`patch` changeset
0xAlec Aug 6, 2024
94ccc3b
add comment for `deadline`
0xAlec Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/five-berries-warn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@coinbase/onchainkit": minor
Copy link
Contributor

Choose a reason for hiding this comment

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

Why minor? is this causing a breaking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

let's do patch as it is experimental.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

---

**feat**: add Permit2 approval process for UniversalRouter by @0xAlec #980
1 change: 1 addition & 0 deletions src/swap/components/SwapProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export function SwapProvider({
sendTransactionAsync,
onStart,
onSuccess,
useAggregator,
});

// TODO: refresh balances
Expand Down
4 changes: 4 additions & 0 deletions src/swap/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ export const GENERAL_SWAP_ERROR_CODE = 'SWAP_ERROR';
export const GENERAL_SWAP_QUOTE_ERROR_CODE = 'SWAP_QUOTE_ERROR';
export const GENERAL_SWAP_BALANCE_ERROR_CODE = 'SWAP_BALANCE_ERROR';
export const LOW_LIQUIDITY_ERROR_CODE = 'SWAP_QUOTE_LOW_LIQUIDITY_ERROR';
export const PERMIT2_CONTRACT_ADDRESS =
'0x000000000022D473030F116dDEE9F6B43aC78BA3';
export const TOO_MANY_REQUESTS_ERROR_CODE = 'TOO_MANY_REQUESTS_ERROR';
export const UNCAUGHT_SWAP_QUOTE_ERROR_CODE = 'UNCAUGHT_SWAP_QUOTE_ERROR';
export const UNCAUGHT_SWAP_ERROR_CODE = 'UNCAUGHT_SWAP_ERROR';
export const UNIVERSALROUTER_CONTRACT_ADDRESS =
'0x3fC91A3afd70395Cd496C647d5a6CC9D4B2b7FAD';
export const USER_REJECTED_ERROR_CODE = 'USER_REJECTED';
109 changes: 109 additions & 0 deletions src/swap/utils/processSwapTransaction.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ describe('processSwapTransaction', () => {
.fn()
.mockResolvedValueOnce('approveTxHash')
.mockResolvedValueOnce('txHash');
const sendTransactionAsyncPermit2 = vi
.fn()
.mockResolvedValueOnce('approveTxHash')
.mockResolvedValueOnce('permit2TxHash')
.mockResolvedValueOnce('txHash');
const onSuccess = vi.fn();
const onStart = vi.fn();
const onSuccessAsync = vi.fn().mockImplementation(async (_txHash: string) => {
Expand Down Expand Up @@ -117,6 +122,7 @@ describe('processSwapTransaction', () => {
sendTransactionAsync,
onSuccess,
onStart,
useAggregator: true,
});

expect(setPendingTransaction).toHaveBeenCalledTimes(4);
Expand Down Expand Up @@ -210,6 +216,7 @@ describe('processSwapTransaction', () => {
sendTransactionAsync,
onSuccess,
onStart,
useAggregator: true,
});

expect(setPendingTransaction).toHaveBeenCalledTimes(2);
Expand Down Expand Up @@ -304,6 +311,7 @@ describe('processSwapTransaction', () => {
sendTransactionAsync: sendTransactionAsync2,
onSuccess: onSuccessAsync,
onStart: onStartAsync,
useAggregator: true,
});

expect(setPendingTransaction).toHaveBeenCalledTimes(4);
Expand All @@ -320,4 +328,105 @@ describe('processSwapTransaction', () => {
expect(onStartAsync).toHaveBeenNthCalledWith(1, 'approveTxHash');
expect(onStartAsync).toHaveBeenNthCalledWith(2, 'txHash');
});

it('should successfully use Permit2 approval process for `useAggregators`=false', async () => {
const swapTransaction: BuildSwapTransaction = {
transaction: {
to: '0x123',
value: 0n,
data: '0x',
chainId: 8453,
gas: 0n,
},
approveTransaction: {
to: '0x456',
value: 0n,
data: '0x123',
chainId: 8453,
gas: 0n,
},
quote: {
from: {
name: 'USDC',
address: '0x833589fcd6edb6e08f4c7c32d4f71b54bda02913',
symbol: 'USDC',
decimals: 6,
image:
'https://d3r81g40ycuhqg.cloudfront.net/wallet/wais/44/2b/442b80bd16af0c0d9b22e03a16753823fe826e5bfd457292b55fa0ba8c1ba213-ZWUzYjJmZGUtMDYxNy00NDcyLTg0NjQtMWI4OGEwYjBiODE2',
chainId: 8453,
},
to: {
address: '0x4ed4e862860bed51a9570b96d89af5e1b0efefed',
chainId: 8453,
decimals: 18,
image:
'https://d3r81g40ycuhqg.cloudfront.net/wallet/wais/3b/bf/3bbf118b5e6dc2f9e7fc607a6e7526647b4ba8f0bea87125f971446d57b296d2-MDNmNjY0MmEtNGFiZi00N2I0LWIwMTItMDUyMzg2ZDZhMWNm',
name: 'DEGEN',
symbol: 'DEGEN',
},
fromAmount: '100000000000000',
toAmount: '19395353519910973703',
amountReference: 'from',
priceImpact: '0.94',
hasHighPriceImpact: false,
slippage: '3',
warning: undefined,
},
fee: {
baseAsset: {
name: 'DEGEN',
address: '0x4ed4e862860bed51a9570b96d89af5e1b0efefed',
symbol: 'DEGEN',
decimals: 18,
image:
'https://d3r81g40ycuhqg.cloudfront.net/wallet/wais/3b/bf/3bbf118b5e6dc2f9e7fc607a6e7526647b4ba8f0bea87125f971446d57b296d2-MDNmNjY0MmEtNGFiZi00N2I0LWIwMTItMDUyMzg2ZDZhMWNm',
chainId: 8453,
},
percentage: '1',
amount: '195912661817282562',
},
};
const config = createConfig({
chains: [mainnet, sepolia],
connectors: [
mock({
accounts: [
'0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
'0x70997970c51812dc3a010c7d01b50e0d17dc79c8',
'0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC',
],
}),
],
transports: {
[mainnet.id]: http(),
[sepolia.id]: http(),
},
});

await processSwapTransaction({
swapTransaction,
config,
setPendingTransaction,
setLoading,
sendTransactionAsync: sendTransactionAsyncPermit2,
onSuccess,
onStart,
useAggregator: false,
});

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');
});
});
46 changes: 44 additions & 2 deletions src/swap/utils/processSwapTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import type { TransactionReceipt } from 'viem';
import type { Address, TransactionReceipt } from 'viem';
import { encodeFunctionData, parseAbi } from 'viem';
import type { Config } from 'wagmi';
import { waitForTransactionReceipt } from 'wagmi/actions';
import type { SendTransactionMutateAsync } from 'wagmi/query';
import {
PERMIT2_CONTRACT_ADDRESS,
UNIVERSALROUTER_CONTRACT_ADDRESS,
} from '../constants';
import type { BuildSwapTransaction } from '../types';

export async function processSwapTransaction({
Expand All @@ -12,6 +17,7 @@ export async function processSwapTransaction({
sendTransactionAsync,
onStart,
onSuccess,
useAggregator,
}: {
swapTransaction: BuildSwapTransaction;
config: Config;
Expand All @@ -22,12 +28,15 @@ export async function processSwapTransaction({
onSuccess:
| ((txReceipt: TransactionReceipt) => void | Promise<void>)
| undefined;
useAggregator: boolean;
}) {
const { transaction, approveTransaction } = swapTransaction;
const { transaction, approveTransaction, quote } = swapTransaction;

// for swaps from ERC-20 tokens,
// if there is an approveTransaction present,
// request approval for the amount
// for V1 API, `approveTx` will be an ERC-20 approval against the Router
// for V2 API, `approveTx` will be an ERC-20 approval against the `Permit2` contract
if (approveTransaction?.data) {
setPendingTransaction(true);
const approveTxHash = await sendTransactionAsync({
Expand All @@ -41,6 +50,39 @@ export async function processSwapTransaction({
confirmations: 1,
});
setPendingTransaction(false);

// for the V2 API, we use Uniswap's `UniversalRouter`, which uses `Permit2` for ERC-20 approvals
// this adds an additional transaction/step to the swap process
// since we need to make an extra transaction to `Permit2` to allow the UniversalRouter to spend the approved funds
// this would typically be a (gasless) signature, but we're using a transaction here to allow batching for Smart Wallets
// read more: https://blog.uniswap.org/permit2-and-universal-router
if (!useAggregator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@0xAlec 0xAlec Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

setPendingTransaction(true);
const permit2ContractAbi = parseAbi([
'function approve(address token, address spender, uint160 amount, uint48 expiration) external',
]);
const data = encodeFunctionData({
abi: permit2ContractAbi,
functionName: 'approve',
args: [
quote.from.address as Address,
UNIVERSALROUTER_CONTRACT_ADDRESS,
BigInt(quote.fromAmount),
20_000_000_000_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this number? what's the meaning behind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment with a link to the above

],
});
const permitTxnHash = await sendTransactionAsync({
to: PERMIT2_CONTRACT_ADDRESS,
data: data,
value: 0n,
});
await Promise.resolve(onStart?.(permitTxnHash));
await waitForTransactionReceipt(config, {
hash: permitTxnHash,
confirmations: 1,
});
setPendingTransaction(false);
}
}

// make the swap
Expand Down