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

Wagmi v2 migration #700

Merged
merged 77 commits into from
Apr 15, 2024
Merged

Wagmi v2 migration #700

merged 77 commits into from
Apr 15, 2024

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Jan 24, 2024

Current noticed bugs:

  • Rainbowkit modal not showing burnerwallet icon
  • When args are type address, then you need to assert 0x${string} for example checkout this (fixed at 23179fa)

📝 Notes:

  1. Wagmi migration guide
  2. Viem migration guide
  3. Rainbowkit migration guide

Misc Notes:

  1. In case we get Chain not configured error make sure we pass chainId to hooks (eg b6f406a)
  2. Alchemy Networks mapping: https://github.com/alchemyplatform/alchemy-sdk-js/blob/master/src/types/types.ts#L81
  3. why callbacks like onError were removed from useQuery (RFC: remove callbacks from useQuery TanStack/query#5279)
  4. React query defaults: https://tanstack.com/query/latest/docs/framework/react/guides/important-defaults
Helpful resources for burnerWallet :

Done tasks:

  • Fix TODO comments before merging
  • Think about alchemy links mapping (07e91f5)
  • Update blockexplorer page related hooks and utilities
  • Get Types working properly for scaffold hooks and overall app (updating contract.ts file)
  • Update scaffold hooks
  • Get Burner Connector working (@technophile-04)
  • UI breaks if other chains(expect localhost) are used in targetNetworks (@technophile-04)
  • Get yarn start working (@technophile-04)
  • Make faucet work and remove all warning from home page (@technophile-04)
  • Update debug page and get it working (@technophile-04)
  • Update useTransactor (@technophile-04)

Fixes #678

@technophile-04 technophile-04 marked this pull request as draft January 24, 2024 16:04
@KcPele
Copy link
Contributor

KcPele commented Jan 27, 2024

[ ] Update scaffold hooks

IN PROGRESS

@KcPele
Copy link
Contributor

KcPele commented Jan 27, 2024

Should we also change the names of some hooks and the variables in the files
@technophile-04 @carletex
here is an example:

Old Name New Name
useContractWrite useWriteContract wagmi
useContractRead useReadContract wagmi
useScaffoldContractWrite useScaffoldWriteContract
writeAsync writeContract
wagmiContractWrite wagmiWriteContract
useScaffoldContractRead useScaffoldReadContract

@technophile-04
Copy link
Collaborator Author

Hey @KcPele thanks for this !! Actually we are planning to remove useAutoConnect hook in this migration checkout #535 (comment)

Also for Wagmi v2 migration, I think its better if core contributors take up the tasks since they have overall context and also its easy for review since this migration seems a bit messier

But thanks so much for showing enthusiasm!

@technophile-04 technophile-04 mentioned this pull request Jan 27, 2024
2 tasks
@KcPele
Copy link
Contributor

KcPele commented Jan 27, 2024

Ok, thanks for the feedback, I am very much available if you need any task done, I would like to be assigned any minor migration task

Copy link
Collaborator

@damianmarti damianmarti left a comment

Choose a reason for hiding this comment

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

It's working great for me!!

Awesome job @technophile-04 !!!

@technophile-04
Copy link
Collaborator Author

Been testing this for a while on the branch wagmi-v2-test and everything looks good to me : https://wagmi-v2-test.vercel.app/


Thanks to @Pabl0cks, noticed a bug yesterday that if you are using useScaffoldWatchContractEvent and useScaffoldEventHistory (with watch true) at same time for same event then for some reason the onLogs function of useScaffoldWatchContractEvent is not triggered.

In SE-2 wagmi v1 it seems to works fine.

I created two branches to test wagmi-v1-test and another wagmi-v2-contractEvent.

Here is wagmi-v1-test demo video :

Demo video:
Screen.Recording.2024-04-10.at.2.55.33.PM.mov
  • Its working nicely here, we see the listener is triggered.
  • Its only triggered once.
  • Notice in hardhat logs there is no call to eth_uninstallFilter

Here is wagmi-v2-contractEvent demo video :

Demo Video:
Screen.Recording.2024-04-10.at.3.01.02.PM.mov
  • If you remove pollingInterval from useScaffoldWatchContractEvent then it does not work at all (the onLogs is never triggered).
  • If you add pollingInterval in this branch I hardcoded it to 1s then it works.
  • ^ But its triggered multiple times even when there is no event fired.
  • There is this eth_uninstallFilter call in hardhat logs.

Not sure if it's something to do with how tanstack query is working internall (maybe since both hooks has same query key and its kindof getting confused / not able to handle it properly since our useScaffoldEventHistory has also some wired behaviours #633 ). Or maybe with wagmi v2 / viem v2 creating this eth_uninstallFilter call.

Nevertheless, I think we should merge this PR if everything looks good to all and then we can tackle this issue separately and nicely

Thanks all for testing and reviews !!!

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

Nevertheless, I think we should merge this PR if everything looks good to all and then we can tackle this issue separately and nicely

Agree! Approving 🙌

Two points:

  • During my tests I've also noticed it was taking some time to refresh the updates on balances/variables in localhost, something like 10-12 secs. Maybe we could lower the pollingInterval for hardhat.
  • Maybe we could sync the merge with docs update.

Incredible job @technophile-04 tackling this update, hats off to you! 🎩🤓

@technophile-04
Copy link
Collaborator Author

During my tests I've also noticed it was taking some time to refresh the updates on balances/variables in localhost, something like 10-12 secs. Maybe we could lower the pollingInterval for hardhat.

My stupid brain used the reversed condition to configure pollingInterval :(, Should be fixed at 643d061, Thank @Pabl0cks for noticing it 🙏!


Ok so, I think we are ready to merge this maybe this week or early next week, as soon as is scaffold-eth/se2-docs#61 is ready 🚀

Just writing summary / pending things from this PR :

Discussion on useScaffoldWriteContract :

For this PR, we are going with useScaffoldWriteContract("YourContract") API

  • Advantages: Nicely inlined with Wagmi v2
  • Disadvantages: the same values returned from hook like {data, isPending} will represent every writContract(Async) more details here
    • workaround: You can create multiple instances of same hook.
  • Conclusion: Lets keep eye on discussion / issues / TG chats if lot of people are really feeling uncomfortable / confused with this. Substitute useScaffoldWriteContract => useScaffoldWriteContractFunction and keep the API similar to Wagmi v1

Bug using useScaffoldWatchContractEvent & useScaffoldEventHistory({watch: true}) at same time :

  • BUG: If you are using both hooks on same page/component for the same event name and useScaffoldEventHistory has watch enabled then the onLogs callback of useScaffoldWatchContractEvent is not fired.
  • Workaround : Don't use both the hook at same time for same event, Incase you use them at same time make sure watch: false (by default its false) for useScaffoldEventHistory
  • Potential Cause :
    • Since this occurs when both hook watches for same event, I think this is something to do with how tanstack query / wagmi v2 is handling things (maybe because they will have same query key and its confused)
    • I maybe me completely wrong on this and will try to debug this
  • Conclusion: Create an issue around this and handle this in separate PR since it's not that common people using both this hooks at same time

TYSM ALL for testing and reviewing it 🙌!!

@carletex
Copy link
Member

Thank you @technophile-04 for this amazing work! And @damianmarti @Pabl0cks @rin-st for the great feedback & discussions.

Happy to merge this when the docs are ready.

@technophile-04
Copy link
Collaborator Author

technophile-04 commented Apr 12, 2024

Happy to merge this when the docs are ready.

I think we are ready with docs scaffold-eth/se2-docs#61, Thanks to @Pabl0cks 🚀

@carletex
Copy link
Member

Great! Let's wait until Monday haha

@carletex
Copy link
Member

@technophile-04 Whenever you want => push the 🔴 button haha

@technophile-04
Copy link
Collaborator Author

TYSM All !! Just did a final test locally, deployed contracts and works nice (https://wagmi-v2-test.vercel.app) here is the wagmi-v2-test (We could probably delete this branch after 3-4 days), Lol I am sure there might be some bugs / edge cases which we have missed but we can conquer them as they come 🙌

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.

Wagmi V2
7 participants