-
Notifications
You must be signed in to change notification settings - Fork 194
lukas/isolated positions sdk #1832
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
base: crispheaney/isolated-position
Are you sure you want to change the base?
lukas/isolated positions sdk #1832
Conversation
sdk/src/marginCalculation.ts
Outdated
@@ -0,0 +1,351 @@ | |||
import { BN } from './'; |
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.
might move this to margin file
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.
you are saying... don't import BN? Or like import directly from anchor?
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.
oh I see you are saying just keep this in the margin.ts file don't make a separate marginCalculation.ts file?
sdk/src/user.ts
Outdated
enterHighLeverageMode = false, | ||
perpMarketIndex?: number | ||
): BN { | ||
const totalCollateral = this.getTotalCollateral(marginCategory, true); |
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 also needs to be using the margin type?
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.
As in you think maybe it doesn't need to be using that?
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.
like we don't need to pass perpMarketIndex ? 'Isolated' : undefined,
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.
I think in line with your other comment, we would just keep perpMarketIndex and only use that, not add a marginType parameter which would be redundant.
sdk/src/user.ts
Outdated
public getHealth(): number { | ||
if (this.isBeingLiquidated()) { | ||
public getHealth(perpMarketIndex?: number): number { | ||
if (this.isBeingLiquidated() && !perpMarketIndex) { |
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.
why do you have this line?
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.
we should also discuss if we want to change the isBeingLiquidated implementation
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.
I added that because if we want just the health for an isolated position, then the whole account being liq'd shouldn't affect that. BUT if we change the logic of isBeingLIquidated maybe we would remove this then.
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.
leaving this right now since we need to discuss more specifically what we want to do
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.
We discussed and I've implemented something based on that.
sdk/src/user.ts
Outdated
return { | ||
perpLiabilityValue: perpLiability, | ||
perpPnl: positionUnrealizedPnl, | ||
spotAssetValue: perpPosition.isolatedPositionScaledBalance, |
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.
need to convert this to token 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.
added this. I think you want it converted to token value though right? Like multiplied by the price?
sdk/src/user.ts
Outdated
} | ||
|
||
public canBeLiquidated(): { | ||
public canBeLiquidated(perpMarketIndex?: number): { |
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.
I think this fn should just return all the markets that can be liquidated instead of the caller needing to pass in a perp market index param
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.
Would the return be like this?
{
canBeLiquidated: boolean;
marginRequirement: BN;
totalCollateral: BN;
isolatedPositions: Array<{
canBeLiquidated: boolean;
marginRequirement: BN;
totalCollateral: BN}>;
}>;
}
9933deb
to
89272cc
Compare
4e2952c
to
a9224f6
Compare
Uh oh!
There was an error while loading. Please reload this page.