Gracefully handle requesting gas fee estimates for nonexistent network #5084
Conversation
mcmire
force-pushed
the
handle-nonexistent-network-client-when-fetching-estimates
branch
from
8188720 to
26a8094
Compare
mcmire
force-pushed
the
handle-nonexistent-network-client-when-fetching-estimates
branch
from
26a8094 to
441df67
Compare
Currently, when GasFeeController tries to fetch gas fee estimates for a network that does not exist for some reason, it consults NetworkController for the corresponding network client and then NetworkController throws an error. It could be argued that we don't really care that the network doesn't exist and there is no reason to expose this information to the client. Given this assumption, this commit gracefully handles this situation by returning an empty set of estimates instead. It also extracts the error message produced when a network client is not found so that it can be checked more easily.
mcmire
force-pushed
the
handle-nonexistent-network-client-when-fetching-estimates
branch
from
441df67 to
f92f1c7
Compare
| ); | ||
| } catch (error) { | ||
| if (error instanceof NoNetworkClientFoundError) { | ||
| log(error.message); |
There was a problem hiding this comment.
I figured that we ought to be aware of the problem on some level, I am just unsure what the best place is.
@Gudahtt What would be your recommendation here? I seem to recall a comment you made in some PR a while back about switching away from using console.error / console.warn. There's also this PR where we introduced a separate production-level logger, but it seems like this log message would continue to surface in Sentry which we might not want.
There was a problem hiding this comment.
The pattern in the linked PR seems ideal to me for non-debug logging because the client can choose whether it's enabled or not. That was generally the goal: let the client control logging, don't make it mandatory.
Logs can definitely be useful as Sentry breadcrumbs.
There was a problem hiding this comment.
About this specific scenario, can we make this situation impossible? I don't quite follow how it could happen.
Is there an active poll on a deleted network that we haven't shut down, or is something else trying to get estimates from a non-existent network? I'd think the root problem lies elsewhere.
There was a problem hiding this comment.
Yeah, I guess you're right. Maybe hiding the error isn't such a good thing in this case.
Explanation
Currently, when GasFeeController tries to fetch gas fee estimates for a network that does not exist for some reason, it consults NetworkController for the corresponding network client and then NetworkController throws an error.
It could be argued that we don't really care that the network doesn't exist and there is no reason to expose this information to the client. Given this assumption, this commit gracefully handles this situation by returning an empty set of estimates instead.
References
There are a series of errors in Sentry for both Extension and Mobile which refer to the uncaught error:
This fix would prevent those errors from occurring.
Changelog
(Updated in PR.)
Checklist