build: refund accountant #27
Conversation
| @@ -7,6 +7,9 @@ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol | |||
|
|
|||
| import {IVault} from "@yearn-vaults/interfaces/IVault.sol"; | |||
|
|
|||
| /// @title Health Check Accountant. | |||
| /// @dev Will charge fees issue refunds as well as run healthcheck on any | |||
There was a problem hiding this comment.
added commas, and fix typo...
"Will charge fees, issue refunds, and run health check on any reported gains or losses during a strategy's report."
| @@ -17,18 +20,24 @@ contract HealthCheckAccountant { | |||
| event UpdateDefaultFeeConfig(Fee defaultFeeConfig); | |||
There was a problem hiding this comment.
style: should put struct definition ahead of its usage Fee
There was a problem hiding this comment.
would prefer to keep the events about the struct definition
| @@ -66,8 +75,29 @@ contract HealthCheckAccountant { | |||
| _; | |||
| } | |||
|
|
|||
| modifier onlyVaultManagers() { | |||
There was a problem hiding this comment.
small: technically this should be onlyVaultOrFeeManagers? maybe we feel that fee manager is already implied?
| @@ -320,7 +366,7 @@ contract HealthCheckAccountant { | |||
| uint16 customMaxFee, | |||
| uint16 customMaxGain, | |||
| uint16 customMaxLoss | |||
| ) external onlyFeeManager { | |||
| ) external virtual onlyFeeManager { | |||
There was a problem hiding this comment.
small suggestion for natspec:
on updateDefaultConfig instead of "used for all strategies", i would say: "... effective for all strategies that don't already have a custom config set"
| } | ||
|
|
||
| function _checkVaultIsAdded() internal view virtual { | ||
| require(vaults[msg.sender], "!authorized"); |
There was a problem hiding this comment.
small: i would suggest slightly more informative message like "vault not added" rather than generic access control.
| @@ -390,7 +432,7 @@ contract HealthCheckAccountant { | |||
| function custom( | |||
| address vault, | |||
| address strategy | |||
| ) external view returns (bool) { | |||
There was a problem hiding this comment.
what do you think about isCustomConfig or hasCustomConfig instead of custom?
There was a problem hiding this comment.
Went with useCustomConfig
No description provided.