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

Market debt #1700

Merged
merged 20 commits into from
Jul 31, 2023
Merged

Market debt #1700

merged 20 commits into from
Jul 31, 2023

Conversation

leomassazza
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #1700 (dbbfe29) into main (08ea86d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1700   +/-   ##
=======================================
  Coverage   72.77%   72.77%           
=======================================
  Files          57       57           
  Lines         720      720           
  Branches      236      236           
=======================================
  Hits          524      524           
  Misses        167      167           
  Partials       29       29           
Flag Coverage Δ
core-contracts 93.26% <ø> (ø)
core-modules 90.47% <ø> (ø)
core-utils 68.57% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@leomassazza leomassazza self-assigned this Jul 18, 2023
}

// TODO Should revert if totalDebt is positive and larger than totalCollateralValue???
return MathUtil.abs(totalCollateralValue.toInt() - totalDebt);
Copy link
Contributor

Choose a reason for hiding this comment

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

absolute value here is wrong. lets return 0 if totalDebt is larger than the total collateral value.

this is super edge case where liquidations aren't happening properly and the market continues accruing debt. regardless, if this does happen, the debt is just 0; the LPs could continue accruing value but there's no real way to pay that out since we already have all of trader's collateral.

Copy link
Contributor

Choose a reason for hiding this comment

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

also shouldn't it be totalCollateralValue + totalDebt?

example 1:
total collateral: $1000 
total markets debt: $500

reported Debt == $1500

example 2:
total collateral value = $1000
total market debt = -$800

reported debt = $200

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to double check together with the + or - on oldPositionPnl since it depends on how marketDebt is seen (if positive or negative)


self.debtCorrectionAccumulator +=
currentPrice.toInt() *
(newPosition.size - oldPositionSize) -
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this is + oldPositionPnl in the sheet? i could be wrong but just double check

@leomassazza leomassazza marked this pull request as ready for review July 24, 2023 19:53
Copy link
Contributor

@sunnyvempati sunnyvempati left a comment

Choose a reason for hiding this comment

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

some comments but lgtm!

@@ -36,7 +36,7 @@ contract LiquidationModule is ILiquidationModule {
.load()
.liquidatableAccounts;

for (uint i = 0; i < liquidatableAccounts.length(); i++) {
for (uint i = 1; i <= liquidatableAccounts.length(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

fixed this in my PR; we actually can't use liquidateAccounts directly because if there's a full account liquidation, it gets removed and we get an error. instead i move the liquidate accounts values into an array in memory which solves the issue 👍🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to keep this; and i'll fix merge conflict or vice versa depending on which PR goes in first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep it otherwise tests will fail, but will fix if your PR comes first

}
}

// TODO Should revert if perpsMarketId is not correct???
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is fine. we probably should never revert on reported debt since it could brick the core system

Comment on lines 152 to 157
uint currentPrice = newPosition.latestInteractionPrice;
(int oldPositionPnl, , , ) = oldPosition.getPnl(currentPrice);

self.debtCorrectionAccumulator +=
currentPrice.toInt().mulDecimal(newPosition.size - oldPositionSize) +
oldPositionPnl;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to do anything with accumulator on liquidiations? i dont think so since it becomes realized right? i'll think through this a bit more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

most likely yes. Maybe the numbers get balanced together, but not completely sure on some corner cases

Copy link
Contributor

@sunnyvempati sunnyvempati left a comment

Choose a reason for hiding this comment

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

hey, suggested some changes. verified on the google sheet, but this makes the most sense to me. let me know your thoughts

Comment on lines 152 to 157
uint currentPrice = newPosition.latestInteractionPrice;
(int oldPositionPnl, , , ) = oldPosition.getPnl(currentPrice);

self.debtCorrectionAccumulator +=
currentPrice.toInt().mulDecimal(newPosition.size - oldPositionSize) +
oldPositionPnl;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uint currentPrice = newPosition.latestInteractionPrice;
(int oldPositionPnl, , , ) = oldPosition.getPnl(currentPrice);
self.debtCorrectionAccumulator +=
currentPrice.toInt().mulDecimal(newPosition.size - oldPositionSize) +
oldPositionPnl;
uint currentPrice = newPosition.latestInteractionPrice;
(int oldPositionPnl, int accruedFunding, , ) = oldPosition.getPnl(currentPrice);
self.debtCorrectionAccumulator +=
currentPrice.toInt().mulDecimal(newPosition.size - oldPositionSize) +
oldPositionPnl - accruedFunding; // don't include accrued funding in this

Comment on lines 301 to 316
function marketDebt(Data storage self, uint price) internal view returns (int) {
// all positions sizes multiplied by the price is equivalent to skew times price
// and the debt correction accumulator is the sum of all positions pnl

return self.skew.mulDecimal(price.toInt()) - self.debtCorrectionAccumulator;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
function marketDebt(Data storage self, uint price) internal view returns (int) {
// all positions sizes multiplied by the price is equivalent to skew times price
// and the debt correction accumulator is the sum of all positions pnl
return self.skew.mulDecimal(price.toInt()) - self.debtCorrectionAccumulator;
}
function marketDebt(Data storage self, uint price) internal view returns (int) {
// all positions sizes multiplied by the price is equivalent to skew times price
// and the debt correction accumulator is the sum of all positions pnl
int traderUnrealizedPnl = self.skew.mulDecimal(price.toInt()) - self.debtCorrectionAccumulator;
int totalAccruedFunding = self.skew.mulDecimal(price.toInt()).mulDecimal(currentFundingRate();
return traderUnrealizedPnl + totalAccruedFunding;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

this will take care of unrealized funding. if we subtract it out of the accumulator then it would be counted double, so we only use trader pnl in the accumulator.

@sunnyvempati sunnyvempati self-assigned this Jul 31, 2023
@sunnyvempati sunnyvempati enabled auto-merge (squash) July 31, 2023 13:15
@sunnyvempati sunnyvempati merged commit 3d52128 into main Jul 31, 2023
18 of 19 checks passed
@sunnyvempati sunnyvempati deleted the market-debt branch July 31, 2023 13:17
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.

2 participants