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

feat: support environment variables in foundry.toml #1034

Closed
mds1 opened this issue Mar 24, 2022 · 32 comments
Closed

feat: support environment variables in foundry.toml #1034

mds1 opened this issue Mar 24, 2022 · 32 comments
Labels
C-forge Command: forge P-low Priority: low T-feature Type: feature

Comments

@mds1
Copy link
Collaborator

mds1 commented Mar 24, 2022

Component

Forge

Describe the feature you would like

A common use case is configuring test params with environment variables, with the RPC URL used for forking tests being a popular example. Right now there is now way to specify in the config file to use a certain environment variable for fork URLs, and instead you'd have to pass this in to the test command, e.g. forge test --fork-url $MY_RPC_URL

Additional context

No response

@mds1 mds1 added the T-feature Type: feature label Mar 24, 2022
@onbjerg
Copy link
Member

onbjerg commented Mar 24, 2022

It should be the case that all things in foundry.toml can be configured using FOUNDRY_*, e.g. FOUNDRY_ETH_RPC_URL in your case

Edit: Ah, you want to override the environment variable used?

@mds1
Copy link
Collaborator Author

mds1 commented Mar 24, 2022

Yea I want someone to be able to clone a repo, configure a .env file, and have forge test run with those environment variables which get loaded because they're used the config file. Currently you have to run forge test --fork-url $RPC_URL which obviously isn't a huge deal, but being able to do everything in the config file, instead of splitting between config file and command line, is better UX IMO

@rpedroni
Copy link

+1 not having to change tracked files makes for better DX

@onbjerg onbjerg added C-forge Command: forge P-low Priority: low labels Mar 27, 2022
@onbjerg
Copy link
Member

onbjerg commented Mar 27, 2022

Still unsure about this, I don't know of any tools that allow this other than DappTools because they used a full bash script for configuration, and I don't think there's a clean way to do this in TOML either. Any reason you don't just do the renaming in your environment script that we could autoload instead?

@mds1
Copy link
Collaborator Author

mds1 commented Mar 27, 2022

You could argue hardhat does allow for this: In hardhat.config.ts you can access env vars like normal with process.env.MY_ENV_VAR. So a common approach is something like this:

  • Commit a file called .env.example with the contents FORK_RPC_URL=mainnetRpcUrlHere.
  • Readme instructs users to run cp .env.example .env and add their RPC URL.
  • The hardhat config defines the hardhat network configuration to fork form process.env.FORK_RPC_URL.
  • Now, running the standard hardhat test command reads the env var in the config and runs tests against that fork.

Though to your point I don't think there's a way to do this natively in TOML files. I think the easiest way is to define a custom syntax like env:MY_ENV_VAR or $MY_ENV_VAR and manually replace that string with the corresponding env var at runtime.

Any reason you don't just do the renaming in your environment script that we could autoload instead?

Sorry, not sure I follow what environment script forge would autoload. Do you mean e.g. using a makefile to define test commands with env vars and running make test instead of forge test?

@onbjerg
Copy link
Member

onbjerg commented Mar 28, 2022

I mean if Forge auto-loaded .env for example

@mds1
Copy link
Collaborator Author

mds1 commented Mar 28, 2022

Ahh interesting! Let me make sure I'm understanding: Since every param in foundry.toml can also be set with FOUNDRY_PARAM_NAME, you're suggestion the way to use env vars would be define those params in .env instead of foundry.toml, then add a PR which auto-loads .env at the beginning of a forge command? And presumably the env file would always take precedence over the config?

I'd say that's an acceptable short term solution, with the reasons I don't love it being that:

  1. If an env var isn't set, tests will still run but likely fail (e.g. because they're not running against a forked network), which isn't very intuitive behavior. Whereas with the "env vars in config file" approach we can have an explicit ENV_VAR not set error
  2. It still fragments config so it's partially in the config file and partially in the env file
  3. You can't use different env vars for different profiles (admittedly I don't have a use case in mind for this, it just seems like a nice property)

@onbjerg
Copy link
Member

onbjerg commented Apr 7, 2022

