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

[1.48.0] UNI token in production is not showing as permitable #3221

Closed
anxolin opened this issue Oct 12, 2023 · 5 comments
Closed

[1.48.0] UNI token in production is not showing as permitable #3221

anxolin opened this issue Oct 12, 2023 · 5 comments
Assignees
Labels
Bug Something isn't working Medium Severity indicator. It causes some undesirable behavior, but the system is still functional RELEASE Included in the release that is being closed

Comments

@anxolin
Copy link
Contributor

anxolin commented Oct 12, 2023

Description
Is triggering the normal Approve flow and not ERC-2612 one.

This happened to me several times in a row, even after deleting the local storage and refreshing.
However, now i cannot reproduce

Somewhere in the code we have something that if it fails, it can make us think is NOT PERMITABLE. This happened over Wallet Connect, so maybe is related? Maybe we had an issue with our ethereum provider and we interpreted the error as a "no support"

How to Reproduce
I just tried to sell UNI, and triggered the normal approval

Expected behavior
Trigger reliably always ERC-2612 flow for UNI and other permitable tokens

Additional context
@alfetopito knows, cause i showed it live, however i cannot reproduce now. Not sure if related, but we saw this in the log:

image

I think this error comes from Wallet connect library somehow
image

However, its still not clear this is the reason, because i still get this error, but it doesn't reproduce now

More CTX: https://cowservices.slack.com/archives/C0361CDG8GP/p1697131326306699

@anxolin anxolin added Bug Something isn't working Medium Severity indicator. It causes some undesirable behavior, but the system is still functional RELEASE Included in the release that is being closed labels Oct 12, 2023
@alfetopito alfetopito self-assigned this Oct 13, 2023
@alfetopito alfetopito changed the title UNI token in production is not showing as permitable [1.48.0] UNI token in production is not showing as permitable Oct 13, 2023
@alfetopito
Copy link
Collaborator

Connected via WC2, I got the error:
image

But still, permit data loaded fine.


After awhile I can reproduce.
Not 100% on the steps how to get here, but once I got it "broken":

  1. WCv2 connected on Goerli
  2. Tokens permit info load fine
  3. Switch to mainnet in the UI
  4. All permit requests fail.
  5. I simulated one of the permit requests, and it succeeds as expected https://www.tdly.co/shared/simulation/6f4f261c-9c9d-4d48-8e48-481e6e5230d7
  6. This particular ethCall throws, triggering the catch, which cycles throgh 4 different permit methods.
  7. When all the options fail, it rejects with nonce not supported

This leads me to believe there must be some sort of caching with the provider being in the old chain maybe?

@alfetopito
Copy link
Collaborator

Observing the provider used while creating the Eip2612 utils instance, we see that indeed, the network in the provider remains the same
Screenshot 2023-10-13 at 15 43 59

@anxolin
Copy link
Contributor Author

anxolin commented Oct 13, 2023

Yes, i think one issue with this implementation is that this method blindly assumes that if the calls fail, its because the contract doesn't have the specific method

See
https://github.com/1inch/permit-signed-approvals-utils/blob/b190197a45c3289867ee4e6da93f10dea51ef276/src/eip-2612-permit.utils.ts#L317

They ignore the received error, and just do the recursive call.

Since its a library, is hard to modify this (unless we want to copy or patch it). If these are 2 time consuming, we can just hope there's not a lot of cases, and do it later. We would drastically reduce the problem if we do the pre-calculation of which tokens are permitable.

@alfetopito
Copy link
Collaborator

Yes, i think one issue with this implementation is that this method blindly assumes that if the calls fail, its because the contract doesn't have the specific method

See https://github.com/1inch/permit-signed-approvals-utils/blob/b190197a45c3289867ee4e6da93f10dea51ef276/src/eip-2612-permit.utils.ts#L317

They ignore the received error, and just do the recursive call.

Since its a library, is hard to modify this (unless we want to copy or patch it). If these are 2 time consuming, we can just hope there's not a lot of cases, and do it later. We would drastically reduce the problem if we do the pre-calculation of which tokens are permitable.

That's one problem, yes, but the main issue here is that the provider instance is not changing networks when the UI thinks it did.
Once the app starts in a given network, changing it in the top does not affect the network in the provider instance.
It requires a refresh in the desired network for the provider change to "stick", and changing to a different network will cause it to be in the same situation again.

@alfetopito
Copy link
Collaborator

We would drastically reduce the problem if we do the pre-calculation of which tokens are permitable.

Yes... and no. Sure, we do not need to calculate which tokens are permittable as they don't change.
But we still need to fetch the nonce in all other situations (generating the permit signature and validating whether an existing permit is still valid), and the problem would be worse in this case as users would be stuck unable to place orders with their permit failing.
And not just with the nonce, all methods will fail really, if the provider is in the wrong network.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Medium Severity indicator. It causes some undesirable behavior, but the system is still functional RELEASE Included in the release that is being closed
Projects
None yet
Development

No branches or pull requests

2 participants