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

Add collateral you redeem #428

Closed
wants to merge 25 commits into from
Closed

Conversation

deepthinker250
Copy link
Contributor

@deepthinker250 deepthinker250 commented May 5, 2022

Task:

This PR involves refactoring the existing logic of ShortClose so needs a detailed review.

Description

Add collateral Eth input and refactored dependency logic
image

Fixes #370

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Document update

How Has This Been Tested

Please describe how to test to verify the changes. Provide instructions so we can reproduce.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Added video recordings if it is a UI change

@vercel
Copy link

vercel bot commented May 5, 2022

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

Name Status Preview Updated
continuouscall ✅ Ready (Inspect) Visit Preview May 23, 2022 at 2:31PM (UTC)

@KMKoushik
Copy link
Contributor

@adam5909099 Can you explain what has been done clearly? PRs like this might have to go through @alexisgauba and user story should be documented!

@deepthinker250
Copy link
Contributor Author

@alexisgauba, Can you please review this PR for allowing users to enter collateral? I kept the Spend card for now because I'm not sure how to label the two values contained in the Card.

@alexisgauba
Copy link
Contributor

Yep, Adam and I chatted about this earlier! Main thing is giving users the ability to input the amount of ETH they want to remove from a vault when they are partially closing, some advanced users would just use amount to buy back + burn & collRatio to guess and check amount of ETH to remove, when what they wanted to input was a certain amount of ETH to get out and then see how that affects their position

Adding the field for Collateral to redeem the way you did LGTM visually

One good thing to add would be the liquidation price of the vault after the change. The key things advanced users were looking at were

  • How much ETH can I remove from the vault
  • What liquidation price will that leave me at

Imo could just add "New liquidation price" as a row right above "Current collateral ratio"

From a logic pov, seems like the value is a bit different than on the current deployment? Seems like something might be going on there?
Screen Shot 2022-05-06 at 2 59 00 PM
Screen Shot 2022-05-06 at 2 59 12 PM

@KMKoushik
Copy link
Contributor

@adam5909099 Can you take a look at this?

@deepthinker250
Copy link
Contributor Author

deepthinker250 commented May 9, 2022

@alexisgauba For different collateral value between this PR and production isn't an issue. It's related to the time when the website is loaded.

image
2.
image

The above screenshots are from production when closing two oSQTH in short. They show different values because they were loaded at other times. Maybe we can implement polling for all values to get the latest date in real-time, but it might cause performance issues. So I don't think a slight value difference is an issue.

CC @KMKoushik

@alexisgauba
Copy link
Contributor

alexisgauba commented May 9, 2022

Ok gotcha, then in that case I think the main think would just be adding "New liquidation price" as a row right above "Current collateral ratio" @adam5909099

@deepthinker250
Copy link
Contributor Author

@alexisgauba, I got it, thank you. I added a new liquidation price, but I don't think it's calculated correctly. Can you or @KMKoushik provide me the formula for calculating it in a short close or a notion doc that I can reference?

@deepthinker250
Copy link
Contributor Author

@KMKoushik I updated PR as you mentioned. When I set 0.2869 collateral eth, the new liquidation price was 4866.64, like in the following screenshot.
image
After partially closing short (it took about 15 seconds) and going to the manage vault page, the liquidation price was shown as 4866.63
image
There's a 0.01 difference, but I think it's related to underlying value changes while closing short and not an issue.

Copy link
Contributor

@KMKoushik KMKoushik left a comment

Choose a reason for hiding this comment

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

Can we stop having -ve value here? We can stop slider when it reaches 0
Screen Shot 2022-05-13 at 1 44 57 pm

@deepthinker250
Copy link
Contributor Author

@KMKoushik I add a restriction to the slider to ensure eth is bigger than 0. Can you please review it again?

@deepthinker250
Copy link
Contributor Author

@KMKoushik
I updated PR to meet Idea B requirement @alexisgauba wrote in https://www.notion.so/opynopyn/Partial-Close-Doc-44615fd7758647549cb995729817b470
Can you please review if all values are implemented correctly?
image
image

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