Skip to content

fix(deploy): resolve WCT proxy from artifact, not recomputed CREATE2#48

Merged
rplusq merged 2 commits into
mainfrom
fix/wctdeploy-proxy-from-artifact
Jul 14, 2026
Merged

fix(deploy): resolve WCT proxy from artifact, not recomputed CREATE2#48
rplusq merged 2 commits into
mainfrom
fix/wctdeploy-proxy-from-artifact

Conversation

@rplusq

@rplusq rplusq commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

WCTDeploy.upgradeToL2WCT/upgradeToWCT recomputed the proxy CREATE2 address using ADMIN_ADDRESS as the initialOwner. The proxy address actually depends on initialOwner + init calldata, which differ from what other deploy paths used (OptimismDeploy uses the admin timelock as owner), so the computed address could point at a codeless phantom. Read the deployed proxy from the persisted artifact instead (OP vs Ethereum, mirroring _updateDeploymentJson) and assert it has code. Removes the now-unused _computeLegacyAddress + TransparentUpgradeableProxy import.

🤖 Generated with Claude Code

WCTDeploy.upgradeToL2WCT/upgradeToWCT recomputed the proxy CREATE2 address using ADMIN_ADDRESS
as the initialOwner, but the proxy address depends on initialOwner + init calldata — which differ
from what other deploy paths deployed (OptimismDeploy uses the admin timelock as owner). The
computed address could point at a codeless phantom. Read the deployed proxy from the persisted
artifact instead (OP vs Ethereum, mirroring _updateDeploymentJson), and assert it has code.
Removes the unused _computeLegacyAddress + TransparentUpgradeableProxy import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 13, 2026

Copy link
Copy Markdown

Claude finished @rplusq's task in 2m 2s —— View job


PR Review

  • Gather context
  • Read changed file
  • Analyze changes
  • Post review

The fix is correct — recomputing a proxy's CREATE2 address is inherently fragile because the address encodes the initialOwner/calldata used at deploy time, and those differ across deploy paths. Reading from the persisted artifact is the right source of truth, and the proxy.code.length > 0 guard is a solid safety net. JSON key alignment (.L2WCT.address / .WCT.address) matches what DeploymentJsonWriter actually writes.

Two issues found:

Found 2 issue(s)

Issue 1: upgradeToL2WCT loads unnecessary env vars that can fail the upgrade

ID: wctdeploy-upgradel2wct-unused-envload-a3f1
File: evm/script/deploy/WCTDeploy.s.sol:79
Severity: MEDIUM
Category: code_quality

Context:

  • Pattern: _readDeploymentParamsFromEnv() calls vm.envAddress for ADMIN_ADDRESS, MANAGER_ADDRESS, OP_BRIDGE_ADDRESS, REMOTE_TOKEN_ADDRESS, and salt, but after this PR only params.superchainBridge is consumed by upgradeToL2WCT.
  • Risk: vm.envAddress reverts if the env var is absent. An operator running upgradeToL2WCT who has only SUPERCHAIN_BRIDGE_ADDRESS in their environment (reasonable for a pure upgrade) will get a hard revert on none-needed vars.
  • Impact: Deploy script unusable without setting env vars that serve no purpose in the upgrade path.
  • Trigger: Any upgradeToL2WCT invocation where legacy vars like ADMIN_ADDRESS / OP_BRIDGE_ADDRESS aren't exported.

Recommendation: Extract only what's needed, or add a narrower env reader:

function upgradeToL2WCT() public broadcast {
    address superchainBridge = vm.envAddress("SUPERCHAIN_BRIDGE_ADDRESS");
    address legacyAddress = _readDeployedProxy();
    // ... use superchainBridge directly
    upgraded.setBridge(superchainBridge);

Fix this →


Issue 2: LegacyDeploymentParams.salt and its computation are dead code

ID: wctdeploy-legacyparams-salt-deadcode-b7c2
File: evm/script/deploy/WCTDeploy.s.sol:21
Severity: LOW
Category: code_quality

Context:

  • Pattern: LegacyDeploymentParams.salt (line 21) is populated in _readDeploymentParamsFromEnv() (line 157: salt: keccak256(...)) but never read by any caller now that _computeLegacyAddress is gone.
  • Risk: None at runtime, but dead fields in a struct mislead future readers into thinking salt is still used somewhere.
  • Impact: Minor maintainability / confusion.
  • Trigger: N/A — compile-time dead code.

Recommendation: Remove salt from LegacyDeploymentParams and the corresponding line in _readDeploymentParamsFromEnv.

Comment thread evm/script/deploy/WCTDeploy.s.sol Outdated
@@ -78,7 +77,10 @@ contract WCTDeploy is BaseScript {

function upgradeToL2WCT() public broadcast {
LegacyDeploymentParams memory params = _readDeploymentParamsFromEnv();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Auto Review Issue: upgradeToL2WCT loads unnecessary env vars that can fail the upgrade

Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review

Context:

  • Pattern: _readDeploymentParamsFromEnv() calls vm.envAddress for ADMIN_ADDRESS, MANAGER_ADDRESS, OP_BRIDGE_ADDRESS, REMOTE_TOKEN_ADDRESS, and salt, but after this PR only params.superchainBridge is consumed by upgradeToL2WCT.
  • Risk: vm.envAddress reverts if the env var is absent. An operator running upgradeToL2WCT who has only SUPERCHAIN_BRIDGE_ADDRESS in their environment (reasonable for a pure upgrade) will get a hard revert on none-needed vars.
  • Impact: Deploy script unusable without setting env vars that serve no purpose in the upgrade path.
  • Trigger: Any upgradeToL2WCT invocation where legacy vars like ADMIN_ADDRESS / OP_BRIDGE_ADDRESS aren't exported.

Recommendation: Extract only what's needed, or add a narrower env reader:

function upgradeToL2WCT() public broadcast {
    address superchainBridge = vm.envAddress("SUPERCHAIN_BRIDGE_ADDRESS");
    address legacyAddress = _readDeployedProxy();
    // ... use superchainBridge directly
    upgraded.setBridge(superchainBridge);

Fix this →

upgradeToL2WCT loaded the full LegacyDeploymentParams, so a pure upgrade
would revert on unrelated unset env vars (ADMIN_ADDRESS, OP_BRIDGE_ADDRESS,
...). Read the one var it uses directly. Also drop the now-dead salt and
superchainBridge fields from the struct so run() only requires the vars it
actually consumes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rplusq rplusq merged commit a9845d8 into main Jul 14, 2026
7 of 8 checks passed
@rplusq rplusq deleted the fix/wctdeploy-proxy-from-artifact branch July 14, 2026 20:47
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.

1 participant