Implement code review feedback

packages/contracts/contracts/generator/GenArt721GeneratorV0.sol

@@ -8,10 +8,11 @@ import "../interfaces/v0.8.x/IGenArt721CoreTokenHashProviderV0.sol";
 import "../interfaces/v0.8.x/IGenArt721CoreTokenHashProviderV1.sol";
 import "../libs/v0.8.x/Bytes32Strings.sol";
 import "../libs/v0.8.x/BytecodeStorageV1.sol";
+import {ABHelpers} from "../libs/v0.8.x/ABHelpers.sol";
+import {AddressChunks} from "./AddressChunks.sol";
 
 import "@openzeppelin-4.7/contracts/utils/Strings.sol";
 
-import {AddressChunks} from "./AddressChunks.sol";
 import {IScriptyBuilderV2, HTMLRequest, HTMLTagType, HTMLTag} from "scripty.sol/contracts/scripty/interfaces/IScriptyBuilderV2.sol";
 
 /**
@@ -25,8 +26,6 @@ contract GenArt721GeneratorV0 {
     using Bytes32Strings for bytes32;
     using Bytes32Strings for string;
 
-    uint256 constant ONE_MILLION = 1_000_000;
-
     DependencyRegistryV0 public dependencyRegistry;
     IScriptyBuilderV2 public scriptyBuilder;
     address public ethFS;
@@ -69,9 +68,9 @@ contract GenArt721GeneratorV0 {
      * supported by the DependencyRegistry or if the script storage version is
      * not supported.
      */
-    function getDependencyScript(
+    function getDependencyScriptBytes(
         bytes32 dependencyNameAndVersion
-    ) external view returns (bytes memory) {
+    ) internal view returns (bytes memory) {
         uint256 scriptCount = dependencyRegistry.getDependencyScriptCount(
             dependencyNameAndVersion
         );
@@ -102,9 +101,30 @@ contract GenArt721GeneratorV0 {
             revert("Unsupported storage version");
         }
 
+        // @dev We concatenate the script chunks together with assembly
+        // in AddressChunks for gas efficiency.
         return AddressChunks.mergeChunks(scriptBytecodeAddresses, offset);
     }
 
+    /**
+     * @notice Get the dependency script for a given dependency name and version string.
+     * @param dependencyNameAndVersion The name and version of the dependency
+     * to retrieve the script for.
+     * @return The dependency script stiched together from all of the script chunks as a string.
+     * @dev This function will revert if the dependency name and version is not
+     * supported by the DependencyRegistry or if the script storage version is
+     * not supported.
+     */
+    function getDependencyScript(
+        string memory dependencyNameAndVersion
+    ) external view returns (string memory) {
+        bytes memory dependencyScript = getDependencyScriptBytes(
+            dependencyNameAndVersion.stringToBytes32()
+        );
+
+        return string(dependencyScript);
+    }
+
     /**
      * @notice Get the project script for a given core contract address and
      * project ID.
@@ -116,12 +136,13 @@ contract GenArt721GeneratorV0 {
      * supported by the DependencyRegistry or if the contract at the address
      * does not implement the IGenArt721CoreProjectScriptV0 or IGenArt721CoreProjectScriptV1.
      */
-    function getProjectScript(
+    function getProjectScriptBytes(
         address coreContractAddress,
         uint256 projectId
-    ) external view returns (bytes memory) {
+    ) internal view returns (bytes memory) {
         _onlySupportedCoreContract(coreContractAddress);
 
+        // @dev Attempt to get project script info from V3 and up core contracts first.
         try
             IGenArt721CoreProjectScriptV1(coreContractAddress)
                 .projectScriptDetails(projectId)
@@ -154,11 +175,15 @@ contract GenArt721GeneratorV0 {
                 revert("Unsupported storage version");
             }
 
+            // @dev We concatenate the script chunks together with assembly
+            // in AddressChunks for gas efficiency.
             return AddressChunks.mergeChunks(scriptBytecodeAddresses, offset);
         } catch {
             // Noop try again for older contracts.
         }
 
+        // @dev If we failed to get the project script info try again using the interface
+        // for V2 and older core contracts.
         try
             IGenArt721CoreProjectScriptV0(coreContractAddress)
                 .projectScriptInfo(projectId)
@@ -175,12 +200,35 @@ contract GenArt721GeneratorV0 {
                 script = string.concat(script, scriptChunk);
             }
 
-            return abi.encodePacked(script);
+            return bytes(script);
         } catch {
             revert("Unable to retrieve project script info");
         }
     }
 
