Fix twin inefficiencies

contracts/protocol/bases/TwinBase.sol

@@ -76,7 +76,6 @@ contract TwinBase is ProtocolBase, IBosonTwinEvents {
             TokenRange[] storage twinRanges = lookups.twinRangesBySeller[sellerId][_twin.tokenAddress];
 
             uint256 twinRangesLength = twinRanges.length;
-
             // Checks if token range isn't being used in any other twin of seller
             for (uint256 i = 0; i < twinRangesLength; i++) {
                 // A valid range has:
@@ -89,6 +88,8 @@ contract TwinBase is ProtocolBase, IBosonTwinEvents {
             TokenRange storage tokenRange = twinRanges.push();
             tokenRange.start = tokenId;
             tokenRange.end = lastTokenId;
+
+            lookups.rangeIdByTwin[twinId] = ++twinRangesLength;
         } else if (_twin.tokenType == TokenType.MultiToken) {
             // If token is Fungible or MultiToken amount should not be zero
             // Also, the amount of tokens should not be more than the available token supply.

contracts/protocol/facets/ExchangeHandlerFacet.sol

@@ -720,11 +720,6 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
             // Get seller account
             (, Seller storage seller, ) = fetchSeller(bundle.sellerId);
 
-            // Variable to track whether some twin transfer failed
-            bool transferFailed;
-
-            uint256 exchangeId = _exchange.id;
-
             ProtocolLib.ProtocolLookups storage lookups = protocolLookups();
 
             address sender = msgSender();
@@ -738,85 +733,115 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
                 // N.B. Using call here so as to normalize the revert reason
                 bytes memory result;
                 bool success;
+
                 uint256 tokenId = twin.tokenId;
-                TokenType tokenType = twin.tokenType;
-
-                // Shouldn't decrement supply if twin supply is unlimited
-                if (twin.supplyAvailable != type(uint256).max) {
-                    // Decrement by 1 if token type is NonFungible otherwise decrement amount (i.e, tokenType is MultiToken or FungibleToken)
-                    twin.supplyAvailable = twin.tokenType == TokenType.NonFungibleToken
-                        ? twin.supplyAvailable - 1
-                        : twin.supplyAvailable - twin.amount;
-                }
 
-                if (tokenType == TokenType.FungibleToken) {
-                    // ERC-20 style transfer
-                    (success, result) = twin.tokenAddress.call(
-                        abi.encodeWithSignature(
-                            "transferFrom(address,address,uint256)",
-                            seller.assistant,
-                            sender,
-                            twin.amount
-                        )
-                    );
-                } else if (tokenType == TokenType.NonFungibleToken) {
-                    // Token transfer order is ascending to avoid overflow when twin supply is unlimited
-                    if (twin.supplyAvailable == type(uint256).max) {
-                        twin.tokenId++;
-                    } else {
-                        // Token transfer order is descending
-                        tokenId = twin.tokenId + twin.supplyAvailable;
+                {
+                    TokenType tokenType = twin.tokenType;
+
+                    // Shouldn't decrement supply if twin supply is unlimited
+                    if (twin.supplyAvailable != type(uint256).max) {
+                        // Decrement by 1 if token type is NonFungible otherwise decrement amount (i.e, tokenType is MultiToken or FungibleToken)
+                        twin.supplyAvailable = tokenType == TokenType.NonFungibleToken
+                            ? twin.supplyAvailable - 1
+                            : twin.supplyAvailable - twin.amount;
+                    }
+
+                    if (tokenType == TokenType.FungibleToken) {
+                        // ERC-20 style transfer
+                        (success, result) = twin.tokenAddress.call(
+                            abi.encodeWithSignature(
+                                "transferFrom(address,address,uint256)",
+                                seller.assistant,
+                                sender,
+                                twin.amount
+                            )
+                        );
+                    } else if (tokenType == TokenType.NonFungibleToken) {
+                        // Token transfer order is ascending to avoid overflow when twin supply is unlimited
+                        if (twin.supplyAvailable == type(uint256).max) {
+                            twin.tokenId++;
+                        } else {
+                            // Token transfer order is descending
+                            tokenId = twin.tokenId + twin.supplyAvailable;
+                        }
+                        // ERC-721 style transfer
+                        (success, result) = twin.tokenAddress.call(
+                            abi.encodeWithSignature(
+                                "safeTransferFrom(address,address,uint256,bytes)",
+                                seller.assistant,
+                                sender,
+                                tokenId,
+                                ""
+                            )
+                        );
+                    } else if (tokenType == TokenType.MultiToken) {
+                        // ERC-1155 style transfer
+                        (success, result) = twin.tokenAddress.call(
+                            abi.encodeWithSignature(
+                                "safeTransferFrom(address,address,uint256,uint256,bytes)",
+                                seller.assistant,
+                                sender,
+                                tokenId,
+                                twin.amount,
+                                ""
+                            )
+                        );
                     }
-                    // ERC-721 style transfer
-                    (success, result) = twin.tokenAddress.call(
-                        abi.encodeWithSignature(
-                            "safeTransferFrom(address,address,uint256,bytes)",
-                            seller.assistant,
-                            sender,
-                            tokenId,
-                            ""
-                        )
-                    );
-                } else if (twin.tokenType == TokenType.MultiToken) {
-                    // ERC-1155 style transfer
-                    (success, result) = twin.tokenAddress.call(
-                        abi.encodeWithSignature(
-                            "safeTransferFrom(address,address,uint256,uint256,bytes)",
-                            seller.assistant,
-                            sender,
-                            tokenId,
-                            twin.amount,
-                            ""
-                        )
-                    );
                 }
 
                 // If token transfer failed
                 if (!success || (result.length > 0 && !abi.decode(result, (bool)))) {
-                    transferFailed = true;
-                    emit TwinTransferFailed(twin.id, twin.tokenAddress, exchangeId, tokenId, twin.amount, sender);
+                    // Raise a dispute if caller is a contract
+                    if (sender.isContract()) {
+                        raiseDisputeInternal(_exchange, _voucher, seller.id);
+                    } else {
+                        // Revoke voucher if caller is an EOA
+                        revokeVoucherInternal(_exchange);
+                        // N.B.: If voucher was revoked because transfer twin failed, then voucher was already burned
+                        shouldBurnVoucher = false;
+                    }
+
+                    emit TwinTransferFailed(twin.id, twin.tokenAddress, _exchange.id, tokenId, twin.amount, sender);
                 } else {
+                    uint256 exchangeId = _exchange.id;
+                    uint256 twinId = twin.id;
                     // Store twin receipt on twinReceiptsByExchange
                     TwinReceipt storage twinReceipt = lookups.twinReceiptsByExchange[exchangeId].push();
-                    twinReceipt.twinId = twin.id;
+                    twinReceipt.twinId = twinId;
                     twinReceipt.tokenAddress = twin.tokenAddress;
                     twinReceipt.tokenId = tokenId;
                     twinReceipt.amount = twin.amount;
                     twinReceipt.tokenType = twin.tokenType;
 
-                    emit TwinTransferred(twin.id, twin.tokenAddress, exchangeId, tokenId, twin.amount, sender);
-                }
-            }
+                    if (twin.tokenType == TokenType.NonFungibleToken) {
+                        // Get all ranges of twins that belong to the seller and to the same token address of the new twin to validate if range is available
+                        TokenRange[] storage twinRanges = lookups.twinRangesBySeller[seller.id][twin.tokenAddress];
 
-            if (transferFailed) {
-                // Raise a dispute if caller is a contract
-                if (sender.isContract()) {
-                    raiseDisputeInternal(_exchange, _voucher, seller.id);
-                } else {
-                    // Revoke voucher if caller is an EOA
-                    revokeVoucherInternal(_exchange);
-                    // N.B.: If voucher was revoked because transfer twin failed, then voucher was already burned
-                    shouldBurnVoucher = false;
+                        bool unlimitedSupply = twin.supplyAvailable == type(uint256).max;
+
+                        uint256 rangeIndex = lookups.rangeIdByTwin[twinId] - 1;
+                        TokenRange storage range = twinRanges[rangeIndex];
+
+                        if (unlimitedSupply ? range.end == tokenId : range.start == tokenId) {
+                            uint256 lastIndex = twinRanges.length - 1;
+
+                            if (rangeIndex != lastIndex) {
+                                // Replace range with last range
+                                twinRanges[rangeIndex] = twinRanges[lastIndex];
+                            }
+
+                            // Remove from ranges mapping
+                            twinRanges.pop();
+
+                            // Delete rangeId from rangeIdByTwin mapping
+                            lookups.rangeIdByTwin[twinId] = 0;
+                        } else {
+                            unlimitedSupply ? range.start++ : range.end--;
+                        }
+                    }
+
+                    emit TwinTransferred(twinId, twin.tokenAddress, exchangeId, tokenId, twin.amount, sender);
                 }
             }
         }

contracts/protocol/facets/ProtocolInitializationHandlerFacet.sol

@@ -97,6 +97,9 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
             if (_version == bytes32("2.2.1")) {
                 initV2_2_1();
             }
+            if (_version == bytes32("2.3.0")) {
+                initV2_3_0();
+            }
         }
 
         removeInterfaces(_interfacesToRemove);
@@ -133,6 +136,20 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
         require(protocolStatus().version == bytes32("2.2.0"), WRONG_CURRENT_VERSION);
     }
 
+    /**
+     * @notice Initializes the version 2.3.0.
+     *
+     * Reverts if:
+     *  - Current version is not 2.2.1
+     *  - There are already twins. This version adds a new mapping for twins which make it incompatible with previous versions.
+     */
+    function initV2_3_0() internal view {
+        // Current version must be 2.2.1
+        require(protocolStatus().version == bytes32("2.2.1"), WRONG_CURRENT_VERSION);
+
+        require(protocolCounters().nextTwinId == 1, "Should not have any twins yet");
+    }
+
     /**
      * @notice Gets the current protocol version.
      *

contracts/protocol/facets/TwinHandlerFacet.sol

@@ -83,18 +83,18 @@ contract TwinHandlerFacet is IBosonTwinHandler, TwinBase {
         if (twin.tokenType == TokenType.NonFungibleToken) {
             TokenRange[] storage twinRanges = lookups.twinRangesBySeller[sellerId][twin.tokenAddress];
             uint256 lastIndex = twinRanges.length - 1;
-            for (uint256 index = 0; index <= lastIndex; index++) {
-                if (twinRanges[index].start == twin.tokenId) {
-                    // If not removing last element, move the last to the removed index
-                    if (index != lastIndex) {
-                        twinRanges[index] = twinRanges[lastIndex];
-                    }
-
-                    // Remove last element
-                    twinRanges.pop();
-                    break;
-                }
+
+            uint256 rangeIndex = lookups.rangeIdByTwin[_twinId] - 1;
+
+            if (rangeIndex != lastIndex) {
+                twinRanges[rangeIndex] = twinRanges[lastIndex];
             }
+
+            // Remove last element
+            twinRanges.pop();
+
+            // Delete rangeIdByTwin mapping
+            delete lookups.rangeIdByTwin[_twinId];
         }
 
         // Notify watchers of state change

contracts/protocol/libs/ProtocolLib.sol

@@ -166,7 +166,6 @@ library ProtocolLib {
         mapping(BosonTypes.AuthTokenType => mapping(uint256 => uint256)) sellerIdByAuthToken;
         // seller id => token address (only ERC721) => start and end of token ids range
         mapping(uint256 => mapping(address => BosonTypes.TokenRange[])) twinRangesBySeller;
-        // seller id => token address (only ERC721) => twin ids
         // @deprecated twinIdsByTokenAddressAndBySeller is no longer used. Keeping it for backwards compatibility.
         mapping(uint256 => mapping(address => uint256[])) twinIdsByTokenAddressAndBySeller;
         // exchange id => BosonTypes.TwinReceipt
@@ -185,6 +184,8 @@ library ProtocolLib {
         mapping(uint256 => BosonTypes.AuthToken) pendingAuthTokenUpdatesBySeller;
         // dispute resolver id => DisputeResolver
         mapping(uint256 => BosonTypes.DisputeResolver) pendingAddressUpdatesByDisputeResolver;
+        // twin id => range id
+        mapping(uint256 => uint256) rangeIdByTwin;
     }
 
     // Incrementing id counters