-
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
support new buy artworks function for John Gerrard show #42
Conversation
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.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
solhint
contracts/FeralfileVaultV2.sol|49 col 9| GC: Use Custom Errors instead of require statements
contracts/IFeralfileVaultV2.sol|4| global import of path @openzeppelin/contracts/access/Ownable.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
contracts/IFeralfileVaultV2.sol|6| global import of path ./IFeralfileSaleDataV2.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
contracts/IFeralfileVaultV2.sol|7| global import of path ./ECDSASigner.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
import {IFeralfileVaultV2} from "./IFeralfileVaultV2.sol"; | ||
import {FeralfileSaleDataV2} from "./FeralfileSaleDataV2.sol"; | ||
|
||
contract FeralfileExhibitionV4_2 is |
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.
Contract, Structs and Enums should be in CamelCase
|
||
mapping(uint256 => uint256) private seriesNextSaleTokenIds; // seriesID -> tokenID | ||
|
||
constructor( |
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.
Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function buyBulkArtworks( |
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 has cyclomatic complexity 11 but allowed no more than 7
contracts/FeralfileArtworkV4_2.sol
Outdated
uint8 v_, | ||
SaleDataV2 calldata saleData_ | ||
) external payable { | ||
require(_selling, "FeralfileExhibitionV4: sale is not started"); |
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.
Error message for require is too long: 42 counted / 32 allowed
abi.encode(block.chainid, msg.sender, saleData_) | ||
); | ||
require(!_paidSale[message], "FeralfileVault: paid sale"); | ||
require( |
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.
Error message for require is too long: 33 counted / 32 allowed
abi.encode(block.chainid, msg.sender, saleData_) | ||
); | ||
require(!_paidSale[message], "FeralfileVault: paid sale"); | ||
require( |
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.
GC: String exceeds 32 bytes
abi.encode(block.chainid, msg.sender, saleData_) | ||
); | ||
require(!_paidSale[message], "FeralfileVault: paid sale"); | ||
require( |
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.
GC: Use Custom Errors instead of require statements
} | ||
|
||
function withdrawFund(uint256 weiAmount) external onlyOwner { | ||
require( |
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.
Error message for require is too long: 36 counted / 32 allowed
} | ||
|
||
function withdrawFund(uint256 weiAmount) external onlyOwner { | ||
require( |
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.
GC: String exceeds 32 bytes
634b9a4
to
399bdff
Compare
} | ||
|
||
function withdrawFund(uint256 weiAmount) external onlyOwner { | ||
require( |
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.
GC: Use Custom Errors instead of require statements
74e7534
to
712f255
Compare
} | ||
|
||
// send NFT | ||
_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.
There would be a case that the remaining purchasable tokens is not enough compare to the requested quantity, the tx would be failed eventually but it will cost much gas since we transferred the token and emitted event already. So i'd suggest to accumulate the purchasable tokens into an array then loop over them to transfer.
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.
this is a rare case that only happens when out of tokens. but storing tokens in an array and performing an additional loop would cost some additional gas for each transaction. Since it's a rare case, so I think it would be more efficient to keep the transfer inside this loop.
contracts/FeralfileArtworkV4_2.sol
Outdated
continue; | ||
} | ||
distributedRevenue += totalRev; | ||
payable(recipient).transfer(totalRev); |
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.
Should accumulate into one tx for the same recipient.
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.
we only use 1 revenue shares array for all the tokens, so we don't need to accumulate
anymore
contracts/FeralfileArtworkV4_2.sol
Outdated
} | ||
|
||
/// @notice Event emitted when Artwork has been sold with the additional nonce | ||
event BuyArtworkV2(address indexed buyer, uint256 indexed tokenId, uint256 nonce); |
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.
Should be BuyBulkArtworks
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.
currently, the event is for each single token
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 What would you prefer? Single event for this bulk sale or multiple events for individual?
import {IFeralfileSaleDataV2} from "./IFeralfileSaleDataV2.sol"; | ||
|
||
contract FeralfileSaleDataV2 is IFeralfileSaleDataV2 { | ||
function validateSaleDataV2(SaleDataV2 calldata saleData_) internal view { |
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.
Should be validateSaleData
.
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function payForSaleV2( |
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.
Should be payForSale
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.
the payload is different from the old Vault, so I changed the name to reduce the mistake when calling.
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.
Unless it makes us confused, otherwise i dont see any reason to name it with the version suffix.
This function belongs to separate contract FeralfileVaultV2
that actually doesn't make us confused when using it.
import {IFeralfileSaleData} from "./IFeralfileSaleData.sol"; | ||
|
||
interface IFeralfileSaleDataV2 is IFeralfileSaleData { | ||
struct SaleDataV2 { |
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.
Should be SaleData
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.
Since the SaleData
name has already been used, we need a different name for this struct.
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. I thought you could use separate interface but it seems like we have to reuse the RevenueShare
21c2b0b
to
fff85ef
Compare
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function buyBulkArtworks( |
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 has cyclomatic complexity 15 but allowed no more than 7
} | ||
|
||
/// @notice Event emitted when Artwork has been sold with the additional nonce | ||
event BuyArtworkV2( |
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.
GC: [nonce] on Event [BuyArtworkV2] could be Indexed
10819c9
to
fc62a43
Compare
5ec5d8c
to
1b8902e
Compare
5e681b1
to
1e50e63
Compare
a692573
to
d875e32
Compare
eb2b72b
to
68109c5
Compare
42e85ba
to
6df789a
Compare
2f69db2
to
46673c9
Compare
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function buyBulkArtworks( |
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 has cyclomatic complexity 16 but allowed no more than 7
seriesMaxSupplies_ | ||
) | ||
{ | ||
vaultV2 = IFeralfileVaultV2(payable(vault_)); |
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.
Minor: check equal length of seriesIds_ and seriesNextPurchasableTokenIds_
if ( | ||
saleData_.price - saleData_.cost < | ||
distributedRevenue + platformRevenue | ||
) { | ||
revert TotalBpsOver(); | ||
} |
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.
It's a miss from long time ago that we should add a check for saleData_.price > saleData_.cost
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.
or we could discuss should we allow to submit a buy tx that cost larger than price.
contracts/FeralfileArtworkV4_2.sol
Outdated
? remainingRev | ||
: remainingAdvanceAmount; | ||
uint256 totalRev = saleData_.quantity * rev; | ||
platformRevenue += totalRev; |
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.
we have some logic error her, discuss in slack
9af45c5
to
806cc26
Compare
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function buyBulkArtworks( |
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 has cyclomatic complexity 18 but allowed no more than 7
53affbe
to
03f1e4e
Compare
/// @param s_ - part of signature for validating parameters integrity | ||
/// @param v_ - part of signature for validating parameters integrity | ||
/// @param saleData_ - the sale data | ||
function buyBulkArtworks( |
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 has cyclomatic complexity 17 but allowed no more than 7
04bf821
to
b4f3da0
Compare
Story link: https://github.com/bitmark-inc/feral-file/issues/2476