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

Make js-feature non default #10445

Merged
merged 4 commits into from
Aug 22, 2024
Merged

Conversation

nkysg
Copy link
Contributor

@nkysg nkysg commented Aug 22, 2024

towards #10441

crates/rpc/rpc-eth-types/Cargo.toml Outdated Show resolved Hide resolved
@@ -84,6 +84,8 @@ jsonrpsee-types.workspace = true
jsonrpsee = { workspace = true, features = ["client"] }

[features]

js-tracer = ["revm-inspectors/js-tracer"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

this likely needs to also be enabled for eth-rpc-types

@@ -84,6 +84,8 @@ jsonrpsee-types.workspace = true
jsonrpsee = { workspace = true, features = ["client"] }

[features]

Copy link
Collaborator

Choose a reason for hiding this comment

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

let's make js-tracer default here, for this pr, and solve making it non-default in this crate in a followup pr

@mattsse
Copy link
Collaborator

mattsse commented Aug 22, 2024

wait, why does this even compile

this should not compile if the feature is not enabled

let mut inspector =
JsInspector::new(code, config).map_err(Eth::Error::from_eth_err)?;

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@mattsse mattsse added the A-dependencies Pull requests or issues that are about dependencies label Aug 22, 2024
@@ -27,7 +27,7 @@ reth-tasks = { workspace = true, features = ["rayon"] }
reth-transaction-pool.workspace = true
reth-chainspec.workspace = true
reth-execution-types.workspace = true
reth-rpc-eth-types = { workspace = true, features = ["js-tracer"] }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe this place make compile.

@@ -50,6 +50,8 @@ dyn-clone.workspace = true
tracing.workspace = true

[features]
default = ["js-tracer"]
Copy link
Contributor Author

@nkysg nkysg Aug 22, 2024

Choose a reason for hiding this comment

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

remove this feature then debug.rs can't compile。 The errors

error[E0422]: cannot find struct, variant or union type `TransactionContext` in this scope
   --> crates/rpc/rpc/src/debug.rs:121:30
    |
121 |                         Some(TransactionContext {
    |                              ^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TransactionRequest`
    |
   ::: /Users/ysg/.cargo/registry/src/index.crates.io-6f17d22bba15001f/alloy-rpc-types-eth-0.2.1/src/transaction/request.rs:17:1
    |
17  | pub struct TransactionRequest {
    | ----------------------------- similarly named struct `TransactionRequest` defined here

error[E0422]: cannot find struct, variant or union type `TransactionContext` in this scope
   --> crates/rpc/rpc/src/debug.rs:269:26
    |
269 |                     Some(TransactionContext {
    |                          ^^^^^^^^^^^^^^^^^^ help: a struct with a similar name exists: `TransactionRequest`

Do we need to touch revm-inspectors? @mattsse

@nkysg nkysg requested a review from mattsse August 22, 2024 08:55
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

okay, since dealing with features is a bit weird, I want to merge it like this and transition to non default js-trace crate

@mattsse mattsse added this pull request to the merge queue Aug 22, 2024
@mattsse
Copy link
Collaborator

mattsse commented Aug 22, 2024

@nkysg next step would be

feature gating all the jstracer usage, e.g. if js-tracer feature is missing this should return an unsupported error:

GethDebugTracerType::JsTracer(code) => {

Merged via the queue into paradigmxyz:main with commit aa8b3de Aug 22, 2024
34 checks passed
@nkysg nkysg deleted the js-feature-non-default branch August 22, 2024 09:39
@nkysg
Copy link
Contributor Author

nkysg commented Aug 22, 2024

Got it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Pull requests or issues that are about dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants