feat: QoL changes Create3 + foundry.toml #138
Conversation
The `StdStorage` contract was changed as part of this upgrade and hence the slot calculation in the test needs to be upgraded
WalkthroughThis pull request introduces several configuration and testing-related changes across multiple files. The modifications primarily focus on environment variable corrections, workflow adjustments, and test enhancements. Key changes include updating the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (65dc2e0) |
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (65dc2e0) |
|
✅ The Solhint workflow has completed successfully. Check the workflow run for details. (65dc2e0) |
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (65dc2e0) |
- Whenever running `forge script` and other such commands, having an assigned chain_id in the default profile causes problems. Meanwhile, our tests require the chain_id to be set to be that of a known (live) network. To that end, create a dedicated profile to handle this split. - Add RPC endpoints for chains and their explorers (based on env vars) to allow passing `--rpc-url <name>` and `--etherscan-api-key <name>` instead of the URL endpoints. This does not, however, impact any of the scripts that use environment variables to decide the chain.
![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/foundry/DepositWithdrawPrinciple.t.sol (1)
413-417: Remove hardcoding and ensure flexible beacon assignment.The beacon slot usage references an EIP-1967 pattern to store the beacon address. The comment hints at a TODO to load this “dynamically.” Implementing that sooner would reduce the risk of accidental misconfiguration, especially if the beacon address or storage pattern changes. For better clarity, consider abstracting the storage slot logic into a small library or constant that references the EIP and provides a clear explanation.
foundry.toml (1)
39-51: Etherscan configuration might fail if env vars are unset.This is a useful pattern for quick contract verification. However, as noted in the comments, forging can fail when these vars are not present. Consider conditionally loading or mocking these values in your dev environment, or wrap them in a condition so that the config exists only when the environment variables are found.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.env.example(2 hunks).github/workflows/forge-ci.yml(1 hunks)Makefile(1 hunks)foundry.toml(1 hunks)lib/forge-std(1 hunks)script/12_RedeployClientChainGateway.s.sol(1 hunks)script/20_DeployCreate3.s.sol(1 hunks)script/7_DeployBootstrap.s.sol(1 hunks)script/BaseScript.sol(1 hunks)script/TokenTransfer.s.sol(1 hunks)script/integration/1_DeployBootstrap.s.sol(1 hunks)test/foundry/BootstrapDepositNST.t.sol(1 hunks)test/foundry/Delegation.t.sol(0 hunks)test/foundry/DepositThenDelegateTo.t.sol(0 hunks)test/foundry/DepositWithdrawPrinciple.t.sol(1 hunks)test/foundry/ExocoreDeployer.t.sol(0 hunks)test/foundry/unit/Bootstrap.t.sol(0 hunks)test/foundry/unit/ClientChainGateway.t.sol(0 hunks)test/foundry/unit/CustomProxyAdmin.t.sol(0 hunks)test/foundry/unit/ExoCapsule.t.sol(2 hunks)test/foundry/unit/ExocoreGateway.t.sol(0 hunks)
💤 Files with no reviewable changes (7)
- test/foundry/DepositThenDelegateTo.t.sol
- test/foundry/Delegation.t.sol
- test/foundry/ExocoreDeployer.t.sol
- test/foundry/unit/ExocoreGateway.t.sol
- test/foundry/unit/CustomProxyAdmin.t.sol
- test/foundry/unit/ClientChainGateway.t.sol
- test/foundry/unit/Bootstrap.t.sol
✅ Files skipped from review due to trivial changes (3)
- script/TokenTransfer.s.sol
- lib/forge-std
- Makefile
🧰 Additional context used
📓 Learnings (3)
script/12_RedeployClientChainGateway.s.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-11-12T10:03:15.304Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
script/7_DeployBootstrap.s.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#16
File: test/foundry/Bootstrap.t.sol:156-156
Timestamp: 2024-11-12T10:03:15.304Z
Learning: The `setClientChainGatewayLogic` method in `src/core/Bootstrap.sol` now includes checks to ensure the address is non-zero and the initialization data is at least 4 bytes long to include the function selector.
test/foundry/BootstrapDepositNST.t.sol (3)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:193-196
Timestamp: 2024-11-12T10:03:15.304Z
Learning: When using real-world beacon chain validator information in `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 works around the problem of incorrect withdrawal credentials by manipulating the capsule address directly.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:285-300
Timestamp: 2024-11-12T10:03:05.536Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 prefers to use `vm.etch` and direct storage manipulation in tests, instead of deploying the capsule contract with the desired state or using Solidity inheritance and overrides.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:80-282
Timestamp: 2024-11-12T10:03:10.791Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, assigning deployed code to the address representing withdrawal credentials is necessary when real-world data is required.
🪛 actionlint (1.7.4)
.github/workflows/forge-ci.yml
85-85: shellcheck reported issue in this script: SC2209:warning:1:55: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
87-87: shellcheck reported issue in this script: SC2086:info:1:51: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (18)
script/BaseScript.sol (1)
110-110: Use the correctedEXOCORE_TESTNET_RPCenvironment variable consistently.
The fix at line 110 looks good, correcting the variable name from the misspelled one. Ensure that any references to the old variable in other scripts or documentation are removed or updated to avoid confusion.script/20_DeployCreate3.s.sol (7)
1-5: License & imports check.
These initial lines define the SPDX license identifier and import theforge-std/Script.sollibrary. Everything looks standard and correct for Foundry-based deployments.
6-13: Informative docstrings.
The docstrings provide valuable context regarding the script’s purpose and usage, explaining the creation of deterministic factories.
15-28: Globals and constants.
The constants and addresses defined here are critical for reproducible factory deployments across networks. Double-check that the hex addresses match the official references (for instance, the Create2 deployed addresses documented by the library author).
29-31: Empty setUp function.
ThesetUpfunction is intentionally left empty, which is acceptable. If any environment-specific preparations are needed in the future (e.g., loading environment variables), consider adding them here.
33-52: Deployment logic inrun.
The flow correctly checks whether the factories are already deployed before calling their respective deployment functions. This fails gracefully if the factories already exist. Good job.
53-61: FunctiondeployCreate2Factory(): Gas top-up and raw transaction broadcast.
Broadcasting a raw transaction is a neat approach to deploying the Create2 factory. Ensure that debugging logs or error checks are placed in external scripts or logs if issues arise during the raw transaction.
62-69: FunctiondeployCreate3Factory(): Creation via Create2.
Using a static code and salt ensures consistent addresses across chains. Good approach for multi-chain determinism.script/7_DeployBootstrap.s.sol (1)
96-96: Initialization argument update.
Removing the previously unused array argument and simplifying theinitializecall to accept onlyexocoreValidatorSet.addris likely a cleanup of extra parameters. Confirm that theClientChainGateway.initializesignature matches the new call to avoid runtime errors.test/foundry/BootstrapDepositNST.t.sol (1)
290-294: Write to the EIP-1967 beacon slot.
This block manually sets the beacon address in the capsule using the EIP-1967 slot calculation. Ensure the new approach is thoroughly tested for both expected and edge-case addresses. Encouraging that you rely on the standard slot pattern instead of a hardcoded offset.script/integration/1_DeployBootstrap.s.sol (1)
245-245: Confirm no side-effects from removing empty array argument.Here, the initialization now only takes the contract deployer address. Ensure that any logic depending on the removed empty address array is not affected, especially if the original initialization expected or validated a second parameter. If nothing else depends on it, this change should be safe.
test/foundry/unit/ExoCapsule.t.sol (2)
224-224: Validate snapshot usage.
vm.snapshotState()is used above, but this line reverts to that snapshot. If any new state changes occur between the snapshot and this revert, confirm they won't disrupt the tests. Also ensure you revert only when absolutely necessary to avoid confusion in test flows.
233-233: Check revert logic on partial rollback.Reverting to state after pushing and popping from
validatorContaineris correct for test coverage. Confirm that handling ofvalidatorContainerin the next test steps is correct; specifically, ensure your test environment is properly reset and remains consistent for subsequent test cases.foundry.toml (2)
17-21: Good practice to separate profiles for different environments.Using an explicit
[profile.test]with a chain ID of 1 can help unify test assumptions. Double-check whether tests referencing chain-specific logic (like mainnet addresses) are truly correct for all test scenarios, especially if any local or specialized networks are used in certain scripts.
30-38: Convenient approach for specifying RPC endpoints.Defining named endpoints helps standardize usage. Validate at runtime that any environment variables (e.g.,
CLIENT_CHAIN_RPC,EXOCORE_LOCAL_RPC,EXOCORE_TESTNET_RPC) are set. Missing variables could break commands dependent on these endpoints..env.example (1)
3-3: LGTM! Configuration improvements.The changes enhance the configuration accuracy by:
- Fixing the typo in the RPC endpoint name
- Setting mock flags to false, indicating a shift to real endpoints
- Adding a new explorer variable placeholder
Also applies to: 13-14, 18-18
script/12_RedeployClientChainGateway.s.sol (1)
77-77: LGTM! Simplified initialization while maintaining security.The initialization data still includes the function selector and meets the minimum 4-byte requirement for
setClientChainGatewayLogic, while removing unnecessary empty list parameter..github/workflows/forge-ci.yml (1)
84-85: LGTM! Improved CI workflow configuration.Good solution to avoid environment variable complaints by commenting out the etherscan section, and ensuring consistent test environment with FOUNDRY_PROFILE.
🧰 Tools
🪛 actionlint (1.7.4)
85-85: shellcheck reported issue in this script: SC2209:warning:1:55: Use var=$(command) to assign output (or quote to assign string)
(shellcheck)
+ add comment but EIP155
MaxMustermann2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/forge-ci.yml (2)
83-84: Quote the sed command to prevent shell interpretation issues.The sed command should be quoted to prevent potential shell interpretation issues.
- run: sed -i '/\[etherscan\]/,/^\[/ s/^/#/' foundry.toml + run: "sed -i '/\[etherscan\]/,/^\[/ s/^/#/' foundry.toml"
90-93: Quote the GITHUB_STEP_SUMMARY variable to prevent globbing.The
$GITHUB_STEP_SUMMARYshould be quoted to prevent potential globbing and word splitting issues.- run: forge snapshot >> $GITHUB_STEP_SUMMARY + run: forge snapshot >> "$GITHUB_STEP_SUMMARY"🧰 Tools
🪛 actionlint (1.7.4)
93-93: shellcheck reported issue in this script: SC2086:info:1:19: Double quote to prevent globbing and word splitting
(shellcheck)
script/20_DeployCreate3.s.sol (2)
41-61: Enhance error handling in the run function.While the code checks for deployment status, it could benefit from additional error handling:
- Add try-catch around the deployment calls
- Log specific error messages for deployment failures
function run() public { vm.startBroadcast(); + try { if (CREATE2_DESTINATION.code.length == 0) { deployCreate2Factory(); console.log("Deployed create2 factory"); } else { vm.assertEq(CREATE2_DESTINATION.code, CREATE2_RUNTIME_CODE); console.log("Create2 factory already deployed"); } if (CREATE3_DESTINATION.code.length == 0) { deployCreate3Factory(); console.log("Deployed create3 factory"); } else { vm.assertEq(CREATE3_DESTINATION.code, CREATE3_RUNTIME_CODE); console.log("Create3 factory already deployed"); } + } catch Error(string memory reason) { + console.log("Deployment failed:", reason); + revert(reason); + } vm.stopBroadcast(); }
63-70: Optimize gas usage in deployCreate2Factory.The function could be optimized by checking if the deployer needs exactly the required balance:
function deployCreate2Factory() public { - if (CREATE2_DEPLOYER.balance < CREATE2_BALANCE) { + uint256 currentBalance = CREATE2_DEPLOYER.balance; + if (currentBalance < CREATE2_BALANCE) { - payable(CREATE2_DEPLOYER).transfer(CREATE2_BALANCE); + payable(CREATE2_DEPLOYER).transfer(CREATE2_BALANCE - currentBalance); } vm.broadcastRawTransaction(CREATE2_RAW_TRANSACTION); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/forge-ci.yml(1 hunks)script/20_DeployCreate3.s.sol(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/forge-ci.yml
93-93: shellcheck reported issue in this script: SC2086:info:1:19: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (2)
.github/workflows/forge-ci.yml (1)
85-88: LGTM!The test execution step with the
FOUNDRY_PROFILEenvironment variable is well-structured.script/20_DeployCreate3.s.sol (1)
15-17: Verify chain compatibility for pre-EIP155 transactions.The comment warns about pre-EIP155 transactions not working on most chains. Consider adding a verification step to check chain compatibility before deployment.
✅ Verification successful
Let me gather more information about the deployment script and related configurations.
Let me check the deployment script content and related files to understand the deployment context better.
Let me check the README for any deployment-related information.
Based on the gathered information, I can now provide the final response:
Pre-EIP155 transaction handling requires chain-specific deployment strategy
The deployment script uses a pre-EIP155 transaction for the CREATE2 factory deployment, which is explicitly noted as incompatible with most modern chains. The code already implements the correct mitigation strategy by:
- Suggesting chains should add the contract as a predeploy
- Including a check that skips deployment if the CREATE2 factory is already deployed
- Verifying the deployed bytecode matches the expected runtime code
The warning comment is accurate and the implementation properly handles the compatibility concern through these safeguards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the target chain supports pre-EIP155 transactions # Note: This is a basic check and may need to be enhanced based on specific chain requirements # Get the chain ID from the RPC endpoint rg -l "RPC.*=" --glob "*.env*"Length of output: 45
Script:
#!/bin/bash # Check the content of .env.example and search for chain-related configurations cat .env.example # Look for any chain ID or network configurations in the deployment script rg -A 5 "chainId|CHAIN_ID|network" --type sol # Search for any deployment-related configurations or checks ast-grep --pattern 'function run() { $$$ }'Length of output: 3373
Script:
#!/bin/bash # Check the content of the deployment script cat script/20_DeployCreate3.s.sol # Look for any related deployment scripts or configuration files fd -e sol -e json . script/ # Search for any documentation about deployment requirements fd README -t fLength of output: 12205
Script:
#!/bin/bash # Check README content for deployment instructions or chain compatibility information cat README.md # Look for any documentation about chain compatibility or deployment requirements rg -i "chain|eip155|deployment" README.mdLength of output: 6528
| // optimizer enabled | ||
| // optimizer-runs 200 | ||
| // The metadata can be stripped from the runtime bytecode fetched via `eth_getCode` by using | ||
| // the last 2 bytes as the length of the metadata, and then removing that many bytes + 2 |
There was a problem hiding this comment.
can you explain a little about why this logic works?
foundry.tomlto support chains + explorers by name and not just idtestprofile to avoidforge scriptandcast sendcomplaining aboutchain-id mismatchlib/forge-stdto addbroadcastRawTransactionand rename / remove deprecated functionsBeaconProxyBytecode.solto the extent possibleSummary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
.env.exampleChores
Refactor