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: implement boundInt #253

Merged
merged 11 commits into from
Dec 14, 2022
Merged

feat: implement boundInt #253

merged 11 commits into from
Dec 14, 2022

Conversation

JhChoy
Copy link
Contributor

@JhChoy JhChoy commented Dec 9, 2022

Implementing boundInt(int256,int256,int256) at StdUtils.sol to help fuzzing test with integer arguments.

One problem is that log(string,int256) does not exist so I used log(string,uint256) instead of it.

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks a lot! Pinging @PaulRBerg since I know he was interesting in this method.

Just a few nits for you 🙂

test/StdUtils.t.sol Outdated Show resolved Hide resolved
@@ -4,12 +4,12 @@ pragma solidity >=0.6.2 <0.9.0;
import "./console2.sol";

abstract contract StdUtils {
uint256 private constant INT256_MIN_ABS =
Copy link
Collaborator

Choose a reason for hiding this comment

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

(arbitrary comment location to start a thread)

One problem is that log(string,int256) does not exist so I used log(string,uint256) instead of it.

If you want to add support for this:

  1. Add the required method to console2.sol in forge-std
  2. Update the ABI file in foundry

I think that should be all that's needed, but I may be missing something. Also related is #129 if you wanted to add that overload at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll check this out.

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 will be the better one, log(string,int256) or logInt(string,int256)?
At the related thread of #129, they decided to name logInt(int256...) rather than log(int256...) because of the ambiguity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with log(string,int256) because in practice you'll be doing console.log(myVar) (unambiguous since myVar has a type) instead of console.log(3) (ambiguous because we don't know 3's type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied at here!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! Note that the logs won't actually be shown until we upstream the corresponding change to forge. I can do that soon though

src/StdUtils.sol Show resolved Hide resolved
src/StdUtils.sol Outdated Show resolved Hide resolved
src/StdUtils.sol Show resolved Hide resolved
src/StdUtils.sol Outdated Show resolved Hide resolved
src/StdUtils.sol Show resolved Hide resolved
@PaulRBerg
Copy link
Contributor

I don't currently have bandwidth to review but this is good stuff, thanks for implementing a signed bound, @JhChoy! I needed this for my PRBMath tests.

Copy link
Collaborator

@mds1 mds1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @JhChoy!

@ZeroEkkusu mind taking a look before we merge?

@mds1
Copy link
Collaborator

mds1 commented Dec 13, 2022

@JhChoy just a heads up I pushed two small commits to this branch and opened foundry-rs/foundry#3884

@JhChoy
Copy link
Contributor Author

JhChoy commented Dec 13, 2022

@mds1 Thanks for the review! 🙌

Copy link
Collaborator

@ZeroEkkusu ZeroEkkusu left a comment

Choose a reason for hiding this comment

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

Thanks.

@ZeroEkkusu
Copy link
Collaborator

Pushed changes for 1.2.0 release.

After foundry-rs/foundry#3884 is resolved, we will remove the workaround.

Note: console2_log functions are part of the upcoming "remove Components" PR, so I made those changes here to avoid conflicts.

src/StdUtils.sol Outdated Show resolved Hide resolved
@ZeroEkkusu
Copy link
Collaborator

Thanks, @JhChoy - good work.

@ZeroEkkusu ZeroEkkusu merged commit a5ecf5d into foundry-rs:master Dec 14, 2022
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.

4 participants