[CLOB-1017] final settlement deleveraging step in PrepareCheckState #848
Conversation
…e price instead of bankruptcy price
|
Important Auto Review SkippedAuto reviews are disabled on base/target branches other than the default branch. Please add the base/target branch pattern to the list of additional branches to be reviewed in the settings. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the WalkthroughThe changes across various files in the protocol primarily involve the introduction and handling of final settlement for clob pairs. This includes updates to function signatures to accommodate a new 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 X ? TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…nt flag instead of calculating implicitly based off of clob pair status, and update indexer deleveraging event so we can send isFinalSettlement flag
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/indexer/events/deleveraging.go (1 hunks)
- protocol/indexer/events/deleveraging_test.go (2 hunks)
- protocol/x/clob/keeper/deleveraging.go (8 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (4 hunks)
- protocol/x/clob/keeper/process_operations.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/process_operations.go
Additional comments: 11
protocol/indexer/events/deleveraging.go (1)
- 14-29: The addition of the
isFinalSettlementparameter to theNewDeleveragingEventfunction is consistent with the PR objectives and the AI-generated summaries. This parameter will allow the function to handle events related to final settlement markets, which is a new feature being introduced.protocol/indexer/events/deleveraging_test.go (2)
3-10: The order of import statements has been altered. Ensure this change aligns with the project's import ordering conventions.
27-33: The addition of the
falseargument to theNewDeleveragingEventfunction call aligns with the PR's objective to handle subaccounts in final settlement markets. Confirm that this value is appropriate for the test scenario.protocol/x/clob/keeper/deleveraging.go (4)
35-52: The logic to allow deleveraging for markets in final settlement even for subaccounts with non-negative TNC is consistent with the PR objectives. This change ensures that subaccounts can be deleveraged in a specific scenario, which is an important aspect of the new feature being introduced.
272-308: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [238-305]
The updated logic in
OffsetSubaccountPerpetualPositionto handle final settlement deleveraging by fetching delta quote quantums at the oracle price for final settlement markets is consistent with the PR objectives. The addition of theisFinalSettlementflag to theNewDeleveragingEventfunction call ensures that the correct price is used for the deleveraging operation.
366-393: The changes to
getDeleveragingQuoteQuantumsDeltato return an additional boolean indicating if the deleveraging is for a final settlement market are crucial for the correct calculation of the quote quantums delta. This allows the system to use the oracle price for final settlement deleveraging, aligning with the PR objectives.535-577: The addition of
DeleverageSubaccountsInFinalSettlementMarketsis a significant change that directly addresses the PR's objective to deleverage subaccounts in final settlement markets. This function ensures that the deleveraging process is correctly applied to both negative and non-negative TNC subaccounts, using the appropriate price based on the market's status.protocol/x/clob/keeper/deleveraging_test.go (4)
711-717: The changes from lines 711 to 717 add a new parameter
falseto theNewDeleveragingEventfunction calls. This aligns with the PR objectives to handle new conditions for deleveraging subaccounts in final settlement markets. Ensure that thefalseparameter correctly represents the expected behavior in the context of these tests.1127-1133: The changes from lines 1127 to 1133 also add the
falseparameter to theNewDeleveragingEventfunction calls. Similar to the previous comment, verify that this parameter is correctly representing the expected behavior in the context of the test cases.1336-1342: The changes from lines 1336 to 1342 follow the same pattern as the previous comments, adding the
falseparameter to theNewDeleveragingEventfunction calls. Confirm that this parameter accurately reflects the intended test conditions.1496-1502: The changes from lines 1496 to 1502 again introduce the
falseparameter to theNewDeleveragingEventfunction calls within the test cases. As with the other instances, ensure that this parameter is appropriate for the test scenarios being executed.
| return 0, err | ||
| } | ||
|
|
||
| // Skip liquidation if the ClobPair for the liquidation order is in final settlement. There is a separate flow |
There was a problem hiding this comment.
We skip attempting to place the liquidation if the market is in final settlement, as liquidation matches are not allowed in closed markets. We do append the order to unfilledLiquidations so that the deleveraging flow may naturally pick up negative TNC subaccounts in the market and deleverage them as part of the standard deleveraging flow
There was a problem hiding this comment.
Why not pick another perpetual ID to liquidate?
What if the subaccount is liquidatable but has multiple positions? Closing their open but final settled position at the oracle price won't change their collateralization, so IMO could make sense to just liquidate another position
However I could see us leaning towards the simplest approach and just waiting another block since I think it picks a random position to liquidate. Also I guess this logic is necessary to trigger the deleveraging flow
Wdyt?
There was a problem hiding this comment.
yeah admittedly was going for simplicity here. Picking a new position to liquidate would mean introducing a new flow here where we selectively re-generate liquidation orders depending on clob pair status and I just don't think that is worth it for now. If you feel strongly, we could implement this later but for now I think this is fine
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/liquidations.go (8 hunks)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
Additional comments: 8
protocol/x/clob/keeper/liquidations.go (6)
24-30: The changes to the function signature and the introduction of
numDeleveragingAttemptsare consistent with the PR objectives to track the number of deleveraging attempts within a block.35-39: The early return when there are no subaccounts to liquidate is a good practice for performance optimization.
68-74: Proper error handling is implemented for the case where a subaccount is not liquidatable, and unexpected errors are correctly returned, halting the process.
106-123: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [94-117]
The introduction of
clobPairStatusesto track the status ofClobPairand skip liquidation for pairs in final settlement status aligns with the PR objectives to handle subaccounts in final settlement markets.
- 127-133: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [123-130]
The error handling and logging for failed liquidation attempts are correctly implemented, ensuring that issues are logged and the function returns early in case of an error.
- 158-175: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [145-171]
The loop correctly implements the logic to attempt deleveraging for unfilled liquidations, respecting the
MaxDeleveragingAttemptsPerBlocklimit, which is consistent with the PR objectives.protocol/x/clob/keeper/process_operations.go (2)
678-692: The changes from lines 678 to 692 handle the calculation of
deltaQuoteQuantumsdifferently depending on whether the match is a final settlement. This is consistent with the PR objectives and ensures that the correct calculation method is used based on the match type.715-732: The changes from lines 715 to 732 correctly emit an on-chain update for the deleveraging event. The use of
TransientStoreensures that the events are rolled back if the branched state is discarded, which is a good practice for blockchain state management.
| @@ -499,23 +529,50 @@ func (k Keeper) ProcessDeleveraging( | |||
| ), | |||
| ) | |||
|
|
|||
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |||
| // if the branched state is discarded, so batching is not necessary. | |||
| k.GetIndexerEventManager().AddTxnEvent( | |||
There was a problem hiding this comment.
Pulled this onchain event out of ProcessDeleveraging so that we can easily set the isFinalSettlement flag on the event. This information is not known to the ProcessDeleveraging function.
This is similar to how ProcessSingleMatch works, the caller of the function is responsible for emitting the event instead of the event being in ProcessSingleMatch. This allows us to treat these "Process" functions solely as state-updating functions. Their callers are responsible for generating events.
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/abci.go (2 hunks)
- protocol/x/clob/keeper/process_operations.go (3 hunks)
- protocol/x/clob/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/process_operations.go
Additional comments: 3
protocol/x/clob/abci.go (2)
14-17: The addition of the
satypesimport aligns with the changes made in thePrepareCheckStatefunction.199-211: The changes to
PrepareCheckStatefunction correctly implement the new step for deleveraging subaccounts with open positions in final settlement markets, as described in the PR objectives and the summary.protocol/x/clob/types/errors.go (1)
- 311-314: The addition of
ErrInvalidFinalSettlementDeleveragingMatchwith code 1021 correctly implements the new error handling logic as described in the PR objectives and the AI-generated summaries.
…/final-settlement-pcs
| @@ -25,6 +26,15 @@ func (server *Server) WithDaemonLiquidationInfo( | |||
| return server | |||
| } | |||
|
|
|||
| // SetSubaccountOpenPositions stores the list of subaccount open positions in a go-routine safe map. | |||
| // Placeholder to allow for testing. | |||
| func (s *Server) SetSubaccountOpenPositions( | |||
There was a problem hiding this comment.
@jayy04 I've added this as a placeholder so I can modify this datastructure for testing. I imagine there will be a new function created for this in one of your PRs
| subaccountsToDeleverage = append(subaccountsToDeleverage, subaccountToDeleverage{ | ||
| SubaccountId: liquidationOrder.GetSubaccountId(), | ||
| PerpetualId: liquidationOrder.MustGetLiquidatedPerpetualId(), | ||
| }) |
There was a problem hiding this comment.
we don't need to append here, right? since this is going to get picked up by GetSubaccountsWithOpenPositionsInFinalSettlementMarkets
There was a problem hiding this comment.
my motivation in still appending here is that this negative TNC deleveraging attempt will occur earlier if I append it here rather than wait for the other function to pick it up. It's more important that we deleverage negative TNC subaccounts than it is that we do the final settlement deleveraging for non-negative TNC subaccounts
| subaccountIds := daemonLiquidationInfo.GetLiquidatableSubaccountIds() | ||
| if err := keeper.LiquidateSubaccountsAgainstOrderbook(ctx, subaccountIds); err != nil { | ||
| liquidatableSubaccountIds := daemonLiquidationInfo.GetLiquidatableSubaccountIds() | ||
| subaccountsToDeleverage, err := keeper.LiquidateSubaccountsAgainstOrderbook(ctx, liquidatableSubaccountIds) |
There was a problem hiding this comment.
thinking about this more - subaccountsToDeleverage here should mostly contain subaccounts with unfilled liquidations and need to be deleveraged at bankruptcy price (-ve TNC), right?
we do have DaemonLiquidationInfo.NegativeTncSubaccounts, maybe we can just use that? this will de-couple step 6&7 completely. wdyt?
There was a problem hiding this comment.
yes it does contain -ve TNC subaccounts, paired with a perpetual id that we tried to liquidate with. I think it probably makes sense to use DaemonLiquidationInfo.NegativeTncSubaccounts as you suggested. Unsure if we want to deleverage using the same perpetual we tried to liquidate with though, if so we would need to add some extra logic for that
There was a problem hiding this comment.
yeah maybe we can do this in a separate PR. no need to block on this
There was a problem hiding this comment.
maybe create a ticket and add a TODO?
| // Can deleverage subaccount if net collateral is negative or if the market is in final settlement. | ||
| canDeleverageSubaccount = bigNetCollateral.Sign() == -1 || clobPair.Status == types.ClobPair_STATUS_FINAL_SETTLEMENT |
There was a problem hiding this comment.
nit: maybe move this to above k.GetClobPairIdForPerpetual. this save us 1 state read if TNC < 0.
also curious to get your thoughts on making these two bools mutually exclusive. something like shouldDeleverageAtBankruptcyPrice and shouldDeleverageAtOraclePrice. I think it improves code readability slightly
There was a problem hiding this comment.
hmm, not a bad idea
jakob-dydx
changed the title
) * add missing indexer constants * update proto formatting * fix indexer test * set up ground work for allowing only deleveraging events for final settlement markets * test trading is blocked in process proposer operations for final settlement * update comments * add DeliverTx tests for process operations final settlement deleveraging operations * pr nits * nit
…/final-settlement-pcs
jakob-dydx
merged commit 5dd3bae
into
jakob-dydx/final-settlement-endblocker
…#829) * add protos for final settlement clob pair status and final settlement removal reason * implement cancellation of open stateful orders for final settlement clob pairs upon acceptance of gov proposal * format * pr nits * pr nits * [CLOB-1010] final settlement checktx validation (#842) * checktx logic to reject order placements for final settlement markets * fix test with incorrect order * [CLOB-1017] final settlement deleveraging step in PrepareCheckState (#848) * add functionality in ProcessDeleveraging to allow settlement at oracle price instead of bankruptcy price * pull deltaQuoteQuantums out of ProcessDeleveraging * fix lint * fix issue with test * pr nits * update getDeleveragingQuoteQuantumsDelta helper * initial implementation of final settlement deleveraging * add delivertx logic for deleveraging match * update to have DeliverTx calculate price based off of IsFinalSettlement flag instead of calculating implicitly based off of clob pair status, and update indexer deleveraging event so we can send isFinalSettlement flag * update delivertx validation * allow final settlement subaccounts to be deleveraged by regular deleveraging flow * remove superfluous comment * update err name * fix import formatting * lint * begin adding tests * update logic so that isFinalSettlement flag on operation is used * re-use helper function * pr nits, redefine CanDeleverageSubaccount * update tests for CanDeleverageSubaccount * split up PCS steps 6 and 7 to be for liquidations and then for deleveraging * update to use new subaccountOpenPositionInfo type * update tests * pr nits * lint * formatting * nits and tests * more nits and comment updates * [CLOB-1021] final settlement DeliverTx validation to block trading (#834) * add missing indexer constants * update proto formatting * fix indexer test * set up ground work for allowing only deleveraging events for final settlement markets * test trading is blocked in process proposer operations for final settlement * update comments * add DeliverTx tests for process operations final settlement deleveraging operations * pr nits * nit * merge from upstream * format * update test to verify transitions from a status to itself is accepted * fix defer statement with latency metric * pr nits
This PR adds a new step in PrepareCheckState to deleverage subaccounts with open positions in final settlement markets. It does this by leveraging the new field from the liquidations daemon which contains a mapping of perpetual -> position side -> subaccount id which allows us to know which subaccounts have open positions in specific markets. We re-use the
MaxDeleveragingAttemptsPerBlockclob flag to limit the number of attempts. The regular liquidations/deleveraging flow now returns a total number of deleveraging attempts so we don't go over the limit.Note: this PR needs to be updated once the liquidations daemon actually outputs this information. Right now a placeholder variable of the expected type has been used and initialized to be empty.