Skip to content

Commit

Permalink
Revert OpenZeppelin#3950 fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@ernestognw
ernestognw committed Jul 27, 2023
1 parent 9777b62 commit aa0bb6f
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 60 deletions.
28 changes: 26 additions & 2 deletions contracts/mocks/InitializableMock.sol
View file
Expand Up @@ -25,6 +25,10 @@ contract InitializableMock is Initializable {
onlyInitializingRan = true;
}

function initializerNested() public initializer {
initialize();
}

function onlyInitializingNested() public initializer {
initializeOnlyInitializing();
}
Expand Down Expand Up @@ -60,6 +64,18 @@ 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;

Expand Down Expand Up @@ -93,14 +109,22 @@ contract ReinitializerMock is Initializable {
}
}

contract DisableOk is Initializable {
contract DisableNew is Initializable {
constructor() {
_disableInitializers();
}
}

contract DisableBad is Initializable {
contract DisableOld is Initializable {
constructor() initializer {}
}

contract DisableBad1 is DisableNew, DisableOld {}

contract DisableBad2 is Initializable {
constructor() initializer {
_disableInitializers();
}
}

contract DisableOk is DisableOld, DisableNew {}
77 changes: 25 additions & 52 deletions contracts/proxy/utils/Initializable.sol
View file
Expand Up @@ -102,9 +102,22 @@ abstract contract Initializable {
* Emits an {Initialized} event.
*/
modifier initializer() {
bool isTopLevelCall = _beforeInitialize(1);
// solhint-disable-next-line var-name-mixedcase
InitializableStorage storage $ = _getInitializableStorage();

bool isTopLevelCall = !$._initializing;
if (!(isTopLevelCall && $._initialized < 1) && !(address(this).code.length == 0 && $._initialized == 1)) {
revert AlreadyInitialized();
}
$._initialized = 1;
if (isTopLevelCall) {
$._initializing = true;
}
_;
_afterInitialize(isTopLevelCall, 1);
if (isTopLevelCall) {
$._initializing = false;
emit Initialized(1);
}
}

/**
Expand All @@ -126,9 +139,17 @@ abstract contract Initializable {
* Emits an {Initialized} event.
*/
modifier reinitializer(uint64 version) {
bool isTopLevelCall = _beforeInitialize(version);
// solhint-disable-next-line var-name-mixedcase
InitializableStorage storage $ = _getInitializableStorage();

if ($._initializing || $._initialized >= version) {
revert AlreadyInitialized();
}
$._initialized = version;
$._initializing = true;
_;
_afterInitialize(isTopLevelCall, version);
$._initializing = false;
emit Initialized(version);
}

/**
Expand Down Expand Up @@ -186,52 +207,4 @@ abstract contract Initializable {
$.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);
}
}
}
27 changes: 21 additions & 6 deletions test/proxy/utils/Initializable.test.js
View file
Expand Up @@ -4,9 +4,11 @@ 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 DisableBad = artifacts.require('DisableBad');
const DisableBad1 = artifacts.require('DisableBad1');
const DisableBad2 = artifacts.require('DisableBad2');
const DisableOk = artifacts.require('DisableOk');

contract('Initializable', function () {
Expand Down Expand Up @@ -44,6 +46,10 @@ 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);
Expand All @@ -61,6 +67,13 @@ 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();
Expand Down Expand Up @@ -192,13 +205,15 @@ contract('Initializable', function () {
});

describe('disabling initialization', function () {
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 bad sequence', async function () {
await expectRevertCustomError(DisableBad1.new(), 'AlreadyInitialized', []);
await expectRevertCustomError(DisableBad2.new(), 'AlreadyInitialized', []);
});

it('reverts disabling initializers and using the initializer modifier', async function () {
await expectRevertCustomError(DisableBad.new(), 'AlreadyInitialized', []);
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: '18446744073709551615' }); // MAX_UINT64
});
});
});

0 comments on commit aa0bb6f

Please sign in to comment.