Skip to content

Commit 230a611

Browse files
Eikixdartdart26
authored andcommitted
feat(contracts): add contract upgrade version check (#2112)
* feat(contracts): enable deterministic bytecode output Add `cbor_metadata = false` and `bytecode_hash = 'none'` to both host-contracts and gateway-contracts foundry.toml. This strips compiler metadata from deployed bytecode, making bytecode comparison across branches reliable for upgrade hygiene checks. Also add OZ remappings to gateway-contracts for Foundry compatibility. * feat(contracts): add contract upgrade hygiene CI check New workflow + Bun/TypeScript script that compares deployed bytecode between main and PR for every contract in upgrade-manifest.json. When bytecode changes, the check enforces: - REINITIALIZER_VERSION must be bumped - A reinitializeV{N-1}() function must exist (not just commented out) - At least one of MAJOR/MINOR/PATCH_VERSION must be bumped When bytecode is unchanged, spurious REINITIALIZER_VERSION bumps are flagged. Uses matrix strategy for host-contracts and gateway-contracts. Addresses are generated via `make ensure-addresses` (same as upgrade tests). * feat(ci): compare bytecode against last deployed release tag Compare PR bytecode against UPGRADE_FROM_TAG (v0.11.0) instead of main. This avoids unnecessary reinitializer bumps when multiple PRs modify the same contract between deployments — only the first change after a release requires a bump. Keeps the tag in sync with *-upgrade-tests.yml workflows. * fix(ci): remove deleted source files from baseline before compiling When comparing against the v0.11.0 baseline, forge tries to compile all .sol files including ones that were deleted in the PR (e.g. MultichainACLChecks.sol). These reference removed address constants and break compilation of unrelated contracts like GatewayConfig, causing false positives. Strip deleted files from the baseline before compiling. * fix(ci): fall back to source comparison when baseline compilation fails When forge can't compile the baseline (e.g. deleted contracts break unrelated imports), compare source files directly instead of blindly treating all contracts as changed. If the contract source is identical between baseline and PR, bytecode is unchanged — no version bump needed. Fixes false positive for GatewayConfig which is unchanged since v0.11.0 but couldn't be compiled because MultichainACLChecks.sol (deleted in PR) was still imported by other v0.11.0 files. * fix(ci): merge address constants from both baseline and PR When comparing bytecode between the baseline tag and the PR, both sides must compile with identical address constants (they're embedded in bytecode). Previously we generated addresses only on the PR side and copied them to the baseline. This broke when contracts were removed between versions — the baseline still had source files importing the deleted constant, causing forge compilation to fail for the entire project, including unrelated unchanged contracts. Add ci/merge-address-constants.ts: generates addresses on both sides independently, then merges them. PR values win for shared constants, baseline-only constants (removed contracts) are preserved so the baseline compiles, and PR-only constants (new contracts) are preserved so the PR compiles. Reverts the source-comparison fallback in check-upgrade-hygiene.ts — both sides now compile successfully so we always compare real bytecode. * refactor(ci): rename upgrade hygiene to upgrade version check Rename workflow, script, job names, and all references from the ambiguous "upgrade hygiene" to the descriptive "upgrade version check". * chore: remove accidentally committed plan file
1 parent c5178f3 commit 230a611

File tree

5 files changed

+421
-0
lines changed

5 files changed

+421
-0
lines changed
Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
name: contracts-upgrade-version-check
2+
3+
permissions: {}
4+
5+
on:
6+
pull_request:
7+
8+
# Compare PR bytecode against the last deployed release, not main.
9+
# This avoids unnecessary reinitializer bumps when multiple PRs modify
10+
# the same contract between deployments. Keep in sync with *-upgrade-tests.yml.
11+
env:
12+
UPGRADE_FROM_TAG: v0.11.0
13+
14+
concurrency:
15+
group: ${{ github.workflow }}-${{ github.ref }}
16+
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}
17+
18+
jobs:
19+
check-changes:
20+
name: contracts-upgrade-version-check/check-changes
21+
permissions:
22+
contents: 'read' # Required to checkout repository code
23+
pull-requests: 'read' # Required to read pull request for paths-filter
24+
runs-on: ubuntu-latest
25+
outputs:
26+
packages: ${{ steps.filter.outputs.changes }}
27+
steps:
28+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
29+
with:
30+
persist-credentials: 'false'
31+
- uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2
32+
id: filter
33+
with:
34+
filters: |
35+
host-contracts:
36+
- .github/workflows/contracts-upgrade-version-check.yml
37+
- ci/check-upgrade-versions.ts
38+
- ci/merge-address-constants.ts
39+
- host-contracts/**
40+
gateway-contracts:
41+
- .github/workflows/contracts-upgrade-version-check.yml
42+
- ci/check-upgrade-versions.ts
43+
- ci/merge-address-constants.ts
44+
- gateway-contracts/**
45+
46+
check:
47+
name: contracts-upgrade-version-check/${{ matrix.package }} (bpr)
48+
needs: check-changes
49+
if: ${{ needs.check-changes.outputs.packages != '[]' }}
50+
permissions:
51+
contents: 'read' # Required to checkout repository code
52+
runs-on: ubuntu-latest
53+
strategy:
54+
fail-fast: false
55+
matrix:
56+
package: ${{ fromJSON(needs.check-changes.outputs.packages) }}
57+
include:
58+
- package: host-contracts
59+
extra-deps: forge soldeer install
60+
- package: gateway-contracts
61+
extra-deps: ''
62+
steps:
63+
- name: Checkout PR branch
64+
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
65+
with:
66+
persist-credentials: 'false'
67+
68+
- name: Checkout baseline (last deployed release)
69+
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
70+
with:
71+
ref: ${{ env.UPGRADE_FROM_TAG }}
72+
path: baseline
73+
persist-credentials: 'false'
74+
75+
- name: Install Bun
76+
uses: oven-sh/setup-bun@735343b667d3e6f658f44d0eca948eb6282f2b76 # v2.0.2
77+
78+
- name: Install Foundry
79+
uses: foundry-rs/foundry-toolchain@82dee4ba654bd2146511f85f0d013af94670c4de # v1.4.0
80+
81+
- name: Install PR dependencies
82+
working-directory: ${{ matrix.package }}
83+
run: npm ci
84+
85+
- name: Install baseline dependencies
86+
working-directory: baseline/${{ matrix.package }}
87+
run: npm ci
88+
89+
- name: Install Forge dependencies
90+
if: matrix.extra-deps != ''
91+
env:
92+
PACKAGE: ${{ matrix.package }}
93+
EXTRA_DEPS: ${{ matrix.extra-deps }}
94+
run: |
95+
(cd "$PACKAGE" && $EXTRA_DEPS)
96+
(cd "baseline/$PACKAGE" && $EXTRA_DEPS)
97+
98+
- name: Setup compilation
99+
env:
100+
PACKAGE: ${{ matrix.package }}
101+
run: |
102+
# Generate addresses on both sides independently, then merge them.
103+
# Address constants are embedded in bytecode, so both sides must compile
104+
# with identical values. We can't just copy one side's addresses to the
105+
# other because contracts may be added or removed between versions — the
106+
# baseline would fail to compile if it references a removed constant, or
107+
# the PR would fail if it references a new one. Merging gives both sides
108+
# the full union of constants with consistent values (PR wins for shared).
109+
(cd "$PACKAGE" && make ensure-addresses)
110+
(cd "baseline/$PACKAGE" && make ensure-addresses)
111+
bun ci/merge-address-constants.ts "baseline/$PACKAGE/addresses" "$PACKAGE/addresses"
112+
# Use PR's foundry.toml for both so compiler settings match (cbor_metadata, bytecode_hash)
113+
cp "$PACKAGE/foundry.toml" "baseline/$PACKAGE/foundry.toml"
114+
115+
- name: Run upgrade version check
116+
env:
117+
PACKAGE: ${{ matrix.package }}
118+
run: bun ci/check-upgrade-versions.ts "baseline/$PACKAGE" "$PACKAGE"

ci/check-upgrade-versions.ts

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
#!/usr/bin/env bun
2+
// Checks that upgradeable contracts have proper version bumps when bytecode changes.
3+
// Usage: bun ci/check-upgrade-versions.ts <baseline-pkg-dir> <pr-pkg-dir>
4+
5+
import { readFileSync, existsSync } from "fs";
6+
import { execSync } from "child_process";
7+
import { join } from "path";
8+
9+
const [baselineDir, prDir] = process.argv.slice(2);
10+
if (!baselineDir || !prDir) {
11+
console.error("Usage: bun ci/check-upgrade-versions.ts <baseline-pkg-dir> <pr-pkg-dir>");
12+
process.exit(1);
13+
}
14+
15+
const manifestPath = join(prDir, "upgrade-manifest.json");
16+
if (!existsSync(manifestPath)) {
17+
console.error(`::error::upgrade-manifest.json not found in ${prDir}`);
18+
process.exit(1);
19+
}
20+
21+
const VERSION_RE = /(?<name>REINITIALIZER_VERSION|MAJOR_VERSION|MINOR_VERSION|PATCH_VERSION)\s*=\s*(?<value>\d+)/g;
22+
23+
function extractVersions(filePath: string) {
24+
const source = readFileSync(filePath, "utf-8");
25+
const versions: Record<string, number> = {};
26+
for (const { groups } of source.matchAll(VERSION_RE)) {
27+
versions[groups!.name] = Number(groups!.value);
28+
}
29+
return { versions, source };
30+
}
31+
32+
function forgeInspect(contract: string, root: string): string | null {
33+
try {
34+
const raw = execSync(`forge inspect "contracts/${contract}.sol:${contract}" --root "${root}" deployedBytecode`, {
35+
encoding: "utf-8",
36+
stdio: ["pipe", "pipe", "pipe"],
37+
env: { ...process.env, NO_COLOR: "1" },
38+
});
39+
// Extract hex bytecode — forge may prepend ANSI codes or compilation progress to stdout
40+
const match = raw.match(/0x[0-9a-fA-F]+/);
41+
return match ? match[0] : null;
42+
} catch (e: any) {
43+
if (e.stderr) console.error(String(e.stderr));
44+
return null;
45+
}
46+
}
47+
48+
const contracts: string[] = JSON.parse(readFileSync(manifestPath, "utf-8"));
49+
let errors = 0;
50+
51+
for (const name of contracts) {
52+
console.log(`::group::Checking ${name}`);
53+
try {
54+
const baseSol = join(baselineDir, "contracts", `${name}.sol`);
55+
const prSol = join(prDir, "contracts", `${name}.sol`);
56+
57+
if (!existsSync(baseSol)) {
58+
console.log(`Skipping ${name} (new contract, not in baseline)`);
59+
continue;
60+
}
61+
62+
if (!existsSync(prSol)) {
63+
console.error(`::error::${name} listed in upgrade-manifest.json but missing in PR`);
64+
errors++;
65+
continue;
66+
}
67+
68+
const { versions: baseV } = extractVersions(baseSol);
69+
const { versions: prV, source: prSrc } = extractVersions(prSol);
70+
71+
let parseFailed = false;
72+
for (const key of ["REINITIALIZER_VERSION", "MAJOR_VERSION", "MINOR_VERSION", "PATCH_VERSION"]) {
73+
if (baseV[key] == null || prV[key] == null) {
74+
console.error(`::error::Failed to parse ${key} for ${name}`);
75+
errors++;
76+
parseFailed = true;
77+
}
78+
}
79+
if (parseFailed) continue;
80+
81+
const prBytecode = forgeInspect(name, prDir);
82+
if (prBytecode == null) {
83+
console.error(`::error::Failed to compile ${name} on PR`);
84+
errors++;
85+
continue;
86+
}
87+
88+
const baseBytecode = forgeInspect(name, baselineDir);
89+
if (baseBytecode == null) {
90+
console.error(`::error::Failed to compile ${name} on baseline`);
91+
errors++;
92+
continue;
93+
}
94+
const bytecodeChanged = baseBytecode !== prBytecode;
95+
const reinitChanged = baseV.REINITIALIZER_VERSION !== prV.REINITIALIZER_VERSION;
96+
const versionChanged =
97+
baseV.MAJOR_VERSION !== prV.MAJOR_VERSION ||
98+
baseV.MINOR_VERSION !== prV.MINOR_VERSION ||
99+
baseV.PATCH_VERSION !== prV.PATCH_VERSION;
100+
101+
if (!bytecodeChanged) {
102+
console.log(`${name}: bytecode unchanged`);
103+
if (reinitChanged) {
104+
console.error(
105+
`::error::${name} REINITIALIZER_VERSION bumped (${baseV.REINITIALIZER_VERSION} -> ${prV.REINITIALIZER_VERSION}) but bytecode is unchanged`,
106+
);
107+
errors++;
108+
}
109+
continue;
110+
}
111+
112+
console.log(`${name}: bytecode CHANGED`);
113+
114+
if (!reinitChanged) {
115+
console.error(
116+
`::error::${name} bytecode changed but REINITIALIZER_VERSION was not bumped (still ${prV.REINITIALIZER_VERSION})`,
117+
);
118+
errors++;
119+
} else {
120+
// Convention: reinitializeV{N-1} for REINITIALIZER_VERSION=N
121+
const expectedFn = `reinitializeV${prV.REINITIALIZER_VERSION - 1}`;
122+
const uncommented = prSrc.replace(/\/\*[\s\S]*?\*\//g, "").replace(/\/\/.*$/gm, "");
123+
if (!new RegExp(`function\\s+${expectedFn}\\s*\\(`).test(uncommented)) {
124+
console.error(
125+
`::error::${name} has REINITIALIZER_VERSION=${prV.REINITIALIZER_VERSION} but no ${expectedFn}() function found`,
126+
);
127+
errors++;
128+
}
129+
}
130+
131+
if (!versionChanged) {
132+
console.error(
133+
`::error::${name} bytecode changed but semantic version was not bumped (still v${prV.MAJOR_VERSION}.${prV.MINOR_VERSION}.${prV.PATCH_VERSION})`,
134+
);
135+
errors++;
136+
}
137+
} finally {
138+
console.log("::endgroup::");
139+
}
140+
}
141+
142+
if (errors > 0) {
143+
console.error(`::error::Upgrade version check failed with ${errors} error(s)`);
144+
process.exit(1);
145+
}
146+
147+
console.log("All contracts passed upgrade version checks");

0 commit comments

Comments
 (0)