Skip to content

Conversation

@karim-agha
Copy link
Contributor

📝 Summary

This PR closes this issue: #55

We no longer need to compile two separate binaries to pick between having flashblocks builder versus vanilla (standard) builder. By default the flashblocks feature is disabled until it is stabilized.

As part of this change few things happened:

  • implementations of different builders now live under builders/
  • Reduced type complexity in builders implementation
  • Unified configuration structs between standard and flashblocks builders

💡 Motivation and Context


✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Removes compile-time feature gates for flashblocks and unifies builder implementations under a runtime switch.

  • Eliminates cfg(feature = "flashblocks") conditionals and merges builder code into a builders/ folder
  • Introduces a BuilderMode enum and PayloadBuilder trait to select between standard and flashblocks modes at runtime
  • Simplifies configuration and execution data types and consolidates argument parsing

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/traits.rs Define unified node, pool, client, and transaction bounds
src/tests/mod.rs Always include vanilla and flashblocks tests
src/tests/framework/txs.rs Remove unnecessary clone of bundle options
src/primitives/reth/execution.rs Generalize ExecutionInfo to carry extra builder data
src/metrics.rs Drop feature-gated metrics fields
src/main.rs Switch to runtime builder mode and refactor startup logic
src/builders/mod.rs Add PayloadBuilder trait and common BuilderConfig
src/builders/standard/mod.rs Implement standard block builder
src/builders/flashblocks/config.rs New FlashblocksConfig and extension trait
src/args/mod.rs Enhance CLI with builder mode and playground defaults
Comments suppressed due to low confidence (5)

crates/op-rbuilder/src/builders/mod.rs:108

  • [nitpick] Using debug_struct("Config") may be ambiguous in logs; consider renaming to debug_struct("BuilderConfig") for clarity.
.debug_struct("Config")

crates/op-rbuilder/src/builders/flashblocks/config.rs:44

  • [nitpick] The trait is named FlashBlocksConfigExt but the struct is FlashblocksConfig (lowercase b). For consistency, align the casing (e.g., rename to FlashblocksConfigExt).
pub trait FlashBlocksConfigExt {

crates/op-rbuilder/src/traits.rs:13

  • The bound syntax FullNodeTypes<Types: NodeTypes<…>> relies on unstable Rust features. For compatibility with stable Rust, constrain the associated type in a where clause, e.g.: T: FullNodeTypes, <T as FullNodeTypes>::Types: NodeTypes<Payload = …>.
FullNodeTypes<
    Types: NodeTypes<Payload = OpEngineTypes, ChainSpec = OpChainSpec, Primitives = OpPrimitives>,

crates/op-rbuilder/src/tests/framework/txs.rs:145

  • The bundle_opts variable is assigned but never used afterward, leading to an unused‐variable warning. Consider removing this binding or prefixing it with an underscore.
let bundle_opts = self.bundle_opts;

crates/op-rbuilder/src/main.rs:60

  • Since builder_args is already owned, you can move it directly into try_from and avoid requiring Clone by writing try_from(builder_args) instead of cloning.
let builder_config = BuilderConfig::<B::Config>::try_from(builder_args.clone())

@SozinM
Copy link
Collaborator

SozinM commented May 26, 2025

Great pr!

@karim-agha karim-agha merged commit 92b8f6a into main May 26, 2025
3 checks passed
@karim-agha karim-agha deleted the karim/issue-55 branch May 26, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants