Skip to content

refactor(contracts): separate v4/v5 init slot clearing in L2ContractsManagerUtils#19797

Open
maurelian wants to merge 3 commits intodevelopfrom
refactor/l2cm-separate-v4-v5-init-clearing
Open

refactor(contracts): separate v4/v5 init slot clearing in L2ContractsManagerUtils#19797
maurelian wants to merge 3 commits intodevelopfrom
refactor/l2cm-separate-v4-v5-init-clearing

Conversation

@maurelian
Copy link
Copy Markdown
Contributor

Summary

  • Separate the interleaved v4/v5 initialization slot clearing into distinct code blocks — v4 path is fully contained in an if (_slot != v5Slot) block, v5 path runs unconditionally below
  • Add L2ContractsManager_InvalidV5Offset revert when v5 slot is passed with a non-zero offset
  • Add tests for non-zero _offset (previously untested) and the new v5 offset validation
  • Bump L2ContractsManager to 1.3.0

Motivation

The previous code always ran both v4 and v5 clearing logic, relying on implicit no-ops for the version that doesn't apply. When _slot == v5Slot, this caused a redundant SSTORE and a confusing re-read of a slot that was just written to. The refactored version is semantically identical but easier to reason about.

Test plan

  • All 9 L2ContractsManagerUtils_UpgradeToAndCall_Test tests pass
  • New: test_upgrade_v4NonZeroOffset_succeeds — exercises offset=2 bit-shifting
  • New: test_upgrade_v4InitializingNonZeroOffset_reverts — revert with offset=2
  • New: test_upgrade_v5InvalidOffset_reverts — validates new offset guard

@maurelian maurelian marked this pull request as ready for review March 27, 2026 18:35
@maurelian maurelian requested review from a team and Inphi March 27, 2026 18:35
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.4%. Comparing base (e7a1f70) to head (f5ffd51).

Additional details and impacted files
@@            Coverage Diff             @@
##           develop   #19797     +/-   ##
==========================================
+ Coverage     75.4%    80.4%   +5.0%     
==========================================
  Files          194      139     -55     
  Lines        11286     7258   -4028     
==========================================
- Hits          8519     5842   -2677     
+ Misses        2623     1416   -1207     
+ Partials       144        0    -144     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 80.4% <100.0%> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...-bedrock/src/libraries/L2ContractsManagerUtils.sol 52.6% <100.0%> (+2.6%) ⬆️

... and 56 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@0xOneTony 0xOneTony left a comment

Choose a reason for hiding this comment

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

lgtm

…ManagerUtils

The v4 and v5 initialization clearing paths were interleaved, making
the code hard to follow and causing a redundant SSTORE when _slot == v5Slot.
Separate them into distinct blocks and add a validation that v5 offset must be 0.
@maurelian maurelian force-pushed the refactor/l2cm-separate-v4-v5-init-clearing branch from 7395c34 to f5ffd51 Compare March 27, 2026 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants