Skip to content
This repository has been archived by the owner on Oct 6, 2023. It is now read-only.

Closes AP-727, AP-791: EndowmentMultiSig proxy admin update & update EndowmentMultiSigFactory's registrar on new Registrar deployment #377

Merged
merged 56 commits into from
Sep 20, 2023

Conversation

0xNeshi
Copy link
Contributor

@0xNeshi 0xNeshi commented Sep 13, 2023

Explanation of the solution

  • Noticed we have a missing EndowmentMultiSigFactory's registrar update on new Registrar deployment - addressed this
  • EndowmentMultiSig proxy admin update on calling EndowmentMultiSigFactory.updateProxyAdmin
  • smaller refactors to EndowmentMultiSigFactory

Instructions on making this work

  • run yarn or yarn install to install npm dependencies

@0xNeshi 0xNeshi added bug Something isn't working enhancement New feature or request WIP labels Sep 13, 2023
@0xNeshi 0xNeshi self-assigned this Sep 13, 2023
@0xNeshi 0xNeshi changed the title [WIP]: Closes AP-727, AP-791: EndowmentMultiSig proxy admin update & update EndowmentMultiSigFactory's registrar on new Registrar deployment Closes AP-727, AP-791: EndowmentMultiSig proxy admin update & update EndowmentMultiSigFactory's registrar on new Registrar deployment Sep 14, 2023
@0xNeshi 0xNeshi removed the WIP label Sep 14, 2023

/// @notice Get all EndowmentMultiSig proxy contract instantiations.
/// @return Array of instantiation addresses.
function getInstantiations() external view returns (address[] memory) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this to be able to update all of the instantiations' proxy admins

proxyAdmin = _proxyAdmin;
emit ProxyAdminUpdated(_proxyAdmin);
constructor(address _implementationAddress, address _proxyAdmin, address registrarAddress) {
updateImplementation(_implementationAddress);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructor behavior was the same as if these functions were called one-by-one

yes: boolean;
};

task("manage:changeProxyAdmin", "Will update the proxy admin for all proxy contracts")
task("manage:changeProxyAdmin", "Will update the proxy admin the target proxy contract")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the purpose of this task to focus on changing proxy admin for a single proxy contract.
See changeProxyAdminForAll task for a task doing this for all proxy contracts

Copy link
Contributor

@stevieraykatz stevieraykatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved tho see my longer comments in the Solidity Crew chat about:

  1. Whether this needs to be a proxy
  2. Generally how we approach deciding what's worth changing for prod-deployed contracts

@0xNeshi
Copy link
Contributor Author

0xNeshi commented Sep 15, 2023

@stevieraykatz let me know which changes you think need to be reverted and will do on Monday

@stevieraykatz
Copy link
Contributor

stevieraykatz commented Sep 15, 2023

@stevieraykatz let me know which changes you think need to be reverted and will do on Monday

re: EndowmentMultiSigFactory
It looks like we've already deployed a number of endowments using the existing version of the Factory contract. Do we need this state for any reason? We'd "lose" it in this upgrade. If we need this state at all then this is a bigger conversation and we should put a hold on this PR until we determine what we need.

If we don't need this state, then I think all of the changes to EndowmentMultiSigFactory are good AND we should deploy it behind a proxy so that it's easier to upgrade in the future if we need to.

re: import removal
We can keep this change but shouldn't proactively upgrade affected contracts. It makes no functional change to the contracts affected so having both versions in the wild (the outcome of leaving this change in) is negligible.

re: errors removal
If we weren't ever emitting this error in any contracts that use IterableMappingAddr then the logic to leave this change in is the same as the unused import. We don't need to proactively upgrade inheriting contracts and it won't cause any "out of sync" issues between contracts expecting different versions.

@0xNeshi
Copy link
Contributor Author

0xNeshi commented Sep 18, 2023

re #377 (comment):

re: import removal
...
re: errors removal

Will add those back then

@0xNeshi 0xNeshi merged commit 3ba7eb4 into master Sep 20, 2023
1 check passed
@0xNeshi 0xNeshi deleted the ap-727 branch September 20, 2023 05:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants