Add NFT contract and token metadata handling#69
Conversation
| function tokenURI(uint256) public pure override returns (string memory) { | ||
| return ""; | ||
| function tokenURI(uint256 tokenId) public view override returns (string memory) { | ||
| return bytes(baseURI).length > 0 ? string.concat(baseURI, tokenId.toString()) : ""; |
There was a problem hiding this comment.
should do an existence check, revert if know ownerOf
There was a problem hiding this comment.
Good call. Added onlyNonExpired modifier for reverting when queried for either an unowned or an expired name.
There was a problem hiding this comment.
Hm I feel like maybe we still have metadata for expired names and show through the metadata image/name that it is expired.
There was a problem hiding this comment.
I would be concerned as a prospective renter of an expired name that the token is just borked for some reason if no metadata loads for it vs. if it shows me intentionally it is expired, I feel more comfortable to rent it out.
There was a problem hiding this comment.
From an offline conversation with @ilikesymmetry I think we landed on this being sufficient.
Cases:
- Name has never been registered -- This will revert but we also don't expect that this method will be queried for any such token.
- Name has been registered but is expired -- This will revert but the previous metadata will be cached on opensea
- Name is registered: return happily
There was a problem hiding this comment.
One thought is to format the url param for tokenId as hex versus a raw string conversion of uint256. This will help keep urls shorter, but that's more for aesthetic than anything else. I've found it unwieldy sharing ENS token links before because of this but maybe that's just me.
There was a problem hiding this comment.
I think formatting as strings rather than hex will lead to less headaches
There was a problem hiding this comment.
How will this revert if I pass a tokenId that hasn't been minted?
There was a problem hiding this comment.
/// @notice Decorator for determining if a name has expired.
///
/// @param id The id being checked for expiry.
modifier onlyNonExpired(uint256 id) {
if (nameExpires[id] <= block.timestamp) revert Expired(id);
_;
}nameExpires[id] == 0 for tokens that haven't been minted.
ilikesymmetry
left a comment
ilikesymmetry
left a comment
There was a problem hiding this comment.
Most important imo is to just follow convention set by ENS. I'm not sure how they handle behavior for previously-owned, expired tokens, but not returning any metadata for those feels weird to me.
Additionally, I think we should use 4906 to be more indexer-friendly if we do ever make refactors to the metadata in the future.
stevieraykatz
changed the title
stevieraykatz
changed the title
ilikesymmetry
left a comment
ilikesymmetry
left a comment
There was a problem hiding this comment.
Only remaining thought from me is follow through on editing the deployment scripts PR to use the proper uri arguments now that the constructor is different
|
Review Error for ilikesymmetry @ 2024-07-22 20:01:18 UTC |
|
Review Error for wilsoncusack @ 2024-07-22 21:14:46 UTC |
cb-elileers
left a comment
cb-elileers
left a comment
There was a problem hiding this comment.
Might need a change on the using LibString, need @stevieraykatz to doublecheck.
Otherwise, looks good from security standpoint. Just adds a couple of string vars, and functions to read them publicly and update them as Owner.
|
Review Error for cb-elileers @ 2024-07-23 20:11:05 UTC |
5 participants
Our secondary marketplace experience is extremely lacking without token metadata. In lieu of setting metadata through proving ownership of the contracts with OpenSea (and for each other marketplace), we should just support standard on-chain metadata standards.
For reference: existing BaseRegistrar opensea experience https://testnets.opensea.io/collection/unidentified-contract-360b18e5-733c-4d39-b08e-dacf
Desired cleanliness: https://opensea.io/collection/ens