Clean up open-interest liquidity tier calculations #1592
Conversation
WalkthroughThe updates primarily focus on refining margin requirements calculations and enhancing test coverage in the perpetuals module. Key changes include variable name updates, refactoring margin calculation logic, adding benchmarks, and improving subaccount management tests. Changes
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: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/perpetuals/types/liquidity_tier_test.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
protocol/x/perpetuals/types/liquidity_tier_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (1)
protocol/x/perpetuals/types/liquidity_tier_test.go (1)
222-242: The benchmark functionBenchmarkGetInitialMarginQuoteQuantumsis well-structured and effectively tests the performance ofGetInitialMarginQuoteQuantumsunder various open interest values.
| bigExpectedInitialMargin *big.Int | ||
| bigExpectedMaintenanceMargin *big.Int |
There was a problem hiding this comment.
these are not ppm values, they are in quantums, so I renamed them
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (5)
protocol/x/perpetuals/types/liquidity_tier.go (1)
Line range hint
89-140: Consider replacing the panic with a more graceful error handling mechanism to improve robustness.- panic(fmt.Sprintf("invalid open interest values for liquidity tier %d", liquidityTier.Id)) + return nil, fmt.Errorf("invalid open interest values for liquidity tier %d", liquidityTier.Id)Other than that, the implementation of
GetAdjustedInitialMarginPpmis robust and aligns with the PR objectives.protocol/x/perpetuals/keeper/perpetual_test.go (4)
Line range hint
10-100: Ensure proper cleanup and teardown in tests to prevent state leakage between tests.Consider adding
t.Cleanup()to ensure that any setup done in your tests is properly cleaned up afterwards. This is especially important in a testing environment where multiple tests are run in sequence and may share the same context or resources.
Line range hint
101-200: Consider parameterizing tests for better coverage and maintainability.Using table-driven tests can help make your tests more organized and easier to extend. For example, you can define a struct for test cases that includes inputs and the expected output, and then range over a slice of these test cases.
Line range hint
201-300: Enhance error messages for better debugging and user feedback.When checking for errors in your tests, consider including more descriptive messages that explain what the test was checking for. This can help quickly identify what went wrong without needing to dig through the test code.
Line range hint
401-500: Optimize data retrieval and sorting mechanisms for efficiency.Consider optimizing the retrieval and sorting of liquidity tiers, especially if the number of tiers can be large. Efficient sorting algorithms or database queries can significantly improve the performance of this functionality.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/perpetuals/keeper/perpetual_test.go (13 hunks)
- protocol/x/perpetuals/types/liquidity_tier.go (2 hunks)
- protocol/x/perpetuals/types/liquidity_tier_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/perpetuals/types/liquidity_tier_test.go
Additional Context Used
Path-based Instructions (2)
protocol/x/perpetuals/types/liquidity_tier.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/perpetuals/keeper/perpetual_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (2)
protocol/x/perpetuals/types/liquidity_tier.go (1)
76-87: LGTM! The refactoring ofGetInitialMarginQuoteQuantumsenhances clarity and testability by usingGetAdjustedInitialMarginPpm.protocol/x/perpetuals/keeper/perpetual_test.go (1)
Line range hint
301-400: Verify the use of mocks to ensure they are correctly simulating the external dependencies.Ensure that the mocks used in your tests accurately represent the behavior of the external dependencies they are meant to simulate. This is crucial for ensuring that your tests are valid and that they accurately reflect the behavior of your system under test.
| ), | ||
| ) | ||
| // If `open_interest` >= `open_interest_upper_cap` where `upper_cap` is non-zero, OIMF is 1. | ||
| if oiCapUpper.Sign() > 0 && oiQuoteQuantums.Cmp(oiCapUpper) >= 0 { |
There was a problem hiding this comment.
Nit: oiCapUpper.Sign() > 0 is an invariant here since we know oiCapUpper is non-negative (from uint64) and not equal to 0 from above.
There was a problem hiding this comment.
Yes I agree and was considering removing it, but I explicitly added it here in case code gets moved around later, or the check above gets removed or something
| @@ -219,6 +219,28 @@ func TestLiquidityTierGetMaxAbsFundingClampPpm(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func BenchmarkGetInitialMarginQuoteQuantums(b *testing.B) { | |||
There was a problem hiding this comment.
On darwin/arm64 for reference:
Before
BenchmarkGetInitialMarginQuoteQuantums-10 1322443 907.5 ns/op 832 B/op 44 allocs/op
After
BenchmarkGetInitialMarginQuoteQuantums-10 2147283 573.7 ns/op 600 B/op 28 allocs/op
Perhaps in the future we can add some workflow that compare these benchmarks on linux/amd64?
There was a problem hiding this comment.
There is a workflow to run on amd64 that runs in CI. I added it, but currently only runs on changes to protocol/lib/. I can change it to run on any changes to protocol/. I'm not very worried about negative impact on performance in this PR given that it's significantly better on arm64 but will be good to keep and eye on in the future
| // Total IMF. | ||
| capScalePpm := lib.BigU(lib.OneMillion - liquidityTier.InitialMarginPpm) | ||
| result := new(big.Int).Mul(capScalePpm, capNum) | ||
| result.Div(result, capDen) |
There was a problem hiding this comment.
This rounds towards zero right? Should we always round more conservatively (towards positive infinity) when calculating margin requirements? Think this is why (OIMF) match collateralized at base IMF and just failed collateralization at OIMF unit test is failing
There was a problem hiding this comment.
We should always round conservatively when doing the final calculation (imfPpm to quoteQuantums), but rounding conservatively in the case of increased ppm from OI doesn't seem entirely necessary to me.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/perpetuals/types/liquidity_tier.go (1)
Line range hint
89-140: TheGetAdjustedInitialMarginPpmfunction is well-implemented with comprehensive checks for different scenarios of open interest. Consider enhancing the comments to clarify the calculation steps for better maintainability.// - Define // - `Open Notional = Open Interest * Oracle Price` // - `Scaling Factor = (Open Notional - Lower Cap) / (Upper Cap - Lower Cap)` // - `IMF Increase = Scaling Factor * (1 - Base IMF)` // - Then a market’s `Effective IMF = Min(Base IMF + Max(IMF Increase, 0), 1.0)` + // These steps calculate the effective IMF based on the scaling factor and the base IMF, adjusting as the open interest moves between the defined caps.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/protocol-benchmark.yml (1 hunks)
- protocol/x/perpetuals/types/liquidity_tier.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (23 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/protocol-benchmark.yml
- protocol/x/subaccounts/keeper/subaccount_test.go
Additional Context Used
Path-based Instructions (1)
protocol/x/perpetuals/types/liquidity_tier.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (2)
protocol/x/perpetuals/types/liquidity_tier.go (2)
76-87: The implementation ofGetInitialMarginQuoteQuantumscorrectly calculates the initial margin requirement using the adjusted PPM value and rounds up the result, aligning with conservative financial practices.
Line range hint
9-34: TheValidatefunction effectively ensures that all properties of aLiquidityTierare within defined limits, using clear and specific error messages for each type of validation failure.
(cherry picked from commit df00d84) # Conflicts: # .github/workflows/protocol-benchmark.yml
…1805) Co-authored-by: Brendan Chou <[email protected]>
Changelist
Since IMF due to open-interest is calculated a lot, simplify the code paths, make them more testable, benchmark them, and improve performance by not using
big.Rat.Note, this is a (slightly) breaking change as the adjusted IMF is now calculated as a ppm before applying. This is generally fine to round the initial margin requirement to the nearest ppm value (as the base IMF is already in ppm). When multiplying the value by the notional value of the position, the margin requirement is already rounded up.
Test Plan
Added benchmarks and additional tests.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores