Skip to content

Commit c59007d

Browse files
frisitanofrisitano
committed
address feedback
1 parent 0493ec9 commit c59007d

File tree

9 files changed

+87
-69
lines changed

9 files changed

+87
-69
lines changed

‎crates/chain-orchestrator/src/event.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,6 @@ pub enum ChainOrchestratorEvent {
4949
l1_block_info: BlockInfo,
5050
/// The list of batches that have been triggered for the derivation pipeline.
5151
triggered_batches: Vec<BatchInfo>,
52-
/// The finalized block info after finalizing the consolidated batches.
53-
finalized_block_info: Option<BlockInfo>,
5452
},
5553
/// A batch has been reverted returning the batch info and the new safe head.
5654
BatchReverted {

‎crates/chain-orchestrator/src/lib.rs‎

Lines changed: 11 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -546,8 +546,8 @@ impl<
546546
Ok(None)
547547
}
548548
L1Notification::NewBlock(block_info) => self.handle_l1_new_block(*block_info).await,
549-
L1Notification::Finalized(block_info) => {
550-
metered!(Task::L1Finalization, self, handle_l1_finalized(*block_info))
549+
L1Notification::Finalized(block_number) => {
550+
metered!(Task::L1Finalization, self, handle_l1_finalized(*block_number))
551551
}
552552
L1Notification::BatchCommit { block_info, data } => {
553553
metered!(Task::BatchCommit, self, handle_batch_commit(*block_info, data.clone()))
@@ -706,22 +706,21 @@ impl<
706706
/// the new finalized L2 chain block and the list of finalized batches.
707707
async fn handle_l1_finalized(
708708
&mut self,
709-
block_info: BlockInfo,
709+
block_number: u64,
710710
) -> Result<Option<ChainOrchestratorEvent>, ChainOrchestratorError> {
711711
let (finalized_block_info, triggered_batches) = self
712712
.database
713713
.tx_mut(move |tx| async move {
714714
// Set the latest finalized L1 block in the database.
715-
tx.set_finalized_l1_block_number(block_info.number).await?;
715+
tx.set_finalized_l1_block_number(block_number).await?;
716716

717717
// Finalize consolidated batches up to the finalized L1 block number.
718-
let finalized_block_info =
719-
tx.finalize_consolidated_batches(block_info.number).await?;
718+
let finalized_block_info = tx.finalize_consolidated_batches(block_number).await?;
720719

721720
// Get all unprocessed batches that have been finalized by this L1 block
722721
// finalization.
723722
let triggered_batches =
724-
tx.fetch_and_update_unprocessed_finalized_batches(block_info.number).await?;
723+
tx.fetch_and_update_unprocessed_finalized_batches(block_number).await?;
725724

726725
Ok::<_, ChainOrchestratorError>((finalized_block_info, triggered_batches))
727726
})
@@ -736,7 +735,7 @@ impl<
736735
self.derivation_pipeline.push_batch(*batch, BatchStatus::Finalized).await;
737736
}
738737

739-
Ok(Some(ChainOrchestratorEvent::L1BlockFinalized(block_info.number, triggered_batches)))
738+
Ok(Some(ChainOrchestratorEvent::L1BlockFinalized(block_number, triggered_batches)))
740739
}
741740

742741
/// Handles a batch input by inserting it into the database.
@@ -760,7 +759,7 @@ impl<
760759
}
761760

762761
let event = ChainOrchestratorEvent::BatchCommitIndexed {
763-
batch_info: BatchInfo::new(batch.index, batch.hash),
762+
batch_info: (&batch).into(),
764763
l1_block_number: batch.block_number,
765764
};
766765

