feat: initial implementation of ERC1046#933
Conversation
| * @dev TODO - update https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC721/ERC721.sol#L17 when 1046 is finalized | ||
| */ | ||
| contract ERC20TokenMetadata is ERC20 { | ||
| function tokenURI() public view returns (string); |
There was a problem hiding this comment.
Note that this interface is public here rather than external as in the draft.
Are the minor gas savings worth it? https://ethereum.stackexchange.com/questions/19380/external-vs-public-best-practices
|
|
||
| function tokenURI() public view returns (string) { | ||
| // in the effort of minimum-effort compatibility, | ||
| // tokenURI MUST throw if not exists https://eips.ethereum.org/EIPS/eip-721 |
There was a problem hiding this comment.
how do we feel about this interpretation?
| function ERC20WithMetadata(string _tokenURI) | ||
| public | ||
| { | ||
| tokenURI_ = _tokenURI; |
There was a problem hiding this comment.
using a state variable with explicit getter for extensibility
|
👍 |
b3b65d4 to
2e3ed3c
Compare
2e3ed3c to
eda22e5
Compare
36e088a to
fcd5486
Compare
frangio
left a comment
There was a problem hiding this comment.
LGTM save for the revert in tokenURI. 🙂
|
|
||
| function tokenURI() external view returns (string) { | ||
| // in the effort of minimum-effort compatibility, | ||
| // tokenURI MUST throw if not exists https://eips.ethereum.org/EIPS/eip-721 |
There was a problem hiding this comment.
I don't understand how this is about compatibility. ER721 "throws if _tokenId is not a valid NFT". There's no _tokenId argument here, but as I understand it there is an implicit token id which is the token address, which sounds like a "valid token" under any interpretation.
I guess an empty string can never be a valid URI though. If that is the reason though, IMO we should check this in the setter of tokenURI_ and not when returning it. Or not check it at all, because we're not going to validate that it's a valid URI anyway.
There was a problem hiding this comment.
Yeah you're right, this check doesn't really make sense. Will update
59eb8bb to
4da5e11
Compare
| const invalidMetadataURI = ''; | ||
|
|
||
| describe('ERC20WithMetadata', function () { | ||
| context('invalid metadata', function () { |
There was a problem hiding this comment.
The last commit removed the need for testing for invalid metadata.
There was a problem hiding this comment.
thanks, that's what I get for letting travis do my testing for me 😅
| this.token.tokenURI() | ||
| ); | ||
| }); | ||
| before(async function () { |
There was a problem hiding this comment.
See #1021. I'm fine with merging and fixing afterwards though because there is only one test here.
* feat: initial implementation of ERC1046
🚀 Description
This is the initial implementation of ERC1046, which adds (optional)
tokenURIto ERC20 tokens to allow them to exist on-par with ERC721 tokens.https://eips.ethereum.org/EIPS/eip-1046
npm run lint:all:fix).