-
Notifications
You must be signed in to change notification settings - Fork 470
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
Fix panic when MetaMask returns error #549
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.
Nice, thanks! Any chance we can cover this behaviour with a test case?
We can't run tests directly in browser, so I think the only solution is to mock |
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.
What if we extract response parsing to a standalone (rust) function and just unit test that? I.e. that given the expected payload it parses correctly. wdyt?
Sounds like a solution, I'll try to commit it today |
I'm wondering why there is a panic at all in case of invalid response while we have |
Yeah, I just looked at the code and indeed we shouldn't be panicking and definitely the code is not |
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.
Thanks a lot!
Fix #544