Max oi cap #1669

fritzschoff · 2023-06-19T14:37:05Z

this check needs to be implemented for every order type. Limit and market

codecov · 2023-06-19T14:39:33Z

Codecov Report

Merging #1669 (cc4a682) into main (b481ab2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1669   +/-   ##
=======================================
  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

0xjocke · 2023-06-20T01:19:49Z

markets/perps-market/contracts/storage/AsyncOrder.sol

+        uint256 sizeDeltaInUint256 = order.sizeDelta < 0
+            ? order.sizeDelta.mulDecimal(-1).toUint()
+            : order.sizeDelta.toUint();
+        if (marketConfig.maxMarketValue < perpsMarketData.size + sizeDeltaInUint256) {


I think we need some additional logic here..
The perpsMarketData.size is currently storing the absolute value of all shorts and long. I think maxMarketValue is suppose to be a cap on either side, at least that's how it works on perps v2.

So I think we need to track shortSize and longSize in PerpsMarket.sol. (I would also think it's clearer if we name them shortOI and longOI).
Maybe @sunnyvempati can shine in if this is suppose to work differently compared to preps v2.

After talking to sunny, We can figure it out by looking at size + skew. see https://github.com/Synthetixio/synthetix-v3/blob/main/markets/perps-market/contracts/storage/PerpsMarket.sol#L226

cool thanks! Will have a look!

into max-oi-cap

sunnyvempati

I'd recommend creating a new test file for testing max market size. Some scenarios we can test:

max market size is 100, current size is 50, someone creates new position for -100
max market size is 100, current size is 100, someone creates new position for -101 (fails)
one trader going from long to short, test both positive/negative cases
any others?

let's be as thorough as possible. pretty much every part of validatePositionSize should be tested with different order scenarios. pretty critical to get this right 👍🏽

sunnyvempati · 2023-06-23T04:07:44Z

markets/perps-market/contracts/storage/AsyncOrder.sol

+            PerpsMarket.validatePositionSize(
+                perpsMarketData,
+                marketConfig.maxMarketValue,
+                oldPosition.size,
+                order.sizeDelta
+            )


let the validatePositionSize just revert instead of it returning a bool.

that function needs to be refactored; but first would write the tests and make sure its working properly.

sunnyvempati · 2023-06-23T04:16:42Z

markets/perps-market/test/integration/Market/Size.test.ts

@@ -11,6 +11,7 @@ describe('Market - size test', () => {
        name: 'Ether',
        token: 'snxETH',
        price: bn(2000),
+        maxMarketValue: bn(9999999),


instead of needing to pass it in everywhere, in the bootstrapPerpsMarket, have a default value it can set it to when its not present 👍🏽

sunnyvempati · 2023-06-23T04:18:48Z

markets/perps-market/test/integration/Orders/OffchainAsyncOrder.commit.test.ts

+    it('reverts if max market value is reached', async () => {
+      await depositCollateral({
+        accountId: () => 2,
+        collaterals: [{ snxUSDAmount: () => bn(100_000) }],
+        systems,
+        trader: trader1,
+      });
+      await assertRevert(
+        systems()
+          .PerpsMarket.connect(trader1())
+          .commitOrder({
+            marketId: ethMarketId,
+            accountId: 2,
+            sizeDelta: bn(90_000),
+            settlementStrategyId: 0,
+            acceptablePrice: bn(1450), // 5% slippage
+            trackingCode: ethers.constants.HashZero,
+          }),
+        `MaxOpenInterestReached(${ethMarketId}, ${bn(5000).toString()})`
+      );
+    });


unfortunately, there are many other cases to be tested here. see review comment for other scenarios

fritzschoff · 2023-06-23T09:32:20Z

I'd recommend creating a new test file for testing max market size. Some scenarios we can test:

max market size is 100, current size is 50, someone creates new position for -100

max market size is 100, current size is 100, someone creates new position for -101 (fails)

one trader going from long to short, test both positive/negative cases
any others?

let's be as thorough as possible. pretty much every part of validatePositionSize should be tested with different order scenarios. pretty critical to get this right 👍🏽

on it. Found a TODO comment from you in the Size.test.ts file where you wrote // TODO: test maxMarketSize here as well

sunnyvempati

most of the comments are nonblocking, but lets remove the hardcoded value in bootstrapPerpsMarket

sunnyvempati · 2023-06-26T17:53:38Z

markets/perps-market/contracts/storage/PerpsMarket.sol

-        if (MathUtil.sameSide(oldSize, newSize) && MathUtil.abs(newSize) <= MathUtil.abs(oldSize)) {
-            return false;
+        if (
+            !(MathUtil.sameSide(oldSize, newSize) && MathUtil.abs(newSize) <= MathUtil.abs(oldSize))


maybe create a new variable for this condition?

you mean for readability ?

there is a comment above the if statement

sunnyvempati · 2023-06-26T17:55:19Z

markets/perps-market/test/integration/Market/Size.test.ts

+    describe('success', () => {
+      beforeEach(restore);
+      beforeEach('add collateral to margin', async () => {
+        await depositCollateral({


can call modifyCollateral directly if only depositing snxUSD, just fyi 👍🏽

sunnyvempati · 2023-06-26T18:01:55Z

markets/perps-market/test/integration/bootstrap/bootstrapPerpsMarkets.ts

+      before('set max market value', async () => {
+        await contracts.PerpsMarket.connect(marketOwner).setMaxMarketValue(
+          marketId,
+          bn(10_000_000)


dont hardcode this for every market. create an optional parameter that gets passed in here. if it doesn't exist then default to this value

sunnyvempati · 2023-06-28T15:12:37Z

markets/perps-market/test/integration/bootstrap/bootstrapPerpsMarkets.ts

+      before('set max market value', async () => {
+        await contracts.PerpsMarket.connect(marketOwner).setMaxMarketValue(
+          marketId,
+          maxMarketValue ? maxMarketValue : bn(10_000_000)


sunnyvempati

nice work on this!

fritzschoff added 2 commits June 19, 2023 15:20

added if statement

b8ff0d7

casting int to uint

2f6be99

fritzschoff requested a review from sunnyvempati June 19, 2023 14:37

fritzschoff added 2 commits June 19, 2023 15:43

added test

223b7f8

safe casting into uint

50309d2

0xjocke reviewed Jun 20, 2023

View reviewed changes

fritzschoff and others added 6 commits June 21, 2023 18:04

Merge branch 'main' into max-oi-cap

ecacae3

Merge branch 'main' into max-oi-cap

4929977

Merge branch 'max-oi-cap' of https://github.com/Synthetixio/synthetix-v3

e291b23

into max-oi-cap

fixing test

7273a63

removed comment

ce949b4

added max io test

989f968

sunnyvempati suggested changes Jun 23, 2023

View reviewed changes

fritzschoff added 6 commits June 23, 2023 14:25

working on tests

cb25f91

fixing tests

d52da02

removed resetMarket function

d435394

merged main

84639ea

removed only

422b7b4

moved check

250ec19

sunnyvempati suggested changes Jun 26, 2023

View reviewed changes

fritzschoff and others added 6 commits June 27, 2023 11:08

Merge branch 'main' into max-oi-cap

badf232

fixing default value

c596d8e

removed boolean casting

e9f7068

Merge branch 'main' into max-oi-cap

b634b9e

put bool into var

c57fec9

Merge branch 'main' into max-oi-cap

60c76c4

sunnyvempati reviewed Jun 28, 2023

View reviewed changes

sunnyvempati approved these changes Jun 28, 2023

View reviewed changes

fritzschoff added 2 commits June 28, 2023 17:22

fix build test

18ae444

Merge remote-tracking branch 'origin' into max-oi-cap

cc4a682

fritzschoff force-pushed the max-oi-cap branch from efe2e98 to cc4a682 Compare June 28, 2023 16:23

fritzschoff merged commit 106ac44 into main Jun 28, 2023
19 checks passed

fritzschoff deleted the max-oi-cap branch June 28, 2023 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Max oi cap #1669

Max oi cap #1669

fritzschoff commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading

0xjocke Jun 20, 2023

0xjocke Jun 20, 2023

fritzschoff Jun 20, 2023

sunnyvempati left a comment

sunnyvempati Jun 23, 2023

sunnyvempati Jun 23, 2023

sunnyvempati Jun 23, 2023 •

edited

Loading

fritzschoff commented Jun 23, 2023

sunnyvempati left a comment

sunnyvempati Jun 26, 2023

fritzschoff Jun 27, 2023

fritzschoff Jun 27, 2023

sunnyvempati Jun 26, 2023

sunnyvempati Jun 26, 2023

sunnyvempati Jun 28, 2023

sunnyvempati left a comment

Max oi cap #1669

Max oi cap #1669

Conversation

fritzschoff commented Jun 19, 2023 • edited Loading

codecov bot commented Jun 19, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnyvempati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnyvempati Jun 23, 2023 • edited Loading

Choose a reason for hiding this comment

fritzschoff commented Jun 23, 2023

sunnyvempati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnyvempati left a comment

Choose a reason for hiding this comment

fritzschoff commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 19, 2023 •

edited

Loading

sunnyvempati Jun 23, 2023 •

edited

Loading