Conversation
charlesndalton
left a comment
charlesndalton
left a comment
There was a problem hiding this comment.
Left some comments. Please feel free to reach me on TG @charlesndalton if you have any questions/disagreements. Also, Mickey tells me that you are new to yStrategies – welcome!
|
|
||
| interface IDecimals { | ||
| function decimals() external view returns (uint8); | ||
| function decimals() external view returns (uint256); |
There was a problem hiding this comment.
Instead of introducing IDecimals, you can use IERC20Metadata. Imported with 'import {IERC20Metadata} from "@yearnvaults/contracts/yToken.sol";'
There was a problem hiding this comment.
Can't do this with the standard library, which brings the decimal in as uint8.
When we use the decimals in getting to converted base:
uint256 convertedBase = base.mul(10 ** IDecimals(address(want)).decimals()).div(sharePrice);
A uint8 would overflow and so it needed to be casted to a uint256.
| return balanceOfWant(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
re: expTime in _sellRewards, since this is initiated from a contract and not an EOA I'm pretty sure you don't need to declare expTime and can just input 'now' into the function
| uint256 public constant max = type(uint256).max; | ||
|
|
||
| //Base amount for floor $LOOKS token | ||
| uint256 private constant base = 1e18; |
There was a problem hiding this comment.
suggest renaming this variable to something more descriptive (e.g., LOOKS_DUST)
There was a problem hiding this comment.
Done:
base -> LOOKS_MIN
| event profitEvent(uint256 _amount); | ||
| event sharePriceEvent(uint256 _amount); | ||
| event amountEvent(uint256 _amount); | ||
| event sharesEvent(uint256 _amount); | ||
| event convertedBaseEvent(uint256 _amount); | ||
| event upperLimitEvent(uint256 _amount); |
There was a problem hiding this comment.
You've got some funny line justification here
There was a problem hiding this comment.
I see for the below events as well
| event sharesEvent(uint256 _amount); | ||
| event convertedBaseEvent(uint256 _amount); | ||
| event upperLimitEvent(uint256 _amount); | ||
| function withdrawProfitOrFloor(uint profit) internal { |
There was a problem hiding this comment.
I feel the name 'withdrawProfitOrFloor' is not super intuitive–would suggest renaming
There was a problem hiding this comment.
Also, have you considered consolidating this logic and the liquidatePosition() logic (if it is different) in liquidatePosition() and calling that?
There was a problem hiding this comment.
Also, have you considered consolidating this logic and the liquidatePosition() logic (if it is different) in liquidatePosition() and calling that?
Will need to evaluate that more. My hunch is that we do want these to be distinct assuming there are other times we'd want to call liquidatePosition() that doesn't come from the reporting?
| looksRareContract.withdrawAll(false); | ||
|
|
||
| //We do a check to see if we have any WETH from the last rewards set that isn't converted. | ||
| uint256 wethCheck = IERC20(WETH).balanceOf(address(this)); |
| //We do a check to see if we have any WETH from the last rewards set that isn't converted. | ||
| uint256 wethCheck = IERC20(WETH).balanceOf(address(this)); | ||
|
|
||
| //If the WETH balance is non-zero we'll initiate a swap. |
There was a problem hiding this comment.
nit: "we'll initiate a swap" -> "we'll transfer it to the new strategy"
| //check if tokens are approved as needed. | ||
| if(IERC20(LOOKSToken).allowance(address(this),LooksRareStaking) == 0){ | ||
| IERC20(LOOKSToken).approve(LooksRareStaking, max); | ||
| if(LOOKSToken.allowance(address(this),LooksRareStaking) == 0){ |
There was a problem hiding this comment.
If this is a first time approval, why do you need to check allowance? Shouldn't it always be 0?
There was a problem hiding this comment.
Yeah, this was a relic from a version where I had univ2 router as changable and this function recallable so we could switch between other AMMS.
Resolved this.
|
|
||
| //We then withdraw with claim to get the full balance including the latest rewards | ||
| looksRareContract.withdrawAll(true); | ||
| //Full withdraw excluding rewards incase of reward errors. |
There was a problem hiding this comment.
Just curious, what are the reward errors?
There was a problem hiding this comment.
It's a MasterChef system, they internally calculate rewards based on their rewards curve, and when a user claims it, it mints it from the ERC20 contract it owns.
Those can have issues in three locations:
-
If the ERC20 mint has a cap, the MasterChef doesn't check for that before assigning a user rewards. So if there was a 1M token cap, and 0.9M had been minted, but an additional 200k tokens were waiting in our pending rewards if we try and claim it would all fail.
-
In the event that MasterChef loses minting rights over the token (such as if they migrate versions) the rewards claiming fails.
-
If they change the reward curve after launch, pending rewards can down right break in the way they are calculated depending on last check in (underflow usually), so the claiming breaks.
Most team's realize these things and either modify MasterChef, or plan their rewards curve before hand and do not change it so it all aligns, but it is possible that you run into these issues.
| @@ -332,7 +336,7 @@ event postWithdrawBalanceEvent(uint256 _amount); | |||
| } | |||
There was a problem hiding this comment.
I'm not sure but ySwaps may be required here, instead of selling from strat. WDYT @Micky?
There was a problem hiding this comment.
Open to it, but the liquidity is pretty good, so unless this strategy gets quite large, I can't see the sells being worth a frontrun. But not as experienced with that part.
2 participants
No description provided.