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 8 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
50809a3b304d1d8dbbb777ae35c5ab80b29ee284

@ariesgun
Copy link
Author

QA:

image

image

@ariesgun ariesgun marked this pull request as ready for review October 25, 2024 23:15
@ariesgun ariesgun requested a review from Keyrxng as a code owner October 25, 2024 23:15
@Keyrxng
Copy link
Contributor

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

@ariesgun ariesgun marked this pull request as ready for review October 26, 2024 10:04
package.json Outdated Show resolved Hide resolved
@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?

@gentlementlegen
Copy link
Member

I published it under ubiquity-os: https://www.npmjs.com/package/@ubiquity-os/ethers-decode-error @ariesgun please try using this package.

@rndquu You can delete this repo: https://github.com/ubiquity/ethers-decode-error thank you.

@ariesgun ariesgun closed this Dec 12, 2024
@ariesgun ariesgun changed the title feat: decode metamask transaction error ^^^ Dec 12, 2024
@ariesgun ariesgun changed the title ^^^ feat: decode metamask transaction error Dec 12, 2024
@ariesgun ariesgun reopened this Dec 12, 2024
@0x4007
Copy link
Member

0x4007 commented Dec 13, 2024

I published it under ubiquity-os: https://www.npmjs.com/package/@ubiquity-os/ethers-decode-error @ariesgun please try using this package.

@rndquu You can delete this repo: https://github.com/ubiquity/ethers-decode-error thank you.

The crypto related stuff should be under @ubiquity

@gentlementlegen
Copy link
Member

@0x4007 Please move it because I have no access and the NPM token under ubiquity seems to be invalid to publish packages, or at least I don't know what is expected, maybe ubiquibot/ethers?
https://github.com/ubiquity/ethers-decode-error/actions/runs/12107146609/job/33753718023#step:6:58

@0x4007
Copy link
Member

0x4007 commented Dec 13, 2024

That's right we might not have access to the ubiquity npm for some reason. @rndquu im mobile but do you have access? Can you solve for this

@ariesgun
Copy link
Author

Is there anything I can do to help close this one?

@0x4007
Copy link
Member

0x4007 commented Dec 22, 2024

@rndquu @gentlementlegen lets finalize this

@gentlementlegen
Copy link
Member

@0x4007 Please move it because I have no access and the NPM token under ubiquity seems to be invalid to publish packages, or at least I don't know what is expected, maybe ubiquibot/ethers? https://github.com/ubiquity/ethers-decode-error/actions/runs/12107146609/job/33753718023#step:6:58

@0x4007 would need the token to be updated in order to publish to the proper NPM organization.

@rndquu rndquu requested review from rndquu and removed request for Keyrxng and rndquu December 28, 2024 07:24
@rndquu
Copy link
Member

rndquu commented Dec 28, 2024

As far as I understand everything is fine right now:

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