Use config file for network and other configuration #231

technophile-04 · 2023-03-09T19:39:13Z

Fixes #230

…pollingInterval

sverps · 2023-03-10T08:24:18Z

packages/nextjs/scaffold.config.ts

+  pollingInterval: number;
+};
+
+const ScaffoldConfig: TScaffoldConfig = {


I think we should settle on a styleguide and apply it on the whole project (I like google's styleguide as a good starting point).

That means here we'd follow default naming convention and make the const lowerCamelCase.

As for the type, I think the convention should be to not prefix or suffix it and use regular UpperCamelCase, since typescript itself already contains the info about what kind of type it is (https://google.github.io/styleguide/tsguide.html#naming-style).
Prefixes should, in my opinion, be reserved for generic types, to mark a clear distinction between defined types and generics.

@rin-st @carletex any thoughts?

I agree 👍

I'm up for a having a styleguide.

Besides applying it here, we should create an issue an for it, discuss what guidelines we want to follow..... and make a PR refactoring all the names / types / etc.

Opened an issue about it 👍

carletex · 2023-03-10T16:58:19Z

packages/nextjs/utils/scaffold-eth/networks.ts

+  const chainNames = Object.keys(chains);
+
+  const targetChainArr = chainNames.filter(chainName => {
+    const wagmiChain = chains[chainName as keyof typeof chains];
+    return wagmiChain.id === chainId;
+  });
+


An idea (without testing it 👼 ).

What if we make the argument network to be wagmi's Chain? So we don't need to do this conversion.

In the transactor, we could get the connected network with wagmi's useNetwork() (and maybe throw an error / return if the signer is on a different network)

// useTransactor const { chain } = useNetwork(); // check that network.chainId === chain.id => if not, error / return getBlockExplorerTxLink(chain, ...)

Since we are using wagmi's Chains as source of truth, maybe we should rely on them whenever we can? Instead of using internal ethers/providers.

Yup makes sense, lol even I was a bit uncomfortable while writing the conversion,

What if we make the argument network to be wagmi's Chain? So we don't need to do this conversion.

This is a great idea but the problem is useTransactor accepts custom signer too and the reason we are relying on the signers network is that :
NITPICK: There might a case where people pass in the custom signer without connecting their wallet and in that case useNetwork and signer network will be different and might cause some inconsistency, we are using similar pattern in Faucet.jsx.

Again the above NITPICK is a very rare case.

Will try exploring some more alternatives 🙌

Hey Shiv, good point!!

After pulling the threads on this, some notes:

The only instance where we are passing a custom signer, is for the hardhat faucet (where we want to send funds from a hardhat account with funds). Can we think about other use case? Since we are in the front-end, 99.99% of the times we'll want to use the connected signer (besides the faucet, and maybe a exotic build where we do deterministic private key derivation or something)

getBlockExplorerTxLink doesn't return anything for the local chain (the only instance where we are using a custom signer for the transactor 🤔 )

getLocalProvider is only used in the Faucet stuff. Could be an unneeded early abstraction.

Maybe an util like getChainFromChainId that returns a wagmi Chain for a given Id?

In any case, no need to make any updates rn!! just thinking out loud and pointing out some possible inconsistencies. Maybe we made some past decisions that could use some refactors for the sake of consistency / simplicity / better DX.

Thanks!

Tysm everything makes sense !!

Maybe we made some past decisions that could use some refactors for the sake of consistency / simplicity / better DX

+++

technophile-04 · 2023-03-13T18:47:34Z

Small update

Moved scaffold.config to root and same is being utilised by both next and hardhat

btw I came across some patterns where people have created config directory similar to nextjs and harhdat where all the base configurations live like .eslintrc, .tsconfig etc, just wanted to put here since we are in "refactoring phase" :) and also we can put scaffold.config inside it. I like the way currently it is now but would love to know others thought

Will mark this PR as ready once proper testing and try to make things more consistent

sverps · 2023-03-13T19:47:22Z

btw I came across some patterns where people have created config directory similar to nextjs and harhdat where all the base configurations live like .eslintrc, .tsconfig etc, just wanted to put here since we are in "refactoring phase" :) and also we can put scaffold.config inside it. I like the way currently it is now but would love to know others thought

I'm used to those types of config files being at the root of their package, that's where I would generally look for them, so it doesn't bother me personally.

Do you know if all config files will play nice if they're not at the root of a package (e.g. tailwind.config.js, postcss.config.js, next.config.js)? I'd only consider it if all configs can be grouped, otherwise there will be 2 locations for config files, which would be a bit confusing.

carletex · 2023-03-14T10:12:54Z

scaffold.config.ts

+	targetNetwork: chain.Chain;
+	pollingInterval: number;


I think this are tabs

carletex · 2023-03-14T10:20:33Z

packages/nextjs/tsconfig.json

@@ -15,7 +15,8 @@
    "jsx": "preserve",
    "incremental": true,
    "paths": {
-      "~~/*": ["./*"]
+      "~~/*": ["./*"],
+      "@root/*": ["../../*"]


I still get this on my nextjs console:

I ran yarn install and restarted nextjs. Am I missing something?

Yes !! encountered the same problem, don't know for some reason it was working but I am sure i am missing some configuration, working on it

carletex · 2023-03-14T10:40:04Z

Moved scaffold.config to root and same is being utilised by both next and hardhat

This is great! I think having an unique config file it's more dx friendly. A few thoughts/questions:

Could/Should we add a single source of truth here about networks? We have the networks.ts file on next, and we use the networks also in hardhat.config.ts. I'm just wondering if we could define a network object with all the data we need in scaffold.config, for hardhat & next (wagmi) to use. "Networks" feel like a key & transversal piece for SE-2 (or any dApp)
Should we also support env vars in the config file? Specifically for secrets, since people are going to commit the config file. Things like "DEPLOYER_PRIVATE_KEY" or even alchemy/infura keys that could live in config.

const scaffoldConfig: ScaffoldConfig = {
	targetNetwork: chain.hardhat,
	pollingInterval: 30000,
        deployerdPrivateKey: process.env.DEPLOYER_PRIVATE_KEY
} as const;

I think we'll just need the dotenv package on the root. Handling all the config & env vars in the root seems more friendly. I think it will work on Vercel when manually defining the env vars in their UI.

This is just me thinking out loud, would love to hear your view on it.

btw I came across some patterns where people have created config directory similar to nextjs and harhdat where all the base configurations live like .eslintrc, .tsconfig etc, just wanted to put here since we are in "refactoring phase" :) and also we can put scaffold.config inside it. I like the way currently it is now but would love to know others thought
I'm used to those types of config files being at the root of their package, that's where I would generally look for them, so it doesn't bother me personally.

Since this is a monorepo with two packages, it also doesn't bother me having separate config files. But I've also seen monorepos with a shared (or config) directory.. I guess it's an alternative to having everything shared in the root (when is too bloated). It's definitely something that we should explore, but maybe in the future / another PR? This PR is already a big refactor.

…for hardhat

…-truth

technophile-04 · 2023-03-15T17:17:31Z

Edit

Found a way to solve vercel deployments, updating it ...

Update

Now scaffold.config.ts at root should work but there are some catches :

The way the front end was deployed previously is changed.

Since we are utilizing scaffold.config.ts which is at root, outside the nextjs dir when you deploy to vercel it gives the below error :

Previously we were uploading the whole source code to vercel and then running build there.

Now, the flow is: first you need to build locally and then publish only build dir to vercel similar to surge.sh in SE

I have create script alias which will do it :
yarn vercel:prod - will build and deploy to vercel on production
yarn vercel:dev - will build and deploy as preview
yarn vercel:yolo - ignore errors and deploy to production

Approaches explored :

Although using a .ts file at monorepo root seems like very simple / famous case but was not able to find good examples/references to deal with it having nextjs.

Using externalDir which is experimental feature by nextjs, it seems to do the work but it prints the warning when you start the app :

checkout this disccusion / comment => vercel/next.js#32192 (comment)

And this issue: vercel/next.js#20374 why there are problems.

Currently using nextjs built in module-transpilation which was built in nextjs by author of next-transpile-modules, I saw people using next-transpile-modules as a workaround for the problem which we faced and now its inbuild so utilzing it.

I tried comparing build size of both approaches and they came to be same :

using `module-transpilation`	using `experimental`

Others, there some approaches where people used reconfigured webpack in next.config.js but found them really hacky and not future proof
Maybe we can just put back scaffold.config.ts inside nextjs

Some thoughts :

Yes I think new deployment flow is a bit off but I think if we want scaffold.config.ts to stay out of nextjs dir then we need to go with current approach, btw the current approach build time is really fast. Also would love to know others thoughts / approaches to conquer this
This was just a draft to see how it will look if we put scaffold.config.ts in root, happy to revamp this whole thing 🙌

Regarding putting .envs in scaffold.config.ts love the idea even tried exploring it but again there seems problem with nextjs using external env's

People seem to use dotenv inside next.config.js to read from external .env and pass in env's inside nextConfig the problem with this approach is that the env's you pass in are inlined in javascript while bundling i.e only allowed to pass in NEXT_PUBLIC and this will lead to exposing unnecessary keys if you are not mindful + found this approach a bit hacky

cehkcout -> https://stackoverflow.com/questions/69259896/set-environment-variables-outside-of-pages-dir-in-nextjs
and https://stackoverflow.com/questions/59462614/how-to-use-different-env-files-with-nextjs/69619744#69619744

PS : the above stackoverflow links were picked from differnt next js issues

I think maybe we can keep our api key's as hardcoded in scaffold.config.ts so that it can be used in both hardhat.config.ts and wagmiClient and let nextjs handle env's since nextjs aslo has env.production and etc we would manually need to handle its a bit messy chekcout -> https://stackoverflow.com/a/74154252/16480510

Also I saw one pattern where env's were managed in .yml file checkout -> https://stackoverflow.com/a/69619744/16480510

cc @carletex @rin-st @sverps please feel free to suggest other approaches/suggestions happy to give it another try, I am sure I might have missed something 🙌

carletex · 2023-03-16T08:30:40Z

Hey @technophile-04 Thank you for this and your detailed explanation. Super helpful!

I feel that this got out of hand: my fault for suggesting the root config file haha. But we had to explore it and see what the issues were. I still feel that we want to aim for it, but let's do it in a more iterative / incremental way.

Proposal

Since the most important reason for this was to enable #202, I think we should go with the simple approach for now, and keep exploring the root / shared stuff for the monorepo.

Let's create a config file only for Next (replacing the .env.* files). This way we won't have any issues with Vercel are the changes will be minimal.

What do you think?

Thank you Shiv!

technophile-04 · 2023-03-16T09:52:13Z

In the latest commit 4c4e4a6 now deployments are back to normal and no need to prebuilt stuff as mentioned here #231 (comment) but just a very small catch 🙃 :

Although this is one time config, you need to mention the path to nexjts dir :

Reason :

Since we using monorepo, vercel offically suggests to invoke vercel cli from root of monorepo and then pass in source code location to solve the problem mentioned here 4c4e4a6

Checkout Vercel docs monorepo FAQ -> https://vercel.com/docs/concepts/monorepos

Now the flow for the target network is :

scaffold.config.ts is a single source of truth for both nextjs and hardhat.

Just doing yarn deploy will deploy to network configured in scaffold.config.ts and yarn start will use it as target network in frontend.

Flow for configuring env's :

How env's works remain the same as the main branch.

What's changed?
I have hardcoded our API keys inside scaffoldConfig so that they can be used by both hardhat and `nextjs, for example Alchemy providerKey.

Also removed .env.* from frontend since all handled by scaffold.config now.

reason: It's now saying that Scaffold-eth is "configured" with some handy api keys which you can access through scaffold.config.

TODO :

Handle this properly Use config file for network and other configuration #231 (comment)
If we plan to have global env's then implement them, checkout out the last section for resistance faced Use config file for network and other configuration #231 (comment)

Happy to handle them both in different PRs, since this is already big and don't want us blocking on #202 🙌

rin-st · 2023-03-16T14:22:42Z

package.json

  },
  "packageManager": "yarn@3.2.3",
  "devDependencies": {
+    "@types/node": "^18.15.3",


Are we really need it? It seems everything works without it

Tysm !! yes yes we can remove it, it was added here 8f19b21 and since now we are not using .env at root we can completely remove it

rin-st · 2023-03-16T14:24:16Z

packages/hardhat/hardhat.config.ts

 // If not set, it uses the hardhat account 0 private key.
-const deployerPrivateKey =
-  process.env.DEPLOYER_PRIVATE_KEY ?? "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80";
+const deployerPrivateKey = process.env.DEPLOYER_PRIVATE_KEY ?? scaffoldConfig.deployerPrivateKey;


minor nitpick: above and below we use ||, so probably it's better to use it here too

rin-st · 2023-03-16T15:25:37Z

packages/nextjs/next.config.js

@@ -9,6 +9,7 @@ const nextConfig = {
  eslint: {
    ignoreDuringBuilds: process.env.NEXT_PUBLIC_IGNORE_BUILD_ERROR === "true",
  },
+  transpilePackages: ["se-2"],


could you explain please, why we need it?

if you look at #231 (comment) in the section

Approaches explored :
2.

TLDR;
Since we have scaffold.config.ts outside the nextjs directory if you don't pass in this it gives this error -> #231 (comment)

The reason I have put se-2 there is because we had scaffold.config.ts at se-2 workspace and needed to transpile it during next build :

Although would highly recommend giving a quick read of the section Approaches explored especially the links mentioned in that section 🙌

Also would love to hear if there are any alternatives / suggestions 🙌

Thank you! ❤️ You've done such a big research, it's hard even to understand everything at first sight :D

rin-st · 2023-03-16T15:37:15Z

Great job, @technophile-04 !

Sorry for the delays in comments, sometimes I suddenly need to go 🤷‍♂️

carletex · 2023-03-16T16:07:00Z

Great job @technophile-04

I think we are not far, but still doesn't feel 100% right to me.

1. Vercel root directory

It might be like a little thing, but for me, it decreases the deploy experience by a bunch. I tried to search for options for the CLI / vercel.json config file, but it doesn't seem that you can provide a default root directory option. The --cwd option doesn't work either.

Not sure what we could try.

2. .env vars

If we plan to have global env's then implement them, checkout out the last section for resistance faced #231 (comment)

I think we need .env vars for secrets.

Answer 1
Right now on scaffold.config.ts:

const scaffoldConfig: ScaffoldConfig = {
  // ....
  deployerPrivateKey:
    "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80",
 // ..
} as const;

and then const deployerPrivateKey = process.env.DEPLOYER_PRIVATE_KEY ?? scaffoldConfig.deployerPrivateKey; on hardhat.config.ts. It's confusing, since we have the deployerPrivateKey on the config file and it's not clear that people need to create a .env file inside the hardhat folder

I think it should be:

const scaffoldConfig: ScaffoldConfig = {
  // ....
  deployerPrivateKey: process.env.deployerPrivateKey || 
    "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80",
 // ..
} as const;

So people can create a .env file in the root folder (we might have a .env.example) that it's meant for secrets. And secrets consumers just read scaffoldConfig.deployerPrivateKey. Same for alchemyApiKey and etherscanApiKey.

And make sure people understand that they should never use scaffoldConfig to store secrets directly (even if we are doing so 😅)

Maybe we could pass the secrets/env vars like this to Vercel:

If people create/update an env var from the Vercel UI, they will take effect if they rebuild (they are being used on the scaffold.config.file)?

Answer 2

Use scaffold.config.ts only for public variables (targetNetwork && pollingInterval), and handle the secrets env vars in its own package:

Hardhat.. Same as in main (.env file)
NextJS. We'd replace .env.* with the config file, but people could still use .env / .env.local / Vercel UI to set up secrets and use them in their API routes or whatever.

This way we don't mix config & secret env vars... and things might be simpler.

Handle this properly #231 (comment)

This is not a big deal for now. We could create an issue for that refactor.

Definitely having a root config file makes things a bit more complicated. I'll be up for it if we find a nice balance. But #231 (comment) is still on my mind.

Thoughts?

technophile-04 · 2023-03-16T18:43:13Z

Tysm all for the review !! 🙌

I think we are not far, but still doesn't feel 100% right to me.

Lol yes !, even I have this feeling it doesn't feel 100% right

I have made #246 which has config in frontend only as suggested in #231 (comment)

I plan to explore more on putting .config at the root, Planning to also open some issues/ discussions on vercel github to find/suggest better solutions. Example: configuring project root as Carlos mention for verceli-cli even I tried finding hard and there were no options to configure it

Marking this as a draft, Thanks 🙌

…-truth

technophile-04 · 2023-06-07T06:41:22Z

I have created a discussion on Vercel for Pt.1 of #231 (comment) -> https://github.com/orgs/vercel/discussions/2145

Closing this for now, since not under active development and until SE-2 cli is up

technophile-04 added 2 commits March 9, 2023 17:16

add config and refactor getTargetNetwork, update wagmiConnectes with …

6c38c96

…pollingInterval

remove usage of env's and add ignoreBuildError to next.config

667850d

technophile-04 marked this pull request as draft March 9, 2023 19:39

sverps reviewed Mar 10, 2023

View reviewed changes

update getBlockTxnUrl inline with scaffold.config

89e6363

carletex reviewed Mar 10, 2023

View reviewed changes

rin-st mentioned this pull request Mar 12, 2023

TypeScript styleguide #232

Closed

technophile-04 added 4 commits March 13, 2023 22:12

move config file to root

8af5ee4

update import of scaffold.config from root file

6ba8641

add root path alias to hardhat dir

223ceee

update default network to hardhat

7eb48fb

use chainIds for comparison instead of objects

ffc66c5

carletex reviewed Mar 14, 2023

View reviewed changes

scaffold.config.ts Outdated

Comment on lines 4 to 5

targetNetwork: chain.Chain;

pollingInterval: number;

Copy link

Member

carletex Mar 14, 2023

Choose a reason for hiding this comment

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

I think this are tabs

carletex reviewed Mar 14, 2023

View reviewed changes

Merge branch 'main' into refactor/singleSource-truth

012783b

carletex mentioned this pull request Mar 14, 2023

Contracts type Inference / autocompletions (& custom hooks vs wagmi hooks) #116

Closed

technophile-04 added 8 commits March 15, 2023 10:18

add config at root, use experimental next feature, use relative path …

bf5b3ee

…for hardhat

put hardhat env's in root config

8f19b21

add dotenv in nextjs and get ENVS from scaffold.config

ee08128

add next_public_client example in env.example

eb40c96

remove dotenv from next, and update scripts to deploy frontend

d1a21cb

remove NEXT_PUBLIC example from .env.example

e4c379c

Merge remote-tracking branch 'origin/main' into refactor/singleSource…

494901f

…-truth

remove expermintal feature and use transpilePackages instead

3aab99f

technophile-04 added 2 commits March 16, 2023 13:08

remove vercel from nextjs and add it to root

7d5727a

update deploy scripts

4c4e4a6

carletex mentioned this pull request Mar 16, 2023

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

Merged

technophile-04 added 2 commits March 16, 2023 15:01

put back env to original places

9a2a9da

remove env's from nextjs

672dd89

carletex marked this pull request as ready for review March 16, 2023 10:16

rin-st reviewed Mar 16, 2023

View reviewed changes

provideApiKey => alchemyApiKey. Add some comments in config file

4b28387

rin-st reviewed Mar 16, 2023

View reviewed changes

technophile-04 mentioned this pull request Mar 16, 2023

Use .config for frontend instead of env's #246

Merged

technophile-04 marked this pull request as draft March 16, 2023 18:43

technophile-04 mentioned this pull request Mar 29, 2023

use targetNetwork blockExplorer link for AddressComponent #266

Merged

technophile-04 added 6 commits April 20, 2023 19:35

Merge remote-tracking branch 'origin/main' into refactor/singleSource…

cdfba57

…-truth

add burner config in root config

26271c5

remove scaffold.config from nextjs

3b039b4

udpate paths for scaffold.config

f342f45

fix yarn.lock dependencies

29a818e

Merge branch 'main' into refactor/singleSource-truth

9be4d59

technophile-04 closed this Jun 7, 2023

technophile-04 mentioned this pull request Jul 14, 2023

bug: Vercel deployment fails #419

Closed

1 task

carletex deleted the refactor/singleSource-truth branch July 28, 2023 16:59

technophile-04 mentioned this pull request Sep 8, 2023

Upgrade to pnpm #444

Closed

This was referenced Sep 26, 2023

fix vercel deployments version mismatch #544

Closed

Vercel CLI not able to pass "directory of code location" in monorepo vercel/vercel#10609

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use config file for network and other configuration #231

Use config file for network and other configuration #231

technophile-04 commented Mar 9, 2023

sverps Mar 10, 2023 •

edited

Loading

rin-st Mar 10, 2023

carletex Mar 10, 2023

sverps Mar 10, 2023

carletex Mar 10, 2023

technophile-04 Mar 10, 2023

carletex Mar 11, 2023

technophile-04 Mar 13, 2023 •

edited

Loading

technophile-04 commented Mar 13, 2023 •

edited

Loading

sverps commented Mar 13, 2023 •

edited

Loading

carletex Mar 14, 2023

carletex Mar 14, 2023

technophile-04 Mar 14, 2023 •

edited

Loading

carletex commented Mar 14, 2023

technophile-04 commented Mar 15, 2023 •

edited

Loading

carletex commented Mar 16, 2023

technophile-04 commented Mar 16, 2023 •

edited

Loading

rin-st Mar 16, 2023

technophile-04 Mar 16, 2023

rin-st Mar 16, 2023

rin-st Mar 16, 2023

technophile-04 Mar 16, 2023

rin-st Mar 16, 2023

rin-st commented Mar 16, 2023

carletex commented Mar 16, 2023

technophile-04 commented Mar 16, 2023 •

edited

Loading

technophile-04 commented Jun 7, 2023

Use config file for network and other configuration #231

Use config file for network and other configuration #231

Conversation

technophile-04 commented Mar 9, 2023

sverps Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technophile-04 Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

technophile-04 commented Mar 13, 2023 • edited Loading

Small update

sverps commented Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technophile-04 Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

carletex commented Mar 14, 2023

technophile-04 commented Mar 15, 2023 • edited Loading

Edit

Update

Approaches explored :

Some thoughts :

carletex commented Mar 16, 2023

technophile-04 commented Mar 16, 2023 • edited Loading

Reason :

Now the flow for the target network is :

Flow for configuring env's :

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rin-st commented Mar 16, 2023

carletex commented Mar 16, 2023

1. Vercel root directory

2. .env vars

technophile-04 commented Mar 16, 2023 • edited Loading

technophile-04 commented Jun 7, 2023

sverps Mar 10, 2023 •

edited

Loading

technophile-04 Mar 13, 2023 •

edited

Loading

technophile-04 commented Mar 13, 2023 •

edited

Loading

sverps commented Mar 13, 2023 •

edited

Loading

technophile-04 Mar 14, 2023 •

edited

Loading

technophile-04 commented Mar 15, 2023 •

edited

Loading

technophile-04 commented Mar 16, 2023 •

edited

Loading

technophile-04 commented Mar 16, 2023 •

edited

Loading