feat: update L2 genesis specs #23
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the configuration management within the system's experimental standard L2 genesis specification. It consolidates separate fee vault configuration functions into a single function named Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
specs/experimental/standard-l2-genesis.md (3)
324-327: Nitpick: Hyphenate "network-specific"
In the description for thesetIsthmusfunction, consider hyphenating “network-specific” (i.e. “network-specific config”) for improved clarity and consistency with common usage styles.🧰 Tools
🪛 LanguageTool
[uncategorized] ~326-~326: When ‘network-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...alled, it MUST call each getter for the network specific config and set the returndata into stor...(SPECIFIC_HYPHEN)
418-418: Grammar Improvement: Insert Comma Before "but"
In the sentence “The bridge is no longer set in the constructor but instead it uses thePredeploys.L2_ERC721_BRIDGE”, inserting a comma before “but” would better separate the independent clauses for improved readability.🧰 Tools
🪛 LanguageTool
[uncategorized] ~418-~418: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...idge is no longer set in the constructor but instead it uses the `Predeploys.L2_ERC7...(COMMA_COMPOUND_SENTENCE)
425-425: Grammar Improvement: Insert Comma Before "but"
Similarly, in the section describing theOptimismMintableERC20Factorychanges for L2—where it states that “the bridge is no longer set in the constructor but instead it uses thePredeploys.L2_STANDARD_BRIDGE”—a comma should be added before “but” to improve the sentence structure.🧰 Tools
🪛 LanguageTool
[uncategorized] ~425-~425: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...idge is no longer set in the constructor but instead it uses the `Predeploys.L2_STAN...(COMMA_COMPOUND_SENTENCE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
specs/experimental/standard-l2-genesis.md(14 hunks)
🧰 Additional context used
🪛 LanguageTool
specs/experimental/standard-l2-genesis.md
[uncategorized] ~326-~326: When ‘network-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...alled, it MUST call each getter for the network specific config and set the returndata into stor...
(SPECIFIC_HYPHEN)
[uncategorized] ~418-~418: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...idge is no longer set in the constructor but instead it uses the `Predeploys.L2_ERC7...
(COMMA_COMPOUND_SENTENCE)
[uncategorized] ~425-~425: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...idge is no longer set in the constructor but instead it uses the `Predeploys.L2_STAN...
(COMMA_COMPOUND_SENTENCE)
🔇 Additional comments (6)
specs/experimental/standard-l2-genesis.md (6)
15-18: Update TOC for Fee Vault Config and Fee Admin Roles
The updated Table of Contents now clearly introduces the consolidated fee vault configuration function (setFeeVaultConfig) and the new fee admin role with its associated functions (feeAdminandsetFeeVaultAdmin). Please verify that the anchor links (e.g.#setfeevaultconfig,#feeadmin,#setfeevaultadmin) correctly navigate to their respective sections.
65-74: Review ConfigType Enum Table Formatting
The ConfigType table is well-structured with clear names, values, and descriptions. Ensure that these enum values remain consistent with the contract implementations and that no additional configuration types (such as a fee vault admin type) are inadvertently omitted.
100-101: Emit Fee Vault Admin Update During Initialization
The initialization section now includes the emission ofConfigUpdate.FEE_VAULT_ADMIN, which is critical for updating the fee vault admin setting during system initialization. Confirm that the underlying implementation enforces that only the designated fee admin can later update fee vault configurations.
122-127: Update Function Interface for setFeeVaultConfig
The new function signaturesetFeeVaultConfig(ConfigType, address, uint256, WithdrawalNetwork)consolidates fee vault configuration management into one interface. Please verify that the smart contract implementation reflects this new signature and that access restrictions (e.g., caller permissions) are properly enforced.
128-144: New Fee Admin Role and Associated Functions
The addition of the Fee Admin section—with thefeeAdmin()getter and the settersetFeeVaultAdmin(address)—enhances the control around fee vault configurations. Ensure that thesetFeeVaultAdminfunction is secured so that only the system config owner can call it, and that the documentation clearly describes the responsibilities of this new role.
189-205: Enhance OptimismPortal Upgrade Function
The upgrade function in the OptimismPortal now accepts a_gasLimitparameter, and the documentation indicates that the corresponding event now targetsPredeploys.L2_PROXY_ADMIN. Verify that event data packing—including the new gas limit—and the upgrade logic are updated accordingly in the contract implementation.🧰 Tools
🪛 LanguageTool
[uncategorized] ~200-~200: Did you mean “there is”?
Context: ...owing fields are included: -fromis theDEPOSITOR_ACCOUNT-toisPredeploys.L2_PROXY_ADMIN-version...(THE_THERE)
|
Following on ethereum-optimism#597 |
Closes OPT-647
Summary by CodeRabbit
New Features
Refactor