[Performance] Remove the need to get each perpetual and price twice when checking collateralization #1681
Conversation
WalkthroughThis update involves significant refactoring related to the handling of margin requirements, net collateral, and related financial calculations in the Perpetuals module. Key changes include the removal of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClobKeeper as CLOB Keeper
participant SubaccountKeeper as Subaccount Keeper
participant PerpetualsKeeper as Perpetuals Keeper
participant Perplib as Perplib
Client->>ClobKeeper: Request Deleveraging
ClobKeeper->>PerpetualsKeeper: GetPerpetualAndMarketPrice
PerpetualsKeeper->>ClobKeeper: Return Perpetual, MarketPrice
ClobKeeper->>Perplib: Calculate DeleveragedQuoteQuantums
Perplib-->>ClobKeeper: Return Calculations
Client->>SubaccountKeeper: Query Net Collateral
SubaccountKeeper->>PerpetualsKeeper: GetPerpetualAndMarketPrice
PerpetualsKeeper->>SubaccountKeeper: Return Perpetual, MarketPrice
SubaccountKeeper->>Perplib: Calculate Net Collateral
Perplib-->>SubaccountKeeper: Return Calculations
SubaccountKeeper-->>Client: Return Net Collateral
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
BrendanChou
force-pushed
the
bc/perf/get-perpetuals-twice
branch
from
8aef11e to
4dce300
Compare
BrendanChou
changed the title
BrendanChou
force-pushed
the
bc/perf/get-perpetuals-twice
branch
from
4dce300 to
9065511
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (4)
protocol/mocks/PerpetualsKeeper.go (1)
Line range hint
21-21: Issues with_m.Calledandmock.AssertExpectationsmethods.It appears that methods
_m.Calledandmock.AssertExpectationsare undefined for thePerpetualsKeepertype, which likely indicates missing imports or incorrect mock setup. Please verify the mock setup and ensure that all necessary packages are correctly imported.Also applies to: 39-39, 67-67, 441-441
protocol/x/clob/keeper/deleveraging.go (1)
Line range hint
29-29: Undefined typeKeepercould indicate a missing import or declaration.Please ensure that
Keeperis correctly defined or imported in this file.protocol/x/subaccounts/keeper/subaccount.go (1)
959-974: Optimize the use ofGetPerpetualAndMarketPriceAndLiquidityTierby caching results.The function
GetPerpetualAndMarketPriceAndLiquidityTieris called within a loop, which could lead to performance issues if each call is expensive. Consider caching the results of this function, especially if the data does not change frequently or if the function is called with the same parameters multiple times during a single transaction.protocol/x/clob/keeper/liquidations.go (1)
Line range hint
683-683: The functiongetFillQuoteQuantumsis used but not defined or imported. This could lead to runtime errors.Please ensure that this function is properly defined or imported. If you need assistance implementing this function, feel free to ask.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/x/clob/keeper/deleveraging.go (3 hunks)
- protocol/x/clob/keeper/liquidations.go (3 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (2 hunks)
- protocol/x/perpetuals/keeper/perpetual_test.go (3 hunks)
- protocol/x/perpetuals/lib/lib.go (1 hunks)
- protocol/x/perpetuals/lib/lib_test.go (3 hunks)
- protocol/x/perpetuals/types/types.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (3 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/perpetuals/types/types.go
Additional context used
golangci-lint
protocol/mocks/PerpetualsKeeper.go
21-21: _m.Called undefined (type *PerpetualsKeeper has no field or method Called) (typecheck)
39-39: _m.Called undefined (type *PerpetualsKeeper has no field or method Called) (typecheck)
67-67: _m.Called undefined (type *PerpetualsKeeper has no field or method Called) (typecheck)
441-441: mock.AssertExpectations undefined (type *PerpetualsKeeper has no field or method AssertExpectations) (typecheck)
protocol/x/clob/keeper/deleveraging.go
29-29: undefined: Keeper (typecheck)
136-136: undefined: Keeper (typecheck)
155-155: undefined: Keeper (typecheck)
641-641: undefined: subaccountToDeleverage (typecheck)
678-678: undefined: subaccountToDeleverage (typecheck)
659-659: undefined: subaccountToDeleverage (typecheck)
protocol/x/clob/keeper/liquidations.go
36-36: undefined: Keeper (typecheck)
165-165: undefined: Keeper (typecheck)
198-198: undefined: Keeper (typecheck)
683-683: undefined: getFillQuoteQuantums (typecheck)
Additional comments not posted (17)
protocol/x/subaccounts/types/expected_keepers.go (2)
17-23: Introducing theIsPositionUpdatablemethod in theProductKeeperinterface.This addition aligns with the PR's goal of enhancing functionality around position updates, allowing for more granular control over position states.
75-83: AddedGetPerpetualAndMarketPriceAndLiquidityTiermethod to thePerpetualsKeeperinterface.This new method consolidates the retrieval of perpetual details, market price, and liquidity tier into a single call, which is a clear improvement in terms of reducing the number of state reads, aligning perfectly with the PR's objectives.
protocol/x/clob/types/expected_keepers.go (1)
108-114: AddedGetPerpetualAndMarketPriceAndLiquidityTiermethod in thePerpetualsKeeperinterface.This method is crucial for the centralization of data retrieval in the system, reducing the number of state reads and thus improving performance as per the PR's objectives.
protocol/x/perpetuals/lib/lib.go (1)
38-62: IntroducedGetNetCollateralAndMarginRequirementsfunction.This function centralizes the calculations for net collateral, initial margin, and maintenance margin requirements, which enhances code modularity and maintainability. It's a solid implementation that supports the PR's goal of performance optimization by reducing redundant calculations.
protocol/x/clob/keeper/deleveraging.go (4)
Line range hint
136-136: The typeKeeperis flagged again as undefined.This is a duplicate of the previous comment regarding the undefined
Keepertype.
Line range hint
155-155: The typeKeeperis undefined, which is a recurring issue in this file.This is a duplicate of the previous comment regarding the undefined
Keepertype.
595-600: The new functionGetPerpetualAndMarketPriceand its usage here are crucial for the performance improvements described in the PR. Ensure it is implemented correctly and integrates well with existing systems.This change aligns with the PR objectives to optimize performance by reducing state reads.
20-21: Ensure the new importperplibis utilized effectively in the file.Verification successful
The new import
perplibis effectively utilized in the file.
perplibis used in the line involving theGetNetNotionalInQuoteQuantumsfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the newly imported `perplib` library. # Test: Search for `perplib` usage. Expect: Multiple occurrences across the file. rg --type go $'perplib' protocol/x/clob/keeper/deleveraging.goLength of output: 200
protocol/x/perpetuals/lib/lib_test.go (1)
100-164: LGTM! Good coverage and use of assertions inTestGetNetCollateralAndMarginRequirements.protocol/x/subaccounts/keeper/subaccount.go (1)
959-974: Ensure the new functionGetPerpetualAndMarketPriceAndLiquidityTieris thoroughly tested.Verification successful
The search results reveal that the function
GetPerpetualAndMarketPriceAndLiquidityTieris tested for scenarios where the perpetual, market, or liquidity tier is not found. These test cases indicate that the function's error handling is being tested comprehensively.
TestGetPerpetualAndMarketPriceAndLiquidityTier_PerpetualNotFoundTestGetPerpetualAndMarketPriceAndLiquidityTier_MarketNotFoundTestGetPerpetualAndMarketPriceAndLiquidityTier_LiquidityTierNotFoundThese tests cover key aspects of the function's behavior, ensuring that it correctly handles cases where required data is missing.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify thorough testing of the new function. # Search for test cases for `GetPerpetualAndMarketPriceAndLiquidityTier`. rg --type go 'TestGetPerpetualAndMarketPriceAndLiquidityTier'Length of output: 461
Script:
#!/bin/bash # Description: Check the content of the test cases for thorough testing. # Display the content of the test cases for `GetPerpetualAndMarketPriceAndLiquidityTier`. rg --type go --context 10 'func TestGetPerpetualAndMarketPriceAndLiquidityTier_' protocol/x/perpetuals/keeper/perpetual_test.goLength of output: 1944
protocol/x/clob/keeper/liquidations.go (3)
19-19: Import ofperplibaligns with refactoring to centralize margin and collateral calculations.
459-481: The refactoring ofGetBankruptcyPriceInQuoteQuantumsto useGetPerpetualAndMarketPriceAndLiquidityTierandperplib.GetNetCollateralAndMarginRequirementseffectively reduces the number of state reads, aligning with the PR's performance goals.
554-567: The refactoring ofGetFillablePriceto useGetPerpetualAndMarketPriceAndLiquidityTierandperplib.GetNetCollateralAndMarginRequirementseffectively reduces the number of state reads, aligning with the PR's performance goals.protocol/x/perpetuals/keeper/perpetual.go (1)
1189-1213: Review the implementation ofGetPerpetualAndMarketPriceAndLiquidityTier.The new function
GetPerpetualAndMarketPriceAndLiquidityTierconsolidates the retrieval of perpetual, market price, and liquidity tier into a single function call. This should effectively reduce the number of state reads, aligning with the PR's goal of optimizing performance. Ensure that all calls to the oldGetPerpetualandGetMarketPricefunctions are replaced with calls to this new function to fully realize the performance benefits.protocol/x/perpetuals/keeper/perpetual_test.go (3)
677-681: Ensure error handling for non-existent perpetual ID is thoroughly tested.This test case correctly validates that the system handles non-existent perpetual IDs gracefully by returning an appropriate error. It's good practice to include such negative tests to ensure robust error handling.
Line range hint
717-733: Validate edge cases for liquidity tier not found scenarios.This test case effectively checks the system's response when a non-existent liquidity tier is referenced. Including such edge cases in tests ensures that the system can handle unexpected inputs gracefully.
Line range hint
688-714: Consider adding more assertions to validate the state after the operation.While the test checks for the error condition when the market ID does not exist, it might be beneficial to also assert that no changes were made to the state as a side effect of the failed operation. This helps ensure that the function is not only returning the correct error but also not altering any state unexpectedly.
| openInterestLowerCap: 25_000_000_000_000, | ||
| openInterestUpperCap: 50_000_000_000_000, | ||
| // openInterestNotional = 1_123_456_789 * 36_750 = 41_287_036_995_750 | ||
| // percentageOfCap = (openInterestNotional - lowerCap) / (upperCap - lowerCap) = 0.65148147983 | ||
| // adjustedIMF = (0.65148147983) * 0.8 + 0.2 = 0.721185183864 (rounded is 721_185 ppm) | ||
| // bigExpectedInitialMargin = bigBaseQuantums * price * adjustedIMF = 318_042_585 | ||
| bigExpectedInitialMargin: big.NewInt(318_042_585), | ||
| bigExpectedMaintenanceMargin: big.NewInt(88_200_000 / 2), | ||
| }, | ||
| } | ||
|
|
||
| // Run tests. | ||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| // Individual test setup. | ||
| pc := keepertest.PerpetualsKeepers(t) | ||
| // Create a new market param and price. | ||
| marketId := keepertest.GetNumMarkets(t, pc.Ctx, pc.PricesKeeper) | ||
| _, err := pc.PricesKeeper.CreateMarket( | ||
| pc.Ctx, | ||
| pricestypes.MarketParam{ | ||
| Id: marketId, | ||
| Pair: "marketName", | ||
| Exponent: tc.exponent, | ||
| MinExchanges: uint32(1), | ||
| MinPriceChangePpm: uint32(50), | ||
| ExchangeConfigJson: "{}", | ||
| }, | ||
| pricestypes.MarketPrice{ | ||
| Id: marketId, | ||
| Exponent: tc.exponent, | ||
| Price: 1_000, // leave this as a placeholder b/c we cannot set the price to 0 | ||
| }, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Update `Market.price`. By updating prices this way, we can simulate conditions where the oracle | ||
| // price may become 0. | ||
| err = pc.PricesKeeper.UpdateMarketPrices( | ||
| pc.Ctx, | ||
| []*pricestypes.MsgUpdateMarketPrices_MarketPrice{pricestypes.NewMarketPriceUpdate( | ||
| marketId, | ||
| tc.price, | ||
| )}, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create `LiquidityTier` struct. | ||
| _, err = pc.PerpetualsKeeper.SetLiquidityTier( | ||
| pc.Ctx, | ||
| 0, | ||
| "name", | ||
| tc.initialMarginPpm, | ||
| tc.maintenanceFractionPpm, | ||
| 1, // dummy impact notional value | ||
| tc.openInterestLowerCap, | ||
| tc.openInterestUpperCap, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create `Perpetual` struct with baseAssetAtomicResolution and marketId. | ||
| perpetual, err := pc.PerpetualsKeeper.CreatePerpetual( | ||
| pc.Ctx, | ||
| 0, // PerpetualId | ||
| "getMarginRequirementsTicker", // Ticker | ||
| marketId, // MarketId | ||
| tc.baseCurrencyAtomicResolution, // AtomicResolution | ||
| int32(0), // DefaultFundingPpm | ||
| 0, // LiquidityTier | ||
| types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS, // MarketType | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // If test case contains non-nil open interest, set it up. | ||
| if tc.openInterest != nil { | ||
| require.NoError(t, pc.PerpetualsKeeper.ModifyOpenInterest( | ||
| pc.Ctx, | ||
| perpetual.Params.Id, | ||
| tc.openInterest, // initialized as zero, so passing `openInterest` as delta amount. | ||
| )) | ||
| } | ||
|
|
||
| // Verify initial and maintenance margin requirements are calculated correctly. | ||
| perpetual, marketPrice, liquidityTier, err := pc.PerpetualsKeeper.GetPerpetualAndMarketPriceAndLiquidityTier( | ||
| pc.Ctx, | ||
| perpetual.Params.Id, | ||
| ) | ||
| require.NoError(t, err) | ||
| imr, mmr := lib.GetMarginRequirementsInQuoteQuantums( | ||
| perpetual, | ||
| marketPrice, | ||
| liquidityTier, | ||
| tc.bigBaseQuantums, | ||
| ) | ||
|
|
||
| require.Equal(t, tc.bigExpectedInitialMargin, imr, "Initial margin mismatch") | ||
| require.Equal(t, tc.bigExpectedMaintenanceMargin, mmr, "Maintenance margin mismatch") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider refactoring TestGetMarginRequirementsInQuoteQuantums_2 to enhance readability and maintainability. Splitting this large test into smaller, more focused tests could improve clarity and make individual scenarios easier to manage.
| // Iterate over all assets and updates and calculate change to net collateral and margin requirements. | ||
| for _, size := range assetSizes { | ||
| id := size.GetId() | ||
| bigQuantums := size.GetBigQuantums() | ||
|
|
||
| bigNetCollateralQuoteQuantums, err := pk.GetNetCollateral(ctx, id, bigQuantums) | ||
| nc, err := k.assetsKeeper.GetNetCollateral(ctx, id, bigQuantums) | ||
| if err != nil { | ||
| return err | ||
| return big.NewInt(0), big.NewInt(0), big.NewInt(0), err | ||
| } | ||
|
|
||
| bigNetCollateral.Add(bigNetCollateral, bigNetCollateralQuoteQuantums) | ||
|
|
||
| bigInitialMarginRequirements, | ||
| bigMaintenanceMarginRequirements, | ||
| err := pk.GetMarginRequirements( | ||
| imr, mmr, err := k.assetsKeeper.GetMarginRequirements( | ||
| ctx, | ||
| id, | ||
| bigQuantums, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| bigInitialMargin.Add(bigInitialMargin, bigInitialMarginRequirements) | ||
| bigMaintenanceMargin.Add(bigMaintenanceMargin, bigMaintenanceMarginRequirements) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // Iterate over all assets and updates and calculate change to net collateral and margin requirements. | ||
| for _, size := range assetSizes { | ||
| err := calculate(k.assetsKeeper, size) | ||
| if err != nil { | ||
| return big.NewInt(0), big.NewInt(0), big.NewInt(0), err | ||
| } | ||
| bigNetCollateral.Add(bigNetCollateral, nc) | ||
| bigInitialMargin.Add(bigInitialMargin, imr) | ||
| bigMaintenanceMargin.Add(bigMaintenanceMargin, mmr) |
There was a problem hiding this comment.
Consider consolidating repetitive code for margin and collateral calculations.
The loop iterating over assets to calculate net collateral and margin requirements is structurally similar to the one for perpetuals. It might be beneficial to abstract this logic into a helper function that can handle both assets and perpetuals, reducing code duplication and improving maintainability.
…hen checking collateralization (backport #1681) (#1794) Co-authored-by: Brendan Chou <[email protected]> Co-authored-by: Teddy Ding <[email protected]>
Changelist
Speed up collateralization checks by not getting the perpetual and price from state twice (once for net collateral, once for margin requirements). Instead, just get it a single time.
Breaking change as this reduces the number of state reads which modifies the gas used.
Test Plan
Updated and added unit tests
Blocked on
#1678
Summary by CodeRabbit
New Features
GetPerpetualAndMarketPriceAndLiquidityTierto retrieve perpetual details, market price, and liquidity tier.Refactor
perplibfunctions.Tests
Removals
GetMarginRequirementsfunction from several modules to centralize calculations using the newperpliblibrary.