feat: add Optimism bindings in N-API by Wodann · Pull Request #664 · NomicFoundation/edr

Wodann · 2024-09-13T15:43:58Z

This PR exposes Optimism through the N-API bindings, toggled using the optimism feature flag. It's a modified version of #659.

It also incorporates a fix for linker failure on Linux when building the test target and creates a clear split between internal (Rust) and external (N-API) types. The internal types live in in edr_napi_core.

I noticed that we need to be aware of mixed usage of pnpm build and pnpm build:optimism, as it will likely result in the Optimism bindings being committed by accident. This mistake will be caught by CI.

changeset-bot · 2024-09-13T15:44:01Z

⚠️ No Changeset found

Latest commit: 92e9191

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

Wodann · 2024-09-13T15:55:35Z

+use itertools::izip;
+
+/// Trait for a function that decodes console log inputs.
+pub trait DecodeConsoleLogInputsFn: Fn(Vec<Bytes>) -> Vec<String> + Send + Sync {}


We now deal with Rust Fns instead of the ThreadSafeFunction and JsFunction in this file. This creates a clear demarcation between marshalled types and internal types.

Wodann · 2024-09-13T15:59:56Z

+optimism = ["dep:edr_optimism"]
 scenarios = ["rand"]
-
-[profile.release]


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

Wodann · 2024-09-13T16:00:16Z

  workflow_dispatch:

 env:
-  RUSTFLAGS: -Dwarnings


This was overriding the .cargo/config.toml file's rustflags.

This change 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

Thanks for the explanatory comment! I'm wondering if removing it has effect on the check-edr and test-edr-rs? E.g. if there are some unused imports with some feature combination, would the CI not fail now? It's not a merge blocker for me if it doesn't, just want to make sure we consider it.

Good catch! Given that we use cargo hack in check-edr, we would lose some functionality - I think. I've re-added the environment variables for that specific command here.

Clippy catches all warnings that cargo check|build|test would catch, as it is a superset of the check command - under the hood. Since we run cargo clippy with --all-targets, the test target is also run.

This means that it's possible for the test-edr-rs step to succeed with warnings, but the step running cargo clippy would fail the CI.

To validate this, I just added two simple functions (dead code) to lib.rs:

fn foo() { println!("foo"); } #[cfg(test)] fn bar() { prinltn!("bar"); }

Running cargo check --all-targets gives the following warnings:

