-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Fix promise in accounts.signTransaction()
throwing errors that cannot be caught
#5080
Fix promise in accounts.signTransaction()
throwing errors that cannot be caught
#5080
Conversation
Unfortunately I'm not skilled in JS (esp. in raw Promises) enough to review your PR, but it's great you could write a unit test covering this case. |
68b8b2a
to
c79883b
Compare
c79883b
to
67958f7
Compare
Hi @jdevcs! Can you guys review the PR and let me know if everything is alright? |
accounts.signTransaction()
throwing errors that cannot be caught
Pull Request Test Coverage Report for Build 2629557193
💛 - Coveralls |
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.
Thanks for the fix. but need to check why tests fails
Hi @avkos! Looking at the failed test, the promise: https://github.com/ChainSafe/web3.js/blob/9d5ce480d8f127e6868605fe684f9707a765e72b/test/e2e.contract.deploy.js#L112-L114 takes more that 2 seconds to resolve during e2e testing with Firefox browser. However, the same test using Chrome browser passes... Also, this other very similar promise: resolves both on Chrome and on Firefox.. I'm wondering, if we restart the tests, will it pass the second time? I'm really struggling to find a connection between the changes on the PR and the test that fails, without success |
Maybe you can rebase onto the latest HEAD and force-push to rerun the tests? |
2e17bdb
to
69ec36e
Compare
# Conflicts: # CHANGELOG.md
Ok, so the second run was successful |
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.
@andreistefanwork This branch has conflicts that must be resolved
@jdevcs yes, some |
5ff34c0
to
4d1500e
Compare
# Conflicts: # CHANGELOG.md
4d1500e
to
a84da19
Compare
@jdevcs @nazarhussain Can this PR be merged & closed? |
… be caught #4724 (#5080) (#5252) Co-authored-by: Andrei Stefan <[email protected]>
Merged via #5252 |
Description
Accounts.signTransaction() can generate a stray Promise under JS job queue, and there's no way for library users to catch any exceptions the stray promise throws, which can kill the entire Node.js process.
Fixes (#4724)
Type of change
Checklist:
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.