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

hardhat chai matchers: emit(...).withArgs does not properly work with BigInt closeTo in the callback #4934

Closed
WardenJakx opened this issue Feb 26, 2024 · 5 comments
Assignees
Labels
status:needs-more-info There's not enough information to start working on this issue

Comments

@WardenJakx
Copy link

WardenJakx commented Feb 26, 2024

Version of Hardhat

2.20.1

What happened?

I am upgrading my codebase from using ethers v5 and hardhat-toolbox v2 to ethers v6 and hardhat toolbox v4. I have most of the codebase updated, but there is a recurring error with event based tests that keep failing unless I remove the withArgs part of the chain

Ive tried different versions of the packages responsible for this upgrade but still run into issues. Ive run hardhat clean, deleted node_modules, but still run into the issue

When doing tx.to.emit(...).withArgs( ... (value:bigint) => expect(value).to.be.closeTo(value, delta) ), the withArgs will not work if with the closeTo callback (even though it should pass)

eg:

tx.to.emit(...).withArgs(
 signerAddress,
 (amount:bigint) => expect(amount).to.be.closeTo(parseEther("1"), parseUnits("1", "gwei"))
)

Reverts with

 AssertionError: Error in "Swap" event: Error in the 2nd argument assertion: The predicate did not return true

However, the amount parameter is exactly 1 ETH so this should be passing as true.

If i do

tx.to.emit(...).withArgs(
signerAddress,
parseEther("1")
)

it passes successfully

Is this possibly related to chaijs/chai#1606 ?

Minimal reproduction steps

tx.to.emit(...).withArgs(
...
(amount:bigint) => expect(amount).to.be.closeTo(parseEther("1"), parseUnits("1", "gwei"))
)

Search terms

hardhat-chai-matchers, assertions, closeTo

@WardenJakx WardenJakx changed the title hardhat chai matchers: .withArgs does not properly work with BigNumber closeTo assertions hardhat chai matchers: .withArgs does not properly work with BigInt closeTo assertions Feb 26, 2024
@WardenJakx WardenJakx changed the title hardhat chai matchers: .withArgs does not properly work with BigInt closeTo assertions hardhat chai matchers: emit(...).withArgs does not properly work with BigInt closeTo assertions Feb 26, 2024
@WardenJakx WardenJakx changed the title hardhat chai matchers: emit(...).withArgs does not properly work with BigInt closeTo assertions hardhat chai matchers: emit(...).withArgs does not properly work with BigInt closeTo in the callback Feb 26, 2024
@WardenJakx
Copy link
Author

@ChristopherDedominici bump on this

@ChristopherDedominici
Copy link
Contributor

Hi @WardenJakx. I just took a look at the code.
Currently the function that you're using is this one:

(amount:bigint) => expect(amount).to.be.closeTo(parseEther("1"), parseUnits("1", "gwei"))

The function is not returning a boolean value, see the docs here

If you modify the function to return true, it should work. In case the expect check fails, an error will be thrown:

(amount: bigint) => {
    expect(amount).to.be.closeTo(
        parseEther("1"),
        parseUnits("1", "gwei")
    );
    return true;
}         

@WardenJakx
Copy link
Author

WardenJakx commented Mar 11, 2024

Hi @WardenJakx. I just took a look at the code. Currently the function that you're using is this one:

(amount:bigint) => expect(amount).to.be.closeTo(parseEther("1"), parseUnits("1", "gwei"))

The function is not returning a boolean value, see the docs here

If you modify the function to return true, it should work. In case the expect check fails, an error will be thrown:

(amount: bigint) => {
    expect(amount).to.be.closeTo(
        parseEther("1"),
        parseUnits("1", "gwei")
    );
    return true;
}         

Hi, thanks for the response. This code block worked with previous versions of hardhat and ethers. Also, any callback for events will fail even if it just returns true so unfortunately your proposed fix does not solve the problem

Example:

await expect(tx)
          .to.emit(this.multiUserStrategy, "Mint")
          .withArgs(async (data: IMultiUserLLSDStrategy.MintEventDataStruct) => {
            return true;
          }),

will still fail @ChristopherDedominici

@ChristopherDedominici
Copy link
Contributor

Hi @WardenJakx, thanks for getting back to me.

Can I ask in which version of Hardhat your code was working?
I'm asking because I tried to reproduce the issue you described in Hardhat version 2.18.0 and 2.14.0, and I'm getting the same behavior that I'm getting with Hardhat version 2.21.0.
The behavior is that if I return true in the function, everything works fine, whereas if I do not return a true, the code fails. So, even in older versions, the behavior was the same as in the new ones.

Also, in your last message, you mentioned that your code does not even work if you return true, which makes me wonder: is it possible that there is some logic in your code triggering this weird behavior? Because I cannot reproduce the failure when I return true in the function. Could you please try to reproduce the problem with minimal steps in a new clean project and share it with me so I can investigate? Thanks

@ChristopherDedominici ChristopherDedominici added status:needs-more-info There's not enough information to start working on this issue and removed status:triaging labels Mar 13, 2024
@WardenJakx
Copy link
Author

Hi @ChristopherDedominici, thanks for the suggestion. I tried it on my codebase and it worked properly.

It seems like there was an unwritten change as before i could pass cases where (num => expect(...)) was all the logic there was in the callback

However, with some change, this is no longer possible unless you do num => { expect(...); return true; }

Not sure if this is a change with the hardhat library or chai library but if it is hardhat related it should be documented somewhere to help others facing this migration issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:needs-more-info There's not enough information to start working on this issue
Projects
Archived in project
Development

No branches or pull requests

2 participants