support new buy artworks function for John Gerrard show #42

hvthhien · 2024-06-06T03:36:52Z

Story link: https://github.com/bitmark-inc/feral-file/issues/2476

github-actions

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)

github-actions · 2024-06-06T03:37:16Z

contracts/FeralfileArtworkV4_2.sol

+import {IFeralfileVaultV2} from "./IFeralfileVaultV2.sol";
+import {FeralfileSaleDataV2} from "./FeralfileSaleDataV2.sol";
+
+contract FeralfileExhibitionV4_2 is


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

github-actions · 2024-06-06T03:37:16Z

contracts/FeralfileArtworkV4_2.sol

+
+    mapping(uint256 => uint256) private seriesNextSaleTokenIds; // seriesID -> tokenID
+
+    constructor(


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

github-actions · 2024-06-06T03:37:16Z

contracts/FeralfileArtworkV4_2.sol

+    /// @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(


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

github-actions · 2024-06-06T03:37:16Z

contracts/FeralfileArtworkV4_2.sol

+        uint8 v_,
+        SaleDataV2 calldata saleData_
+    ) external payable {
+        require(_selling, "FeralfileExhibitionV4: sale is not started");


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

contracts/FeralfileArtworkV4_2.sol

github-actions · 2024-06-06T03:37:17Z

contracts/FeralfileVaultV2.sol

+            abi.encode(block.chainid, msg.sender, saleData_)
+        );
+        require(!_paidSale[message], "FeralfileVault: paid sale");
+        require(


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

github-actions · 2024-06-06T03:37:17Z

contracts/FeralfileVaultV2.sol

+            abi.encode(block.chainid, msg.sender, saleData_)
+        );
+        require(!_paidSale[message], "FeralfileVault: paid sale");
+        require(


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

github-actions · 2024-06-06T03:37:17Z

contracts/FeralfileVaultV2.sol

+            abi.encode(block.chainid, msg.sender, saleData_)
+        );
+        require(!_paidSale[message], "FeralfileVault: paid sale");
+        require(


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

github-actions · 2024-06-06T03:37:17Z

contracts/FeralfileVaultV2.sol

+    }
+
+    function withdrawFund(uint256 weiAmount) external onlyOwner {
+        require(


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

github-actions · 2024-06-06T03:37:18Z

contracts/FeralfileVaultV2.sol

+    }
+
+    function withdrawFund(uint256 weiAmount) external onlyOwner {
+        require(


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

github-actions · 2024-06-06T07:23:43Z

contracts/FeralfileVaultV2.sol

+    }
+
+    function withdrawFund(uint256 weiAmount) external onlyOwner {
+        require(


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

contracts/FeralfileArtworkV4_2.sol

jollyjoker992 · 2024-06-06T08:18:52Z

contracts/FeralfileArtworkV4_2.sol

+            }
+
+            // send NFT
+            _safeTransfer(


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.

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.

jollyjoker992 · 2024-06-06T08:31:30Z

contracts/FeralfileArtworkV4_2.sol

+                    continue;
+                }
+                distributedRevenue += totalRev;
+                payable(recipient).transfer(totalRev);


Should accumulate into one tx for the same recipient.

we only use 1 revenue shares array for all the tokens, so we don't need to accumulate anymore

jollyjoker992 · 2024-06-06T08:43:00Z

contracts/FeralfileArtworkV4_2.sol

+    }
+
+    /// @notice Event emitted when Artwork has been sold with the additional nonce
+    event BuyArtworkV2(address indexed buyer, uint256 indexed tokenId, uint256 nonce);


Should be BuyBulkArtworks

currently, the event is for each single token

@lpopo0856 What would you prefer? Single event for this bulk sale or multiple events for individual?

jollyjoker992 · 2024-06-06T08:43:30Z

contracts/FeralfileSaleDataV2.sol

+import {IFeralfileSaleDataV2} from "./IFeralfileSaleDataV2.sol";
+
+contract FeralfileSaleDataV2 is IFeralfileSaleDataV2 {
+    function validateSaleDataV2(SaleDataV2 calldata saleData_) internal view {


Should be validateSaleData.

jollyjoker992 · 2024-06-06T08:43:47Z

contracts/FeralfileVaultV2.sol

+    /// @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(


Should be payForSale

the payload is different from the old Vault, so I changed the name to reduce the mistake when calling.

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.

jollyjoker992 · 2024-06-06T08:44:23Z

contracts/IFeralfileSaleDataV2.sol

+import {IFeralfileSaleData} from "./IFeralfileSaleData.sol";
+
+interface IFeralfileSaleDataV2 is IFeralfileSaleData {
+    struct SaleDataV2 {


Should be SaleData

Since the SaleData name has already been used, we need a different name for this struct.

Ok. I thought you could use separate interface but it seems like we have to reuse the RevenueShare

github-actions · 2024-06-06T08:49:26Z

contracts/FeralfileArtworkV4_2.sol

+    /// @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(


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

github-actions · 2024-06-06T08:49:26Z

contracts/FeralfileArtworkV4_2.sol

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


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

github-actions · 2024-06-13T07:08:12Z

contracts/FeralfileArtworkV4_2.sol

+    /// @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(


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

lpopo0856 · 2024-06-14T04:33:19Z

contracts/FeralfileArtworkV4_2.sol

+            seriesMaxSupplies_
+        )
+    {
+        vaultV2 = IFeralfileVaultV2(payable(vault_));


Minor: check equal length of seriesIds_ and seriesNextPurchasableTokenIds_

lpopo0856 · 2024-06-14T07:28:31Z

contracts/FeralfileArtworkV4_2.sol

+        if (
+            saleData_.price - saleData_.cost <
+            distributedRevenue + platformRevenue
+        ) {
+            revert TotalBpsOver();
+        }


It's a miss from long time ago that we should add a check for saleData_.price > saleData_.cost

or we could discuss should we allow to submit a buy tx that cost larger than price.

lpopo0856 · 2024-06-14T07:29:50Z

contracts/FeralfileArtworkV4_2.sol

+                ? remainingRev
+                : remainingAdvanceAmount;
+            uint256 totalRev = saleData_.quantity * rev;
+            platformRevenue += totalRev;


we have some logic error her, discuss in slack

github-actions · 2024-06-14T08:16:51Z

contracts/FeralfileArtworkV4_2.sol

+    /// @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(


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

github-actions · 2024-06-14T08:36:43Z

contracts/FeralfileArtworkV4_2.sol

+    /// @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(


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

support new buy artworks function for John Gerrard show

f2e970b

hvthhien requested a review from jollyjoker992 June 6, 2024 03:36

github-actions bot reviewed Jun 6, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 6, 2024

Generate ABIs for PR #42

634b9a4

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 634b9a4 to 399bdff Compare June 6, 2024 07:23

github-actions bot reviewed Jun 6, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 6, 2024

Generate ABIs for PR #42

74e7534

refactor

712f255

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 74e7534 to 712f255 Compare June 6, 2024 07:49

github-actions bot reviewed Jun 6, 2024

View reviewed changes

contracts/FeralfileArtworkV4_2.sol Outdated Show resolved Hide resolved

github-actions bot pushed a commit that referenced this pull request Jun 6, 2024

Generate ABIs for PR #42

21c2b0b

jollyjoker992 reviewed Jun 6, 2024

View reviewed changes

contracts/FeralfileArtworkV4_2.sol Show resolved Hide resolved

jollyjoker992 reviewed Jun 6, 2024

View reviewed changes

contracts/FeralfileArtworkV4_2.sol Outdated Show resolved Hide resolved

jollyjoker992 reviewed Jun 6, 2024

View reviewed changes

contracts/FeralfileArtworkV4_2.sol Outdated Show resolved Hide resolved

jollyjoker992 reviewed Jun 6, 2024

View reviewed changes

contracts/FeralfileArtworkV4_2.sol Outdated Show resolved Hide resolved

jollyjoker992 reviewed Jun 6, 2024

View reviewed changes

refactor & rename

fff85ef

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 21c2b0b to fff85ef Compare June 6, 2024 08:49

github-actions bot reviewed Jun 6, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 6, 2024

Generate ABIs for PR #42

10819c9

add tests

fc62a43

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 10819c9 to fc62a43 Compare June 6, 2024 10:27

github-actions bot pushed a commit that referenced this pull request Jun 6, 2024

Generate ABIs for PR #42

5ec5d8c

allow to update vaulV2

1b8902e

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 5ec5d8c to 1b8902e Compare June 7, 2024 01:40

github-actions bot pushed a commit that referenced this pull request Jun 12, 2024

Generate ABIs for PR #42

5e681b1

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 5e681b1 to 1e50e63 Compare June 12, 2024 03:39

github-actions bot pushed a commit that referenced this pull request Jun 12, 2024

Generate ABIs for PR #42

a692573

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from a692573 to d875e32 Compare June 12, 2024 08:23

github-actions bot pushed a commit that referenced this pull request Jun 12, 2024

Generate ABIs for PR #42

eb2b72b

update tests

68109c5

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from eb2b72b to 68109c5 Compare June 13, 2024 02:36

github-actions bot pushed a commit that referenced this pull request Jun 13, 2024

Generate ABIs for PR #42

42e85ba

override revert setVault

6df789a

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 42e85ba to 6df789a Compare June 13, 2024 03:43

github-actions bot pushed a commit that referenced this pull request Jun 13, 2024

Generate ABIs for PR #42

2f69db2

update contract optimized gas

46673c9

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 2f69db2 to 46673c9 Compare June 13, 2024 07:07

github-actions bot reviewed Jun 13, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 13, 2024

Generate ABIs for PR #42

9af45c5

jollyjoker992 approved these changes Jun 13, 2024

View reviewed changes

lpopo0856 reviewed Jun 14, 2024

View reviewed changes

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 9af45c5 to 806cc26 Compare June 14, 2024 08:16

github-actions bot reviewed Jun 14, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 14, 2024

Generate ABIs for PR #42

53affbe

update revenue sharing logic

03f1e4e

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 53affbe to 03f1e4e Compare June 14, 2024 08:36

github-actions bot reviewed Jun 14, 2024

View reviewed changes

github-actions bot pushed a commit that referenced this pull request Jun 14, 2024

Generate ABIs for PR #42

04bf821

refactor & additional check for buying artwork

b4f3da0

hvthhien force-pushed the update_smart_contract_buy_sequencely branch from 04bf821 to b4f3da0 Compare June 14, 2024 09:14

Generate ABIs for PR #42

dbebe04

lpopo0856 approved these changes Jun 17, 2024

View reviewed changes

jollyjoker992 merged commit b5d26d3 into main Jun 17, 2024

jollyjoker992 deleted the update_smart_contract_buy_sequencely branch June 17, 2024 04:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support new buy artworks function for John Gerrard show #42

support new buy artworks function for John Gerrard show #42

hvthhien commented Jun 6, 2024

github-actions bot left a comment

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

jollyjoker992 Jun 6, 2024

hvthhien Jun 6, 2024

jollyjoker992 Jun 6, 2024

hvthhien Jun 7, 2024

jollyjoker992 Jun 6, 2024 •

edited

Loading

hvthhien Jun 7, 2024

jollyjoker992 Jun 11, 2024

jollyjoker992 Jun 6, 2024

jollyjoker992 Jun 6, 2024

hvthhien Jun 7, 2024

jollyjoker992 Jun 11, 2024

jollyjoker992 Jun 6, 2024

hvthhien Jun 7, 2024

jollyjoker992 Jun 11, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 6, 2024

github-actions bot Jun 13, 2024

lpopo0856 Jun 14, 2024

lpopo0856 Jun 14, 2024

lpopo0856 Jun 14, 2024

lpopo0856 Jun 14, 2024

github-actions bot Jun 14, 2024

github-actions bot Jun 14, 2024


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

		constructor(

support new buy artworks function for John Gerrard show #42

support new buy artworks function for John Gerrard show #42

Conversation

hvthhien commented Jun 6, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jollyjoker992 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 6, 2024

Choose a reason for hiding this comment

github-actions bot Jun 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Jun 14, 2024

Choose a reason for hiding this comment

github-actions bot Jun 14, 2024

Choose a reason for hiding this comment

jollyjoker992 Jun 6, 2024 •

edited

Loading