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 a double ended queue #3153

Merged
merged 21 commits into from Feb 16, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -2,15 +2,15 @@

pragma solidity ^0.8.0;

import "../utils/structs/Vector.sol";
import "../utils/structs/DoubleEndedQueue.sol";

// Bytes32Vector
contract Bytes32VectorMock {
using Vector for Vector.Bytes32Vector;
// Bytes32Deque
contract Bytes32DequeMock {
using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque;

event OperationResult(bytes32 value);

Vector.Bytes32Vector private _vector;
DoubleEndedQueue.Bytes32Deque private _vector;

function pushBack(bytes32 value) public {
_vector.pushBack(value);
Expand Down
90 changes: 90 additions & 0 deletions contracts/utils/structs/DoubleEndedQueue.sol
@@ -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;
}
Copy link
Collaborator Author

@Amxx Amxx Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the previous implementation

unchecked {
     deque.data[deque.end++] = value;
}

was way more easy to read/understand (same argument apply to all push and pop functions)

Copy link
Contributor

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:

int128 backIndex = deque.end;
deque.data[backIndex] = value;
deque.end = backIndex + 1;

The unchecked makes it harder to read. Perhaps if we put everything in the unchecked block it would look better?

Copy link
Collaborator Author

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)

}

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));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 type(int256).max, this code will fail.

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
}
}
79 changes: 0 additions & 79 deletions contracts/utils/structs/Vector.sol

This file was deleted.

96 changes: 96 additions & 0 deletions test/utils/structs/DoubleEndedQueue.test.js
@@ -0,0 +1,96 @@
const { expectEvent, expectRevert } = 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;
}

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.unspecified(this.deque.popBack());
await expectRevert.unspecified(this.deque.popFront());
await expectRevert.unspecified(this.deque.back());
await expectRevert.unspecified(this.deque.front());
});
});

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.unspecified(this.deque.at(this.content.length));
});

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([]);
});
});
});
91 changes: 0 additions & 91 deletions test/utils/structs/Vector.test.js

This file was deleted.