ci(contracts): gate the published ABI and run the storage-layout check

[penumbra23]

Summary

Adds an ABI compatibility gate alongside the existing storage-layout one, and wires both into CI. Day to day this means two new make targets next to the storage pair you already have:

Check	Snapshot	Verify	Refresh
Published ABI	.abi/	make check-abi	make update-abi
Storage layout	.storage-layout	make check-storage-layout	make update-storage-layout

CI runs both on every pull request, plus a third step that regenerates .abi/ and fails if the committed snapshot is stale. Every fingerprint change has to be committed, additive ones included.

The ABI snapshot covers the seven contracts shell/create_artifacts.sh publishes to npm, plus AdminVerifierWhitelist — credible-layer-dapp/scripts/seed-onchain-test-data.sh calls addToWhitelist(address,address) as a literal string through cast send, so a signature change there breaks a cross-repo caller with no compile-time error. One snapshot file per contract, so a diff is attributable and a scope change is visible as a file add or remove.

The storage check was worse than unwired

grep -rn "storage-layout\|check_storage" .github/ returned nothing — it had never run. But it also could not have passed. On a clean rebuild of unmodified main it exits 2 with a fake collision:

CRITICAL: Variable 'adminVerifiers' type changed from
  t_mapping(t_contract(IAdminVerifier)2524,t_bool) to
  t_mapping(t_contract(IAdminVerifier)2554,t_bool)!
STORAGE LAYOUT COLLISION DETECTED

Those trailing digits are solc AST node ids. They shift whenever anything earlier in the compilation unit changes — adding a single storage-neutral function is enough. .storage-layout and src/ were last touched by the same commit, so this was never staleness. Turning the check on as written would have failed essentially every PR that touched src/, which is worse than no gate at all.

Repaired without changing what it means:

• AST ids normalized out of contract, struct, enum and UDVT type identifiers. Array lengths and integer widths are deliberately left intact — t_array(t_uint256)5_storage vs 4_storage, dyn, and t_uint256 vs t_uint128 all still compare.
• Entries sorted by slot, so output reordering is not read as a layout change.
• The types table is compared, not just .storage. A struct behind a mapping keeps its top-level slot, offset and type id while its members move underneath it — reordering AssertionAdopter relocates every deployed adopter record, and comparing .storage alone reports "unchanged".
• Tool failure now exits 3 instead of being indistinguishable from a benign change.

The committed .storage-layout needed no edit; the fix is entirely in the comparison.

What the ABI gate catches

Compared on selector and topic0 identity, never on document text.

Breaking (exit 2): function or event removed; parameter retyped or reordered; function renamed; state mutability tightened; return type or arity changed; event indexed layout changed; custom error removed.

Additive (exit 1, passes CI with a warning): new function, new event, mutability relaxed.

Two of these need explaining:

• indexed gets its own bitmask. topic0 is keccak of the canonical signature and does not encode indexedness, so flipping a field between topic and data leaves topic0 byte-identical. A topic0-only diff misses it entirely, while every historical log decoder breaks. Verified on a zero-input event too, where a field-splitting bug had previously been eating the flag.
• Return types are fingerprinted separately. The selector covers only inputs, so returns (uint256) → returns (bytes32) keeps the same key while breaking every eth_call decoder.

Known limitation

Swapping two parameters of the same type — the two address arguments of addToWhitelist(address,address) — leaves the canonical signature and therefore the selector untouched. No selector-based check can detect it. Documented in the README and in the script; argument order of same-typed parameters still needs human review.

Test Plan

Every probe applied to a real Solidity source edit, then reverted; the tree is byte-clean afterwards.

ABI gate

Probe	Exit
unchanged	0
remove function	2
change parameter type	2
reorder parameters	2
rename function	2
tighten mutability (incl. zero-arg)	2
change return type / arity	2
flip event indexed	2
break AdminVerifierWhitelist	2
add function / add event	1 (CI passes with warning)

Storage gate

Probe	Exit
unchanged, with full AST churn	0
additive storage-neutral function	0
benign appended top-level variable	1
safe struct member append	1
reorder top-level variables	2
uint16 → uint32	2
reorder struct members behind a mapping	2
broken compile	3

