[Chai-matchers] Add polling mechanism when automine is not available#7997
[Chai-matchers] Add polling mechanism when automine is not available#7997ChristopherDedominici merged 28 commits intomainfrom
Conversation
…undation/hardhat into fix-hardhat-ethers-chai-matchers
🦋 Changeset detectedLatest commit: 838b6a2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
| }); | ||
| }); | ||
|
|
||
| describe("Matchers without automining", () => { |
There was a problem hiding this comment.
TMP file to test in example project that the bug is fixed, it will be removed before merging
There was a problem hiding this comment.
I don't mind leaving it to be honest.
…undation/hardhat into chai-matchers-poll
There was a problem hiding this comment.
I think this is looking really solid.
I have left a few small comments and two larger tasks:
- fast failing an invalid hash in
emit: #7997 (comment) - waiting for the number of confirmations: #7997 (comment)
I also addressed the race condition (with help from Claude), can you look over my change: d3eef66 and resolve the converation if your happy?
@ChristopherDedominici I will re-review once your happy with those changes. I will do a manual test using your additions in the example-project (thanks for those - they are really helpful). I will also run the update version from within the OpenZeppelin mocha test suite to confirm we haven't regressed anything in the normal automine path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kanej
left a comment
There was a problem hiding this comment.
I ran through the example project tests and then ran the changed packages against OpenZeppelin to confirm the standard cases are still working.
| const tx = await signer.sendTransaction({ to: signer.address }); | ||
|
|
||
| let resolved = false; | ||
| const waitPromise = ethers.provider | ||
| .waitForTransaction(tx.hash, confirms) | ||
| .then((r) => { | ||
| resolved = true; | ||
| return r; | ||
| }); | ||
|
|
||
| // mine the transaction into a block (1 confirmation) | ||
| await ethereumProvider.request({ method: "hardhat_mine" }); | ||
|
|
||
| // give the polling loop time to check | ||
| await sleep(100); | ||
| assert.equal( | ||
| resolved, | ||
| false, | ||
| "should not resolve with only 1 confirmation", | ||
| ); | ||
|
|
||
| // mine a second block (2 confirmations) | ||
| await ethereumProvider.request({ method: "hardhat_mine" }); | ||
| await sleep(100); | ||
| assert.equal( | ||
| resolved, | ||
| false, | ||
| "should not resolve with only 2 confirmations", | ||
| ); | ||
|
|
||
| // mine a third block (3 confirmations) — should now resolve | ||
| await ethereumProvider.request({ method: "hardhat_mine" }); | ||
|
|
||
| const receipt = await waitPromise; | ||
| assert.equal(resolved, true); | ||
| assertIsNotNull(receipt); | ||
| assert.equal(receipt.hash, tx.hash); | ||
| assert.equal(receipt.status, 1); | ||
|
|
||
| // restore automining | ||
| await ethereumProvider.request({ | ||
| method: "evm_setAutomine", | ||
| params: [true], | ||
| }); |
There was a problem hiding this comment.
In this test, automining is disabled but restored only at the end of the happy path. If any assertion (or the awaited promise) throws before the restore call, later tests can run with automine still disabled and fail unpredictably. Wrap the automine toggle in a try/finally (or use a helper) so evm_setAutomine(true) always runs.
| const tx = await signer.sendTransaction({ to: signer.address }); | |
| let resolved = false; | |
| const waitPromise = ethers.provider | |
| .waitForTransaction(tx.hash, confirms) | |
| .then((r) => { | |
| resolved = true; | |
| return r; | |
| }); | |
| // mine the transaction into a block (1 confirmation) | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| // give the polling loop time to check | |
| await sleep(100); | |
| assert.equal( | |
| resolved, | |
| false, | |
| "should not resolve with only 1 confirmation", | |
| ); | |
| // mine a second block (2 confirmations) | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| await sleep(100); | |
| assert.equal( | |
| resolved, | |
| false, | |
| "should not resolve with only 2 confirmations", | |
| ); | |
| // mine a third block (3 confirmations) — should now resolve | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| const receipt = await waitPromise; | |
| assert.equal(resolved, true); | |
| assertIsNotNull(receipt); | |
| assert.equal(receipt.hash, tx.hash); | |
| assert.equal(receipt.status, 1); | |
| // restore automining | |
| await ethereumProvider.request({ | |
| method: "evm_setAutomine", | |
| params: [true], | |
| }); | |
| try { | |
| const tx = await signer.sendTransaction({ to: signer.address }); | |
| let resolved = false; | |
| const waitPromise = ethers.provider | |
| .waitForTransaction(tx.hash, confirms) | |
| .then((r) => { | |
| resolved = true; | |
| return r; | |
| }); | |
| // mine the transaction into a block (1 confirmation) | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| // give the polling loop time to check | |
| await sleep(100); | |
| assert.equal( | |
| resolved, | |
| false, | |
| "should not resolve with only 1 confirmation", | |
| ); | |
| // mine a second block (2 confirmations) | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| await sleep(100); | |
| assert.equal( | |
| resolved, | |
| false, | |
| "should not resolve with only 2 confirmations", | |
| ); | |
| // mine a third block (3 confirmations) — should now resolve | |
| await ethereumProvider.request({ method: "hardhat_mine" }); | |
| const receipt = await waitPromise; | |
| assert.equal(resolved, true); | |
| assertIsNotNull(receipt); | |
| assert.equal(receipt.hash, tx.hash); | |
| assert.equal(receipt.status, 1); | |
| } finally { | |
| // restore automining | |
| await ethereumProvider.request({ | |
| method: "evm_setAutomine", | |
| params: [true], | |
| }); | |
| } |
| let provider: EthereumProvider; | ||
|
|
||
| before(async () => { | ||
| ({ provider } = await initEnvironment("events")); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This block calls initEnvironment("events") a second time and uses that new provider to toggle automine/mine blocks, but the contracts/transactions in this suite were created with the ethers instance from the first initEnvironment call. If the second initEnvironment creates a separate in-process network/provider, these requests won’t affect the transactions under test, so the test may pass without actually exercising the non-automining behavior. Reuse the same provider obtained alongside ethers in the suite’s before() instead of creating a new environment here.
| let provider: EthereumProvider; | |
| before(async () => { | |
| ({ provider } = await initEnvironment("events")); | |
| }); |
| describe("Matchers without automining", () => { | ||
| it("emit should wait for the tx to be mined", async () => { | ||
| const { ethers, provider } = await hre.network.connect(); |
There was a problem hiding this comment.
The PR description includes a TODO to “Drop the development helper in the example project”, but this change adds a new “Matchers without automining” section to the example project tests. If these tests are intended only as a temporary dev helper, consider removing them before merge (or update the PR description/TODO if they’re meant to stay as a permanent regression example).
Fixes: #7952.
Related to: #7993.
The underlying PR fixes the Hardhat invariant error, but it is not enough to resolve the bug. Hardhat needs to support polling for networks where
automineis not defined, so this PR adds that feature.TODO