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 custom errors with native panic codes in DoubleEndedQueue #4872

Merged
merged 5 commits into from
Feb 6, 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
5 changes: 5 additions & 0 deletions .changeset/gentle-bulldogs-turn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`DoubleEndedQueue`: Custom errors replaced with native panic codes.
2 changes: 1 addition & 1 deletion .changeset/smart-bugs-switch.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@
'openzeppelin-solidity': minor
---

`Math`: MathOverflowedMulDiv error was replaced with native panic codes.
`Math`: Custom errors replaced with native panic codes.
2 changes: 1 addition & 1 deletion contracts/utils/Panic.sol
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ pragma solidity ^0.8.20;
* }
* ```
*
* Follows the list from libsolutil: https://github.com/ethereum/solidity/blob/v0.8.24/libsolutil/ErrorCodes.h
* Follows the list from https://github.com/ethereum/solidity/blob/v0.8.24/libsolutil/ErrorCodes.h[libsolutil].
*/
// slither-disable-next-line unused-state
library Panic {
Expand Down
3 changes: 3 additions & 0 deletions contracts/utils/README.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Miscellaneous contracts and libraries containing utility functions you can use t
* {StorageSlot}: Methods for accessing specific storage slots formatted as common primitive types.
* {Multicall}: Abstract contract with an utility to allow batching together multiple calls in a single transaction. Useful for allowing EOAs to perform multiple operations at once.
* {Context}: An utility for abstracting the sender and calldata in the current execution context.
* {Panic}: A library to revert with https://docs.soliditylang.org/en/v0.8.20/control-structures.html#panic-via-assert-and-error-via-require[Solidity panic codes].

[NOTE]
====
Expand Down Expand Up @@ -106,3 +107,5 @@ Ethereum contracts have no native concept of an interface, so applications must
{{Multicall}}

{{Context}}

{{Panic}}
45 changes: 16 additions & 29 deletions contracts/utils/structs/DoubleEndedQueue.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
// OpenZeppelin Contracts (last updated v5.0.0) (utils/structs/DoubleEndedQueue.sol)
pragma solidity ^0.8.20;

import {Panic} from "../Panic.sol";

/**
* @dev A sequence of items with the ability to efficiently push and pop items (i.e. insert and remove) on both ends of
* the sequence (called front and back). Among other access patterns, it can be used to implement efficient LIFO and
Expand All @@ -15,21 +17,6 @@ pragma solidity ^0.8.20;
* ```
*/
library DoubleEndedQueue {
/**
* @dev An operation (e.g. {front}) couldn't be completed due to the queue being empty.
*/
error QueueEmpty();

/**
* @dev A push operation couldn't be completed due to the queue being full.
*/
error QueueFull();

/**
* @dev An operation (e.g. {at}) couldn't be completed due to an index being out of bounds.
*/
error QueueOutOfBounds();

/**
* @dev Indices are 128 bits so begin and end are packed in a single storage slot for efficient access.
*
Expand All @@ -48,12 +35,12 @@ library DoubleEndedQueue {
/**
* @dev Inserts an item at the end of the queue.
*
* Reverts with {QueueFull} if the queue is full.
* Reverts with {Panic-RESOURCE_ERROR} if the queue is full.
*/
function pushBack(Bytes32Deque storage deque, bytes32 value) internal {
unchecked {
uint128 backIndex = deque._end;
if (backIndex + 1 == deque._begin) revert QueueFull();
if (backIndex + 1 == deque._begin) Panic.panic(Panic.RESOURCE_ERROR);
deque._data[backIndex] = value;
deque._end = backIndex + 1;
}
Expand All @@ -62,12 +49,12 @@ library DoubleEndedQueue {
/**
* @dev Removes the item at the end of the queue and returns it.
*
* Reverts with {QueueEmpty} if the queue is empty.
* Reverts with {Panic-EMPTY_ARRAY_POP} if the queue is empty.
*/
function popBack(Bytes32Deque storage deque) internal returns (bytes32 value) {
unchecked {
uint128 backIndex = deque._end;
if (backIndex == deque._begin) revert QueueEmpty();
if (backIndex == deque._begin) Panic.panic(Panic.EMPTY_ARRAY_POP);
--backIndex;
value = deque._data[backIndex];
delete deque._data[backIndex];
Expand All @@ -78,12 +65,12 @@ library DoubleEndedQueue {
/**
* @dev Inserts an item at the beginning of the queue.
*
* Reverts with {QueueFull} if the queue is full.
* Reverts with {Panic-RESOURCE_ERROR} if the queue is full.
*/
function pushFront(Bytes32Deque storage deque, bytes32 value) internal {
unchecked {
uint128 frontIndex = deque._begin - 1;
if (frontIndex == deque._end) revert QueueFull();
if (frontIndex == deque._end) Panic.panic(Panic.RESOURCE_ERROR);
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
deque._data[frontIndex] = value;
deque._begin = frontIndex;
}
Expand All @@ -92,12 +79,12 @@ library DoubleEndedQueue {
/**
* @dev Removes the item at the beginning of the queue and returns it.
*
* Reverts with `QueueEmpty` if the queue is empty.
* Reverts with {Panic-EMPTY_ARRAY_POP} if the queue is empty.
*/
function popFront(Bytes32Deque storage deque) internal returns (bytes32 value) {
unchecked {
uint128 frontIndex = deque._begin;
if (frontIndex == deque._end) revert QueueEmpty();
if (frontIndex == deque._end) Panic.panic(Panic.EMPTY_ARRAY_POP);
value = deque._data[frontIndex];
delete deque._data[frontIndex];
deque._begin = frontIndex + 1;
Expand All @@ -107,20 +94,20 @@ library DoubleEndedQueue {
/**
* @dev Returns the item at the beginning of the queue.
*
* Reverts with `QueueEmpty` if the queue is empty.
* Reverts with {Panic-ARRAY_OUT_OF_BOUNDS} if the queue is empty.
*/
function front(Bytes32Deque storage deque) internal view returns (bytes32 value) {
if (empty(deque)) revert QueueEmpty();
if (empty(deque)) Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
return deque._data[deque._begin];
}

/**
* @dev Returns the item at the end of the queue.
*
* Reverts with `QueueEmpty` if the queue is empty.
* Reverts with {Panic-ARRAY_OUT_OF_BOUNDS} if the queue is empty.
*/
function back(Bytes32Deque storage deque) internal view returns (bytes32 value) {
if (empty(deque)) revert QueueEmpty();
if (empty(deque)) Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
unchecked {
return deque._data[deque._end - 1];
}
Expand All @@ -130,10 +117,10 @@ library DoubleEndedQueue {
* @dev Return the item at a position in the queue given by `index`, with the first item at 0 and last item at
* `length(deque) - 1`.
*
* Reverts with `QueueOutOfBounds` if the index is out of bounds.
* Reverts with {Panic-ARRAY_OUT_OF_BOUNDS} if the index is out of bounds.
*/
function at(Bytes32Deque storage deque, uint256 index) internal view returns (bytes32 value) {
if (index >= length(deque)) revert QueueOutOfBounds();
if (index >= length(deque)) Panic.panic(Panic.ARRAY_OUT_OF_BOUNDS);
// By construction, length is a uint128, so the check above ensures that index can be safely downcast to uint128
unchecked {
return deque._data[deque._begin + uint128(index)];
Expand Down
26 changes: 26 additions & 0 deletions docs/templates/contract.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,23 @@ import "@openzeppelin/{{__item_context.file.absolutePath}}";
--
{{/if}}

{{#if has-internal-variables}}
[.contract-index]
.Internal Variables
--
{{#each inheritance}}
{{#unless @first}}
[.contract-subindex-inherited]
.{{name}}
{{/unless}}
{{#each internal-variables}}
* {xref-{{anchor~}} }[`++{{typeDescriptions.typeString}} {{#if constant}}constant{{/if}} {{name}}++`]
{{/each}}

{{/each}}
--
{{/if}}

{{#each modifiers}}
[.contract-item]
[[{{anchor}}]]
Expand Down Expand Up @@ -109,3 +126,12 @@ import "@openzeppelin/{{__item_context.file.absolutePath}}";
{{{natspec.dev}}}

{{/each}}

{{#each internal-variables}}
[.contract-item]
[[{{anchor}}]]
==== `{{typeDescriptions.typeString}} [.contract-item-name]#++{{name}}++#` [.item-kind]#internal{{#if constant}} constant{{/if}}#

{{{natspec.dev}}}

{{/each}}
8 changes: 8 additions & 0 deletions docs/templates/properties.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ module.exports['has-errors'] = function ({ item }) {
return item.inheritance.some(c => c.errors.length > 0);
};

module.exports['internal-variables'] = function ({ item }) {
return item.variables.filter(({ visibility }) => visibility === 'internal');
};

module.exports['has-internal-variables'] = function ({ item }) {
return module.exports['internal-variables']({ item }).length > 0;
};

module.exports.functions = function ({ item }) {
return [
...[...findAll('FunctionDefinition', item)].filter(f => f.visibility !== 'private'),
Expand Down
6 changes: 3 additions & 3 deletions test/governance/extensions/GovernorTimelockControl.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
const { anyValue } = require('@nomicfoundation/hardhat-chai-matchers/withArgs');
const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic');

const { GovernorHelper, timelockSalt } = require('../../helpers/governance');
const { OperationState, ProposalState, VoteType } = require('../../helpers/enums');
Expand Down Expand Up @@ -377,9 +378,8 @@ describe('GovernorTimelockControl', function () {
await time.increaseBy.timestamp(delay);

// Error bubbled up from Governor
await expect(this.timelock.connect(this.owner).execute(...call)).to.be.revertedWithCustomError(
this.mock,
'QueueEmpty',
await expect(this.timelock.connect(this.owner).execute(...call)).to.be.revertedWithPanic(
PANIC_CODES.POP_ON_EMPTY_ARRAY,
);
});
});
Expand Down
13 changes: 8 additions & 5 deletions test/utils/structs/DoubleEndedQueue.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
const { ethers } = require('hardhat');
const { expect } = require('chai');
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers');
const { PANIC_CODES } = require('@nomicfoundation/hardhat-chai-matchers/panic');

async function fixture() {
const mock = await ethers.deployContract('$DoubleEndedQueue');
Expand Down Expand Up @@ -36,10 +37,10 @@ describe('DoubleEndedQueue', function () {
});

it('reverts on accesses', async function () {
await expect(this.mock.$popBack(0)).to.be.revertedWithCustomError(this.mock, 'QueueEmpty');
await expect(this.mock.$popFront(0)).to.be.revertedWithCustomError(this.mock, 'QueueEmpty');
await expect(this.mock.$back(0)).to.be.revertedWithCustomError(this.mock, 'QueueEmpty');
await expect(this.mock.$front(0)).to.be.revertedWithCustomError(this.mock, 'QueueEmpty');
await expect(this.mock.$popBack(0)).to.be.revertedWithPanic(PANIC_CODES.POP_ON_EMPTY_ARRAY);
await expect(this.mock.$popFront(0)).to.be.revertedWithPanic(PANIC_CODES.POP_ON_EMPTY_ARRAY);
await expect(this.mock.$back(0)).to.be.revertedWithPanic(PANIC_CODES.ARRAY_ACCESS_OUT_OF_BOUNDS);
await expect(this.mock.$front(0)).to.be.revertedWithPanic(PANIC_CODES.ARRAY_ACCESS_OUT_OF_BOUNDS);
});
});

Expand All @@ -60,7 +61,9 @@ describe('DoubleEndedQueue', function () {
});

it('out of bounds access', async function () {
await expect(this.mock.$at(0, this.content.length)).to.be.revertedWithCustomError(this.mock, 'QueueOutOfBounds');
await expect(this.mock.$at(0, this.content.length)).to.be.revertedWithPanic(
PANIC_CODES.ARRAY_ACCESS_OUT_OF_BOUNDS,
);
});

describe('push', function () {
Expand Down
Loading