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.

Sign up for GitHub

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

Jump to bottom

Update chai matchers #4899

Merged
merged 8 commits into from
Feb 19, 2024
Merged

Update chai matchers #4899

merged 8 commits into from
Feb 19, 2024

Conversation

RenanSouza2
Copy link
Contributor

Hey this PR updates the hardhat-chai-matchers library form version 2.0.3 to 2.0,5,

Rationale: version 2.0.5 also includes the ethers.Typed in the address verification, needed to simplify vesting wallet tests

version 2.0.4 changed the name of an internal variable from ARITHMETIC_UNDER_OR_OVERFLOW to ARITHMETIC_OVERFLOW

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 14, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: c4c5046

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@socket-security Socket Security
Copy link

socket-security bot commented Feb 14, 2024 •
edited
Loading

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@nomicfoundation/hardhat-chai-matchers@2.0.6 Transitive: environment, eval, filesystem, network, shell, unsafe +305 106 MB alcuadrado, fvictorio, nomic-foundation-publisher, ...1 more

🚮 Removed packages: npm/@nomicfoundation/hardhat-chai-matchers@2.0.3

View full report↗︎

ernestognw
ernestognw reviewed Feb 15, 2024
View reviewed changes
test/utils/Panic.test.js
@@ -15,7 +15,7 @@ describe('Panic', function () {
for (const [name, code] of Object.entries({
GENERIC: 0x0,
ASSERT: PANIC_CODES.ASSERTION_ERROR,
UNDER_OVERFLOW: PANIC_CODES.ARITHMETIC_UNDER_OR_OVERFLOW,
UNDER_OVERFLOW: PANIC_CODES.ARITHMETIC_OVERFLOW,
Copy link
Member

Choose a reason for hiding this comment

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

We might want to update UNDER_OVERFLOW in Panic.sol as well.

@ernestognw ernestognw requested a review from Amxx February 15, 2024 23:26
ernestognw
ernestognw approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Needs approval from @Amxx

.changeset/pretty-steaks-push.md Outdated Show resolved Hide resolved
contracts/utils/Panic.sol Outdated Show resolved Hide resolved
ernestognw
ernestognw requested changes Feb 15, 2024
View reviewed changes
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

Just realized we need to remove the changeset. Sorry 😅

ernestognw
ernestognw reviewed Feb 16, 2024
View reviewed changes
contracts/utils/Panic.sol
Comment on lines -50 to +52
mstore(0x00, shl(0xe0, 0x4e487b71))
mstore(0x04, code)
revert(0x00, 0x24)
mstore(0x00, 0x4e487b71)
mstore(0x20, code)
revert(0x1c, 0x24)
Copy link
Member

Choose a reason for hiding this comment

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

Cool

ernestognw
ernestognw previously approved these changes Feb 16, 2024
View reviewed changes
Copy link
Member

@ernestognw ernestognw 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 @RenanSouza2

Let's wait on @Amxx

@Amxx
Copy link
Collaborator

Amxx commented Feb 16, 2024

version 2.0.4 changed the name of an internal variable from ARITHMETIC_UNDER_OR_OVERFLOW to ARITHMETIC_OVERFLOW

💯

Amxx
Amxx reviewed Feb 16, 2024
View reviewed changes
contracts/utils/Panic.sol Outdated
/// @dev arithmetic underflow or overflow
uint256 internal constant UNDER_OVERFLOW = 0x11;
/// @dev arithmetic overflow
uint256 internal constant OVERFLOW = 0x11;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that overflow is a better name, and hardhat was right to change it, but this file is supposed to follow https://github.com/ethereum/solidity/blob/v0.8.24/libsolutil/ErrorCodes.h

@Amxx
Copy link
Collaborator

Amxx commented Feb 16, 2024

Hey this PR updates the hardhat-chai-matchers library form version 2.0.3 to 2.0,5,

This PR jumps to 2.0.6, not 2.0.5.

Rationale: version 2.0.5 also includes the ethers.Typed in the address verification, needed to simplify vesting wallet tests

I'm not seeing any simplification of the vesting wallet tests in this PR. Can you showcase how what the improvements are?

@RenanSouza2
Copy link
Contributor Author

RenanSouza2 commented Feb 17, 2024 •
edited
Loading

This PR jumps to 2.0.6, not 2.0.5.

yes, they released 2.0.6 in the meanwile, I should have updated the comment here

I'm not seeing any simplification of the vesting wallet tests in this PR. Can you showcase how what the improvements are?

I didn't want to put the simplifications in this PR to no overblow it but I made the changes here:

https://github.com/RenanSouza2/openzeppelin-contracts/tree/possible-finance-pr/test/finance

it removes the parts I was least sure about when migrating the finance tests

@ernestognw
Copy link
Member

If I'm getting it correctly, the update improves support for Typed arguments when using .withArgs, so we can remove the argsVerify argument of the shouldBehaveLikeVesting behavior.

The changes don't look too big. I'm fine either merging this PR and then making the test simplification or including it in this same PR. Consider I'll need to re-review if it's the later.

@RenanSouza2
Copy link
Contributor Author

If I'm getting it correctly, the update improves support for Typed arguments when using .withArgs, so we can remove the argsVerify argument of the shouldBehaveLikeVesting behavior.

The changes don't look too big. I'm fine either merging this PR and then making the test simplification or including it in this same PR. Consider I'll need to re-review if it's the later.

Considering the assembly change in this PR I suggest using 2 PRs

@ernestognw
Copy link
Member

Right, almost forgot! Then I'd be in favor of merging this. Still requires approval from @Amxx

@Amxx Amxx merged commit 141c947 into OpenZeppelin:master Feb 19, 2024
20 checks passed
@RenanSouza2 RenanSouza2 deleted the update-chai-matchers branch February 19, 2024 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Amxx Amxx Amxx approved these changes

@ernestognw ernestognw ernestognw left review comments

Assignees
No one assigned
Labels
ignore-changeset
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RenanSouza2 @Amxx @ernestognw