-
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: add flashmint hyeth #64
Conversation
exchange: Exchange.None, | ||
} | ||
} | ||
if (inputOutputToken.symbol === USDC.symbol) { |
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.
Now that I see this function again. If I make another check for ETH
to return null, I can in this case probably just always return with path [WETH.address!, inputOutputToken.address]
no matter the ERC-20 input/output token, correct? @ckoopmann So I'd delete the specific check for USDC.
This would make it more future proof.
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.
If there is a UniV3 pool between the two tokens for the given fee level then yes you can just return that path.
// https://etherscan.io/address/0xa0d3707c569ff8c87fa923d3823ec5d81c98be78#readProxyContract | ||
const tokenContract = new Contract(component, VAULT_ABI, provider) | ||
// TODO: confirm previewWithdraw (and not previewRedeem) | ||
const stEthAmount: BigNumber = await tokenContract.previewWithdraw(position) |
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.
@pblivin0x please confirm previewWithdraw
is the correct function to call (and not previewRedeem)
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.
this is actually a redeem() call in FlashMintHyETH https://github.com/IndexCoop/index-coop-smart-contracts/blob/72243db162ad34124db681aad363746bed075944/contracts/exchangeIssuance/FlashMintHyETH.sol#L521
, so i would expect
const stEthAmount: BigNumber = await tokenContract.previewWithdraw(position) | |
const stEthAmount: BigNumber = await tokenContract.previewRedeem(position) |
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.
Updated to previewRedeem
. Thank you!
isMinting ? inputToken.decimals : outputToken.decimals, | ||
slippage, | ||
isMinting | ||
) |
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.
@pblivin0x we used this slippage adjusting function for ZeroEx as well. it seems to makes sense here as otherwise mint quotes can revert.
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.
it makes sense to use some slippage adjustments, but they may be better placed in the integrations
- Instadapp - implementation need to replace preview method feat: add flashmint hyeth #64 (comment)
- Across - implementation already applies a small rounding error on deposits feat: add flashmint hyeth #64 (comment)
- Pendle - its known that large deposits/withdrawals will move the exchangeRate() oracle, and this created issues in FlashMintHyETH and auction bots. here are some notes on handling feat: add flashmint hyeth #64 (comment) it will probably be best check previews before applying a similar inflation factor
with these itemized adjustments made, it may not be necessary to do a overall slippage adjustment here
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.
Individual changes have been made but we still might wanna watch slippage for a bit.
const ethAmount = | ||
(exchangeRate.toBigInt() * acrossLpAmount) / BigInt(1e18) + | ||
this.roundingError |
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.
const ethAmount = | ||
(exchangeRate.toBigInt() * acrossLpAmount) / BigInt(1e18) + | ||
this.roundingError |
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.
note that this conversion with roundingError isn't the same as FlashMintHyETH https://github.com/IndexCoop/index-coop-smart-contracts/blob/72243db162ad34124db681aad363746bed075944/contracts/exchangeIssuance/FlashMintHyETH.sol#L573
adding the rounding error here increase the amount of expected output (as opposed to increasing the amount of input for deposits)
should be able to omit this.roundingError or subtract is instead
// console.log(sy, 'SY', component) | ||
const routerContract = this.getRouterStatic(this.routerStaticMainnet) | ||
const assetRate: BigNumber = await routerContract.getPtToAssetRate(market) | ||
const ethAmount = (position * assetRate.toBigInt()) / BigInt(1e18) |
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.
noting that if the deposit is large enough to move the exchangeRate()
, then this calculation will under quote the amount of ETH required.
on FlashMintHyETH we check the previewDeposit()
method to confirm if our ethAmount needs to be inflated or not
const assetRate: BigNumber = await routerContract.getPtToAssetRate(market) | ||
const ethAmount = (position * assetRate.toBigInt()) / BigInt(1e18) |
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.
there isn't any logic for values large enough to move the assetRate()
on FlashMintHyETH because it isn't necessary for the computation, but some may be necessary here
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.
Let's watch this. It does seem like slippage has to be higher for redemptions but I think Allan also mentioned that this would be the case for FlashMint (thus the request to also allow direct redemption).
isMinting: true, | ||
inputToken: eth, | ||
outputToken: indexToken, | ||
indexTokenAmount: wei('1'), |
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.
can we test with much larger amounts? like 1000 ether
? that will let us know if any of the integrations quotes break with large values
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.
Highest I could do for now is 300 but that's 1+m. Should be enough?
TODOs
Most of these only work once the token is rebalanced/migrated.