Use _disableInitializers() instead of constructor() intializer
frangio opened this issue · 6 comments
OpenZeppelin Contracts 4.7 introduced reinitializers and with it _disableInitializers()
which is now prefered instead of the pattern constructor() initializer
. Forta is using the latter, for example here:
There is no issue with keeping this as is, as long as we don't use reinitializers. Since they are a useful feature and we will likely use them in the future, we should try to switch to _disableInitializers
ahead of time to avoid future issues.
Just to know if I understood:
- We should use
_disableInitializers()
in the constructor instead ofconstructor() initializer
to lock implementation contracts - This disables
reinitializer
s in the implementation contract - We can still user
reinitializer
for the proxy contract
Is that correct? I'm thinking we could ditch IVersioned
in favour of using reinitializer
, although the only way of exposing the current version is by events. Maybe
contract SomeContract is Initializable {
uint public constant version = 2;
function firstInitializer() reinitializer(1) {
}
function secondInitializer() reinitializer(version) {
}
}
Yes that's correct!
although the only way of exposing the current version is by events
If you mean the "initialized version", 4.8 adds a function _getInitializedVersion
.
I don't think we should ditch IVersioned
. It could be interesting to find a way to use that version as the argument to the reinitializer
modifier but I think it will be easier and just fine to consider them separate "version" concepts.
IVersioned
is there because I wanted to keep track of the implementation version, to know which ABI to call. So I think it's literally the same concept.
I could keep IVersioned
and have it call _getInitializedVersion
when updating to 4.8, and have an initializer that calls reinitializer(version()+1)
and changes with every version if needed.
I still see them as slightly separate concepts but I'm open to the suggestion.
I think we should move to _disableInitializers
before we update to 4.8 though.
Separate because semver carries more meaning (patch, minor, major)? I think it may be easier to use semver for the whole package, and _version
for the individual contracts. Please let me know how you see them separate, open to suggestions too!
+1 to _disableInitializers
The difference I see is that the initialized version tracks the version of the contract "state" whereas IVersioned
tracks the version of the interface and they can change independently, for example a new interface version may not require bumping the version and initializing new state at all. You could also in theory upgrade the contract and "forget" to run the reinitializer thus leave the state at a past version and the interface at a new version, but this would be an error probably.