Fix autoConnect when contract not deployed #292

technophile-04 · 2023-04-07T18:55:46Z

Issue :

It seems that due to the below check autoConnect does not work if you use SE-2 that doesn't use deployed contract :
https://github.com/scaffold-eth/se-2/blob/ce83d166df62083a3aac86a6dda1802737a02fa8/packages/nextjs/hooks/scaffold-eth/useAutoConnect.ts#L68

Demo not working :

Selected Mainnet as targetNetwork

Screen.Recording.2023-04-08.at.12.14.00.AM.mov

Fixed by removing the if(contract) from both useAutoConnect & Faucet, it seems that check was added due console getting bloated with errors and warning but I think it was fixed in other commits

checkout OG comment : https://github.com/scaffold-eth/se-2/pull/255/files/43f82e58bab2f624b60f520e68cfff749fe541e7#r1147414048

Demo with only `yarn chain` runnig and deployed contract is null :

Screen.Recording.2023-04-08.at.12.05.47.AM.mov

Demo with `yarn chain` not running :

Screen.Recording.2023-04-08.at.12.08.58.AM.mov

carletex · 2023-04-07T19:34:15Z

Thanks for the videos and the explanation, Shiv.

Demo with only yarn chain runnig and deployed contract is null :

This one looks good!

Demo with yarn chain not running

I added this one to avoid those errors on projects that don't use contracts. E.g: https://hackathon.buidlguidl.com/

What would be the alternative? Switching targetNetwork to something different than hardhat?

damianmarti · 2023-04-10T19:02:54Z

Thanks for the videos and the explanation, Shiv.

Really nice explanations and videos to save PR review time!!

Demo with only yarn chain runnig and deployed contract is null :

This one looks good!

Demo with yarn chain not running

I added this one to avoid those errors on projects that don't use contracts. E.g: https://hackathon.buidlguidl.com/

What would be the alternative? Switching targetNetwork to something different than hardhat?

Maybe something like targetNetwork: false?

sverps · 2023-04-10T21:39:43Z

Maybe something like targetNetwork: false?

Not a fan of assigning values that don't really match the variable. Would also break some types where targetNetwork is used and expected to be a network.

We could, however, add another config in scaffold.config, something like autoConnect.enabled?

damianmarti · 2023-04-10T21:55:33Z

Maybe something like targetNetwork: false?

Not a fan of assigning values that don't really match the variable. Would also break some types where targetNetwork is used and expected to be a network.

We could, however, add another config in scaffold.config, something like autoConnect.enabled?

You are right!

And what about targetNetwork: null? I mean, if you are not using a chain at all, sounds right not to be needed to configure any network. But it's true that if we set it to null, we should take care that the targetNetwork can be null...

Maybe we can just add another config param and that's it!

technophile-04 · 2023-04-12T16:11:16Z

Just summarising the discussion so far:-

Reason for discussion :

We don't want to show a "Cannot connect to the local provider" and other errors if the user wants to build only the frontend app.

Solution discussed :

Giving users a single config option to do so

Question :

Just wanted to know what this option should actually do meaning should it remove "Connecting with wallet" and any other stuff?

The reason I am saying this is because people using SE-2 will surely at least want "Connecting with wallet" functionality be it signing message or other, If they want this functionality it will lead to the user configuring targetNetwork to some network because it will also handle "Switch to right network" and other things

So I see telling people to configure the network to "mainnet" or another network in the README as the best option to get started building only frontend since requires 0 to minimal changes and aslo won't confuse with more options.

I really appreciate the solutions suggested and am happy to explore but had little doubt about what things should we should "turn off" with this configuration option ?

Tysm all

sverps · 2023-04-12T18:34:41Z

So I see telling people to configure the network to "mainnet" or another network in the README as the best option to get started building only frontend since requires 0 to minimal changes and aslo won't confuse with more options.

Yeah, lets keep things simple and just explain it in the readme, since the existing config already allows users to resolve the issue. 👍

damianmarti · 2023-04-13T13:03:13Z

So I see telling people to configure the network to "mainnet" or another network in the README as the best option to get started building only frontend since requires 0 to minimal changes and aslo won't confuse with more options.

