-
Notifications
You must be signed in to change notification settings - Fork 111
chore(builder): move op-rbuilder primitives into shared base-primitives crate #502
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
base: main
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
|
|
My take here is lets remove the feature flags (telemetary and op-rbuilder) and only move the bundle type for now. We can leave the execution type etc. in op-rbuilder |
| #[cfg(feature = "telemetry")] | ||
| { | ||
| use crate::telemetry::setup_telemetry_layer; | ||
| let telemetry_layer = setup_telemetry_layer(&telemetry_args)?; | ||
| cli_app.access_tracing_layers()?.add_layer(telemetry_layer); | ||
| } | ||
| #[cfg(not(feature = "telemetry"))] | ||
| { | ||
| return Err(eyre::eyre!("telemetry feature is disabled")); | ||
| } |
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.
Any reason to reintroduce this feature flag? I personally would rather avoid feature flags if possible.
| base-builder-cli.workspace = true | ||
| base-bundles.workspace = true | ||
| base-flashtypes.workspace = true | ||
| base-primitives = { workspace = true, features = ["op-rbuilder"] } |
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.
Lets remove the op-rbuilder feature flag on primitives. No need to gate the basic types
Hey! Just to clarify - when you say "only move the bundle type for now", should I revert the reth/ folder (engine_api_builder, execution types) back to op-rbuilder and keep only bundle.rs in shared? I moved everything initially based on the issue saying "split out primitives", but if you want a smaller PR with just bundles for now, I can revert the reth stuff back. Let me know what you prefer! |
Apologies for the confusion @vikions Yes, for now, let's only add |
This PR moves the op-rbuilder primitives into the shared
base-primitivescrate, de-duplicating the Bundle type and aligning with the current builder structure.Summary
crates/shared/primitives/src/op_rbuilderbase-primitivesand addedop-rbuilderfeaturebase_primitives::op_rbuilderBundlein favor of the existingbase-bundlescratetelemetryfeature and updated call sitesTesting
cargo check -p op-rbuildercargo check -p op-rbuilder --features telemetryThis is a clean PR rebased on the latest
mainto avoid the conflicts in the previous one.Closes #370