Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: enhance CI to check storage layout backwards compatibility #111

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 87 additions & 0 deletions .github/workflows/compare_deployed_storage_layout.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#!/usr/bin/env python

import json
import subprocess
import pandas as pd
import os
from compare_storage_layout import parse_output, compare_layouts, get_current_layout

def get_deployed_addresses():
with open('script/deployedContracts.json', 'r') as f:
data = json.load(f)
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use absolute paths to ensure file accessibility

The script assumes that script/deployedContracts.json is in the current working directory. This can lead to issues if the script is executed from a different directory. To make the script more robust, use an absolute path relative to the script's location.

Apply this change:

+script_dir = os.path.dirname(os.path.abspath(__file__))

 def get_deployed_addresses():
-    with open('script/deployedContracts.json', 'r') as f:
+    file_path = os.path.join(script_dir, 'script', 'deployedContracts.json')
+    with open(file_path, 'r') as f:
         data = json.load(f)

Committable suggestion was skipped due to low confidence.

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')
}

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.")
return pd.DataFrame()

result = subprocess.run(['cast', 'storage', address, '--rpc-url', rpc_url, '--etherscan-api-key', etherscan_api_key], capture_output=True, text=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Check for the 'cast' command before execution

If the cast command is not installed or not in the system's PATH, the script will raise a FileNotFoundError. To handle this gracefully, check if cast is available before attempting to run it, and provide a clear error message if it isn't.

Add this check before line 25:

import shutil

if not shutil.which('cast'):
    raise EnvironmentError("'cast' command not found. Please install Foundry to proceed.")

print(f"finish executing: cast storage {address} --rpc-url ...")

if result.returncode != 0:
raise Exception(f"Error getting current layout for {contract_name}: {result.stderr}")

return parse_output(contract_name, result.stdout.split('\n'))

def load_and_parse_layout(contract_name, path):
with open(path, 'r') as f:
lines = f.readlines()
return parse_output(contract_name, lines)

if __name__ == "__main__":
try:
api_key = os.environ.get('ALCHEMY_API_KEY')
if not api_key:
raise ValueError("ALCHEMY_API_KEY environment variable is not set")
etherscan_api_key = os.environ.get('ETHERSCAN_API_KEY')
if not etherscan_api_key:
raise ValueError("ETHERSCAN_API_KEY environment variable is not set")

# Construct the RPC URL for Sepolia
rpc_url = f"https://eth-sepolia.g.alchemy.com/v2/{api_key}"

addresses = get_deployed_addresses()
all_mismatches = {}

for contract_name, address in addresses.items():
print(f"Checking {contract_name}...")
deployed_layout = get_storage_layout(contract_name, address, rpc_url, etherscan_api_key)
if deployed_layout.empty:
print(f"No deployed layout found for {contract_name}.")
continue

current_layout = get_current_layout(contract_name)
if current_layout.empty:
raise ValueError(f"Error: No valid entries of current layout found for {contract_name}.")

mismatches = compare_layouts(deployed_layout, current_layout)
if mismatches:
all_mismatches[contract_name] = mismatches

# then we load the layout file of ExocoreGateway on target branch and compare it with the current layout
print("Checking ExocoreGateway...")
target_branch_layout = load_and_parse_layout('ExocoreGateway', 'ExocoreGateway_target.txt')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure correct file path for 'ExocoreGateway_target.txt'

The script expects ExocoreGateway_target.txt to be in the current working directory, which may not always be the case. To prevent file not found errors, use a path relative to the script's location.

Apply this change:

+        script_dir = os.path.dirname(os.path.abspath(__file__))
+        layout_path = os.path.join(script_dir, 'ExocoreGateway_target.txt')
-        target_branch_layout = load_and_parse_layout('ExocoreGateway', 'ExocoreGateway_target.txt')
+        target_branch_layout = load_and_parse_layout('ExocoreGateway', layout_path)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
target_branch_layout = load_and_parse_layout('ExocoreGateway', 'ExocoreGateway_target.txt')
script_dir = os.path.dirname(os.path.abspath(__file__))
layout_path = os.path.join(script_dir, 'ExocoreGateway_target.txt')
target_branch_layout = load_and_parse_layout('ExocoreGateway', layout_path)

current_layout = get_current_layout('ExocoreGateway')
mismatches = compare_layouts(target_branch_layout, current_layout)
if mismatches:
all_mismatches['ExocoreGateway'] = mismatches

if all_mismatches:
print("Mismatches found for current contracts:")
for contract, mismatches in all_mismatches.items():
print(f"{contract}:")
for mismatch in mismatches:
print(f" {mismatch}")
exit(1)
else:
print("Storage layout is compatible with all deployed contracts.")
except Exception as e:
print(f"Error: {e}")
exit(1)
Comment on lines +82 to +87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use sys.exit() instead of exit() for script termination

The built-in exit() function is meant for interactive shells and may not work as expected in scripts. Using sys.exit() ensures proper termination of the script in all environments.

Apply this change:

+import sys

             print(f"  {mismatch}")
-        exit(1)
+        sys.exit(1)
     else:
         print("Storage layout is compatible with all deployed contracts.")
 except Exception as e:
     print(f"Error: {e}")
-    exit(1)
+    sys.exit(1)

Committable suggestion was skipped due to low confidence.

105 changes: 59 additions & 46 deletions .github/workflows/compare_storage_layout.py
Original file line number Diff line number Diff line change
@@ -1,68 +1,81 @@
#!/usr/bin/env python

import pandas as pd
import os

def parse_layout(file_path):
expected_headers = ['Unnamed: 0', 'Name', 'Type', 'Slot', 'Offset', 'Bytes', 'Contract', 'Unnamed: 7']

if not os.path.isfile(file_path):
raise FileNotFoundError(f"Error: File {file_path} does not exist.")

# Read the file using pandas, with '|' as the delimiter
df = pd.read_csv(file_path, delimiter='|', engine='python', header=0)

# Trim leading/trailing whitespace from all columns
df.columns = [col.strip() for col in df.columns]
df = df.apply(lambda x: x.strip() if isinstance(x, str) else x)

# Check headers
if not all([df.columns[i] == expected_headers[i] for i in range(len(expected_headers))]):
raise ValueError(f"Error: Headers in {file_path} do not match expected headers.")

# Drop the second row (assuming it's a separator row)
df = df.drop(df.index[1])

# Combine relevant columns into a single string for comparison
df['Combined'] = df[['Name', 'Type', 'Slot', 'Offset', 'Bytes']].apply(lambda row: '|'.join(row.values), axis=1)

return df['Combined'].tolist()

def compare_layouts(clientChainGateway_entries, bootstrap_entries):
import subprocess

def parse_output(contract_name, lines):
# Clean up the output and create a dataframe
data = []
separator_line = len(lines);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unnecessary semicolon

There's an unnecessary semicolon at the end of the line. In Python, semicolons are not required at the end of statements.

Apply this diff to remove the semicolon:

-    separator_line = len(lines);
+    separator_line = len(lines)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
separator_line = len(lines);
separator_line = len(lines)
🧰 Tools
🪛 Ruff

9-9: Statement ends with an unnecessary semicolon

Remove unnecessary semicolon

(E703)

for i, line in enumerate(lines): # start from the line next to the separator
if i > separator_line and line.startswith('|'):
parts = [part.strip() for part in line.split('|')[1:-1]] # Remove empty first and last elements
data.append(parts[:6]) # Keep Name, Type, Slot, Offset, Bytes, Contract
elif line.startswith('|') and 'Name' in line:
separator_line = i + 1

Comment on lines +11 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix the loop condition to correctly parse the output

The current loop condition may prevent the parsing logic from working as intended. Since separator_line is initialized to len(lines), the condition i > separator_line will always be False because i ranges from 0 to len(lines) - 1. This means the loop might skip processing the necessary lines, resulting in data being empty.

Consider initializing separator_line to -1 so that the condition i > separator_line becomes True after the header line is identified.

Apply this diff to adjust the initialization:

-    separator_line = len(lines)
+    separator_line = -1

Alternatively, adjust the condition in the loop to correctly process the lines after the header.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if i > separator_line and line.startswith('|'):
parts = [part.strip() for part in line.split('|')[1:-1]] # Remove empty first and last elements
data.append(parts[:6]) # Keep Name, Type, Slot, Offset, Bytes, Contract
elif line.startswith('|') and 'Name' in line:
separator_line = i + 1
if i > separator_line and line.startswith('|'):
parts = [part.strip() for part in line.split('|')[1:-1]] # Remove empty first and last elements
data.append(parts[:6]) # Keep Name, Type, Slot, Offset, Bytes, Contract
elif line.startswith('|') and 'Name' in line:
separator_line = i + 1
separator_line = -1

if not data:
raise Exception(f"No valid storage layout data found for {contract_name}")

df = pd.DataFrame(data, columns=['Name', 'Type', 'Slot', 'Offset', 'Bytes', 'Contract'])

# Convert numeric columns
for col in ['Slot', 'Offset', 'Bytes']:
df[col] = pd.to_numeric(df[col])

return df

def get_current_layout(contract_name):
result = subprocess.run(['forge', 'inspect', f'src/core/{contract_name}.sol:{contract_name}', 'storage-layout', '--pretty'], capture_output=True, text=True)
print(f"finished executing forge inspect for {contract_name}")

if result.returncode != 0:
raise Exception(f"Error getting current layout for {contract_name}: {result.stderr}")

return parse_output(contract_name, result.stdout.split('\n'))

def compare_layouts(old_layout, new_layout):
mismatches = []
length = len(bootstrap_entries)

if length > len(clientChainGateway_entries):
mismatches.append("Error: Bootstrap entries are more than ClientChainGateway entries.")
return mismatches

for i in range(length):
if bootstrap_entries[i] != clientChainGateway_entries[i]:
mismatches.append(f"Mismatch at position {i + 1}: {bootstrap_entries[i]} != {clientChainGateway_entries[i]}")
# Ensure both dataframes have the same columns
columns = ['Name', 'Type', 'Slot', 'Offset', 'Bytes']
old_layout = old_layout[columns].copy()
new_layout = new_layout[columns].copy()

# Compare non-gap variables
for index, row in old_layout.iterrows():
if row['Name'] != '__gap':
current_row = new_layout.loc[new_layout['Name'] == row['Name']]
if current_row.empty:
mismatches.append(f"Variable {row['Name']} is missing in the current layout")
elif not current_row.iloc[0].equals(row):
mismatches.append(f"Variable {row['Name']} has changed")

if not mismatches:
print(f"No mismatches found")

return mismatches

if __name__ == "__main__":
try:
clientChainGateway_entries = parse_layout("ClientChainGateway.md")
bootstrap_entries = parse_layout("Bootstrap.md")
clientChainGateway_layout = get_current_layout("ClientChainGateway")
bootstrap_layout = get_current_layout("Bootstrap")

if not clientChainGateway_entries:
raise ValueError("Error: No valid entries found in ClientChainGateway.md.")
if clientChainGateway_layout.empty:
raise ValueError("Error: No valid entries found for ClientChainGateway.")

if not bootstrap_entries:
raise ValueError("Error: No valid entries found in Bootstrap.md.")
if bootstrap_layout.empty:
raise ValueError("Error: No valid entries found for Bootstrap.")

mismatches = compare_layouts(clientChainGateway_entries, bootstrap_entries)
mismatches = compare_layouts(bootstrap_layout, clientChainGateway_layout)

if mismatches:
print(f"Mismatches found: {len(mismatches)}")
for mismatch in mismatches:
print(mismatch)
exit(1)
else:
print("All entries in Bootstrap are present in ClientChainGateway at the correct positions.")
print("All entries in Bootstrap match ClientChainGateway at the correct positions.")
except Exception as e:
print(e)
print(f"Error: {e}")
exit(1)

35 changes: 29 additions & 6 deletions .github/workflows/forge-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -156,18 +156,41 @@ jobs:
./cache
./broadcast
key: ${{ runner.os }}-build-${{ github.sha }}
- name: Run forge inspect storage layout on ClientChainGateway
run: forge inspect --pretty src/core/ClientChainGateway.sol:ClientChainGateway storageLayout > ClientChainGateway.md
- name: Run forge inspect storage layout on Bootstrap
run: forge inspect --pretty src/core/Bootstrap.sol:Bootstrap storageLayout > Bootstrap.md
- name: Checkout target branch
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.base.ref }}
submodules: recursive
- name: Generate target branch layout files
run: |
forge inspect src/core/ExocoreGateway.sol:ExocoreGateway storage-layout --pretty > ExocoreGateway_target.txt
- name: Cache target branch layout file
uses: actions/cache/save@v3
with:
path: ExocoreGateway_target.txt
key: ${{ runner.os }}-exocore-target-${{ github.sha }}
- name: Checkout back to PR
uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Restore target branch layout file
uses: actions/cache/restore@v3
with:
path: ExocoreGateway_target.txt
key: ${{ runner.os }}-exocore-target-${{ github.sha }}
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: '3.12.4'
- name: Install pandas
run: pip install --root-user-action=ignore pandas==2.2.2
- name: Run the comparison script
- name: Run the comparison script for Bootstrap and ClientChainGateway
run: python .github/workflows/compare_storage_layout.py
- name: Run the comparison script for deployed contracts
run: python .github/workflows/compare_deployed_storage_layout.py
env:
ALCHEMY_API_KEY: ${{ secrets.ALCHEMY_API_KEY }}
ETHERSCAN_API_KEY: ${{ secrets.ETHERSCAN_API_KEY }}
- name: Add comment for storage layout mismatch failure
if: failure()
uses: actions/github-script@v6
Expand All @@ -178,5 +201,5 @@ jobs:
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'The storage layout of Bootstrap and ClientChainGateway is not the same. Please check the logs.'
body: 'Storage layout compatibility check failed. This could be due to a mismatch between Bootstrap and ClientChainGateway, or incompatibility with deployed contracts on Sepolia. Please check the logs for details.'
})
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,6 @@ node_modules
.idea

.gas-snapshot

## secret
.secrets
Loading