feat(L1): add ProtocolVersions upgrade schedule contract#353
Open
PelleKrab wants to merge 19 commits into
Open
feat(L1): add ProtocolVersions upgrade schedule contract#353PelleKrab wants to merge 19 commits into
PelleKrab wants to merge 19 commits into
Conversation
Add the Security Council-controlled upgrade activation schedule contract, its interface, and tests. Maintains an ordered registry of upgrades and their L2 activation timestamps, committed via scheduleId. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…o conventions Replace the history-dependent rolling hash with a per-upgrade cumulative hash chain reproducible from (l2ChainId, address, ordered keys, current timestamps). When a timestamp changes, only the affected suffix is recomputed (O(n-j) bubble-up) rather than hashing the full state on every mutation. - Add MIN_NOTICE (1 hour) guard on setTimestamp; expose it in the interface - Require non-zero protocolVersion; use _protocolVersions as existence sentinel, removing the separate _upgradeIndex mapping - Move all errors and events into IProtocolVersions; add MIN_NOTICE to interface - Replace assembly in _keyFromUpgradeId with bytes32(raw) Solidity cast - Rename constructor params to leading-underscore convention - Restructure test file into grouped contracts with snake_case naming, external visibility, and @notice on each test function per repo style Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Returns the full ordered upgrade schedule in a single eth_call — name, activation timestamp, protocol version, and cumulative scheduleId per entry. Name is recovered from the bytes32 key at read time via _nameFromKey, avoiding a separate storage mapping. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Replace the per-call keccak256(abi.encode(l2ChainId, address(this))) in _refreshScheduleId with a cached immutable _seed set once in the constructor, saving a hash + encode on every schedule mutation. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
🟡 Heimdall Review Status
|
Reflects bytecode change from caching hash chain seed as immutable. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
jackchuma
reviewed
Jul 6, 2026
jackchuma
left a comment
Contributor
There was a problem hiding this comment.
I think this is a great start, however ProtocolVersions seems a bit over-engineered. How simple can we make it? We really only need:
- mapping of arbitrary upgradeId
bytes32=> upgrade timestamp - We could probably also include another mapping to represent the upgrade schedule as a linked list instead of an array which will simplify lookups (gnosis safes are implemented this way)
- mapping of upgradeId
bytes32=> scheduleIdbytes32
…s control - Replace Ownable with ProxyAdminOwnedBase; drop explicit _owner constructor param and ProtocolVersions_ZeroOwner error - Replace per-upgrade protocolVersion mapping with a single latestProtocolVersion uint256 updated on each registerUpgrade call - Remove redundant storage vars: lastUpdatedAtBlock, l2ChainId immutable, _scheduleId (scheduleId() now reads the tip of _upgradeScheduleId directly) - Remove redundant view functions: upgradeCount, upgradeIdAt, upgradeIds, getProtocolVersion (getSchedule() covers all registry reads) - Inline _registerUpgrade into registerUpgrade and _applyTimestamp into setTimestamp; inline onlyChainTeam guard into delayTimestamp - Drop ScheduleIdUpdated old-scheduleId param; enforce MIN_NOTICE in delayTimestamp Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
…y pattern Replace the string-based upgrade registry (bytes32 key from packed upgradeId) with a purely numeric scheme: each upgrade gets an ascending uint256 id equal to its registration index. Human-readable names are kept off-chain; the id is permanent and stable because the registry is strictly append-only. Convert the constructor to an initializer so the contract can be deployed behind an OP proxy. _seed is now stored (not immutable) so it can be set inside initialize() against the proxy's address(this), binding the schedule commitment to the proxy address that callers interact with. Replace the mapping-based storage (_registered, _timestamps, _upgradeScheduleId keyed by bytes32) with parallel arrays indexed by id, simplifying the hash chain and eliminating the three-mapping structure. Drop getTimestamp(string) and the duplicate-registration guard; add setLatestProtocolVersion() so the informational field can be updated independently of registrations. Add LatestProtocolVersionUpdated and ScheduleIdUpdated events; update the interface to inherit IProxyAdminOwnedBase and IReinitializableBase. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Seven findings from a multi-angle review; fixes applied: * emit LatestProtocolVersionUpdated in registerUpgrade — previously only setLatestProtocolVersion fired the event, so indexers missed every version bump that came through registration. * Guard scheduleId() before initialization — returns bytes32(0) while _seed is unset, which could be picked up as a valid schedule commitment; now reverts with ProtocolVersions_NotInitialized. * Dedicate ProtocolVersions_InsufficientNotice for setTimestamp — the MIN_NOTICE check in setTimestamp reused DelayMustBeLater with current=0 as the floor, which was misleading; a new error carries only the offending timestamp. * Split delayTimestamp compound condition — the combined || condition reported current as the floor even when only the MIN_NOTICE leg fired; each constraint now reverts independently with the correct floor value. * Extract _setTimestamp internal — setTimestamp and delayTimestamp duplicated the activation-already-passed check and the write+emit+refresh triple; _setTimestamp holds those shared invariants and each public function keeps only its distinct preconditions. * Drop redundant zero-write in registerUpgrade — push(bytes32(0)) wrote zero to a slot that _refreshScheduleId immediately overwrites; replaced with push() (no argument) to save one SSTORE. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
3405a31 to
358db8c
Compare
…elper, init guard Rename latestProtocolVersion → minimumProtocolVersion throughout (state variable, event, setter, interface, tests). The field represents the minimum version nodes must run, not an informational latest; the old name implied recommendation rather than a floor. Remove the protocolVersion parameter from registerUpgrade. Upgrade registration is purely a scheduling concern (assign id, extend hash chain); the minimum version is set independently via setMinimumProtocolVersion. Drops the InvalidProtocolVersion check from registerUpgrade and removes the protocolVersion field from the UpgradeRegistered event. Extract _writeTimestamp(id, newTs) as the shared write helper for setTimestamp and delayTimestamp. Each public function owns its complete validation sequence; _writeTimestamp is a pure write (store + emit TimestampSet + refreshScheduleId). Eliminates the mixed-responsibility _setTimestamp that owned some validation but not all, which caused the ActivationAlreadyPassed check to appear in two places. Guard registerUpgrade against pre-initialization calls. _seed is zero between upgradeTo and initialize; any registerUpgrade call in that window would compute hash chain links from a zero seed, permanently desynchronizing scheduleId from its reproducible derivation (l2ChainId, address(this)). Revert with NotInitialized when _seed == 0, consistent with the existing guard in scheduleId(). Regenerate ABI, storage layout, and semver-lock snapshots. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
358db8c to
c034032
Compare
Co-authored-by: Codex <codex-noreply@coinbase.com>
jackchuma
reviewed
Jul 7, 2026
| // Cannot delay an activation that has already passed. | ||
| if (uint64(block.timestamp) >= current) revert ProtocolVersions_ActivationAlreadyPassed(id, current); | ||
| // The role can only push the activation later, never to the same time or earlier. | ||
| if (newTimestamp <= current) revert ProtocolVersions_DelayMustBeLater(current, newTimestamp); |
Contributor
There was a problem hiding this comment.
I'm debating if we need this. Since the following if statement is enforcing a min notice, maybe it's ok to allow newTimestamp to be <= current. What do you think?
Author
There was a problem hiding this comment.
We should be good to remove it from a technical standpoint. Although I think it is best to keep it, as SC should be the only one who moves the timestamps back based on prev. discussions about what chain team should be able to control.
Deploy the ProtocolVersions impl + per-chain proxy as part of SystemDeploy and initialize it via initialize(l2ChainId), rather than excluding it from initializer tracking. Adds it to Types.Implementations/DeployOutput, artifact saves, getImplementations, and _assertValidImplementations. Tags the contract @Custom:proxied true so the initializer test verifies both impl and proxy, and wires it into Setup (via getAddress for fork safety). Updates the ProtocolVersions sourceCodeHash in semver-lock accordingly. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Codex <codex-noreply@coinbase.com>
Co-authored-by: Codex <codex-noreply@coinbase.com>
Co-authored-by: Codex <codex-noreply@coinbase.com>
3fbdf54 to
2257727
Compare
5 tasks
jackchuma
reviewed
Jul 9, 2026
0570814 to
7cb1c5d
Compare
…no-op setTimestamp Apply @jackchuma review nits on ProtocolVersions: - Rename the chainTeam role to incidentResponder; delaying a bad hardfork activation is an incident-response action. NatSpec scopes the power so the shared name doesn't imply SuperchainConfig's pause authority. - Drop the redundant chainTeam deploy config key and source the role from superchainConfigIncidentResponder (same entity). - setTimestamp: short-circuit no-op writes ahead of the change-validation guards so idempotent resubmissions succeed. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
7cb1c5d to
e1ebc49
Compare
Run ProtocolVersions tests through CommonTest so they use the SystemDeploy-created proxy and implementation. Reorder ProtocolVersions members to match the Solidity style guide and restrict implementation internals to private. Co-authored-by: Codex <codex-noreply@coinbase.com>
jackchuma
reviewed
Jul 10, 2026
Events before errors, and mutating externals before view getters, mirroring the ProtocolVersions contract body and the repo style-guide ordering. ABI is generator-sorted, so no snapshot or semver-lock impact. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
PelleKrab
added a commit
to base/base
that referenced
this pull request
Jul 10, 2026
Replace the per-upgrade IUpgradeSignal reads (getTimestamp/getProtocolVersion by name) with the new ProtocolVersions interface from base/contracts#353: - read the full id-ordered schedule with one getSchedule() call and the global minimumProtocolVersion() in a single pinned L1 block read - map schedule entries onto the node hardfork ladder in activation (timestamp) order: id 0 (lowest timestamp) aligns with the oldest contract-backed hardfork; entries newer than the known ladder are logged and ignored, and unregistered hardforks produce no signal - attach the global minimum protocol version to every mapped signal so the existing validation and sink pipeline is unchanged - bump the supported node protocol version from 7 to packed semver 1.1.0 - drop the now-impossible TimestampOverflow error (timestamps are uint64 onchain)
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.
Summary
Adds
ProtocolVersions, a Security Council-controlled L1 contract thatmaintains an ordered registry of L2 upgrade activation timestamps and
commits to a reproducible
scheduleIdhash chain."canyon") with anowner-assigned protocol version; timestamps are set/cleared separately
by the owner
that can only push already-scheduled activations further into the future
via
delayTimestamp, it cannot register, clear, or pull timestamps earlier(l2ChainId, address, ordered keys, current timestamps).eth_callview returning the full ordered schedule(name, timestamp, protocol version, per-entry hash) for off-chain consumers
Test plan
forge test --match-path test/L1/ProtocolVersions.t.solpassesscheduleIdreproducibility manually: compute chain from(l2ChainId, address, ordered keys, timestamps)and compare toscheduleId()return valuedelayTimestampcannot be used to pull an activation earlieror to schedule a fresh one
setTimestamp(id, 0)correctly clears a pending timestamp andreverts the
scheduleIdto the post-registration value