fix(types): better types and autocomplete for custom contract hooks

[sverps]

Fixes #116

Implemented types throughout all the hooks.

useScaffoldEventSubscriber

autocompletes contractName and eventName
infers correct type for event inputs in the callback

useScaffoldContractWrite

autocompletes contractName and function name (only allows write functions)

useScaffoldContractRead

autocompletes contractName and function name (only allows read functions)
proper return types

useDeployedContractInfo

autocompletes contractName
Returns typed abi and address

TODO

Source of truth for chain id

@carletex @rin-st @technophile-04

[rin-st]

Hey! Is this pr updates #147? Why didn't you add last changes there?

[sverps]

Hey! Is this pr updates #147? Why didn't you add last changes there?

Yes, this builds off of those changes, but when I created this PR, I wasn't added to the repo yet, so couldn't push to that other branch.

[technophile-04]

Thanks, @sverps love the clean-up looks a lot better and more sync with main 🙌

Some of my thoughts on TODO(Source of truth for chain id) :
I think TODO was mentioned because we cannout infer literal chain number if we make use of .env right ? Please correct me if I am wrong @sverps

1. Keep the chain in .env as it is currently there and let developer pass in ABI to our hooks :

As suggested here -> #147 (comment) as the first option.
Advantage : We don't need to restructure other things and also code become's alot simpler
Disadvantage : Dev need to pass in ABI himself, we may need to find better way to generate abi's if there will be 2 or more contracts, btw I hardhat-deploy export and we can export it as const in .ts file

2. Maybe create scaffold.config.ts file which exports things `as const.

Advantages: This way we will be able to infer type without letting Developer pass in Abi's and also we can leverage it in other hooks
Disadvantage: We may need to restructure other things too

^ Above are solutions that I can come up with and there might be others, would love to hear others' suggestions.

[carletex]

@sverps Could you fix conflicts here?

Will love to start testing, and I think #231 will be ready soon.

Thanks!

[sverps]

I'm on it 👍

[carletex]

More conflicts, sorry :/

• Conflicts
• Update export type DefaultChain = "31337"; with the scaffoldConfig.targetNetwork.id value.
• I think we also need update github workflow to run yarn chain and yarn deploy #191, right?

Anything else?

I'll do a full review when ready!

Thanks

[sverps]

On it 👍

[sverps]

Making progress on this, but found a small issue that is a bit tricky to resolve. Currently, typescript doesn't show any error when you pick a functionName that needs arguments in the read or write hook, but don't provide them.

useScaffoldContractWrite("YourContract", "setGreeting"); // Doesn't show TS error

I think it would be nice if typescript tells the user that he's missing the arguments.

This became a bit tricky to resolve (especially due to the optional config in the read hook and the optional value param in the write hook, and the missing optional config for the write hook).

Here are a couple of options on how to proceed:

fix/116-type-inference-option-1

• Continues on the API style that we had for the hooks (i.e. first param is the contract name, second is the function name, ...)
• Requires some additional logic to resolve the optional arguments to their proper value. When in the read hook, no arguments were required, but you wanted to provide a config param, you needed to pass an empty array for args to have it work properly.
• Typescript errors are indicating what is wrong, but the actual errors are a bit cryptic.

fix/116-type-inference-option-2

• Clear typescript errors that indicate in a more understandable way what is missing or wrong
• API is a bit closer to wagmi API (args need to be passed in the a config object, but with good typing), which makes a possible transition to using wagmi hooks directly a bit lower.

So please check out those two branches and look at the ContractData.tsx file. There you can see how the typescript errors are shown. There you'll get a better understanding of the pros and cons of both options.

Personally, I like option 2 the most. It's the least complex in implementation, and has the best type error messages, which make for the best DX.

[rin-st]

I like second too.

What do you think about the third parameter being like

{
    args: ...,
    value: ...,
    config: ...
}

so it should be much easier to configure types in hooks and you'll not need params like argsOrValueOrConfig? It makes params more explicit and I believe improves dx too. And you can add default params, for example [] for args

[carletex]

Will review it later today. Just wanted to un-draft it to see if the GH Actions worked. It did! :D

[technophile-04]

I like second too.

++

My thoughts :

I actually like how our current main branch hooks are configured where you pass the third argument as arguments to the contract function and is really handy, and easy to use :

useScaffoldContractRead("YourContract", "testRead", [BigNumber.from(1)])

But looking fix/116-type-inference-option-1, I agree with pt 2, 3 which @sverps mentioned #202 (comment)

Also, it might cause confusion :

---

I really liked fix/116-type-inference-option-2 !! Since it shows a nice verbose error for the args.

But I think passing args in object is a bit unintuitive and a bit PITA to open those {} only for args and then put args in it.

My Suggestion :

I would love if we could make hook signature something like this :

  useScaffoldContractRead({
    contractName: "YourContract",
    functionName: "greeting",
    args: [BigNumber.from(1)],
    enabled: false,
  });

Wherein we are passing an object as the first argument itself, similar to wagmi hooks.

Reason: I think now it will be more intuitive to pass in args, because before you wanted open {} only for args. Also I feel it won't cause that hit to DX because typescript will help in autoCompleting keys when using hooks and points mentiond by Samuel.

PS : It's great we are having this discussion, love it !! because Wagmi doesn't show you an error if you forgot to pass in args and it will be great if we do so 🙌

[carletex]

This is looking really good. Great job @sverps !! Great reviews & hindsight @technophile-04 @rin-st <3

I feel that what Shiv is suggesting is the way to go if all the type hinting / auto-completions / errors work. Basically the same as using wagmi hooks but with some extra magic :)

[sverps]

Thanks for the suggestion @technophile-04, I like this option even more 👍

One thing I'm still a bit unsure about is whether or not the args should be strongly typed, or allow for a tupple of the needed type and undefined (what I did in the last commit). The reason being that a user might use this hook in combination with some form, so the initial values of the form might not be valid args values. Will need to play a bit more with it to better understand the implications.

Maybe that's the reason why wagmi makes the args optional, in order for the dev to code it in a way that he can prepare the args tupple before providing it...

[technophile-04]

Even I am confused a bit, although I tend to like the current state where we are doing union with undefined for each index inside a tuple because I think it makes hooks easy to use and also will increase DX for beginners too.

But would love to hear @carletex and @rin-st thoughts on this, I have also created test branch -> https://github.com/scaffold-eth/se-2/tree/fix/116-type-inference-test where I have put wagmi's useContractRead and useContractWrite below our useScaffoldRead/write to compare for Errors / DX and find out edge cases if we are missing in our hooks compared to wagmi's native hooks

[carletex]

This looks great, good job everyone.

Merged some conflicts from #248 and pushed a couple of tiny tweaks.

I've been testing it, and everything looks great. I'm going to merge, let's keep building and fix any issue that we might encounter.

---

Something that we need to figure out at some point

1. Think if we can handle the not-deploy issue better (running yarn start before yarn chain / deploy
2. Docs for people that don't want to use SE2 smart contracts (some dApps just use external contracts, or don't use contracts at all, e.g. signing with a backend). This might be related to NPX installs.

I'll create an issue for this.

[carletex]

Just read your last comment @technophile-04 , sorry!

Even I am confused a bit, although I tend to like the current state where we are doing union with undefined for each index inside a tuple because I think it makes hooks easy to use and also will increase DX for beginners too.

I agree!

But would love to hear @carletex and @rin-st thoughts on this, I have also created test branch -> https://github.com/scaffold-eth/se-2/tree/fix/116-type-inference-test where I have put wagmi's useContractRead and useContractWrite below our useScaffoldRead/write to compare for Errors / DX and find out edge cases if we are missing in our hooks compared to wagmi's native hooks

Thanks!!! I'll take a look.

Stil going to merge, so we can start testing. I have a couple of builds lined up and can't wait to use the new hooks :) Using them would be the best way to see if need to make some tweaks.