Base strategy #93
Base strategy #93
Conversation
cleanups
syntax nits
| factory = _factory; | ||
|
|
||
| for (uint256 i = 0; i < _allowedPaths.length; i++) { | ||
| allowedPaths[keccak256(abi.encodePacked(_allowedPaths[i]))] = true; |
There was a problem hiding this comment.
probably good to specify paths here, though should we already make this compatible with trident?
There was a problem hiding this comment.
Switched this so it uses a "bridge token". For now it will stay on the current amm as we will deploy these pre trident
| function _harvest(uint256 balance) internal override returns (int256) { | ||
| uint256 keep = toShare(balance); | ||
| uint256 total = sushiBar.balanceOf(address(this)); | ||
| if (total > keep) sushiBar.leave(total - keep); |
There was a problem hiding this comment.
toShare can also return the total Shares to save one contract call here.
There was a problem hiding this comment.
We are interested in the share balance of address(this) which we can't return in toShare unfortunately
| return int256(contractBalance); | ||
| } else if (contractBalance > 0) { | ||
| // we might have made a loss | ||
| int256 diff = amount + int256(contractBalance); |
There was a problem hiding this comment.
Why would we sum the amount and contractBalance if a loss is already reported?
Shouldn't anything sitting on the strategy contract be accounted for?
There was a problem hiding this comment.
The thought process here is that _harvest shouldn't concern itself about external rewards.
| (success, ) = to.call{value: value}(data); | ||
| } | ||
|
|
||
| // **** SWAP **** |
There was a problem hiding this comment.
Do we want to include the current swap architecture even if we are moving on soon enough?
| _; | ||
| } | ||
|
|
||
| function toggleStrategyExecutor(address executor) external onlyOwner { |
There was a problem hiding this comment.
Why a toggle? Why not just pass in a boolean? Both work, but this may be more prone to mistakes?
| uint256 maxChangeAmount, | ||
| bool harvestRewards | ||
| ) external onlyExecutor { | ||
| if (harvestRewards) _harvestRewards(); |
There was a problem hiding this comment.
Purely a style thing, but personally I never write if statements without braces. I know you can have endless debates about this, but with the string of exploits of Solidity code I think anything that leads to more robust code is worth adopting, even if it adds 4 characters.
|
|
||
| if (diff > 0) { | ||
| // we still made some profit | ||
| _skim(uint256(-amount)); |
There was a problem hiding this comment.
Took me a while to figure out what's going on here... important contracts like this can benefit from commenting every line ;)
| using BoringMath for uint256; | ||
| using BoringERC20 for IERC20; | ||
|
|
||
| IERC20 public immutable underlying; |
There was a problem hiding this comment.
Using the term underlying implies that it underlies some other token. For readability, why not just call this variable 'token'?
| returned by the internal _harvest function isn't necessary the final profit/loss amount */ | ||
| int256 amount = _harvest(balance); | ||
|
|
||
| uint256 contractBalance = IERC20(underlying).safeBalanceOf(address(this)); |
There was a problem hiding this comment.
No need to cast here and in the rest of the contract. underlying is already IERC20 ;)
| return int256(0); | ||
| } | ||
|
|
||
| /// @notice Harvests any rewards and rebalances them to the underlying token |
There was a problem hiding this comment.
"Harvests rewards and swaps all rewards into the token and places them on this contract." ?
| @@ -0,0 +1,217 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Did a quick review and left some comments. Nice contract and this will make it easier to create robust strategies.
If I understand this correctly, the goal is to make implementing strategies simpler by providing a simplified virtual interface. The idea seems to be to allow 'normal' harvesting to happen anytime, but to keep rewards harvesting to only executors. Swapping is already provided, including whitelisting paths. After exit recovery is built in.
You might want to put all the virtual methods at the end and document them thoroughly, so anyone using this known what to implement and how.
There was a problem hiding this comment.
Appreciate the opinionated review! I have updated the code and added samples accordingly.
| contract ERC20Mock is ERC20 { | ||
| uint256 public totalSupply; | ||
|
|
||
| contract ERC20Mock is ERC20WithSupply { |
There was a problem hiding this comment.
ERC20WithSupply is undeclared
| @@ -10,7 +10,7 @@ import "@boringcrypto/boring-solidity/contracts/libraries/BoringMath.sol"; | |||
| contract SushiBarMock is ERC20 { | |||
| using BoringMath for uint256; | |||
| ERC20 public sushi; | |||
| uint256 public totalSupply; | |||
| uint256 public override totalSupply; | |||
There was a problem hiding this comment.
There is currently no override, I guess this is supposed to be an inheritance of the missing ERC20WithSupply @gasperbr
| E.g. reward tokens that have been sold into the underlying tokens which are now sitting in the contract. | ||
| Meaning the amount returned by the internal _harvest function isn't necessary the final profit/loss amount */ | ||
|
|
||
| uint256 contractBalance = strategyToken.safeBalanceOf(address(this)); |
There was a problem hiding this comment.
safeBalanceOf does not exist in the library
Add an abstract contract for BentoBox strategies. The abstract contract makes the harvest function effectively private and minimizes the risk of a sandwich attack exploit.
To write a strategy extend the abstract contract and implement the _skim, _harvest, _withdraw, and _exit functions.