docs(release-process): fix wrong monorepo paths, invalid bash, and stale release-script flags - #2295
Conversation
|
Most of this is right, and more of it is right than the body claims. You wrote that you could not verify the flag and filename semantics against the scripts, so I did: they are correct, including one subtlety I doubt was luck. What stops me approving is the layer underneath. The commands those flags hang off were deleted from celo-monorepo, so the page still cannot be followed end to end. The blocker: four dead script names
129 is the one that hides. The package.json key still exists, so a grep finds it, but For 180, yarn release:verify-deployed:foundry -p "upgrade_proposal.json" -b $RELEASE_CANDIDATE -n $NETWORK -f -i initialization_data.jsonRead that file before the next push. Its opening table is this exact mapping, and it would have caught all four. The page probably ought to link it, since it is the release owners' own doc. Stale prose mentions of the old names sit on 87, 98, 132, 170 and 185. What's right
The fix here is four script renames and a tag. No command needs reworking. Smaller thingsLine 56 still carries the wrong tag. You corrected the file path in that sentence and left On line 136, "(Use Line 156 is the one block that never got a preamble, and this PR is what broke it. Main had the literal Lines 144 and 145: Line 171 says "the proposal.json you fetched in the step prior", naming a file that now appears nowhere else on the page. The artifact fetched in that step is Line 69, the prerequisites callout, still says One nit: only the first block says The bodySix statements do not hold, and reviewers approve on the strength of the body:
Happy to push the renames myself if that is easier than another round. |
…ale release-script flags Supersedes #2139, which was opened in February and has since diverged from main: merging it as-is would revert four fixes main has picked up (the `description` frontmatter, the pinned celo-monorepo setup link, the OpenZeppelin proxies link, and the anvil network option). This applies only the parts of #2139 that main still needs, on top of current main. Verified against celo-org/celo-monorepo: - `packages/what-is-celo/about-celo-l1/protocol/releaseData/` 404s — a bad find-and-replace from an old docs migration. The real path is `packages/protocol/releaseData/initializationData/`, which exists. - `.circleci/` 404s; the repo moved to GitHub Actions. `RELEASE_TAG` is defined at `.github/workflows/celo-monorepo.yml:32`. Bash that could not run: - `${N-1}` is parameter expansion with a default, not arithmetic — it evaluates to "N" whenever N is unset, so `PREVIOUS_RELEASE` came out as `core-contracts.vN`. Now `$((N-1))`. - `NETWORK=${"anvil"|"celo-sepolia"|"mainnet"}` is a syntax error. Now a real assignment with the options in a comment. - Six blocks used `$N`, `$PREVIOUS_RELEASE`, `$RELEASE_CANDIDATE` or `$PRIVATE_KEY` without defining them. Each block now stands alone (AGENTS.md section 5). The examples use N=18, the next release after the `core-contracts.v17` currently pinned in the workflow. Release script flags and outputs brought in line with the current implementation: `-r`/`-p` no longer name files that the scripts write themselves, `-k` is required by `release:make`, and the generated filenames (`report-*`, `*-libraries.json`, `proposal-*`) are stated where they are produced and used. Also drops the `export const N = "N"` Docusaurus leftover and three typos (Vaildator, valildator, EXCECUTED). Verified: `mintlify broken-links` reports no broken links. Co-Authored-By: martinvol <martinvol@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a6c8ead to
06307b2
Compare
The four Truffle release scripts this page invoked were deleted from
celo-monorepo, so the process could not be followed end to end:
release:verify-deployed -> release:verify-deployed:foundry
release:make -> release:make:foundry
release:check-versions -> release:check-versions:foundry
release:verify-release -> deleted; verify-deployed:foundry takes -p
check-versions is the one that hides: the package.json key still exists, so
grep finds it, but check-versions.sh calls build_tag and release-lib.sh now
defines only build_tag_foundry, so it dies under set -euo pipefail before
compiling anything.
Also:
- correct the pre-audit tag to the core-contracts.v${N} family
- drop "(Use -d to dry-run)": the Foundry script has no -d; -u takes an RPC
URL, so a local anvil fork is the way to rehearse
- quote $PRIVATE_KEY and give it a visible placeholder — empty and unquoted,
it word-split to a bare trailing -k and getopts bailed before the script's
own "Need to set the private key" message could print
- define N, NETWORK and RELEASE_CANDIDATE in the celocli propose block, which
used two of them undefined and resolved to proposal--.json
- name upgrade_proposal.json in the verify step instead of a proposal.json
that appears nowhere else on the page
- add forge, cast and jq to the prerequisites, and the packages/protocol
working-directory comment to every block that needs it
- link RELEASE_PROCESS_FOUNDRY.md, which maps each retired command
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All of it addressed, and I verified your findings independently before acting on them rather than taking them on trust. The blocker — four dead script names. Confirmed against Your The key surviving in package.json is what makes it dangerous — a grep confirms the command and it still dies before compiling. Smaller ones, all taken: pre-audit tag corrected to the The body. All six were wrong and I have rewritten it, keeping the corrections visible rather than quietly deleting them. Two worth calling out: the Every command and flag now checked against upstream: One thing I left out, deliberately. You noted a mainnet deployer key in argv deserves a warning. I quoted The deeper problem stands: this page hand-copies command names from another repo with nothing checking them, which is how four names went stale unnoticed. Linking |
palango
left a comment
There was a problem hiding this comment.
Blocker closed. Re-checked against celo-monorepo master rather than against your summary: packages/protocol/package.json defines exactly six release:* scripts, all four names on the page resolve to one of them, and grepping the head for a non-:foundry variant returns nothing.
The check-versions subtlety holds. release-lib.sh:29 defines build_tag_foundry and nothing else, and check-versions.sh:45 and :47 call bare build_tag. RELEASE_PROCESS_FOUNDRY.md returns 200 and its opening table maps release:verify-release to verify-deployed with a proposal, which is what the page now does. Flags match all three getopts strings: 'a:b:r:l:i', 'b:n:fl:i:p:', 'b:k:i:l:n:p:r:u:sa:'.
The -u pointer is better than the sentence it replaced. make-release-foundry.sh:14 documents it as "Custom RPC URL (optional, overrides network default)", assigned at :38 and threaded through :67 and :83-84, so an anvil fork genuinely rehearses the deploy.
The rest verified: pre-audit tag against RELEASE_TAG: core-contracts.v17 at celo-monorepo.yml:32, PRIVATE_KEY quoted with a visible placeholder, the three variables in the propose block, upgrade_proposal.json in the verify step, forge/cast/jq in the prerequisites, and the run-from comment on all six packages/protocol blocks and correctly absent from the two celocli ones. On the body I spot-checked the correction that could have done damage: the version-less OpenZeppelin URL returns 200 and /upgrades-plugins/1.x/proxies 404s, so it now reads the right way round.
On your question, add the sentence. -k "$PRIVATE_KEY" puts a mainnet deployer key in a command line that is world-readable through /proc on Linux and lands in shell history. That it is the upstream script's interface is the reason to warn, not the reason to stay quiet: this page is where the reader meets that interface, and it is this page telling them to run it against mainnet.
Two residuals, neither blocking. Fold them in with that sentence or leave them for the next pass:
$YOUR_ADDRESSon line 162 is still undefined. It predates this PR, but correction 5 says the propose block now stands alone, and with one undefined variable it does not quite.- The
tags:viewblock carries# Run from packages/protocol. That one isgit for-each-refand works anywhere in the repo; it is the only block where the comment does no work.
Your point about the deeper problem stands. Hand-copied command names from another repo with nothing checking them is how four went stale, and a link is an anchor rather than a test. Worth its own issue.
…UR_ADDRESS, drop a stray run-from comment Three follow-ups from review: - Add a Warning after the release:make:foundry block. The script's -k flag puts the deployer key on the command line, which is readable by other processes (/proc on Linux) and lands in shell history. This page is where a release engineer meets that interface and it tells them to run it against mainnet, so the page carries the warning. - Define YOUR_ADDRESS in the celocli governance:propose block so it stands alone like the others. - Drop the 'Run from packages/protocol' comment on the tags:view block. That one is git for-each-ref and works from anywhere in the repo; it was the only block where the comment did no work. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Added the sentence, and folded in both residuals, in
The hand-copied script names problem is filed as #2310, with your two remedies (a script check against |
The hole, and the fix
/contribute-to-celo/release-process/smart-contractscould not be followed end to end. Two layers were broken.The commands did not exist. The four Truffle release scripts this page invoked were deleted from celo-monorepo.
packages/protocol/package.jsonon master defines only the:foundryvariants:yarn release:verify-deployedrelease:verify-deployed:foundryyarn release:makerelease:make:foundryyarn release:check-versionsrelease:check-versions:foundryyarn release:verify-release:foundrysibling —verify-deployed:foundrytakes-pcheck-versionsis the one that hides. The package.json key still exists, so a grep finds it, butcheck-versions.shcallsbuild_tagandrelease-lib.shnow defines onlybuild_tag_foundry. Underset -euo pipefailit dies before compiling anything.The flags and filenames hanging off them were also wrong, which is what the first commit fixed: the
packages/what-is-celo/...path that 404s,NETWORK=${"anvil"|...}which was a shell syntax error, the-p/-rflags the Foundry scripts hard-error on, the mandatory-k, and the-llibraries filename that only validates against the previous release.Also in this pass:
core-contracts.v${N}.pre-auditfamily. Thecelo-core-contracts-v${N}family stops at v7 upstream.-dto dry-run the deploy)".-dbelonged to the deleted Truffle script;getopts 'b:k:i:l:n:p:r:u:sa:'has nod.-utakes an RPC URL, so a local anvil fork is the way to rehearse.PRIVATE_KEYquoted and given a visible placeholder. Empty and unquoted,-k $PRIVATE_KEYword-split to a bare trailing-kand getopts bailed before the script's own "Need to set the private key" message could print.celocli governance:proposeblock now definesN,NETWORKandRELEASE_CANDIDATE. It used two of them undefined, so copied on its own it resolved toproposal--.json.upgrade_proposal.json, the artifact actually fetched, instead of aproposal.jsonthat appears nowhere else on the page.forge,castandjqadded to prerequisites — every rewritten block runs throughforge build.# Run from packages/protocoladded to every block that needs it; the scripts source relative paths and-itakes a repo-relative path.RELEASE_PROCESS_FOUNDRY.md, the release owners' own doc, whose opening table is exactly this retired-to-Foundry mapping.Corrections to the previous version of this body
Six statements in the earlier body did not hold. Recording them rather than deleting them, since reviewers approve on the strength of the body:
— it is${N-1}is "the literal N"${parameter-word}: unset yieldscore-contracts.v1, and withN=18it yieldscore-contracts.v18, the release being prepared rather than the previous one. Silently wrong rather than obviously wrong, which is a stronger argument for the fix than the one originally made.The OpenZeppelin link rowwas inverted.maincarries the version-less URL, which returns 200. The/1.x/variant 404s. Acting on the row as written would have replaced a working link with a dead one."Four reverts"— one. On three of those four lines the other branch's head is byte-identical to its merge base, so a merge would revert nothing. Only the anvil option genuinely conflicts."Only the first block defined anything"— four of the five later blocks defined their own variables. What none defined wasN, and$PRIVATE_KEYwas not on the page at all before this PR."Each block now stands alone"— was false for thecelocli proposeblock until this commit.— it arrived inexport const N = "N"was inheritedb555aecb(the Docusaurus removal), so the Mintlify migration wrote it. Removing it is still safe: every${N}on the page sits inside inline code or a fence, and MDX evaluates neither.What this does NOT do / residual risk
Documentation only; no script is changed. The page is verified against celo-monorepo
master— if a release is cut from a branch where these scripts differ, the page will drift again. The underlying fragility is that the page hand-copies command names from another repo with nothing checking them;RELEASE_PROCESS_FOUNDRY.mdis now linked so there is at least an authoritative anchor.tags:viewand the twoceloclicommands were already correct and are unchanged.Judgement calls
release:verify-release→release:verify-deployed:foundry -p …rather than dropping the section.RELEASE_PROCESS_FOUNDRY.mdmaps it that way andverify-deployed-forge.shaccepts both-pand-i(getopts 'b:n:fl:i:p:'), so the substitution is mechanical. Reversal: one line.-u/anvil pointer instead of deleting the dry-run sentence outright, since rehearsing a mainnet deploy is the reader's actual goal. Reversal: half a sentence./procon Linux — but it is the upstream script's interface, not this page's, and inventing guidance here felt like the wrong place. Happy to add a sentence if you want it.Issues
Refs #2139 — that PR touches the same file; see below.
Stacking / conflicts
Branched off
main, currently 2 behind (from #2305 and #2286), no conflict. No other open PR touches this file. #2139 overlaps on one line (the anvil option); whichever lands second takes the other's wording.Verification evidence
Every command on the page checked against celo-monorepo
master:Every flag checked against the scripts' own getopts:
The
check-versionsbreakage, confirmed directly:No dead script names remain:
Remaining ops steps
Questions for the maintainer
Do you want the sentence warning that a mainnet deployer key passed via
-klands in argv? It is the upstream interface, so I left it out — see judgement calls.