Mark visibility of initialize(...) functions as external

[CJ42]

This topic has probably been discussed already in the past, and reached agreement. But I believe it is worth discussing again since 4.4.1 release.

Background

The initialize(...) function of the upgradeable contracts is marked public.

Someone asked already in the OpenZeppelin forum why this function was not marked public. The initial intention as mentioned by @frangio was:

We use public because it can be useful to call the function internally if you extend it via inheritance.

Suggested changes

Since the 4.4.1 release and the introduction of the onlyInitializing modifier, it feels like the initialize(...) function might fit better with the external visibility. The reasons and my arguments for this would be:

• If someone wants to extend via inheritance, it might make more sense that the overridden initialize(...) function calls the internal {...}_init function, not the parent public initialize(...) function.
• external instead of public would give more the sense of the initialize(...) functions to behave like a constructor (only called on deployment, so should only be called externally.
• I believe from a security point of view, it might be safer so that it cannot be called internally by accident in the child contract (lack of insights here, so would be interested by opinions).
• it might cost a bit less gas to use external over public (but this is not the main argument).

My main argument for changing the initialize(...) function to external would be about the issue of overriding function visibility (see screenshots below). With Solidity

• it is possible to override a function from external to public (= "opening it up") ✅
• but it is not possible to override a function from public to external (= "narrow it down"). ❌

So people who want to override the function to lock it down cannot currently with the public visibility.

TLDR;

What are the reasons for keeping the initialize(...) function as public overall?
Why not marking it as external to prevent the issue of visibility overriding showed in the screenshots below?

I would be interested to hear your opinions on this suggested change. @Amxx @frangio (anyone else from OZ team)

Screenshots

[frangio]

I think I agree with your reasoning. However, this is a breaking change (removes internal availability of the function) so we will want to do it for 5.0.

[balajipachai]

@frangio should I open a PR for the same?

[Amxx]

If we make the function external, it will no longer be possible to call super. when overriding. I'm not sure why we would want that.

[frangio]

In Contracts 5.0 the contracts that had initialize() functions (presets) were removed. So this issue doesn't have any actionables unless we want to change our documentation to recommend using external rather than public. I don't think there are particularly strong reasons to favor one or the other so I will close this.