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 more uint assertions #168

Merged
merged 5 commits into from
Sep 3, 2022

Conversation

odyslam
Copy link
Contributor

@odyslam odyslam commented Sep 1, 2022

Comment on lines 251 to 254
assertUint32Eq(uint32(1), uint32(1));
assertUint64Eq(uint64(1), uint64(1));
assertUint96Eq(uint96(1), uint96(1));
assertUint128Eq(uint128(1), uint128(1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to comment on changing all the names to assertEq, then realized solc errors due to ambiguity. So then I deleted the 4 new assertions and changed these 4 tests to just use assertEq, and as expected solc upcasts them to uint256 using the existing assertions. So now I'm struggling to remember why we need these 😅

To clarify, if you remove the changes in Test.sol, all of these pass:

function testAssertions() public {
    assertEq(uint32(1), uint32(1));
    assertEq(uint64(1), uint64(1));
    assertEq(uint96(1), uint96(1));
    assertEq(uint128(1), uint128(1));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While coding in 0.7.6, I have to explicitly cast the uint32 to uint256 to use assert.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh got it. Interesting, so these are for < 0.8 and if you're on >= 0.8 they get upcasted automatically so you don't need them? If so sgtm, let's just add a comment explaining that so if we ever drop compatibility for <0.8 (#125) we remember to delete these overloads. cc @ZeroEkkusu for any other thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know that they are automatically upcasted in >0.8 . It's been so long in 0.7.0 land

Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh sorry to hear that 😛

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 LGTM, @ZeroEkkusu would love a quick review to make sure there's nothing that affect v0.3 branch here / anything else blocking merge

src/Test.sol Outdated
Comment on lines 333 to 348
function assertUint128Eq(uint128 a, uint128 b) internal {
assertEq(uint256(a), uint256(b));
}

function assertUint64Eq(uint64 a, uint64 b) internal {
assertEq(uint256(a), uint256(b));
}

function assertUint96Eq(uint96 a, uint96 b) internal {
assertEq(uint256(a), uint256(b));
}

function assertUint32Eq(uint32 a, uint32 b) internal {
assertEq(uint256(a), uint256(b));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function assertUint128Eq(uint128 a, uint128 b) internal {
assertEq(uint256(a), uint256(b));
}
function assertUint64Eq(uint64 a, uint64 b) internal {
assertEq(uint256(a), uint256(b));
}
function assertUint96Eq(uint96 a, uint96 b) internal {
assertEq(uint256(a), uint256(b));
}
function assertUint32Eq(uint32 a, uint32 b) internal {
assertEq(uint256(a), uint256(b));
}
function assertEqUint(uint256 a, uint256 b) internal {
assertEq(a, b);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you suggest @ZeroEkkusu

Copy link
Collaborator

@ZeroEkkusu ZeroEkkusu Sep 2, 2022

Choose a reason for hiding this comment

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

Sorry, @odyslam, accidentally clicked Add single comment. I'll write up a full review real quick.

Comment on lines 250 to 256
function testAssertions() public {
assertUint32Eq(uint32(1), uint32(1));
assertUint64Eq(uint64(1), uint64(1));
assertUint96Eq(uint96(1), uint96(1));
assertUint128Eq(uint128(1), uint128(1));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
function testAssertions() public {
assertUint32Eq(uint32(1), uint32(1));
assertUint64Eq(uint64(1), uint64(1));
assertUint96Eq(uint96(1), uint96(1));
assertUint128Eq(uint128(1), uint128(1));
}

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.

Solidity versions <=0.8.0 have a problem differentiating between uint and int overloaded functions when an argument is of type uintX, where X is less than 256. This does not apply to int arguments.

We are running into an issue because there are both assertEq(uint,uint) and assertEq(int,int), and not because the types do not get upscaled automatically.

I suggest we add a single function, assertEqUint(uint256,uint256), which will work for any uint type.

Let me know what you think.

@odyslam
Copy link
Contributor Author

odyslam commented Sep 3, 2022

sounds good

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.

LGTM. Thank you!

It is worth noting that we can also mix the types; e.g. this works:

assertEqUint(uint8(5), uint128(5));

One nit:

Tests should be in StdAssertions.t.sol (right now they're in StdCheats.t.sol).

If you want, I'll move the tests.

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.

support more uint types in assert
3 participants