[chore] Move perpetuals/keeper helper functions to perpetuals/lib
#1678
Conversation
WalkthroughThe recent changes involve refactoring and modularizing code related to perpetual contract calculations. Key functions for net notional, margin requirements, and settlement calculations have been moved from the Changes
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
changed the title
BrendanChou
changed the title
perpetuals/keeper helper functions to perpetuals/lib
![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 (6)
- protocol/daemons/liquidation/client/sub_task_runner.go (3 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (5 hunks)
- protocol/x/perpetuals/keeper/perpetual_test.go (2 hunks)
- protocol/x/perpetuals/lib/lib.go (1 hunks)
- protocol/x/perpetuals/lib/lib_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/perpetuals/keeper/perpetual_test.go
Additional context used
golangci-lint
protocol/daemons/liquidation/client/sub_task_runner.go
28-28: undefined: Client (typecheck)
42-42: undefined: Client (typecheck)
108-108: undefined: Client (typecheck)
127-127: undefined: newContextWithQueryBlockHeight (typecheck)
protocol/x/perpetuals/keeper/perpetual.go
37-37: undefined: Keeper (typecheck)
49-49: undefined: Keeper (typecheck)
65-65: undefined: Keeper (typecheck)
Additional comments not posted (16)
protocol/x/perpetuals/lib/lib.go (2)
11-36: The functionGetSettlementPpmWithPerpetualis well-implemented with clear logic handling the settlement calculation based on the perpetual's funding index and the provided index. The use ofbig.Intfor high precision arithmetic is appropriate here. Good use of early return whenindexDeltais zero to optimize performance.
38-58: The functionGetNetNotionalInQuoteQuantumscorrectly converts base quantums to quote quantums using theBaseToQuoteQuantumsfunction. It handles the conversion based on market price and atomic resolution, which is crucial for financial calculations. The function is concise and maintains readability.protocol/x/perpetuals/lib/lib_test.go (6)
16-29: The benchmarkBenchmarkGetSettlementPpmWithPerpetualis correctly set up to measure the performance of theGetSettlementPpmWithPerpetualfunction. It uses a variety of inputs to simulate different scenarios.
31-96: The unit tests forGetSettlementPpmWithPerpetualcover a variety of cases including zero, positive, and negative index deltas. The use of table-driven tests enhances the readability and maintainability of the test code. Good use of therequire.Equalto assert expected outcomes.
98-116: The benchmarkBenchmarkGetNetNotionalInQuoteQuantumsis set up properly. It tests the function under a heavy load to ensure performance is maintained.
118-164: The tests forGetNetNotionalInQuoteQuantumsare comprehensive, covering scenarios with zero, positive, and negative quantums. These tests ensure that the function behaves correctly across different market conditions.
166-190: The benchmarkBenchmarkGetMarginRequirementsInQuoteQuantumsis well-constructed, testing the function with a large number of iterations to assess its performance under stress.
192-278: The tests forGetMarginRequirementsInQuoteQuantumsare thorough, covering various scenarios including different levels of open interest. The use of custom types and helper functions likebig_testutil.MustFirstis noted for ensuring the correctness of big integer operations.protocol/daemons/liquidation/client/sub_task_runner.go (1)
Line range hint
347-360: The integration ofperplibfunctionsGetNetNotionalInQuoteQuantumsandGetMarginRequirementsInQuoteQuantumsin the liquidation logic is correctly implemented. These changes are part of the refactoring to use the newly separated library functions. Ensure that all references to these functions are updated across the entire codebase.Verification successful
The search results confirm that the functions
GetNetNotionalInQuoteQuantumsandGetMarginRequirementsInQuoteQuantumsare correctly referenced in the codebase, including thesub_task_runner.gofile. The integration of these functions appears consistent with the new library usage.
protocol/x/perpetuals/keeper/perpetual.goprotocol/daemons/liquidation/client/sub_task_runner.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetNetNotionalInQuoteQuantums` and `GetMarginRequirementsInQuoteQuantums` match the new library usage. # Test: Search for the function usage. Expect: Only occurrences of the new library usage. rg --type python $'GetNetNotionalInQuoteQuantums|GetMarginRequirementsInQuoteQuantums'Length of output: 118
Script:
#!/bin/bash # Description: Verify all function calls to `GetNetNotionalInQuoteQuantums` and `GetMarginRequirementsInQuoteQuantums` match the new library usage. # Test: Search for the function usage. Expect: Only occurrences of the new library usage. rg --type go 'GetNetNotionalInQuoteQuantums|GetMarginRequirementsInQuoteQuantums'Length of output: 1779
protocol/x/subaccounts/keeper/subaccount.go (2)
25-25: The import ofperplibaligns with the PR's objective to use functions from the newperplibpackage. Ensure that all functions previously accessed throughperpkeeperare now correctly accessed throughperplib.
491-491: The replacement of the old function withperplib.GetSettlementPpmWithPerpetualis consistent with the PR's goals. Verify that this new function integrates seamlessly with the existing system and that all dependent functionalities are updated accordingly.protocol/x/perpetuals/keeper/perpetual.go (5)
31-31: Ensure the new import aliasperplibis consistently used across the file for accessing functions from theperplibpackage.
Line range hint
37-37: The static analysis tool has flagged theKeepertype as undefined. This is likely a false positive, as theKeepertype should be defined in another part of the codebase or an imported package. Verify that theKeepertype is correctly defined and accessible in this context.Also applies to: 49-49, 65-65
821-821: The functionGetNetNotionalInQuoteQuantumshas been correctly updated to use the new library. Ensure that all calls to this function are updated to handle any new parameters or changes in behavior.
926-926: The functionGetMarginRequirementsInQuoteQuantumshas been moved to the new library and is being correctly referenced here. Review all usages to ensure they are updated accordingly.Verification successful
The function
GetMarginRequirementsInQuoteQuantumshas been correctly referenced in the new library across the codebase.
protocol/x/perpetuals/lib/lib_test.goprotocol/x/perpetuals/lib/lib.goprotocol/x/perpetuals/keeper/perpetual.goprotocol/daemons/liquidation/client/sub_task_runner.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetMarginRequirementsInQuoteQuantums` match the new implementation. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type python $'GetMarginRequirementsInQuoteQuantums'Length of output: 86
Script:
#!/bin/bash # Description: Verify all function calls to `GetMarginRequirementsInQuoteQuantums` match the new implementation. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go 'GetMarginRequirementsInQuoteQuantums'Length of output: 903
961-961: The functionGetSettlementPpmWithPerpetualhas been moved to the new library. This change should be reflected wherever the function is called. Verify that the new function signature and behavior are correctly integrated.
| // GetMarginRequirementsInQuoteQuantums returns initial and maintenance margin requirements | ||
| // in quote quantums, given the position size in base quantums. | ||
| func GetMarginRequirementsInQuoteQuantums( | ||
| perpetual types.Perpetual, | ||
| marketPrice pricestypes.MarketPrice, | ||
| liquidityTier types.LiquidityTier, | ||
| bigQuantums *big.Int, | ||
| ) ( | ||
| bigInitialMarginQuoteQuantums *big.Int, | ||
| bigMaintenanceMarginQuoteQuantums *big.Int, | ||
| ) { | ||
| // Always consider the magnitude of the position regardless of whether it is long/short. | ||
| bigAbsQuantums := new(big.Int).Abs(bigQuantums) | ||
|
|
||
| // Calculate the notional value of the position in quote quantums. | ||
| bigQuoteQuantums := lib.BaseToQuoteQuantums( | ||
| bigAbsQuantums, | ||
| perpetual.Params.AtomicResolution, | ||
| marketPrice.Price, | ||
| marketPrice.Exponent, | ||
| ) | ||
| // Calculate the perpetual's open interest in quote quantums. | ||
| openInterestQuoteQuantums := lib.BaseToQuoteQuantums( | ||
| perpetual.OpenInterest.BigInt(), // OpenInterest is represented as base quantums. | ||
| perpetual.Params.AtomicResolution, | ||
| marketPrice.Price, | ||
| marketPrice.Exponent, | ||
| ) | ||
|
|
||
| // Initial margin requirement quote quantums = size in quote quantums * initial margin PPM. | ||
| bigBaseInitialMarginQuoteQuantums := liquidityTier.GetInitialMarginQuoteQuantums( | ||
| bigQuoteQuantums, | ||
| big.NewInt(0), // pass in 0 as open interest to get base IMR. | ||
| ) | ||
| // Maintenance margin requirement quote quantums = IM in quote quantums * maintenance fraction PPM. | ||
| bigMaintenanceMarginQuoteQuantums = lib.BigMulPpm( | ||
| bigBaseInitialMarginQuoteQuantums, | ||
| lib.BigU(liquidityTier.MaintenanceFractionPpm), | ||
| true, | ||
| ) | ||
|
|
||
| bigInitialMarginQuoteQuantums = liquidityTier.GetInitialMarginQuoteQuantums( | ||
| bigQuoteQuantums, | ||
| openInterestQuoteQuantums, // pass in current OI to get scaled IMR. | ||
| ) | ||
| return bigInitialMarginQuoteQuantums, bigMaintenanceMarginQuoteQuantums | ||
| } |
There was a problem hiding this comment.
The GetMarginRequirementsInQuoteQuantums function is comprehensive and handles both initial and maintenance margin calculations. It correctly considers the absolute value of quantums to ensure that the direction of the position does not affect the margin requirements. The use of helper functions like BaseToQuoteQuantums and BigMulPpm helps maintain modularity and reusability. However, consider adding more comments to explain the logic, especially around the calculations involving open interest caps.
Consider adding detailed comments to explain the logic behind the calculations, especially how the open interest affects the margin requirements.
|
@Mergifyio backport release/protocol/v5.1.x |
✅ Backports have been created
|
… (backport #1678) (#1791) Co-authored-by: Brendan Chou <[email protected]> Co-authored-by: Teddy Ding <[email protected]>
Changelist
Move
x/perpetuals/keeper/perpetual.gohelper functions tox/perpeptuals/libfor clarity. This is the first step in refactoring some of the margining functions for performance.Test Plan
Summary by CodeRabbit
Refactor
Tests
Style