GEN-1630: increase test coverage

README.md

@@ -22,7 +22,7 @@ Here are some important concepts to understand so you know how the auto-roller w
 * `AutoRollerFactory` - on-chain factory for easy deployment of new auto-roller instances.
 
 * `RollerPeriphery` - the slippage protected LP management interface to all AutoRollers.
-* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`.
+* `OwnableAdapter` - Sense adapter owned by a unique auto-roller. Through the auto-roller, series rollers can sponsor new series via `AutoRoller.roll` and settle series via `AutoRoller.settle`. This adapter must **override** `getMaturityBounds` so only the roller can sponsor preventing a potential DoS attack.
 
 ### Phases
 The auto-roller has an **active** and a **cooldown** phase. During the active phase, there is a specific Space pool that the auto-roller is managing, and the liquidity it holds is in the form of Space LP shares and YTs (e.g. sY-wstETH). In contrast, the cooldown phase is in-between active phases when the auto-roller has settled one series but not yet sponsored a new one and deposited liquidity into the new pool (also at the very beginning of the contract’s lifecycle before it has entered the first pool). 

codecov.yml

@@ -0,0 +1,4 @@
+# interfaces are automatically ignored by forge coverage
+ignore:
+  - "**/**.t*.sol"                          # ignore test files (ending in t*.sol)
+  - "**/**.s.sol"                           # ignore script files (ending in s.sol)

src/AutoRoller.sol

@@ -76,7 +76,7 @@ contract AutoRoller is ERC4626 {
 
     DividerLike      internal immutable divider;
     BalancerVault    internal immutable balancerVault;
-    OwnedAdapterLike internal immutable adapter;
+    OwnedAdapterLike internal immutable adapter; // This adapter must **override** `getMaturityBounds` so only the roller can sponsor preventing a potential DoS attack.
 
     uint256 internal immutable ifee; // Cached issuance fee.
     uint256 internal immutable minSwapAmount; // Min number of PTs that can be swapped out when exiting.
@@ -200,9 +200,9 @@ contract AutoRoller is ERC4626 {
         uint256 targetBal = _asset.balanceOf(address(this));
 
         // Get the reserve balances that would imply the given targetedRate in the Space pool,
-        // assuming that we we're going to deposit the amount of Target currently in this contract.
+        // assuming that we're going to deposit the amount of Target currently in this contract.
         // In other words, this function simulating the reserve balances that would result from the actions:
-        // 1) Use the some Target to issue PTs/YTs
+        // 1) Use some Target to issue PTs/YTs
         // 2) Deposit some amount of Target
         // 3) Swap PTs into the pool to initialize the targeted rate
         // 4) Deposit the rest of the PTs and Target in this contract (which remain in the exact ratio the pool expects)
@@ -272,7 +272,7 @@ contract AutoRoller is ERC4626 {
     }
 
     /// @notice Settle the active Series, transfer stake and ifees to the settler, and enter a cooldown phase.
-    /// @dev Because the auto-roller is the series sponsor from the Divider's perspective, this.settle is the only entrypoint for athe lastRoller to settle during the series' sponsor window.
+    /// @dev Because the auto-roller is the series sponsor from the Divider's perspective, this.settle is the only entrypoint for the lastRoller to settle during the series' sponsor window.
     ///      More info on the series lifecylce: https://docs.sense.finance/docs/series-lifecycle-detail/#phase-3-settling.
     function settle() public {
         if(msg.sender != lastRoller) revert InvalidSettler();
@@ -369,7 +369,7 @@ contract AutoRoller is ERC4626 {
 
             balances[1 - _pti] = targetToJoin;
 
-            if (assets - targetToJoin > 0) { // Assumption: this is false if Space has only Target liquidity.
+            if (assets - targetToJoin > 0) { // Assumption: this is false if Space has only Target liquidity. TODO: can this ever happen?
                 balances[_pti] = divider.issue(address(adapter), maturity, assets - targetToJoin);
             }
 

src/test/AutoRoller.t.sol

@@ -152,6 +152,42 @@ contract AutoRollerTest is DSTestPlus {
 
     // Auth
 
+    function testUpdateAdminParams() public {
+        vm.record();
+
+        // 1. Impersonate owner and try to update admin params
+        vm.startPrank(address(this));
+
+        autoRoller.setParam("SPACE_FACTORY", address(0xbabe));
+
+        autoRoller.setParam("PERIPHERY", address(0xbabe));
+
+        vm.expectRevert();
+        autoRoller.setParam("UNKNOWN", address(0xbabe));
+
+        autoRoller.setParam("OWNER", address(0xbabe));
+
+        vm.stopPrank();
+
+        // 2. Impersonate new owner try to update rest of admin params
+        vm.startPrank(address(0xbabe));
+
+        autoRoller.setParam("MAX_RATE", 1337);
+
+        autoRoller.setParam("TARGET_DURATION", 1337);
+
+        autoRoller.setParam("COOLDOWN", 1337);
+
+        vm.expectRevert();
+        autoRoller.setParam("UNKNOWN", 1337);
+
+        vm.stopPrank();
+
+        (, bytes32[] memory writes) = vm.accesses(address(autoRoller));
+        // Check that no storage slots were written to
+        assertEq(writes.length, 6);
+    }
+
     function testFuzzUpdateAdminParams(address lad) public {
         vm.record();
         vm.assume(lad != address(this)); // For any address other than the testing contract
@@ -167,6 +203,9 @@ contract AutoRollerTest is DSTestPlus {
         vm.expectRevert();
         autoRoller.setParam("OWNER", address(0xbabe));
 
+        vm.expectRevert();
+        autoRoller.setParam("UNKNOWN", address(0xbabe));
+
         vm.expectRevert();
         autoRoller.setParam("MAX_RATE", 1337);
 
@@ -176,6 +215,9 @@ contract AutoRollerTest is DSTestPlus {
         vm.expectRevert();
         autoRoller.setParam("COOLDOWN", 1337);
 
+        vm.expectRevert();
+        autoRoller.setParam("UNKNOWN", 1337);
+
         (, bytes32[] memory writes) = vm.accesses(address(autoRoller));
         // Check that no storage slots were written to
         assertEq(writes.length, 0);
@@ -936,12 +978,195 @@ contract AutoRollerTest is DSTestPlus {
         utils.getNewTargetedRate(0, address(mockAdapter), maturity, space);
 
         vm.warp(maturity);
+
+        // Scale goes down
+        mockAdapter.setScale(0.9e18);
         autoRoller.settle();
 
         // Targeted rate is 0 if scale has gone down.
-        mockAdapter.setScale(0.9e18);
         uint256 targetedRate = utils.getNewTargetedRate(0, address(mockAdapter), maturity, space);
         assertEq(targetedRate, 0);
+
+        vm.warp(maturity + autoRoller.cooldown());
+
+        // Roll into new series
+        autoRoller.roll();
+
+        // Scale goes up
+        mockAdapter.setScale(1.9e18);
+
+        maturity = autoRoller.maturity();
+        vm.warp(maturity);
+
+        autoRoller.settle();
+
+        // Targeted rate is not 0 TODO: maybe assert against the rate that should be 
+        targetedRate = utils.getNewTargetedRate(0, address(mockAdapter), maturity, space);
+        assertGt(targetedRate, 0);
+    }
+
+    // OTHER TESTS (LINES NOT COVERED ACCORDING CODECOV) TODO: re-order these tests?
+
+    function testAfterDepositWithOnlyTargetLiquidity() public {
+        // The idea is making a test that covers line case on the `afterDeposit` function
+        // where `if (assets - targetToJoin > 0)` is false.
+        // I think can never happen since `onSponsorWindowOpened`, when calling `_space.getEQReserves`
+        // we are doing `targetedRate < 0.01e18 ? 0.01e18 : targetedRate` which means that we are always
+        // getting a `eqPTReserves` > 0 so we will be issuing PTs. 
+    }
+
+    function testPreviewWithdrawInsufficentLiquidity(uint256 amount) public {
+        // 1. Roll into the first Series.
+        autoRoller.roll();
+
+        // 2. Make a deposit.
+        autoRoller.deposit(1e18, address(this));
+
+        // 3. Expect revert because of InsufficientLiquidity.
+        vm.expectRevert(abi.encodeWithSelector(AutoRoller.InsufficientLiquidity.selector));
+        autoRoller.previewWithdraw(100e18);
+    }
+
+    function testMaxWithdrawDuringCooldown(uint256 amount) public {
+        // 1. Make a deposit durring cooldown.
+        autoRoller.deposit(1e18, address(this));
+
+        // 2. Assert max withdraw is same as deposit.
+        uint256 maxWithdraw = autoRoller.maxWithdraw(address(this));
+        assertEq(maxWithdraw, 1e18);
+    }
+
+    function testRedeemWithYTsExcess() public {
+        // 1. Deposit during the initial cooldown phase.
+        autoRoller.deposit(0.2e18, address(this));
+        assertEq(autoRoller.balanceOf(address(this)), 0.2e18);
+
+        // 3. Roll the Target into the first Series.
+        autoRoller.roll();
+
+        // 4. Deposit during the first active phase.
+        autoRoller.deposit(0.3e18, address(this));
+        assertRelApproxEq(autoRoller.balanceOf(address(this)), 0.5e18, 0.0001e18 /* 0.01% */);
+
+        // 5. Redeem all shares while still in the active phase.
+        uint256 targetBalPre = target.balanceOf(address(this));
+        vm.expectCall(address(periphery), abi.encodeWithSelector(periphery.swapYTsForTarget.selector));
+        autoRoller.redeem(autoRoller.balanceOf(address(this)), address(this), address(this));
+        uint256 targetBalPost = target.balanceOf(address(this));
+        // assertRelApproxEq(targetBalPost - targetBalPre, 0.5e18, 0.0001e18 /* 0.01% */); // TODO: what should it be the expected value?
+
+        // Check that the Target dust leftover is small
+        assertLt(target.balanceOf(address(autoRoller)), 1e9);
+    }
+
+    function testMaxRedeemDuringCooldown(uint256 amount) public {
+        // 1. Make a deposit during cooldown.
+        uint256 sharesOut = autoRoller.deposit(0.1e18, address(this));
+
+        // 2. Assert max withdraw is same sharesOut.
+        assertEq(autoRoller.maxRedeem(address(this)), sharesOut);
+
+        // 4. Roll into the first Series.
+        autoRoller.roll();
+
+        // 3. Make another deposit during cooldown.
+        sharesOut += autoRoller.deposit(0.1e18, address(this));
+
+        vm.warp(autoRoller.maturity());
+
+        // 4. Increase scale and settle the first Series.
+        autoRoller.settle();
+
+        // 5. Assert max withdraw is same total shares.
+        assertEq(autoRoller.maxRedeem(address(this)), sharesOut);
+    }
+
+    function testMaxRedeemWithPTsExcess(uint256 amount) public {
+        // 1. Make a deposit durring cooldown.
+        uint256 sharesOut = autoRoller.deposit(1e18, address(this));
+
+        // 2. Roll into the first Series.
+        autoRoller.roll();
+
+        // 3. Swap PTs in to generate excess of PTs.
+        target.mint(address(this), 1e18);
+        target.approve(address(divider), 1e18);
+        divider.issue(address(mockAdapter), autoRoller.maturity(), 1e18);
+        ERC20 pt = ERC20(divider.pt(address(mockAdapter), autoRoller.maturity()));
+        Space space = Space(spaceFactory.pools(address(mockAdapter), autoRoller.maturity()));
+        pt.approve(address(balancerVault), 1e18);
+
+        // 4. Swap PTs in to generate excess of PTs and that the number of PTs we can sell
+        // is less than the diff between YTs and PTs.
+        _swap(
+            BalancerVault.SingleSwap({
+                poolId: space.getPoolId(),
+                kind: BalancerVault.SwapKind.GIVEN_IN,
+                assetIn: address(pt),
+                assetOut: address(autoRoller.asset()),
+                amount: 0.001e18, // TODO: how do I calculate a number such that it would make the maxPTSale < diff? This number works though
+                userData: hex""
+            })
+        );
+
+        // 5. Assert max redeem is same as sharesOut.
+        assertEq(autoRoller.maxRedeem(address(this)), sharesOut);
+
+        // 6. Swap PTs in to generate excess of PTs and that the number of PTs we can sell
+        // is more than the diff between YTs and PTs.
+        _swap(
+            BalancerVault.SingleSwap({
+                poolId: space.getPoolId(),
+                kind: BalancerVault.SwapKind.GIVEN_IN,
+                assetIn: address(pt),
+                assetOut: address(autoRoller.asset()),
+                amount: 0.01e18, // TODO: how do I calculate a number such that it would make the maxPTSale >= diff? This number works though
+                userData: hex""
+            })
+        );
+
+        // 6. Assert max redeem is same as TODO.
+        sharesOut = autoRoller.maxRedeem(address(this));
+        // assertEq(sharesOut, TODO);
+
+        // 7. TODO: add missing scenario: `if (ptReserves >= diff)`.
+    }
+
+    function testEjectWithPTsExcess() public {
+        // 1. Deposit during the initial cooldown phase.
+        autoRoller.deposit(1e18, address(this));
+
+        // 2. Roll into the first Series.
+        autoRoller.roll();
+
+        // 3. Swap PTs in to generate excess of PTs.
+        target.mint(address(this), 1e18);
+        target.approve(address(divider), 1e18);
+        divider.issue(address(mockAdapter), autoRoller.maturity(), 1e18);
+
+        ERC20 pt = ERC20(divider.pt(address(mockAdapter), autoRoller.maturity()));
+        Space space = Space(spaceFactory.pools(address(mockAdapter), autoRoller.maturity()));
+
+        pt.approve(address(balancerVault), 1e18);
+        _swap(
+            BalancerVault.SingleSwap({
+                poolId: space.getPoolId(),
+                kind: BalancerVault.SwapKind.GIVEN_IN,
+                assetIn: address(pt),
+                assetOut: address(autoRoller.asset()),
+                amount: 0.001e18,
+                userData: hex""
+            })
+        );
+
+        // 4. Eject everything.
+        uint256 ptBalBefore = pt.balanceOf(address(this));
+        ( , uint256 excessBal, bool isExcessPTs) = autoRoller.eject(autoRoller.balanceOf(address(this)), address(this), address(this));
+
+        // Expect just a little PT excess.
+        assertBoolEq(isExcessPTs, true);
+        assertLt(excessBal, 1e16);
+        assertEq(excessBal, pt.balanceOf(address(this)) - ptBalBefore);
     }
 
     // function testRedeemPreviewReversion() public {