Refactor payload builder to accept generic builder tx #217
Conversation
| if let Some(flashtestations_builder_tx) = &self.flashtestations_builder_tx { | ||
| // We only include flashtestations txs in the last flashblock | ||
|
|
||
| let mut simulation_state = self.simulate_builder_txs_state::<FlashblocksExtraCtx>( |
There was a problem hiding this comment.
returns empty vec for now
| } | ||
| } | ||
|
|
||
| pub fn simulate_builder_tx<ExtraCtx: Debug + Default>( |
There was a problem hiding this comment.
replace with flashblocks builder tx contract call
| EvmExecutionError(Box<dyn core::error::Error + Send + Sync>), | ||
| /// Any other builder transaction errors. | ||
| #[error(transparent)] | ||
| Other(Box<dyn core::error::Error + Send + Sync>), |
There was a problem hiding this comment.
Wdyt about using anyhow in here?
| /// Current flashblock index | ||
| pub flashblock_index: u64, | ||
| /// Target flashblock count | ||
| pub target_flashblock_count: u64, | ||
| /// Whether the current flashblock is the first fallback block |
There was a problem hiding this comment.
Can we determine this from index?
|
|
||
| let (payload, fb_payload, mut bundle_state) = build_block(db, &ctx, &mut info)?; | ||
|
|
||
| best_payload.set(payload.clone()); | ||
| self.ws_pub | ||
| .publish(&fb_payload) | ||
| .map_err(PayloadBuilderError::other)?; | ||
| // We set the first fallback block flag to false after building the first fallback block |
There was a problem hiding this comment.
i'd rather handle via index
this side effect seems like something that would cause a bug in the long run
| let mut info = execute_pre_steps(&mut db, &ctx)?; | ||
| let sequencer_tx_time = sequencer_tx_start_time.elapsed(); | ||
| ctx.metrics.sequencer_tx_duration.record(sequencer_tx_time); | ||
| ctx.metrics.sequencer_tx_gauge.set(sequencer_tx_time); | ||
|
|
||
| // If we have payload with txpool we add first builder tx right after deposits | ||
| if !ctx.attributes().no_tx_pool { |
There was a problem hiding this comment.
Why have you removed this?
If we are building deposits only block we shouldn't include builder tx
There was a problem hiding this comment.
I thought this was never hit since rollup-boost does not send if no_tx_pool = true but I'll add it back in
There was a problem hiding this comment.
You are correct, buy removing this will break no_txpool support in case we change boost
Co-authored-by: Solar Mithril <[email protected]>
Co-authored-by: Solar Mithril <[email protected]>
Co-authored-by: Solar Mithril <[email protected]>
Co-authored-by: Solar Mithril <[email protected]>
📝 Summary
Refactor to allow the payload builder accept generics for how to construct the builder tx.
💡 Motivation and Context
Easier to add custom logic to builder tx based on the payload builder such as contract calls etc.
✅ I have completed the following steps:
make lintmake test