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

Simplify Initializable #3450

Merged
merged 12 commits into from
Jun 3, 2022
Merged

Conversation

frangio
Copy link
Contributor

@frangio frangio commented Jun 2, 2022

In #3344 we introduced changes to Initializable in order to enable the use of _disableInitializers() in the constructor of multiple parents:

contract A {
    constructor() { _disableInitializers(); }
}
contract B {
    constructor() { _disableInitializers(); }
}
contract C is A, B {
    constructor() { _disableInitializers(); }
}

However, the resulting code was difficult to reason about, and it introduced a bug when mixing _disableInitializers and the old pattern constructor() initializer {}.

We sat down to rethink it and trim it down to more straightforward code.

This PR slightly changes how functions with the initializer and reinitializer modifiers can be nested in the context of the constructor.

  • initializer functions work the same as 4.6: they can be nested in this context. This is present for backwards compatibility with versions prior to 4.4.1 and we may remove it in 5.0.
  • reinitializer functions can't be nested. If one is invoked in the context of another, it will revert. This is a small breaking change for reinitializer(1). We don't think anyone will be affected by this change given that reinitializers were released only 1 month ago and the broken pattern was not a recommended one. It allows us to make the code much simpler.

@frangio frangio marked this pull request as ready for review June 2, 2022 19:38
@frangio frangio requested a review from Amxx June 2, 2022 19:38
@frangio frangio added this to the 4.7 milestone Jun 2, 2022
frangio and others added 2 commits June 3, 2022 12:14
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@frangio frangio requested a review from Amxx June 3, 2022 19:19
@Amxx Amxx enabled auto-merge (squash) June 3, 2022 19:24
@Amxx Amxx merged commit d506e3b into OpenZeppelin:master Jun 3, 2022
@frangio frangio deleted the simplify-initializable branch June 6, 2022 14:27
tynes added a commit to ethereum-optimism/optimism that referenced this pull request Jul 28, 2022
Standardize on the upgradable initializable since the contracts
are technically upgradable it is more clear. There are no real
implementation differences between the upgradable and standard
initializable implementations. `OwnableUpgradable` is `initializable`
imported from the upgradable package, and the `L2OutputOracle`
inherits from `OwnableUpgradable`. This means that the only
way to standardize on a single implementation of `Initializable`
is to use the upgradable version.

This also bumps the version of the openzeppelin contracts
dependency because they refactored the initializable implementation
and made it easier to understand.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v4.7.0
OpenZeppelin/openzeppelin-contracts#3450
mergify bot pushed a commit to ethereum-optimism/optimism that referenced this pull request Jul 28, 2022
* contracts-bedrock: fix build

Standardize on the upgradable initializable since the contracts
are technically upgradable it is more clear. There are no real
implementation differences between the upgradable and standard
initializable implementations. `OwnableUpgradable` is `initializable`
imported from the upgradable package, and the `L2OutputOracle`
inherits from `OwnableUpgradable`. This means that the only
way to standardize on a single implementation of `Initializable`
is to use the upgradable version.

This also bumps the version of the openzeppelin contracts
dependency because they refactored the initializable implementation
and made it easier to understand.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v4.7.0
OpenZeppelin/openzeppelin-contracts#3450

* op-bindings: regenerate
maurelian pushed a commit to ethereum-optimism/optimism that referenced this pull request Sep 15, 2022
* contracts-bedrock: fix build

Standardize on the upgradable initializable since the contracts
are technically upgradable it is more clear. There are no real
implementation differences between the upgradable and standard
initializable implementations. `OwnableUpgradable` is `initializable`
imported from the upgradable package, and the `L2OutputOracle`
inherits from `OwnableUpgradable`. This means that the only
way to standardize on a single implementation of `Initializable`
is to use the upgradable version.

This also bumps the version of the openzeppelin contracts
dependency because they refactored the initializable implementation
and made it easier to understand.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v4.7.0
OpenZeppelin/openzeppelin-contracts#3450

* op-bindings: regenerate
sam-goldman pushed a commit to ethereum-optimism/optimism that referenced this pull request Sep 15, 2022
* contracts-bedrock: fix build

Standardize on the upgradable initializable since the contracts
are technically upgradable it is more clear. There are no real
implementation differences between the upgradable and standard
initializable implementations. `OwnableUpgradable` is `initializable`
imported from the upgradable package, and the `L2OutputOracle`
inherits from `OwnableUpgradable`. This means that the only
way to standardize on a single implementation of `Initializable`
is to use the upgradable version.

This also bumps the version of the openzeppelin contracts
dependency because they refactored the initializable implementation
and made it easier to understand.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/releases/tag/v4.7.0
OpenZeppelin/openzeppelin-contracts#3450

* op-bindings: regenerate
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.

2 participants