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

Add ERC721URIStorage extension #2555

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

Amxx
Copy link
Collaborator

@Amxx Amxx commented Mar 3, 2021

Fixes #2554

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx Amxx changed the title add ERC721TokenUri extension Add ERC721TokenUri extension Mar 3, 2021
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

It seems the contract was designed as fully compatible with Contracts 3.x, but I thought the only thing we wanted to preserve was the ability to customize the URI per-token, so only _setTokenURI. Defining the base URI through overriding is a better mechanism for gas, and I don't think we lose a significant feature if we remove _setBaseURI.

And I would consider defining tokenURI so that it delegates to super.tokenURI in the fallback case.

I would also remove the public baseURI.

Regarding the name, I was thinking something like ERC721URIStorage?

@@ -7,7 +7,19 @@ import "../token/ERC721/extensions/ERC721Burnable.sol";
contract ERC721BurnableMock is ERC721Burnable {
constructor(string memory name, string memory symbol) ERC721(name, symbol) { }

function exists(uint256 tokenId) public view returns (bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes related to the PR? I don't understand why they're here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some ERC721 mocks make all internal functions available, other don't. I wanted to improve consistency by making all the mocks show all the functions.

@frangio frangio changed the title Add ERC721TokenUri extension Add ERC721URIStorage extension Mar 3, 2021
@frangio frangio merged commit 1705067 into OpenZeppelin:master Mar 3, 2021
Amxx added a commit that referenced this pull request Mar 3, 2021
@Amxx Amxx deleted the feature/ERC721TokenUri branch March 3, 2021 15:16
@sambacha
Copy link

sambacha commented Mar 8, 2021

Any chance this will be backported? 

Thanks!

@Amxx
Copy link
Collaborator Author

Amxx commented Mar 8, 2021

Hello @sambacha,

The ERC721 implementation that is part of previous version of contracts (3.x) does include this by default. We are only adding this as a separate module because this logic was removed from the base ERC721 implementation in 4.0.

What version of contracts are you using, and what exactly do you find missing ?

@superphly
Copy link

superphly commented Mar 12, 2021

So, how does one implement this contract without having to mutate the other contracts in the repo? I'm currently developing on this 4.0 branch, but I don't want a mutated copy.

@frangio
Copy link
Contributor

frangio commented Mar 15, 2021

@superphly I'm not sure I understood your question! But this is an example of how you would use this contract:

import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";

contracts MyNFT is ERC721URIStorage {
  constructor() ERC721("Name", "SYM") 
}

It's not necessary to add ERC721 in the inheritance list because it is implied by ERC721URIStorage.

If you have any further questions feel free to ask for help in the OpenZeppelin Forum.

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.

ERC721TokenUri extension to ERC721
4 participants