[Upgrade] Add owner getter and setter to Asset and Catalyst #1345
Conversation
wojciech-turek
requested review from
mvanmeerbeck and
adjisb
and removed request for
a team
[Upgrade] Add owner getter and setter to Asset and Catalyst
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
wojciech-turek
changed the title
packages/asset/contracts/Asset.sol
Outdated
| @@ -384,5 +384,21 @@ contract Asset is | |||
| return "ASSET"; | |||
| } | |||
|
|
|||
| uint256[49] private __gap; | |||
| address private _owner; | |||
There was a problem hiding this comment.
Yeah I have declared it there so its all close together, but yeah will move it up
| @@ -324,5 +324,21 @@ contract Catalyst is | |||
| return "CATALYST"; | |||
| } | |||
|
|
|||
| uint256[49] private __gap; | |||
| address private _owner; | |||
|
|
||
| /// @notice Sets the owner of the contract | ||
| /// @param newOwner address of the new owner | ||
| function transferOwnership(address newOwner) external { |
There was a problem hiding this comment.
why not onlyRole(DEFAULT_ADMIN_ROLE) ?
There was a problem hiding this comment.
I think the main reason is to be able to provide custom revert message.
Otherwise we could switch to using the modifier.
| /// @notice Sets the owner of the contract | ||
| /// @param newOwner address of the new owner | ||
| function transferOwnership(address newOwner) external { | ||
| require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "Asset: Unauthorized"); |
| if ((await read('Asset', 'owner')) === ethers.constants.AddressZero) { | ||
| await catchUnknownSigner( | ||
| execute( | ||
| 'Asset', | ||
| {from: assetAdmin, log: true}, | ||
| 'transferOwnership', | ||
| assetAdmin | ||
| ) | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
is better to split this in a separated step, because if something goes wrong it is easier to re-run it separately
There was a problem hiding this comment.
If the deployment of the upgrade fails this won't be called, and its a function that we likely call only once during the upgrade process.
I feel like its integral to this upgrade.
| if ((await read('Catalyst', 'owner')) === ethers.constants.AddressZero) { | ||
| await catchUnknownSigner( | ||
| execute( | ||
| 'Catalyst', | ||
| {from: catalystAdmin, log: true}, | ||
| 'transferOwnership', | ||
| catalystAdmin | ||
| ) | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
is better to split this in a separated step, because if something goes wrong it is easier to re-run it separately
Description
This PR builds on top of #1342 and requires that PR to be merged before this one.
ownerandtransferOwnershipthat sets the private variable_owner.Testing
Run the deployment process in the deploy package by calling
yarn void:deploy