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

Add EnvExists cheatcode #3732

Closed
vicnaum opened this issue Nov 21, 2022 · 15 comments
Closed

Add EnvExists cheatcode #3732

vicnaum opened this issue Nov 21, 2022 · 15 comments
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge D-easy Difficulty: easy good first issue Good for newcomers T-feature Type: feature

Comments

@vicnaum
Copy link

vicnaum commented Nov 21, 2022

Component

Forge

Describe the feature you would like

Proposing to add a cheatcode like envExists(string calldata key) returns (bool) that will return true if the ENV key exists, and false if it doesn't exist.

Reasoning:
Right now there is no way to check if a specific ENV variable was set, and the whole script just reverts. This doesn't allow to have optional ENV keys, and requires setting all ENVs explicitly, even if they're optional for some test cases.

Examples:

  • Having a FORK key to test on a fork VS testing on a local deployment. Currently the FORK= ENV variable should be present for the tests to run at all, even if the FORK is not used. Things get complicated if there are many such optional variables.
  • Having a MNEMONIC or PRIVATE_KEY variables - there is no sense to have both of them - can use either. But with current behavior they both should be set (one left as an empty string) for the script to not revert.

Additional context

No response

@vicnaum vicnaum added the T-feature Type: feature label Nov 21, 2022
@mattsse
Copy link
Member

mattsse commented Nov 21, 2022

I can see how having a envIsSet() code is useful.

a workaround for this is something like:

  try vm.envBool("MYVAR") returns (bool myVar) {
            if (myVar) {...};
        } catch {
           
        }

@mds1
Copy link
Collaborator

mds1 commented Nov 21, 2022

Partly related to foundry-rs/forge-std#226, where we discuss how vm.parseJson returns an empty string if the provided key isn't found, and we'll add a keyExists helper method to validate the result. One issue with doing the same here is then you can't support env vars that are actually empty.

I've used the try/catch workaround in the past, and try/catch was introduced in solidity 0.6.2, so we can implement that workaround in forge-std too. Perhaps something like tryEnvBool(string name, bool default) where the second arg is the default return value if not found

@mds1
Copy link
Collaborator

mds1 commented Nov 21, 2022

@mattsse if you think the forge-std solution is the preferable way to handle it / @vicnaum if you're ok with that solution lmk, and in that case can we transfer this issue to forge-std?

I'm unsure whether the forge-std solution or an envIsSet cheatcode is preferable, curious to hear thoughts here

@vicnaum
Copy link
Author

vicnaum commented Nov 22, 2022

@mattsse Oh, for some reason I remember try/catch not working for me before, but maybe I'm just confusing it with smth else (I definitely saw Try can only be used with external function calls and contract creation calls. when I was trying to parse non-existing JSON keys).

But in this case of ENVs - it works! So yes, as @mds1 says - we can go with forge-std solution with try/catch - that sounds perfectly fine (I just was mistakenly thinking that try/catch wouldn't work, but it does).

I just have several questions I want to discuss before I start:

  • If we want universal isEnvSet helper - am I correct that vm.envString is the most universal solution that wouldn't fail on any key value, and only fails when the key doesn't exist?
  • Should it be envIsSet or isEnvSet? I'm leaning towards isEnvSet but would like to hear your opinions.
  • Are you ok with me creating StdEnv.sol and putting all the helper functions (like tryEnvBool() etc) there?
  • What's the best way to create an environment fixture for testing? Should I do setEnv in setUp, or better init from a file? If file - then how? My forge test doesn't automatically load .env for some reason, only if I source it beforehand (and use export keyword).

@mattsse
Copy link
Member

mattsse commented Nov 22, 2022

isEnvSet

sgtm, let's add that

@rkrasiuk rkrasiuk added C-forge Command: forge A-cheatcodes Area: cheatcodes labels Nov 22, 2022
@alcueca
Copy link

alcueca commented Nov 26, 2022

May I suggest a different implementation?

Instead of isEnvSet (or maybe as an addition to it), the current env* functions should be overloaded to accept an extra parameter, and if the environment variable is not found, the extra parameter is returned.

Example:
bool mock = vm.envBool("MOCK", true) would assign the contents of $MOCK to mock, or true if $MOCK is not set.
string memory network = vm.envString("NETWORK", "mainnet") would assign the contents of $NETWORK to network, or "mainnet" if $NETWORK is not set.

The existing vm.env* methods should of course stay, in case users prefer the call to revert if the variable is not set.

Just a suggestion that would fit better my use case, feel free to ignore it. I'll be happy if this is implemented in any way.

@mattsse
Copy link
Member

mattsse commented Nov 26, 2022

this is pretty useful and could be added easily, don't think we'd run into issues with solidity here.

But I think isEnvSet is still useful on its own to check if a var is set, regardless of the type of value it holds.

@mattsse mattsse added good first issue Good for newcomers D-easy Difficulty: easy labels Nov 26, 2022
@vicnaum
Copy link
Author

vicnaum commented Nov 26, 2022

May I suggest a different implementation?

Instead of isEnvSet (or maybe as an addition to it), the current env* functions should be overloaded to accept an extra parameter, and if the environment variable is not found, the extra parameter is returned.

Example: bool mock = vm.envBool("MOCK", true) would assign the contents of $MOCK to mock, or true if $MOCK is not set. string memory network = vm.envString("NETWORK", "mainnet") would assign the contents of $NETWORK to network, or "mainnet" if $NETWORK is not set.

The existing vm.env* methods should of course stay, in case users prefer the call to revert if the variable is not set.

Just a suggestion that would fit better my use case, feel free to ignore it. I'll be happy if this is implemented in any way.

This sounds amazing! I like that.
Ok, will implement both.

@mds1
Copy link
Collaborator

mds1 commented Nov 28, 2022

+1 to @alcueca's suggestion, came to this issue to make the same comment in favor of the solution in foundry-rs/forge-std#237

Thanks for implementing @vicnaum!

@vicnaum
Copy link
Author

vicnaum commented Nov 28, 2022

+1 to @alcueca's suggestion, came to this issue to make the same comment in favor of the solution in foundry-rs/forge-std#237

Thanks for implementing @vicnaum!

Oh wait, I've meant implementing that in Solidity, cause I don't know any Rust (yet). Now I see those are all native vm.cheatcodes (not solidity helpers like the JSON case) - so I won't be able to overload them in Rust, sorry :-/

Anyways, when I looked at what's available, I have concerns about these array with delimiter functions, and especially envString one:

function envString(string calldata, string calldata) external returns (string[] memory);

It's (string,string) - so it will clash with overloaded (string key, string default) one, I guess?

@mds1
Copy link
Collaborator

mds1 commented Nov 30, 2022

It's (string,string) - so it will clash with overloaded (string key, string default) one, I guess?

Good point, what if we call all the new functions here vm.envWithDefault(string name, <defaultVal>) instead?

  • Avoids the clash pointed out above
  • Since the default vals are all different types, the methods can have the same name with the overloads to distinguish them

@slvDev
Copy link
Contributor

slvDev commented Dec 1, 2022

Good point, what if we call all the new functions here vm.envWithDefault(string name, <defaultVal>) instead?

@mds1 please take a look at my implementation - created a pull request here #3810

gakonst pushed a commit that referenced this issue Dec 1, 2022
* feat: envWithDefault cheatcode initial implementation #3732

* fmt: fix test formatting

* fix: rename envWithDefault to envOr
@vicnaum
Copy link
Author

vicnaum commented Dec 2, 2022

Should we also modify Vm.sol in forge-std repo to make it work?

@gakonst
Copy link
Member

gakonst commented Dec 2, 2022

Yep!

@mds1
Copy link
Collaborator

mds1 commented Mar 10, 2023

We have vm.envOr, and I'm not sure of a use case for vm.isEnvSet where you strictly care about if it's set and don't care about it's value, so going to close this. However if someone does have a need for isEnvSet feel free to create a new issue

@mds1 mds1 closed this as completed Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge D-easy Difficulty: easy good first issue Good for newcomers T-feature Type: feature
Projects
None yet
Development

No branches or pull requests

7 participants