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

Add revert matchers #2652

Merged
merged 23 commits into from
May 10, 2022
Merged

Conversation

fvictorio
Copy link
Member

@fvictorio fvictorio commented Apr 28, 2022

revertedWithPanic

The .revertedWithPanic matcher can be used in two ways:

expect(c.f()).to.be.revertedWithPanic() // reverts with *some* panic
expect(c.f()).to.be.revertedWithPanic(PANIC_CODES.ASSERTION_ERROR)

This is different than revertedWith, which always expects a reason string. I did it that way because adding a reason string to an assertion is quite straightforward, and very useful. On the other hand, adding the proper panic code has more friction, and it seems to me that some users might just want to check that a call panics without checking why.


Closes HH-554
Closes HH-579
Closes HH-580
Closes HH-582
Closes HH-583

@linear
Copy link

linear bot commented Apr 28, 2022

@changeset-bot
Copy link

changeset-bot bot commented Apr 28, 2022

⚠️ No Changeset found

Latest commit: bf50263

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

@linear
Copy link

linear bot commented Apr 28, 2022

@fvictorio fvictorio force-pushed the francovictorio/hh-581/reverted-with-panic branch from 733f7f7 to 8e16667 Compare April 28, 2022 16:50
@linear
Copy link

linear bot commented Apr 28, 2022

HH-583 Detect errors that don't come from a reverted transaction, if possible

Something like this will pass now:

await expect(Promise.reject(new Error())).to.be.reverted;

This is important if a transaction fails for some reason unrelated to the actual transaction execution.

One option is to check if there's .data in the Error. But this should be tested both with different ethers.js versions and with geth (and maybe ganache?)

@fvictorio fvictorio changed the title Add .revertedWithPanic matcher Add revert matchers Apr 28, 2022
@fvictorio fvictorio force-pushed the francovictorio/hh-581/reverted-with-panic branch 2 times, most recently from 5c14654 to 21304fc Compare April 28, 2022 17:24
@linear
Copy link

linear bot commented Apr 30, 2022

Copy link
Contributor

@feuGeneA feuGeneA left a comment

Choose a reason for hiding this comment

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

Still not finished reviewing everything, but wanted to ship you the comments I already have.

Copy link
Contributor

@feuGeneA feuGeneA left a comment

Choose a reason for hiding this comment

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

I love the way you validate the subject in the .reverted matcher, but is there a reason why you don't do that same validation in the other matchers?

@fvictorio fvictorio force-pushed the francovictorio/hh-581/reverted-with-panic branch from 8e41d10 to 0887967 Compare May 10, 2022 10:20
@fvictorio fvictorio changed the base branch from hardhat-chai-matchers to master May 10, 2022 21:01
@fvictorio fvictorio force-pushed the francovictorio/hh-581/reverted-with-panic branch from 221867d to bf50263 Compare May 10, 2022 21:12
@fvictorio fvictorio merged commit 2b30eba into master May 10, 2022
@fvictorio fvictorio deleted the francovictorio/hh-581/reverted-with-panic branch May 10, 2022 21:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants