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
Merged

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

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
@linear
Copy link

linear bot commented Sep 13, 2023

AP-791 Update EndowmentMultiSigFactory's registrar address when deploying a new Registrar contract with task `deploy:Registrar`

  • Add a function EndowmentMultiSigFactory.updateRegistrar that updates the registrar address
  • Call the function when running task deploy:Registrar

AP-727 Update all existing EndowmentMultiSig proxy admins on calling `EndowmentMultiSigFactory.updateProxyAdmin`

https://github.com/AngelProtocolFinance/evm-smart-contracts/blob/master/contracts/multisigs/endowment-multisig/EndowmentMultiSigFactory.sol#L74

@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
0xNeshi
0xNeshi commented Sep 14, 2023
View reviewed changes
contracts/multisigs/endowment-multisig/EndowmentMultiSigFactory.sol

/// @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

0xNeshi
0xNeshi commented Sep 14, 2023
View reviewed changes
contracts/multisigs/endowment-multisig/EndowmentMultiSigFactory.sol
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

0xNeshi
0xNeshi commented Sep 14, 2023
View reviewed changes
tasks/manage/changeProxyAdmin.ts
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

stevieraykatz
stevieraykatz approved these changes Sep 14, 2023
View reviewed changes
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

contracts/multisigs/endowment-multisig/EndowmentMultiSigFactory.sol Show resolved Hide resolved
contracts/multisigs/endowment-multisig/EndowmentMultiSigFactory.sol Outdated Show resolved Hide resolved
@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 •
edited
Loading

@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
Update manage:changeProxyAdminForAll to use changeProxyAdmin task
d0939bd
@0xNeshi
Add manage:endowmentMultiSigFactory:updateProxyAdmin task
bb537d1
@0xNeshi
Call manage:endowmentMultiSigFactory:updateProxyAdmin in changeProxyA…
33f865a 
…dminForAll
@0xNeshi
Update log 'Submitting Tx...' to comment
43a8231
@0xNeshi
Make 'to' optional, read from contract-address.json if null
3f8b77f
@0xNeshi
Fix old proxyAdmin checking logic
1921ac9
@0xNeshi
Target curAdmin when submitting Tx to changeProxyAdmin
4568c56
@0xNeshi
submitMultiSigTx > owner -> msOwner
cc670ee
@0xNeshi
Remove redundant log in update reg. config
c9459a3
@0xNeshi
Fix tasks/manage/changeProxyAdmin.ts
73490ce
@0xNeshi
Fix tasks/manage/changeProxyAdminForAll.ts
4e0f4c5
@0xNeshi
Add divider to changeProxyAdminForAll > changeProxiesAdmin
37e8441
@0xNeshi
Remove temp_task import
2da8830
@0xNeshi
Revert contract-address.json
92c0cc8
@0xNeshi
Remove isInstantiation
2708898
@0xNeshi
Refactor manage:endowmentMultiSigFactory:updateProxyAdmin
73aaa90
@0xNeshi
Minor refactor manage:registrar:updateConfig
f16dc42
@0xNeshi
Add comment to updateEndowmentMultiSigFactory
db70fba
@0xNeshi
Remove mention in EndowmentMultiSigFactory
3853daa
@0xNeshi
Revert "Remove unused errors from IterableMappingAddr"
ac6a68a 
This reverts commit 81e414d.
@0xNeshi
Revert "Remove unused IterableMappingAddr import from LibAccounts"
cd97aba 
This reverts commit 4b7c78f.
@0xNeshi
Revert "Revert "Remove unused IterableMappingAddr import from LibAcco…
ca09a49 
…unts""

This reverts commit 9c6a062.
@0xNeshi
Revert "Revert "Remove unused errors from IterableMappingAddr""
7174fd7 
This reverts commit 98756ce.
@0xNeshi
Add basic checkIfProxyAdmin to changeProxyAdmin
113a36c
@0xNeshi
Move checkIfProxyAdmin to checkIfManagedByProxyAdmin
4bd7310
@0xNeshi
Fix issues after rebase
8fe4f63
@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
@stevieraykatz stevieraykatz mentioned this pull request Sep 20, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ap-justin ap-justin ap-justin left review comments

@stevieraykatz stevieraykatz stevieraykatz approved these changes

@jp-mariano jp-mariano jp-mariano left review comments

Assignees

@0xNeshi 0xNeshi

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xNeshi @stevieraykatz @jp-mariano @ap-justin