[CLOB-1054] trigger conditional orders using traded price within the … #945
Conversation
WalkthroughThe recent changes integrate a 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 (
|
![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 (12)
- protocol/app/app.go (1 hunks)
- protocol/testutil/constants/orders.go (2 hunks)
- protocol/testutil/constants/stateful_orders.go (3 hunks)
- protocol/testutil/constants/subaccounts.go (2 hunks)
- protocol/testutil/keeper/clob.go (3 hunks)
- protocol/x/clob/e2e/conditional_orders_test.go (1 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/process_single_match.go (1 hunks)
- protocol/x/clob/keeper/untriggered_conditional_orders.go (1 hunks)
- protocol/x/clob/types/constants.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/clob/types/memclob.go (1 hunks)
Files not summarized due to errors (1)
- protocol/x/clob/e2e/conditional_orders_test.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- protocol/x/clob/types/memclob.go
Additional comments: 26
protocol/x/clob/types/constants.go (1)
- 7-17: The constants
ShortBlockWindow,StatefulOrderTimeWindow, andConditionalOrderTriggerMultiplierare well-named, appropriately typed, and documented with comments explaining their purpose. This follows best practices for constant declarations in Go.protocol/x/clob/types/expected_keepers.go (1)
- 144-146: The
PricesKeeperinterface has been added with the methodGetMarketParam. The method signature is clear and the parameter names are appropriate. Ensure that the rest of the codebase is updated to include implementations of this interface where necessary.Verification successful
The executed script has successfully identified several instances in the codebase where the
PricesKeeperinterface is implemented. TheKeeperstructs in different packages (clob,rewards,assets,prices,perpetuals) have been updated to include aPricesKeeperfield, indicating that the necessary implementations of the interface are in place.Based on this information, it can be concluded that the
PricesKeeperinterface has been integrated into the relevant parts of the application, as expected from the PR objectives and the AI-generated summaries.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the PricesKeeper interface is implemented wherever required. ast-grep --lang go --pattern $'type $_ struct { $$$ $_ types.PricesKeeper $$$ }'Length of output: 6048
protocol/x/clob/keeper/keeper.go (2)
40-40: The
pricesKeeperfield has been added to theKeeperstruct. Ensure that it is being used appropriately throughout the codebase and that any necessary error handling is in place when interacting with this new field.78-84: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [81-106]
The
NewKeeperfunction has been updated to accept apricesKeeperparameter and initializes it correctly. This change is consistent with the addition of thepricesKeeperfield in theKeeperstruct. Ensure that all calls toNewKeeperare updated to pass the newpricesKeeperargument.Verification successful
The output from the script provides a partial view of the
NewKeeperfunction call within theprotocol/app/app.gofile. It shows thatapp.PricesKeeperis indeed passed as an argument toNewKeeper. However, the output is still truncated and does not show all the arguments, particularly the ones afterapp.IndexerEventManager.To ensure that all calls to
NewKeeperinclude thepricesKeeperargument and to verify the complete list of arguments, I need to see the entire function call. I will generate a new script to capture more lines following the match to ensure we get the full context.
The extended output from the
rgcommand now shows the full call toclobmodulekeeper.NewKeeperwithin theprotocol/app/app.gofile. It confirms thatapp.PricesKeeperis included as an argument, along with other dependencies required by theNewKeeperfunction.With this information, we can conclude that the
NewKeeperfunction in theclobmodule is being called with thepricesKeeperargument as required. This verifies that the necessary changes have been made in the application setup to accommodate the newpricesKeeperparameter in theNewKeeperfunction.Based on the provided output, the review comment stating that all calls to
NewKeepershould be updated to pass the newpricesKeeperargument has been addressed in the application setup.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to NewKeeper include the pricesKeeper argument. ast-grep --lang go --pattern $'NewKeeper($_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 118
Script:
#!/bin/bash # Search for all instances of "NewKeeper" to verify the number of arguments passed in each call. rg 'NewKeeper\('Length of output: 3765
Script:
#!/bin/bash # Extract the specific lines from keeper.go and app.go where NewKeeper is called in the clob module. rg 'NewKeeper\(' protocol/x/clob/keeper/keeper.go rg 'NewKeeper\(' protocol/app/app.goLength of output: 1577
Script:
#!/bin/bash # Extract the full call to NewKeeper in the clob module from the protocol/app/app.go file. rg -A 10 'app.ClobKeeper = clobmodulekeeper.NewKeeper\(' protocol/app/app.goLength of output: 457
Script:
#!/bin/bash # Extract the full call to NewKeeper in the clob module from the protocol/app/app.go file, ensuring we capture all arguments. rg -A 20 'app.ClobKeeper = clobmodulekeeper.NewKeeper\(' protocol/app/app.goLength of output: 676
Script:
#!/bin/bash # Extract more lines following the 'app.ClobKeeper = clobmodulekeeper.NewKeeper(' call to ensure we capture all arguments. rg -A 30 'app.ClobKeeper = clobmodulekeeper.NewKeeper\(' protocol/app/app.goLength of output: 969
protocol/testutil/keeper/clob.go (2)
144-144: The
PricesKeeperis correctly added as a parameter to theNewClobKeepersTestContextWithUninitializedMemStorefunction. Ensure that the test cases using this function are updated to provide a mock or stub implementation of thePricesKeeper.181-181: The
createClobKeeperfunction now includes apricesKeeperparameter. This is consistent with the changes in the mainkeeper.gofile. Ensure that the tests are updated to pass this new parameter when creating aClobKeeper.Verification successful
The review comment is correct. The
createClobKeeperfunction in the tests has been updated to include thepricesKeeperparameter, as verified by the script output.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the createClobKeeper function is called with the pricesKeeper argument in tests. ast-grep --lang go --pattern $'createClobKeeper($_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_, $_)'Length of output: 1126
protocol/testutil/constants/subaccounts.go (2)
286-292: The addition of
Carl_Num1_100000USDsubaccount declaration seems correct and consistent with the existing pattern of subaccount declarations in the file.523-529: The addition of
Dave_Num1_500000USDsubaccount declaration also follows the established pattern and appears to be correctly implemented.protocol/x/clob/keeper/untriggered_conditional_orders.go (8)
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [1-19]
The introduction of the
UntriggeredConditionalOrdersstruct and its associated methods appears to be well-structured and follows Go conventions. The struct is documented, explaining its purpose and current implementation details. The TODO comment suggests a future improvement to optimize runtime performance by using a priority queue instead of a list.
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [20-32]
The
NewUntriggeredConditionalOrdersfunction is straightforward and initializes the struct with empty slices for both order types. This is a standard factory pattern in Go for creating new instances of a struct.
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [33-38]
The
IsEmptymethod is a simple and efficient way to check if there are any untriggered orders. It follows best practices by providing a clear and concise utility function for the struct.
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [39-76]
The
AddUntriggeredConditionalOrdersmethod adds new conditional orders to the in-memory structure. It correctly filters out cancelled or expired orders. The use ofpanicfor error handling is consistent with Go's approach when a program reaches an unrecoverable state. However, it's important to ensure that this panic won't be triggered during normal operation, as it would crash the entire application.Verification successful
The verification process has examined the usage of the
AddUntriggeredConditionalOrdersfunction and the conditions under which it might trigger a panic. The codebase shows that there are checks in place to ensure that only valid, uncancelled, and unexpired conditional order IDs are processed, and the memstore is actively updated with order placements corresponding to these IDs. While this suggests that the panic condition is unlikely to be encountered during normal operation, it cannot be definitively guaranteed without a comprehensive system review and testing. The reviewer's caution regarding the use ofpanicis appropriate, and maintainers should be vigilant about this aspect of the system's error handling.* 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [77-104]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the panic condition in AddUntriggeredConditionalOrders is not encountered during normal operation. # This script checks for any instances where the panic might be triggered. rg --type go "AddUntriggeredConditionalOrders" --context 5Length of output: 4239
Script:
#!/bin/bash # Check the implementation of GetUntriggeredConditionalOrderPlacement and any related error handling or consistency checks. rg --type go "GetUntriggeredConditionalOrderPlacement" --context 5Length of output: 10348
Script:
#!/bin/bash # Look for the logic that handles the addition of order IDs to the list of newly-placed conditional orders # and the updating of the memstore with order placements. rg --type go "placedConditionalOrderIds" --context 5 rg --type go "memStore.Set" --context 5Length of output: 7036
The
AddUntriggeredConditionalOrdermethod correctly categorizes orders into the appropriate slices based on their type and direction. The use ofMustBeConditionalOrderensures that only conditional orders are added, which is a good defensive programming practice.
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [105-181]
The
PruneUntriggeredConditionalOrdersmethod removes expired and cancelled orders. The use ofpanicfor error handling in case of duplicate order IDs is again noted. The method's logic is sound, but it's crucial to ensure that the panic condition is not a common occurrence.
- 286-344: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [182-238]
The
RemoveUntriggeredConditionalOrdersmethod is responsible for removing a list of order IDs from theUntriggeredConditionalOrdersdata structure. The method includes a panic condition if the order IDs involve more than oneClobPairId, which is a critical consistency check. The logic for removing orders is clear and follows Go best practices.
- 286-344: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [239-340]
The
PollTriggeredConditionalOrdersandMaybeTriggerConditionalOrdersmethods contain the core logic for triggering conditional orders based on oracle and traded prices. The logic is complex and involves several steps, including price clamping and event emission. It's important to ensure that the logic for triggering orders is correct and that the rounding for subticks is done as intended. The use ofpanicin error handling should be carefully considered, as it could lead to application crashes if not managed properly.Verification successful
Reviewing
PollTriggeredConditionalOrdersMethodThe
PollTriggeredConditionalOrdersmethod is responsible for removing triggered conditional orders from theUntriggeredConditionalOrdersstruct given a new oracle price for aclobPairId. It returns a list of order ids that were triggered. The method uses pessimistic rounding for subticks, rounding up for LTE (less than or equal to) and down for GTE (greater than or equal to) conditions, which is intended to ensure that orders are only triggered when they are definitely triggerable.Reviewing
MaybeTriggerConditionalOrdersMethodThe
MaybeTriggerConditionalOrdersmethod queries the prices module for price updates and triggers any conditional orders that can be triggered. It moves the stateful order placement from untriggered to triggered state and emits an event for each triggered order. The method is intended to return a sorted list of conditional order ids that were triggered, which is used in theEndBlocker.The use of
panicin this method is notable. Whilepaniccan be appropriate for unrecoverable errors, its use should be carefully considered, especially in a blockchain context where a panic could halt the entire chain. The method should have robust error handling to prevent such scenarios.Next Steps
To verify the correctness of the logic and rounding, it would be beneficial to see the implementation of the
CanTriggermethod called withinPollTriggeredConditionalOrders, as well as theBigRatRoundfunction from thelibpackage to understand the rounding mechanism.For the
MaybeTriggerConditionalOrdersmethod, it would be useful to examine the error handling more closely, particularly the conditions under which apanicis triggered.Based on the review comment and the provided context, the next step is to generate scripts to examine the
CanTriggermethod and theBigRatRoundfunction, as well as to look at the error handling in more detail.
Confirming
CanTriggerMethod LogicThe
CanTriggermethod checks if a conditional order can be triggered based on the subticks value. It ensures that the order is a conditional order and then compares the order's trigger subticks with the provided subticks value. The logic for triggering orders based on the condition type and whether the order is a buy or sell seems to be consistent with typical trading logic, where take profit and stop loss conditions are triggered based on the price moving in specific directions.Verifying
BigRatRoundFunction RoundingThe
BigRatRoundfunction performs rounding of a*big.Ratto the nearest*big.Int. If there is a non-zero remainder after division, and theroundUpflag is true, it rounds up by adding 1 to the result. This behavior is consistent with the description of pessimistic rounding mentioned in thePollTriggeredConditionalOrdersmethod, where rounding up for LTE and down for GTE conditions is intended to ensure that orders are only triggered when they are definitely triggerable.Assessing Use of
panicfor Error HandlingThe use of
panicin theuntriggered_conditional_orders.gofile is extensive. Whilepaniccan be appropriate for handling unrecoverable errors, its use in a blockchain context should be carefully considered due to the potential impact on the entire chain. The code should have robust error handling to gracefully handle errors without causing a chain halt.Conclusion
Based on the provided context and the output from the scripts, the logic for triggering orders and the rounding for subticks within the
PollTriggeredConditionalOrdersmethod appears to be correct. The use ofpanicfor error handling is present in the code, which could be a concern for robustness, but without further context on the overall error handling strategy of the application, it's difficult to definitively assess its appropriateness.The review comment is correct in highlighting the importance of ensuring the logic for triggering orders is correct and that the rounding for subticks is done as intended. The use of
panicshould be carefully considered, and it may be worth discussing with the development team whether there are more graceful ways to handle errors.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of the logic for triggering orders and the rounding for subticks. # This script checks for any instances where the logic might be incorrect or the rounding might not work as intended. rg --type go "PollTriggeredConditionalOrders" --context 10 rg --type go "MaybeTriggerConditionalOrders" --context 10Length of output: 15734
Script:
#!/bin/bash # Examine the CanTrigger method and the BigRatRound function to verify the logic and rounding. rg --type go "func \(.*Order\) CanTrigger" --context 10 rg --type go "func BigRatRound" --context 10 # Look at the error handling in the MaybeTriggerConditionalOrders method in more detail. rg --type go "panic" protocol/x/clob/keeper/untriggered_conditional_orders.goLength of output: 3532
protocol/x/clob/keeper/process_single_match.go (1)
- 457-459: The addition of the line to update the last trade price for the perpetual is a critical part of the new feature implementation. It's important to ensure that this update is consistent with the rest of the system and that the updated price is used correctly in subsequent calculations or state changes.
protocol/testutil/constants/stateful_orders.go (4)
853-866: The addition of
ConditionalOrder_Alice_Num0_Id0_Clob0_Buy1BTC_Price50000_GTBT10_TP_49750introduces a new conditional order with a take profit condition. Ensure that theConditionalOrderTriggerSubticksvalue aligns with the intended business logic for triggering the order.938-951: The new conditional order
ConditionalOrder_Alice_Num0_Id0_Clob0_Buy1BTC_Price50000_GTBT10_SL_50250specifies a stop loss condition. It is important to verify that theConditionalOrderTriggerSubticksvalue is set correctly according to the desired trigger price for the stop loss.980-993: The addition of
ConditionalOrder_Bob_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10_TP_50250introduces a new take profit order for Bob. Confirm that theConditionalOrderTriggerSubticksis consistent with the expected trigger price for the take profit condition.994-1007: The new conditional order
ConditionalOrder_Bob_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10_SL_49750has a stop loss condition. Double-check that theConditionalOrderTriggerSubticksvalue is set to the correct trigger price for the stop loss to function as intended.protocol/app/app.go (1)
- 935-935: The addition of
app.PricesKeeperto theNewfunction as a parameter is a significant change. It's important to ensure that all calls to this function across the codebase have been updated to include this new parameter. Additionally, the initialization and usage ofPricesKeeperwithin theAppstruct should be carefully reviewed to ensure it aligns with the application's architecture and design principles.protocol/testutil/constants/orders.go (2)
816-828: The addition of
Order_Carl_Num1_Id0_Clob0_Buy1BTC_Price50003_GTB10andOrder_Carl_Num1_Id0_Clob0_Buy1BTC_Price50500_GTB10seems consistent with the existing pattern of order declarations. TheGoodTilBlockvalue is set to 10 for both, which aligns with the PR objectives of testing orders with aGoodTilBlockvalue of 10. TheQuantumsandSubticksvalues are also consistent with the BTC price and quantity format used in other order declarations.979-983: The order
Order_Dave_Num1_Id0_Clob0_Sell1BTC_Price49500_GTB10follows the established pattern for order declarations, including theGoodTilBlockvalue of 10, which is part of the PR objectives. TheQuantumsvalue corresponds to 1 BTC, and theSubticksvalue corresponds to the price, both of which are formatted correctly.protocol/x/clob/e2e/conditional_orders_test.go (2)
961-1946: The test function
TestConditionalOrder_TriggeringUsingMatchedPriceis well-structured with clear test cases and expected outcomes. It's important to ensure that the test cases cover all the scenarios for the new feature of triggering conditional orders using the traded prices within a block. The use of constants for subaccounts and orders helps maintain readability and consistency.1948-2078: The test execution logic within the
t.Runfunction is comprehensive, covering the creation of orders, advancing blocks, and checking the state of orders after each block. The assertions are correctly checking the expected state of orders and subaccounts. It's crucial that the test cases are exhaustive and that the assertions accurately reflect the intended behavior of the system after the changes.
| @@ -958,6 +958,1125 @@ func TestConditionalOrder(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestConditionalOrder_TriggeringUsingMatchedPrice(t *testing.T) { | |||
There was a problem hiding this comment.
mostly replicated from the above e2e tests.
only difference is that instead of price updates, updated to use match price.
| 4: {constants.ConditionalOrder_Alice_Num0_Id0_Clob0_Buy1BTC_Price50000_GTBT10_TP_49995.OrderId: false}, | ||
| }, | ||
| }, | ||
| "TakeProfit/Buy conditional order is placed and not triggered by matched price (bounded)": { |
There was a problem hiding this comment.
for the first 12 tests (4 untriggered, 4 trigger immediately, 4 trigger in later blocks), added a corresponding test using bounded match prices
| lowerBound, | ||
| upperBound, | ||
| ) | ||
| triggeredOrderIds := untriggeredConditionalOrders.PollTriggeredConditionalOrders(clampedTradePrice) |
There was a problem hiding this comment.
I haven't quantified the performance but running PollTriggeredConditionalOrders 3x is probably less ideal than running it once.
What do you think about having one PollTriggeredConditionalOrders(oraclePrice, clampedMin, clampedMax), and then modifying PollTriggeredConditionalOrders to trigger GTE withmax(oraclePrice, clampedMin, clampedMax) and LTE with min(oraclePrice, clampedMin, clampedMax)? Alternatively PollTriggeredConditionalOrders can take in (max_trigger_price, min_trigger_price) with max and min being defined in the sentence before
We can also punt this to later if you want.
There was a problem hiding this comment.
i feel pretty similarly, I think PollTriggeredConditionalOrders(oraclePrice, clampedMin, clampedMax) would be nice
There was a problem hiding this comment.
hmm I agree. I thought it was okay because we probably only have a few thousand conditional orders, so I didn't think it would make too much of a difference. but looking at the CPU profile, conditional order triggering does account for 60% of the CLOB Endblocker latency (still a small amount though).
PollTriggeredConditionalOrders(oraclePrice, clampedMin, clampedMax) might not be the best solution since max/min prices do not necessarily exist (they are in the transient store). maybe sorting is the way to go?
let me address this in a separate PR since this PR is pretty huge. don't think this is release blocking unless you guys have a strong preference after looking at the profiles
| ordersForSecondBlock: []clobtypes.Order{ | ||
| // Create a match with price $50,500. | ||
| // This price can trigger the conditional order if unbounded. | ||
| // The bounded price is $50,000 + $50,000 * 0.4% = $50,200, which would not trigger the conditional order. |
There was a problem hiding this comment.
How did you get 0.4%? const ConditionalOrderTriggerMultiplier uint64 = 5, which means that marketParam.MinPriceChangePpm has to be 0.08% = 0.0008 = 800?
There was a problem hiding this comment.
ah good catch - i meant to say 0.5% (0.1% * 5), but just double checked the test constants that we use, min_price_change_ppm = 50 😢
created a new test constant (min_price_change_ppm = 1k) and updated tests and comments.
|
|
||
| // ConditionalOrderTriggerMultiplier represents the multiplier used to calculate the upper and lower bounds of | ||
| // the trigger price for a conditional order. | ||
| const ConditionalOrderTriggerMultiplier uint64 = 5 |
There was a problem hiding this comment.
how was this value picked? I know this change is coming from someone experiencing an issue where their order does not trigger (or maybe triggers early), would using this value have prevented the issue properly?
There was a problem hiding this comment.
maybe a comment explaining the value selection could be helpful
There was a problem hiding this comment.
research came up with this constant empirically. initially we were just going to use ppm but they felt like it was too tight to make any meaningful differences.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/testutil/constants/stateful_orders.go (3 hunks)
- protocol/x/clob/e2e/conditional_orders_test.go (2 hunks)
- protocol/x/clob/types/constants.go (1 hunks)
Files not summarized due to errors (1)
- protocol/x/clob/e2e/conditional_orders_test.go: Error: Message exceeds token limit
Additional comments: 7
protocol/x/clob/types/constants.go (2)
7-9: The definition of
ShortBlockWindowis clear and seems to be unchanged. It's important to ensure that this value is consistent with the protocol's requirements for order validity.11-13: The
StatefulOrderTimeWindowconstant is set to 95 days. This is a significant duration and should be validated against the protocol's design to ensure that it aligns with the intended order lifecycle.protocol/testutil/constants/stateful_orders.go (4)
853-866: The new conditional order declaration
ConditionalOrder_Alice_Num0_Id0_Clob0_Buy1BTC_Price50000_GTBT10_TP_49700specifies aGoodTilBlockTimeof 10 and aConditionalOrderTriggerSubticksof 49,700,000,000. Ensure that these values align with the intended business logic and that the trigger subticks represent a valid and expected price movement for a take profit condition.938-950: The new conditional order declaration
ConditionalOrder_Alice_Num0_Id0_Clob0_Buy1BTC_Price50000_GTBT10_SL_50300specifies aGoodTilBlockTimeof 10 and aConditionalOrderTriggerSubticksof 50,300,000,000. As with the previous order, verify that the trigger subticks are set correctly for a stop loss condition and that theGoodTilBlockTimeis consistent with the desired order lifetime.980-993: The new conditional order declaration
ConditionalOrder_Bob_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10_TP_50300is for a sell order with a take profit condition. It is crucial to confirm that theConditionalOrderTriggerSubticksvalue of 50,300,000,000 is appropriate for triggering a take profit condition on a sell order and that there are no logical inconsistencies with the order's side and condition type.994-1007: The new conditional order declaration
ConditionalOrder_Bob_Num0_Id0_Clob0_Sell1BTC_Price50000_GTBT10_SL_49700is for a sell order with a stop loss condition. Double-check that theConditionalOrderTriggerSubticksvalue of 49,700,000,000 is correctly set for a stop loss condition on a sell order and that theGoodTilBlockTimealigns with the protocol's requirements for order expiration.protocol/x/clob/e2e/conditional_orders_test.go (1)
- 13-13: Ensure that all imported packages are used within the test file to avoid unnecessary dependencies which can lead to confusion and slower compilation times.
Verification successful
The output from the shell scripts indicates that all the imported packages in the
conditional_orders_test.gofile are indeed used within the file. Eachrgcommand searched for matches of the package names within the file, and all commands returned the file path, confirming their usage.Based on this information, the review comment suggesting to ensure that all imported packages are used within the test file is addressed, and the imports are verified to be correctly utilized.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the imported packages are used in the conditional_orders_test.go file rg --type go --files-with-matches 'github.com/dydxprotocol/v4-chain/protocol/lib' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'testapp' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'constants' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'exchange_config' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'encoding' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'clobtypes' protocol/x/clob/e2e/conditional_orders_test.go rg --type go --files-with-matches 'feetiertypes' protocol/x/clob/e2e/conditional_orders_test.goLength of output: 1021
protocol/x/clob/types/constants.go
Outdated
| // ConditionalOrderTriggerMultiplier represents the multiplier used to calculate the upper and lower bounds of | ||
| // the trigger price for a conditional order. | ||
| // The upper bound is calculated as: (1 + min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | ||
| // The lower bound is calculated as: (1 - min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | ||
| const ConditionalOrderTriggerMultiplier uint64 = 5 |
There was a problem hiding this comment.
The ConditionalOrderTriggerMultiplier is set to 5. Based on the previous comments, this value was empirically determined. It would be beneficial to include a comment explaining the rationale behind choosing this specific multiplier to provide context for future maintainers.
+ // ConditionalOrderTriggerMultiplier is empirically set to 5 to provide a balance between sensitivity
+ // and meaningful price movement for triggering conditional orders.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.
| // ConditionalOrderTriggerMultiplier represents the multiplier used to calculate the upper and lower bounds of | |
| // the trigger price for a conditional order. | |
| // The upper bound is calculated as: (1 + min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | |
| // The lower bound is calculated as: (1 - min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | |
| const ConditionalOrderTriggerMultiplier uint64 = 5 | |
| // ConditionalOrderTriggerMultiplier represents the multiplier used to calculate the upper and lower bounds of | |
| // the trigger price for a conditional order. | |
| // The upper bound is calculated as: (1 + min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | |
| // The lower bound is calculated as: (1 - min_price_change_ppm / 1_000_000 * ConditionalOrderTriggerMultiplier) * orderPrice | |
| // ConditionalOrderTriggerMultiplier is empirically set to 5 to provide a balance between sensitivity | |
| // and meaningful price movement for triggering conditional orders. | |
| const ConditionalOrderTriggerMultiplier uint64 = 5 |
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/types/constants.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/types/constants.go
…block
Changelist
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.