-
Notifications
You must be signed in to change notification settings - Fork 89
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(permit): modals SWAP #3158
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
2201e0b
to
852c354
Compare
dcc53f7
to
88121d9
Compare
const { account } = useWalletInfo() | ||
|
||
const icon = useMemo(() => (account ? <Identicon account={account} size={80} /> : undefined), [account]) |
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.
We need a container to load the account and identicon
'new modal + content top/bottom': ( | ||
<Wrapper> |
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.
Moved to its own file
|
||
// New Modal to be used going forward ================================= | ||
const ModalInner = styled.div` |
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.
Moved to its own file
{showPermitModal(swapConfirmState) ? ( | ||
<PermitModal | ||
onDismiss={onDismiss} | ||
inputAmount={trade?.inputAmountWithoutFee} | ||
outputAmount={trade?.outputAmountWithoutFee} | ||
step={swapConfirmState?.permitSignatureState === 'signed' ? 'submit' : 'approve'} | ||
/> | ||
) : attemptingTxn ? ( |
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.
This is the main magic!
(took me a quite awhile to figure it out :( )
permitInfo: PermitInfo | undefined | ||
hasEnoughAllowance: boolean | undefined | ||
permitInfo: IsTokenPermittableResult |
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.
Simplified the parameters.
Pass undefined when hasEnoughAllowance is falsy rather than passing that property independently down
const [swapConfirmState, setSwapConfirmState] = useAtom(swapConfirmAtom) | ||
const setSwapConfirmState = useSetAtom(swapConfirmAtom) |
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.
Simplified this part turning into a setter only.
We don't need to read the value anywhere, and actually depending on that to update the state causes race conditions.
Replaced (where needed) with the setter taking a fn instead of the new state
requestPermitSignature() { | ||
setSwapConfirmState((prev) => { | ||
const state: SwapConfirmState = { ...prev, permitSignatureState: 'requested' } | ||
console.debug('[Swap confirm state] requestPermitSignature: ', state) | ||
return state | ||
}) | ||
}, |
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.
Call this only if permit is needed
apps/cowswap-frontend/src/modules/swap/hooks/useSwapConfirmManager.ts
Outdated
Show resolved
Hide resolved
if (input.permitInfo) input.swapConfirmManager.requestPermitSignature() | ||
|
||
input.orderParams.appData = await handlePermit({ | ||
appData: input.orderParams.appData, | ||
hasEnoughAllowance: input.hasEnoughAllowance, | ||
|
||
inputToken: input.context.trade.inputAmount.currency as Token, | ||
provider: input.orderParams.signer.provider as Web3Provider, | ||
account: input.orderParams.account, | ||
chainId: input.orderParams.chainId, | ||
permitInfo: input.permitInfo, | ||
}) | ||
input.swapConfirmManager.permitSigned() |
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.
Here's the other side of the magic.
Request signature when we get to this state to show the permit modal.
Move it to next stage after signed.
@@ -12,6 +12,7 @@ | |||
"start": "nx run cowswap-frontend:serve", | |||
"preview": "cross-env NODE_OPTIONS=--max-old-space-size=32768 nx run cowswap-frontend:preview", | |||
"cosmos:export": "cross-env NODE_OPTIONS=--max-old-space-size=32768 nx run cowswap-frontend:cosmos:export", | |||
"cosmos": "nx run cowswap-frontend:cosmos:run", |
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.
Exposing it for easier access
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.
I thought we had it, anyways great to add it if it wasn't
d444cce
to
e10d855
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.
Also, step 2, should be grayed out? (the 2 steps look active)
It didn't work for me in mainnet. I saw that:
- After signing the SWAP, the order was not sent
- The modal of confirmation showed only with the title (no content)
I'm sorry, but I didn't take a screenshot of this last thing. Because I showed you in person, I think you know what I'm talking about :)
console.debug('[Swap confirm state] permitSigned: ', state) | ||
return state | ||
} | ||
console.debug('[Swap confirm state] permitSigned was a no-op') |
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.
what does this mean, and also, do we want to keep this debug message?
apps/cowswap-frontend/src/modules/swap/hooks/useSwapConfirmManager.ts
Outdated
Show resolved
Hide resolved
@@ -12,6 +12,7 @@ | |||
"start": "nx run cowswap-frontend:serve", | |||
"preview": "cross-env NODE_OPTIONS=--max-old-space-size=32768 nx run cowswap-frontend:preview", | |||
"cosmos:export": "cross-env NODE_OPTIONS=--max-old-space-size=32768 nx run cowswap-frontend:cosmos:export", | |||
"cosmos": "nx run cowswap-frontend:cosmos:run", |
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.
I thought we had it, anyways great to add it if it wasn't
Irritatingly, it worked for me on mainnet for both COW and DAI. Can you share your wallet address for me to check if there was any request against backend? Also, can you clear the |
Upon investigation, both order placements failed because of invalid quote ids See the logs: https://production-6de61f.kb.eu-central-1.aws.cloud.es.io/app/r/s/MIXrT
Searching for the quote id, it was created 2 min before Logs: https://production-6de61f.kb.eu-central-1.aws.cloud.es.io/app/r/s/M1ojG
With the expiration at |
3415e1e
to
2547434
Compare
apps/cowswap-frontend/src/legacy/components/TransactionConfirmationModal/index.tsx
Outdated
Show resolved
Hide resolved
* You probably want to use containers/PermitModal instead | ||
* This is the pure component for cosmos | ||
*/ | ||
export function PermitModal(props: PermitModalProps) { |
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.
Very clean and structural component!
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.
Works great to me!
Tested the main flow and haven't noticed any problems
2547434
to
8e54633
Compare
8e54633
to
a0dc493
Compare
Summary
Fixes #3145
Add the new permit modal to SWAP (LIMIT will come in another PR)
Screen.Recording.2023-09-28.at.14.25.22.mov
It even has cosmos!
Notes
Styles might need some work
To Test