@@ -788,7 +787,7 @@ impl<
788787
batch_index: u64,
789788
l1_block_info: BlockInfo,
790789
) -> Result<Option<ChainOrchestratorEvent>, ChainOrchestratorError> {
791-
let (triggered_batches, finalized_block_info) = self
790+
let triggered_batches = self
792791
.database
793792
.tx_mut(move |tx| async move {
794793
// Insert the L1 block info.
@@ -797,8 +796,6 @@ impl<
797796
// finalize all batches up to `batch_index`.
798797
tx.finalize_batches_up_to_index(batch_index, l1_block_info.number).await?;
799798
let finalized_block_number = tx.get_finalized_l1_block_number().await?;
800-
let finalized_block_info =
801-
tx.finalize_consolidated_batches(finalized_block_number).await?;
802799

803800
// Get all unprocessed batches that have been finalized by this L1 block
804801
// finalization.
@@ -809,24 +806,15 @@ impl<
809806
vec![]
810807
};
811808

812-
Ok::<_, ChainOrchestratorError>((triggered_batches, finalized_block_info))
809+
Ok::<_, ChainOrchestratorError>(triggered_batches)
813810
})
814811
.await?;
815812

816-
if finalized_block_info.is_some() {
817-
tracing::info!(target: "scroll::chain_orchestrator", ?finalized_block_info, "Updating FCS with new finalized block info from batch finalization");
818-
self.engine.update_fcs(None, None, finalized_block_info).await?;
819-
}
820-
821813
for batch in &triggered_batches {
822814
self.derivation_pipeline.push_batch(*batch, BatchStatus::Finalized).await;
823815
}
824816

825-
Ok(Some(ChainOrchestratorEvent::BatchFinalized {
826-
l1_block_info,
827-
triggered_batches,
828-
finalized_block_info,
829-
}))
817+
Ok(Some(ChainOrchestratorEvent::BatchFinalized { l1_block_info, triggered_batches }))
830818
}
831819

832820
/// Handles a batch revert event by updating the database.

‎crates/database/db/src/operations.rs‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,10 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
325325
models::batch_commit::Entity::update_many()
326326
.filter(models::batch_commit::Column::Hash.is_in(batch_hashes.iter().cloned()))
327327
.col_expr(models::batch_commit::Column::RevertedBlockNumber, Expr::value(None::<i64>))
328-
.col_expr(models::batch_commit::Column::Status, Expr::value("consolidated"))
328+
.col_expr(
329+
models::batch_commit::Column::Status,
330+
Expr::value(BatchStatus::Consolidated.as_str()),
331+
)
329332
.exec(self.get_connection())
330333
.await?;
331334

@@ -342,8 +345,11 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
342345
tracing::trace!(target: "scroll::db", "Changing batch status from processing to committed in database.");
343346

344347
models::batch_commit::Entity::update_many()
345-
.filter(models::batch_commit::Column::Status.eq("processing"))
346-
.col_expr(models::batch_commit::Column::Status, Expr::value("committed"))
348+
.filter(models::batch_commit::Column::Status.eq(BatchStatus::Processing.as_str()))
349+
.col_expr(
350+
models::batch_commit::Column::Status,
351+
Expr::value(BatchStatus::Committed.as_str()),
352+
)
347353
.exec(self.get_connection())
348354
.await?;
349355

@@ -359,7 +365,7 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
359365

360366
models::batch_commit::Entity::update_many()
361367
.filter(models::batch_commit::Column::Hash.eq(batch_hash.to_vec()))
362-
.col_expr(models::batch_commit::Column::Status, Expr::value(status.to_string()))
368+
.col_expr(models::batch_commit::Column::Status, Expr::value(status.as_str()))
363369
.exec(self.get_connection())
364370
.await?;
365371

@@ -465,7 +471,7 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
465471
let filter = Condition::all()
466472
.add(models::batch_commit::Column::FinalizedBlockNumber.is_not_null())
467473
.add(models::batch_commit::Column::FinalizedBlockNumber.lte(finalized_l1_block_number))
468-
.add(models::batch_commit::Column::Status.eq("consolidated"));
474+
.add(models::batch_commit::Column::Status.eq(BatchStatus::Consolidated.as_str()));
469475
let batch = models::batch_commit::Entity::find()
470476
.filter(filter.clone())
471477
.order_by_desc(models::batch_commit::Column::Index)
@@ -482,7 +488,10 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
482488
.expect("Finalized batch must have at least one L2 block.");
483489
models::batch_commit::Entity::update_many()
484490
.filter(filter)
485-
.col_expr(models::batch_commit::Column::Status, Expr::value("finalized"))
491+
.col_expr(
492+
models::batch_commit::Column::Status,
493+
Expr::value(BatchStatus::Finalized.as_str()),
494+
)
486495
.exec(self.get_connection())
487496
.await?;
488497

