Skip to content
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

Merged
merged 3 commits into from
Mar 8, 2024

Conversation

corwinthill
Copy link
Contributor

Description

  • These changes reflect what is summarized at the bottom of issue useScaffoldContractWrite does not throw errors #753
  • Added line 88 to useScaffoldContractWrite.ts and changed line 98 in useTransactor.tsx
  • These changes throw errors forward to a function utilizing useScaffoldContractWrite, and removes one notification error to prevent duplicate error notifications displaying.

Additional Information

Related Issues

Closes #753

Your ENS/address: zurriken.eth

Copy link
Collaborator

@technophile-04 technophile-04 left a 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:

  1. Just update useTransactor to show notification error + throw whole error forward

  2. useScaffoldWriteContract's writeContractAsync 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.

@damianmarti
Copy link
Collaborator

@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...

Copy link
Collaborator

@rin-st rin-st left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm!

@rin-st
Copy link
Collaborator

rin-st commented Mar 7, 2024

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...

Good point. I think we need to create a new issue for it, and your updates of getParsedError can be helpful. If you still have these projects somewhere of course :)

@technophile-04
Copy link
Collaborator

I think we need to create a new issue for it, and your updates of getParsedError can be helpful. If you still have these projects somewhere of course :)

++, Tysm Damu ! and also Rinat for reveiw! 🙌

Merging this

@technophile-04 technophile-04 merged commit 03a2dfa into scaffold-eth:main Mar 8, 2024
1 check passed
@damianmarti
Copy link
Collaborator

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...

Good point. I think we need to create a new issue for it, and your updates of getParsedError can be helpful. If you still have these projects somewhere of course :)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useScaffoldContractWrite does not throw errors
4 participants