Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ISemver and abstract contract, and define semver in a struct #13748

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

maurelian
Copy link
Contributor

@maurelian maurelian commented Jan 14, 2025

This changes how contract level semver values are defined, which unlocks an improvement to how the input X to reinitializer(X) is defined.

Context

During Solidity development, developers need a clear way to determine the appropriate argument for the reinitializer modifier,
ie. How should they decide what is the correct value for X?

function upgrade() reinitializer(X)

The previous plan of defining a new Releases.sol as either an abstract contract or library had the flaw that changing the
release version would modify the bytecode of all contracts
, thus forcing all implementations to be redeployed for each upgrade. This unnecessarily increases the state diff of each upgrade. Our update approach instead leverages the already existing per-contract semver versioning system.

Before outlining this new approach, we first recap the goals:

  1. Provide devs with a straightforward method for determining the reinitializer value
  2. Maintain contract-level semantic versioning. Ie. we want to avoid a breaking change here.
  3. Making clear which contracts are 'production ready'. This is currently done by appending the -beta.n tag to contract semvers.

Solution

Approach

We propose to replace ISemver with an abstract Semver.

Details:

  1. Instead of using a global shared reinitializer value, each contract's semantic version number
    will be converted into a reinitializer value. This is done by converting the major, minor and
    patch numbers into a single uint64 value:
 function reinitializerValue() public pure returns (uint64) {
      Versions memory v = _version();
      return (v.major * SPACING ** 2) + (v.minor * SPACING) + v.patch;
 }

Then the initialize() and upgrade() methods of a contract will simply have reinitializer(reinitializerValue()).

  1. We don't want to generate the reinitializer argument by parsing strings, therefore we will store the semver values as integers in a struct rather than as string. Thus instead of each contract defining string constant version = "1.2.3", each contract will need to define an internal function:

     /// @notice Semantic version.
     /// @custom:semver 1.1.0
     function _version() internal pure override returns (Versions memory) {
         return Versions(1, 1, 0);
     }
  2. In order to maintain the the existing interface (function version() external view returns (string memory)), the
    version() function moves into Semver, and converts the struct values to a string:

    function version() external view returns (string memory) {
         Versions memory v = _version();
         return string.concat(
             LibString.toString(v.major), ".", LibString.toString(v.minor), ".", LibString.toString(v.patch)
         );
     }

Consequences for releases

  1. Recall that in future upgrades, the upgrade() function will only be required when new storage values are being
    added. The previous plan had been to remove these functions after the upgrade was completed. However, because we only want to redeploy contracts when their bytecode changes, we will not remove the upgrade() functions until some other change
    necessitates an increase of the semver value. Automation will be created to identify this situation.
  2. Rather than applying -beta tags to clarify when a contract is not production ready, the OPCM contract and superchain-registry
    will combine to become the source of truth for what is included in a release, by including the addresses of the implementations of released contracts. Importantly, implementations are now deterministically deployed, so that they are not
    replaced if their bytecode is unchanged. Taking the OptimismMintableErc20Factory as an example, while every OPCM will contain
    an address optimismMintableErc20FactoryImpl, that address will only change if its bytecode has been modified.
  3. As a consequence of the previous point, for unchanged contracts, opcm.upgrade() will not update the value at the Proxy.IMPLEMENTATION_SLOT, thus minimizing the state diff of an upgrade.
  4. Perhaps most importantly: When L2 contracts are finally upgraded, they will also need to take these changes into account
    and ensure that the reinitializerValue is set properly.

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 31.70732% with 56 lines in your changes missing coverage. Please review.

Project coverage is 46.84%. Comparing base (49ea3d0) to head (fde400d).

