refactor: centralize coverage detection with type-safe helper

- Replace manual environment variable management with hre.__SOLIDITY_COVERAGE_RUNNING
- Add centralized isRunningUnderCoverage() utility function
- Simplify test coverage detection using clean if statements
- Remove duplicate .solcover.js file (keep only test/.solcover.js)
- Update coverage configuration for better reporting

Author: RembrandtK

packages/contracts/.solcover.js

@@ -28,7 +28,7 @@
   ],
   "scripts": {
     "prepack": "pnpm build",
-    "clean": "rm -rf artifacts/ cache/ types/ abis/ build/ dist/",
+    "clean": "rm -rf artifacts/ cache/ types/ abis/ build/ dist/ coverage/",
     "build": "pnpm build:self",
     "build:self": "pnpm compile",
     "compile": "hardhat compile",

packages/contracts/package.json

@@ -6,20 +6,12 @@ module.exports = {
     network_id: 1337,
   },
   skipFiles,
-  // Use default istanbulFolder: './coverage'
+  istanbulFolder: '../coverage',
   // Remove 'html' reporter to avoid duplicates, keep lcov for lcov.info
   istanbulReporter: ['lcov', 'text', 'json'],
   configureYulOptimizer: true,
   mocha: {
     grep: '@skip-on-coverage',
     invert: true,
   },
-  onCompileComplete: async function (/* config */) {
-    // Set environment variable to indicate we're running under coverage
-    process.env.SOLIDITY_COVERAGE = 'true'
-  },
-  onIstanbulComplete: async function (/* config */) {
-    // Clean up environment variable
-    delete process.env.SOLIDITY_COVERAGE
-  },
 }

packages/contracts/test/.solcover.js

@@ -56,7 +56,7 @@
   },
   "scripts": {
     "postinstall": "scripts/setup-symlinks",
-    "clean": "rm -rf artifacts/ cache/ reports/ types/",
+    "clean": "rm -rf artifacts/ cache/ types/",
     "build": "pnpm build:dep",
     "build:dep": "pnpm --filter '@graphprotocol/contracts-tests^...' run build:self",
     "test": "pnpm build && pnpm test:self",

packages/contracts/test/package.json

@@ -5,7 +5,6 @@ set -eo pipefail
 echo {} > addresses-local.json
 
 DISABLE_SECURE_ACCOUNTS=true \
-SOLIDITY_COVERAGE=true \
 L1_GRAPH_CONFIG=config/graph.hardhat.yml \
 L2_GRAPH_CONFIG=config/graph.arbitrum-hardhat.yml \
 ADDRESS_BOOK=addresses-local.json \

packages/contracts/test/scripts/coverage

@@ -27,6 +27,7 @@ import { BigNumber, ContractTransaction, ethers, Event } from 'ethers'
 import { defaultAbiCoder } from 'ethers/lib/utils'
 import hre from 'hardhat'
 
+import { isRunningUnderCoverage } from '../../utils/coverage'
 import { NetworkFixture } from './lib/fixtures'
 import {
   AccountDefaultName,
@@ -787,16 +788,9 @@ describe.skip('L1GNS @skip-on-coverage', () => {
       const tx = gns.connect(me).multicall([bogusPayload, tx2.data])
 
       // Under coverage, the error message may be different due to instrumentation
-      const isRunningUnderCoverage =
-        hre.network.name === 'coverage' ||
-        process.env.SOLIDITY_COVERAGE === 'true' ||
-        process.env.npm_lifecycle_event === 'test:coverage'
-
-      if (isRunningUnderCoverage) {
-        // Under coverage, the transaction should still revert, but the message might be empty
+      if (isRunningUnderCoverage()) {
         await expect(tx).to.be.reverted
       } else {
-        // Normal test run should have the specific error message
         await expect(tx).revertedWith("function selector was not recognized and there's no fallback function")
       }
     })
@@ -1295,14 +1289,8 @@ describe.skip('L1GNS @skip-on-coverage', () => {
         await expect(tx2).revertedWith('NO_SIGNAL')
       })
       it('sets the curator signal to zero so they cannot withdraw', async function () {
-        // Check if we're running under coverage
-        const isRunningUnderCoverage =
-          hre.network.name === 'coverage' ||
-          process.env.SOLIDITY_COVERAGE === 'true' ||
-          process.env.npm_lifecycle_event === 'test:coverage'
-
-        if (isRunningUnderCoverage) {
-          // Under coverage, skip this test as it has issues with BigNumber values
+        // Under coverage, skip this test as it has issues with BigNumber values
+        if (isRunningUnderCoverage()) {
           this.skip()
           return
         }
@@ -1328,14 +1316,8 @@ describe.skip('L1GNS @skip-on-coverage', () => {
         await expect(tx).revertedWith('GNS: No signal to withdraw GRT')
       })
       it('gives each curator an amount of tokens proportional to their nSignal', async function () {
-        // Check if we're running under coverage
-        const isRunningUnderCoverage =
-          hre.network.name === 'coverage' ||
-          process.env.SOLIDITY_COVERAGE === 'true' ||
-          process.env.npm_lifecycle_event === 'test:coverage'
-
-        if (isRunningUnderCoverage) {
-          // Under coverage, skip this test as it has issues with BigNumber values
+        // Under coverage, skip this test as it has issues with BigNumber values
+        if (isRunningUnderCoverage()) {
           this.skip()
           return
         }

packages/contracts/test/tests/unit/gns.test.ts

@@ -6,6 +6,7 @@ import { expect, use } from 'chai'
 import { constants, ContractTransaction, Signer, utils, Wallet } from 'ethers'
 import hre from 'hardhat'
 
+import { isRunningUnderCoverage } from '../../../utils/coverage'
 import { NetworkFixture } from '../lib/fixtures'
 
 use(smock.matchers)
@@ -80,12 +81,7 @@ describe('L2GraphTokenGateway', () => {
     await fixture.setUp()
     // Thanks to Livepeer: https://github.com/livepeer/arbitrum-lpt-bridge/blob/main/test/unit/L2/l2LPTGateway.test.ts#L86
     // Skip smock setup when running under coverage due to provider compatibility issues
-    const isRunningUnderCoverage =
-      hre.network.name === 'coverage' ||
-      process.env.SOLIDITY_COVERAGE === 'true' ||
-      process.env.npm_lifecycle_event === 'test:coverage'
-
-    if (!isRunningUnderCoverage) {
+    if (!isRunningUnderCoverage()) {
       arbSysMock = await smock.fake('ArbSys', {
         address: '0x0000000000000000000000000000000000000064',
       })
@@ -237,12 +233,7 @@ describe('L2GraphTokenGateway', () => {
       // expect(arbSysMock.sendTxToL1).to.have.been.calledOnce
 
       // Only check smock expectations when not running under coverage
-      const isRunningUnderCoverage =
-        hre.network.name === 'coverage' ||
-        process.env.SOLIDITY_COVERAGE === 'true' ||
-        process.env.npm_lifecycle_event === 'test:coverage'
-
-      if (!isRunningUnderCoverage && arbSysMock) {
+      if (!isRunningUnderCoverage() && arbSysMock) {
         expect(arbSysMock.sendTxToL1).to.have.been.calledWith(l1GRTGatewayMock.address, calldata)
       }
       const senderBalance = await grt.balanceOf(tokenSender.address)
@@ -271,14 +262,8 @@ describe('L2GraphTokenGateway', () => {
         await expect(tx).revertedWith('TOKEN_NOT_GRT')
       })
       it('burns tokens and triggers an L1 call', async function () {
-        // Check if we're running under coverage
-        const isRunningUnderCoverage =
-          hre.network.name === 'coverage' ||
-          process.env.SOLIDITY_COVERAGE === 'true' ||
-          process.env.npm_lifecycle_event === 'test:coverage'
-
-        if (isRunningUnderCoverage) {
-          // Skip this test under coverage due to complex instrumentation issues
+        // Skip this test under coverage due to complex instrumentation issues
+        if (isRunningUnderCoverage()) {
           this.skip()
           return
         }
@@ -287,14 +272,8 @@ describe('L2GraphTokenGateway', () => {
         await testValidOutboundTransfer(tokenSender, defaultData)
       })
       it('decodes the sender address from messages sent by the router', async function () {
-        // Check if we're running under coverage
-        const isRunningUnderCoverage =
-          hre.network.name === 'coverage' ||
-          process.env.SOLIDITY_COVERAGE === 'true' ||
-          process.env.npm_lifecycle_event === 'test:coverage'
-
-        if (isRunningUnderCoverage) {
-          // Skip this test under coverage due to complex instrumentation issues
+        // Skip this test under coverage due to complex instrumentation issues
+        if (isRunningUnderCoverage()) {
           this.skip()
           return
         }

packages/contracts/test/tests/unit/l2/l2GraphTokenGateway.test.ts

@@ -28,6 +28,8 @@ import {
 import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'
 import { providers, Wallet } from 'ethers'
 
+import { isRunningUnderCoverage } from '../../../utils/coverage'
+
 export interface L1FixtureContracts {
   controller: Controller
   disputeManager: DisputeManager
@@ -72,7 +74,7 @@ export class NetworkFixture {
 
   async load(deployer: SignerWithAddress, l2Deploy?: boolean): Promise<GraphNetworkContracts> {
     // Use instrumented artifacts when running coverage tests, otherwise use local artifacts
-    const artifactsDir = process.env.SOLIDITY_COVERAGE ? './artifacts' : '../artifacts'
+    const artifactsDir = isRunningUnderCoverage() ? './artifacts' : '../artifacts'
 
     const contracts = await deployGraphNetwork(
       'addresses-local.json',

packages/contracts/test/tests/unit/lib/fixtures.ts

@@ -0,0 +1,13 @@
+import hre from 'hardhat'
+
+/**
+ * Utility functions for detecting and handling coverage test execution
+ */
+
+/**
+ * Checks if tests are currently running under solidity-coverage instrumentation
+ * @returns true if running under coverage, false otherwise
+ */
+export function isRunningUnderCoverage(): boolean {
+  return hre.__SOLIDITY_COVERAGE_RUNNING === true
+}

packages/contracts/test/utils/coverage.ts

@@ -172,8 +172,10 @@ contract HorizonStakingSlashTest is HorizonStakingTest {
         vm.assume(delegationTokensToSlash <= delegationTokens);
         vm.assume(delegationTokensToUndelegate <= delegationTokens);
         vm.assume(delegationTokensToUndelegate > 0);
-        // Ensure that after undelegating, either we undelegate everything or leave at least MIN_DELEGATION
-        vm.assume(delegationTokensToUndelegate == delegationTokens || delegationTokens - delegationTokensToUndelegate >= MIN_DELEGATION);
+        vm.assume(
+            delegationTokensToUndelegate == delegationTokens ||
+                MIN_DELEGATION <= delegationTokens - delegationTokensToUndelegate
+        );
 
         resetPrank(users.delegator);
         _delegate(users.indexer, subgraphDataServiceAddress, delegationTokens, 0);