-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feralfile Exhibition V4 #19
Conversation
ac4b47c
to
08c7af9
Compare
…tract Remove ipfs and add vault contract
Send remaining funds to cost addr
/// @param tokenIds an array of token ID | ||
function burnArtworks(uint256[] memory tokenIds) external { | ||
for (uint256 i = 0; i < tokenIds.length; i++) { | ||
_burnArtwork(tokenIds[i]); |
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.
_burnArtwork(tokenIds[i]); | |
require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner or approved"); | |
_burnArtwork(tokenIds[i]); |
@jollyjoker992 the external function can be called for users so we need to check the ownership before burn tokens.
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.
interfaceId == type(IERC721Enumerable).interfaceId || | ||
super.supportsInterface(interfaceId); | ||
/// @notice Stop token selling and transfer remaining tokens back to the underlying addresses | ||
function stopSaleAndTransfer( |
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 a chance to stop the sale with only part of series transfer tokens to addresses. This means the sale can start with the remaining tokens again. I think we need to ensure either:
- the length of
seriesIds
is same to the length of_seriesMaxSupplies
- balanceOf(address(this)) == 0
- The first approach would need to introduce another key since we can not count the length of a map directly.
- The later one would waste more gas since we can only check after we burn all tokens.
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.
LGTM
contracts/FeralfileArtworkV4.sol
Outdated
address signer_, | ||
bool burnable_, | ||
bool bridgeable_, | ||
address vault_, | ||
address costReceiver_, |
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.
Small: put the same type params in order. though it wont save gas in function param, but it's better for reading
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.
@lpopo0856 I dont think so, we should put the params based on the meaning like something is similar could put together. In this case, i will bring the signer next to vault and cost receiver.
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.
ok!
contracts/FeralfileArtworkV4.sol
Outdated
} | ||
|
||
/// @notice Set vault contract | ||
/// @dev don't allow to set signer as zero address |
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.
typo: set vault
contracts/FeralfileArtworkV4.sol
Outdated
resumeSale(); | ||
} | ||
|
||
/// @notice Resume token selling |
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.
small: consistent wording between selling
and sale
contracts/FeralfileArtworkV4.sol
Outdated
_selling = false; | ||
} | ||
|
||
/// @notice Stop token selling and burn remaining tokens |
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.
same here
} | ||
} | ||
|
||
/// @notice Stop token selling and transfer remaining tokens back to the underlying addresses |
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.
same here
/// @notice the cost receiver address | ||
/// @param costReceiver_ - the address of cost receiver | ||
function setCostReceiver(address costReceiver_) external onlyOwner { | ||
costReceiver = costReceiver_; |
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.
missing costReceiver_ != address(0)
check
function setTokenBaseURI(string memory baseURI_) external onlyAuthorized { | ||
_tokenBaseURI = baseURI_; | ||
function setTokenBaseURI(string memory baseURI_) external onlyOwner { | ||
tokenBaseURI = 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.
missing bytes(baseURI_).length > 0
check
contracts/FeralfileArtworkV4.sol
Outdated
for (uint16 j = 0; j < seriesIds.length; j++) { | ||
if (artwork.seriesId == seriesIds[j]) { | ||
address to = recipientAddresses[j]; | ||
_transfer(from, to, tokenId); |
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.
why not using _safeTransfer
?
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.
LGTM
No description provided.