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

fix: avoid panic when using smock #429

Merged
merged 18 commits into from
May 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/nasty-teachers-brush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@nomicfoundation/edr": patch
---

Fixed `smock.fake` causing a panic
10 changes: 6 additions & 4 deletions .github/workflows/edr-npm-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,9 @@ jobs:
run: ls -R .
shell: bash
- name: Test bindings
run: docker run --rm -v $(pwd):/build -w /build/crates/edr_napi node:${{ matrix.node }} bash -c "wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node; pnpm testNoBuild"
# Setting CI=1 is important to make PNPM install non-interactive
# https://github.com/pnpm/pnpm/issues/6615#issuecomment-1656945689
run: docker run --rm -e CI=1 -v $(pwd):/build -w /build/crates/edr_napi node:${{ matrix.node }} bash -c "wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node; pnpm testNoBuild"
test-linux-x64-musl-binding:
name: Test bindings on x86_64-unknown-linux-musl - node@${{ matrix.node }}
needs:
Expand Down Expand Up @@ -255,7 +257,7 @@ jobs:
run: ls -R .
shell: bash
- name: Test bindings
run: docker run --rm -v $(pwd):/build -w /build/crates/edr_napi node:${{ matrix.node }}-alpine sh -c "wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node; pnpm testNoBuild"
run: docker run --rm -e CI=1 -v $(pwd):/build -w /build/crates/edr_napi node:${{ matrix.node }}-alpine sh -c "wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node; pnpm testNoBuild"
test-linux-aarch64-gnu-binding:
name: Test bindings on aarch64-unknown-linux-gnu - node@${{ matrix.node }}
needs:
Expand Down Expand Up @@ -294,7 +296,7 @@ jobs:
uses: addnab/docker-run-action@v3
with:
image: node:${{ matrix.node }}
options: "--platform linux/arm64 -v ${{ github.workspace }}:/build -w /build/crates/edr_napi"
options: "--platform linux/arm64 -v ${{ github.workspace }}:/build -w /build/crates/edr_napi -e CI=1"
run: |
wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node
set -e
Expand Down Expand Up @@ -332,7 +334,7 @@ jobs:
uses: addnab/docker-run-action@v3
with:
image: node:lts-alpine
options: "--platform linux/arm64 -v ${{ github.workspace }}:/build -w /build/crates/edr_napi"
options: "--platform linux/arm64 -v ${{ github.workspace }}:/build -w /build/crates/edr_napi -e CI=1"
run: |
wget -qO- 'https://unpkg.com/@pnpm/self-installer' | node
set -e
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/hardhat-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ jobs:
DO_NOT_SET_THIS_ENV_VAR____IS_HARDHAT_CI: true
FORCE_COLOR: 3
NODE_OPTIONS: --max-old-space-size=4096
run: cd hardhat-tests && pnpm test
run: cd hardhat-tests && pnpm test:ci
lint-hardhat-tests:
name: Lint Hardhat tests
runs-on: ubuntu-latest
Expand Down
32 changes: 21 additions & 11 deletions crates/edr_napi/src/call_override.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ use std::sync::mpsc::channel;

