-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(contracts): add contract upgrade hygiene check #2108
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
Closed
Closed
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
bb6b207
feat(ci): add contract upgrade hygiene check
Eikix c9aff6f
fix: align foundry.toml between main and PR checkouts
Eikix cee37c6
refactor: harden and optimize check-upgrade-hygiene script
Eikix 7633721
fix: extract first integer after = in awk version parsing
Eikix 2133dfe
fix: prevent stderr warnings from contaminating bytecode comparison
Eikix feb1667
refactor: deduplicate workflow with matrix, extract stubs to files
Eikix 7c838fb
fix: use working-directory for npm ci, skip empty extra-deps
Eikix 99a4931
fix: add explanatory comments on permissions (zizmor)
Eikix 93c8523
fix: use env vars instead of template expansion in run blocks (zizmor)
Eikix 3514718
refactor: rewrite check-upgrade-hygiene in TypeScript (Bun)
Eikix 1d771ca
refactor: harden upgrade hygiene check
Eikix df7988b
docs: add explanatory comments to CI address stubs
Eikix 48d1154
fix: add (bpr) suffix and conditional cancel-in-progress
Eikix File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,155 @@ | ||
| name: contracts-upgrade-hygiene | ||
|
|
||
| permissions: {} | ||
|
|
||
| on: | ||
| pull_request: | ||
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| check-changes: | ||
| name: contracts-upgrade-hygiene/check-changes | ||
| permissions: | ||
| contents: 'read' | ||
| pull-requests: 'read' | ||
|
||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| host-contracts: ${{ steps.filter.outputs.host-contracts }} | ||
| gateway-contracts: ${{ steps.filter.outputs.gateway-contracts }} | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: 'false' | ||
| - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 | ||
| id: filter | ||
| with: | ||
| filters: | | ||
| host-contracts: | ||
| - .github/workflows/contracts-upgrade-hygiene.yml | ||
| - ci/check-upgrade-hygiene.sh | ||
| - host-contracts/** | ||
| gateway-contracts: | ||
| - .github/workflows/contracts-upgrade-hygiene.yml | ||
| - ci/check-upgrade-hygiene.sh | ||
| - gateway-contracts/** | ||
|
|
||
| host-contracts: | ||
| name: contracts-upgrade-hygiene/host-contracts | ||
| needs: check-changes | ||
| if: ${{ needs.check-changes.outputs.host-contracts == 'true' }} | ||
| permissions: | ||
| contents: 'read' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout PR branch | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: 'false' | ||
|
|
||
| - name: Checkout main branch | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| ref: main | ||
| path: main-branch | ||
| persist-credentials: 'false' | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0 | ||
|
|
||
| - name: Install PR dependencies | ||
| working-directory: host-contracts | ||
| run: | | ||
| npm ci | ||
| forge soldeer install | ||
|
|
||
| - name: Install main dependencies | ||
| working-directory: main-branch/host-contracts | ||
| run: | | ||
| npm ci | ||
| forge soldeer install | ||
|
|
||
| - name: Align compilation settings | ||
| run: cp host-contracts/foundry.toml main-branch/host-contracts/foundry.toml | ||
|
|
||
| - name: Generate address stubs | ||
| run: | | ||
| # Host contracts need addresses/FHEVMHostAddresses.sol (generated at deploy time). | ||
| # Create identical stubs in both copies so bytecode comparison is not affected. | ||
| mkdir -p host-contracts/addresses main-branch/host-contracts/addresses | ||
| cat > /tmp/FHEVMHostAddresses.sol << 'SOL' | ||
| // SPDX-License-Identifier: BSD-3-Clause-Clear | ||
| pragma solidity ^0.8.24; | ||
| address constant aclAdd = 0x0000000000000000000000000000000000000001; | ||
| address constant fhevmExecutorAdd = 0x0000000000000000000000000000000000000002; | ||
| address constant kmsVerifierAdd = 0x0000000000000000000000000000000000000003; | ||
| address constant inputVerifierAdd = 0x0000000000000000000000000000000000000004; | ||
| address constant hcuLimitAdd = 0x0000000000000000000000000000000000000005; | ||
| address constant pauserSetAdd = 0x0000000000000000000000000000000000000006; | ||
| SOL | ||
| cp /tmp/FHEVMHostAddresses.sol host-contracts/addresses/FHEVMHostAddresses.sol | ||
| cp /tmp/FHEVMHostAddresses.sol main-branch/host-contracts/addresses/FHEVMHostAddresses.sol | ||
|
|
||
| - name: Run upgrade hygiene check | ||
| run: | | ||
| chmod +x ci/check-upgrade-hygiene.sh | ||
| ./ci/check-upgrade-hygiene.sh main-branch/host-contracts host-contracts | ||
|
|
||
| gateway-contracts: | ||
| name: contracts-upgrade-hygiene/gateway-contracts | ||
| needs: check-changes | ||
| if: ${{ needs.check-changes.outputs.gateway-contracts == 'true' }} | ||
| permissions: | ||
| contents: 'read' | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout PR branch | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| persist-credentials: 'false' | ||
|
|
||
| - name: Checkout main branch | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| ref: main | ||
| path: main-branch | ||
| persist-credentials: 'false' | ||
|
|
||
| - name: Install Foundry | ||
| uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0 | ||
|
|
||
| - name: Install PR dependencies | ||
| working-directory: gateway-contracts | ||
| run: npm ci | ||
|
|
||
| - name: Install main dependencies | ||
| working-directory: main-branch/gateway-contracts | ||
| run: npm ci | ||
|
|
||
| - name: Align compilation settings | ||
| run: cp gateway-contracts/foundry.toml main-branch/gateway-contracts/foundry.toml | ||
|
|
||
| - name: Generate address stubs | ||
| run: | | ||
| # Gateway contracts need addresses/GatewayAddresses.sol (generated at deploy time). | ||
| mkdir -p gateway-contracts/addresses main-branch/gateway-contracts/addresses | ||
| cat > /tmp/GatewayAddresses.sol << 'SOL' | ||
| // SPDX-License-Identifier: BSD-3-Clause-Clear | ||
| pragma solidity ^0.8.24; | ||
| address constant gatewayConfigAddress = 0x0000000000000000000000000000000000000001; | ||
| address constant decryptionAddress = 0x0000000000000000000000000000000000000002; | ||
| address constant ciphertextCommitsAddress = 0x0000000000000000000000000000000000000003; | ||
| address constant inputVerificationAddress = 0x0000000000000000000000000000000000000004; | ||
| address constant kmsGenerationAddress = 0x0000000000000000000000000000000000000005; | ||
| address constant protocolPaymentAddress = 0x0000000000000000000000000000000000000006; | ||
| address constant pauserSetAddress = 0x0000000000000000000000000000000000000007; | ||
| SOL | ||
| cp /tmp/GatewayAddresses.sol gateway-contracts/addresses/GatewayAddresses.sol | ||
| cp /tmp/GatewayAddresses.sol main-branch/gateway-contracts/addresses/GatewayAddresses.sol | ||
|
|
||
| - name: Run upgrade hygiene check | ||
| run: | | ||
| chmod +x ci/check-upgrade-hygiene.sh | ||
| ./ci/check-upgrade-hygiene.sh main-branch/gateway-contracts gateway-contracts | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| #!/usr/bin/env bash | ||
| # ci/check-upgrade-hygiene.sh | ||
| # | ||
| # Validates that upgradeable contracts have proper version bumps when bytecode changes. | ||
| # Compares deployed bytecodes between two copies of a contract package (e.g. main vs PR branch). | ||
| # | ||
| # Usage: | ||
| # ./ci/check-upgrade-hygiene.sh <main-pkg-dir> <pr-pkg-dir> | ||
| # | ||
| # Example: | ||
| # ./ci/check-upgrade-hygiene.sh main-branch/host-contracts host-contracts | ||
| # | ||
| # Requires: forge (Foundry), jq | ||
| # Both directories must have: | ||
| # - foundry.toml with cbor_metadata=false and bytecode_hash='none' | ||
| # - upgrade-manifest.json listing contract names | ||
| # - contracts/<Name>.sol for each manifest entry | ||
| # - addresses/ stub (generated file) so contracts compile | ||
|
|
||
| set -euo pipefail | ||
|
|
||
| MAIN_DIR="$1" | ||
| PR_DIR="$2" | ||
|
|
||
| if [ ! -f "$PR_DIR/upgrade-manifest.json" ]; then | ||
| echo "::error::upgrade-manifest.json not found in $PR_DIR" | ||
| exit 1 | ||
| fi | ||
|
|
||
| ERRORS=0 | ||
|
|
||
| extract_const() { | ||
| local file="$1" const="$2" | ||
| sed -n "s/.*${const}[[:space:]]*=[[:space:]]*\([0-9]*\).*/\1/p" "$file" | ||
| } | ||
|
|
||
| for name in $(jq -r '.[]' "$PR_DIR/upgrade-manifest.json"); do | ||
| echo "::group::Checking $name" | ||
|
|
||
| main_sol="$MAIN_DIR/contracts/${name}.sol" | ||
| pr_sol="$PR_DIR/contracts/${name}.sol" | ||
|
|
||
| # Skip contracts not present on main (newly added) | ||
| if [ ! -f "$main_sol" ]; then | ||
| echo "Skipping $name (new contract, not on main)" | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| if [ ! -f "$pr_sol" ]; then | ||
| echo "::error::$name is in upgrade-manifest.json but contracts/${name}.sol not found in PR" | ||
| ERRORS=$((ERRORS + 1)) | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| # --- Extract version constants from both --- | ||
| main_reinit=$(extract_const "$main_sol" "REINITIALIZER_VERSION") | ||
| pr_reinit=$(extract_const "$pr_sol" "REINITIALIZER_VERSION") | ||
| main_major=$(extract_const "$main_sol" "MAJOR_VERSION") | ||
| pr_major=$(extract_const "$pr_sol" "MAJOR_VERSION") | ||
| main_minor=$(extract_const "$main_sol" "MINOR_VERSION") | ||
| pr_minor=$(extract_const "$pr_sol" "MINOR_VERSION") | ||
| main_patch=$(extract_const "$main_sol" "PATCH_VERSION") | ||
| pr_patch=$(extract_const "$pr_sol" "PATCH_VERSION") | ||
|
|
||
| for var in main_reinit pr_reinit main_major pr_major main_minor pr_minor main_patch pr_patch; do | ||
| if [ -z "${!var}" ]; then | ||
| echo "::error::Failed to parse $var for $name" | ||
| ERRORS=$((ERRORS + 1)) | ||
| echo "::endgroup::" | ||
| continue 2 | ||
| fi | ||
| done | ||
|
|
||
| # --- Compare bytecodes (paths relative to --root) --- | ||
| main_bytecode=$(forge inspect "contracts/${name}.sol:$name" --root "$MAIN_DIR" deployedBytecode) | ||
| pr_bytecode=$(forge inspect "contracts/${name}.sol:$name" --root "$PR_DIR" deployedBytecode) | ||
|
|
||
| bytecode_changed=false | ||
| if [ "$main_bytecode" != "$pr_bytecode" ]; then | ||
| bytecode_changed=true | ||
| fi | ||
|
|
||
| version_changed=false | ||
| if [ "$main_major" != "$pr_major" ] || [ "$main_minor" != "$pr_minor" ] || [ "$main_patch" != "$pr_patch" ]; then | ||
| version_changed=true | ||
| fi | ||
|
|
||
| reinit_changed=false | ||
| if [ "$main_reinit" != "$pr_reinit" ]; then | ||
| reinit_changed=true | ||
| fi | ||
|
|
||
| if [ "$bytecode_changed" = true ]; then | ||
| echo "$name: bytecode CHANGED" | ||
|
|
||
| # Check 1: REINITIALIZER_VERSION must be bumped | ||
| if [ "$reinit_changed" = false ]; then | ||
| echo "::error::$name bytecode changed but REINITIALIZER_VERSION was not bumped (still $pr_reinit)" | ||
| ERRORS=$((ERRORS + 1)) | ||
| fi | ||
|
|
||
| # Check 2: reinitializeVN function must exist (convention: N = REINITIALIZER_VERSION - 1) | ||
| if [ "$reinit_changed" = true ]; then | ||
| expected_n=$((pr_reinit - 1)) | ||
| expected_fn="reinitializeV${expected_n}" | ||
| # Look for function declaration (not just any mention) | ||
| if ! grep -qE "function[[:space:]]+${expected_fn}[[:space:]]*\(" "$pr_sol"; then | ||
| echo "::error::$name has REINITIALIZER_VERSION=$pr_reinit but no $expected_fn() function found" | ||
| ERRORS=$((ERRORS + 1)) | ||
| fi | ||
| fi | ||
|
|
||
| # Check 3: Semantic version must be bumped | ||
| if [ "$version_changed" = false ]; then | ||
| echo "::error::$name bytecode changed but semantic version was not bumped (still v${pr_major}.${pr_minor}.${pr_patch})" | ||
| ERRORS=$((ERRORS + 1)) | ||
| fi | ||
|
|
||
| else | ||
| echo "$name: bytecode unchanged" | ||
|
|
||
| # Inverse check: reinitializer should NOT be bumped if bytecode didn't change | ||
| if [ "$reinit_changed" = true ]; then | ||
| echo "::error::$name REINITIALIZER_VERSION bumped ($main_reinit -> $pr_reinit) but bytecode is unchanged" | ||
| ERRORS=$((ERRORS + 1)) | ||
| fi | ||
| fi | ||
|
|
||
| echo "::endgroup::" | ||
| done | ||
|
|
||
| if [ "$ERRORS" -gt 0 ]; then | ||
| echo "::error::Upgrade hygiene check failed with $ERRORS error(s)" | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "All contracts passed upgrade hygiene checks" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[AUTOMATED] Nit: Unconditional
cancel-in-progress: true.18 out of 22 workflows in this repo use the conditional pattern:
Since this workflow only triggers on
pull_request, the difference is purely cosmetic (it can never run on main). However, using the conditional pattern maintains consistency and is future-proof if the trigger is later expanded.Confidence: 95/100