Skip to content

chore(deploy): harmonize Periphery ownership to refundWallet#1815

Merged
0xDEnYO merged 5 commits into
mainfrom
chore/harmonize-periphery-ownership
May 20, 2026
Merged

chore(deploy): harmonize Periphery ownership to refundWallet#1815
0xDEnYO merged 5 commits into
mainfrom
chore/harmonize-periphery-ownership

Conversation

@0xDEnYO
Copy link
Copy Markdown
Contributor

@0xDEnYO 0xDEnYO commented May 18, 2026

Summary

Aligns the deploy scripts (and the healthCheck script) of the three Periphery contracts whose owner key was inconsistent with the agreed policy.

Policy

All Periphery contracts that hold owner / withdraw authority use refundWallet, with two intentional exceptions:

  • FeeCollector — keeps its dedicated feeCollectorOwner (on the way to deprecation, intentionally left as-is).
  • FeeForwarder — keeps withdrawWallet (treasury reporting context).

Changes

Contract Before After
ERC20Proxy deployerAddress (ephemeral — has produced a stale-EOA-owned mainnet deployment) refundWallet
GasZipPeriphery per-network safeAddress refundWallet
Permit2Proxy per-network safeAddress refundWallet

All six deploy script files (3 EVM + 3 zksync mirrors) updated to read .refundWallet from config/global.json, matching the canonical pattern used by DeployExecutor.s.sol, DeployTokenWrapper.s.sol, the receiver family, etc.

script/deploy/healthCheck.ts updated: the EVM + Tron ERC20Proxy ownership checks now expect refundWallet instead of deployerWallet. (GasZipPeriphery and Permit2Proxy have no ownership checks in healthCheck.) Note that the current mainnet ERC20Proxy is owned by a stale historical deployer EOA (0x11f1…c00c, not deployerWallet), so healthCheck was already failing on it today — this PR aligns the expectation; the migration will close the gap on-chain.

Out of scope

Existing on-chain deployments keep their current owners. A migration script (transferOwnership per chain) is a separate follow-up — particularly important for mainnet ERC20Proxy.

Context

Supersedes #1106, which became unrebaseable after main's config.sh → .env deploy-idiom migration. Full audit of the periphery surface was done before this PR — only the three contracts above were misaligned.

Test plan

  • forge build clean.
  • tsc --noEmit clean for healthCheck.ts.
  • Local CodeRabbit pre-flight: No findings ✔ on both commits.
  • Next deployment of any of the 3 contracts on any chain verifies the constructor arg matches refundWallet.
  • HealthCheck on a freshly redeployed network reports ERC20Proxy owner is correct.

🤖 Generated with Claude Code

Aligns deploy scripts with the agreed Periphery ownership policy:

- All Periphery contracts that hold owner/withdraw authority use
  refundWallet, with two exceptions:
  - FeeCollector keeps its dedicated feeCollectorOwner (on the way to
    deprecation, intentionally left as-is).
  - FeeForwarder keeps withdrawWallet (treasury reporting context).

Updated:
- DeployERC20Proxy(.zksync).s.sol: was deployerAddress -> refundWallet.
- DeployGasZipPeriphery(.zksync).s.sol: was per-network safeAddress
  -> refundWallet.
- DeployPermit2Proxy(.zksync).s.sol: was per-network safeAddress
  -> refundWallet.

Existing on-chain deployments are not touched by this PR; ownership
migration is tracked separately.

Supersedes #1106 (which became unrebaseable after main's deploy-idiom
migration).
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

Review Change Stack

Walkthrough

Deployment scripts for ERC20Proxy, GasZipPeriphery, and Permit2Proxy across Solidity and ZkSync are refactored to read owner/withdrawal recipient addresses from centralized config/global.json instead of per-network SAFE addresses. Health checks are updated accordingly, and deployment requirements metadata is augmented with _owner configuration dependencies.

Changes

Global Refund Wallet Configuration

