[CLOB-1028] allow settlement at oracle price in ProcessDeleveraging #835
Conversation
…e price instead of bankruptcy price
WalkthroughThe updates involve significant changes to the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/keeper/deleveraging.go (5 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (3 hunks)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
Additional comments: 7
protocol/x/clob/keeper/deleveraging.go (4)
3-14: The reorganization of import statements is a standard cleanup and does not impact functionality.
242-245: The addition of the
fillPriceIsOraclePriceboolean parameter to theOffsetSubaccountPerpetualPositionfunction aligns with the PR objectives to support settlement at oracle price.357-385: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [325-384]
The logic modification in the
ProcessDeleveragingfunction to calculatefillPriceDeltaQuoteQuantumsbased on thefillPriceIsOraclePriceflag is consistent with the PR objectives to alter the settlement process.
- 477-483: The code correctly emits a cometbft event and an on-chain update for the deleveraging, which is standard for recording state changes in blockchain applications.
protocol/x/clob/keeper/deleveraging_test.go (3)
1096-1100: The
ProcessDeleveragingfunction has been updated to include a new boolean parameterfillPriceIsOraclePriceas per the PR objective. This parameter is hardcoded tofalsein the test case, which aligns with the PR objective that the default behavior is not to use the oracle price unless specified otherwise.1124-1304: The new test function
TestProcessDeleveragingAtOraclePricehas been added to test the functionality of theProcessDeleveragingfunction when thefillPriceIsOraclePriceflag is set totrue. This aligns with the PR objective to support final settlement fills at the oracle price.1458-1462: The
ProcessDeleveragingfunction is called with thefillPriceIsOraclePriceparameter set tofalsein theTestProcessDeleveraging_Roundingtest case. This is consistent with the default behavior described in the PR objective, where the oracle price is not used unless specified otherwise.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/keeper/deleveraging.go (5 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (8 hunks)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/deleveraging.go
Additional comments: 5
protocol/x/clob/keeper/deleveraging_test.go (4)
- 642-683: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [625-678]
The changes from lines 625 to 678 show the creation of perpetuals and ClobPairs for testing purposes. The loop correctly handles the creation of perpetuals and the addition of events for each ClobPair creation. The use of
mockIndexerEventManager.Onto set up expectations for the mock event manager is correct and follows good testing practices.
1103-1157: The changes from lines 1103 to 1157 show the setup and execution of the
ProcessDeleveragingfunction within the test cases. The setup includes setting subaccounts and calculating the bankruptcy price. The test cases are well-structured and cover various scenarios, including error handling. The use ofrequire.NoErrorandrequire.ErrorContainsfor assertions is appropriate.1162-1277: The changes from lines 1162 to 1277 introduce the
TestProcessDeleveragingAtOraclePricefunction, which tests the deleveraging process using oracle prices. The test cases are comprehensive and cover various scenarios, including well-collateralized and under-collateralized subaccounts. The setup for each test case is thorough, and the error handling checks withrequire.ErrorContainsare correctly implemented.1470-1484: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1470-1507]
The changes from lines 1470 to 1507 show the setup and execution of the
TestProcessDeleveraging_Roundingfunction within the test cases. The setup includes setting subaccounts and calculating the bankruptcy price with rounding considerations. The test cases are well-structured and cover scenarios where rounding could affect the outcome of deleveraging. The use ofrequire.NoErrorandrequire.ErrorContainsfor assertions is appropriate.protocol/x/clob/keeper/process_operations.go (1)
- 651-691: The changes from lines 651 to 691 introduce a new variable
deltaQuoteQuantumsand renamedeltaQuantumstodeltaBaseQuantums, which is consistent with the PR objectives and the summary provided. The logic for determining the sign ofdeltaBaseQuantumsand the error handling within the loop for processing deleveraging fills appear to be correct. Ensure that thegetDeleveragingQuoteQuantumsDeltaandProcessDeleveragingfunctions are implemented correctly and handle the new logic as intended.
| deltaQuoteQuantums, err := k.getDeleveragingQuoteQuantumsDelta( | ||
| ctx, | ||
| perpetualId, | ||
| liquidatedSubaccountId, | ||
| deltaQuantums, | ||
| ) |
There was a problem hiding this comment.
errors returned from bankruptcy price calculation were previously caught within ProcessDeleveraging and recovered from in OffsetSubaccountPerpetualPosition, continuing onto the next iteration without breaking. I am opting to continue this pattern, but will log a debug msg
| deltaBaseQuantums.Neg(deltaBaseQuantums) | ||
| } | ||
|
|
||
| deltaQuoteQuantums, err := k.getDeleveragingQuoteQuantumsDelta( |
There was a problem hiding this comment.
this is where we call ProcessDeleveraging in DeliverTx, we call the helper function to get the delta quote quantums to pass in
| subaccountId satypes.SubaccountId, | ||
| deltaQuantums *big.Int, | ||
| ) (deltaQuoteQuantums *big.Int, err error) { | ||
| clobPair := k.mustGetClobPairForPerpetualId(ctx, perpetualId) |
There was a problem hiding this comment.
Question: is it okay if we panic here? This logic is executed in PrepareCheckState. Would we rather recover from this and continue? I think it is fine to panic but just want to double check.
There was a problem hiding this comment.
yeah should be fine to panic - if clob pair is missing, then something is really really wrong
There was a problem hiding this comment.
Just realized this function isn't doing exactly what I want for usability. The purpose of this function is to allow the deleveraging flow to be reusable such that non-negative TNC subaccounts with positions in a final settlement market can be closed at the oracle price.
Currently this function is written such that ALL subaccounts (including negative TNC) for the closed market will be settled at oracle price. This isn't what we want. I think the easiest way to updated this would be to do a collateral check and use the oracle price iff isFinalSettlement && nonNegativeTNC.
I think what I'll do here is use CanDeleverageSubaccount to determine if the account has non negative TNC and only call this if the market is in final settlement.
| deltaQuoteQuantums, err = k.perpetualsKeeper.GetNetNotional(ctx, perpetualId, deltaQuantums) | ||
| } else { | ||
| deltaQuoteQuantums, err = k.GetBankruptcyPriceInQuoteQuantums( |
There was a problem hiding this comment.
nit: we can just return k.perpetualsKeeper.GetNetNotional or return k.GetBankruptcyPriceInQuoteQuantums. up to you
There was a problem hiding this comment.
true, will update
| ) | ||
| if err != nil { | ||
| liquidatedSubaccount := k.subaccountsKeeper.GetSubaccount(ctx, liquidatedSubaccountId) | ||
| k.Logger(ctx).Debug( |
There was a problem hiding this comment.
should this be Debug or Error?
There was a problem hiding this comment.
will change to Error
| "Encountered error when getting quote quantums for deleveraging", | ||
| "error", err, | ||
| "blockHeight", ctx.BlockHeight(), | ||
| "checkTx", ctx.IsCheckTx(), |
There was a problem hiding this comment.
nit: k.Logger() already has this, so we can remove this here
| @@ -1095,6 +1136,215 @@ func TestProcessDeleveraging(t *testing.T) { | |||
| *tc.offsettingSubaccount.GetId(), | |||
| uint32(0), | |||
| tc.deltaQuantums, | |||
| bankruptcyPriceQuoteQuantums, | |||
There was a problem hiding this comment.
for my understanding - the bankruptcyPriceQuoteQuantums will be 0 if tc.expectedErr != nil, is that okay?
bankruptcy price should always exist, should we pull that out of the if statement above? similar to what you have below for quote quantums using oracle price
There was a problem hiding this comment.
yeah I needed to push this into the if statement though, because the GetBankruptcyPriceInQuoteQuantums returns an error for my test cases where tc.expectedErr != nil. So the test code was erroring out before executing the ProcessDeleveraging logic.
I am intentionally skipping this calculation for error cases.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/deleveraging.go (5 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (7 hunks)
Additional comments: 8
protocol/x/clob/keeper/deleveraging.go (4)
236-258: The error handling within
OffsetSubaccountPerpetualPositionwhen fetchingdeltaQuoteQuantumsfromgetDeleveragingQuoteQuantumsDeltalogs an error but does not halt the process or propagate the error. This behavior is consistent with the previous pattern mentioned byjakob-dydxin the existing comments, where errors in price calculation were caught and logged without breaking the loop. Ensure that this is the intended behavior and that the system can safely continue without this data.328-354: The
getDeleveragingQuoteQuantumsDeltafunction correctly determines the type of deleveraging event and returns the appropriate quote quantums delta. This is a key part of the new feature to use oracle prices for settlement. The function uses theclobPairstatus to decide whether to use the bankruptcy price or the oracle price, which aligns with the PR objectives.387-401: The validation logic in
ProcessDeleveragingensures thatdeltaBaseQuantumsis on the opposite side of the liquidated position and the same side as the offsetting position, and that its magnitude is not larger than both positions. This is crucial for maintaining the integrity of the deleveraging process. Ensure that this validation covers all necessary cases and that the error message provides sufficient context for debugging.500-508: The
ProcessDeleveragingfunction emits a cometbft event and sends an on-chain update for the deleveraging, which is necessary for the indexer to pick up and process the event. This is consistent with the system's design for handling state changes and notifying external components.protocol/x/clob/keeper/deleveraging_test.go (4)
2-15: The import modifications are minor and do not affect functionality.
642-684: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [625-680]
The addition of loops to create
perpsandclobPairsis a significant change that should be reflected in the summary, as it likely affects the setup of the test environment.
1164-1350: The addition of the
TestProcessDeleveragingAtOraclePricefunction with detailed test cases is significant and should be reflected in the summary to highlight the complexity and nature of the tests.1472-1486: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1472-1509]
The addition of the
TestProcessDeleveraging_Roundingfunction with detailed test cases is significant and should be reflected in the summary to highlight the complexity and nature of the tests.
deltaQuoteQuantumsNo tests for OffsetSubaccountPerpetualPosition have been included for final settlement clob pairs yet, because the final settlement status is still not allowed on
main. Tests will be added for this in a separate PR for when the status is enabled. Holding off on enabling the status to properly gate these changes