fix(world-modules): properly concat baseURI and tokenURI in ERC721 mo…

…dule (#2686)

Co-authored-by: Fraser Scott <fraserdscott@gmail.com>
Co-authored-by: Kevin Ingersoll <kingersoll@gmail.com>

.changeset/kind-fireants-battle.md

@@ -0,0 +1,5 @@
+---
+"@latticexyz/world-modules": patch
+---
+
+Fixed ERC721 module to properly encode token ID as part of token URI.

packages/world-modules/src/modules/erc721-puppet/ERC721System.sol

@@ -21,9 +21,11 @@ import { TokenApproval } from "./tables/TokenApproval.sol";
 import { TokenURI } from "./tables/TokenURI.sol";
 
 import { _balancesTableId, _metadataTableId, _tokenUriTableId, _operatorApprovalTableId, _ownersTableId, _tokenApprovalTableId } from "./utils.sol";
+import { LibString } from "./libraries/LibString.sol";
 
 contract ERC721System is IERC721Mintable, System, PuppetMaster {
   using WorldResourceIdInstance for ResourceId;
+  using LibString for uint256;
 
   /**
    * @dev See {IERC721-balanceOf}.
@@ -64,7 +66,7 @@ contract ERC721System is IERC721Mintable, System, PuppetMaster {
 
     string memory baseURI = _baseURI();
     string memory _tokenURI = TokenURI.get(_tokenUriTableId(_namespace()), tokenId);
-    _tokenURI = bytes(_tokenURI).length > 0 ? _tokenURI : string(abi.encodePacked(tokenId));
+    _tokenURI = bytes(_tokenURI).length > 0 ? _tokenURI : tokenId.toString();
     return bytes(baseURI).length > 0 ? string.concat(baseURI, _tokenURI) : _tokenURI;
   }
 

packages/world-modules/src/modules/erc721-puppet/libraries/LibString.sol

@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: MIT
+pragma solidity >=0.8.0;
+
+/// @notice Efficient library for creating string representations of integers.
+/// @author Solmate (https://github.com/transmissions11/solmate/blob/main/src/utils/LibString.sol)
+/// @author Modified from Solady (https://github.com/Vectorized/solady/blob/main/src/utils/LibString.sol)
+library LibString {
+  function toString(int256 value) internal pure returns (string memory str) {
+    if (value >= 0) return toString(uint256(value));
+
+    unchecked {
+      str = toString(uint256(-value));
+
+      /// @solidity memory-safe-assembly
+      assembly {
+        // Note: This is only safe because we over-allocate memory
+        // and write the string from right to left in toString(uint256),
+        // and thus can be sure that sub(str, 1) is an unused memory location.
+
+        let length := mload(str) // Load the string length.
+        // Put the - character at the start of the string contents.
+        mstore(str, 45) // 45 is the ASCII code for the - character.
+        str := sub(str, 1) // Move back the string pointer by a byte.
+        mstore(str, add(length, 1)) // Update the string length.
+      }
+    }
+  }
+
+  function toString(uint256 value) internal pure returns (string memory str) {
+    /// @solidity memory-safe-assembly
+    assembly {
+      // The maximum value of a uint256 contains 78 digits (1 byte per digit), but we allocate 160 bytes
+      // to keep the free memory pointer word aligned. We'll need 1 word for the length, 1 word for the
+      // trailing zeros padding, and 3 other words for a max of 78 digits. In total: 5 * 32 = 160 bytes.
+      let newFreeMemoryPointer := add(mload(0x40), 160)
+
+      // Update the free memory pointer to avoid overriding our string.
+      mstore(0x40, newFreeMemoryPointer)
+
+      // Assign str to the end of the zone of newly allocated memory.
+      str := sub(newFreeMemoryPointer, 32)
+
+      // Clean the last word of memory it may not be overwritten.
+      mstore(str, 0)
+
+      // Cache the end of the memory to calculate the length later.
+      let end := str
+
+      // We write the string from rightmost digit to leftmost digit.
+      // The following is essentially a do-while loop that also handles the zero case.
+      // prettier-ignore
+      for { let temp := value } 1 {} {
+                // Move the pointer 1 byte to the left.
+                str := sub(str, 1)
+
+                // Write the character to the pointer.
+                // The ASCII index of the '0' character is 48.
+                mstore8(str, add(48, mod(temp, 10)))
+
+                // Keep dividing temp until zero.
+                temp := div(temp, 10)
+
+                 // prettier-ignore
+                if iszero(temp) { break }
+            }
+
+      // Compute and cache the final total length of the string.
+      let length := sub(end, str)
+
+      // Move the pointer 32 bytes leftwards to make room for the length.
+      str := sub(str, 32)
+
+      // Store the string's length at the start of memory allocated for our string.
+      mstore(str, length)
+    }
+  }
+}

packages/world-modules/test/ERC721.t.sol

@@ -18,6 +18,7 @@ import { PuppetModule } from "../src/modules/puppet/PuppetModule.sol";
 import { ERC721Module } from "../src/modules/erc721-puppet/ERC721Module.sol";
 import { ERC721MetadataData } from "../src/modules/erc721-puppet/tables/ERC721Metadata.sol";
 import { IERC721Mintable } from "../src/modules/erc721-puppet/IERC721Mintable.sol";
+import { IERC721Metadata } from "../src/modules/erc721-puppet/IERC721Metadata.sol";
 import { registerERC721 } from "../src/modules/erc721-puppet/registerERC721.sol";
 import { IERC721Errors } from "../src/modules/erc721-puppet/IERC721Errors.sol";
 import { IERC721Events } from "../src/modules/erc721-puppet/IERC721Events.sol";
@@ -77,7 +78,11 @@ contract ERC721Test is Test, GasReporter, IERC721Events, IERC721Errors {
     StoreSwitch.setStoreAddress(address(world));
 
     // Register a new ERC721 token
-    token = registerERC721(world, "myERC721", ERC721MetadataData({ name: "Token", symbol: "TKN", baseURI: "" }));
+    token = registerERC721(
+      world,
+      "myERC721",
+      ERC721MetadataData({ name: "Token", symbol: "TKN", baseURI: "base-uri/" })
+    );
   }
 
   function _expectAccessDenied(address caller) internal {
@@ -164,6 +169,15 @@ contract ERC721Test is Test, GasReporter, IERC721Events, IERC721Errors {
     assertEq(token.ownerOf(id), owner);
   }
 
+  function testTokenURI(address owner) public {
+    vm.assume(owner != address(0));
+
+    token.mint(owner, 1);
+
+    IERC721Metadata tokenMetadata = IERC721Metadata(address(token));
+    assertEq(tokenMetadata.tokenURI(1), "base-uri/1");
+  }
+
   function testMintRevertAccessDenied(uint256 id, address owner, address operator) public {
     _assumeDifferentNonZero(owner, operator, address(this));