Skip to content

refactor(deploy): remove unused SystemDeploy upgrade path [CHAIN-5392] - #452

Merged
jackchuma merged 2 commits into
mainfrom
refactor/remove-systemdeploy-upgrade-api
Oct 13, 2026
Merged

jackchuma merged 2 commits into
mainfrom
refactor/remove-systemdeploy-upgrade-api

Conversation

@jackchuma

@jackchuma jackchuma commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Linear: https://linear.app/coinbase/issue/CHAIN-5392/remove-unused-upgrade-path-from-systemdeploy

What changed? Why?

Remove the unused script-level SystemDeploy upgrade API. Actual system upgrades use supported external tooling; keeping a second path required separate helpers and tests that could diverge.

  • Remove upgrade input/output types, the Upgraded event, upgrade-only helpers, tests, and the now-unused legacy SuperchainConfig interface.
  • Remove the local-upgrade fallback and its configuration toggles from fork setup.
  • Require SUPERCHAIN_OPS_ALLOCS_PATH for fork tests. Load the simulated post-upgrade state before deriving proxy and implementation addresses.

Notes to reviewers

Fresh deployment proxy initialization and implementation artifact saving are preserved. The unreachable MissingImplementations error and check are also removed; the sole validation caller already rejects the empty branch by selecting fresh implementation deployment. The invalid NitroValidator test now exercises supplied deployment implementations rather than the removed upgrade API.

SuperchainConfigNeedsUpgrade was already absent on main. The SystemConfig documentation no longer recommends the removed API. That comment changes its source hash in the regenerated semver lock; all production init-code hashes, ABI snapshots, and storage layouts are unchanged.

The useOpsRepo getter remains for existing test skip behavior. Fork tests without alloc input now fail clearly instead of running local upgrades or reading an unmodified live fork.

The local alloc regression uses a process-wide environment variable. The supported fork recipes restrict tests to test/L1/**, excluding this regression, so it cannot change their alloc input. Both independent reviewers reported no findings; the acceptance-criteria check confirmed all four criteria.

How has it been tested?

Initial implementation validated on commit 17f26133 with pinned Foundry 1.5.1:

  • mise exec -- just test --match-path test/deploy/SystemDeploy.t.sol: 18 passed.
  • mise exec -- just test --match-path test/setup/ForkLive.t.sol: 1 regression passed, covering missing input and alloc loading before address derivation.
  • mise exec -- just test: 1,219 passed, 0 failed, 1 existing skip.
  • mise exec -- just snapshots: passed with no uncommitted snapshot changes.
  • mise exec -- just lint-check and git diff --check: passed.

Live fork validation could not run because ETH_RPC_URL and SUPERCHAIN_OPS_ALLOCS_PATH are unset. The alloc-loading regression uses locally generated state, without live RPC. The unused-import checker is absent from this checkout; manual import-reference checks passed.

Follow-up dead-code cleanup in 561ec068 passed all 18 deployment tests, mise exec -- just lint-check, and git diff --check. No new test was added for removing an unreachable branch.

Generated with Toshi

@cb-heimdall

cb-heimdall commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

✅ Heimdall Review Status

Requirement Status More Info
Reviews ✅ 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@cb-heimdall

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@jackchuma
jackchuma marked this pull request as ready for review October 13, 2026 13:35
@0xth4nh

0xth4nh commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

LGTM !!!

Comment thread scripts/deploy/SystemDeploy.s.sol Outdated
@jackchuma
jackchuma requested a review from leopoldjoy October 13, 2026 19:53
@jackchuma
jackchuma merged commit a65f1e3 into main Oct 13, 2026
6 of 7 checks passed
@jackchuma
jackchuma deleted the refactor/remove-systemdeploy-upgrade-api branch October 13, 2026 20:17
@jackchuma jackchuma changed the title refactor(deploy): remove unused SystemDeploy upgrade path refactor(deploy): remove unused SystemDeploy upgrade path [CHAIN-5392] Oct 14, 2026
@linear

linear Bot commented Oct 14, 2026

Copy link
Copy Markdown

CHAIN-5392

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.

4 participants