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

Replace revert strings with custom errors #4261

Merged
merged 122 commits into from
Jun 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
122 commits
Select commit Hold shift + click to select a range
4307e5b
Replace error strings with custom errors
ernestognw May 18, 2023
ddf387c
Lint
ernestognw May 18, 2023
0800e32
Finish original list of errors
ernestognw May 19, 2023
199093b
Finish missing require statements
ernestognw May 19, 2023
d3703bd
Self review
ernestognw May 19, 2023
11931ca
Finish revert statements
ernestognw May 19, 2023
bb3a12f
Applied spreadsheet suggestion
ernestognw May 19, 2023
9f58996
Refactor Address.sol
ernestognw May 19, 2023
a76e4b6
Finish custom errors replacement
ernestognw May 19, 2023
c65b76b
Fix SignatureChecker
ernestognw May 20, 2023
f58c06b
Add account to `GovernorAlreadyCastVote` error
ernestognw May 21, 2023
810468d
Finish access, finance, and governance testing
ernestognw May 22, 2023
adbe841
Fix proxy tests
ernestognw May 22, 2023
c29e041
First round of reviews
ernestognw May 22, 2023
bc00094
Fix tests for ERC20 and ERC1155 tokens
ernestognw May 23, 2023
faf33a4
Lint
ernestognw May 23, 2023
cdbea1e
Bump Pragma to 0.8.19
ernestognw May 23, 2023
77eee99
Finish token tests
ernestognw May 23, 2023
ba42df6
Fix ERC20Capped
ernestognw May 23, 2023
c31e10d
Fix Address tests
ernestognw May 23, 2023
b3b7e08
Advancements on utils
ernestognw May 24, 2023
960066b
Fix utils test
ernestognw May 24, 2023
b815a4f
Remove unnecessary test file
ernestognw May 24, 2023
99347e4
Fix conflicts with master
ernestognw May 24, 2023
d7d3f90
Lint
ernestognw May 24, 2023
5bc0812
Add changeset
ernestognw May 24, 2023
3276d35
Fix generation script
ernestognw May 24, 2023
23a7be2
Fix flaky Create2 test
ernestognw May 24, 2023
3f4f84a
Update comment in Ownable
ernestognw May 24, 2023
3dc21e2
Add `Unset` state to TimelockController
ernestognw May 24, 2023
1c119f7
Replace `GovernorMissingETA` with `GovernorProposalNotQueued`
ernestognw May 24, 2023
6541481
Revert interface changes to MinimalForwarder
ernestognw May 24, 2023
9c71d9c
Remove ERC-6093 from token interfaces
ernestognw May 24, 2023
5f8417b
Replace `msg.sender` with `_msgSender()` in ERC721Wrapper
ernestognw May 24, 2023
55f0eef
Make `onRevert` view visilibity
ernestognw May 24, 2023
5fe9511
Change Ownable2Step's error for invalid ownership acceptance
ernestognw May 24, 2023
c927bc3
Replace SafeERC20's domain
ernestognw May 25, 2023
0a5bb3f
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 25, 2023
eac424e
Lint fix
ernestognw May 25, 2023
e11f4fa
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 29, 2023
5dc5ada
Fix checkpoint tests
ernestognw May 29, 2023
44535a2
Enabled all tests
ernestognw May 29, 2023
0d1d6c0
Remove GovernorDuplicatedProposal error
ernestognw May 29, 2023
07d4fc9
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 29, 2023
058b672
Lint
ernestognw May 29, 2023
1d63212
Update contracts/governance/IGovernor.sol
ernestognw May 30, 2023
276331c
Update contracts/utils/Address.sol
ernestognw May 30, 2023
28e2e16
Round of review
ernestognw May 30, 2023
2b0ce34
Merge branch 'next-v5.0' into lib-839-custom-errors
ernestognw May 30, 2023
4946dd9
Rename AccessContol renounce error and parameter
ernestognw May 31, 2023
3173d53
Rename `Unsuccessful` -> `Failed`
ernestognw May 31, 2023
6c8d0e1
Merge Address failed call errors
ernestognw May 31, 2023
cb79ab3
Rename *FailedLowLevelCall to *FailedCall
ernestognw May 31, 2023
be68001
Round of review
ernestognw Jun 1, 2023
f02af6e
Change token paused errors
ernestognw Jun 1, 2023
2d735e6
Fix tests
ernestognw Jun 1, 2023
2ecfd14
Round of review
ernestognw Jun 1, 2023
b3af988
Round of review
ernestognw Jun 1, 2023
c862ca5
More review
ernestognw Jun 1, 2023
5069bdf
Fixed example in StorageSlot.sol
ernestognw Jun 1, 2023
48fe940
Changed Checkpoint error
ernestognw Jun 1, 2023
c15fbf8
Added value to StringsInsufficientHexLength
ernestognw Jun 1, 2023
a10ee8b
Added `useCheckedNonce` for Nonces
ernestognw Jun 1, 2023
8d62e91
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 1, 2023
861d341
Replace ERC20DecreasedAllowance for SafeERC20
ernestognw Jun 1, 2023
61a6026
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 2, 2023
c796d15
Improved MinimalForwarder errors
ernestognw Jun 2, 2023
6a9c40f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 2, 2023
a4063c9
Review
ernestognw Jun 2, 2023
5876d1c
Remove mutable variables in ERC1155 tests
ernestognw Jun 2, 2023
6d8c498
Remove unnecessary error
ernestognw Jun 2, 2023
219923b
Rename `Inexistent` to `Nonexistent`
ernestognw Jun 2, 2023
1fa5acc
Rename AccessControlDefaultAdminRules errors
ernestognw Jun 2, 2023
602df5a
Overflown -> Overflowed
ernestognw Jun 2, 2023
4a14bec
Simplified SafeCast errors
ernestognw Jun 2, 2023
ff12505
Review round
ernestognw Jun 2, 2023
65ad2e5
Lint
ernestognw Jun 2, 2023
91bdb7c
Mooar review
ernestognw Jun 3, 2023
a33a4c8
Revert ERC721 changes
ernestognw Jun 3, 2023
2253c40
Rename Paused errors by using a library
ernestognw Jun 3, 2023
892dbcc
Lint
ernestognw Jun 3, 2023
028f383
Fix tests
ernestognw Jun 5, 2023
9c70af8
Review suggestions
ernestognw Jun 5, 2023
7647774
Move Pausable errors to its own file
ernestognw Jun 6, 2023
45ce67f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 6, 2023
11092da
Moar suggestions
ernestognw Jun 6, 2023
6009844
Moar suggestions
ernestognw Jun 6, 2023
55433e7
Improved custom error matcher
ernestognw Jun 7, 2023
4dac3c7
Lint
ernestognw Jun 7, 2023
aa44323
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 7, 2023
cb4594f
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 7, 2023
6df52b0
Simplify custom error matcher regex
ernestognw Jun 7, 2023
ce83461
Attempt to remove the optimizations branch check for custom errors
ernestognw Jun 7, 2023
e5475a2
Simplify custom error matcher
ernestognw Jun 8, 2023
03f1152
Revert ERC1155Supply _update order
ernestognw Jun 8, 2023
7e320a7
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 8, 2023
0e158b5
Update GUIDELINES.md
ernestognw Jun 8, 2023
f1717a6
Use verifyCallResult in Timelock
ernestognw Jun 9, 2023
db4bcf9
Fix TimelockController tests
ernestognw Jun 9, 2023
e0949a3
Add domain to DoubleEndedQueue errors
ernestognw Jun 9, 2023
46904d7
Suggestions
ernestognw Jun 9, 2023
3a74c47
Change Pausable error names
ernestognw Jun 9, 2023
7bf1afc
Remove address context from UUPSUnauthorizedCallContext
ernestognw Jun 9, 2023
13dd54a
More suggestions
ernestognw Jun 9, 2023
c97d95c
Remove errorPrefix in AccessControl tests
Amxx Jun 9, 2023
fc1e4b0
Remove errorPrefix in ERC20 tests
Amxx Jun 9, 2023
27b15e3
move ERC20Permit.test.js out of draft
Amxx Jun 9, 2023
2ad6e65
Remove errorPrefix in ERC721 tests
Amxx Jun 9, 2023
9304b9b
Fix test
ernestognw Jun 9, 2023
d4f7071
Remove Pausable.errors.sol library
ernestognw Jun 9, 2023
88cf256
Applied suggestions
ernestognw Jun 9, 2023
4432ee6
More suggestions
ernestognw Jun 9, 2023
b2aaf9e
Remove _custom revert functions
ernestognw Jun 9, 2023
d5815a8
Fix tests
ernestognw Jun 9, 2023
d4e27a0
increase gas for test
frangio Jun 9, 2023
73ac6dd
document unspecified revert, and add custom error check
Amxx Jun 12, 2023
8994817
document bubbling of panic code
Amxx Jun 12, 2023
c1c6a75
Update contracts/utils/Address.sol
ernestognw Jun 12, 2023
3da76b0
Update test/helpers/customError.js
ernestognw Jun 12, 2023
4aa35c8
Remove `bitsize` from SafeCastOverflowed cast errors
ernestognw Jun 12, 2023
007d6b0
Lint
ernestognw Jun 12, 2023
1cd28fe
Merge branch 'master' into lib-839-custom-errors
ernestognw Jun 12, 2023
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
5 changes: 5 additions & 0 deletions .changeset/lovely-geckos-hide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': major
---

