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: Add StdConstants library #654

Merged
merged 4 commits into from
Feb 28, 2025
Merged

Conversation

CodeSandwich
Copy link
Contributor

@CodeSandwich CodeSandwich commented Feb 4, 2025

Fixes #267. Gathers up all constants used throughout forge-std in a single library so they are easier to access.

The refactoring around the constants is minimal, it's mostly replacements of arbitrary values with references to the new library. No changes are visible from the outside, the 3 constants removed from StdUtil were private. Many more refactorings, like dropping other private constants that are used more than once or are variations about type(uintXXX).max, may be done in separate PRs.

@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Feb 4, 2025

Well, it doesn't compile with 0.6.2 as it doesn't allow assigning constants to constants, which defeats one of two purposes of this PR. A library with the constants is still useful for the forge-std users, but there's no way to deduplicate the constants inside forge-std without introducing breaking changes.

What should we do now?

  1. Only introduce the library, but don't use it internally.
  2. Drop support for Solidity 0.6.2 (It's a 5 years old compiler which has a smooth path of upgrade to the 4.5 years old 0.6.12. Does anybody use the 0.6 family for active development with modern tools in 2025?).
  3. Close this PR.

Edit: It's much worse, it breaks on anything below 0.8.16 which explicitly introduced assignment of library constants to other constants. I guess that the only way forward is to introduce the library and not use it internally until we drastically increase the minimum compiler version.

@zerosnacks
Copy link
Member

Hi @CodeSandwich, thanks for your PR

Why is inheriting the CommonBase contract where you want access to the constants an issue?

Given the described limitations I would prefer not duplicating these constants

Appreciate the clarifying fix on Base.sol, makes sense

@CodeSandwich
Copy link
Contributor Author

CodeSandwich commented Feb 5, 2025

Why is inheriting the CommonBase contract where you want access to the constants an issue?

It's discussed in #267. Inheritance is a barrier when accessing these constants, it excludes usage in libraries and free functions, which are excellent ways to create helpers and utilities for tests and scripts.

Free constants are another option, but they tend to pollute namespaces and they don't work with Solidity older than 0.7.4.

Expressing constants as free functions could work, but they would be clunky they wouldn't work with Solidity older than 0.7.1.

@zerosnacks zerosnacks requested a review from mds1 February 5, 2025 10:40
@CodeSandwich
Copy link
Contributor Author

So what's the verdict, shall we move forward or shall we close this PR? @zerosnacks @mds1

@zerosnacks
Copy link
Member

I can see the use of this, it being implemented as stated in #267 (comment) and the overhead of maintaining duplicate constant definitions in Base and Constants seems limited and OK.

I would prefer renaming Constants to StdConstants with this reasoning: #267 (comment)

cc @mds1 / @klkvr

@CodeSandwich
Copy link
Contributor Author

Thanks, that's great news. I pushed the renaming to StdConstants.

@CodeSandwich CodeSandwich changed the title feat: Add Constants library feat: Add StdConstants library Feb 26, 2025
@zerosnacks zerosnacks dismissed a stale review February 27, 2025 14:13
@zerosnacks
Copy link
Member

zerosnacks commented Feb 27, 2025

Regarding the failing CI, this should be fixed by pulling from master, had some troubles with Alchemy's API key

@zerosnacks zerosnacks self-requested a review February 27, 2025 15:37
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! All LGTM

@yash-atreya
Copy link
Member

#654 (comment)

Duplicating seems reasonable.

Copy link
Member

@yash-atreya yash-atreya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@zerosnacks zerosnacks merged commit 87989d2 into foundry-rs:master Feb 28, 2025
3 checks passed
@CodeSandwich CodeSandwich deleted the constants branch February 28, 2025 15:46
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

Successfully merging this pull request may close these issues.

Thoughts on creating a "StdConstants" contract?
3 participants