From afc800787865075c0c5d7eab857eb36c46a7539b Mon Sep 17 00:00:00 2001 From: Schlag <89420541+Schlagonia@users.noreply.github.com> Date: Mon, 28 Aug 2023 16:47:43 -0600 Subject: [PATCH] fix: interface (#23) * fix: interface * fix: check health functions * fix: read me and comments * fix: spelling --- README.md | 44 +++++++--------- src/HealthCheck/BaseHealthCheck.sol | 19 +++---- ...{IHealthCheck.sol => IBaseHealthCheck.sol} | 4 +- src/test/HealthCheck.sol | 51 +++++++++++++++++++ src/test/mocks/MockHealthCheck.sol | 50 +++++++++++++++--- 5 files changed, 120 insertions(+), 48 deletions(-) rename src/HealthCheck/{IHealthCheck.sol => IBaseHealthCheck.sol} (79%) diff --git a/README.md b/README.md index 51fac8c..551a1f4 100644 --- a/README.md +++ b/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%). diff --git a/src/HealthCheck/BaseHealthCheck.sol b/src/HealthCheck/BaseHealthCheck.sol index da0b04d..0bf1895 100644 --- a/src/HealthCheck/BaseHealthCheck.sol +++ b/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 diff --git a/src/HealthCheck/IHealthCheck.sol b/src/HealthCheck/IBaseHealthCheck.sol similarity index 79% rename from src/HealthCheck/IHealthCheck.sol rename to src/HealthCheck/IBaseHealthCheck.sol index 72399bc..786d404 100644 --- a/src/HealthCheck/IHealthCheck.sol +++ b/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); diff --git a/src/test/HealthCheck.sol b/src/test/HealthCheck.sol index 3d88615..78c0884 100644 --- a/src/test/HealthCheck.sol +++ b/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); + } } diff --git a/src/test/mocks/MockHealthCheck.sol b/src/test/mocks/MockHealthCheck.sol index a21bf02..93cedf8 100644 --- a/src/test/mocks/MockHealthCheck.sol +++ b/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; +}