-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
better handle of the revert raison of a call (issue #4454) #5962
Conversation
Pull Request Test Coverage Report for Build 4537279824Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
…)' to manage revert embedded error details got from some providers like MetaMask (related to issue web3#4454)
…llback(err, result)' to handle Revert in all cases: use `originalError` sub-object if provided
0b5be80
to
6e9dce5
Compare
My only feedback would be @nicos99 can you also include a reference/link to what err.data looks like in metamask? other than that I dont see any other concern in this PR, other than u needing to fix the conflicting file |
@luu-alex I put two screenshots inside the issue #4454 when I open the PR. I've juste updated the PR description to add a link to (See this comment to know what err.data looks like in MetaMask.). |
(resolved Conflicts for CHANGELOG.md)
Ok @jdevcs , I'm working on it...
|
…sendTxCallback(err, result)' to better manage revert call reason from some providers like MetaMask (related to issue web3#4454)
@nicos99 There is need to fix conflicting files Thanks |
…ert-call-from-metamask (resoled Conflict with 'CHANGELOG.md' and 'packages/web3-core-method/src/index.js' caused last PR on the same bug)
@Muhammad-Altabba I'm sorry to have canceled your work (PR #6000) but I've been working on the resolution for a long time, which integrates 2 scenarios and adds Unit Tests |
@jdevcs, ok, conflict solved. Can you please merge the PR for me now? (I do not have write access) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks for your contribution
@Muhammad-Altabba, you're welcome! Now can you or someone else complete the PR please? ( Maybe @avkos or @nikoulai ?) |
Description
Improvement of
web3-core-method.buildCall.sendTxCallback(err, result)
to manage various object data structures when looking for the revert reason info got from some providers like MetaMask. See this comment to know what err.data looks like in MetaMask.Fixes #4454. I am aware that this is not realy a bug of web3 and that the problem should be corrected at the source, however as the error generated breaks the promise system (uncaught exception) and that this was well handled until version 1.2.9, I considers that it is necessary to adapt the code to better strengthen the library.
Type of change
Checklist for 1.x:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:cov
and my test cases cover all the lines and branches of the added code.npm run build
with success.dist/web3.min.js
in a browser.CHANGELOG.md
file in the root folder.