From 7fddfffd91e4bf2745720e2e61e1f08a0950df79 Mon Sep 17 00:00:00 2001 From: Jan Cibulka Date: Thu, 30 May 2024 20:10:39 +0300 Subject: [PATCH] Contract and tests cleanups --- backend/contracts/NftMarketplaceV2.sol | 33 ++---- backend/scripts/NftMarketplaceV2.interact.ts | 101 ------------------- backend/test/NftMarketplaceV2.test.ts | 100 ++++++++---------- 3 files changed, 51 insertions(+), 183 deletions(-) delete mode 100644 backend/scripts/NftMarketplaceV2.interact.ts diff --git a/backend/contracts/NftMarketplaceV2.sol b/backend/contracts/NftMarketplaceV2.sol index acba656..991524c 100644 --- a/backend/contracts/NftMarketplaceV2.sol +++ b/backend/contracts/NftMarketplaceV2.sol @@ -42,12 +42,6 @@ contract NftMarketplaceV2 is ReentrancyGuard, Ownable { /// @notice NftAddress -> Token ID -> Price mapping(address => mapping(uint256 => uint256)) public listings; - /// @notice UserAddress -> Funds - // For increased security: https://fravoll.github.io/solidity-patterns/pull_over_push.html - // Why using call: https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/#dont-use-transfer-or-send - // Good practice: https://docs.soliditylang.org/en/develop/security-considerations.html?highlight=check%20effects#use-the-checks-effects-interactions-pattern - // mapping(address => uint256) private funds; - /// @notice UserAddress -> Deployed NFT contract addresses mapping(address => address[]) public nfts; @@ -113,11 +107,10 @@ contract NftMarketplaceV2 is ReentrancyGuard, Ownable { // Set the price of the token (which lists it in the marketplace) listings[_nftAddress][_tokenId] = _price; - // Add the listing fee to the funds for the contract owner + // Send the listing fee to the marketplace owner address owner = owner(); - // funds[owner] += msg.value; - (bool success, ) = payable(owner).call{value: msg.value}(""); // when funds aren't used - require(success, "Transfer failed"); // when funds aren't used + (bool success, ) = payable(owner).call{value: msg.value}(""); + require(success, "Transfer failed"); // Emit event emit ItemListed(msg.sender, _nftAddress, _tokenId, _price); @@ -131,9 +124,9 @@ contract NftMarketplaceV2 is ReentrancyGuard, Ownable { alreadyListed(_nftAddress, _tokenId) { IERC721 nft = IERC721(_nftAddress); - address owner = nft.ownerOf(_tokenId); + address listingOwner = nft.ownerOf(_tokenId); require( - msg.sender == owner || msg.sender == _nftAddress, + msg.sender == listingOwner || msg.sender == owner() || msg.sender == _nftAddress, "Caller isn't owner or nft contract." ); delete (listings[_nftAddress][_tokenId]); @@ -153,11 +146,10 @@ contract NftMarketplaceV2 is ReentrancyGuard, Ownable { require(msg.value == listings[_nftAddress][_tokenId], "Price mismatch."); IERC721 nft = IERC721(_nftAddress); address owner = nft.ownerOf(_tokenId); - // funds[owner] += msg.value; delete (listings[_nftAddress][_tokenId]); IERC721(_nftAddress).safeTransferFrom(owner, msg.sender, _tokenId); - (bool success, ) = payable(owner).call{value: msg.value}(""); // when funds aren't used - require(success, "Transfer failed"); // when funds aren't used + (bool success, ) = payable(owner).call{value: msg.value}(""); + require(success, "Transfer failed"); emit ItemBought(msg.sender, _nftAddress, _tokenId, msg.value); } @@ -174,17 +166,6 @@ contract NftMarketplaceV2 is ReentrancyGuard, Ownable { emit CollectionAdded(msg.sender, _nftAddress); } - /// @notice Method for withdrawing earned funds - /* - function withdrawFunds() external { - require(funds[msg.sender] > 0, "No funds to withdraw."); - uint256 amountToWithdraw = funds[msg.sender]; - funds[msg.sender] = 0; - (bool success, ) = payable(msg.sender).call{value: amountToWithdraw}(""); - require(success, "Transfer failed"); - } - */ - /// @notice Method for changing the listing fee function setListingFee(uint256 _newFee) external onlyOwner { require(_newFee > 0, "Fee should be above zero."); diff --git a/backend/scripts/NftMarketplaceV2.interact.ts b/backend/scripts/NftMarketplaceV2.interact.ts deleted file mode 100644 index d7df437..0000000 --- a/backend/scripts/NftMarketplaceV2.interact.ts +++ /dev/null @@ -1,101 +0,0 @@ -import * as dotenv from "dotenv"; - -import { Contract } from "ethers"; -import hre, { ethers } from "hardhat"; -import { - createClient, - debugExchange, - cacheExchange, - fetchExchange, -} from "@urql/core"; - -dotenv.config(); - -const contractName = "NftMarketplaceV2"; -const sepoliaContractAddress = "0xb0C95A36B54d02Ea5714518Fa3371C42D33EC132"; -const graphUrl = - "https://api.studio.thegraph.com/query/28136/nft-marketplace-sepolia/version/latest"; - -async function interact() { - // The wallets - const [alicesWallet, bobsWallet] = await ethers.getSigners(); - console.log( - "Alice's balance is", - ethers.utils.formatEther(await alicesWallet.getBalance()), - ); - console.log( - "Bob's balance is", - ethers.utils.formatEther(await bobsWallet.getBalance()), - ); - - // Contract - let contract: Contract; - if (hre.hardhatArguments.network === "sepolia") { - const Contract = await ethers.getContractFactory(contractName); - contract = await Contract.attach(sepoliaContractAddress); - } else { - // Redeploy to clean state - const Contract = await ethers.getContractFactory(contractName); - contract = await Contract.deploy(); - } - console.log("Contract address is", contract.address); - - const graphQuery = ` - query { - itemListeds { - id - seller - nftAddress - tokenId - price - timestamp - owner - nftUri - } - itemCanceleds { - id - seller - nftAddress - tokenId - timestamp - owner - nftUri - } - itemBoughts { - id - buyer - nftAddress - tokenId - price - timestamp - owner - nftUri - } - } - `; - - const urqlClient = createClient({ - url: graphUrl, - exchanges: [debugExchange, cacheExchange, fetchExchange], - }); - const data = await urqlClient.query(graphQuery, undefined).toPromise(); - const { itemListeds, itemCanceleds, itemBoughts } = data.data; - const mergedFilter = [...itemCanceleds, ...itemBoughts]; - const currentItems = itemListeds.filter((el: any) => { - return !mergedFilter.some((f) => { - return ( - f.nftAddress === el.nftAddress && - f.tokenId === el.tokenId && - f.timestamp > el.timestamp - ); - }); - }); - console.log(currentItems); -} - -interact().catch((error) => { - console.error(error); - process.exitCode = 1; -}); - -module.exports = interact; diff --git a/backend/test/NftMarketplaceV2.test.ts b/backend/test/NftMarketplaceV2.test.ts index b82ac7f..66c1d5c 100644 --- a/backend/test/NftMarketplaceV2.test.ts +++ b/backend/test/NftMarketplaceV2.test.ts @@ -198,9 +198,9 @@ describe("NftMarketplaceV2", function () { await expect( collectionContract .connect(accounts[actor]) - .approve(nftMarketplaceContract.address, tokenId) + .approve(nftMarketplaceContract.address, tokenId), ).to.be.revertedWith( - "ERC721: approve caller is not owner nor approved for all" + "ERC721: approve caller is not token owner or approved for all", ); }); @@ -228,7 +228,7 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ).to.be.revertedWith("Should be owner of the token."); }); @@ -246,7 +246,7 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ).to.be.revertedWith("Price not set."); }); @@ -265,7 +265,7 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: 0, - }) + }), ).to.be.revertedWith("Listing fee not met."); }); @@ -288,14 +288,14 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ) .to.emit(nftMarketplaceContract, "ItemListed") .withArgs( accounts[actor].address, collectionContract.address, tokenId, - price + price, ); }); @@ -318,7 +318,7 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ).to.be.revertedWith("Shouldn't be listed."); }); @@ -333,7 +333,7 @@ describe("NftMarketplaceV2", function () { await expect( nftMarketplaceContract .connect(accounts[actor]) - .cancelListing(collectionContract.address, tokenId) + .cancelListing(collectionContract.address, tokenId), ).to.be.revertedWith("Caller isn't owner or nft contract."); }); @@ -344,7 +344,7 @@ describe("NftMarketplaceV2", function () { await expect( nftMarketplaceContract .connect(accounts[actor]) - .cancelListing(collectionContract.address, tokenId) + .cancelListing(collectionContract.address, tokenId), ) .to.emit(nftMarketplaceContract, "ItemCanceled") .withArgs(accounts[actor].address, collectionContract.address, tokenId); @@ -357,7 +357,7 @@ describe("NftMarketplaceV2", function () { await expect( nftMarketplaceContract .connect(accounts[actor]) - .cancelListing(collectionContract.address, tokenId) + .cancelListing(collectionContract.address, tokenId), ).to.be.revertedWith("Should be listed."); }); @@ -382,14 +382,14 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ) .to.emit(nftMarketplaceContract, "ItemListed") .withArgs( accounts[actor].address, collectionContract.address, tokenId, - price + price, ); // Transferring the token outside of the marketplace @@ -399,14 +399,14 @@ describe("NftMarketplaceV2", function () { .transferFrom( accounts[actor].address, accounts[actor2].address, - tokenId - ) + tokenId, + ), ) .to.emit(nftMarketplaceContract, "ItemCanceled") .withArgs( collectionContract.address, collectionContract.address, - tokenId + tokenId, ); }); @@ -422,15 +422,21 @@ describe("NftMarketplaceV2", function () { await expect( nftMarketplaceContract .connect(accounts[actor]) - .buyItem(collectionContract.address, tokenId, { value: price }) + .buyItem(collectionContract.address, tokenId, { value: price }), ).to.be.revertedWith("Should be listed."); }); - it("Should set token for sale again", async function () { + it("Should set token for sale again and credit marketplace owner", async function () { const tokenId = 1; const price = ethers.utils.parseEther("1"); const actor = 1; + const provider = waffle.provider; + const marketplaceOwner = 0; + const balanceBefore = await provider.getBalance( + accounts[marketplaceOwner].address, + ); + const tx = await collectionContract .connect(accounts[actor]) .approve(nftMarketplaceContract.address, tokenId); @@ -445,15 +451,23 @@ describe("NftMarketplaceV2", function () { .connect(accounts[actor]) .listItem(collectionContract.address, tokenId, price, { value: listingFee, - }) + }), ) .to.emit(nftMarketplaceContract, "ItemListed") .withArgs( accounts[actor].address, collectionContract.address, tokenId, - price + price, ); + + const balanceAfter = await provider.getBalance( + accounts[marketplaceOwner].address, + ); + + expect(Number(ethers.utils.formatEther(balanceBefore))).to.be.lessThan( + Number(ethers.utils.formatEther(balanceAfter)), + ); }); it("Should not sell for incorrect price", async function () { @@ -464,62 +478,36 @@ describe("NftMarketplaceV2", function () { await expect( nftMarketplaceContract .connect(accounts[actor]) - .buyItem(collectionContract.address, tokenId, { value: price }) + .buyItem(collectionContract.address, tokenId, { value: price }), ).to.be.revertedWith("Price mismatch."); }); - it("Should sell market item", async function () { + it("Should sell market item and credit the seller", async function () { const tokenId = 1; const price = ethers.utils.parseEther("1"); + const seller = 1; const actor = 2; + const provider = waffle.provider; + const balanceBefore = await provider.getBalance(accounts[seller].address); + await expect( nftMarketplaceContract .connect(accounts[actor]) - .buyItem(collectionContract.address, tokenId, { value: price }) + .buyItem(collectionContract.address, tokenId, { value: price }), ) .to.emit(nftMarketplaceContract, "ItemBought") .withArgs( accounts[actor].address, collectionContract.address, tokenId, - price + price, ); - }); - - xit("Should withdraw funds of seller", async function () { - const actor = 1; - - const provider = waffle.provider; - const balanceBefore = await provider.getBalance(accounts[actor].address); - - const tx = await nftMarketplaceContract - .connect(accounts[actor]) - .withdrawFunds(); - await tx.wait(); - - const balanceAfter = await provider.getBalance(accounts[actor].address); - - expect(Number(ethers.utils.formatEther(balanceBefore))).to.be.lessThan( - Number(ethers.utils.formatEther(balanceAfter)) - ); - }); - - xit("Should withdraw funds of marketplace owner", async function () { - const actor = 0; - - const provider = waffle.provider; - const balanceBefore = await provider.getBalance(accounts[actor].address); - - const tx = await nftMarketplaceContract - .connect(accounts[actor]) - .withdrawFunds(); - await tx.wait(); - const balanceAfter = await provider.getBalance(accounts[actor].address); + const balanceAfter = await provider.getBalance(accounts[seller].address); expect(Number(ethers.utils.formatEther(balanceBefore))).to.be.lessThan( - Number(ethers.utils.formatEther(balanceAfter)) + Number(ethers.utils.formatEther(balanceAfter)), ); }); });