Skip to content

chore: l2cm round comments2#883

Merged
0xiamflux merged 9 commits intosc-feat/l2cm-impl-l2contractsmanagerfrom
chore/l2cm-round-comments2
Feb 20, 2026
Merged

chore: l2cm round comments2#883
0xiamflux merged 9 commits intosc-feat/l2cm-impl-l2contractsmanagerfrom
chore/l2cm-round-comments2

Conversation

@0xiamflux
Copy link
Copy Markdown

@0xiamflux 0xiamflux commented Feb 20, 2026

✨ PR Description

Purpose: Refactor L2ContractsManager contract to expose implementation getters and improve upgrade logic for custom gas token network handling.

Main changes:

  • Added 27 public getter functions to expose predeploy implementation addresses through IL2ContractsManager interface
  • Fixed custom gas token detection by moving isCustomGasToken assignment earlier in _loadFullConfig method
  • Simplified Predeploys.isUpgradeable to remove runtime implementation check, moving validation into upgrade methods with L2ProxyAdmin

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how

@0xiamflux 0xiamflux self-assigned this Feb 20, 2026
Copy link
Copy Markdown

@linearb linearb bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ PR Review

The PR refactors L2ContractsManager configuration logic and adds implementation getter functions. The changes appear functionally correct but contain redundant external calls that impact performance.

1 issues detected:

🚀 Performance - Making two external calls to fetch the same data when one call's result is already stored in a variable wastes gas. 🛠️

Details: The function fetches the implementation address twice using different casts (L2ProxyAdmin on line 39, ProxyAdmin on line 45). The second call should reuse the implementation variable from line 39 to avoid the redundant external call. The same pattern occurs in the upgradeAndCall function at lines 98 and 104.
File: packages/contracts-bedrock/src/libraries/L2ContractsManagerUtils.sol (45-45)
🛠️ A suggested code correction is included in the review comments.

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how

// TODO(#19195): Remove this code skipping the ProxyAdmin once version is implemented.
_proxy != Predeploys.PROXY_ADMIN
_proxy != Predeploys.PROXY_ADMIN && implementation.code.length != 0
&& ProxyAdmin(Predeploys.PROXY_ADMIN).getProxyImplementation(_proxy) != address(0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance - Redundant External Call: Replace ProxyAdmin(Predeploys.PROXY_ADMIN).getProxyImplementation(_proxy) != address(0) with implementation != address(0) on line 45 (and similarly on line 104) to reuse the already-fetched implementation address.

Suggested change
&& ProxyAdmin(Predeploys.PROXY_ADMIN).getProxyImplementation(_proxy) != address(0)
&& implementation != address(0)
Is this review accurate? Use 👍 or 👎 to rate it

If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over

@linearb linearb bot added the 5 min review label Feb 20, 2026
0xOneTony
0xOneTony previously approved these changes Feb 20, 2026
Copy link
Copy Markdown

@linearb linearb bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ PR Review

LGTM

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how

@0xiamflux 0xiamflux merged commit d4812bb into sc-feat/l2cm-impl-l2contractsmanager Feb 20, 2026
5 checks passed
@0xiamflux 0xiamflux deleted the chore/l2cm-round-comments2 branch February 20, 2026 17:55
0xOneTony pushed a commit that referenced this pull request Feb 25, 2026
* feat: l2cm impl l2contractsmanager (#837)

* feat: add initial iteration of L2ContractsManager

* feat: add network configuration structs

* feat: load full config for L2ContractsManager

* feat: implement L2CM::_apply

* feat: add gas price oracle

* refactor: move L2CM types to library

* fix: upgrade ProxyAdmin predeploy

* chore: enforce delegatecall for L2CM::upgrade

* feat: add conditional upgrade for CGT

* refactor: remove non-proxied predeploys

* chore: renamed l2cm

* refactor: l2cm address comments (#839)

* refactor: rename _fullConfig to _loadFullConfig to match OPCM v2

* chore: remove non-proxied weth from implementations struct

* test: add config preservation test

* test: add CGT specific tests

* refactor: avoid casting network config values to address

* test: add test cases

* chore: pr ready (#844)

* chore: remove unnecesary casting on L2CM

* feat: add interface for XForkL2ContractsManager

* chore: add natspec to XForkL2ContractsManager

* chore: pr ready

* refactor: moves util functions out of L2CM implementation (#848)

* feat: l2cm address comments (#850)

* chore: add comment clarifying use `useCustomGasToken`

* chore: upgrade both native native asset liquidity and liquidity controller predeploys together

* feat: prohibit downgrading predeploy implementations

* refactor: make isCustomGasToken part of the network full config

* fix: add missing import

* fix: use FeeVault legacy getters for backward compat

* chore: update name XForkL2ContractsManager to L2ContractsManager

* feat: conditionally skip some predeploys based on them being supported in a given chain (#857)

* fix: l2cm address comments (#872)

* chore: add todo tracking removal of L2ProxyAdmin skips

* chore: add natspec comment for isPredeployNamespace

* chore: use vm.prank(address,bool) to prank a delegatecall

* chore: add todo for dev flags for CrossL2Inbox and L2ToL2CrossDomainMessenger

* feat: allow immutables for L2CM in semgrep rules

* chore: pr ready

* test: L2CM verify testing (#874)

* test: add coverage test for predeploy upgrades

* chore: update test natspec

* chore: just pr ready

* chore: L2CM round comments (#877)

* refactor: move helper function into Predeploys.s.sol

* fix: add conditional deployer to L2CM

* chore: update to l1block and l1blockCGT

* test: fixes issue where OptimismSuperchainERC20 tests fail due to profile ambiguity

* chore: just pr ready

* chore: l2cm round comments2 (#883)

* fix: move code length check out of isUpgradeable

* chore: inline fullCofig_.isCustomGasToken initialization

* chore: add public getters for the implementations on the L2CM

* chore: remove XForkL2ContractsManager sol rule exclusion

* test: add downgrade prevention test suite

* chore: just pr ready

* refactor: check for address 0 instead code length

* Revert "refactor: check for address 0 instead code length"

This reverts commit 1fa8694.

* chore: remove non-needed check

* chore: remove unused function in tests (#884)

* refactor: l2cm group impls (#885)

* refactor: remove individual getters in favor of a unified one

* test: add test for getImplementations

* test: add OZ v5 Initializable compatibility in L2ContractsManagerUtils (#887)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants