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.

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

fix: do not send permit to quote when enough allowance #3433

Merged
merged 4 commits into from
Nov 24, 2023

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Nov 23, 2023

Summary

This PR is a continuation of Leandro's #3411

I went through multiple rabbit holes to debug this one.
Basically, making the story short, there was 3 issues:

  • One ciclic dependency making an infinite loop
  • Too many re-renders! (even if you remove the infinite loop)
  • Not clearing the hooks

Ciclic dependency

  • Hooks is required for the quote
  • Therefore: If you change the hooks, it will re-calculate the quote
  • When the quote is recalculated, the "v2Trade" becomes null. This is a critical entity used to calculate the actual sellAmount after slippage
  • Because we don't know the sellAmount during the quote re-calculation, we... BECOME UNSURE if our allowance is enough
  • The previous code, would assume that, if we are not sure, we will add hooks (just in case!)
  • Adding the hooks triggers another quote... and so on 🔂

Part of the solution is to only add the permit if we are sure that the user don't have enough allowance. Before was treating undefined as a reason to add it.
https://github.com/cowprotocol/cowswap/pull/3433/files#diff-eb9b3098bb927275324420456886b1e05659b33bca5eca07e0af1a672bcccf7bR21

However, the key thing to solve the issue was to use v2Trade in the hook that updates the hooks.
https://github.com/cowprotocol/cowswap/pull/3433/files#diff-eb9b3098bb927275324420456886b1e05659b33bca5eca07e0af1a672bcccf7bR43

Basically, only when we have the quote and all the other data we consider update.

Too many re-renders! (even if you remove the infinite loop)

It's still not amazing but this PR reduces the re-renders quite a bit.

Part of the reduction is the creation of usePermitDataIfNotAllowance which will return a memoized object that depends on the hook data only

I did some improvements so it only updates the hook (which triggers re-renders) if the hook really changes

Not clearing the hook

After solving the issues above became easy

https://github.com/cowprotocol/cowswap/pull/3433/files#diff-eb9b3098bb927275324420456886b1e05659b33bca5eca07e0af1a672bcccf7bR61

Bonus

Added a diagram with the state of the permit
permit-state.drawio

It's very basic, but since i did it to understand better the atoms, I just pushed it.

Also changed the signature of useSwapAmountsWithSlippage to make it easier to read. The positional arguments are error prone:
https://github.com/cowprotocol/cowswap/pull/3433/files#diff-45b0bcb8baef0bafb447c88c917e750a718f7c7bdae6dadfd6f73a6d54fa9ac6R85

Bonus 2

Added a notification for Sentry if we leak the fake signer account used in permit in the appData.

https://github.com/cowprotocol/cowswap/pull/3433/files#diff-808338a054c7b81dd64d500d781a534b581f4b4012a1927bbef2f6bd2a0696e6R73

Test

Test for Limit Orders and Swaps

Basically we need to assert that:

  • We only add info about the hooks for the quotes when the user don't have enough ALLOWANCE
  • If we don't have allowance and we make a trade, once is executed, the app should detect the new allowance and remove the hook from both the quote request, and the appData

(*) please, note there might be a delay in the update of a few seconds

How to remove allowances, to test the permit

https://revoke.cash, click on top right, my approvals

How to check when the app clears or sets a hook

While testing, you can filter to see when hooks are added and removed, as the image shows:

image

How to check if there's a hook in the posted order

Go to the explorer, and check the appData in the bottom:
https://explorer.cow.fi/orders/0xe9d079fa47fdfd47c483a282e805620361cea3860adefbb374c69f43c2a9fd81020ca66c30bec2c4fe3861a94e4db4a498a35872655f04eb?tab=overview

How to check when a quote request has hooks

Check in the Chrome extension tools for the appData we send

image

Copy link

vercel bot commented Nov 23, 2023

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

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Nov 23, 2023 8:34pm
swap-dev ✅ Ready (Inspect) Visit Preview Nov 23, 2023 8:34pm
widget-configurator ✅ Ready (Inspect) Visit Preview Nov 23, 2023 8:34pm

@elena-zh
Copy link
Contributor

Hi @anxolin , in great job! In most of cases it works as you described!
However, I have noticed some issues:

  1. Sometimes the 'appdatahooksupdater' log does not appear on Limit orders page (for a token that does not have allowance, and is in the gasless approval list)
    no hooks
  2. It takes a few time to clear hooks, so withing this time I can place one more order, and it will call again a gasless approval request (Swap page)
    2nd trade
    The same issue I faced on the Limit orders page when an order has been filled, and I placed one more (please notice, there are no hooks on Console)
    once again
  3. When I opened the PR for the 1st time, and specified a trade, I got a BE error
    image
    image
    However, it was only once and I was no longer able to reproduce it.

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

see my comments above

@anxolin
Copy link
Contributor Author

anxolin commented Nov 24, 2023

Sometimes the 'appdatahooksupdater' log does not appear on Limit orders page (for a token that does not have allowance, and is in the gasless approval list)

I will investigate

  1. It takes a few time to clear hooks, so withing this time I can place one more order, and it will call again a gasless approval request (Swap page)

This is normal. Its only when we detect the change in the allowance. We are working to make this faster, but is independent of this fix/pr. I added a disclaimer that this could happen.

  1. When I opened the PR for the 1st time, and specified a trade, I got a BE error

I will investigate. I would think is not related to this PR, but I still want to know what happens...

THANKS!!!

@anxolin anxolin mentioned this pull request Nov 24, 2023
@anxolin
Copy link
Contributor Author

anxolin commented Nov 24, 2023

Sometimes the 'appdatahooksupdater' log does not appear on Limit orders page (for a token that does not have allowance, and is in the gasless approval list)

@elena-zh , i can't reproduce. In principle, it will show it if it needs to be shown (approval needed), but you also need a quote. Could it be that the sell token or buy token was not selected? Do you have a sequence I can try to reproduce?

@elena-zh
Copy link
Contributor

@anxolin , I will try finding a dependency and will let you know.
I will open a new issue for this then.

@elena-zh
Copy link
Contributor

For now:

  1. I revoke a token approval in Etherscan
  2. Open Limit orders page
  3. Refresh the page
  4. Place an order with the token

No hooks
no hook
no hook1

Copy link
Contributor

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Hey @anxolin , here is what I found: even if I don't see sometimes a hook log in the console, it appears in the appdata log in the order details.
I'm approving this PR.

@anxolin
Copy link
Contributor Author

anxolin commented Nov 24, 2023

Hey @anxolin , here is what I found: even if I don't see sometimes a hook log in the console, it appears in the appdata log in the order details.
I'm approving this PR.

Yes, for limit orders i believe this is fine. Here is ok the quote doesn't have it. The price in limit orders is an indication, and should not include the hook. The important thing is that the SWAP includes the hook, so its approved if it happens.

image

@anxolin anxolin merged commit 58b6ade into hotfix/1.49.7 Nov 24, 2023
7 of 8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 24, 2023
Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Nice, thanks for following up and fixing my mess :)

@alfetopito alfetopito deleted the dont-quote-with-permit branch December 11, 2023 11:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants