Skip to content
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

Merged
merged 11 commits into from
Jun 17, 2024
6 changes: 6 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@ merge: sol-merger-check
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralfileArtworkV2.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralfileArtworkV3.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralfileArtworkV4_1.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralfileArtworkV4_2.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralfileEnglishAuction.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/FeralFileAirdropV1.sol /tmp/sol-merger && \
sol-merger --export-plugin SPDXLicenseRemovePlugin ./contracts/OwnerData.sol /tmp/sol-merger && \
mv /tmp/sol-merger/FeralfileArtworkV2.sol ./contracts/sol-merger/FeralfileExhibitionV2.sol && \
mv /tmp/sol-merger/FeralfileArtworkV3.sol ./contracts/sol-merger/FeralfileExhibitionV3.sol && \
mv /tmp/sol-merger/FeralfileArtworkV4_1.sol ./contracts/sol-merger/FeralfileExhibitionV4.sol && \
mv /tmp/sol-merger/FeralfileArtworkV4_2.sol ./contracts/sol-merger/FeralfileExhibitionV4_2.sol && \
mv /tmp/sol-merger/FeralfileEnglishAuction.sol ./contracts/sol-merger/FeralfileEnglishAuction.sol && \
mv /tmp/sol-merger/FeralFileAirdropV1.sol ./contracts/sol-merger/FeralFileAirdropV1.sol && \
mv /tmp/sol-merger/OwnerData.sol ./contracts/sol-merger/OwnerData.sol
Expand All @@ -41,6 +43,8 @@ build-contract: check
jq -r ".abi" build/contracts/FeralfileExhibitionV3.json > ./build/FeralfileExhibitionV3.abi && \
jq -r ".bytecode" build/contracts/FeralfileExhibitionV4_1.json > ./build/FeralfileExhibitionV4.bin && \
jq -r ".abi" build/contracts/FeralfileExhibitionV4_1.json > ./build/FeralfileExhibitionV4.abi && \
jq -r ".bytecode" build/contracts/FeralfileExhibitionV4_2.json > ./build/FeralfileExhibitionV4_2.bin && \
jq -r ".abi" build/contracts/FeralfileExhibitionV4_2.json > ./build/FeralfileExhibitionV4_2.abi && \
jq -r ".bytecode" build/contracts/FeralfileEnglishAuction.json > ./build/FeralfileEnglishAuction.bin && \
jq -r ".abi" build/contracts/FeralfileEnglishAuction.json > ./build/FeralfileEnglishAuction.abi && \
jq -r ".bytecode" build/contracts/FeralFileAirdropV1.json > ./build/FeralFileAirdropV1.bin && \
Expand All @@ -52,12 +56,14 @@ build: build-contract
mkdir -p ./go-binding/feralfile-exhibition-v2 && \
mkdir -p ./go-binding/feralfile-exhibition-v3 && \
mkdir -p ./go-binding/feralfile-exhibition-v4 && \
mkdir -p ./go-binding/feralfile-exhibition-v4_2 && \
mkdir -p ./go-binding/feralfile-english-auction && \
mkdir -p ./go-binding/feralfile-airdrop-v1 && \
mkdir -p ./go-binding/owner-data && \
abigen --abi ./build/FeralfileExhibitionV2.abi --bin ./build/FeralfileExhibitionV2.bin --pkg feralfilev2 -type FeralfileExhibitionV2 --out ./go-binding/feralfile-exhibition-v2/abi.go
abigen --abi ./build/FeralfileExhibitionV3.abi --bin ./build/FeralfileExhibitionV3.bin --pkg feralfilev3 -type FeralfileExhibitionV3 --out ./go-binding/feralfile-exhibition-v3/abi.go
abigen --abi ./build/FeralfileExhibitionV4.abi --bin ./build/FeralfileExhibitionV4.bin --pkg feralfilev4 -type FeralfileExhibitionV4 --out ./go-binding/feralfile-exhibition-v4/abi.go
abigen --abi ./build/FeralfileExhibitionV4_2.abi --bin ./build/FeralfileExhibitionV4_2.bin --pkg feralfilev4_2 -type FeralfileExhibitionV4_2 --out ./go-binding/feralfile-exhibition-v4_2/abi.go
abigen --abi ./build/FeralfileEnglishAuction.abi --bin ./build/FeralfileEnglishAuction.bin --pkg english_auction -type FeralfileEnglishAuction --out ./go-binding/feralfile-english-auction/abi.go
abigen --abi ./build/FeralFileAirdropV1.abi --bin ./build/FeralFileAirdropV1.bin --pkg airdropv1 -type FeralFileAirdropV1 --out ./go-binding/feralfile-airdrop-v1/abi.go && \
abigen --abi ./build/OwnerData.abi --bin ./build/OwnerData.bin --pkg ownerdata -type OwnerData --out ./go-binding/owner-data/abi.go
2 changes: 1 addition & 1 deletion contracts/FeralfileArtworkV4.sol
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ contract FeralfileExhibitionV4 is

