ci: enforce additive-only storage layout changes#450
Conversation
Chaitu-Tatipamula
force-pushed
the
ci/enforce-additive-storage-layout
branch
from
8e3271e to
b8b07cd
Compare
There was a problem hiding this comment.
Pull request overview
Adds a CI-enforced guardrail to prevent destructive storage layout edits for the upgradeable FilecoinWarmStorageService proxy by verifying that FilecoinWarmStorageServiceLayout.sol changes are append-only.
Changes:
- Introduces
tools/check_storage_layout.shto compare storage slot mappings and reject destructive diffs. - Wires the layout check into CI (
check-genjob) and exposes it locally viamake check-layout. - Updates
SPEC.mdto document the automated and manual release verification process.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
service_contracts/tools/check_storage_layout.sh |
New bash script to compare base vs new layout slots and fail on destructive changes. |
service_contracts/Makefile |
Adds check-layout target and updates help output. |
SPEC.md |
Documents the new storage layout verification expectations/process. |
.github/workflows/check.yml |
Fetches base branch on PRs and runs the new storage layout check in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check 1: No existing slots removed or modified | ||
| while IFS=' ' read -r name num; do | ||
| if ! grep -q "^${name} ${num}$" "$new_slots_file"; then | ||
| if grep -q "^${name} " "$new_slots_file"; then | ||
| local new_num=$(grep "^${name} " "$new_slots_file" | awk '{print $2}') |
There was a problem hiding this comment.
The comparison logic treats the constant/variable name as the identity (NAME SLOTNUM). This will flag a pure rename (same slot number, different label) as “removed” + “inserted”, even though storage compatibility is unchanged. If renames should be allowed, compare by slot number (or by slot+offset+type from forge inspect) rather than by name; if renames should be forbidden, the script/spec should explicitly document that constraint because it’s stricter than “slot numbers only change by appending”.
There was a problem hiding this comment.
I am ok to handle the rename case if / when it actually comes up, therefore I think it's ok to ignore this comment.
| # Check 1: No existing slots removed or modified | ||
| while IFS=' ' read -r name num; do | ||
| if ! grep -q "^${name} ${num}$" "$new_slots_file"; then | ||
| if grep -q "^${name} " "$new_slots_file"; then | ||
| local new_num=$(grep "^${name} " "$new_slots_file" | awk '{print $2}') |
There was a problem hiding this comment.
I am ok to handle the rename case if / when it actually comes up, therefore I think it's ok to ignore this comment.
There was a problem hiding this comment.
I don't love these huge bash scripts. It might just be me, but code reviewing more intricate bash is hard. I would be more comfortable with it if we had tests in place so I could reason about it with the tests.
Anyways, that is a bigger problem with this repo's "tools", so we can ignore this comment.
- Use portable shebang (#!/usr/bin/env bash) for environment compatibility - Enable set -euo pipefail for stricter error handling (unset vars, pipe failures) - Add trap cleanup for temp files to ensure cleanup on any exit path - Validate both layouts before comparison in CI path to catch extraction failures - Simplify inline comments for consistency with codebase style - Remove verbose Storage Layout Verification Process section from SPEC.md
|
One gap in this check is that it only compares
Because that generated file does not include type or offset metadata, a destructive storage change that keeps the same slot number, such as changing a field width/type or repacking values within the same slot, would still pass this check even though the proxy layout is no longer compatible. If we want this to be a stronger upgrade-safety guard, we likely need to diff the full |
| # Storage layout JSON (full metadata for upgrade safety checks) | ||
| $(LAYOUT_JSON): src/FilecoinWarmStorageService.sol | ||
| forge inspect --json $< storageLayout | jq '[.storage[] | {label, slot, offset, type}]' > $@ | ||
|
|
There was a problem hiding this comment.
The new JSON snapshot is a nice improvement, but I think storing raw .storage[].type here may cause false positives. Forge includes compiler-generated IDs in user-defined types (for example t_struct(Foo)6_storage), and those IDs can change after harmless source edits even when the storage layout is unchanged. That would make the CI check fail on a spurious “type changed” diff. For a follow-up, it may be safer to canonicalize the type metadata or resolve it through .types before comparing.
There was a problem hiding this comment.
oh yes thanks for pointing this, i'll update the MakeFile to strip out those IDs before it writes to the JSON snapshot so it just leaves the pure type behind
There was a problem hiding this comment.
Hmmm... I now think we introduced a more important false negative. For example, if struct PlannedUpgradeor struct DataSetInfo changes internally, the stored type string can remain struct FilecoinWarmStorageService.PlannedUpgrade / mapping(uint256 => struct FilecoinWarmStorageService.DataSetInfo), so the JSON does not change and the check still passes.
Instead of snapshotting only the top-level type label, recursively resolve the referenced .types entries and compare a normalized nested representation of the full type shape. If that is too much for this PR, we can at least narrow the docs/claims so we don’t say this catches all type changes yet.
There was a problem hiding this comment.
I pushed a small follow-up commit to close the remaining storage-layout gap above^^.
The previous canonicalization fixed the compiler-generated type ID false positive, but it only preserved the top-level type label. That meant a nested struct change could still pass if the outer storage entry stayed as something like struct PlannedUpgrade or mapping(uint256 => struct DataSetInfo).
The new commit keeps the stable top-level type string, but also adds a recursive typeDetails snapshot generated from Forge’s .types dictionary. The checker now compares typeDetails when available, so nested struct/member layout changes are caught while harmless compiler-ID churn still does not fail CI.
Chaitu-Tatipamula
force-pushed
the
ci/enforce-additive-storage-layout
branch
from
e09ed4c to
d308cd9
Compare
…o prevent false positives
github-project-automation
bot
moved this from 🔎 Awaiting review
to ✔️ Approved by reviewer
in FOC
Resolves #446
Description
Introduces a robust CI check to guarantee that updates to the UUPS proxy implement additive-only changes to FilecoinWarmStorageServiceLayout.sol. This strictly enforces that existing storage slots cannot be removed, reordered, or modified, effectively preventing storage collision bugs upon contract upgrades.
Changes
origin/$GITHUB_BASE_REF(orHEAD~1) to automatically detect dropped slots, shifted numbers, or illegal mid-contract insertions.make check-layoutto facilitate local testing prior to commit.