feat: proactively disable read hook when it would result in error

[sverps]

Fixes #249

Proposed fix still allows the enabled value to be overridden by devs that use the hook. It basically aligns the default enabled behavior with the args that are now a tuple of elements that are unions with undefined.

[carletex]

Thanks @sverps

From #249

I think a more intuitive behavior is to disable the hook when we know it would result in an error, and only call the contract function when the params are good. That way, the types force a dev to plug in correctly typed variables for args, and when a user using the UI does some action (i.e. fill in a form) that actually provides a value for the variables, only then is the actual contract function called.

In this PR we are only checking if one of the args is undefined, so yes, we avoid a bunch of console errors in the case that a read Hook has multiple arguments and the builder uses a form (until the user starts typing in the last argument input).

If it only takes 1 args:

const [address, setAddress] = useState("");

const { data } = useScaffoldContractRead({
  contractName: "YourContract",
  functionName: "userGreetingCounter",
  args: [address],
});

return (
  <>
    <AddressInput value={address} onChange={v => setAddress(v)} />
    <p>{data?.toString()}</p>
  </>
)

In this case, you'd still get all the errors, right? You might save 1 error if you use useState<string | undefined>(); on load.

Reduce contract calls (or at least reduce triggers of the wagmi hook)

Yes, I think the contract is never called (RPC calls not fired) until the args are OK. We just get the errors from Ethers (that wagmi uses under the hood)

---

We can definitely merge this, but I guess a better solution could be to check if the args are valid depending on their type (Address, number, etc)

Thoughts?

[rin-st]

Good job @sverps !

But I agree with Carlos, it works until we start to change the last value (or change other values after it).

common approach is to create a form to let a user provide his input. These form states are then used as params to pass into the args

I believe it's not a best practice, and the user should trigger a reading hook only when he completes filling out the form by clicking to "Submit"/"Read" button and switching enabled until the next values change. Ideally, all the input errors are handled in the form. If we can try to do it inside the hook, it will be difficult to understand why it doesn't work.

[sverps]

We can definitely merge this, but I guess a better solution could be to check if the args are valid depending on their type (Address, number, etc)

Thoughts?

The tricky thing is that we're pinched between widening the types to allow transitory values (e.g. an empty string or a partial ENS before it's resolved to its actual address), and keeping better type hints.

For example: the IntegerInput currently allows a string | BigNumber value, and our hooks complain about the value being used since an arg might need BigNumber | undefined. We could widen the arg to also accept string, but then the type hints a dev gets are less informative, and he might plug in an entirely wrong string under a falsy assumltion based on type hints.

So we'll have to explore a bit further and figure out how to nicely have our hooks play together with our inputs.

The main use-case this fix solves is when using outputs from one contract read to use as inputs of another read (e.g. in the multisig challenge, you read the nonce and some other things, and then plug it into the getTransactionHash). Here is where this fix works best, because the value for this nonce will either be undefined (while resolving the contract call) or of the correct BigNumber type (after it has resolved).

[technophile-04]

I think we can merge this as it is for now

but I guess a better solution could be to check if the args are valid depending on their type (Address, number, etc)

Yup definitely agree with this but after as Samuel mentioned in #250 (comment) and also playing around with it some time I do feel its kind of tricker and need through exploration and playing around.

Maybe we can think about this more once #352 is merged, what do you guys think?