Estimates and routerArray

[GainsGoblin]

Fixes:

• Fixed a lot of swap path logic in strategies.
• Added harvest estimations to all strategies.
• BaseStrategy now uses routerArray to swap between routers during harvest.
• SetRouter now can modify routerArray and approves all needed tokens for all routers.
• Harvester now harvests using _estimates array instead of individual parameters.

[gpersoon]

At the _harvest functions: Either use arrayCounter everywhere were applicable (more readable, but slightly more gas) Or (if you really want to save gas): Add comments where arrayCounter is replaced with constants (e.g. 0 / i+1) After the _extraRewardsLength loop, arrayCounter can be calculated directly: arrayCounter = _extraRewardsLength + 1; and no need to increase arrayCounter in the loop; The last increase "arrayCounter += 2;" for example in ETHConvexStrategy.sol isn't necessary

[gpersoon]

Why isn't getEstimates() using safemath consistently? (now it's mix of mul / sub / div / - )

[gpersoon]

I don't think the following constructions in getEstimates() are valid: function getEstimates() public view returns (uint256[] memory _estimates) { _estimates[0]= ... _estimates[_estimates.length] = ...

I can't get them working in remix. It does compile but I get an invalid opcode when executing the code. I think you should allocate memory first and then index the items, something like: _estimates = new uint[](crvRewards.extraRewardsLength() + 5);

[gpersoon]

in getEstimates() _slippage is always used as: ONE_HUNDRED_PERCENT - _slippage might as well calculate that directly to save some gas: uint256 _notSlippage = ONE_HUNDRED_PERCENT - IHarvester(manager.harvester()).slippage();

and then replace "ONE_HUNDRED_PERCENT - _slippage" with "_notSlippage"