fix(pg): Improve reliability of internal hardhat simulation in monorepos #1710

RPate97 · 2024-05-08T23:14:31Z

Purpose

Fixes an issue where some mono repo installations would not work correctly because of the location of the hardhat.config.js file used to spin up our internal simulation.
Fixes an issue where the user's version of hardhat may be used for our internal simulation if they have hardhat installed, causing unpredictable behavior.

Solution

We no longer rely on a hardhat task to create the in-process hardhat node. Instead, we create the internal hardhat process provider from scratch and then convert it into an ethers-compatible provider, which we then use for the simulation. This required creating a slightly modified version of the EthersHardhatProvider, which is functionally equivalent but does not require a hardhat project to work. This setup eliminates the brittle process of invoking a hardhat subtask in a child process using the hardhatRunner script and an actual hardhat.config.js configuration file.

Riders

This PR also includes a number of changes that resolve issues recently introduced by updates to Foundry and Hardat.

Fixes CHU-1067 by updating to hardhat 2.22.3.
In hardhat 2.22.3, a bug that causes the simulation to fail in certain situations when running against local nodes was introduced. I tried to resolve that issue but could not, so I disabled the simulation when the target network is a local node. This also required removing/updating some tests that relied on the simulation. I've created a ticket to reverse these changes if/when hardhat fixes the bug.

packages/plugins/script/Sample.s.sol

RPate97 · 2024-05-09T00:26:35Z

packages/core/src/provider/hardhat-provider.ts

+    /**
+     * Sphinx: We modified this class to accept an entire Network configuration object
+     * instead of just the name. We later pass this class to the associated signer so that
+     * it does not need to read from the hre global variable.
+     */
+    private readonly _network: Network
+  ) {}


See comment.

RPate97 · 2024-05-09T00:26:50Z

packages/core/src/provider/hardhat-provider.ts

+    // Account index
+    if (typeof address === 'number') {
+      const accounts: string[] = await accountsPromise
+      if (address >= accounts.length) {
+        throw new AccountIndexOutOfRange(address, accounts.length)
+      }
+      return InProcessEthersSigner.create(
+        this,
+        accounts[address],
+        // Sphinx: Here we pass the network object to the signer
+        this._network
+      )
+    }
+
+    if (typeof address === 'string') {
+      // Sphinx: Here we pass the network object to the signer
+      return InProcessEthersSigner.create(this, address, this._network)
+    }


See comments.

There are no other changes to this class.

RPate97 · 2024-05-09T00:30:11Z

packages/core/src/provider/hardhat-signer.ts

+    // Sphinx: We accept a network object here which is then used to configure the signer instead of reading from the HRE.
+    network: Network


See comment

packages/core/src/provider/hardhat-signer.ts

packages/plugins/test/mocha/simulate.spec.ts

RPate97 · 2024-05-09T00:36:53Z

packages/core/src/provider/hardhat-provider.ts

+/**
+ * This class is a fork of the Nomic Foundation HardhatEthersProvider, credit to them:
+ * https://github.com/NomicFoundation/hardhat/blob/3e32ba0e16bdbf52b8aeb7e696461a221a435dc2/packages/hardhat-ethers/src/internal/hardhat-ethers-provider.ts
+ * We've also forked the associated HardhatEthersSigner class.
+ *
+ * Sphinx needs in-process network for its internal simulation step. This network is created using the `createProvider` function of hardhat which
+ * creates an isolated network suitable for this use case:
+ * https://github.com/NomicFoundation/hardhat/blob/3e32ba0e16bdbf52b8aeb7e696461a221a435dc2/packages/hardhat-core/src/internal/core/providers/construction.ts#L49
+ *
+ * The `createProvider` function returns an `EthereumProvider` object which is not immediately compatible with our execution logic which uses ethers. So to make
+ * the execution process work with this provider, we must convert it a compatible provider. For that purpose, we've created this fork of the `HardhatEthersProvider`.
+ * The `HardhatEthersProvider` is perfect for our use case since it is designed to make the in-process hardhat node compatible with ethers. However, the standard
+ * implementation requires a hardhat project to be used which is problematic for our situation.
+ *
+ * So this fork makes a minor modification to the `HardhatEthersProvider` and `HardhatEthersSigner` classes to allow them to be used outside of a hardhat project.
+ * Where the original implementation relied on reading the hardhat configuration options directly from the `HardhatRuntimeEnvironment` global var, this implementation
+ * accepts a `network` object in the `InProcessEthersProvider` constructor which includes all of the necessary configuration options.
+ *
+ * To review the exact changes we made to this class, you can search `Sphinx` in this file and in the associated `hardhat-signer.ts` file.
+ *
+ * You might wonder why we made a full copy of the both classes just to make these modifications. The reason is that `HardhatEthersSigner` only implements a private
+ * constructor which makes it impossible for us to use subclasses to make the required changes.
+ */


Please read the whole comment carefully.

I don't recommend reviewing much else in this file, as it's just a copy of the HardhatEthersProvider with minimal modifications. I've left comments indicating my specific changes, though, if you'd like to look through those quickly.

RPate97 · 2024-05-09T02:32:26Z

packages/plugins/package.json

@@ -65,7 +64,7 @@
    "typescript": "^5.1.3",
    "yargs": "^17.7.2",
    "yesno": "^0.4.0",
-    "hardhat": "2.20.1"
+    "hardhat": "2.22.3"


This fixes CHU-1067.

.circleci/config.yml

RPate97 · 2024-05-09T23:11:28Z

packages/plugins/src/cli/index.ts

+let getaddrinfoErrors = 0
+// If an error occurs, we write the error message and stack trace to `stdout` then exit the process
+// with exit code `1`. We write the error to `stdout` instead of `stderr` because `stderr` may
+// contain warnings that were written via `console.warn`, which are indistinguishable from the
+// actual error message in `stderr`. By using `stdout`, we can throw an error that doesn't contain
+// warnings in the parent process.
+process.on('uncaughtException', (error) => {


We still need to catch getaddrinfo ENOTFOUND errors, but since we no longer invoke the hardhat runner as a separate script, we set this up when the CLI is first called.

packages/plugins/test/mocha/cli/deploy.spec.ts

packages/plugins/test/mocha/cli/propose.spec.ts

packages/plugins/test/mocha/foundry/utils.spec.ts

packages/plugins/src/hardhat/simulate.ts

sam-goldman

Looks great

RPate97 force-pushed the pate/improve-hardhat-sim-reliability branch from 0289669 to ee5e275 Compare May 8, 2024 23:24

RPate97 commented May 8, 2024

View reviewed changes

packages/plugins/script/Sample.s.sol Show resolved Hide resolved

RPate97 force-pushed the pate/improve-hardhat-sim-reliability branch 4 times, most recently from 72245d4 to b90553e Compare May 9, 2024 00:24