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

feat: add Optimism as separate N-API package #659

Draft
wants to merge 4 commits into
base: feat/multichain
Choose a base branch
from

Conversation

Wodann
Copy link
Member

@Wodann Wodann commented Sep 10, 2024

This PR restructures the edr_napi_core and edr_napi to be able to share code with the new edr_napi_optimism package.

I ran into one problem that I was able to solve. Namely, when a crate that generates NAPI bindings is added as a dependency to another crate that generates NAPI bindings, the test target of the dependant crate will throw linker errors. This is a known issue. I was only able to work around it by adding the recommended rustflags to both the workspace- and project-level .cargo/config.toml. If I only added it to the project, the cargo test --all-features --all-targets --workspace command would still throw the linker errors.

There is currently still a bug in the edr_napi_optimism JS tests. In the background, the hyper Client is sporadically trying to execute a future using its executor. By default, this executor uses tokio::spawn but since the edr_optimism NPM package doesn't have a tokio runtime, this panics.

thread '' panicked at /home/vscode/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-0.14.28/src/common/exec.rs:49:21:
there is no reactor running, must be called from the context of a Tokio 1.x runtime
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

The solution is to specify a custom executor, but reqwest always uses the Tokio executor. An issue exists to add this executor, but a previous PR was rejected because it was deemed unnecessary.

For now, my proposal would be to merge the changes to the project layout and move forward with the release of the GenericChainSpec, as edr_optimism won't be exposed anyway.

A future issue can then:

  1. fork reqwest
  2. add setting of the executor
  3. set the executor with the tokio::Runtime of our edr NPM package's runtime

Could you please pay special attention to these two aspects when reviewing?

@Wodann Wodann added the no changeset needed This PR doesn't require a changeset label Sep 10, 2024
@Wodann Wodann requested a review from a team September 10, 2024 22:05
@Wodann Wodann self-assigned this Sep 10, 2024
Copy link

changeset-bot bot commented Sep 10, 2024

⚠️ No Changeset found

Latest commit: fb093cf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

scenarios = ["rand"]

[profile.release]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo was reporting that this setting was ignored. Only the workspace Cargo.toml's profile are respected.

@@ -4,17 +4,15 @@ version = "0.3.5"
edition = "2021"

[lib]
crate-type = ["cdylib"]
crate-type = ["cdylib", "lib"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is required for edr_napi_optimism to use this crate as a dependency

@@ -0,0 +1,1193 @@
use core::fmt::Debug;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File moved from edr_napi, with a modification to the config. This removes the dependency on napi types.

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

Attention: Patch coverage is 0% with 1193 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat/multichain@05a8cb6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/edr_napi_core/src/logger.rs 0.00% 879 Missing ⚠️
crates/edr_napi/src/logger.rs 0.00% 61 Missing ⚠️
crates/edr_napi_core/src/provider/config.rs 0.00% 56 Missing ⚠️
crates/edr_napi_core/src/provider.rs 0.00% 43 Missing ⚠️
crates/edr_napi_core/src/spec.rs 0.00% 32 Missing ⚠️
crates/edr_napi_core/src/subscription.rs 0.00% 28 Missing ⚠️
crates/edr_napi/src/provider/response.rs 0.00% 19 Missing and 2 partials ⚠️
crates/edr_napi_optimism/src/factory.rs 0.00% 18 Missing ⚠️
crates/edr_optimism/src/spec.rs 0.00% 11 Missing ⚠️
crates/edr_napi/src/subscription.rs 0.00% 10 Missing ⚠️
... and 8 more
Additional details and impacted files
@@                Coverage Diff                 @@
##             feat/multichain     #659   +/-   ##
==================================================
  Coverage                   ?   63.07%           
==================================================
  Files                      ?      216           
  Lines                      ?    23689           
  Branches                   ?    23689           
==================================================
  Hits                       ?    14941           
  Misses                     ?     7819           
  Partials                   ?      929           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Wodann Wodann temporarily deployed to github-action-benchmark September 10, 2024 22:19 — with GitHub Actions Inactive
@Wodann Wodann temporarily deployed to github-action-benchmark September 12, 2024 20:22 — with GitHub Actions Inactive
@@ -10,7 +10,6 @@ on:
workflow_dispatch:

env:
RUSTFLAGS: -Dwarnings
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not have any adverse side-effects, as we also manually pass this argument to cargo clippy which should fail if there are any warnings.

For reference:

cargo clippy --workspace --all-targets --all-features -- -D warnings

@Wodann Wodann temporarily deployed to github-action-benchmark September 13, 2024 15:20 — with GitHub Actions Inactive
@Wodann Wodann removed the request for review from a team September 13, 2024 15:45
@Wodann Wodann marked this pull request as draft September 13, 2024 15:46
@Wodann Wodann changed the title feat: add Optimism N-API package feat: add Optimism as separate N-API package Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changeset needed This PR doesn't require a changeset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant