Openzeppelin-contracts: Minor: declare the constructor of contract Ownable internal instead of public

[barakman]

🧐 Motivation
This doesn't really fall under the category of a feature-request, nor does it fall under the category of a bug-report. The motivation is to be a little more pedantic with the object-oriented implementation of contract Ownable . That said, it might have been proposed here before...

📝 Details
It makes very little sense to deploy this contract as is, so its constructor can and should be declared internal instead of public. True, it would make testing it slightly harder, as you'd need to add a helper contract inheriting from it (you could probably name it OwnableProxy or OwnableAdapter).
Nevertheless, it would emphasize to all users that this contract has no purpose other than being part of an extended contract.

[nventuro]

We may need to better define what a feature request and an improvement are :)

I think this makes a lot of sense, and I like the idea of guiding users into the usage we intend. This same thing applies to some other contracts, such as Pausable, or PullPayment.

[barakman]

@nventuro:
So any chance of pushing it in before the official release of 2.0? (after passing it through your review process of course).

P.S.: I wouldn't call it a breaking change, since doubtfully anyone has ever deployed a standalone instance of Ownable for anything other than testing (unless you consider that a break).

[nventuro]

Technically it's a breaking change of the API, though a minor one :stuck_out_tongue:

We're currently in the process of reviewing the last minor changes, it is possible this will make it into 2.0, since the scope is so small.

[nventuro]

After thinking about this some more, it is definitely something we want to include in the release. Off the top of my head, the contracts that should have this sort of thing are:

• All roles (e.g. MinterRole, PauserRole, etc.)
• SignatureBouncer
• ERC165
• Pausable
• Ownable and Secondary
• PullPayment
• ReentrancyGuard

Some of these have no constructor: maybe we should give them an empty internal one?

[barakman]

@nventuro:
That's exactly what I do in every contract of this sort that we have in our system, so I'm a natural supporter of this approach (empty internal constructor).
AFAIK, this is also the conventional method in other OO languages, such as C++ (empty protected constructor in that case).

I have mentioned only the Ownable contract, because that's the only OpenZeppelin contract of this type that we use (well, except for the Claimable contract, but it's been removed in the upcoming release, right?).

[nventuro]

Claimable was indeed removed, yes. And you're right regarding this being conventional in other languages too :)