@@ -499,7 +508,7 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
499508
let conditions = Condition::all()
500509
.add(models::batch_commit::Column::FinalizedBlockNumber.is_not_null())
501510
.add(models::batch_commit::Column::FinalizedBlockNumber.lte(finalized_l1_block_number))
502-
.add(models::batch_commit::Column::Status.eq("committed"));
511+
.add(models::batch_commit::Column::Status.eq(BatchStatus::Committed.as_str()));
503512

504513
let batches = models::batch_commit::Entity::find()
505514
.filter(conditions.clone())
@@ -517,7 +526,10 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
517526
})?;
518527

519528
models::batch_commit::Entity::update_many()
520-
.col_expr(models::batch_commit::Column::Status, Expr::value("processing"))
529+
.col_expr(
530+
models::batch_commit::Column::Status,
531+
Expr::value(BatchStatus::Processing.as_str()),
532+
)
521533
.filter(conditions)
522534
.exec(self.get_connection())
523535
.await?;
@@ -528,7 +540,8 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
528540
async fn fetch_and_update_unprocessed_committed_batches(
529541
&self,
530542
) -> Result<Vec<BatchInfo>, DatabaseError> {
531-
let conditions = Condition::all().add(models::batch_commit::Column::Status.eq("committed"));
543+
let conditions = Condition::all()
544+
.add(models::batch_commit::Column::Status.eq(BatchStatus::Committed.as_str()));
532545

533546
let batches = models::batch_commit::Entity::find()
534547
.filter(conditions.clone())
@@ -546,7 +559,10 @@ impl<T: WriteConnectionProvider + ?Sized + Sync> DatabaseWriteOperations for T {
546559
})?;
547560

548561
models::batch_commit::Entity::update_many()
549-
.col_expr(models::batch_commit::Column::Status, Expr::value("processing"))
562+
.col_expr(
563+
models::batch_commit::Column::Status,
564+
Expr::value(BatchStatus::Processing.as_str()),
565+
)
550566
.filter(conditions)
551567
.exec(self.get_connection())
552568
.await?;

‎crates/node/tests/e2e.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,7 @@ async fn shutdown_consolidates_most_recent_batch_on_startup() -> eyre::Result<()
915915
.await?;
916916

917917
// Lets finalize the first batch
918-
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_0_info))).await?;
918+
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_0_info.number))).await?;
919919

920920
// Lets iterate over all blocks expected to be derived from the first batch commit.
921921
let consolidation_outcome = loop {
@@ -943,7 +943,7 @@ async fn shutdown_consolidates_most_recent_batch_on_startup() -> eyre::Result<()
943943
.await?;
944944

945945
// Lets finalize the second batch.
946-
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_1_info))).await?;
946+
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_1_info.number))).await?;
947947

948948
// The second batch commit contains 42 blocks (5-57), lets iterate until the rnm has
949949
// consolidated up to block 40.
@@ -1024,7 +1024,7 @@ async fn shutdown_consolidates_most_recent_batch_on_startup() -> eyre::Result<()
10241024

10251025
// Send the second batch again to mimic the watcher behaviour.
10261026
let block_1_info = BlockInfo { number: 18318215, hash: B256::random() };
1027-
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_1_info))).await?;
1027+
l1_notification_tx.send(Arc::new(L1Notification::Finalized(block_1_info.number))).await?;
10281028

