Add opcm upgrades spec #468
Conversation
maurelian
requested review from
protolambda,
tynes,
ajsutton,
sebastianst and
mslipper
as code owners
maurelian
force-pushed
the
opcm/upgrades
branch
from
83e8af3 to
115c61e
Compare
Co-authored-by: blaine <[email protected]>
| value to `true`. The `upgrade` function body should be empty unless it is used to set a new state | ||
| variable added to that contract since the last upgrade. | ||
|
|
||
| #### `NewChainConfig` struct |
There was a problem hiding this comment.
I added the following content below:
By way of example, if an upgrade is adding a new variable address foo to the SystemConfig contract, for
an upgrade named Example, the struct could have the following definition:
struct NewChainConfigForExampleUpgrade {
address systemConfigFoo;
}We could create a structure in this document wherein each upgrade is expected to document the specific struct required, but I think this is sufficient instruction for an implementer, and I'm hesitant to add that overhead to each upgrade.
| or more chains. | ||
|
|
||
| ```solidity | ||
| struct NewGameConfig { |
There was a problem hiding this comment.
You should define the game types and the definitions per game type. This spec is not clear enough to enable a second implementation or a reviewer to know that an impl is correct
There was a problem hiding this comment.
Having just learned that the Proofs contracts refactor has been deprioritized, I will need to rewrite this section. Will let you know when that's done.
There was a problem hiding this comment.
This commit should address your comment:
7703c51
| uint32 blobBasefeeScalar; | ||
| uint256 l2ChainId; | ||
| // The correct type is AnchorStateRegistry.StartingAnchorRoot[] memory, | ||
| // but OP Deployer does not yet support structs. |
There was a problem hiding this comment.
support arrays? Roles is a struct
There was a problem hiding this comment.
The limitation is that neither the setters, nor getters on the Input/Output contracts that op-deployer uses be a complex/reference type, because those I/O contracts are simulated as predeploys in go code, and that simulation does not have the ability to handle the more complicated encodings.
That however is an implementation detail specific to the monorepo, which would not be required for another implementer to know, so I've removed these comments which had been copied from the impl source.
I've also removed the monorepo specific type wrappers from the DeployInput and replaced them with their simple type definitions (ie. Duration -> uint64, Claim -> bytes32).
| @@ -379,7 +379,7 @@ Address: | |||
|
|
|||
| **Description:** Account authorized to change values in the SystemConfig contract. All configuration is stored on L1 and | |||
| picked up by L2 as part of the [derivation](./derivation.md) of the L2 chain.<br/> | |||
| **Administrator:** <br/> | |||
| **Administrator:** [L1 Proxy Admin](#admin-roles)<br/> | |||
There was a problem hiding this comment.
noticed this was missing. Other changes in this file are either from linting or my IDE's formatting, but are purely cosmetic.
| /// format `op-contracts/vX.Y.Z`. | ||
| function l1ContractsRelease() external view returns (string memory); | ||
| /// @notice Represents the interface version so consumers know how to decode the DeployOutput struct | ||
| function OUTPUT_VERSION() external view returns (uint256); |
There was a problem hiding this comment.
Why are we mixing snake case and camel case in the interface? We generally agreed on not doing that because it introduces tech debt if we move from something being an immutable to not. This couples the implementation details to the interface
| struct Implementation { | ||
| address logic; // Address containing the deployed logic contract. | ||
| bytes4 initializer; // Function selector for the initializer. | ||
| keccak256(abi.encode(l2ChainId, saltMixer)) |
There was a problem hiding this comment.
The implementation of salts have now been simplied as per this pr: ethereum-optimism/optimism#13273
Now all salts are computed the same way. So both proxied and non-proxied contracts have their salts computed as:
keccak256(abi.encode(_l2ChainId, _saltMixer, _contractName));
Description
upgrade()based on the design doc.Note that I have included the
addGameType()method discussed in the design doc, but serious questions have come up around its feasibility. That content is included for discussion, but can be reverted with this commit: 115c61e