Harvesting basic structure #644
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
| * @notice Returns the address of the `token` that is harvested from the protocol. | ||
| * | ||
| */ | ||
| function getHarvestToken() external view returns (address token); |
There was a problem hiding this comment.
What about there is more than 1 token to harvest, like in the case of Morpho?
There was a problem hiding this comment.
good point. I will change to return address[] tokens . I've also added IVault as parameter because I've noticed a lot of them depend on the asset (makes sense).
| if (strategy == Strategy.ConvertToCollateral) { | ||
| //check if reward token is asset | ||
| if (address(token) != asset()) { | ||
| //swap for collateral | ||
| } | ||
| } | ||
|
|
||
| //strategy 2 = repay debt | ||
| /// @dev only implemented in BorrowingVault | ||
| if (strategy == Strategy.RepayDebt) { | ||
| revert BaseVault__harvest_strategyNotImplemented(); | ||
| } | ||
|
|
||
| //strategy 3 = distribute rewards | ||
| if (strategy == Strategy.Distribute) {} |
There was a problem hiding this comment.
Wouldn't it be more flexible if this logic is in the provider?
There was a problem hiding this comment.
Yes, it would. I actually was thinking about changing it because compoundV3 has a claimTo function which can be leveraged if we know the strategy. Well spotted !
There was a problem hiding this comment.
While implementing this, I realised it's not as useful as I thought to have this logic in the provider as it is in the vault. The claimTo function of CompoundV3 cannot be leveraged (we alway need to claimTo(address(vault)):
strategy 1: claim to vault to swap for collateral
strategy 2: claim to vault to swap for debt
strategy 3: cannot claim to all users to distribute rewards
In the general functionality of the harvest:
If we want to swap for collateral or debt (strategy 1 and 2) we will harvest and then swap. The strategy will still be in the ILendingProvider.harvest (and ILendingProvider.previewHarvest) in case we're able to choose which token to get rewards in. In those cases, the preview harvest will return the address[] tokens and uint256[] amounts according to the strategy (ex. previewHarvest will return the collateralAsset and amount if the strategy is convertToCollateral).
The logic present in the vault will be the swap (depending on the strategy and values returned by previewHarvest). If we put the swap logic inside the provider, we will be repeating a lot of code.
Will do this changes in this PR.
Let me know your thoughts on this.
…t from view function
| if (currentVaultHarvest != address(0)) { | ||
| revert HarvestManager__harvest_harvestAlreadyInProgress(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider also adding a check at Chief.sol if the vault is a "valid" vault.
There was a problem hiding this comment.
Added a verification using the vault status (chief.isVaultActive). what do you think?
| internal | ||
| returns (bytes memory data) | ||
| { | ||
| uint256 totalAmount = 0; |
There was a problem hiding this comment.
There is no need to explicitly declare a new stack variable = zero, it defaults to zero. The following should be good:
uint256 totalAmount;
The same comment applies in repayDebt(...)
| returns (address[] memory tokens, uint256[] memory amounts) | ||
| { | ||
| //check strategy is valid for this vault | ||
| if (strategy == Strategy.RepayDebt) { |
There was a problem hiding this comment.
No need to do it now, but I think this strategy check could be "pushed" to the HarvestManager.
There was a problem hiding this comment.
Same applies to BaseVaultUpgradeable.
There was a problem hiding this comment.
Good idea. I will add a TODO comment to be aware of this when pushing down the contract size
| //collect rewards from provider | ||
| (tokens, amounts) = _harvest(provider, data); | ||
|
|
||
| if (tokens.length != amounts.length) { |
There was a problem hiding this comment.
I believe this check was done here because you did not want to repeat this at the providers. However, for optimization note in the future I think we can create an abstract contract "Harvestable" that inherits "IHarvestable" that is inherited by a provider that is "harvestable". In there we can put repeated logic and checks, that we could offboard from the vault contracts.
There was a problem hiding this comment.
Same applies to BaseVaultUpgradeable.
There was a problem hiding this comment.
Makes sense :) Will add a TODO comment here as well
| callData, string(abi.encodePacked("harvest", ": delegate call failed")) | ||
| ); | ||
| (tokens, amounts) = abi.decode(returnData, (address[], uint256[])); | ||
| } |
There was a problem hiding this comment.
There should be some type of event emitted here, essentially for all the tokens and amounts harvested and the provider.
Same applies for the upgradeable vault.
| } | ||
|
|
||
| currentVaultHarvest = address(0); | ||
| } |
There was a problem hiding this comment.
Completing the harvest through the HarvestManager should also emit an Event.
This should not overlap with the information on the event that should also be emitted on the vault itself.
I probably suggest that this event here focuses on the treasury profit and what the vault "gained" by the strategy.
| @@ -109,7 +109,7 @@ contract UniswapV2Swapper is ISwapper { | |||
| { | |||
| address[] memory path = _buildPath(assetIn, assetOut); | |||
| uint256[] memory amounts = uniswapRouter.getAmountsOut(amountIn, path); | |||
| amountOut = amounts[1]; | |||
| amountOut = amounts[amounts.length - 1]; | |||
|
|
||
| contract BorrowingVault is BaseVault { | ||
| using Math for uint256; | ||
| using SafeERC20 for IERC20Metadata; | ||
| using SafeERC20 for IERC20; |
There was a problem hiding this comment.
Since IERC20Metadata is IERC20, just feel free to use IERC20Metadata. Same applies to upgradeable borrowing vault.
No description provided.