feat(registry): contract registry and chain deployment - #66
Open
bdchatham wants to merge 9 commits into
Open
Conversation
A contract scenario deploys its contract at the start of every run, so a run against a long-lived chain leaves another copy behind and every canary restart moves its contract to a new address. Nothing in the repo can say "this contract already exists on this chain." This adds the leaf package that will answer that. It holds the types, the chain file format, and the loader. It resolves nothing and verifies nothing yet; PLT-1056 is the first caller that reads a chain. The import boundary comes first, deliberately. boundary_test.go asserts that registry imports no sei-load package except generator/bindings, and it went in before any type existed so it guards every later task. It shells out to `go list -deps` rather than walking imports by hand, so a package this one imports cannot smuggle a forbidden dependency in behind it. Verified by adding a config import and watching it fail. Two notes on what landed differently from the plan: chains/ carries a README rather than a .gitkeep. `//go:embed chains` cannot compile against a directory holding only a dotfile, because embed excludes them. The README satisfies the pattern and tells an operator what belongs there, which the dotfile did not. Load validates rather than trusting the file. A misspelled key would leave a field at its zero value, and a contract at the zero address holds no code on any chain, so the run would send load that does nothing. The strict decode mirrors config.decodeStrict: unknown fields and trailing data both fail. Closes CDR-005, CDR-014, CDR-015, CDR-017, CDR-018, CDR-019, CDR-020. Verifiers: gofmt clean, go vet clean, staticcheck clean, golangci-lint 0 issues, go test ./registry/ 8 tests passing. `make verify` reaches check-bindings and stops there: the Makefile fetches solc-static-linux and this host is Darwin, so that step is CI-only. No .sol file or binding changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Startup ( Config & CLI: Scenarios drop per-scenario Reviewed by Cursor Bugbot for commit eff6aa1. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Adds the surface a run calls. The registry now answers "does this contract already exist on this chain?", and produces the entry to write when it did not. Verify hashes what eth_getCode serves and compares it to the recorded hash. A mismatch returns *MismatchError, which is a distinct type so a caller can tell a stale registry from a dial failure — the two need different responses from an operator, and collapsing them would send someone looking for the wrong problem. A test asserts that separation. An address holding no code hashes to the zero hash rather than to Keccak-256 of the empty string. One error type therefore covers both mismatches, and Error tells them apart. Resolve verifies before it returns an address, and reports a deploy for an ordinary miss rather than failing. A mismatch is an error and never a deploy signal: redeploying over a stale entry looks like a fix and is not, because it hides that the registry no longer describes the chain. WriteChain refuses to write into the compiled-in chains directory. The binary cannot know where its own source tree is, so the check matches the two paths a run can realistically be given from inside the repo, "registry/chains" and a bare "chains". It is a guardrail against that mistake rather than a sandbox, and the doc comment says so. Record reads the code back from the chain rather than hashing the compiled bytecode. Creation bytecode runs the constructor and returns the runtime code, and only the runtime code is what eth_getCode serves. It refuses an address holding nothing, so a deployment that did not take effect fails here instead of producing an entry no later run can verify. Both guards were verified failing before being trusted. The write refusal test runs from a temp working directory with parents created, so a broken guard fails by writing the file rather than by hitting a missing directory; neutering the check turns both refusal cases red. Two control cases assert a directory that merely contains the word is still allowed. Closes CDR-001, CDR-002, CDR-003, CDR-006, CDR-007, CDR-008, CDR-009, CDR-010, CDR-011. Verifiers: gofmt clean, go vet clean, staticcheck clean, 17 tests passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A run now asks the registry for each contract its profile drives, deploys only what the registry does not name, and records what it deployed. A stale entry stops the run at startup instead of corrupting it. Removing AttachScenario found more than the spec described. Each of the six contract scenarios overrode Attach to dial and bind — and that override called the base, which dialed and bound again. Every contract scenario was dialing twice and binding twice on attach. Both paths are gone. What replaces them: Ready(config) marks a scenario able to generate, and Binder() returns a closure that binds one contract and stores the instance. The closure is built where the contract type is known, so the preparation step drives it without knowing that type, and the step owns the backend and the address. No scenario dials, and none panics. ScenarioBase no longer has an address field, and GetAddress is gone. Both were dead — nothing outside the type read either — and deleting them makes CDR-021 structurally true rather than a comment: a scenario cannot hold what does not exist. staticcheck flagged the leftover write, which is what surfaced it. Address precedence is fixed and explicit: a configured contractAddress, then forceDeploy, then a registry entry, then a deployment because nothing named one. Every assertion was verified failing before being trusted. The no-deployment test has a control that points the fixture at a different address and asserts the run refuses to start. The CI gate on committed chain files was checked against a deliberately malformed file. Closes CDR-004, CDR-012, CDR-013, CDR-016, CDR-019, CDR-020, CDR-021, CDR-022, CDR-023, and the T014/T019/T022/T023/T026/T027/T028 assertions. Verifiers: gofmt clean, go vet clean, staticcheck clean, golangci-lint 0 issues, all 14 packages passing. check-bindings is CI-only on Darwin and this change touches no .sol file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An xreview slate of four independent reviewers dissented. The systems lens reproduced seven defects with real tests; the platform lens found a cross-cutting one. This closes the ship-blocking set. Each fix carries a test that fails without it. AS-3.3 was false for any profile of more than one scenario. Resolution and deployment ran interleaved per instance, so a stale entry on the second scenario left the first one's contract deployed, paid for, and recorded nowhere — the litter this feature exists to remove. Reproduced, then fixed: prepareAll now runs three passes, and every address is decided and every recorded address verified before anything deploys. Startup had no timeout. ethclient.Dial over HTTP performs no I/O and its client sets no deadline, so an endpoint that accepts and never answers held startup open with nothing logged. Both chain-reading phases now run inside WithinBudget, which the deploy path already used. Two paths failed open, which is the one behaviour that cannot be right here. A run naming no genesisHash matched nothing and redeployed on every restart in silence; it now fails when the registry describes that chain id. An explicit contractAddress skipped verification entirely, so a typo produced a full run of green metrics against an address holding nothing; it now checks for code. The registry key was the raw profile string while every other name lookup in sei-load lowercases. Profiles write "ERC20Conflict"; the constants are lowercase. My test passed only because it keyed off the constant, which no real profile writes. The frozen file format admitted duplicate contract names, which the generator could itself produce from a profile naming one scenario twice. The later entry was unreachable forever. validate now rejects it, before any file is committed and the format is load-bearing. TestTwoRunsOnOneChainDeployTheirOwn certified CDR-013, which does not hold. It minted a fresh key per run; funder.Deployer hands every pod the same funding root. Renamed to say what it proves, and the real limitation is now in the package doc: concurrent runs on one key produce identical deployments. No test asserts that — reproducing it needs a race, and a test that asserts a bad property by winning a race is worse than the gap. Also: three orphaned AttachScenario methods survived on the EVMTransfer family, so SC-006's verifier — grep for the removed method — returned seven hits. Both package docs still described deployAll and Attach. A regex deletion had truncated CreateContractTransaction's doc comment in StorageRW.go, taking the frozen draw-order invariant with it; no verifier caught that, because none of them read comments. recordDeployments dialed a second client against CDR-023. The two new config fields skipped Scenario.Validate, so contractAddress and forceDeploy set together were accepted and one silently won. Verifiers: gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing. SC-006's grep now returns nothing. Known and not fixed, recorded in the review notes: chain-record-path cannot write in either deployed pod shape except /dev/stdout; symlinks walk past checkWritePath; the code hash cannot distinguish two instances of one contract; startup emits no span and the run summary carries no contract address. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nt volume Adds the startup check for the one pairing that still failed late: chainRecordPath set with genesisHash empty deployed every contract and then failed on WriteChain's validation, leaving contracts on the chain that nothing recorded. ValidateRecording refuses it before the first dial. It runs in runLoadTest rather than loadConfig, because the flag that sets the field is merged after the profile is parsed. Then a brandon-code pass over the feature. prepareAll now reads as its step sequence — plan, deploy, bind, record — with the WithinBudget ceremony behind planAllWithinBudget. The closure had also been reassigning the orchestration's own err from inside itself, which worked and read badly. Comment volume came down against principles 13 and 14. An unexported function defaults to no comment and has to earn one: addEmbedded, describeHash and bindAll lost theirs, and the ones that hold an invariant a future editor could break silently kept theirs, trimmed. codeHashAt keeps its zero-hash rule because both error branches depend on it. deployPlanned keeps the nonce-ordering invariant. validate, decodeStrict and contractNameFor each keep the one sentence that says why. Verified placement with go doc -u rather than by reading the file: a group fused by a missing blank line renders under the wrong symbol and looks correct in source. No fused groups. checkWritePath led with a claim its own second paragraph withdrew — "enforceable rather than a convention" and "still cannot edit it" — and the review proved a symlink walks past it. It now leads with the two paths it actually matches and names the escape. chains/README.md had copied the strong form and dropped the caveat; it now carries the honest one, says bootstrapping is a local operation rather than a deployed one, and states the forward-compatibility cost of adding a field. Verifiers: gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing unchanged — which is the proof this changed structure and comments only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chain file is a one-way door and these three sit inside it. Approved 2026-08-24 before any file is committed, which is the last moment they are free. Contract names are lowercase. The scenario factory already resolves a name that way, every profile in this repo writes CamelCase, and the constants are lowercase. Keying on the raw string would record "ERC20Conflict" from one profile and miss it from another that wrote "erc20conflict". A scenario may set contractKey to override the key its contract is recorded under. Two runs driving one chain must not share a contract: they write the same storage slots, and that contention is in neither profile. The three arctic-1 canary cells run one profile against one chain, so without distinct keys all three would measure a workload nobody configured. Long-lived chains are committed; re-genesised ones are supplied. pacific-1 and atlantic-2 keep their addresses, so committing puts them in a reviewed pull request and a signed image. arctic-1 changes its genesis hash on every re-genesis, and recovering a committed entry needs a sei-load pull request, a CI build, and a hand-edited image pin per cell — where the three cells are pinned independently on purpose. The cost of supplying a file is stated rather than left implicit: whoever can edit that source can name a contract of their choosing, and the code-hash check cannot catch it, because it proves the address holds the code the file recorded and not that the code is ours. The canary mounts a funded key, so the loss is bounded by that key's balance. That bound is why this is acceptable for arctic-1 and not for pacific-1. Verifiers: gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
changed the title
bdchatham
commented
Aug 24, 2026
Bugbot caught a defect my own duplicate-name check introduced. A profile naming one scenario twice deployed two contracts, then WriteChain rejected the file for holding two entries under one name — so the run died after both deployments, leaving them on-chain and unrecorded. That is the same failure class as the AS-3.3 violation this branch already fixed, reintroduced by the fix for a different finding. Reproduced before fixing. The root cause was two vocabularies. createScenarios suffixes the display name to storagerw_0 and storagerw_1, while contractNameFor keys on the unsuffixed profile name, so two instances shared one key. Resolution now groups instances by contract name: resolve once, deploy once, bind every instance in the group. That also closes the asymmetry the systems review raised separately — deploying per instance gave a fresh chain two contracts and a covered chain one, from the same profile, so the same profile measured a different workload depending on which chain it ran against. An operator who wants two contracts sets a distinct contractKey on each, which is what that field is for. Renames prepared to binding, per review. A past participle reads as something that happened rather than a thing the code holds; binding names what it is, and pairs with the plan-then-bind sequence around it. prepareAll now reads as five named steps: plan, deploy what is missing, ready, bind, record. Verifiers: gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bdchatham
commented
Aug 24, 2026
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ff25edd. Configure here.
…ording check Round 2 of the systems review verified F1, F2, F4, F5, F6 and F15 closed against real probes. It found one new defect and one incomplete fix, both reproduced before this change. Grouping instances by contract name let the group silently inherit the first instance's selection. forceDeploy on a second instance was dropped without a word, and config validation accepted it: an operator sets forceDeploy to get virgin storage for a contention measurement and binds a contract carrying another run's state instead. That is the hazard the comment on ContractAddress already names, moved from the field to the group. groupByContractName now rejects a group whose instances disagree, and names contractKey as the way to have two contracts on purpose. planOne's doc had asserted "validate rejects the config where they do not". Nothing did. A comment long enough to assert a clause, plausible enough that nobody checked — the P13 failure mode, inside the change that cites P13. The clause is now true because the guard exists and the doc names it. ValidateRecording sat only in main, so NewGenerator still deployed a contract and then failed the write the function exists to pre-empt. It now runs at prepareAll as well, which is the function every caller of the package reaches. main keeps its call for the early exit before the metrics server starts. MismatchError asserted a cause it had not checked. A lagging or syncing endpoint produces the identical symptom, and the run reads one endpoint at latest. It now states what was observed and offers both causes, so nobody chases a re-genesis that did not happen at 3am. Structure, from the same review: the budget wrapper is folded into planAll and now covers only the chain reads, which is what its constant says it bounds; grouping needs no timeout. prepareAll returns an error rather than a slice nobody read. recordDeployments gets its own line instead of riding a return that paired a non-nil value with a non-nil error. The doc said "three passes" in the commit that made it five. Two comments moved rather than being deleted. The read-only-pod claim was a cross-repo invariant a comment cannot hold; it is now in the --chain-record-path flag help, where the operator who needs it reads it. The duplicated green-metrics rationale now lives once, on VerifyHasCode. Verifiers: git status clean, gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing. Reporting the tree state explicitly because last round I called it clean while an untracked failing probe sat in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checked claims against behaviour rather than reading for typos. Six were wrong, and one was a code defect the sweep surfaced. The registry imports no sei-load package at all — `go list -deps` returns only itself. doc.go said "imports no other sei-load package except generator/bindings", which implies a dependency that does not exist, and boundary_test.go justified the allowance as "the bytecode the registry hashes against". It hashes what eth_getCode serves, not compiled bytecode, so nothing needs the bindings yet. Both now say the allowance is CDR-017's ceiling rather than a list of what is used. --dry-run previewed something a real run would not do. mockPrepareAll gave each instance its own random address while the live path groups by contract name and shares one, so a dry-run of a profile naming one scenario twice showed two contracts where a run gives one. It groups now. A preview that does not match is worse than no preview. generator/doc.go told an operator to "give each concurrent run its own deployer key until this is fixed". funder.Deployer returns the funding root, so no profile can do that — the advice reads as actionable and is not. It now says closing the gap needs a code change. Three stale names: deployPlanned for deployMissing in generator/doc.go, mockDeployAll for mockPrepareAll in StorageRW_test.go, and the mock-deploy paragraph describing per-instance binding. README's Command Line Options table gained --chain-file and --chain-record-path, including the two things an operator learns the hard way otherwise: a --chain-file path that does not exist fails startup, and recording needs genesisHash. Verified no phantom documentation: every flag named in a doc exists in main.go, and every JSON field named in a doc has a struct tag. Verifiers: gofmt, go vet, staticcheck, golangci-lint 0 issues, all 14 packages passing, git status clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes PLT-1055, PLT-1056, PLT-1057, PLT-1058 and PLT-1059 — the whole of PLT-1060.
Scope note: this PR opened as PLT-1055 alone and grew to the full feature. One PR rather than five stacked, because both gating workflows are
pull_request: branches: [main]— a PR based on a feature branch runs zero checks. Five stacked PRs would mean four merging untested. The five commits are the review path:2e6ce8bregistry/package: types, chain file format, embedded loader, import boundary58a8f5fa8b2534generator/to consult the registry before deploying356777384276b6+HEAD/brandon-codepass; the three format decisionsThe problem
A contract scenario deploys its contract at the start of every run. Correct on a fresh chain, wrong on every long-lived one. Each run against arctic-1, atlantic-2 or pacific-1 leaves another copy behind, and every canary restart moves the contract — so a metric series spanning two restarts describes two different contracts, and nothing says the address changed.
sei-load already had the mechanism and no profile could reach it:
Attachbound a scenario to an existing address, only the unit-test path called it, andconfig.Scenariohad no address field.What a run does now
Resolve every contract the profile drives, verify every recorded address, deploy only what nothing named, bind, and optionally record what it deployed. A stale entry stops the run before it sends a transaction.
Address precedence is fixed: a configured
contractAddress, thenforceDeploy, then a registry entry, then a deployment.The review found things I had not
Four independent blinded reviewers — idiom, systems (assigned dissenter), platform, prose. All four dissented. The dissenter wrote probes and seven of seven failed. Everything ship-blocking is fixed, each with a test that fails without it.
AS-3.3 was false. The spec says a failed run deployed nothing. Resolution and deployment ran interleaved per scenario, so a stale entry on the second scenario left the first one's contract deployed, paid for, and recorded nowhere — the exact litter this feature removes. Reproduced, then fixed with three passes: every address decided and every recorded address verified before anything deploys.
Two paths failed open. A run naming no
genesisHashmatched nothing and redeployed on every restart in silence. An explicitcontractAddressskipped verification entirely, so a typo produced a full run of green metrics against an address holding nothing.Startup had no timeout.
ethclient.Dialover HTTP performs no I/O and its client sets no deadline, so an endpoint that accepts and never answers held startup open with nothing logged.The registry key was case-sensitive while every other name lookup in sei-load lowercases. Profiles write
"ERC20Conflict"; the constants are lowercase.The frozen format admitted duplicate names that the generator could itself produce, leaving the later entry unreachable forever.
A test certified a property the system does not have.
TestTwoRunsOnOneChainDeployTheirOwnminted a fresh key per run;funder.Deployerhands every pod the same funding root. Renamed to say what it proves; the real limitation is in the package doc.Three defects were mine and slipped every verifier:
AttachScenariomethods survived. They compile because nothing requires them, so the build stayed green — and SC-006's verifier is grep for the removed method, which returned seven hits. Now zero.CreateContractTransaction's doc comment inStorageRW.go, taking the frozen draw-order invariant with it.gofmt,vet,staticcheckandgolangci-lintwere all clean over it, because none of them read comments.Scenario.Validate, socontractAddressandforceDeployset together were accepted and one silently won.One-way doors, approved 2026-08-24
The chain file format is frozen once a file exists.
chains/ships empty, so this was the last free moment.contractKeyoverrides the recording key. Two runs on one chain must not share a contract — they write the same storage slots, and that contention is in neither profile. The three arctic-1 canary cells are the case.--chain-file. Recovering a committed arctic-1 entry after a re-genesis needs a PR, a build, and a hand-edited image pin per cell — and the three cells are pinned independently on purpose.chains/README.mdstates the security cost of supplying instead.Verification
gofmt,go vet,staticcheck,golangci-lint— clean, 0 issues. All 14 packages pass. 19 registry tests, 17 generator tests.Every guard was proven failing before being trusted: the import boundary against a real
configimport, the write refusal by neutering the check, the CI gate against a malformed chain file, the no-deployment assertion by pointing the fixture elsewhere.make verifystops atcheck-bindingslocally — the Makefile fetchessolc-static-linuxand this host is Darwin. CI covers it, and this change touches no.solfile.Known and not fixed
Filed rather than dropped:
--chain-record-pathcan only write to/dev/stdoutin a pod; both deployed shapes setreadOnlyRootFilesystemwith read-only mounts. Bootstrapping a long-lived chain is a local operation, and the README now says so.checkWritePath. The doc says guardrail, not sandbox.🤖 Generated with Claude Code