fix: incompatible storage layout for Bootstrap and ClientChainGateway #72
Conversation
WalkthroughThe recent changes enhance the functionality and security of multiple contracts within the project. Key updates include structured storage representations for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Contract
participant Owner
User->>Contract: Request action
Contract->>Owner: Check ownership
alt Owner permissions granted
Owner->>Contract: Approve action
Contract-->>User: Action executed
else Owner permissions denied
Contract-->>User: Action failed
end
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
after we apply the fix, now two contracts' storage layout look completely compatible: | Name | Type | Slot | Offset | Bytes | Contract |
|-------------------------------|--------------------------------------------------------------------|------|--------|-------|----------------------------------|
| _initialized | uint8 | 0 | 0 | 1 | src/core/Bootstrap.sol:Bootstrap |
| _initializing | bool | 0 | 1 | 1 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[50] | 1 | 0 | 1600 | src/core/Bootstrap.sol:Bootstrap |
| _paused | bool | 51 | 0 | 1 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[49] | 52 | 0 | 1568 | src/core/Bootstrap.sol:Bootstrap |
| _owner | address | 101 | 0 | 20 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[49] | 102 | 0 | 1568 | src/core/Bootstrap.sol:Bootstrap |
| _status | uint256 | 151 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[49] | 152 | 0 | 1568 | src/core/Bootstrap.sol:Bootstrap |
| peers | mapping(uint32 => bytes32) | 201 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| _whiteListFunctionSelectors | mapping(enum GatewayStorage.Action => bytes4) | 202 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| inboundNonce | mapping(uint32 => mapping(bytes32 => uint64)) | 203 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[40] | 204 | 0 | 1280 | src/core/Bootstrap.sol:Bootstrap |
| exocoreSpawnTime | uint256 | 244 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| offsetDuration | uint256 | 245 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| depositsByToken | mapping(address => uint256) | 246 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| registeredValidators | address[] | 247 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| ethToExocoreAddress | mapping(address => string) | 248 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| validators | mapping(string => struct IValidatorRegistry.Validator) | 249 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| commissionEdited | mapping(string => bool) | 250 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| delegationsByValidator | mapping(string => mapping(address => uint256)) | 251 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| depositors | address[] | 252 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| isDepositor | mapping(address => bool) | 253 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| totalDepositAmounts | mapping(address => mapping(address => uint256)) | 254 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| withdrawableAmounts | mapping(address => mapping(address => uint256)) | 255 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| delegations | mapping(address => mapping(string => mapping(address => uint256))) | 256 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| bootstrapped | bool | 257 | 0 | 1 | src/core/Bootstrap.sol:Bootstrap |
| customProxyAdmin | address | 257 | 1 | 20 | src/core/Bootstrap.sol:Bootstrap |
| clientChainGatewayLogic | address | 258 | 0 | 20 | src/core/Bootstrap.sol:Bootstrap |
| clientChainInitializationData | bytes | 259 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| whitelistTokens | address[] | 260 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| isWhitelistedToken | mapping(address => bool) | 261 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| tokenToVault | mapping(address => contract IVault) | 262 | 0 | 32 | src/core/Bootstrap.sol:Bootstrap |
| __gap | uint256[40] | 263 | 0 | 1280 | src/core/Bootstrap.sol:Bootstrap || Name | Type | Slot | Offset | Bytes | Contract |
|-------------------------------|--------------------------------------------------------------------|------|--------|-------|----------------------------------------------------|
| _initialized | uint8 | 0 | 0 | 1 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _initializing | bool | 0 | 1 | 1 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[50] | 1 | 0 | 1600 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _paused | bool | 51 | 0 | 1 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[49] | 52 | 0 | 1568 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _owner | address | 101 | 0 | 20 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[49] | 102 | 0 | 1568 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _status | uint256 | 151 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[49] | 152 | 0 | 1568 | src/core/ClientChainGateway.sol:ClientChainGateway |
| peers | mapping(uint32 => bytes32) | 201 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _whiteListFunctionSelectors | mapping(enum GatewayStorage.Action => bytes4) | 202 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| inboundNonce | mapping(uint32 => mapping(bytes32 => uint64)) | 203 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[40] | 204 | 0 | 1280 | src/core/ClientChainGateway.sol:ClientChainGateway |
| exocoreSpawnTime | uint256 | 244 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| offsetDuration | uint256 | 245 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| depositsByToken | mapping(address => uint256) | 246 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| registeredValidators | address[] | 247 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| ethToExocoreAddress | mapping(address => string) | 248 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| validators | mapping(string => struct IValidatorRegistry.Validator) | 249 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| commissionEdited | mapping(string => bool) | 250 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| delegationsByValidator | mapping(string => mapping(address => uint256)) | 251 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| depositors | address[] | 252 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| isDepositor | mapping(address => bool) | 253 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| totalDepositAmounts | mapping(address => mapping(address => uint256)) | 254 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| withdrawableAmounts | mapping(address => mapping(address => uint256)) | 255 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| delegations | mapping(address => mapping(string => mapping(address => uint256))) | 256 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| bootstrapped | bool | 257 | 0 | 1 | src/core/ClientChainGateway.sol:ClientChainGateway |
| customProxyAdmin | address | 257 | 1 | 20 | src/core/ClientChainGateway.sol:ClientChainGateway |
| clientChainGatewayLogic | address | 258 | 0 | 20 | src/core/ClientChainGateway.sol:ClientChainGateway |
| clientChainInitializationData | bytes | 259 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| whitelistTokens | address[] | 260 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| isWhitelistedToken | mapping(address => bool) | 261 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| tokenToVault | mapping(address => contract IVault) | 262 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[40] | 263 | 0 | 1280 | src/core/ClientChainGateway.sol:ClientChainGateway |
| outboundNonce | uint64 | 303 | 0 | 8 | src/core/ClientChainGateway.sol:ClientChainGateway |
| ownerToCapsule | mapping(address => contract IExoCapsule) | 304 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _registeredRequests | mapping(uint64 => bytes) | 305 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| _registeredRequestActions | mapping(uint64 => enum GatewayStorage.Action) | 306 | 0 | 32 | src/core/ClientChainGateway.sol:ClientChainGateway |
| __gap | uint256[40] | 307 | 0 | 1280 | src/core/ClientChainGateway.sol:ClientChainGateway | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- docs/bootstrap_storage.txt (1 hunks)
- docs/clientchaingateway_storage.txt (1 hunks)
- src/core/BaseRestakingController.sol (2 hunks)
- src/core/LSTRestakingController.sol (2 hunks)
- src/core/NativeRestakingController.sol (2 hunks)
Additional comments not posted (37)
src/core/LSTRestakingController.sol (2)
9-9: Import statement forOwnableUpgradeablelooks good.The import statement is correct and necessary for the changes made to the contract.
18-18: Inheritance change forLSTRestakingControllerlooks good.The contract now inherits from
OwnableUpgradeable, enhancing its access control by allowing ownership management.src/core/BaseRestakingController.sol (2)
12-12: Import statement forOwnableUpgradeablelooks good.The import statement is correct and necessary for the changes made to the contract.
23-23: Inheritance change forBaseRestakingControllerlooks good.The contract now inherits from
OwnableUpgradeable, enhancing its access control by allowing ownership management.docs/bootstrap_storage.txt (1)
1-36: Storage layout documentation looks good.The documentation is clear and provides a comprehensive overview of the storage layout. This is crucial for understanding the storage structure and ensuring compatibility between contracts.
src/core/NativeRestakingController.sol (7)
12-12: Import statement forOwnableUpgradeablelooks good.The import statement is necessary for the added functionality.
24-24: Inheritance ofOwnableUpgradeablelooks good.The inheritance order is correct and consistent with the intended functionality.
Line range hint
34-48:
Functionstakelooks good.The function logic is correct and consistent with the added
OwnableUpgradeablefunctionality.
Line range hint
54-72:
FunctioncreateExoCapsulelooks good.The function logic is correct and consistent with the added
OwnableUpgradeablefunctionality.
Line range hint
78-89:
FunctiondepositBeaconChainValidatorlooks good.The function logic is correct and consistent with the added
OwnableUpgradeablefunctionality.
Line range hint
95-115:
FunctionprocessBeaconChainWithdrawallooks good.The function logic is correct and consistent with the added
OwnableUpgradeablefunctionality.
Line range hint
29-33:
ModifiernativeRestakingEnabledlooks good.The modifier logic is correct and consistent with the added
OwnableUpgradeablefunctionality.docs/clientchaingateway_storage.txt (25)
3-3: Storage slot_initializedlooks good.The storage slot documentation is correct.
4-4: Storage slot_initializinglooks good.The storage slot documentation is correct.
5-5: Storage slot__gaplooks good.The storage slot documentation is correct.
6-6: Storage slot_pausedlooks good.The storage slot documentation is correct.
7-7: Storage slot__gaplooks good.The storage slot documentation is correct.
8-8: Storage slot_ownerlooks good.The storage slot documentation is correct.
9-9: Storage slot__gaplooks good.The storage slot documentation is correct.
10-10: Storage slot_statuslooks good.The storage slot documentation is correct.
11-11: Storage slot__gaplooks good.The storage slot documentation is correct.
12-12: Storage slotpeerslooks good.The storage slot documentation is correct.
13-13: Storage slot_whiteListFunctionSelectorslooks good.The storage slot documentation is correct.
14-14: Storage slotinboundNoncelooks good.The storage slot documentation is correct.
15-15: Storage slot__gaplooks good.The storage slot documentation is correct.
16-16: Storage slotexocoreSpawnTimelooks good.The storage slot documentation is correct.
17-17: Storage slotoffsetDurationlooks good.The storage slot documentation is correct.
18-18: Storage slotdepositsByTokenlooks good.The storage slot documentation is correct.
19-19: Storage slotregisteredValidatorslooks good.The storage slot documentation is correct.
20-20: Storage slotethToExocoreAddresslooks good.The storage slot documentation is correct.
21-21: Storage slotvalidatorslooks good.The storage slot documentation is correct.
22-22: Storage slotcommissionEditedlooks good.The storage slot documentation is correct.
23-23: Storage slotdelegationsByValidatorlooks good.The storage slot documentation is correct.
24-24: Storage slotdepositorslooks good.The storage slot documentation is correct.
25-25: Storage slotisDepositorlooks good.The storage slot documentation is correct.
26-26: Storage slottotalDepositAmountslooks good.The storage slot documentation is correct.
27-27: **Storage slot `
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
.github/workflows/compare_storage_layout.py (2)
23-24: Clarify the assumption about the separator row.The code unconditionally drops the second row assuming it is a separator row. Ensure this assumption holds for all input files or add a check to verify the second row is a separator before dropping it.
.github/workflows/compare_storage_layout.py:df = df.drop(df.index[1])Analysis chain
Clarify the assumption about the separator row.
Dropping the second row assumes it is a separator row. Ensure this assumption holds for all input files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the assumption about the separator row. # Test: Check if the second row is always a separator row. # Expect: The script should handle cases where the second row is not a separator row. echo "Name|Type|Slot|Offset|Bytes|Contract|Unnamed: 7" > test.csv echo "separator_row" >> test.csv echo "test_name|test_type|test_slot|test_offset|test_bytes|test_contract|test_unnamed" >> test.csv python .github/workflows/compare_storage_layout.pyLength of output: 293
Script:
#!/bin/bash # Search for the code that drops the second row in compare_storage_layout.py ast-grep --lang python --pattern $'df = df.drop(df.index[1])' --jsonLength of output: 72
Script:
#!/bin/bash # Search for any instances of `drop` within the compare_storage_layout.py file rg 'drop' .github/workflows/compare_storage_layout.py -A 5Length of output: 309
19-21: Ensure case-insensitive header validation.The current header validation logic in
compare_storage_layout.pyperforms a case-sensitive comparison. To handle potential discrepancies in header names, modify the comparison to be case-insensitive by converting both actual and expected headers to lowercase.
- Location:
.github/workflows/compare_storage_layout.py- Lines: 19-21
Suggested modification:
# Check headers if not all([df.columns[i].lower() == expected_headers[i].lower() for i in range(len(expected_headers))]): raise ValueError(f"Error: Headers in {file_path} do not match expected headers.")Analysis chain
Verify header validation logic.
The header validation logic checks if the headers match the expected headers. Ensure that this comparison is case-insensitive and handles potential discrepancies in header names.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify header validation logic. # Test: Check for case-insensitive header validation. # Expect: The script should validate headers correctly irrespective of case. echo "name|type|slot|offset|bytes|contract|unnamed: 7" > test.csv python .github/workflows/compare_storage_layout.pyLength of output: 183
Script:
#!/bin/bash # Extract the relevant lines from compare_storage_layout.py to check header validation logic fd compare_storage_layout.py --exec cat {} | sed -n '19,21p'Length of output: 62
Script:
#!/bin/bash # Find the compare_storage_layout.py file and extract lines 19-21 file_path=$(fd compare_storage_layout.py) if [ -n "$file_path" ]; then cat "$file_path" | sed -n '19,21p' else echo "compare_storage_layout.py not found" fiLength of output: 141
Script:
#!/bin/bash # Find Python files in the .github/workflows directory and extract lines 19-21 from compare_storage_layout.py file_path=$(fd .py .github/workflows) if [ -n "$file_path" ]; then for file in $file_path; do if [[ $file == *"compare_storage_layout.py"* ]]; then cat "$file" | sed -n '19,21p' fi done else echo "No Python files found in .github/workflows" fiLength of output: 543
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/compare_storage_layout.py (1 hunks)
- .github/workflows/forge-ci.yml (7 hunks)
- .github/workflows/foundry-setup.yml (1 hunks)
Files skipped from review due to trivial changes (1)
- .github/workflows/foundry-setup.yml
Additional comments not posted (35)
.github/workflows/compare_storage_layout.py (10)
9-10: Ensure proper error handling for file existence check.The file existence check is correct and raises a
FileNotFoundErrorif the file does not exist.
15-17: Ensure trimming of whitespace is applied correctly.The code correctly trims leading/trailing whitespace from all columns.
26-27: Combine relevant columns for comparison.The code combines relevant columns into a single string for comparison. Ensure that this step is necessary and correctly implemented.
35-37: Verify the length check logic.The length check logic ensures that the
bootstrap_entrieslist is not longer than theclientChainGateway_entrieslist. This is a good practice to prevent out-of-bounds errors.
39-41: Confirm the comparison logic.The comparison logic correctly identifies mismatches between the two lists and appends them to the
mismatcheslist.
46-48: Verify the parsing steps for both files.The parsing steps for both
ClientChainGateway.mdandBootstrap.mdare correctly implemented.
50-54: Ensure proper error handling for empty entries.The code correctly raises an error if no valid entries are found in either file.
56-64: Confirm the comparison and output steps.The comparison and output steps are correctly implemented, providing meaningful messages for mismatches or successful comparisons.
65-67: Handle exceptions gracefully.The code handles exceptions gracefully and provides meaningful error messages.
12-13: Handle potential errors when reading the CSV file.Ensure to handle potential errors that might occur while reading the CSV file, such as incorrect delimiters or encoding issues.
.github/workflows/forge-ci.yml (25)
1-1: Update the workflow name.The workflow name has been updated to reflect the new functionality of comparing storage layouts.
3-3: Verify the triggering events.The workflow is triggered on merge group, pull request, and push events, including specific branches and tags.
Line range hint
10-12:
Verify the usage of the predefined workflow for setup.The
setupjob correctly uses a predefined workflow to set up the Foundry toolchain.
26-27: Verify the checkout step.The checkout step correctly uses the
actions/checkout@v4action with submodules set to recursive.
28-30: Confirm the restoration of the Foundry toolchain.The restoration step correctly uses the
actions/cache/restore@v3action to restore the Foundry toolchain.
Line range hint
31-32:
Add Foundry to PATH.The step to add Foundry to PATH is correctly implemented.
Line range hint
33-34:
Verify the build step.The build step correctly runs
forge build.
Line range hint
35-39:
Handle build failures gracefully.The step to add a comment for build failures is correctly implemented using
actions/github-script@v6.
Line range hint
40-42:
Cache build artifacts.The step to cache build artifacts is correctly implemented using
actions/cache/save@v3.
65-66: Verify the checkout step.The checkout step correctly uses the
actions/checkout@v4action.
67-69: Confirm the restoration of the Foundry toolchain.The restoration step correctly uses the
actions/cache/restore@v3action to restore the Foundry toolchain.
Line range hint
70-71:
Add Foundry to PATH.The step to add Foundry to PATH is correctly implemented.
Line range hint
72-74:
Restore build artifacts.The step to restore build artifacts is correctly implemented using
actions/cache/restore@v3.
83-84: Verify the test step.The test step correctly runs
forge test.
Line range hint
85-89:
Handle test failures gracefully.The step to add a comment for test failures is correctly implemented using
actions/github-script@v6.
139-141: Verify the checkout step.The checkout step correctly uses the
actions/checkout@v4action.
142-144: Confirm the restoration of the Foundry toolchain.The restoration step correctly uses the
actions/cache/restore@v3action to restore the Foundry toolchain.
145-147: Add Foundry to PATH.The step to add Foundry to PATH is correctly implemented.
148-150: Restore build artifacts.The step to restore build artifacts is correctly implemented using
actions/cache/restore@v3.
157-158: Inspect storage layout of ClientChainGateway.The step to inspect the storage layout of
ClientChainGatewayis correctly implemented usingforge inspect.
159-160: Inspect storage layout of Bootstrap.The step to inspect the storage layout of
Bootstrapis correctly implemented usingforge inspect.
161-164: Set up Python.The step to set up Python is correctly implemented using
actions/setup-python@v5.
165-166: Install pandas.The step to install pandas is correctly implemented.
167-168: Run the comparison script.The step to run the comparison script is correctly implemented.
169-175: Handle storage layout mismatch failures gracefully.The step to add a comment for storage layout mismatch failures is correctly implemented using
actions/github-script@v6.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- .github/workflows/forge-ci.yml (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/forge-ci.yml
Description
When we use
forge inspect --pretty [Contract] storageLayoutcommand to print the storage layout forBootstrapandClientChainGateway, we get:Except the appended states, most state variables are exactly the same but the order for
_statusand_owneris different for two contracts. The root causeClientChainGatewayputReentrancyUpgradeablebeforeOwnableUpgradeablewhen serializing the inherited contracts, whileBootstrapturn it over.So we should adjust the inheritance order to keep storage layout compatible
Summary by CodeRabbit
New Features
BootstrapandClientChainGatewaycontracts, enhancing clarity for developers.BaseRestakingController,LSTRestakingController, andNativeRestakingControllercontracts, improving security and governance.Bug Fixes
OwnableUpgradeablefunctionality.