Fix twin inefficiencies (#656)

* Fix Twin transfer inefficiences

* Unit tests

* Fixing broken unit tests

* Refactor

* Adds missing test

* Improve adding a mapping for range ids

* Tidy

* Fix corrupted lock file

* Review changes

* Revert protocol initialization if nextTwinId is not 1

* Move revert reason to BosonConstants

* fix tests

---------

Co-authored-by: zajck <klemen@impressieve.com>

contracts/domain/BosonConstants.sol

@@ -34,6 +34,7 @@ string constant VERSION_MUST_BE_SET = "Version cannot be empty";
 string constant ADDRESSES_AND_CALLDATA_LENGTH_MISMATCH = "Addresses and calldata must be same length";
 string constant WRONG_CURRENT_VERSION = "Wrong current protocol version";
 string constant DIRECT_INITIALIZATION_NOT_ALLOWED = "Direct initializtion is not allowed";
+string constant TWINS_ALREADY_EXIST = "Should not have any twins yet";
 
 // Revert Reasons: Access related
 string constant ACCESS_DENIED = "Access denied, caller doesn't have role";

contracts/protocol/bases/GroupBase.sol

@@ -96,10 +96,10 @@ contract GroupBase is ProtocolBase, IBosonGroupEvents {
     /**
      * @notice Validates that condition parameters make sense.
      *
-     * A invalid condition is one that fits any of the following criteria:
+     * An invalid condition is one that fits any of the following criteria:
      * - EvaluationMethod.None: any field different from zero
      * - EvaluationMethod.Threshold: 
-          -Token address, maxCommits, or threshold is zero. 
+          - Token address, maxCommits, or threshold is zero.
      *    - TokenType is FungibleToken or NonFungibleToken and length and tokenId are not 0.
      * - EvaluationMethod.Threshold:
      *    - token address, maxCommits or threshold is zero

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

@@ -814,6 +814,7 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
                 (, Twin storage twin) = fetchTwin(twinIds[i]);
 
                 bool success;
+
                 uint256 tokenId = twin.tokenId;
 
                 {
@@ -881,16 +882,44 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
 
                     emit TwinTransferFailed(twin.id, twin.tokenAddress, _exchange.id, tokenId, twin.amount, sender);
                 } else {
-                    // Store twin receipt on twinReceiptsByExchange
                     uint256 exchangeId = _exchange.id;
-                    TwinReceipt storage twinReceipt = lookups.twinReceiptsByExchange[exchangeId].push();
-                    twinReceipt.twinId = twin.id;
-                    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);
+                    uint256 twinId = twin.id;
+                    {
+                        // Store twin receipt on twinReceiptsByExchange
+                        TwinReceipt storage twinReceipt = lookups.twinReceiptsByExchange[exchangeId].push();
+                        twinReceipt.twinId = twinId;
+                        twinReceipt.tokenAddress = twin.tokenAddress;
+                        twinReceipt.tokenId = tokenId;
+                        twinReceipt.amount = twin.amount;
+                        twinReceipt.tokenType = twin.tokenType;
+                    }
+                    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];
+
+                        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

@@ -141,17 +141,24 @@ contract ProtocolInitializationHandlerFacet is IBosonProtocolInitializationHandl
      *
      * V2.3.0 adds the minimal resolution period. Cannot be initialized with ConfigHandlerFacet.initialize since it would reset the counters.
      *
+     * 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.
+     *  - minResolutionPeriond is not present in _initializationData parameter
+     *
      * @param _initializationData - data representing uint256 _minResolutionPeriod
      */
     function initV2_3_0(bytes calldata _initializationData) internal {
         // Current version must be 2.2.1
         require(protocolStatus().version == bytes32("2.2.1"), WRONG_CURRENT_VERSION);
 
+        require(protocolCounters().nextTwinId == 1, TWINS_ALREADY_EXIST);
+
         // Initialize limits.maxPremintedVouchers (configHandlerFacet initializer)
-        uint256 _minResolutionPeriod = abi.decode(_initializationData, (uint256));
-        require(_minResolutionPeriod != 0, VALUE_ZERO_NOT_ALLOWED);
-        protocolLimits().minResolutionPeriod = _minResolutionPeriod;
-        emit MinResolutionPeriodChanged(_minResolutionPeriod, msgSender());
+        uint256 minResolutionPeriod = abi.decode(_initializationData, (uint256));
+        require(minResolutionPeriod != 0, VALUE_ZERO_NOT_ALLOWED);
+        protocolLimits().minResolutionPeriod = minResolutionPeriod;
+        emit MinResolutionPeriodChanged(minResolutionPeriod, msgSender());
     }
 
     /**

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

@@ -187,8 +187,11 @@ library ProtocolLib {
         mapping(uint256 => BosonTypes.AuthToken) pendingAuthTokenUpdatesBySeller;
         // dispute resolver id => DisputeResolver
         mapping(uint256 => BosonTypes.DisputeResolver) pendingAddressUpdatesByDisputeResolver;
+        // twin id => range id
+        mapping(uint256 => uint256) rangeIdByTwin;
         // tokenId => groupId =>  commit count (count how many times a token has been used as gate for this group)
         mapping(uint256 => mapping(uint256 => uint256)) conditionalCommitsByTokenId;
+        // seller id => collections
         mapping(uint256 => BosonTypes.Collection[]) additionalCollections;
     }
 

scripts/config/revert-reasons.js

@@ -25,6 +25,7 @@ exports.RevertReasons = {
   ADDRESSES_AND_CALLDATA_MUST_BE_SAME_LENGTH: "Addresses and calldata must be same length",
   WRONG_CURRENT_VERSION: "Wrong current protocol version",
   DIRECT_INITIALIZATION_NOT_ALLOWED: "Direct initializtion is not allowed",
+  TWINS_ALREADY_EXIST: "Should not have any twins yet",
 
   // Offer related
   NOT_ASSISTANT: "Not seller's assistant",