+    /**
+     * @notice Get the project script for a given core contract address and
+     * project ID.
+     * @param coreContractAddress The address of the core contract to retrieve
+     * the project script from.
+     * @param projectId The ID of the project to retrieve the script for.
+     * @return The project script stiched together from all of the script chunks as a string.
+     * @dev This function will revert if the core contract address is not
+     * supported by the DependencyRegistry or if the contract at the address
+     * does not implement the IGenArt721CoreProjectScriptV0 or IGenArt721CoreProjectScriptV1.
+     */
+    function getProjectScript(
+        address coreContractAddress,
+        uint256 projectId
+    ) external view returns (string memory) {
+        bytes memory projectScript = getProjectScriptBytes(
+            coreContractAddress,
+            projectId
+        );
+
+        return string(projectScript);
+    }
+
     /**
      * @notice Get the HTMLRequest for a given token.
      * @param coreContractAddress The core contract address the token belongs to.
@@ -193,7 +241,7 @@ contract GenArt721GeneratorV0 {
     ) internal view returns (HTMLRequest memory) {
         _onlySupportedCoreContract(coreContractAddress);
 
-        uint256 projectId = tokenId / ONE_MILLION;
+        uint256 projectId = ABHelpers.tokenIdToProjectId(tokenId);
         // This will revert for older contracts that do not have an override set.
         bytes32 dependencyNameAndVersion = dependencyRegistry
             .getDependencyNameAndVersionForProject(
@@ -203,17 +251,18 @@ contract GenArt721GeneratorV0 {
             .stringToBytes32();
 
         bytes32 tokenHash;
-        if (tokenHash == bytes32(0)) {
-            try
-                IGenArt721CoreTokenHashProviderV1(coreContractAddress)
-                    .tokenIdToHash(tokenId)
-            returns (bytes32 _tokenHash) {
-                tokenHash = _tokenHash;
-            } catch {
-                // Noop try again for older contracts.
-            }
+        // @dev Attempt to get token hash from V1 and up core contracts first.
+        try
+            IGenArt721CoreTokenHashProviderV1(coreContractAddress)
+                .tokenIdToHash(tokenId)
+        returns (bytes32 _tokenHash) {
+            tokenHash = _tokenHash;
+        } catch {
+            // Noop try again for older contracts.
         }
 
+        // @dev If we failed to get the token hash try again using the interface
+        // for V0 core contracts.
         if (tokenHash == bytes32(0)) {
             try
                 IGenArt721CoreTokenHashProviderV0(coreContractAddress)
@@ -265,18 +314,20 @@ contract GenArt721GeneratorV0 {
             bodyTags[0].tagContent = "// Noop"; // ScriptyBuilder requires scriptContent for this to work
             bodyTags[0].tagClose = "</script>";
         } else {
-            bytes memory dependencyScript = this.getDependencyScript(
+            bytes memory dependencyScript = getDependencyScriptBytes(
                 dependencyNameAndVersion
             );
             bodyTags[0].tagContent = dependencyScript;
             bodyTags[0].tagType = HTMLTagType.scriptGZIPBase64DataURI; // <script type="text/javascript+gzip" src="data:text/javascript;base64,[script]"></script>
         }
 
+        // @dev We expect all of our dependencies to be added gzip'd and base64 encoded
+        // so we need to include this so we can gunzip them in the browser.
         bodyTags[1].name = "gunzipScripts-0.0.1.js";
         bodyTags[1].tagType = HTMLTagType.scriptBase64DataURI; // <script src="data:text/javascript;base64,[script]"></script>
         bodyTags[1].contractAddress = ethFS;
 
-        bytes memory projectScript = this.getProjectScript(
+        bytes memory projectScript = getProjectScriptBytes(
             coreContractAddress,
             projectId
         );

packages/contracts/contracts/generator/scripty-support/ETHFSFileStorage.sol

@@ -27,12 +27,12 @@ contract ETHFSFileStorage is IContractScript {
     /**
      * @notice Get the full script from ethfs's FileStore contract
      * @param name - Name given to the script. Eg: threejs.min.js_r148
-     * @param data - Arbitrary data. Not used by this contract.
+     * @param - Arbitrary data. Not used by this contract.
      * @return script - Full script from merged chunks
      */
     function getScript(
         string calldata name,
-        bytes memory data
+        bytes memory /*data*/
     ) external view returns (bytes memory script) {
         return bytes(fileStore.getFile(name).read());
     }

packages/contracts/contracts/interfaces/v0.8.x/IGenArt721CoreContractV3_Base.sol

@@ -6,6 +6,7 @@ pragma solidity ^0.8.0;
 import "./IAdminACLV0.sol";
 /// use the Royalty Registry's IManifold interface for token royalties
 import "./IManifold.sol";
+import "./IGenArt721CoreProjectScriptV1.sol";
 
 /**
  * @title This interface is intended to house interface items that are common
@@ -14,7 +15,10 @@ import "./IManifold.sol";
  * add support the Royalty Registry by default.
  * @author Art Blocks Inc.
  */
-interface IGenArt721CoreContractV3_Base is IManifold {
+interface IGenArt721CoreContractV3_Base is
+    IManifold,
+    IGenArt721CoreProjectScriptV1
+{
     /**
      * @notice Token ID `_tokenId` minted to `_to`.
      */

packages/contracts/contracts/libs/v0.8.x/Bytes32Strings.sol

@@ -69,6 +69,7 @@ library Bytes32Strings {
      * https://ethereum.stackexchange.com/a/9152
      * @param source string to be converted.
      * @return result bytes32 representation of string.
+     * @dev If string is longer than 32 bytes, it truncates to the right.
      */
     function stringToBytes32(
         string memory source