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

Migrate to foundry #867

Merged
merged 27 commits into from
Jun 28, 2024
Merged

Migrate to foundry #867

merged 27 commits into from
Jun 28, 2024

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Jun 19, 2024

TODOs:

  • Port foundry from create-eth and get it working
  • Figure out a way to set packages/foundry/.env
    • In future maybe as soon the repo is cloned / installed (maybe postinstall script? or maybe a make file or initial bash script?)
    • Solved: For now we are suggesting cp packages/foundry/.env.example packages/foundry/.env in README
  • Clean up scripts in foundry package a bit
    • Solved: For now moved all JS files under scripts-js so that people who are familiar with foundry don't feel weird by seeing js files in script dir.
    • In future, we can move all JS scripts to from foundry package to another package in monorepo (we could use TS too there)
  • Update readme (recursive submodules or forge install)
  • Configure husky pre-commit hook to do basic foundry lint
    • Runs foundry fmt && prettier as pre-commit step in packages/foundry

Testing steps:

Follow this branch README: https://github.com/scaffold-eth/scaffold-eth-2/tree/foundry-main?tab=readme-ov-file#-scaffold-eth-2

Next todos:

@technophile-04 technophile-04 marked this pull request as ready for review June 24, 2024 13:54
@technophile-04
Copy link
Collaborator Author

#812 maybe something we could handle in different PR nicely?

@portdeveloper
Copy link
Contributor

Do you think if it could be any better if we could combine the following steps into one?
image

Could yarn install && forge install --root packages/foundry and cp packages/foundry/.env.example packages/foundry/.env be one step in the install process for people who have just started learning onchain dev?

Maybe it could be one command: yarn setup or yarn buidl

@carletex
Copy link
Member

This is great stuff @technophile-04 tysm!! I'll start testing and report back!

README.md Outdated
Comment on lines 41 to 49
2. Create `packages/foundry/.env` file based of on `packages/foundry/.env.example`

```
cp packages/foundry/.env.example packages/foundry/.env
```

> NOTE: If you are using windows and not using git bash or wsl, use `copy packages\foundry\.env.example packages\foundry\.env`

3. Run a local network in the first terminal:
Copy link
Member

Choose a reason for hiding this comment

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

