Add custom errors for common and ownership packages (#73)

Fantom-foundation · Oct 22, 2024 · 78101a8 · 78101a8
1 parent 582037b
commit 78101a8
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 21 deletions.
diff --git a/contracts/common/Initializable.sol b/contracts/common/Initializable.sol
@@ -24,11 +24,18 @@ contract Initializable {
      */
     bool private initializing;
 
+    /**
+     * @dev The contract instance has already been initialized.
+     */
+    error ContractInitialized();
+
     /**
      * @dev Modifier to use in the initializer function of a contract.
      */
     modifier initializer() {
-        require(initializing || !initialized, "Contract instance has already been initialized");
+        if (!initializing && initialized) {
+            revert ContractInitialized();
+        }
 
         bool isTopLevelCall = !initializing;
         if (isTopLevelCall) {

diff --git a/contracts/common/ReentrancyGuard.sol b/contracts/common/ReentrancyGuard.sol
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.9;
 
-import "./Initializable.sol";
+import {Initializable} from "./Initializable.sol";
 
 /**
  * @dev Contract module that helps prevent reentrant calls to a function.
@@ -19,6 +19,11 @@ contract ReentrancyGuard is Initializable {
     // counter to allow mutex lock with only one SSTORE operation
     uint256 private _guardCounter;
 
+    /**
+     * @dev Reentrant call.
+     */
+    error ReentrantCall();
+
     function initialize() internal initializer {
         // The counter starts at one to prevent changing it from zero to a non-zero
         // value, which is a more expensive operation.
@@ -36,7 +41,9 @@ contract ReentrancyGuard is Initializable {
         _guardCounter += 1;
         uint256 localCounter = _guardCounter;
         _;
-        require(localCounter == _guardCounter, "ReentrancyGuard: reentrant call");
+        if (localCounter != _guardCounter) {
+            revert ReentrantCall();
+        }
     }
 
     uint256[50] private ______gap;

diff --git a/contracts/ownership/Ownable.sol b/contracts/ownership/Ownable.sol
@@ -1,8 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.9;
 
-import "../common/Initializable.sol";
-
+import {Initializable} from "../common/Initializable.sol";
 /**
  * @dev Contract module which provides a basic access control mechanism, where
  * there is an account (an owner) that can be granted exclusive access to
@@ -15,6 +14,16 @@ import "../common/Initializable.sol";
 contract Ownable is Initializable {
     address private _owner;
 
+    /**
+     * @dev The caller account is not authorized to perform an operation.
+     */
+    error OwnableUnauthorizedAccount(address account);
+
+    /**
+     * @dev The owner is not a valid owner account. (eg. `address(0)`)
+     */
+    error OwnableInvalidOwner(address owner);
+
     event OwnershipTransferred(address indexed previousOwner, address indexed newOwner);
 
     /**
@@ -36,7 +45,9 @@ contract Ownable is Initializable {
      * @dev Throws if called by any account other than the owner.
      */
     modifier onlyOwner() {
-        require(isOwner(), "Ownable: caller is not the owner");
+        if (!isOwner()) {
+            revert OwnableUnauthorizedAccount(msg.sender);
+        }
         _;
     }
 
@@ -71,7 +82,9 @@ contract Ownable is Initializable {
      * @dev Transfers ownership of the contract to a new account (`newOwner`).
      */
     function _transferOwnership(address newOwner) internal {
-        require(newOwner != address(0), "Ownable: new owner is the zero address");
+        if (newOwner == address(0)) {
+            revert OwnableInvalidOwner(address(0));
+        }
         emit OwnershipTransferred(_owner, newOwner);
         _owner = newOwner;
     }

diff --git a/test/NodeDriver.ts b/test/NodeDriver.ts
@@ -38,8 +38,9 @@ describe('NodeDriver', () => {
 
     it('Should revert when not owner', async function () {
       const account = ethers.Wallet.createRandom();
-      await expect(this.nodeDriverAuth.connect(this.nonOwner).migrateTo(account)).to.be.revertedWith(
-        'Ownable: caller is not the owner',
+      await expect(this.nodeDriverAuth.connect(this.nonOwner).migrateTo(account)).to.be.revertedWithCustomError(
+        this.nodeDriverAuth,
+        'OwnableUnauthorizedAccount',
       );
     });
   });
@@ -52,9 +53,9 @@ describe('NodeDriver', () => {
 
     it('Should revert when not owner', async function () {
       const address = ethers.Wallet.createRandom();
-      await expect(this.nodeDriverAuth.connect(this.nonOwner).copyCode(this.sfc, address)).to.be.revertedWith(
-        'Ownable: caller is not the owner',
-      );
+      await expect(
+        this.nodeDriverAuth.connect(this.nonOwner).copyCode(this.sfc, address),
+      ).to.be.revertedWithCustomError(this.nodeDriverAuth, 'OwnableUnauthorizedAccount');
     });
   });
 
@@ -66,8 +67,9 @@ describe('NodeDriver', () => {
     });
 
     it('Should revert when not owner', async function () {
-      await expect(this.nodeDriverAuth.connect(this.nonOwner).updateNetworkVersion(1)).to.be.revertedWith(
-        'Ownable: caller is not the owner',
+      await expect(this.nodeDriverAuth.connect(this.nonOwner).updateNetworkVersion(1)).to.be.revertedWithCustomError(
+        this.nodeDriverAuth,
+        'OwnableUnauthorizedAccount',
       );
     });
   });
@@ -78,8 +80,9 @@ describe('NodeDriver', () => {
     });
 
     it('Should revert when not owner', async function () {
-      await expect(this.nodeDriverAuth.connect(this.nonOwner).advanceEpochs(10)).to.be.revertedWith(
-        'Ownable: caller is not the owner',
+      await expect(this.nodeDriverAuth.connect(this.nonOwner).advanceEpochs(10)).to.be.revertedWithCustomError(
+        this.nodeDriverAuth,
+        'OwnableUnauthorizedAccount',
       );
     });
   });

diff --git a/test/SFC.ts b/test/SFC.ts
@@ -290,15 +290,16 @@ describe('SFC', () => {
     });
 
     it('Should revert when transferring ownership if not owner', async function () {
-      await expect(this.sfc.connect(this.user).transferOwnership(ethers.ZeroAddress)).to.be.revertedWith(
-        'Ownable: caller is not the owner',
+      await expect(this.sfc.connect(this.user).transferOwnership(ethers.ZeroAddress)).to.be.revertedWithCustomError(
+        this.nodeDriverAuth,
+        'OwnableUnauthorizedAccount',
       );
     });
 
     it('Should revert when transferring ownership to zero address', async function () {
-      await expect(this.sfc.transferOwnership(ethers.ZeroAddress)).to.be.revertedWith(
-        'Ownable: new owner is the zero address',
-      );
+      await expect(this.sfc.transferOwnership(ethers.ZeroAddress))
+        .to.be.revertedWithCustomError(this.nodeDriverAuth, 'OwnableInvalidOwner')
+        .withArgs(ethers.ZeroAddress);
     });
   });