fix: do not throw when estimating gas #209
Conversation
| e instanceof EstimateGasExecutionError && | ||
| (e.cause instanceof InsufficientFundsError || | ||
| (e.cause instanceof ExecutionRevertedError && // viem does not reliably label node errors as InsufficientFundsError when the user has enough to pay for the transfer, but not for the transfer + gas | ||
| (/transfer value exceeded balance of sender/.test(e.cause.details) || | ||
| /transfer amount exceeds balance/.test(e.cause.details))) || | ||
| (e.cause instanceof InvalidInputRpcError && | ||
| /gas required exceeds allowance/.test(e.cause.details))) |
There was a problem hiding this comment.
the change is to get rid of this guessing
sviderock
left a comment
sviderock
left a comment
There was a problem hiding this comment.
🚀 Great proposal IMO!
Approved considering that you definitely know the flow better than me and acknowledging that removal of throw won't affect anything else. Feel free to ignore my approval otherwise 😄
There was a problem hiding this comment.
I have been thinking about this too lately, related to another bug:
https://github.com/jeanregisser/celo-fee-currency-estimate-gas-bug
Anyway, one downside is we won't be able to surface revert reason to the user while estimating:
valora-xyz/wallet#4864
But I guess we don't have much choice if the error is [EstimateGasExecutionError: Execution reverted for an unknown reason. 🤔
|
ok, it seems the root cause is the celo error. |
Description
Currently, when we try to estimate gas as a part of fee currency selection, and run into an error we don’t recognize, we throw it to the user. We rely on a hand-picked list of “known” errors, and anything else gets thrown.
This approach is hurting users — they can’t complete swaps in certain edge cases we didn’t think of.
Context: https://valora-app.slack.com/archives/C04B61SJ6DS/p1746461883367569
In this case, an
RpcRequestErrorwith details ”execution reverted” was thrown, but the root cause was the insufficient amount of that particular fee currency on the user balance.This PR proposes to silently handle any estimation error instead of cherry-picked ones. It seems that re-throwing during estimation does not bring any value to the user, just the contrary. Since we are calling the estimation while looping over all possible fee currencies, it seems fine to just skip a particular currency if we encounter any error, and move on, enabling users to complete their intended task.
Test plan
Related issues
NA
Backwards compatibility
Y
Network scalability
Y