Implement previewDeposit/previewRedeem functions#48
Conversation
|
Is this ready for review? @jribbink |
jribbink
changed the title
previewDeposit & previewRedeem functions
jribbink
changed the title
previewDeposit & previewRedeem functionspreviewDeposit/previewRedeem functions
|
Yeah, can be reviewed now @jordanschalm 👍 |
| /// does). It is an estimate, not a guarantee: slippage is bounded by a | ||
| /// min-out at the deposit call site, not by this preview. |
There was a problem hiding this comment.
It is an estimate, not a guarantee
The spec says:
MUST return as close to and no more than the exact amount of assets that would be withdrawn in a redeem call in the same transaction
I think we aren't satisfying that right now. I am wondering if we can satisfy that part of the spec by assuming worst-case slippage in the preview functions. Not necessarily something to implement right away: it might be more important for us to have more accurate preview functions than less accurate but spec-compliant. Not sure.
What I would do now is explicitly call out that this part of the spec is not being satisfied by the current implementation, in the docs.
There was a problem hiding this comment.
Yeah - this is what I was referring to in the PR description. As of today, worst-case slippage doesn't exist for deposit/withdrawal; it's only enforced for rebalancing. I tried to leave these comments to call this out, but I can reword it to make it more explicit.
Ultimately we can't quote a preview with worst-case slippage for deposit/withdrawal, because that bound doesn't exist - swap slippage protection is only on rebalances right now, and not deposits/withdrawals (#44 (comment)).
On user safety: end users only ever touch the vault through the ERC4626Router, whose minSharesOut/minAssetsOut bound the realized output, so they're protected from excessive slippage. For third-party integrators, the guidance is the standard EIP-4626 one - don't rely on the preview as an exact quote or price oracle (per the spec, previews are "as close as possible" estimates and "not always safe to be used as price oracles"; use convertToShares/convertToAssets for valuation).
| // Previews are fee-inclusive oracle estimates that exclude AMM price impact. | ||
| // Against the 1:1 mock swap router these assert the EIP-4626 "no more than | ||
| // actual" direction + fee-inclusiveness. |
There was a problem hiding this comment.
Since the MockIrm always charges a 0% rate, I don't think we are testing the separate expectedDebt accrual logic. Probably worth adding a test case that exercises that.
There was a problem hiding this comment.
A couple other test suggestions:
- A test where yield appreciates before we redeem (exercising Path A of the redemption logic)
- A test that we satisfy this part of the spec, in relation to our TVL limit:
MUST NOT account for deposit limits like those returned from maxDeposit and should always act as though the deposit would be accepted, regardless if the user has enough tokens approved, etc.
Co-authored-by: Jordan Schalm <jordan.schalm@gmail.com>
2 participants
Works torwards #18
Implements
previewDeposit/previewRedeemfunctions by mirroring existingdeposit/redeemimpementations and calculating the expected outcome, with static fees and simulated debt accrual, in aviewcontext.Note: due to UniswapV3 quotes requiring state manipulations, it is not possible to calculate AMM price impact within the view context, we can only account for static fees. It is expected that callers will bound the slippage using an external wrapper like the Yearn router from #5. This is a limitation that exists because we swap funds during the user's funds during the deposit/withdrawal step, and make the user pay for their own fees instead of socializing them.