Set ownership of proxy contracts on deployment #210
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## celo7 #210 +/- ##
=======================================
Coverage 61.32% 61.32%
=======================================
Files 20 20
Lines 1753 1753
Branches 71 71
=======================================
Hits 1075 1075
Misses 646 646
Partials 32 32
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
I miss some context here. Do you know why this is done the way it is? For me it seems like the safe is the "better" owner. Can we we maybe upstream this? |
|
hey @palango , I don't know why it's done this way. I would also agree that the safe is the better owner, especially in the case of Celo which doesn't have a well defined security council or ownership policy yet. I don't see why it couldn't be upstreamed |
|
@pahor167 @martinvol I'd like to get your input here as well. |
rkachowski
force-pushed
the
dhutch/proxy_ownership
branch
from
cec1f8b
to
2908dce
Compare
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
| @@ -72,7 +72,6 @@ library ChainAssertions { | |||
| ResourceMetering.ResourceConfig memory resourceConfig = config.resourceConfig(); | |||
|
|
|||
| if (_isProxy) { | |||
| require(config.owner() == _cfg.finalSystemOwner()); | |||
| require(config.basefeeScalar() == _cfg.basefeeScalar()); | |||
There was a problem hiding this comment.
Rather then deleting this check - can we update it so it checks the owner based on config ?
| /// This is expected to be used in conjusting with deployERC1967ProxyWithOwner after setup actions | ||
| /// have been performed on the proxy. | ||
| /// @param _name The name of the proxy to transfer ownership of. | ||
| /// @param _name The name of the proxy to transfer admin of. |
There was a problem hiding this comment.
Any reason why not to also change owner of InitializeDataAvailabilityChallenge ?
There was a problem hiding this comment.
iirc, this function actually changed the admin - and not owner (as described in the comment) - and was updated for the sake of clarity.
|
Closed in favor of #255 |
Sets the ownership of two proxy contracts (SystemConfigProxy and ProtocolVersionsProxy) to the SystemOwnerSafe contract during deployment if the correct config var is set. This removes the deployment account from directly owning L1 contracts, and instead indirectly owns them through multisig membership.
closes https://github.com/celo-org/celo-blockchain-planning/issues/518