Conversation
|
WalkthroughThis pull request introduces a new ERC1967Clones library in Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/proxy/ERC1967/ERC1967Clones.test.js (2)
36-36: Add a positive non-contract implementation case.
allowNonContractAddress: truedisables the shared rejection test, but this file never replaces it with an assertion that deploying againstthis.nonContractAddressactually succeeds and stores that address in the implementation slot. That is one of the main behavioral deltas ofERC1967Clones, so it should be covered directly.Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/proxy/ERC1967/ERC1967Clones.test.js` at line 36, The test disables the shared rejection for non-contract addresses but never adds the positive path; add a new assertion in the same suite (where shouldBehaveLikeProxy is called with allowNonContractAddress: true) that deploys the proxy/clone pointing at this.nonContractAddress and then reads the implementation slot (via the helper used elsewhere in the tests, e.g., the ERC1967 implementation getter or direct slot read) to assert it equals this.nonContractAddress; ensure the test actually performs the deployment call that was previously skipped and then verifies the stored implementation value.
39-46: Assert the predicted CREATE2 address.The deterministic path currently recovers the clone address from
return$deployDeterministiconly, so the newcomputeAddresshelpers can drift without failing this suite. A fixed-salt case that compares the predicted address to the emitted/deployed address would cover the new API directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/proxy/ERC1967/ERC1967Clones.test.js` around lines 39 - 46, Update the deterministic deployment test to assert the predicted CREATE2 address matches the emitted/deployed address: in the createProxy helper (the async function using this.factory.$deployDeterministic and reading the address from the return$deployDeterministic event) pass a fixed salt (instead of a random one) and call the new computeAddress helper (or factory.computeAddress / computeCreate2Address) with the same implementation and salt to derive the expected address, then assert that this predicted address equals the address recovered from the event before returning the contract instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/proxy/ERC1967/ERC1967Clones.sol`:
- Around line 64-68: The init/template bytecode in ERC1967Clones.sol stores
implementationSlot and returns without emitting the Upgraded(address) event;
update the init code sequences around the mstore calls that write
implementationSlot and implementation (the blocks using implementationSlot and
implementation at the ptr locations shown) to also emit Upgraded(implementation)
before returning. Concretely, add the LOG1 opcode sequence (topic =
keccak256("Upgraded(address)")) that reads the stored implementation value (the
same value put via the implementation variable) and logs it as the event
topic/argument in both places matching the two code blocks (the current
sequences at the ptr offsets and the repeated sequence later), ensuring the
event uses the ERC1967 Upgraded signature and the implementation value already
stored at ptr so the runtime/creation behavior matches ERC1967Proxy.
---
Nitpick comments:
In `@test/proxy/ERC1967/ERC1967Clones.test.js`:
- Line 36: The test disables the shared rejection for non-contract addresses but
never adds the positive path; add a new assertion in the same suite (where
shouldBehaveLikeProxy is called with allowNonContractAddress: true) that deploys
the proxy/clone pointing at this.nonContractAddress and then reads the
implementation slot (via the helper used elsewhere in the tests, e.g., the
ERC1967 implementation getter or direct slot read) to assert it equals
this.nonContractAddress; ensure the test actually performs the deployment call
that was previously skipped and then verifies the stored implementation value.
- Around line 39-46: Update the deterministic deployment test to assert the
predicted CREATE2 address matches the emitted/deployed address: in the
createProxy helper (the async function using this.factory.$deployDeterministic
and reading the address from the return$deployDeterministic event) pass a fixed
salt (instead of a random one) and call the new computeAddress helper (or
factory.computeAddress / computeCreate2Address) with the same implementation and
salt to derive the expected address, then assert that this predicted address
equals the address recovered from the event before returning the contract
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20ab8844-08bf-4c3d-9c26-584d3d11a3b3
📒 Files selected for processing (4)
contracts/proxy/ERC1967/ERC1967Clones.soltest/proxy/ERC1967/ERC1967Clones.test.jstest/proxy/ERC1967/ERC1967Proxy.test.jstest/proxy/Proxy.behaviour.js
| mstore(add(ptr, 0x52), 0x545af43d5f5f3e5f3d91603757fd5bf3) | ||
| mstore(add(ptr, 0x42), implementationSlot) | ||
| mstore(add(ptr, 0x22), 0x60095155f3365f5f375f5f365f7f) | ||
| mstore(add(ptr, 0x14), implementation) | ||
| mstore(ptr, 0x60395f8160225f3973) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Emit Upgraded from the init code.
This template stores the implementation slot and returns without ever logging Upgraded(implementation). That is a meaningful ERC-1967/tooling compatibility gap, and it introduces an extra behavioral difference vs ERC1967Proxy beyond the two called out for this PR. If the bytecode-size tradeoff is intentional, it should be documented explicitly.
Also applies to: 83-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/proxy/ERC1967/ERC1967Clones.sol` around lines 64 - 68, The
init/template bytecode in ERC1967Clones.sol stores implementationSlot and
returns without emitting the Upgraded(address) event; update the init code
sequences around the mstore calls that write implementationSlot and
implementation (the blocks using implementationSlot and implementation at the
ptr locations shown) to also emit Upgraded(implementation) before returning.
Concretely, add the LOG1 opcode sequence (topic =
keccak256("Upgraded(address)")) that reads the stored implementation value (the
same value put via the implementation variable) and logs it as the event
topic/argument in both places matching the two code blocks (the current
sequences at the ptr offsets and the repeated sequence later), ensuring the
event uses the ERC1967 Upgraded signature and the implementation value already
stored at ptr so the runtime/creation behavior matches ERC1967Proxy.
1 participant
These minimal proxy differ from the "normal" ERC1967Proxy:
PR Checklist
npx changeset add)