Simplify Initializable

CHANGELOG.md

@@ -17,6 +17,11 @@
  * `ERC721`, `ERC1155`: simplified revert reasons. ([#3254](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3254), ([#3438](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3438)))
  * `ERC721`: removed redundant require statement. ([#3434](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3434))
  * `PaymentSplitter`: add `releasable` getters. ([#3350](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3350))
+ * `Initializable`: refactored implementation of modifiers for easier understanding. ([#3450](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3450))
+
+### Breaking changes
+
+ * `Initializable`: functions decorated with the modifier `reinitializer(1)` may no longer invoke each other.
 
 ## 4.6.0 (2022-04-26)
 

contracts/mocks/InitializableMock.sol

@@ -100,3 +100,21 @@ contract ReinitializerMock is Initializable {
         counter++;
     }
 }
+
+contract Disable1 is Initializable {
+    constructor() {
+        _disableInitializers();
+    }
+}
+
+contract Disable2 is Initializable {
+    constructor() initializer {}
+}
+
+contract Disable3 is Disable1, Disable2 {}
+
+contract Disable4 is Initializable {
+    constructor() initializer {
+        _disableInitializers();
+    }
+}

contracts/proxy/utils/Initializable.sol

@@ -76,7 +76,12 @@ abstract contract Initializable {
      * `onlyInitializing` functions can be used to initialize parent contracts. Equivalent to `reinitializer(1)`.
      */
     modifier initializer() {
-        bool isTopLevelCall = _setInitializedVersion(1);
+        bool isTopLevelCall = !_initializing;
+        require(
+            (isTopLevelCall && _initialized < 1) || (!Address.isContract(address(this)) && _initialized == 1),
+            "Initializable: contract is already initialized"
+        );
+        _initialized = 1;
         if (isTopLevelCall) {
             _initializing = true;
         }
@@ -100,15 +105,12 @@ abstract contract Initializable {
      * a contract, executing them in the right order is up to the developer or operator.
      */
     modifier reinitializer(uint8 version) {
-        bool isTopLevelCall = _setInitializedVersion(version);
-        if (isTopLevelCall) {
-            _initializing = true;
-        }
+        require(!_initializing && _initialized < version, "Initializable: contract is already initialized");
+        _initialized = version;
+        _initializing = true;
         _;
-        if (isTopLevelCall) {
-            _initializing = false;
-            emit Initialized(version);
-        }
+        _initializing = false;
+        emit Initialized(version);
     }
 
     /**
@@ -127,27 +129,8 @@ abstract contract Initializable {
      * through proxies.
      */
     function _disableInitializers() internal virtual {
-        _setInitializedVersion(type(uint8).max);
-    }
-
-    function _setInitializedVersion(uint8 version) private returns (bool) {
-        // If the contract is initializing we ignore whether _initialized is set in order to support multiple
-        // inheritance patterns, but we only do this in the context of a constructor, and for the lowest level
-        // of initializers, because in other contexts the contract may have been reentered.
-
-        bool isTopLevelCall = !_initializing; // cache sload
-        uint8 currentVersion = _initialized; // cache sload
-
-        require(
-            (isTopLevelCall && version > currentVersion) || // not nested with increasing version or
-                (!Address.isContract(address(this)) && (version == 1 || version == type(uint8).max)), // contract being constructed
-            "Initializable: contract is already initialized"
-        );
-
-        if (isTopLevelCall) {
-            _initialized = version;
-        }
-
-        return isTopLevelCall;
+        require(!_initializing, "Initializable: contract is initializing");
+        _initialized = type(uint8).max;
+        emit Initialized(type(uint8).max);
     }
 }

test/proxy/utils/Initializable.test.js

@@ -6,6 +6,8 @@ const ConstructorInitializableMock = artifacts.require('ConstructorInitializable
 const ChildConstructorInitializableMock = artifacts.require('ChildConstructorInitializableMock');
 const ReinitializerMock = artifacts.require('ReinitializerMock');
 const SampleChild = artifacts.require('SampleChild');
+const Disable3 = artifacts.require('Disable3');
+const Disable4 = artifacts.require('Disable4');
 
 contract('Initializable', function (accounts) {
   describe('basic testing without inheritance', function () {
@@ -184,4 +186,11 @@ contract('Initializable', function (accounts) {
       expect(await this.contract.child()).to.be.bignumber.equal(child);
     });
   });
+
+  describe('disabling initialization', function () {
+    it('old and new patterns dont interfere', async function () {
+      await expectRevert(Disable3.new(), 'Initializable: contract is already initialized');
+      await expectRevert(Disable4.new(), 'Initializable: contract is initializing');
+    });
+  });
 });