Code coverage 2.2.0

.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/[email protected]
+      - name: Setup node
+        uses: actions/[email protected]
+        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

.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"],
 };

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);

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;
     }
 }

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

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",

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",

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}`
+      );
+    });
   });
 });

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

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

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]);

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 () {

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
           );