Initial PrepareCheckState audit
#695
Conversation
WalkthroughThe changes primarily focus on improving error handling, logging, and code clarity in the clob and memclob modules. Key modifications include better propagation of errors in the liquidation process, enhanced logging for missing conditional orders, and renaming of variables for better readability. The logic for handling short-term orders and purging invalid memclob state has also been refined. Changes
TipsChat with CodeRabbit Bot (
|
| @@ -271,6 +271,9 @@ func (k Keeper) PlacePerpetualLiquidation( | |||
| ctx, | |||
| liquidationOrder, | |||
| ) | |||
| if err != nil { | |||
There was a problem hiding this comment.
IIRC we were previously ignoring this error and re-assigning it below
| @@ -454,6 +454,17 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( | |||
|
|
|||
| orderPlacement, exists := k.GetLongTermOrderPlacement(ctx, orderId) | |||
| if !exists { | |||
| // Error log if this is a conditional orders and it doesn't exist in state, since it | |||
| // can't have been canceled. | |||
| if orderId.IsConditionalOrder() { | |||
There was a problem hiding this comment.
Conditional orders should always exist in state, so error logging here if they don't exist
There was a problem hiding this comment.
Also might be good to assert IsConditionalOrderTriggered is True. Since the conditional order has to be triggered at this point in time. It is possible to read out from untriggered state inGetLongTermOrderPlacement.
There was a problem hiding this comment.
^ should definitely assert that the conditional order is triggered
There was a problem hiding this comment.
This validation is performed in PlaceConditionalOrdersTriggeredInLastBlock so I'm going to skip this for now if that's fine! Let me know if anyone disagrees.
| @@ -1004,7 +1004,7 @@ func (m *MemClobPriceTimePriority) ReplayOperations( | |||
| } | |||
|
|
|||
| // Log an error if there are two Order Removals for the same OrderId | |||
| if _, found := placedPreexistingStatefulOrderIds[orderId]; found { | |||
| if _, found := placedOrderRemovalOrderIds[orderId]; found { | |||
There was a problem hiding this comment.
This check was previously the exact same as the above check, which was an oversight
| existingOrder, found := m.openOrders.getOrder(ctx, otpOrderId) | ||
| if found && existingOrder.GetOrderHash() == otpOrderHash { | ||
| m.mustRemoveOrder(ctx, otpOrderId) | ||
| } else if otpOrderId.IsShortTermOrder() { | ||
| } else { |
There was a problem hiding this comment.
This shouldn't be necessary since all of these operations should be short-term orders
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/keeper/liquidations.go (1 hunks)
- protocol/x/clob/keeper/orders.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (5 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/keeper/orders.go
Additional comments: 6
protocol/x/clob/keeper/liquidations.go (1)
- 274-276: The addition of error handling here is a good practice. It ensures that any error that occurs during the placement of the liquidation order is properly propagated and can be handled upstream. This can help in debugging and maintaining the code.
protocol/x/clob/memclob/memclob.go (5)
1007-1007: The change from
placedPreexistingStatefulOrderIdstoplacedOrderRemovalOrderIdsis noted. Ensure that the new mapplacedOrderRemovalOrderIdsis properly initialized and populated before this check. Also, verify that this change does not affect other parts of the code that might still be usingplacedPreexistingStatefulOrderIds.1135-1140: The condition
otpOrderId.IsShortTermOrder()has been removed in the new hunk. This might lead to unintended removal of non-short-term orders fromShortTermOrderTxBytes. If this is intentional, ensure that the implications of this change are well understood and tested.1168-1168: The parameter
fullyFilledOrderIdshas been replaced withfilledOrderIds. Ensure that this change is reflected in all calls toPurgeInvalidMemclobStateand thatfilledOrderIdscan include both partially and fully filled orders, as the function name suggests purging of invalid state, not just fully filled orders.1186-1188: The loop now iterates over
filledOrderIdsinstead offullyFilledOrderIds. Ensure thatRemoveOrderIfFilledcan handle both partially and fully filled orders, as the change in parameter name suggests.2006-2013: The addition of error logging when an order that should be partially or fully filled has no fill amount is a good practice. It will help in debugging and identifying issues faster.
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/orders.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (5 hunks)
Additional comments: 3
protocol/x/clob/memclob/memclob.go (2)
- 1135-1140: The condition for removing the order hash from
ShortTermOrderTxByteshas been changed. Previously, it was removed if the order was a Short-Term order. Now, it's removed if the order is not found or the order hash doesn't match. This change could potentially lead to different behavior. Please verify if this is the intended behavior.1168:
The parameterfullyFilledOrderIdshas been renamed tofilledOrderIds. Ensure that all calls to this function throughout the codebase have been updated to match the new parameter name.1186:
The loop iterating overfilledOrderIds(previouslyfullyFilledOrderIds) and callingRemoveOrderIfFilledremains the same, only the parameter name has changed. Ensure that the logic withinRemoveOrderIfFilledis compatible with this change.
- 2006-2016: Error logging has been added when a filled Short-Term order ID has no fill amount. This is a good practice as it helps in debugging and understanding the system's state. However, ensure that this logging does not introduce any performance issues, especially if the number of Short-Term orders is large.
protocol/x/clob/keeper/orders.go (1)
- 457-467: The addition of an error log when a conditional order doesn't exist in the state is a good practice for debugging and understanding the system's behavior. However, it's important to ensure that this logging doesn't introduce performance issues, especially if the number of non-existing conditional orders is high. Consider adding a rate limit to your logging or making this log optional based on a debug flag.
+ if orderId.IsConditionalOrder() { + k.Logger(ctx).Error( + fmt.Sprintf( + "PlaceStatefulOrdersFromLastBlock: Triggered conditional Order ID %+v doesn't exist in state", + orderId, + ), + ) + }Committable suggestion (Beta)
if orderId.IsConditionalOrder() { if debugFlag { k.Logger(ctx).Error( fmt.Sprintf( "PlaceStatefulOrdersFromLastBlock: Triggered conditional Order ID %+v doesn't exist in state", orderId, ), ) } }
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/keeper/liquidations.go (1 hunks)
Additional comments: 1
protocol/x/clob/keeper/liquidations.go (1)
- 274-276: The error handling here is a good addition. It ensures that any error that occurs during the execution of
PlacePerpetualLiquidationis propagated up the call stack and handled appropriately. This is a good practice as it allows for better debugging and error management.
protocol/x/clob/memclob/memclob.go
Outdated
| // If there is no fill amount for this order, return early. Note this is only valid if the | ||
| // order is a stateful order that was fully-filled or partially-filled and expired / canceled / | ||
| // removed in the last block. |
There was a problem hiding this comment.
| // If there is no fill amount for this order, return early. Note this is only valid if the | |
| // order is a stateful order that was fully-filled or partially-filled and expired / canceled / | |
| // removed in the last block. | |
| // If there is no fill amount for this order, return early. Note this is only valid if the | |
| // order is a stateful order that was fully-filled or partially-filled and expired / canceled / | |
| // removed in the last block. For short-term orders, their fill amounts are stored past expiration, | |
| // so this return is invalid. |
There was a problem hiding this comment.
Updated, lmk what you think!
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/memclob/memclob.go (5 hunks)
Additional comments: 5
protocol/x/clob/memclob/memclob.go (5)
1007-1007: The variable
placedOrderRemovalOrderIdsis used instead ofplacedPreexistingStatefulOrderIdsto check for duplicate order removals. Ensure that this change is intentional and that the new variable is correctly populated elsewhere in the code.1135-1140: The condition to remove the order hash from
ShortTermOrderTxByteshas been simplified. Previously, it was only removed if the order was a Short-Term order. Now, it's removed for all cases where the order is not found or the order hash doesn't match. This change could potentially affect the behavior of the system if there are cases where non-Short-Term orders are involved. Please verify this change.1168-1168: The parameter
fullyFilledOrderIdshas been renamed tofilledOrderIds. Ensure that all calls to this function throughout the codebase have been updated to match the new parameter name.1186-1186: The loop variable
fullyFilledOrderIdshas been renamed tofilledOrderIds. Ensure that this variable is correctly populated elsewhere in the code.2006-2017: Added error logging for the case where a filled Short-Term order ID has no fill amount. This is a good practice as it helps in identifying potential issues in the system.
Changelist
Small
PrepareCheckStatefixes from audit (nothing major that needs to be backported to the release).Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.