foundry-rs / foundry

Foundry is a blazing fast, portable and modular toolkit for Ethereum application development written in Rust.
https://getfoundry.sh
Apache License 2.0
8.17k stars 1.7k forks source link

bug(cheatcodes): differentiate between `FOO=` (unset) and `FOO=""` (empty) in `vm.envOr` #7408

Open Ratimon opened 6 months ago

Ratimon commented 6 months ago

Component

Forge

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

What version of Foundry are you on?

forge 0.2.0 (2d54c1f 2024-03-03T00:21:12.158864000Z)

What command(s) is the bug in?

forge script

Operating System

None

Describe the bug

Reference

My attempts to call writeJson cheat code from the following snippet:

Screenshot 2567-03-15 at 15 10 00

It is called from this snippet

I note that if I hardcode the env var of DEPLOYMENT_OUTFILE=/deployments/.deploy in my .env.local , it works. However when I tried to fallback as following, it suddenly fails as shown above

Screenshot 2567-03-15 at 15 09 13

Config file

For further reference, this is my foundry config and I have set the write access.

Screenshot 2567-03-15 at 15 10 41

Foundry config file

All source code can be found here: code

klkvr commented 6 months ago

We are currently explicitly disallow writing to foundry.toml: https://github.com/foundry-rs/foundry/blob/3fa02706ca732c994715ba42d923605692062375/crates/cheatcodes/src/config.rs#L126

so this is expected, not sure if we consider it reasonable to remove this check at this point cc @mds1 @mattsse

mattsse commented 6 months ago

yeah we don't allow this, because this could be used to inject settings such as permissions, however for the current run the config is already loaded and would be unaffected by this.

what's your use case for this @Ratimon ?

Ratimon commented 6 months ago

yeah we don't allow this, because this could be used to inject settings such as permissions, however for the current run the config is already loaded and would be unaffected by this.

what's your use case for this @Ratimon ?

I am sorry for not writing clear issue at first. What I encountered is that I can not access to other folders/directory even though I have set the permission access in foundry.toml

In my case, it is /deployments/ where I set to read-write as shown aove.

mattsse commented 6 months ago

ah okay, so this is actually a bug because you don't actually target the foundry.toml file?

could you please add the test as plaintext here so we can try to replicate the issue?

Ratimon commented 6 months ago

ah okay, so this is actually a bug because you don't actually target the foundry.toml file?

could you please add the test as plaintext here so we can try to replicate the issue?

Sure!!

Repro Steps

1)

git clone -b reproduce-bug git@github.com:Ratimon/optimism-contracts-foundry.git

2) install dependencies

pnpm i

3) run pnpm deploy_000_safe

I Note that I target [.deploy.json] (https://github.com/Ratimon/optimism-contracts-foundry/blob/main/deployments/31337/.deploy.json)

mattsse commented 6 months ago

@klkvr

looks like this check is just flawed:

https://github.com/foundry-rs/foundry/blob/3fa02706ca732c994715ba42d923605692062375/crates/cheatcodes/src/config.rs#L118-L120

mind looking into this @klkvr
I think we can convert this into a repro easily:

https://github.com/Ratimon/optimism-contracts-foundry/blob/64260ec561e29111dae93e2e2257cf728232ea40/script/deployer/Config.sol#L17-L19

klkvr commented 6 months ago

@Ratimon from your trace I see that vm.writeJson is getting called with empty "path" parameter, any chance DEPLOYMENT_OUTFILE might be an empty variable in your environment?

This check is getting hit because it's being performed via .starts_with and any path starts with an empty path

Ratimon commented 6 months ago

@Ratimon from your trace I see that vm.writeJson is getting called with empty "path" parameter, any chance DEPLOYMENT_OUTFILE might be an empty variable in your environment?

This check is getting hit because it's being performed via .starts_with and any path starts with an empty path

Yes, DEPLOYMENT_OUTFILE is empty in the .env.local and it should be fallback to this Config file as shown below:

Screenshot 2567-03-15 at 15 09 13
klkvr commented 6 months ago

So the issue here is that we treat DEPLOYMENT_OUTFILE= entry in .env file the same way as DEPLOYMENT_OUTFILE="", thus we don't actually fallback to the provided default. Not sure if this is expected, but I am pretty sure that there are usecases for env vars with empty strings

You can try removing DEPLOYMENT_OUTFILE line from .env.local at all and this should get fixed

Ratimon commented 6 months ago

So the issue here is that we treat DEPLOYMENT_OUTFILE= entry in .env file the same way as DEPLOYMENT_OUTFILE="", thus we don't actually fallback to the provided default. Not sure if this is expected, but I am pretty sure that there are usecases for env vars with empty strings

You can try removing DEPLOYMENT_OUTFILE line from .env.local at all and this should get fixed

Yes!! I tried removing and my issue is resolved!! Thanks!!

Personally, I think that env vars with empty strings are important for code readability in smart contract project.

zerosnacks commented 2 months ago

Converting this issue into an actionable ticket focused on differentiating between unset variables (FOO=) and empty string variables (FOO="")