Yeah, lets keep things simple and just explain it in the readme, since the existing config already allows users to resolve the issue. +1

I agree with that too!

And maybe @carletex can check if setting the network to mainnet fix the issue that he had at https://hackathon.buidlguidl.com/

technophile-04 · 2023-04-20T10:38:37Z

And maybe @carletex can check if setting the network to mainnet fix the issue that he had at https://hackathon.buidlguidl.com/

cc @carletex for review and merge / thoughts

…ployed contract

technophile-04 · 2023-04-20T13:28:38Z

README.md

@@ -123,6 +123,8 @@ If you want to redeploy to the same production URL you can run `yarn vercel --pr

 **Hint**: We recommend connecting the project GitHub repo to Vercel so you the gets automatically deployed when pushing to `main`

+**Hint**: If you don't have any contract deployed through SE-2 and plan to just deploy NextJS app maybe only with "connect wallet" functionality you can do so by just setting `targetNetwork : chains.mainnet` or your preferred network in `packages/nextjs/scaffold.config.ts`, rest all instruction remains same as [above](#deploying-your-nextjs-app)


Added a small hint it can be improved a lot please feel free to suggest idea, aslo was wondering should we create a separate section ? I feel like its not worth it because README.md has really grown alot

What if we add a minimal hint in the faucet error, so we don't need to add more stuff here?

Like adding "You can also change your targetNetwork in scaffold.config.ts" below "Did you forget to run yarn chain?".

We could also add some "code" style to the yarn chain and scaffold.config.ts text like we do here:

Update the hint text a bit to fit in properly, hope it makes sense :

Before :

The empty space looked weird to me but happy to revert 🙌

packages/nextjs/components/scaffold-eth/Faucet.tsx

carletex

Left some minor comments @technophile-04 thanks! ;)

packages/nextjs/hooks/scaffold-eth/useAutoConnect.ts

packages/nextjs/components/scaffold-eth/Faucet.tsx

carletex · 2023-04-21T08:07:43Z

README.md

@@ -123,6 +123,8 @@ If you want to redeploy to the same production URL you can run `yarn vercel --pr

 **Hint**: We recommend connecting the project GitHub repo to Vercel so you the gets automatically deployed when pushing to `main`

+**Hint**: If you don't have any contract deployed through SE-2 and plan to just deploy NextJS app maybe only with "connect wallet" functionality you can do so by just setting `targetNetwork : chains.mainnet` or your preferred network in `packages/nextjs/scaffold.config.ts`, rest all instruction remains same as [above](#deploying-your-nextjs-app)


What if we add a minimal hint in the faucet error, so we don't need to add more stuff here?

Like adding "You can also change your targetNetwork in scaffold.config.ts" below "Did you forget to run yarn chain?".

We could also add some "code" style to the yarn chain and scaffold.config.ts text like we do here:

carletex · 2023-04-21T15:39:43Z

packages/nextjs/components/scaffold-eth/Faucet.tsx

@@ -21,22 +20,28 @@ export const Faucet = () => {
  const [sendValue, setSendValue] = useState("");

  const { chain: ConnectedChain } = useNetwork();
-  const provider = getLocalProvider(localhost);
+  const provider = useMemo(() => getLocalProvider(localhost), []);


What if we move this outside/above of the component?

const provider = getLocalProvider(localhost); export const Faucet = () => { //... }

Would it have the same effect?

Nice !! I should have thought about this before using useMemo, this works !

Update at c7ef8b7

* Update SE-2 nomenclautre (#317) * Fix autoConnect when contract not deployed (#292) * oxford comma lol * Fix icon visibility on dark mode in EtherInput (#320) * Update README: Add Vercel ENV var to disable type/lint checking (#327) * Fix typo (#334) Fix typo at code snippet * README: Updated SE-2 custom hook section (#330) Co-authored-by: KcPele <fidekg123@gmail.com> Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com> Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com> * Remove duplicated HINT on README (#335) * PR and Issue templates (#329) * Native currency symbol and price as per target network (#322) * useAppStore => useGlobalState (#338) * Event history filter type (#333) * fix: event history filter type Fixes #332 * fix: make scaffold event history config separate type * fix: prettier broke due to invalid syntax * fix: ts error after generating contract * Indexed prop + example --------- Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com> * Update README: custom hooks section (#339) * fix: assert type as Abi to avoid ts error (#350) * fix: assert type as Abi to avoid ts error Fixes #344 * assert type as Abi to avoid ts error on useScaffoldContract --------- Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com> * Fix useEffect filter dep on event hook (#349) * Block confirmation options / callback for useScaffoldContractWrite (#348) * Patch useLocalStorage setting the right initial value to avoid a hydration error (#356) Co-authored-by: sverps <sverps@gmail.com> * Save selected contract to local storage (#326) * Save selected contract to local storage * refactor: Replaced native localStorage with useLocalStorage from usehooks-ts * Fixed empty string parameter error. * Updated parameter requirements to fix issue. * Fixed error. * Fixed parameter assignment error. * Use useLocalStorage to save selected contract --------- Co-authored-by: Damian <damianmarti@gmail.com> * Refactor meta tags into separate component and add default thumbnail / image (#359) * Hotfix: thumbnail.png => thumbnail.jpg * Local Block Explorer (#351) Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com> * Fix daisyUI dropdown on IOS (#342) * allow `overrides` in useScaffoldContractWrite (#345) * allow overrides for useScaffoldContractWrite * spread overrides and restConfig separately * feat: proactively disable read hook when it would result in error (#250) Fixes #249 Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com> * Size prop on Address component (#365) Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com> * Generate TS ABIs from deploy script (#368) * enable solidity optimizer by default (#360) * fix(Balance): font-size of symbol should be relative to current font-size, not document root (#375) * fix(contract.ts): very long abi's now no longer go over type instantiation limit (#377) Fixes #374 * fix(contract.ts): improve missing config check (#372) Fixes #371 * fix listner types error when contracts missing (#379) * zkSyncEra Testnet config (#383) * Upgrade hardhat-deploy to 0.11.26 where zkSync is supported * Dependency for @matterlabs/hardhat-zksync-solc * Add zkSyncEraTestnet to networks * Import @matterlabs/hardhat-zksync-solc * Add artifacts-zk and cache-zk to gitignore * yarn install * Add zkSync mainnet config * add hardhat-zksync-verify for contract verification --------- Co-authored-by: moltam89 <moltam89@gmail.com> Co-authored-by: Damian <damianmarti@gmail.com> Co-authored-by: Shiv Bhonde <shivbhonde04@gmail.com> * allow writeAsync by useScaffoldContractWrite to be called with updated args (#385) * update wagmi & rainbow for walletConnectV2 (#381) * meged changes from main, additional configuration in subgraph.yaml --------- Co-authored-by: Shiv Bhonde | shivbhonde.eth <shivbhonde04@gmail.com> Co-authored-by: Austin Griffith <austin@ethereum.org> Co-authored-by: Kevin Joshi <kevinjoshi46b@gmail.com> Co-authored-by: Carlos Sánchez <oceanrdn@gmail.com> Co-authored-by: AlehN <natsevsky@gmail.com> Co-authored-by: Pablo Alayeto <55535804+Pabl0cks@users.noreply.github.com> Co-authored-by: KcPele <fidekg123@gmail.com> Co-authored-by: Samuel | solidixen.eth <sverps@gmail.com> Co-authored-by: Eda Akturk <edakturk96@gmail.com> Co-authored-by: Tamas Molnar <tamas.molnar@liferay.com> Co-authored-by: Damian Martinelli <damianmarti@gmail.com> Co-authored-by: Alexander <a00112699@gmail.com> Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com> Co-authored-by: moltam89 <moltam89@gmail.com>

technophile-04 added 2 commits April 8, 2023 00:01

remove contract present check from useAutoConnect

eef0aef

remove contact check and stringify provider in useEffect

bca02c4

technophile-04 requested review from carletex and sverps April 7, 2023 18:55

Merge branch 'main' into fix/burner-autoConnect

72416a7

damianmarti closed this Apr 13, 2023

damianmarti reopened this Apr 13, 2023

technophile-04 added 3 commits April 19, 2023 13:51

Merge branch 'main' into fix/burner-autoConnect

57d01d9

Merge branch 'main' into fix/burner-autoConnect

99710d3

Merge branch 'main' into fix/burner-autoConnect

d6394b9

add small hint in README for only deployment of NextJs app without de…

1cc475e

…ployed contract

technophile-04 commented Apr 20, 2023

View reviewed changes

sverps reviewed Apr 20, 2023

View reviewed changes

packages/nextjs/components/scaffold-eth/Faucet.tsx Show resolved Hide resolved

technophile-04 added 2 commits April 21, 2023 11:21

use useMemo for getting provider

eb3a0de

use then/catch pattern inside useEffect

c68d2f4

carletex reviewed Apr 21, 2023

View reviewed changes

technophile-04 added 3 commits April 21, 2023 18:45

Merge branch 'main' into fix/burner-autoConnect

7bf267c

add hint in notifcation instead of README

39d1b0e

revert useEffect to async/await

9e109cf

technophile-04 requested review from carletex, sverps and damianmarti April 21, 2023 14:21

carletex reviewed Apr 21, 2023

View reviewed changes

move getLocalProider outside the component

c7ef8b7

technophile-04 requested a review from carletex April 21, 2023 16:03

carletex merged commit 0f1eabc into main Apr 21, 2023

carletex deleted the fix/burner-autoConnect branch April 21, 2023 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix autoConnect when contract not deployed #292

Fix autoConnect when contract not deployed #292

technophile-04 commented Apr 7, 2023

carletex commented Apr 7, 2023

damianmarti commented Apr 10, 2023 •

edited

Loading

sverps commented Apr 10, 2023

damianmarti commented Apr 10, 2023

technophile-04 commented Apr 12, 2023

sverps commented Apr 12, 2023

damianmarti commented Apr 13, 2023

technophile-04 commented Apr 20, 2023

technophile-04 Apr 20, 2023

carletex Apr 21, 2023

technophile-04 Apr 21, 2023

carletex left a comment

carletex Apr 21, 2023

carletex Apr 21, 2023 •

edited

Loading

technophile-04 Apr 21, 2023

		@@ -123,6 +123,8 @@ If you want to redeploy to the same production URL you can run `yarn vercel --pr

		Hint: We recommend connecting the project GitHub repo to Vercel so you the gets automatically deployed when pushing to `main`

		Hint: If you don't have any contract deployed through SE-2 and plan to just deploy NextJS app maybe only with "connect wallet" functionality you can do so by just setting `targetNetwork : chains.mainnet` or your preferred network in `packages/nextjs/scaffold.config.ts`, rest all instruction remains same as [above](#deploying-your-nextjs-app)

Fix autoConnect when contract not deployed #292

Fix autoConnect when contract not deployed #292

Conversation

technophile-04 commented Apr 7, 2023

Issue :

Demo not working :

Demo with only yarn chain runnig and deployed contract is null :

Demo with yarn chain not running :

carletex commented Apr 7, 2023

damianmarti commented Apr 10, 2023 • edited Loading

sverps commented Apr 10, 2023

damianmarti commented Apr 10, 2023

technophile-04 commented Apr 12, 2023

Reason for discussion :

Solution discussed :

Question :

sverps commented Apr 12, 2023

damianmarti commented Apr 13, 2023

technophile-04 commented Apr 20, 2023

technophile-04 Apr 20, 2023

Choose a reason for hiding this comment

carletex Apr 21, 2023

Choose a reason for hiding this comment

technophile-04 Apr 21, 2023

Choose a reason for hiding this comment

carletex left a comment

Choose a reason for hiding this comment

carletex Apr 21, 2023

Choose a reason for hiding this comment

carletex Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

technophile-04 Apr 21, 2023

Choose a reason for hiding this comment

Demo with only `yarn chain` runnig and deployed contract is null :

Demo with `yarn chain` not running :

damianmarti commented Apr 10, 2023 •

edited

Loading

carletex Apr 21, 2023 •

edited

Loading