Files with missing lines Patch % Lines
...ckages/contracts-bedrock/src/L2/L1BlockInterop.sol 0.00% 4 Missing ⚠️
...ontracts-bedrock/src/L1/L1CrossDomainMessenger.sol 0.00% 2 Missing ⚠️
...ckages/contracts-bedrock/src/L1/L1ERC721Bridge.sol 0.00% 2 Missing ⚠️
...ages/contracts-bedrock/src/L1/L1StandardBridge.sol 0.00% 2 Missing ⚠️
...es/contracts-bedrock/src/L1/OPContractsManager.sol 0.00% 2 Missing ⚠️
...ages/contracts-bedrock/src/L1/ProtocolVersions.sol 0.00% 2 Missing ⚠️
...ages/contracts-bedrock/src/L1/SuperchainConfig.sol 0.00% 2 Missing ⚠️
packages/contracts-bedrock/src/L2/BaseFeeVault.sol 0.00% 2 Missing ⚠️
packages/contracts-bedrock/src/L2/CrossL2Inbox.sol 0.00% 2 Missing ⚠️
packages/contracts-bedrock/src/L2/ETHLiquidity.sol 0.00% 2 Missing ⚠️
... and 17 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13748      +/-   ##
===========================================
- Coverage    47.07%   46.84%   -0.23%     
===========================================
  Files          961      964       +3     
  Lines        80429    80493      +64     
  Branches       753      753              
===========================================
- Hits         37858    37706     -152     
- Misses       39664    39916     +252     
+ Partials      2907     2871      -36     
Flag Coverage Δ
cannon-go-tests-32 62.26% <ø> (ø)
cannon-go-tests-64 57.35% <ø> (ø)
contracts-bedrock-tests 87.98% <31.70%> (-3.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ontracts-bedrock/scripts/checks/interfaces/main.go 47.80% <ø> (ø)
...racts-bedrock/src/L1/DataAvailabilityChallenge.sol 100.00% <100.00%> (ø)
...kages/contracts-bedrock/src/L1/OptimismPortal2.sol 96.80% <100.00%> (ø)
...contracts-bedrock/src/L1/OptimismPortalInterop.sol 100.00% <100.00%> (ø)
packages/contracts-bedrock/src/L1/SystemConfig.sol 100.00% <100.00%> (ø)
...s/contracts-bedrock/src/L1/SystemConfigInterop.sol 100.00% <100.00%> (ø)
...ages/contracts-bedrock/src/L2/L2StandardBridge.sol 94.28% <100.00%> (ø)
...ntracts-bedrock/src/L2/L2StandardBridgeInterop.sol 100.00% <100.00%> (ø)
...ackages/contracts-bedrock/src/universal/Semver.sol 100.00% <100.00%> (ø)
...ontracts-bedrock/src/L1/L1CrossDomainMessenger.sol 90.47% <0.00%> (-9.53%) ⬇️
... and 26 more

... and 30 files with indirect coverage changes

@maurelian maurelian marked this pull request as ready for review January 14, 2025 14:58
@maurelian maurelian requested a review from a team as a code owner January 14, 2025 14:58
@maurelian maurelian requested a review from mds1 January 14, 2025 14:58
@maurelian maurelian changed the title Demo of struct based semver Make ISemver and abstract contract, and define semver in a struct Jan 14, 2025
@tynes
Copy link
Contributor

tynes commented Jan 14, 2025

Why did you make these changes on L2? We aren't doing a release of the L2 contracts based on this, we should reduce the diff in this PR and not modify any L2 contracts. We can integrate these changes into the standard L2 genesis project.

I don't love reinitializerValue as a function name because it is verbose. Generally think its ok as an internal function.

Inlining the struct makes it nice and human readable.

@tynes
Copy link
Contributor

tynes commented Jan 14, 2025

This is a case in which enforcing codecov at a very high % for merge will bite us

@maurelian
Copy link
Contributor Author

Why did you make these changes on L2?

We put them in a separate commit so that we could easily handle such a comment. The main challenge will be that the semver check script needed to be refactored for the struct based approach.

This will require an L1 and L2 semver check, but we can probably deal with that.

I don’t love reinitializerValue as a function name because it is verbose. Generally think its ok as an internal function.

We can come up with a shorter name.

The purpose for making it public is to expose the uint64 value which is encoded in the bytecode. We are also adding a public function which will expose the _initialized value in storage (getInitializedValue()).
This will then enable us to do an onchain comparison getInitializedValue() == reinitializerValue() to verify that the contract cannot be reinit’d.

@zhiqiangxu
Copy link
Contributor

zhiqiangxu commented Jan 15, 2025

The STYLE_GUIDE.md says reinitializer is not used:

image

Is the style changed after this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants