-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(op-isthmus): l1tol2-msg-passer
storage root validation
#13311
base: main
Are you sure you want to change the base?
Conversation
#[derive(Debug, PartialEq, Eq, Clone, Display, Error, From)] | ||
pub enum OpConsensusError { | ||
/// Block body has non-empty withdrawals list. | ||
#[display("non-empty withdrawals list")] |
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.
we can now use thiserror again and should stick to that for new error types
#[display("custom l2 error (search for it in debug logs)")] | ||
Other, |
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.
hmm, can we use box core::Error and remove the clone from this error type?
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.
I tried to remove the clone from the error type but it required too many changes and grew the scope of the pr significantly
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.
I see, I believe this is due to some other error type Clone derives
we can keep this but perhaps add a string value?
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 failed for another reason I recall, think it was requirement to be Send + Sync
, in that case wrapped in an Arc
@@ -0,0 +1,8 @@ | |||
//! Addresses of OP pre-deploys. | |||
// todo: move to op-alloy |
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.
yeah let's do that
pub fn validate_l2_to_l1_msg_passer( | ||
provider: impl StateProviderFactory, | ||
header: &SealedHeader, | ||
) -> Result<(), ConsensusError> { |
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 we split this into two, one fn that validates given the storage root, and one that computes the storage root and then calls this other fn (which we could move to op-alloy as well) because I assume kona could maybe also use this
Ref #13144
Branched out from #13214
Implements block validation according to isthmus consensus rules