fix: interface (#23)

* fix: interface

* fix: check health functions

* fix: read me and comments

* fix: spelling

README.md

@@ -53,7 +53,7 @@ EX:
         
         ...
         
-        function _harvestAndReport() exeternal override returns (uint256) {
+        function _harvestAndReport() internal override returns (uint256) {
             ... Claim rewards
             
             uint256 rewardBalance = ERC20(rewardToken).balanceOf(address(this));
@@ -68,41 +68,31 @@ NOTE: Its very important to read through all the comments in the swapper contrac
 ## HealthCheck
 Health Checks can be used by a strategy to assure automated reports are not unexpectedly reporting a loss or a extreme profit that a strategist doesn't expect.
 
-It's important to note that the healthcheck does not stop losses from being reported, rather will require manual intervention from 'management' for out of range losses or gains.
+It's important to note that the health check does not stop losses from being reported, rather will require manual intervention from 'management' for out of range losses or gains.
 
-A strategist simply has to inherit the `HealthCheck` contract in their strategy, set the profit and loss limit ratios with the needed setters, and then call `_executeHealthCheck(uint256)` with the expected return value as the parameter during `_harvestAndReport`.
+A strategist simply has to inherit the [BaseHealthCheck](https://github.com/yearn/tokenized-strategy-periphery/blob/master/src/HealthCheck/BaseHealthCheck.sol) contract in their strategy, set the profit and loss limit ratios with the needed setters, and then call `_executeHealthCheck(uint256)` with the expected return value as the parameter during `_harvestAndReport`.
 
 EX:
 
-    contract Strategy is HealthCheck, BaseTokenizedStrategy {
-         ...
-         
-        function setProfitLimitRatio(
-            uint256 _profitLimitRatio
-        ) external onlyManagement {
-            _setProfitLimitRatio(_profitLimitRatio);
-        }
-
-        function setLossLimitRatio(
-            uint256 _lossLimitRatio
-        ) external onlyManagement {
-            _setLossLimitRatio(_lossLimitRatio);
-        }
-
-        function setDoHealthCheck(bool _doHealthCheck) external onlyManagement {
-            doHealthCheck = _doHealthCheck;
-        }
-        
+    contract Strategy is BaseHealthCheck {
         ...
         
-        function _harvestAndReport() internal override returns (uint256) {
+        function _harvestAndReport() internal override returns (uint256 _totalAssets) {
             ...
             
-            if (doHealthCheck) {
-                require(_executeHealthCheck(_totalAssets), "!healthcheck");
-            }
+            _executeHealthCheck(_totalAssets);
+        
         }
     }
+
+The profit and loss ratios can adjusted by management through their specific setters as well as turning the healthCheck off for a specific report. If turned off the health check will automatically turn back on for the next report.
+
+The Health check contract also comes with a [checkHealth](https://github.com/yearn/tokenized-strategy-periphery/blob/master/src/HealthCheck/BaseHealthCheck.sol#L28) modifier that can be put on functions that will check any strategy specific invariants/checks that can be defined in the [_checkHealth](https://github.com/yearn/tokenized-strategy-periphery/blob/master/src/HealthCheck/BaseHealthCheck.sol#L124) function. 
+
+NOTE: This should revert to work properly as a modifier if the check is false.
+
+see [MockHealthCheck](https://github.com/yearn/tokenized-strategy-periphery/blob/master/src/test/mocks/MockHealthCheck.sol) for an example.
+
 ## Apr Oracle
 For easy integration with on chain debt allocator's as well as off chain interfaces, strategist's can implement their own custom 'AprOracle'.
 
@@ -112,4 +102,4 @@ The goal of the APR oracle to is to return the expected current APY the strategy
 ## Report Triggers
 The default use of the Tokenized Strategies and V3 Vaults is for report cycles to be based off of the individual `maxProfitUnlockTime`. The triggers are an easy way for keepers to monitor the status of each strategy and know when `report` should be called on each.
 
-However, if a strategist wants to implement a custom trigger for their strategy or vault you can use the simple `CustomTriggerBase` contracts to inherit the proper interface. Then return the expected APY repersented as 1e18 (1e17 == 10%).
+However, if a strategist wants to implement a custom trigger for their strategy or vault you can use the simple `CustomTriggerBase` contracts to inherit the proper interface. Then return the expected APY represented as 1e18 (1e17 == 10%).

src/HealthCheck/BaseHealthCheck.sol

@@ -24,7 +24,8 @@ import {BaseTokenizedStrategy} from "@tokenized-strategy/BaseTokenizedStrategy.s
  */
 abstract contract BaseHealthCheck is BaseTokenizedStrategy {
     // Optional modifier that can be placed on any function
-    // to perform checks such as debt/ PPS before running.
+    // to perform checks such as debt/PPS before running.
+    // Must override `_checkHealth()` for this to work.
     modifier checkHealth() {
         _checkHealth();
         _;
@@ -118,20 +119,12 @@ abstract contract BaseHealthCheck is BaseTokenizedStrategy {
 
     /**
      * @notice Check important invariants for the strategy.
-     * @dev This deafults to checking totalDebt but can be overriden
-     * to check any important strategy specific invariants.
-     */
-    function _checkHealth() internal virtual {
-        require(TokenizedStrategy.totalDebt() <= _currentDebt(), "debt");
-    }
-
-    /**
-     * @dev Return the current expected debt the strategy has for
-     * the HealthCheck modifier.
+     * @dev This can be overriden to check any important strategy
+     *  specific invariants.
      *
-     * @param . Estimate of the current value of the strategies debt.
+     *  NOTE: Should revert if unhealthy for the modifier to work.
      */
-    function _currentDebt() internal view virtual returns (uint256);
+    function _checkHealth() internal virtual {}
 
     /**
      * @dev To be called during a report to make sure the profit

src/HealthCheck/IHealthCheck.sol → src/HealthCheck/IBaseHealthCheck.sol

@@ -1,7 +1,9 @@
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity 0.8.18;
 
-interface IHealthCheck {
+import {IStrategy} from "@tokenized-strategy/interfaces/IStrategy.sol";
+
+interface IBaseHealthCheck is IStrategy {
     function doHealthCheck() external view returns (bool);
 
     function profitLimitRatio() external view returns (uint256);

src/test/HealthCheck.sol

@@ -428,4 +428,55 @@ contract HealthCheckTest is Setup {
             "doHealthCheck should be true"
         );
     }
+
+    function test__checkHealthModifier(uint256 _amount) public {
+        vm.assume(_amount >= minFuzzAmount && _amount <= maxFuzzAmount);
+
+        // deposit
+        mintAndDepositIntoStrategy(
+            IStrategy(address(healthCheck)),
+            user,
+            _amount
+        );
+
+        assertEq(healthCheck.healthy(), true);
+        assertEq(healthCheck.availableDepositLimit(user), type(uint256).max);
+        assertEq(healthCheck.availableWithdrawLimit(user), type(uint256).max);
+
+        healthCheck.setHealthy(false);
+
+        assertEq(healthCheck.healthy(), false);
+        assertEq(healthCheck.availableDepositLimit(user), 0);
+        assertEq(healthCheck.availableWithdrawLimit(user), 0);
+
+        vm.expectRevert("unhealthy");
+        vm.prank(keeper);
+        healthCheck.report();
+
+        healthCheck.setHealthy(true);
+
+        assertEq(healthCheck.healthy(), true);
+        assertEq(healthCheck.availableDepositLimit(user), type(uint256).max);
+        assertEq(healthCheck.availableWithdrawLimit(user), type(uint256).max);
+
+        vm.prank(keeper);
+        (uint256 realProfit, ) = healthCheck.report();
+
+        // Make sure we reported the correct profit
+        assertEq(0, realProfit, "Reported profit mismatch");
+
+        // Healtch Check should still be on
+        assertEq(
+            healthCheck.doHealthCheck(),
+            true,
+            "doHealthCheck should be true"
+        );
+
+        skip(healthCheck.profitMaxUnlockTime());
+
+        vm.prank(user);
+        healthCheck.redeem(_amount, user, user);
+
+        assertGe(asset.balanceOf(user), _amount);
+    }
 }

src/test/mocks/MockHealthCheck.sol

@@ -6,12 +6,19 @@ import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
 import {BaseHealthCheck} from "../../HealthCheck/BaseHealthCheck.sol";
 
 contract MockHealthCheck is BaseHealthCheck {
+    bool public healthy = true;
+
     constructor(address _asset) BaseHealthCheck(_asset, "Mock Health Check") {}
 
-    function _deployFunds(uint256) internal override checkHealth {}
+    // `healthy` is already implemented in deposit limit so
+    // doesn't need to be checked again.
+    function _deployFunds(uint256) internal override {}
 
-    function _freeFunds(uint256) internal override checkHealth {}
+    // `healthy` is already implemented in withdraw limit so
+    // doesn't need to be checked again.
+    function _freeFunds(uint256) internal override {}
 
+    // Uses `checkHealth` modifier
     function _harvestAndReport()
         internal
         override
@@ -23,12 +30,41 @@ contract MockHealthCheck is BaseHealthCheck {
         _executeHealthCheck(_totalAssets);
     }
 
-    function _currentDebt() internal view override returns (uint256) {
-        return 0;
+    // Can't deposit if its not healthy
+    function availableDepositLimit(
+        address _owner
+    ) public view override returns (uint256) {
+        if (!_healthy()) return 0;
+
+        return super.availableDepositLimit(_owner);
+    }
+
+    // Can't Withdraw if not healthy.
+    function availableWithdrawLimit(
+        address _owner
+    ) public view override returns (uint256) {
+        if (!_healthy()) return 0;
+
+        return super.availableWithdrawLimit(_owner);
+    }
+
+    function _checkHealth() internal view override {
+        require(_healthy(), "unhealthy");
+    }
+
+    function _healthy() internal view returns (bool) {
+        return healthy;
+    }
+
+    function setHealthy(bool _health) external {
+        healthy = _health;
     }
 }
 
-import {IStrategy} from "@tokenized-strategy/interfaces/IStrategy.sol";
-import {IHealthCheck} from "../../HealthCheck/IHealthCheck.sol";
+import {IBaseHealthCheck} from "../../HealthCheck/IBaseHealthCheck.sol";
+
+interface IMockHealthCheck is IBaseHealthCheck {
+    function healthy() external view returns (bool);
 
-interface IMockHealthCheck is IStrategy, IHealthCheck {}
+    function setHealthy(bool _health) external;
+}