Skip to content
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

One-Click LP: Close Position #597

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

One-Click LP: Close Position #597

wants to merge 29 commits into from

Conversation

daryakaviani
Copy link
Contributor

Task: One-Click LP Close Position

Description

Close an LP position and withdraw the LP NFT that was used as collateral using a flashloan via the ControllerHelper's flashloanCloseVaultLpNft function.

Learn more about the One-Click LP API here.

@vercel
Copy link

vercel bot commented Jul 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
continuouscall ✅ Ready (Inspect) Visit Preview Aug 29, 2022 at 7:35AM (UTC)

@deepthinker250 deepthinker250 changed the base branch from main to one-click-lp-open July 13, 2022 17:51
Copy link
Contributor

@aleone aleone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes to limit price as well as way the tx is generated / what its trying to do.

@@ -68,6 +69,62 @@ export const useOpenPositionDeposit = () => {
return openPositionDeposit
}

// Close position with flashloan
export const useClosePosition = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe naming could be improved (can have close positions w/o flashloans, etc)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flashClosePosition?

vaultId: vaultId,
tokenId: uniTokenId,
liquidity: position.liquidity,
liquidityPercentage: fromTokenAmount(1, 18).toFixed(0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO for a more flexible API this should be flexible

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users could decide to close 50% of their LP to pay off 75% their debt as an example

tokenId: uniTokenId,
liquidity: position.liquidity,
liquidityPercentage: fromTokenAmount(1, 18).toFixed(0),
wPowerPerpAmountToBurn: shortAmount.toFixed(0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO for a more flexible API this should be flexible

const debtInEthPromise = getDebtAmount(shortAmount)
const limitEthPromise = getQuote(shortAmount, true)
const [debtInEth, limitEth] = await Promise.all([debtInEthPromise, limitEthPromise])
const collateralToFlashloan = debtInEth.multipliedBy(COLLAT_RATIO)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be higher, detailed comments on CollectFees PR

liquidityPercentage: fromTokenAmount(1, 18).toFixed(0),
wPowerPerpAmountToBurn: shortAmount.toFixed(0),
collateralToFlashloan: collateralToFlashloan.toFixed(0),
collateralToWithdraw: 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO for a more flexible API this should be flexible

Most people that close 100% of an LP and burn 100% of debt will want to get the ETH out, the current construction requires them to do it in 2 txs.

IMO should be able to be set by the f /e

wPowerPerpAmountToBurn: shortAmount.toFixed(0),
collateralToFlashloan: collateralToFlashloan.toFixed(0),
collateralToWithdraw: 0,
limitPriceEthPerPowerPerp: limitEth,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this param is incorrect and a couple of issues with how its being calculated.

  • This should be a limit price in units of ETHPerPowerPerp ie 0.1 ETH per 1 squeeth, but scaled by 1e18.
  • The calculation is assuming that you are selling oSQTH, but this is not always the case, sometimes you have to buy, so you have to check if the debt being burned is > or < than the LP composition.
  • Right now you are getting the amountOut from the swap (which is a reallllllly high limit price and will be sandwiched/always execute regardless of what market does)
  • This also needs to use slippage somewhere a user should specify a slippage for this trade and that needs to be taken into account when the limit price is calculated.

amount0Min: 0,
amount1Min: 0,
poolFee: POOL_FEE,
burnExactRemoved: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

burnExactRemoved means that none of the oSQTH will be bought or sold to burn the amount of debt listed (and the wPowerPerpAmountToBurn is not used), so there is no need for the above calculations and many of the params included are not used.

I think this is a good flag to expose in the API (most users may want to burnExactRemoved, but not all)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to discuss this and the above if any qs

@@ -210,6 +210,17 @@ export const useGetTwapEthPrice = () => {
return getTwapEthPrice
}

export const useGetTwapSqueethPrice = () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe + useGetSpotPrice using pool tick

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, using this in the new approach here!

.minus(vaultCollateralAmt)
const flashLoanAmountPos = BigNumber.max(flashLoanAmount, 0)

const limitPrice = new BigNumber(limitEth)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think this should be (1+slippage) if we're buying oSQTH when amountToLiquidate > amountToBurn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agreed


const limitPrice = new BigNumber(limitEth)
.div(amountToLiquidate.minus(amountToBurn).abs())
.times(new BigNumber(1).minus(slippage))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slippage for remove liquidity probably different to slippage for oSQTH/eth swap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah should probably be different variables, though for LP withdrawal slippage, it can probably not be surfaced for the user

Copy link
Contributor

@aleone aleone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small changes needed


const collateralToWithdraw = fromTokenAmount(withdrawAmount, WETH_DECIMALS)
const ethIndexPrice = toTokenAmount(index, WETH_DECIMALS).sqrt()
const vaultShortAmt = fromTokenAmount(vaultBefore.shortAmount, OSQUEETH_DECIMALS)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a repeat of shortAmount, we can delete

.minus(vaultCollateralAmt)
const flashLoanAmountPos = BigNumber.max(flashLoanAmount, 0)

const limitPrice = new BigNumber(limitEth)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, agreed


const limitPrice = new BigNumber(limitEth)
.div(amountToLiquidate.minus(amountToBurn).abs())
.times(new BigNumber(1).minus(slippage))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah should probably be different variables, though for LP withdrawal slippage, it can probably not be surfaced for the user

const amountToLiquidate = new BigNumber(wPowerPerpAmountInLP).times(liquidityPercentage)
const amountToBurn = shortAmount.times(burnPercentage)
const limitEth = await (amountToLiquidate.gt(amountToBurn)
? getExactIn(amountToLiquidate.minus(amountToBurn), true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right now, this doesn't take the pool fee in and do any autorouting, so we have to restrict this functionality to to be only the 0.3% pool

Copy link
Contributor

@aleone aleone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving to not block, but small changes to be made

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants