Repository navigation
test: pin the deploy script's target set and the registries' dependency lists - #186
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
…ependency lists Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
What
Two assertions about this repo's own deploy declaration and the script that
broadcasts it, plus the harness one of them reads through.
test/src/abstract/RegistryDeploySuites.t.solpins both candidates'dependency lists as empty.
test/script/Deploy.t.solpins the network set a dispatch ofscript/Deploy.solreaches, againstfoundry.tomlrather than against thebody it inherits.
test/concrete/DeployHarness.solexposesdeployNetworks()externally, theway
BuildHarnessexposesBuild's internal declarations.No source changed.
Why
Every other field of a candidate has something behind it. The recorded address,
code hash and runtime code are checked against what the recorded creation code
derives; the suite key, the artifact path and the candidate set are checked
against the generator's list and against the committed snapshots; the source
anchor checks the recorded creation code against current source.
dependencieshad none of that. Nothing derives it, the generator neitherwrites it nor reads it for a rolling candidate, and the only consumer is
deployToNetworks, on a fork, behind an RPC. An entry added to either listtakes that registry off every network where the named address has no code --
per network, after the fork, at dispatch time -- and both of these contracts are
declared to have no such address at all:
AddressRegistryreads nothing andcalls nothing at construction, and
MigrationRegistrytakes no constructorargument and resolves its namespace from
msg.sender.script/Deploy.solwas reached by no test at all. Its empty body is a claimthat a dispatch goes to every network this repo is configured for, and half of
that claim is already a compile-time guarantee --
RegistryDeploySuitesspellsreleasedSuitesandcandidateSuitesasoverridewithoutvirtual, so ascript broadcasting a different declaration than the verification contracts
check does not build.
deployNetworksis the half that is left: it IS virtual,because a repo bootstrapping onto one chain at a time overrides it, so this repo
taking the inherited answer is a decision and a narrowed one here is a dispatch
that silently skips chains with the whole suite green.
The network assertion is written against the
[rpc_endpoints]keys rather thanagainst
supportedNetworks(), which is the value the inherited body returns.Written against that value it would be the implementation compared to itself and
would pass for any override that happened to call it.
foundry.tomlis aseparate statement of the same set, and
testSupportedNetworksAreFullyConfiguredis what holds the two together.QA
src/abstract/RegistryDeploySuites.sol,script/Deploy.solandscript/CreditHyperCore.sol, 19 of 27 killed before this branch and 22 of 27 after, with the killing test named per row.RegistryDeploySuites,RainDeploySuitesBaseandCreditHyperCore, plus the README sections on deploying and on crediting the deployer's HyperCore account. The network assertion readsfoundry.tomlrather thansupportedNetworks(), so its expected value is config the implementation does not produce; the dependency assertion reads the two contracts' own documented constructor behaviour rather than the declaration it checks.src/abstract/RegistryDeploySuites.sol,script/Deploy.solandscript/CreditHyperCore.sol; all three are covered, with the two unfillable rows named and explained below rather than dropped.Suite script:
forge testminus the 61 fork tests that need RPC env vars absenton this machine.
check-nopin.shis not used and is not needed here: nothing inthis unit is compiled into
AddressRegistryorMigrationRegistry, so nomutation of it moves a pinned bytecode and the pin tests carry real behaviour
for it. Baseline before: 397 passed, 0 failed. Baseline after: 399 passed, 0
failed.
Mutations were applied one at a time to the unmutated tree and the whole suite
run against each.
src/abstract/RegistryDeploySuites.solreleasedSuitesreturns the generated aggregatereleasedSuitesreturns the WHOLE aggregatecandidateSuitesdeclares exactly 2candidateSuitesdeclares exactly 2address-registrytype(AddressRegistry).creationCodeaddress(0)bytes32(0)migration-registrytype(MigrationRegistry).creationCodescript/Deploy.soldeployNetworksstayssupportedNetworks()releasedSuitesandcandidateSuitesareoverridewithoutvirtual, so an override inDeploydoes not compilescript/CreditHyperCore.solAll three mutations survive, before and after. They are left surviving rather
than filled, because the contract's own NatSpec argues that filling them is the
worse trade and this PR agrees with it: driving
run()means writingDEPLOYMENT_KEY,vm.setEnvwrites the forge PROCESS environment, forge runstest contracts concurrently, and
RainDeployBroadcastTestalready documentsthat every
vm.setEnvin this repo is one of its own four sequenced writes --after that contract was seen losing exactly this race. A fifth writer is a race
against a suite that is currently green. The three are reported rather than
filled, and are in the worker notes as a hazard.
DEPLOYMENT_KEYHYPERCORE_CREDIT_WEIfor itvm.envOrwith a zero defaultLibHyperCore.creditCoreinstead🤖 Generated with Claude Code
https://claude.ai/code/session_01V8ViHcKLVk2YoS2joH4HdN