[CT-988] Include position quote balance in nc and mmr calculations #1869
Conversation
WalkthroughThis update introduces enhancements to the risk calculation process, improvements to perpetual positions handling in the ClOB (Central Limit Order Book), and adjustments to subaccount risk assessments. Key changes include renaming and refactoring functions, adding new test cases, and integrating additional logic for handling quote balances in perpetual positions. Changes
Sequence Diagram(s)Here's a high-level sequence diagram illustrating the changes to the sequenceDiagram
participant Keeper
participant ClOBModule
participant PerpLib
Keeper->>+ClOBModule: Call GetBankruptcyPriceInQuoteQuantums
ClOBModule->>+PerpLib: Call GetPositionNetNotionalValueAndMarginRequirements for riskPosOld
PerpLib-->>-ClOBModule: Return riskPosOld
ClOBModule->>+PerpLib: Call GetPositionNetNotionalValueAndMarginRequirements for riskPosNew
PerpLib-->>-ClOBModule: Return riskPosNew
ClOBModule-->>-Keeper: Return Bankruptcy Price in Quote Quantums
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/clob/keeper/liquidations.go (2 hunks)
- protocol/x/perpetuals/lib/lib.go (2 hunks)
- protocol/x/perpetuals/lib/lib_test.go (2 hunks)
- protocol/x/subaccounts/lib/updates.go (1 hunks)
- protocol/x/subaccounts/types/position_size.go (1 hunks)
Additional comments not posted (7)
protocol/x/perpetuals/lib/lib.go (2)
Line range hint
39-65:
LGTM!The renaming of the method aligns with its functionality. The implementation is correct.
67-86: LGTM!The method correctly calls
GetPositionNetNotionalValueAndMarginRequirementsand includes additional calculations for the quote balance.protocol/x/subaccounts/lib/updates.go (1)
366-367: LGTM!The method correctly includes
pos.GetQuoteBalance()for risk calculation, aligning with the PR objectives.protocol/x/perpetuals/lib/lib_test.go (1)
Line range hint
162-220:
LGTM!The test cases comprehensively cover various scenarios with different
quoteBalancevalues.protocol/x/clob/keeper/liquidations.go (3)
454-454: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetPositionNetNotionalValueAndMarginRequirementsmatch the new usage.Verification successful
Verification Successful!
The function
GetPositionNetNotionalValueAndMarginRequirementsis used correctly across the codebase.
protocol/x/perpetuals/lib/lib.goprotocol/x/clob/keeper/liquidations.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetPositionNetNotionalValueAndMarginRequirements` match the new usage. # Test: Search for the function usage. Expect: Only occurrences of the new function name. rg --type go $'GetPositionNetNotionalValueAndMarginRequirements'Length of output: 716
Script:
#!/bin/bash # Fetch lines around each occurrence of `GetPositionNetNotionalValueAndMarginRequirements` for detailed inspection. # Fetch lines in `protocol/x/perpetuals/lib/lib.go` rg --type go --context 5 'GetPositionNetNotionalValueAndMarginRequirements' protocol/x/perpetuals/lib/lib.go # Fetch lines in `protocol/x/clob/keeper/liquidations.go` rg --type go --context 5 'GetPositionNetNotionalValueAndMarginRequirements' protocol/x/clob/keeper/liquidations.goLength of output: 1660
544-544: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetPositionNetNotionalValueAndMarginRequirementsmatch the new usage.Verification successful
Verified: The function
GetPositionNetNotionalValueAndMarginRequirementsis used consistently and correctly across the codebase.
protocol/x/perpetuals/lib/lib.go: Definition and usage verified.protocol/x/clob/keeper/liquidations.go: Usage verified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetPositionNetNotionalValueAndMarginRequirements` match the new usage. # Test: Search for the function usage. Expect: Only occurrences of the new function name. rg --type go $'GetPositionNetNotionalValueAndMarginRequirements'Length of output: 716
Script:
#!/bin/bash # Description: Fetch the context around each occurrence of `GetPositionNetNotionalValueAndMarginRequirements` for verification. # Fetch context around the definition and usage in lib.go rg --type go --context 5 'GetPositionNetNotionalValueAndMarginRequirements' protocol/x/perpetuals/lib/lib.go # Fetch context around the usage in liquidations.go rg --type go --context 5 'GetPositionNetNotionalValueAndMarginRequirements' protocol/x/clob/keeper/liquidations.goLength of output: 1660
460-460: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetPositionNetNotionalValueAndMarginRequirementsmatch the new usage.Verification successful
Verified: Function usage is consistent
All function calls to
GetPositionNetNotionalValueAndMarginRequirementsinprotocol/x/clob/keeper/liquidations.gomatch the expected parameters as defined inprotocol/x/perpetuals/lib/lib.go.
protocol/x/clob/keeper/liquidations.go: Lines 460-460protocol/x/clob/keeper/liquidations.go: Other occurrencesScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetPositionNetNotionalValueAndMarginRequirements` match the new usage. # Test: Search for the function usage. Expect: Only occurrences of the new function name. rg --type go $'GetPositionNetNotionalValueAndMarginRequirements'Length of output: 716
Script:
#!/bin/bash # Extract the function definition from protocol/x/perpetuals/lib/lib.go ast-grep --lang go --pattern $'func GetPositionNetNotionalValueAndMarginRequirements($_, $_) { $$$ }' | head -n 20 # Extract the three occurrences in protocol/x/clob/keeper/liquidations.go rg -A 5 'GetPositionNetNotionalValueAndMarginRequirements' protocol/x/clob/keeper/liquidations.goLength of output: 638
Script:
#!/bin/bash # Extract the function definition from protocol/x/perpetuals/lib/lib.go ast-grep --lang go --pattern $'func GetPositionNetNotionalValueAndMarginRequirements($_, $_) { $$$ }'Length of output: 104
Script:
#!/bin/bash # Search for the function definition in protocol/x/perpetuals/lib/lib.go rg 'func GetPositionNetNotionalValueAndMarginRequirements' protocol/x/perpetuals/lib/lib.go -A 10Length of output: 369
| // Get the perpetual position quote balance in big.Int. | ||
| func (m *PerpetualPosition) GetQuoteBalance() *big.Int { | ||
| if m == nil { | ||
| return new(big.Int) | ||
| } | ||
|
|
||
| return m.QuoteBalance.BigInt() | ||
| } |
There was a problem hiding this comment.
Consider returning nil in case of a nil receiver.
Returning nil instead of a new big.Int can better indicate the absence of a PerpetualPosition.
- if m == nil {
- return new(big.Int)
+ if m == nil {
+ return nilCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the perpetual position quote balance in big.Int. | |
| func (m *PerpetualPosition) GetQuoteBalance() *big.Int { | |
| if m == nil { | |
| return new(big.Int) | |
| } | |
| return m.QuoteBalance.BigInt() | |
| } | |
| // Get the perpetual position quote balance in big.Int. | |
| func (m *PerpetualPosition) GetQuoteBalance() *big.Int { | |
| if m == nil { | |
| return nil | |
| } | |
| return m.QuoteBalance.BigInt() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/lib/updates.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/subaccounts/lib/updates.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/subaccounts/types/position_size.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/types/position_size.go
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/indexer/events/subaccount_update.go (2 hunks)
Additional comments not posted (2)
protocol/indexer/events/subaccount_update.go (2)
4-9: Imports look good.The new imports for
math/big,assettypes, andsalibare necessary for the added functionalities.
28-35: LGTM! Ensure integration with the rest of the codebase.The updated logic in
NewSubaccountUpdateEventcorrectly usesAddQuoteBalanceFromPerpetualPositionsfor updating asset positions.Please verify that this change integrates correctly with the rest of the codebase and that all relevant parts of the application are updated accordingly.
| func AddQuoteBalanceFromPerpetualPositions( | ||
| perpetualPositions []*satypes.PerpetualPosition, | ||
| assetPositions []*satypes.AssetPosition, | ||
| ) []*satypes.AssetPosition { | ||
| quoteBalance := new(big.Int) | ||
| for _, position := range perpetualPositions { | ||
| quoteBalance.Add(quoteBalance, position.GetQuoteBalance()) | ||
| } | ||
|
|
||
| if quoteBalance.Sign() == 0 { | ||
| return assetPositions | ||
| } | ||
|
|
||
| // Add the quote balance to asset positions. | ||
| return salib.CalculateUpdatedAssetPositions( | ||
| assetPositions, | ||
| []satypes.AssetUpdate{ | ||
| { | ||
| AssetId: assettypes.AssetUsdc.Id, | ||
| BigQuantumsDelta: quoteBalance, | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Consider simplifying the logic for handling quote balances.
The function AddQuoteBalanceFromPerpetualPositions can be simplified by directly returning the updated asset positions without checking if quoteBalance is zero.
func AddQuoteBalanceFromPerpetualPositions(
perpetualPositions []*satypes.PerpetualPosition,
assetPositions []*satypes.AssetPosition,
) []*satypes.AssetPosition {
quoteBalance := new(big.Int)
for _, position := range perpetualPositions {
quoteBalance.Add(quoteBalance, position.GetQuoteBalance())
}
// Simplify by directly returning updated asset positions
return salib.CalculateUpdatedAssetPositions(
assetPositions,
[]satypes.AssetUpdate{
{
AssetId: assettypes.AssetUsdc.Id,
BigQuantumsDelta: quoteBalance,
},
},
)
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func AddQuoteBalanceFromPerpetualPositions( | |
| perpetualPositions []*satypes.PerpetualPosition, | |
| assetPositions []*satypes.AssetPosition, | |
| ) []*satypes.AssetPosition { | |
| quoteBalance := new(big.Int) | |
| for _, position := range perpetualPositions { | |
| quoteBalance.Add(quoteBalance, position.GetQuoteBalance()) | |
| } | |
| if quoteBalance.Sign() == 0 { | |
| return assetPositions | |
| } | |
| // Add the quote balance to asset positions. | |
| return salib.CalculateUpdatedAssetPositions( | |
| assetPositions, | |
| []satypes.AssetUpdate{ | |
| { | |
| AssetId: assettypes.AssetUsdc.Id, | |
| BigQuantumsDelta: quoteBalance, | |
| }, | |
| }, | |
| ) | |
| func AddQuoteBalanceFromPerpetualPositions( | |
| perpetualPositions []*satypes.PerpetualPosition, | |
| assetPositions []*satypes.AssetPosition, | |
| ) []*satypes.AssetPosition { | |
| quoteBalance := new(big.Int) | |
| for _, position := range perpetualPositions { | |
| quoteBalance.Add(quoteBalance, position.GetQuoteBalance()) | |
| } | |
| // Simplify by directly returning updated asset positions | |
| return salib.CalculateUpdatedAssetPositions( | |
| assetPositions, | |
| []satypes.AssetUpdate{ | |
| { | |
| AssetId: assettypes.AssetUsdc.Id, | |
| BigQuantumsDelta: quoteBalance, | |
| }, | |
| }, | |
| ) | |
| } |
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit