feat: ownership transfer script can set arbitrary manager #234
Conversation
README.md
Outdated
| export NEW_OWNER=0x1111111111111111111111111111111111111111 | ||
| export RESET_MANAGER=true # true if the new owner should also become the manager, false otherwise | ||
| export NEW_OWNER=0x1111111111111111111111111111111111111111 | ||
| export NEW_MANAGER=0x2222222222222222222222222222222222222222 # optional parameter, the manager does not change if this variable is unset |
There was a problem hiding this comment.
for safety purposes, wouldn't it be better than we change manager and owner is optional? mostly because if we change the manager but not the owner, it is easy to fix, but if we change the owner and not the manager, and the owner is a SAFE... it will be hard to amend.
There was a problem hiding this comment.
This script is intended to be used only during the first deployment of the script, after that both the owner and the manager are expected to be a multisig and this script wouldn't be used anymore.
That is, I wouldn't expect this script to be used to just change the manager.
But this is maybe a good argument to make the manager mandatory and not offer a default behavior if there's no manager.
There was a problem hiding this comment.
I removed the unneeded complexity in 14c8f9f.
There was a problem hiding this comment.
thank you! the README still applies as it is?
There was a problem hiding this comment.
Yep! The only change is in d3d1a69.
script/TransferOwnership.s.sol
Outdated
| // Optional input | ||
| string private constant INPUT_ENV_AUTHENTICATOR_PROXY = "AUTHENTICATOR_PROXY"; | ||
|
|
||
| address public constant NO_MANAGER = address(0); |
There was a problem hiding this comment.
how does this work in order to determine there's no manager?
There was a problem hiding this comment.
This was a flag to internally communicate in the script that the address was actually unset. This workaround isn't needed after 14c8f9f.
Description
When the contract is deployed to a new network, we often need to update the manager and the owner to two separate addresses (for example, some administrative and the DAO respectively).
These changes make this easier.
Test Plan
Updated tests.
Try the updated
Note
I added the
--slowflag so that the two transactions aren't submitted at the same time. This is to prevent the unlikely scenario where a malicious block builder wouldn't be able to just execute the second transaction by overriding the first from another attempt (which could happen if the first attempt was done with incorrect parameters).