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

Decode transaction errors #346

Open
rndquu opened this issue Oct 22, 2024 · 11 comments · May be fixed by #353
Open

Decode transaction errors #346

rndquu opened this issue Oct 22, 2024 · 11 comments · May be fixed by #353

Comments

@rndquu
Copy link
Member

rndquu commented Oct 22, 2024

There's this package which is able to decode transaction errors which really helps in finding out the root cause of why permit or card minting is throwing an error.

As a part of this issue we should decode and show the decoded error to a user on:

  • permit claim
  • card mint (because we send transaction to the treasury address)

Original comment

@ariesgun
Copy link

/start

Copy link
Contributor

ubiquity-os bot commented Oct 25, 2024

Deadline Fri, Oct 25, 5:49 PM UTC
Beneficiary 0xE80FC2700ec6faF5f0347a2E7E7798FAf548e1c3

Tip

  • Use /wallet 0x0000...0000 if you want to update your registered payment wallet address.
  • Be sure to open a draft pull request as soon as possible to communicate updates on your progress.
  • Be sure to provide timely updates to us when requested, or you will be automatically unassigned from the task.

@ariesgun
Copy link

ariesgun commented Oct 25, 2024

This library needs ethers v6, so we need to upgrade ethers from v5 to v6.
We can't use /ethers-decode-error/v/1.0.0 because it supports node version < 19.

Shall we upgrade ethers to v6?

@0x4007
Copy link
Member

0x4007 commented Oct 25, 2024

I think retaining v5 is more maintainable because LLMs are trained on it but not v6.

@rndquu
Copy link
Member Author

rndquu commented Oct 25, 2024

This library needs ethers v6, so we need to upgrade ethers from v5 to v6. We can't use /ethers-decode-error/v/1.0.0 because it supports node version < 19.

Shall we upgrade ethers to v6?

We've just recently downgraded from v6 to v5. Perhaps there're other libs.

@ariesgun
Copy link

This library needs ethers v6, so we need to upgrade ethers from v5 to v6. We can't use /ethers-decode-error/v/1.0.0 because it supports node version < 19.
Shall we upgrade ethers to v6?

We've just recently downgraded from v6 to v5. Perhaps there're other libs.

I see.
Will check if there is any alternative..

@Keyrxng
Copy link
Member

Keyrxng commented Oct 25, 2024

One potentially hacky method but it would give better insight maybe is the 4byte API where we can search for fn signatures and log them to the console on tx fail. Obv there are collisions and sometimes the sig has not been uploaded but it's something we can setup internally and it's free to use.

I personally copy paste into 4byte when I'm debugging errors so this would make my life easier at least lmao

@ariesgun ariesgun linked a pull request Oct 25, 2024 that will close this issue
@ariesgun
Copy link

ariesgun commented Nov 1, 2024

One potentially hacky method but it would give better insight maybe is the 4byte API where we can search for fn signatures and log them to the console on tx fail. Obv there are collisions and sometimes the sig has not been uploaded but it's something we can setup internally and it's free to use.

I personally copy paste into 4byte when I'm debugging errors so this would make my life easier at least lmao

Do you still have the result from 4byte? Curious how it looks like

@Keyrxng
Copy link
Member

Keyrxng commented Nov 1, 2024

Copy the selector from the start of the data and then paste it into the function signature search feature on 4byte and it'll show you. You'll need to write logic to deal with the API for my suggestion ofc

I do this when I'm writing tests, debugging, etc that sort of thing. It's tedious manually as sometimes there are 10-15 calls you need to manually track not always in the same order etc. I don't have any stored results or anything, whatever I gather at the time I comment in the code then remove it after I've fixed it.

@ariesgun
Copy link

ariesgun commented Nov 2, 2024

Copy the selector from the start of the data and then paste it into the function signature search feature on 4byte and it'll show you. You'll need to write logic to deal with the API for my suggestion ofc

I do this when I'm writing tests, debugging, etc that sort of thing. It's tedious manually as sometimes there are 10-15 calls you need to manually track not always in the same order etc. I don't have any stored results or anything, whatever I gather at the time I comment in the code then remove it after I've fixed it.

I am not sure why we need to use function search feature from 4byte. I tried the data value from here. We can only get the function signature. And it is already visible in the console logger.

image

I thought what we need is to decode the transaction error and not function signature. The transaction error can be decoded by simply using this library ethers-decode-error/. I think it is sufficient.

Did I miss something?

@Keyrxng
Copy link
Member

Keyrxng commented Nov 2, 2024

I thought what we need is to decode the transaction error and not function signature.

They both help when debugging in my opinion but both are not required. The spec says to use the lib and decode, so do that and ignore my suggestion.

My suggestion to implement it that way and then to embed it into our official rpc-handler is because it'd be repurposed and likely provide a lot more value across the org. MetaMask lives in the browser and is detached from our rpc-handler but also controls all writes so it does have it's own error handling.

Did I miss something?

No, both approaches were suggested but the spec only lists one. My comment was given a thumbs-up which is usually a sign of approval but only I've mentioned it beyond that. Implement the spec unless someone explicitly approves my suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants