forge-std icon indicating copy to clipboard operation
forge-std copied to clipboard

Thoughts on creating a "StdConstants" contract?

Open PaulRBerg opened this issue 3 years ago • 3 comments

Would you be open to a PR that separated the constants defined in CommonBase in a separate contract StdConstants, which CommonBase would then inherit from?

PaulRBerg avatar Jan 05 '23 15:01 PaulRBerg

Hmm, what's the rationale for it? I would lean towards keeping constants in CommonBase to avoid over-abstracting things.

Currently all Std* contracts are independent of CommonBase, so this suggestion would also break that convention

mds1 avatar Jan 05 '23 17:01 mds1

Well, simply being able to inherit from StdConstants and make use of the constants defined in Foundry without also getting the vm and the stdstore variables. I can't do this currently because I'm using PRBTest.

Good point re convention, I didn't consider that.

I guess, I don't have a strong opinion here. Copy-pasting the constants that I need is not a big chore, not at all, so I'm happy to close this as not planned if you can't think of any easy solution that would at the same not break the contract independence convention and would make it possible to re-use the constants in Test contract equivalents.

PaulRBerg avatar Jan 05 '23 20:01 PaulRBerg

There are a few constants from CommonBase that wound up being implemented in Std* files like UINT256_MAX in StdUtils.sol. Some minor duplication to think about.

Sabnock01 avatar Jul 03 '23 23:07 Sabnock01

I want to bump this issue, but I think that StdConstants should be just a Solidity file with a bunch of free constants, not another contract.

This will simplify composition because the user won't need to complicate their inheritance tree just to access a few constants. It will enable massive constants deduplication in forge-std itself without introducing artificial dependencies between components. E.g. address(uint160(uint256(keccak256("hevm cheat code")))) is currently repeated 10 times across the source code. It will also enable usage of the constants in libraries and free functions e.g. to use VM cheatcodes. This is what actually inspired me to write this comment, as of now I need to use one of the specialized forge-std libraries to get the single VM address if I want to avoid hardcoding my own copy.

CodeSandwich avatar Nov 03 '24 12:11 CodeSandwich

I do like the idea of having them as free constants. The main blocker there is forge-std currently supports as early as solidity 0.6.2, but I believe free functions (and presumably free constants) were not introduced until 0.7.x. I think we could add them as it's own file with it's own pragma, and because nothing in forge-std inherits or imports it, it shouldn't break the codebase of anyone on older versions, though I would want to confirm to be sure solidity doesn't end up compiling it for some reason

cc @klkvr for other thoughts or considerations

mds1 avatar Nov 08 '24 02:11 mds1

though I would want to confirm to be sure solidity doesn't end up compiling it for some reason

if it would be just a standalone file not imported by any of forge-std components it should be OK

another approach could be to add library Constants which should also be easy to import and would be supported by all solc versions. that way we can also deduplicate constants in forge-std code as well

klkvr avatar Nov 08 '24 02:11 klkvr

That's true, I am equally supportive of a library, which does have the nice side effect of free namespacing of the constants. My nit there would be to name it ForgeConstants since Constants is already a common lib name in projects

mds1 avatar Nov 08 '24 02:11 mds1

I agree that a library should be as flexible and non-intrusive as free constants. It will be usable in all kinds of functions, including library functions and free functions, and should be even usable in initialization of all kinds of constants.

I especially like that it will contain the namespace avoiding pollution. Writing something like import {MY_CONSTANT_1, MY_CONSTANT_2, <...>} from "forge-std/Constants.sol" would unavoidably reexport all the constants that happen to be used in the file causing spread of pollution and leakage of the implementation details. Packing the constants into a library will still pollute the namespace, but only with a single name of a single library, regardless of how many constants are actually needed.

CodeSandwich avatar Nov 08 '24 15:11 CodeSandwich