Yes, that's what I mean.

I think the main reason I don't like the env vars in foundry.toml approach is that it's non-standard TOML and requires us to do a little hacking, which I'd rather avoid if possible.

@mds1
Copy link
Collaborator Author

mds1 commented Apr 7, 2022

Makes sense. Maybe a good solution to mitigate items 1 and 2 is adding a field to the config file called something like env-vars=['FOUNDRY_FORK_URL', 'FOUNDRY_FORK_BLOCK'], and foundry auto-loads those env vars specifically? (Those might not be the right names for this example, I don't see them in the foundry book config reference page and don't remember offhand). This way if an env var is missing, foundry knows and can show a warning or error.

Not sure whether it'd be better to specify the env var names specifically there, or the config file field names which foundry would convert to the env var format

@sambacha
Copy link
Contributor

sambacha commented Apr 7, 2022

Why not use something like Justfile? eg https://github.com/sambacha/foundry-scripts/blob/master/justfile

This loads env file, etc

@gakonst
Copy link
Member

gakonst commented May 17, 2022

Here's one idea:

  1. We autoload .env if present (does this still work on Windows?) to set env vars
  2. We allow the env("<VALUE>") syntax inside foundry.toml. Before running anything, we search + replace any instances of env("FOO") with the value of the environment value FOO.

Example:

[default]
src = 'src'
test = 'test'
eth-rpc-url = 'https://mainnet.infura.io/v3/env("INFURA_API")'

[rinkeby]
eth-rpc-url = 'https://rinkeby.infura.io/v3/env("INFURA_API")'

Edge case 1

Hardhat will error if no env var is present (because process.env.INFURA_API will return undefined and it'll try to read an undefined value), so there's a world where we do that. There's also a world where if a parameter needs an env var but that env var is not present, then we simply ignore that parameter and use the default value from the config (e.g. if INFURA_API isn't set above, we just consider eth-rpc-url as unset, but if it's set, forge test will assume that it's called as forge test --rpc-url)

@mds1
Copy link
Collaborator Author

mds1 commented May 18, 2022

Personally I like that approach, though I know above @onbjerg mentioned he didn't love the idea of non-standard TOML, but I do think it provides the best and simplest UX.

If you specify an env var in your config but it's not present, we should probably error. I suspect most cases (such as fork URLs and API keys) would end up erroring anyway, or resulting in unexpected behavior, if you tried to fall back to some default value.

@gakonst
Copy link
Member

gakonst commented May 18, 2022

@onbjerg curious for any strong opinions re: my comment above?

@onbjerg
Copy link
Member

onbjerg commented May 19, 2022

I think if we want inline env vars I'd rather go with something standard like ${ENV}. Additionally, there is a question around how this would work for non-string config since the TOML library we use parse stuff strongly typed, so it probably wouldn't work without a custom deserializer

@gakonst
Copy link
Member

gakonst commented May 19, 2022

I'm 👍 with calling it ${ENV}

@onbjerg
Copy link
Member

onbjerg commented Jun 9, 2022

This comment might be relevant: #1457 (comment)

@StErMi
Copy link

StErMi commented Jun 10, 2022

Here's one idea:

  1. We autoload .env if present (does this still work on Windows?) to set env vars
  2. We allow the env("<VALUE>") syntax inside foundry.toml. Before running anything, we search + replace any instances of env("FOO") with the value of the environment value FOO.

Example:

[default]
src = 'src'
test = 'test'
eth-rpc-url = 'https://mainnet.infura.io/v3/env("INFURA_API")'

[rinkeby]
eth-rpc-url = 'https://rinkeby.infura.io/v3/env("INFURA_API")'

Quoting from my #1457 (comment) comment an enhancement to your suggestion (that's the same thing I wanted to implement on the other thread) would be to also allow the developer to have different .env files.

.env is the default one, and it acts like the default profile for foundry.
You can have multiple .env file, each one for a profile you have defined on the foundry.toml config file.

If you specify a profile when running a foundry script, foundry will load the .env file and the corresponding .env.profilename file, merge them (the more specific one will override when possible the generic one) and at that point you will apply those env variables to the script.

Would this make sense?

Side question: how can I specify the profile when launching forge test? There's no way to specify it via --profile but only via an ENV var, so I would need to have the script like this to make it work?

{
  "scripts": {
    "test": "FOUNDRY_PROFILE=rinkeby forge test"
  }
}

@gakonst
Copy link
Member

gakonst commented Jun 11, 2022

I think I prefer the approach mentioned above with ${ENV}. Don't like forcing dotenv files to the user.

Side question: how can I specify the profile when launching forge test? There's no way to specify it via --profile but only via an ENV var, so I would need to have the script like this to make it work?

Yep.

@StErMi
Copy link

StErMi commented Jun 12, 2022

I think I prefer the approach mentioned above with ${ENV}. Don't like forcing dotenv files to the user.

Side question: how can I specify the profile when launching forge test? There's no way to specify it via --profile but only via an ENV var, so I would need to have the script like this to make it work?

Yep.

Maybe I didn't understand it, how can I pass locally those env vars without the support of a .env standard?

@onbjerg
Copy link
Member

onbjerg commented Jun 13, 2022

It depends on your shell, but in bash you would prepend the variables like so:

FOO=bar ./my-program

Alternatively you can source your .env before running commands:

source .env
./my-program

@StErMi
Copy link

StErMi commented Jun 13, 2022

It depends on your shell, but in bash you would prepend the variables like so:

FOO=bar ./my-program

Alternatively you can source your .env before running commands:

source .env
./my-program

Ok, so you can't make them to have the same npm script to act the same on both CI and local dev env.

@onbjerg
Copy link
Member

onbjerg commented Jun 13, 2022

I'm not entirely sure what you mean - it is definitely possible to run source .env in a CI environment before running the test as well

@mds1
Copy link
Collaborator Author

mds1 commented Jul 21, 2022

@mattsse Did #2334 add general support for this, or only for certain config keys?

@onbjerg
Copy link
Member

onbjerg commented Jul 21, 2022

Only for RPC endpoints in the [rpc_endpoints] section

@gakonst
Copy link
Member

gakonst commented Jul 25, 2022

Where else do you want it @mds1 ?

@mds1
Copy link
Collaborator Author

mds1 commented Jul 30, 2022

Mainly anywhere you might have API keys, which would also include eth_rpc_url and etherscan_api_key. But more generally, it feels inconsistent to only support this in one section or only for certain keys—why not support env var interpolation for all config keys?

Related to this—and maybe this is a foundry book issue—I'm unclear on the recommend way to run a portion of tests as fork tests within your test suite. Right now we use source .env && forge test along with vm.createFork(vm.envString("OPTIMISM_RPC_URL")) (and the related fork cheatcodes). It seems an alternative is to still use source .env && forge test but instead use the [rpc_endpoints] section to define an optimism RPC and then use vm.createFork(vm. rpcUrl("optimism"))? Is there a tradeoff here or are these two identical?

@onbjerg
Copy link
Member

onbjerg commented Jul 30, 2022

Tradeoff is that it's easier to figure out what environment variables to set if you use [rpc_endpoints] since it's all in the config vs having to read through all of the tests to find vm.envString calls :) Other than that they are equivalent

@gakonst
Copy link
Member

gakonst commented Jul 30, 2022

Related to this—and maybe this is a foundry book issue—I'm unclear on the recommend way to run a portion of tests as fork tests within your test suite. Right now we use source .env && forge test along with vm.createFork(vm.envString("OPTIMISM_RPC_URL")) (and the related fork cheatcodes). It seems an alternative is to still use source .env && forge test but instead use the [rpc_endpoints] section to define an optimism RPC and then use vm.createFork(vm. rpcUrl("optimism"))? Is there a tradeoff here or are these two identical?

They're basically identical, and I prefer the latter to the former because you put all your env vars in the foundry toml and then refer only by aliases in the code.

We could also allow etherscan_api_key = ${NAME_OF_YOUR_ENV_VAR} although that's already read by default via the ETHERSCAN_API_KEY env var (or CLI option), so not sure how helpful that is?

