Allow Initializable versions greater than 256

.changeset/thick-pumpkins-exercise.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`Initializable`: Allow versions greater than 256.

contracts/mocks/InitializableMock.sol

@@ -25,10 +25,6 @@ contract InitializableMock is Initializable {
         onlyInitializingRan = true;
     }
 
-    function initializerNested() public initializer {
-        initialize();
-    }
-
     function onlyInitializingNested() public initializer {
         initializeOnlyInitializing();
     }
@@ -64,38 +60,26 @@ contract ConstructorInitializableMock is Initializable {
     }
 }
 
-contract ChildConstructorInitializableMock is ConstructorInitializableMock {
-    bool public childInitializerRan;
-
-    constructor() initializer {
-        childInitialize();
-    }
-
-    function childInitialize() public initializer {
-        childInitializerRan = true;
-    }
-}
-
 contract ReinitializerMock is Initializable {
     uint256 public counter;
 
-    function getInitializedVersion() public view returns (uint8) {
+    function getInitializedVersion() public view returns (uint64) {
         return _getInitializedVersion();
     }
 
     function initialize() public initializer {
         doStuff();
     }
 
-    function reinitialize(uint8 i) public reinitializer(i) {
+    function reinitialize(uint64 i) public reinitializer(i) {
         doStuff();
     }
 
-    function nestedReinitialize(uint8 i, uint8 j) public reinitializer(i) {
+    function nestedReinitialize(uint64 i, uint64 j) public reinitializer(i) {
         reinitialize(j);
     }
 
-    function chainReinitialize(uint8 i, uint8 j) public {
+    function chainReinitialize(uint64 i, uint64 j) public {
         reinitialize(i);
         reinitialize(j);
     }
@@ -109,22 +93,14 @@ contract ReinitializerMock is Initializable {
     }
 }
 
-contract DisableNew is Initializable {
+contract DisableOk is Initializable {
     constructor() {
         _disableInitializers();
     }
 }
 
-contract DisableOld is Initializable {
-    constructor() initializer {}
-}
-
-contract DisableBad1 is DisableNew, DisableOld {}
-
-contract DisableBad2 is Initializable {
+contract DisableBad is Initializable {
     constructor() initializer {
         _disableInitializers();
     }
 }
-
-contract DisableOk is DisableOld, DisableNew {}

contracts/proxy/utils/Initializable.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: MIT
 // OpenZeppelin Contracts (last updated v4.9.0) (proxy/utils/Initializable.sol)
 
-pragma solidity ^0.8.19;
+pragma solidity ^0.8.20;
 
 /**
  * @dev This is a base contract to aid in writing upgradeable contracts, or any kind of contract that will be deployed
@@ -55,14 +55,27 @@ pragma solidity ^0.8.19;
  */
 abstract contract Initializable {
     /**
-     * @dev Indicates that the contract has been initialized.
+     * @dev Storage of the initializable contract.
+     *
+     * It's implemented on a custom ERC-7201 namespace to reduce the risk of storage collisions
+     * when using with upgradeable contracts.
+     *
+     * @custom:storage-location erc7201:openzeppelin.storage.Initializable
      */
-    uint8 private _initialized;
+    struct InitializableStorage {
+        /**
+         * @dev Indicates that the contract has been initialized.
+         */
+        uint64 _initialized;
+        /**
+         * @dev Indicates that the contract is in the process of being initialized.
+         */
+        bool _initializing;
+    }
 
-    /**
-     * @dev Indicates that the contract is in the process of being initialized.
-     */
-    bool private _initializing;
+    // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1))
+    bytes32 private constant _INITIALIZABLE_STORAGE =
+        0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a0e;
 
     /**
      * @dev The contract is already initialized.
@@ -77,7 +90,7 @@ abstract contract Initializable {
     /**
      * @dev Triggered when the contract has been initialized or reinitialized.
      */
-    event Initialized(uint8 version);
+    event Initialized(uint64 version);
 
     /**
      * @dev A modifier that defines a protected initializer function that can be invoked at most once. In its scope,
@@ -89,19 +102,9 @@ abstract contract Initializable {
      * Emits an {Initialized} event.
      */
     modifier initializer() {
-        bool isTopLevelCall = !_initializing;
-        if (!(isTopLevelCall && _initialized < 1) && !(address(this).code.length == 0 && _initialized == 1)) {
-            revert AlreadyInitialized();
-        }
-        _initialized = 1;
-        if (isTopLevelCall) {
-            _initializing = true;
-        }
+        bool isTopLevelCall = _beforeInitialize(1);
         _;
-        if (isTopLevelCall) {
-            _initializing = false;
-            emit Initialized(1);
-        }
+        _afterInitialize(isTopLevelCall, 1);
     }
 
     /**
@@ -122,23 +125,18 @@ abstract contract Initializable {
      *
      * Emits an {Initialized} event.
      */
-    modifier reinitializer(uint8 version) {
-        if (_initializing || _initialized >= version) {
-            revert AlreadyInitialized();
-        }
-        _initialized = version;
-        _initializing = true;
+    modifier reinitializer(uint64 version) {
+        bool isTopLevelCall = _beforeInitialize(version);
         _;
-        _initializing = false;
-        emit Initialized(version);
+        _afterInitialize(isTopLevelCall, version);
     }
 
     /**
      * @dev Modifier to protect an initialization function so that it can only be invoked by functions with the
      * {initializer} and {reinitializer} modifiers, directly or indirectly.
      */
     modifier onlyInitializing() {
-        if (!_initializing) {
+        if (!_isInitializing()) {
             revert NotInitializing();
         }
         _;
@@ -153,26 +151,87 @@ abstract contract Initializable {
      * Emits an {Initialized} event the first time it is successfully executed.
      */
     function _disableInitializers() internal virtual {
-        if (_initializing) {
+        // solhint-disable-next-line var-name-mixedcase
+        InitializableStorage storage $ = _getInitializableStorage();
+
+        if ($._initializing) {
             revert AlreadyInitialized();
         }
-        if (_initialized != type(uint8).max) {
-            _initialized = type(uint8).max;
-            emit Initialized(type(uint8).max);
+        if ($._initialized != type(uint64).max) {
+            $._initialized = type(uint64).max;
+            emit Initialized(type(uint64).max);
         }
     }
 
     /**
      * @dev Returns the highest version that has been initialized. See {reinitializer}.
      */
-    function _getInitializedVersion() internal view returns (uint8) {
-        return _initialized;
+    function _getInitializedVersion() internal view returns (uint64) {
+        return _getInitializableStorage()._initialized;
     }
 
     /**
      * @dev Returns `true` if the contract is currently initializing. See {onlyInitializing}.
      */
     function _isInitializing() internal view returns (bool) {
-        return _initializing;
+        return _getInitializableStorage()._initializing;
+    }
+
+    /**
+     * @dev Returns a pointer to the storage namespace.
+     */
+    // solhint-disable-next-line var-name-mixedcase
+    function _getInitializableStorage() private pure returns (InitializableStorage storage $) {
+        assembly {
+            $.slot := _INITIALIZABLE_STORAGE
+        }
+    }
+
+    /**
+     * @dev Sets the initialized version.
+     *
+     * Requirements:
+     *
+     * - If the contract is initializing the version set must not be that of an reinitializer.
+     * - If the contract is not initializing the version must not be already set.
+     */
+    function _setInitializedVersion(uint64 version) private returns (bool) {
+        // solhint-disable-next-line var-name-mixedcase
+        InitializableStorage storage $ = _getInitializableStorage();
+
+        bool initializing = $._initializing;
+
+        // When it's initializing check if it's a reinitializer, otherwise check if
+        // the intended version is not already set.
+        if (initializing ? version > 1 : version <= $._initialized) {
+            revert AlreadyInitialized();
+        }
+
+        $._initialized = version;
+
+        return !initializing;
+    }
+
+    /**
+     * @dev Runs before initialization.
+     * It sets the initialized version and sets the initializing flag to true.
+     */
+    function _beforeInitialize(uint64 version) private returns (bool) {
+        bool isTopLevelCall = _setInitializedVersion(version);
+        if (isTopLevelCall) {
+            _getInitializableStorage()._initializing = true;
+        }
+        return isTopLevelCall;
+    }
+
+    /**
+     * @dev Runs after initialization.
+     * It clears the initializing flag and emits an {Initialized} event if it is a top level call.
+     */
+    function _afterInitialize(bool isTopLevelCall, uint64 version) private {
+        if (isTopLevelCall) {
+            _getInitializableStorage()._initializing = false;
+            emit Initialized(version);
+        }
     }
 }

test/proxy/utils/Initializable.test.js

@@ -4,11 +4,9 @@ const { expectRevertCustomError } = require('../../helpers/customError');
 
 const InitializableMock = artifacts.require('InitializableMock');
 const ConstructorInitializableMock = artifacts.require('ConstructorInitializableMock');
-const ChildConstructorInitializableMock = artifacts.require('ChildConstructorInitializableMock');
 const ReinitializerMock = artifacts.require('ReinitializerMock');
 const SampleChild = artifacts.require('SampleChild');
-const DisableBad1 = artifacts.require('DisableBad1');
-const DisableBad2 = artifacts.require('DisableBad2');
+const DisableBad = artifacts.require('DisableBad');
 const DisableOk = artifacts.require('DisableOk');
 
 contract('Initializable', function () {
@@ -46,10 +44,6 @@ contract('Initializable', function () {
     });
 
     describe('nested under an initializer', function () {
-      it('initializer modifier reverts', async function () {
-        await expectRevertCustomError(this.contract.initializerNested(), 'AlreadyInitialized', []);
-      });
-
       it('onlyInitializing modifier succeeds', async function () {
         await this.contract.onlyInitializingNested();
         expect(await this.contract.onlyInitializingRan()).to.equal(true);
@@ -67,13 +61,6 @@ contract('Initializable', function () {
     expect(await contract2.onlyInitializingRan()).to.equal(true);
   });
 
-  it('multiple constructor levels can be initializers', async function () {
-    const contract2 = await ChildConstructorInitializableMock.new();
-    expect(await contract2.initializerRan()).to.equal(true);
-    expect(await contract2.childInitializerRan()).to.equal(true);
-    expect(await contract2.onlyInitializingRan()).to.equal(true);
-  });
-
   describe('reinitialization', function () {
     beforeEach('deploying', async function () {
       this.contract = await ReinitializerMock.new();
@@ -205,15 +192,13 @@ contract('Initializable', function () {
   });
 
   describe('disabling initialization', function () {
-    it('old and new patterns in bad sequence', async function () {
-      await expectRevertCustomError(DisableBad1.new(), 'AlreadyInitialized', []);
-      await expectRevertCustomError(DisableBad2.new(), 'AlreadyInitialized', []);
+    it('disables initializers', async function () {
+      const ok = await DisableOk.new();
+      await expectEvent.inConstruction(ok, 'Initialized', { version: '18446744073709551615' }); // MAX_UINT64
     });
 
-    it('old and new patterns in good sequence', async function () {
-      const ok = await DisableOk.new();
-      await expectEvent.inConstruction(ok, 'Initialized', { version: '1' });
-      await expectEvent.inConstruction(ok, 'Initialized', { version: '255' });
+    it('reverts disabling initializers and using the initializer modifier', async function () {
+      await expectRevertCustomError(DisableBad.new(), 'AlreadyInitialized', []);
     });
   });
 });