-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: make js-tracer feature optional for node builder crate #12178
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
Conversation
mattsse
left a comment
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.
need some clarification on the issue first to proceed here
crates/node/builder/Cargo.toml
Outdated
| "reth-provider/test-utils", | ||
| "reth-transaction-pool/test-utils" | ||
| ] | ||
| js-tracer = ["reth-rpc/js-tracer"] |
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.
this should go above test-utils
mattsse
left a comment
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.
we also need to enable this in the node crates for ethereum and optimism to actually enable this:
reth/crates/optimism/node/Cargo.toml
Line 23 in 129f3ba
| reth-node-builder.workspace = true |
reth/crates/ethereum/node/Cargo.toml
Line 19 in 129f3ba
| reth-node-builder.workspace = true |
Sure! Should I make this a default or also make it as a feature in optimism and eth crate? |
If possible make it all a feature and opt-in. |
|
I'm also needing this. This should be opt-in, at the very least because (as it is right now) it makes API consumer have to build the whole JS engine even when they don't use it. I don't know if there's some issue with the crate hierarchy that makes the exposing the types in the API necessitate building the engine crates, that seems strange. |
|
Hey @mattsse, i think this is ready for review if you could have a look :) |
could please elaborate, which crates exactly @delbonis |
mattsse
left a comment
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.
this still removes js-tracer support from the binary because now it is no longer enabled.
we either need this for ethereum node and optimism node by default or must enable this in the binary.
Done ! |
mattsse
left a comment
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.
we also n
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.
we need this for the optimimism/bin as well
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.
My bad, I've added it as well in there
|
friendly bump 😄 |
|
thanks, wrapping up here now |
e06a8e4 to
0455def
Compare
|
Merge is blocked, we need an approval. |
|
Nice thanks! :) |
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
re paradigmxyz/reth#12178 opening this so we don't forget to enable it after next reth bump
Closes #11548