feat(testnet): --steps=admin — a guarded Step-8-only re-run (route A) - #66
Conversation
Route A, per the owner's decision on Flashstack-ajv.6.3. §6b already proved the
BC1 property on this deployment, so what §6a is missing is assertion strength on
the positive path — not a deployment. Republishing a whole contract line to fix
that would be the wrong shape of change, and route B additionally needed new
.clar sources because stx-test-receiver-v2.clar:16 calls .flashstack-stx-core-v2
statically.
node scripts/deploy-testnet.mjs --steps=admin
runs Step 8 and nothing else against an already-deployed core: no publishes, no
reserve funding, no flash loan, no STX moved. Three calls at 100_000 uSTX.
--steps=all remains the default and is unchanged.
Step 8 is EXTRACTED into proveBC1(), not copied. A second implementation of the
assertion sequence is how the two drift, and a drifted copy that still prints OK
is the F-7 shape again. Both modes call the same function; the diff for it is a
pure move.
THE GUARD (asked for explicitly). Skipping Steps 1-7 means the script no longer
establishes the state it then assumes — it inherits whatever is on chain. So
scripts/lib/testnet-preconditions.mjs refuses to broadcast anything unless:
1. the contract exists at that principal — otherwise the admin calls hit
nothing and it reads like a contract bug, not a targeting mistake;
2. get-admin is the signing deployer — transfer-admin is admin-gated, so
otherwise the run aborts mid-sequence and can strand a live pending-admin.
This is precisely the window deploy-testnet-bc1-negative.mjs documents;
3. get-pending-admin is none — a leftover proposal means an earlier run did
not finish, so the contract is not in the state §6a/§6b describe, and
someone else's offer is sitting there to be accepted.
It lives in scripts/lib/ for the reason #58 moved callReadOnly/assertEqual there:
deploy-testnet.mjs process.exit(1)s at import without TESTNET_MNEMONIC, so
nothing inline in it can be tested — and a guard whose failure mode is "silently
allows the run" is the last thing that should be untestable.
9 tests, weighted toward the passes-when-it-shouldn't direction: admin moved,
proposal outstanding, (some <deployer>) not mistaken for clear, an (err ...) read
not swallowed, and no state read attempted at all when the contract is absent.
Also exercised against the live chain, no key needed: GREEN on the real
ST3XQ5DM... deployment (exists / admin is deployer / pending none), RED on a
nonexistent contract name and RED on the wrong principal. Both reds were caught
at the existence check, which is where they should be; the admin and pending
branches are covered by the unit tests.
The balance pre-flight is now mode-aware — its 5000/3000 STX thresholds describe
the full run and would have warned about 5000 STX for a 0.3 STX job.
Suite 230/230 across 21 files, up from 221/20 by exactly this file's 9 tests.
No contract, no Clarinet.toml, no change to the --steps=all path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ts it Owner picked route A. §6b already proves the property on this deployment, so the gap is assertion strength on the positive path, not a deployment — and B would rewrite contracts to fix a documentation problem. B is parked: staging flashstack-stx-pool-v3 is worth more than a -v3 rename. §6c's route table and the plan file's execution-decision block updated to say so, and to name deploy-testnet.mjs --steps=admin (PR #66) as what makes A executable. Recorded honestly rather than quietly: --steps=admin runs deploy-testnet.mjs's own Step 8 path — extracted into proveBC1(), not copied — but not the whole script, so it does not literally satisfy ajv.6.3's original "a run of the CURRENT deploy-testnet.mjs". That criterion was amended in Beads to say so openly. Consequence for the plan file, now stated in it: steps 1-7 stay unexecuted for this line, since they are already on chain from the §6a run. Only the 8a/8b/8c evidence: slots get filled. Steps 1-7 remain the written record of how that deployment was produced, and the plan a new line would follow. Docs only; the YAML still parses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattglory
left a comment
There was a problem hiding this comment.
Requesting one change. Everything else is approve-quality, and the design is right: extract rather than copy, guard before the first broadcast, and put the guard where a test can reach it.
The change: malformed --steps spellings silently select full-deploy mode
The parser is argv.find(a => a.startsWith("--steps="))?.split("=")[1] ?? "all", so anything that doesn't start with exactly --steps= falls through to the default, and the default is the expensive mode. Reproduced on head 0419e9c with the public BIP-39 test-vector mnemonic (unfunded, so nothing can broadcast):
| input | result |
|---|---|
--steps=admin |
admin mode |
--steps admin |
full-deploy mode |
--step=admin |
full-deploy mode |
--steps= |
refused |
--steps=bogus |
refused |
With a funded key, the space form or a one-letter typo would start Step 1 re-publishing a name that already exists. That isn't a fund-loss path, but it runs the wrong mode with a funded key, which is exactly what the guard exists to prevent, and the guard only runs in admin mode.
Suggest rejecting anything that isn't exactly one --steps=all|admin, and moving it into scripts/lib so it's testable (same reasoning as #58 and testnet-preconditions.mjs, since deploy-testnet.mjs exits at import):
export function parseSteps(argv) {
const args = argv.slice(2);
if (args.length > 1 || args.some(a => !/^--steps=(all|admin)$/i.test(a))) {
throw new Error(`unrecognised argument(s): ${args.join(" ")}. Use --steps=all (default) or --steps=admin.`);
}
return (args[0]?.split("=")[1] ?? "all").toLowerCase();
}Cases worth pinning: no args → all; --steps=ADMIN → admin; --steps admin, --step=admin, --steps= and --steps=all --steps=admin all throw.
Verified independently
proveBC1()is a pure move. I normalised the old Step 8 block against the function body: 44 lines each side, zero differences.- Guard placement. It runs before any broadcast, and the
--steps=allbalance thresholds are unchanged (the inner 500 STX minimum stays nested). - Tests. 9/9 on this head; 230/230 when I merged it with
mainat the time. - Mutation-tested the guard. I broke it six ways: existence check ignored, admin check removed, pending check removed,
(some <deployer>)treated as clear, an(err …)read swallowed, admin check inverted. Every mutant fails at least one test. - Live chain. Green on
ST3XQ5DM….flashstack-stx-core-v2; red on a wrong contract name and on a wrong deployer. Matches your evidence.
Non-blocking
OTHERis the public Clarinet key. Between proposing to it and re-proposing to self, anyone can callaccept-adminasST1PQHQ…and take the staging core. That was my choice in #56, and it's testnet with nothing at stake, but a stolen admin can't be recovered (the deployer's re-propose would failERR-NOT-ADMIN).ST000000000000000000002AMW42His a valid principal and hosts the system boot contracts (pox-4,costs-3andbnsexist there), so nobody can accept for it. The assertions use theOTHERconstant, so it's a one-line change now that Step 8 lives inproveBC1(). I'd do it as a follow-up so this PR stays a pure move.contractExistsreads any non-2xx as "no contract". A 429 or 5xx prints "no contract at …", which is misleading, though it fails closed. Distinguishing 404 from the rest would make the message honest.
Happy to re-review as soon as the parser change is in.
unixwhisperer
left a comment
There was a problem hiding this comment.
Fresh review, treating this as a changed control path rather than a CLI change — the flag decides whether a funded key performs a full deploy or a narrow Step-8 re-run, so the parser is the control.
Head is still 0419e9c2, the single original commit. Your requested change isn't in yet. So this is a confirmation, not a new finding: I reproduced it independently rather than taking it on trust, and it's worse than "malformed spellings pick the default".
Your finding — reproduced, confirmed, and it bypasses the guard
[] -> all
--steps=admin -> admin
--steps admin -> all <- space form, silently full-deploy
--step=admin -> all <- one-letter typo, silently full-deploy
--stepsXadmin -> all
--steps= -> refused
--steps=bogus -> refused
--steps=ADMIN -> admin (intended)
--steps=all --steps=admin -> all <- duplicate not rejected, first wins
Matches your table exactly, plus the duplicate-flag case you listed as worth pinning.
The part I'd underline: the precondition guard only runs in admin mode. So a typo doesn't merely pick the expensive path — it routes around the entire testnet-preconditions.mjs check in the same step. Contract-exists, admin-is-deployer, pending-admin-is-none: none of them evaluate. The guard I wrote to make this flag safe is inert precisely in the case where the operator got the flag wrong, which is the case it exists for.
It's a fail-open default on a control path. Not fund-loss — Step 1 re-publishing an existing contract name fails on chain — but it broadcasts with a funded key in a mode the operator didn't ask for, with no guard. That's an accidental-bypass path, and your framing of it as the one blocking change is right.
I'll take your parseSteps shape as-is, in scripts/lib/ for the same reason testnet-preconditions.mjs lives there — deploy-testnet.mjs process.exit(1)s at import without TESTNET_MNEMONIC, so nothing inline in it is reachable by a test, and a guard whose failure mode is "silently allows the run" is the last thing that should be untestable. Cases pinned: no args → all; --steps=ADMIN → admin; and throwing for --steps admin, --step=admin, --steps=, --steps=bogus, and duplicate flags.
One addition to your version: the error should name the mode it refused to run, not just echo the argument. "unrecognised argument(s): --step=admin" tells the operator less than it could when the consequence of ignoring it is a full deploy.
The rest holds up
Confirming independently, since this is my own PR and I'd rather not be the only one who checked it:
proveBC1()is a pure move, not a copy. This matters more than it looks — a drifted copy that still printsOKis the F-7 shape again, which is the failure this repo has already had once.- Guard placement is before any broadcast, and the
--steps=allbalance thresholds are unchanged. - Mode-aware balance pre-flight is right: warning about 5000 STX for a 0.3 STX job trains operators to ignore the warning.
- 9 tests, weighted toward the passes-when-it-shouldn't direction, which is the correct weighting for a guard.
Thanks for mutation-testing the guard six ways — that's the check I'd have asked for, and "every mutant fails at least one test" is the answer that makes the 9 tests mean something.
Your two non-blocking notes — both accepted
1. OTHER is the public Clarinet key. Agreed, and it's my call from #56 to fix. Anyone can accept-admin as ST1PQHQ… between propose and re-propose, and a stolen admin can't be recovered since the deployer's re-propose fails ERR-NOT-ADMIN. Testnet with nothing at stake, but it's a one-liner now that Step 8 lives in proveBC1(). ST000000000000000000002AMW42H is the right target — valid principal, hosts pox-4/costs-3/bns, nobody can accept for it. Follow-up PR so this one stays a pure move, as you suggested.
2. contractExists reads any non-2xx as "no contract". Agreed — fails closed, which is the right direction, but a 429 or 5xx printing "no contract at …" sends the operator to debug a targeting mistake they didn't make. I'll distinguish 404 from the rest in the same follow-up.
Status
Taking this back to make the parser change; I'll re-request review when it's in. Holding the OTHER change and the contractExists message for a follow-up so this PR stays reviewable as the pure move it is.
The parser fell through to the default on anything not starting with exactly `--steps=`, and the default is the full deploy: --steps admin -> all (space form) --step=admin -> all (one-letter typo) --stepsXadmin -> all --steps=all --steps=admin -> all (first wins, duplicate not rejected) Reproduced all four before changing anything. The sharp edge is not the wasted deploy. assertAdminStepPreconditions only runs in admin mode, so a typo did not merely pick the wrong mode -- it skipped the guard entirely, in exactly the case the guard exists for. Fail-open on a control path. parseSteps() is allow-list only: exactly one argument matching exactly --steps=all or --steps=admin, case-insensitive, normalised to lowercase. Anything else throws. No best-effort interpretation -- guessing what the operator meant is how the space form became a full deploy in the first place. In scripts/lib/ for the reason #58 moved callReadOnly/assertEqual there: deploy-testnet.mjs process.exit(1)s at import without TESTNET_MNEMONIC, so nothing inline in it is reachable by a test, and a guard whose failure mode is "silently allows the run" is the last thing that should be untestable. The error names the rejected argument and states that it is NOT falling back to the full deploy -- an error that omits that invites a shrug and a re-run. Drops the now-redundant ["all","admin"].includes(STEPS) check, which parseSteps subsumes. 16 tests, weighted toward the passes-when-it-shouldn't direction. Mutation- tested: restoring the old one-liner fails 13 of the 16. Suite 246/246 across 22 files, up from 230/21 by exactly this file. No contract changes, no Clarinet.toml change, --steps=all path untouched.
|
Parser change is in — What changed
All four of your cases now throw, plus the duplicate-flag case you flagged as worth pinning:
I also dropped the One thing I added to your versionThe error names the mode it refused to run, not just the bad argument: The failure being fixed is a silent fall-through to full deploy. An error that doesn't say so invites the operator to shrug and re-run, which is most of the way back to the original problem. Two tests pin that wording. Evidence16 tests, weighted toward the passes-when-it-shouldn't direction, same as Mutation-tested, since green tests on their own prove nothing — I restored the old one-liner as the mutant: The 3 survivors are the genuinely-valid spellings, which is correct — they behaved identically before. Every fail-open case is caught. End-to-end through the real CLI, confirming exit codes rather than just the function: The last one matters: a valid flag still falls through to the mnemonic check exactly as before, so the parser sits in front of the existing flow rather than replacing part of it. Suite 246/246 across 22 files, up from 230/21 by exactly this file's 16. No contract changes, no Still holding for a follow-up, as agreedYour two non-blocking notes — Also: this now takes the suite to 246, so the test-count reconciliation you assigned to #65 (221 + 8 = 229 from #68) will need a third update once this lands. Happy to carry it here instead if you'd rather — say which. |
#68 merged first as agreed. This carries the reconciliation: - FINDINGS_REGISTER F-8: split by deployment status. Live v2 pools stay Open (immutable, can't be patched). Undeployed v3 pools now Fixed -- the actual asserts! line quoted, matching PR #68's code exactly. Severity moves from "Low (proposed)" to "Low (confirmed by the Security & Contract Lead)", per her review on #68. - AUDIT_SCOPE.md: three passages updated to match -- the "tested only for admin transfer" line now also credits the 8 mutation-checked pause-gate tests; the "what is not done" bullet no longer describes the v3 pools as vulnerable, only the live v2 ones; test count 221 -> 229 across 21 files (not folding in #66's +9, still in review). Reviewed-state hash updated to main's current tip post-#68 (9210540). Verified on this tree: clarinet check 211/0, suite 229/229, and the v3-pools-pause-gate + bc1-two-step-fix suites specifically green (16/16). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…irst You flagged this in the #66 review and offered either of us carry it -- taking it here since #68/#65/#69 all merged to main in between your "246" figure (computed against pre-#68 main) and now. 221 (post-#65) + 8 (#68, v3-pools-pause-gate) + 9 (testnet-preconditions, this PR's first commit) + 16 (parse-steps, this PR's second commit) = 254, across 23 files. Verified by running the suite on this branch merged with current main, not by arithmetic alone. README.md (badge, Security bullet, Quick start) and ROADMAP.md (x2) were still at 221 -- a gap I left when I did the #65 reconciliation and only updated AUDIT_SCOPE.md's count, not these. Fixed here too so the same drift doesn't need a fourth pass. clarinet check: 211 contracts, 0 errors (unchanged). Suite: 254/254 across 23 files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
mattglory
left a comment
There was a problem hiding this comment.
Approving. Everything in the writeup reproduced independently, and one thing was stale for a timing reason, now fixed.
Reproduced exactly, end-to-end through the real CLI (public test-vector, unfunded, so nothing could broadcast):
| input | result |
|---|---|
| (none) | falls through to TESTNET_MNEMONIC check (i.e. all) |
--steps=admin |
falls through to TESTNET_MNEMONIC check (i.e. admin) |
--steps admin |
exit 1, parser error |
--step=admin |
exit 1, parser error |
--stepsXadmin |
exit 1, parser error |
--steps=all --steps=admin |
exit 1, parser error |
--steps= |
exit 1, parser error |
--steps=bogus |
exit 1, parser error |
Matches your table exactly.
Mutation test, reproduced exactly. Swapped parseSteps for the old one-liner: 13 failed | 3 passed (16), same number you reported. The 3 survivors are the genuinely-valid spellings, correctly unaffected.
Also verified: proveBC1() re-checked after this commit — still a pure move, zero diff against the pre-refactor block. clarinet check 211/0, unchanged. Diff is exactly the 5 files you listed — no contract, no Clarinet.toml.
The one thing that needed fixing: the suite count. Your 246 was correct when you computed it, but #68/#65/#69 all merged to main in between, adding 8 more (from #68). Pushed ff33481 directly to this branch: full suite is 254 across 23 files, verified by running it, not arithmetic. Also fixed README.md (badge, Security bullet, Quick start) and ROADMAP.md (×2), which were still at 221 — a gap I left when I did the #65 reconciliation and only updated AUDIT_SCOPE.md's count. Took the "say which" you offered since it's mechanical and #68/#65/#69 are all mine to have kept in sync.
Your OTHER and contractExists follow-ups are still deliberately out of this PR, agreed. Merging.
Route A, per your call on
ajv.6.3. §6b already proved the property on this deployment, so what §6a is missing is assertion strength on the positive path — not a deployment.Step 8 and nothing else, against the already-deployed core: no publishes, no reserve funding, no flash loan, no STX moved. Three calls at 100,000 µSTX.
--steps=allstays the default and is unchanged.Step 8 is extracted, not copied
proveBC1()— both modes call the same function. A second implementation of the assertion sequence is how the two drift, and a drifted copy that still printsOKis the F-7 shape again. Its diff is a pure move; worth reading with--color-moved.The guard
Skipping Steps 1–7 means the script no longer establishes the state it then assumes — it inherits whatever is on chain.
scripts/lib/testnet-preconditions.mjsrefuses to broadcast anything unless:get-adminis the signing deployer.transfer-adminis admin-gated, so otherwise the run aborts mid-sequence and can strand a live pending-admin — precisely the windowdeploy-testnet-bc1-negative.mjsdocuments in its own header.get-pending-adminisnone. A leftover proposal means an earlier run didn't finish, so the contract isn't in the state §6a/§6b describe — and someone else's offer is sitting there to be accepted.It's in
scripts/lib/for the reason #58 movedcallReadOnly/assertEqualthere:deploy-testnet.mjsprocess.exit(1)s at import withoutTESTNET_MNEMONIC, so nothing inline in it can be tested. A guard whose failure mode is "silently allows the run" is the last thing that should be untestable.Evidence
9 unit tests, weighted toward the passes-when-it-shouldn't direction: admin moved to someone else, proposal outstanding,
(some <deployer>)not mistaken for clear, an(err …)read not swallowed into a pass, and no state read attempted at all when the contract is absent.Against the live chain (read-only, no key):
Both reds tripped at the existence check, which is where they should trip; the admin and pending branches are covered by the unit tests, not by this.
Also
Balance pre-flight is mode-aware — its 5000/3000 STX thresholds describe the full run and would otherwise warn about 5000 STX for a 0.3 STX job.
Suite 230/230 across 21 files, up from 221/20 by exactly this file's 9 tests. No contract, no
Clarinet.toml, no change to the--steps=allpath.Stacked on nothing — independent of #64, which carries the plan file and §6c. I'll update those to name this flag.
🤖 Generated with Claude Code