Add ERC721URIStorage extension #2555
Conversation
There was a problem hiding this comment.
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) { | |||
There was a problem hiding this comment.
Are these changes related to the PR? I don't understand why they're here.
There was a problem hiding this comment.
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.
(cherry picked from commit 1705067)
|
Any chance this will be backported? Thanks! |
|
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 ? |
|
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. |
|
@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 If you have any further questions feel free to ask for help in the OpenZeppelin Forum. |
Fixes #2554
PR Checklist