Merge pull request #1280 from ProjectOpenSea/dan/2023/09/navigator-cl…

…eanup

Dan/2023/09/navigator cleanup

contracts/helpers/navigator/SeaportNavigator.sol

@@ -4,10 +4,6 @@ pragma solidity ^0.8.17;
 import {
     ConsiderationInterface
 } from "seaport-types/src/interfaces/ConsiderationInterface.sol";
-import {
-    AdvancedOrder,
-    CriteriaResolver
-} from "seaport-types/src/lib/ConsiderationStructs.sol";
 
 import {
     SeaportValidatorInterface
@@ -37,16 +33,16 @@ import { HelperInterface } from "./lib/HelperInterface.sol";
  *         and optionally generate criteria resolvers from provided token IDs.
  */
 contract SeaportNavigator is SeaportNavigatorInterface {
-    using NavigatorContextLib for NavigatorContext;
     using CriteriaHelperLib for uint256[];
+    using NavigatorContextLib for NavigatorContext;
 
-    HelperInterface public immutable requestValidator;
     HelperInterface public immutable criteriaHelper;
-    HelperInterface public immutable validatorHelper;
-    HelperInterface public immutable orderDetailsHelper;
+    HelperInterface public immutable executionsHelper;
     HelperInterface public immutable fulfillmentsHelper;
+    HelperInterface public immutable orderDetailsHelper;
+    HelperInterface public immutable requestValidator;
     HelperInterface public immutable suggestedActionHelper;
-    HelperInterface public immutable executionsHelper;
+    HelperInterface public immutable validatorHelper;
 
     HelperInterface[] public helpers;
 

contracts/helpers/navigator/lib/CriteriaHelper.sol

@@ -1,10 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.17;
 
-import {
-    CriteriaResolver
-} from "seaport-types/src/lib/ConsiderationStructs.sol";
-
 import {
     NavigatorCriteriaResolverLib
 } from "./NavigatorCriteriaResolverLib.sol";

contracts/helpers/navigator/lib/CriteriaHelperLib.sol

@@ -72,18 +72,23 @@ library CriteriaHelperLib {
     function sortByHash(
         uint256[] memory tokenIds
     ) internal pure returns (uint256[] memory sortedIds) {
+        // Instantiate a new array of HashAndIntTuple structs.
         HashAndIntTuple[] memory toSort = new HashAndIntTuple[](
             tokenIds.length
         );
+
+        // Populate the array of HashAndIntTuple structs.
         for (uint256 i = 0; i < tokenIds.length; i++) {
             toSort[i] = HashAndIntTuple(
                 tokenIds[i],
                 keccak256(abi.encode(tokenIds[i]))
             );
         }
 
+        // Sort the array of HashAndIntTuple structs.
         _quickSort(toSort, 0, int256(toSort.length - 1));
 
+        // Populate the sortedIds array with the sorted token ids.
         sortedIds = new uint256[](tokenIds.length);
         for (uint256 i = 0; i < tokenIds.length; i++) {
             sortedIds[i] = toSort[i].num;
@@ -97,25 +102,52 @@ library CriteriaHelperLib {
     function toSortedHashes(
         uint256[] memory tokenIds
     ) internal pure returns (bytes32[] memory hashes) {
+        // Instantiate a new array of hashes.
         hashes = new bytes32[](tokenIds.length);
+
+        // Sort the token ids by their hashes.
         uint256[] memory ids = sortByHash(tokenIds);
+
+        // Hash each token id and store it in the hashes array.
         for (uint256 i; i < ids.length; ++i) {
             hashes[i] = keccak256(abi.encode(ids[i]));
         }
     }
 
+    /**
+     *  @dev This function performs the quick sort algorithm to sort an array of
+     *       HashAndIntTuple structs.
+     *
+     *  @param arr   The array of HashAndIntTuple structs to be sorted.
+     *  @param left  The starting index of the segment to be sorted.
+     *  @param right The ending index of the segment to be sorted.
+     */
     function _quickSort(
         HashAndIntTuple[] memory arr,
         int256 left,
         int256 right
     ) internal pure {
+        // Initialize pointers i and j to the left and right ends of the array
+        // segment.
         int256 i = left;
         int256 j = right;
+
+        // If the segment has one element or none, it's already sorted.
         if (i == j) return;
+
+        // Pick a 'pivot' element from the middle of the list.
         bytes32 pivot = arr[uint256(left + (right - left) / 2)].hash;
+
+        // The main loop to rearrange elements around the pivot.
         while (i <= j) {
+            // Find an element larger than or equal to the pivot from the left.
             while (arr[uint256(i)].hash < pivot) i++;
+
+            // Find an element smaller than or equal to the pivot from the
+            // right.
             while (pivot < arr[uint256(j)].hash) j--;
+
+            // Swap the elements at i and j if needed.
             if (i <= j) {
                 (arr[uint256(i)], arr[uint256(j)]) = (
                     arr[uint256(j)],
@@ -125,7 +157,11 @@ library CriteriaHelperLib {
                 j--;
             }
         }
+
+        // Recursively sort the segment before 'j'.
         if (left < j) _quickSort(arr, left, j);
+
+        // Recursively sort the segment after 'i'.
         if (i < right) _quickSort(arr, i, right);
     }
 }

contracts/helpers/navigator/lib/HelperItemLib.sol

@@ -17,6 +17,21 @@ library HelperItemLib {
      */
     error UnknownItemType();
 
+    /**
+     * @dev Normalizes the type of a NavigatorOfferItem based on the presence of
+     *      criteria. This originated in the context of the fuzz tests in
+     *      ./test/foundry/new, where an item might be assigned a pseudorandom
+     *      type and a pseudorandom criteria. In this context, it will just
+     *      correct an incorrect item type. If the item has criteria, ERC721 and
+     *      ERC1155 items will be normalized to ERC721_WITH_CRITERIA and
+     *      ERC1155_WITH_CRITERIA, respectively. Reverts with UnknownItemType if
+     *      the item type is neither ERC721,  ERC721_WITH_CRITERIA, ERC1155, nor
+     *      ERC1155_WITH_CRITERIA.
+     *
+     * @param item The NavigatorOfferItem to normalize.
+     *
+     * @return ItemType The normalized item type.
+     */
     function normalizeType(
         NavigatorOfferItem memory item
     ) internal pure returns (ItemType) {
@@ -40,6 +55,19 @@ library HelperItemLib {
         }
     }
 
+    /**
+     * @dev Normalizes the type of a NavigatorConsiderationItem based on the
+     *      presence of criteria. This originated in the context of the fuzz
+     *      tests in ./test/foundry/new, where an item might be assigned a
+     *      pseudorandom type and a pseudorandom criteriaOrIdentifier. In this
+     *      context, it will just correct an incorrect item type. If the item
+     *      has criteria, ERC721 and ERC1155 items will be normalized to
+     *      ERC721_WITH_CRITERIA and ERC1155_WITH_CRITERIA, respectively.
+     *
+     * @param item The NavigatorConsiderationItem to normalize.
+     *
+     * @return ItemType The normalized item type.
+     */
     function normalizeType(
         NavigatorConsiderationItem memory item
     ) internal pure returns (ItemType) {
@@ -66,32 +94,47 @@ library HelperItemLib {
     function hasCriteria(
         NavigatorOfferItem memory item
     ) internal pure returns (bool) {
+        // Candidate identifiers are passed in by the caller as an array of
+        // uint256s and converted by Navigator.
         return item.candidateIdentifiers.length > 0;
     }
 
     function hasCriteria(
         NavigatorConsiderationItem memory item
     ) internal pure returns (bool) {
+        // Candidate identifiers are passed in by the caller as an array of
+        // uint256s and converted by Navigator.
         return item.candidateIdentifiers.length > 0;
     }
 
     function validate(NavigatorOfferItem memory item) internal pure {
         ItemType itemType = item.itemType;
+
+        // If the item has criteria, the item type must be ERC721 or ERC1155.
         if (itemType == ItemType.ERC20 || itemType == ItemType.NATIVE) {
-            if (item.candidateIdentifiers.length > 0) {
+            if (hasCriteria(item)) {
                 revert InvalidItemTypeForCandidateIdentifiers();
             } else {
                 return;
             }
         }
-        // If the item has candidate identifiers, the item identifier must be
-        // zero for wildcard or one of the candidates.
+
+        // If the item has no candidate identifiers, the item identifier must be
+        // non-zero.
+        //
+        // NOTE: This is only called after `item.hasCriteria()` checks
+        // which ensure that `item.candidateIdentifiers.length > 0` but if it
+        // were used in other contexts, this would prohibit the use of
+        // legitimate 0 identifiers.
         if (item.candidateIdentifiers.length == 0 && item.identifier == 0) {
             revert InvalidIdentifier(
                 item.identifier,
                 item.candidateIdentifiers
             );
         }
+
+        // If the item has candidate identifiers, the item identifier must be
+        // zero or wildcard for one of the candidates.
         if (item.candidateIdentifiers.length > 0) {
             bool identifierFound;
             for (uint256 i; i < item.candidateIdentifiers.length; i++) {
@@ -111,22 +154,28 @@ library HelperItemLib {
 
     function validate(NavigatorConsiderationItem memory item) internal pure {
         ItemType itemType = item.itemType;
+
+        // If the item has criteria, the item type must be ERC721 or ERC1155.
         if (itemType == ItemType.ERC20 || itemType == ItemType.NATIVE) {
-            if (item.candidateIdentifiers.length > 0) {
+            if (hasCriteria(item)) {
                 revert InvalidItemTypeForCandidateIdentifiers();
             } else {
                 return;
             }
         }
-        // If the item has candidate identifiers, the item identifier must be
-        // zero for wildcard or one of the candidates.
+
+        // If the item has no candidate identifiers, the item identifier must be
+        // non-zero.
         if (item.candidateIdentifiers.length == 0 && item.identifier == 0) {
             revert InvalidIdentifier(
                 item.identifier,
                 item.candidateIdentifiers
             );
         }
-        if (item.candidateIdentifiers.length > 0) {
+
+        // If the item has candidate identifiers, the item identifier must be
+        // zero or wildcard for one of the candidates.
+        if (hasCriteria(item)) {
             bool identifierFound;
             for (uint256 i; i < item.candidateIdentifiers.length; i++) {
                 if (item.candidateIdentifiers[i] == item.identifier) {

contracts/helpers/navigator/lib/MerkleLib.sol

@@ -6,59 +6,91 @@ pragma solidity ^0.8.17;
  * Murky: https://github.com/dmfxyz/murky
  */
 library MerkleLib {
+    /**
+     * @dev Computes the Merkle tree hash of two child nodes.
+     *
+     * @param left The hash of the left child node.
+     * @param right The hash of the right child node.
+     *
+     * @return _hash The Merkle tree hash of the two input hashes.
+     */
     function merkleHash(
         bytes32 left,
         bytes32 right
     ) internal pure returns (bytes32 _hash) {
         assembly {
+            // Compare the left and right hash to order them lt(left, right)
+            // returns true if left is less than right
             switch lt(left, right)
+            // If left is not less than right, switch the order.
             case 0 {
                 mstore(0x0, right)
                 mstore(0x20, left)
             }
+            // If left is less than right, keep the order.
             default {
                 mstore(0x0, left)
                 mstore(0x20, right)
             }
-            _hash := keccak256(0x0, 0x40)
-        }
-    }
 
-    function xorkleHash(
-        bytes32 left,
-        bytes32 right
-    ) internal pure returns (bytes32 _hash) {
-        assembly {
-            mstore(0x0, xor(left, right))
-            _hash := keccak256(0x0, 0x20)
+            // Compute the keccak256 hash of the 64-byte input (two concatenated
+            // 32-byte hashes) The result is stored in '_hash'
+            _hash := keccak256(0x0, 0x40)
         }
     }
 
+    /**
+     * @dev Verifies a Merkle proof.
+     *
+     * @param root          The root of the Merkle tree.
+     * @param proof         An array containing the Merkle proof hashes.
+     * @param valueToProve  The leaf node value to prove membership for.
+     * @param hashLeafPairs A function to hash pairs of leaf nodes.
+     *
+     * @return True if the proof is valid, otherwise false.
+     */
     function verifyProof(
         bytes32 root,
         bytes32[] memory proof,
         bytes32 valueToProve,
         function(bytes32, bytes32) internal pure returns (bytes32) hashLeafPairs
     ) internal pure returns (bool) {
-        // proof length must be less than max array size
+        // Proof length must be less than max array size.
         bytes32 rollingHash = valueToProve;
         uint256 length = proof.length;
+
+        // Loop through each proof element to compute the rolling hash.
         unchecked {
             for (uint i = 0; i < length; ++i) {
                 rollingHash = hashLeafPairs(rollingHash, proof[i]);
             }
         }
+
+        // The final rolling hash must equal the Merkle root for the proof to be
+        // valid
         return root == rollingHash;
     }
 
+    /**
+     * @dev Computes the Merkle root from an array of leaf node hashes.
+     *
+     * @param data          An array containing the leaf node hashes.
+     * @param hashLeafPairs A function to hash pairs of leaf nodes.
+     *
+     * @return The Merkle root.
+     */
     function getRoot(
         bytes32[] memory data,
         function(bytes32, bytes32) internal pure returns (bytes32) hashLeafPairs
     ) internal pure returns (bytes32) {
         require(data.length > 1, "won't generate root for single leaf");
+
+        // Loop until only the Merkle root remains.
         while (data.length > 1) {
             data = hashLevel(data, hashLeafPairs);
         }
+
+        // Return the computed Merkle root.
         return data[0];
     }