Use _disableInitializers() instead of constructor() intializer

[frangio]

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:

forta-contracts/contracts/components/staking/FortaStaking.sol

Line 122 in c21e283

constructor(address _forwarder) initializer ForwardedContext(_forwarder) {

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.

[Ramarti]

Just to know if I understood:

• We should use _disableInitializers() in the constructor instead of constructor() initializer to lock implementation contracts
• This disables reinitializers 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) {
     }
}

[frangio]

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.

[Ramarti]

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.

[frangio]

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.

[Ramarti]

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

[frangio]

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.