diff --git a/.github/workflows/publish-prerelease.yml b/.github/workflows/publish-prerelease.yml index f4f73dad..6e40e24e 100644 --- a/.github/workflows/publish-prerelease.yml +++ b/.github/workflows/publish-prerelease.yml @@ -186,6 +186,11 @@ jobs: find release/abis -maxdepth 1 -type f -printf '%f\n' | sort > release/expected-assets.txt echo "Extracted $(wc -l < release/expected-assets.txt) ABIs" + # Record which published ABIs each contract inherits from, read from the same build. + # The ABI diff step uses it to list a change once, under the interface that declares + # it, even when the names do not match. Kept out of release/, since it is not an asset. + bun scripts/js/release-metadata.mjs bases --out "$RUNNER_TEMP/abi-bases.json" + # No addresses: a pre-release is cut to be deployed, so the recorded addresses still # belong to the previous deployment of different code. Publishing them under this tag # would break the promise that a release's addresses and ABIs come from the same release. @@ -280,7 +285,8 @@ jobs: prev_args=(--previous prev-abis/abis --previous-tag "$PREV") fi bun scripts/js/release-metadata.mjs abidiff --current release/abis \ - "${prev_args[@]}" --json release/abi-diff.json >> release-body.md + "${prev_args[@]}" --bases "$RUNNER_TEMP/abi-bases.json" \ + --json release/abi-diff.json >> release-body.md - name: Create draft pre-release with artifacts uses: softprops/action-gh-release@v2 diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 88cbbcb4..4f73e3c2 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -182,6 +182,11 @@ jobs: find release/abis -maxdepth 1 -type f -printf '%f\n' | sort > release/expected-assets.txt echo "Extracted $(wc -l < release/expected-assets.txt) ABIs" + # Record which published ABIs each contract inherits from, read from the same build. + # The ABI diff step uses it to list a change once, under the interface that declares + # it, even when the names do not match. Kept out of release/, since it is not an asset. + bun scripts/js/release-metadata.mjs bases --out "$RUNNER_TEMP/abi-bases.json" + # Addresses come from the committed deployment manifests, the same files the # deployment pipeline in dotns-releases measures a live deploy against. A consumer # that only has the release needs them to reach any contract at all. @@ -296,7 +301,8 @@ jobs: prev_args=(--previous prev-abis/abis --previous-tag "$PREV") fi bun scripts/js/release-metadata.mjs abidiff --current release/abis \ - "${prev_args[@]}" --json release/abi-diff.json >> release-body.md + "${prev_args[@]}" --bases "$RUNNER_TEMP/abi-bases.json" \ + --json release/abi-diff.json >> release-body.md - name: Create draft release with artifacts uses: softprops/action-gh-release@v2 diff --git a/RELEASE_ARTIFACTS.md b/RELEASE_ARTIFACTS.md index 1a480c93..d46e3cf8 100644 --- a/RELEASE_ARTIFACTS.md +++ b/RELEASE_ARTIFACTS.md @@ -68,18 +68,22 @@ The release surface is decided in `.github/abi-contracts.txt` so a contract reac ```json { "version": "v1.2.3", + "hashScheme": 2, "build": { "solcVersion": "0.8.34+commit...", "optimizer": {}, "viaIr": true, "evmVersion": "cancun", "foundryLockSha256": "..." }, "hashes": { "DotnsRegistrar": "0x..." } } ``` -- `hashes` maps each deployable contract to the keccak256 of its built runtime bytecode with the trailing CBOR metadata stripped, so a comment-only edit does not read as a code change. Comparing two releases' files tells you exactly which contracts a release changed; an upgrade must cover that whole set before the release may be declared on a network (see `DEPLOYMENT_CHECKLIST.md`). +- `hashes` maps each deployable contract to the keccak256 of its built runtime bytecode with the trailing CBOR metadata stripped, so a comment-only edit does not read as a code change. A contract that deploys other contracts with `new` (such as `StoreFactory`) also contains their creation code, and each copy ends with that contract's own metadata. The hash inside those copies is set to zeros before hashing, so a comment-only edit to `LabelStore` does not make `StoreFactory` look changed either. Comparing two releases' files tells you exactly which contracts a release changed; an upgrade must cover that whole set before the release may be declared on a network (see `DEPLOYMENT_CHECKLIST.md`). +- `hashScheme` says how `hashes` were computed. `2` is the method described above. `1` skips the zeroing of embedded metadata, and files without `hashScheme` (from releases made before it was added) use it. The two only give different hashes for contracts that deploy other contracts with `new`, so only compare two files that use the same scheme. `release-metadata.mjs changedset` does this for you: it hashes the current build with the previous file's scheme. - These are artifact-side hashes, for comparing builds with builds. A deployed contract hashes differently on chain (its bytecode carries the metadata and any immutable values), so compare this file against another release's copy of it. - `build` records the toolchain inputs. The same source under a different toolchain hashes differently, and that difference is a real code change on chain, so treat the hashes as comparable only alongside their build inputs. ## `abi-diff.json` -The machine-readable form of the "ABI changes since ..." section of the release body: per contract, the functions, events, and errors added, removed, or changed since the previous release, at selector level. A **changed signature** entry is the one to alert on: the name still exists but the selector moved (a struct parameter gained a field, say), so an un-updated caller gets a bare revert with no data. Contracts new to the release or no longer published are flagged as such. When no earlier release carries ABIs to diff against, the file says so instead of guessing. +The full, machine-readable ABI diff behind the release body: per contract, the functions, events, and errors added, removed, or changed since the previous release, at selector level. A **changed signature** entry is the one to alert on: the name still exists but the selector moved (a struct parameter gained a field, say), so an un-updated caller gets a bare revert with no data. Contracts new to the release or no longer published are flagged as such. When no earlier release carries ABIs to diff against, the file says so instead of guessing. + +The release body is a short summary of this file. Under "Breaking ABI changes since ..." it lists only the changes that break an existing caller or indexer, one line each: changed function signatures, removed functions, contracts no longer published, then changed or removed events. Additions, new contracts, and custom errors are in a collapsed block with a count. A contract and the interfaces it implements usually carry the same change, so the body lists it once, under the interface that declares it, which is what callers bind to. Which published ABIs a contract inherits from is read from the build, not guessed from names, so `DotnsFlatPricing` is matched with `IDotnsPricing` too. A change that only the contract has is still listed. This file always has every contract, both the contract and its interfaces included. ## Stability diff --git a/scripts/js/release-metadata.mjs b/scripts/js/release-metadata.mjs index 5fd2975b..acaf49fb 100644 --- a/scripts/js/release-metadata.mjs +++ b/scripts/js/release-metadata.mjs @@ -5,12 +5,15 @@ // changelog --current [--previous ] release-note lines about address changes // [--previous-tag ] // changedset --previous contracts whose built code differs, one per line +// bases --out published ABIs each contract inherits from // abidiff --current [--previous ] selector-level ABI diff for the release body // [--previous-tag ] [--json ] +// [--bases ] // verify --network --rpc [--tag ] check a committed manifest against a chain // -// `build` and `changelog` run in both publish workflows; `validate` runs on pull requests, so a -// broken manifest fails there rather than at release time. `verify` stays out of the release +// `build`, `bases` and `abidiff` run in both publish workflows, and `changelog` in the release +// one. `validate` runs on pull requests, so a broken manifest fails there rather than at release +// time. `verify` stays out of the release // path, which must work without reaching a chain; with `--tag` it also checks the chain's // declared protocol version and code identity. `changedset` names exactly what a release // changed, for upgrade tooling (which lives outside this repository) and for humans. No @@ -166,15 +169,67 @@ function stripCborMetadata(name, hex) { return `0x${code.slice(0, stripped * 2)}`; } +// The metadata at the end is not the only metadata in the code. A contract that deploys other +// contracts (with `new`, for example) carries a copy of their creation code, and each copy ends +// with that contract's own metadata. StoreFactory is an example: it contains LabelStore's +// creation code, so a comment-only edit to LabelStore changes a few bytes inside StoreFactory. +// +// To ignore that, the hash (digest) inside each embedded metadata block is set to zeros. The +// code keeps its length, so nothing else moves, and the compiler version bytes are kept. A real +// change to an embedded contract's code, or to the compiler, still changes the hash. +// +// A block is found by its exact shape, not by where it sits: {"ipfs": <34-byte hash>, "solc": +// <3-byte version>} followed by its length, 0x0033. That is what solc writes with foundry's +// default `bytecode_hash = "ipfs"`. The older `bzzr1` shape is handled too. With +// `bytecode_hash = "none"` there is no hash to clear. Normal code does not contain these exact +// bytes by chance, and a match only counts if it starts on a whole byte. +const EMBEDDED_METADATA_SHAPES = [ + { prefix: "a264697066735822", digestHexLength: 68, suffix: "0033" }, + { prefix: "a265627a7a72315820", digestHexLength: 64, suffix: "0032" }, +]; + +function zeroEmbeddedMetadataDigests(hex) { + let code = hex.toLowerCase(); + for (const { prefix, digestHexLength, suffix } of EMBEDDED_METADATA_SHAPES) { + const shape = new RegExp( + `${prefix}[0-9a-f]{${digestHexLength}}64736f6c6343[0-9a-f]{6}${suffix}`, + "g", + ); + const zeros = "0".repeat(digestHexLength); + let match; + while ((match = shape.exec(code)) !== null) { + // `code` starts with "0x", so every byte starts at an even index. A match at an odd index + // starts in the middle of a byte, so it is not a real block. Keep searching from the next + // character. + if (match.index % 2 !== 0) { + shape.lastIndex = match.index + 1; + continue; + } + const digestStart = match.index + prefix.length; + code = code.slice(0, digestStart) + zeros + code.slice(digestStart + digestHexLength); + } + } + return code; +} + // keccak256 via `cast keccak`, keeping the script dependency-free like the chain reads. function keccakHex(hex) { return cast(["keccak", hex]); } +// Saved in codehashes.json as `hashScheme`, so that `changedset` can hash the current build the +// same way the previous file was hashed. Comparing hashes made in two different ways would show +// changes that are not there. +// 1: the metadata at the end is removed. Files written before `hashScheme` existed use this. +// 2: the same, and the hashes inside embedded metadata are set to zeros as well. +// Both give the same result for a contract that does not deploy other contracts. +const HASH_SCHEME = 2; +const HASH_SCHEMES = [1, 2]; + // Stripped-metadata hash of each deployable contract's built runtime bytecode. These are // artifact-side hashes for comparing builds with builds (the changed-set); they are never // compared against on-chain hashes, which live in a different domain (see `verify`). -function builtCodehashes() { +function builtCodehashes(scheme = HASH_SCHEME) { const { contracts } = classifyContracts(readContractNames()); const hashes = {}; for (const name of contracts) { @@ -183,7 +238,8 @@ function builtCodehashes() { ); const runtime = artefact?.deployedBytecode?.object; if (!runtime || runtime === "0x") fail(`${name}: no deployed bytecode in the artefact`); - hashes[name] = keccakHex(stripCborMetadata(name, runtime)); + const stripped = stripCborMetadata(name, runtime); + hashes[name] = keccakHex(scheme >= 2 ? zeroEmbeddedMetadataDigests(stripped) : stripped); } return hashes; } @@ -216,7 +272,23 @@ function changedset(args) { const previous = JSON.parse(readFileSync(resolve(process.cwd(), args.previous), "utf8")); const previousHashes = previous?.hashes; if (!previousHashes) fail(`${args.previous} has no 'hashes' map`); - const current = builtCodehashes(); + // Hash the build the same way the previous file was hashed. Otherwise a change in how the + // hash is computed would look like a change in the code. + const scheme = previous.hashScheme ?? 1; + if (!HASH_SCHEMES.includes(scheme)) { + fail( + `${args.previous} uses hashScheme ${scheme}; this script knows ${HASH_SCHEMES.join(", ")}`, + ); + } + if (scheme < HASH_SCHEME) { + // Printed to stderr, because stdout is the list that upgrade tooling reads. + console.error( + `[release-metadata] ${args.previous} uses hashScheme ${scheme}, so this build is hashed ` + + `the same way. A contract that deploys other contracts with \`new\` is listed if any ` + + `contract it deploys changed at all, even if only a comment changed.`, + ); + } + const current = builtCodehashes(scheme); // Union, not just the current set: a contract only in the previous release was removed and a // contract only in this one is new. Neither is coverable by an in-place upgrade, so both must // surface and force the coverage gate to refuse rather than dropping out of the diff. @@ -315,66 +387,241 @@ function diffAbi(previous, current) { return result; } -// Markdown fragment for the release body plus a machine-readable JSON asset. Prints "no -// changes" rather than nothing, so absence is a statement and not a gap; a missing previous -// release degrades the same way rather than failing the release. +// For each published ABI, the other published ABIs it inherits from, nearest first. This comes +// from the build itself, not from names, so a contract is matched with its interface whatever +// they are called. For example, DotnsFlatPricing implements IDotnsPricing. Bases that are not +// published ABIs are skipped, because the release body can only point at ABIs that ship in the +// release. +// +// A contract's artifact lists its bases as AST ids. Ids only mean something inside the compiler +// run that made them, and an incremental `forge build` can leave artifacts from several runs in +// out/. So each artifact is matched to the build info of its own run (foundry.toml keeps +// `build_info = true`), and its base ids are turned into names there. +function publishedBases() { + const buildInfoDir = join(ROOT, "out", "build-info"); + if (!existsSync(buildInfoDir)) { + fail(`${buildInfoDir} not found; foundry.toml needs \`build_info = true\`, then forge build`); + } + // For each compiler run: contract id -> { name, path }. + const runs = readdirSync(buildInfoDir) + .filter((file) => file.endsWith(".json")) + .map((file) => { + const info = JSON.parse(readFileSync(join(buildInfoDir, file), "utf8")); + const byId = new Map(); + for (const [path, source] of Object.entries(info?.output?.sources ?? {})) { + for (const node of source?.ast?.nodes ?? []) { + if (node.nodeType === "ContractDefinition") byId.set(node.id, { name: node.name, path }); + } + } + return byId; + }); + + const published = readContractNames(); + const publishedSet = new Set(published); + const bases = {}; + for (const name of published) { + const path = join(ROOT, "out", `${name}.sol`, `${name}.json`); + if (!existsSync(path)) { + fail(`${path} not found; run forge build, or check .github/abi-contracts.txt`); + } + const artefact = JSON.parse(readFileSync(path, "utf8")); + const def = artefact?.ast?.nodes?.find( + (node) => node.nodeType === "ContractDefinition" && node.name === name, + ); + if (!def) fail(`${path} has no AST for ${name}; foundry.toml needs \`ast = true\``); + // The run that built this artifact is the one that has this contract, in this file, under + // the same id. + const run = runs.find((byId) => { + const known = byId.get(def.id); + return known?.name === name && known.path === artefact.ast.absolutePath; + }); + if (!run) { + fail(`no build info in out/build-info matches ${name}; run forge clean && forge build`); + } + // The first id is the contract itself. + const found = (def.linearizedBaseContracts ?? []) + .slice(1) + .map((id) => run.get(id)?.name) + .filter((base) => base && publishedSet.has(base)); + if (found.length > 0) bases[name] = found; + } + return bases; +} + +function writeBases(args) { + if (!args.out) fail("bases needs --out "); + const bases = publishedBases(); + writeJson(resolve(process.cwd(), args.out), bases); + log(`${Object.keys(bases).length} published ABI(s) inherit from another published ABI`); +} + +function readBases(path) { + const bases = JSON.parse(readFileSync(resolve(process.cwd(), path), "utf8")); + const valid = + bases && + typeof bases === "object" && + !Array.isArray(bases) && + Object.values(bases).every((list) => Array.isArray(list)); + if (!valid) { + fail(`${path} does not map contract names to lists of bases; write it with \`bases\``); + } + return bases; +} + +// Order of the lines in the release body. Breaking changes are the ones that stop an existing +// caller or indexer from working. The rest are grouped after them. +const BREAKING_GROUPS = [ + "functionChanged", + "functionRemoved", + "contractRemoved", + "eventChanged", + "eventRemoved", +]; +const OTHER_GROUPS = [ + "contractAdded", + "functionAdded", + "eventAdded", + "errorChanged", + "errorAdded", + "errorRemoved", +]; + +// Writes the ABI part of the release body, plus the full diff as JSON. +// +// The body is kept short so that people read it. It lists only the breaking changes, one line +// each: changed function signatures, removed functions, contracts no longer published, then +// changed or removed events. Everything else (additions, new contracts, custom errors) goes in a +// collapsed block with a count. The JSON always has the full diff for every contract. +// +// A contract and the interfaces it implements usually publish the same functions and events, so +// one change would be listed several times. `--bases` (written by `bases`) says which published +// ABIs each contract inherits from. A contract's line is left out when one of those bases has +// exactly the same line, so the change is listed once, under the interface that declares it, +// which is what callers bind to. A change that only the contract has is still listed. Without +// `--bases`, every contract is listed on its own. +// +// When there is nothing to report, it says so, so an empty section is never mistaken for a +// missing one. A previous release without ABIs is reported the same way instead of failing. function abidiff(args) { if (!args.current) fail("abidiff needs --current "); const previousTag = args["previous-tag"] ?? "the previous release"; - const lines = []; const report = { previousTag: args["previous-tag"] ?? null, contracts: {} }; + const lines = [""]; if (!args.previous) { - lines.push("", "No earlier release carries ABIs to diff against."); + lines.push("No earlier release carries ABIs to diff against."); report.previousUnavailable = true; } else { const currentAbis = readAbiDir(resolve(process.cwd(), args.current)); const previousAbis = readAbiDir(resolve(process.cwd(), args.previous)); - const changedLines = []; - const otherLines = []; - for (const [name, abi] of currentAbis) { + const code = (text) => `\`${text}\``; + const signatures = (list) => list.map(code).join(", "); + const member = (signature) => signature.slice(0, signature.indexOf("(")); + + // One entry per change. `change` describes the change without naming the contract, so a + // contract's entry can be matched against its interface's. + const entries = []; + const add = (contract, group, change, text) => entries.push({ contract, group, change, text }); + + for (const name of [...currentAbis.keys()].sort()) { const previousAbi = previousAbis.get(name); if (!previousAbi) { - otherLines.push(`- \`${name}\`: new contract`); + add(name, "contractAdded", "added", "new contract"); report.contracts[name] = { newContract: true }; continue; } - const diff = diffAbi(previousAbi, abi); + const diff = diffAbi(previousAbi, currentAbis.get(name)); if (Object.keys(diff).length === 0) continue; report.contracts[name] = diff; for (const [kind, { changed, added, removed }] of Object.entries(diff)) { + // Functions need no label. Events and errors say what they are. + const label = kind === "function" ? "" : ` ${kind}`; for (const entry of changed) { - changedLines.push( - `- \`${name}\`: ${kind} \`${entry.name}\` changed signature: ` + - `${entry.was.map((s) => `\`${s}\``).join(", ") || "(none)"} is now ` + - `${entry.now.map((s) => `\`${s}\``).join(", ") || "(none)"}`, + add( + name, + `${kind}Changed`, + `${kind} ${entry.was.join(" ")} > ${entry.now.join(" ")}`, + `${code(`${name}.${entry.name}`)}${label}: ` + + `${signatures(entry.was)} → ${signatures(entry.now)}`, ); } - for (const signature of added) otherLines.push(`- \`${name}\`: ${kind} \`${signature}\` added`); for (const signature of removed) { - otherLines.push(`- \`${name}\`: ${kind} \`${signature}\` removed`); + add( + name, + `${kind}Removed`, + `${kind} removed ${signature}`, + `${code(`${name}.${member(signature)}`)}${label}: ${code(signature)} removed`, + ); + } + for (const signature of added) { + add( + name, + `${kind}Added`, + `${kind} added ${signature}`, + `${code(`${name}.${member(signature)}`)}${label}: ${code(signature)} added`, + ); } } } - for (const name of previousAbis.keys()) { + for (const name of [...previousAbis.keys()].sort()) { if (!currentAbis.has(name)) { - otherLines.push(`- \`${name}\`: no longer published`); + add(name, "contractRemoved", "removed", "no longer published"); report.contracts[name] = { removedContract: true }; } } - lines.push("", `## ABI changes since ${previousTag}`, ""); - if (changedLines.length + otherLines.length === 0) { - lines.push(`No ABI changes since ${previousTag}.`); + // Leave out a contract's line when one of its bases has the same one. A contract that is new + // together with its bases is named on its base's line instead. That is the furthest base + // that is also new, which has no new base of its own, so the whole family ends up on one + // line. A removed contract is not in the current build, so its bases are not known and it + // keeps its own line. + const bases = args.bases ? readBases(args.bases) : {}; + const byChange = new Map(entries.map((entry) => [`${entry.contract} ${entry.change}`, entry])); + const isContractLine = (entry) => + entry.group === "contractAdded" || entry.group === "contractRemoved"; + const shown = entries.filter((entry) => { + const covering = (bases[entry.contract] ?? []).filter((base) => + byChange.has(`${base} ${entry.change}`), + ); + if (covering.length === 0) return true; + if (isContractLine(entry)) { + const home = byChange.get(`${covering.at(-1)} ${entry.change}`); + home.names = [...(home.names ?? [home.contract]), entry.contract]; + } + return false; + }); + const nameList = (names) => { + const quoted = names.map(code); + return quoted.length > 1 + ? `${quoted.slice(0, -1).join(", ")} and ${quoted.at(-1)}` + : quoted[0]; + }; + const line = (entry) => + isContractLine(entry) + ? `- ${nameList(entry.names ?? [entry.contract])}: ` + + (entry.names && entry.group === "contractAdded" ? "new contracts" : entry.text) + : `- ${entry.text}`; + const inOrder = (groups) => + groups.flatMap((group) => shown.filter((entry) => entry.group === group)).map(line); + const breaking = inOrder(BREAKING_GROUPS); + const others = inOrder(OTHER_GROUPS); + + if (breaking.length + others.length === 0) { + lines.push(`## ABI changes since ${previousTag}`, "", `No ABI changes since ${previousTag}.`); } else { - if (changedLines.length > 0) { + lines.push(`## Breaking ABI changes since ${previousTag}`, ""); + lines.push(...(breaking.length > 0 ? breaking : ["None."])); + if (others.length > 0) { lines.push( - "**Changed signatures.** Existing callers of these get a bare revert until updated:", - ...changedLines, "", + "
", + `Other ABI changes (${others.length})`, + "", + ...others, + "", + "
", ); } - lines.push(...otherLines); } } @@ -413,6 +660,7 @@ function build(args) { // pre-release tags, and an upgrade diffs its build against the previous release's file. writeJson(join(outDir, "codehashes.json"), { version: tag, + hashScheme: HASH_SCHEME, build: buildInputs(), hashes: builtCodehashes(), }); @@ -712,6 +960,7 @@ if (mode === "build") build(args); else if (mode === "validate") validate(); else if (mode === "changelog") changelog(args); else if (mode === "changedset") changedset(args); +else if (mode === "bases") writeBases(args); else if (mode === "abidiff") abidiff(args); else if (mode === "verify") verify(args); else { @@ -719,7 +968,9 @@ else { "usage: release-metadata.mjs build --tag [--out ] | validate | " + "changelog --current [--previous ] [--previous-tag ] | " + "changedset --previous | " + - "abidiff --current [--previous ] [--previous-tag ] [--json ] | " + + "bases --out | " + + "abidiff --current [--previous ] [--previous-tag ] [--json ] " + + "[--bases ] | " + "verify --network --rpc [--tag ]", ); }