@mds1
Copy link
Collaborator Author

mds1 commented Aug 3, 2022

Heh thought that might be the answer. IMO best practice is to have a committed .env.template type of file committed to a repo so devs know which env vars they need, which means you don't really need the [rpc_endpoints] section to find all env vars. Additionally, there might be env vars part of e.g. helper scripts in the repo, so you can't guarantee all env vars in the toml are the only ones you need.

But the vm.rpcUrl is already implemented so fine to keep and is a bit more readable, so mainly just personal preference.

We could also allow etherscan_api_key = ${NAME_OF_YOUR_ENV_VAR} although that's already read by default via the ETHERSCAN_API_KEY env var (or CLI option), so not sure how helpful that is?

Etherscan API keys differ by network, so a single ETHERSCAN_API_KEY env var isn't sufficient for multichain cases. I'm not sure of what flow people use for multi-chain verification of contracts / if toml env var interpolation would be helpful there, though. Similar comment/question for eth_rpc_url.

Perhaps we want an [etherscan_api_keys] section to map chain IDs/network names to API keys, and that way foundry can automatically use the right one based on the specified chain? And for that section we'd probably want env var interpolation, unless we choose to hardcode env var names that foundry looks for and remove the need for that section (e.g. hardcode MAINNET_ETHERSCAN_API_KEY as what foundry looks for on mainnet, OPTIMISM_ETHERSCAN_API_KEY for optimism, etc. (need to confirm which networks share API keys))

@gakonst
Copy link
Member

gakonst commented Aug 3, 2022

Perhaps we want an [etherscan_api_keys] section to map chain IDs/network names to API keys, and that way foundry can automatically use the right one based on the specified chain?

Yep I like that. @mattsse wdyt? also relevant in the context of multichain deployments' verifications (cc @joshieDo)

@mattsse
Copy link
Member

mattsse commented Aug 4, 2022

that would be useful indeed, basically the same thing as the rpc_endpoint table.

tracking this separately.

@mattsse
Copy link
Member

mattsse commented Aug 25, 2022

closing this, as .env file is now loaded and we support env vars in [etherscan] and [rpc_endpoints]

if we need to support env vars anywhere else, let's track that individually

@mattsse mattsse closed this as completed Aug 25, 2022
xyek added a commit to ekonomia-tech/protocol-alpha that referenced this issue Sep 9, 2022
To run tests, we have to do `forge test --fork-url <rpc url>`. That
works but is not super convenient. Foundry recently added support for
auto-sourcing .env. See: foundry-rs/foundry#1034
xyek added a commit to ekonomia-tech/protocol-alpha that referenced this issue Sep 9, 2022
To run tests, we have to do `forge test --fork-url <rpc url>`. That
works but is not super convenient. Recently, Foundry added support for
auto-sourcing .env. See: foundry-rs/foundry#1034
xyek added a commit to ekonomia-tech/protocol-alpha that referenced this issue Sep 12, 2022
To run tests, we have to do `forge test --fork-url <rpc url>`. That
works but is not super convenient. Recently, Foundry added support for
auto-sourcing .env. See: foundry-rs/foundry#1034
xyek added a commit to ekonomia-tech/protocol-alpha that referenced this issue Sep 13, 2022
* Setup `BaseSetup.t.sol` to create fork from `RPC_URL`

To run tests, we have to do `forge test --fork-url <rpc url>`. That
works but is not super convenient. Recently, Foundry added support for
auto-sourcing .env. See: foundry-rs/foundry#1034

* Update README

Updates the cli command and uses correct env var name.

* Add workflow to see if `forge fmt` was run

* Run `forge fmt`

* Update env variable

* Remove boolean eq checks

* Add .vscode to gitignore file

Personal config files should not be committed. It doesn't seem right
for me to commit my personal vscode extension settings which don't
make sense for everyone. .vscode should ideally be in global
gitignore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge P-low Priority: low T-feature Type: feature
Projects
No open projects
Archived in project
Development

No branches or pull requests

7 participants