bug: deployedContracts file being out of sync

[kevinjoshi46b]

Current Behavior

Currently the deployments folder in hardhat has been gitignored leading to all the data related to deployments being stored only on local computer.

The deployedContracts file in nextjs is being generated based on this deployments folder whenever a new deployment takes place.

Consider a scenario where you deployed the smart contract on polygon from your machine which updated the deployedContracts file with its address, abi etc. You committed and pushed all the changes to github. Now someone else working on the same project pulled the changes and deployed the contract on arbitrum, what happens is the deployedContracts file gets updated with the deployement data of only arbitrum and looses the data from polygon since the second person doesn't have the deployements folder containing data related to deployment on polygon chain on their computer.

Expected Behavior

The expected behavior would be when the second person deploys the contract on arbitrum the deployedContracts file should be updated by appending the data related to arbitrum deployment and not loosing any previously existing deployment data related to other chains

Anything else?

Removing the deployments folder in hardhat from gitignore solves this issue of inconsistency. I am not sure on why it was added but I am currently unable to see any reason for its existence if there is some please do enlighten me.

[sverps]

Fair point. One remark, though, is that anything generated on a local chain is only present locally, so I think it's fine to keep deployments/localhost in .gitignore in any case.

Committing stuff deployed to other chains to a repo makes sense.

[carletex]

Hey @kevinjoshi46b

Thanks for posting this issue.

I noticed this when working with multiple people and deploying different smart contracts. As you said, the source of truth is the deployments folder inside hardhat (this is because we use the hardhat-deploy lib).

I saw your PR #370 where you un-gitignore the deployments folders. That will fix the issue but also will create a lot of noise with the localhost contracts. So maybe we want to ignore the localhost folder.

Related to this (thinking about the localhost contracts «noise»), I'm wondering if we should have different files for the generated folder in NextJS.

1. deployedContracts.ts. All non-localhost contracts deployed from SE-2
2. deployedLocalhostContracts.ts. All localhost contracts deployed from SE-2 (gitignored)
3. externalContracts.ts. Allow people to add external contracts (existing contract, not deployed from your SE-2 repo) External contract #271

No need to fix it in the same PR/Issues, just saying it out loud for future action. 2 also might be tricky with types (we ended up having an "empty" deployedContracts.ts file to detect that no contracts have been deployed.)

What do you guys think? @technophile-04 @sverps

[carletex]

(Sorry I missed @sverps comment while writing mine) Same page tho! : D

[sverps]

Hmmm, if we were to split the generated contract files and gitignore the local ones, we'd again run into the issues that lead to it being included. We'd need to figure out a post-install step etc. So not sure if we win much by doing so.

Keeping the generated contract files in the repo also ensures that types aren't broken until someone runs a deploy step, which is easier for pre-commit hooks and pipelines to validate stuff. So I'd keep things as they are now concerning the generated contract typescript files.

[damianmarti]

I think that it's helpful to have different files for the local, non-local, and external contracts, and have the non-local and external contracts inside the repository.

About the comment from @sverps "Hmmm, if we were to split the generated contract files and gitignore the local ones, we'd again run into the issues that lead to it being included.", I'm not sure what was "the issues that lead to it being included".

In doubt, I think that it is better to add all files to git, and then the developer can decide if a change inside these files makes sense to commit the changes or not.

[sverps]

I'm not sure what was "the issues that lead to it being included".

In summary, the types derived from this contract file, which is imported somewhere, so it must exist. When it wasn't in the repo (due to gitignore), we had 2 options:

• the user needed to run yarn deploy in a first step. Not ideal, since not all projects require a deployed contract, they might use only external contracts, etc.
• we could put the file with null contract from a postinstall script. Here we ran into issues that postinstall scripts are removed from the last yarn version, and it wasn't working reliably.

So we went for the option to just include it in the repo, and simplify a bunch of scripts.

[kevinjoshi46b]

I feel the current state where the deployedContracts file is being added to git is the correct approach since it makes things easier for projects where there are multiple people working. (Easy to keep deployed contracts in sync via git rather than manual exchange if gitignored)

About ignoring the deployments/localhost folder I do agree that ignoring localhost deployments is the correct approach. But yes as @carletex mentioned its not just the deployments/localhost folder, the local deployment details are also updated in the deployedContracts file in nextjs which should also be ignored and I feel the multifile approach is the best solution for it. Probably it would make sense to change the folder name from generated -> contracts in nextjs

I would like to work on this issue, for now atleast splitting the current single deployedContracts file into two, local contracts and non-local contracts.
Maybe implementing external contracts can be kept for #271

[carletex]

Thanks all for the feedback, good discussion :) It's a bit tricky!

A few thoughts:

=> As @damianmarti and @kevinjoshi46b say, having the contracts split into 3 files (local, non-local, external) is a good DX. In the same way that we don't want to commit the deployments/localhost hardhat folder, I can't think of a use case where we need/want to deploy the localhost contracts in the front end (a bunch of PRs have that file changed with localhost deployments, and we have to tell people to reset it)

But, as @sverps said, we had some issues with the types, and ended up adding the file (was .gitignored before). Not the ideal solution, but it works!

Some context: #255, #260, #282

This is the file in question: packages/nextjs/utils/scaffold-eth/contract.ts. If the deployedContracts.ts file exports null, we do some conditional logic with the Types, so the type checking doesn't break (pre-commits hooks, CI/CD, etc). This is not perfect, but it solves the issue. Maybe is there an alternative?

Probably it would make sense to change the folder name from generated -> contracts in nextjs

Yeah, this makes sense to me

[sverps]

@carletex @kevinjoshi46b Maybe we can create some custom resolver logic in webpack to try and resolve the generated local contracts, and if it doesn't exist, take some fallback file that exports null. This would have the same behavior we have now and that could resolve the earlier challenges we had.

It'll be tricky to properly resolve the types based on this custom webpack resolver and based on configured network though, but maybe there's a way to get this working.

[kevinjoshi46b]

You are talking about editing the hardhat/scripts/generateTsAbis.ts file and do something similar for local contracts right?

PS, I am new to TS and I got overwhelmed after opening the packages/nextjs/utils/scaffold-eth/contract.ts file but I am willing to understand and learn it so, I might require a little guidance here and there. Also please don't mind if I end up saying anything that doesn't make sense cause probably I might be understanding things a bit incorrectly.

[sverps]

The functionality of hardhat/scripts/generateTsAbis.ts has moved to hardhat/deploy/99_generateTsAbis.ts.

[kevinjoshi46b]

Ya I think we can edit it to read the chain id and apply our separation for local chain.

I will try working on this.

[technophile-04]

I think this has gotten a lot easier after #592,

We could probably achieve this by :

1. Updating 99_generateTsAbis.ts to create two separate deployments files as discussed in here :

   1. deployedContracts.ts
   2. deployedLocalhostContracts.ts

2. In nextjs/utils/scaffold-eth/contract.ts :

// contract.ts
const allDeployedContracts = deepMergeContracts(deployedContracts, deployedLocalhostContracts)
const contractsData = deepMergeContracts(allDeployedContracts, externalContractsData);

@kevinjoshi46b could you please revamp #370 / create a fresh PR since a lot of things have changed, please let us know if you need any help 🙌, also happy to take this if you are busy 🙌