Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom errors for common and ownership packages #73

Merged
merged 7 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion contracts/common/Initializable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 9 additions & 2 deletions contracts/common/ReentrancyGuard.sol
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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.
Expand All @@ -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;
Expand Down
21 changes: 17 additions & 4 deletions contracts/ownership/Ownable.sol
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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);

/**
Expand All @@ -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);
}
_;
}

Expand Down Expand Up @@ -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;
}
Expand Down
21 changes: 12 additions & 9 deletions test/NodeDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
);
});
});
Expand All @@ -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');
});
});

Expand All @@ -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',
);
});
});
Expand All @@ -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',
);
});
});
Expand Down
11 changes: 6 additions & 5 deletions test/SFC.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down
Loading