refactor(contracts): use OwnableUpgradeable by OpenZeppelin (#775)

* Switch to upgradable contracts * Remove initializable * Use OwnableUpgradeable * Remove preventOwner * Remove CallerIsOwner error * User OwnableUpgradeable * Remove preventOwner * Remove errors * Update abis * Remove directTransfer abi * Update network * Update e2e
ArkEcosystem · Nov 22, 2024 · 1aabb0f · 1aabb0f
1 parent 1918e40
commit 1aabb0f
Show file tree

Hide file tree

Showing 30 changed files with 1,568 additions and 1,543 deletions.
diff --git a/contracts/src/consensus/ConsensusV1.sol b/contracts/src/consensus/ConsensusV1.sol
@@ -1,8 +1,8 @@
 // SPDX-License-Identifier: GNU GENERAL PUBLIC LICENSE
 pragma solidity ^0.8.27;
 
-import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
-import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
+import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 struct ValidatorData {
     uint256 votersCount;
@@ -48,9 +48,6 @@ event Voted(address voter, address validator);
 
 event Unvoted(address voter, address validator);
 
-error CallerIsNotOwner();
-error CallerIsOwner();
-
 error CallerIsNotValidator();
 error ValidatorNotRegistered();
 error ValidatorAlreadyRegistered();
@@ -89,9 +86,7 @@ error ImportIsNotAllowed();
 // Block is processed: Original wallet balance: 100, new wallet balance: 88, difference 12
 // This process will only work fine if we pass the new wallet balance (88) and keep track of voteBalances in EVM contract.
 
-contract ConsensusV1 is Initializable, UUPSUpgradeable {
-    address private _owner;
-
+contract ConsensusV1 is UUPSUpgradeable, OwnableUpgradeable {
     mapping(address => ValidatorData) private _validatorsData;
     mapping(address => bool) private _hasValidator;
     mapping(bytes32 => bool) private _blsPublicKeys;
@@ -112,24 +107,9 @@ contract ConsensusV1 is Initializable, UUPSUpgradeable {
 
     RoundValidator[][] private _rounds;
 
-    // Modifiers
-    modifier onlyOwner() {
-        if (msg.sender != _owner) {
-            revert CallerIsNotOwner();
-        }
-        _;
-    }
-
-    modifier preventOwner() {
-        if (msg.sender == _owner) {
-            revert CallerIsOwner();
-        }
-        _;
-    }
-
     // Initializers
     function initialize() public initializer {
-        _owner = msg.sender;
+        __Ownable_init(msg.sender);
         _minValidators = 1;
     }
 
@@ -203,7 +183,7 @@ contract ConsensusV1 is Initializable, UUPSUpgradeable {
         emit Voted(voter, validator);
     }
 
-    function registerValidator(bytes calldata blsPublicKey) external preventOwner {
+    function registerValidator(bytes calldata blsPublicKey) external {
         if (_hasValidator[msg.sender]) {
             revert ValidatorAlreadyRegistered();
         }
@@ -221,7 +201,7 @@ contract ConsensusV1 is Initializable, UUPSUpgradeable {
         emit ValidatorRegistered(msg.sender, blsPublicKey);
     }
 
-    function updateValidator(bytes calldata blsPublicKey) external preventOwner {
+    function updateValidator(bytes calldata blsPublicKey) external {
         if (!isValidatorRegistered(msg.sender)) {
             revert ValidatorNotRegistered();
         }
@@ -253,7 +233,7 @@ contract ConsensusV1 is Initializable, UUPSUpgradeable {
         emit ValidatorResigned(msg.sender);
     }
 
-    function vote(address addr) external preventOwner {
+    function vote(address addr) external {
         if (!isValidatorRegistered(addr)) {
             revert ValidatorNotRegistered();
         }

diff --git a/contracts/src/usernames/UsernamesV1.sol b/contracts/src/usernames/UsernamesV1.sol
@@ -1,11 +1,8 @@
 // SPDX-License-Identifier: GNU GENERAL PUBLIC LICENSE
 pragma solidity ^0.8.27;
 
-import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol";
-import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";
-
-error CallerIsNotOwner();
-error CallerIsOwner();
+import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 error InvalidUsername();
 error TakenUsername();
@@ -20,30 +17,13 @@ struct User {
     string username;
 }
 
-contract UsernamesV1 is Initializable, UUPSUpgradeable {
-    address private _owner;
-
+contract UsernamesV1 is UUPSUpgradeable, OwnableUpgradeable {
     mapping(address => string) private _usernames;
     mapping(bytes32 => bool) private _usernameExists;
 
-    // Modifiers
-    modifier onlyOwner() {
-        if (msg.sender != _owner) {
-            revert CallerIsNotOwner();
-        }
-        _;
-    }
-
-    modifier preventOwner() {
-        if (msg.sender == _owner) {
-            revert CallerIsOwner();
-        }
-        _;
-    }
-
     // Initializers
     function initialize() public initializer {
-        _owner = msg.sender;
+        __Ownable_init(msg.sender);
     }
 
     // Overrides
@@ -60,7 +40,7 @@ contract UsernamesV1 is Initializable, UUPSUpgradeable {
         _registerUsername(user, username, b);
     }
 
-    function registerUsername(string memory username) external preventOwner {
+    function registerUsername(string memory username) external {
         // Register username
         bytes memory b = bytes(username);
         if (!_verifyUsername(b)) {

diff --git a/contracts/test/consensus/Consensus-CalculateTop.sol b/contracts/test/consensus/Consensus-CalculateTop.sol
@@ -5,12 +5,12 @@ import {
     ConsensusV1,
     ValidatorData,
     Validator,
-    CallerIsNotOwner,
     InvalidParameters,
     NoActiveValidators
 } from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_should_work_with_one_validator() public {
@@ -26,7 +26,7 @@ contract ConsensusTest is Base {
     function test_should_allow_only_caller() public {
         address addr = address(1);
         vm.startPrank(addr);
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, addr));
         consensus.calculateActiveValidators(1);
     }
 

diff --git a/contracts/test/consensus/Consensus-Proxy.sol b/contracts/test/consensus/Consensus-Proxy.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GNU GENERAL PUBLIC LICENSE
 pragma solidity ^0.8.13;
 
-import {ConsensusV1, ValidatorData, Validator, CallerIsNotOwner} from "@contracts/consensus/ConsensusV1.sol";
+import {ConsensusV1, ValidatorData, Validator} from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
 import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.sol";

diff --git a/contracts/test/consensus/Consensus-Rounds.sol b/contracts/test/consensus/Consensus-Rounds.sol
@@ -1,15 +1,16 @@
 // SPDX-License-Identifier: GNU GENERAL PUBLIC LICENSE
 pragma solidity ^0.8.13;
 
-import {ConsensusV1, Round, CallerIsNotOwner} from "@contracts/consensus/ConsensusV1.sol";
+import {ConsensusV1, Round} from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_revert_if_caller_is_not_owner() public {
         vm.startPrank(address(1));
 
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, address(1)));
         consensus.getRounds(0, 10);
     }
 

diff --git a/contracts/test/consensus/Consensus-UpdateVoters.sol b/contracts/test/consensus/Consensus-UpdateVoters.sol
@@ -1,23 +1,17 @@
 // SPDX-License-Identifier: GNU GENERAL PUBLIC LICENSE
 pragma solidity ^0.8.13;
 
-import {
-    ConsensusV1,
-    ValidatorData,
-    Validator,
-    Unvoted,
-    Voted,
-    CallerIsNotOwner
-} from "@contracts/consensus/ConsensusV1.sol";
+import {ConsensusV1, ValidatorData, Validator, Unvoted, Voted} from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_updateVoters_should_allow_only_caller() public {
         address addr = address(1);
         vm.startPrank(addr);
         address[] memory voters = new address[](0);
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, addr));
         consensus.updateVoters(voters);
     }
 }
diff --git a/contracts/test/consensus/Consensus-ValidatoUpdate.sol b/contracts/test/consensus/Consensus-ValidatoUpdate.sol
@@ -6,7 +6,6 @@ import {
     ValidatorData,
     Validator,
     ValidatorUpdated,
-    CallerIsOwner,
     ValidatorAlreadyRegistered,
     BlsKeyAlreadyRegistered,
     BlsKeyIsInvalid,

diff --git a/contracts/test/consensus/Consensus-ValidatorAdd.sol b/contracts/test/consensus/Consensus-ValidatorAdd.sol
@@ -6,15 +6,14 @@ import {
     ValidatorData,
     Validator,
     ValidatorRegistered,
-    CallerIsOwner,
-    CallerIsNotOwner,
     ValidatorAlreadyRegistered,
     BlsKeyAlreadyRegistered,
     ImportIsNotAllowed,
     BlsKeyIsInvalid
 } from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_validator_add_pass() public {
@@ -85,7 +84,7 @@ contract ConsensusTest is Base {
     function test_validator_add_revert_if_caller_is_not_owner() public {
         address addr = address(1);
         vm.startPrank(addr);
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, addr));
         consensus.addValidator(addr, prepareBLSKey(addr), false);
     }
 

diff --git a/contracts/test/consensus/Consensus-ValidatorRegistration.sol b/contracts/test/consensus/Consensus-ValidatorRegistration.sol
@@ -6,7 +6,6 @@ import {
     ValidatorData,
     Validator,
     ValidatorRegistered,
-    CallerIsOwner,
     ValidatorAlreadyRegistered,
     BlsKeyAlreadyRegistered,
     BlsKeyIsInvalid
@@ -36,11 +35,6 @@ contract ConsensusTest is Base {
         assertEq(validator.data.isResigned, false);
     }
 
-    function test_validator_registration_revert_if_caller_is_owner() public {
-        vm.expectRevert(CallerIsOwner.selector);
-        consensus.registerValidator(prepareBLSKey(address(1)));
-    }
-
     function test_validator_registration_revert_if_validator_is_already_registered() public {
         address addr = address(1);
 

diff --git a/contracts/test/consensus/Consensus-Vote.sol b/contracts/test/consensus/Consensus-Vote.sol
@@ -9,8 +9,6 @@ import {
     Unvoted,
     Voted,
     VoteResult,
-    CallerIsNotOwner,
-    CallerIsOwner,
     ValidatorNotRegistered,
     VoteResignedValidator,
     VoteSameValidator,
@@ -19,6 +17,7 @@ import {
 } from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_vote() public {
@@ -79,11 +78,6 @@ contract ConsensusTest is Base {
         assertEq(allVoters[0].validator, addr);
     }
 
-    function test_vote_revert_if_caller_is_owner() public {
-        vm.expectRevert(CallerIsOwner.selector);
-        consensus.vote(address(1));
-    }
-
     function test_unvote_revert_if_did_not_vote() public {
         vm.expectRevert(MissingVote.selector);
         consensus.unvote();
@@ -92,7 +86,7 @@ contract ConsensusTest is Base {
     function test_get_voters_revert_if_caller_is_not_owner() public {
         vm.startPrank(address(1));
 
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, address(1)));
         consensus.getVotes(address(0), 10);
     }
 

diff --git a/contracts/test/consensus/Consensus-VoteAdd.sol b/contracts/test/consensus/Consensus-VoteAdd.sol
@@ -9,8 +9,6 @@ import {
     Unvoted,
     Voted,
     VoteResult,
-    CallerIsNotOwner,
-    CallerIsOwner,
     ValidatorNotRegistered,
     VoteResignedValidator,
     VoteSameValidator,
@@ -20,6 +18,7 @@ import {
 } from "@contracts/consensus/ConsensusV1.sol";
 import {Base} from "./Base.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract ConsensusTest is Base {
     function test_add_vote_pass() public {
@@ -130,7 +129,7 @@ contract ConsensusTest is Base {
     function test_add_vote_revert_if_caller_is_not_owner() public {
         address addr = address(1);
         vm.startPrank(addr);
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, addr));
         consensus.addVote(addr, addr);
     }
 

diff --git a/contracts/test/usernames/Usernames-Proxy.sol b/contracts/test/usernames/Usernames-Proxy.sol
@@ -4,8 +4,6 @@ pragma solidity ^0.8.13;
 import {Test, console} from "@forge-std/Test.sol";
 import {
     UsernamesV1,
-    CallerIsOwner,
-    CallerIsNotOwner,
     InvalidUsername,
     TakenUsername,
     UsernameNotRegistered,

diff --git a/contracts/test/usernames/UsernamesV1.sol b/contracts/test/usernames/UsernamesV1.sol
@@ -4,8 +4,6 @@ pragma solidity ^0.8.13;
 import {Test, console} from "@forge-std/Test.sol";
 import {
     UsernamesV1,
-    CallerIsOwner,
-    CallerIsNotOwner,
     InvalidUsername,
     TakenUsername,
     UsernameNotRegistered,
@@ -14,6 +12,7 @@ import {
     User
 } from "@contracts/usernames/UsernamesV1.sol";
 import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
 
 contract UsernamesTest is Test {
     UsernamesV1 public usernames;
@@ -26,7 +25,7 @@ contract UsernamesTest is Test {
 
     function test_add_username_should_revert_if_not_owner() public {
         vm.startPrank(address(1));
-        vm.expectRevert(CallerIsNotOwner.selector);
+        vm.expectRevert(abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, address(1)));
         usernames.addUsername(address(1), "test");
     }
 
@@ -210,11 +209,6 @@ contract UsernamesTest is Test {
         assertTrue(usernames.isUsernameRegistered("test2"));
     }
 
-    function test_register_username_revert_if_owner() public {
-        vm.expectRevert(CallerIsOwner.selector);
-        usernames.registerUsername("test");
-    }
-
     function test_register_username_revert_if_empty() public {
         vm.startPrank(address(1));
         vm.expectRevert(InvalidUsername.selector);