[Provers M02] Missing Event on Prover Implementation setting on Pointer#84
[Provers M02] Missing Event on Prover Implementation setting on Pointer#84
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 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)
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 |
![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 (1)
src/contracts/StateProverPointer.sol (1)
71-75: Cache_newImplementationAddress.codehashto avoid doubleEXTCODEHASHopcode.
_newImplementationAddress.codehashis evaluated twice: once passed into_setCodeHash(line 71) and again as thenewCodeHashargument to the emit (line 74).♻️ Cache in a local variable
- _implementationAddress = _newImplementationAddress; - _setCodeHash(_newImplementationAddress.codehash); - - emit ImplementationAddressSet( - newVersion, _newImplementationAddress, _newImplementationAddress.codehash, currentImplementationAddress - ); + _implementationAddress = _newImplementationAddress; + bytes32 newCodeHash = _newImplementationAddress.codehash; + _setCodeHash(newCodeHash); + + emit ImplementationAddressSet(newVersion, _newImplementationAddress, newCodeHash, currentImplementationAddress);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 71 - 75, Cache the value of _newImplementationAddress.codehash in a local bytes32 variable and use that variable for both the _setCodeHash call and the ImplementationAddressSet emit to avoid executing EXTCODEHASH twice; e.g., compute bytes32 newCodeHash = _newImplementationAddress.codehash, then call _setCodeHash(newCodeHash) and emit ImplementationAddressSet(newVersion, _newImplementationAddress, newCodeHash, currentImplementationAddress).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/contracts/StateProverPointer.sol`:
- Around line 19-24: Move the event declaration for ImplementationAddressSet
from the concrete StateProverPointer contract into the IStateProverPointer
interface so off-chain consumers using the interface ABI can subscribe to it;
specifically, add the event signature (ImplementationAddressSet(uint256 indexed
newVersion, address newImplementationAddress, bytes32 newCodeHash, address
oldImplementationAddress)) to IStateProverPointer, then remove the duplicate
event declaration from StateProverPointer and keep only the emit
ImplementationAddressSet(...) call where the pointer update occurs.
---
Nitpick comments:
In `@src/contracts/StateProverPointer.sol`:
- Around line 71-75: Cache the value of _newImplementationAddress.codehash in a
local bytes32 variable and use that variable for both the _setCodeHash call and
the ImplementationAddressSet emit to avoid executing EXTCODEHASH twice; e.g.,
compute bytes32 newCodeHash = _newImplementationAddress.codehash, then call
_setCodeHash(newCodeHash) and emit ImplementationAddressSet(newVersion,
_newImplementationAddress, newCodeHash, currentImplementationAddress).
src/contracts/StateProverPointer.sol
Outdated
| event ImplementationAddressSet( | ||
| uint256 indexed newVersion, | ||
| address newImplementationAddress, | ||
| bytes32 newCodeHash, | ||
| address oldImplementationAddress | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
ImplementationAddressSet should be declared in IStateProverPointer.
The event is declared only in the concrete contract. Since the PR's explicit goal is enabling relayers/operators/indexers to detect pointer updates, any off-chain consumer (or on-chain subscriber) that works against the IStateProverPointer ABI will not see this event in its ABI and won't be able to subscribe to it without importing the concrete contract.
♻️ Move event declaration to IStateProverPointer
In IStateProverPointer:
+ event ImplementationAddressSet(
+ uint256 indexed newVersion,
+ address newImplementationAddress,
+ bytes32 newCodeHash,
+ address oldImplementationAddress
+ );In StateProverPointer.sol, remove the duplicate declaration and keep only the emit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/StateProverPointer.sol` around lines 19 - 24, Move the event
declaration for ImplementationAddressSet from the concrete StateProverPointer
contract into the IStateProverPointer interface so off-chain consumers using the
interface ABI can subscribe to it; specifically, add the event signature
(ImplementationAddressSet(uint256 indexed newVersion, address
newImplementationAddress, bytes32 newCodeHash, address
oldImplementationAddress)) to IStateProverPointer, then remove the duplicate
event declaration from StateProverPointer and keep only the emit
ImplementationAddressSet(...) call where the pointer update occurs.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/contracts/StateProverPointer.sol (2)
70-75: Cachecodehashto avoid reading it twice
_newImplementationAddress.codehashis evaluated on both line 71 and line 74 (twoEXTCODEHASHopcode invocations). Caching it in a local variable makes the intent clearer and avoids the redundant read.♻️ Proposed refactor
+ bytes32 newCodeHash = _newImplementationAddress.codehash; _implementationAddress = _newImplementationAddress; - _setCodeHash(_newImplementationAddress.codehash); + _setCodeHash(newCodeHash); emit ImplementationAddressSet( - newVersion, _newImplementationAddress, _newImplementationAddress.codehash, currentImplementationAddress + newVersion, _newImplementationAddress, newCodeHash, currentImplementationAddress );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 70 - 75, Cache the codehash of _newImplementationAddress in a local variable to avoid calling _newImplementationAddress.codehash twice; compute e.g. bytes32 newImplCodeHash = _newImplementationAddress.codehash once, call _setCodeHash(newImplCodeHash) and use newImplCodeHash in the emit ImplementationAddressSet(newVersion, _newImplementationAddress, newImplCodeHash, currentImplementationAddress) while keeping the assignment _implementationAddress = _newImplementationAddress unchanged.
19-24: MoveImplementationAddressSettoIStateProverPointerThe event is declared only in the concrete contract. Off-chain indexers and relayers that subscribe to events via the
IStateProverPointerABI (which is the public contract surface) won't find the event definition there. The PR's stated goal is to let relayers/operators detect upgrades via on-chain signals, so the event belongs in the interface.♻️ Suggested change
In
IStateProverPointer:+ event ImplementationAddressSet( + uint256 indexed newVersion, + address newImplementationAddress, + bytes32 newCodeHash, + address oldImplementationAddress + );In
StateProverPointer.sol:- event ImplementationAddressSet( - uint256 indexed newVersion, - address newImplementationAddress, - bytes32 newCodeHash, - address oldImplementationAddress - );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/StateProverPointer.sol` around lines 19 - 24, The ImplementationAddressSet event is declared only in StateProverPointer but should be part of the public ABI; move the event declaration into the IStateProverPointer interface and remove the duplicate from StateProverPointer so the interface exposes the event to off‑chain indexers. Ensure the event signature (ImplementationAddressSet(uint256 indexed newVersion, address newImplementationAddress, bytes32 newCodeHash, address oldImplementationAddress)) is identical in IStateProverPointer, update any imports or references if needed, and run/update tests that assert the event is emitted by StateProverPointer (no other code changes required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/Receiver.ethereum.t.sol`:
- Around line 135-139: The test uses the incorrect function-call syntax
parentToChildProver.codehash() when emitting and asserting
StateProverPointer.ImplementationAddressSet; change all occurrences to the
property access parentToChildProver.codehash (no parentheses) so the emitted
event and the call to
stateProverPointer.setImplementationAddress(address(parentToChildProver))
compare the correct codehash value; update the expectEmit/emit line referencing
StateProverPointer.ImplementationAddressSet accordingly.
---
Nitpick comments:
In `@src/contracts/StateProverPointer.sol`:
- Around line 70-75: Cache the codehash of _newImplementationAddress in a local
variable to avoid calling _newImplementationAddress.codehash twice; compute e.g.
bytes32 newImplCodeHash = _newImplementationAddress.codehash once, call
_setCodeHash(newImplCodeHash) and use newImplCodeHash in the emit
ImplementationAddressSet(newVersion, _newImplementationAddress, newImplCodeHash,
currentImplementationAddress) while keeping the assignment
_implementationAddress = _newImplementationAddress unchanged.
- Around line 19-24: The ImplementationAddressSet event is declared only in
StateProverPointer but should be part of the public ABI; move the event
declaration into the IStateProverPointer interface and remove the duplicate from
StateProverPointer so the interface exposes the event to off‑chain indexers.
Ensure the event signature (ImplementationAddressSet(uint256 indexed newVersion,
address newImplementationAddress, bytes32 newCodeHash, address
oldImplementationAddress)) is identical in IStateProverPointer, update any
imports or references if needed, and run/update tests that assert the event is
emitted by StateProverPointer (no other code changes required).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/contracts/interfaces/IStateProverPointer.sol (2)
11-17: Event addition is correct and well-structured — consider indexingnewImplementationAddress.The core fix is solid. One ergonomic gap: relayers and indexers whose primary task is detecting a specific prover's upgrade will want to filter logs by implementation address, but
newImplementationAddressis not indexed.newVersionalone forces consumers to scan allImplementationAddressSetevents across all pointer contracts rather than issuing a targeted filter by address.Since only one of the three available
indexedslots is used, adding a second index here costs nothing:♻️ Suggested tweak
event ImplementationAddressSet( uint256 indexed newVersion, - address newImplementationAddress, + address indexed newImplementationAddress, bytes32 newCodeHash, address oldImplementationAddress );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/interfaces/IStateProverPointer.sol` around lines 11 - 17, The event ImplementationAddressSet should index the newImplementationAddress to allow log filtering by implementation; update the event declaration for ImplementationAddressSet to add the indexed keyword to the newImplementationAddress parameter so relayers/indexers can efficiently filter by that address while preserving existing fields (newVersion, newCodeHash, oldImplementationAddress).
11-17:setImplementationAddressis absent from the interface.The interface declares the event emitted by
setImplementationAddressbut not the function itself. Consumers ofIStateProverPointercan observe the event ABI but cannot call the mutation through this interface, which may be intentional (access-controlled admin function) or an oversight. If any cross-chain tooling or governance contract needs to invoke the setter polymorphically via this interface, consider adding the declaration:/// `@notice` Set a new implementation address. function setImplementationAddress(address newImplementation) external;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contracts/interfaces/IStateProverPointer.sol` around lines 11 - 17, The interface IStateProverPointer currently only declares the ImplementationAddressSet event but omits the setter function; add a declaration for function setImplementationAddress(address newImplementation) external (with an appropriate NatSpec line like "/// `@notice` Set a new implementation address.") so callers can invoke the mutation polymorphically; ensure the function name setImplementationAddress and the event ImplementationAddressSet remain consistent with the implementing contract's signature and access control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/contracts/interfaces/IStateProverPointer.sol`:
- Around line 11-17: The event ImplementationAddressSet should index the
newImplementationAddress to allow log filtering by implementation; update the
event declaration for ImplementationAddressSet to add the indexed keyword to the
newImplementationAddress parameter so relayers/indexers can efficiently filter
by that address while preserving existing fields (newVersion, newCodeHash,
oldImplementationAddress).
- Around line 11-17: The interface IStateProverPointer currently only declares
the ImplementationAddressSet event but omits the setter function; add a
declaration for function setImplementationAddress(address newImplementation)
external (with an appropriate NatSpec line like "/// `@notice` Set a new
implementation address.") so callers can invoke the mutation polymorphically;
ensure the function name setImplementationAddress and the event
ImplementationAddressSet remain consistent with the implementing contract's
signature and access control.
…to fix/provers-M02
2 participants
StateProverPointer.setImplementationAddress() updates the stored implementation address and code hash, but does not emit an event. In this system, prover updates on the source chain must be mirrored on destination chains by deploying a matching StateProverCopy and calling Receiver.updateStateProverCopy so the cached copy’s codehash matches the expected remote value.
Without an event, relayers/operators/indexers have no reliable on-chain signal to detect that the prover has been upgraded. They must instead continuously poll state or perform trace/storage-diff monitoring to notice that the pointer slot changed. This increases the chance of missed or delayed updates across chains.
This becomes especially problematic during security response. If the previous prover version is discovered to be vulnerable and the owner upgrades the pointer to a patched prover on the source chain, destination chains may continue verifying proofs using their cached (old) StateProverCopy until operators detect the change and update the copy. That unnecessarily extends the window in which the known-vulnerable prover remains usable on the other chains.
This PR adds an event to the StateProverPointer contract to be emitted when the
setImplementationAddressis called.Summary by CodeRabbit
New Features
Tests