Add transaction_error_msg and send error to sentry #1752
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
estebanmino
reviewed
Sep 14, 2020
| { text: strings('navigation.ok') } | ||
| ]); | ||
| error && Logger.error(error, 'error while trying to send transaction (Confirm)'); |
There was a problem hiding this comment.
what about having the error message still displayed to users? maybe that's valuable for cases like low gas or something
There was a problem hiding this comment.
I think displaying the error might be useful, but we should be doing more than just displaying the issue? I'm honestly not sure what makes the most sense here. a failure could happen for any number of reason, so maybe a generic failure message isn't that useful? I think ultimately it should be a generic message with maybe an option to expand to show error details? idk 🤷♀️
estebanmino
approved these changes
Sep 25, 2020
rickycodes
force-pushed
the
feature/error-handling
branch
from
f895ee8
to
3958ad2
Compare
rickycodes
added a commit
that referenced
this pull request
Jan 31, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
previously we were showing the error to the end user for send/confirm transactions:
instead we now display a more generic error message and send the error to sentry.
there's a few other spots where we likely want to do the same thing:
here: https://github.com/MetaMask/metamask-mobile/blob/develop/app/components/Nav/Main/index.js#L764
here: https://github.com/MetaMask/metamask-mobile/blob/develop/app/components/Views/Approval/index.js#L209
here: https://github.com/MetaMask/metamask-mobile/blob/develop/app/components/Views/ApproveView/Approve/index.js#L666
and here: https://github.com/MetaMask/metamask-mobile/blob/develop/app/components/Views/SendFlow/Confirm/index.js#L801we should probably update those as well.