CI: enhance CI to check storage layout backwards compatibility #112
Conversation
WalkthroughThe pull request introduces several modifications to enhance the GitHub Actions workflow for comparing storage layouts of smart contracts. It updates the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
.github/workflows/forge-ci.yml (2)
162-174: LGTM: Target branch checkout and layout generationThe addition of steps to checkout the target branch and generate a layout file for ExocoreGateway is well-implemented. This allows for accurate comparison of storage layouts between the current changes and the target branch.
Suggestion for improvement:
Consider adding error handling for theforge inspectcommand. If the command fails, it might be helpful to capture and log the error for easier debugging.You could modify the "Generate target branch layout files" step as follows:
- name: Generate target branch layout files run: | forge inspect src/core/ExocoreGateway.sol:ExocoreGateway storage-layout --pretty > ExocoreGateway_target.txt || echo "Error generating layout file for ExocoreGateway"
190-193: LGTM: Comparison scripts executionThe execution of two separate comparison scripts is well-implemented. This allows for comprehensive checking of storage layouts for both local contracts (Bootstrap and ClientChainGateway) and deployed contracts.
Suggestion for improvement:
Consider adding more verbose output or logging for these script executions. This could help in debugging if issues arise.You could modify the steps as follows:
- name: Run the comparison script for Bootstrap and ClientChainGateway run: | echo "Starting comparison for Bootstrap and ClientChainGateway..." python .github/workflows/compare_storage_layout.py echo "Comparison for Bootstrap and ClientChainGateway completed." - name: Run the comparison script for deployed contracts run: | echo "Starting comparison for deployed contracts..." python .github/workflows/compare_deployed_storage_layout.py echo "Comparison for deployed contracts completed.".github/workflows/compare_deployed_storage_layout.py (1)
86-87: Include stack trace in error output for debuggingWhen an exception is caught, only the error message is printed. Including the stack trace can aid in debugging by providing more context about the error source.
import traceback except Exception as e: print(f"Error: {e}") + traceback.print_exc() exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/compare_deployed_storage_layout.py (1 hunks)
- .github/workflows/compare_storage_layout.py (1 hunks)
- .github/workflows/forge-ci.yml (3 hunks)
- .gitignore (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Ruff
.github/workflows/compare_storage_layout.py
9-9: Statement ends with an unnecessary semicolon
Remove unnecessary semicolon
(E703)
46-46: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
55-55: f-string without any placeholders
Remove extraneous
fprefix(F541)
🔇 Additional comments (4)
.github/workflows/forge-ci.yml (4)
140-142: LGTM: Environment variables added for API accessThe addition of
ALCHEMY_API_KEYandETHERSCAN_API_KEYas environment variables is appropriate. This change aligns with the PR objectives of comparing storage layouts of deployed contracts, which likely requires access to these APIs. Using GitHub secrets for storing these keys is a good security practice.
175-183: LGTM: PR branch checkout and target layout restorationThe steps to checkout the PR branch and restore the target layout file are well-implemented. This ensures an accurate comparison between the PR changes and the target branch while efficiently using the cached layout file.
204-204: LGTM: Improved failure commentThe updated failure comment provides more specific information about potential issues, mentioning both local contract mismatches and incompatibility with deployed contracts on Sepolia. This improvement aligns well with the PR objectives and will help developers quickly understand and address any storage layout compatibility issues.
Line range hint
140-204: Overall assessment: Well-implemented enhancements to CI processThe changes to the
compare-storage-layoutjob in this workflow file are well-implemented and align closely with the PR objectives. The additions effectively enhance the CI process to check storage layout backwards compatibility for both local and deployed contracts. The use of caching, separate comparison scripts, and improved error reporting contribute to a robust and informative CI process.A few minor suggestions have been made for potential improvements, mainly around error handling and verbose logging. These suggestions, if implemented, could further enhance the debugging process if issues arise.
Great job on implementing these important changes to ensure contract upgradeability!
adu-web3
requested review from
magj2006,
bwhour,
cloud8little,
cary007,
trestinlsd,
mikebraver,
TimmyExogenous and
leonz789
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
adu-web3
force-pushed
the
compare-storage
branch
from
6abeb59 to
5d5b914
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
.github/workflows/compare_storage_layout.py (3)
6-25: LGTM! Consider using a more specific exception type.The
parse_outputfunction effectively processes the command output and creates a structured DataFrame. The use of pandas for data manipulation is appropriate for this task.Consider using a more specific exception type, such as
ValueError, instead of the genericExceptionon line 18. This aligns with the learning from previous reviews that it's acceptable to use generic exceptions in CI job scripts, but using more specific types can still improve error handling clarity.- raise Exception(f"No valid storage layout data found for {contract_name}") + raise ValueError(f"No valid storage layout data found for {contract_name}")
28-35: LGTM! Consider consistent error handling.The
get_current_layoutfunction effectively executes theforge inspectcommand and processes its output. The use of subprocess and error handling is appropriate.For consistency with the previous suggestion, consider using
ValueErrorinstead ofExceptionon line 33:- raise Exception(f"Error getting current layout for {contract_name}: {result.stderr}") + raise ValueError(f"Error getting current layout for {contract_name}: {result.stderr}")
37-55: LGTM! Consider renaming unused variable.The
compare_layoutsfunction effectively compares the old and new layouts, focusing on non-gap variables and reporting mismatches. The comparison logic aligns well with the PR objectives.Consider renaming the unused loop variable
indexto_to indicate that it's intentionally unused:- for index, row in old_layout.iterrows(): + for _, row in old_layout.iterrows():This change improves code readability and addresses the static analysis hint.
🧰 Tools
🪛 Ruff
46-46: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
.github/workflows/forge-ci.yml (3)
162-174: LGTM: Steps added for target branch layout generation.The addition of steps to checkout the target branch and generate a storage layout file for ExocoreGateway is well-implemented. Caching the layout file is an efficient approach for later comparison.
Consider adding a comment explaining the purpose of these steps for better readability:
# Generate and cache storage layout for the target branch - name: Checkout target branch uses: actions/checkout@v4 with: ref: ${{ github.event.pull_request.base.ref }} submodules: recursive # ... (rest of the steps)
190-193: LGTM: Comparison scripts added for comprehensive layout checks.The addition of steps to run comparison scripts for Bootstrap, ClientChainGateway, and deployed contracts is well-implemented and aligns with the PR objectives.
Consider adding error handling and logging to these steps:
- name: Run the comparison script for Bootstrap and ClientChainGateway run: python .github/workflows/compare_storage_layout.py continue-on-error: true id: bootstrap_client_comparison - name: Run the comparison script for deployed contracts run: python .github/workflows/compare_deployed_storage_layout.py continue-on-error: true id: deployed_comparison - name: Check comparison results if: steps.bootstrap_client_comparison.outcome == 'failure' || steps.deployed_comparison.outcome == 'failure' run: | echo "Storage layout comparison failed. Check the logs for details." exit 1This approach will provide more granular information about which comparison failed.
204-204: Improved failure message, but could be more actionable.The updated failure message provides more specific information about potential issues, which is an improvement.
Consider making the message more actionable:
body: | Storage layout compatibility check failed. This could be due to: 1. A mismatch between Bootstrap and ClientChainGateway 2. Incompatibility with deployed contracts on Sepolia Action items: - Review the CI logs for detailed error messages - Check for unintended changes in contract storage variables - If changes are intentional, update the comparison scripts or deployed contracts as necessary For assistance, please contact the core development team.This format provides clear next steps for developers to resolve the issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- .github/workflows/compare_deployed_storage_layout.py (1 hunks)
- .github/workflows/compare_storage_layout.py (1 hunks)
- .github/workflows/forge-ci.yml (3 hunks)
- .gitignore (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/compare_deployed_storage_layout.py
- .gitignore
🧰 Additional context used
📓 Learnings (1)
.github/workflows/compare_storage_layout.py (2)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#112 File: .github/workflows/compare_storage_layout.py:28-28 Timestamp: 2024-10-17T03:13:58.602Z Learning: In `.github/workflows/compare_storage_layout.py`, validation of `contract_name` is not required as it does not accept untrusted input.Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#112 File: .github/workflows/compare_storage_layout.py:18-18 Timestamp: 2024-10-17T03:09:51.221Z Learning: In Python scripts used for CI jobs (such as `.github/workflows/compare_storage_layout.py`), it is acceptable to use generic `Exception` types for error handling.
🪛 Ruff
.github/workflows/compare_storage_layout.py
46-46: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
🔇 Additional comments (5)
.github/workflows/compare_storage_layout.py (2)
59-81: LGTM! Main execution block is well-structured.The main execution block effectively utilizes the new functions and implements appropriate error handling. The error messages are clear and informative, and the exit codes are used correctly to indicate success or failure.
The changes align well with the PR objectives of enhancing the CI process to check storage layout backwards compatibility.
1-81: Overall assessment: Significant improvements to the storage layout comparison process.The changes to this script represent a substantial improvement in the approach to comparing storage layouts:
- The transition from file parsing to using the
forge inspectcommand output is likely to be more robust and maintainable.- The use of pandas for data manipulation enhances the script's efficiency and readability.
- The new structure with separate functions for parsing, retrieving layouts, and comparing them improves the overall organization of the code.
These changes align well with the PR objectives of enhancing the CI process to check storage layout backwards compatibility. The new implementation should provide a more reliable mechanism for detecting potential issues in contract upgrades.
Great job on this refactoring! The new implementation significantly improves the reliability and maintainability of the storage layout comparison process.
🧰 Tools
🪛 Ruff
46-46: Loop control variable
indexnot used within loop bodyRename unused
indexto_index(B007)
.github/workflows/forge-ci.yml (3)
140-142: LGTM: Environment variables added for API access.The addition of
ALCHEMY_API_KEYandETHERSCAN_API_KEYas environment variables is appropriate. This change aligns with the PR objectives of comparing storage layouts of deployed contracts, which likely requires access to these APIs. Using GitHub secrets for storing API keys is a good security practice.
175-183: LGTM: Steps added to return to PR branch and restore layout file.The addition of steps to checkout back to the PR branch and restore the cached target branch layout file is well-implemented. This approach ensures that the workflow can compare the current PR changes against the target branch's storage layout.
Line range hint
140-204: Overall, excellent enhancements to the CI workflow.The changes to the
compare-storage-layoutjob significantly improve the process of checking storage layout compatibility. The additions align well with the PR objectives and follow good practices. The workflow now provides a comprehensive comparison of storage layouts for both local contracts and deployed contracts, which is crucial for maintaining upgradeability.A few minor suggestions have been made to improve readability and error handling, but these are not critical. Great job on implementing this important feature!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
.github/workflows/compare_deployed_storage_layout.py (2)
21-23: Consider using theloggingmodule instead ofUsing the
loggingmodule provides better control over logging levels and outputs. It allows for more flexible logging configurations and is a best practice for larger applications.Apply this diff to enhance logging practices:
+import logging +logging.basicConfig(level=logging.INFO) def get_storage_layout(contract_name, address, rpc_url, etherscan_api_key): if not address: - print(f"Skipping {contract_name} as it's not deployed.") + logging.info(f"Skipping {contract_name} as it's not deployed.") return pd.DataFrame() result = subprocess.run(['cast', 'storage', address, '--rpc-url', rpc_url, '--etherscan-api-key', etherscan_api_key], capture_output=True, text=True) - print(f"finish executing: cast storage {address} --rpc-url ...") + logging.info(f"Finished executing: cast storage {address} --rpc-url ...") ... # In the main execution block - print(f"Checking {contract_name}...") + logging.info(f"Checking {contract_name}...") - print(f"No deployed layout found for {contract_name}.") + logging.warning(f"No deployed layout found for {contract_name}.") ... - print("Checking ExocoreGateway...") + logging.info("Checking ExocoreGateway...") ... - print("Mismatches found for current contracts:") + logging.error("Mismatches found for current contracts:") for contract, mismatches in all_mismatches.items(): - print(f"{contract}:") + logging.error(f"{contract}:") for mismatch in mismatches: - print(f" {mismatch}") + logging.error(f" {mismatch}") ... - print(f"Error: {e}") + logging.error(f"Error: {e}")Also applies to: 26-27, 54-55, 57-58, 69-70, 77-81, 85-86
9-18: Add docstrings to functions for better documentationAdding docstrings to your functions enhances code readability and maintainability by providing clear descriptions of their purpose, parameters, and return values.
Here's an example for the
get_deployed_addressesfunction:def get_deployed_addresses(): + """ + Reads deployed contract addresses from 'script/deployedContracts.json' and returns a dictionary mapping contract names to their addresses. + + Returns: + dict: A dictionary containing contract names and their deployed addresses. + """ with open('script/deployedContracts.json', 'r') as f: data = json.load(f)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .github/workflows/compare_deployed_storage_layout.py (1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/compare_deployed_storage_layout.py (1)
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#112 File: .github/workflows/compare_deployed_storage_layout.py:10-12 Timestamp: 2024-10-17T03:16:14.054Z Learning: The `deployedContracts.json` file in the `script/` directory is enforced to be created for each release, so exception handling for its absence in `compare_deployed_storage_layout.py` is not necessary.
🔇 Additional comments (1)
.github/workflows/compare_deployed_storage_layout.py (1)
10-12:⚠️ Potential issueAdd exception handling for JSON decoding errors
If
deployedContracts.jsoncontains invalid JSON,json.load(f)will raise ajson.JSONDecodeError. Consider adding error handling to manage this scenario gracefully.Apply this diff to enhance error handling:
def get_deployed_addresses(): + try: with open('script/deployedContracts.json', 'r') as f: data = json.load(f) + except json.JSONDecodeError as e: + raise Exception(f"Error decoding 'deployedContracts.json': {e}") return { 'Bootstrap': data['clientChain'].get('bootstrapLogic'), 'ClientChainGateway': data['clientChain'].get('clientGatewayLogic'), 'Vault': data['clientChain'].get('vaultImplementation'), 'RewardVault': data['clientChain'].get('rewardVaultImplementation'), 'ExoCapsule': data['clientChain'].get('capsuleImplementation') }⛔ Skipped due to learnings
Learnt from: adu-web3 PR: ExocoreNetwork/exocore-contracts#112 File: .github/workflows/compare_deployed_storage_layout.py:10-12 Timestamp: 2024-10-17T03:16:14.054Z Learning: The `deployedContracts.json` file in the `script/` directory is enforced to be created for each release, so exception handling for its absence in `compare_deployed_storage_layout.py` is not necessary.
Description
Since we have decided to freeze the contract storage layout, to make deployed contracts upgradeable, we need to keep backwards compatibility for contract storage layout, so that these deployed contracts would be upgradeable in the next release.
By comparing storage layout against deployed contracts, we ensure future pull requests would not break the upgradeability for most of the contracts(client chain contracts). For
ExocoreGateway, we can compare against the main branch to make it sincecast storagedoes not work with Exocore chain for now.Summary by CodeRabbit
New Features
compare-storage-layoutjob with new checks for deployed contracts and improved failure message clarity.Chores
.gitignoreto ignore.secretsfiles for better security practices.