-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Add condition in balanceChange #3751
[hardhat-chai-matchers] Add condition in balanceChange #3751
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: cf29c78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Hi @SevenSwen, thanks for this. I believe that if we add this, we should also add it to the Also, we have a similar feature for event/custom error arguments, and we use the term "predicate" to refer to these functions, so we should do that here too for consistency. |
Hi @fvictorio I have done :) If something else needs to be changed / added to the PR, please let me know. |
balanceChanges: BigNumberish[], | ||
balanceChanges: BigNumberish[] | Array<(change: BigNumber) => boolean>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make function with array of changes instead of array of functions, it will be much more powerful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, I have finished with this case. Unfortunately negated construction doesn't make sense with new predicate, so I had to ban it.
Hey, sorry for not reviewing this yet, we've been busy with the Shanghai hardfork and with supporting the new version of ethers. I'll try to make some time this week to look into this. Feel free to ping me if I don't 😅 |
Hi @fvictorio, you asked to be reminded of this issue. These changes are still relevant to us :) |
Hey @SevenSwen, thanks for the ping and sorry for not reviewing this sooner. The code looks pretty good, I only refactored some things and added tests, but I also made a change to how If that change is fine by you, I'll merge this PR so that we can include it in our next release. |
Thanks for your edits @fvictorio! This implementation suits us just fine. We will wait for the release with this PR |
up :) |
@SevenSwen @k06a I'm closing this in favor of #5219, which is essentially the same but rebased and with some minor wording fixes. |
Accurate comparison of balances is not suitable for all user cases. I propose to extend the value of the balanceChange parameter by adding a custom function support.