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

Improper Ordering of Contract Verification API Keys (Env vars vs foundry.toml) #7699

Closed
2 tasks done
hexonaut opened this issue Apr 18, 2024 · 8 comments
Closed
2 tasks done
Labels
T-bug Type: bug

Comments

@hexonaut
Copy link
Contributor

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 (440ec52 2024-04-11T00:22:09.585245000Z)

What command(s) is the bug in?

forge script --verify

Operating System

macOS (Apple Silicon)

Describe the bug

I have this defined in my foundry.toml:

[etherscan]
mainnet = { key = "${ETHERSCAN_API_KEY}" }
optimism = { key = "${OPTIMISMSCAN_API_KEY}" }
base = { key = "${BASESCAN_API_KEY}" }
gnosis_chain = { key = "${GNOSISSCAN_API_KEY}", url = "https://api.gnosisscan.io/api" }

However, if ETHERSCAN_API_KEY is defined in the environment then it will override all the other chains and produce "invalid API key" when verifying contracts. The ordering on this should be reversed where it will look for this explicit definition for each chain inside foundry.toml and only fall back to ETHERSCAN_API_KEY if they do not exist.

@hexonaut hexonaut added the T-bug Type: bug label Apr 18, 2024
@grandizzy
Copy link
Collaborator

I think this could be workarounded by using a MAINNET_ETHERSCAN_API_KEY in env and in foundry.toml

mainnet = { key = "${MAINNET_ETHERSCAN_API_KEY}" }

@hexonaut
Copy link
Contributor Author

hexonaut commented Apr 18, 2024

Sure, but I have ETHERSCAN_API_KEY set in my environment all the time for easy use of cli. It's counter-intuitive to have this be silently overriding the config file.

@sambacha
Copy link
Contributor

sambacha commented May 2, 2024

Setting an ENV value always does this this is standard

@hexonaut
Copy link
Contributor Author

hexonaut commented May 3, 2024

Setting an ENV value always does this this is standard

Ok, but it doesn't make any sense intuitively. Why should a global API key value override network-specific configured ones?

@sambacha
Copy link
Contributor

Setting an ENV value always does this this is standard

Ok, but it doesn't make any sense intuitively. Why should a global API key value override network-specific configured ones?

ENV is supposed to take precedence over CLI. ENV is not stored on the heap, nor is there a global variable in the data section pointing to it. The environment is stored entirely on the stack and is a part of the initial process stack that is set up before the program starts running. Ergo, it takes precedence.

@hexonaut
Copy link
Contributor Author

After thinking on this some more the issue is not so much about environment preference ordering. It's more that specifying a global etherscan api key doesn't make sense when doing a multichain deploy. Really what should happen is this parameter is either ignored or an error is thrown.

@sambacha
Copy link
Contributor

sambacha commented Jun 2, 2024

After thinking on this some more the issue is not so much about environment preference ordering. It's more that specifying a global etherscan api key doesn't make sense when doing a multichain deploy. Really what should happen is this parameter is either ignored or an error is thrown.

I argued against even having this as a setting for foundry.toml, it breaks across chains just like you stated.

The latest release has support for defining env: CHAIN= that will respect the etherscan verification api.

What we have here is an overload of concerns. It is best to load such settings on a per-deploy basis.

env vars are granular controls, each fully orthogonal to other env vars. grouping them by environments does not scale, and the "grouping" of such settings will result in a combinatorial explosion of config nested settings, which makes managing deploys very brittle.

Personally, if I am doing a production main net deployment, I create a clean env with a modified config that is specific to that deployment. It contains no test files or other unneeded files for the purposes of deployment

@hexonaut
Copy link
Contributor Author

hexonaut commented Jun 6, 2024

Ok can close this if need to work around with a different environment configuration.

@zerosnacks zerosnacks closed this as not planned Won't fix, can't repro, duplicate, stale Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-bug Type: bug
Projects
None yet
Development

No branches or pull requests

4 participants