Improve 🍴 Fork Tests (#503)

### Description Another branch PR on THE BIG ONE. This is a long-overdue cleanup and improvement of our fork tests structure. These tests are important and we will want to maintain and grow them, but the structure they were in made this very hard. They were also terribly brittle and fleaky. The [new structure](https://github.com/mento-protocol/mento-core/blob/81bd18666ea088d6df1cfa461d5f902e36a2c49a/test/fork/README.md) I'm proposing aims to tackle two main issues: - Fork tests were executing assertions for all exchanges in a single test making it hard to debug and follow. - Helpers and utility functions weren't structured and were hard to debug and maintain. Note for reviewers: 99% of the code inside of the functions is just copied from the old structure so I wouldn't do a full review of everything, I would focus on the structure itself. I did make some small tweaks to improve flakiness, for example skipping some of the tests when the limits prevent them, which I think is a fair thing to do, as long as the tests run periodically. ### Other changes I added a CI job for running fork-tests daily on `develop`. ### Tested Yeah, they are, after all, tests. ### Related issues N/A ### Backwards compatibility N/A ### Documentation N/A --------- Co-authored-by: Bayological <6872903+bayological@users.noreply.github.com> Co-authored-by: chapati <philip.paetz@me.com> Co-authored-by: philbow61 <80156619+philbow61@users.noreply.github.com> Co-authored-by: baroooo <baranseltekin@gmail.com>
mento-protocol · Aug 27, 2024 · 24a2e68 · 24a2e68
1 parent d916e50
commit 24a2e68
Show file tree

Hide file tree

Showing 23 changed files with 1,828 additions and 1,386 deletions.
diff --git a/.github/workflows/fork-tests.yaml b/.github/workflows/fork-tests.yaml
@@ -0,0 +1,43 @@
+name: "ForkTests"
+
+env:
+  FOUNDRY_PROFILE: "fork-tests"
+  ALFAJORES_RPC_URL: ${{secrets.ALFAJORES_RPC_URL}}
+  CELO_MAINNET_RPC_URL: ${{secrets.CELO_MAINNET_RPC_URL}}
+
+on: 
+  workflow_dispatch:
+  schedule:
+    - cron: "0 0 * * *"  # everyday at midnight
+
+jobs:
+  test:
+    name: Run fork tests
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          submodules: "recursive"
+          ref: develop
+
+      - name: Install Foundry
+        uses: foundry-rs/foundry-toolchain@v1
+
+      - name: "Install Node.js"
+        uses: "actions/setup-node@v3"
+        with:
+          cache: "yarn"
+          node-version: "20"
+
+      - name: "Install the Node.js dependencies"
+        run: "yarn install --immutable"
+
+      - name: "Show the Foundry config"
+        run: "forge config"
+
+      - name: "Run the tests"
+        run: "forge test"
+
+      - name: "Add test summary"
+        run: |
+          echo "## Tests" >> $GITHUB_STEP_SUMMARY
diff --git a/contracts/interfaces/IBroker.sol b/contracts/interfaces/IBroker.sol
@@ -114,7 +114,7 @@ interface IBroker {
    * @dev This can be used by UI or clients to discover all pairs.
    * @return exchangeProviders the addresses of all exchange providers.
    */
-  function getExchangeProviders() external view returns (address[] memory exchangeProviders);
+  function getExchangeProviders() external view returns (address[] memory);
 
   function burnStableTokens(address token, uint256 amount) external returns (bool);
 
@@ -149,4 +149,6 @@ interface IBroker {
   function tradingLimitsConfig(bytes32 id) external view returns (ITradingLimits.Config memory);
 
   function tradingLimitsState(bytes32 id) external view returns (ITradingLimits.State memory);
+
+  function exchangeProviders(uint256 i) external view returns (address);
 }
diff --git a/package.json b/package.json
@@ -40,7 +40,7 @@
     "fork-test:alfajores": "env FOUNDRY_PROFILE=fork-tests forge test --match-contract Alfajores",
     "fork-test:celo-mainnet": "env FOUNDRY_PROFILE=fork-tests forge test --match-contract CeloMainnet",
     "check-no-ir": "./bin/check-contracts.sh",
-    "check-contract-sizes": "env FOUNDRY_PROFILE=optimized forge build --sizes --skip test/**/*"
+    "check-contract-sizes": "env FOUNDRY_PROFILE=optimized forge build --sizes --skip \"test/**/*\""
   },
   "dependencies": {
     "@celo/contracts": "^11.0.0"

diff --git a/test/fork/BaseForkTest.sol b/test/fork/BaseForkTest.sol
@@ -4,21 +4,16 @@ pragma solidity ^0.8;
 
 import { Test } from "mento-std/Test.sol";
 import { CELO_REGISTRY_ADDRESS } from "mento-std/Constants.sol";
-import { console } from "forge-std/console.sol";
 
 import { FixidityLib } from "celo/contracts/common/FixidityLib.sol";
 import { IRegistry } from "celo/contracts/common/interfaces/IRegistry.sol";
 
-import { Utils } from "./Utils.sol";
-import { TestAsserts } from "./TestAsserts.sol";
-
 import { IBreakerBox } from "contracts/interfaces/IBreakerBox.sol";
 import { IBroker } from "contracts/interfaces/IBroker.sol";
-import { IExchangeProvider } from "contracts/interfaces/IExchangeProvider.sol";
-import { IERC20 } from "contracts/interfaces/IERC20.sol";
 import { IReserve } from "contracts/interfaces/IReserve.sol";
 import { ISortedOracles } from "contracts/interfaces/ISortedOracles.sol";
 import { ITradingLimitsHarness } from "test/utils/harnesses/ITradingLimitsHarness.sol";
+import { toRateFeed } from "./helpers/misc.sol";
 
 interface IMint {
   function mint(address, uint256) external;
@@ -32,19 +27,11 @@ interface IMint {
  * therfore it doesn't make assumptions about the systems, nor tries to configure
  * the system to test specific scenarios.
  * However, it should be exausitve in testing invariants across all tradable pairs
- * in the system, therfore each test should.
+ * in the system, therefore each test should.
  */
-contract BaseForkTest is Test, TestAsserts {
+abstract contract BaseForkTest is Test {
   using FixidityLib for FixidityLib.Fraction;
 
-  using Utils for Utils.Context;
-  using Utils for uint256;
-
-  struct ExchangeWithProvider {
-    address exchangeProvider;
-    IExchangeProvider.Exchange exchange;
-  }
-
   IRegistry public registry = IRegistry(CELO_REGISTRY_ADDRESS);
 
   address governance;
@@ -56,12 +43,9 @@ contract BaseForkTest is Test, TestAsserts {
 
   address public trader;
 
-  ExchangeWithProvider[] public exchanges;
-  mapping(address => mapping(bytes32 => ExchangeWithProvider)) public exchangeMap;
-
-  uint8 public constant L0 = 1; // 0b001 Limit0
-  uint8 public constant L1 = 2; // 0b010 Limit1
-  uint8 public constant LG = 4; // 0b100 LimitGlobal
+  // @dev The number of collateral assets 5 is hardcoded here:
+  // [CELO, AxelarUSDC, EUROC, NativeUSDC, NativeUSDT]
+  uint8 public constant COLLATERAL_ASSETS_COUNT = 5;
 
   uint256 targetChainId;
 
@@ -77,8 +61,13 @@ contract BaseForkTest is Test, TestAsserts {
     return addr;
   }
 
+  mapping(address rateFeed => uint8 count) rateFeedDependenciesCount;
+
   function setUp() public virtual {
     fork(targetChainId);
+    // @dev Updaing the target fork block every 200 blocks, about ~8 min.
+    // This means that when running locally RPC calls will be cached.
+    fork(targetChainId, (block.number / 100) * 100);
     // The precompile handler needs to be reinitialized after forking.
     __CeloPrecompiles_init();
 
@@ -91,43 +80,22 @@ contract BaseForkTest is Test, TestAsserts {
     trader = makeAddr("trader");
     reserve = IReserve(broker.reserve());
 
-    vm.startPrank(trader);
-
-    address[] memory exchangeProviders = broker.getExchangeProviders();
-    for (uint256 i = 0; i < exchangeProviders.length; i++) {
-      vm.label(exchangeProviders[i], "ExchangeProvider");
-      IExchangeProvider.Exchange[] memory _exchanges = IExchangeProvider(exchangeProviders[i]).getExchanges();
-      for (uint256 j = 0; j < _exchanges.length; j++) {
-        exchanges.push(ExchangeWithProvider(exchangeProviders[i], _exchanges[j]));
-        exchangeMap[exchangeProviders[i]][_exchanges[j].exchangeId] = ExchangeWithProvider(
-          exchangeProviders[i],
-          _exchanges[j]
-        );
-      }
-    }
-    require(exchanges.length > 0, "No exchanges found");
-
-    // The number of collateral assets 5 is hardcoded here [CELO, AxelarUSDC, EUROC, NativeUSDC, NativeUSDT]
-    for (uint256 i = 0; i < 5; i++) {
-      address collateralAsset = reserve.collateralAssets(i);
-      vm.label(collateralAsset, IERC20(collateralAsset).symbol());
-      _deal(collateralAsset, address(reserve), Utils.toSubunits(25_000_000, collateralAsset), true);
-      console.log("Minting 25mil %s to reserve", IERC20(collateralAsset).symbol());
-    }
-
-    console.log("Exchanges(%d): ", exchanges.length);
-    for (uint256 i = 0; i < exchanges.length; i++) {
-      Utils.Context memory ctx = Utils.newContext(address(this), i);
-      console.log("%d | %s | %s", i, ctx.ticker(), ctx.exchangeProvider);
-      console.logBytes32(ctx.exchange.exchangeId);
-    }
+    /// @dev Hardcoded number of dependencies for each ratefeed.
+    /// Should be updated when they change, there is a test that will
+    /// validate that.
+    rateFeedDependenciesCount[lookup("StableTokenXOF")] = 2;
+    rateFeedDependenciesCount[toRateFeed("EUROCXOF")] = 2;
+    rateFeedDependenciesCount[toRateFeed("USDCEUR")] = 1;
+    rateFeedDependenciesCount[toRateFeed("USDCBRL")] = 1;
   }
 
-  function _deal(address asset, address to, uint256 amount, bool updateSupply) public {
+  function mint(address asset, address to, uint256 amount, bool updateSupply) public {
     if (asset == lookup("GoldToken")) {
-      vm.startPrank(address(0));
+      if (!updateSupply) {
+        revert("BaseForkTest: can't mint GoldToken without updating supply");
+      }
+      vm.prank(address(0));
       IMint(asset).mint(to, amount);
-      vm.startPrank(trader);
       return;
     }
 

diff --git a/test/fork/ChainForkTest.sol b/test/fork/ChainForkTest.sol
@@ -2,21 +2,26 @@
 // solhint-disable func-name-mixedcase, var-name-mixedcase, state-visibility, const-name-snakecase, max-states-count
 pragma solidity ^0.8;
 
+import { console } from "forge-std/console.sol";
 import "./BaseForkTest.sol";
 
+import { IExchangeProvider } from "contracts/interfaces/IExchangeProvider.sol";
 import { IBiPoolManager } from "contracts/interfaces/IBiPoolManager.sol";
 import { IStableTokenV2DeprecatedInit } from "contracts/interfaces/IStableTokenV2DeprecatedInit.sol";
 
-contract ChainForkTest is BaseForkTest {
+abstract contract ChainForkTest is BaseForkTest {
   using FixidityLib for FixidityLib.Fraction;
 
-  using Utils for Utils.Context;
-  using Utils for uint256;
+  uint256 expectedExchangeProvidersCount;
+  uint256[] expectedExchangesCount;
 
-  uint256 expectedExchangesCount;
-
-  constructor(uint256 _chainId, uint256 _expectedExchangesCount) BaseForkTest(_chainId) {
+  constructor(
+    uint256 _chainId,
+    uint256 _expectedExchangesProvidersCount,
+    uint256[] memory _expectedExchangesCount
+  ) BaseForkTest(_chainId) {
     expectedExchangesCount = _expectedExchangesCount;
+    expectedExchangeProvidersCount = _expectedExchangesProvidersCount;
   }
 
   function test_biPoolManagerCanNotBeReinitialized() public {
@@ -53,8 +58,35 @@ contract ChainForkTest is BaseForkTest {
     );
   }
 
-  function test_testsAreConfigured() public view {
-    assertEq(expectedExchangesCount, exchanges.length);
+  /**
+   * @dev If this fails it means we have added new exchanges
+   * and haven't updated the fork test configuration which
+   * can be found in ForkTests.t.sol.
+   */
+  function test_exchangeProvidersAndExchangesCount() public view {
+    address[] memory exchangeProviders = broker.getExchangeProviders();
+    assertEq(expectedExchangeProvidersCount, exchangeProviders.length);
+    for (uint256 i = 0; i < exchangeProviders.length; i++) {
+      address exchangeProvider = exchangeProviders[i];
+      IBiPoolManager biPoolManager = IBiPoolManager(exchangeProvider);
+      IExchangeProvider.Exchange[] memory exchanges = biPoolManager.getExchanges();
+      assertEq(expectedExchangesCount[i], exchanges.length);
+    }
+  }
+
+  /**
+   * @dev If this fails it means we have added a new collateral
+   * so we need to update the COLLATERAL_ASSETS constant.
+   * This is because we don't have an easy way to determine
+   * the number of collateral assets in the system.
+   */
+  function test_numberCollateralAssetsCount() public {
+    address collateral;
+    for (uint256 i = 0; i < COLLATERAL_ASSETS_COUNT; i++) {
+      collateral = reserve.collateralAssets(i);
+    }
+    vm.expectRevert();
+    reserve.collateralAssets(COLLATERAL_ASSETS_COUNT);
   }
 
   function test_stableTokensCanNotBeReinitialized() public {
@@ -89,4 +121,28 @@ contract ChainForkTest is BaseForkTest {
     vm.expectRevert("Initializable: contract is already initialized");
     stableTokenKES.initialize("", "", 8, address(10), 0, 0, new address[](0), new uint256[](0), "");
   }
+
+  function test_rateFeedDependenciesCountIsCorrect() public {
+    address[] memory rateFeedIds = breakerBox.getRateFeeds();
+    for (uint256 i = 0; i < rateFeedIds.length; i++) {
+      address rateFeedId = rateFeedIds[i];
+      uint8 count = rateFeedDependenciesCount[rateFeedId];
+
+      vm.expectRevert();
+      breakerBox.rateFeedDependencies(rateFeedId, count); // end of array
+
+      for (uint256 j = 0; j < count; j++) {
+        (bool ok, ) = address(breakerBox).staticcall(
+          abi.encodeWithSelector(breakerBox.rateFeedDependencies.selector, rateFeedId, j)
+        );
+        if (!ok) {
+          console.log("Dependency missing for rateFeedId=%s, expectedCount=%d, missingIndex=%d", rateFeedId, count, j);
+          console.log(
+            "If the configuration has changed, update the rateFeedDependenciesCount mapping in BaseForfTest.sol"
+          );
+        }
+        require(ok, "rateFeedDependenciesCount out of sync");
+      }
+    }
+  }
 }
diff --git a/test/fork/EnvForkTest.t.sol b/test/fork/EnvForkTest.t.sol