update deps and add check bytecode hash#13
Conversation
WalkthroughUpdates two git submodule pointers, expands interpreter opcode allow/disallow sets, adds bytecode-validation errors and an internal bytecode-check, introduces a Forge Arbitrum fork helper and tests, and applies assorted test, formatting, .gitignore, and .gas-snapshot edits. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test (caller)
participant Lib as LibExtrospectBytecode
participant EVM as EVM / Account
Test->>Lib: checkCBORTrimmedBytecodeHash(account, expectedHash)
Lib->>EVM: retrieve account bytecode
EVM-->>Lib: bytecode
Lib->>Lib: trimSolidityCBORMetadata(bytecode) -> (trimmed, didTrim)
alt didTrim == false
Lib-->>Test: revert MetadataNotTrimmed()
else didTrim == true
Lib->>Lib: compute keccak256(trimmed) -> actualHash
alt actualHash != expectedHash
Lib-->>Test: revert BytecodeHashMismatch(expectedHash, actualHash)
else actualHash == expectedHash
Lib-->>Test: return success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In `@test/lib/LibExtrospectTestProd.sol`:
- Line 3: Update the Solidity pragma in this file to match the project's exact
version pinning: replace the caret-style pragma declaration (^0.8.25) with the
exact version (=0.8.25) in the top-level pragma statement so it is consistent
with other tests and avoids accidental compiler version drift.
- Around line 10-12: The helper function createSelectForkArbitrum currently
calls vm.envString("RPC_URL_ARBITRUM_FORK") which will revert with a generic
error if the env var is missing; update createSelectForkArbitrum to either use
vm.envOr("RPC_URL_ARBITRUM_FORK", "<placeholder_or_default>") or explicitly
check the result and revert with a clear message like "missing
RPC_URL_ARBITRUM_FORK environment variable" so test failures give a descriptive
error; refer to the createSelectForkArbitrum function and vm.envString/vm.envOr
when making the change.
| @@ -0,0 +1,13 @@ | |||
| // SPDX-License-Identifier: LicenseRef-DCL-1.0 | |||
| // SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd | |||
| pragma solidity ^0.8.25; | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Pragma version inconsistency.
This file uses ^0.8.25 (caret) while other test files in this PR use =0.8.25 (exact version pinning). Consider aligning for consistency.
♻️ Suggested fix
-pragma solidity ^0.8.25;
+pragma solidity =0.8.25;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pragma solidity ^0.8.25; | |
| pragma solidity =0.8.25; |
🤖 Prompt for AI Agents
In `@test/lib/LibExtrospectTestProd.sol` at line 3, Update the Solidity pragma in
this file to match the project's exact version pinning: replace the caret-style
pragma declaration (^0.8.25) with the exact version (=0.8.25) in the top-level
pragma statement so it is consistent with other tests and avoids accidental
compiler version drift.
| function createSelectForkArbitrum(Vm vm) internal { | ||
| vm.createSelectFork(vm.envString("RPC_URL_ARBITRUM_FORK"), PROD_TEST_BLOCK_NUMBER_ARBITRUM); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a descriptive error message if the environment variable is missing.
If RPC_URL_ARBITRUM_FORK is not set, vm.envString will revert with a generic error. This could confuse developers who run tests without the proper environment setup.
♻️ Suggested improvement
function createSelectForkArbitrum(Vm vm) internal {
+ // Will revert if RPC_URL_ARBITRUM_FORK is not set
vm.createSelectFork(vm.envString("RPC_URL_ARBITRUM_FORK"), PROD_TEST_BLOCK_NUMBER_ARBITRUM);
}Alternatively, you could use vm.envOr with a fallback or add a custom revert message, but a simple comment may suffice for test helper code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function createSelectForkArbitrum(Vm vm) internal { | |
| vm.createSelectFork(vm.envString("RPC_URL_ARBITRUM_FORK"), PROD_TEST_BLOCK_NUMBER_ARBITRUM); | |
| } | |
| function createSelectForkArbitrum(Vm vm) internal { | |
| // Will revert if RPC_URL_ARBITRUM_FORK is not set | |
| vm.createSelectFork(vm.envString("RPC_URL_ARBITRUM_FORK"), PROD_TEST_BLOCK_NUMBER_ARBITRUM); | |
| } |
🤖 Prompt for AI Agents
In `@test/lib/LibExtrospectTestProd.sol` around lines 10 - 12, The helper function
createSelectForkArbitrum currently calls vm.envString("RPC_URL_ARBITRUM_FORK")
which will revert with a generic error if the env var is missing; update
createSelectForkArbitrum to either use vm.envOr("RPC_URL_ARBITRUM_FORK",
"<placeholder_or_default>") or explicitly check the result and revert with a
clear message like "missing RPC_URL_ARBITRUM_FORK environment variable" so test
failures give a descriptive error; refer to the createSelectForkArbitrum
function and vm.envString/vm.envOr when making the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/src/concrete/Extrospection.ERC1167Proxy.t.sol`:
- Around line 26-30: The test tightens the precompile exclusion by assuming
vm.assume(uint160(proxy) > type(uint160).max / 2) which reduces fuzz coverage
and diverges from the success test’s check; change the exclusion to match the
success test (e.g., vm.assume(uint160(proxy) > 10)) or introduce and reuse a
shared constant like PRECOMPILE_BOUND used by both tests so both extrospection
checks (the vm.assume for proxy and any corresponding checks in Extrospection
tests) consistently avoid only low precompile addresses while preserving fuzz
coverage.
| // Proxy can't be extrospection, otherwise we'll etch over it. | ||
| vm.assume(proxy != address(extrospection)); | ||
| // Proxy can't be a precompile either. | ||
| vm.assume(uint160(proxy) > 10); | ||
| vm.assume(uint160(proxy) > type(uint160).max / 2); | ||
| // Force incorrect proxy implementation into the proxy address. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align the precompile exclusion criteria between tests.
Line 29 now filters to the upper half of the address space, which sharply narrows fuzz coverage and diverges from the success test’s > 10 check. If the goal is only to avoid precompiles, keep the same explicit boundary (or a shared constant) so low-address edge cases are still exercised consistently.
♻️ Suggested tweak
- vm.assume(uint160(proxy) > type(uint160).max / 2);
+ vm.assume(uint160(proxy) > 10);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Proxy can't be extrospection, otherwise we'll etch over it. | |
| vm.assume(proxy != address(extrospection)); | |
| // Proxy can't be a precompile either. | |
| vm.assume(uint160(proxy) > 10); | |
| vm.assume(uint160(proxy) > type(uint160).max / 2); | |
| // Force incorrect proxy implementation into the proxy address. | |
| // Proxy can't be extrospection, otherwise we'll etch over it. | |
| vm.assume(proxy != address(extrospection)); | |
| // Proxy can't be a precompile either. | |
| vm.assume(uint160(proxy) > 10); | |
| // Force incorrect proxy implementation into the proxy address. |
🤖 Prompt for AI Agents
In `@test/src/concrete/Extrospection.ERC1167Proxy.t.sol` around lines 26 - 30, The
test tightens the precompile exclusion by assuming vm.assume(uint160(proxy) >
type(uint160).max / 2) which reduces fuzz coverage and diverges from the success
test’s check; change the exclusion to match the success test (e.g.,
vm.assume(uint160(proxy) > 10)) or introduce and reuse a shared constant like
PRECOMPILE_BOUND used by both tests so both extrospection checks (the vm.assume
for proxy and any corresponding checks in Extrospection tests) consistently
avoid only low precompile addresses while preserving fuzz coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/src/concrete/Extrospection.ERC1167Proxy.t.sol`:
- Line 36: Remove the leftover debug call console2.logBytes(bytecode) from the
test (Extrospection.ERC1167Proxy.t.sol) so test output stays clean; locate the
call to console2.logBytes(bytecode) and delete it (or replace it with an
appropriate assertion if you intended to validate bytecode), and scan for any
other console2.* debug calls to remove before merging.
♻️ Duplicate comments (3)
test/src/concrete/Extrospection.ERC1167Proxy.t.sol (1)
29-30: Precompile exclusion bound differs from success test.The failure test now uses
uint160(proxy) > type(uint160).max / 2, dramatically limiting fuzz coverage to just the upper half of the address space, while the success test at line 52 uses> 10. If both tests are intended to avoid precompiles, consider using a consistent bound or a shared constant.test/lib/LibExtrospectTestProd.sol (2)
3-3: Pragma version inconsistency.This file uses
^0.8.25while other test files use=0.8.25. Consider aligning for consistency.
10-12: Consider adding clarity for missing environment variable.If
RPC_URL_ARBITRUM_FORKis not set,vm.envStringreverts with a generic error. A comment orvm.envOrwith a descriptive fallback would help developers understand the setup requirement.
1 participant
Motivation
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.