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

Feature: add chai matchers for viem #5252

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

johnpm-12
Copy link

  • 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.

This adds a new package hardhat-chai-matchers-viem which is forked from hardhat-chai-matchers but supports viem instead of ethers.

I originally planned to update the existing ethers-based package to support both viem and ethers, but that proved to be difficult. So I made it a separate package, similar to how there is hardhat-toolbox and hardhat-toolbox-viem.

I understand that issue #4874 was to make the existing package compatible, so if that's still the plan, no skin off my back if ya'll decide not to merge this. I wrote this because I needed it internally. It was very time consuming. I pushed it to npm if anyone else finds it useful in the meantime: https://www.npmjs.com/package/hardhat-chai-matchers-viem

All existing tests have been updated to use viem instead of ethers, and all tests pass. However, viem with an external hardhat node does not return contract error data in some cases, so the tests to use an external hardhat node are skipped. All tests pass with the in-process hardhat node. I was going to parse the viem error to piece together what I can about the contract error, but I figured it would be better if the hardhat node or viem or hardhat-viem were updated to include the contract error data instead.

Linting succeeds.

I don't know what your process is for publishing to npm, so I'm not sure if anything needs to be done to add a new package there.

If you want to compare this package to the ethers one, you can use git diff --no-index packages/hardhat-chai-matchers/src/index.ts packages/hardhat-chai-matchers-viem/src/index.ts in the root folder, for example. VS Code also has a command to compare 2 local files.

Copy link

changeset-bot bot commented May 20, 2024

🦋 Changeset detected

Latest commit: bb45ca8

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-viem Major

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 May 20, 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 May 20, 2024 2:40am

@alcuadrado
Copy link
Member

Hey, thanks for submitting this pr! I'll review it asap and we can discuss how to move forward

@johnpm-12
Copy link
Author

Sounds good.

After using this for a few days, I think it might be worth updating the addressable stuff to lowercase (or convert to checksum) any address it find in a .equal or .not.equal matcher before comparison.
Since the original ethers version doesn't do that, I won't add it here, but worth discussing adding it to that package too.

It's annoying to have to use viem's getAddress() constantly (or .toLowerCase()) because RPC calls typically return lowercase addresses (contract addresses, wallet address, etc), while viem contract calls return checksummed addresses (contract.read.someAddress()).

@TateB
Copy link
Contributor

TateB commented May 24, 2024

hey, i also wrote something very similar here just last week!
my rewrite is significantly more opinionated than this so i think this is a much more suitable match for the existing matchers package, but it also includes significantly more advanced types so maybe worth looking at for ideas. i also added a .toEqualAddress matcher after running into the same checksummed address issue you mentioned.

@johnpm-12
Copy link
Author

Hah, wish I knew that before I started this!

At first glance, definitely some useful things in your more opinionated version.

Could always bring the good stuff from your version into the ethers one.
Keeping the ethers and viem ones in parity could get annoying.
I might take another crack at combining them, if I have the time

@vrde
Copy link

vrde commented May 31, 2024

👋 for a new project I switched from ethers to viem and I was expecting a better developer experience. So far it's good, but today I discovered that I cannot use the chai matchers I'm used to, specifically the ones to check if a tx reverted or if it emitted events.

Once it's ready, it would be great to see this PR merged, as it would help devs to better test their contracts (plus I don't want to go back to ethers only for this).

@johnpm-12
Copy link
Author

👋 for a new project I switched from ethers to viem and I was expecting a better developer experience. So far it's good, but today I discovered that I cannot use the chai matchers I'm used to, specifically the ones to check if a tx reverted or if it emitted events.

Once it's ready, it would be great to see this PR merged, as it would help devs to better test their contracts (plus I don't want to go back to ethers only for this).

You can use this for now: https://www.npmjs.com/package/hardhat-chai-matchers-viem

@alcuadrado alcuadrado self-assigned this Jun 6, 2024
@johnpm-12
Copy link
Author

@alcuadrado any updates on this?

@kanej
Copy link
Member

kanej commented Jun 27, 2024

Hey @johnpm-12, we are currently in depths of our Hardhat v3 work, a major overhaul.
How we approach viem test assertions, either taking this PR as the basis or a different approach is something we will have more visibility on once we are working through our new test subsystem.
Sorry for the delay in getting you a full response.

@kanej kanej removed their assignment Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

5 participants