Failure injection — 7 malformed .abi/ states (empty, {}, merge-conflict markers, truncated, missing section, wrong root type, unreadable) and 8 malformed .storage-layout states all exit 3 rather than reporting a false pass. A failing cast keccak exits 3, emits zero bogus findings, and writes no snapshot.

Other — .abi/ byte-identical across three regenerations and from a fresh clone with no out/; forge build and 275 tests pass; shellcheck -x clean on both scripts; actionlint clean; clean-room simulation of all three CI steps green.

Pre-existing forge fmt differences in test/ and script/ are untouched — no file in this diff is affected.

Related

• ENG-4333 — https://linear.app/phylaxsystems/issue/ENG-4333
• Follow-up from the ENG-3801 contract-break coverage audit, which found no maintained third-party Solidity ABI differ and recommended the snapshot-plus-script shape this repo already used for storage layout.

Note: this repository has no branch protection, so both checks are advisory until required checks are configured.

🤖 Generated with Claude Code

[linear-code]

ENG-4333

[odyslam]

@penumbra23 the abi should be an artifact of the release, not committed in the repository.

If anything, the check should just check that the semver is updated correctly if the ABI changes. If it's a new thing, it should be a minor. If it breaks the old ABI, it should be a major.

External services (e.g. the dapp) should not depend on the commit hash, but instead on released artifacts, which is the github release and the npm package.

[penumbra23]

@odyslam you're right, and the committed snapshot is gone as of 9985381.

Concretely why: artifacts/ is gitignored (.gitignore:18), generated by shell/create_artifacts.sh, and release.yml publishes it to npm and a GitHub release on every *.*.* tag. So the ABI already is a release artifact, and .abi/ was a second, differently shaped copy of it committed into a repo that deliberately excludes generated ABI. AGENTS.md also lists artifacts/ under files not to hand-edit.

What changed. The gate no longer stores anything. It checks the base revision out into a temporary worktree, builds it, fingerprints it, and compares that against the working tree — both sides computed, nothing committed. The base is resolved the way our proto gate in credible-sdk does it: the PR base sha, or the commit before a push, and never origin/main on a push, since on main that's the commit under test and the comparison would be vacuous. If the base doesn't resolve, or resolves to something that leaves nothing to compare, it exits non-zero rather than reporting a green check having verified nothing. Adds ~3s: the repo builds cold in about 3.2s and the temp worktree is torn down unconditionally.

The detection logic is unchanged — selectors, event topic0s, the indexed bitmask, return types, error selectors.

On the semver check. I agree that's the right end state, and it subsumes what this does: ABI changed additively → minor, breaks the old ABI → major, enforced against the last released artifact rather than a git ref. I've deliberately not built it here, for two reasons:

1. It needs a policy decision first. We're at 0.2.0, and under semver 0.x a breaking change is conventionally a minor bump, not 1.0.0. "Breaking means major" only becomes unambiguous at 1.0.0, so someone has to decide whether a break now means 0.3.0 or whether we cut 1.0.0.
2. It should land at the same time as the equivalent gates in credible-sdk, so the rule is the same everywhere rather than diverging per repo.

Comparing against the base branch is the useful subset that doesn't need either decision: it catches the break at review time, on the PR that introduces it. The released-artifact-plus-semver version can replace it once the policy call is made — happy to take that as a follow-up issue and put it behind whoever owns the versioning decision.

Worth flagging separately: AdminVerifierWhitelist is in the gate's scope but is not in create_artifacts.sh, so its ABI is never published at all — yet credible-layer-dapp/scripts/seed-onchain-test-data.sh calls addToWhitelist(address,address) as a literal string through cast send. That's a real gap in the published surface rather than a gate problem, and it's the kind of thing the semver check wouldn't catch either since there's no artifact to compare. Want me to add it to create_artifacts.sh?

Storage layout is unaffected by all of this and keeps its committed .storage-layout. It isn't guarding a published package — it guards proxy upgrade safety against state that's already deployed on chain, so the baseline has to be a durable record of what the live proxies are using, not a release artifact. Separately, that check had never actually run in CI, and couldn't have passed if it had: its snapshot embeds solc AST node ids that shift whenever any storage-neutral function is added, so on a clean rebuild of unmodified main it reported a fake collision. That's fixed in this PR too.