-
Notifications
You must be signed in to change notification settings - Fork 3
feat: check continuity of payload attributes #346
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
attributes.push(WithL1FinalizedBlockNumber::new( | ||
batch.block_number, | ||
WithL2BlockNumber::new( | ||
block.context.number, | ||
WithBatchInfo::new(batch.index, batch.hash, attribute), | ||
), | ||
)); |
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.
The update adds a little work when initializing the structures, but the information attached to the attribute is more explicit.
// we reset `latest_processed_l2_block_number` to 0. | ||
self.latest_processed_l2_block_number = 0; |
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.
resetting to 0 means the next batch that we receive won't be checked for contiguity.
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.
Left some comments inline.
crates/engine/src/future/mod.rs
Outdated
payload_attributes_with_batch_info.index, | ||
payload_attributes_with_batch_info.hash, | ||
); | ||
let mut payload_attributes = payload_attributes_with_batch_info.inner.inner.inner.clone(); |
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.
This type is getting unwieldy with the requirement of .inner.inner.inner
. Should we consider refactoring the base types to include the additional metadata?
crates/primitives/src/block.rs
Outdated
} | ||
} | ||
/// Type alias for a wrapper type with the full L2 metadata. | ||
pub type WithFullL2Meta<T> = WithL1FinalizedBlockNumber<WithL2BlockNumber<WithBatchInfo<T>>>; |
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.
Same comment as above, this is a complex type. Should we push the additional metadata into the inner type T
.
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.
can do, I like the idea of having these composable wrappers, so we can just quickly attach some extra information to structures. But I can push this info to the T
@frisitano what do you think about bumping to rust edition 2024? Reth bumped it recently. |
Resolves #328