-
Notifications
You must be signed in to change notification settings - Fork 164
feat: add generic msg on failed transactions #587
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
base: develop
Are you sure you want to change the base?
feat: add generic msg on failed transactions #587
Conversation
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.
GM @Jonatan-Chaverri , thanks for your PR! You got the idea right. Leaving some comments and thoughts for improvement
- Can we improve the style? (fix the padding and margins)
-
Can we have a clickable link that points us to the specific log from the toast? so that it is easier to trace the error logs for other developers
-
Can we extract some common errors like insufficient balance or approval required? We can discuss this further if you think this is possible / not possible
GM @Jonatan-Chaverri, any updates on this? |
@metalboyrick I will work on this today and provide an update at the EOD |
c187bc3
to
b7e8824
Compare
@metalboyrick I have addressed the first two requests. For the third, we can have a file similar to this and translate each code to a more verbose error message, is that what you mean? if not, please elaborate a little more. Thanks. |
4a36e4e
to
82646cc
Compare
@Jonatan-Chaverri Yeah something like that would be great, because it gives better clarity to users rather than some strange JSON. You can try out some common ones like insufficient balance, no approval. (be mindful of which errors are common which errors can be user-specific) |
1bf9c79
to
d85efc2
Compare
@metalboyrick I updated the PR, let me know your thoughts, thanks. |
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.
d85efc2
to
4b28de7
Compare
@metalboyrick ok, made some changes to fix the margins, this is how it looks now Also, I tried getting insufficient balance error to test too and this is the error content my function is getting
I'm not aware of how all starknet common errors are printed, so the file I created is based on assumptions that at least keywords will appear in the error, but again is not 100% reliable until we can confirm that (or if you know how to confirm error messages, you can point me to it and I will update) |
Task name here
Fixes #573
Types of change
Comments (optional)