Conversation
…from superchain ops.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c2a55e39c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| revert OPContractsManagerUpgrader_SuperchainConfigMismatch(); | ||
| } else if (_upgradeSuperchainConfig) { | ||
| // Celo: some chains (like Celo Mainnet) follow external superchain config that is not desired to be upgraded | ||
| __upgradeSuperchainConfig(superchainConfig, _opChainConfigs[i].proxyAdmin); |
There was a problem hiding this comment.
When _upgradeSuperchainConfig is true, this call passes each chain’s proxyAdmin into __upgradeSuperchainConfig, but that function upgrades the shared SuperchainConfig proxy and expects the Superchain proxy admin. In deployments where chain ProxyAdmins differ from the Superchain ProxyAdmin, the upgrade path will revert and abort the entire upgrade(...) call.
Useful? React with 👍 / 👎.
| if (!SemverComp.lt(superchainConfig.version(), ISuperchainConfig(impls.superchainConfigImpl).version())) { | ||
| revert OPContractsManagerUpgrader_SuperchainConfigMismatch(); | ||
| } else if (_upgradeSuperchainConfig) { |
There was a problem hiding this comment.
This loop checks that superchainConfig.version() < target for every chain, then conditionally upgrades SuperchainConfig inside the same loop. If multiple entries reference the same underlying SuperchainConfig and _upgradeSuperchainConfig is enabled, the first iteration upgrades it and the next iteration fails this < target check, reverting the whole batched chain upgrade.
Useful? React with 👍 / 👎.
| _errors = internalRequire(_sysCfg.operatorFeeConstant() == 0, "SYSCON-120", _errors); | ||
| _errors = internalRequire(getProxyAdmin(address(_sysCfg)) == _admin, "SYSCON-130", _errors); | ||
| _errors = internalRequire(_sysCfg.superchainConfig() == superchainConfig, "SYSCON-140", _errors); | ||
| _errors = internalRequire(celoSuperchainConfig(address(_sysCfg.superchainConfig())) == superchainConfig, "SYSCON-140", _errors); |
There was a problem hiding this comment.
This now assumes _sysCfg.superchainConfig() is a Celo wrapper implementing IHasSuperchainConfig, then dereferences one level deeper. On standard deployments where SystemConfig.superchainConfig() already returns the actual SuperchainConfig, that extra call is made against a contract that does not expose superchainConfig(), causing validate(...) to revert instead of reporting validation errors.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4f3ffdb5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| revert OPContractsManagerUpgrader_SuperchainConfigMismatch(); | ||
| } else if (_upgradeSuperchainConfig) { | ||
| // Celo: some chains (like Celo Mainnet) follow external superchain config that is not desired to be upgraded | ||
| __upgradeSuperchainConfig(superchainConfig, _opChainConfigs[i].proxyAdmin); |
There was a problem hiding this comment.
When _upgradeSuperchainConfig is true, the loop upgrades superchainConfig using _opChainConfigs[i].proxyAdmin, but that field is the per-chain ProxyAdmin, not necessarily the shared SuperchainConfig ProxyAdmin. In deployments where those admins differ (the normal topology with one shared superchain config and per-chain admins), this path will revert during upgradeTo, so the new upgrade(..., true) mode cannot be used even for a valid chain config.
Useful? React with 👍 / 👎.
| if (!SemverComp.lt(superchainConfig.version(), ISuperchainConfig(impls.superchainConfigImpl).version())) { | ||
| revert OPContractsManagerUpgrader_SuperchainConfigMismatch(); | ||
| } else if (_upgradeSuperchainConfig) { |
There was a problem hiding this comment.
This version gate runs on every iteration and requires superchainConfig.version() < target. If _upgradeSuperchainConfig is enabled, the first chain upgrades the shared superchain config, then the next chain sees an equal version and immediately reverts with OPContractsManagerUpgrader_SuperchainConfigMismatch. Because the superchain config is shared across chains, this breaks multi-chain upgrades whenever _opChainConfigs has more than one entry.
Useful? React with 👍 / 👎.
2 participants
No description provided.