/// @notice Set vault contract
/// @dev don't allow to set vault as zero address
function setVault(address vault_) external onlyOwner {
function setVault(address vault_) external virtual onlyOwner {
require(
vault_ != address(0),
"FeralfileExhibitionV4: vault_ is zero address"
Expand Down
2 changes: 1 addition & 1 deletion contracts/FeralfileArtworkV4_1.sol
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ contract FeralfileExhibitionV4_1 is FeralfileExhibitionV4 {
bytes32 s_,
uint8 v_,
SaleData calldata saleData_
) external payable override {
) external payable override virtual {
require(_selling, "FeralfileExhibitionV4: sale is not started");
super._checkContractOwnedToken();
validateSaleData(saleData_);
Expand Down
222 changes: 222 additions & 0 deletions contracts/FeralfileArtworkV4_2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import {Nonces} from "./Nonces.sol";
import {FeralfileExhibitionV4_1} from "./FeralfileArtworkV4_1.sol";
import {IFeralfileVaultV2} from "./IFeralfileVaultV2.sol";
import {FeralfileSaleDataV2} from "./FeralfileSaleDataV2.sol";

contract FeralfileExhibitionV4_2 is
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Contract, Structs and Enums should be in CamelCase

FeralfileExhibitionV4_1,
FeralfileSaleDataV2,
Nonces
{
error NotEnoughToken();
error TokenIDNotFound();
error FunctionNotSupported();
error SaleNotStarted();
error InvalidPaymentAmount();
error TotalBpsOver();
error InvalidAddress();

// vault contract instance
IFeralfileVaultV2 public vaultV2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.
There is no another vault v1 in this contract so it doesn't make us confused, so it should be named as vault and setVault is clear enough and remove the confusion.


mapping(uint256 => uint256) private seriesNextPurchasableTokenIds; // seriesID -> tokenID

constructor(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)

string memory name_,
string memory symbol_,
bool burnable_,
bool bridgeable_,
address signer_,
address vault_,
address costReceiver_,
string memory contractURI_,
uint256[] memory seriesIds_,
uint256[] memory seriesMaxSupplies_,
uint256[] memory seriesNextPurchasableTokenIds_
)
FeralfileExhibitionV4_1(
name_,
symbol_,
burnable_,
bridgeable_,
signer_,
vault_,
costReceiver_,
contractURI_,
seriesIds_,
seriesMaxSupplies_
)
{
vaultV2 = IFeralfileVaultV2(payable(vault_));
Copy link
Contributor

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_

for (uint256 i = 0; i < seriesIds_.length; i++) {
seriesNextPurchasableTokenIds[seriesIds_[i]] = seriesNextPurchasableTokenIds_[i];
}
}

/// @notice Set vaultV2 contract
/// @dev don't allow to set vaultV2 as zero address
function setVaultV2(address vault_) external onlyOwner {
if (vault_ == address(0)) {
revert InvalidAddress();
}

vaultV2 = IFeralfileVaultV2(payable(vault_));
}

/// @notice override revert setVault
function setVault(address) external pure override {
revert FunctionNotSupported();
}

/// @notice pay to get artworks to a destination address. The pricing, costs and other details is included in the saleData
/// @param r_ - part of signature for validating parameters integrity
/// @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(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Function has cyclomatic complexity 11 but allowed no more than 7

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Function has cyclomatic complexity 15 but allowed no more than 7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Function has cyclomatic complexity 16 but allowed no more than 7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Function has cyclomatic complexity 18 but allowed no more than 7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Function has cyclomatic complexity 17 but allowed no more than 7

bytes32 r_,
bytes32 s_,
uint8 v_,
SaleDataV2 calldata saleData_
) external payable {
if (!_selling) {
revert SaleNotStarted();
}

uint256 balance = balanceOf(address(this));
if (balance < saleData_.quantity) {
revert NotEnoughToken();
}

validateSaleDataV2(saleData_);

bytes32 message = keccak256(
abi.encode(block.chainid, address(this), saleData_)
);

if (!isValidSignature(message, r_, s_, v_)) {
revert InvalidSignature();
}

//check nonce
_useCheckedNonce(saleData_.destination, saleData_.nonce);
hvthhien marked this conversation as resolved.
Show resolved Hide resolved

if (saleData_.payByVaultContract) {
vaultV2.payForSaleV2(r_, s_, v_, saleData_);
} else {
if (saleData_.price != msg.value) {
revert InvalidPaymentAmount();
}
}

uint256 totalRevenue;
if (saleData_.price > saleData_.cost) {
totalRevenue = saleData_.price - saleData_.cost;
}

uint256 nextPurchasableTokenId = seriesNextPurchasableTokenIds[saleData_.seriesID];
uint256 i = 0;
while (i < saleData_.quantity) {
uint256 tokenIdForSale = nextPurchasableTokenId;

if (!_exists(tokenIdForSale)) {
revert TokenIDNotFound();
}

nextPurchasableTokenId++;
if (ownerOf(tokenIdForSale) != address(this)) {
continue;
}

// send NFT
_safeTransfer(
Copy link
Member

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.

Copy link
Member Author

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.

address(this),
saleData_.destination,
tokenIdForSale,
""
);

emit BuyArtworkV2(
saleData_.destination,
tokenIdForSale,
saleData_.nonce
);
i++;
}

// save next sale token id for seriesID
seriesNextPurchasableTokenIds[saleData_.seriesID] = nextPurchasableTokenId;

// distribute royalty
uint256 distributedRevenue;
uint256 platformRevenue;

RevenueShare[] memory revenueShares = saleData_.revenueShares;
uint256 remainingRev = totalRevenue;

// deduct advances payment from revenue
for (uint256 j = 0; j < revenueShares.length && remainingRev > 0; j++) {
uint256 remainingAdvanceAmount = advances[
revenueShares[j].recipient
];

if (remainingAdvanceAmount == 0) {
continue;
}

uint256 prePaidRev = remainingAdvanceAmount >= remainingRev
? remainingRev
: remainingAdvanceAmount;
platformRevenue += prePaidRev;
advances[revenueShares[j].recipient] -= prePaidRev;
remainingRev -= prePaidRev;
}

// distribute revenue
if (remainingRev > 0) {
for (uint256 j = 0; j < revenueShares.length; j++) {
address recipient = revenueShares[j].recipient;
uint256 rev = (remainingRev * revenueShares[j].bps) / 10000;
if (recipient == costReceiver) {
platformRevenue += rev;
continue;
}
distributedRevenue += rev;
payable(recipient).transfer(rev);
}
}

if (
saleData_.price - saleData_.cost <
distributedRevenue + platformRevenue
) {
revert TotalBpsOver();
}
Comment on lines +197 to +202
Copy link
Contributor

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

Copy link
Contributor

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.


// Transfer cost, platform revenue and remaining funds
uint256 leftOver = saleData_.price - distributedRevenue;
if (leftOver > 0) {
payable(costReceiver).transfer(leftOver);
}
}

/// @notice override revert buyArtworks
function buyArtworks(
bytes32,
bytes32,
uint8,
SaleData calldata
) external payable override {
revert FunctionNotSupported();
}

/// @notice Event emitted when Artwork has been sold with the additional nonce
event BuyArtworkV2(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: [nonce] on Event [BuyArtworkV2] could be Indexed

address indexed buyer,
uint256 indexed tokenId,
uint256 nonce
);
}
13 changes: 13 additions & 0 deletions contracts/FeralfileSaleDataV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import {IFeralfileSaleDataV2} from "./IFeralfileSaleDataV2.sol";

contract FeralfileSaleDataV2 is IFeralfileSaleDataV2 {
function validateSaleDataV2(SaleDataV2 calldata saleData_) internal view {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be validateSaleData.

require(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Error message for require is too long: 34 counted / 32 allowed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: String exceeds 32 bytes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

saleData_.expiryTime > block.timestamp,
"FeralfileSaleData: sale is expired"
);
}
}
57 changes: 57 additions & 0 deletions contracts/FeralfileVaultV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";

import {FeralfileSaleDataV2} from "./FeralfileSaleDataV2.sol";
import {ECDSASigner} from "./ECDSASigner.sol";

contract FeralfileVaultV2 is Ownable, FeralfileSaleDataV2, ECDSASigner {
mapping(bytes32 => bool) private _paidSale;

constructor(address signer_) ECDSASigner(signer_) {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)


/// @notice pay for buyArtwork to a FFV4 contract destination.
/// @param r_ - part of signature for validating parameters integrity
/// @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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be payForSale

Copy link
Member Author

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.

Copy link
Member

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.

bytes32 r_,
bytes32 s_,
uint8 v_,
SaleDataV2 calldata saleData_
) external {
require(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

saleData_.payByVaultContract,
"FeralfileVault: not pay by vault"
);
require(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Error message for require is too long: 36 counted / 32 allowed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: String exceeds 32 bytes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

address(this).balance >= saleData_.price,
"FeralfileVault: insufficient balance"
);

validateSaleDataV2(saleData_);

bytes32 message = keccak256(
abi.encode(block.chainid, msg.sender, saleData_)
);
require(!_paidSale[message], "FeralfileVault: paid sale");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

require(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Error message for require is too long: 33 counted / 32 allowed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: String exceeds 32 bytes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

isValidSignature(message, r_, s_, v_),
"FeralfileVault: invalid signature"
);
_paidSale[message] = true;
payable(msg.sender).transfer(saleData_.price);
}

function withdrawFund(uint256 weiAmount) external onlyOwner {
require(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
Error message for require is too long: 36 counted / 32 allowed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: String exceeds 32 bytes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ [solhint] reported by reviewdog 🐶
GC: Use Custom Errors instead of require statements

address(this).balance >= weiAmount,
"FeralfileVault: insufficient balance"
);
payable(msg.sender).transfer(weiAmount);
}

receive() external payable {}
}
18 changes: 18 additions & 0 deletions contracts/IFeralfileSaleDataV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import {IFeralfileSaleData} from "./IFeralfileSaleData.sol";

interface IFeralfileSaleDataV2 is IFeralfileSaleData {
struct SaleDataV2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be SaleData

Copy link
Member Author

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.

Copy link
Member

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

uint256 price; // in wei
uint256 cost; // in wei
uint256 expiryTime;
address destination;
uint256 nonce;
uint256 seriesID;
uint16 quantity;
RevenueShare[] revenueShares; // address and royalty bps (500 means 5%)
bool payByVaultContract; // get eth from vault contract, used by credit card pay that proxy by ITX
}
}
17 changes: 17 additions & 0 deletions contracts/IFeralfileVaultV2.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import {IFeralfileSaleDataV2} from "./IFeralfileSaleDataV2.sol";

interface IFeralfileVaultV2 is IFeralfileSaleDataV2 {
function payForSaleV2(
bytes32 r_,
bytes32 s_,
uint8 v_,
SaleDataV2 calldata saleData_
) external;

function withdrawFund(uint256 weiAmount) external;

receive() external payable;
}
Loading
Loading