[Provers N07] Constants not using UPPER_CASE format#77
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request standardizes naming conventions across all prover contracts by renaming public constants and immutable state variables from camelCase to UPPER_SNAKE_CASE. All internal references and corresponding test cases are updated consistently. No functional behavior changes are introduced. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
🚥 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
🤖 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/provers/arbitrum/ParentToChildProver.sol`:
- Around line 14-31: The ABI changed because immutable state variables were
renamed to OUTBOX, ROOTS_SLOT, HOME_CHAIN_ID which changes their autogenerated
getter names and breaks the pre-deployed contract; fix by restoring ABI
compatibility either by renaming the variables back to their original camelCase
names (outbox, rootsSlot, homeChainId) or by adding explicit public view getter
functions named outbox(), rootsSlot(), and homeChainId() that return the
corresponding immutable values (OUTBOX, ROOTS_SLOT, HOME_CHAIN_ID) so callers
and test/Receiver.t.sol can still use the old getters; update whichever approach
you choose across the constructor assignments and references to these symbols.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/contracts/provers/arbitrum/ChildToParentProver.solsrc/contracts/provers/arbitrum/ParentToChildProver.solsrc/contracts/provers/linea/ChildToParentProver.solsrc/contracts/provers/linea/ParentToChildProver.solsrc/contracts/provers/optimism/ChildToParentProver.solsrc/contracts/provers/optimism/ParentToChildProver.solsrc/contracts/provers/scroll/ChildToParentProver.solsrc/contracts/provers/scroll/ParentToChildProver.solsrc/contracts/provers/taiko/ChildToParentProver.solsrc/contracts/provers/taiko/ParentToChildProver.solsrc/contracts/provers/zksync/ChildToParentProver.solsrc/contracts/provers/zksync/ParentToChildProver.soltest/Receiver.t.soltest/VerifyBroadcastMessageBenchmark.t.soltest/provers/linea/ParentToChildProver.t.soltest/provers/scroll/ParentToChildProver.t.soltest/provers/taiko/ChildToParentProver.t.soltest/provers/taiko/ParentToChildProver.t.sol
| address public immutable OUTBOX; | ||
| /// @dev Storage slot the Outbox contract uses to store roots. | ||
| /// Should be set to 3 unless the outbox contract has been modified. | ||
| /// See https://github.com/OffchainLabs/nitro-contracts/blob/9d0e90ef588f94a9d2ffa4dc22713d91a76f57d4/src/bridge/AbsOutbox.sol#L32 | ||
| uint256 public immutable rootsSlot; | ||
| uint256 public immutable ROOTS_SLOT; | ||
|
|
||
| /// @dev The chain ID of the home chain (where this prover reads from). | ||
| uint256 public immutable homeChainId; | ||
| uint256 public immutable HOME_CHAIN_ID; | ||
|
|
||
| error CallNotOnHomeChain(); | ||
| error CallOnHomeChain(); | ||
| error TargetBlockHashNotFound(); | ||
|
|
||
| constructor(address _outbox, uint256 _rootsSlot, uint256 _homeChainId) { | ||
| outbox = _outbox; | ||
| rootsSlot = _rootsSlot; | ||
| homeChainId = _homeChainId; | ||
| OUTBOX = _outbox; | ||
| ROOTS_SLOT = _rootsSlot; | ||
| HOME_CHAIN_ID = _homeChainId; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the actual code in ParentToChildProver.sol
cat -n src/contracts/provers/arbitrum/ParentToChildProver.sol | head -40Repository: openintentsframework/broadcaster
Length of output: 2338
🏁 Script executed:
# Search for remaining references to old getter names across the codebase
rg -n --type=sol -e '\boutbox\(\)' -e '\brootsSlot\(\)' -e '\bhomeChainId\(\)' \
-e '\bblockHashBuffer\(\)' -e '\bblockHashMappingSlot\(\)' \
-e '\bgatewayZkChain\(\)' -e '\bl2LogsRootHashSlot\(\)' \
-e '\bchildChainId\(\)' -e '\bgatewayChainId\(\)' \
-e '\bl1BlockPredeploy\(\)' -e '\bsignalService\(\)' \
-e '\bcheckpointsSlot\(\)' -e '\blineaRollup\(\)' \
-e '\bstateRootHashesSlot\(\)'Repository: openintentsframework/broadcaster
Length of output: 103
🏁 Script executed:
# Check if test/Receiver.t.sol exists and contains the reference mentioned
if [ -f test/Receiver.t.sol ]; then
echo "=== test/Receiver.t.sol exists ==="
grep -n "ON_CHAIN_ARB_PROVER" test/Receiver.t.sol
else
echo "test/Receiver.t.sol does not exist"
fiRepository: openintentsframework/broadcaster
Length of output: 267
🏁 Script executed:
# Search for any TypeScript/JavaScript files that might be calling these getters
fd -e ts -e js -e tsx -e jsx | head -20Repository: openintentsframework/broadcaster
Length of output: 197
🏁 Script executed:
# Search for references to OUTBOX, ROOTS_SLOT, HOME_CHAIN_ID in all files
rg -n "OUTBOX|ROOTS_SLOT|HOME_CHAIN_ID" --max-count=50 | head -50Repository: openintentsframework/broadcaster
Length of output: 5161
🏁 Script executed:
# Search TypeScript/JavaScript files for any calls to getter functions
rg -e "\.outbox\(\)" -e "\.rootsSlot\(\)" -e "\.homeChainId\(\)" -e "\.OUTBOX\(\)" -e "\.ROOTS_SLOT\(\)" -e "\.HOME_CHAIN_ID\(\)"Repository: openintentsframework/broadcaster
Length of output: 1065
🏁 Script executed:
# Check git history to confirm the rename happened
cd src/contracts/provers/arbitrum && git log --oneline -n 20 ParentToChildProver.sol 2>/dev/null || echo "Git history not available"Repository: openintentsframework/broadcaster
Length of output: 94
🏁 Script executed:
# Look for contract deployment or interaction scripts that might call these getters
fd -e ts -e js | xargs grep -l "ParentToChildProver\|OUTBOX\|ROOTS_SLOT" 2>/dev/null | head -10Repository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Check wagmi.config.js and hardhat.config.ts for contract ABI definitions
cat wagmi.config.js 2>/dev/null | head -50Repository: openintentsframework/broadcaster
Length of output: 279
🏁 Script executed:
# Search for any ABIs or contract interfaces that might be affected
fd -e json -e ts | xargs grep -l "outbox\|rootsSlot\|homeChainId" 2>/dev/null | head -10Repository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Check if the old names appear in any branch or commit message
git log --all --oneline --grep="outbox\|rootsSlot\|homeChainId" 2>/dev/null | head -5Repository: openintentsframework/broadcaster
Length of output: 291
🏁 Script executed:
# Search for any README or documentation that might mention deployment or API stability
fd -e md -e txt | xargs grep -l "deployment\|API\|breaking\|getter" 2>/dev/null | head -5Repository: openintentsframework/broadcaster
Length of output: 190
🏁 Script executed:
# Look for any .gitignore patterns that might hide SDK/ABI artifacts
cat .gitignore 2>/dev/null | grep -i "abi\|sdk\|dist\|build"Repository: openintentsframework/broadcaster
Length of output: 89
🏁 Script executed:
# Check the git commit that added homeChainId to see if it shows the naming pattern
git show 37b9d68 --stat 2>/dev/null | head -20Repository: openintentsframework/broadcaster
Length of output: 577
🏁 Script executed:
# Check the documentation to see if it documents the getter function names
cat docs/provers/ARBITRUM.md | head -100Repository: openintentsframework/broadcaster
Length of output: 4109
🏁 Script executed:
# Look for any ABI files or interface definitions in the codebase
fd -e json -path "*/abi*" -o -path "*/artifacts*" | head -20Repository: openintentsframework/broadcaster
Length of output: 244
🏁 Script executed:
# Search for IStateProver interface to understand the contract structure
cat -n src/contracts/interfaces/IStateProver.sol 2>/dev/null | head -50Repository: openintentsframework/broadcaster
Length of output: 3934
🏁 Script executed:
# Check the previous version of ParentToChildProver.sol to see the old naming
git show 37b9d68~1:src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null || echo "Cannot retrieve previous version"Repository: openintentsframework/broadcaster
Length of output: 4104
🏁 Script executed:
# Look for any JSON ABIs or exported interface definitions
find . -name "*.json" -type f | xargs grep -l "OUTBOX\|ROOTS_SLOT\|outbox\|rootsSlot" 2>/dev/null | head -10Repository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Check the full diff of commit 37b9d68 for ParentToChildProver.sol
git show 37b9d68 -- src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null | head -80Repository: openintentsframework/broadcaster
Length of output: 2946
🏁 Script executed:
# Check if there's a more recent commit that renamed variables to UPPER_SNAKE_CASE
git log --all --oneline -- src/contracts/provers/arbitrum/ParentToChildProver.sol | head -10Repository: openintentsframework/broadcaster
Length of output: 430
🏁 Script executed:
# Get the full diff showing the transition from camelCase to UPPER_SNAKE_CASE
git log -p --all -- src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null | grep -A 20 "address public immutable" | head -40Repository: openintentsframework/broadcaster
Length of output: 58
🏁 Script executed:
# Check the most recent commits to see when variables were renamed to UPPER_SNAKE_CASE
git show e93f65d:src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null | head -35Repository: openintentsframework/broadcaster
Length of output: 1888
🏁 Script executed:
# Check commit 3b4b69d to see if it involved variable naming changes
git show 3b4b69d -- src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null | head -50Repository: openintentsframework/broadcaster
Length of output: 2382
🏁 Script executed:
# Check if there's a commit message or change that explains the renaming
git log --all --oneline --grep="UPPER\|snake\|naming\|convention" 2>/dev/nullRepository: openintentsframework/broadcaster
Length of output: 128
🏁 Script executed:
# Look at the diff between HEAD and commit 37b9d68 to see all changes
git diff 37b9d68 HEAD -- src/contracts/provers/arbitrum/ParentToChildProver.sol 2>/dev/null | head -100Repository: openintentsframework/broadcaster
Length of output: 5119
ABI breaking change confirmed — pre-deployed contract at 0x2aA03593adf471d6dd7c041984e6D7D6045E373f no longer compatible with this code.
Renaming public immutable state variables from camelCase to UPPER_SNAKE_CASE changes the auto-generated getter function signatures: outbox() → OUTBOX(), rootsSlot() → ROOTS_SLOT(), homeChainId() → HOME_CHAIN_ID().
The contract instance referenced in test/Receiver.t.sol (0x2aA03593adf471d6dd7c041984e6D7D6045E373f) is already deployed on-chain with the old ABI. If this contract is called via its getters from off-chain tooling, scripts, or other contracts, those calls will fail with the new getter names. Confirm whether this contract will be redeployed, upgraded via proxy, or if callers have been updated to use the new getter names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/contracts/provers/arbitrum/ParentToChildProver.sol` around lines 14 - 31,
The ABI changed because immutable state variables were renamed to OUTBOX,
ROOTS_SLOT, HOME_CHAIN_ID which changes their autogenerated getter names and
breaks the pre-deployed contract; fix by restoring ABI compatibility either by
renaming the variables back to their original camelCase names (outbox,
rootsSlot, homeChainId) or by adding explicit public view getter functions named
outbox(), rootsSlot(), and homeChainId() that return the corresponding immutable
values (OUTBOX, ROOTS_SLOT, HOME_CHAIN_ID) so callers and test/Receiver.t.sol
can still use the old getters; update whichever approach you choose across the
constructor assignments and references to these symbols.
|
I updated the PR so only the constants are upper case, not the immutable variables. |
2 participants
Summary by CodeRabbit