Restrict order replacement to only changing size, price, GTT/GTB #1623
Conversation
WalkthroughThe recent changes mainly focus on updating test cases and validation logic for order replacements in the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant TestFramework as Test Framework
participant ClobModule as CLOB Module
participant Validator as Validator
User->>TestFramework: Run reduce-only order tests
TestFramework->>ClobModule: Place reduce-only order
ClobModule->>Validator: Validate order
Validator-->>ClobModule: Return validation result
ClobModule-->>TestFramework: Return test result
TestFramework-->>User: Display test outcome
User->>TestFramework: Run short-term order tests
TestFramework->>ClobModule: Place short-term order
ClobModule->>Validator: Validate order
Validator-->>ClobModule: Return validation result
ClobModule-->>TestFramework: Return test result
TestFramework-->>User: Display test outcome
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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 (3)
protocol/x/clob/memclob/memclob.go (3)
Line range hint
404-404: ### Missing Method Implementations
Several methods such asGenerateStreamOrderbookFill,GetOrderbookUpdatesForOrderUpdate,GetOrderbookUpdatesForOrderPlacement, andGetOrderbookUpdatesForOrderRemovalare called but not defined in theMemClobPriceTimePrioritystruct. This will cause runtime errors if not addressed.+ // TODO: Implement the missing methods or ensure they are correctly linked if defined elsewhere.Also applies to: 873-873, 877-877, 1512-1512, 1952-1952
Line range hint
934-934: ### Unused Variables
The variablesplacedPreexistingStatefulOrderIdsandplacedOrderRemovalOrderIdsare declared but not used within theReplayOperationsmethod. This could lead to inefficiencies and should be addressed either by utilizing these variables or removing them if they are unnecessary.- placedPreexistingStatefulOrderIds := make(map[types.OrderId]struct{}) - placedOrderRemovalOrderIds := make(map[types.OrderId]struct{})Also applies to: 935-935
Line range hint
36-36: ### Undefined Type: Orderbook
The typeOrderbookis referenced but not defined within the file. This will cause compilation errors. Ensure that theOrderbooktype is correctly defined and imported if it's located in another package.+ type Orderbook struct { + // Define the structure fields here. + }Also applies to: 70-70, 2174-2174
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/clob/e2e/reduce_only_orders_test.go (2 hunks)
- protocol/x/clob/e2e/short_term_orders_test.go (5 hunks)
- protocol/x/clob/memclob/memclob.go (2 hunks)
- protocol/x/clob/memclob/memclob_place_order_test.go (6 hunks)
- protocol/x/clob/types/errors.go (1 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/clob/types/errors.go
Additional context used
Learnings (1)
protocol/x/clob/memclob/memclob_place_order_test.go (1)
User: jonfung-dydx PR: dydxprotocol/v4-chain#1009 File: protocol/x/clob/memclob/memclob_place_order_test.go:4108-4108 Timestamp: 2024-02-05T20:01:55.204Z Learning: The `assertPlaceOrderOffchainMessages` function in `memclob_test_util.go` indeed requires `ctx` as a parameter for its operations, contrary to the initial comment made.
golangci-lint
protocol/x/clob/memclob/memclob.go
36-36: undefined: Orderbook
70-70: undefined: Orderbook
2174-2174: undefined: Orderbook
404-404: m.GenerateStreamOrderbookFill undefined (type *MemClobPriceTimePriority has no field or method GenerateStreamOrderbookFill)
873-873: m.GetOrderbookUpdatesForOrderUpdate undefined (type *MemClobPriceTimePriority has no field or method GetOrderbookUpdatesForOrderUpdate)
877-877: m.GetOrderbookUpdatesForOrderUpdate undefined (type *MemClobPriceTimePriority has no field or method GetOrderbookUpdatesForOrderUpdate)
934-934: placedPreexistingStatefulOrderIds declared and not used
935-935: placedOrderRemovalOrderIds declared and not used
1512-1512: m.GetOrderbookUpdatesForOrderPlacement undefined (type *MemClobPriceTimePriority has no field or method GetOrderbookUpdatesForOrderPlacement)
1952-1952: m.GetOrderbookUpdatesForOrderRemoval undefined (type *MemClobPriceTimePriority has no field or method GetOrderbookUpdatesForOrderRemoval)
Additional comments not posted (8)
protocol/x/clob/e2e/reduce_only_orders_test.go (1)
677-690: New test functionTestReduceOnlyOrderFailurecorrectly handles scenarios where reduce-only orders are disabled.protocol/x/clob/e2e/short_term_orders_test.go (2)
Line range hint
975-1001: Ensure the test cases for FOK and IOC order types are correctly failing as expected.The modifications to the test descriptions and expected outcomes correctly reflect the new behavior for FOK and IOC orders, which should now fail under the new restrictions. This is consistent with the PR's objectives and the changes made in the codebase.
699-709: Update test description to reflect the new expected behavior.- "Success: Replace in same block on opposite side" + "Fail: Replace in same block on opposite side"This change aligns with the PR's objective to restrict order replacements, ensuring that replacements that change the order side are correctly handled as failures.
Likely invalid or redundant comment.
protocol/x/clob/memclob/memclob.go (2)
1421-1429: ### Use ofvalidateReplacementinvalidateNewOrder
The use ofvalidateReplacementwithinvalidateNewOrderfor both resting and matched orders is a good practice as it ensures that replacements adhere to the defined constraints. This is crucial for maintaining the integrity of order operations within the system.
1337-1366: ### Ensure Consistency in Order Replacement Validation
ThevalidateReplacementfunction correctly enforces that only size, price, and GTB/GTT can change during an order replacement. However, it's crucial to ensure that this function is consistently used wherever order replacements are validated to maintain the integrity of the order replacement logic.Verification successful
"""
"""
"""
"""
"""
"""
The search results indicate that there are multiple functions related to order processing and replacement across various files. However, the key functions that directly handle order validation and replacement logic are primarily located in
protocol/x/clob/memclob/memclob.go.The
validateReplacementfunction is used in two places withinmemclob.go, which suggests that it is being utilized for validating order replacements in the relevant contexts. Additionally, the search did not reveal any other functions that perform order replacement validation without usingvalidateReplacement.Based on the gathered information, it appears that the
validateReplacementfunction is consistently used for order replacement validation in the relevant parts of the codebase.
protocol/x/clob/memclob/memclob.go: validateReplacement(existingRestingOrder, order)protocol/x/clob/memclob/memclob.go: validateReplacement(existingMatchedOrder, order)These usages indicate that the function is applied where necessary to ensure the integrity of the order replacement logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `validateReplacement` is used consistently across all relevant parts of the codebase. # Test: Search for usage of `validateReplacement`. Expect: Usage in all relevant parts. rg --type go $'validateReplacement'Length of output: 452
Script:
#!/bin/bash # Description: Search for other potential order replacement validation points by looking for keywords related to order replacement. # Test: Search for keywords related to order replacement. Expect: Identify all relevant parts. rg --type go 'order.*replace|replace.*order|validate.*order'Length of output: 12329
Script:
#!/bin/bash # Description: Search for function definitions related to order validation and check if they call `validateReplacement`. # Test: Search for function definitions related to order validation. rg --type go 'func.*validate.*order' --context 10Length of output: 51
Script:
#!/bin/bash # Description: Search for functions related to order processing and replacement, and check if they call `validateReplacement`. # Test: Search for functions related to order processing and replacement. rg --type go 'func.*order' --context 10Length of output: 190290
protocol/x/clob/memclob/memclob_place_order_test.go (3)
207-215: The test case "Replacing an order fails if GoodTilBlock is greater than existing order and changes sides" is well-structured and covers the scenario comprehensively.
2190-2204: The test case "IOC Taker cannot replace unfilled non IOC order" correctly handles the scenario where an Immediate-Or-Cancel (IOC) order attempts to replace a non-IOC order, which should fail as expected.
2206-2220: The test case "FOK Taker cannot replace unfilled non IOC order" is correctly implemented to ensure that a Fill-Or-Kill (FOK) order cannot replace a non-IOC order. This is an important check to maintain the integrity of order types.
Changelist
Order replacement cannot change things like side, time in force, etc. anymore.
Test Plan
Modified tests
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
Bug Fixes
Tests