Add a double ended queue

[Amxx]

DoubleEndedQueue is an array with O(1) read access and with O(1) push to front and back as well as O(1) pop from front and back. It can also be cleared (reset) in O(1).

This structure can be used to create FIFO, LIFO or other similar structures.

PR Checklist

• Tests
• Documentation
• Changelog entry

[frangio]

If FIFO and LIFO are just wrappers that restrict the available functions, we should just have Vector.

But FIFO doesn't need negative indices. So I don't know if this is the best implementation.

What is the extra cost of using a mapping vs using a contiguous array?

[Amxx]

contract TestArray {
    uint256[] internal _array;

    function push() external { // cost: 65410 gas
        unchecked {
            _array.push(1);
        }
    }
}

contract TestMapping {
    mapping(uint256 => uint256) internal _mapping;
    uint256 internal _mappingLength;

    function push() external { // cost: 65464 gas
        unchecked {
            _mapping[_mappingLength++] = 1;
        }
    }
}

Using an array could save some gas, but it would prevent pushFront(). This might not be needed for FIFO/LIFO, but I think it would be useful in future usecases.

[Amxx]

Note that, if we want to keep begin and end in the same slot, we cannot use the array.length that solidity provides.

AFAIK, this means that we cannot use .push and that we have to do all read/write in assembly :/

[frangio]

I think the array would win against the mapping if you do multiple pushes in a row, because there is a single hash you need to do and then you just increment the slot number.

[Amxx]

contract TestArray {
    uint256[] internal _array;

    function push() public { // cost: 65410 gas
        unchecked {
            _array.push(1);
        }
    }
    function pushMultiple() public { // cost: 110188 gas
        push();
        push();
        push();
    }
}

contract TestMapping {
    mapping(uint256 => uint256) internal _mapping;
    uint256 internal _mappingLength;

    function push() public { // cost: 65464 gas
        unchecked {
            _mapping[_mappingLength++] = 1;
        }
    }
    function pushMultiple() public { // cost: 110302 gas
        push();
        push();
        push();
    }
}

[frangio]

I was wrong in my previous comment, in the case of the array you don't even need to hash on-chain, because you know the position statically, so the compiler just precomputes the hash and embeds it in the bytecode.

Anyway, the difference between these two methods is less than 0.1% of the cost of a push so I think either option is fine in terms of gas cost.

[Amxx]

I really think that having an array is not realistic if you are not using the built-in length. And if we are using the built-in, that is one slot that we cannot share. IMO, it makes a lot of sense to use the same slot for begin+end (2x uint128).

And we need begin + end for FIFO operations.

[frangio]

Same here. Why don't we do just use checked arithmetic and even SafeCast?

[Amxx]

begin and end are only updated by ++ or --. They are not expected to overflow (it would take 2**127 calls for it to overflow).

a consequence is that any checks (like the safemath that solidity includes by default and safecast) would just be wasted gas.

For example, on a pushX/popX operation, unchecked saves ~100 gas per operation. Not sure what the safecast impact would be here, but it would be wasted gas

[Amxx]

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

[Amxx]

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)

[frangio]

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?

[Amxx]

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)

[frangio]

@Amxx Please review and merge if everything looks good.