-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: Update tokenURI mechanism #103
Conversation
contracts/NestedAsset.sol
Outdated
|
||
/// @dev Stores the original asset of each asset | ||
mapping(uint256 => uint256) public originalAsset; | ||
|
||
/// @dev Stores owners of burnt assets | ||
mapping(uint256 => address) public lastOwnerBeforeBurn; | ||
|
||
/// @dev True if using the API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 "using the API" is not very explicit. Consider rewording this
Same for L20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
function _baseURI() internal view override returns (string memory) { | ||
return baseUri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Is this function really necessary? If used by the ERC721 contract, shouldn't it be declared in it already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because used in super.tokenURI()
contracts/NestedAsset.sol
Outdated
function reveal() external onlyOwner { | ||
isRevealed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no mechanism to unreveal to correct a mistake. Consider changing this function to setIsRevealed(bool _isRevealed)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…om known issues It was handled in MassDotMoney/nested-core-lego#103
It was handled in MassDotMoney/nested-core-lego#103
We updated the mechanism to set the
tokenURI
:mintWithMetadata
andbackfillTokenURI
(with the_tokenURIs
map).