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

#51 breaks backwards compatibility before 0.8.0 #53

Closed
ernestognw opened this issue Apr 28, 2022 · 4 comments · Fixed by #54
Closed

#51 breaks backwards compatibility before 0.8.0 #53

ernestognw opened this issue Apr 28, 2022 · 4 comments · Fixed by #54

Comments

@ernestognw
Copy link
Contributor

Description

Recently #51 was merged and it included an unchecked in the stdMath.abs function, which requires to bump the Solidity version for the forge-std package to be pragma solidity >=0.8.0 <0.9.0; instead of pragma solidity >=0.6.0 <0.9.0;

I'm myself running into an issue here since I wanted to test a 0.6.0 contract and this was broken. You either will need to remove the unchecked or bump the version.

@ernestognw ernestognw changed the title #51 breaks backwards compatibility #51 breaks backwards compatibility before 0.8.0 Apr 28, 2022
@mds1
Copy link
Collaborator

mds1 commented Apr 28, 2022

Great catch, thanks! Seems we can just change the abs method to return a >= 0 ? a : -a; and be all set? I can get a PR open for that tomorrow unless you want to beat me to it.

@ZeroEkkusu We should probably update the CI to run against a few different solc versions, since this has happened before

@OliverNChalk
Copy link
Contributor

If you don't have unchecked, the following will fail:

int256 inverse = -(type(int256).min)

@ernestognw
Copy link
Contributor Author

If you don't have unchecked, the following will fail:

int256 inverse = -(type(int256).min)

This is correct.
Here's my 20 wei (#54)

@OliverNChalk
Copy link
Contributor

Assuming we don't want to raise the minimum solidity version, I'm leaning towards #54

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 a pull request may close this issue.

3 participants