fix(invariants): zero is a checked case, never a skipped one - #301
fix(invariants): zero is a checked case, never a skipped one#301thedavidmeister wants to merge 1 commit into
Conversation
Two surfaces treated address(0) as a case to route around instead of the single most likely drift value to catch: - testPinsMatchDerivedAddressesOnceHydrated guarded each pin equality with 'if not zero' — hydration-lifecycle machinery that is now spent (all three deploys executed) and inverted into a hole: zeroing a pin made the test silently skip. Renamed testPinsMatchDerivedAddresses; the three equalities are now unconditional. A future chain's placeholder phase gets its own guarded branch when its arm is added; these three never go back. - LibMigrationInvariant's address overload accepted actual == post, so on any chain in the unhydrated window (post pin == 0) an owner renounced to address(0) read as 'already migrated' and passed until the deadline. The overload now rejects a zero actual unconditionally, on either side of the deadline: for every address-valued surface it asserts (owners, role holders) zero is the default/renounced/unset reading, so it is its own checked case and an unhydrated post means 'post-state unreachable', never 'post-state equals whatever reads as unset'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 33 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
|
Folded into the stack instead: cherry-picked onto #285's branch, where this work line lives. |
1 participant
What
Ruling: avoiding the zero case skips the single most common value we need to check —
address(0)is Solidity's default-everything reading (renounced ownership, unset storage, failed lookup), so everyif (x != address(0))guard around an assertion turns the most likely drift value into the one value never checked. Swept every not-zero guard on the invariant surfaces:testPinsMatchDerivedAddressesOnceHydrated→testPinsMatchDerivedAddresses. The three per-chain pin equalities were guarded for the hydration window; all three deploys have executed, so the guards had inverted into a hole — zeroing a pin (exactly what the optimizer incident once did) made the test silently skip. The equalities are now unconditional; a future chain's placeholder phase gets its own guarded branch when its arm is added.LibMigrationInvariant.assertMigration(address overload) rejects a zeroactualunconditionally. Previouslyactual == postwas accepted — so on any chain in the unhydrated window (postpin = 0), a vault or beacon owner renounced toaddress(0)read as "already migrated" and passed until the deadline. Zero now tripsMigrationStateDrifton either side of the deadline: an unhydratedpostmeans "post-state unreachable", never "post-state equals whatever reads as unset".Guards that stay, because their zero case IS explicitly handled rather than skipped:
TIMELOCK_CANCELLER(genuine placeholder; while zero the Safe's canceller grant is asserted unconditionally), the deploy script's pin cross-check (bootstrap step — zero is the pre-hydration state, handled with an explicitPIN OUTSTANDINGlog), and the migration script'sTimelockNotPinned(refuses zero outright — the model pattern).QA
testAddressZeroActualAlwaysRejected— fails on the base lib (verified bygit checkout origin/main -- src/lib/LibMigrationInvariant.soland re-running: base yieldsMigrationDeadlinePassedat the deadline leg instead of unconditional drift, and would silently accept the unhydrated-post leg);testPinsMatchDerivedAddresses— fails on any zeroed pin (verified by mutating the Base pin toaddress(0):0x0 != 0xdb4b2187…assertion failure observed, then restored green).STOX_GOVERNANCE_TIMELOCK→address(0)→ killed bytestPinsMatchDerivedAddresses; zero-rejection branch removed (lib reverted to origin/main) → killed bytestAddressZeroActualAlwaysRejected. Both kills observed directly, both restorations re-verified green.TIMELOCK_CREATION_CODEand each chain's Safe — independent of the pinned literals under test; the primitive's expected reverts come from its stated acceptance semantics, with zero defined as the default/renounced reading rather than a sanctioned side.🤖 Generated with Claude Code