diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f0c6076d9..31b8959e0 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -101,6 +101,46 @@ jobs: - name: Integration tests run: npx hardhat test test/integration/*.js + coverage: + needs: build + runs-on: ubuntu-latest + if: ${{ !github.event.pull_request.draft }} + env: + GAS_REPORTER_COINMARKETCAP_API_KEY: ${{ secrets.COINMARKETCAP_API_KEY }} + name: code coverage + steps: + - name: Checkout repository + uses: actions/checkout@v3.1.0 + - name: Setup node + uses: actions/setup-node@v3.5.1 + with: + node-version: 16.14.x + cache: "npm" + - name: Install Dependencies + run: npm install + - name: Prepare Environment + shell: bash + run: | + cp .env.example .env + - name: Compile Contracts + run: npm run build + - name: Code coverage + run: npm run coverage + - name: Upload code coverage results + uses: actions/upload-artifact@v3 + with: + name: code-coverage-report + path: coverage/ + - name: Check Code Coverage + shell: bash + run: | + MAX_SKIPPED=1 # at this moment msgData() in BosonVoucher is not covered + { read TOTAL; read COVERED; read COVERAGE; } <<< $(jq '.total.lines.total, .total.lines.covered, .total.lines.pct' coverage/coverage-summary.json) + SKIPPED=$(($TOTAL - $COVERED)) + echo "solidity code coverage is '$COVERAGE'" + if (( $(echo "$SKIPPED > $MAX_SKIPPED" | bc -l) )); then echo "Fail: number of skipped statements '$SKIPPED' is higher than configured '$MAX_SKIPPED'" >&2; exit 1; fi + echo "Number of skipped statements '$SKIPPED' is within configured '$MAX_SKIPPED'" + deploy-dry-run: needs: build runs-on: ubuntu-latest diff --git a/.solcover.js b/.solcover.js index b2df8a6f8..9945b3963 100644 --- a/.solcover.js +++ b/.solcover.js @@ -5,9 +5,12 @@ module.exports = { "protocol/clients/proxy/ClientProxy.sol", "protocol/libs/ClientLib.sol", "ext_libs", + "example", ], + modifierWhitelist: ["onlyUninitialized", "nonReentrant"], mocha: { grep: "@skip-on-coverage", // Find everything with this tag invert: true, // Run the grep's inverse set. }, + istanbulReporter: ["html", "json-summary"], }; diff --git a/contracts/interfaces/clients/IBosonVoucher.sol b/contracts/interfaces/clients/IBosonVoucher.sol index aa55c79ad..2200503b2 100644 --- a/contracts/interfaces/clients/IBosonVoucher.sol +++ b/contracts/interfaces/clients/IBosonVoucher.sol @@ -3,6 +3,7 @@ pragma solidity 0.8.9; import { IERC721Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721Upgradeable.sol"; import { IERC721MetadataUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/IERC721MetadataUpgradeable.sol"; +import { IERC721ReceiverUpgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol"; /** * @title IBosonVoucher @@ -11,7 +12,7 @@ import { IERC721MetadataUpgradeable } from "@openzeppelin/contracts-upgradeable/ * * The ERC-165 identifier for this interface is: 0xaf16da6e */ -interface IBosonVoucher is IERC721Upgradeable, IERC721MetadataUpgradeable { +interface IBosonVoucher is IERC721Upgradeable, IERC721MetadataUpgradeable, IERC721ReceiverUpgradeable { event ContractURIChanged(string contractURI); event RoyaltyPercentageChanged(uint256 royaltyPercentage); event VoucherInitialized(uint256 indexed sellerId, uint256 indexed royaltyPercentage, string indexed contractURI); diff --git a/contracts/mock/Foreign20.sol b/contracts/mock/Foreign20.sol index 8addfb1e2..f91e5a7c4 100644 --- a/contracts/mock/Foreign20.sol +++ b/contracts/mock/Foreign20.sol @@ -209,8 +209,7 @@ contract Foreign20WithFee is Foreign20 { * @notice Mock ERC-(20) for Unit Testing */ contract Foreign20TransferReturnFalse is Foreign20 { - function transfer(address recipient, uint256 amount) public virtual override returns (bool) { - super.transfer(recipient, amount); + function transfer(address, uint256) public virtual override returns (bool) { return false; } } @@ -223,11 +222,10 @@ contract Foreign20TransferReturnFalse is Foreign20 { */ contract Foreign20TransferFromReturnFalse is Foreign20 { function transferFrom( - address sender, - address recipient, - uint256 amount + address, + address, + uint256 ) public virtual override returns (bool) { - super.transferFrom(sender, recipient, amount); return false; } } diff --git a/contracts/protocol/clients/voucher/BosonVoucher.sol b/contracts/protocol/clients/voucher/BosonVoucher.sol index 8fa1be806..506fc632f 100644 --- a/contracts/protocol/clients/voucher/BosonVoucher.sol +++ b/contracts/protocol/clients/voucher/BosonVoucher.sol @@ -29,13 +29,7 @@ import { IBosonFundsHandler } from "../../../interfaces/handlers/IBosonFundsHand * N.B. Although this contract extends OwnableUpgradeable and ERC721Upgradeable, * that is only for convenience, to avoid conflicts with mixed imports. */ -contract BosonVoucherBase is - IBosonVoucher, - BeaconClientBase, - OwnableUpgradeable, - ERC721Upgradeable, - IERC721ReceiverUpgradeable -{ +contract BosonVoucherBase is IBosonVoucher, BeaconClientBase, OwnableUpgradeable, ERC721Upgradeable { using Address for address; // Struct that is used to manipulate private variables from ERC721UpgradeableStorage diff --git a/package.json b/package.json index bca53a639..7e608dad8 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "tidy:contracts": "solhint --fix contracts/**/*.sol && prettier --write contracts/**", "tidy:scripts": "eslint --fix test/** scripts/** '*.js' && prettier --write test/** scripts/** '*.js'", "size": "npx hardhat size-contracts", - "coverage": "npx hardhat coverage", + "coverage": "npx hardhat clean && npx hardhat coverage", "deploy-suite:hardhat": "npx hardhat clean && npx hardhat compile && npx hardhat deploy-suite --network hardhat", "deploy-suite:local": "npx hardhat clean && npx hardhat compile && npx hardhat deploy-suite --network localhost", "deploy-suite:test": "npx hardhat clean && npx hardhat compile && npx hardhat deploy-suite --network test --env test >> logs/test.deploy.contracts.txt", diff --git a/scripts/config/revert-reasons.js b/scripts/config/revert-reasons.js index ef7a07770..0f98e98aa 100644 --- a/scripts/config/revert-reasons.js +++ b/scripts/config/revert-reasons.js @@ -167,6 +167,7 @@ exports.RevertReasons = { OWNABLE_ZERO_ADDRESS: "Ownable: new owner is the zero address", SAFE_ERC20_LOW_LEVEL_CALL: "SafeERC20: low-level call failed", SAFE_ERC20_NOT_SUCCEEDED: "SafeERC20: ERC20 operation did not succeed", + INITIALIZABLE_ALREADY_INITIALIZED: "Initializable: contract is already initialized", // Meta-Transactions related NONCE_USED_ALREADY: "Nonce used already", diff --git a/test/access/AccessControllerTest.js b/test/access/AccessControllerTest.js index 7d0c6d4bb..3bc2fe409 100644 --- a/test/access/AccessControllerTest.js +++ b/test/access/AccessControllerTest.js @@ -457,5 +457,15 @@ describe("AccessController", function () { `AccessControl: account ${rando.address.toLowerCase()} is missing role ${Role.ADMIN}` ); }); + + it("Should revert if caller tries to revokeRole but doesn't have ADMIN role", async function () { + // Grant role + await accessController.connect(deployer).grantRole(Role.PAUSER, pauser.address); + + // Revoke Role, expecting revert + await expect(accessController.connect(rando).revokeRole(Role.PAUSER, pauser.address)).to.be.revertedWith( + `AccessControl: account ${rando.address.toLowerCase()} is missing role ${Role.ADMIN}` + ); + }); }); }); diff --git a/test/protocol/ExchangeHandlerTest.js b/test/protocol/ExchangeHandlerTest.js index f8fe238a6..a773ca04b 100644 --- a/test/protocol/ExchangeHandlerTest.js +++ b/test/protocol/ExchangeHandlerTest.js @@ -39,10 +39,10 @@ const { prepareDataSignatureParameters, calculateContractAddress, applyPercentage, + deriveTokenId, setupTestEnvironment, getSnapshot, revertToSnapshot, - deriveTokenId, } = require("../util/utils.js"); const { oneWeek, oneMonth } = require("../util/constants"); const { FundsList } = require("../../scripts/domain/Funds"); @@ -1383,6 +1383,30 @@ describe("IBosonExchangeHandler", function () { .withArgs(offerId, buyerId, exchange.id, rando.address); }); + it("should emit an ExchangeCompleted event if another buyer calls after dispute period", async function () { + // Set time forward to the offer's voucherRedeemableFrom + await setNextBlockTimestamp(Number(voucherRedeemableFrom)); + + // Redeem the voucher + await exchangeHandler.connect(buyer).redeemVoucher(exchange.id); + + // Get the current block info + blockNumber = await ethers.provider.getBlockNumber(); + block = await ethers.provider.getBlock(blockNumber); + + // Set time forward to run out the dispute period + newTime = ethers.BigNumber.from(block.timestamp).add(disputePeriod).add(1).toNumber(); + await setNextBlockTimestamp(newTime); + + // Create a rando buyer account + await accountHandler.connect(rando).createBuyer(mockBuyer(rando.address)); + + // Complete exchange + await expect(exchangeHandler.connect(rando).completeExchange(exchange.id)) + .to.emit(exchangeHandler, "ExchangeCompleted") + .withArgs(offerId, buyerId, exchange.id, rando.address); + }); + context("💔 Revert Reasons", async function () { it("The exchanges region of protocol is paused", async function () { // Pause the exchanges region of the protocol @@ -1440,6 +1464,22 @@ describe("IBosonExchangeHandler", function () { ); }); + it("caller is a buyer, but not the buyer of the exchange and offer dispute period has not elapsed", async function () { + // Set time forward to the offer's voucherRedeemableFrom + await setNextBlockTimestamp(Number(voucherRedeemableFrom)); + + // Redeem the voucher + await exchangeHandler.connect(buyer).redeemVoucher(exchange.id); + + // Create a rando buyer account + await accountHandler.connect(rando).createBuyer(mockBuyer(rando.address)); + + // Attempt to complete the exchange, expecting revert + await expect(exchangeHandler.connect(rando).completeExchange(exchange.id)).to.revertedWith( + RevertReasons.DISPUTE_PERIOD_NOT_ELAPSED + ); + }); + it("caller is seller's assistant and offer dispute period has not elapsed", async function () { // Set time forward to the offer's voucherRedeemableFrom await setNextBlockTimestamp(Number(voucherRedeemableFrom)); @@ -2189,7 +2229,7 @@ describe("IBosonExchangeHandler", function () { context("Twin transfer fail", async function () { it("should revoke exchange when buyer is an EOA", async function () { - // Remove the approval for the protocal to transfer the seller's tokens + // Remove the approval for the protocol to transfer the seller's tokens await foreign20.connect(assistant).approve(protocolDiamondAddress, "0"); const tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id); @@ -2251,7 +2291,7 @@ describe("IBosonExchangeHandler", function () { }); it("should raise a dispute when buyer account is a contract", async function () { - // Remove the approval for the protocal to transfer the seller's tokens + // Remove the approval for the protocol to transfer the seller's tokens await foreign20.connect(assistant).approve(protocolDiamondAddress, "0"); // Deploy contract to test redeem called by another contract diff --git a/test/protocol/OfferHandlerTest.js b/test/protocol/OfferHandlerTest.js index 2dc7e2434..31b8858c0 100644 --- a/test/protocol/OfferHandlerTest.js +++ b/test/protocol/OfferHandlerTest.js @@ -688,6 +688,17 @@ describe("IBosonOfferHandler", function () { ).to.revertedWith(RevertReasons.INVALID_DISPUTE_RESOLVER); }); + it("Dispute resolver wallet is not registered", async function () { + // Set some address that is not registered as a dispute resolver + offer.price = "0"; + disputeResolver.id = "16"; + + // Attempt to Create an offer, expecting revert + await expect( + offerHandler.connect(assistant).createOffer(offer, offerDates, offerDurations, disputeResolver.id, agentId) + ).to.revertedWith(RevertReasons.INVALID_DISPUTE_RESOLVER); + }); + // TODO - revisit when account deactivations are supported it.skip("Dispute resolver is not active", async function () { // create another dispute resolver, but don't activate it diff --git a/test/protocol/OrchestrationHandlerTest.js b/test/protocol/OrchestrationHandlerTest.js index 533588a53..6a2ab87dd 100644 --- a/test/protocol/OrchestrationHandlerTest.js +++ b/test/protocol/OrchestrationHandlerTest.js @@ -470,6 +470,18 @@ describe("IBosonOrchestrationHandler", function () { * - If calling transferFrom on token fails for some reason (e.g. protocol is not approved to transfer) * - Received ERC20 token amount differs from the expected value */ + it("The orchestration region of protocol is paused", async function () { + // Pause the orchestration region of the protocol + await pauseHandler.connect(pauser).pause([PausableRegion.Orchestration]); + + // Attempt to raise a dispute, expecting revert + await expect( + orchestrationHandler + .connect(buyer) + .raiseAndEscalateDispute(exchangeId, { value: buyerEscalationDepositNative }) + ).to.revertedWith(RevertReasons.REGION_PAUSED); + }); + it("The disputes region of protocol is paused", async function () { // Pause the disputes region of the protocol await pauseHandler.connect(pauser).pause([PausableRegion.Disputes]); diff --git a/test/protocol/PauseHandlerTest.js b/test/protocol/PauseHandlerTest.js index 9421b4a42..3218d8803 100644 --- a/test/protocol/PauseHandlerTest.js +++ b/test/protocol/PauseHandlerTest.js @@ -64,7 +64,7 @@ describe("IBosonPauseHandler", function () { // Regions to pause regions = [PausableRegion.Offers, PausableRegion.Twins, PausableRegion.Bundles]; - // Pause the protocal, testing for the event + // Pause the protocol, testing for the event await expect(pauseHandler.connect(pauser).pause(regions)) .to.emit(pauseHandler, "ProtocolPaused") .withArgs(regions, pauser.address); @@ -106,7 +106,7 @@ describe("IBosonPauseHandler", function () { // Pause protocol await pauseHandler.connect(pauser).pause([PausableRegion.Sellers, PausableRegion.DisputeResolvers]); - // Unpause the protocal, testing for the event + // Unpause the protocol, testing for the event await expect(pauseHandler.connect(pauser).unpause()) .to.emit(pauseHandler, "ProtocolUnpaused") .withArgs(pauser.address); @@ -116,10 +116,10 @@ describe("IBosonPauseHandler", function () { // Pause protocol await pauseHandler.connect(pauser).pause([PausableRegion.Sellers, PausableRegion.DisputeResolvers]); - // Unpause the protocal, testing for the event + // Unpause the protocol, testing for the event await pauseHandler.connect(pauser).unpause(); - // Pause the protocal, testing for the event + // Pause the protocol, testing for the event regions = [PausableRegion.Funds]; await expect(pauseHandler.connect(pauser).pause(regions)) .to.emit(pauseHandler, "ProtocolPaused") @@ -132,7 +132,7 @@ describe("IBosonPauseHandler", function () { await pauseHandler.connect(pauser).pause([PausableRegion.Exchanges]); // Attempt to unpause without PAUSER role, expecting revert - await expect(pauseHandler.connect(rando).pause([])).to.revertedWith(RevertReasons.ACCESS_DENIED); + await expect(pauseHandler.connect(rando).unpause([])).to.revertedWith(RevertReasons.ACCESS_DENIED); }); it("Protocol is not currently paused", async function () { diff --git a/test/protocol/TwinHandlerTest.js b/test/protocol/TwinHandlerTest.js index 990f1198a..e75edc3d5 100644 --- a/test/protocol/TwinHandlerTest.js +++ b/test/protocol/TwinHandlerTest.js @@ -377,12 +377,10 @@ describe("IBosonTwinHandler", function () { context("💔 Revert Reasons", async function () { it("The twins region of protocol is paused", async function () { // Pause the twins region of the protocol - await pauseHandler - .connect(pauser) - .pause([PausableRegion.Offers, PausableRegion.Twins, PausableRegion.Bundles]); + await pauseHandler.connect(pauser).pause([PausableRegion.Twins]); // Attempt to Remove a twin, expecting revert - await expect(twinHandler.connect(assistant).removeTwin(twin.id)).to.revertedWith(RevertReasons.REGION_PAUSED); + await expect(twinHandler.connect(assistant).createTwin(twin)).to.revertedWith(RevertReasons.REGION_PAUSED); }); it("Caller not assistant of any seller", async function () { @@ -557,6 +555,7 @@ describe("IBosonTwinHandler", function () { await twinHandler.connect(assistant).createTwin(twin); // Create new twin with same token address + twin.supplyAvailable = "2"; await expect(twinHandler.connect(assistant).createTwin(twin)).to.be.revertedWith( RevertReasons.INVALID_TWIN_TOKEN_RANGE ); diff --git a/test/protocol/clients/BosonVoucherTest.js b/test/protocol/clients/BosonVoucherTest.js index 437d16b57..48475fff9 100644 --- a/test/protocol/clients/BosonVoucherTest.js +++ b/test/protocol/clients/BosonVoucherTest.js @@ -150,6 +150,13 @@ describe("IBosonVoucher", function () { const balanceAfter = await ethers.provider.getBalance(bosonVoucher.address); expect(balanceAfter.sub(balanceBefore)).to.eq(amount); }); + + it("Cannot initialize voucher twice", async function () { + const initalizableClone = await ethers.getContractAt("IInitializableVoucherClone", bosonVoucher.address); + await expect(initalizableClone.initializeVoucher(2, assistant.address, voucherInitValues)).to.be.revertedWith( + RevertReasons.INITIALIZABLE_ALREADY_INITIALIZED + ); + }); }); context("issueVoucher()", function () { @@ -175,6 +182,29 @@ describe("IBosonVoucher", function () { expect(balanceAfter.sub(balanceBefore)).eq(1); }); + it("should issue a voucher if it does not overlap with range", async function () { + const offerId = "5"; + const start = "10"; + const length = "123"; + const tokenId = deriveTokenId(offerId, start); // token within reserved range + + // Deploy mock protocol + await deployMockProtocol(); + + // Reserve a range + await bosonVoucher.connect(protocol).reserveRange(offerId, start, length, assistant.address); + + // Token id just below the range + await expect(() => + bosonVoucher.connect(protocol).issueVoucher(tokenId.sub(1), buyerWallet) + ).to.changeTokenBalance(bosonVoucher, buyer, 1); + + // Token id just above the range + await expect(() => + bosonVoucher.connect(protocol).issueVoucher(tokenId.add(length), buyerWallet) + ).to.changeTokenBalance(bosonVoucher, buyer, 1); + }); + context("💔 Revert Reasons", async function () { it("should revert if caller does not have PROTOCOL role", async function () { // Expect revert if random user attempts to issue voucher @@ -200,10 +230,7 @@ describe("IBosonVoucher", function () { const tokenId = deriveTokenId(offerId, "15"); // token within reserved range // Deploy mock protocol - const mockProtocol = await deployMockProtocol(); - - // Define what should be returned when getExchange is called - await mockProtocol.mock.getExchange.withArgs(tokenId).returns(true, mockExchange({ offerId }), mockVoucher()); + await deployMockProtocol(); // Reserve a range await bosonVoucher.connect(protocol).reserveRange(offerId, start, length, assistant.address); @@ -1910,6 +1937,11 @@ describe("IBosonVoucher", function () { expect(tokenURI).eq(metadataUri); }); + it("should return empty tokenURI if token does not exist", async function () { + const tokenURI = await bosonVoucher.tokenURI(10); + expect(tokenURI).eq(""); + }); + context("pre-minted", async function () { let start, tokenId; beforeEach(async function () { @@ -2362,6 +2394,22 @@ describe("IBosonVoucher", function () { .to.emit(bosonVoucher, "ApprovalForAll") .withArgs(bosonVoucher.address, rando.address, true); }); + + context("💔 Revert Reasons", async function () { + it("should revert if caller is not the owner", async function () { + // Expect revert if random user attempts to set approval + await expect(bosonVoucher.connect(rando).setApprovalForAllToContract(rando.address, true)).to.revertedWith( + RevertReasons.OWNABLE_NOT_OWNER + ); + }); + + it("should revert if operator is zero address", async function () { + // Expect revert if random user attempts to set approval + await expect( + bosonVoucher.connect(assistant).setApprovalForAllToContract(ethers.constants.AddressZero, true) + ).to.revertedWith(RevertReasons.INVALID_ADDRESS); + }); + }); }); context("withdrawToProtocol", function () { @@ -2410,6 +2458,19 @@ describe("IBosonVoucher", function () { }); }); + context("onERC721Received", function () { + it("Should return correct selector value", async function () { + const expectedSelector = bosonVoucher.interface.getSighash("onERC721Received(address,address,uint256,bytes)"); + const returnedSelector = await bosonVoucher.callStatic.onERC721Received( + assistant.address, + rando.address, + "1", + "0x" + ); + expect(returnedSelector).to.equal(expectedSelector); + }); + }); + async function deployMockProtocol() { const exchangeHandlerABI = exchangeHandler.interface.format(FormatTypes.json); const configHandlerABI = configHandler.interface.format(FormatTypes.json); diff --git a/test/util/utils.js b/test/util/utils.js index 6e7b299fe..da1d19a79 100644 --- a/test/util/utils.js +++ b/test/util/utils.js @@ -121,6 +121,8 @@ function compareOfferStructs(returnedOffer) { } async function setNextBlockTimestamp(timestamp) { + if (typeof timestamp == "string" && timestamp.startsWith("0x0") && timestamp.length > 3) + timestamp = "0x" + timestamp.substring(3); await provider.send("evm_setNextBlockTimestamp", [timestamp]); await provider.send("evm_mine", []); }