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 new maxes #538

Closed
wants to merge 1 commit into from

Conversation

PaulRBerg
Copy link
Contributor

@@ -18,10 +18,12 @@ abstract contract StdUtils {
address private constant CONSOLE2_ADDRESS = 0x000000000000000000636F6e736F6c652e6c6f67;
uint256 private constant INT256_MIN_ABS =
57896044618658097711785492504343953926634992332820282019728792003956564819968;
uint40 internal constant MAX_UNIX_TIMESTAMP = 2_147_483_647; // 2^31 - 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm why is this one relevant for solidity devs? I'd think UINT32_MAX would be more appropriate, since I don't think may devs are storing timestamps as signed ints (this value equals type(int32).max)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I said in the OP - bounding fuzzed values so that they correspond to real-world values. Unix timestamps cannot go higher than 2^31-1

This isn't about signer or unsigned ints.

@mds1
Copy link
Collaborator

mds1 commented Apr 24, 2024

Sorry for the slow response here. Thinking about this more, I'd prefer to close this PR. The existing constants are there because they're needed by StdUtils functions, and these are not used anywhere in the StdUtils methods.

Base.sol is where constants are defined for user consumption, and even then I'd prefer to keep that minimal, and only add values that are not exposed in modern solidity (e.g. as you know type([u]intN).max is supported, making the constants not needed for most users anymore).

Going to close optimistically as part of the triage I'm doing now, but of course still open to further discussion even if the PR is closed :)

@mds1 mds1 closed this Apr 24, 2024
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.

2 participants