PlumeGoon #160
PlumeGoon #160
Conversation
ungaro
left a comment
ungaro
left a comment
There was a problem hiding this comment.
can we add:
- proxy contract
- deployment and upgrade scripts
- tests
thanks.
|
|
||
| require(ps.hasMinted[user], "User should have already minted an NFT to reset them"); | ||
| require(balanceOf(user) > 0, "User should have already minted an NFT to reset them"); | ||
| require(ownerOf(burnTokenId) == user, "Incorrect Token Id provided for reset"); |
There was a problem hiding this comment.
can we change this to custom errors?
error UserHasNotMinted();
error UserHasNoTokens();
error IncorrectTokenOwner();
if (!ps.hasMinted[user]) revert UserHasNotMinted();
if (balanceOf(user) == 0) revert UserHasNoTokens();
if (ownerOf(burnTokenId) != user) revert IncorrectTokenOwner();
| import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; | ||
| import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; | ||
| import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; | ||
| import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721BurnableUpgradeable.sol"; |
There was a problem hiding this comment.
ERC721BurnableUpgradeable.sol is not inherited or used
| mapping(bytes32 => bool) usedNonces; | ||
| } | ||
|
|
||
| // keccak256(abi.encode(uint256(keccak256("plume.storage.PlumeStorage")) - 1)) & ~bytes32(uint256(0xff)) |
There was a problem hiding this comment.
should be plume.storage.PlumeGoonStorage?
|
|
||
| _grantRole(DEFAULT_ADMIN_ROLE, _admin); | ||
| _grantRole(PAUSER_ROLE, pauser); | ||
| _grantRole(MINTER_ROLE, minter); |
There was a problem hiding this comment.
why do we have 2 roles for PAUSER_ROLE and MINTER_ROLE? can PAUSER_ROLE be UPGRADER_ROLE? I assume MINTER_ROLE is required.
| ) internal view returns (bool) { | ||
| PlumeGoonStorage storage ps = _getPlumeGoonStorage(); | ||
| bytes32 message = prefixed(keccak256(abi.encodePacked(_tokenUri, user, tier, nonce))); | ||
| return recoverSignerFromSignature(message, signature) == ps.admin; |
There was a problem hiding this comment.
recoverSignerFromSignature is not used anywhere else, maybe can put it here directly?
| ) internal override(ERC721Upgradeable, ERC721EnumerableUpgradeable, ERC721PausableUpgradeable) returns (address) { | ||
| return super._update(to, tokenId, auth); | ||
| } | ||
|
|
There was a problem hiding this comment.
can we add natspec comments for functions that have them missing:
/**
* @dev Internal function to increase the balance of an account
* @notice This function overrides both ERC721Upgradeable and ERC721EnumerableUpgradeable implementations
* @param account The address of the account whose balance should be increased
* @param value The amount to increase the balance by
* @inheritdoc ERC721Upgradeable
*/
| _grantRole(MINTER_ROLE, minter); | ||
| _grantRole(UPGRADER_ROLE, _admin); | ||
| } | ||
|
|
There was a problem hiding this comment.
can you add reinitialize for upgrading?
| bytes32 nonce | ||
| ) internal view returns (bool) { | ||
| PlumeGoonStorage storage ps = _getPlumeGoonStorage(); | ||
| bytes32 message = prefixed(keccak256(abi.encodePacked(_tokenUri, user, tier, nonce))); |
| { | ||
| return super.supportsInterface(interfaceId); | ||
| } | ||
|
|
There was a problem hiding this comment.
should we add _beforeTokenTransfer override? not sure, since there is no compilation error.
| modifier _onlyWithAdminSignature(string memory _tokenUri, address user, uint8 tier, bytes memory signature, bytes32 nonce) { | ||
| PlumeGoonStorage storage ps = _getPlumeGoonStorage(); | ||
| require(!isNonceUsed(nonce), "Nonce already used"); | ||
| require(onlySignedByAdmin(_tokenUri, user, tier, signature, nonce), "Invalid signature"); |
Old goon nft contract