[WORKFLOW/ETHEREUM-CONTRACTS] Fix Up Coverage (#1348)

* fix up coverage tests

- Add some tests to be pedantic for BatchLiquidator/SuperAppBaseCFA foundry
- Recover CFAv1Library and IDAv1Library tests
- fix typo in SuperToken.sol
- extract more foundry specific tests

* ci.feature codecov

* remove extraction step

* fix up coverage

- add tests for supertokenlibv1 coverage
- rename idaV1Lib => superTokenLibIDA and cfaV1Lib => superTokenLibCFA

* add comments for reason for skipping coverage

* cleanup

- add back extraction
- move ignore list to codecov.yml
- remove skipFiles from solcover.js
- add test for SF_GOV_INVALID_LIQUIDATION_OR_PATRICIAN_PERIOD
- add safegas prop test
- try out separate upload to allow codecov to merge on their end

* >= 63

the multiplication takes some gas

* bring back extract n merge

* it likes the skipFiles

no extract step + add back skipFiles

* extraction required

* safe gas

* out of gas consistency

* remove safe gas lib test

* cleanup extraction!

* Revert "cleanup extraction!"

This reverts commit 861240c.

* cleaner extraction

- extract out only packages/ethereum-contracts/contracts for coverage
- remove files we don't care about for coverage (see .solcover.js)

* remove mocks

Author: 0xdavinchee

.github/workflows/ci.canary.yml

@@ -140,7 +140,9 @@ jobs:
       - name: Clean up and merge coverage artifacts
         run: |
           # extract coverage for NFT contracts from forge coverage
-          lcov -e lcov.info -o lcov.info 'packages/ethereum-contracts/contracts/superfluid/*NFT*.sol'
+          lcov -e lcov.info 'packages/ethereum-contracts/contracts/*' -o lcov.info
+          # remove mocks, base super app, test and deployer contracts (see .solcover.js)
+          lcov -r lcov.info 'packages/ethereum-contracts/contracts/mocks/*' 'packages/ethereum-contracts/contracts/apps/*Base*' 'packages/ethereum-contracts/contracts/utils/*Test*' 'packages/ethereum-contracts/contracts/utils/*Deployer*' -o lcov.info
           # merge hardhat and forge coverage files
           lcov -a lcov.info -a packages/ethereum-contracts/coverage/lcov.info -o packages/ethereum-contracts/coverage/lcov.info
 

.github/workflows/ci.feature.yml

@@ -118,6 +118,26 @@ jobs:
         run: |
           yarn workspace @superfluid-finance/ethereum-contracts test-coverage
 
+      - name: Install lcov
+        run: sudo apt-get -y install lcov
+
+      - name: Clean up and merge coverage artifacts
+        run: |
+          # extract coverage for NFT contracts from forge coverage
+          lcov -e lcov.info 'packages/ethereum-contracts/contracts/*' -o lcov.info
+          # remove mocks, base super app, test and deployer contracts (see .solcover.js)
+          lcov -r lcov.info 'packages/ethereum-contracts/contracts/mocks/*' 'packages/ethereum-contracts/contracts/apps/*Base*' 'packages/ethereum-contracts/contracts/utils/*Test*' 'packages/ethereum-contracts/contracts/utils/*Deployer*' -o lcov.info
+          # merge hardhat and forge coverage files
+          lcov -a lcov.info -a packages/ethereum-contracts/coverage/lcov.info -o packages/ethereum-contracts/coverage/lcov.info
+
+      - name: Upload ethereum-contracts-coverage to codecov
+        uses: codecov/codecov-action@v2
+        with:
+          files: lcov.info
+          name: ethereum-contracts-coverage
+          flags: ethereum-contracts
+          fail_ci_if_error: true
+
   test-hot-fuzz:
     uses: ./.github/workflows/call.test-hot-fuzz.yml
     needs: [ check ]

codecov.yml

@@ -15,9 +15,11 @@ parsers:
       macro: no
 
 comment:
-  layout: "reach,diff,flags,files,footer"
+  layout: "reach, diff, flags, files"
   behavior: default
-  require_changes: no
+  require_changes: false  # if true: only post the comment if coverage changes
+  require_base: no        # [yes :: must have a base report to post]
+  require_head: yes       # [yes :: must have a head report to post]
 
 flags:
   ethereum-contracts:

package.json

@@ -58,12 +58,12 @@
         "husky": "^8.0.2",
         "lerna": "^6.0.3",
         "node-jq": "^2.3.4",
-        "shellcheck": "^2.2.0",
         "nodemon": "^2.0.20",
         "npm-run-all": "^4.1.5",
         "nyc": "^15.1.0",
         "prettier": "^2.7.1",
         "prettier-eslint": "^15.0.1",
+        "shellcheck": "^2.2.0",
         "syncpack": "^8.3.9",
         "truffle": "^5.6.6",
         "ts-node": "^10.9.1",

packages/ethereum-contracts/.solcover.js

@@ -1,10 +1,23 @@
+// This file is used by solidity-coverage to configure the coverage report.
+
 module.exports = {
     providerOptions: {
         network_id: 8555,
     },
     skipFiles: [
         "mocks/",
+        // we skip the coverage for the SuperAppBase contracts because
+        // we override the functions in child contracts
         "apps/SuperAppBase.sol",
+        "apps/SuperAppBaseCFA.sol",
+
+        // we skip the coverage for these contracts because they are
+        // only used for testing
+        "utils/SuperfluidFrameworkDeployer",
+        "utils/SuperTokenDeployer",
+        "utils/TestToken",
+        "utils/TestResolver",
+        "utils/TestGovernance",
     ],
     mocha: {
         grep: "@skip-on-coverage", // Find everything with this tag

packages/ethereum-contracts/contracts/libs/SafeGasLibrary.sol

@@ -5,15 +5,19 @@ pragma solidity 0.8.19;
 /// @author Superfluid
 /// @notice An internal library used to handle out of gas errors
 library SafeGasLibrary {
-    error OUT_OF_GAS();
+    error OUT_OF_GAS(); // 0x20afada5
+
+    function _isOutOfGas(uint256 gasLeftBefore) internal view returns (bool) {
+        return gasleft() <= gasLeftBefore / 63;
+    }
 
     /// @dev A function used in the catch block to handle true out of gas errors
     /// @param gasLeftBefore the gas left before the try/catch block
-    function _revertWhenOutOfGas(uint256 gasLeftBefore) internal {
+    function _revertWhenOutOfGas(uint256 gasLeftBefore) internal view {
 // If the function actually runs out of gas, not just hitting the safety gas limit, we revert the whole transaction.
 // This solves an issue where the gas estimaton didn't provide enough gas by default for the function to succeed.
 // See https://medium.com/@wighawag/ethereum-the-concept-of-gas-and-its-dangers-28d0eb809bb2
-        if (gasleft() <= gasLeftBefore / 63) {
+        if (_isOutOfGas(gasLeftBefore)) {
             revert OUT_OF_GAS();
         }
     }

packages/ethereum-contracts/contracts/libs/SuperTokenDeployerLibrary.sol

@@ -74,28 +74,112 @@ contract SuperTokenLibraryCFAMock {
      * CFA Operations
      *************************************************************************/
 
-    function createFlowTest(ISuperToken token, address receiver, int96 flowRate) public {
+    function createFlowTest(
+        ISuperToken token,
+        address receiver,
+        int96 flowRate
+    ) public {
         token.createFlow(receiver, flowRate);
     }
-    function deleteFlowTest(ISuperToken token, address sender, address receiver) public {
+
+    function createFlowWithUserDataTest(
+        ISuperToken token,
+        address receiver,
+        int96 flowRate,
+        bytes memory userData
+    ) public {
+        token.createFlow(receiver, flowRate, userData);
+    }
+
+    function deleteFlowTest(
+        ISuperToken token,
+        address sender,
+        address receiver
+    ) public {
         token.deleteFlow(sender, receiver);
     }
 
-    function updateFlowTest(ISuperToken token, address receiver, int96 flowRate) public {
+    function deleteFlowWithUserDataTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        bytes memory userData
+    ) public {
+        token.deleteFlow(sender, receiver, userData);
+    }
+
+    function updateFlowWithUserDataTest(
+        ISuperToken token,
+        address receiver,
+        int96 flowRate,
+        bytes memory userData
+    ) public {
+        token.updateFlow(receiver, flowRate, userData);
+    }
+
+    function updateFlowTest(
+        ISuperToken token,
+        address receiver,
+        int96 flowRate
+    ) public {
         token.updateFlow(receiver, flowRate);
     }
 
-    function createFlowFromTest(ISuperToken token, address sender, address receiver, int96 flowRate) public {
+    function createFlowFromTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        int96 flowRate
+    ) public {
         token.createFlowFrom(sender, receiver, flowRate);
     }
-    function deleteFlowFromTest(ISuperToken token, address sender, address receiver) public {
+
+    function createFlowFromWithUserDataTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        int96 flowRate,
+        bytes memory userData
+    ) public {
+        token.createFlowFrom(sender, receiver, flowRate, userData);
+    }
+
+    function deleteFlowFromTest(
+        ISuperToken token,
+        address sender,
+        address receiver
+    ) public {
         token.deleteFlowFrom(sender, receiver);
     }
 
-    function updateFlowFromTest(ISuperToken token, address sender, address receiver, int96 flowRate) public {
+    function deleteFlowFromWithUserDataTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        bytes memory userData
+    ) public {
+        token.deleteFlowFrom(sender, receiver, userData);
+    }
+
+    function updateFlowFromTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        int96 flowRate
+    ) public {
         token.updateFlowFrom(sender, receiver, flowRate);
     }
 
+    function updateFlowFromWithUserDataTest(
+        ISuperToken token,
+        address sender,
+        address receiver,
+        int96 flowRate,
+        bytes memory userData
+    ) public {
+        token.updateFlowFrom(sender, receiver, flowRate, userData);
+    }
+
     function setFlowPermissionsTest(
         ISuperToken token,
         address flowOperator,

packages/ethereum-contracts/contracts/mocks/SuperTokenLibraryV1Mock.sol

@@ -2,7 +2,7 @@
 pragma solidity 0.8.19;
 
 // solhint-disable max-states-count
-// Notes: SueperToken is rich with states, disable this default rule here.
+// Notes: SuperToken is rich with states, disable this default rule here.
 
 import { UUPSProxiable } from "../upgradability/UUPSProxiable.sol";
 import { IConstantFlowAgreementV1 } from "../interfaces/agreements/IConstantFlowAgreementV1.sol";

packages/ethereum-contracts/contracts/superfluid/SuperToken.sol

@@ -2,9 +2,9 @@
 pragma solidity 0.8.19;
 
 import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol";
-
 import { UUPSProxiable } from "../upgradability/UUPSProxiable.sol";
 import { UUPSProxy } from "../upgradability/UUPSProxy.sol";
+import { SafeGasLibrary } from "../libs/SafeGasLibrary.sol";
 
 import {
     ISuperfluid,
@@ -1022,7 +1022,7 @@ contract Superfluid is
             // "/ 63" is a magic to avoid out of gas attack. 
             // See https://medium.com/@wighawag/ethereum-the-concept-of-gas-and-its-dangers-28d0eb809bb2.
             // A callback may use this to block the APP_RULE_NO_REVERT_ON_TERMINATION_CALLBACK jail rule.
-            if (gasleft() > gasLeftBefore / 63) {
+            if (!SafeGasLibrary._isOutOfGas(gasLeftBefore)) {
                 if (!isTermination) {
                     CallUtils.revertFromReturnedData(returnedData);
                 } else {