fix: report what the run did - #25
Merged
Merged
Conversation
`forge script` runs the script body as a SIMULATION and dispatches the recorded
transactions only after it returns. Two mechanisms turned that gap into a false
report. The first is closed at its root; the second is detected where it does
harm, because the write itself cannot be moved out of the simulation.
Scripts printed a checkmark from the body. Every write primitive ended with a
line like "Pool set successfully!", emitted before any transaction existed, so
a broadcast that then failed left the claim on screen with the operation
undone - observed when UpdateRateLimiters announced success and died on a stale
nonce with the old rate still live. Nothing under script/ or src/ read anything
back, and no read-back inside the body could: a value re-read after
stopBroadcast is still the simulation's. So the line now reports what the run
actually did. OutcomeLog owns the three wordings and selects between them from
what the executor did and whether forge will send at all: SENDING (unconfirmed)
when the calls reached forge, NOT SENT (simulation only) for a dry run, NOT
SENT (Safe batch to sign) when a batch was written for the owners. The deploy
scripts broadcast directly and never inherit the executor, so the dry-run check
lives in the library where they cannot bypass it. Confirmation is a separate
read: make doctor, make roles-check, or the matching Get* script.
The store recorded the simulation. A deploy writes project/<chain>.json from
that same body, so a broadcast that failed afterwards left an address recorded
that no chain knows, and the redeploy guard then refused the retry - naming a
contract that was never deployed. The guard now distinguishes the two: an
address the chain confirms keeps the original refusal, one holding no code gets
a message naming both possible causes, because absent code reads the same for a
phantom record as for a stale or wrong RPC. It refuses rather than dropping the
entry itself; guessing wrong deletes the record of a deployment that exists.
make doctor gained the matching rung, code-checking every deployments{} entry
rather than only the two active pointers, so a phantom lock box or hooks
contract is named instead of passing clean.
Both readers in RegistryWriter also had parseJsonString inside a try's success
block, where Solidity does not route the revert to that catch, so a hand-edited
store aborted the doctor before its verdict. They are now one tolerant read at a
JSON path, which both are two lines over.
Three decisions were each written out twice, and the point of this change is that
the copies cannot diverge. Whether forge will send anything decided both what the
store may record and what the run may claim, written out separately in each;
ForgeContext now owns it, so a disagreement cannot say "recorded" and "not sent"
about the same run. _isEoaMode moves next to _isSafeMode, and the two doctor
rungs that walk deployments{} share one tolerant key read, so an unreadable store
reaches the verdict as an unverified check instead of a silent skip.
No line printed from a script body can report a landed transaction; forge's
exit code remains the authority on the send. A balance is not evidence that this run
moved it, and v1 fee config lives in the FeeQuoter where no pool getter reaches
it, so neither gets a verdict. Nor does code presence prove that this run
deployed the contract it found - only a broadcast receipt would.
|
👋 aelmanaa, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
PR 25 routed the project store through one question - will forge actually send? - but left three writers asking their own. A dry run still wrote the history/ ledger, still dropped registry entries under FORCE_REDEPLOY, and still told the operator the address was registered. The ledger recorded simulations. DeploymentRecorder guarded its five history/ writes against forge test only, so a dry run left a timestamped file naming an address no chain had seen, indistinguishable from a real deployment. Both halves of the recorder now ask ForgeContext._sendsNothing(), so the ledger and the store cannot disagree about whether a deployment happened. FORCE_REDEPLOY on a dry run destroyed records. The guard runs on a dry run by design - the refusal is worth previewing - but the forced branch dropped the deployments entry and, through it, the active pointer naming the same address, while record() no-opped because nothing was sent. The entry was deleted and never replaced: data loss from a command run to preview. Dropping now requires the run to be one that can also record; otherwise it reports what it would replace and leaves the store alone. project/ is gitignored, so this was silent - the troubleshooting page names it and how to re-record with adopt-token. Four scripts claimed the address was registered whatever the run did, then printed a paste-ready export of it, and the lock box printed a --broadcast command carrying it. One helper now prints either the resolution block or why there is none, and no copy-paste address when the contract does not exist. The regression net is a real dry run against anvil in test-tooling.sh, because no in-process test can reach these branches: _sendsNothing() answers the same under forge test as on a dry run. That is why both defects shipped - the suite had never run a deploy script without --broadcast. It skips where anvil is absent.
aelmanaa
force-pushed
the
fix/record-only-deployed
branch
from
August 19, 2026 20:00
db14c86 to
dc5fd46
Compare
SyedAsadKazmi
approved these changes
Aug 20, 2026
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.
This pull request introduces comprehensive improvements to the deployment verification process, focusing on robust detection and reporting of "phantom" or codeless deployment records. It also enhances documentation to clarify script output and the lifecycle of deployment records, and updates test tooling to cover new scenarios. The main themes are: improved detection of incomplete or failed deployments, clearer user guidance in documentation, and expanded test coverage.
Deployment Verification and Error Detection
doctorandVerifyChain.s.solscripts now check every recorded artifact address (not just active token/pool) for deployed code, and report a[FAIL]if any recorded address has no code on-chain—catching "phantom" deployments left by failed or simulated runs. This includes robust handling of unreadable or malformed project stores. [1] [2]_deploymentKeysand_failCodelessDeploymentsto modularize and harden the logic for reading deployment records and reporting issues, including proper handling of unreadable stores and forkless runs.checkRegistryAndExtrasForTest,failCodelessDeploymentsForTest) to assert correct behavior in tests, especially regarding codeless artifact detection.Documentation and User Guidance
script-output.md, detailing the meaning of script outcome lines (SENDING,NOT SENT (simulation only),NOT SENT (Safe batch to sign)) and emphasizing that only reading state back confirms a deployment. Linked this in multiple places across the docs. [1] [2] [3] [4]make doctorfailures. [1] [2] [3] [4] [5]Test Tooling Enhancements
test-tooling.sh) to include a dedicated dry-run chain and clean up its artifacts, supporting new test scenarios around simulated deployments and phantom records. [1] [2]Supporting Code Changes
keysOfonChainProbeto allow tolerant reading of deployment keys in the presence of malformed or partial project stores.These changes collectively make the deployment process more robust, transparent, and user-friendly, ensuring that failed or simulated deployments are correctly identified and documented, and that users have clear guidance on interpreting outcomes and resolving issues.