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

Allow the re-initialization of contracts #3232

Merged
merged 20 commits into from Mar 22, 2022

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 1, 2022

Fixes #2901

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx changed the title allow re-initialization of contracts Allow the re-initialization of contracts Mar 1, 2022
@frangio
Copy link
Contributor

frangio commented Mar 4, 2022

This is a sensitive piece of code. It needs comments explaining in depth what behavior it implements and why.

We also need to develop and document the alternative to constructor() initializer {}.

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 5, 2022

We also need to develop and document the alternative to constructor() initializer {}.

Should we add a lockImplementation modifier that could be used for constructors?

@frangio
Copy link
Contributor

frangio commented Mar 7, 2022

lockImplementation sounds too specific for upgrades. It also doesn't need to be a modifier, does it? What about something like this:

constructor() {
  _preventInitialize();
}

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 7, 2022

How should this function behave? Should it check that the contract is not yet initialized? Should it be allowed to run within the scope of an initializer?

Here are possible implementation

    function _preventInitialize() internal virtual {
        require(!_initializing && _initialized == 0, "Initializable: contract initializing of already initialized");
        _initialized = type(uint8).max;
        _initializing = false;
    }

    function _preventInitialize() internal virtual {
         if (_setInitializeStep(type(uint8).max)) {
            _initializing = false;
        }
    }

@frangio
Copy link
Contributor

frangio commented Mar 8, 2022

I think we want the second option:

    function _preventInitialize() internal virtual {
        if (_setInitializeStep(type(uint8).max)) {
            _initializing = false;
        }
    }

The behavior that results is that after this function is called no more initializer or reinitializer functions will be callable. As a result, onlyInitializing functions will not be callable either.

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 9, 2022

not, it will be incorrect to write

constructor() initializer {
    _preventInitialize();
}

frangio
frangio previously approved these changes Mar 22, 2022
contracts/proxy/utils/Initializable.sol Show resolved Hide resolved
@frangio frangio dismissed their stale review March 22, 2022 06:32

meant to comment

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you!

@Amxx Amxx enabled auto-merge (squash) March 22, 2022 17:46
@Amxx Amxx merged commit 0eba511 into OpenZeppelin:master Mar 22, 2022
@Amxx Amxx deleted the feature/reinitialize branch March 22, 2022 19:34
@Amxx Amxx mentioned this pull request Mar 25, 2022
3 tasks
nkuba added a commit to keep-network/keep-core that referenced this pull request Apr 20, 2022
We use RC as it has a funcionality of `reinitializer` which supports
upgrades of the contracts from OpenZeppelin/openzeppelin-contracts#3232

We need to revisit this before mainnet deployment and switch to a final
package release if available.
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.

Initializer must be reusable for upgrade Initializers
2 participants