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

Thoughts on creating a "StdConstants" contract? #267

Open
PaulRBerg opened this issue Jan 5, 2023 · 8 comments
Open

Thoughts on creating a "StdConstants" contract? #267

PaulRBerg opened this issue Jan 5, 2023 · 8 comments

Comments

@PaulRBerg
Copy link
Contributor

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?

@mds1
Copy link
Collaborator

mds1 commented Jan 5, 2023

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

@PaulRBerg
Copy link
Contributor Author

PaulRBerg commented Jan 5, 2023

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.

@Sabnock01
Copy link
Contributor

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.

@CodeSandwich
Copy link

CodeSandwich commented Nov 3, 2024

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.

@mds1
Copy link
Collaborator

mds1 commented Nov 8, 2024

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

@klkvr
Copy link
Member

klkvr commented Nov 8, 2024

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

@mds1
Copy link
Collaborator

mds1 commented Nov 8, 2024

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

@CodeSandwich
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants