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

Consider simpler initializer modifier #3950

Open
frangio opened this issue Jan 11, 2023 · 4 comments
Open

Consider simpler initializer modifier #3950

frangio opened this issue Jan 11, 2023 · 4 comments
Labels
breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@frangio
Copy link
Contributor

frangio commented Jan 11, 2023

The current version of the initializer modifier includes some logic for backwards compatibility that was added in #3450, which was in fact a simplification of the backwards compatibility code introduced in #3344.

We should consider further simplifications that break backwards compatibility for 5.0.

Haven't thought about this yet but it would probably come down to making initializer exactly equivalent to reinitializer(1). This means that instead of:

require(
(isTopLevelCall && _initialized < 1) || (!Address.isContract(address(this)) && _initialized == 1),
"Initializable: contract is already initialized"
);

the code should look more like:

require(!_initializing && _initialized < version, "Initializable: contract is already initialized");

@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Jan 11, 2023
@frangio frangio added this to the 5.0 milestone Jan 11, 2023
@frangio
Copy link
Contributor Author

frangio commented Feb 23, 2023

Note: In #3344 it is explained that the isConstructor exception was added back (after removal in 4.0) in order to use the initializer modifier to disable the initialization of implementation contracts with the following pattern:

contract A is Initializable {
    constructor() initializer { /* ... */ }
}

contract B is A {
    constructor() initializer { /* ... */ }
}

This is no longer a requirement since the introduction of _disableInitializers.

@balajipachai
Copy link
Contributor

@frangio is this open for a PR or just a suggestion citing Consider?

@frangio
Copy link
Contributor Author

frangio commented Jun 9, 2023

Consider means undecided.

@balajipachai
Copy link
Contributor

balajipachai commented Jun 10, 2023 via email

ernestognw added a commit to ernestognw/openzeppelin-contracts that referenced this issue Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

No branches or pull requests

2 participants