-
Notifications
You must be signed in to change notification settings - Fork 869
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 useScaffoldContractWrite so it properly throws errors #758
Fix useScaffoldContractWrite so it properly throws errors #758
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.
Hey thanks @corwinthill 🙌, just pushed few tweaks :
Updated changes:
-
Just update
useTransactor
to show notification error + throw whole error forward -
useScaffoldWriteContract
'swriteContractAsync
will just throw error forward without showing any notifications.
Reason for above changes:
useTransactor:
The main aim of this hook is to execute given txn + show opinionated/general UI feedback, so it makes sense here to show error notification. Hence b036b23
useScaffoldWriteContract:
Since we are internally using useTransactor
it should the show UI feedback needed but we don't need it to explicitly show error notification again and eat up the error.Similar to how wagmi's writeAsync
works it should just throw error forward. Hence 2aace01
cc @carletex @rin-st for thoughts / any other suggestions
Since we are using useTransactor
internally for useScaffoldWriteContract
it makes UI feedback for useScaffoldWriteContract
opinionated. Which kind of makes sense for beginners / intermediate users.
If advanced users want to show custom UI feedback they maybe shouldn't use useScaffoldWriteContract
and use wagmi's useWriteContract
and handle themselves.
But maybe in future we could make useScaffoldWriteContract
less opinionated and let user handle the UI feedback themselves.
@corwinthill @technophile-04 This looks great!! One related comment. Sometimes I ended up adding ifs to getParsedError, because the error thrown is not good enough, and you can get a better message checking something inside the error. Maybe adding a way to use a custom getParsedError is a nice way to improve that. What do you think? Did you have this issue before? We can move to a discussion if you think this is something we should implement... |
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!
Good point. I think we need to create a new issue for it, and your updates of |
++, Tysm Damu ! and also Rinat for reveiw! 🙌 Merging this |
These are particular for each project/contract, you can take a look at https://github.com/damianmarti/fruit-market2/blob/main/packages/nextjs/components/scaffold-eth/Contract/utilsContract.tsx#L27 |
Description
Additional Information
Related Issues
Closes #753
Your ENS/address: zurriken.eth