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] Add condition in balanceChange #3751

Conversation

SevenSwen
Copy link
Contributor

@SevenSwen SevenSwen commented Mar 7, 2023

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.

  • Set balanceChange as lamba function

@vercel
Copy link

vercel bot commented Mar 7, 2023

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 Apr 6, 2023 11:10am
hardhat-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 6, 2023 11:10am

@changeset-bot
Copy link

changeset-bot bot commented Mar 7, 2023

🦋 Changeset detected

Latest commit: cf29c78

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

@SevenSwen SevenSwen changed the title add condition in balanceChange Add condition in balanceChange Mar 9, 2023
@SevenSwen SevenSwen changed the title Add condition in balanceChange [hardhat-chai-matchers] Add condition in balanceChange Mar 9, 2023
@fvictorio
Copy link
Member

Hi @SevenSwen, thanks for this. I believe that if we add this, we should also add it to the changeTokenBalances, changeEtherBalance and changeEtherBalances too. Otherwise the API would feel inconsistent. Do you want to do that?

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.

@fvictorio fvictorio added status:ready This issue is ready to be worked on and removed status:triaging labels Mar 9, 2023
@SevenSwen
Copy link
Contributor Author

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>,
Copy link
Contributor

@k06a k06a Mar 20, 2023

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.

Copy link
Contributor Author

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.

@fvictorio
Copy link
Member

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 😅

@SevenSwen
Copy link
Contributor Author

Hi @fvictorio, you asked to be reminded of this issue. These changes are still relevant to us :)

@fvictorio
Copy link
Member

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 changeEtherBalances and changeTokenBalances work when receiving a predicate. In your implementation, this was just a function that was called, and you could run some assertions inside. But I think it's better to be consistent and to also use a function that returns a boolean.

If that change is fine by you, I'll merge this PR so that we can include it in our next release.

@SevenSwen
Copy link
Contributor Author

Thanks for your edits @fvictorio! This implementation suits us just fine. We will wait for the release with this PR

@SevenSwen
Copy link
Contributor Author

up :)

@schaable
Copy link
Contributor

@SevenSwen @k06a I'm closing this in favor of #5219, which is essentially the same but rebased and with some minor wording fixes.

@schaable schaable closed this May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready This issue is ready to be worked on
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants