-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Use code 3 for JSON-RPC responses where there has been a revert #8104
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
Changes from 17 commits
4a1208f
58c6d15
693af7b
38469a8
e27a7ad
fdbe457
6f69bf0
e8a096b
c2b356e
2f315c7
604a1e1
fa00124
aee237a
4a78a6d
ccd22ea
ebb6b1d
6e54860
b854121
b5e2f69
5cb67ab
752401d
92ba0a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| "@nomicfoundation/hardhat-ethers": patch | ||
| "@nomicfoundation/hardhat-ignition-viem": patch | ||
| "@nomicfoundation/hardhat-ignition": patch | ||
| "@nomicfoundation/hardhat-toolbox-viem": patch | ||
| "@nomicfoundation/hardhat-viem-assertions": patch | ||
| "@nomicfoundation/hardhat-viem": patch | ||
| "hardhat": patch | ||
| --- | ||
|
|
||
| Use code 3 for JSON-RPC revert error codes to align with standard node behavior and preserve error causes in viem/ethers. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,6 @@ | |
| "prettier": "3.2.5", | ||
| "rimraf": "^5.0.5", | ||
| "typescript": "~5.8.0", | ||
| "viem": "^2.43.0" | ||
| "viem": "^2.47.6" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import type { HardhatEthers } from "../src/types.js"; | ||
| import type { JsonRpcServer } from "hardhat/types/network"; | ||
|
|
||
| import assert from "node:assert/strict"; | ||
| import { after, before, beforeEach, describe, it } from "node:test"; | ||
|
|
||
| import { ensureError } from "@nomicfoundation/hardhat-utils/error"; | ||
|
|
||
| import { initializeTestEthers, spawnTestRpcServer } from "./helpers/helpers.js"; | ||
|
|
||
| let ethers: HardhatEthers; | ||
|
|
||
| describe("revert error cause chain", () => { | ||
| describe("in-process EDR network", async () => { | ||
| beforeEach(async () => { | ||
| ({ ethers } = await initializeTestEthers([ | ||
| { artifactName: "Contract", fileName: "error-messages" }, | ||
| ])); | ||
| }); | ||
|
|
||
| it("should have SolidityError as the thrown error", async () => { | ||
| const contract = await ethers.deployContract("Contract"); | ||
|
|
||
| let caughtError: Error | undefined; | ||
| try { | ||
| await contract.revertsWithReasonString(); | ||
| } catch (error) { | ||
| ensureError(error); | ||
| caughtError = error; | ||
| } | ||
|
|
||
| assert.ok( | ||
| caughtError !== undefined && caughtError.name === "SolidityError", | ||
| `Expected SolidityError, got ${caughtError?.name}`, | ||
| ); | ||
| assert.ok( | ||
| "code" in caughtError && caughtError.code === 3, | ||
| `Expected error code 3, got ${"code" in caughtError ? String(caughtError.code) : "undefined"}`, | ||
| ); | ||
| assert.ok( | ||
| caughtError.message.includes("some reason"), | ||
| `Expected message to include "some reason", got: "${caughtError.message}"`, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("hardhat node (HTTP)", async () => { | ||
| let server: JsonRpcServer; | ||
| let port: number; | ||
| let address: string; | ||
|
|
||
| before(async () => { | ||
| ({ server, port, address } = await spawnTestRpcServer()); | ||
| }); | ||
|
|
||
| after(async () => { | ||
| await server.close(); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| ({ ethers } = await initializeTestEthers( | ||
| [{ artifactName: "Contract", fileName: "error-messages" }], | ||
| { | ||
| networks: { | ||
| localhost: { type: "http", url: `http://${address}:${port}` }, | ||
| }, | ||
| }, | ||
| )); | ||
| }); | ||
|
|
||
| it("should have ProviderError as the thrown error", async () => { | ||
| const contract = await ethers.deployContract("Contract"); | ||
|
|
||
| let caughtError: Error | undefined; | ||
| try { | ||
| await contract.revertsWithReasonString(); | ||
| } catch (error) { | ||
| ensureError(error); | ||
| caughtError = error; | ||
| } | ||
|
|
||
| assert.ok( | ||
| caughtError !== undefined && caughtError.name === "ProviderError", | ||
| `Expected ProviderError, got ${caughtError?.name}`, | ||
| ); | ||
| assert.ok( | ||
| "code" in caughtError && caughtError.code === 3, | ||
| `Expected error code 3, got ${"code" in caughtError ? String(caughtError.code) : "undefined"}`, | ||
| ); | ||
| assert.ok( | ||
| caughtError.message.includes("some reason"), | ||
| `Expected message to include "some reason", got: "${caughtError.message}"`, | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,57 +2,43 @@ import type { Hex } from "viem"; | |
|
|
||
| import { assertHardhatInvariant } from "@nomicfoundation/hardhat-errors"; | ||
| import { ensureError } from "@nomicfoundation/hardhat-utils/error"; | ||
| import { isPrefixedHexString } from "@nomicfoundation/hardhat-utils/hex"; | ||
| import { | ||
| ContractFunctionExecutionError, | ||
| ContractFunctionRevertedError, | ||
| } from "viem"; | ||
|
|
||
| /** | ||
| * Recursively extracts, if it exists, the revert data hex string from an error. | ||
| * Extracts the revert data hex string from a viem contract error. | ||
| * | ||
| * When a contract call reverts, Viem throws an `Error` whose `data` property | ||
| * contains the hex-encoded revert reason. That `data` may live on the top-level | ||
| * error or deeper in a chain of `cause` errors. This function walks down the | ||
| * `cause` chain until it finds a valid `0x` hex string. | ||
| * When a contract call reverts, viem throws a | ||
| * `ContractFunctionExecutionError` whose cause is a | ||
| * `ContractFunctionRevertedError` that carries the raw hex revert data | ||
| * in its `raw` property. | ||
| * | ||
| * @param error - The thrown Viem `Error` object, which may include a `data` field | ||
| * or nested `cause` errors carrying the revert data. | ||
| * @returns The `0x` hex string representing the revert data. | ||
| * @param error - The thrown viem `Error` object. | ||
| * @returns An object with the error `name`, `message`, and `data` (`0x`-prefixed hex revert data). | ||
| * | ||
| * @throws If no valid `0x` prefixed hex string revert data is found anywhere in the error chain. | ||
| * @throws If the error is not the expected viem revert error structure. | ||
| */ | ||
| export function extractRevertError(error: unknown): { | ||
| name: string; | ||
| message: string; | ||
| data: Hex; | ||
| } { | ||
| ensureError(error); | ||
| ensureError(error, ContractFunctionExecutionError); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change can probably be removed, but with the error code 3, the logic can be simplified. So I think it's a nice refactoring to have a cleaner code
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the previous version feel more robust. It also doesn't force you to change the test in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done here: b5e2f69 |
||
|
|
||
| let dataReason: Hex | undefined; | ||
| const { cause } = error; | ||
|
|
||
| let current: Error | undefined = error; | ||
| let message: string = ""; | ||
| while (current !== undefined) { | ||
| // Traverse the cause chain to find a valid raw error reason, if one exists. | ||
| if ("data" in current) { | ||
| const { data } = current; | ||
|
|
||
| if (typeof data === "string" && isPrefixedHexString(data)) { | ||
| dataReason = data; | ||
| message = current.message; | ||
| } | ||
| } | ||
|
|
||
| /* eslint-disable-next-line @typescript-eslint/consistent-type-assertions | ||
| -- all the nested errors might contain a `cause` field */ | ||
| current = current.cause as Error | undefined; | ||
| } | ||
| ensureError(cause, ContractFunctionRevertedError); | ||
|
|
||
| assertHardhatInvariant( | ||
| dataReason !== undefined, | ||
| `No revert data found on error.\nError name: "${error.name}", message: ${error.message}`, | ||
| cause.raw !== undefined, | ||
| `Expected raw revert data on ContractFunctionRevertedError, but got none. Error: "${cause.message}"`, | ||
|
ChristopherDedominici marked this conversation as resolved.
Outdated
|
||
| ); | ||
|
|
||
| return { | ||
| name: error.name, | ||
| message, | ||
| data: dataReason, | ||
| message: cause.message, | ||
| data: cause.raw, | ||
| }; | ||
| } | ||
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.
I don't get why this needs to be changed. I would have assumed that it was still working as expected.
If I revert the changes to this package, except for the
package.json, the tests still pass.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.
Done here: b5e2f69