I saw that you already commented about this in the PR comment. Having to do this is a PITA (the 3 magic commands don't work out of the box), so I've been trying some options (postinstall as you mentioned)

branch: https://github.com/scaffold-eth/scaffold-eth-2/commits/foundry-main--cp-env/
commit: 951f733

Using the smallest copy lib that I could find (even tho it installs a few other deps, maybe we can explore other options), which should work on any OS.

I remember that we had some issues with the postinstall script (not running at all in some OS? Or running after every time you run yarn install? Added the --no-overwrite in case it's the latter).

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

If we take this path, this can help to decide about the lib to use. https://pkg-size.dev/

eg. https://pkg-size.dev/cpy-cli => 1.3MB vs the 5 KB advertised (which doesn't count the deps)

Copy link
Collaborator Author

@technophile-04 technophile-04 Jun 25, 2024

Choose a reason for hiding this comment

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

After researching for a while I think postinstall seems a safe option.

Some notes on `postinstall` script:
  1. Postinstall in yarn will run only when something in root node_modules changes(which means some package is added in dependency or devDependency of root package.json) or node_moduels is initialized

    example image :

    Screenshot 2024-06-25 at 1 55 55 PM

  2. Postinstall in yarn does not log output to stdout, which means If you do postinstall: echo 'hello world' it won't log hello world. It will only show output on error (example this image )

  3. Running yarn install multiple times won't run postinstall multiple times until you add any package to root pacakage.json. If you add npm package in nextjs it won't run.

So considering all the above points, I think postinstall seems a fine options for us

not running at all in some OS?

Even I had the same feeling, but went through SE-2 issues throughly and could only found this : #275

The above issue was not related too much related to postinstall(since postinstall ran) but it was related to node js script not resolving proper path on windows.

So yeah 951f733 seems good if it works nicely on windows (hopefully it resolves path correctly internally)

Another options we could also put our own node script at root of monorepo and call it in postinstall: node startUp.js but we need to make sure it works properly on windows the path resolution thing.

Copy link
Collaborator Author

@technophile-04 technophile-04 Jun 26, 2024

Choose a reason for hiding this comment

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

Was just trying https://github.com/scaffold-eth/scaffold-eth-2/tree/foundry-main--cp-env and found a minor disadvantage that:

  1. First you clone above repo and yarn install this creates .env.example nicely in pacakge/foundry
  2. If you try to install node dependency in foundry package cd packages/foundry and yarn add viem it will fail :
    Screenshot 2024-06-26 at 2 31 44 PM

basically you won't be able to install any node / js dependency in foundry package because it will fail since we are using ---no-overwrite it throwing error

An alternatie solution will be finding a library which doesn't throw error or writting our own node script which doesn't throw error if .env is already present. The only reason I am afraid of writing our own script is because of #275 where isn't properly able to get the path but maybe we should give it another shot?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is what we need: https://www.npmjs.com/package/shx (we can use commands from linux, and we could copy... or even test if the file exist before copying)

And it takes less space https://pkg-size.dev/shx

I'll give it a shot

Copy link
Member

Choose a reason for hiding this comment

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

What about this (using shx): 46443fb

I pushed it to https://github.com/scaffold-eth/scaffold-eth-2/commits/foundry-main--cp-env/

If we like it, we can have Pablo to test on Windows.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Noiicee, just tried it out and works great!! Tysm!!, I think this works great!! Will merge in this changes!

@rin-st
Copy link
Member

rin-st commented Jun 25, 2024

Is it only for me that when I'm using metamask with foundry network, my setGreeting transactions are always pending?

Reset account fixed this

@technophile-04 technophile-04 changed the base branch from main to foundry June 25, 2024 10:56
@technophile-04
Copy link
Collaborator Author

Was talking with Carlos and just switched the base branch from main to foundry , thinking that #867 (comment) would be really nice if we get it working before we merge all the foundry changes to main branch and then announce the changes also was mentioned in #868.

So for this PR we get this #867 (comment) solved merge it to foundry . And then tackle #812 merge it to foundry and after that finally merge foundry => main

@technophile-04
Copy link
Collaborator Author

technophile-04 commented Jun 27, 2024

Just merged the postinstallchanges mentioned in #867 (comment) works nicely for me!. cc @Pabl0cks could you test it on windows once w/ without wsl and gitbash also if others could give another try too would be great 🙌

To test first run:

git clone https://github.com/scaffold-eth/scaffold-eth-2.git -b foundry-main scaffold-eth-foundry
cd scaffold-eth-foundry
yarn install && forge install --root packages/foundry

And then the three command yarn chain, yarn deploy, yarn start should work out of the box

@rin-st
Copy link
Member

rin-st commented Jun 27, 2024

if others could give another try too would be great

for me it also works! 👍

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.

Just merged the postinstallchanges mentioned in #867 (comment) works nicely for me!. cc @Pabl0cks could you test it on windows once w/ without wsl and gitbash also if others could give another try too would be great 🙌

Awesome job @technophile-04 for tackling this and @carletex @rin-st for the great reviews! ❤

It's working great on Windows with Git Bash, also installed and configured WSL with Ubuntu 20.04 and is working great too 🙌

@damianmarti
Copy link
Member

Just merged the postinstallchanges mentioned in #867 (comment) works nicely for me!. cc @Pabl0cks could you test it on windows once w/ without wsl and gitbash also if others could give another try too would be great 🙌

To test first run:

git clone https://github.com/scaffold-eth/scaffold-eth-2.git -b foundry-main scaffold-eth-foundry
cd scaffold-eth-foundry
yarn install && yarn install && forge install --root packages/foundry

And then the three command yarn chain, yarn deploy, yarn start should work out of the box

Everything working great on Ubuntu 20.04!!

@technophile-04 Great job!!!

@portdeveloper
Copy link
Contributor

Tried it on a win11 + git bash machine
It works great, without any issues!
🚀🚀🚀
Incredible work @technophile-04 !!!

@technophile-04
Copy link
Collaborator Author

Thanks all merging this in foundry branch 🙌

@technophile-04 technophile-04 merged commit e1b39d0 into foundry Jun 28, 2024
@technophile-04 technophile-04 deleted the foundry-main branch June 28, 2024 08:34
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.

6 participants