Replace revert strings and require statements with custom errors.
14 changes: 14 additions & 0 deletions GUIDELINES.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,17 @@ In addition to the official Solidity Style Guide we have a number of other conve
```

* Unchecked arithmetic blocks should contain comments explaining why overflow is guaranteed not to happen. If the reason is immediately apparent from the line above the unchecked block, the comment may be omitted.

* Custom errors should be declared following the [EIP-6093](https://eips.ethereum.org/EIPS/eip-6093) rationale whenever reasonable. Also, consider the following:

* The domain prefix should be picked in the following order:
1. Use `ERC<number>` if the error is a violation of an ERC specification.
2. Use the name of the underlying component where it belongs (eg. `Governor`, `ECDSA`, or `Timelock`).

* The location of custom errors should be decided in the following order:
1. Take the errors from their underlying ERCs if they're already defined.
2. Declare the errors in the underlying interface/library if the error makes sense in its context.
3. Declare the error in the implementation if the underlying interface/library is not suitable to do so (eg. interface/library already specified in an ERC).
4. Declare the error in an extension if the error only happens in such extension or child contracts.

* Custom error names should not be declared twice along the library to avoid duplicated identifier declarations when inheriting from multiple contracts.
21 changes: 7 additions & 14 deletions contracts/access/AccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -107,16 +107,7 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
*/
function _checkRole(bytes32 role, address account) internal view virtual {
if (!hasRole(role, account)) {
revert(
string(
abi.encodePacked(
"AccessControl: account ",
Strings.toHexString(account),
" is missing role ",
Strings.toHexString(uint256(role), 32)
)
)
);
revert AccessControlUnauthorizedAccount(account, role);
}
}

Expand Down Expand Up @@ -173,14 +164,16 @@ abstract contract AccessControl is Context, IAccessControl, ERC165 {
*
* Requirements:
*
* - the caller must be `account`.
* - the caller must be `callerConfirmation`.
*
* May emit a {RoleRevoked} event.
*/
function renounceRole(bytes32 role, address account) public virtual {
require(account == _msgSender(), "AccessControl: can only renounce roles for self");
function renounceRole(bytes32 role, address callerConfirmation) public virtual {
if (callerConfirmation != _msgSender()) {
revert AccessControlBadConfirmation();
}

_revokeRole(role, account);
_revokeRole(role, callerConfirmation);
}

/**
Expand Down
36 changes: 25 additions & 11 deletions contracts/access/AccessControlDefaultAdminRules.sol
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
* @dev Sets the initial values for {defaultAdminDelay} and {defaultAdmin} address.
*/
constructor(uint48 initialDelay, address initialDefaultAdmin) {
require(initialDefaultAdmin != address(0), "AccessControl: 0 default admin");
if (initialDefaultAdmin == address(0)) {
revert AccessControlInvalidDefaultAdmin(address(0));
}
_currentDelay = initialDelay;
_grantRole(DEFAULT_ADMIN_ROLE, initialDefaultAdmin);
}
Expand All @@ -80,15 +82,19 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
* @dev See {AccessControl-grantRole}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function grantRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly grant default admin role");
if (role == DEFAULT_ADMIN_ROLE) {
revert AccessControlEnforcedDefaultAdminRules();
}
super.grantRole(role, account);
}

/**
* @dev See {AccessControl-revokeRole}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function revokeRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't directly revoke default admin role");
if (role == DEFAULT_ADMIN_ROLE) {
revert AccessControlEnforcedDefaultAdminRules();
}
super.revokeRole(role, account);
}

Expand All @@ -108,10 +114,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
function renounceRole(bytes32 role, address account) public virtual override(AccessControl, IAccessControl) {
if (role == DEFAULT_ADMIN_ROLE && account == defaultAdmin()) {
(address newDefaultAdmin, uint48 schedule) = pendingDefaultAdmin();
require(
newDefaultAdmin == address(0) && _isScheduleSet(schedule) && _hasSchedulePassed(schedule),
"AccessControl: only can renounce in two delayed steps"
);
if (newDefaultAdmin != address(0) || !_isScheduleSet(schedule) || !_hasSchedulePassed(schedule)) {
revert AccessControlEnforcedDefaultAdminDelay(schedule);
}
delete _pendingDefaultAdminSchedule;
}
super.renounceRole(role, account);
Expand All @@ -128,7 +133,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
*/
function _grantRole(bytes32 role, address account) internal virtual override {
if (role == DEFAULT_ADMIN_ROLE) {
require(defaultAdmin() == address(0), "AccessControl: default admin already granted");
if (defaultAdmin() != address(0)) {
revert AccessControlEnforcedDefaultAdminRules();
}
_currentDefaultAdmin = account;
}
super._grantRole(role, account);
Expand All @@ -148,7 +155,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
* @dev See {AccessControl-_setRoleAdmin}. Reverts for `DEFAULT_ADMIN_ROLE`.
*/
function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual override {
require(role != DEFAULT_ADMIN_ROLE, "AccessControl: can't violate default admin rules");
if (role == DEFAULT_ADMIN_ROLE) {
revert AccessControlEnforcedDefaultAdminRules();
}
super._setRoleAdmin(role, adminRole);
}

Expand Down Expand Up @@ -236,7 +245,10 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
*/
function acceptDefaultAdminTransfer() public virtual {
(address newDefaultAdmin, ) = pendingDefaultAdmin();
require(_msgSender() == newDefaultAdmin, "AccessControl: pending admin must accept");
if (_msgSender() != newDefaultAdmin) {
// Enforce newDefaultAdmin explicit acceptance.
revert AccessControlInvalidDefaultAdmin(_msgSender());
}
_acceptDefaultAdminTransfer();
}

Expand All @@ -247,7 +259,9 @@ abstract contract AccessControlDefaultAdminRules is IAccessControlDefaultAdminRu
*/
function _acceptDefaultAdminTransfer() internal virtual {
(address newAdmin, uint48 schedule) = pendingDefaultAdmin();
require(_isScheduleSet(schedule) && _hasSchedulePassed(schedule), "AccessControl: transfer delay not passed");
if (!_isScheduleSet(schedule) || !_hasSchedulePassed(schedule)) {
revert AccessControlEnforcedDefaultAdminDelay(schedule);
}
_revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin());
_grantRole(DEFAULT_ADMIN_ROLE, newAdmin);
delete _pendingDefaultAdmin;
Expand Down
16 changes: 14 additions & 2 deletions contracts/access/IAccessControl.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,18 @@ pragma solidity ^0.8.19;
* @dev External interface of AccessControl declared to support ERC165 detection.
*/
interface IAccessControl {
/**
* @dev The `account` is missing a role.
*/
error AccessControlUnauthorizedAccount(address account, bytes32 neededRole);

/**
* @dev The caller of a function is not the expected one.
*
* NOTE: Don't confuse with {AccessControlUnauthorizedAccount}.
*/
error AccessControlBadConfirmation();

/**
* @dev Emitted when `newAdminRole` is set as ``role``'s admin role, replacing `previousAdminRole`
*
Expand Down Expand Up @@ -82,7 +94,7 @@ interface IAccessControl {
*
* Requirements:
*
* - the caller must be `account`.
* - the caller must be `callerConfirmation`.
*/
function renounceRole(bytes32 role, address account) external;
function renounceRole(bytes32 role, address callerConfirmation) external;
}
22 changes: 22 additions & 0 deletions contracts/access/IAccessControlDefaultAdminRules.sol
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,28 @@ import "./IAccessControl.sol";
* _Available since v4.9._
*/
interface IAccessControlDefaultAdminRules is IAccessControl {
/**
* @dev The new default admin is not a valid default admin.
*/
error AccessControlInvalidDefaultAdmin(address defaultAdmin);

/**
* @dev At least one of the following rules was violated:
*
* - The `DEFAULT_ADMIN_ROLE` must only be managed by itself.
* - The `DEFAULT_ADMIN_ROLE` must only be held by one account at the time.
* - Any `DEFAULT_ADMIN_ROLE` transfer must be in two delayed steps.
*/
error AccessControlEnforcedDefaultAdminRules();

/**
* @dev The delay for transferring the default admin delay is enforced and
* the operation must wait until `schedule`.
*
* NOTE: `schedule` can be 0 indicating there's no transfer scheduled.
*/
error AccessControlEnforcedDefaultAdminDelay(uint48 schedule);

/**
* @dev Emitted when a {defaultAdmin} transfer is started, setting `newAdmin` as the next
* address to become the {defaultAdmin} by calling {acceptDefaultAdminTransfer} only after `acceptSchedule`
Expand Down
18 changes: 16 additions & 2 deletions contracts/access/Ownable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ import "../utils/Context.sol";
abstract contract Ownable is Context {
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 Down Expand Up @@ -48,7 +58,9 @@ abstract contract Ownable is Context {
* @dev Throws if the sender is not the owner.
*/
function _checkOwner() internal view virtual {
require(owner() == _msgSender(), "Ownable: caller is not the owner");
if (owner() != _msgSender()) {
revert OwnableUnauthorizedAccount(_msgSender());
}
}

/**
Expand All @@ -67,7 +79,9 @@ abstract contract Ownable is Context {
* Can only be called by the current owner.
*/
function transferOwnership(address newOwner) public virtual onlyOwner {
require(newOwner != address(0), "Ownable: new owner is the zero address");
if (newOwner == address(0)) {
revert OwnableInvalidOwner(address(0));
}
_transferOwnership(newOwner);
}

Expand Down
4 changes: 3 additions & 1 deletion contracts/access/Ownable2Step.sol
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ abstract contract Ownable2Step is Ownable {
*/
function acceptOwnership() public virtual {
address sender = _msgSender();
require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner");
if (pendingOwner() != sender) {
revert OwnableUnauthorizedAccount(sender);
}
_transferOwnership(sender);
}
}
9 changes: 8 additions & 1 deletion contracts/finance/VestingWallet.sol
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ contract VestingWallet is Context {
event EtherReleased(uint256 amount);
event ERC20Released(address indexed token, uint256 amount);

/**
* @dev The `beneficiary` is not a valid account.
*/
error VestingWalletInvalidBeneficiary(address beneficiary);

uint256 private _released;
mapping(address => uint256) private _erc20Released;
address private immutable _beneficiary;
Expand All @@ -33,7 +38,9 @@ contract VestingWallet is Context {
* @dev Set the beneficiary, start timestamp and vesting duration of the vesting wallet.
*/
constructor(address beneficiaryAddress, uint64 startTimestamp, uint64 durationSeconds) payable {
require(beneficiaryAddress != address(0), "VestingWallet: beneficiary is zero address");
if (beneficiaryAddress == address(0)) {
revert VestingWalletInvalidBeneficiary(address(0));
}
_beneficiary = beneficiaryAddress;
_start = startTimestamp;
_duration = durationSeconds;
Expand Down
Loading