Ownable should support setting the initial owner through its constructor

[wighawag]

🧐 Motivation Currently Ownable force you to set the initial owner to be the msg sender. This brings a dependence on the account that deploy the contract, which is often not the same as the expected owner of the contract. For security it is often better to ensure the account making deployment do not have any responsibility.

it also brings issue when you need to deploy the contract through a factory where the factory become the defacto owner of the contract.

While contracts using Ownable could call transferOwnership this would trigger another event.

It would be better if Ownable constructor had a owner parameter so contract using Ownable can decide whose address is going to be the initial owner

[Amxx]

Hello @wighawag

This feature has already been requested multiple times. In a perfect world, solidity would support optional arguments, and we would be able to use the provided value or fallback to initializing using msg.sender. Unfortunately, overloading constructor is not yet a thing.

While the change you ask for is easy to do, and make a lot of sense, it would also be a breaking change. Any contract that uses Ownable right now (and there are many!) would not be compatible with the newer version, and this would require users to change their code. We want to avoid this, and we certainly cannot do such a change in a minor version.

We might possibly consider this for whenever we release contracts 5.0, but until then, calling transferOwnership in the constructor is the way to go. While it is true that there are 2 events, the 2 are consistent with the ownership logic, so they should not be to confusing for an outside observer.

[wighawag]

Hi @Amxx

That's a shame we cannot have it sooner :) I have actually complained secretly about the issue for years :D

I understand the backward compatibility requirement though, thanks for explaning.

My main use case for now is ProxyAdmin that I have to reimplement so I can specify the onwer at deployment time. it would great in version 5.0 that no assumption was made as to the role of msg.sender and that every contract that extends Ownable also allow users to pass the owner in their constructor transitively.

[frangio]

This is a duplicate of https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2402 (closed). See https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2402#issuecomment-780877304, although it has been summarized by @Amxx above.

Is it possible for you to inherit ProxyAdmin and use transferOwnership in the constructor, without reimplementing the entire contract?

[wighawag]

@frangio yes this is possible but feel like a waste (even if small), it would also emit the OwnershipTransfer event twice.

It also prevent me from deploying such proxyAdmin directly through a create2 factory to ensure deterministic deployment

Edit: my last point is not true if I use your suggestion of creating a contract inheriting ProxyAdmin though

[Manik-Jain]

Hi,

Please see PR : https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2647

Suggestions are welcome to improve this

[frangio]

Is this something we should do on a 5.0 release?

Do we have other constructors that use msg.sender, and if so should we change them all to use an argument?

[Amxx]

Its is always possible to call _transferOwnership(admin) in the constructor so I don't think its that critical

But yes, we should consider weither we want to change that for the next major.

[frangio]

Yes, it can be worked around, but we should also see from a security and good defaults angle, like https://github.com/OpenZeppelin/openzeppelin-contracts/issues/3720.

[waynehoover]

Its is always possible to call _transferOwnership(admin) in the constructor so I don't think its that critical

But yes, we should consider weither we want to change that for the next major.

It's not possible to call it in the constructor if you are deploying a proxy contract. Because you can't use a constructor with a proxy.

My use case: deploying a transparent proxy contract through a create2 contract.

[Amxx]

It's not possible to call it in the constructor if you are deploying a proxy contract. Because you can't use a constructor with a proxy.

My use case: deploying a transparent proxy contract through a create2 contract.

In that case, your implementation should have an initializer, that you are going to call from the factory.

Example:

• this factory creates a clone, and calls the initializer
• the initializer is here it could very well include a call to _transferOwnership is this was an ownable contract.

[Amxx]

Fixed in #4267