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

forge script --watch is not watching anything #4476

Closed
2 tasks done
dalechyn opened this issue Mar 4, 2023 · 10 comments · Fixed by #7247
Closed
2 tasks done

forge script --watch is not watching anything #4476

dalechyn opened this issue Mar 4, 2023 · 10 comments · Fixed by #7247
Labels
C-forge Command: forge Cmd-forge-script Command: forge script T-bug Type: bug

Comments

@dalechyn
Copy link

dalechyn commented Mar 4, 2023

Component

Forge

Have you ensured that all of these are up to date?

  • Foundry
  • Foundryup

What version of Foundry are you on?

forge 0.2.0 (28b2ae6 2023-03-04T00:10:50.272293Z)

What command(s) is the bug in?

forg escript

Operating System

macOS (Intel)

Describe the bug

Greetings folks!
Have a specific use-case where I need to watch-on-file changes and redeploy the script to the anvil (trying to make recompile on-watch along with @wagmi/cli hooks work), but it just looks like the --watch mode does ... nothing?

2023-03-04.21.26.30.mp4
@dalechyn dalechyn added the T-bug Type: bug label Mar 4, 2023
@gakonst gakonst added this to Foundry Mar 4, 2023
@github-project-automation github-project-automation bot moved this to Todo in Foundry Mar 4, 2023
@mds1
Copy link
Collaborator

mds1 commented Mar 5, 2023

What if you explicitly specify the script dir, e.g. forge script -w script/? I'm not sure if watch mode is not supported for forge script, or if the script dir is just not watched by default

@mds1 mds1 added C-forge Command: forge Cmd-forge-script Command: forge script labels Mar 5, 2023
@dalechyn
Copy link
Author

dalechyn commented Mar 5, 2023

What if you explicitly specify the script dir, e.g. forge script -w script/? I'm not sure if watch mode is not supported for forge script, or if the script dir is just not watched by default

Same behavior.

Since it's stayed in the docs I wanted to leverage on that. Thus it's either about removing the --watch mode from docs (maybe it implements the etherscan verifying feature instead of the actual watch?), or actually implementing the watch

@mds1
Copy link
Collaborator

mds1 commented Mar 7, 2023

I don't think there's much value in having a watch mode for scripts, because in many cases it won't work anyway (e.g. create2 deployments), so my preference would be to fix the docs/CLI help

@dalechyn
Copy link
Author

dalechyn commented Mar 7, 2023

@mds1 Why won't it work sorry? Wouldn't you pass the salt via parameter for a factory anyway?

I do understand however that shipping such a feature could be a hard task.

But from the perspective of DX, I would find it really useful to build a custom script that watches the changes in the smart contracts, and automatically deploys the contracts locally with @wagmi/cli parsing the broadcasted contracts to populate in the config to have the addresses set automatically in hooks, which could be a no-env-variables solution for local dApp development.

Since @wagmi/cli config is a js/ts script I could synchronously read the deployment result, but that won't do much if the scripts are not re-deployed on the fly.

Let's reference back to this issue if similar feature requests are opened in the future to be considered needed.

I'm good with changing the docs for now.

@mds1
Copy link
Collaborator

mds1 commented Mar 7, 2023

Why won't it work sorry? Wouldn't you pass the salt via parameter for a factory anyway?

If you pass a salt to the script, in watch mode it's always going to use the same salt, meaning it will deploy to the same address, and since there's already code there from the first deploy, the deploy would fail. So you'd have to restart anvil in between / clear the code from that address

But otherwise sounds good, I'll leave this issue open until CLI help/docs are updated

@mattsse
Copy link
Member

mattsse commented Mar 8, 2023

the only reason --watch is in there is because of code debt, some argument types used by build are reused by script. but --watch is not supported in script.

But from the perspective of DX, I would find it really useful to build a custom script that watches the changes in the smart contracts, and automatically deploys the contracts locally with @wagmi/cli parsing the broadcasted contracts to populate in the config to have the addresses set automatically in hooks, which could be a no-env-variables solution for local dApp development.

this sounds like a valid usecase.
however as @mds1 mentioned, --watch+deploy is probably problematic

@dalechyn
Copy link
Author

dalechyn commented Mar 8, 2023

and since there's already code there from the first deploy

Yes, it would be possible only if forge script --watch could roll back the anvil state to the block before the deployment of the contracts, thus making two pieces of software tied up altogether.

But isn't a local development node running when forge test is called? Can forge script --watch become Anvil-in-Foundry feature too?

@mds1
Copy link
Collaborator

mds1 commented Mar 9, 2023

Unlike hardhat, forge test does not make RPC queries to a local development node, the VM is part of the forge process

@dalechyn
Copy link
Author

Well, then it's certain that the --help docs for forge script has to be modified and that's fair.

@mds1
Copy link
Collaborator

mds1 commented Apr 13, 2023

Just reviewed the above: summary seems to be that the action here is to fix the forge script -h output so the watch flags aren't shown. More generally we might want to take a pass through the all output there, I suspect there's other fields erroneously shown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-script Command: forge script T-bug Type: bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants