Remove unused code in _cancelOrder and improve variable naming

contracts/SamWitchOrderBook.sol

@@ -964,52 +964,52 @@ 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);
       }
     } 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;
       }
     }
   }

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",

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<ISamWitchOrderBook.LimitOrderStruct>(8).fill({
+        side: OrderSide.Buy,
+        tokenId,
+        price,
+        quantity,
+      });
+      await orderBook.limitOrders(limitOrders);
+
+      // Consume whole order to add a tombstone offset
+      const sellLimitOrders = new Array<ISamWitchOrderBook.LimitOrderStruct>(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);
 

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"