feat(intercept): add bridge intercept in op block builder#204
feat(intercept): add bridge intercept in op block builder#204JimmyShi22 wants to merge 1 commit intomainfrom
Conversation
Vui-Chee
left a comment
Vui-Chee
left a comment
There was a problem hiding this comment.
Code Review
Summary: This PR wires bridge intercept into the non-flashblocks payload builder path by introducing XLayerBlockBuilder (wraps any BlockBuilder to intercept at the per-tx execution level) and XLayerPayloadBuilder (wraps OpPayloadBuilder and drives XLayerBlockBuilder). The design is clean, well-documented, and the intercept crate has solid test coverage.
Verdict: Request Changes — one formatting issue will fail cargo fmt --check in CI, and a couple of minor observability/consistency gaps are worth fixing before merge.
| self | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
🟡 Missing builder method for consistency — XLayerPayloadBuilderConfig exposes with_da_config and with_gas_limit_config as chained builder methods, but bridge_config is only settable via direct field mutation (xlayer_config.bridge_config = config in with_bridge_config). Add a builder method to keep the API consistent:
/// Set the bridge intercept config.
pub fn with_bridge_config(mut self, bridge_config: BridgeInterceptConfig) -> Self {
self.bridge_config = bridge_config;
self
}This also lets the field become private in the future without a breaking change.
| // are not re-included in future blocks. | ||
| let intercepted = builder.take_intercepted_hashes(); | ||
| if !intercepted.is_empty() { | ||
| self.inner.pool.remove_transactions_and_descendants(intercepted); |
There was a problem hiding this comment.
🔵 Observability gap — intercepted transactions are silently removed from the pool. Add a log so operators can see which transactions were blocked and why (useful when debugging why a transaction never gets included):
let intercepted = builder.take_intercepted_hashes();
if !intercepted.is_empty() {
warn!(
target: "xlayer::payload_builder",
count = intercepted.len(),
hashes = ?intercepted,
"removing intercepted bridge transactions from pool"
);
self.inner.pool.remove_transactions_and_descendants(intercepted);
}
sieniven
left a comment
sieniven
left a comment
There was a problem hiding this comment.
Maybe we can leave to adding the intercept logic just on flashblocks builder for now. We could just cherry-pick the old intercept PR into okx/optimism op-reth crates if needed too in the future.
|
Converted to draft now |
| // ExecutionResult::logs() returns &[] for Revert/Halt, so no | ||
| // special-casing needed here. | ||
| if intercept_bridge_transaction_if_need( | ||
| exec_result.logs(), | ||
| signer, | ||
| &bridge_config, | ||
| ) | ||
| .is_err() |
There was a problem hiding this comment.
Why intercept after execution of tx?
3 participants
No description provided.