Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

[playground] Upgradeable authenticator: split proxy ownership and authenticator ownership #379

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Jan 27, 2021

Findings of a discussion with @bh2smith. This PR is not supposed to be merged nor reviewed.

The base PR #351 makes the authenticator contract upgradeable with a proxy.
This PR removes OwnableUpgradeable from the contract and separates the ownership of the proxy (owned by owned) from the ownership of the authenticator (previously called owner, which conflicted, now called manager). The "managership" of the authenticator is set as part of the proxy deployment. This makes the "proxiness" of the authenticator implicit and not explicit, since it's only evident from the deployment scripts.

The next task would be to write the authenticator contract so that it "inherits" the owner of the proxy. The function setting the manager should be restricted (maybe only called once?). Then I'd test that the upgrading changes the manager and has the right owner.
I didn't investigate on who owns the proxy, however. I suppose the deployer?

Test Plan

Note that the end to end tests are passing, showing that the proxy contract is deployed correctly and the owner-manager is being set correctly in the deployment.

@bh2smith
Copy link
Contributor

This looks really good as a starting point so I am going to merge this into the existing branch. I would like to make use of

import "@openzeppelin/contracts-upgradeable/proxy/Initializable.sol";

Somehow in order to make the setManager method idempotent. Ideally this can also be called at deployment time to set the manager once and only once.

@bh2smith bh2smith marked this pull request as ready for review January 27, 2021 11:02
@bh2smith bh2smith merged commit 8ed0a12 into 167/upgradable_authenticator Jan 27, 2021
@bh2smith bh2smith deleted the 167/upgradable_authenticator-split-ownership-playground branch January 27, 2021 11:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants