-
Notifications
You must be signed in to change notification settings - Fork 182
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 allowance flow for zrx swaps #7758
Conversation
ded154d
to
033a6f7
Compare
196e53c
to
ed932ac
Compare
7a2d63b
to
641cfe2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First very very high-level pass to get familiar with this - untested at runtime just yet and allowance hooks still not reviewed, but looks solid from this first read!
Some comments re: generalization and naming mostly, though nothing obviously stands out as wrong here.
...ponents/MultiHopTrade/components/MultiHopTradeConfirm/hooks/useIsApprovalInitiallyNeeded.tsx
Show resolved
Hide resolved
packages/swapper/src/swappers/ZrxSwapper/utils/helpers/helpers.ts
Outdated
Show resolved
Hide resolved
...nents/MultiHopTrade/components/MultiHopTradeConfirm/components/ApprovalStep/ApprovalStep.tsx
Outdated
Show resolved
Hide resolved
src/components/MultiHopTrade/components/MultiHopTradeConfirm/hooks/helpers.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First runtime pass with permit2 flag enabled
- RUNE -> AVAX (THOR) - note Tx history is sad but irrelevant to this PR, got a Zerion notification that fundus were received
https://jam.dev/c/96cc06bb-74fd-4257-9bed-e694e20d4aa2 - XDAI -> FOX (Li.Fi)
https://jam.dev/c/af077d67-8da7-4513-8206-f1970109de02 - FOX -> XDAI (CoW, no allowance granted)
https://jam.dev/c/34b13a10-c595-4f45-905c-9701b73571a7 - FOX -> XDAI (CoW, allowance granted)
https://jam.dev/c/4ff9213d-c873-4da6-beb2-d046be67fd0b - Shitcoin -> ETH bridge (Li.Fi, no allowance granted)
https://jam.dev/c/5b65b4af-3d15-4bb8-8320-cb1645b0774e - FOX -> ETH (0x) quote 🚫
Getting this error response back from ZRX
Also tested with the flag off:
- USDT.AVA -> AVA.AVA (0x, no approval granted)
https://jam.dev/c/8089bdbc-40d3-4df5-96b5-ffdd763162e3 - USDT.AVA -> AVA.AVA (0x, approval granted)
https://jam.dev/c/700cb7f9-bb00-4ecb-b3ca-bc00ac057312 - ETH -> USDT (Li.Fi)
https://jam.dev/c/3b55d215-c082-4574-9e93-297caf73d6d0
Unable to test more USDT flow re: unlimited, 0, and approval reset flows for the time being because of low fundus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally after local origin/develop rebase and LGTM for all tested flows, minus one spotted issue re: permit2 contract approval being prompted (with the flag on) when there is enough allowance, and there shouldn't be any new allowance Tx, hence no permit2 contract approval nor actual allowance Tx/permit2 EIP712 message signed and broadcasted.
See testing in https://gist.github.com/gomesalexandre/6c0b209f940b64a09d726c53ad7f7527
4057645
to
7a1fbfc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/swapper/src/swappers/ZrxSwapper/getZrxTradeQuote/getZrxTradeQuote.ts
Outdated
Show resolved
Hide resolved
src/components/MultiHopTrade/components/MultiHopTradeConfirm/components/Hop.tsx
Show resolved
Hide resolved
packages/swapper/src/swappers/ZrxSwapper/getZrxTradeQuote/getZrxTradeQuote.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retested locally as a last paranoia pass after last changes and confirmed we're looking good at runtime:
https://gist.github.com/gomesalexandre/ab10310c3aff40bd37c00fb15c3ed4da
One q: re price or quote logic for permit2, but otherwise looks sane to land :fridaydog:
3dba081
to
73d5cbc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My nitpick feedback was addressed, and I'm putting the quote issues I had down to dev endpoint shenanigans.
gm
Description
Breaks token allowance approval UI components into reusable ones in prep for permit2 UI flow.
Notable non-permit2 changes:
Permit2 changes:
TODO:
Issue (if applicable)
progresses #7648
Risk
This is a high risk change because it heavily modifies the state management of trades and completely rewrites the UI associated with allowance approvals.
All trades, especially those with token allowance approvals. Due to the surface area of the change, no feature toggle exists for the UI and state changes so extra care is required to ensure we roll this out without issues.
Testing
General
non 0x trades
0x trades
with
REACT_APP_FEATURE_ZRX_PERMIT2
feature flag enabledwith
REACT_APP_FEATURE_ZRX_PERMIT2
feature flag disabledEngineering
Operations
There is a feature flag in this PR but its used only as a fallback if we need to disable permit2 trades for any reason. The changes made in this PR still affect the app majorly regardless of whether the flag is enabled.
Screenshots (if applicable)
non-0x trades
https://jam.dev/c/27122198-3e0e-4669-84a9-37bdcb76fc52
https://jam.dev/c/0aadbb0a-681a-400d-86bb-80bc6a9e36e5
https://jam.dev/c/671e6347-c888-4b11-95ab-0275728a1729
0x trades
https://jam.dev/c/7c1535c7-1966-4ed0-807d-40ef332b46f6
https://jam.dev/c/d6d8af5d-0e6d-486d-90bf-fcb4e3976bf5