Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ownable should support setting the initial owner through its constructor #2639

Closed
wighawag opened this issue Apr 23, 2021 · 11 comments · Fixed by #4267
Closed

Ownable should support setting the initial owner through its constructor #2639

wighawag opened this issue Apr 23, 2021 · 11 comments · Fixed by #4267
Labels
breaking change Changes that break backwards compatibility of the public API.

Comments

@wighawag
Copy link

wighawag commented Apr 23, 2021

🧐 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
Copy link
Collaborator

Amxx commented Apr 23, 2021

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
Copy link
Author

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
Copy link
Contributor

frangio commented Apr 23, 2021

This is a duplicate of #2402 (closed). See #2402 (comment), 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
Copy link
Author

wighawag commented Apr 23, 2021

@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

@frangio frangio added the breaking change Changes that break backwards compatibility of the public API. label Apr 26, 2021
@Manik-Jain
Copy link
Contributor

Hi,

Please see PR : #2647

Suggestions are welcome to improve this

@frangio
Copy link
Contributor

frangio commented Sep 16, 2022

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
Copy link
Collaborator

Amxx commented Sep 21, 2022

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
Copy link
Contributor

frangio commented Sep 23, 2022

Yes, it can be worked around, but we should also see from a security and good defaults angle, like #3720.

@waynehoover
Copy link

waynehoover commented Feb 15, 2023

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
Copy link
Collaborator

Amxx commented Feb 16, 2023

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:

@Amxx
Copy link
Collaborator

Amxx commented Jun 8, 2023

Fixed in #4267

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants