feat(universal-router-sdk): add SwapProxy support for no-Permit2 approve+swap flow#518
feat(universal-router-sdk): add SwapProxy support for no-Permit2 approve+swap flow#518
Conversation
🤖 Claude Code Review
SummaryThis PR adds SwapProxy support to the universal-router-sdk, enabling a no-Permit2 approve+swap flow for ERC20 tokens. The implementation adds new options ( Review FindingsCritical Issues1. Potential type mismatch: URVersion vs UniversalRouterVersion In const urVersion = (options.urVersion as string as UniversalRouterVersion) ?? UniversalRouterVersion.V2_0The 2. Missing chainId validation for proxy mode The 3. Placeholder addresses (0x0...0) for SwapProxy All
Minor Issues4. Hardcoded 30-minute default deadline In const deadline = options.deadlineOrPreviousBlockhash
? BigNumber.from(options.deadlineOrPreviousBlockhash)
: BigNumber.from(Math.floor(Date.now() / 1000) + 1800) // 30 min defaultUsing 5. Inconsistent recipient requirement enforcement The Positive Notes
Recommendations
💡 Want a fresh review? Add a comment containing |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review verdict: REQUEST_CHANGES
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 3 inline comment(s) are attached below.
| private static encodeProxyPlan(planner: RoutePlanner, trade: UniswapTrade, options: SwapOptions): MethodParameters { | ||
| const { commands, inputs } = planner | ||
|
|
||
| const urVersion = (options.urVersion as string as UniversalRouterVersion) ?? UniversalRouterVersion.V2_0 |
There was a problem hiding this comment.
Casting URVersion to UniversalRouterVersion via string could fail silently if the enum values don't match. Consider explicit mapping or type assertion with validation.
|
|
||
| export const SWAP_PROXY_ADDRESS = (chainId: number): string => { | ||
| if (!(chainId in CHAIN_CONFIGS)) throw new Error(`SwapProxy not deployed on chain ${chainId}`) | ||
| const proxy = CHAIN_CONFIGS[chainId].swapProxy |
There was a problem hiding this comment.
This will return 0x0000000000000000000000000000000000000000 for configured chains, which could cause funds loss. Consider adding a check:
if (proxy === '0x0000000000000000000000000000000000000000') throw new Error(`SwapProxy not yet deployed on chain ${chainId}`)| const proxy = CHAIN_CONFIGS[chainId].swapProxy | |
| export const SWAP_PROXY_ADDRESS = (chainId: number): string => { | |
| if (!(chainId in CHAIN_CONFIGS)) throw new Error(`SwapProxy not deployed on chain ${chainId}`) | |
| const proxy = CHAIN_CONFIGS[chainId].swapProxy | |
| if (!proxy) throw new Error(`SwapProxy not configured for chain ${chainId}`) | |
| if (proxy === '0x0000000000000000000000000000000000000000') throw new Error(`SwapProxy not yet deployed on chain ${chainId}`) | |
| return proxy | |
| } |
| const inputAmount = BigNumber.from(trade.trade.maximumAmountIn(options.slippageTolerance).quotient.toString()) | ||
| const deadline = options.deadlineOrPreviousBlockhash | ||
| ? BigNumber.from(options.deadlineOrPreviousBlockhash) | ||
| : BigNumber.from(Math.floor(Date.now() / 1000) + 1800) // 30 min default |
There was a problem hiding this comment.
Using Date.now() makes output non-deterministic. The regular flow doesn't have a default deadline. Consider requiring deadline when useProxy: true to maintain consistency.
github-actions
bot
changed the title
1 participant
PR Scope
Please title your PR according to the following types and scopes following conventional commits:
fix(SDK name):will trigger a patch versionchore(<type>):will not trigger any release and should be used for internal repo changes<type>(public):will trigger a patch version for non-code changes (e.g. README changes)feat(SDK name):will trigger a minor versionfeat(breaking):will trigger a major version for a breaking changeDescription
[Summary of the change, motivation, and context]
How Has This Been Tested?
[e.g. Manually, E2E tests, unit tests, Storybook]
Are there any breaking changes?
[e.g. Type definitions, API definitions]
If there are breaking changes, please ensure you bump the major version Bump the major version (by using the title
feat(breaking): ...), post a notice in #eng-sdks, and explicitly notify all Uniswap Labs consumers of the SDK.(Optional) Feedback Focus
[Specific parts of this PR you'd like feedback on, or that reviewers should pay closer attention to]
(Optional) Follow Ups
[Things that weren't addressed in this PR, ways you plan to build on this work, or other ways this work could be extended]
✨ Claude-Generated Content
Description
Adds SwapProxy support to universal-router-sdk, enabling a no-Permit2 approve+swap flow where the proxy pulls ERC20 tokens directly from the user into the Universal Router before executing swap commands.
Changes
New Options
useProxyandchainIdoptions toSwapOptionstype insdks/universal-router-sdk/src/entities/actions/uniswap.tsuseProxy: boolean- when true, encodes calldata for SwapProxy instead of Universal RouterchainId: number- required in proxy mode to resolve the correct UR addressSwapRouter Updates
PROXY_INTERFACEwith the SwapProxyexecute(address router, address token, uint256 amount, bytes commands, bytes[] inputs, uint256 deadline)function signatureencodeProxyPlan()private method to encode calldata targeting the SwapProxy contractswapCallParameters()to detect proxy mode and route to proxy encodingchainId, explicit recipient, and disallows Permit2 permitsUniswapTrade Updates
payerIsUser = falsein proxy mode (proxy transfers tokens to UR)PERMIT2_TRANSFER_FROMcommand when using proxy with WETH unwrap (proxy already transferred tokens)Constants Updates
swapProxyfield toChainConfigtypeSWAP_PROXY_ADDRESS(chainId)helper functionTests
sdks/universal-router-sdk/test/unit/swapProxy.test.tscovering:How Has This Been Tested?
Unit tests added covering proxy mode encoding, validation, and edge cases.
Are there any breaking changes?
No - the
useProxyparameter is optional and defaults tofalse, maintaining backward compatibility with existing code.Follow Ups
0x0000...0000) after SWAP-2046 deployment