scaffold-eth / scaffold-eth-2

Open source forkable Ethereum dev stack
https://scaffoldeth.io
MIT License
1.3k stars 808 forks source link

Handling wagmi & viem vesions #492

Closed technophile-04 closed 1 year ago

technophile-04 commented 1 year ago

Context :

We have configured AddressType to string instead of 0x{string} for better developer experience here in this file.

  1. We are using viem and wagmi as our dependencies

  2. wagmi and viem both internally are using abitype as their dependency

Ideally, we shouldn't care about patch version releases but since wagmi and viem are under constant development they are pretty actively fixing bugs and publishing new versions.

Since wagmi and viem are kind of the backbone of SE-2 and since we are a development template/starter kit and not a product , I think it makes sense to keep them updated obviously not every release but at least keep them checked after 1month or so because they add new chains, fix bugs, etc. <-- Would love to know what others thoughts on this

-> The problem happens when there is a new patch version of wagmi and viem and we need to update them.

Ideally, it should have been as easy as yarn workspace @se-2/nextjs up wagmi viem since we are just updating them to **patch** version. Checkout #419 "Steps to reproduce" section what chaos it causes when you do yarn workspace @se-2/nextjs up wagmi viem :(

Since we have AddressType as mentioned in begging this causes problems with types :(, We have faced this multiple times when trying to update wagmi

This occurs whenever wagmi's internal version of abitype differs from viem's internal version of abitype.

Checkout this issue https://github.com/wagmi-dev/wagmi/issues/1712#issue-1554554144 comments, We created this issue even before viem was launched but there was a version mismatch of abitype between wagmi's repo internal packages

And now since viem also uses abitype anytime there is a version mismatch (which there is currently). Checkout https://github.com/wagmi-dev/wagmi/issues/2736 because of which we have to lock version's of wagmi and viem at #420

Solutions :

  1. Make PR's / create issues to Wagmi repo to let them know and fix on their side (which we have been doing from the start). Not best solution because it might take time to resolve (like in current situtaion)

  2. Keep updating wagmi normally as it is and update viem only if its internal abitype version matches with wagmi. In recent PR #491 I just updated wagmi keeping viem same because they both have abittype version 0.8.7

Obviously removing AddressType configuration is not an option since it will decrease the developer experience for beginners a lot :(

I don't know if this is something we could handle on our own like forcing typescript to use our configuration of AddressType irrespective what abitype versions wagmi and viem are using, I tried playing and didn't find any good option, Would you love to hear from others 🙌

We can go with point 2. of the solution but there might be a point when we will break because in case in future if wagmi might break with viem version we are using and we also won't be able to update viem since its abitype version will be different from wagmi

dgrcode commented 1 year ago

Let's see if I understood the issue. There are 4 elements at play here:

The problem comes when the versions we use from wagmi and viem depend on different abitype versions. Correct?

I don't fully understand why the type mismatch ("Type error: Type 'string' is not assignable to type '`0x${string}`'") only happens when there's a version mismatch between wagmi and viem. Do we know why this is?

Regarding the two proposed solutions, I think 2 makes more sense as it doesn't force us to rely on other projects.

Two additional options that come to mind:

  1. Force yarn/pnpm to use the version of abitype we choose, even for our dependencies. From a quick search it looks like it's possible with pnpm, although I haven't checked anything myself.

  2. Use a different approach to redefining AddressType. You already mention that removing the redefinition of the type would lead to worse DX, but I'm not sure if a different approach would make things work while keeping a good DC. For example, adding a new AddressHashType like this:

    type AddressHashType: string
    type AddressType: `0x${AddressHashType}`

    Then we would use AddressHashType on our end, and AddressType when interacting with the libraries. We might even provide wrappers that just check for the first two characters to be 0x. But as I say, I'm don't know how AddressType is used and if this option makes sense. Just sharing in case it (or some other approach) could work.

technophile-04 commented 1 year ago

I don't fully understand why the type mismatch ("Type error: Type 'string' is not assignable to type '0x${string}'") only happens when there's a version mismatch between wagmi and viem. Do we know why this is?

Yeah as I explained https://github.com/wagmi-dev/wagmi/issues/1712#issue-1554554144 here, but here is the problem with current context :

Checkout packages/nexjs/node_module/viem/node_moudles :

Screenshot 2023-08-16 at 5 25 57 PM

Here as you can see there is not abitype in viem/node_modules


Now run this command : yarn workspace @se-2/nextjs up wagmi viem

Screenshot 2023-08-16 at 5 30 50 PM

You will see that it created a new abitype under viem/node_moudles, since there was a version mismatch it installed the particular version viem wants

Also now please do: yarn next:check-types

Screenshot 2023-08-16 at 5 35 31 PM

Along with AddressType not being resolved correctly, for some weird reason typescript starts checking types inside node_modules :(, This is another bug which needs to solved


Force yarn/pnpm to use the version of abitype we choose, even for our dependencies. From a quick search it looks like it's possible with pnpm, although I haven't checked anything myself.

This might be the solution indeed(Lol hopefully it dosen't mess up viem typing while using it) !! I just tried adding yarn resolution and now I am just getting 7 errors after yarn workspace @se-2/nextj up wagmi viem :

Screenshot 2023-08-16 at 5 46 19 PM

Still don't know why its checking node_moduels :(, But I feel this may be an option 🙌 Tysm !!!!


  1. Then we would use AddressHashType on our end, and AddressType when interacting with the libraries. We might even provide wrappers that just check for the first two characters to be 0x. But as I say, I'm don't know how AddressType is used and if this option makes sense. Just sharing in case it (or some other approach) could work.

We actually had parseAdderess0x function earlier but checkout this comment -> https://github.com/scaffold-eth/scaffold-eth-2/pull/96#issuecomment-1397536834

Also AddressType is deep-rooted in wagmi and viem and every time the user uses any of this will have to use our utility and will also need to refactor alot of stuff to handle it

But really thanks Dani really appreciate it 🙌 Will keep digging on pt.3 of your suggestion kind of look promising to me 🙌

damianmarti commented 1 year ago

Yes, using yarn resolutions may fix this error. I had a similar problem with ethers some time ago and I was able to fix it using resolutions at package.json

If this doesn't work, I think that 2 is a good option at least for now (and in the meantime trying to have a final fix from Wagmi).

Thanks!!

dgrcode commented 1 year ago

@technophile-04 Thanks for the explanation! Now I understand it better.

Still don't know why its checking node_moduels

I'm guessing (not sure at all), it might check those files because they're imported by some of our source files? Maybe there's a way to tell TypeScript to resolve our types, and not the types from each package's node_modules. I found something worth trying here. It talks about using the compilerOptions.path option in tsconfig.json. In our case it'd look something like:

{
    "compilerOptions": {
       ...
        "paths": {
            "*": [
                "src/*",
                "types/*"
            ]
        }
    },
    ...
}

We actually had parseAdderess0x function earlier but checkout this comment

I agree it's not the best DX if the utility has to be used all over the place. But maybe having some wagmiWrappers like this:

// wagmiWrappers.ts
import { foo as wagmiFoo } from 'wagmi'

export const foo = address => wagmiFoo(`0x${address}`) // with the 0x

// in the code
import { foo } from './wagmiWrappers' // this could be `from 'wagmi-wrappers'` if we wanted to do remappings

const address = 'd8dA6BF26964aF9D7eEd9e03E53415D37aA96045' // without the 0x
const result = foo(address)

Just thinking out loud. Again, I don't know if something like this would be feasible or if it makes sense to implement.

sverps commented 1 year ago

Looking into it, my first feeling is it might be best to just get rid of the abi type override in abi.d.ts. Users that use se-2 now get the wrong impression that abitype and wagmi/viem internals use string for addresses, whereas that's not really true and might lead to confusion down the line if someone ever makes a project leveraging these libraries directly. This practice isn't ideal in any case, since doing so could result in run-time errors that would otherwise be caught by TypeScript.

So to resolve this issue, it might be better to either use the Address type as it is (0x${string}) where possible, and where we prefer to use string (for simplicity or to accept an ENS string or so), we can just cast the string to Address when needed. This might resolve the root issue we have, and at the same time will serve as an example to users in how to manage types.

I'll try and look into what this approach would look like, maybe there's additional drawbacks that I haven't considered yet.

rin-st commented 1 year ago

So to resolve this issue, it might be better to either use the Address type as it is (0x${string}) where possible, and where we prefer to use string (for simplicity or to accept an ENS string or so), we can just cast the string to Address when needed

As I understand, we need AddressType: string just in one place - AddressInput. Tried to use 0x{string} in other places and didn't find problems with it.

What about dividing AddressInput component to BaseInput for entering a string, real AddressInput for 0x{string} and EnsInput for strings which is ENS (isEns(value) is true)? So we'll not need cast address to string anymore. Yes, working with different inputs in one place could be not so smooth, but I think we can make it good enough. For example for editing mode always show base input. If value becomes valid just use needed component for address/ens and drop focus. Maybe show suggestions before it.

What do you think? I can try to implement it next days

Obviously removing AddressType configuration is not an option since it will decrease the developer experience for beginners a lot :(

And could you please explain how is it decrease dx?