fix: add correct holesky + sepolia addrs #145
Conversation
MaxMustermann2
requested review from
magj2006,
bwhour,
adu-web3 and
cloud8little
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
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 Solhint workflow has completed successfully. Check the workflow run for details. (ced8e52) |
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (ced8e52) |
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (ced8e52) |
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (ced8e52) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
script/integration/1_DeployBootstrap.s.sol (1)
Line range hint
471-474: Security: Improve random number generation.The current implementation uses
block.timestampandblock.prevrandaofor random number generation, which could be manipulated by validators. While this might be acceptable for testing, it should not be used in production.Consider using a more secure random number generation method:
- function random(uint256 _range) internal view returns (uint256) { - // Basic random number generation; consider a more robust approach for production - return (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % (_range - 1)) + 1; - } + function random(uint256 _range) internal view returns (uint256) { + // For testing only - In production, use a secure randomness source like Chainlink VRF + bytes32 entropy = keccak256(abi.encodePacked( + block.timestamp, + block.prevrandao, + msg.sender, + address(this) + )); + return (uint256(entropy) % (_range - 1)) + 1; + }
🧹 Nitpick comments (1)
script/integration/1_DeployBootstrap.s.sol (1)
Line range hint
67-77: Security: Consider environment variables for test private keys.The script contains hardcoded private keys for testing. While these are Anvil's default keys, it's generally better to load such values from environment variables to prevent accidental commits of real private keys.
Consider refactoring to use environment variables with fallbacks:
- ANVIL_VALIDATORS[0] = uint256(0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80); - ANVIL_VALIDATORS[1] = uint256(0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d); - ANVIL_VALIDATORS[2] = uint256(0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a); + ANVIL_VALIDATORS[0] = uint256(vm.envOr("TEST_VALIDATOR_KEY_1", "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80")); + ANVIL_VALIDATORS[1] = uint256(vm.envOr("TEST_VALIDATOR_KEY_2", "0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d")); + ANVIL_VALIDATORS[2] = uint256(vm.envOr("TEST_VALIDATOR_KEY_3", "0x5de4111afa1a4b94908f83103eb1f1706367c2e68ca870fc3fb9a804cdab365a"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
script/integration/1_DeployBootstrap.s.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup / install
🔇 Additional comments (4)
script/integration/1_DeployBootstrap.s.sol (4)
245-246: LGTM! Helpful comments added.The added comments provide useful guidance for debugging deposit-related issues by checking addresses and validator configuration values.
147-150: Verify the hardcoded signature fallback value.The default signature value is used when the environment variable is not set. Ensure this is a valid test signature and not accidentally copied from production.
✅ Verification successful
✓ The signature is confirmed to be test-specific
The signature is only used in the integration test script and does not appear anywhere else in the codebase, confirming it's a test-specific value and not a production signature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of this signature in the codebase # to ensure it's not used in production configurations rg -l "922a316bdc3516bfa66e88259d5e93e339ef81bc85b70e6c715542222025a28fa1e3644c853beb8c3ba76a2c5c03b726081bf605bde3a16e1f33f902cc1b6c01093c19609de87da9383fa4b1f347bd2d4222e1ae5428727a7896c8e553cc8071"Length of output: 243
142-145: Verify the hardcoded pubkey fallback value.The default pubkey value is used when the environment variable is not set. Ensure this is a valid test key and not accidentally copied from production.
✅ Verification successful
The hardcoded pubkey fallback appears to be a safe test value
The pubkey is isolated to the integration deployment script and isn't used anywhere else in the codebase, suggesting it's specifically designated for testing purposes. The
vm.envOrpattern allows for easy override via environment variables when needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of this pubkey in the codebase # to ensure it's not used in production configurations rg -l "98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9"Length of output: 147
Script:
#!/bin/bash # Search for any documentation or test files mentioning this pubkey rg -t md -t txt -l "98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9" # Check test files fd -e t.sol -x rg -l "98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9" {}Length of output: 242
152-154: Verify the hardcoded depositDataRoot fallback value.The default depositDataRoot value is used when the environment variable is not set. Ensure this is a valid test value and not accidentally copied from production.
✅ Verification successful
✓ Confirmed: This is a valid test-specific depositDataRoot value
The depositDataRoot is correctly used as a default test value in the integration script:
- It's only used in test code and is overridable via the
INTEGRATION_DEPOSIT_DATA_ROOTenvironment variable- The script includes documentation explaining its relationship with specific test addresses
- Production code doesn't contain any hardcoded depositDataRoot values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other occurrences of this depositDataRoot in the codebase # to ensure it's not used in production configurations rg -l "456934ced8f08ff106857418a6d885ba69d31e1b7fab9a931be06da25490cd1d"Length of output: 115
Script:
#!/bin/bash # Search for other depositDataRoot occurrences to understand its usage pattern rg -i "depositDataRoot" -A 2 -B 2Length of output: 7223
| @@ -6,7 +6,7 @@ | |||
| "bootstrapLogic": "0xB97A39004Ba6900FAE801Ac04F8ce4DA70689879", | |||
| "capsuleBeacon": "0xB8D032a30a3B950CBcc6c1689E2381ab4290D4BB", | |||
| "capsuleImplementation": "0x8638502De2001e0dF71BbB5dd503E2008b2Ae948", | |||
| "clientGatewayLogic": "0x92A645a44DFf3e5499F9e6A1d6738520971267AA", | |||
| "clientGatewayLogic": "0x591433Ba4bD17973bbD02F201D4256e42fed141B", | |||
There was a problem hiding this comment.
should we change the key clientChain to sepolia since we have two client chains right now, and jobs in workflow may also need to be updated if the key changed
Summary by CodeRabbit