Layer / File(s) Summary
ERC20Proxy owner initialization from global config
script/deploy/facets/DeployERC20Proxy.s.sol, script/deploy/zksync/DeployERC20Proxy.zksync.s.sol, script/deploy/healthCheck.ts
ERC20Proxy deployment scripts add stdJson imports and refactor getConstructorArgs() to read .refundWallet from config/global.json instead of deployerAddress. The view modifier is removed from function signatures. Health checks are updated to expect refundWallet as the owner on both Tron (via checkOwnershipTron) and EVM (direct owner() comparison).
GasZipPeriphery and Permit2Proxy owner initialization from global config
script/deploy/facets/DeployGasZipPeriphery.s.sol, script/deploy/facets/DeployPermit2Proxy.s.sol, script/deploy/zksync/DeployGasZipPeriphery.zksync.s.sol, script/deploy/zksync/DeployPermit2Proxy.zksync.s.sol
Both proxy deployment scripts switch from reading per-network SAFE addresses from config/networks.json to reading .refundWallet from config/global.json. The third constructor argument (owner/withdrawal recipient) is now sourced uniformly from the global configuration using vm.readFile and stdJson.readAddress.
Deployment requirements configuration updates
script/deploy/resources/deployRequirements.json
Executor, ERC20Proxy, and GasZipPeriphery now declare configData._owner dependencies pointing to global.json/.refundWallet with allowToDeployWithZeroAddress set to "false", formalizing the owner-address configuration metadata across the deployment framework.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • lifinance/contracts#907: Both PRs modify script/deploy/healthCheck.ts to adjust ERC20Proxy ownership assertions, with the main PR changing the expected owner from deployerWallet to refundWallet.
  • lifinance/contracts#1574: Both PRs refactor owner/authorization assumptions in deployment workflows; the main PR switches to global refundWallet while PR #1574 removes deployer-wallet verification logic in script/deploy/healthCheck.ts.
  • lifinance/contracts#941: Both PRs modify script/deploy/resources/deployRequirements.json, including Executor configuration updates, with the main PR additionally adding _owner dependencies sourced from global.json/.refundWallet.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: harmonizing Periphery contract ownership to use refundWallet across three contracts and their deploy scripts.
Description check ✅ Passed The PR description provides comprehensive context including policy, affected contracts, changes summary, scope boundaries, and test plan, covering all key template sections except the Linear task reference.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/harmonize-periphery-ownership

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Aligns healthCheck with the policy in this PR. Both the Tron and EVM
paths previously compared the on-chain ERC20Proxy owner against
deployerWallet — switch them to refundWallet.

After this PR, fresh deployments will pass owner verification. Existing
on-chain deployments will keep flagging until the separate ownership
migration runs (the current mainnet ERC20Proxy is owned by a stale
historical deployer EOA 0x11f1..c00c, not deployerWallet, so healthCheck
was already failing on it).
@0xDEnYO 0xDEnYO marked this pull request as ready for review May 18, 2026 05:35
@0xDEnYO 0xDEnYO enabled auto-merge (squash) May 18, 2026 05:35
Comment thread script/deploy/facets/DeployERC20Proxy.s.sol
@lifi-action-bot lifi-action-bot changed the title chore(deploy): harmonize Periphery ownership to refundWallet chore(deploy): harmonize Periphery ownership to refundWallet [ExcessivelySafeCall v1.0.0] May 18, 2026
…mit2Proxy/GasZipPeriphery

Adds the missing `_owner` entries to `deployRequirements.json` so the
pre-deploy validator catches a zero `refundWallet` before the periphery
contracts get deployed bricked. `TransferrableOwnership` does not check
for `address(0)` on the owner, and `ERC20Proxy`/`Permit2Proxy` have no
inline check either — without this guard a missing/renamed
`.refundWallet` key in `global.json` would silently deploy contracts
with `owner = address(0)` and no recovery path.

Per Michał's review on #1815.
@lifi-action-bot lifi-action-bot changed the title chore(deploy): harmonize Periphery ownership to refundWallet [ExcessivelySafeCall v1.0.0] chore(deploy): harmonize Periphery ownership to refundWallet May 19, 2026
Same gap as ERC20Proxy/Permit2Proxy/GasZipPeriphery — Executor's
constructor relies on TransferrableOwnership, which doesn't reject
address(0). The deploy script reads _owner from global.json
.refundWallet, so a missing/renamed key would silently deploy with
owner = address(0).

Per Michał's addendum on #1815.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
script/deploy/healthCheck.ts (1)

721-850: ⚡ Quick win

Add post-deploy ownership checks for GasZipPeriphery and Permit2Proxy.

This PR also moves those two contracts' owner/withdraw authority to refundWallet, but this ownership block still only validates ERC20Proxy, FeeCollector, and Receiver. A wrong non-zero refundWallet on either contract would currently pass healthCheck.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@script/deploy/healthCheck.ts` around lines 721 - 850, The health check omits
post-deploy ownership validation for GasZipPeriphery and Permit2Proxy, so add
the same ownership checks used for ERC20Proxy/FeeCollector/Receiver: call
checkOwnershipTron(...) in the Tron branch and checkOwnership(...) in the
EVM/publicClient branch for the deployedContracts entries 'GasZipPeriphery' and
'Permit2Proxy', verifying their owner/withdraw authority equals refundWallet
(use the same pattern as the existing FeeCollector and Receiver checks and the
erc20Proxy owner read logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@script/deploy/healthCheck.ts`:
- Around line 791-795: The health check now expects the ERC20Proxy owner to be
refundWallet (uses getAddress(erc20ProxyOwner) !== getAddress(refundWallet)),
which will falsely fail existing mainnet deployments; revert this change by
restoring the original expected owner (the old EOA) or wrap the new expectation
in a gated check that only runs when a migration/redeployment marker is set
(e.g., check a feature flag or isMigrationComplete) before comparing
getAddress(erc20ProxyOwner) to getAddress(refundWallet); update the healthCheck
logic around the getAddress/erc20ProxyOwner/refundWallet comparison accordingly
so existing deployments remain unchanged until rollout is completed.

---

Nitpick comments:
In `@script/deploy/healthCheck.ts`:
- Around line 721-850: The health check omits post-deploy ownership validation
for GasZipPeriphery and Permit2Proxy, so add the same ownership checks used for
ERC20Proxy/FeeCollector/Receiver: call checkOwnershipTron(...) in the Tron
branch and checkOwnership(...) in the EVM/publicClient branch for the
deployedContracts entries 'GasZipPeriphery' and 'Permit2Proxy', verifying their
owner/withdraw authority equals refundWallet (use the same pattern as the
existing FeeCollector and Receiver checks and the erc20Proxy owner read logic).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f0d5fab1-2a35-4ee6-ba66-0ba5c1f149c7

📥 Commits

Reviewing files that changed from the base of the PR and between fbf23bd and 96018c5.

📒 Files selected for processing (8)
  • script/deploy/facets/DeployERC20Proxy.s.sol
  • script/deploy/facets/DeployGasZipPeriphery.s.sol
  • script/deploy/facets/DeployPermit2Proxy.s.sol
  • script/deploy/healthCheck.ts
  • script/deploy/resources/deployRequirements.json
  • script/deploy/zksync/DeployERC20Proxy.zksync.s.sol
  • script/deploy/zksync/DeployGasZipPeriphery.zksync.s.sol
  • script/deploy/zksync/DeployPermit2Proxy.zksync.s.sol

Comment thread script/deploy/healthCheck.ts
@0xDEnYO 0xDEnYO merged commit 46a7fe8 into main May 20, 2026
29 checks passed
@0xDEnYO 0xDEnYO deleted the chore/harmonize-periphery-ownership branch May 20, 2026 07:15
mirooon added a commit to lifinance/contracts-tron that referenced this pull request May 20, 2026
Upstream PR lifinance#1815 changed the expected ERC20Proxy owner on every active
network from the Safe to refundWallet. On somnia (newly added in this
sync) the on-chain owner is still the Safe, so the deploy health check
fails on a sync PR that introduces zero protocol code.

Short-term unblock per Slack thread with smartcontract_core (Goran ack):
flip skipHealthcheck=true on somnia only. The proper ownership transfer
to refundWallet is tracked separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mirooon added a commit to lifinance/contracts-tron that referenced this pull request May 21, 2026
Upstream PR lifinance#1815 changed the expected ERC20Proxy owner on every active
network from the Safe to refundWallet. On somnia (newly added in this
sync) the on-chain owner is still the Safe, so the deploy health check
fails on a sync PR that introduces zero protocol code.

Short-term unblock per Slack thread with smartcontract_core (Goran ack):
flip skipHealthcheck=true on somnia only. The proper ownership transfer
to refundWallet is tracked separately.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

4 participants