OpenZeppelin / openzeppelin-upgrades

Plugins for Hardhat and Foundry to deploy and manage upgradeable contracts on Ethereum.
MIT License
618 stars 262 forks source link

Let users ask for a new ProxyAdmin to be created or detect when ownership of it was transferred #136

Open mverzilli opened 4 years ago

mverzilli commented 4 years ago

I don't want to be prescriptive with this, so I'll do my best to explain the use case instead of suggesting solutions as in the title :).

  1. I deploy an upgradeable contract (C1) using deployProxy.
  2. I then transferOwnership of the ProxyAdmin to a multisig (let's call it M1).
  3. I now deploy another contract (C2) using deployProxy.
  4. I want another multisig (M2) to control this contract, but the underlying ProxyAdmin is now owned by M1.

So what I'd need in that case is to deploy C2 using a brand new ProxyAdmin that the deployer account controls. In practice, I solved the issue by deleting the .openzeppelin folder, which made the plugin create a new ProxyAdmin for me. But it's a hacky solution.

It'd be nice to be able to ask for a new ProxyAdmin to be created, for the plugin to detect that I'm no longer owner of the current one and/or to get a warning that I'm using a ProxyAdmin that I no longer control.

frangio commented 4 years ago

Interesting!

In practice, I solved the issue by deleting the .openzeppelin folder

That's definitely quite extreme, because you're also losing the C1 deployment which you may want to reuse later. A more specific workaround would be to open the network file and delete the admin field so that a new one is deployed. (Though I understand why you wouldn't want to manually make that edit.)


There's several ways to approach this.

  1. Letting the user "forget" the current admin (an admin.forget() function).
  2. Managing multiple admins at the same time, letting the user request a new one.
  3. Not reusing an admin that is not owned by an available account?

I feel that in some (most?) scenarios you would want to reuse the admin even if you don't own it, and in that case 3 is out of the question.

Managing multiple admins is a feature we've thought about before, but at this point I don't know if the use case would be common enough, and it feels like a significant change that I want us to consider a bit more carefully.

Adding a forget function to the admin module would be very simple to implement and it has the potential to be very useful, so I think it makes a lot of sense to implement.

mverzilli commented 4 years ago

I feel that in some (most?) scenarios you would want to reuse the admin even if you don't own it, and in that case 3 is out of the question.

Agree! In my (short) experience working with OZ CLI and now the Buidler plugin, I find that there is a lot of effort put in concealing the upgrades contract architecture that powers the whole thing. I understand where this design decision comes from, it's a good thing to want to abstract users away from the underlying implementation model.

But I think in this case, being in the developer tools space, it makes it harder for you to come up with your own solutions when you're not exactly in the common use scenario (as is here).

I know our docs explain how upgrades work, but maybe there's opportunities to have our tools educate users on it organically and unobtrusively? Something simple that comes to mind as an example: we could print to console the address of the ProxyAdmin used for a command we use.

frangio commented 4 years ago

we could print to console the address of the ProxyAdmin used for a command we use

We are purposely avoiding printing anything in the plugins because they are library functions and they don't "own" the console in that way. There is a debug mode that prints debug logs, but I've been thinking maybe we should allow users to opt in to a simpler kind of logging.

I agree with everything you said, but I think it was better for the initial release to focus on solving the common use case through a simple API. We are probably going to slowly go towards more transparency and control for the user. The most immediate thing that comes to mind that could start to make the contract architecture more obvious would be https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/85.

arijoon commented 1 year ago

An additional use case is having multiple user envs (e.g. lets say dev and staging) within the same network but with different accounts controlling them and therefore requiring multiple ProxyAdmin requirements. In this scenario we would still want implementations shared to minimise deployments but not ProxyAdmin.

An interesting idea would be to key the proxy under default and allow overrides, so I can provide PROXY_ADMIN_NAME=dev and then manifest is checked for dev entry, if one does exist it'll assume no proxy exists and deploys one. If no env var is set then default is used (again use existing one or deploy a new one)

this would be less instrusive as it makes no assumptions to the current user deploying and only requires a migration of existing format to an extra level of nesting (can simply check if admin key exists and migrate it to under default)

This will allow substantial user control for the admins and enable additional admins without losing existing functionality. Main part to refactor will be any place that fetches admin address now needs to respect this new env var (I think its just one function)

I don't mind making a PR if you're happy with this flow

ericglau commented 1 year ago

Partially fixed by https://github.com/OpenZeppelin/openzeppelin-upgrades/pull/859 to allow using proxies with different admin addresses than what is in the network manifest.

The remaining part is the ability to deploy different proxy admins, and this is planned to be addressed in OpenZeppelin Contracts 5.0 which will automatically deploy a separate proxy admin for each transparent proxy.