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 a double ended queue #3153
Merged
Merged
Add a double ended queue #3153
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
d3854ee
add vector, lifo and fifo structures
Amxx b5358a9
fix lint
Amxx 6140b20
need more memory for coverage
Amxx 1157045
remove Vector wrappers and gas optimization
Amxx 9e9dd6d
refactor Vector testing
Amxx bfb6d57
revert package.json changes
Amxx c7ec6fd
rename to DoubleEndedQueue
frangio 933337e
rename and refactor
frangio b7fb096
refactor tests and expand coverage
frangio 17a8fea
test for custom errors
frangio 8eea114
Merge branch 'master' into feature/vector
frangio 595d5ef
add changelog entry
frangio 914e1df
add docs
frangio c5ef056
add sample code and note about storage vs. memory
frangio 1cf95f1
add available since
frangio adc9a5a
lint
frangio 0649a31
use underscore for struct members
frangio bbb091d
add struct documentation
frangio 3aff9f0
remove SafeCast in length
frangio 35064a7
Merge branch 'master' into feature/vector
frangio 248d479
rename i -> index and improve docs
frangio File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
// SPDX-License-Identifier: MIT | ||
|
||
pragma solidity ^0.8.0; | ||
|
||
import "../utils/structs/DoubleEndedQueue.sol"; | ||
|
||
// Bytes32Deque | ||
contract Bytes32DequeMock { | ||
using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; | ||
|
||
event OperationResult(bytes32 value); | ||
|
||
DoubleEndedQueue.Bytes32Deque private _vector; | ||
|
||
function pushBack(bytes32 value) public { | ||
_vector.pushBack(value); | ||
} | ||
|
||
function pushFront(bytes32 value) public { | ||
_vector.pushFront(value); | ||
} | ||
|
||
function popFront() public returns (bytes32) { | ||
bytes32 value = _vector.popFront(); | ||
emit OperationResult(value); | ||
return value; | ||
} | ||
|
||
function popBack() public returns (bytes32) { | ||
bytes32 value = _vector.popBack(); | ||
emit OperationResult(value); | ||
return value; | ||
} | ||
|
||
function front() public view returns (bytes32) { | ||
return _vector.front(); | ||
} | ||
|
||
function back() public view returns (bytes32) { | ||
return _vector.back(); | ||
} | ||
|
||
function at(uint256 i) public view returns (bytes32) { | ||
return _vector.at(i); | ||
} | ||
|
||
function clear() public { | ||
_vector.clear(); | ||
} | ||
|
||
function length() public view returns (uint256) { | ||
return _vector.length(); | ||
} | ||
|
||
function empty() public view returns (bool) { | ||
return _vector.empty(); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity ^0.8.4; | ||
|
||
import "../math/SafeCast.sol"; | ||
|
||
library DoubleEndedQueue { | ||
error Empty(); | ||
error OutOfBounds(); | ||
|
||
struct Bytes32Deque { | ||
int128 begin; // inclusive: the first item is at data[begin] | ||
int128 end; // exclusive: the last item is at data[end-1] | ||
mapping(int128 => bytes32) data; | ||
} | ||
|
||
function pushBack(Bytes32Deque storage deque, bytes32 value) internal { | ||
int128 backIndex = deque.end; | ||
deque.data[backIndex] = value; | ||
unchecked { | ||
deque.end = backIndex + 1; | ||
} | ||
} | ||
|
||
function popBack(Bytes32Deque storage deque) internal returns (bytes32 value) { | ||
if (empty(deque)) revert Empty(); | ||
int128 backIndex; | ||
unchecked { | ||
backIndex = deque.end - 1; | ||
} | ||
value = deque.data[backIndex]; | ||
delete deque.data[backIndex]; | ||
deque.end = backIndex; | ||
} | ||
|
||
function pushFront(Bytes32Deque storage deque, bytes32 value) internal { | ||
int128 frontIndex; | ||
unchecked { | ||
frontIndex = deque.begin - 1; | ||
} | ||
deque.data[frontIndex] = value; | ||
deque.begin = frontIndex; | ||
} | ||
|
||
function popFront(Bytes32Deque storage deque) internal returns (bytes32 value) { | ||
if (empty(deque)) revert Empty(); | ||
int128 frontIndex = deque.begin; | ||
value = deque.data[frontIndex]; | ||
delete deque.data[frontIndex]; | ||
unchecked { | ||
deque.begin = frontIndex + 1; | ||
} | ||
} | ||
|
||
function front(Bytes32Deque storage deque) internal view returns (bytes32 value) { | ||
if (empty(deque)) revert Empty(); | ||
int128 frontIndex = deque.begin; | ||
return deque.data[frontIndex]; | ||
} | ||
|
||
function back(Bytes32Deque storage deque) internal view returns (bytes32 value) { | ||
if (empty(deque)) revert Empty(); | ||
int128 backIndex; | ||
unchecked { | ||
backIndex = deque.end - 1; | ||
} | ||
return deque.data[backIndex]; | ||
} | ||
|
||
function at(Bytes32Deque storage deque, uint256 i) internal view returns (bytes32 value) { | ||
// int256(deque.begin) is a safe upcast | ||
int128 idx = SafeCast.toInt128(int256(deque.begin) + SafeCast.toInt256(i)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will never happen in practice, but if the queue is longer then IMO, we will never really get any deque of length > 2**64, so I would argue that using SafeCast makes all operations more expensive, with no real security gains |
||
if (idx >= deque.end) revert OutOfBounds(); | ||
return deque.data[idx]; | ||
} | ||
|
||
function clear(Bytes32Deque storage deque) internal { | ||
deque.begin = 0; | ||
deque.end = 0; | ||
} | ||
|
||
function length(Bytes32Deque storage deque) internal view returns (uint256) { | ||
unchecked { | ||
return SafeCast.toUint256(int256(deque.end) - int256(deque.begin)); | ||
} | ||
} | ||
|
||
function empty(Bytes32Deque storage deque) internal view returns (bool) { | ||
return deque.end <= deque.begin; | ||
frangio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
const { expectEvent } = require('@openzeppelin/test-helpers'); | ||
const { expect } = require('chai'); | ||
|
||
const Bytes32DequeMock = artifacts.require('Bytes32DequeMock'); | ||
|
||
/** Rebuild the content of the deque as a JS array. */ | ||
async function getContent (deque) { | ||
const length = await deque.length().then(bn => bn.toNumber()); | ||
const values = await Promise.all(Array(length).fill().map((_, i) => deque.at(i))); | ||
return values; | ||
} | ||
|
||
/** Revert handler that supports custom errors. */ | ||
async function expectRevert(promise, reason) { | ||
try { | ||
await promise; | ||
expect.fail(`Expected promise to throw but it didn't`); | ||
} catch (error) { | ||
if (reason) { | ||
expect(error.message).to.include(reason); | ||
} | ||
} | ||
} | ||
|
||
contract('DoubleEndedQueue', function (accounts) { | ||
const bytesA = '0xdeadbeef'.padEnd(66, '0'); | ||
const bytesB = '0x0123456789'.padEnd(66, '0'); | ||
const bytesC = '0x42424242'.padEnd(66, '0'); | ||
const bytesD = '0x171717'.padEnd(66, '0'); | ||
|
||
beforeEach(async function () { | ||
this.deque = await Bytes32DequeMock.new(); | ||
}); | ||
|
||
describe('when empty', function () { | ||
it('getters', async function () { | ||
expect(await this.deque.empty()).to.be.equal(true); | ||
expect(await getContent(this.deque)).to.have.ordered.members([]); | ||
}); | ||
|
||
it('reverts on accesses', async function () { | ||
await expectRevert(this.deque.popBack(), 'Empty()'); | ||
await expectRevert(this.deque.popFront(), 'Empty()'); | ||
await expectRevert(this.deque.back(), 'Empty()'); | ||
await expectRevert(this.deque.front(), 'Empty()'); | ||
}); | ||
}); | ||
|
||
describe('when not empty', function () { | ||
beforeEach(async function () { | ||
await this.deque.pushBack(bytesB); | ||
await this.deque.pushFront(bytesA); | ||
await this.deque.pushBack(bytesC); | ||
this.content = [ bytesA, bytesB, bytesC ]; | ||
}); | ||
|
||
it('getters', async function () { | ||
expect(await this.deque.empty()).to.be.equal(false); | ||
expect(await this.deque.length()).to.be.bignumber.equal(this.content.length.toString()); | ||
expect(await this.deque.front()).to.be.equal(this.content[0]); | ||
expect(await this.deque.back()).to.be.equal(this.content[this.content.length - 1]); | ||
expect(await getContent(this.deque)).to.have.ordered.members(this.content); | ||
}); | ||
|
||
it('out of bounds access', async function () { | ||
await expectRevert(this.deque.at(this.content.length), 'OutOfBounds()'); | ||
}); | ||
|
||
describe('push', function () { | ||
it('front', async function () { | ||
await this.deque.pushFront(bytesD); | ||
this.content.unshift(bytesD); // add element at the begining | ||
|
||
expect(await getContent(this.deque)).to.have.ordered.members(this.content); | ||
}); | ||
|
||
it('back', async function () { | ||
await this.deque.pushBack(bytesD); | ||
this.content.push(bytesD); // add element at the end | ||
|
||
expect(await getContent(this.deque)).to.have.ordered.members(this.content); | ||
}); | ||
}); | ||
|
||
describe('pop', function () { | ||
it('front', async function () { | ||
const value = this.content.shift(); // remove first element | ||
expectEvent(await this.deque.popFront(), 'OperationResult', { value }); | ||
|
||
expect(await getContent(this.deque)).to.have.ordered.members(this.content); | ||
}); | ||
|
||
it('back', async function () { | ||
const value = this.content.pop(); // remove last element | ||
expectEvent(await this.deque.popBack(), 'OperationResult', { value }); | ||
|
||
expect(await getContent(this.deque)).to.have.ordered.members(this.content); | ||
}); | ||
}); | ||
|
||
it('clear', async function () { | ||
await this.deque.clear(); | ||
|
||
expect(await this.deque.empty()).to.be.equal(true); | ||
expect(await getContent(this.deque)).to.have.ordered.members([]); | ||
}); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the previous implementation
was way more easy to read/understand (same argument apply to all push and pop functions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre/post inc/decrements are quick to read, but definitely not easy...
The statement
deque.data[deque.end++] = value
is too dense: there are two different storage updates, and it's not immediately clear what value is used to index the data mapping.I tried a few alternatives and ended up going with the current option. I think this code is quite ok:
The
unchecked
makes it harder to read. Perhaps if we put everything in theunchecked
block it would look better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely think we want the
+1
unchecked.Putting everything in the unchecked block might make it cleaner to read (all the logic is indented at the same level)