[CT-1327] place post only orders first in prepare check state #2618
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the order handling functionality within the CLOB (Central Limit Order Book) system. Key updates include the addition of a boolean parameter Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Outside diff range and nitpick comments (7)
protocol/x/clob/types/memclob.go (1)
113-113: Breaking change: New parameter added to ReplayOperations interface method.The addition of
onlyPlacePostOnlyparameter is well-aligned with the PR objective of prioritizing post-only orders. However, this is a breaking change that requires updates to all implementations of theMemClobinterface.Consider adding this change to the state-breaking changes documentation if not already done.
protocol/x/clob/types/order.go (1)
183-186: LGTM! Consider adding unit tests.The
IsPostOnlyOrdermethod is well-implemented and aligns with the PR objectives to modify post-only order processing. The implementation is clean, concise, and follows the codebase's conventions.Consider adding unit tests to verify the behavior of this method for both post-only and non-post-only orders. I can help generate the test cases if needed.
protocol/x/clob/keeper/orders.go (2)
499-499: LGTM! Consider adding documentation for the new parameter.The implementation correctly filters orders based on the
onlyPlacePostOnlyflag, aligning with the PR objective to modify post-only order processing.Consider adding documentation for the
onlyPlacePostOnlyparameter in the function's comment block to explain its purpose and behavior:// PlaceStatefulOrdersFromLastBlock validates and places stateful orders from the last block onto the memclob. // Note that stateful orders could fail to be placed due to various reasons such as collateralization // check failures, self-trade errors, etc. In these cases the `checkState` will not be written to. // This function is used in: // 1. `PrepareCheckState` to place newly placed long term orders from the last // block from ProcessProposerMatchesEvents.PlacedStatefulOrderIds. This is step 3 in PrepareCheckState. // 2. `PlaceConditionalOrdersTriggeredInLastBlock` to place conditional orders triggered in the last block // from ProcessProposerMatchesEvents.ConditionalOrderIdsTriggeredInLastBlock. This is step 4 in PrepareCheckState. +// +// Parameters: +// - onlyPlacePostOnly: When true, only post-only orders will be placed, skipping all other orders. +// This is used to prioritize post-only orders in the prepare check state.Also applies to: 525-530
589-589: LGTM! Consider adding documentation for the new parameter.The implementation correctly forwards the
onlyPlacePostOnlyparameter toPlaceStatefulOrdersFromLastBlock.Consider adding documentation for the
onlyPlacePostOnlyparameter in the function's comment block:// PlaceConditionalOrdersTriggeredInLastBlock takes in a list of conditional order ids that were triggered // in the last block, verifies they are conditional orders, verifies they are in triggered state, and places // the orders on the memclob. +// +// Parameters: +// - onlyPlacePostOnly: When true, only post-only orders will be placed, skipping all other orders. +// This parameter is forwarded to PlaceStatefulOrdersFromLastBlock.Also applies to: 619-624
protocol/x/clob/memclob/memclob.go (1)
927-931: Consider improving readability with a descriptive variable.While the logic is correct, readability could be enhanced by extracting the condition into a descriptive variable.
Consider this refactor:
-if onlyPlacePostOnly && !order.IsPostOnlyOrder() { +shouldSkipNonPostOnlyOrder := onlyPlacePostOnly && !order.IsPostOnlyOrder() +if shouldSkipNonPostOnlyOrder { continue }protocol/x/clob/keeper/orders_test.go (2)
2221-2222: Enhance test clarity by using subtestsConsider using subtests or separate test cases to distinguish between the scenarios when
PlaceStatefulOrdersFromLastBlockis called withtrueandfalse. This will improve the readability and maintainability of your tests.
2380-2381: Improve test organization by separating parameter variationsSimilarly, for
PlaceConditionalOrdersTriggeredInLastBlock, consider splitting the calls withtrueandfalseinto separate subtests or test cases. This will make it clearer what each test is validating and aid future enhancements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
protocol/mocks/MemClob.go(1 hunks)protocol/x/clob/abci.go(3 hunks)protocol/x/clob/keeper/orders.go(4 hunks)protocol/x/clob/keeper/orders_test.go(2 hunks)protocol/x/clob/memclob/memclob.go(2 hunks)protocol/x/clob/types/memclob.go(1 hunks)protocol/x/clob/types/order.go(1 hunks)
🔇 Additional comments (9)
protocol/x/clob/types/memclob.go (1)
113-113: Verify all MemClob implementations are updated.
Let's verify that all implementations of the MemClob interface have been updated with the new parameter.
✅ Verification successful
All MemClob implementations are properly updated with the new parameter
Based on the search results, I can confirm that all implementations of the MemClob interface have been properly updated with the new onlyPlacePostOnly parameter:
- The interface definition in
protocol/x/clob/types/memclob.goincludes the parameter - The main implementation
MemClobPriceTimePriorityinprotocol/x/clob/memclob/memclob.goincludes the parameter - The mock implementation in
protocol/mocks/MemClob.gohas also been updated
All implementations are consistent with the interface change, and no issues were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementations of MemClob interface and verify they include the new parameter
# Search for struct types that implement MemClob
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) ReplayOperations(ctx sdk.Context, localOperations []InternalOperation, shortTermOrderTxBytes map[OrderHash][]byte, existingOffchainUpdates *OffchainUpdates$_) $_ {'
# Also check mock implementations
fd -e go -E 'mock.*\.go' --exec rg -A 5 'ReplayOperations'
Length of output: 6350
Script:
#!/bin/bash
# Let's search for all MemClob interface implementations more effectively
# First, find the MemClob interface definition
ast-grep --pattern 'type MemClob interface {
$$$
}'
# Then find all struct implementations
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) ReplayOperations'
# Also search for any ReplayOperations function implementations
rg "func.*ReplayOperations" -A 5
Length of output: 10712
protocol/x/clob/abci.go (4)
188-213: LGTM: First pass implementation for post-only orders
The implementation correctly prioritizes post-only orders by processing them first, which aligns with the PR objective. The approach is consistent across all three order placement operations.
188-188: LGTM: Clear step organization and documentation
The step numbering and comments have been updated to clearly reflect the new two-pass approach while maintaining the logical flow of operations.
Also applies to: 215-215, 238-238, 246-246, 258-258, 271-271, 278-278
188-188: Verify the keeper parameter type change
The function signature changed from pointer to value receiver for the keeper parameter. This change seems unrelated to the PR objective and could have performance implications.
Let's check the size of the keeper struct and its usage across the codebase:
Line range hint 215-256: Verify the completeness of second pass order processing
The second pass processes non-post-only orders using the same functions. However, we should verify that orders aren't accidentally processed twice.
Let's check the implementation of these functions:
✅ Verification successful
Let me gather more specific information about the order processing implementation.
Let me gather one more piece of information about how orders are placed on the memclob.
Let me gather more specific information about how orders are processed in the second pass.
Based on the code analysis, I can now generate the final response:
Orders are safely processed in both passes without duplication
The implementation prevents double processing through:
- The
onlyPlacePostOnlyparameter inPlaceStatefulOrdersFromLastBlockwhich controls whether only post-only orders or non-post-only orders are processed in each pass - Each order is processed exactly once in each pass since the orders are taken from distinct slices (
longTermOrderIdsandconditionalOrderIdsTriggeredInLastBlock) that are populated during block processing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of order placement functions
ast-grep --pattern 'func $_(ctx $_, $_, $_, postOnly bool) $_ {
$$$
}'
# Check for potential duplicate processing prevention
rg -A 5 'postOnly.*bool'
Length of output: 3073
Script:
#!/bin/bash
# Check the implementation of PlaceStatefulOrdersFromLastBlock and PlaceConditionalOrdersTriggeredInLastBlock
rg -A 20 "func.*PlaceStatefulOrdersFromLastBlock|func.*PlaceConditionalOrdersTriggeredInLastBlock" protocol/
# Check if there's any tracking of processed orders
rg "ProcessedOrders|processedOrders|processed_orders" protocol/
# Look for any order state tracking in memclob
rg -A 10 "type MemClob struct" protocol/
Length of output: 7979
Script:
#!/bin/bash
# Check the implementation of order placement in memclob
rg -A 15 "func.*PlaceOrder" protocol/
# Look for any order filtering based on postOnly
rg -B 2 -A 5 "if.*postOnly" protocol/
Length of output: 101790
Script:
#!/bin/bash
# Check the implementation of PlaceStatefulOrdersFromLastBlock to see how orders are processed
rg -B 5 -A 15 "func.*PlaceStatefulOrdersFromLastBlock" protocol/x/clob/keeper/
# Look for any state tracking or order processing in the memclob implementation
rg -B 2 -A 10 "func.*ReplayOperations" protocol/x/clob/memclob/
Length of output: 5042
protocol/mocks/MemClob.go (2)
581-582: LGTM: Method signature updated correctly.
The ReplayOperations method signature has been properly updated to include the new onlyPlacePostOnly boolean parameter, maintaining consistency with the interface changes.
590-591: LGTM: Implementation handles new parameter correctly.
The type assertion and function call have been properly updated to include the new onlyPlacePostOnly parameter in the mock implementation.
protocol/x/clob/memclob/memclob.go (2)
882-882: LGTM! Method signature change is clear and purposeful.
The new parameter onlyPlacePostOnly is well-named and aligns with the PR's objective to modify post-only order processing.
Line range hint 882-931: Verify all callers of ReplayOperations are updated.
The addition of the onlyPlacePostOnly parameter requires updates to all callers of this method.
Let's verify all callers are properly updated:
✅ Verification successful
Let me gather more context about the actual calls to verify if they are properly updated.
All callers of ReplayOperations are properly updated
The verification shows that all callers of ReplayOperations have been updated to include the new onlyPlacePostOnly parameter. The method is called in two places in abci.go, and both calls properly set the parameter:
- One call with
truefor post-only orders - One call with
falsefor all orders
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to ReplayOperations to ensure they include the new parameter
rg -A 2 "ReplayOperations\("
Length of output: 1210
Script:
#!/bin/bash
# Get more context around the ReplayOperations calls in abci.go
rg -B 2 -A 5 "ReplayOperations\(" protocol/x/clob/abci.go
Length of output: 441
protocol/x/clob/keeper/orders.go
Outdated
| @@ -521,6 +522,12 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( | |||
| } | |||
|
|
|||
| order := orderPlacement.GetOrder() | |||
|
|
|||
| // Prioritize placing post-only orders if the flag is set. | |||
There was a problem hiding this comment.
Nit:
| // Prioritize placing post-only orders if the flag is set. | |
| // Only place post-only orders if the flag is set. |
protocol/x/clob/keeper/orders.go
Outdated
| @@ -521,6 +522,12 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( | |||
| } | |||
|
|
|||
| order := orderPlacement.GetOrder() | |||
|
|
|||
| // Prioritize placing post-only orders if the flag is set. | |||
| if onlyPlacePostOnly && !order.IsPostOnlyOrder() { | |||
There was a problem hiding this comment.
We call this function twice, second time with onlyPlacePostOnly = false, which doesn't exclude the PO stateful orders. Does that mean PO orders are placed twice and is that an issue?
There was a problem hiding this comment.
Ah good catch. Meant to do this for all orders
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/clob/memclob/memclob.go (1)
927-931: Correct the misleading comment about skipping ordersThe comment on line 927 incorrectly states:
// Skip the order if it is a post-only order and we are only replaying post-only orders.However, the condition in the code is:
if onlyPlacePostOnly && !order.IsPostOnlyOrder() { continue }This logic skips non-post-only orders when
onlyPlacePostOnlyis true. Please update the comment to accurately reflect the code behavior, such as:// Skip the order if it is not a post-only order and we are only replaying post-only orders.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/x/clob/memclob/memclob.go(4 hunks)
🔇 Additional comments (3)
protocol/x/clob/memclob/memclob.go (3)
882-883: Addition of onlyPlacePostOnly parameter to ReplayOperations
The addition of the onlyPlacePostOnly boolean parameter to the ReplayOperations function enhances its functionality by allowing conditional replay of post-only orders. This change is well-integrated and properly updates the method signature.
1022-1026: Correct the misleading comment about skipping orders
The comment in this block similarly misrepresents the code logic. The comment should be updated to correctly describe that non-post-only orders are skipped when onlyPlacePostOnly is true.
1080-1084: Correct the misleading comment about skipping orders
As noted previously, the comment here should be corrected to accurately reflect the code logic, indicating that non-post-only orders are skipped when onlyPlacePostOnly is true.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
protocol/testutil/constants/stateful_orders.go(2 hunks)protocol/x/clob/e2e/conditional_orders_test.go(1 hunks)protocol/x/clob/e2e/order_removal_test.go(2 hunks)
🔇 Additional comments (5)
protocol/x/clob/e2e/order_removal_test.go (3)
46-46: LGTM: Constant renamed to reflect post-only status
The constant has been appropriately renamed from LongTermOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTBT5 to LongTermOrder_Alice_Num0_Id0_Clob0_Buy5_Price10_GTBT5_PO to clearly indicate it's a post-only order.
632-632: LGTM: Consistent naming convention for post-only orders
The constant renaming follows the same pattern, maintaining consistency in the codebase for post-only order identifiers.
Line range hint 1-1000: Verify test coverage for post-only order scenarios
The test suite comprehensively covers various order removal scenarios. However, let's verify the coverage of post-only specific test cases.
✅ Verification successful
Let me verify the test coverage for post-only orders by checking the test assertions and expected behaviors.
Let me check one more aspect of post-only order testing by looking at the conditional order scenarios.
Based on the analysis of the test coverage for post-only orders, I can now provide the final response:
Post-only order removal scenarios are well tested
The test suite provides comprehensive coverage for post-only order removal scenarios including:
- Basic post-only order crossing with maker orders
- Conditional post-only order behaviors
- Invalid post-only order removal validations
- Post-only order state verification after removal
- Error cases for non post-only orders being removed with post-only reasons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for post-only order scenarios
# Test: Search for post-only related test cases and assertions
rg -A 5 "post-only.*order" protocol/x/clob/e2e/
Length of output: 5391
Script:
#!/bin/bash
# Search for post-only order removal test assertions and behaviors
rg -A 10 "expectedFirstOrderRemoved|expectedSecondOrderRemoved|expectedOrderRemovals.*true.*PO" protocol/x/clob/e2e/
Length of output: 7944
Script:
#!/bin/bash
# Search for post-only conditional order test assertions
rg -B 2 -A 5 "expectedOrderRemovals.*true.*conditional.*PO|expectedErr.*post-only" protocol/x/clob/e2e/
Length of output: 571
protocol/testutil/constants/stateful_orders.go (2)
1311-1323: LGTM! Well-structured post-only order definition.
The order definition follows the established pattern and correctly sets all required fields, including the post-only time-in-force flag.
1376-1388: LGTM! Well-structured post-only order definition with appropriate BTC test values.
The order definition correctly sets up a post-only sell order for 0.25 BTC with appropriate price and time parameters.
| @@ -1869,7 +1869,7 @@ func TestConditionalOrder_TriggeringUsingMatchedPrice(t *testing.T) { | |||
| constants.Order_Dave_Num1_Id0_Clob0_Sell1BTC_Price49997_GTB10, | |||
| constants.Order_Carl_Num1_Id0_Clob0_Buy1BTC_Price50003_GTB10, | |||
| // Place the conditional order. | |||
| constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10, | |||
| constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO, | |||
There was a problem hiding this comment.
Potential issue with using a post-only order in this test case
In the test case "TakeProfit/Buy post-only conditional order can place, trigger, cross, and be removed from state", you have changed the order to a post-only order by using LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO. Post-only orders are intended to add liquidity and will not execute immediately against existing orders. This may prevent the expected cross and removal from state, which contradicts the test's objective to verify that the conditional order can cross and be removed from state.
Consider using a non-post-only order to ensure the test behaves as intended.
Apply this diff to fix the issue:
- constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO,
+ constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10,📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO, | |
| constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10, |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
protocol/x/clob/e2e/conditional_orders_test.go (1)
Line range hint
1-2618: Consider improving test maintainabilityThe test suite is comprehensive but could benefit from the following improvements:
- Add comments explaining the test logic and expected behavior for complex test cases
- Extract common setup code into helper functions to reduce duplication
- Consider grouping related test cases into subtests using
t.Run()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
protocol/x/clob/e2e/conditional_orders_test.go(2 hunks)
🔇 Additional comments (2)
protocol/x/clob/e2e/conditional_orders_test.go (2)
1872-1872: Duplicate issue with post-only order usage
The same issue exists here where a post-only order is used in a test case that expects order crossing behavior.
708-708:
Potential issue with using a post-only order in this test case
The test case "TakeProfit/Buy post-only conditional order can place, trigger, cross, and be removed from state" uses a post-only order LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO. However, post-only orders are designed to add liquidity and should not cross the book. This seems to contradict the test's objective of testing order crossing.
Consider using a regular limit order instead of a post-only order to properly test the crossing scenario:
-constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10_PO,
+constants.LongTermOrder_Dave_Num0_Id0_Clob0_Sell025BTC_Price50000_GTBT10,Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/x/clob/keeper/orders.go (1)
499-500: Consider adding documentation for the new parameter.The new
onlyPlacePostOnlyparameter should be documented in the function's comment block to explain its purpose and behavior.// PlaceStatefulOrdersFromLastBlock validates and places stateful orders from the last block onto the memclob. // Note that stateful orders could fail to be placed due to various reasons such as collateralization // check failures, self-trade errors, etc. In these cases the `checkState` will not be written to. // This function is used in: // 1. `PrepareCheckState` to place newly placed long term orders from the last // block from ProcessProposerMatchesEvents.PlacedStatefulOrderIds. This is step 3 in PrepareCheckState. // 2. `PlaceConditionalOrdersTriggeredInLastBlock` to place conditional orders triggered in the last block // from ProcessProposerMatchesEvents.ConditionalOrderIdsTriggeredInLastBlock. This is step 4 in PrepareCheckState. +// @param onlyPlacePostOnly If true, only post-only orders will be placed. If false, only non-post-only orders will be placed.protocol/x/clob/memclob/memclob.go (1)
882-883: Consider adding documentation for the new parameter.The new
onlyPlacePostOnlyparameter should be documented in the function's comment block to explain its purpose and behavior.// ReplayOperations will replay the provided operations onto the memclob. // This is used to replay all local operations from the local `operationsToPropose` from the previous block. // The following operations are supported: // - Short-Term orders. // - Newly-placed stateful orders. // - Pre-existing stateful orders. // - Stateful cancelations. // Note that match operations are no-op. +// @param onlyPlacePostOnly If true, only post-only orders will be replayed. If false, only non-post-only orders will be replayed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
protocol/x/clob/keeper/orders.go(4 hunks)protocol/x/clob/keeper/orders_test.go(2 hunks)protocol/x/clob/memclob/memclob.go(4 hunks)
🔇 Additional comments (5)
protocol/x/clob/keeper/orders.go (2)
525-529: LGTM! Clean implementation of post-only order filtering.
The logic correctly implements the filtering of orders based on the onlyPlacePostOnly flag, ensuring that orders are processed in the desired order.
589-590: LGTM! Consistent parameter propagation.
The onlyPlacePostOnly parameter is correctly propagated through to PlaceStatefulOrdersFromLastBlock, maintaining consistent behavior for both regular stateful orders and conditional orders.
Also applies to: 619-624
protocol/x/clob/keeper/orders_test.go (2)
2221-2222: LGTM! Comprehensive test coverage for both flag values.
The test correctly verifies the behavior by calling the method twice with different onlyPlacePostOnly values, ensuring both post-only and non-post-only orders are properly handled.
2251-2353: LGTM! Well-structured test cases for post-only order handling.
The test cases thoroughly cover all scenarios:
- Places PO orders when onlyPlacePostOnly = true
- Skips non-PO orders when onlyPlacePostOnly = true
- Skips PO orders when onlyPlacePostOnly = false
- Places non-PO orders when onlyPlacePostOnly = false
protocol/x/clob/memclob/memclob.go (1)
927-931: LGTM! Consistent implementation of post-only filtering across all operation types.
The logic correctly and consistently implements the filtering of orders based on the onlyPlacePostOnly flag for:
- Short-term order placements
- Pre-existing stateful orders
- Order removals
This ensures uniform handling of post-only orders throughout the replay process.
Also applies to: 1022-1026, 1080-1084
protocol/x/clob/keeper/orders.go
Outdated
| @@ -496,6 +496,7 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( | |||
| ctx sdk.Context, | |||
| placedStatefulOrderIds []types.OrderId, | |||
| existingOffchainUpdates *types.OffchainUpdates, | |||
| onlyPlacePostOnly bool, | |||
There was a problem hiding this comment.
Nit: current naming might suggest that if false, the function processes ALL orders including post-only. How abt sth like postOnlyFilter and comment explicitly what this var does?
protocol/x/clob/keeper/orders.go
Outdated
| @@ -521,6 +522,12 @@ func (k Keeper) PlaceStatefulOrdersFromLastBlock( | |||
| } | |||
|
|
|||
| order := orderPlacement.GetOrder() | |||
|
|
|||
| // Skip the order if it is a post-only order and we are only placing post-only orders. | |||
There was a problem hiding this comment.
Nit: comment could be more accurate/complete - right now it just describes one of the two scenarios
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
protocol/x/clob/memclob/memclob.go (2)
878-885: Improve comment clarity forpostOnlyFilterparameterConsider rephrasing the comment to enhance clarity. For example:
"This function now accepts a
postOnlyFilterparameter. When true, it places post-only orders; when false, it places non-post-only orders."
930-934: Consider reducing code duplication in filtering logicThe filtering logic for
postOnlyFilteris repeated in multiple places. To improve maintainability and reduce code duplication, consider extracting this logic into a helper function.Also applies to: 1025-1029, 1083-1087
protocol/x/clob/keeper/orders_test.go (2)
2267-2273: Fix grammatical error in test descriptionIn the test description, change "does not places non-PO stateful orders when postOnlyFilter = true" to "does not place non-PO stateful orders when postOnlyFilter = true" for correct grammar.
2274-2280: Fix grammatical error in test descriptionIn the test description, change "does not places PO stateful orders from last block, when postOnlyFilter = false" to "does not place PO stateful orders from last block when postOnlyFilter = false" to correct the grammar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
protocol/mocks/MemClob.go(1 hunks)protocol/x/clob/keeper/orders.go(5 hunks)protocol/x/clob/keeper/orders_test.go(2 hunks)protocol/x/clob/memclob/memclob.go(4 hunks)protocol/x/clob/types/memclob.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/types/memclob.go
- protocol/mocks/MemClob.go
🔇 Additional comments (3)
protocol/x/clob/keeper/orders.go (2)
490-492: Enhance comment to cover both scenarios
The comment currently describes one scenario. To improve completeness, rephrase it to explicitly state both scenarios:
"This function takes a postOnlyFilter parameter and places post-only orders when postOnlyFilter is true, and non-post-only orders when postOnlyFilter is false."
625-630: Code logic appears correct for postOnlyFilter usage
The call to PlaceStatefulOrdersFromLastBlock with postOnlyFilter correctly filters orders based on their post-only status.
protocol/x/clob/keeper/orders_test.go (1)
2251-2353: Test cases effectively validate postOnlyFilter functionality
The added test cases in TestPlaceStatefulOrdersFromLastBlock_PostOnly comprehensively validate the behavior of the postOnlyFilter parameter for both post-only and non-post-only orders.
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
IsPostOnlyOrderto determine if an order is post-only.postOnlyFilterfor conditional processing of orders.Bug Fixes
Tests