warning: function `foo` is never used --> crates/edr_eth/src/lib.rs:62:4 | 62 | fn foo() { | ^^^ | = note: `#[warn(dead_code)]` on by default warning: `edr_eth` (lib) generated 1 warning warning: function `bar` is never used --> crates/edr_eth/src/lib.rs:67:4 | 67 | fn bar() { | ^^^ warning: `edr_eth` (lib test) generated 2 warnings (1 duplicate)

These become errors when running cargo clippy --all-targets -- -D warnings (failing one at a time):

error: function `foo` is never used --> crates/edr_eth/src/lib.rs:62:4 | 62 | fn foo() { | ^^^ | = note: `-D dead-code` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(dead_code)]` error: could not compile `edr_eth` (lib) due to 1 previous error error: function `bar` is never used --> crates/edr_eth/src/lib.rs:63:4 | 63 | fn bar() { | ^^^ | = note: `-D dead-code` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(dead_code)]`

codecov · 2024-09-13T16:01:46Z

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/chains/optimism.rs	0.00%	24 Missing ⚠️
crates/edr_napi/src/provider/response.rs	0.00%	21 Missing ⚠️
crates/edr_optimism/src/spec.rs	0.00%	11 Missing ⚠️
crates/edr_napi/src/subscription.rs	0.00%	10 Missing ⚠️
... and 7 more

Additional details and impacted files

@@                Coverage Diff                 @@
##             feat/multichain     #664   +/-   ##
==================================================
  Coverage                   ?   63.05%           
==================================================
  Files                      ?      215           
  Lines                      ?    23689           
  Branches                   ?    23689           
==================================================
  Hits                       ?    14938           
  Misses                     ?     7837           
  Partials                   ?      914

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

agostbiro

Thanks for the PR! I left some questions + there seems to be a lot of CI errors

agostbiro · 2024-09-13T16:42:32Z

  workflow_dispatch:

 env:
-  RUSTFLAGS: -Dwarnings


Thanks for the explanatory comment! I'm wondering if removing it has effect on the check-edr and test-edr-rs? E.g. if there are some unused imports with some feature combination, would the CI not fail now? It's not a merge blocker for me if it doesn't, just want to make sure we consider it.

agostbiro · 2024-09-13T16:52:05Z

+
+# To be able to run unit tests on Linux, support compilation to 'x86_64-unknown-linux-gnu'.
+[target.'cfg(target_os = "linux")']
+rustflags = ["-C", "link-args=-Wl,--warn-unresolved-symbols"]


Do we have any unit tests that need this now? If not, I'd rather not add it tbh, because if I understand correctly, this will turn linker errors into warnings for all targets which means we might have build errors and not notice it.

FYI, it sounds like napi_build always does this under the hood because it's linked dynamically.

napi-rs/napi-rs#1005 (comment)
napi-rs/napi-rs#1782 (comment)

I agree that we should avoid setting this for the workspace. I have removed both instances for now, as there are no tests using edr_napi

agostbiro · 2024-09-13T16:54:59Z

+
+# To be able to run unit tests on Linux, support compilation to 'x86_64-unknown-linux-gnu'.
+[target.'cfg(target_os = "linux")']
+rustflags = ["-C", "link-args=-Wl,--warn-unresolved-symbols"]


Same comment: https://github.com/NomicFoundation/edr/pull/664/files#r1759191016

Wodann · 2024-09-13T19:52:11Z

+  l1ProviderFactory,
+  MineOrdering,
+  SubscriptionEvent,
+  // HACK: There is no way to exclude tsc type checking for a file from the


There is no way to exclude files from tsc via the command-line (microsoft/TypeScript#46005)

agostbiro

Thanks for looking into my comments, LGTM!

Wodann added 5 commits September 13, 2024 14:57

refactor: move prepublish script

09d7311

feat: add Optimism N-API package

4f8e763

fix: edr_napi (test) target

2499926

ci: prevent RUSTFLAGS from overriding .cargo/config.toml

fb093cf

refactor: revert to single NPM package

74f55e8

Wodann requested a review from a team September 13, 2024 15:43

Wodann self-assigned this Sep 13, 2024

Wodann had a problem deploying to github-action-benchmark September 13, 2024 15:44 — with GitHub Actions Error

Wodann added the no changeset needed This PR doesn't require a changeset label Sep 13, 2024

fix: remove Optimism bindings

80c3dc7

Wodann had a problem deploying to github-action-benchmark September 13, 2024 15:52 — with GitHub Actions Failure

Wodann commented Sep 13, 2024

View reviewed changes

agostbiro reviewed Sep 13, 2024

View reviewed changes

ci: add environment variables for cargo hack

65a545e

Wodann temporarily deployed to github-action-benchmark September 13, 2024 18:20 — with GitHub Actions Inactive

fix: pnpm testNoBuild

e750ef0

Wodann temporarily deployed to github-action-benchmark September 13, 2024 19:01 — with GitHub Actions Inactive

misc: apply review suggestions

7dd2b47

Wodann had a problem deploying to github-action-benchmark September 13, 2024 19:41 — with GitHub Actions Error

Wodann commented Sep 13, 2024

View reviewed changes

ci: increase timeout for issue 543

fa4f9e4

Wodann temporarily deployed to github-action-benchmark September 13, 2024 19:54 — with GitHub Actions Inactive

agostbiro approved these changes Sep 16, 2024

View reviewed changes

ci: increase timeout for macOS

e6c1908

Wodann temporarily deployed to github-action-benchmark September 16, 2024 16:09 — with GitHub Actions Inactive

ci: increase timeout for macOS

1bb2508

Wodann temporarily deployed to github-action-benchmark September 16, 2024 17:49 — with GitHub Actions Inactive

ci: increase timeout for macOS

f335282

Wodann temporarily deployed to github-action-benchmark September 16, 2024 23:16 — with GitHub Actions Inactive

ci: increase timeout for macOS

35219e5

Wodann temporarily deployed to github-action-benchmark September 17, 2024 00:03 — with GitHub Actions Inactive

ci: decrease timeout for macOS

92e9191

Wodann temporarily deployed to github-action-benchmark September 17, 2024 00:42 — with GitHub Actions Inactive

Wodann merged commit bcdcafa into feat/multichain Sep 17, 2024

Wodann deleted the refactor/single-npm-package branch September 17, 2024 00:42

github-actions Bot locked as resolved and limited conversation to collaborators Dec 18, 2024

Uh oh!

Conversation

Wodann commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot Bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wodann Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

agostbiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wodann Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agostbiro left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Wodann commented Sep 13, 2024 •

edited

Loading

changeset-bot Bot commented Sep 13, 2024 •

edited

Loading

Wodann Sep 13, 2024 •

edited

Loading

codecov Bot commented Sep 13, 2024 •

edited

Loading

Wodann Sep 13, 2024 •

edited

Loading