🔨 contracts: avoid redundant deployments#865
Conversation
🦋 Changeset detectedLatest commit: 5cb98c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds preparatory CREATE3-based deployments and cached protocol state to the Redeployer script, replaces Changes
Sequence DiagramsequenceDiagram
participant Test
participant Redeployer
participant Registry
participant CREATE3
participant ProposalManager
participant ExaPlugin
participant ExaFactory
Test->>Redeployer: call prepare()
Redeployer->>Registry: lookup protocol components (auditor/markets/plugins)
alt component exists
Registry-->>Redeployer: return existing address
else missing
Redeployer->>CREATE3: compute salt & CREATE3-deploy stub (rgba(100,150,240,0.5))
CREATE3-->>Redeployer: return stub address
end
Redeployer->>CREATE3: ensure ExaFactory implementation per version (rgba(120,200,140,0.5))
Redeployer-->>Test: prepare completed
Test->>Redeployer: call proxyThrough(targetNonce) or deployExaFactory(version)/deployExaFactory(proxy)
Redeployer->>ExaFactory: deploy/upgrade or reuse implementation
ExaFactory-->>Redeployer: result/status
Redeployer-->>Test: return status/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #865 +/- ##
==========================================
+ Coverage 71.62% 71.71% +0.08%
==========================================
Files 228 228
Lines 8234 8277 +43
Branches 2642 2661 +19
==========================================
+ Hits 5898 5936 +38
- Misses 2106 2111 +5
Partials 230 230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
itofarina
changed the title
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
68-86:⚠️ Potential issue | 🟠 MajorGuard and backfill all plugin dependencies, not just
auditor.Line 68 only backfills when
auditoris zero, and Line 180 only validatesauditor. Ifauditoris set butmarketUSDC/marketWETHare zero, plugin deployment can fail at constructor-time with invalid markets.🔧 Suggested fix
- if (address(auditor) == address(0)) { + if (address(auditor) == address(0)) { auditor = IAuditor( CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAuditor")), vm.getCode("Redeployer.s.sol:StubAuditor")) ); - address stubAsset = - CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAsset")), vm.getCode("Redeployer.s.sol:StubAsset")); + } + + if (address(marketUSDC) == address(0) || address(marketWETH) == address(0)) { + address stubAsset = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("StubAsset"))); + if (stubAsset.code.length == 0) { + stubAsset = + CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAsset")), vm.getCode("Redeployer.s.sol:StubAsset")); + } + + if (address(marketUSDC) == address(0)) { + address stubUSDC = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("StubMarketUSDC"))); + marketUSDC = IMarket( + stubUSDC.code.length == 0 + ? CREATE3_FACTORY.deploy( + keccak256(abi.encode("StubMarketUSDC")), + abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) + ) + : stubUSDC + ); + } + + if (address(marketWETH) == address(0)) { + address stubWETH = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("StubMarketWETH"))); + marketWETH = IMarket( + stubWETH.code.length == 0 + ? CREATE3_FACTORY.deploy( + keccak256(abi.encode("StubMarketWETH")), + abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) + ) + : stubWETH + ); + } - marketUSDC = IMarket( - CREATE3_FACTORY.deploy( - keccak256(abi.encode("StubMarketUSDC")), - abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) - ) - ); - marketWETH = IMarket( - CREATE3_FACTORY.deploy( - keccak256(abi.encode("StubMarketWETH")), - abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) - ) - ); } @@ - if (address(auditor) == address(0)) revert NotPrepared(); + if (address(auditor) == address(0) || address(marketUSDC) == address(0) || address(marketWETH) == address(0)) { + revert NotPrepared(); + }Also applies to: 179-180
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b9ddd18-4e44-47bb-b692-023871716822
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
itofarina
force-pushed
the
redeployer
branch
2 times, most recently
from
681ea16 to
478caf9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
68-86:⚠️ Potential issue | 🟠 MajorHarden readiness checks for partially resolved dependencies.
prepare()and_deployPlugins()gate onauditoronly. Ifauditoris present butmarketUSDC/marketWETHare not, deployment can fail later inExaPluginconstruction instead of reverting withNotPrepared().🔧 Suggested fix
- if (address(auditor).code.length == 0) { + if (address(auditor).code.length == 0) { auditor = IAuditor( CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAuditor")), vm.getCode("Redeployer.s.sol:StubAuditor")) ); + } + if (address(marketUSDC).code.length == 0 || address(marketWETH).code.length == 0) { address stubAsset = CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAsset")), vm.getCode("Redeployer.s.sol:StubAsset")); - marketUSDC = IMarket( - CREATE3_FACTORY.deploy( - keccak256(abi.encode("StubMarketUSDC")), - abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) - ) - ); - marketWETH = IMarket( - CREATE3_FACTORY.deploy( - keccak256(abi.encode("StubMarketWETH")), - abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) - ) - ); + if (address(marketUSDC).code.length == 0) { + marketUSDC = IMarket( + CREATE3_FACTORY.deploy( + keccak256(abi.encode("StubMarketUSDC")), + abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) + ) + ); + } + if (address(marketWETH).code.length == 0) { + marketWETH = IMarket( + CREATE3_FACTORY.deploy( + keccak256(abi.encode("StubMarketWETH")), + abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) + ) + ); + } } @@ - if (address(auditor).code.length == 0) revert NotPrepared(); + if ( + address(auditor).code.length == 0 + || address(marketUSDC).code.length == 0 + || address(marketWETH).code.length == 0 + ) revert NotPrepared();Also applies to: 179-193
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27201171-afe2-4d37-9f59-68bbbe95d8e4
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
179-193:⚠️ Potential issue | 🟠 MajorExpand readiness guard to cover both markets.
_deployPlugins()currently guards onlyauditor, but it immediately consumesmarketUSDCandmarketWETHas constructor inputs. Missing market code can surface as an opaque low-level revert instead ofNotPrepared.🔧 Proposed fix
function _deployPlugins(address admin) internal returns (IPlugin, IPlugin) { - if (address(auditor).code.length == 0) revert NotPrepared(); + if ( + address(auditor).code.length == 0 || address(marketUSDC).code.length == 0 || address(marketWETH).code.length == 0 + ) revert NotPrepared();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b34488d-6631-4754-8b0f-08ec0d9f6758
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/script/Redeployer.s.sol (1)
90-103:⚠️ Potential issue | 🟡 Minor
run()NatSpec says exclusive nonce, but implementation is inclusive.The parameter docs and loop behavior diverge (
<= targetNonce). Please make these consistent to prevent incorrect caller assumptions.📝 Minimal fix (doc alignment)
- /// `@param` targetNonce The nonce to stop at (exclusive) + /// `@param` targetNonce The nonce to stop at (inclusive)
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
68-86:⚠️ Potential issue | 🟠 Major
NotPreparedcan be skipped while markets are still unresolved.
prepare()backfills stubs only whenauditoris missing, and_deployPlugins()only validatesauditor. Ifauditorresolves butmarketUSDC/marketWETHdo not, plugin deployment can fail with a low-level revert instead ofNotPrepared, andprepare()won’t repair that state.🔧 Suggested fix
- if (address(auditor).code.length == 0) { + if (address(auditor).code.length == 0) { auditor = IAuditor( CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAuditor")), vm.getCode("Redeployer.s.sol:StubAuditor")) ); - address stubAsset = - CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAsset")), vm.getCode("Redeployer.s.sol:StubAsset")); + } + + if (address(marketUSDC).code.length == 0 || address(marketWETH).code.length == 0) { + address stubAsset = CREATE3_FACTORY.getDeployed(admin, keccak256(abi.encode("StubAsset"))); + if (stubAsset.code.length == 0) { + stubAsset = CREATE3_FACTORY.deploy(keccak256(abi.encode("StubAsset")), vm.getCode("Redeployer.s.sol:StubAsset")); + } + if (address(marketUSDC).code.length == 0) { marketUSDC = IMarket( CREATE3_FACTORY.deploy( keccak256(abi.encode("StubMarketUSDC")), abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) ) ); + } + if (address(marketWETH).code.length == 0) { marketWETH = IMarket( CREATE3_FACTORY.deploy( keccak256(abi.encode("StubMarketWETH")), abi.encodePacked(vm.getCode("Redeployer.s.sol:StubMarket"), abi.encode(stubAsset)) ) ); + } } @@ - if (address(auditor).code.length == 0) revert NotPrepared(); + if (address(auditor).code.length == 0 || address(marketUSDC).code.length == 0 || address(marketWETH).code.length == 0) { + revert NotPrepared(); + }Also applies to: 179-193
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad9fb905-5aa6-418a-9507-a42a5b07c847
📒 Files selected for processing (3)
contracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
itofarina
changed the title
itofarina
force-pushed
the
redeployer
branch
5 times, most recently
from
76965b3 to
7789af5
Compare
itofarina
force-pushed
the
redeployer
branch
2 times, most recently
from
9345b59 to
13997b4
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
itofarina
force-pushed
the
redeployer
branch
2 times, most recently
from
78d7b91 to
449ada9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/script/Redeployer.s.sol (1)
205-224: 🧹 Nitpick | 🔵 TrivialConsider adding
ownerPluginto theNotPreparedcheck.Line 210 only verifies
exaPlugin.code.length, but line 219 also usesownerPluginin the factory constructor. While the intended flow (prepare()deploysownerPluginbeforeexaPlugin) should guarantee both are available, an explicit check would make the invariant clearer and prevent misuse.🔧 Suggested improvement
function deployExaFactory(string calldata version) external returns (ExaAccountFactory f) { bytes32 salt = keccak256(abi.encode("Exa Plugin", version)); f = ExaAccountFactory(payable(CREATE3_FACTORY.getDeployed(acct("admin"), salt))); if (address(f).code.length != 0) return f; - if (address(exaPlugin).code.length == 0) revert NotPrepared(); + if (address(ownerPlugin).code.length == 0 || address(exaPlugin).code.length == 0) revert NotPrepared();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac128abc-2a58-4311-946b-ef16e079f4d3
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
148-150: 🧹 Nitpick | 🔵 TrivialConsider using
ProposalManager.PROPOSER_ROLEconstant.The inline
keccak256("PROPOSER_ROLE")works but is less maintainable.ProposalManageralready exposes this as a public constant.♻️ Suggested improvement
- if (!ProposalManager(proposalManagerAddr).hasRole(keccak256("PROPOSER_ROLE"), address(exaPlugin))) { - ProposalManager(proposalManagerAddr).grantRole(keccak256("PROPOSER_ROLE"), address(exaPlugin)); + ProposalManager pm = ProposalManager(proposalManagerAddr); + if (!pm.hasRole(pm.PROPOSER_ROLE(), address(exaPlugin))) { + pm.grantRole(pm.PROPOSER_ROLE(), address(exaPlugin)); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e21e6346-9d52-44dc-b3b4-6ec268b11725
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/script/Redeployer.s.sol (1)
198-202:⚠️ Potential issue | 🟠 MajorGuard the upgrade path against undeployed addresses.
Line 202 can still look successful while doing nothing if either
proxyAdminorproxyhas no code. In that statedeployExaFactory(address)— and, downstream,deployExaFactories()— silently no-op instead of surfacing a bad precondition.🔧 Proposed guard
function deployExaFactory(address proxy) external { if (address(factory).code.length == 0) revert NotPrepared(); + if (address(proxyAdmin).code.length == 0) revert ProxyAdminNotDeployed(); + if (proxy.code.length == 0) revert ProxyNotDeployed(); if (address(uint160(uint256(vm.load(proxy, ERC1967Utils.IMPLEMENTATION_SLOT)))) == address(factory)) return; vm.broadcast(acct("admin")); proxyAdmin.upgradeAndCall(ITransparentUpgradeableProxy(proxy), address(factory), ""); } + +error ProxyNotDeployed();In Solidity/OpenZeppelin, does calling a no-return external function on an address with no code revert, or can it succeed with empty returndata? Specifically, how do `ProxyAdmin.upgradeAndCall(...)` and `TransparentUpgradeableProxy.upgradeToAndCall(...)` behave when the `proxyAdmin` or `proxy` address has no code?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d2d24fc-832c-4b3e-ac29-281aab463114
📒 Files selected for processing (4)
.changeset/bumpy-foxes-refuse.mdcontracts/.gas-snapshotcontracts/script/Redeployer.s.solcontracts/test/Redeployer.t.sol
| function deployExaFactories() external { | ||
| this.deployExaFactory(0x8D493AF799162Ac3f273e8918B2842447f702163); | ||
| this.deployExaFactory(0x6E1b5A67adD32E8dC034c23b8022b54821ED297b); | ||
| this.deployExaFactory(0x3427a595eD6E05Cc2D8115e28BAd151cB879616e); | ||
| this.deployExaFactory(0xcbeaAF42Cc39c17e84cBeFe85160995B515A9668); | ||
| this.deployExaFactory("1.0.0"); | ||
| this.deployExaFactory("1.1.0"); | ||
| } |
There was a problem hiding this comment.
🚩 deployExaFactories hardcodes proxy addresses without chain-awareness
The deployExaFactories() function at contracts/script/Redeployer.s.sol:227-234 hardcodes four proxy addresses and two version strings. These addresses are presumably factory proxies deployed on specific chains. If called on a chain where these addresses don't correspond to valid proxies owned by the proxyAdmin, the proxyAdmin.upgradeAndCall inside deployExaFactory(address) would revert. The function doesn't check which chain it's running on, relying on the caller to invoke it on the correct chain.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary by CodeRabbit
Refactor
Tests
Chores