-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Sup 9265 perform a supervault test with a 5115 in between 4626s #27
feat: Sup 9265 perform a supervault test with a 5115 in between 4626s #27
Conversation
…supervault-test-with-a-5115-in-between-4626s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Someone could alter the the balance by sending SuperPositions
but I think it may be best to merge this and address that in a subsequent PR
superformContract = IBaseForm(superform); | ||
|
||
vars.spBalances[i] = | ||
ISuperPositions(_getAddress(keccak256("SUPER_POSITIONS"))).balanceOf(address(this), _superformIds_[i]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_getAddress(keccak256("SUPER_POSITIONS")))
can be cached to improve readbility and safe gas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
mvData.outputAmounts[i] = isERC5115 ? amountOut - TOLERANCE_CONSTANT : amountOut; | ||
|
||
mvData.amounts[i] = superformContract.previewDepositTo(amountOut); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be the withdraw version? (previewWithdrawFrom()
) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yearn tokenized strategies provide only "assets" and not shares when redeeming. So, first we have outputAmounts (which in Superform is the output "assets") . Then for it to interact with Superform core we must work back to shares, hence the reason for previewDepositTo
// For partial withdrawals, use proportional amounts | ||
uint256 amountOut = amount_.mulDiv(weights[i], TOTAL_WEIGHT, Math.Rounding.Down); | ||
|
||
mvData.outputAmounts[i] = isERC5115 ? amountOut - TOLERANCE_CONSTANT : amountOut; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps have a function to adjust for the tolerance, for easier maintenance, something like:
function tolerance(bool isERC5115, uint256 amount) view returns (uint256){
return isERC5115 ? amount - TOLERANCE_CONSTANT : amount;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
30f5f82 added
No description provided.