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

feat: decode metamask transaction error #353

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

ariesgun
Copy link

Resolves #346

@ubiquity-os-deployer
Copy link

ubiquity-os-deployer bot commented Oct 25, 2024

Copy link
Contributor

github-actions bot commented Oct 25, 2024

Preview Deployment
980d99f1669d9522c9402cb0211041e658f2e715

@ariesgun
Copy link
Author

QA:

image

image

@ariesgun ariesgun marked this pull request as ready for review October 25, 2024 23:15
@Keyrxng
Copy link
Member

Keyrxng commented Oct 25, 2024

Looks good. Idk if it's worth it to also log the onchain function name that was called too using the 4byte api .. @rndquu rfc

I think if we can make this robust and re-useable @ariesgun then we can open a task and implement it in rpc-handler as core error handling. That might be a task you'd like to complete if so.

@rndquu rndquu self-requested a review October 26, 2024 07:31
Copy link
Member

@rndquu rndquu left a comment

Choose a reason for hiding this comment

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

This QA has some drawbacks:

  1. Result is shown in the console instead of a toaster
  2. Error is not in a human readable way

Expected behavior: user sees a toaster with a human friendly error instead of a hex string.

Pls use https://github.com/superical/ethers-decode-error/tree/1.x, I'm sure we can simply fork it and set node version to <20.

static/scripts/rewards/web3/erc721-permit.ts Outdated Show resolved Hide resolved
@rndquu rndquu marked this pull request as draft October 26, 2024 07:51
@ariesgun
Copy link
Author

Expected behavior: user sees a toaster with a human friendly error instead of a hex string.

It should be made clear in the spec.

@ariesgun
Copy link
Author

ariesgun commented Oct 26, 2024

image

image

@ariesgun ariesgun requested a review from rndquu October 26, 2024 08:21
Copy link
Contributor

github-actions bot commented Oct 26, 2024

Unused dependencies (1)

Filename dependencies
package.json @ariesgun/ethers-decode-error

Unlisted dependencies (2)

Filename unlisted
static/scripts/rewards/web3/erc20-permit.ts ethers-decode-error
static/scripts/rewards/web3/erc721-permit.ts ethers-decode-error

@@ -50,6 +50,7 @@
"countries-and-timezones": "^3.6.0",
"dotenv": "^16.4.4",
"ethers": "^5.7.2",
"@ariesgun/ethers-decode-error": "^1.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

I think we should self host @rndquu

Copy link
Author

Choose a reason for hiding this comment

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

Is it something I can do?

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we need to get a copy of your repo and then host it. @rndquu @whilefoo @gentlementlegen

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we do.

Also Ana shared QA on another issue that showed the toaster displaying the same error that aries provided as QA, so there is little difference compared to what prod is running rn as far as I can tell.

We should use 4byte and pull the onchain fn names into the console using the selector from the data in the call that fails. Saves having to manually do it while debugging.

@ariesgun
Copy link
Author

ariesgun commented Nov 3, 2024

For now, waiting for the ubiquite-hosted npm package for ethers-decode-error to be created.

@gentlementlegen
Copy link
Member

I opened a PR on that regard here: ubiquity/ethers-decode-error#1
@0x4007 after thought, should I create it there or in ubiquity-os

@0x4007
Copy link
Member

0x4007 commented Nov 4, 2024

I suppose all crypto related software should be under ubiquity org.

Copy link
Contributor

ubiquity-os bot commented Nov 6, 2024

@ariesgun, this task has been idle for a while. Please provide an update.

@ariesgun
Copy link
Author

ariesgun commented Nov 6, 2024

Still ongoing

Copy link
Contributor

ubiquity-os bot commented Nov 8, 2024

@ariesgun, this task has been idle for a while. Please provide an update.

@gentlementlegen
Copy link
Member

I had implemented our own ethers-decode-error but every time the publish fails:
https://github.com/ubiquity/ethers-decode-error/actions/runs/11735404423/job/32693050614#step:7:21

@ariesgun Did you manually publish it? If so I do not have the NPM token myself and @0x4007 should publish it manually. If not let me know how you proceeded.

@ariesgun
Copy link
Author

ariesgun commented Nov 8, 2024

I had implemented our own ethers-decode-error but every time the publish fails: https://github.com/ubiquity/ethers-decode-error/actions/runs/11735404423/job/32693050614#step:7:21

@ariesgun Did you manually publish it? If so I do not have the NPM token myself and @0x4007 should publish it manually. If not let me know how you proceeded.

yes, I did it manually

Copy link
Contributor

ubiquity-os bot commented Nov 10, 2024

@ariesgun, this task has been idle for a while. Please provide an update.

@ariesgun
Copy link
Author

@ariesgun, this task has been idle for a while. Please provide an update.

Still waiting for the package to be uploaded

@gentlementlegen
Copy link
Member

@0x4007 You are the one with NPM credentials if you can please publish the package.

@rndquu
Copy link
Member

rndquu commented Nov 10, 2024

@0x4007 You are the one with NPM credentials if you can please publish the package.

Isn't NPM_TOKEN relevant? The latest release workflow silently failed with The local branch 1.x is behind the remote one, therefore a new version won't be published. . Should we publish manually first or there's smth wrong with the package.json?

@gentlementlegen
Copy link
Member

@rndquu yeah I tried several times:
#353 (comment)

Either we should change the workflow, either manually publish because it seems to compare the version to the main forked repo so we cannot make any modification and publish it. We could copy the publishing action we usually use for our other projects.

Copy link
Contributor

ubiquity-os bot commented Nov 12, 2024

@ariesgun, this task has been idle for a while. Please provide an update.

@ariesgun
Copy link
Author

@rndquu yeah I tried several times: #353 (comment)

Either we should change the workflow, either manually publish because it seems to compare the version to the main forked repo so we cannot make any modification and publish it. We could copy the publishing action we usually use for our other projects.

Is there anything I can do to finalize this PR? As far as I understand, the package has not been uploaded yet due to token issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decode transaction errors
5 participants