[feat][protocol]: gate withdrawals if negative TNC subaccount encountered after liquidation and deleveraging steps #936
Conversation
WalkthroughThe recent updates involve two main changes. Firstly, the error return values have been eliminated from several functions, simplifying the interface and possibly the error handling process. Secondly, the system has introduced a zero-fill deleveraging mechanism. This feature adds new checks and controls to manage withdrawals, ensuring that deleveraging operations can be executed without leaving any negative balances, which is crucial for maintaining the platform's financial integrity. 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 (
|
lucas-dydx
force-pushed
the
lucas-dydx/protocol-pcs-insert-zero-fill-delev
branch
from
0e1da1f
to
7759ace
Compare
| } | ||
|
|
||
| if !foundNegativeTncSubaccount { | ||
| return nil |
There was a problem hiding this comment.
Planning to add more metrics here.
- Add counters for total calls to this method.
- Add counters for halted withdrawals (can add alerts on this).
- Add latency metrics to this method.
lucas-dydx
changed the title
| // Note the first TX is MsgProposeOperations, the second is all other TXs. | ||
| execResult := response.TxResults[1] | ||
| require.True(t, execResult.IsErr()) | ||
| require.Equal(t, satypes.ErrFailedToUpdateSubaccounts.ABCICode(), execResult.Code) |
There was a problem hiding this comment.
Planning to add the following additional tests:
- Transfers and withdrawals become unblocked after
WITHDRAWAL_AND_TRANSFERS_BLOCKED_AFTER_NEGATIVE_TNC_SUBACCOUNT_SEEN_BLOCKSblocks - Both transfers and withdrawals fail
- Transfers and withdrawals are not blocked if subaccounts specified in negative TNC subaccounts have non-negative TNC after liquidations and deleveraging steps
![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: 13
Configuration used: CodeRabbit UI
Files selected for processing (9)
- protocol/mocks/MemClob.go (17 hunks)
- protocol/mocks/MemClobKeeper.go (13 hunks)
- protocol/x/clob/abci.go (1 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
- protocol/x/clob/keeper/deleveraging.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
- protocol/x/clob/types/internal_operation.go (1 hunks)
- protocol/x/clob/types/memclob.go (1 hunks)
- protocol/x/clob/types/operations_to_propose.go (1 hunks)
Additional comments: 22
protocol/x/clob/types/memclob.go (1)
- 140-144: The addition of the
InsertZeroFillDeleveragingIntoOperationsQueuemethod to theMemClobinterface is consistent with the interface's design and the method signature follows Go's conventions.protocol/x/clob/types/internal_operation.go (1)
- 107-118: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [26-26]
The removal of the panic condition in
NewMatchPerpetualDeleveragingInternalOperationfor zero maker fills seems to align with the introduction of zero-fill deleveraging operations. Ensure this change is intentional and does not introduce any unintended side effects.Verification successful
The review of the
operations_to_propose.gofile confirms that theNewMatchPerpetualDeleveragingInternalOperationfunction is used correctly in the context of zero-fill deleveraging operations, as intended by the PR. The logic surrounding its usage is consistent with the system's design to differentiate between zero-fill and regular deleveraging operations. The removal of the panic condition for zero maker fills is intentional and does not introduce any unintended side effects based on the current usage within the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the removal of the panic condition is handled correctly in all usages of the function. ast-grep --lang go --pattern $'NewMatchPerpetualDeleveragingInternalOperation($_, $_, $$$, $_)'Length of output: 96
Script:
#!/bin/bash # Attempt to find usages of the function with a modified pattern that accounts for any number of parameters. ast-grep --lang go --pattern $'NewMatchPerpetualDeleveragingInternalOperation($$$)'Length of output: 998
Script:
#!/bin/bash # Review the implementation of the NewMatchPerpetualDeleveragingInternalOperation function and the surrounding logic. cat ./protocol/x/clob/types/operations_to_propose.goLength of output: 16408
protocol/x/clob/abci.go (1)
- 235-240: The addition of logic to gate withdrawals by inserting a zero-fill deleveraging operation into the operations queue if any negative TNC subaccounts are detected is a significant change for the system's financial integrity. Ensure this logic is correctly implemented and thoroughly tested.
protocol/mocks/MemClobKeeper.go (9)
38-38: The changes in the
AddOrderToOrderbookCollatCheckfunction seem to move the declaration ofr1outside of the conditional block. This change could potentially alter the behavior of the mock, depending on how theCalledmethod is implemented. It's important to ensure that the mock's behavior remains consistent with the actualMemClobKeeperinterface it's mocking.52-71: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [61-77]
In the
AddPreexistingStatefulOrderfunction, the return variablesr0,r1,r2, andr3are declared outside of the conditional blocks. This change appears to be a refactor to simplify the code. Ensure that the refactor does not change the intended behavior of the mock, especially in cases where theCalledmethod might return different types or fewer values than expected.
98-105: The
CanDeleverageSubaccountfunction has been refactored similarly to the previous functions, with return variablesr0,r1, andr2declared outside of the conditional blocks. Verify that the changes do not affect the mock's behavior, particularly in error handling and the return of boolean values.156-156: The
GetLongTermOrderPlacementfunction's changes follow the same pattern as the previous functions. Ensure that the changes do not introduce any unintended side effects, especially since boolean return values can be critical in control flow within tests.177-184: The
GetOrderFillAmountfunction's refactor also needs to be verified for consistency with the expected behavior. The mock should accurately reflect the behavior of the actual implementation it represents.221-221: The
IsLiquidatablefunction's refactor includes moving the error return variable outside of the conditional block. Verify that this does not affect error handling in tests that use this mock.265-265: In the
OffsetSubaccountPerpetualPositionfunction, the return variables are declared outside of the conditional blocks. This is a significant change since it involves a slice and a pointer tobig.Int. Ensure that the mock continues to return the correct types and values as expected in various test scenarios.279-305: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [288-311]
The
ProcessSingleMatchfunction has multiple return variables, and their declarations have been moved outside of the conditional blocks. This function seems to be more complex than the others, so it's crucial to verify that the changes do not introduce any regressions or behavior changes in the mock.
- 323-342: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [332-348]
The
ReplayPlaceOrderfunction's refactor is similar to the others, with return variables declared outside of the conditional blocks. Verify that the mock's behavior is consistent with the actual implementation, especially in terms of error handling and the return of complex types like*clobtypes.OffchainUpdates.protocol/mocks/MemClob.go (1)
- 285-288: The new function
InsertZeroFillDeleveragingIntoOperationsQueuehas been added. Ensure that this function is correctly implemented and that it matches the interface it's mocking.protocol/x/clob/types/operations_to_propose.go (2)
- 239-254: The new function
AddZeroFillDeleveragingToOperationsQueuehas been added without any panic conditions, unlike other functions in this file which have various checks that cause a panic if conditions are not met. It's important to ensure that this is intentional and that no checks are needed here for consistency and safety.Verification successful
The output from the
rgcommand indicates that theAddZeroFillDeleveragingToOperationsQueuefunction is the only function with anAddprefix in theOperationsToProposetype within theoperations_to_propose.gofile. This suggests that there are no other similar functions in this file that we can compare with regarding the presence or absence of panic conditions or checks.Given this information, we cannot determine if the lack of panic conditions in the
AddZeroFillDeleveragingToOperationsQueuefunction is consistent with similar functions in the same file, as there are no similar functions to compare with. The absence of otherAddfunctions means that the review comment pointing out the lack of panic conditions cannot be confirmed as an inconsistency within the context of this specific file.* 236-258: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-238]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if there are any other instances where similar functions do not perform checks. rg "func \(o \*OperationsToPropose\) Add" protocol/x/clob/types/operations_to_propose.goLength of output: 162
The existing functions in the file have several panic conditions to ensure data integrity. It's crucial to maintain consistency across the file for error handling. The new function should likely follow the same pattern unless there's a specific reason not to.
Verification successful
The use of
panicfor error handling is consistent throughout theoperations_to_propose.gofile, as indicated by the multiple instances found in the output. This pattern is applied in various functions to ensure data integrity and handle exceptional cases that should not occur during normal operation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the panic pattern is consistent across similar functions. rg "panic" protocol/x/clob/types/operations_to_propose.goLength of output: 1266
protocol/x/clob/memclob/memclob.go (7)
218-230: The new method
InsertZeroFillDeleveragingIntoOperationsQueuehas been added to theMemClobPriceTimePrioritystruct. This method is responsible for inserting a zero-fill deleveraging operation into the operations queue, which is used to indicate that a subaccount has a negative TNC and withdrawals should be disabled. The method signature and logic appear to be correct and align with the PR objectives.231-231: The comment for the method
mustUpdateMemclobStateWithMatcheshas been updated to clarify its functionality. It's important that comments accurately reflect the purpose and behavior of the code they describe.231-231: The method
mustPerformTakerOrderMatchingis complex and critical for the trading protocol's operation. It appears to handle the matching of taker orders against maker orders correctly, including various checks and updates to the state. However, due to its complexity, it is recommended to ensure thorough testing, particularly for edge cases and error conditions, to ensure the robustness of the matching logic.231-231: The method
GetPricePremiumperforms a financial calculation that is critical to the protocol. It is important to ensure that the logic for calculating the price premium is correct and that all edge cases are handled properly. Given the financial implications, it is recommended to have this method reviewed by a domain expert in financial calculations to confirm its accuracy and robustness.231-231: The method
GetMidPricecalculates the mid-price of an order book. It is important to ensure that the calculation correctly handles scenarios where the best bid or ask may not be present and that the method returns accurate and expected results. Given the importance of this metric in trading systems, it is recommended to verify the correctness of the calculation through unit tests.231-231: Methods like
getImpactPriceSubticksandGetPricePremiumFromSideperform price calculations that are essential to the protocol's operation. It is crucial to ensure that these calculations are correct, handle edge cases, and avoid potential division by zero errors. It is recommended to verify the correctness of these methods through comprehensive unit tests and possibly peer review by domain experts.215-234: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [1-231]
The file
memclob.gois extensive and complex, containing critical logic for the trading protocol's operation. Due to the complexity and the financial implications of the code, it is imperative to ensure thorough testing, including unit tests, integration tests, and end-to-end tests, to validate the correctness of the logic. Additionally, it is recommended to involve domain experts in the review process to confirm the accuracy of financial calculations and the robustness of the trading mechanisms.
| // GateWithdrawalsIfNegativeTncSubaccountSeen gates withdrawals if a negative TNC subaccount exists. | ||
| // It does this by inserting a zero-fill deleveraging operation into the operations queue iff any of | ||
| // the provided negative TNC subaccounts are still negative TNC. | ||
| func (k Keeper) GateWithdrawalsIfNegativeTncSubaccountSeen( | ||
| ctx sdk.Context, | ||
| negativeTncSubaccountIds []satypes.SubaccountId, | ||
| ) (err error) { | ||
| foundNegativeTncSubaccount := false | ||
| var negativeTncSubaccountId satypes.SubaccountId | ||
| for _, subaccountId := range negativeTncSubaccountIds { | ||
| bigNetCollateral, | ||
| _, | ||
| _, | ||
| err := k.subaccountsKeeper.GetNetCollateralAndMarginRequirements( | ||
| ctx, | ||
| satypes.Update{SubaccountId: subaccountId}, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // If the subaccount has negative TNC, mark that a negative TNC subaccount was found. | ||
| if bigNetCollateral.Sign() == -1 { | ||
| foundNegativeTncSubaccount = true | ||
| negativeTncSubaccountId = subaccountId | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !foundNegativeTncSubaccount { | ||
| return nil | ||
| } | ||
|
|
||
| // A negative TNC subaccount was found, therefore insert a zero-fill deleveraging operation into | ||
| // the operations queue to indicate withdrawals should be gated. | ||
| subaccount := k.subaccountsKeeper.GetSubaccount(ctx, negativeTncSubaccountId) | ||
| perpetualPositions := subaccount.GetPerpetualPositions() | ||
| if len(perpetualPositions) == 0 { | ||
| return errorsmod.Wrapf( | ||
| types.ErrNoPerpetualPositionsToLiquidate, | ||
| "GateWithdrawalsIfNegativeTncSubaccountSeen: subaccount has no open positions: (%s)", | ||
| lib.MaybeGetJsonString(subaccount), | ||
| ) | ||
| } | ||
| perpetualId := subaccount.PerpetualPositions[0].PerpetualId | ||
| k.MemClob.InsertZeroFillDeleveragingIntoOperationsQueue(ctx, negativeTncSubaccountId, perpetualId) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The new function GateWithdrawalsIfNegativeTncSubaccountSeen correctly implements the logic to gate withdrawals by inserting a zero-fill deleveraging operation into the operations queue if a negative TNC subaccount is found. However, there are a few points to consider:
-
Error Handling: The function returns immediately if an error is encountered while fetching the net collateral and margin requirements. This is good practice as it prevents further execution with potentially invalid state.
-
Loop Break: The loop breaks as soon as a negative TNC subaccount is found, which is efficient.
-
Metrics and Logging: There are no metrics or logging statements within this function. Considering the previous comments on adding metrics, it would be beneficial to include metrics and logging here to monitor the frequency and context in which withdrawals are gated.
-
Error Messages: The error message in line 229 is clear and provides enough information for debugging.
-
Function Naming: The function name is descriptive and clearly indicates its purpose.
Overall, the function appears to be logically sound and aligns with the PR objectives. However, adding metrics and logging would enhance observability.
+ // TODO: Consider adding metrics and logging to monitor the gating of withdrawals.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.
| // GateWithdrawalsIfNegativeTncSubaccountSeen gates withdrawals if a negative TNC subaccount exists. | |
| // It does this by inserting a zero-fill deleveraging operation into the operations queue iff any of | |
| // the provided negative TNC subaccounts are still negative TNC. | |
| func (k Keeper) GateWithdrawalsIfNegativeTncSubaccountSeen( | |
| ctx sdk.Context, | |
| negativeTncSubaccountIds []satypes.SubaccountId, | |
| ) (err error) { | |
| foundNegativeTncSubaccount := false | |
| var negativeTncSubaccountId satypes.SubaccountId | |
| for _, subaccountId := range negativeTncSubaccountIds { | |
| bigNetCollateral, | |
| _, | |
| _, | |
| err := k.subaccountsKeeper.GetNetCollateralAndMarginRequirements( | |
| ctx, | |
| satypes.Update{SubaccountId: subaccountId}, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| // If the subaccount has negative TNC, mark that a negative TNC subaccount was found. | |
| if bigNetCollateral.Sign() == -1 { | |
| foundNegativeTncSubaccount = true | |
| negativeTncSubaccountId = subaccountId | |
| break | |
| } | |
| } | |
| if !foundNegativeTncSubaccount { | |
| return nil | |
| } | |
| // A negative TNC subaccount was found, therefore insert a zero-fill deleveraging operation into | |
| // the operations queue to indicate withdrawals should be gated. | |
| subaccount := k.subaccountsKeeper.GetSubaccount(ctx, negativeTncSubaccountId) | |
| perpetualPositions := subaccount.GetPerpetualPositions() | |
| if len(perpetualPositions) == 0 { | |
| return errorsmod.Wrapf( | |
| types.ErrNoPerpetualPositionsToLiquidate, | |
| "GateWithdrawalsIfNegativeTncSubaccountSeen: subaccount has no open positions: (%s)", | |
| lib.MaybeGetJsonString(subaccount), | |
| ) | |
| } | |
| perpetualId := subaccount.PerpetualPositions[0].PerpetualId | |
| k.MemClob.InsertZeroFillDeleveragingIntoOperationsQueue(ctx, negativeTncSubaccountId, perpetualId) | |
| return nil | |
| } | |
| // GateWithdrawalsIfNegativeTncSubaccountSeen gates withdrawals if a negative TNC subaccount exists. | |
| // It does this by inserting a zero-fill deleveraging operation into the operations queue iff any of | |
| // the provided negative TNC subaccounts are still negative TNC. | |
| // TODO: Consider adding metrics and logging to monitor the gating of withdrawals. | |
| func (k Keeper) GateWithdrawalsIfNegativeTncSubaccountSeen( | |
| ctx sdk.Context, | |
| negativeTncSubaccountIds []satypes.SubaccountId, | |
| ) (err error) { | |
| foundNegativeTncSubaccount := false | |
| var negativeTncSubaccountId satypes.SubaccountId | |
| for _, subaccountId := range negativeTncSubaccountIds { | |
| bigNetCollateral, | |
| _, | |
| _, | |
| err := k.subaccountsKeeper.GetNetCollateralAndMarginRequirements( | |
| ctx, | |
| satypes.Update{SubaccountId: subaccountId}, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| // If the subaccount has negative TNC, mark that a negative TNC subaccount was found. | |
| if bigNetCollateral.Sign() == -1 { | |
| foundNegativeTncSubaccount = true | |
| negativeTncSubaccountId = subaccountId | |
| break | |
| } | |
| } | |
| if !foundNegativeTncSubaccount { | |
| return nil | |
| } | |
| // A negative TNC subaccount was found, therefore insert a zero-fill deleveraging operation into | |
| // the operations queue to indicate withdrawals should be gated. | |
| subaccount := k.subaccountsKeeper.GetSubaccount(ctx, negativeTncSubaccountId) | |
| perpetualPositions := subaccount.GetPerpetualPositions() | |
| if len(perpetualPositions) == 0 { | |
| return errorsmod.Wrapf( | |
| types.ErrNoPerpetualPositionsToLiquidate, | |
| "GateWithdrawalsIfNegativeTncSubaccountSeen: subaccount has no open positions: (%s)", | |
| lib.MaybeGetJsonString(subaccount), | |
| ) | |
| } | |
| perpetualId := subaccount.PerpetualPositions[0].PerpetualId | |
| k.MemClob.InsertZeroFillDeleveragingIntoOperationsQueue(ctx, negativeTncSubaccountId, perpetualId) | |
| return nil | |
| } |
| @@ -37,6 +33,7 @@ func (_m *MemClob) CancelOrder(ctx types.Context, msgCancelOrder *clobtypes.MsgC | |||
| } | |||
| } | |||
|
|
|||
| var r1 error | |||
There was a problem hiding this comment.
The error variable r1 is declared but not used in the CancelOrder function. This is likely a leftover from the removal of error return values. The mock should be consistent with the interface it's mocking.
- var r1 errorCommittable 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.
| var r1 error |
| @@ -82,6 +75,7 @@ | |||
| } | |||
| } | |||
|
|
|||
| var r1 error | |||
There was a problem hiding this comment.
Similar to the CancelOrder function, the DeleverageSubaccount function has an unused error variable r1. This should be removed to reflect the changes in the interface.
- var r1 errorCommittable 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.
| var r1 error |
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.OrderId) uint32); ok { | ||
| r0 = rf(ctx, orderId) | ||
| } else { | ||
| r0 = ret.Get(0).(uint32) | ||
| } | ||
|
|
||
| var r1 bool |
There was a problem hiding this comment.
The GetCancelOrder function has an unused boolean variable r1. If the function's signature has changed and it no longer returns an error, this variable should be removed.
- var r1 boolCommittable 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.
| var r1 bool |
| var r1 clobtypes.Order | ||
| if rf, ok := ret.Get(1).(func(types.Context, clobtypes.ClobPairId) clobtypes.Order); ok { | ||
| r1 = rf(ctx, clobPairId) | ||
| } else { | ||
| r1 = ret.Get(1).(clobtypes.Order) | ||
| } | ||
|
|
||
| var r2 clobtypes.Order | ||
| if rf, ok := ret.Get(2).(func(types.Context, clobtypes.ClobPairId) clobtypes.Order); ok { | ||
| r2 = rf(ctx, clobPairId) | ||
| } else { | ||
| r2 = ret.Get(2).(clobtypes.Order) | ||
| } | ||
|
|
||
| var r3 bool |
There was a problem hiding this comment.
The GetMidPrice function has multiple return variables (r1, r2, r3) that are declared but not used. If the function's signature has changed, these should be removed.
- var r1 clobtypes.Order
- var r2 clobtypes.Order
- var r3 boolCommittable 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.
| var r1 clobtypes.Order | |
| if rf, ok := ret.Get(1).(func(types.Context, clobtypes.ClobPairId) clobtypes.Order); ok { | |
| r1 = rf(ctx, clobPairId) | |
| } else { | |
| r1 = ret.Get(1).(clobtypes.Order) | |
| } | |
| var r2 clobtypes.Order | |
| if rf, ok := ret.Get(2).(func(types.Context, clobtypes.ClobPairId) clobtypes.Order); ok { | |
| r2 = rf(ctx, clobPairId) | |
| } else { | |
| r2 = ret.Get(2).(clobtypes.Order) | |
| } | |
| var r3 bool |
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.ClobPair, perpetualstypes.GetPricePremiumParams) int32); ok { | ||
| r0 = rf(ctx, clobPair, params) | ||
| } else { | ||
| r0 = ret.Get(0).(int32) | ||
| } | ||
|
|
||
| var r1 error |
There was a problem hiding this comment.
The GetPricePremium function has an unused error variable r1. This should be removed to reflect the changes in the interface.
- var r1 errorCommittable 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.
| var r1 error |
| @@ -300,6 +272,7 @@ | |||
| } | |||
| } | |||
|
|
|||
| var r1 error | |||
There was a problem hiding this comment.
The GetSubaccountOrders function has an unused error variable r1. This should be removed to clean up the code.
- var r1 errorCommittable 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.
| var r1 error |
| return r0, r1 | ||
| } | ||
|
|
||
| // InsertZeroFillDeleveragingIntoOperationsQueue provides a mock function with given fields: ctx, subaccountId, perpetualId | ||
| func (_m *MemClob) InsertZeroFillDeleveragingIntoOperationsQueue(ctx types.Context, subaccountId subaccountstypes.SubaccountId, perpetualId uint32) { | ||
| _m.Called(ctx, subaccountId, perpetualId) | ||
| } | ||
|
|
||
| // PlaceOrder provides a mock function with given fields: ctx, order | ||
| func (_m *MemClob) PlaceOrder(ctx types.Context, order clobtypes.Order) (subaccountstypes.BaseQuantums, clobtypes.OrderStatus, *clobtypes.OffchainUpdates, error) { | ||
| ret := _m.Called(ctx, order) | ||
|
|
||
| var r0 subaccountstypes.BaseQuantums | ||
| var r1 clobtypes.OrderStatus | ||
| var r2 *clobtypes.OffchainUpdates | ||
| var r3 error | ||
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.Order) (subaccountstypes.BaseQuantums, clobtypes.OrderStatus, *clobtypes.OffchainUpdates, error)); ok { | ||
| return rf(ctx, order) | ||
| } | ||
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.Order) subaccountstypes.BaseQuantums); ok { | ||
| r0 = rf(ctx, order) | ||
| } else { | ||
| r0 = ret.Get(0).(subaccountstypes.BaseQuantums) | ||
| } | ||
|
|
||
| var r1 clobtypes.OrderStatus | ||
| if rf, ok := ret.Get(1).(func(types.Context, clobtypes.Order) clobtypes.OrderStatus); ok { | ||
| r1 = rf(ctx, order) | ||
| } else { | ||
| r1 = ret.Get(1).(clobtypes.OrderStatus) | ||
| } | ||
|
|
||
| var r2 *clobtypes.OffchainUpdates | ||
| if rf, ok := ret.Get(2).(func(types.Context, clobtypes.Order) *clobtypes.OffchainUpdates); ok { | ||
| r2 = rf(ctx, order) | ||
| } else { |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [301-317]
The PlaceOrder function has an unused error variable r3. This should be removed if the function's signature has been updated.
- var r3 error| ret := _m.Called(ctx, liquidationOrder) | ||
|
|
||
| var r0 subaccountstypes.BaseQuantums | ||
| var r1 clobtypes.OrderStatus | ||
| var r2 *clobtypes.OffchainUpdates | ||
| var r3 error | ||
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.LiquidationOrder) (subaccountstypes.BaseQuantums, clobtypes.OrderStatus, *clobtypes.OffchainUpdates, error)); ok { | ||
| return rf(ctx, liquidationOrder) | ||
| } | ||
| if rf, ok := ret.Get(0).(func(types.Context, clobtypes.LiquidationOrder) subaccountstypes.BaseQuantums); ok { | ||
| r0 = rf(ctx, liquidationOrder) | ||
| } else { | ||
| r0 = ret.Get(0).(subaccountstypes.BaseQuantums) | ||
| } | ||
|
|
||
| var r1 clobtypes.OrderStatus | ||
| if rf, ok := ret.Get(1).(func(types.Context, clobtypes.LiquidationOrder) clobtypes.OrderStatus); ok { | ||
| r1 = rf(ctx, liquidationOrder) | ||
| } else { | ||
| r1 = ret.Get(1).(clobtypes.OrderStatus) | ||
| } | ||
|
|
||
| var r2 *clobtypes.OffchainUpdates | ||
| if rf, ok := ret.Get(2).(func(types.Context, clobtypes.LiquidationOrder) *clobtypes.OffchainUpdates); ok { | ||
| r2 = rf(ctx, liquidationOrder) | ||
| } else { |
There was a problem hiding this comment.
Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [338-354]
The PlacePerpetualLiquidation function has an unused error variable r3. This should be removed to reflect the changes in the interface.
- var r3 error| ) | ||
| } | ||
|
|
||
| // AddZeroFillDeleveragingToOperationsQueue adds a zero-fill deleveraging match operation to the | ||
| // operations queue. | ||
| func (o *OperationsToPropose) AddZeroFillDeleveragingToOperationsQueue( | ||
| liquidatedSubaccountId satypes.SubaccountId, | ||
| perpetualId uint32, | ||
| ) { | ||
| o.OperationsQueue = append( | ||
| o.OperationsQueue, | ||
| NewMatchPerpetualDeleveragingInternalOperation( | ||
| liquidatedSubaccountId, | ||
| perpetualId, | ||
| []MatchPerpetualDeleveraging_Fill{}, | ||
| false, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| // MustAddDeleveragingToOperationsQueue adds a deleveraging match operation to the | ||
| // operations queue. | ||
| // This function will panic if: |
There was a problem hiding this comment.
Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [9-9]
The TODO comment indicates that the public fields in the OperationsToPropose struct are a hack. This should be addressed to encapsulate the struct's fields properly, ensuring that they are only modified through methods that can maintain the integrity of the data.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/e2e/withdrawal_gating_test.go
| // A negative TNC subaccount was found, therefore insert a zero-fill deleveraging operation into | ||
| // the operations queue to indicate withdrawals should be gated. | ||
| subaccount := k.subaccountsKeeper.GetSubaccount(ctx, negativeTncSubaccountId) | ||
| perpetualPositions := subaccount.GetPerpetualPositions() | ||
| if len(perpetualPositions) == 0 { | ||
| return errorsmod.Wrapf( | ||
| types.ErrNoPerpetualPositionsToLiquidate, | ||
| "GateWithdrawalsIfNegativeTncSubaccountSeen: subaccount has no open positions: (%s)", | ||
| lib.MaybeGetJsonString(subaccount), | ||
| ) | ||
| } | ||
| perpetualId := subaccount.PerpetualPositions[0].PerpetualId | ||
| k.MemClob.InsertZeroFillDeleveragingIntoOperationsQueue(ctx, negativeTncSubaccountId, perpetualId) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
I wonder if we can just modify memclob.DeleverageSubaccount for this.
We can set deltaQuantums to 0 and special case this within DeleverageSubaccount, so that we don't need these new functions like InsertZeroFillDeleveragingIntoOperationsQueue
There was a problem hiding this comment.
honestly would prefer to avoid this. the current approach is simpler since it avoids modifying existing critical paths to add on a hack (this is prob not long-term withdrawal gating solution)
for example if we modify memclob.DeleverageSubaccount, then wouldn't we have to update the logic to handle cases where zero-fills are returned from OffsetSubaccountPerpetualPosition? maybe it's fine to add these to the op queue, but imo just makes things more complicated for little gain
given this is not the long-term solution, would rather add this new function so it's easy to rip out later once we build long-term solution for withdrawal gating
| `Can place a liquidation order that is unfilled and cannot be deleveraged due to | ||
| non-overlapping bankruptcy prices, withdrawals are gated`: { |
There was a problem hiding this comment.
with the current implementation, liquidation order doesn't need to be unfilled and deleverage can still happen for withdrawals to be gated, right? since this is happening in step 8 which is after liquidations/deleveraging steps.
can we add tests for these?
There was a problem hiding this comment.
yeah I'll add tests for all variants of the below possible scenarios that can happen for verifying withdrawals are gated / not gated:
- liquidation order unfilled, partial deleveraging
- liquidation order filled, deleveraging fails
- liquidation order filled, partial deleveraging
| &withdrawMsg, | ||
| ) { | ||
| resp := tApp.CheckTx(checkTx) | ||
| require.Conditionf(t, resp.IsOK, "Expected CheckTx to succeed. Response: %+v", resp) |
There was a problem hiding this comment.
Is there a reason why we can't reject withdrawals / transfers here? is it because we don't actually process the transfer until DeliverTx?
There was a problem hiding this comment.
yeah iirc txs only execute their handler in DeliverTx right? https://github.com/dydxprotocol/cosmos-sdk/blob/b48250d4e0efacbd40f3e0b980c1e1004d29e351/baseapp/baseapp.go#L1106-L1119
There was a problem hiding this comment.
hmm yeah not sure if it's worth building an ante handler for this 🤔 nbd for now
There was a problem hiding this comment.
yeah i dont think it is personally since this is an edge case
ideally frontend's query whether withdrawals are blocked before submitting in vast majority of cases
| lib.MaybeGetJsonString(subaccount), | ||
| ) | ||
| } | ||
| perpetualId := subaccount.PerpetualPositions[0].PerpetualId |
There was a problem hiding this comment.
Is there any case a subaccount can have negative TNC and have a no perpetual positions? ie price slips fast enough and we match/liquidate at a less favorable price than the bankruptcy price?
In this case do we want to default to some value if this doesn't exist? I don't think the perpetual id is used for zero fill deleveraging operations, so having a default would be good to protect against the error when we access a nonexisting array element.
There was a problem hiding this comment.
Is there any case a subaccount can have negative TNC and have a no perpetual positions? ie price slips fast enough and we match/liquidate at a less favorable price than the bankruptcy price?
shouldn't be possible since subaccounts shouldn't be able to close out all of their positions and still have negative tnc
if this was possible, it would require a subaccount state transition to occur at a worse price than subaccount's bankruptcy price (price that subaccount tnc goes to zero), and that should be invalid
i agree it's still good to think through this though in case invariant gets violated. want me to create an m7 ticket to handle this? also maybe worth discussing with rest of CLOB team to ensure it never happens
There was a problem hiding this comment.
i think quick fix we should just add a defeault value of 0, and make a ticket to think through edge cases later
There was a problem hiding this comment.
synced on slack and we're not doing this for simplicity. created ticket here: https://linear.app/dydx/issue/CLOB-1061/use-default-perpetual-id-for-withdrawal-gating
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/lib/metrics/metric_keys.go (1 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
- protocol/x/clob/keeper/deleveraging.go (1 hunks)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (2 hunks)
Additional comments: 6
protocol/lib/metrics/metric_keys.go (1)
- 42-51: The metric key names have been updated and a new metric
GateWithdrawalsIfNegativeTncSubaccountSeenhas been added. Ensure that these changes are reflected in any dashboards or alerts that use these metrics. Also, verify that the new metric is being used correctly in the code where withdrawal gating logic is implemented.protocol/x/clob/e2e/withdrawal_gating_test.go (1)
- 28-347: The test cases provided seem to cover various scenarios including the gating and ungating of withdrawals and transfers based on the state of TNC subaccounts. It's important to ensure that all the scenarios mentioned in the previous comments are indeed covered by these tests. Additionally, verify that the new logic does not introduce any regressions by running the full test suite.
protocol/x/clob/keeper/deleveraging.go (1)
- 191-256: The new function
GateWithdrawalsIfNegativeTncSubaccountSeenhas been added to handle gating withdrawals when negative TNC subaccounts are detected. The function iterates over the provided subaccount IDs, checks for negative TNC, and if found, inserts a zero-fill deleveraging operation. This is in line with the PR objectives to enhance the protocol's financial stability.However, there are a few points to consider:
- Ensure that the function is called in the correct context where the gating of withdrawals is necessary.
- Verify that the insertion of a zero-fill deleveraging operation is the appropriate action for all cases where a negative TNC subaccount is found.
- Confirm that the function's error handling is consistent with the rest of the codebase, particularly with the use of
errorsmod.Wrapffor error wrapping.protocol/x/clob/keeper/process_operations.go (1)
- 695-695: The addition of a new metric label
DeliverTxwith a value oftrueto thePersistMatchDeleveragingToStatefunction is noted. This change is likely intended to provide more granular telemetry data. Ensure that this new metric is consistent with the telemetry strategy and that it is being used effectively in monitoring and alerting systems.protocol/x/clob/memclob/memclob.go (2)
- 10-10: The addition of the
cmtlogimport should be verified to ensure that it is used within the file and that it does not introduce any unused dependencies.Verification successful
The output from the shell script confirms that the
cmtlogimport is indeed used within thememclob.gofile. Specifically, it is used to create a new lazy sprintf with the order hash.* 219-231: The new function `InsertZeroFillDeleveragingIntoOperationsQueue` is designed to insert a zero-fill deleveraging operation into the operations queue, which is used to indicate that a subaccount has negative TNC and withdrawals should be disabled. This change seems to align with the PR objectives of enhancing the deleveraging process and withdrawal gating mechanisms. Ensure that the logic of adding the operation to the queue is correct and that the function is called appropriately within the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the cmtlog import is used in the file memclob.go rg --type go 'cmtlog\.' protocol/x/clob/memclob/memclob.goLength of output: 115
Changelist
Gate withdrawals if a negative TNC subaccount is encountered after the liquidation and deleveraging steps in
PrepareCheckState.Note that all potentially liquidatable subaccounts specified by the daemon will be checked for simplicity.
Test Plan
Wrote E2E tests verifying that when there's negative TNC subaccounts found in state, that withdrawals are blocked.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.