E2E Tests for Replacement RO Orders, allow all types of RO order replacements #949
Conversation
WalkthroughThe recent code updates involve an order processing system, focusing on the introduction of new order types and refining the validation logic for reduce-only orders. The updates also streamline test cases to align with the new order functionalities and remove an obsolete error type. The improvements emphasize error handling and testing for specific order replacement scenarios, ensuring that the system adheres to the updated order management rules. 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 (
|
| @@ -314,27 +315,6 @@ func TestReduceOnlyOrderFailure(t *testing.T) { | |||
| clobtypes.ErrReduceOnlyWouldIncreasePositionSize.Error(), | |||
| }, | |||
| }, | |||
| "Reduce only order fails to replace non-reduce only order": { | |||
There was a problem hiding this comment.
Deleted since now reduce only orders can replace non reduce only 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: 2
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/testutil/constants/orders.go (4 hunks)
- protocol/x/clob/e2e/reduce_only_orders_test.go (3 hunks)
- protocol/x/clob/memclob/memclob.go (2 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 comments: 13
protocol/x/clob/e2e/reduce_only_orders_test.go (6)
7-7: The addition of the
sdk "github.com/cosmos/cosmos-sdk/types"import is noted. Ensure that this import is used within the test cases and that it is necessary for the test logic.4-10: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [10-43]
The test case "IOC Reduce only order partially matches short term order same block, maker order fully filled" is well-structured and seems to cover the intended scenario. It sets up subaccounts, places orders, and defines expected outcomes for both orders and subaccounts. The use of constants for order and subaccount definitions enhances readability and maintainability.
- 4-10: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [44-77]
The test case "IOC Reduce only order partially matches short term order second block, maker order fully filled" follows the same pattern as the previous one, with the difference being the block in which the orders are placed. The test logic is consistent and the assertions are appropriate for the scenario being tested.
- 4-10: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [78-111]
The test case "IOC Reduce only order partially matches short term order second block, maker order partially filled" correctly sets up a scenario where a maker order is partially filled. The expectations and assertions are aligned with the scenario, ensuring that the reduce-only logic is being tested as intended.
372-372: The test case "Zero perpetual position subaccount position cannot place sell RO order" and similar failure scenarios are crucial for testing the robustness of the order validation logic. They appear to be correctly asserting the expected errors when a reduce-only order would incorrectly increase a position size.
374-430: The test cases within the "TestReduceOnlyOrderReplacement" function are comprehensive and cover various scenarios involving order replacements. The use of a function to modify the fill amount state after the first block is a good approach to simulate state changes that would occur in a live environment. The assertions for errors are correctly checking for specific error messages, which is good for ensuring the validation logic is functioning as expected.
protocol/testutil/constants/orders.go (4)
347-359: The order declarations for Alice with varying price and good-til block values seem to be consistent with the intended functionality of adding more test cases for reduce-only orders. Ensure that the
QuantumsandSubticksvalues are within expected ranges and adhere to the system's constraints.634-647: The order declarations for Carl include both buy and sell orders with the same
Subticksvalue. Verify that theSubticksvalue is intended to be the same for different order types and that it aligns with the system's requirements for order granularity.1189-1206: The declarations for Alice's reduce-only orders with
TIME_IN_FORCE_FILL_OR_KILLare well-defined. It's important to confirm that theReduceOnlyflag is being checked correctly in the order processing logic to maintain the integrity of reduce-only orders.1253-1261: The declaration for
Order_Alice_Num1_Id0_Clob0_Sell110_Price50000_GTB21_IOC_ROincludes theTIME_IN_FORCE_IOCflag along withReduceOnly. Ensure that the interaction between Immediate-Or-Cancel (IOC) and Reduce-Only (RO) orders is well-tested and behaves as expected in the system.protocol/x/clob/memclob/memclob.go (3)
- 10-11: The addition of
cmtlog "github.com/cometbft/cometbft/libs/log"anderrorsmod "cosmossdk.io/errors"seems to be for improved logging and error handling. Ensure that these new imports are used in the codebase to avoid unnecessary imports.Verification successful
The search results confirm that both
cmtloganderrorsmodare indeed used in thememclob.gofile and other files within theprotocol/x/clob/memclob/directory. This indicates that the new imports are not unnecessary and are utilized for logging and error handling as intended by the changes in the PR.* 1356-1370: The logic added for handling reduce-only orders seems to correctly consider the existing position size and any partial fills from a replaced order. However, ensure that the `existingPositionSize.Add` method correctly updates the `existingPositionSize` variable, as some languages or libraries might require the result to be assigned back to the variable.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new imports are used in the codebase rg --type go 'cmtlog|errorsmod' protocol/x/clob/memclob/Length of output: 1020
Verification successful
The
Addmethod calls in thememclob.gofile, as seen in the provided context, follow the usage pattern of Go'smath/bigpackage, where the method has a pointer receiver and modifies the value it points to. The result of the addition is stored in the variable that is passed as the first argument to theAddmethod. This confirms that theexistingPositionSize.Addmethod call in the snippet correctly updates theexistingPositionSizevariable in place, and no additional assignment is needed.* 1371-1371: The provided code snippet ends with a comment indicating a condition check for the reduce-only order against the existing position size, but the actual comparison logic is not included. Please provide the remaining part of the validation logic for a complete review.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the behavior of the Add method in the context of the codebase rg --type go 'func \(.*\) Add\(' protocol/x/clob/memclob/Length of output: 59
Script:
#!/bin/bash # Search for the Add method across the entire codebase rg --type go 'func \(.*\) Add\(' # Additionally, look for comments or documentation strings in the `memclob.go` file that might explain the behavior of the Add method rg --type go -C 3 'Add\(' protocol/x/clob/memclob/memclob.goLength of output: 1241
| Quantums: 10, | ||
| Subticks: 1_000_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } |
There was a problem hiding this comment.
The declaration for Order_Bob_Num0_Id0_Clob1_Sell10_Price15_GTB20 is incomplete in the provided code snippet. Please provide the full order declaration to ensure all necessary fields are set correctly.
| Quantums: 110, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment for Bob's order, the declaration for Order_Carl_Num0_Id2_Clob0_Sell5_Price10_GTB15 is incomplete. Please ensure that the full order declaration is provided for review.
jonfung-dydx
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/testutil/app/app.go (4 hunks)
- protocol/testutil/constants/orders.go (5 hunks)
- protocol/x/clob/e2e/reduce_only_orders_test.go (2 hunks)
- protocol/x/clob/memclob/memclob.go (2 hunks)
Additional comments: 8
protocol/x/clob/e2e/reduce_only_orders_test.go (1)
- 373-706: The new test cases added in
TestReduceOnlyOrderReplacementfunction cover various scenarios for reduce-only order replacements. It's important to ensure that these tests are comprehensive and that they cover all the edge cases mentioned in the PR objectives. The tests seem to be well-structured and follow the pattern of setting up subaccounts, placing orders, advancing blocks, and checking the results, which is consistent with the rest of the test suite.However, there are a few points to consider:
- Ensure that the test cases cover all new logic paths introduced by the changes in the validation logic for reduce-only orders.
- Verify that the test cases are not only positive but also include negative scenarios where the orders should not be replaced.
- Confirm that the test cases handle the expected error messages correctly and that these messages are aligned with the changes in the error handling mechanisms.
Given the complexity of the scenarios, it would be beneficial to add comments explaining the rationale behind each test case, especially for future maintainers of the code.
protocol/testutil/app/app.go (3)
18-23: The summary indicates that certain imports were removed and then re-added. However, the provided code snippet only shows the current imports without any indication of removal. This could be a non-issue if the imports were indeed removed and then re-added as part of the PR changes, but it's worth verifying to ensure that the summary is accurate.
1083-1083: The error message has been modified to provide more detailed information about non-determinism detected during
CheckTx. This change improves the clarity of the error message, which is beneficial for debugging purposes. Ensure that the variablesres,err,parallelRes, andparallelErrare properly logged and that the message is clear and informative.1097-1097: Similar to the previous comment, the error message for crashing app non-determinism has been updated. This change is consistent with the goal of providing clearer error messages for debugging. Again, verify that the variables
res,err,crashingRes, andcrashingErrare properly logged and that the message is clear and informative.protocol/testutil/constants/orders.go (2)
1217-1234: The order declarations for Alice's FOK (Fill or Kill) and RO (Reduce Only) orders are added, and the structure is consistent with the expected format. These declarations include the
TimeInForceandReduceOnlyfields, which are crucial for the specific order types being tested.1281-1298: The order declarations for Alice's IOC (Immediate or Cancel) and RO (Reduce Only) orders are added, and the structure is consistent with the expected format. These declarations include the
TimeInForceandReduceOnlyfields, which are crucial for the specific order types being tested.protocol/x/clob/memclob/memclob.go (2)
10-11: The addition of new import statements for
cmtloganderrorsmodseems to be in line with the changes in the validation logic. Ensure that these new imports are used within the file and that there are no unused imports which could lead to unnecessary package dependencies.1356-1361: The comments added provide a clear explanation of the logic for handling reduce-only orders. However, ensure that the actual implementation of this logic is consistent with the comments and that the validation checks for reduce-only orders are correctly implemented in the subsequent code.
| Order_Alice_Num1_Id0_Clob0_Sell100_Price500000_GTB20 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Alice_Num1, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: 100, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } | ||
| Order_Alice_Num1_Id0_Clob0_Sell100_Price51000_GTB20 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Alice_Num1, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: 100, | ||
| Subticks: 51_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } | ||
| Order_Alice_Num1_Id0_Clob0_Sell100_Price100000_GTB20 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Alice_Num1, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: 100, | ||
| Subticks: 1_000_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } |
There was a problem hiding this comment.
The order declarations for Alice's orders appear to be complete and consistent with the expected structure. However, the declaration for Order_Bob_Num0_Id0_Clob1_Sell10_Price15_GTB20 is incomplete, as previously noted in the outdated comments. Please provide the full order declaration to ensure all necessary fields are set correctly.
| Order_Carl_Num0_Id0_Clob0_Buy70_Price500000_GTB10 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Carl_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 25_000_000, | ||
| Quantums: 70, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 10}, | ||
| } | ||
| Order_Carl_Num0_Id0_Clob0_Sell1BTC_Price500000_GTB10 = clobtypes.Order{ | ||
| Order_Carl_Num0_Id0_Clob0_Buy110_Price500000_GTB10 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Carl_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_SELL, | ||
| Quantums: 100_000_000, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 110, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 10}, | ||
| } | ||
| Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB10 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Carl_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 110, | ||
| Subticks: 50_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 10}, | ||
| } |
There was a problem hiding this comment.
The order declarations for Carl's orders are added, and the structure seems consistent with the expected format. However, the declaration for Order_Carl_Num0_Id0_Clob0_Buy10_Price500000_GTB20 is incomplete. It is essential to provide the full order declaration for a thorough review.
| Order_Carl_Num0_Id0_Clob0_Buy10_Price50000_GTB20 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Carl_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 10, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } | ||
| Order_Carl_Num0_Id0_Clob0_Buy110_Price50000_GTB20 = clobtypes.Order{ | ||
| OrderId: clobtypes.OrderId{SubaccountId: Carl_Num0, ClientId: 0, ClobPairId: 0}, | ||
| Side: clobtypes.Order_SIDE_BUY, | ||
| Quantums: 110, | ||
| Subticks: 500_000_000_000, | ||
| GoodTilOneof: &clobtypes.Order_GoodTilBlock{GoodTilBlock: 20}, | ||
| } |
There was a problem hiding this comment.
The order declarations for Carl's orders continue here, and the structure is consistent with the expected format. However, the declaration for Order_Carl_Num0_Id2_Clob0_Sell5_Price10_GTB15 is incomplete. Please provide the full order declaration to ensure all necessary fields are set correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/testutil/constants/orders.go (5 hunks)
- protocol/x/clob/e2e/reduce_only_orders_test.go (2 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/testutil/constants/orders.go
- protocol/x/clob/e2e/reduce_only_orders_test.go
Additional comments: 2
protocol/x/clob/memclob/memclob.go (2)
1370-1375: The comments provide a clear explanation of the logic for handling reduce-only orders, including how the position size is determined and the importance of considering partial fills. This context is crucial for understanding the subsequent code and its purpose in the validation process.
1375-1375: The logic to determine if an order is reduce-only and to retrieve the existing position size appears to be correct. However, it's important to ensure that the
GetStatePositionmethod accurately reflects the current state size, including any partial fills from replaced orders. If this method's implementation has been modified as part of the PR, it would be necessary to review those changes as well.
protocol/x/clob/memclob/memclob.go
Outdated
| // If the order is a reduce-only order, we should ensure that the sign of the order size is the opposite of | ||
| // the current position size. Note that we do not validate the size/quantity of the reduce only order fill, | ||
| // as that will be validated if the order is matched. | ||
| // The subaccount's current position size is defined as is the current state size + any partial fills |
There was a problem hiding this comment.
| // The subaccount's current position size is defined as is the current state size + any partial fills | |
| // The subaccount's current position size is defined as the current state size + any partial fills |
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/e2e/reduce_only_orders_test.go (6 hunks)
- protocol/x/clob/memclob/memclob.go (1 hunks)
Additional comments: 4
protocol/x/clob/e2e/reduce_only_orders_test.go (3)
27-27: The variable
crashingAppCheckTxNonDeterminsmChecksDisabledhas been renamed tocrashingAppCheckTxNonDeterminismChecksDisabledto correct a spelling mistake. This is a good change for code clarity.718-718: The
TestReduceOnlyOrderFailurefunction has been removed. Based on the previous comment fromjonfung-dydx, this deletion is intentional as reduce-only orders can now replace non-reduce only orders. This change aligns with the PR objectives to refine the validation logic for reduce-only orders.719-1052: The new
TestReduceOnlyOrderReplacementfunction has been added to test various scenarios of order replacements. The test cases seem to cover different scenarios, including partial fills and immediate execution orders. Ensure that these tests align with the updated business logic and that the expected outcomes are correct. The test cases are well-structured and appear to cover the necessary scenarios for the new functionality.protocol/x/clob/memclob/memclob.go (1)
- 1370-1375: The changes from lines 1370 to 1375 refine the validation logic for reduce-only orders. The new logic ensures that the sign of the order size is the opposite of the current position size, which aligns with the PR objectives to enhance the validation process for replacement orders. This change appears to correctly implement the intended functionality without increasing the position size inadvertently.
However, it's important to ensure that the rest of the codebase correctly handles the new validation logic, especially where the
IsReduceOnly()method is used. Additionally, the comment on line 1372 mentions that the size/quantity of the reduce-only order fill is not validated here as it will be validated if the order is matched. This implies a dependency on the order matching logic to perform the necessary validation, which should be verified to ensure it is indeed the case.Verification successful
To further verify that the order matching logic validates the size/quantity of reduce-only order fills, we need to examine the usage of
IsReduceOnlyin the context of order matching. The filesmemclob.go,orders.go, andprocess_operations.goare likely to contain this logic.
The output indicates that
IsReduceOnlyis used in several places withinmemclob.go, which is likely where the order matching logic is implemented. The references inorders.goandprocess_operations.goalso suggest involvement in order processing, but it's not clear how they relate to the validation of reduce-only orders.To understand how the validation of the size/quantity of reduce-only order fills is performed, we need to examine the surrounding code where
IsReduceOnlyis used in these files.
The code in
memclob.goconfirms that the order matching logic includes checks to ensure that reduce-only orders do not increase the position size and are on the opposite side of the existing position. The validation of the size/quantity of reduce-only order fills is also addressed during the order matching process, with logic to resize the matched amount if necessary.The
orders.gofile contains a functionMustValidateReduceOnlyOrderthat validates reduce-only orders against the current position size, which supports the validation process.In
process_operations.go, there is a commented-out section related to the removal of invalid reduce-only orders, which suggests that there is a planned feature or a feature under development for additional validation.Based on the provided context, the changes in the PR and the surrounding code in these files, it can be concluded that the order matching logic validates the size/quantity of reduce-only order fills as intended by the PR objectives.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the order matching logic validates the size/quantity of reduce-only order fills. grep -R "IsReduceOnly" . --include=\*.goLength of output: 1923
Script:
#!/bin/bash # Examine the usage of `IsReduceOnly` in the context of order matching within memclob.go, orders.go, and process_operations.go rg "IsReduceOnly" protocol/x/clob/memclob/memclob.go rg "IsReduceOnly" protocol/x/clob/keeper/orders.go rg "IsReduceOnly" protocol/x/clob/keeper/process_operations.goLength of output: 398
Script:
#!/bin/bash # Examine the surrounding code for the usage of `IsReduceOnly` in memclob.go rg -C 10 "IsReduceOnly" protocol/x/clob/memclob/memclob.go # Examine the surrounding code for the usage of `IsReduceOnly` in orders.go rg -C 10 "IsReduceOnly" protocol/x/clob/keeper/orders.go # Examine the surrounding code for the usage of `IsReduceOnly` in process_operations.go rg -C 10 "IsReduceOnly" protocol/x/clob/keeper/process_operations.goLength of output: 4876
No description provided.