-
Notifications
You must be signed in to change notification settings - Fork 174
chore(protocol): refactor common logic of hardfork specific protocol code #2939
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
chore(protocol): refactor common logic of hardfork specific protocol code #2939
Conversation
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.
Pull Request Overview
This PR refactors common logic across hardfork-specific protocol code by extracting shared encoding/decoding functionality into centralized utilities. The refactoring eliminates code duplication while maintaining the same functional behavior.
- Introduces
CommonL1BlockFields
struct to handle shared encoding/decoding logic for Ecotone, Isthmus, and Interop hardforks - Creates shared validation utilities for SystemConfig updates with common error handling patterns
- Consolidates duplicated validation logic across multiple update types (batcher, EIP1559, operator fee, unsafe block signer)
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
crates/protocol/protocol/src/info/mod.rs |
Adds module declaration and export for new common utilities |
crates/protocol/protocol/src/info/common.rs |
Implements shared CommonL1BlockFields struct with encoding/decoding methods |
crates/protocol/protocol/src/info/isthmus.rs |
Refactors to use common encoding/decoding utilities, removes duplicated code |
crates/protocol/protocol/src/info/interop.rs |
Refactors to use common encoding/decoding utilities, removes duplicated code |
crates/protocol/protocol/src/info/ecotone.rs |
Refactors to use common encoding/decoding utilities, removes duplicated code |
crates/protocol/genesis/src/updates/mod.rs |
Adds module declaration for new common validation utilities |
crates/protocol/genesis/src/updates/common.rs |
Implements shared validation logic for SystemConfig updates |
crates/protocol/genesis/src/updates/signer.rs |
Refactors to use common validation utilities, removes duplicated validation code |
crates/protocol/genesis/src/updates/operator_fee.rs |
Refactors to use common validation utilities, removes duplicated validation code |
crates/protocol/genesis/src/updates/eip1559.rs |
Refactors to use common validation utilities, removes duplicated validation code |
crates/protocol/genesis/src/updates/batcher.rs |
Refactors to use common validation utilities, removes duplicated validation code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
}; | ||
common.encode_into(&mut buf); | ||
|
||
buf.into() |
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.
Maybe you could have a method that encodes directly the common l1 block fields into bytes so that you don't need to use a buffer here
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 is a great first step. I think we could go further and refactor each hardfork logic by building on top of the previous one: for instance Isthmus would would call Ecotone methods to encode all the fields before Isthmus. Then you would just need to add Isthmus specific logic. If a hardfork changes existing logic from the previous hardforks, you can refactor the method into a common helper that branches on the specific behavior that gets updated.
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. |
Thank you! I'll have a look to this in the next days, and would be happy to propose a follow-up PR 👍 |
This is expected to close #2933.
I ran successfully the CI pipeline locally using just.