-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Some exceptions thrown by web3-eth-account cannot be caught #4724
Comments
Thanks @kai-alpha for reporting the issue. We will look into to investigate further. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
wut, this is a real bug |
Are there any updates on this? Seems like a very serious issue.. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
Hmm nobody is fixing this bug? Last attempt to get attention (I'll let the bot close it next time) |
… be caught web3#4724 Signed-off-by: Andrei Stefan <[email protected]>
… be caught web3#4724 Signed-off-by: Andrei Stefan <[email protected]>
… be caught web3#4724 Signed-off-by: Andrei Stefan <[email protected]>
Hi @kai-alpha! I was able to reproduce this issue as well. The problem seems to be indeed inside the inner Promise.all([promise1, promise2]) and its then() chain. If an error occurs in either promise1, promise2 or inside then(...), it's not propagated to the outer Promise This can be fixed using a catch(...) on the inner Promise.all() I created the following PR, in which I also included a test to cover this change. Can you double check everything looks alright? |
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
… be caught #4724 (#5080) (#5252) Co-authored-by: Andrei Stefan <[email protected]>
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
Still problem? |
@TangMonk @kai-alpha are you still able to reproduce ? |
It still happens on 4.6.0
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment. |
Is there an existing issue for this?
Current Behavior
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. A library shouldn't allow this to happen.Expected Behavior
try { ... await account.signTransaction(tx) } catch (e) { ... }
should be able to catch any exceptions thrown (conceptually) withinsignTransaction()
methodSteps to Reproduce
Set
tx.maxPriorityFeePerGas
but notmaxFeePerGas
and send it to BSC (or any EVM-compatible blockchains without EIP-1559) to hit thisthrow
athttps://github.com/ChainSafe/web3.js/blob/2b7c50a4cd27e5b21def779dd757cdacd2afd31a/packages/web3-eth-accounts/src/index.js#L433
Yes I know that doesn't make sense at all, but that's another story. A bug in our quick & dirty sandbox script led me to the issue.
Web3.js Version
1.7.0
Environment
No response
Anything Else?
_handleTxPricing()
runs something liketo asynchronously return the suggested values with
resolve()
, but I think the problem is that any exceptions thrown within the innerPromise.all()
(and itsthen()
chain) aren't propagated toreject()
from the outer Promise.Here is what I believe is a minimal repro: TS playground
JS isn't among my core expertise, I can't use JS Promise without async/await so this is all I can share.
Ping @spacesailor24 according to Git history
The text was updated successfully, but these errors were encountered: