feat(contracts): add contract upgrade hygiene check#2108
feat(contracts): add contract upgrade hygiene check#2108
Conversation
Add a lightweight CI workflow that compares PR contract bytecodes against main and fails fast if bytecode changed without proper version bumps. When bytecode changes, the check asserts: - REINITIALIZER_VERSION was bumped - reinitializeVN() function exists (N = REINITIALIZER_VERSION - 1) - Semantic version (MAJOR/MINOR/PATCH) was bumped When bytecode is unchanged, asserts reinitializer was NOT bumped. Covers both host-contracts and gateway-contracts via a shared script. Runs independently of existing upgrade-tests workflows (no Docker/Anvil). Also updates both foundry.toml files with cbor_metadata=false and bytecode_hash='none' for deterministic path-independent bytecode, and adds OZ remappings to gateway-contracts so forge can compile it.
🧪 CI InsightsHere's what we observed from your CI run for 48d1154. 🟢 All jobs passed!But CI Insights is watching 👀 |
Eikix
changed the title
Copy the PR's foundry.toml (with cbor_metadata=false and bytecode_hash='none') to the main checkout before compiling. Without this, main compiles with its original settings, producing different metadata → false bytecode diffs for every contract.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Replace 8 per-contract sed calls with single-pass awk extraction (fixes latent bug where multiple matches could silently corrupt values, and [0-9]* matching zero digits) - Pre-compile both roots in parallel (forge build) so all forge inspect calls are cache hits — halves compilation wall time in CI - Handle forge inspect failures gracefully (report + continue) instead of aborting the entire script under set -e - Remove unnecessary chmod +x in workflow (file already has exec bit)
The previous $NF approach grabbed the last whitespace-delimited field, which would silently pick up digits from trailing comments (e.g. `MAJOR_VERSION = 12; // was 11` extracted 11, not 12). Now uses val_after_eq() to extract the first integer after `=`.
forge inspect can emit compiler warnings to stderr even on success. With 2>&1, those warnings were mixed into the bytecode string, causing false positives when warnings differed between main and PR. Now stderr is discarded on success (2>/dev/null) and only captured on failure for the error message.
- Replace two near-identical jobs with a single matrix job (host-contracts, gateway-contracts), cutting workflow from 151 to 92 lines - Move inline Solidity address heredocs to committed ci/stubs/ files - Add missing PaymentBridgingAddresses.sol stub (was breaking gateway CI) - Drop forge build pre-step: forge inspect already compiles and caches on first call, and forge build required ALL stubs including non-manifest contracts which caused failures - Trim script from 161 to 110 lines: shorter header, flattened control flow
There was a problem hiding this comment.
zizmor found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
npm --prefix doesn't work with npm ci when package-lock.json is in the target directory. Use working-directory instead. Also guard extra-deps step with matrix.if to avoid empty subshell syntax error.
zizmor flags ${{ matrix.* }} in run: blocks as code injection risk.
Pass matrix values through env vars instead, which is the recommended
safe pattern for GitHub Actions.
Replace the bash script with a TypeScript version run via Bun. Much more readable: regex instead of awk, typed data structures, standard control flow. Adds setup-bun step to the workflow.
- Fix cumulative error count bug: use local `parseFailed` flag so a
parse failure in contract A doesn't skip contract B
- Use try/finally for ::endgroup:: (7 sites → 1)
- Strip both // and /* */ comments before checking for reinitializeV{N}
function (prevents false positives from commented-out signatures)
- Surface forge stderr on compilation failure instead of swallowing it
- Avoid redundant file read: extractVersions returns source alongside
versions
- Document why foundry.toml is copied to main's checkout
|
@claude /pr-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
[AUTOMATED]
Code Review
Well-designed CI addition that solves a real problem (detecting bytecode changes without version bumps). The script logic is correct, error handling is thorough, and the workflow follows good security practices (pinned actions, minimal permissions, persist-credentials: false).
Two convention issues found — neither is blocking.
Summary
- ✅ No bugs or security issues found (verified by two independent bug-hunting passes)
- ✅ Version-checking logic correctly implements the
reinitializeV{N-1}convention - ✅ Workflow properly isolates PR vs main compilation with shared
foundry.toml - ✅ Stub files provide all needed address constants for compilation
- ✅ Good use of
cbor_metadata = falseandbytecode_hash = 'none'for deterministic comparison - 📝 Two minor convention issues flagged inline
| - gateway-contracts/** | ||
|
|
||
| check: | ||
| name: contracts-upgrade-hygiene/${{ matrix.package }} |
There was a problem hiding this comment.
[AUTOMATED] Nit: Missing (bpr) suffix in job name.
21 out of 25 PR-triggered workflows in this repo use the (bpr) suffix for the main blocking job (e.g., host-contracts-upgrade-tests/sc-upgrade (bpr), gateway-contracts-integrity-checks/contract-integrity-checks (bpr)). This suffix likely identifies jobs configured as required status checks in branch protection rules.
Suggested change:
name: contracts-upgrade-hygiene/${{ matrix.package }} (bpr)Confidence: 85/100
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
[AUTOMATED] Nit: Unconditional cancel-in-progress: true.
18 out of 22 workflows in this repo use the conditional pattern:
cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}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
|
Review posted successfully to PR #2108. Here's a summary: Review ResultsVerdict: COMMENT (no blocking issues) The PR adds a well-designed CI workflow for contract upgrade hygiene checking. Two independent bug-hunting passes (opus-level) found no bugs or security issues. The version-checking logic is correct, the workflow follows good security practices (pinned actions, minimal permissions, Two non-blocking convention issues were flagged inline:
Review URL: #2108 (review) |
|
Reopening as a clean PR to clear stale zizmor findings that block mergeify. |
1 participant
Summary
ci/check-upgrade-hygiene.shMotivation
ACL.sol had logic changed (commit 803f104) without bumping the reinitializer version.
Existing CI only checks if REINITIALIZER_VERSION changed to decide whether to upgrade — it cannot detect bytecode changes without version bumps.
What it checks
Changes
ci/check-upgrade-hygiene.sh— shared validation script.github/workflows/contracts-upgrade-hygiene.yml— new CI workflowhost-contracts/foundry.toml— addcbor_metadata = false,bytecode_hash = 'none'for deterministic bytecodegateway-contracts/foundry.toml— same + OZ remappings for forge compilationTest plan