feat(generate): native restaking genesis #123
Conversation
A lot of the parameters such as the beacon genesis time, the deneb time and the deposit address are dependent on the network chosen for deployment. This change allows a library to provide these constants, based on the `block.chainid`. Additionally, it creates the possibility of using a separately deployed contract for integration networks (refer to the next commit), for which, the parameters can be dynamically provided via `INetworkConfig`. As a side effect, the default chain_id used in tests has been changed to let `NetworkConstants` library provide these constants during tests.
WalkthroughThe pull request introduces significant changes primarily focused on enhancing integration testing for Ethereum Proof of Stake scenarios. Key modifications include updates to the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (9)src/storage/ExoCapsuleStorage.sol (5)
The imports of
The expanded comments provide a clear explanation of the
The addition of the immutable
The constructor correctly sets the
The new internal functions ( src/core/ExoCapsule.sol (4)
The constructor now accepts a
The use of
By passing the dynamic Deneb hard fork timestamp to
The 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
🧹 Outside diff range and nitpick comments (45)
foundry.toml (1)
16-17: Consider making chain_id configurable for different test environmentsWhile setting
chain_id = 1makes the NetworkConstants work, hard-coding it to mainnet might limit testing flexibility. Consider making this configurable through environment variables, similar to how RPC endpoints are handled, to facilitate testing across different networks.Example approach:
-chain_id = 1 +chain_id = "${CHAIN_ID}".env.example (3)
25-25: Add description for BEACON_CHAIN_ENDPOINTThe purpose and expected format of this endpoint should be documented with a comment, similar to other variables in the file.
-BEACON_CHAIN_ENDPOINT= +# Beacon chain HTTP endpoint for querying validator data +BEACON_CHAIN_ENDPOINT=
34-35: Add format specifications for timestampsThe timestamp fields
INTEGRATION_BEACON_GENESIS_TIMESTAMPandINTEGRATION_DENEB_TIMESTAMPshould include comments specifying the expected format (Unix timestamp, ISO 8601, etc.).-INTEGRATION_BEACON_GENESIS_TIMESTAMP= -INTEGRATION_DENEB_TIMESTAMP= +# Unix timestamp for beacon chain genesis +INTEGRATION_BEACON_GENESIS_TIMESTAMP= +# Unix timestamp for Deneb hard fork +INTEGRATION_DENEB_TIMESTAMP=
30-36: Consider grouping related environment variablesThe integration testing variables could be better organized by grouping them under a clear section header and including a description of their collective purpose.
-# These are used for integration testing ETH PoS +# ============================================================================= +# ETH PoS Integration Testing Configuration +# These variables are used to configure the environment for Ethereum PoS +# integration testing, including deposit contract, timing parameters, and +# network upgrade specifications. +# =============================================================================script/integration/deposit.sh (1)
25-31: Improve environment variable naming and documentationWhile the variables are set correctly, consider these improvements for clarity and maintainability:
- Add validation for timestamp values
- Document the expected format and units of each variable
- Consider prefixing variables with a more specific namespace
Consider applying these improvements:
+# Validate timestamp +if ! [[ "$timestamp" =~ ^[0-9]+$ ]]; then + echo "Error: Invalid genesis timestamp" + exit 1 +fi + +# Environment variables for integration testing +# All timestamps are in Unix epoch seconds +# INTEGRATION_BEACON_GENESIS_TIMESTAMP: The genesis time of the beacon chain +# INTEGRATION_DENEB_TIMESTAMP: The activation time of the Deneb fork +# INTEGRATION_SECONDS_PER_SLOT: Number of seconds per slot +# INTEGRATION_SLOTS_PER_EPOCH: Number of slots per epoch +# INTEGRATION_DEPOSIT_ADDRESS: The deposit contract address export INTEGRATION_BEACON_GENESIS_TIMESTAMP=$timestamp export INTEGRATION_DENEB_TIMESTAMP=$timestamp export INTEGRATION_SECONDS_PER_SLOT=$seconds_per_slot export INTEGRATION_SLOTS_PER_EPOCH=$slots_per_epoch export INTEGRATION_DEPOSIT_ADDRESS=$deposit_address -export SENDER=$sender +export INTEGRATION_SENDER=$sendersrc/interfaces/INetworkConfig.sol (3)
10-32: Consider adding validation requirements in documentation.While the interface methods are well-defined, consider adding
@devdocumentation notes specifying that the implementing contract should:
- Ensure returned values are non-zero where appropriate
- Validate that
getSecondsPerEpoch()returns value equal togetSecondsPerSlot() * getSlotsPerEpoch()
36-47: Optimize struct field ordering and complete documentation.
- Consider reordering fields to optimize for storage packing:
struct NetworkParams { + uint64 slotsPerEpoch; + uint64 secondsPerSlot; address depositContractAddress; uint256 denebHardForkTimestamp; - uint64 slotsPerEpoch; - uint64 secondsPerSlot; uint256 beaconGenesisTimestamp; }
- Add documentation for the
beaconGenesisTimestampparameter:/// @param depositContractAddress The address of the deposit contract for the network. /// @param denebHardForkTimestamp The timestamp of the Deneb hard fork for the network. /// @param slotsPerEpoch The number of slots in an epoch for the network. /// @param secondsPerSlot The number of seconds in a slot for the network. +/// @param beaconGenesisTimestamp The genesis timestamp of the beacon chain.
1-47: Well-designed interface that addresses PR objectives.This interface effectively solves the issue mentioned in the PR objectives regarding the use of same constants across different networks. By providing a configurable interface for network parameters, it allows for network-specific values of critical constants like deposit contract address and genesis time.
Consider adding a network identifier or chain ID parameter to prevent configuration mismatches across different networks.
script/integration/prove.sh (2)
37-46: Consider enabling the slot waiting logic.The commented-out slot waiting logic could be useful for ensuring proper synchronization with the beacon chain. However, it needs proper error handling and timeout mechanisms.
Consider implementing this with proper safeguards:
+# Wait till the slot is reached (with timeout) +seconds_per_slot=$(jq -r .data.SECONDS_PER_SLOT "$SCRIPT_DIR/spec.json") +timeout=300 # 5 minutes timeout +elapsed=0 + +while [ $elapsed -lt $timeout ]; do + response=$(curl -s http://localhost:3500/eth/v1/beacon/headers) + if [ $? -ne 0 ]; then + echo "Error: Failed to fetch current slot" + exit 1 + fi + + current_slot=$(echo "$response" | jq -r '.data[0].header.message.slot') + if [ $? -ne 0 ] || [ -z "$current_slot" ]; then + echo "Error: Failed to parse current slot" + exit 1 + } + + if (( current_slot > slot )); then + break + fi + + echo "Waiting for slot $slot, current slot is $current_slot" + sleep "$seconds_per_slot" + elapsed=$((elapsed + seconds_per_slot)) +done + +if [ $elapsed -ge $timeout ]; then + echo "Error: Timeout waiting for slot $slot" + exit 1 +fi
1-55: Consider adding configuration file support.The script contains multiple hardcoded values (URLs, ports) that should be configurable for different environments.
Consider adding a configuration file (e.g.,
config.sh) to store environment-specific values:# config.sh BEACON_NODE_URL=${BEACON_NODE_URL:-"http://localhost:3500"} PROOF_GENERATOR_URL=${PROOF_GENERATOR_URL:-"http://localhost:8989"} PROOF_GENERATOR_TIMEOUT=${PROOF_GENERATOR_TIMEOUT:-300}This would make the script more maintainable and reusable across different environments.
script/integration/2_VerifyDepositNST.s.sol (1)
19-28: Consider usingaddresstype for nstDepositor.The
nstDepositoris declared asuint256but appears to be used as an address instartBroadcast. Consider changing it toaddresstype for better type safety and clarity.- uint256 nstDepositor; + address nstDepositor;test/foundry/unit/NetworkConfig.t.sol (3)
11-16: Consider using a shared test configuration fileThe hardcoded constants could be moved to a shared test configuration file to:
- Enable reuse across different test files
- Make it easier to update values for different test environments
- Maintain consistency across test suites
30-61: Consider adding edge cases for timestamp-related testsWhile the basic test coverage is good, consider adding edge cases for timestamp-related functions:
- Timestamps near uint256 max value
- Timestamps before genesis
- Boundary conditions for epoch calculations
Example test case:
function testTimestampOverflow() public { uint256 maxTimestamp = type(uint256).max; vm.warp(maxTimestamp); // Add assertions here }
1-103: Consider implementing a test fixture patternTo improve test maintainability and reduce duplication:
- Create a base test contract with common setup and utilities
- Extend it for specific test cases
- Use fixtures for different network configurations
This would make it easier to:
- Add tests for different network configurations
- Share common test utilities
- Maintain consistency across test suites
src/libraries/NetworkConstants.sol (4)
3-3: Consider pinning the Solidity versionUsing
^0.8.0allows any version from 0.8.0 to 0.9.0 (excluding 0.9.0). To ensure consistent compilation across different environments, consider pinning to a specific version.-pragma solidity ^0.8.0; +pragma solidity 0.8.23;
22-76: Consider optimizing chainId access and improving maintainabilityThe function could be optimized by storing chainId in a local variable to reduce storage reads. Additionally, consider breaking down the network-specific logic into separate internal functions for better maintainability.
function getNetworkParams() internal view returns (NetworkParams memory) { - uint256 chainId = block.chainid; + uint256 chainId = _getChainId(); if (chainId == 1) { - // mainnet - return NetworkParams( - 0x00000000219ab540356cBB839Cbe05303d7705Fa, - 1_710_338_135, - SLOTS_PER_EPOCH_DEFAULT, - SECONDS_PER_SLOT_DEFAULT, - 1_606_824_023 - ); + return _getMainnetParams(); } else if (chainId == 11_155_111) { - // sepolia - return NetworkParams( - 0x7f02C3E3c98b133055B8B348B2Ac625669Ed295D, - 1_706_655_072, - SLOTS_PER_EPOCH_DEFAULT, - SECONDS_PER_SLOT_DEFAULT, - 1_655_733_600 - ); + return _getSepoliaParams(); } // ... similar for other networks } +function _getChainId() private view returns (uint256) { + return block.chainid; +} +function _getMainnetParams() private pure returns (NetworkParams memory) { + return NetworkParams( + 0x00000000219ab540356cBB839Cbe05303d7705Fa, + 1_710_338_135, + SLOTS_PER_EPOCH_DEFAULT, + SECONDS_PER_SLOT_DEFAULT, + 1_606_824_023 + ); +}
106-109: Optimize getSecondsPerEpoch to reduce storage readsThe function makes two separate calls to
getNetworkParams(), which could be optimized by storing the result in a local variable.function getSecondsPerEpoch() external view returns (uint64) { - return getNetworkParams().slotsPerEpoch * getNetworkParams().secondsPerSlot; + NetworkParams memory params = getNetworkParams(); + return params.slotsPerEpoch * params.secondsPerSlot; }
12-117: Consider a more extensible approach for network configurationsThe current implementation requires code changes to add new networks. Consider implementing a more extensible approach that would allow adding new networks without modifying the library code. This could be achieved through:
- A separate configuration contract that can be updated
- A registry pattern for network configurations
- An upgradeable proxy pattern for network parameters
This would make the system more maintainable and reduce the need for redeployments when adding new networks.
Would you like me to provide a detailed implementation for any of these approaches?
script/17_WithdrawalValidator.s.sol (2)
Line range hint
91-94: Consider parameterizing the proof file paths.The script currently uses hardcoded JSON file paths for loading validator and withdrawal proofs. Consider making these paths configurable through environment variables or constructor parameters for better flexibility and testing.
- string memory validatorInfo = vm.readFile("script/withdrawalProof_fullwithdraw_2495260_2472449.json"); + string memory proofPath = vm.envOr("WITHDRAWAL_PROOF_PATH", + string("script/withdrawalProof_fullwithdraw_2495260_2472449.json")); + string memory validatorInfo = vm.readFile(proofPath);Also applies to: 123-126
Line range hint
73-77: Consider extracting magic numbers into named constants.The proof validation uses magic numbers (3, 46) for array length checks. These should be extracted into named constants for better maintainability and documentation.
+ uint256 private constant EXPECTED_STATE_ROOT_PROOF_LENGTH = 3; + uint256 private constant EXPECTED_VALIDATOR_ROOT_PROOF_LENGTH = 46; + function _loadValidatorProof() internal { // ... - require(validatorProof.stateRootProof.length == 3, "state root proof should have 3 nodes"); + require(validatorProof.stateRootProof.length == EXPECTED_STATE_ROOT_PROOF_LENGTH, + "Invalid state root proof length"); - require(validatorProof.validatorContainerRootProof.length == 46, "validator root proof should have 46 nodes"); + require(validatorProof.validatorContainerRootProof.length == EXPECTED_VALIDATOR_ROOT_PROOF_LENGTH, + "Invalid validator root proof length");script/7_DeployBootstrap.s.sol (2)
64-64: LGTM! Consider enhancing documentation.Using a pre-deployed beacon oracle instead of deploying a new one is a good optimization that saves gas and aligns with the PR's goal of addressing network-specific configurations.
Consider adding a comment explaining why using the pre-requisite is preferred (e.g., gas savings, network consistency) and where to find the pre-requisite contract addresses for different networks.
Line range hint
38-42: Document the rationale for TVL limits.The TVL limits are set to 5% (1/20) of total supply for both tokens. This seems arbitrary and could be a significant parameter that affects system security and economics.
Consider:
- Making the fraction configurable rather than hardcoded
- Documenting the rationale for choosing 5%
- Adding validation to ensure the calculated limits are within safe bounds
src/libraries/BeaconChainProofs.sol (1)
Line range hint
162-186: Consider adding SafeMath for timestamp comparisonWhile the logic for determining the execution payload header field tree height is correct, consider adding SafeMath for the timestamp comparison to prevent potential overflow issues.
- uint256 executionPayloadHeaderFieldTreeHeight = withdrawalTimestamp < denebForkTimestamp + uint256 executionPayloadHeaderFieldTreeHeight = SafeMath.lt(withdrawalTimestamp, denebForkTimestamp) ? EXECUTION_PAYLOAD_HEADER_FIELD_TREE_HEIGHT_CAPELLA : EXECUTION_PAYLOAD_HEADER_FIELD_TREE_HEIGHT_DENEB;test/foundry/unit/ClientChainGateway.t.sol (1)
Line range hint
247-249: Add test cases for networkConfig initializationWhile the test suite is comprehensive, consider adding specific test cases to verify:
- The initialization of networkConfig
- Any functionality that depends on or validates networkConfig
Add a test case in the Initialize contract:
+ function test_NetworkConfigInitialized() public { + assertEq(clientGateway.NETWORK_CONFIG(), address(0)); + }test/foundry/BootstrapDepositNST.t.sol (1)
103-104: Document the purpose of networkConfig fieldThe
networkConfigfield is being added toImmutableConfigbut is initialized withaddress(0). Consider:
- Adding documentation to explain the purpose of this field
- Verifying if
address(0)is an appropriate default valueAdd a comment above the field to explain its purpose:
BootstrapStorage.ImmutableConfig memory config = BootstrapStorage.ImmutableConfig({ exocoreChainId: exocoreChainId, beaconOracleAddress: address(beaconOracle), vaultBeacon: address(vaultBeacon), exoCapsuleBeacon: address(capsuleBeacon), beaconProxyBytecode: address(beaconProxyBytecode), + // Address of the network configuration contract that stores network-specific constants networkConfig: address(0) });test/foundry/unit/ExoCapsule.t.sol (2)
Line range hint
281-289: Consider extracting capsule setup logic into a helper functionThe capsule setup logic (creating a new capsule and setting its storage slots) is duplicated across test cases. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
+ function _setupCapsule(address capsuleAddress) internal returns (ExoCapsule) { + ExoCapsule anotherCapsule = new ExoCapsule(address(0)); + + bytes32 gatewaySlot = bytes32(stdstore.target(capsuleAddress).sig("gateway()").find()); + vm.store(capsuleAddress, gatewaySlot, bytes32(uint256(uint160(address(this))))); + + bytes32 ownerSlot = bytes32(stdstore.target(capsuleAddress).sig("capsuleOwner()").find()); + vm.store(capsuleAddress, ownerSlot, bytes32(uint256(uint160(address(capsuleOwner))))); + + bytes32 beaconOraclerSlot = bytes32(stdstore.target(capsuleAddress).sig("beaconOracle()").find()); + vm.store(capsuleAddress, beaconOraclerSlot, bytes32(uint256(uint160(address(beaconOracle))))); + + return anotherCapsule; + } + - ExoCapsule anotherCapsule = new ExoCapsule(address(0)); - - bytes32 gatewaySlot = bytes32(stdstore.target(address(anotherCapsule)).sig("gateway()").find()); - vm.store(address(anotherCapsule), gatewaySlot, bytes32(uint256(uint160(address(this))))); - - bytes32 ownerSlot = bytes32(stdstore.target(address(anotherCapsule)).sig("capsuleOwner()").find()); - vm.store(address(anotherCapsule), ownerSlot, bytes32(uint256(uint160(address(capsuleOwner))))); - - bytes32 beaconOraclerSlot = bytes32(stdstore.target(address(anotherCapsule)).sig("beaconOracle()").find()); - vm.store(address(anotherCapsule), beaconOraclerSlot, bytes32(uint256(uint160(address(beaconOracle))))); + ExoCapsule anotherCapsule = _setupCapsule(address(anotherCapsule));
Line range hint
367-379: Consider consolidating duplicate capsule initialization logicThe capsule initialization logic in WithdrawalSetup is similar to DepositSetup. Consider creating a base test contract that both setups can inherit from to avoid code duplication.
+contract CapsuleTestBase is Test { + function _initializeCapsule(address withdrawalCredentials) internal returns (ExoCapsule) { + ExoCapsule phantomCapsule = new ExoCapsule(address(0)); + address capsuleAddress = _getCapsuleFromWithdrawalCredentials(withdrawalCredentials); + vm.etch(capsuleAddress, address(phantomCapsule).code); + return ExoCapsule(payable(capsuleAddress)); + } +} + -contract WithdrawalSetup is Test { +contract WithdrawalSetup is CapsuleTestBase { // ... existing code ... function setUp() public { // ... existing code ... - ExoCapsule phantomCapsule = new ExoCapsule(address(0)); - address capsuleAddress = _getCapsuleFromWithdrawalCredentials(_getWithdrawalCredentials(validatorContainer)); - vm.etch(capsuleAddress, address(phantomCapsule).code); - capsule = ExoCapsule(payable(capsuleAddress)); + capsule = _initializeCapsule(_getWithdrawalCredentials(validatorContainer)); // ... rest of the code ... }script/integration/NetworkConfig.sol (4)
7-7: Nitpick: Consistent Numeric Literal FormattingConsider using consistent formatting for numeric literals. The
ALLOWED_CHAIN_IDis written with an underscore (31_337) for readability. Ensure that this style is applied consistently across the codebase or adjust it for uniformity.
40-40: Suggestion: Avoid Hardcoding Values in Error MessagesThe error message in the
requirestatement hardcodes the chain ID value:require(block.chainid == ALLOWED_CHAIN_ID, "only the 31337 chain ID is supported for integration tests");To prevent discrepancies if
ALLOWED_CHAIN_IDchanges, consider referencing the constant directly in the error message. While Solidity doesn't support string interpolation in error messages, you can use custom error types introduced in Solidity 0.8.4 for more informative errors:error UnsupportedChainId(uint256 provided, uint256 expected); require(block.chainid == ALLOWED_CHAIN_ID, UnsupportedChainId(block.chainid, ALLOWED_CHAIN_ID));This approach ensures consistency and provides clearer feedback during integration tests.
71-72: Gas Optimization: Cache Calculated Value to Reduce Storage ReadsIn the
getSecondsPerEpoch()function, you're calculating the value by multiplying two storage variables:return params.slotsPerEpoch * params.secondsPerSlot;While this avoids storing a derived value, it results in two storage reads each time the function is called. If this function is called frequently, consider caching the calculated value in the constructor to minimize gas costs:
Apply this diff to cache the value:
require(beaconGenesisTimestamp > 0, "Beacon genesis timestamp must be set for integration network"); params = NetworkParams(deposit, denebTimestamp, slotsPerEpoch, secondsPerSlot, beaconGenesisTimestamp); + + secondsPerEpoch = slotsPerEpoch * secondsPerSlot; } /// @inheritdoc INetworkConfig function getSecondsPerEpoch() external view returns (uint64) { - // reading from storage is more expensive than performing the calculation - return params.slotsPerEpoch * params.secondsPerSlot; + return secondsPerEpoch; }You'll need to declare
secondsPerEpochas a state variable:uint64 private secondsPerEpoch;This reduces the number of storage reads from two to one when
getSecondsPerEpoch()is called.
35-39: Style Improvement: Streamline Comment SectionsThe explanatory comments in the constructor are detailed but could be more concise for readability:
// the value of 31337 is known to be a reserved chain id for testing. // it is different from Anvil's 1337 to avoid confusion, since it does not support PoS. // the downside of this number is that another chain id must be configured in `foundry.toml` to be used // by default, during tests. setting this configuration also prevents NetworkConstants from complaining // about Unsupported Network during tests, so it is worth it.Consider summarizing the key points to enhance clarity:
// Using chain ID 31337 for integration tests to differentiate from Anvil's 1337, which doesn't support PoS. // Requires configuring this chain ID in `foundry.toml` to prevent Unsupported Network errors during tests.script/integration/BeaconOracle.sol (4)
37-37: Typo in error message documentationThere is a duplicate "the" in the error message documentation. It should read "Timestamp out of range for the beacon roots precompile."
82-82: Typo in documentation: "available" misspelledThe word "available" is misspelled as "avaliable" in the comment. Please correct it to "available."
60-64: Optimize staticcall usage inaddTimestampIn the
addTimestampfunction, the staticcall toBEACON_ROOTSwith_targetTimestamponly checks for success without utilizing the returned data. Since thefindBlockRootfunction subsequently performs staticcalls to retrieve the block root, this initial staticcall may be redundant. Consider removing it or refactoring the logic to avoid unnecessary calls, thereby saving gas.
83-83: Inconsistent comment styleLine 83 uses a single-line comment
//instead of the triple-slash///used in the surrounding documentation comments. For consistency and proper documentation generation, consider changing it to///.src/storage/ExoCapsuleStorage.sol (3)
40-50: Consider makingVERIFY_BALANCE_UPDATE_WINDOW_SECONDSconfigurableWhile the extensive comments provide clarity, consider making
VERIFY_BALANCE_UPDATE_WINDOW_SECONDSa configurable parameter. This would allow flexibility across different networks with varying slot times, ensuring the contract remains adaptable.
99-134: Refactor repetitive getter functions to reduce code duplicationThe internal view functions
getDenebHardForkTimestamp,getSlotsPerEpoch,getSecondsPerEpoch, andgetBeaconGenesisTimestampshare a similar pattern. Consider refactoring to eliminate redundancy and enhance maintainability.Apply this refactor to create a generic getter function:
+function _getNetworkParameterUint256( + function () internal pure returns (uint256) constantFunction, + function (address) external view returns (uint256) configFunction +) internal view returns (uint256) { + if (NETWORK_CONFIG == address(0)) { + return constantFunction(); + } else { + return configFunction(NETWORK_CONFIG); + } +} function getDenebHardForkTimestamp() internal view returns (uint256) { - if (NETWORK_CONFIG == address(0)) { - return NetworkConstants.getDenebHardForkTimestamp(); - } else { - return INetworkConfig(NETWORK_CONFIG).getDenebHardForkTimestamp(); - } + return _getNetworkParameterUint256( + NetworkConstants.getDenebHardForkTimestamp, + INetworkConfig.getDenebHardForkTimestamp + ); } function getSlotsPerEpoch() internal view returns (uint64) { - if (NETWORK_CONFIG == address(0)) { - return NetworkConstants.getSlotsPerEpoch(); - } else { - return INetworkConfig(NETWORK_CONFIG).getSlotsPerEpoch(); - } + return uint64(_getNetworkParameterUint256( + NetworkConstants.getSlotsPerEpoch, + INetworkConfig.getSlotsPerEpoch + )); } function getSecondsPerEpoch() internal view returns (uint64) { - if (NETWORK_CONFIG == address(0)) { - return NetworkConstants.getSecondsPerEpoch(); - } else { - return INetworkConfig(NETWORK_CONFIG).getSecondsPerEpoch(); - } + return uint64(_getNetworkParameterUint256( + NetworkConstants.getSecondsPerEpoch, + INetworkConfig.getSecondsPerEpoch + )); } function getBeaconGenesisTimestamp() internal view returns (uint256) { - if (NETWORK_CONFIG == address(0)) { - return NetworkConstants.getBeaconGenesisTimestamp(); - } else { - return INetworkConfig(NETWORK_CONFIG).getBeaconGenesisTimestamp(); - } + return _getNetworkParameterUint256( + NetworkConstants.getBeaconGenesisTimestamp, + INetworkConfig.getBeaconGenesisTimestamp + ); }This refactor reduces code duplication and makes it easier to maintain and extend in the future.
40-50: Typographical corrections in commentsThere are minor typographical errors in the comments that could be corrected for clarity:
- Line 49~: "this one. So, there is no need" should be "this one, so there is no need".
Apply this diff to correct the typo:
-/// is higher than this one. So, there is no need to make this parameter configurable based on the network. +/// is higher than this one, so there is no need to make this parameter configurable based on the network.src/core/ExoCapsule.sol (1)
410-410: Clarify error message inrequirestatementThe
requirecondition allowstimestampto be equal tobeaconChainGenesisTime, but the error message states "timestamp should be greater than beacon chain genesis timestamp". Consider updating the error message to "timestamp should be greater than or equal to beacon chain genesis timestamp" for consistency.script/integration/1_DeployBootstrap.s.sol (1)
137-139: Consider Separating Virtual Tokens for ClarityAdding
VIRTUAL_STAKED_ETH_ADDRESSat index0ofwhitelistTokensand starting loops fromi = 1could lead to confusion or potential off-by-one errors. To enhance readability and maintainability, consider handling virtual tokens separately or explicitly documenting the indexing strategy.script/generate.js (2)
303-306: Address the TODO regarding public key retrievalThe code contains a TODO comment about retrieving the correct validator public keys instead of hashes. This is crucial for accurate validator identification and affects the
ClientChainGatewayand overall network functionality.Would you like assistance in implementing the retrieval of the correct public keys from their hashes? I can help by providing code snippets or opening a GitHub issue.
827-827: Handle asynchronous write operation errorsWhen writing the updated genesis file, consider handling potential errors from the asynchronous
writeFileoperation. Although unlikely, adding a try-catch around this operation enhances reliability.- await fs.writeFile(RESULT_GENESIS_FILE_PATH, jsonBig.stringify(genesisJSON, null, 2)); + try { + await fs.writeFile(RESULT_GENESIS_FILE_PATH, jsonBig.stringify(genesisJSON, null, 2)); + console.log('Genesis file updated successfully.'); + } catch (writeError) { + console.error('Error writing genesis file:', writeError.message); + }test/foundry/unit/Bootstrap.t.sol (3)
Line range hint
1068-1077: Clarifyvm.dealamount intest24_Stake_InvalidValueIn
test24_Stake_InvalidValue, the test stakes{value: 31 ether}to validate that staking with an incorrect amount reverts. However,vm.dealprovides33 ethertoaddrs[0], which might be confusing. Consider adjustingvm.dealto provide exactly31 etherto make the test intent clearer.
Line range hint
1218-1236: Future implementation note intest28_ProcessBeaconChainWithdrawalThe
test28_ProcessBeaconChainWithdrawalfunction expects a revert due toErrors.NotYetSupported.selector. Since this feature is not implemented yet, consider updating this test when the functionality becomes available to ensure comprehensive coverage.
Line range hint
1276-1295: Optimize storage access in_enableNativeRestakingand_disableNativeRestakingIn the helper functions
_enableNativeRestakingand_disableNativeRestaking, consider caching the storage slot or abstracting the logic to reduce repetitive code. This can enhance readability and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.env.example(1 hunks).gitignore(1 hunks)foundry.toml(1 hunks)package.json(2 hunks)script/12_RedeployClientChainGateway.s.sol(1 hunks)script/13_DepositValidator.s.sol(2 hunks)script/14_CorrectBootstrapErrors.s.sol(1 hunks)script/16_UpgradeExoCapsule.s.sol(1 hunks)script/17_WithdrawalValidator.s.sol(2 hunks)script/2_DeployBoth.s.sol(3 hunks)script/7_DeployBootstrap.s.sol(2 hunks)script/BaseScript.sol(0 hunks)script/generate.js(9 hunks)script/integration/1_DeployBootstrap.s.sol(10 hunks)script/integration/2_VerifyDepositNST.s.sol(1 hunks)script/integration/BeaconOracle.sol(1 hunks)script/integration/NetworkConfig.sol(1 hunks)script/integration/deposit.sh(1 hunks)script/integration/prove.sh(1 hunks)src/core/Bootstrap.sol(5 hunks)src/core/ExoCapsule.sol(4 hunks)src/interfaces/INetworkConfig.sol(1 hunks)src/libraries/BeaconChainProofs.sol(3 hunks)src/libraries/NetworkConstants.sol(1 hunks)src/storage/BootstrapStorage.sol(6 hunks)src/storage/ExoCapsuleStorage.sol(4 hunks)test/foundry/BootstrapDepositNST.t.sol(2 hunks)test/foundry/ExocoreDeployer.t.sol(3 hunks)test/foundry/Governance.t.sol(3 hunks)test/foundry/unit/Bootstrap.t.sol(10 hunks)test/foundry/unit/ClientChainGateway.t.sol(3 hunks)test/foundry/unit/ExoCapsule.t.sol(3 hunks)test/foundry/unit/NetworkConfig.t.sol(1 hunks)
💤 Files with no reviewable changes (1)
- script/BaseScript.sol
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Learnings (3)
script/integration/2_VerifyDepositNST.s.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:170-232
Timestamp: 2024-11-12T10:03:21.957Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 prefers to keep lengthy test functions that cover multiple functionalities within a single test function like `test_VerifyAndDepositNativeStake`.
test/foundry/BootstrapDepositNST.t.sol (2)
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: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.
test/foundry/unit/Bootstrap.t.sol (1)
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/ExocoreDeployer.t.sol:355-367
Timestamp: 2024-11-12T10:03:15.304Z
Learning: In `test/foundry/ExocoreDeployer.t.sol`, the constructor of `BootstrapStorage` already performs validation checks for critical addresses, so adding additional validation before creating the `ImmutableConfig` struct is unnecessary.
🔇 Additional comments (81)
foundry.toml (1)
16-17: Verify test behavior across different networks
Given the PR's mention of issues with constants being reused across networks, we should verify that this change doesn't mask underlying problems with network-specific configurations.
✅ Verification successful
Based on the search results, I can make a determination about the network-specific configurations. The results show:
- No
NetworkConstantscontract was found in the codebase, suggesting it might have been removed or renamed. - Network configuration is now handled through a
NetworkConfigcontract that explicitly checks for chain ID 31337 (for integration tests) and contains proper validation. - The tests demonstrate proper handling of network-specific constants (like BEACON_CHAIN_GENESIS_TIME, SLOTS_PER_EPOCH, etc.) with explicit values.
Let me verify the NetworkConfig contract implementation to ensure it properly handles network-specific configurations:
Let me check the actual implementation of NetworkConfig to understand how it handles network-specific configurations:
Let me check the interface to fully understand the contract's requirements:
Setting chain_id = 1 in foundry.toml is correct and necessary
The codebase shows a well-designed separation between test and production environments:
- The NetworkConfig contract explicitly documents that it's "used for integration testing and is a substitute for the NetworkConstants library"
- It enforces chain_id 31337 for integration tests
- The code comments specifically mention: "another chain id must be configured in
foundry.tomlto be used by default, during tests" - Setting chain_id = 1 in foundry.toml allows tests to run with mainnet constants while the actual test environment uses chain_id 31337
This setup ensures proper testing behavior while maintaining network-specific configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for network-specific constants and verify their usage
# Look for network-specific constants in the codebase
echo "Searching for network-specific constants..."
rg -A 5 "ETH1_DEPOSIT_ADDRESS|GENESIS_TIME"
# Look for chain ID usage in tests
echo "Checking chain ID usage in tests..."
rg -A 5 "chain[_\s]?id|chainId" test/
# Look for NetworkConstants usage
echo "Checking NetworkConstants usage..."
ast-grep --pattern 'contract NetworkConstants {
$$$
}'
Length of output: 32968
Script:
#!/bin/bash
# Search for NetworkConfig implementation
rg -A 10 "contract NetworkConfig"
Length of output: 1950
Script:
#!/bin/bash
# Get the NetworkConfig implementation
cat script/integration/NetworkConfig.sol
Length of output: 3913
Script:
#!/bin/bash
# Get the NetworkConfig interface
cat src/interfaces/INetworkConfig.sol
Length of output: 2021
script/16_UpgradeExoCapsule.s.sol (1)
27-27: Verify the implications of initializing ExoCapsule with address(0)
The initialization of ExoCapsule with address(0) needs careful consideration as this will affect all proxy instances during the upgrade.
Let's verify the ExoCapsule constructor implementation and its dependencies:
✅ Verification successful
Let me gather more information about the ExoCapsule contract implementation to understand how the address(0) parameter is used.
Based on the gathered information, I can now provide the final response:
Using address(0) for ExoCapsule initialization is safe in this upgrade context
The ExoCapsule contract inherits from ExoCapsuleStorage where the constructor parameter networkConfig_ is only used to set the NETWORK_CONFIG immutable variable. In the upgrade script context:
- The address(0) parameter is safe as this is just an implementation contract that will be used as a logic contract behind a beacon proxy
- All existing proxy instances will retain their original storage, including their network configuration
- This pattern is consistently used across tests and deployment scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ExoCapsule constructor implementation and usage
# Expected: Find constructor implementation and any address(0) validation
# Search for ExoCapsule constructor implementation
ast-grep --pattern 'contract ExoCapsule {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for other occurrences of ExoCapsule initialization
rg -A 5 "new ExoCapsule"
Length of output: 8373
Script:
#!/bin/bash
# Search for ExoCapsule contract implementation
fd -e sol -x cat {} \; | ast-grep --pattern 'contract ExoCapsule {
$$$
}'
# Search for any references to the constructor parameter
rg -A 5 -B 5 "constructor.*\(address" src/
Length of output: 5407
.env.example (3)
36-36: Document NST_DEPOSITOR purpose and format
The purpose and expected format of NST_DEPOSITOR should be documented. Is this an address, a private key, or something else?
#!/bin/bash
# Search for usage of NST_DEPOSITOR to understand its purpose
rg -A 3 "NST_DEPOSITOR"31-31: Verify the hardcoded deposit address
The hardcoded deposit address 0x696969... appears to be a placeholder. This could be dangerous if accidentally used in production.
✅ Verification successful
The previous output showed partial information about the deposit address usage in integration scripts. Let's gather more context about how this address is used and if there are any other deposit addresses defined in environment files or integration scripts.
The hardcoded deposit address is a safe default for integration testing
The address 0x696969... is only used as a default fallback value in integration testing scripts when the INTEGRATION_DEPOSIT_ADDRESS environment variable is not set. This is evident from:
- The address is only used in
script/integration/1_DeployBootstrap.s.solwithvm.envOr()which provides a default value if the environment variable is not set - In actual usage,
script/integration/deposit.shexports the real deposit address from genesis configuration:export INTEGRATION_DEPOSIT_ADDRESS=$deposit_address - The placeholder address is not used in any production code, only in integration test scripts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of this address in the codebase
rg -l "0x6969696969696969696969696969696969696969"
# Check if this address is used in any test files
fd -e test.sol -e spec.ts -e test.ts --exec grep -l "0x6969696969696969696969696969696969696969" {}
Length of output: 1942
Script:
#!/bin/bash
# Search for the deposit address in the codebase with context
rg -A 3 "INTEGRATION_DEPOSIT_ADDRESS"
# Check .env files for any other deposit addresses
fd -e env -e env.example --exec grep -l "DEPOSIT" {}
# Look for any other hardcoded addresses in integration scripts
rg "0x[a-fA-F0-9]{40}" script/integration/
Length of output: 4660
32-33: Verify slot and epoch configurations
The values INTEGRATION_SECONDS_PER_SLOT=4 and INTEGRATION_SLOTS_PER_EPOCH=3 seem unusually low compared to mainnet values (12 seconds per slot, 32 slots per epoch). While this might be intentional for testing, it should be documented why these values differ.
✅ Verification successful
Based on the search results, I can now generate a final response. The code shows these values are used in integration tests with proper validation, and the README confirms these are specifically for localnet testing purposes.
Integration test configuration values are properly validated
The low values for INTEGRATION_SECONDS_PER_SLOT and INTEGRATION_SLOTS_PER_EPOCH are specifically used for local integration testing, as evidenced by:
- Values are properly validated in
DeployBootstrap.s.solwith bounds checking - They are used within the integration test scripts (
script/integration/directory) - The README explicitly mentions these are localnet test configurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these constants to ensure they're properly validated
ast-grep --pattern 'INTEGRATION_SECONDS_PER_SLOT'
ast-grep --pattern 'INTEGRATION_SLOTS_PER_EPOCH'
Length of output: 1942
Script:
#!/bin/bash
# Search for files containing these environment variables
rg "INTEGRATION_SECONDS_PER_SLOT|INTEGRATION_SLOTS_PER_EPOCH" -A 3
# Look for any documentation about integration test configuration
fd README.md -X cat {}
# Check for any spec or configuration files that might explain these values
fd "spec|config" -e json -e yaml -e yml -X cat {}
Length of output: 7814
script/integration/deposit.sh (1)
1-4: LGTM: Robust script initialization
The script setup is well-implemented with proper shebang and robust directory resolution using readlink -f.
src/interfaces/INetworkConfig.sol (1)
1-8: LGTM! Well-structured interface declaration with proper documentation.
The file follows Solidity best practices with:
- Appropriate SPDX license identifier
- Pinned compiler version
- Comprehensive NatSpec documentation
script/integration/2_VerifyDepositNST.s.sol (2)
1-17: LGTM! Well-organized imports and contract structure.
The imports are properly organized and the contract inherits from Script.sol as expected for an integration test.
1-68: Verify the usage context of this script.
Since this is an integration test script with direct contract interactions and manual oracle triggers, please ensure this is never deployed or used in a production environment.
✅ Verification successful
This script is safely used only in integration testing environment
Based on the comprehensive analysis of the codebase:
- The script is part of a well-structured integration test setup, located in
script/integration/ - It's specifically designed to work with a test environment (chain ID 31337)
- The script is used in conjunction with other integration test scripts like
DeployContracts.s.sol - It contains safety checks and requires specific test environment variables
- The script is not referenced in any production deployment configurations
- The contract explicitly checks for test chain ID:
require(block.chainid == ALLOWED_CHAIN_ID, "only the 31337 chain ID is supported for integration tests")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this script is referenced in any deployment configurations
rg -l "VerifyDepositNST" --type yaml --type json
Length of output: 48
Script:
#!/bin/bash
# Let's check more thoroughly for any references to this script
# 1. Check all files for the script name
rg -l "VerifyDepositNST"
# 2. Check specifically in script directories for deployment or production configs
fd -t f . script/ -x grep -l "VerifyDepositNST" {} \;
# 3. Look for integration test patterns
rg -l "integration.*test" --type yaml --type json
# 4. Check package.json or similar config files for test scripts
fd "package.json|foundry.toml|hardhat.config" -x cat {}
Length of output: 12752
Script:
#!/bin/bash
# Let's check the integration directory structure and related files
# 1. Check the prove.sh script that references VerifyDepositNST
cat script/integration/prove.sh
# 2. Look for test-specific configurations or markers
fd -t f . script/integration/ -x cat {}
# 3. Check for any production deployment scripts that might use this
rg -l "deploy.*VerifyDepositNST"
Length of output: 36634
script/12_RedeployClientChainGateway.s.sol (1)
67-68: Verify implications of initializing networkConfig to address(0)
The initialization of networkConfig to address(0) requires careful consideration:
- This might be problematic if other contracts or the price feeder tool expect a valid network configuration address.
- Given that this PR aims to fix the issue of using same constants across networks, we should ensure that
address(0)doesn't reintroduce similar problems.
Let's verify the usage of networkConfig in other contracts:
Consider:
- Adding inline documentation explaining why
address(0)is acceptable here - Adding validation in the ClientChainGateway constructor if a non-zero network config is required
- Implementing a mechanism to update the network config post-deployment if needed
script/13_DepositValidator.s.sol (3)
22-22: LGTM: Import aligns with network-specific constant requirements.
The addition of NetworkConstants import supports the PR's objective of fixing network-specific constant handling.
32-32: Verify initialization timing of GENESIS_BLOCK_TIMESTAMP.
The change from constant to immutable is appropriate for network-specific values. However, we should verify:
- The initialization timing in the constructor
- The value's consistency across different networks
#!/bin/bash
# Search for NetworkConstants implementation and usage
echo "Searching for NetworkConstants implementation..."
ast-grep --pattern 'contract NetworkConstants {
$$$
function getBeaconGenesisTimestamp() $$ {
$$$
}
$$$
}'
echo "Checking for other usages of getBeaconGenesisTimestamp..."
rg "getBeaconGenesisTimestamp" -A 3Line range hint 95-96: Consider adding timestamp validation checks.
The beaconBlockTimestamp calculation now uses a network-specific genesis time. Consider adding validation to ensure:
- The calculated timestamp is not in the future
- The slot number is within reasonable bounds
- The timestamp aligns with the network's epoch boundaries
This will help prevent potential issues with cross-network validator proofs.
test/foundry/unit/NetworkConfig.t.sol (2)
19-27: LGTM! Well-structured test setup
The setUp function follows best practices:
- Clear documentation
- Proper use of forge's vm for chain ID simulation
- Clean contract initialization
12-12: Verify the deposit contract address
The hardcoded deposit contract address appears to be a placeholder. Ensure this matches the expected address for the integration network.
✅ Verification successful
The hardcoded address is correct for test environment
The address 0x1234567890AbcdEF1234567890aBcdef12345678 is intentionally used as a placeholder in the test file NetworkConfig.t.sol. This is appropriate because:
-
The actual deposit contract addresses for different networks are properly defined in
NetworkConstants.sol:- Mainnet:
0x00000000219ab540356cBB839Cbe05303d7705Fa - Sepolia:
0x7f02C3E3c98b133055B8B348B2Ac625669Ed295D - Holesky:
0x4242424242424242424242424242424242424242
- Mainnet:
-
The test file uses a mock implementation (
ETHPOSDepositMock) and is specifically designed for integration tests on chain ID 31337 (local testnet).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the actual deposit contract address usage across the codebase
rg -l "0x1234567890AbcdEF1234567890aBcdef12345678"
Length of output: 88
Script:
#!/bin/bash
# Search for any other deposit contract addresses in the codebase
rg -i "deposit.*contract.*address" -B 2 -A 2
# Also search for any configuration files that might contain network addresses
fd -e json -e yaml -e yml -e toml | xargs rg -l "address"
Length of output: 9821
Script:
#!/bin/bash
# Check the genesis.json file mentioned in the deposit.sh script
cat script/integration/genesis.json 2>/dev/null || echo "File not found"
# Check if there's a NetworkConstants implementation that might have the actual address
rg -B 2 -A 2 "getNetworkParams\(\)"
Length of output: 2912
Script:
#!/bin/bash
# Let's check the NetworkConstants implementation for the actual mainnet deposit contract address
rg -B 5 -A 5 "chainId == 1"
# Also check for any test mock implementations
rg -B 2 -A 2 "ETHPOSDepositMock"
Length of output: 4642
src/libraries/NetworkConstants.sol (2)
12-18: LGTM! Well-documented constants with correct values
The constants are properly defined and match the Ethereum specifications. The documentation clearly explains their purpose.
29-30: Verify the hardcoded Deneb fork timestamps
The hardcoded Deneb fork timestamps need verification as they are critical network parameters.
Also applies to: 43-45, 58-60
script/14_CorrectBootstrapErrors.s.sol (3)
Line range hint 1-24: Verify the reinitializer pattern implementation
The comment at the top indicates this script uses a reinitializer pattern, requiring changes to modifier versions in both Bootstrap and ClientChainGateway contracts.
Let's verify the reinitializer implementation:
#!/bin/bash
# Check reinitializer usage in both contracts
ast-grep --pattern 'reinitializer($VERSION)'
# Verify the initialize function modifiers
ast-grep --pattern 'function initialize($$$) $MODIFIER {
$$$
}'Line range hint 82-91: Verify the initialization timestamp logic
The initialization uses a hardcoded timeframe of "1 week from now" (168 hours). This could be problematic if:
- The deployment gets delayed
- Different networks need different initialization windows
- The script needs to be rerun
Let's check for similar timestamp handling:
#!/bin/bash
# Search for other timestamp-based initializations
rg "block\.timestamp \+"
# Check if this timing is referenced in tests
rg "168 hours" test/Consider making the initialization window configurable through environment variables or constructor parameters instead of hardcoding it.
99-100: Verify the implications of using address(0) for networkConfig
Setting networkConfig to address(0) might be problematic:
- The PR objectives mention issues with network-specific constants, but using a zero address doesn't solve this problem
- If the Bootstrap contract depends on this configuration for network-specific behavior, a zero address could lead to unexpected behavior
Let's verify how this configuration is used:
Consider implementing a proper NetworkConfig contract that encapsulates network-specific constants (ETH1_DEPOSIT_ADDRESS, GENESIS_TIME, etc.) instead of using address(0). This would:
- Provide better separation of concerns
- Make network-specific configurations explicit
- Prevent potential issues from uninitialized configurations
✅ Verification successful
Let's try with simpler commands to understand how networkConfig is used and verify if address(0) is a valid value.
Based on the search results, I can now provide a definitive response about using address(0) for networkConfig:
Setting networkConfig to address(0) is valid and intentionally supported
The codebase explicitly handles the zero address case for networkConfig:
- In
BootstrapStorage.sol:
// networkConfig is allowed to be 0
if (config.networkConfig == address(0)) {
depositContract = NetworkConstants.getDepositContractAddress();
} else {
depositContract = INetworkConfig(config.networkConfig).getDepositContractAddress();
}- In
ExoCapsuleStorage.sol, similar fallback patterns exist for all network configuration values:
if (NETWORK_CONFIG == address(0)) {
return NetworkConstants.getDenebHardForkTimestamp();
} else {
return INetworkConfig(NETWORK_CONFIG).getDenebHardForkTimestamp();
}The system is designed to fall back to hardcoded constants in NetworkConstants library when networkConfig is not provided, making address(0) a valid configuration option.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for networkConfig usage in Bootstrap and related contracts
ast-grep --pattern 'function $FUNC($$$) {
$$$
networkConfig
$$$
}'
# Search for address(0) checks
ast-grep --pattern 'require($COND != address(0), $_)'
# Search for potential network-specific constants
rg -l "ETH1_DEPOSIT_ADDRESS|GENESIS_TIME"
Length of output: 277
Script:
#!/bin/bash
# Search for networkConfig usage
rg -A 5 "networkConfig"
# Search for address(0) checks related to config
rg -A 5 "require.*address\(0\)"
# Search for network-specific constants
rg "ETH1_DEPOSIT|GENESIS_TIME|NETWORK_CONFIG"
# Look for ImmutableConfig struct definition
ast-grep --pattern 'struct ImmutableConfig {
$$$
}'
Length of output: 58233
script/17_WithdrawalValidator.s.sol (2)
26-27: LGTM! Import aligns with PR objectives.
The addition of NetworkConstants library import helps address the issue of constants being reused across different networks, as mentioned in the PR objectives.
38-38: Verify the genesis timestamp initialization across networks.
The change from a hardcoded constant to an immutable value from NetworkConstants is a good improvement. However, we should verify that:
- The genesis timestamp is correctly initialized for different networks
- The timestamp calculation in
_loadValidatorProof()remains accurate with this change
✅ Verification successful
Based on the shell script results, I can now generate a final response as we have sufficient information about the genesis timestamp implementation and usage:
Genesis timestamp initialization is properly handled across networks
The verification shows that:
- The genesis timestamp is correctly initialized through
NetworkConstants.getBeaconGenesisTimestamp()with proper fallback to network-specific configuration - The timestamp calculation in
_loadValidatorProof()remains accurate, using the formulaGENESIS_BLOCK_TIMESTAMP + SECONDS_PER_SLOT * slot - The implementation includes proper validation checks and network-specific configurations through
NetworkConfig - The constant
SECONDS_PER_SLOT = 12is consistently used across the codebase
The changes maintain consistency with the beacon chain specifications while providing flexibility for different network configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the genesis timestamp initialization and usage
# Check for hardcoded genesis timestamps in other files
echo "Checking for other hardcoded genesis timestamps..."
rg -n "GENESIS.*TIMESTAMP.*=.*[0-9]+" --type sol
# Check NetworkConstants implementation
echo "Checking NetworkConstants implementation..."
ast-grep --pattern 'function getBeaconGenesisTimestamp() $_ { $$$_ }'
# Check for other timestamp calculations using GENESIS_BLOCK_TIMESTAMP
echo "Checking other timestamp calculations..."
rg -n "GENESIS_BLOCK_TIMESTAMP.*\+" --type sol
Length of output: 526
Script:
#!/bin/bash
# Let's try with correct file extensions and more specific searches
# Check for NetworkConstants implementation
echo "Checking NetworkConstants implementation..."
fd -e sol -e solidity . -x cat {} | grep -A 5 "getBeaconGenesisTimestamp"
# Check for genesis timestamp usage and initialization
echo "Checking genesis timestamp references..."
fd -e sol -e solidity . -x cat {} | grep -B 2 -A 2 "GENESIS.*TIMESTAMP"
# Check timestamp calculations in WithdrawalValidator
echo "Checking WithdrawalValidator timestamp calculations..."
fd "WithdrawalValidator" -e sol -e solidity -x cat {}
Length of output: 16637
script/7_DeployBootstrap.s.sol (2)
81-82: Verify struct changes across the codebase.
The changes to BootstrapStorage.ImmutableConfig struct require careful verification:
- The reordering of
beaconProxyBytecodefield could break existing deployments if not handled properly - The addition of
networkConfigfield withaddress(0)needs validation
#!/bin/bash
# Search for ImmutableConfig struct usage and initialization
ast-grep --pattern 'struct ImmutableConfig {
$$$
}'
# Search for places where ImmutableConfig is constructed
rg "ImmutableConfig.*memory.*=.*ImmutableConfig" -A 5
# Search for storage layout dependencies
rg "BootstrapStorage" -A 569-69: Verify the safety of passing address(0) to ExoCapsule constructor.
The change from new ExoCapsule() to new ExoCapsule(address(0)) needs verification:
- Confirm that ExoCapsule contract properly handles address(0)
- Verify if this change is consistent with the contract's intended behavior
✅ Verification successful
Let me generate additional scripts to verify the safety of passing address(0) to ExoCapsule constructor.
Based on the search results, I can now generate the final response since we have found that ExoCapsule's initialize function explicitly validates the gateway address to prevent address(0):
Passing address(0) to ExoCapsule constructor is safe as it's only used for implementation contract
The ExoCapsule contract properly validates the gateway address in its initialize function with the check require(gateway_ != address(0), "ExoCapsule: gateway address can not be empty"). The constructor parameter is only used for the implementation contract which is never initialized, while actual ExoCapsule instances are created as proxies and properly initialized with non-zero addresses.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ExoCapsule constructor implementation and usage
ast-grep --pattern 'contract ExoCapsule {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for other instances where ExoCapsule is instantiated
rg "new ExoCapsule" -A 2
Length of output: 123858
Script:
#!/bin/bash
# Search for ExoCapsule constructor implementation
ast-grep --pattern 'constructor($_) {
$$$
}'
# Search for ExoCapsule initialization and usage
rg "initialize.*gateway_.*capsuleOwner_.*beaconOracle_" -A 3
# Search for any validation or checks related to gateway address
rg "require.*gateway" -A 1
Length of output: 86951
script/2_DeployBoth.s.sol (4)
9-9: LGTM: Network-specific constants import added
This import aligns with the PR objective of fixing the use of network-specific constants, replacing hardcoded values that were previously shared across networks.
66-66: Verify the implications of using address(0) in ExoCapsule constructor
The ExoCapsule constructor now requires an address parameter, but it's initialized with address(0). This could indicate:
- A placeholder value that needs to be updated later
- An intentional null state initialization
Please clarify the intended behavior and verify if address(0) is safe to use here.
#!/bin/bash
# Description: Check ExoCapsule implementation for address(0) handling
# Search for address(0) checks or related logic in ExoCapsule
ast-grep --pattern 'require($_address != address(0), $_)'
# Look for constructor implementation
ast-grep --pattern 'constructor(address $_) {
$$$
}'82-83: Review the immutable networkConfig initialization
The addition of networkConfig to ImmutableConfig struct is good for network-specific settings, but two points need attention:
- Since this is an immutable configuration, verify if address(0) is the intended final value or if it needs to be set to a proper address during deployment
- Consider adding validation in the contract to ensure critical addresses are not address(0) if they're required for operation
✅ Verification successful
Based on the shell script results, I can now generate the final response:
The address(0) initialization for networkConfig is intentionally designed
The codebase explicitly handles the zero address case for networkConfig:
- In BootstrapStorage.sol, there's a comment "networkConfig is allowed to be 0"
- When networkConfig is address(0), the contract falls back to NetworkConstants for configuration values
- When set to a valid address, it uses INetworkConfig interface to fetch values
This design pattern is consistently used across the codebase, making it a valid implementation choice rather than an oversight.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for networkConfig usage and validation
# Search for networkConfig usage
rg -A 5 "networkConfig"
# Look for address validation patterns in the codebase
ast-grep --pattern 'require($_config != address(0), $_)'
Length of output: 17861
62-62: Verify beacon genesis timestamp across networks
The beacon oracle now uses network-specific genesis timestamp, which is crucial for correct network synchronization. Please ensure the timestamp values are correctly configured in NetworkConstants for each target network.
✅ Verification successful
Beacon genesis timestamps are correctly configured for all supported networks
The NetworkConstants library has proper beacon genesis timestamps configured for all supported networks:
- Mainnet: 1606824023 (Dec 1, 2020)
- Sepolia: 1655733600 (Jun 20, 2022)
- Holesky: 1695902400 (Sep 28, 2023)
Each timestamp is well-documented with source references and matches the official network configurations. The library also properly handles unsupported networks by reverting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if NetworkConstants.sol contains network-specific genesis timestamps
# and verify they match with the expected values for each network
# Search for genesis timestamp definitions
rg -A 5 "getBeaconGenesisTimestamp"
# Look for network-specific configurations
rg -A 10 "NetworkConstants"
Length of output: 209
Script:
#!/bin/bash
# Let's try a more specific search for NetworkConstants and genesis timestamp related code
# Search for NetworkConstants file
fd "NetworkConstants"
# If found, let's look at its content
fd "NetworkConstants" --exec cat {}
# Also search for any references to beacon genesis timestamp
rg -i "beacon.*genesis.*timestamp"
Length of output: 12002
src/libraries/BeaconChainProofs.sol (3)
26-27: LGTM! Well-documented constants with clear references
The new constants and their documentation are well-structured with:
- Clear references to the Ethereum consensus specs
- Proper explanation of the mathematical reasoning behind the values
- Consistent naming conventions
Also applies to: 32-36, 41-42, 46-56
144-160: LGTM! Clean integration of Deneb fork timestamp
The function properly integrates the new Deneb fork timestamp parameter and maintains all necessary validations.
283-284: LGTM! Proper parameterization of epoch calculation
The withdrawal epoch calculation correctly uses the provided slotsPerEpoch parameter while maintaining proper endianness handling.
test/foundry/Governance.t.sol (4)
38-38: LGTM: Network constants modularization
The addition of NetworkConstants import aligns with the PR's objective of making network-specific parameters configurable.
139-139: LGTM: Network-specific beacon genesis timestamp
Using NetworkConstants.getBeaconGenesisTimestamp() ensures correct initialization across different networks.
162-163: Verify networkConfig initialization
The ImmutableConfig struct includes two notable changes:
- beaconProxyBytecode field position has changed
- networkConfig is initialized with address(0)
Please verify if initializing networkConfig with address(0) is correct for the test environment.
#!/bin/bash
# Description: Check networkConfig usage in production code
# Expected: Find how networkConfig is typically initialized in non-test environments
# Search for ImmutableConfig struct initialization
ast-grep --pattern 'ImmutableConfig memory $_ = ImmutableConfig({
$$$
networkConfig: $_,
$$$
})'
# Search for networkConfig usage in the contract
rg -A 5 'networkConfig'143-143: Verify the zero address initialization for ExoCapsule
The ExoCapsule is being initialized with address(0). Please verify if this is the intended behavior.
✅ Verification successful
Based on the search results, I can now provide a final response:
Zero address initialization for ExoCapsule is correct in test context
The zero address initialization for ExoCapsule in test files is intentional and correct. The ExoCapsule contract is designed to work with both zero and non-zero network config addresses:
- In test environments,
address(0)is commonly used as seen across multiple test files - In integration/production scenarios, it accepts a network config address (as seen in
script/integration/1_DeployBootstrap.s.sol) - The contract handles both cases appropriately by falling back to constants when network config is zero (verified in
src/storage/ExoCapsuleStorage.sol)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ExoCapsule constructor usage across the codebase
# Expected: Find other instances of ExoCapsule initialization to confirm the pattern
# Search for ExoCapsule constructor calls
ast-grep --pattern 'new ExoCapsule($addr)'
# Search for ExoCapsule contract definition to understand the constructor
ast-grep --pattern 'contract ExoCapsule {
$$$
constructor($param) {
$$$
}
$$$
}'
Length of output: 18065
Script:
#!/bin/bash
# Let's search for ExoCapsule contract definition and constructor implementation
rg -A 10 "contract ExoCapsule"
# Also search for any other ExoCapsule instantiations
rg -A 2 "new ExoCapsule"
Length of output: 5542
test/foundry/unit/ClientChainGateway.t.sol (3)
40-40: LGTM: Good use of NetworkConstants for network-specific configuration
The change properly utilizes the NetworkConstants library to fetch network-specific beacon genesis timestamp, which aligns with the PR's objective to fix the issue of using the same constants across different networks.
Also applies to: 108-108
131-132: Verify networkConfig initialization with address(0)
The addition of networkConfig field aligns with the PR objectives. However, we should verify:
- If address(0) is the intended default value
- If there are any validation checks in the contract that might be bypassed
✅ Verification successful
Let me gather more information about the networkConfig usage to understand its validation and purpose better.
Let me try one more search to understand the complete context of networkConfig usage.
Let me check the INetworkConfig interface and ClientChainGateway implementation to complete the verification.
Setting networkConfig to address(0) is the correct implementation
The initialization of networkConfig with address(0) is valid and intentional because:
- The codebase implements a fallback mechanism where address(0) triggers the use of hardcoded NetworkConstants instead of a configurable INetworkConfig contract.
- ClientChainGateway doesn't directly use networkConfig - it's primarily used by BootstrapStorage for deposit contract resolution.
- The pattern is consistently used across all test cases and deployment scripts.
- There's explicit validation in BootstrapStorage that allows networkConfig to be zero:
// networkConfig is allowed to be 0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for networkConfig usage in the codebase
ast-grep --pattern 'struct ImmutableConfig {
$$$
address networkConfig;
$$$
}'
# Search for any validation of networkConfig
rg "networkConfig.*address\(0\)" -A 3
Length of output: 5922
Script:
#!/bin/bash
# Search for INetworkConfig interface definition
ast-grep --pattern 'interface INetworkConfig {
$$$
}'
# Search for NetworkConstants usage
ast-grep --pattern 'library NetworkConstants {
$$$
}'
# Search for any validation in ClientChainGateway
rg "networkConfig" src/ClientChainGateway.sol -A 3
Length of output: 233
Script:
#!/bin/bash
# Search for networkConfig in source files
rg "networkConfig" src/ -A 3
# Search for any files containing ClientChainGateway
fd ClientChainGateway
# Search for INetworkConfig interface
fd INetworkConfig
Length of output: 2455
Script:
#!/bin/bash
# Check INetworkConfig interface
cat src/interfaces/INetworkConfig.sol
# Check ClientChainGateway implementation
cat src/core/ClientChainGateway.sol
# Check ClientChainGatewayStorage
cat src/storage/ClientChainGatewayStorage.sol
Length of output: 12586
112-112: Verify the intention of initializing ExoCapsule with address(0)
While the tests pass with this initialization, it's important to verify that using address(0) as the initialization parameter is intentional and doesn't have any negative implications in production.
✅ Verification successful
Let me gather more information about the ExoCapsule constructor to understand how the address parameter is used.
Let me check the ExoCapsuleStorage contract to understand how the networkConfig parameter is used.
Let me check the inheritance chain and the actual usage of networkConfig in the storage contract.
Let me try one final search to understand the storage contract.
Based on the comprehensive search results, I can now provide the final response:
Using address(0) for networkConfig is a valid initialization pattern
The codebase explicitly supports initializing ExoCapsule with address(0) for networkConfig. When networkConfig is set to address(0), the system falls back to using NetworkConstants library instead of a NetworkConfig contract instance. This is evidenced by:
- The BootstrapStorage contract explicitly allows networkConfig to be 0 with the comment "networkConfig is allowed to be 0"
- The codebase has a fallback mechanism in multiple places when networkConfig is address(0)
- Most test files and deployment scripts intentionally use address(0) for networkConfig
- The integration deployment script is the only one using an actual NetworkConfig contract address
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ExoCapsule initializations to verify consistency
rg -l "new ExoCapsule\(" | xargs rg "new ExoCapsule\([^)]*\)"
# Search for any checks or requirements on the initialization parameter
ast-grep --pattern 'constructor($_) {
$$$
require($_ != address(0), $_);
$$$
}'
Length of output: 1312
Script:
#!/bin/bash
# Search for ExoCapsule constructor implementation
ast-grep --pattern 'contract ExoCapsule {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for any usage of the constructor parameter within ExoCapsule
rg -A 5 "constructor.*ExoCapsule"
Length of output: 498
Script:
#!/bin/bash
# Search for ExoCapsuleStorage constructor and its usage of networkConfig
ast-grep --pattern 'contract ExoCapsuleStorage {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for any usage of networkConfig in ExoCapsuleStorage
rg -A 5 "networkConfig" src/core/
Length of output: 597
Script:
#!/bin/bash
# Check the storage contract content
rg -B 2 -A 10 "contract ExoCapsuleStorage" src/core/
# Look for any references to networkConfig in storage or initialization
rg -B 2 -A 5 "networkConfig" src/core/ --type sol
Length of output: 132
Script:
#!/bin/bash
# Search for storage contract without file type restriction
rg -B 2 -A 10 "contract ExoCapsuleStorage"
# Search for networkConfig usage in any file
rg -B 2 -A 5 "networkConfig"
Length of output: 24090
test/foundry/BootstrapDepositNST.t.sol (1)
84-84: Verify the use of address(0) in ExoCapsule initialization
Using address(0) as a constructor parameter could indicate a potential issue. While this might be acceptable in a test environment, it's important to verify that the contract properly handles this null address.
Let's verify how address(0) is handled in the ExoCapsule contract:
src/storage/BootstrapStorage.sol (4)
4-4: LGTM: Import additions are well-organized
The new imports for network configuration management are properly organized with other interface imports.
Also applies to: 10-10
180-182: LGTM: Well-documented storage addition
The new mapping is properly documented and correctly placed after the storage gap to maintain compatibility with deployed contracts.
377-383: Verify deposit contract addresses across networks
The constructor now dynamically retrieves the deposit contract address based on the network configuration. This is a critical change that affects validator deposits.
#!/bin/bash
# Description: Verify deposit contract addresses across networks
# Check NetworkConstants for default deposit contract address
rg -A 2 "getDepositContractAddress" "src/libraries/"
# Check NetworkConfig implementations for network-specific addresses
ast-grep --pattern 'function getDepositContractAddress() $_ returns ($_) {
$$$
}'316-329: Verify contract deployment scripts for the new config field
The networkConfig field has been added to the ImmutableConfig struct. While the field is optional, deployment scripts should be reviewed to ensure they handle this new parameter correctly.
✅ Verification successful
Let me try a different search strategy to find deployment-related code and ImmutableConfig usage.
Based on the search results, I can now provide a final response as I have sufficient information about how the networkConfig field is handled in deployment scripts and tests.
No changes needed in deployment scripts for the optional networkConfig field
The networkConfig field has been properly handled in all deployment scripts and tests:
- All deployment scripts already include the field in their
ImmutableConfigstruct initialization - The field is correctly set to
address(0)in most deployments, which is valid as per the constructor's documentation - Integration deployment script (
script/integration/1_DeployBootstrap.s.sol) demonstrates proper usage with a non-zero address - The constructor in
BootstrapStorage.solexplicitly allowsnetworkConfigto be zero, as shown in the validation check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check deployment scripts for ImmutableConfig usage
# Search for ImmutableConfig struct initialization in deployment scripts
rg -A 5 "ImmutableConfig\(" "script/"
Length of output: 380
Script:
#!/bin/bash
# Search for files containing ImmutableConfig
rg -l "ImmutableConfig"
# Search for deployment-related files
fd "deploy|Deploy" --type f
# Get detailed context around ImmutableConfig usage
rg -B 2 -A 10 "ImmutableConfig"
Length of output: 27765
test/foundry/unit/ExoCapsule.t.sol (2)
Line range hint 1-600: Comprehensive test coverage for deposit and withdrawal scenarios
The test file provides good coverage of various scenarios including:
- Deposit verification
- Full and partial withdrawals
- Error cases (mismatched credentials, stale proofs, etc.)
- Non-beacon chain ETH withdrawals
This aligns well with the PR objectives of facilitating the deposit process and proving deposits for Bootstrap.
75-75: Verify the purpose of address(0) in ExoCapsule constructor
The ExoCapsule constructor is being called with address(0) as a parameter. This seems to be used for creating a phantom capsule for testing purposes, but the purpose and implications of this parameter should be clearly documented.
✅ Verification successful
Let me gather more context about the ExoCapsuleStorage to understand the implications of passing address(0).
Using address(0) in ExoCapsule constructor is valid for test purposes
The usage of address(0) in the test is valid and intentional. The ExoCapsuleStorage has a fallback mechanism when NETWORK_CONFIG is set to address(0), as seen in the getDenebHardForkTimestamp() function. This allows the contract to function in test environments without requiring a real network configuration contract.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ExoCapsule constructor implementations and usages
ast-grep --pattern 'contract ExoCapsule {
$$$
constructor($_) {
$$$
}
$$$
}'
# Search for any comments or documentation about the constructor parameter
rg -A 3 "constructor.*ExoCapsule"
Length of output: 337
Script:
#!/bin/bash
# Check ExoCapsuleStorage implementation and inheritance
ast-grep --pattern 'contract ExoCapsuleStorage {
$$$
}'
# Search for any usage or validation of networkConfig_ parameter
rg -B 2 -A 5 "networkConfig_"
Length of output: 1507
src/core/Bootstrap.sol (4)
29-29: LGTM: ValidatorContainer library integration
The ValidatorContainer library is properly imported and integrated using the using directive for bytes32[] arrays, which is consistent with its usage in the code.
Also applies to: 52-52
796-796: LGTM: Proper tracking of validator pubkeys
The stakerToPubkeys mapping is updated after successful verification of the deposit proof, maintaining an accurate record of validator keys for each staker.
827-834: LGTM: Well-documented pubkey count accessor
The getPubkeysCount function is properly implemented with clear documentation, providing a view into the number of validators associated with each staker address.
693-701: LGTM: Special handling for virtual native staked ETH token
Good practice to handle the virtual token (VIRTUAL_NST_ADDRESS) differently from regular ERC20 tokens. The hardcoded values are appropriate for representing native staked ETH.
✅ Verification successful
Based on the results, I can generate the final response as the verification is complete. The ripgrep results show consistent handling of VIRTUAL_NST_ADDRESS across the codebase:
Verified: VIRTUAL_NST_ADDRESS is consistently handled as a special case
The virtual native staked ETH token (VIRTUAL_NST_ADDRESS) is properly handled across the codebase with special cases for:
- Token info with hardcoded ETH values (name, symbol, decimals)
- TVL limit restrictions (not allowed)
- Deposit tracking and withdrawal logic
- Special withdrawal handling through ExoCapsule
- Proper error handling with specific error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that VIRTUAL_NST_ADDRESS is consistently handled across the codebase
rg -A 2 "VIRTUAL_NST_ADDRESS"
Length of output: 3307
script/integration/NetworkConfig.sol (1)
1-80: General Assessment: Code Quality and Compliance
The NetworkConfig contract is well-structured and effectively serves its purpose for integration testing. The use of constructor parameters and validation ensures that the contract is correctly initialized.
src/storage/ExoCapsuleStorage.sol (5)
4-7: Imports are appropriate and necessary
The addition of NetworkConstants and INetworkConfig imports correctly brings in the required interfaces and libraries for network configuration.
14-15: Clarification in documentation is helpful
The added comments explaining why the contract does not inherit from INetworkConfig enhance the code's readability and maintainability.
59-61: Immutable NETWORK_CONFIG variable correctly introduced
The introduction of the NETWORK_CONFIG immutable variable is appropriate. It allows the contract to reference the NetworkConfig contract dynamically or fallback to NetworkConstants when not set.
93-97: Constructor properly initializes NETWORK_CONFIG
The constructor correctly sets the NETWORK_CONFIG variable, ensuring that the contract has access to the correct network configuration upon deployment.
119-124: Possible typo in getSecondsPerEpoch function
The getSecondsPerEpoch function seems to retrieve seconds per epoch, but it calls getSecondsPerEpoch() which might return seconds per slot based on standard naming conventions. Please verify that getSecondsPerEpoch() returns the correct value.
Run the following script to check the return values of getSecondsPerEpoch:
src/core/ExoCapsule.sol (4)
135-136: Constructor updated with networkConfig_ parameter
The addition of the networkConfig_ parameter to the constructor and passing it to ExoCapsuleStorage enhances configurability. This change appears correct and aligns with the updates for dynamic network configurations.
209-209: Use of getSlotsPerEpoch() in withdrawalEpoch calculation
Replacing hardcoded values with getSlotsPerEpoch() improves flexibility across different network configurations. This ensures that withdrawalEpoch is accurately calculated based on the network's slot duration.
383-383: Utilization of getDenebHardForkTimestamp() for withdrawal validation
Incorporating getDenebHardForkTimestamp() into the validation process ensures that withdrawal container validation adapts to network upgrades. This enhances correctness and future-proofs the contract against protocol changes.
408-411: Refactored _timestampToEpoch to use dynamic genesis timestamp and epoch duration
The function now retrieves beaconChainGenesisTime using getBeaconGenesisTimestamp() and uses getSecondsPerEpoch() for epoch calculations. This improves adaptability to different network parameters and removes hardcoded values.
script/integration/1_DeployBootstrap.s.sol (3)
119-133: Initialization and Validation of Network Parameters
The network configuration parameters are correctly read from environment variables, and the validations ensure they are within acceptable ranges and types. The casting from uint256 to uint64 is safely guarded by require statements.
233-245: Loop Indexing Aligns with Token Arrays
The loops correctly start from i = 1 to skip the virtual token at whitelistTokens[0], ensuring that only actual tokens are processed. The use of vaults[i - 1] aligns with the adjusted indexing and prevents unintended interactions with the virtual token.
80-84: Verify Replacement of Hardcoded Constants with Configurable Parameters
To ensure consistency across different networks, verify that all instances of hardcoded constants like ETH1_DEPOSIT_ADDRESS and GENESIS_TIME have been replaced with the newly introduced environment variables or parameters.
Run the following script to search for any remaining hardcoded instances:
✅ Verification successful
Based on the search results, I can now generate the final response:
Hardcoded Constants Have Been Properly Replaced with Configurable Parameters
The codebase shows that the deposit address and genesis-related parameters are properly configured through environment variables and network-specific configurations:
- Deposit addresses are managed through
NetworkConstants.solfor different networks (mainnet, sepolia, holesky) - Genesis and network parameters are properly configured in
NetworkConfig.sol - The integration deployment script (
1_DeployBootstrap.s.sol) correctly uses environment variables:depositAddress = vm.envOr("INTEGRATION_DEPOSIT_ADDRESS", ...)- Network parameters like
denebTimestampare read from environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for hardcoded instances of ETH1_DEPOSIT_ADDRESS and GENESIS_TIME
# Expected Result: No occurrences of hardcoded constants in the codebase
rg -F 'ETH1_DEPOSIT_ADDRESS' --type solidity
rg -F 'GENESIS_TIME' --type solidity
Length of output: 147
Script:
#!/bin/bash
# Search for any hardcoded addresses or timestamps in Solidity files
rg -g '*.sol' -i '(0x[a-fA-F0-9]{40}|constant\s+[=]\s+\d+)' -A 2 -B 2
# Search for potential constant definitions related to deposit or genesis
rg -g '*.sol' -i '(deposit|genesis|eth1).*=.*[0-9x]' -A 2 -B 2
Length of output: 75778
test/foundry/ExocoreDeployer.t.sol (4)
49-50: Import NetworkConstants for centralized configuration
The addition of NetworkConstants import helps centralize network constants, ensuring consistency across the codebase.
334-334: Utilize NetworkConstants for beacon genesis timestamp
Initializing beaconOracle with EigenLayerBeaconOracle(NetworkConstants.getBeaconGenesisTimestamp()) centralizes the beacon genesis timestamp retrieval, enhancing consistency and maintainability.
339-339: Initialize ExoCapsule with address(0)
Passing address(0) to the ExoCapsule constructor is acceptable if the contract is designed to handle a null address. Ensure that any operations within ExoCapsule safely handle address(0) without causing unintended side effects.
364-365: Add networkConfig to ImmutableConfig with address(0)
Including networkConfig in BootstrapStorage.ImmutableConfig and initializing it to address(0) is acceptable if the downstream code can handle a null networkConfig address appropriately. Confirm that any logic dependent on networkConfig accounts for this possibility.
script/generate.js (5)
64-64: Ensure accurate prefix comparison in isValidBech32 function
The isValidBech32 function compares the decoded prefix with EXOCORE_BECH32_PREFIX. Confirm that EXOCORE_BECH32_PREFIX correctly represents the expected prefix for Exocore addresses.
368-374: Verify balance_list values with the team
There's a TODO comment indicating the need to check values with Qing. Ensure that the balance_list fields such as round_id, block, index, change, and balance are correctly set according to the application's requirements.
Please confirm these values to ensure accurate staker balance initialization.
804-817: Ensure the correct structure for oracle staker assets
The newly added staker_list_assets and staker_infos_assets should conform to the expected schema of the oracle module.
Confirm that these structures are correctly formed:
staker_list_assetsshould be an array of objects, each containingasset_idandstaker_listwithstaker_addrs.staker_infos_assetsshould includeasset_idandstaker_infos, wherestaker_infosis an array of staker information.
If needed, I can assist in reviewing the schema definitions.
73-73: Ensure all environment variables are defined
The BEACON_CHAIN_ENDPOINT variable is added to the destructured process.env. Confirm that this environment variable is properly defined in your environment or .env file to prevent undefined errors.
If it's not already in your .env.example, consider adding it for clarity.
55-56: Verify compatibility of imported modules
Modules imported from @lodestar/api and @lodestar/config/default are used. Ensure that these dependencies are installed and compatible with your project's Node.js version.
Run npm install @lodestar/api @lodestar/config if not already installed.
test/foundry/unit/Bootstrap.t.sol (10)
105-105: Initialization of capsuleImplementation with address(0)
The change to initialize capsuleImplementation with new ExoCapsule(address(0)) ensures compatibility with the updated constructor of ExoCapsule that requires an address parameter. This is appropriate and aligns with similar updates in other test files.
125-126: Verify the initialization of networkConfig with address(0)
In the initialization of BootstrapStorage.ImmutableConfig, the networkConfig field is set to address(0). Please ensure that this is intentional and that the zero address is acceptable in this context. If networkConfig is required by the contract logic, providing a valid address might be necessary to avoid potential issues.
1135-1136: Consistent usage of networkConfig in initialization tests
In the test15_Initialize_* functions, the networkConfig field is consistently set to address(0) within BootstrapStorage.ImmutableConfig. Since these tests check for initialization failures or edge cases, using the zero address is appropriate to validate that the contract correctly handles invalid configurations.
Also applies to: 1172-1173, 1210-1211, 1247-1248, 1285-1286, 1323-1324, 1360-1361, 1397-1398
Line range hint 1050-1066: Mocking ETHPOS deposit in test24_Stake
The test24_Stake function correctly sets up the environment by enabling native restaking and mocking the ETHPOS deposit. The use of vm.mockCall to simulate the deposit function of the ETHPOS contract ensures that the staking process can be tested without relying on external contracts.
Line range hint 1079-1088: Correctly handling disabled restaking in test24_Stake_WhenDisabled
The test24_Stake_WhenDisabled function appropriately tests the staking behavior when native restaking is disabled, confirming that the contract reverts as expected.
Line range hint 1090-1103: Proper capsule creation in test25_CreateExoCapsule
The test25_CreateExoCapsule function effectively tests the creation of an ExoCapsule and verifies that it is correctly registered to the owner. The assertions confirm that the capsule address is not zero and is properly associated with the owner.
Line range hint 1105-1114: Handling existing capsule in test25_CreateExoCapsule_AlreadyExists
In test25_CreateExoCapsule_AlreadyExists, the test ensures that attempting to create an ExoCapsule when one already exists correctly reverts. This validates the contract's ability to prevent duplicate capsule creation.
Line range hint 1155-1170: Validate state changes in test26_VerifyAndDepositNativeStake
The test26_VerifyAndDepositNativeStake function mocks the capsule to return a specific deposit value and checks the changes in deposit amounts. Ensure that the mocked return value accurately represents the expected behavior, and that the state changes reflect the intended logic.
Line range hint 1172-1181: Correct revert handling in test26_VerifyAndDepositNativeStake_WithEther
The test correctly expects a revert when verifyAndDepositNativeStake is called with a non-zero value, adhering to the contract's requirement that no Ether should be sent with this transaction.
Line range hint 1378-1386: Consistent error handling in test functions
Across the test functions test16_SetSpawnTime, test17_SetOffsetDuration, etc., error handling is correctly implemented using vm.expectRevert. Ensure that error messages and selectors used in these reverts are consistent with the contract's definitions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/forge-ci.yml (3)
Line range hint
186-187: Consider using more flexible Python version constraintsThe Python version is pinned to a very specific patch version (3.12.4). This could cause unnecessary failures if the exact version isn't available in the runner.
Consider using a more flexible version constraint:
- python-version: '3.12.4' + python-version: '3.12'
Line range hint
190-193: Add explicit error handling for storage layout generationThe storage layout comparison lacks explicit error handling for missing files or failed commands.
Add error checking and file existence validation:
- name: Generate target branch layout files run: | + if [ ! -f "src/core/ExocoreGateway.sol" ]; then + echo "ExocoreGateway.sol not found" + exit 1 + fi forge inspect src/core/ExocoreGateway.sol:ExocoreGateway storage-layout --pretty > ExocoreGateway_target.txt + if [ ! -f "ExocoreGateway_target.txt" ]; then + echo "Failed to generate storage layout" + exit 1 + fi
Line range hint
71-79: Improve caching strategy for better efficiencyThe current caching implementation could be enhanced for better hit rates and reliability.
Consider these improvements:
- name: Cache build artifacts uses: actions/cache/save@v3 with: path: | ./lib ./out ./cache ./broadcast - key: ${{ runner.os }}-build-${{ github.sha }} + key: ${{ runner.os }}-build-${{ hashFiles('foundry.toml', '**/Cargo.lock') }}-${{ github.sha }} + restore-keys: | + ${{ runner.os }}-build-${{ hashFiles('foundry.toml', '**/Cargo.lock') }}- + ${{ runner.os }}-build-This change:
- Includes dependency files in the cache key for proper cache busting
- Provides fallback caches for partial restoration
- Maintains the SHA in the key for uniqueness
This reverts commit 183dfab.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (18)
script/integration/prove.sh (1)
9-21: Add URL format validation for endpoint variables.While the existence of environment variables is checked, their format should also be validated to ensure they are valid URLs.
Add URL validation:
for var in "${vars[@]}"; do if [ -z "${!var}" ]; then echo "Error: $var must be set" exit 1 fi + # Validate URL format for endpoint variables + if [[ "$var" == *"ENDPOINT"* || "$var" == *"RPC"* ]]; then + if ! [[ "${!var}" =~ ^https?:// ]]; then + echo "Error: $var must be a valid HTTP/HTTPS URL" + exit 1 + fi + fi done.env.example (2)
15-16: Add instructions for obtaining the Etherscan API key.Please add a comment explaining how to obtain the Etherscan API key and any specific requirements for contract verification.
# For contract verification -ETHERSCAN_API_KEY= +# Get your API key from https://etherscan.io/apis +ETHERSCAN_API_KEY=
24-35: Document ETH PoS parameters and their derivation.The ETH PoS related parameters lack documentation about their purpose and how they're derived. This is particularly important for values like
DEPOSIT_DATA_ROOTwhich is derived from multiple parameters.-# Specially ETH PoS related parameters. +# Ethereum Proof of Stake (PoS) Configuration +# These parameters must match your local testnet configuration + INTEGRATION_DEPOSIT_ADDRESS=0x6969696969696969696969696969696969696969 + +# Consensus layer timing configuration INTEGRATION_SECONDS_PER_SLOT=4 INTEGRATION_SLOTS_PER_EPOCH=3 + +# Network initialization timestamps (Unix timestamp format) INTEGRATION_BEACON_GENESIS_TIMESTAMP= INTEGRATION_DENEB_TIMESTAMP= + +# Native Staking Token (NST) configuration INTEGRATION_NST_DEPOSITOR=0x47c99abed3324a2707c28affff1267e45918ec8c3f20b8aa892e8b065d2942dd -# margin tank lunch prison top episode peanut approve dish seat nominee illness + +# Validator credentials +# WARNING: These are example values. Generate your own for testing. +# Mnemonic: margin tank lunch prison top episode peanut approve dish seat nominee illness INTEGRATION_PUBKEY=0x98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9 INTEGRATION_SIGNATURE=0x922a316bdc3516bfa66e88259d5e93e339ef81bc85b70e6c715542222025a28fa1e3644c853beb8c3ba76a2c5c03b726081bf605bde3a16e1f33f902cc1b6c01093c19609de87da9383fa4b1f347bd2d4222e1ae5428727a7896c8e553cc8071 -# derived from pubkey + network params == chain id + genesis {fork version + validators root} + +# Deposit data root (derived from: pubkey + network params + chain id + genesis {fork version + validators root}) INTEGRATION_DEPOSIT_DATA_ROOT=0x456934ced8f08ff106857418a6d885ba69d31e1b7fab9a931be06da25490cd1d🧰 Tools
🪛 Gitleaks
32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
script/integration/2_VerifyDepositNST.s.sol (1)
64-71: Consider moving hardcoded delegation values to configuration.The delegation address and amount are hardcoded in the test script. Consider moving these to environment variables or configuration file for better maintainability and flexibility across different test environments.
+ string memory delegationAddress = vm.envOr( + "INTEGRATION_DELEGATION_ADDRESS", + "exo1rtg0cgw94ep744epyvanc0wdd5kedwql73vlmr" + ); + uint256 delegationAmount = vm.envOr("INTEGRATION_DELEGATION_AMOUNT", 18 ether); bootstrap.delegateTo( - "exo1rtg0cgw94ep744epyvanc0wdd5kedwql73vlmr", + delegationAddress, address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE), - 18 ether + delegationAmount );test/foundry/unit/NetworkConfig.t.sol (4)
13-13: Consider using a more flexible approach for test addressesInstead of hardcoding the deposit contract address, consider:
- Using
makeAddr("deposit")to generate deterministic addresses for testing- Adding a comment explaining why this specific address was chosen if it represents a real contract
- address public constant DEPOSIT_CONTRACT_ADDRESS = 0x1234567890AbcdEF1234567890aBcdef12345678; + // Generate deterministic address for testing + address public constant DEPOSIT_CONTRACT_ADDRESS = makeAddr("deposit");
20-28: Consider adding post-deployment validations in setUpThe setup function could benefit from additional assertions to ensure the contract is properly initialized:
function setUp() public { vm.chainId(31_337); networkConfig = new NetworkConfig( DEPOSIT_CONTRACT_ADDRESS, DENEB_TIMESTAMP, SLOTS_PER_EPOCH, SECONDS_PER_SLOT, BEACON_GENESIS_TIMESTAMP ); + // Validate initial state + assertEq(address(networkConfig).code.length > 0, true, "Contract deployment failed"); + assertEq(block.chainid, ALLOWED_CHAIN_ID, "Chain ID mismatch"); }
52-55: Enhance epoch-related test coverageThe
testGetSecondsPerEpochfunction could be expanded to include additional test cases:function testGetSecondsPerEpoch() public { assertEq(networkConfig.getSecondsPerEpoch(), SECONDS_PER_SLOT * SLOTS_PER_EPOCH, "Seconds per epoch mismatch"); + + // Test epoch boundary calculations + uint256 timestamp = BEACON_GENESIS_TIMESTAMP; + uint256 secondsPerEpoch = networkConfig.getSecondsPerEpoch(); + + // Verify epoch start + assertEq((timestamp - BEACON_GENESIS_TIMESTAMP) % secondsPerEpoch, 0, "Not at epoch boundary"); + + // Verify mid-epoch + timestamp += secondsPerEpoch / 2; + assertTrue((timestamp - BEACON_GENESIS_TIMESTAMP) % secondsPerEpoch != 0, "Should not be at epoch boundary"); }
86-90: Strengthen invalid input testingConsider expanding the negative test cases to include more edge cases:
function testRevertInvalidSlotsPerEpoch() public { + // Test zero value vm.expectRevert("Slots per epoch must be set for integration network"); new NetworkConfig(DEPOSIT_CONTRACT_ADDRESS, DENEB_TIMESTAMP, 0, SECONDS_PER_SLOT, BEACON_GENESIS_TIMESTAMP); + + // Test extremely large value + vm.expectRevert("Slots per epoch value is unreasonable"); + new NetworkConfig(DEPOSIT_CONTRACT_ADDRESS, DENEB_TIMESTAMP, type(uint64).max, SECONDS_PER_SLOT, BEACON_GENESIS_TIMESTAMP); + + // Test non-power-of-two value + vm.expectRevert("Slots per epoch must be a power of two"); + new NetworkConfig(DEPOSIT_CONTRACT_ADDRESS, DENEB_TIMESTAMP, 31, SECONDS_PER_SLOT, BEACON_GENESIS_TIMESTAMP); }Similar improvements could be applied to other negative test cases.
script/integration/1_DeployBootstrap.s.sol (7)
Line range hint
86-91: Consider removing hardcoded private keys from the code.While these are default Anvil test keys, it's best practice to avoid hardcoding any private keys, even test ones, directly in the source code. Consider moving these to a separate test configuration file.
146-148: Document the purpose of index 0 reservation.The comment indicates that index 0 is reserved for
VIRTUAL_STAKED_ETH_ADDRESS, but this design choice should be documented more clearly as it affects multiple functions in the contract.
211-213: Consider making the initialization buffer configurable.The 24-hour buffer for initialization is hardcoded. Consider making this configurable through environment variables to support different testing scenarios.
218-220: Document the purpose of dummy implementation upgrade values.The implementation upgrade address is set to
0x1with dummy data"123456". These values should be documented to explain their purpose in the testing context.
245-246: Consider using exact approval amounts.Using
type(uint256).maxfor token approvals, while common, might be unnecessary for testing. Consider approving only the required amount for better testing accuracy.
Line range hint
393-396: Improve random number generation implementation.The current random number generation using
block.timestampandblock.prevrandaois not secure for production use. While this might be acceptable for testing, consider adding a comment explicitly stating this limitation.function random(uint256 _range) internal view returns (uint256) { - // Basic random number generation; consider a more robust approach for production + // WARNING: This is a simplified random number generation for testing purposes only. + // DO NOT use this in production as it's vulnerable to manipulation by miners/validators. return (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % (_range - 1)) + 1; }
385-389: Consider making the deployment output path configurable.The deployment JSON file path is hardcoded to
script/integration/deployments.json. Consider making this configurable through environment variables to support different CI/CD environments.script/generate.mjs (3)
45-45: Add missing semicolon after constant declarationThere's a missing semicolon at the end of the constant declaration, which can cause issues in JavaScript parsing.
Apply this diff to add the semicolon:
-const VIRTUAL_STAKED_ETH_ADDR = "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE" +const VIRTUAL_STAKED_ETH_ADDR = "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE";
133-133: Handle errors when parsing exchange ratesWhen processing
INTEGRATION_EXCHANGE_RATES, invalid inputs can cause exceptions. Ensure the code handles parsing errors gracefully.Consider adding error handling:
-const exchangeRates = INTEGRATION_EXCHANGE_RATES.split(',').map(Decimal); +const exchangeRates = INTEGRATION_EXCHANGE_RATES.split(',').map(rateString => { + try { + return new Decimal(rateString); + } catch (error) { + throw new Error(`Invalid exchange rate '${rateString}': ${error.message}`); + } +});
316-316: Validate response fromgetStateValidatorsWhen calling
api.beacon.getStateValidators, ensure that the response contains the expected data and handle cases where validators may not be found.Add a check before processing
validatorStates:const validatorStates = (await api.beacon.getStateValidators({stateId: stateRoot, validatorIds: pubKeys})).value(); +if (!validatorStates || validatorStates.length === 0) { + console.error(`No validator states found for pubKeys: ${pubKeys}`); + continue; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.env.example(1 hunks)package.json(1 hunks)script/generate.mjs(9 hunks)script/integration/1_DeployBootstrap.s.sol(10 hunks)script/integration/2_VerifyDepositNST.s.sol(1 hunks)script/integration/BeaconOracle.sol(1 hunks)script/integration/deposit.sh(1 hunks)script/integration/prove.sh(1 hunks)test/foundry/unit/NetworkConfig.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- script/integration/BeaconOracle.sol
🧰 Additional context used
📓 Learnings (2)
script/integration/2_VerifyDepositNST.s.sol (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/2_VerifyDepositNST.s.sol:41-66
Timestamp: 2024-11-18T08:59:31.314Z
Learning: In integration testing scripts like `script/integration/2_VerifyDepositNST.s.sol`, additional validations beyond file existence checks are left to the transaction and need not be explicitly added in the script.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/2_VerifyDepositNST.s.sol:29-39
Timestamp: 2024-11-18T08:59:05.369Z
Learning: In `script/integration/2_VerifyDepositNST.s.sol`, the fallback value for `NST_DEPOSITOR` can be retained because it originates from Anvil default keys and is acceptable to keep.
Learnt from: adu-web3
PR: ExocoreNetwork/exocore-contracts#121
File: test/foundry/BootstrapDepositNST.t.sol:170-232
Timestamp: 2024-11-12T10:03:21.957Z
Learning: In `test/foundry/BootstrapDepositNST.t.sol`, adu-web3 prefers to keep lengthy test functions that cover multiple functionalities within a single test function like `test_VerifyAndDepositNativeStake`.
test/foundry/unit/NetworkConfig.t.sol (2)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: test/foundry/unit/NetworkConfig.t.sol:2-2
Timestamp: 2024-11-18T08:59:49.059Z
Learning: In the `test/foundry/unit/NetworkConfig.t.sol` file, pinning the Solidity version to a specific version is unnecessary; using `pragma solidity ^0.8.0` is acceptable in this codebase.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: test/foundry/unit/NetworkConfig.t.sol:63-101
Timestamp: 2024-11-18T08:59:46.426Z
Learning: In the `NetworkConfig` contract, overflow checks for epoch calculations are unnecessary because overflows will cause a panic.
🪛 Gitleaks
.env.example
32-32: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (11)
script/integration/prove.sh (1)
90-93: 🛠️ Refactor suggestion
Add error handling for Forge script execution.
The Forge script execution lacks error handling and result validation.
Add error handling:
export INTEGRATION_NST_DEPOSITOR=$INTEGRATION_NST_DEPOSITOR
-forge script script/integration/2_VerifyDepositNST.s.sol --skip-simulation \
+
+echo "Executing Forge script..."
+if ! forge script script/integration/2_VerifyDepositNST.s.sol --skip-simulation \
--rpc-url $CLIENT_CHAIN_RPC --broadcast \
- --evm-version cancun # required, otherwise you get EvmError: NotActivated
+ --evm-version cancun; then # required, otherwise you get EvmError: NotActivated
+ echo "Error: Forge script execution failed"
+ exit 1
+fi
+
+echo "Forge script executed successfully"⛔ Skipped due to learnings
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/2_VerifyDepositNST.s.sol:41-66
Timestamp: 2024-11-18T08:59:31.314Z
Learning: In integration testing scripts like `script/integration/2_VerifyDepositNST.s.sol`, additional validations beyond file existence checks are left to the transaction and need not be explicitly added in the script.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/2_VerifyDepositNST.s.sol:29-39
Timestamp: 2024-11-18T08:59:05.369Z
Learning: In `script/integration/2_VerifyDepositNST.s.sol`, the fallback value for `NST_DEPOSITOR` can be retained because it originates from Anvil default keys and is acceptable to keep.
script/integration/2_VerifyDepositNST.s.sol (4)
1-16: LGTM! Well-organized imports and appropriate license.
The file structure follows best practices with clear organization of imports, separating standard libraries, core contracts, and utility libraries.
17-28: LGTM! Clean contract structure with appropriate state variables.
The contract is well-structured with clear state variable declarations and proper library usage.
29-40: LGTM! Robust setup with proper validation.
The setup function includes appropriate error handling and initialization. The fallback value for INTEGRATION_NST_DEPOSITOR is acceptable as it uses Anvil default keys for testing purposes.
68-68: Verify the native token address convention.
The hardcoded native token address 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE is commonly used to represent the native token, but let's verify this convention in the codebase.
✅ Verification successful
The native token address convention 0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE is consistently used across the codebase
The address is consistently defined as a constant VIRTUAL_STAKED_ETH_ADDRESS or VIRTUAL_NST_ADDRESS throughout the codebase, including in core contracts, tests, and scripts. This convention is well-documented in ExocoreGateway.sol where it's specifically noted as being used for Ethereum.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the native token address convention usage across the codebase
# Search for the native token address usage
rg -i "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE"
# Search for any constants or configurations defining this address
ast-grep --pattern 'constant address $_ = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE)'
Length of output: 1854
test/foundry/unit/NetworkConfig.t.sol (1)
1-104: Well-structured and comprehensive test suite!
The test file demonstrates good testing practices with:
- Clear documentation
- Comprehensive coverage of both positive and negative cases
- Proper use of Forge testing utilities
- Well-organized test structure
While there are opportunities for enhancement, the current implementation provides a solid foundation for testing the NetworkConfig contract.
script/integration/1_DeployBootstrap.s.sol (3)
121-142: Well-structured network parameter validation.
The validation of network parameters is thorough and includes:
- Non-zero checks for timestamps
- Range validation for uint64 parameters
- Length validation for pubkey and signature
- Non-zero check for deposit data root
260-269: Well-implemented NST staking with proper validations.
The stakeNST function correctly:
- Checks for existing capsule
- Creates new capsule if needed
- Uses validated signature and deposit data
Line range hint 164-165: Review TVL limit calculation logic.
The TVL limit calculation (myToken.totalSupply() / ((i + 1) * 20)) uses a simplistic approach that might not be suitable for production. Consider making these limits configurable through environment variables.
script/generate.mjs (2)
46-46: Ensure correct initialization of GWEI_TO_WEI
When using Decimal, initializing with 1e9 may introduce precision issues.
Verify that new Decimal(1e9) accurately represents 1,000,000,000 without precision loss. If there's a risk, initialize using a string:
-const GWEI_TO_WEI = new Decimal(1e9);
+const GWEI_TO_WEI = new Decimal('1000000000');799-809: Check for empty staker lists and info
Ensure that nativeTokenDepositors and staker_infos are populated before adding them to the genesis JSON. Empty arrays may cause issues downstream.
Add a check before assignment:
-if (nativeTokenDepositors.length > 0) {
genesisJSON.app_state.oracle.staker_list_assets = [
{
asset_id: VIRTUAL_STAKED_ETH_ADDR.toLowerCase() + clientChainSuffix,
staker_list: {
staker_addrs: nativeTokenDepositors,
}
}
];
-}
-if (staker_infos.length > 0) {
genesisJSON.app_state.oracle.staker_infos_assets = [{
asset_id: VIRTUAL_STAKED_ETH_ADDR.toLowerCase() + clientChainSuffix,
staker_infos: staker_infos,
}];
-}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
script/integration/deposit.sh (5)
9-26: Consider adding value validation for critical environment variablesWhile the existence check is good, consider adding validation for the format and content of critical variables like:
INTEGRATION_PUBKEY(should be a valid public key format)INTEGRATION_SIGNATURE(should be a valid signature format)INTEGRATION_BEACON_CHAIN_ENDPOINT(should be a valid URL)for var in "${vars[@]}"; do if [ -z "${!var}" ]; then echo "Error: $var must be set" exit 1 fi + # Validate pubkey format + if [ "$var" = "INTEGRATION_PUBKEY" ]; then + if ! [[ "${!var}" =~ ^0x[a-fA-F0-9]{96}$ ]]; then + echo "Error: $var must be a valid 48-byte hex string" + exit 1 + fi + fi + # Validate signature format + if [ "$var" = "INTEGRATION_SIGNATURE" ]; then + if ! [[ "${!var}" =~ ^0x[a-fA-F0-9]{192}$ ]]; then + echo "Error: $var must be a valid 96-byte hex string" + exit 1 + fi + fi + # Validate endpoint URL + if [ "$var" = "INTEGRATION_BEACON_CHAIN_ENDPOINT" ]; then + if ! [[ "${!var}" =~ ^https?:// ]]; then + echo "Error: $var must be a valid HTTP(S) URL" + exit 1 + fi + fi done
28-48: Add timeout handling for network requestsThe curl commands should include timeout parameters to prevent hanging in case of network issues.
-if ! curl -s -X GET "$INTEGRATION_BEACON_CHAIN_ENDPOINT/eth/v1/beacon/genesis" -H "accept: application/json" | jq . >"$SCRIPT_DIR/genesis.json"; then +if ! curl -s --max-time 10 -X GET "$INTEGRATION_BEACON_CHAIN_ENDPOINT/eth/v1/beacon/genesis" -H "accept: application/json" | jq . >"$SCRIPT_DIR/genesis.json"; then echo "Error: Failed to fetch genesis data from the beacon chain" exit 1 fi -if ! curl -s -X GET "$INTEGRATION_BEACON_CHAIN_ENDPOINT/eth/v1/config/spec" | jq >"$SCRIPT_DIR/spec.json"; then +if ! curl -s --max-time 10 -X GET "$INTEGRATION_BEACON_CHAIN_ENDPOINT/eth/v1/config/spec" | jq >"$SCRIPT_DIR/spec.json"; then echo "Error: Failed to fetch spec data from the beacon chain" exit 1 fi
57-57: Add validation for deposit contract addressThe deposit contract address should be validated to ensure it's a valid Ethereum address.
deposit_address=$(jq -r .data.DEPOSIT_CONTRACT_ADDRESS "$SCRIPT_DIR/genesis.json") +if ! [[ "$deposit_address" =~ ^0x[a-fA-F0-9]{40}$ ]]; then + echo "Error: Invalid deposit contract address format" + exit 1 +fi
70-84: Optimize environment variable exportsSome variables are unnecessarily re-exported. Variables that are already in the environment don't need to be exported again.
-export INTEGRATION_VALIDATOR_KEYS=$INTEGRATION_VALIDATOR_KEYS -export INTEGRATION_STAKERS=$INTEGRATION_STAKERS -export INTEGRATION_TOKEN_DEPLOYERS=$INTEGRATION_TOKEN_DEPLOYERS -export INTEGRATION_CONTRACT_DEPLOYER=$INTEGRATION_CONTRACT_DEPLOYER -export INTEGRATION_PUBKEY=$INTEGRATION_PUBKEY -export INTEGRATION_SIGNATURE=$INTEGRATION_SIGNATURE -export INTEGRATION_DEPOSIT_DATA_ROOT=$INTEGRATION_DEPOSIT_DATA_ROOT -export INTEGRATION_NST_DEPOSITOR=$INTEGRATION_NST_DEPOSITOR +# Export only newly created variables export INTEGRATION_BEACON_GENESIS_TIMESTAMP=$timestamp export INTEGRATION_DENEB_TIMESTAMP=$timestamp export INTEGRATION_SECONDS_PER_SLOT=$seconds_per_slot export INTEGRATION_SLOTS_PER_EPOCH=$slots_per_epoch export INTEGRATION_DEPOSIT_ADDRESS=$deposit_address export SENDER=$sender
89-94: Add validation for script file existenceCheck if the deployment script exists before attempting to run it.
+SCRIPT_PATH="script/integration/1_DeployBootstrap.s.sol" +if [ ! -f "$SCRIPT_PATH" ]; then + echo "Error: Deployment script not found: $SCRIPT_PATH" + exit 1 +fi + forge script \ - --skip-simulation script/integration/1_DeployBootstrap.s.sol \ + --skip-simulation "$SCRIPT_PATH" \ --rpc-url $CLIENT_CHAIN_RPC \ --broadcast -v \ --sender $SENDER \ --evm-version cancunscript/generate.mjs (1)
271-277: Simplify oracle token handling logicThe current implementation using object assignment and conditional pushing could be simplified using a Map or Set for better readability and performance.
- if (oracleToken.name in oracleTokens) { - oracleTokens[oracleToken.name].asset_id += ',' + oracleToken.asset_id; - offset += 1; - } else { - oracleTokens[oracleToken.name] = oracleToken; - oracleTokenFeeders.push(oracleTokenFeeder); - } + const existingToken = oracleTokens.get(oracleToken.name); + if (existingToken) { + existingToken.asset_id += ',' + oracleToken.asset_id; + offset += 1; + } else { + oracleTokens.set(oracleToken.name, oracleToken); + oracleTokenFeeders.push(oracleTokenFeeder); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
script/generate.mjs(11 hunks)script/integration/deposit.sh(1 hunks)script/integration/prove.sh(1 hunks)src/core/Bootstrap.sol(5 hunks)src/storage/BootstrapStorage.sol(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- script/integration/prove.sh
- src/storage/BootstrapStorage.sol
🔇 Additional comments (7)
script/generate.mjs (3)
14-16: LGTM: Well-structured constants and clear documentation
The new constants and array modifications are well-organized with clear purposes. The comment explaining the oracle token mapping adds valuable context.
Also applies to: 23-23, 44-46
319-386: Well-structured validator state handling
The validator state handling logic is complex but well-organized with clear comments explaining the slashing and reward calculations. The code effectively handles various edge cases including:
- Inactive validators
- Balance validation against maximum and ejection thresholds
- Slashing and reward calculations
814-826: LGTM: Clean implementation of native restaking for ETH
The implementation of native restaking for ETH in the oracle state is well-structured and maintains consistency with the rest of the genesis state.
src/core/Bootstrap.sol (4)
29-29: LGTM: ValidatorContainer library integration
The ValidatorContainer library is properly imported and integrated using the correct data type (bytes32[]).
Also applies to: 52-53
693-701: LGTM: Special handling for native staked ETH
The function correctly handles the virtual native staked ETH token by returning appropriate hardcoded values while maintaining dynamic deposit amount tracking.
827-834: LGTM: Clear and well-documented getter function
The getPubkeysCount function is properly implemented with clear documentation explaining its purpose and requirements.
796-796: Verify validatorIndex casting safety
The casting of proof.validatorIndex to bytes32 assumes the validator index fits within 32 bytes. Please verify that this assumption holds true across all possible validator indices to prevent data truncation.
✅ Verification successful
Let me analyze the validator index usage in the codebase to verify if there are any constraints or potential issues with casting to bytes32.
Let me check one more thing to understand how the validator index is used in the Beacon Chain proofs.
Based on the analysis of the codebase, I can now provide a final response about the validator index casting safety.
Validator index casting to bytes32 is safe
The casting of proof.validatorIndex to bytes32 is safe because:
- The validator index is defined as uint256 in the BeaconChainProofs library, which means it already fits within 32 bytes (256 bits)
- The only constraint found is that validator index must be non-zero, as checked in multiple test files
- The validator index is used in Merkle proof verification where it's combined with VALIDATOR_TREE_ROOT_INDEX using bitwise operations, indicating it's designed to work within the 256-bit space
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any validator index definitions or constraints in the codebase
rg -A 2 "validatorIndex|validator.*[iI]ndex"
Length of output: 97281
Script:
#!/bin/bash
# Search for validator index type definitions and constraints
ast-grep --pattern 'type $_ = $type;' | grep -i "validator"
# Search for validator index declarations and assignments
ast-grep --pattern 'validatorIndex = $_'
# Search for validator index comparisons or bounds checks
rg -A 2 "validatorIndex [<>=]|MAX.*VALIDATOR|validator.*limit"
Length of output: 4685
Script:
#!/bin/bash
# Search for validator index usage in BeaconChainProofs
rg -A 5 "validatorIndex|validator.*[iI]ndex" src/libraries/BeaconChainProofs.sol
# Check validator index type in the proof struct
ast-grep --pattern 'struct $_ {
$$$
validatorIndex $_
$$$
}'
Length of output: 1338
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.env.example (1)
16-17: Add instructions for obtaining Etherscan API key.Consider adding a comment explaining how to obtain the API key from Etherscan and any specific requirements for contract verification.
-# For contract verification -ETHERSCAN_API_KEY= +# For contract verification +# Get your API key from https://etherscan.io/apis +# Required for verifying smart contracts on Etherscan +ETHERSCAN_API_KEY=script/generate.mjs (3)
130-141: Consider extracting epoch calculation logic into a helper function.The finalized epoch calculation logic could be more readable if extracted into a dedicated helper function.
+function calculateFinalizedEpoch(api, slotsPerEpoch) { + const lastHeader = await api.beacon.getBlockHeader({blockId: "finalized"}).value(); + const finalizedSlot = lastHeader.header.message.slot; + const finalizedEpoch = Math.floor(finalizedSlot / slotsPerEpoch); + + if (finalizedSlot % slotsPerEpoch != 0) { + return { + epoch: finalizedEpoch, + header: await api.beacon.getBlockHeader({blockId: finalizedEpoch * slotsPerEpoch}).value() + }; + } + + return { epoch: finalizedEpoch, header: lastHeader }; +}
351-377: Add detailed documentation for slashing and rewards logic.The slashing and rewards calculation logic is complex and would benefit from detailed documentation explaining:
- The different scenarios (slashing vs rewards)
- The calculation methodology
- The impact on withdrawable amounts
Line range hint
117-847: Consider modularizing the genesis file generation logic.The
updateGenesisFilefunction is quite large and handles multiple concerns. Consider breaking it down into smaller, focused modules:
- Token and oracle configuration
- Validator and operator management
- Native token deposit handling
- Genesis file structure management
This would improve maintainability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.env.example(2 hunks)script/generate.mjs(13 hunks)
🧰 Additional context used
📓 Learnings (2)
.env.example (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: .env.example:28-29
Timestamp: 2024-11-18T15:57:37.587Z
Learning: In `.env.example`, the configuration values `INTEGRATION_BEACON_GENESIS_TIMESTAMP`, `INTEGRATION_DENEB_TIMESTAMP`, `INTEGRATION_BASE_GENESIS_FILE_PATH`, and `INTEGRATION_RESULT_GENESIS_FILE_PATH` are intentionally left empty to be provided by the person running them or loaded on the fly.
script/generate.mjs (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🪛 Gitleaks
.env.example
33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
.env.example (6)
Line range hint 1-15: LGTM! Well-documented basic configuration.
The basic configuration section is clear and follows good practices by:
- Including clear comments about test/mock nature of keys
- Documenting RPC endpoints
- Explaining key generation commands
25-28: LGTM! Network parameters are well-defined.
The ETH PoS parameters are appropriately configured for integration testing with reasonable test values.
29-30: LGTM! Empty values are intentional.
As previously clarified, these values are intentionally left empty to be provided at runtime or loaded dynamically.
Also applies to: 42-43
37-41: LGTM! Integration endpoints are properly configured.
The endpoints and parameters are appropriately set for local testing environment.
19-24: Verify test-only nature of integration keys.
These appear to be Anvil's default test keys. Let's verify they're not used in any production environment.
✅ Verification successful
Integration test keys are correctly used only for local testing
The keys are exclusively used in integration testing with Anvil (local testnet). This is verified by:
- The script is explicitly marked for Anvil/Prysm PoS testing
- Keys are loaded from
.envwith Anvil's default test keys as fallback - Clear comments indicate these are "default values for Anvil's usual mnemonic"
- The script is located in
script/integration/directory - No production deployment code references these keys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for these keys in any deployment or production configuration files
rg -l "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80|0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d" | grep -iE 'prod|deploy|mainnet'
Length of output: 219
Script:
#!/bin/bash
# Let's check the content of the deployment script to verify the context
cat script/integration/1_DeployBootstrap.s.sol
Length of output: 19992
31-36: Verify test validator credentials.
Let's ensure these validator credentials are only used for testing purposes.
✅ Verification successful
Validator credentials confirmed for integration testing only
These credentials are safely used only in integration testing scripts and not in production:
- Found in
script/integration/directory for test deployment and verification - Referenced as
ANVIL_NST_DEPOSITORin integration scripts - Used alongside other test keys (ANVIL prefixed constants)
- No occurrences in production or mainnet deployment files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for these validator credentials in deployment configurations
rg -l "0x98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9" | grep -iE 'prod|deploy|mainnet'
Length of output: 139
Script:
#!/bin/bash
# Search for validator credentials in all files
rg -l "0x98db81971df910a5d46314d21320f897060d76fdf137d22f0eb91a8693a4767d2a22730a3aaa955f07d13ad604f968e9"
# Also search for the depositor key
rg -l "0x47c99abed3324a2707c28affff1267e45918ec8c3f20b8aa892e8b065d2942dd"
# Look for any environment files that might contain these credentials
fd -t f ".env"
Length of output: 283
Script:
#!/bin/bash
# Check the context of these credentials in integration scripts
rg -A 5 -B 5 "0x47c99abed3324a2707c28affff1267e45918ec8c3f20b8aa892e8b065d2942dd" script/integration/
# Look for any test-related comments or configurations
rg -i "test.*validator|integration.*validator"
Length of output: 6029
🧰 Tools
🪛 Gitleaks
33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
script/generate.mjs (3)
14-16: LGTM! Well-structured constants and modern import syntax.
The token meta information and oracle mappings are properly aligned, and the ES6 module imports are appropriate for the .mjs extension.
Also applies to: 21-24, 44-56
73-92: LGTM! Environment validation is complete.
The environment variables are properly validated before use.
264-270: Verify token ID calculation logic.
The token ID calculation (i + 1 - offset) might need verification to ensure it doesn't generate duplicate IDs when tokens are mapped to the same oracle feed.
✅ Verification successful
Token ID calculation is correctly handling duplicates
The token ID calculation (i + 1 - offset) is working as intended. The code shows a proper duplicate handling mechanism where:
- The
offsetis incremented when a token name already exists inoracleTokens - For duplicates, the asset IDs are concatenated with commas in the existing entry
- The offset subtraction in token ID ensures unique and sequential IDs even when tokens map to the same oracle feed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential duplicate token IDs in the contract
ast-grep --pattern 'token_id: $ID'
Length of output: 36
Script:
#!/bin/bash
# Search for token ID related code with more context
rg -n "token_id.*offset" -B 2 -A 2
# Search for token mapping or configuration patterns
rg -n "tokens.*map" -B 2 -A 2
# Look for any offset calculations
rg -n "offset.*=" -B 2 -A 2
Length of output: 89077
Script:
#!/bin/bash
# Search for token mapping logic and offset usage
rg -n -B 5 -A 5 "offset.*=" script/generate.mjs
# Look for token array handling
rg -n -B 5 -A 5 "oracleTokens" script/generate.mjs
# Check for duplicate prevention logic
rg -n -B 5 -A 5 "oracleToken.name in oracleTokens" script/generate.mjs
Length of output: 2950
- The `nstETH` token needs to be added both as a token and as a feeder - The `nstETH` token's `asset_id` needs to map back to the native (non-staked) token's `asset_id`
In the event that the Bootstrap contract has whitelisted `nstETH` but not `ETH`, the `nstETH` entry will only fetch the effective balance and the token price from the `ETH` entry will be missing. This commit fixes that by manually adding an `ETH` entry to the oracle state in such an event.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
script/generate.mjs (4)
14-24: Add documentation for token mapping logic.Consider adding comments to explain:
- The relationship between
tokenMetaInfosandtokenNamesForOraclearrays- Why nstETH asset_id is mapped to ETH in the oracle
84-93: Enhance environment variable validation error messages.Consider providing more specific error messages that indicate which variable is missing:
- throw new Error('One or more required environment variables are missing.'); + const missingVars = [ + ['INTEGRATION_BEACON_CHAIN_ENDPOINT', INTEGRATION_BEACON_CHAIN_ENDPOINT], + ['CLIENT_CHAIN_RPC', CLIENT_CHAIN_RPC], + ['INTEGRATION_BOOTSTRAP_ADDRESS', INTEGRATION_BOOTSTRAP_ADDRESS], + ['INTEGRATION_BASE_GENESIS_FILE_PATH', INTEGRATION_BASE_GENESIS_FILE_PATH], + ['INTEGRATION_RESULT_GENESIS_FILE_PATH', INTEGRATION_RESULT_GENESIS_FILE_PATH], + ['INTEGRATION_EXCHANGE_RATES', INTEGRATION_EXCHANGE_RATES] + ].filter(([name, value]) => !value).map(([name]) => name); + throw new Error(`Missing required environment variables: ${missingVars.join(', ')}`);
386-389: Define constants for validator status values.Consider extracting the validator status string into a named constant for better maintainability:
+const VALIDATOR_STATUS = { + ACTIVE_ONGOING: 'active_ongoing' +}; + - if (validator.status != "active_ongoing") { + if (validator.status != VALIDATOR_STATUS.ACTIVE_ONGOING) {
402-428: Document slashing and rewards logic.The slashing and rewards logic is complex and would benefit from detailed documentation explaining:
- The conditions that trigger slashing
- How the slashing amount is calculated
- How rewards are distributed
Add comments like:
// Case 1: Slashing - When deposit is greater than effective balance // This happens due to downtime penalties or other slashing conditions // The slashing amount is first taken from withdrawable balance // If withdrawable balance is insufficient, the remaining is taken from deposit // Case 2: Rewards - When effective balance is greater than deposit // This represents accumulated rewards // The rewards are added to both deposit and withdrawable amounts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.env.example(2 hunks)script/generate.mjs(13 hunks)script/integration/prove.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- script/integration/prove.sh
🧰 Additional context used
📓 Learnings (2)
.env.example (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: .env.example:28-29
Timestamp: 2024-11-18T15:57:37.587Z
Learning: In `.env.example`, the configuration values `INTEGRATION_BEACON_GENESIS_TIMESTAMP`, `INTEGRATION_DENEB_TIMESTAMP`, `INTEGRATION_BASE_GENESIS_FILE_PATH`, and `INTEGRATION_RESULT_GENESIS_FILE_PATH` are intentionally left empty to be provided by the person running them or loaded on the fly.
script/generate.mjs (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🪛 Gitleaks (8.21.1)
.env.example
33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (5)
.env.example (4)
Line range hint 1-18: LGTM! Well-documented configuration section.
The basic configuration is well-structured with clear comments indicating the source and purpose of test keys.
🧰 Tools
🪛 Gitleaks (8.21.1)
33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
25-36: Good separation of network-specific parameters
This addresses the bug mentioned in PR objectives where same constants were used across networks. Each network can now have its own:
- Deposit address
- Network parameters (seconds per slot, slots per epoch)
- Genesis and fork timestamps
- Deposit credentials
The empty timestamp fields are intentionally left blank as they are loaded dynamically.
🧰 Tools
🪛 Gitleaks (8.21.1)
33-33: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
37-43: LGTM! Integration setup aligns with PR objectives
The configuration supports the integration test scripts for deposit process:
- Local development endpoints for beacon chain and proof generation
- Exchange rates format compatible with price feeder tool
- File paths intentionally left empty for dynamic loading
19-24: Verify these are non-production test keys
These integration test keys appear to follow the Anvil/Hardhat test account pattern. Let's verify they're not used in production.
✅ Verification successful
Keys confirmed as test-only, used exclusively in integration tests
The keys are only found in script/integration/1_DeployBootstrap.s.sol where they are explicitly used as Anvil test accounts, and in the .env.example template. No occurrence in actual configuration files or production code, confirming these are indeed non-production test keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these keys are used in production configurations
# Search for these keys in non-example configuration files
echo "Searching for potential usage of test keys in actual configuration files..."
rg -l "0xac0974bec39a17e36ba4a6b4d238ff944bacb478cbed5efcae784d7bf4f2ff80|0x59c6995e998f97a5a0044966f0945389dc9e86dae88c7a8412f4603b6b78690d" --type-add 'config:*.{env,config,ini,yaml,yml}' -t config
Length of output: 511
Script:
#!/bin/bash
# Let's try a broader search approach for these test keys
# First, let's find all configuration and environment files
echo "Finding all configuration and environment files..."
fd -e env -e yaml -e yml -e config -e ini
echo -e "\nSearching for test keys in these files..."
# Search for the first few characters of each key to catch any format variations
rg "ac0974bec|59c6995e|5de4111a|7c852118|47e179ec|8b3a350c" $(fd -e env -e yaml -e yml -e config -e ini)
# Also check for potential usage in source files
echo -e "\nChecking source files for these keys..."
rg "ac0974bec|59c6995e|5de4111a|7c852118|47e179ec|8b3a350c" -g '!{*.env.example,*.md}'
Length of output: 2326
script/generate.mjs (1)
894-896: LGTM! Error handling improvements look good.
The error logging now includes both the error message and stack trace in a single console.error call.
Yes; the plan is to generate a genesis and then pass it to exocored and start a small network with a few features for testing. |
|
✅ The Slither Analysis workflow has completed successfully. Check the workflow run for details. (3332c96) |
|
✅ The Forge CI workflow has completed successfully. Check the workflow run for details. (3332c96) |
|
✅ The Solhint workflow has completed successfully. Check the workflow run for details. (3332c96) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/storage/BootstrapStorage.sol (1)
Line range hint
372-387: Add validation for deposit contract addressWhile the logic for determining the deposit contract address is correct, consider adding validations to ensure:
- The returned deposit contract address is not zero
- The INetworkConfig interface is implemented correctly when provided
address depositContract; if (config.networkConfig == address(0)) { depositContract = NetworkConstants.getDepositContractAddress(); } else { + // Validate that networkConfig implements INetworkConfig + try INetworkConfig(config.networkConfig).getDepositContractAddress() returns (address _depositContract) { + depositContract = _depositContract; + } catch { + revert Errors.InvalidNetworkConfig(); + } - depositContract = INetworkConfig(config.networkConfig).getDepositContractAddress(); } +if (depositContract == address(0)) { + revert Errors.InvalidDepositContractAddress(); +} ETH_POS = IETHPOSDeposit(depositContract);src/core/Bootstrap.sol (2)
698-706: Consider using constants for Virtual NST token informationWhile the implementation correctly handles the virtual Native Staked ETH token, consider extracting the hardcoded values into constants for better maintainability.
+ // Constants for Virtual NST token + string constant VIRTUAL_NST_NAME = "Native Staked ETH"; + string constant VIRTUAL_NST_SYMBOL = "ETH"; + uint8 constant VIRTUAL_NST_DECIMALS = 18; if (tokenAddress == VIRTUAL_NST_ADDRESS) { return TokenInfo({ - name: "Native Staked ETH", - symbol: "ETH", + name: VIRTUAL_NST_NAME, + symbol: VIRTUAL_NST_SYMBOL, tokenAddress: tokenAddress, - decimals: 18, + decimals: VIRTUAL_NST_DECIMALS, depositAmount: depositsByToken[tokenAddress] }); }
832-848: LGTM: Well-documented view functionsThe new view functions are well-implemented and properly documented. They provide essential functionality for querying the number of pubkeys and validators associated with a staker.
Consider adding
@devtags to provide more implementation details in the NatSpec documentation:/// @notice Returns the number of pubkeys (across all validators) deposited /// by a staker. The deposit must include deposit + verification for inclusion /// into the beacon chain. /// @param stakerAddress the address of the staker. /// @return uint256 The number of pubkeys deposited by the staker. + /// @dev This count is incremented in verifyAndDepositNativeStake when a new validator is verified function getPubkeysCount(address stakerAddress) external view returns (uint256) {script/generate.mjs (1)
118-1086: Consider breaking down the large function into smaller, focused functions.The
updateGenesisFilefunction is over 900 lines long and handles multiple concerns. This makes it difficult to maintain and test.Consider breaking it down into smaller functions like:
updateGenesisTimeupdateAssetStateupdateValidatorStateupdateDelegationStateupdateOracleState
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.json(1 hunks)script/generate.mjs(1 hunks)src/core/Bootstrap.sol(6 hunks)src/storage/BootstrapStorage.sol(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Learnings (1)
script/generate.mjs (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🔇 Additional comments (6)
src/storage/BootstrapStorage.sol (2)
4-10: LGTM! Well-organized imports
The new imports for NetworkConstants and INetworkConfig are properly organized and necessary for the enhanced network configuration functionality.
180-186: LGTM! Well-documented state variables for native restaking
The new mappings are:
- Well-documented with clear NatSpec comments
- Properly placed after the storage gap to maintain compatibility
- Essential for tracking validator relationships in native restaking
src/core/Bootstrap.sol (2)
517-521: LGTM: Proper tracking of validator-token relationships
The code correctly maintains a list of validators for each staker-token pair, which is essential for delegation management and querying purposes.
29-29: Verify the ValidatorContainer library usage
The ValidatorContainer library is imported and used with bytes32[], but its actual usage in the contract isn't immediately clear from the visible changes.
Also applies to: 52-53
✅ Verification successful
Let me verify the ValidatorContainer library usage by searching for its implementation and usage patterns.
ValidatorContainer library usage is properly implemented
The ValidatorContainer library is correctly imported and used in Bootstrap.sol:
- It's imported from "../libraries/ValidatorContainer.sol"
- The library is properly attached to bytes32[] using the
using ValidatorContainer for bytes32[]directive - The library is used in validator-related functions that handle validator container proofs
- The library implementation in ValidatorContainer.sol contains the necessary verification logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ValidatorContainer library usage
ast-grep --pattern 'using ValidatorContainer for bytes32[];'
# Search for methods from ValidatorContainer being called
rg -A 5 'bytes32\[\].*\.'
Length of output: 86800
Script:
#!/bin/bash
# Search for ValidatorContainer library implementation
rg -A 10 "library ValidatorContainer"
# Search for ValidatorContainer usage in Bootstrap.sol
rg -A 5 "ValidatorContainer" src/core/Bootstrap.sol
# Search for bytes32[] usage in Bootstrap.sol
rg -A 5 "bytes32\[\]" src/core/Bootstrap.sol
Length of output: 2850
script/generate.mjs (2)
46-46: 🛠️ Refactor suggestion
Validate the virtual staked ETH address format.
This is a critical constant used throughout the code. Consider adding a validation function to ensure it matches the expected format.
-const VIRTUAL_STAKED_ETH_ADDR = "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE";
+const VIRTUAL_STAKED_ETH_ADDR = "0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE";
+
+// Add validation
+const isValidVirtualStakedETHAddr = (addr) => {
+ return /^0x[Ee]{40}$/.test(addr);
+};
+
+if (!isValidVirtualStakedETHAddr(VIRTUAL_STAKED_ETH_ADDR)) {
+ throw new Error('Invalid virtual staked ETH address format');
+}Likely invalid or redundant comment.
13-25: Consider adding validation to ensure token arrays maintain order alignment.
The comment indicates that these arrays must maintain order alignment with whitelistTokens, but there's no runtime validation to ensure this requirement is met.
|
✅ The Compare Storage Layouts workflow has completed successfully. Check the workflow run for details. (3332c96) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
script/generate.mjs (2)
58-69: Enhance Bech32 address validationThe
isValidBech32function could be improved:
- Add length validation for the decoded words
- Consider validating the checksum
const isValidBech32 = (address) => { try { const { prefix, words } = decode(address); - if (!prefix || !words.length) { + if (!prefix || !words || words.length < 1) { return false; } + // Add minimum/maximum length validation + if (words.length < 32 || words.length > 64) { + return false; + } return prefix === EXOCORE_BECH32_PREFIX; } catch (error) { // If there's any error in decoding, return false return false; } }
182-193: Consider using early returns for validationThe nested if conditions and error handling could be simplified using early returns and more descriptive error messages.
- const existingChainIdIndex = genesisJSON.app_state. - assets.client_chains.findIndex( - chain => - chain.layer_zero_chain_id === clientChainInfo.layer_zero_chain_id - ); - if (existingChainIdIndex >= 0) { - // If found, raise an error - throw new Error( - `An entry with layer_zero_chain_id - ${clientChainInfo.layer_zero_chain_id} already exists.` - ); - } + if (genesisJSON.app_state.assets.client_chains.some( + chain => chain.layer_zero_chain_id === clientChainInfo.layer_zero_chain_id + )) { + throw new Error( + `Duplicate chain ID: ${clientChainInfo.layer_zero_chain_id} already exists in genesis file` + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
script/generate.mjs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
script/generate.mjs (3)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🔇 Additional comments (4)
script/generate.mjs (4)
37-38: Review hardcoded addresses for security implications
The code contains hardcoded addresses including VIRTUAL_STAKED_ETH_ADDR. While this might be a protocol constant, it's good practice to validate these addresses and possibly make them configurable.
Also applies to: 45-46
129-141: 🛠️ Refactor suggestion
Improve error handling for beacon API interactions
The beacon API interactions could benefit from more robust error handling and validation of the response data.
- const api = getClient({baseUrl: INTEGRATION_BEACON_CHAIN_ENDPOINT}, {config});
- const spec = (await api.config.getSpec()).value();
+ const api = getClient({
+ baseUrl: INTEGRATION_BEACON_CHAIN_ENDPOINT,
+ timeoutMs: 10000 // Add timeout
+ }, {config});
+ const specResponse = await api.config.getSpec();
+ if (!specResponse.ok) {
+ throw new Error(`Failed to get beacon spec: ${specResponse.error}`);
+ }
+ const spec = specResponse.value();Likely invalid or redundant comment.
5-11: Consider adding validation for chain information constants
The clientChainInfo object contains critical chain configuration. Consider adding runtime validation to ensure all required fields are present and have valid values.
✅ Verification successful
Chain configuration values appear to be safe and consistent
The clientChainInfo configuration in script/generate.mjs appears to be correctly defined with appropriate values for Sepolia testnet. The values are:
- Consistent with Sepolia's Layer Zero chain ID (40161)
- Using standard Ethereum address length (20 bytes)
- Setting reasonable finalization blocks (10) for testnet environment
No validation issues were found in the codebase that would contradict these values, and the configuration appears to be used appropriately in the genesis file generation process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if these chain parameters match the network configuration
rg -A 5 "layer_zero_chain_id.*40161|finalization_blocks.*10" --type js
Length of output: 72
Script:
#!/bin/bash
# Let's search for any chain configuration or validation related code
rg -l "chain.*id|finalization.*block" --type js
# Also search for any validation logic
ast-grep --pattern 'function validate$_($$$) { $$$ }'
Length of output: 105
Script:
#!/bin/bash
# Let's check the broader context of where this configuration is used
cat script/generate.mjs
# Also check for any related configuration files
fd -e json -e js "config|chain"
Length of output: 43930
Script:
#!/bin/bash
# Let's check the hardhat config for any chain configuration validation
cat hardhat.config.js
# Also check the client chain gateway config
cat script/redeployClientChainGateway.json
Length of output: 800
13-25:
Ensure token arrays maintain strict ordering
The comment states that arrays must maintain the same order as whitelistTokens, but there's no runtime validation to ensure this consistency. Consider adding validation or using a more robust data structure.
|
Tested the cases around native restaking, single ETH validator. Start by depositing 32 ETH and staking 18 ETH.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
script/generate.mjs (4)
30-30: Avoid hardcoding chain IDs.The
layer_zero_chain_id: 0is hardcoded with a comment indicating it's a "virtual chain". Consider making this configurable through environment variables.- "layer_zero_chain_id": 0, // virtual chain + "layer_zero_chain_id": process.env.NATIVE_CHAIN_LAYER_ZERO_ID || 0, // virtual chain
458-466: Consider extracting magic numbers into named constants.The balance validation uses magic numbers (8, 16) for slashing thresholds. These should be extracted into named constants for better maintainability.
+const MIN_SLASHED_BALANCE = ejectionBalance.div(2); // 8 ETH +const MAX_SLASHED_BALANCE = ejectionBalance; // 16 ETH -if (effectiveBalance.gt(ejectionBalance)) { +if (effectiveBalance.gt(MAX_SLASHED_BALANCE)) { throw new Error( `The effective balance of ${effectiveBalance} is greater than the ejection balance.` ); -} else if (effectiveBalance.lt(ejectionBalance.div(2))) { +} else if (effectiveBalance.lt(MIN_SLASHED_BALANCE)) { throw new Error( `The effective balance of ${effectiveBalance} is less than half the ejection balance.` ); }
515-570: Consider breaking down complex slashing logic into smaller functions.The slashing calculation logic is complex and handles multiple scenarios. Breaking it into smaller, well-named functions would improve readability and maintainability.
Example refactoring:
+function calculateSlashProportions(totalDelegated, slashFromWithdrawable, withdrawableValue) { + let pendingSlashAmount = slashFromWithdrawable.minus(withdrawableValue); + if (pendingSlashAmount.gt(ZERO_DECIMAL)) { + return { + slashFromWithdrawable: withdrawableValue, + pendingSlashAmount + }; + } + return { + slashFromWithdrawable, + pendingSlashAmount: ZERO_DECIMAL + }; +} // Use in main code -let slashFromWithdrawable = depositValue.minus(totalEffectiveBalance); -let pendingSlashAmount = slashFromWithdrawable.minus(withdrawableValue); -if (pendingSlashAmount.gt(ZERO_DECIMAL)) { - slashFromWithdrawable = withdrawableValue; -} else { - pendingSlashAmount = new Decimal(0); -} +const { slashFromWithdrawable, pendingSlashAmount } = calculateSlashProportions( + totalDelegated, + depositValue.minus(totalEffectiveBalance), + withdrawableValue +);
944-948: Be explicit about power value truncation and precision.The power calculation truncates decimal places and converts to integer, which could lead to precision loss. Consider documenting the precision requirements and handling edge cases.
validators.forEach((val) => { - val.power = val.power.toFixed(0); + // Truncate to integer as required by the consensus engine + // Note: This may lose precision for small power differences + val.power = val.power.toFixed(0); totalPower += parseInt(val.power, 10); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
script/generate.mjs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
script/generate.mjs (4)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:106-111
Timestamp: 2024-11-28T23:47:59.457Z
Learning: For the `generateAVSAddr` function in `script/generate.mjs`, input validation is unnecessary because the script is not user-facing.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🔇 Additional comments (3)
script/generate.mjs (3)
102-118: LGTM! Well-structured utility functions.
The utility functions are concise, focused, and serve clear purposes.
139-142: Verify slot calculation edge case handling.
The code changes the header if the finalized slot is not at an epoch boundary. This could potentially miss important state information.
✅ Verification successful
Let me analyze the code further to understand the slot calculation handling better.
The slot calculation edge case is handled correctly
The code's behavior of fetching the block header at the epoch boundary when the finalized slot is not at an epoch boundary is actually the correct approach. This ensures consistent state retrieval at epoch boundaries, which is important for beacon chain synchronization. The implementation:
- First gets the latest finalized header
- If that header is not at an epoch boundary, it fetches the header at the start of that epoch
- This guarantees we always work with epoch-aligned state roots
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar slot calculations in the codebase
# to ensure consistent handling of epoch boundaries
rg -A 5 "finalizedSlot|slotsPerEpoch"
Length of output: 7850
Script:
#!/bin/bash
# Check for any additional slot/epoch boundary handling in the codebase
ast-grep --pattern 'finalizedSlot % $_'
Length of output: 111
Script:
#!/bin/bash
# Look for any related beacon chain header retrieval logic
rg -A 5 "getBlockHeader"
Length of output: 832
Script:
#!/bin/bash
# Check for any comments or documentation about epoch boundary handling
rg -i "epoch boundary|slot boundary"
Length of output: 38
12-25: Consider adding validation for token array ordering.
The comment indicates that tokenMetaInfos and tokenNamesForOracle must maintain the same order as whitelistTokens. This ordering dependency could be a source of bugs.
Consider creating a single source of truth by using an object that maps tokens to their metadata and oracle names, eliminating the ordering dependency.
✅ Verification successful
Let me analyze the token arrays usage in the codebase to verify if there are potential issues with the ordering dependency.
The array ordering dependency is properly validated and safe to use.
The code already implements proper validation to ensure array length consistency and correct token mapping:
- Length validation checks ensure all arrays have matching sizes:
if (supportedTokensCount != tokenMetaInfos.length) {
throw new Error(...)
}
if (supportedTokensCount != tokenNamesForOracle.length) {
throw new Error(...)
}- Token type validation ensures correct mapping:
if (tokenNamesForOracle[i].toLowerCase().startsWith('nst')) {
if (token.tokenAddress != VIRTUAL_STAKED_ETH_ADDR) {
throw new Error('Oracle name refers to NST token but this is LST');
}
}While the arrays must maintain order alignment, the code has sufficient safeguards to prevent misalignment issues. The original review comment's concern is addressed by the existing validation logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the token arrays are used consistently across the codebase
# Search for any other arrays that might need to maintain the same order
rg -A 5 "whitelistTokens|tokenMetaInfos|tokenNamesForOracle"
Length of output: 115487
Script:
#!/bin/bash
# Search for the whitelistTokens array definition and its usage in the code
ast-grep --pattern 'const whitelistTokens = $_'
# Search for any validation or mapping logic between these arrays
rg -A 5 "tokenMetaInfos|tokenNamesForOracle" script/generate.mjs
Length of output: 1902
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
script/integration/1_DeployBootstrap.s.sol (1)
249-254: Add comments to clarify loop indexingIn the loops where indexing starts from 1 (e.g.,
for (uint256 i = 1; i < whitelistTokens.length; i++)), it's to skip theVIRTUAL_STAKED_ETH_ADDRESSat index 0. Adding comments to explain this can improve code readability and prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
script/integration/1_DeployBootstrap.s.sol(10 hunks)
🔇 Additional comments (4)
script/integration/1_DeployBootstrap.s.sol (4)
110-113: Avoid hardcoding private keys in code
Including private keys directly in the code, even for testing purposes, poses security risks and can lead to accidental exposure. Consider moving these sensitive values into environment variables or secure configurations.
292-295: Avoid including sensitive information in comments
Mentioning mnemonics or other sensitive keys in comments can pose security risks. Even if placeholders, it's best to remove or sanitize these comments to prevent accidental exposure.
397-401: Verify the usage of the serialize method with StdJson
Ensure that the serialize method is correctly used with the StdJson library to serialize deployment addresses. Improper usage may lead to incorrect JSON formation or runtime errors.
Line range hint 406-409: Random number generation is acceptable for testing purposes
The random() function uses block.timestamp and block.prevrandao, which is acceptable in a testing environment. For production code, consider using a more secure source of randomness.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
script/integration/1_DeployBootstrap.s.sol (3)
Line range hint
74-147: Enhance validation for network configuration parameters.The network configuration validation is thorough but could be improved for better error handling and security.
Consider these enhancements:
- require(denebTimestamp > 0, "Deneb timestamp must be set"); + require(denebTimestamp > block.timestamp, "Deneb timestamp must be in the future"); - require(beaconGenesisTimestamp > 0, "Beacon timestamp must be set"); + require(beaconGenesisTimestamp <= block.timestamp, "Beacon genesis must be in the past");
402-409: Consider enhancing deployment tracking.The deployment tracking could be improved to include more metadata and validation.
Consider adding:
string memory key = "deployments"; key.serialize("beaconOracleAddress", address(beaconOracle)); +key.serialize("deploymentTimestamp", block.timestamp); +key.serialize("networkId", block.chainid); string memory start = key.serialize("bootstrapAddress", address(bootstrap)); +require(address(bootstrap) != address(0), "Bootstrap deployment failed"); vm.writeFile("script/integration/deployments.json", start);
Line range hint
417-420: Improve random number generation for production.The current random number generation is not suitable for production use.
Consider using a more secure random number generation method:
- return (uint256(keccak256(abi.encodePacked(block.timestamp, block.prevrandao))) % (_range - 1)) + 1; + // For testing purposes only + require(_range > 1, "Invalid range"); + bytes32 randomBytes = keccak256(abi.encodePacked( + block.timestamp, + block.prevrandao, + msg.sender + )); + return (uint256(randomBytes) % (_range - 1)) + 1;script/generate.mjs (6)
12-25: Consider using a more robust token mapping system.The current implementation relies on array order matching between
tokenMetaInfosandtokenNamesForOracle. This coupling could lead to subtle bugs if arrays get out of sync during maintenance.Consider using an object mapping or enum to maintain the relationship:
-const tokenMetaInfos = [ - 'Staked ETH', - 'Exocore testnet ETH', - 'Lido wrapped staked ETH', -]; -const tokenNamesForOracle = [ - 'nstETH', 'ETH', 'wstETH' -] +const TOKEN_CONFIG = { + STAKED_ETH: { + metaInfo: 'Staked ETH', + oracleName: 'nstETH' + }, + EXOCORE_ETH: { + metaInfo: 'Exocore testnet ETH', + oracleName: 'ETH' + }, + LIDO_ETH: { + metaInfo: 'Lido wrapped staked ETH', + oracleName: 'wstETH' + } +};
30-31: Extract magic numbers into named constants.The code uses magic numbers for
layer_zero_chain_idandaddress_length.Consider defining these as named constants:
+const VIRTUAL_CHAIN_ID = 0; +const ETH_ADDRESS_LENGTH = 20; const nativeChain = { "name": "Exocore", "meta_info": "The (native) Exocore chain", "finalization_blocks": 10, - "layer_zero_chain_id": 0, // virtual chain - "address_length": 20, + "layer_zero_chain_id": VIRTUAL_CHAIN_ID, + "address_length": ETH_ADDRESS_LENGTH, }
108-113: Consider using a more descriptive prefix for AVS address generation.The prefix 'chain-id-prefix' is generic and its purpose isn't immediately clear.
Consider using a more descriptive prefix:
function generateAVSAddr(chainID) { - const ChainIDPrefix = 'chain-id-prefix'; + const AVS_ADDR_PREFIX = 'exocore-avs-addr'; - const hash = keccak256(ChainIDPrefix + chainID); + const hash = keccak256(AVS_ADDR_PREFIX + chainID); return '0x' + hash.slice(-40); }
784-785: Address TODO comment regarding oracle module setup.The comment indicates a temporary solution that needs to be updated once the oracle module is set up.
Would you like me to help create a GitHub issue to track this improvement task?
1080-1080: Address TODO comment regarding staking data migration.The comment indicates missing implementation for copying staking data from previous genesis.
Would you like me to help create a GitHub issue to track this missing implementation?
989-1034: Potential performance bottleneck in nested loops.The code contains deeply nested loops with multiple contract calls, which could be slow for a large number of operators or tokens.
Consider:
- Batching contract calls using Promise.all
- Using Map/Set for faster lookups
- Caching repeated contract method calls
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
script/generate.mjs(1 hunks)script/integration/1_DeployBootstrap.s.sol(9 hunks)
🧰 Additional context used
📓 Learnings (1)
script/generate.mjs (4)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:106-111
Timestamp: 2024-11-28T23:47:59.457Z
Learning: For the `generateAVSAddr` function in `script/generate.mjs`, input validation is unnecessary because the script is not user-facing.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:837-840
Timestamp: 2024-11-18T16:33:50.341Z
Learning: In `script/generate.mjs`, the `updateGenesisFile` function is already wrapped in a `try/catch` block, so adding additional `try/catch` blocks within this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.mjs:130-141
Timestamp: 2024-11-18T16:33:51.592Z
Learning: In `script/generate.mjs`, the `updateGenesisFile()` function is wrapped in a `try/catch` block, so additional error handling inside this function is unnecessary.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/generate.js:115-122
Timestamp: 2024-11-18T14:47:08.736Z
Learning: In `script/generate.js`, the `updateGenesisFile` function is already wrapped in a global try-catch block, so additional error handling within that function may not be necessary.
🔇 Additional comments (5)
script/integration/1_DeployBootstrap.s.sol (4)
176-178: LGTM: Network configuration integration.
The network configuration is properly integrated with BeaconOracle and ExoCapsule contracts.
Also applies to: 182-182
300-303:
Remove sensitive information from comments.
The comments contain references to mnemonics and key generation instructions that should be moved to documentation.
Apply this diff to remove sensitive information:
- // the mnemonics corresponding to the consensus public keys are given here. to recover,
- // echo "${MNEMONIC}" | exocored init localnet --chain-id exocorelocal_233-1 --recover
- // the value in this script is this one
- // exocored keys consensus-pubkey-to-bytes --output json | jq -r .bytes
+ // Consensus public keys for validators
+ // For key generation instructions, see docs/validator-setup.md151-154: Review the TVL limit for virtual staked ETH.
Setting TVL limit to 0 for virtual staked ETH might need additional consideration.
✅ Verification successful
Based on the search results, I can now provide a final response:
Setting TVL limit to 0 for virtual staked ETH is the intended behavior.
The codebase consistently sets TVL limit to 0 for virtual staked ETH across multiple files, and this is by design because:
- In test/foundry/ExocoreDeployer.t.sol: Comment explicitly states "no limit for native staked ETH"
- In script/3_Setup.s.sol: Comment states "irrelevant for native restaking"
- TVL limits are not enforced for virtual staked ETH as it represents natively staked ETH on Ethereum, which has different validation mechanisms through the beacon chain proof verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other occurrences of TVL validation for virtual staked ETH
rg -A 5 "VIRTUAL_STAKED_ETH_ADDRESS.*tvlLimits"
Length of output: 49
Script:
#!/bin/bash
# Let's search for TVL limit validation patterns in the codebase
rg -A 5 "tvlLimits"
# Also search for any references to virtual staked ETH
rg "VIRTUAL_STAKED_ETH" -A 5
Length of output: 60483
280-289: Verify ETH value in stake function.
The hardcoded 32 ETH value should be validated against network requirements.
✅ Verification successful
Based on the search results, I can see that this is a native staking contract for Ethereum validators, and the 32 ETH value is a fundamental requirement of the Ethereum network for validator deposits. The documentation and code show that this is specifically for depositing to the beacon chain, where 32 ETH is the standard validator deposit amount. This is not a configurable value as it's a core Ethereum protocol requirement.
The 32 ETH stake value is correct as it matches Ethereum network requirements.
The value is hardcoded because it's a protocol-level requirement for Ethereum validator deposits, not a configurable parameter that needs validation against network requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any network-specific stake amount configurations
ast-grep --pattern 'function stake($$$) {
$$$
{value: $_ ether}
$$$
}'
Length of output: 78
Script:
#!/bin/bash
# Search for stake function implementations and their value parameters
rg -U "function stake.*?\{.*?value:.*?ether.*?\}" --multiline
# Search for any constant or configuration related to stake amount
rg -i "stake.*amount|minimum.*stake|deposit.*amount" -A 2
Length of output: 108775
Script:
#!/bin/bash
# Search for stake function and its value parameter in the Bootstrap contract
rg -U "function stake.*?\{.*?value:.*?ether.*?\}" --type sol
# Search for any constants or configurations related to stake amount
rg -i "DEPOSIT_AMOUNT|STAKE_AMOUNT|MIN_STAKE|VALIDATOR_DEPOSIT" --type sol
# Search for validator deposit requirements
rg -i "validator.*deposit.*requirement|beacon.*deposit|eth2.*deposit" --type sol
Length of output: 306
Script:
#!/bin/bash
# Search for stake function and its value parameter in the Bootstrap contract
rg -U "function stake.*?\{.*?value:.*?ether.*?\}"
# Search for any constants or configurations related to stake amount
rg -i "DEPOSIT_AMOUNT|STAKE_AMOUNT|MIN_STAKE|VALIDATOR_DEPOSIT"
# Search for validator deposit requirements or comments
rg -i "validator.*deposit|beacon.*deposit|eth2.*deposit" -A 2
Length of output: 92104
script/generate.mjs (1)
137-139:
Potential race condition in finalized block header retrieval.
The code fetches the finalized block header and calculates the epoch, but there could be a new finalization between these operations.
Consider atomically fetching both values:
-let lastHeader = (await api.beacon.getBlockHeader({blockId: "finalized"})).value();
-const finalizedSlot = lastHeader.header.message.slot;
-const finalizedEpoch = Math.floor(finalizedSlot / slotsPerEpoch);
+const finalizedData = await Promise.all([
+ api.beacon.getBlockHeader({blockId: "finalized"}),
+ api.beacon.getFinalityCheckpoints("head")
+]);
+const lastHeader = finalizedData[0].value();
+const finalizedCheckpoint = finalizedData[1].value().finalized;
+const finalizedSlot = lastHeader.header.message.slot;
+const finalizedEpoch = finalizedCheckpoint.epoch;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
script/integration/1_DeployBootstrap.s.sol (3)
74-74: Consider making NetworkConfig immutable.The
networkConfigstate variable should be marked asimmutablesince it's only set once in the constructor and never modified afterward.- NetworkConfig networkConfig; + NetworkConfig immutable networkConfig;
151-154: Consider using named constants for array indices.Using magic number 0 for array index makes the code less maintainable. Consider defining a named constant for the virtual staked ETH index.
+ uint256 constant VIRTUAL_STAKED_ETH_INDEX = 0; - whitelistTokens.push(VIRTUAL_STAKED_ETH_ADDRESS); - tvlLimits.push(0); // not enforced for virtual staked eth + whitelistTokens.push(VIRTUAL_STAKED_ETH_ADDRESS); + tvlLimits.push(0); // not enforced for virtual staked eth at index VIRTUAL_STAKED_ETH_INDEX
Line range hint
419-422: Improve random number generation for production.The current random number generation implementation using
block.timestampandblock.prevrandaois not suitable for production use.Consider using a more robust random number generation approach, such as Chainlink VRF for production environments. For testing purposes, you might want to make the randomness deterministic and seedable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
script/integration/1_DeployBootstrap.s.sol(9 hunks)
🧰 Additional context used
📓 Learnings (1)
script/integration/1_DeployBootstrap.s.sol (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/1_DeployBootstrap.s.sol:232-235
Timestamp: 2024-11-29T12:19:04.518Z
Learning: When code comments reference specific addresses for informational purposes and they are not hardcoded in the code, it's acceptable and does not require suggesting moving them to configuration.
🔇 Additional comments (2)
script/integration/1_DeployBootstrap.s.sol (2)
407-411: 🛠️ Refactor suggestion
Improve error handling for file operations.
The file writing operation at the end of the script lacks error handling. Consider adding try-catch or validation.
// finally save the bootstrap address
string memory key = "deployments";
key.serialize("beaconOracleAddress", address(beaconOracle));
string memory start = key.serialize("bootstrapAddress", address(bootstrap));
- vm.writeFile("script/integration/deployments.json", start);
+ try vm.writeFile("script/integration/deployments.json", start) {
+ console.log("Successfully wrote deployments to file");
+ } catch {
+ revert("Failed to write deployments to file");
+ }Likely invalid or redundant comment.
302-305:
Remove commented mnemonic instructions.
The comments contain instructions about using mnemonics, which could be security-sensitive information. Consider moving these to external documentation.
- // the mnemonics corresponding to the consensus public keys are given here. to recover,
- // echo "${MNEMONIC}" | exocored init localnet --chain-id exocorelocal_233-1 --recover
- // the value in this script is this one
- // exocored keys consensus-pubkey-to-bytes --output json | jq -r .bytes
+ // Consensus public keys for validatorsLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
script/integration/1_DeployBootstrap.s.sol (5)
74-74: Consider using immutable for networkConfig.Since
networkConfigis set only once in the constructor and never modified afterward, consider marking it asimmutableto save gas and make the contract's behavior more explicit.- NetworkConfig networkConfig; + NetworkConfig immutable networkConfig;
283-292: Add validation for stake amount in stakeNST function.The function stakes exactly 32 ETH without any validation. While this is a known requirement for Ethereum staking, explicit validation would make the code more robust.
+ uint256 constant REQUIRED_STAKE_AMOUNT = 32 ether; vm.startBroadcast(nstDepositor); address myAddress = address(bootstrap.ownerToCapsule(vm.addr(nstDepositor))); if (myAddress == address(0)) { myAddress = bootstrap.createExoCapsule(); } console.log("ExoCapsule address", myAddress); - bootstrap.stake{value: 32 ether}(pubkey, signature, depositDataRoot); + bootstrap.stake{value: REQUIRED_STAKE_AMOUNT}(pubkey, signature, depositDataRoot);
408-412: Consider using a more robust JSON serialization approach.The current JSON serialization approach using string concatenation could be fragile. Consider using a structured JSON library or creating a proper JSON object.
- string memory key = "deployments"; - key.serialize("beaconOracleAddress", address(beaconOracle)); - string memory start = key.serialize("bootstrapAddress", address(bootstrap)); - vm.writeFile("script/integration/deployments.json", start); + string memory json = string(abi.encodePacked( + '{"deployments":{"beaconOracleAddress":"', + toHexString(address(beaconOracle)), + '","bootstrapAddress":"', + toHexString(address(bootstrap)), + '"}}' + )); + vm.writeFile("script/integration/deployments.json", json);
303-306: Remove or update outdated comments.The comments about mnemonics and consensus public keys appear to be outdated or incomplete. Consider updating them to reflect the current implementation or remove if no longer relevant.
Line range hint
419-422: Improve random number generation for production use.The current random number generation implementation using
block.timestampandblock.prevrandaomight not be secure enough for production use.Consider using a more robust random number generation approach, such as Chainlink VRF, for production environments. However, since this is a test script, the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
script/integration/1_DeployBootstrap.s.sol(9 hunks)
🧰 Additional context used
📓 Learnings (1)
script/integration/1_DeployBootstrap.s.sol (2)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/1_DeployBootstrap.s.sol:282-291
Timestamp: 2024-12-02T06:38:11.533Z
Learning: In forge scripts (e.g., `script/integration/1_DeployBootstrap.s.sol`), `msg.value` is not available, as forge scripts are not contracts.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore-contracts#123
File: script/integration/1_DeployBootstrap.s.sol:232-235
Timestamp: 2024-11-29T12:19:04.518Z
Learning: When code comments reference specific addresses for informational purposes and they are not hardcoded in the code, it's acceptable and does not require suggesting moving them to configuration.
🔇 Additional comments (3)
script/integration/1_DeployBootstrap.s.sol (3)
114-148: Well-structured parameter validation in setUp.
The validation of environment variables and parameters is thorough and well-organized. The explicit checks for length requirements and value ranges enhance the reliability of the deployment process.
217-218: Hardcoded time values need review.
The initialization uses hardcoded time values (24 hours and 1 seconds) which might not be suitable for all deployment scenarios.
Consider making these configurable through environment variables to support different deployment environments and testing scenarios.
152-155: Consider adding validation for tvlLimits array.
While the code initializes the tvlLimits array, there's no validation to ensure its elements are within acceptable bounds.
Consider adding validation:
whitelistTokens.push(VIRTUAL_STAKED_ETH_ADDRESS);
-tvlLimits.push(0); // not enforced for virtual staked eth
+require(tvlLimits.length < type(uint256).max, "TVL limits array too large");
+tvlLimits.push(0); // not enforced for virtual staked eth
|
I also tested the truncation case wherein, instead of 32 ETH, the staker has delegated only 30 ETH and the effective balance goes below that number, say, 29. In such a case, the withdrawable balance becomes 0 first and we are still left with 1 ETH to slash from the delegated 30 ETH. The script was able to do that successfully and handled the decimals well without generating any extra tokens. |
However, one question is how to get the validator pub key from the hashed form. It is not clear how the price feeder handles it either.This PR fixes a potentially major (yet unnoticed) bug that the same constants (such as the ETH1_DEPOSIT_ADDRESS and GENESIS_TIME) were used across all networks.
Summary by CodeRabbit
New Features
NetworkConfigcontract for managing network parameters.BeaconOraclecontract for managing beacon chain data.Bootstrapcontract with new public functions for validator management.ExoCapsulecontract to improve initialization and withdrawal logic.VerifyDepositNSTcontract for integration testing of deposit functionalities.Bug Fixes
Documentation
Tests
NetworkConfigcontract and updated existing tests to reflect changes in initialization parameters.