feat: Transfer Ownership on Universal SpokePool Deployment #1121
Conversation
Signed-off-by: Faisal Usmani <[email protected]>
fusmanii
requested review from
bmzig,
mrice32,
nicholaspai and
pxrl
as code owners
|
|
||
| const provider = hre.ethers.provider; | ||
| const safeCode = await provider.getCode(EXPECTED_SAFE_ADDRESS); | ||
| if (safeCode !== "0x" && proxyAddress) { |
There was a problem hiding this comment.
Also worth checking the owners of the safe deployment here?
There was a problem hiding this comment.
used safe kit to match the deployed mutisig 8c03b88
| const owner = await contract.owner(); | ||
| if (owner !== EXPECTED_SAFE_ADDRESS) { | ||
| await contract.transferOwnership(EXPECTED_SAFE_ADDRESS); | ||
| console.log("Transferred ownership to Expected Safe address:", EXPECTED_SAFE_ADDRESS); |
There was a problem hiding this comment.
wdyt about waiting for transaction confirmation and then reading the updated value here?
…isal/transfer-ownership-universal-spokepool
Signed-off-by: Faisal Usmani <[email protected]>
| const isDeployed = await existingProtocolKit.isSafeDeployed(); | ||
| if (proxyAddress) { | ||
| if (isDeployed) { | ||
| const factory = await hre.ethers.getContractFactory("Universal_SpokePool"); | ||
| const contract = factory.attach(proxyAddress); | ||
|
|
||
| const owner = await contract.owner(); | ||
| if (owner !== EXPECTED_SAFE_ADDRESS) { | ||
| await (await contract.transferOwnership(EXPECTED_SAFE_ADDRESS)).wait(); | ||
| console.log("Transferred ownership to Expected Safe address:", await contract.owner()); | ||
| } else { | ||
| console.log("Expected Safe address is already the owner of the Universal SpokePool"); | ||
| } | ||
| } else { | ||
| console.log("Safe is not deployed, skipping ownership transfer"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't quite follow the logic here. If there's no proxyAddress is there any point to the code between lines 66 and 78?
It also seems possible to squash a couple of levels of indentation by inverting the logic and returning early on various conditions, i.e.:
if (!proxyAddress) {
return;
}
if (!isDeployed) {
console.log(...);
return;
}
// ...
There was a problem hiding this comment.
looks much cleaner now, 9bd543b
| console.log("Expected Safe address is already the owner of the Universal SpokePool"); | ||
| } | ||
| } else { | ||
| console.log("Safe is not deployed, skipping ownership transfer"); |
There was a problem hiding this comment.
Actually one additional thing - shouldn't we consider this to be an error condition? We basically require a safe to own each Universal SpokePool; if for whatever reason we end up on this code path, I feel like it should be obvious to the deployer that something is wrong.
There was a problem hiding this comment.
Converted the log to throw an error instead 9bd543b
Signed-off-by: Faisal Usmani <[email protected]>
3 participants
Closes https://linear.app/uma/issue/ACX-4470/contracts-support-ownership-transfer-to-new-safe-address-post