feat(ai-explainer): structural diff between proxy upgrade implementations#226
Merged
Conversation
…ions
When a governance tx upgrades a proxy, the LLM previously got just a diff URL it
couldn't follow. This adds a structural diff of the verified source between the
old and new implementations:
- Functions added / removed / changed visibility or modifiers (by name + arg types)
- Storage layout safety check (append-only is safe; reorderings or removals are
flagged as upgrade-unsafe). Immutable/constant vars are excluded since they
don't occupy storage slots. EIP-7201 namespaced storage is detected and the
positional check is skipped with a note.
The diff is injected into the existing proxy-upgrade prompt block. Best-effort:
if either source is unverified, the diff section is omitted but the rest of the
upgrade context still renders.
Also extends `detect_proxy_upgrade` to recognize the ProxyAdmin
`upgradeAndCall(address,address,bytes)` pattern (proxy is arg 0, new impl is
arg 1) in addition to the proxy-direct upgradeTo / upgradeToAndCall variants.
Return type is now a `ProxyUpgrade(proxy_address, new_implementation)` dataclass
so callers can read the impl from the correct address even when the tx targets
a separate ProxyAdmin.
Smoke-tested live on the 3JANE sUSD3 upgrade (chain 1, ProxyAdmin pattern):
Old: 0x4F66...0645 (sUSD3)
New: 0x2038...e216 (sUSD3)
Functions added (2):
+ nominalBackingFloor() public view returns (uint256)
+ restartStrategy() external reinitializer(3)
Storage layout: uses EIP-7201 namespaced storage; positional check skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two correctness fixes from external review:
P1 — default-internal state vars now captured (security)
The state-var regex required an explicit visibility modifier, so plain
`uint256 cap;` declarations (which Solidity defaults to internal) were
silently skipped. If an upgrade removed or reordered such vars, the diff
compared incomplete slot lists and could report the upgrade as
append-only/safe even when the actual layout was unsafe.
Fix:
- Visibility is now optional in `_STATE_VAR_RE`.
- Locals like `uint256 x = 1;` inside function bodies must still be
excluded — that's now done structurally via brace-depth tracking.
`_strip_solidity_noise` removes comments and string literals (with
same-length space replacement to preserve byte offsets) so the brace
counter can't trip on `{` inside strings. `_brace_depths` builds a
per-character depth array; only matches at depth 1 (inside a contract
body, outside any function) are kept.
- Also filters out false-positive "types" like `function`, `struct`,
`event`, `error`, `using`, `pragma`, etc.
Regression tests:
- `test_default_internal_state_vars_captured`: `uint256 cap;` is now
captured with visibility="".
- `test_function_locals_not_captured_after_visibility_fix`: locals
(including those in nested `if` blocks) are still excluded.
- `test_struct_members_not_captured`: struct fields at depth 2 are not
treated as state vars.
- `test_removing_default_internal_var_now_detected_as_unsafe`: end-to-end
confirmation that the false-safe verdict on the auditor's example is
fixed.
P2 — ProxyAdmin selector added to local table
`0x9623609d` (upgradeAndCall on OZ ProxyAdmin) was not in KNOWN_SELECTORS,
so detect_proxy_upgrade's decode step depended on the Sourcify 4byte API
being reachable. Offline or rate-limited, detection silently failed.
Fix: added the selector + signature to KNOWN_SELECTORS. The other two
proxy selectors were already local.
Regression test: `test_works_offline_for_all_proxy_selectors` patches
`fetch_json` to raise on call and verifies all three proxy selectors
decode locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
spalen0
added a commit
that referenced
this pull request
May 20, 2026
spalen0
added a commit
that referenced
this pull request
May 20, 2026
Expands the README's "Proxy Upgrade Detection" section to cover the new implementation-diff feature shipped in #226 and #227: - All three recognized upgrade selectors (incl. OZ ProxyAdmin's upgradeAndCall) and the ProxyUpgrade dataclass returned. - Selector short-circuit before calldata decoding (no Sourcify lookup for non-upgrade calls). - Structural diff between old/new impl: function add/remove/changed visibility, storage layout safety with OZ trailing-gap consumption, EIP-7201 namespaced storage detection. - Default-internal state vars now captured (visibility no longer required); function locals excluded via brace-depth tracking. Updates the example assembled prompt to show the new "--- Proxy Upgrade ---" block content (with Old/New impl labels and function/state diff) and adds utils/impl_diff.py to the module-structure listing. Co-Authored-By: Claude Opus 4.7 (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
1 participant
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.
Summary
When a governance tx upgrades a proxy, the LLM previously got just a diff URL it couldn't follow — verdicts amounted to "looks like an upgrade, verify the new impl". This PR fetches the verified source of both implementations and produces a structural diff that goes into the prompt:
Best-effort: if either source is unverified or one of the impl extractors fails, the diff section is silently omitted and the existing proxy-upgrade context still renders.
Proxy detection also extended
The user's motivating example (3JANE `upgradeAndCall(address,address,bytes)`) used the ProxyAdmin pattern, which `detect_proxy_upgrade` didn't recognize. Extended the detector:
Return type changed from `str | None` (new impl only) to `ProxyUpgrade(proxy_address, new_implementation) | None` so callers can resolve the proxy correctly even when the tx targets a separate ProxyAdmin. Updated both call sites (`utils/llm/ai_explainer.py`, `timelock/timelock_alerts.py`).
Live smoke (3JANE sUSD3 upgrade, the tx that motivated this)
vs the original alert which said "Critical logic change. HIGH" with no specifics. Now the LLM can name the new functions and notice the `reinitializer(3)` modifier (upgrade-time init), which is exactly the kind of signal that distinguishes a routine feature add from a real risk.
v1 scope / what it doesn't catch
Documented inline in `utils/impl_diff.py`:
Test plan
Opened as draft as requested — let me know after you read the prompt output and I'll mark ready.
🤖 Generated with Claude Code