10291029
// Lets fetch the first consolidated block event - this should be the first block of the batch.
10301030
let l2_block = loop {
@@ -1272,7 +1272,7 @@ async fn consolidates_committed_batches_after_chain_consolidation() -> eyre::Res
12721272
.await?;
12731273
// Send the L1 block finalized notification.
12741274
l1_watcher_tx
1275-
.send(Arc::new(L1Notification::Finalized(batch_0_finalization_block_info)))
1275+
.send(Arc::new(L1Notification::Finalized(batch_0_finalization_block_info.number)))
12761276
.await?;
12771277

12781278
wait_for_event_predicate_5s(&mut rnm_events, |event| {
@@ -1310,7 +1310,7 @@ async fn consolidates_committed_batches_after_chain_consolidation() -> eyre::Res
13101310
}))
13111311
.await?;
13121312
l1_watcher_tx
1313-
.send(Arc::new(L1Notification::Finalized(batch_1_finalization_block_info)))
1313+
.send(Arc::new(L1Notification::Finalized(batch_1_finalization_block_info.number)))
13141314
.await?;
13151315

13161316
wait_for_event_predicate_5s(&mut rnm_events, |event| {

‎crates/primitives/src/batch.rs‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
use crate::RollupNodePrimitiveParsingError;
2+
13
use super::L2BlockInfoWithL1Messages;
24

35
use alloy_primitives::{Bytes, B256};
@@ -81,6 +83,17 @@ impl BatchStatus {
8183
pub const fn is_finalized(&self) -> bool {
8284
matches!(self, Self::Finalized)
8385
}
86+
87+
/// Returns the string representation of the batch status.
88+
pub const fn as_str(&self) -> &'static str {
89+
match self {
90+
Self::Committed => "committed",
91+
Self::Processing => "processing",
92+
Self::Consolidated => "consolidated",
93+
Self::Reverted => "reverted",
94+
Self::Finalized => "finalized",
95+
}
96+
}
8497
}
8598

8699
impl core::fmt::Display for BatchStatus {
@@ -96,7 +109,7 @@ impl core::fmt::Display for BatchStatus {
96109
}
97110

98111
impl core::str::FromStr for BatchStatus {
99-
type Err = ();
112+
type Err = RollupNodePrimitiveParsingError;
100113

101114
fn from_str(s: &str) -> Result<Self, Self::Err> {
102115
match s {
@@ -105,7 +118,7 @@ impl core::str::FromStr for BatchStatus {
105118
"consolidated" => Ok(Self::Consolidated),
106119
"reverted" => Ok(Self::Reverted),
107120
"finalized" => Ok(Self::Finalized),
108-
_ => Err(()),
121+
_ => Err(RollupNodePrimitiveParsingError::InvalidBatchStatusString(s.to_string())),
109122
}
110123
}
111124
}

‎crates/primitives/src/error.rs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
pub enum RollupNodePrimitiveError {
44
/// Error decoding an execution payload.
55
ExecutionPayloadDecodeError(alloy_eips::eip2718::Eip2718Error),
6+
/// Error parsing a rollup node primitive.
7+
ParsingError(RollupNodePrimitiveParsingError),
68
}
79

810
impl From<alloy_eips::eip2718::Eip2718Error> for RollupNodePrimitiveError {
@@ -17,6 +19,16 @@ impl core::fmt::Display for RollupNodePrimitiveError {
1719
Self::ExecutionPayloadDecodeError(e) => {
1820
write!(f, "execution payload decode error: {e}")
1921
}
22+
Self::ParsingError(e) => {
23+
write!(f, "parsing error: {e:?}")
24+
}
2025
}
2126
}
2227
}
28+
29+
/// An error that occurs when parsing a batch status from a string.
30+
#[derive(Debug)]
31+
pub enum RollupNodePrimitiveParsingError {
32+
/// Error parsing batch status from string.
33+
InvalidBatchStatusString(String),
34+
}

‎crates/primitives/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ mod chain;
2626
pub use chain::ChainImport;
2727

2828
mod error;
29-
pub use error::RollupNodePrimitiveError;
29+
pub use error::{RollupNodePrimitiveError, RollupNodePrimitiveParsingError};
3030

3131
mod metadata;
3232
pub use metadata::Metadata;

0 commit comments

Comments
 (0)