diff --git a/contracts/SamWitchOrderBook.sol b/contracts/SamWitchOrderBook.sol index b731274..8d42c5f 100644 --- a/contracts/SamWitchOrderBook.sol +++ b/contracts/SamWitchOrderBook.sol @@ -964,10 +964,7 @@ contract SamWitchOrderBook is ISamWitchOrderBook, ERC1155Holder, UUPSUpgradeable } if (_offset == 0 && segment >> 64 == 0) { - // If only one order is in the segment, remove the segment by shifting all other segments to the left - for (uint i = _index; i < _segments.length.sub(1); ++i) { - _segments[i] = _segments[i.inc()]; - } + // If there is only one order at the start of the segment then remove the whole segment _segments.pop(); if (_segments.length == _tombstoneOffset) { _tree.remove(_price); @@ -975,41 +972,44 @@ contract SamWitchOrderBook is ISamWitchOrderBook, ERC1155Holder, UUPSUpgradeable } else { uint indexToRemove = _index * NUM_ORDERS_PER_SEGMENT + _offset; - uint nextElementIndex = 0; - uint nextOffsetIndex = 0; + // Although this is called next, it also acts as the "last" used later + uint nextSegmentIndex = indexToRemove / NUM_ORDERS_PER_SEGMENT; + uint nextOffsetIndex = indexToRemove % NUM_ORDERS_PER_SEGMENT; // Shift orders cross-segments. // This does all except the last order // TODO: For offset 0, 1, 2 we can shift the whole elements of the segment in 1 go. - for (uint i = indexToRemove; i < _segments.length.mul(NUM_ORDERS_PER_SEGMENT).dec(); ++i) { - nextElementIndex = (i.inc()) / NUM_ORDERS_PER_SEGMENT; + uint totalOrders = _segments.length.mul(NUM_ORDERS_PER_SEGMENT).dec(); + for (uint i = indexToRemove; i < totalOrders; ++i) { + nextSegmentIndex = (i.inc()) / NUM_ORDERS_PER_SEGMENT; nextOffsetIndex = (i.inc()) % NUM_ORDERS_PER_SEGMENT; - bytes32 nextSegment = _segments[nextElementIndex]; + bytes32 currentOrNextSegment = _segments[nextSegmentIndex]; - uint currentElementIndex = i / NUM_ORDERS_PER_SEGMENT; + uint currentSegmentIndex = i / NUM_ORDERS_PER_SEGMENT; uint currentOffsetIndex = i % NUM_ORDERS_PER_SEGMENT; - bytes32 currentElement = _segments[currentElementIndex]; - - uint newOrder = uint64(uint(nextSegment >> nextOffsetIndex.mul(64))); - if (newOrder == 0) { - nextElementIndex = currentElementIndex; + bytes32 currentSegment = _segments[currentSegmentIndex]; + uint nextOrder = uint64(uint(currentOrNextSegment >> nextOffsetIndex.mul(64))); + if (nextOrder == 0) { + // There are no more orders left, reset back to the currently iterated order as the last + nextSegmentIndex = currentSegmentIndex; nextOffsetIndex = currentOffsetIndex; break; } // Clear the current order and set it with the shifted order - currentElement &= _clearOrderMask(currentOffsetIndex); - currentElement |= bytes32(newOrder) << currentOffsetIndex.mul(64); - _segments[currentElementIndex] = currentElement; + currentSegment &= _clearOrderMask(currentOffsetIndex); + currentSegment |= bytes32(nextOrder) << currentOffsetIndex.mul(64); + _segments[currentSegmentIndex] = currentSegment; } + // Only pop if the next offset is 0 which means there is 1 order left in that segment if (nextOffsetIndex == 0) { _segments.pop(); } else { // Clear the last element - bytes32 lastElement = _segments[nextElementIndex]; + bytes32 lastElement = _segments[nextSegmentIndex]; lastElement &= _clearOrderMask(nextOffsetIndex); - _segments[nextElementIndex] = lastElement; + _segments[nextSegmentIndex] = lastElement; } } } diff --git a/package.json b/package.json index 94185d5..9c9ff1f 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,7 @@ "prettier": "^3.2.5", "prettier-plugin-solidity": "^1.3.1", "pretty-quick": "^4.0.0", - "solidity-coverage": "^0.8.9", + "solidity-coverage": "^0.8.10-rc.0", "squirrelly": "^9.0.0", "ts-node": ">=10.9.2", "typechain": "^8.3.2", diff --git a/test/SamWitchOrderBook.ts b/test/SamWitchOrderBook.ts index 7b57735..b7c9c83 100644 --- a/test/SamWitchOrderBook.ts +++ b/test/SamWitchOrderBook.ts @@ -742,6 +742,51 @@ describe("SamWitchOrderBook", function () { expect(orders.length).to.eq(0); }); + it("Cancel an order at the very end of the same segment which has a tombstoneOffset", async function () { + const {orderBook, tokenId} = await loadFixture(deployContractsFixture); + + // Set up order books + const price = 100; + const quantity = 10; + + const limitOrders = new Array(8).fill({ + side: OrderSide.Buy, + tokenId, + price, + quantity, + }); + await orderBook.limitOrders(limitOrders); + + // Consume whole order to add a tombstone offset + const sellLimitOrders = new Array(4).fill({ + side: OrderSide.Sell, + tokenId, + price, + quantity, + }); + await orderBook.limitOrders(sellLimitOrders); + + expect((await orderBook.allOrdersAtPrice(OrderSide.Buy, tokenId, price)).length).to.eq(4); + + // Cancel a buy at the end of the segment + const orderId = 8; + console.log("Cancel a buy at the end"); + await orderBook.cancelOrders([orderId], [{side: OrderSide.Buy, tokenId, price}]); + + const node = await orderBook.getNode(OrderSide.Buy, tokenId, price); + expect(node.tombstoneOffset).to.eq(1); + + // Should be 3 orders remaining + let orders = await orderBook.allOrdersAtPrice(OrderSide.Buy, tokenId, price); + expect(orders.length).to.eq(3); + expect(orders[0].id).to.eq(orderId - 3); + expect(orders[1].id).to.eq(orderId - 2); + expect(orders[2].id).to.eq(orderId - 1); + + expect(await orderBook.getHighestBid(tokenId)).to.equal(price); + expect(await orderBook.getLowestAsk(tokenId)).to.equal(0); + }); + it("Cancel an order which deletes a segment at the beginning, middle and end", async function () { const {orderBook, owner, tokenId, brush, initialBrush} = await loadFixture(deployContractsFixture); diff --git a/yarn.lock b/yarn.lock index 848c639..e62e2a1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4211,10 +4211,10 @@ solidity-comments-extractor@^0.0.8: resolved "https://registry.yarnpkg.com/solidity-comments-extractor/-/solidity-comments-extractor-0.0.8.tgz#f6e148ab0c49f30c1abcbecb8b8df01ed8e879f8" integrity sha512-htM7Vn6LhHreR+EglVMd2s+sZhcXAirB1Zlyrv5zBuTxieCvjfnRpd7iZk75m/u6NOlEyQ94C6TWbBn2cY7w8g== -solidity-coverage@^0.8.9: - version "0.8.9" - resolved "https://registry.yarnpkg.com/solidity-coverage/-/solidity-coverage-0.8.9.tgz#d885207df927faf6b4f729904fab8ce0943cfd14" - integrity sha512-ZhPsxlsLkYyzgwoVGh8RBN2ju7JVahvMkk+8RBVc0vP/3UNq88GzvL8kvbuY48lVIRL8eQjJ+0X8al2Bu9/2iQ== +solidity-coverage@^0.8.10-rc.0: + version "0.8.10-rc.0" + resolved "https://registry.yarnpkg.com/solidity-coverage/-/solidity-coverage-0.8.10-rc.0.tgz#6d008501920132ac2d9280c30c31be41779c8d42" + integrity sha512-x1UaLpDEMLCN4LZuuPD+jrI22YzmkMWbbtH/zESPqYGQolB2jsLw1cyrkY9/gVArsRtRGTy1WuwcbQKL1y2rcg== dependencies: "@ethersproject/abi" "^5.0.9" "@solidity-parser/parser" "^0.18.0"