use edr_eth::{Address, Bytes};
use napi::{
bindgen_prelude::Buffer,
bindgen_prelude::{Buffer, Promise},
threadsafe_function::{
ErrorStrategy, ThreadSafeCallContext, ThreadsafeFunction, ThreadsafeFunctionCallMode,
},
tokio::runtime,
Env, JsFunction, Status,
};
use napi_derive::napi;
Expand Down Expand Up @@ -41,10 +42,15 @@ struct CallOverrideCall {
#[derive(Clone)]
pub struct CallOverrideCallback {
call_override_callback_fn: ThreadsafeFunction<CallOverrideCall, ErrorStrategy::Fatal>,
runtime: runtime::Handle,
}

impl CallOverrideCallback {
pub fn new(env: &Env, call_override_callback: JsFunction) -> napi::Result<Self> {
pub fn new(
env: &Env,
call_override_callback: JsFunction,
runtime: runtime::Handle,
) -> napi::Result<Self> {
let mut call_override_callback_fn = call_override_callback.create_threadsafe_function(
0,
|ctx: ThreadSafeCallContext<CallOverrideCall>| {
Expand All @@ -68,6 +74,7 @@ impl CallOverrideCallback {

Ok(Self {
call_override_callback_fn,
runtime,
})
}

Expand All @@ -78,21 +85,24 @@ impl CallOverrideCallback {
) -> Option<edr_provider::CallOverrideResult> {
let (sender, receiver) = channel();

let runtime = self.runtime.clone();
let status = self.call_override_callback_fn.call_with_return_value(
CallOverrideCall {
contract_address,
data,
},
ThreadsafeFunctionCallMode::Blocking,
move |result: Option<CallOverrideResult>| {
let result = result.try_cast();

sender.send(result).map_err(|_error| {
napi::Error::new(
Status::GenericFailure,
"Failed to send result from call_override_callback",
)
})
move |result: Promise<Option<CallOverrideResult>>| {
runtime.spawn(async move {
let result = result.await?.try_cast();
sender.send(result).map_err(|_error| {
napi::Error::new(
Status::GenericFailure,
"Failed to send result from call_override_callback",
)
})
});
Ok(())
},
);

Expand Down
7 changes: 5 additions & 2 deletions crates/edr_napi/src/provider.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ use crate::{
#[napi]
pub struct Provider {
provider: Arc<edr_provider::Provider<LoggerError>>,
runtime: runtime::Handle,
#[cfg(feature = "scenarios")]
scenario_file: Option<napi::tokio::sync::Mutex<napi::tokio::fs::File>>,
}
Expand Down Expand Up @@ -53,7 +54,7 @@ impl Provider {
))?;

let result = edr_provider::Provider::new(
runtime,
runtime.clone(),
logger,
subscriber_callback,
config,
Expand All @@ -64,6 +65,7 @@ impl Provider {
|provider| {
Ok(Provider {
provider: Arc::new(provider),
runtime,
#[cfg(feature = "scenarios")]
scenario_file,
})
Expand Down Expand Up @@ -185,7 +187,8 @@ impl Provider {
) -> napi::Result<()> {
let provider = self.provider.clone();

let call_override_callback = CallOverrideCallback::new(&env, call_override_callback)?;
let call_override_callback =
CallOverrideCallback::new(&env, call_override_callback, self.runtime.clone())?;
let call_override_callback =
Arc::new(move |address, data| call_override_callback.call_override(address, data));

Expand Down
1 change: 1 addition & 0 deletions crates/edr_napi/test/provider.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import chai, { assert } from "chai";
import chaiAsPromised from "chai-as-promised";

import {
ContractAndFunctionName,
EdrContext,
Expand Down
3 changes: 3 additions & 0 deletions hardhat-tests/integration/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
node_modules/
artifacts/
cache/
12 changes: 12 additions & 0 deletions hardhat-tests/integration/smock/contracts/FooBar.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

contract Foo {
function f() public {}
}

contract Bar {
function callFoo(Foo foo) public {
foo.f();
}
}
6 changes: 6 additions & 0 deletions hardhat-tests/integration/smock/hardhat.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
require("@nomiclabs/hardhat-ethers");

/** @type import('hardhat/config').HardhatUserConfig */
module.exports = {
solidity: "0.8.24",
};
17 changes: 17 additions & 0 deletions hardhat-tests/integration/smock/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"name": "hardhat-edr-smock-test",
"private": true,
"version": "1.0.0",
"description": "",
"main": "index.js",
"keywords": [],
"author": "",
"license": "ISC",
"devDependencies": {
"@defi-wonderland/smock": "^2.4.0",
"@nomiclabs/hardhat-ethers": "^2.2.3",
"chai": "^4.3.6",
"hardhat": "^2.22.3",
"mocha": "^10.0.0"
}
}
18 changes: 18 additions & 0 deletions hardhat-tests/integration/smock/test/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
const { smock } = require("@defi-wonderland/smock");
const { assert } = require("chai");
const chaiAsPromised = require("chai-as-promised");

describe("HH+EDR and smock", function () {
// Ported from https://github.com/fvictorio/edr-smock-issue
it("set call override callback for smock", async function () {
// Hardhat doesn't work with ESM modules, so we have to require it here.
// The pretest hook runs compile.
const { ethers } = require("hardhat");

const mockedFoo = await smock.fake("Foo");
const Bar = await ethers.getContractFactory("Bar");
const bar = await Bar.deploy();

await bar.callFoo(mockedFoo.address);
});
});
5 changes: 4 additions & 1 deletion hardhat-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,12 @@
"lint:fix": "pnpm prettier --write && pnpm eslint --fix",
"eslint": "eslint 'test/**/*.ts'",
"prettier": "prettier \"**/*.{js,md,json}\"",
"pretest": "cd ../crates/edr_napi && pnpm build",
"pretest": "pnpm build:edr",
"test": "mocha --recursive \"test/**/*.ts\"",
"test:integration": "bash run-integration-tests.sh",
"test:ci": "pnpm test && pnpm test:integration",
"build": "tsc --build --force --incremental .",
"build:edr": "cd ../crates/edr_napi && pnpm build",
"clean": "rimraf build-test tsconfig.tsbuildinfo test/internal/hardhat-network/provider/.hardhat_node_test_cache test/internal/hardhat-network/stack-traces/test-files/artifacts"
},
"devDependencies": {
Expand Down
15 changes: 15 additions & 0 deletions hardhat-tests/run-integration-tests.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/usr/bin/env bash

set -e

pnpm build:edr

cd integration
for i in *; do
if [ -d "$i" ]; then
echo "Running integration test: '$i'"
cd $i
pnpm hardhat test
cd ..
fi
done
Loading
Loading