Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(world-modules): properly concat baseURI and tokenURI in ERC721 module #2686

Merged
merged 4 commits into from
Apr 25, 2024

Conversation

Kooshaba
Copy link
Contributor

  • previously tokenURI was not properly returning concat(baseURI, tokenURI)
  • this uses a random uintToString function i found, lmk if you want to use OpenZeppelin or something
  • launch blocker for Sky Strife

Copy link

changeset-bot bot commented Apr 18, 2024

🦋 Changeset detected

Latest commit: 028e08f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 24 packages
Name Type
@latticexyz/world-modules Patch
@latticexyz/cli Patch
mock-game-contracts Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/dev-tools Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/query Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
@latticexyz/services Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/store-indexer Patch
@latticexyz/store-sync Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Kooshaba Kooshaba changed the title fix: convert uint to string in tokenURI fix: properly concat baseURI and tokenURI in ERC721 mododule Apr 18, 2024
@Kooshaba Kooshaba changed the title fix: properly concat baseURI and tokenURI in ERC721 mododule fix: properly concat baseURI and tokenURI in ERC721 module Apr 18, 2024
@alvrs
Copy link
Member

alvrs commented Apr 18, 2024

his uses a random uintToString function i found, lmk if you want to use OpenZeppelin or something

Would definitely prefer the OZ implementation if possible, looks like we can just use this https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Strings.sol#L24 (would just copy over the Math and String lib from there and refer back to the source in the header, so we don't have to add more remappings in consuming projects)

@@ -64,7 +64,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 : uintToString(tokenId);
Copy link
Contributor

@yonadaa yonadaa Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreeing with @alvrs, we can lift the Strings library that OZ uses here in their ERC721 implementation.

In MUD we've copied a lot of files.

yonadaa
yonadaa previously approved these changes Apr 22, 2024
holic
holic previously approved these changes Apr 25, 2024
@holic holic changed the title fix: properly concat baseURI and tokenURI in ERC721 module fix(world-modules): properly concat baseURI and tokenURI in ERC721 module Apr 25, 2024
@holic holic merged commit 78a94d7 into main Apr 25, 2024
11 of 13 checks passed
@holic holic deleted the kooshaba/fix-token-uri branch April 25, 2024 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants