-
Notifications
You must be signed in to change notification settings - Fork 47
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: #513 sign with injected wallets on local node #527
fix: #513 sign with injected wallets on local node #527
Conversation
…account or not and acting accordingly.
✅ Deploy Preview for contracts-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Passing run #284 ↗︎
Details:
Review all test suite changes for PR #527 ↗︎ |
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.
LGTM! Just few nits.
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.
Looks way cleaner
onError && onError(result); | ||
|
||
unsubscribe(); | ||
throw new Error(message); |
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.
Maybe we can provide a brief reason behind transaction failure?
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.
I lines 77-80 we try to decode the given error with the metadata we have. The raw result
gets passed along to a potential error handler, onError
and last but not least we throw a proper Error with a generic or decoded error message. Depending how the rest is implemented the error is caught or an onError
provided, which then presents the error to the user somehow, either a toast popping up or a failure message.
I think that's all we could do here. 🤔
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.
Ah, my bad, did not notice the error decoding
Resolves #513/#520 by referring to the selected account if it's a testing account or not and acting accordingly.
This is the actual change:
https://github.com/paritytech/contracts-ui/pull/527/files#diff-d22473d5f78c6d4516163586c619fa5d61e960611e085cd39c676ab0afe72e81R41-R53
The rest just got reformatted due to a now shorter line:
https://github.com/paritytech/contracts-ui/pull/527/files#diff-d22473d5f78c6d4516163586c619fa5d61e960611e085cd39c676ab0afe72e81R56-R93