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

fix: hardhat-chai-matchers - add check byte32 string to reverted matcher #5738

Merged
merged 3 commits into from
Sep 12, 2024

Conversation

iosh
Copy link
Contributor

@iosh iosh commented Sep 11, 2024

  • Because this PR includes a bug fix, relevant tests have been included.
  • Because this PR includes a new feature, the change was previously discussed on an Issue or with someone from the team.
  • I didn't do anything of this.

Add when receipt is null, check if value is a byte32 string. minimal changes were in PRto resolve #4725

I see the following comment in the code

        // If the subject of the assertion is not connected to a transaction
        // (hash, receipt, etc.), then the assertion fails.
        // Since we use `false` here, this means that `.not.to.be.reverted`
        // assertions will pass instead of always throwing a validation error.
        // This allows users to do things like:
        //   `expect(c.callStatic.f()).to.not.be.reverted`
        assert(false, "Expected transaction to be reverted");

So i think maybe we can change the check to :

      if (
        isTransactionResponse(value) ||
        (typeof value === "string" && isValidTransactionHash(value))
      ) {
        const hash = typeof value === "string" ? value : value.hash;

        // if (!isValidTransactionHash(hash)) {
        //   throw new TypeError(
        //     `Expected a valid transaction hash, but got '${hash}'`
        //   );
        // }

This way, we can enable other data types to pass the test, but they will be broken change.

Copy link

changeset-bot bot commented Sep 11, 2024

🦋 Changeset detected

Latest commit: 36a8891

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nomicfoundation/hardhat-chai-matchers Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented Sep 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 12, 2024 7:55pm

if (receipt === null) {
// If the receipt is null, maybe the string is a bytes32 string
if (isByte32String(hash)) {
assert(false, "Expected transaction to be reverted");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it also can throw an error:

 throw new TypeError(
            `Expected a valid transaction hash, but got bytes32  '${hash}'`
       )

Copy link
Member

@schaable schaable left a comment

Choose a reason for hiding this comment

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

Looks good. Tested with the code example provided in the issue and it works. Thanks @iosh!

@schaable schaable merged commit 60d344b into NomicFoundation:main Sep 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[hardhat-chai-matchers] Edge case with not.to.be.reverted and view functions
3 participants