test: refactor tests to follow best practices (#19)

* Update PerpetualPositionRouter tests

* Clean up DepositRouter tests

* Reformat tests to follow best practices

* Reformat code

* Update Formatting

* Remove unnecessary instantiation

* Rename tests

* Remove last remaining fork test

* Change create 2 to use keccak256

* chore: rename var

---------

Co-authored-by: Matt Solomon <[email protected]>

script/Deploy.s.sol

@@ -16,9 +16,9 @@ contract Deploy is Script, PerpetualContracts {
       new PerpetualRouterFactory(clearingHouse, accountBalance, vault);
 
     vm.broadcast();
-    factory.deploy(PerpetualRouterFactory.RouterTypes.DepositRouterType, USDC);
+    factory.deploy(PerpetualRouterFactory.RouterType.DepositRouterType, USDC);
 
     vm.broadcast();
-    factory.deploy(PerpetualRouterFactory.RouterTypes.PositionRouterType, VETH);
+    factory.deploy(PerpetualRouterFactory.RouterType.PositionRouterType, VETH);
   }
 }

src/PerpetualRouterFactory.sol

@@ -17,7 +17,7 @@ contract PerpetualRouterFactory {
   error RouterTypeDoesNotExist();
 
   /// @dev The different types of routers that can be deployed by the factory.
-  enum RouterTypes {
+  enum RouterType {
     PositionRouterType,
     DepositRouterType
   }
@@ -32,7 +32,7 @@ contract PerpetualRouterFactory {
   IVault public immutable PERPETUAL_VAULT;
 
   /// @dev Emitted on a successfully deployed router.
-  event RouterDeployed(RouterTypes indexed routerType, address indexed asset);
+  event RouterDeployed(RouterType indexed type_, address indexed asset);
 
   /// @param clearingHouse Address of the Perpetual clearing house contract.
   /// @param accountBalance Address of the Perpetual account balance contract.
@@ -49,31 +49,31 @@ contract PerpetualRouterFactory {
   /// the case has not been handled yet. It should never revert in production.
   /// @param type_ The type of router to deploy.
   /// @param asset The token the router uses to manage deposits and positions.
-  function deploy(RouterTypes type_, address asset) external returns (address) {
+  function deploy(RouterType type_, address asset) external returns (address) {
     bytes32 salt = _salt(asset);
     address router;
-    if (type_ == RouterTypes.PositionRouterType) {
+    if (type_ == RouterType.PositionRouterType) {
       router = address(
         new PerpetualPositionRouter{salt: salt}(
           PERPETUAL_CLEARING_HOUSE, PERPETUAL_ACCOUNT_BALANCE, asset
         )
       );
-    } else if (type_ == RouterTypes.DepositRouterType) {
+    } else if (type_ == RouterType.DepositRouterType) {
       router = address(new DepositRouter{salt: salt}(asset, PERPETUAL_VAULT));
     } else {
       revert RouterTypeDoesNotExist();
     }
-    emit RouterDeployed(RouterTypes.PositionRouterType, asset);
+    emit RouterDeployed(type_, asset);
     return router;
   }
 
   /// @notice Returns the address for a router of a given `asset` and router `type_`. This function
   /// will still return an address even if the router has not been deployed.
   /// @dev This function will only revert with `RouterTypeDoesNotExist` if a new router is added and
   /// the case has not been handled yet. It should never revert in production.
-  function computeAddress(RouterTypes type_, address asset) external view returns (address) {
-    if (type_ == RouterTypes.PositionRouterType) return _computePositionAddress(asset);
-    else if (type_ == RouterTypes.DepositRouterType) return _computeDepositAddress(asset);
+  function computeAddress(RouterType type_, address asset) external view returns (address) {
+    if (type_ == RouterType.PositionRouterType) return _computePositionAddress(asset);
+    else if (type_ == RouterType.DepositRouterType) return _computeDepositAddress(asset);
     else revert RouterTypeDoesNotExist();
   }
 

test/DepositRouter.t.sol

@@ -5,40 +5,60 @@ import {Test} from "forge-std/Test.sol";
 import {ERC20} from "solmate/tokens/ERC20.sol";
 import {SafeTransferLib} from "solmate/utils/SafeTransferLib.sol";
 
+import {DepositRouter} from "src/DepositRouter.sol";
 import {PerpetualContracts} from "test/PerpetualContracts.sol";
 import {PerpetualRouterFactory} from "src/PerpetualRouterFactory.sol";
 
-contract DepositRouterForkTestBase is Test, PerpetualContracts {
+contract DepositRouterTest is Test, PerpetualContracts {
   PerpetualRouterFactory factory;
   address routerAddress;
 
   function setUp() public {
     vm.createSelectFork(vm.rpcUrl("optimism"), 87_407_144);
     factory = new PerpetualRouterFactory(clearingHouse, accountBalance, vault);
-    factory.deploy(PerpetualRouterFactory.RouterTypes.DepositRouterType, USDC);
-    deal(USDC, address(this), 100_000_000);
-    deal(address(this), 100 ether);
+    factory.deploy(PerpetualRouterFactory.RouterType.DepositRouterType, USDC);
     routerAddress =
-      address(factory.computeAddress(PerpetualRouterFactory.RouterTypes.DepositRouterType, USDC));
+      address(factory.computeAddress(PerpetualRouterFactory.RouterType.DepositRouterType, USDC));
   }
 }
 
-// Add deposit interface
-contract DepositForkTest is DepositRouterForkTestBase {
-  function test_Deposit() public {
-    uint256 amount = 1_000_000;
+contract Constructor is DepositRouterTest {
+  function test_CorrectlySetsAllConstructorArgs() public {
+    DepositRouter router = new DepositRouter(VETH, vault);
+    assertEq(address(router.PERPETUAL_VAULT()), address(vault), "VAULT not set correctly");
+    assertEq(router.TOKEN(), VETH, "TOKEN not set correctly");
+  }
+}
+
+contract Fallback is DepositRouterTest {
+  function testForkFuzz_TraderDepositsUsdcIntoVault(uint256 amount) public {
+    uint256 settlementTokenBalanceCap = clearingHouseConfig.getSettlementTokenBalanceCap();
+    uint256 vaultBalance = ERC20(USDC).balanceOf(address(vault));
 
+    amount = bound(amount, 1, settlementTokenBalanceCap - vaultBalance);
+
+    deal(USDC, address(this), amount);
     ERC20(USDC).approve(routerAddress, amount);
+
     (bool ok,) = payable(routerAddress).call(abi.encode(amount));
+
     int256 balance = vault.getBalanceByToken(address(this), USDC);
     assertTrue(ok);
     assertEq(balance, int256(amount));
   }
+}
+
+contract Receive is DepositRouterTest {
+  function testForkFuzz_TraderDepositsEtherIntoVault(uint256 amount) public {
+    uint256 depositCap = collateralManager.getCollateralConfig(WETH).depositCap;
+    uint256 vaultBalance = ERC20(WETH).balanceOf(address(vault));
 
-  function test_DepositEth() public {
-    (bool ok,) = payable(routerAddress).call{value: 1 ether}("");
+    amount = bound(amount, 1, depositCap - vaultBalance);
+
+    deal(address(this), amount);
+    (bool ok,) = payable(routerAddress).call{value: amount}("");
     int256 balance = vault.getBalanceByToken(address(this), WETH);
     assertTrue(ok);
-    assertEq(balance, 1 ether);
+    assertEq(balance, int256(amount));
   }
 }

test/PerpetualContracts.sol

@@ -1,9 +1,13 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.0;
 
+import {ERC20} from "solmate/tokens/ERC20.sol";
+
 import {IVault} from "src/interface/IVault.sol";
 import {IClearingHouse} from "src/interface/IClearingHouse.sol";
 import {IAccountBalance} from "src/interface/IAccountBalance.sol";
+import {ICollateralManager} from "test/interface/ICollateralManager.sol";
+import {IClearingHouseConfig} from "test/interface/IClearingHouseConfig.sol";
 import {IDelegateApproval} from "test/interface/IDelegateApproval.sol";
 
 contract PerpetualContracts {
@@ -13,6 +17,11 @@ contract PerpetualContracts {
   IVault vault = IVault(0xAD7b4C162707E0B2b5f6fdDbD3f8538A5fbA0d60);
   IAccountBalance accountBalance = IAccountBalance(0xA7f3FC32043757039d5e13d790EE43edBcBa8b7c);
   IDelegateApproval delegateApproval = IDelegateApproval(0xfd7bB5F6844a43c5469c972640Eddfa99597a547);
+  ICollateralManager collateralManager =
+    ICollateralManager(0x8Ac835C05530f10595C8015467339523154b4D85);
+  IClearingHouseConfig clearingHouseConfig =
+    IClearingHouseConfig(0xA4c817a425D3443BAf610CA614c8B11688a288Fb);
+
   address public immutable VETH = 0x8C835DFaA34e2AE61775e80EE29E2c724c6AE2BB;
   address public immutable USDC = 0x7F5c764cBc14f9669B88837ca1490cCa17c31607;
   address public immutable WETH = 0x4200000000000000000000000000000000000006;

test/PerpetualPositionRouter.t.sol

@@ -3,25 +3,38 @@ pragma solidity ^0.8.0;
 
 import {Test} from "forge-std/Test.sol";
 
+import {IAccountBalance} from "src/interface/IAccountBalance.sol";
+import {IClearingHouse} from "src/interface/IClearingHouse.sol";
 import {PerpetualRouterFactory} from "src/PerpetualRouterFactory.sol";
+import {PerpetualPositionRouter} from "src/PerpetualPositionRouter.sol";
 import {AccountMarket} from "src/lib/AccountMarket.sol";
 import {PerpetualContracts} from "test/PerpetualContracts.sol";
 
-contract PositionRouterForkTestBase is Test, PerpetualContracts {
-  PerpetualRouterFactory factory;
+contract PerpetualPositionRouterTestHarness is PerpetualPositionRouter {
+  constructor(IClearingHouse clearingHouse, IAccountBalance accountBalance, address asset)
+    PerpetualPositionRouter(clearingHouse, accountBalance, asset)
+  {}
+
+  function extractSqrtPriceLimitX96(uint168 args) external pure returns (uint160) {
+    return _extractSqrtPriceLimitX96(args);
+  }
+}
+
+contract PositionRouterTest is Test, PerpetualContracts {
   address vethPositionRouterAddr;
 
-  function setUp() public {
-    vm.createSelectFork(vm.rpcUrl("optimism"), 87_407_144);
-    factory = new PerpetualRouterFactory(clearingHouse, accountBalance, vault);
-    factory.deploy(PerpetualRouterFactory.RouterTypes.PositionRouterType, VETH);
-    deal(address(this), 100 ether);
-    vault.depositEther{value: 10 ether}();
+  function setUp() public virtual {
+    PerpetualRouterFactory factory =
+      new PerpetualRouterFactory(clearingHouse, accountBalance, vault);
     vethPositionRouterAddr =
-      address(factory.computeAddress(PerpetualRouterFactory.RouterTypes.PositionRouterType, VETH));
+      address(factory.computeAddress(PerpetualRouterFactory.RouterType.PositionRouterType, VETH));
+  }
+
+  function encodeArgs(uint8 funcId, uint160 sqrtPriceLimitX96) internal pure returns (uint168) {
+    return (uint168(funcId) << 160) | uint168(sqrtPriceLimitX96);
   }
 
-  function closePosititionHelper(
+  function closePositionHelper(
     uint8 openFunc,
     uint256 amount,
     uint256 oppositeAmountBound,
@@ -38,14 +51,69 @@ contract PositionRouterForkTestBase is Test, PerpetualContracts {
     assertTrue(ok);
     assertTrue(okTwo);
   }
+}
 
-  function encodeArgs(uint8 funcId, uint160 sqrtPriceLimitX96) internal pure returns (uint168) {
-    return (uint168(funcId) << 160) | uint168(sqrtPriceLimitX96);
+contract Constructor is PositionRouterTest {
+  function test_CorrectlySetsAllConstructorArgs() public {
+    PerpetualPositionRouter router = new PerpetualPositionRouter(
+        clearingHouse,
+        accountBalance,
+        VETH
+    );
+    assertEq(
+      address(router.PERPETUAL_CLEARING_HOUSE()),
+      address(clearingHouse),
+      "PERPETUAL_CLEARING_HOUSE not set correctly"
+    );
+    assertEq(
+      address(router.ACCOUNT_BALANCE()),
+      address(accountBalance),
+      "ACCOUNT_BALANCE not set correctly"
+    );
+    assertEq(router.TOKEN(), VETH, "TOKEN not set correctly");
   }
 }
 
-contract OpenPositionLongInputFork is PositionRouterForkTestBase {
-  function test_FallbackLongInput() public {
+contract _ExtractSqrtPriceLimitX96 is PositionRouterTest {
+  function testFuzz_SuccessfullyExtractsSqrtPriceLimitX96(uint8 funcId, uint160 sqrtPriceLimitX96)
+    public
+  {
+    PerpetualPositionRouterTestHarness harness = new PerpetualPositionRouterTestHarness(
+      clearingHouse,
+      accountBalance,
+      VETH
+    );
+    assertEq(
+      harness.extractSqrtPriceLimitX96(encodeArgs(funcId, sqrtPriceLimitX96)), sqrtPriceLimitX96
+    );
+  }
+
+  function testFuzz_SuccessfullyReencodeArgs(uint168 args) public {
+    PerpetualPositionRouterTestHarness harness = new PerpetualPositionRouterTestHarness(
+      clearingHouse,
+      accountBalance,
+      VETH
+    );
+    uint168 firstEightBitMask = ((1 << 8) - 1) << 160;
+    uint8 funcId = uint8((args & firstEightBitMask) >> 160);
+    assertEq(encodeArgs(funcId, harness.extractSqrtPriceLimitX96(args)), args);
+  }
+}
+
+contract Fallback is PositionRouterTest {
+  PerpetualRouterFactory factory;
+
+  function setUp() public override {
+    vm.createSelectFork(vm.rpcUrl("optimism"), 87_407_144);
+    factory = new PerpetualRouterFactory(clearingHouse, accountBalance, vault);
+    factory.deploy(PerpetualRouterFactory.RouterType.PositionRouterType, VETH);
+    deal(address(this), 100 ether);
+    vault.depositEther{value: 10 ether}();
+    vethPositionRouterAddr =
+      address(factory.computeAddress(PerpetualRouterFactory.RouterType.PositionRouterType, VETH));
+  }
+
+  function testFork_OpenLongExactInputPosition() public {
     delegateApproval.approve(vethPositionRouterAddr, 1);
     uint168 combinedArgs = encodeArgs(4, 0);
     (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
@@ -59,7 +127,7 @@ contract OpenPositionLongInputFork is PositionRouterForkTestBase {
     assertEq(info.takerPositionSize, 538_599_759_293_451);
   }
 
-  function test_FallbackLongOutput() public {
+  function testFork_OpenLongExactOutputPosition() public {
     delegateApproval.approve(vethPositionRouterAddr, 1);
     uint168 combinedArgs = encodeArgs(3, 0);
     (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
@@ -74,7 +142,7 @@ contract OpenPositionLongInputFork is PositionRouterForkTestBase {
     assertEq(info.takerPositionSize, 1 ether);
   }
 
-  function test_FallbackShortInput() public {
+  function testFork_OpenShortExactInputPosition() public {
     delegateApproval.approve(vethPositionRouterAddr, 1);
     uint168 combinedArgs = encodeArgs(2, 0);
     (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
@@ -87,7 +155,7 @@ contract OpenPositionLongInputFork is PositionRouterForkTestBase {
     assertEq(info.takerPositionSize, -1 ether);
   }
 
-  function test_FallbackShortOutput() public {
+  function testFork_OpenShortExactOutputPosition() public {
     delegateApproval.approve(vethPositionRouterAddr, 1);
     uint168 combinedArgs = encodeArgs(1, 0);
     (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
@@ -100,33 +168,51 @@ contract OpenPositionLongInputFork is PositionRouterForkTestBase {
     assertEq(info.takerPositionSize, -539_678_586_420_661);
   }
 
-  function test_FallbackClosePositionLong() public {
-    closePosititionHelper(4, 1 ether, 0, 0);
+  function testFork_CloseLongExactInputPosition() public {
+    closePositionHelper(4, 1 ether, 0, 0);
     AccountMarket.Info memory info = accountBalance.getAccountInfo(address(this), VETH);
     assertEq(info.takerOpenNotional, 0);
     assertEq(info.takerPositionSize, 0);
   }
 
-  function test_FallbackClosePositionShortInput() public {
-    closePosititionHelper(2, 5 ether, 0, 0);
+  function testFork_CloseLongExactOutputPosition() public {
+    closePositionHelper(3, 1 ether, 0, 0);
 
     AccountMarket.Info memory info = accountBalance.getAccountInfo(address(this), VETH);
     assertEq(info.takerOpenNotional, 0);
     assertEq(info.takerPositionSize, 0);
   }
 
-  function test_FallbackClosePositionShortOutput() public {
-    closePosititionHelper(1, 1 ether, 0, 0);
+  function testFork_CloseShortExactInputPosition() public {
+    closePositionHelper(2, 5 ether, 0, 0);
+
     AccountMarket.Info memory info = accountBalance.getAccountInfo(address(this), VETH);
     assertEq(info.takerOpenNotional, 0);
     assertEq(info.takerPositionSize, 0);
   }
 
-  function test_FallbackClosePositionLongOutput() public {
-    closePosititionHelper(3, 1 ether, 0, 0);
-
+  function testFork_CloseShortExactOutputPosition() public {
+    closePositionHelper(1, 1 ether, 0, 0);
     AccountMarket.Info memory info = accountBalance.getAccountInfo(address(this), VETH);
     assertEq(info.takerOpenNotional, 0);
     assertEq(info.takerPositionSize, 0);
   }
+
+  function testFork_FailedCallWhenExtraCalldataArgument() public {
+    uint168 combinedArgs = encodeArgs(4, 0);
+    (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0, 100));
+    assertTrue(!ok);
+  }
+
+  function testFork_FailedClosePositionCallWithWrongArguments() public {
+    uint168 combinedArgs = encodeArgs(5, 0);
+    (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
+    assertTrue(!ok);
+  }
+
+  function testFork_FailedFallbackWithZeroFuncId() public {
+    uint168 combinedArgs = encodeArgs(0, 0);
+    (bool ok,) = payable(vethPositionRouterAddr).call(abi.encode(combinedArgs, 1 ether, 0));
+    assertTrue(!ok);
+  }
 }