comment pass

ProjectOpenSea · Sep 29, 2023 · 2833c8d · 2833c8d
1 parent 0f7ea05
commit 2833c8d
Show file tree

Hide file tree

Showing 8 changed files with 163 additions and 72 deletions.
diff --git a/README.md b/README.md
@@ -109,17 +109,11 @@ To generate reports, run
     - [ ] Pin to version
   - [x] Solady
     - [ ] Pin to version
-  - [ ] Shipyard-core (dependent on making public)
+  - [x] Shipyard-core
+    - [ ] Pin to version
 - [ ] Include a base cross-chain deploy script
 - [ ] Figure out if there's a way we can make `forge verify-contract` more ergonomic
 - [ ] Top-level helpers:
   - [x] PRB's `reinit-submodules` script as top-level helper
   - [x] `coverage-report` script as top-level helper
-  - [ ] TODO: are there security concerns about these?
-
-Copilot suggests:
-- [ ] Additional github actions
-  - [ ] Add a `forge deploy` workflow to the Github Actions
-  - [ ] Add a `forge verify` workflow to the Github Actions
-- [ ] Add a `forge verify` script to the top-level helpers
-- [ ] 
+  - [ ] TODO: are there security concerns about these?
diff --git a/exampleNftTutorial/EnvironmentSetup.md b/exampleNftTutorial/EnvironmentSetup.md
@@ -47,7 +47,7 @@ I mean, you could just make a `.env` file based on the sample, source it (`. .en
 
 But hold out, it's not time yet! Or go ahead, we're not the deploy police. But you'll definitely feel better about the contract if you run some test, make some innovative or at least fun changes, write some new tests, run them some more, and *then* deploy. Or do it right now. It's comforting to know that it actually works before you invest real time. For real, doing something at your terminal and then seeing a corresponding change on a block explorer is magical. Do it! Or don't. Either way.
 
-You can update your project to use the latest version of shipyard-core by running `forge update lib/shipyard-core`. But make sure you've got everything backed up, because sometimes upgrading dependencies creates issues that are hard to debug.
+You can update your project to use the latest version of shipyard-core by running `forge update shipyard-core`. But make sure you've got everything backed up, because sometimes upgrading dependencies creates issues that are hard to debug.
 
 ## Testing environment setup
 

diff --git a/exampleNftTutorial/Overview.md b/exampleNftTutorial/Overview.md
@@ -2,6 +2,7 @@
 
 This mini-tutorial will assume that you're already a software engineer, but that you're not yet steeped in the ways of web 3. If you're trying to learn both at the same time, huge props, but it's probably advisable to start with a more structured and guided tutorial, such as [CryptoZombies](https://cryptozombies.io/).
 
+The Dockmaster contract exists exclusively as a reference. It's not meant to be inherited from or otherwise used in Shipyard projects. It's recommended to leave it in place as you work through the tutorial, then rip out all Dockmaster related contracts and tests. Or, work through the tutorial in a disposable directory, save it for later reference and start your real shipyard based project in a fresh, separate directory, immediately ripping out all Dockmaster related code.
 
 ## Deploying Tutorial Table of Contents
 

diff --git a/script/DeployAndMint.s.sol b/script/DeployAndMint.s.sol
@@ -16,7 +16,13 @@ import "../src/Dockmaster.sol";
  */
 contract DeployScript is Script {
     function run() public {
+        // The deploy and the mint need to happen within the same
+        // `startBroadcast`/`stopBroadcast` block. Otherwise, the script will
+        // fail because Foundry will not recognize that the sender is the owner
+        // of the token contract. See
+        // https://book.getfoundry.sh/cheatcodes/start-broadcast for more info.
         vm.startBroadcast();
+
         // Create a new Dockmaster contract.
         Dockmaster targetContract = new Dockmaster("Dockmaster NFT", "DM");
 
@@ -25,12 +31,14 @@ contract DeployScript is Script {
 
         vm.stopBroadcast();
 
+        // Log some links to Etherscan and OpenSea.
         string memory openSeaPrefix =
             block.chainid == 5 ? "https://testnets.opensea.io/assets/goerli/" : "https://opensea.io/assets/ethereum/";
 
         string memory etherscanPrefix =
             block.chainid == 5 ? "https://goerli.etherscan.io/address/" : "https://etherscan.io/address/";
 
+        // The `"\x1b[1m%s\x1b[0m"` causes the string to be printed in bold.
         console.log("\x1b[1m%s\x1b[0m", "Deployed an NFT contract at:");
         console.log(string(abi.encodePacked(etherscanPrefix, vm.toString(address(targetContract)))));
         console.log("");

diff --git a/src/Dockmaster.sol b/src/Dockmaster.sol
@@ -1,14 +1,22 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.17;
 
-import {json} from "lib/shipyard-core/src/onchain/json.sol";
-import {svg} from "lib/shipyard-core/src/onchain/svg.sol";
 import {LibString} from "solady/utils/LibString.sol";
 import {Base64} from "solady/utils/Base64.sol";
 import {Solarray} from "solarray/Solarray.sol";
+import {json} from "lib/shipyard-core/src/onchain/json.sol";
+import {svg} from "lib/shipyard-core/src/onchain/svg.sol";
 import {Metadata, DisplayType} from "lib/shipyard-core/src/onchain/Metadata.sol";
 import {AbstractNFT} from "lib/shipyard-core/src/reference/AbstractNFT.sol";
 
+/**
+ * @title Dockmaster
+ * @dev This is an example NFT contract that demonstrates how to use the
+ *      AbstractNFT contract to create an NFT with onchain metadata and onchain
+ *      dynamic traits. It's not meant to be inherited from or otherwise used in
+ *      production. It's recommended to leave it in place as you work through
+ *      the tutorial, then rip out all Dockmaster related contracts and tests.
+ */
 contract Dockmaster is AbstractNFT {
     using LibString for string;
     using LibString for uint256;
@@ -25,23 +33,34 @@ contract Dockmaster is AbstractNFT {
         emit Hail(string(abi.encodePacked("Hi Mom! I'm deploying my very own ", __name, " contract!")));
     }
 
-    function name() public view override returns (string memory) {
-        return _name;
-    }
-
-    function symbol() public view override returns (string memory) {
-        return _symbol;
-    }
-
+    /**
+     * @dev Returns the URI for a given token ID. It's not necessary to override
+     *      this function, but this contract overrides it to demonstrate that
+     *      it's possible to change the opinions of AbstractNFT, which doesn't
+     *      throw in the event that a supplied token ID doesn't exist.
+     *
+     * @param tokenId The token ID to get the URI for
+     *
+     * @return The URI for the given token ID
+     */
     function tokenURI(uint256 tokenId) public view override returns (string memory) {
         if (tokenId > currentId) {
-            // TokenDoesNotExist would be better, this is just to demonstrate a
-            // revert with a custom message.
+            // The exisintg TokenDoesNotExist error would be better, this is
+            // just to demonstrate a revert with a custom message.
             revert("Token ID does not exist");
         }
         return _stringURI(tokenId);
     }
 
+    /**
+     * @dev Internal helper function to get the raw JSON metadata for a given
+     *      token ID. If this function is not overridden, the default "Example
+     *      NFT" metadata will be returned.
+     *
+     * @param tokenId The token ID to get URI for
+     *
+     * @return The raw JSON metadata for the given token ID
+     */
     function _stringURI(uint256 tokenId) internal view override returns (string memory) {
         return json.objectOf(
             Solarray.strings(
@@ -52,12 +71,20 @@ contract Dockmaster is AbstractNFT {
                         "This is an NFT on the Dockmaster NFT contract. Its slip number is ", tokenId.toString(), "."
                     )
                 ),
+                // Note that the image is a base64-encoded SVG
                 json.property("image", Metadata.base64SvgDataURI(_image(tokenId))),
                 _attributes(tokenId)
             )
         );
     }
 
+    /**
+     * @dev Helper function to get both the static and dynamic attributes for a
+     *      given token ID. It's pulled out for readability and to avoid stack
+     *      pressure issues.
+     *
+     * @param tokenId The token ID to get the static and dynamic attributes for
+     */
     function _attributes(uint256 tokenId) internal view override returns (string memory) {
         string[] memory staticTraits = _staticAttributes(tokenId);
         string[] memory dynamicTraits = _dynamicAttributes(tokenId);
@@ -72,8 +99,11 @@ contract Dockmaster is AbstractNFT {
     }
 
     /**
-     * @notice Helper function to get the static attributes for a given token ID
+     * @dev Helper function to get the static attributes for a given token ID.
+     *
      * @param tokenId The token ID to get the static attributes for
+     *
+     * @return The static attributes for the given token ID
      */
     function _staticAttributes(uint256 tokenId) internal view virtual override returns (string[] memory) {
         return Solarray.strings(
@@ -82,6 +112,13 @@ contract Dockmaster is AbstractNFT {
         );
     }
 
+    /**
+     * @dev Helper function to get the image for a given token ID.
+     *
+     * @param tokenId The token ID to get the image for
+     *
+     * @return The image for the given token ID
+     */
     function _image(uint256 tokenId) internal pure override returns (string memory) {
         return svg.top({
             props: string.concat(svg.prop("width", "500"), svg.prop("height", "500")),
@@ -104,6 +141,16 @@ contract Dockmaster is AbstractNFT {
         });
     }
 
+    /**
+     * @dev The function to call to bring new tokens into existence. This
+     *      function must be overridden, or the contract will not compile. The
+     *      compiler error message would be "Error (3656): Contract "Dockmaster"
+     *      should be marked as abstract."
+     *
+     * @param to The address to mint the token to. If the null address is
+     *           supplied, the token will be minted to the address that called
+     *           this function.
+     */
     function mint(address to) public {
         // Only the contract owner and addresses with the two leading zeros can
         // mint tokens.
@@ -112,14 +159,22 @@ contract Dockmaster is AbstractNFT {
             revert UnauthorizedMinter();
         }
 
+        // If the null address is supplied, mint to the caller.
         to = to == address(0) ? msg.sender : to;
 
         unchecked {
             _mint(to, ++currentId);
         }
     }
 
-    function _isOwnerOrApproved(uint256 tokenId, address addr) internal view virtual override returns (bool) {
+    /**
+     * @dev Internal function that's used in Dynamic Traits to determine whether
+     *      a given address is allowed to set or delete a trait for a given
+     *      token ID. This function must be overridden, or the contract will not
+     *      compile. The compiler error message would be "Error (3656): Contract
+     *      "Dockmaster" should be marked as abstract."
+     */
+    function _isOwnerOrApproved(uint256 tokenId, address addr) internal view override returns (bool) {
         return ownerOf(tokenId) == addr || getApproved(tokenId) == addr || isApprovedForAll(ownerOf(tokenId), addr);
     }
 

diff --git a/src/DockmasterInterface.sol b/src/DockmasterInterface.sol
@@ -41,7 +41,7 @@ interface DockmasterInterface {
     function balanceOf(address owner) external view returns (uint256 result);
     function cancelOwnershipHandover() external payable;
     function completeOwnershipHandover(address pendingOwner) external payable;
-    function currentId() external view returns (uint256 result);
+    function currentId() external view returns (uint256);
     function deleteTrait(bytes32 traitKey, uint256 tokenId) external;
     function getApproved(uint256 id) external view returns (address result);
     function getCustomEditorAt(uint256 index) external view returns (address);

diff --git a/test-ffi/Dockmaster.t.sol b/test-ffi/Dockmaster.t.sol
@@ -42,10 +42,13 @@ contract DockmasterTest is Test {
         // deleted at the end of the test.
         string memory fileName = _fileName(tokenId);
 
+        // Populate the temp file with the json.
         _populateTempFileWithJson(tokenId, fileName);
 
+        // Get the name, description, and image from the json.
         (string memory name, string memory description, string memory image) = _getNameDescriptionAndImage(fileName);
 
+        // Check the name, description, and image against expectations.
         assertEq(name, _generateExpectedTokenName(tokenId), "The token name should be Dockmaster NFT #<tokenId>");
         assertEq(
             description,
@@ -58,6 +61,7 @@ contract DockmasterTest is Test {
         );
         assertEq(image, _generateExpectedTokenImage(tokenId), "The image is incorrect.");
 
+        // Set up the expectations for the two static traits.
         Attribute[] memory attributes = new Attribute[](2);
 
         attributes[0] = Attribute({attrType: "Slip Number", value: vm.toString(tokenId), displayType: "number"});
@@ -67,12 +71,15 @@ contract DockmasterTest is Test {
             displayType: "noDisplayType"
         });
 
+        // Check for the two static traits.
         _checkAttributesAgainstExpectations(tokenId, attributes, fileName);
     }
 
     function testDynamicMetadata(uint256 tokenId) public {
         tokenId = bound(tokenId, 1, 10);
 
+        // Create and set a new trait label on the contract.
+
         // Build the trait label.
         string[] memory acceptableValues = new string[](2);
         acceptableValues[0] = "True";
@@ -174,43 +181,14 @@ contract DockmasterTest is Test {
             displayType: "noDisplayType"
         });
 
+        // Check for the two static traits.
         _checkAttributesAgainstExpectations(tokenId, attributes, fileNameDeletedState);
     }
 
     ////////////////////////////////////////////////////////////////////////////
-    //                          Helpers                                       //
+    //                         ffi Helpers                                    //
     ////////////////////////////////////////////////////////////////////////////
 
-    function _populateTempFileWithJson(uint256 tokenId, string memory file) internal {
-        // Get the raw URI response.
-        string memory rawUri = dockmaster.tokenURI(tokenId);
-
-        // Write the decoded json to a file.
-        vm.writeFile(file, rawUri);
-    }
-
-    function _cleanedSvg(string memory uri) internal pure returns (string memory) {
-        uint256 stringLength;
-
-        // Get the length of the string from the abi encoded version.
-        assembly {
-            stringLength := mload(uri)
-        }
-
-        // Remove the "data:image/svg+xml;base64," prefix.
-        return _substring(uri, 26, stringLength);
-    }
-
-    function _substring(string memory str, uint256 startIndex, uint256 endIndex) public pure returns (string memory) {
-        bytes memory strBytes = bytes(str);
-
-        bytes memory result = new bytes(endIndex - startIndex);
-        for (uint256 i = startIndex; i < endIndex; i++) {
-            result[i - startIndex] = strBytes[i];
-        }
-        return string(result);
-    }
-
     function _getNameDescriptionAndImage(string memory file)
         internal
         returns (string memory name, string memory description, string memory image)
@@ -262,6 +240,56 @@ contract DockmasterTest is Test {
         }
     }
 
+    function _populateTempFileWithJson(uint256 tokenId, string memory file) internal {
+        // Get the raw URI response.
+        string memory rawUri = dockmaster.tokenURI(tokenId);
+
+        // Write the decoded json to a file.
+        vm.writeFile(file, rawUri);
+    }
+
+    function _cleanedSvg(string memory uri) internal pure returns (string memory) {
+        uint256 stringLength;
+
+        // Get the length of the string from the abi encoded version.
+        assembly {
+            stringLength := mload(uri)
+        }
+
+        // Remove the "data:image/svg+xml;base64," prefix.
+        return _substring(uri, 26, stringLength);
+    }
+
+    function _substring(string memory str, uint256 startIndex, uint256 endIndex) public pure returns (string memory) {
+        bytes memory strBytes = bytes(str);
+
+        bytes memory result = new bytes(endIndex - startIndex);
+        for (uint256 i = startIndex; i < endIndex; i++) {
+            result[i - startIndex] = strBytes[i];
+        }
+        return string(result);
+    }
+
+    function _fileName(uint256 tokenId) internal view returns (string memory) {
+        // Create a new file for each token ID and for each call possible token
+        // state. Using gasLeft() prevents collisions across tests imprefectly
+        // but tolerably. The token ID is for reference.
+        return string.concat(
+            TEMP_JSON_PATH_PREFIX, "-", vm.toString(gasleft()), "-", vm.toString(tokenId), TEMP_JSON_PATH_FILE_TYPE
+        );
+    }
+
+    function _cleanUp(string memory file) internal {
+        if (vm.exists(file)) {
+            vm.removeFile(file);
+        }
+        assertFalse(vm.exists(file));
+    }
+
+    ////////////////////////////////////////////////////////////////////////////
+    //                        Assertion Helpers                               //
+    ////////////////////////////////////////////////////////////////////////////
+
     function _generateExpectedTokenName(uint256 tokenId) internal pure returns (string memory) {
         return string(abi.encodePacked("Dockmaster NFT #", vm.toString(uint256(tokenId))));
     }
@@ -313,20 +341,4 @@ contract DockmasterTest is Test {
             )
         );
     }
-
-    function _fileName(uint256 tokenId) internal view returns (string memory) {
-        // Create a new file for each token ID and for each call possible token
-        // state. Using gasLeft() prevents collisions across tests imprefectly
-        // but tolerably. The token ID is for reference.
-        return string.concat(
-            TEMP_JSON_PATH_PREFIX, "-", vm.toString(gasleft()), "-", vm.toString(tokenId), TEMP_JSON_PATH_FILE_TYPE
-        );
-    }
-
-    function _cleanUp(string memory file) internal {
-        if (vm.exists(file)) {
-            vm.removeFile(file);
-        }
-        assertFalse(vm.exists(file));
-    }
 }