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

better handling of deployments #666

Merged
merged 14 commits into from Dec 22, 2023
Merged

better handling of deployments #666

merged 14 commits into from Dec 22, 2023

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Dec 19, 2023

Description:

Summary from #369:

Current Issue:
The deployments folder in Hardhat is .gitignored, leading to deployment data being stored locally. This causes issues when different team members deploy contracts on different networks (like Polygon and Arbitrum). The deployedContracts file in Next.js only reflects the latest deployment, losing data about previous deployments on other networks. Even while one person is working, I think its good to keep track of deployments/{non-localhost} in git.

Expected Behavior:
Ideally, deploying a contract on a new network should append data to the deployedContracts file without losing existing data from other networks.

Solution discussed:

  1. un-ignore the deployments folder in Hardhat (deployments/localhost will still be ignored).
  2. Separate the generated deployedContracts file into two files: deployedLocalhostContracts (git ignored) and deployedContracts. The former will be tracked by git, and the latter will be ignored by git.

Problem with 2.:
To make types work properly deployedLocahostContracts needs to be present so that typescript can infer the types. But if deployedLocalhostContracts is git ignored, it won't be present on a fresh clone. Also we can't require deployedLocalhostContracts optionally since again TS won't be able to infer the types. checkout this comment by Samuel. Also, another thing is we need to push deployedLocalhostContracts (with empty object) to github so that CI checks pass other wise it will fail giving an error (no such file is present).

This PR summary:

Added git update-index --assume-unchanged packages/nextjs/contracts/deployedLocalhostContracts.ts in postinstall script. This is to prevent the file from being tracked by git, so any changes made to this file will be ignored. This ensures we don't get annoying deployedLocalhostContracts modified messages when we run git status and also prevents us from pushing the modified file to github.

Also added a step in pre-commit hook that reverts any changes done to deployedLocalhostContracts.ts (if any). We can probably remove this.

Lol not sure if it looks too hacky, this was just a try and we could completely close it 🙌 / iterate on other approaches to handle this

Testing:

To test it you will need to create a new clone and test since postinstall script only runs when dependency tree is changed.

Try this :

git clone -b manage-deployments https://github.com/scaffold-eth/scaffold-eth-2.git test-deployments

And then cd test-deployments, yarn install and other SE-2 commands.

@carletex
Copy link
Member

Thanks for this @technophile-04 !

I'm all in with 1). But not sure if 2) is worth it: the assume-unchanged is a bit hacky and another extra file in contracts adds more overhead/confusion. Not a big deal committing 31337 contracts. I also like seeing the deployedContracts file changed when they deploy something... even if it's localhost.

I'll think about it a bit more tho.

@technophile-04
Copy link
Collaborator Author

Yup yup completely agree !!! Also, I feel 2. is not worth it due to the extra addition of files which kind of bloats contracts dir too, and hacky solutions to get it properly working.

Also doing 1.) I think it solves the main issue nicely and we don't need 2.) at all for that.

Planning to update this PR to just gitingore deployements/localhost

@technophile-04
Copy link
Collaborator Author

Reverted all the changes, Just added deployments/localhost to .gitignore

Copy link
Member

@carletex carletex left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @technophile-04

@carletex carletex merged commit 07134f5 into main Dec 22, 2023
1 check passed
@carletex carletex deleted the manage-deployments branch December 22, 2023 09:56
@damianmarti
Copy link
Collaborator

@technophile-04 this is great!! Thanks!!

@github-actions github-actions bot mentioned this pull request Dec 29, 2023
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.

None yet

3 participants