Fix vault orderbook flickering #1638
Conversation
This reverts commit 83cc892.
WalkthroughThe recent updates introduce new parameters and logic adjustments across various components, focusing on internal order handling, event management, and test enhancements. Key changes include adding parameters for internal orders, updating event manager integrations, refining order placement and cancellation logic, and enhancing test cases for better validation. Changes
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: 2
Outside diff range and nitpick comments (2)
protocol/testutil/keeper/vault.go (1)
Line range hint
26-26: Resolve undefined references toinitKeepersandGenesisInitializer.It appears that the
initKeepersfunction andGenesisInitializertype are not defined within this file or the imports. You might need to define these or ensure they are correctly imported if they are located in another part of the codebase.Also applies to: 32-32, 35-35
indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (1)
1090-1094: Ensure consistent logging for error conditions.The logging for an error condition related to order quantums is handled well. However, consider adding similar detailed logging for other potential error scenarios within the order handling logic to maintain consistency and aid in debugging.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (16)
- .github/workflows/indexer-build-and-push-dev-staging.yml (1 hunks)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- indexer/packages/redis/src/scripts/place_order.lua (1 hunks)
- indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (2 hunks)
- indexer/services/vulcan/src/handlers/order-place-handler.ts (4 hunks)
- protocol/app/app.go (1 hunks)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/testutil/keeper/vault.go (1 hunks)
- protocol/x/clob/keeper/msg_server_cancel_orders.go (3 hunks)
- protocol/x/clob/keeper/msg_server_place_order.go (2 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/vault/keeper/keeper.go (2 hunks)
- protocol/x/vault/keeper/orders.go (6 hunks)
- protocol/x/vault/keeper/orders_test.go (3 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (2)
- .github/workflows/indexer-build-and-push-dev-staging.yml
- .github/workflows/protocol-build-and-push.yml
Additional context used
golangci-lint
protocol/testutil/keeper/vault.go
26-26: undefined: initKeepers (typecheck)
32-32: undefined: GenesisInitializer (typecheck)
35-35: undefined: GenesisInitializer (typecheck)
Additional comments not posted (17)
protocol/testutil/keeper/vault.go (1)
61-61: Addition ofIndexerEventManagerto theKeeperconstructor aligns with the PR's objectives to enhance order handling.protocol/x/vault/types/expected_keepers.go (1)
26-26: Addition ofisInternalOrderparameter toHandleMsgCancelOrderenhances order handling specificity.protocol/x/vault/keeper/keeper.go (1)
24-24: The integration ofIndexerEventManagerinto theKeeperstructure and its exposure throughGetIndexerEventManagermethod are well-implemented and align with the PR's focus on improving event management.Also applies to: 37-37, 48-48, 53-55
indexer/packages/redis/src/scripts/place_order.lua (1)
69-69: Changing the order data suffix from_falseto_trueinplace_order.luascript aligns with the PR's objectives to refine order handling and should ensure more accurate order state management.protocol/x/clob/types/clob_keeper.go (1)
49-49: Parameter addition toHandleMsgCancelOrdermethod enhances order handling flexibility.Verification successful
The verification confirms that the
isInternalOrderparameter has been correctly integrated into all instances of theHandleMsgCancelOrdermethod across the relevant files.
protocol/x/vault/types/expected_keepers.goprotocol/x/vault/keeper/orders.goprotocol/x/clob/types/clob_keeper.goprotocol/x/clob/keeper/msg_server_cancel_orders.goprotocol/mocks/ClobKeeper.goAll instances include the new parameter, ensuring consistent handling of internal and external orders.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `isInternalOrder` parameter across the system. # Test: Search for the method usage. Expect: All usages include the new parameter. rg --type go $'HandleMsgCancelOrder'Length of output: 945
Script:
#!/bin/bash # Verify the integration of `isInternalOrder` parameter in each instance of `HandleMsgCancelOrder`. # Check `protocol/x/vault/types/expected_keepers.go` echo "Checking protocol/x/vault/types/expected_keepers.go" rg --context 5 --type go 'HandleMsgCancelOrder' protocol/x/vault/types/expected_keepers.go # Check `protocol/x/vault/keeper/orders.go` echo "Checking protocol/x/vault/keeper/orders.go" rg --context 5 --type go 'HandleMsgCancelOrder' protocol/x/vault/keeper/orders.go # Check `protocol/x/clob/types/clob_keeper.go` echo "Checking protocol/x/clob/types/clob_keeper.go" rg --context 5 --type go 'HandleMsgCancelOrder' protocol/x/clob/types/clob_keeper.go # Check `protocol/x/clob/keeper/msg_server_cancel_orders.go` echo "Checking protocol/x/clob/keeper/msg_server_cancel_orders.go" rg --context 5 --type go 'HandleMsgCancelOrder' protocol/x/clob/keeper/msg_server_cancel_orders.go # Check `protocol/mocks/ClobKeeper.go` echo "Checking protocol/mocks/ClobKeeper.go" rg --context 5 --type go 'HandleMsgCancelOrder' protocol/mocks/ClobKeeper.goLength of output: 3174
protocol/x/clob/keeper/msg_server_cancel_orders.go (1)
40-40: Enhanced order cancellation handling withisInternalOrderparameter and conditional event emission.Also applies to: 115-127
protocol/x/clob/keeper/msg_server_place_order.go (1)
91-99: Conditional logic for order replacements and event emissions based onisInternalOrderparameter enhances system integrity.Also applies to: 120-131, 137-148
Verification successful
To ensure the conditional logic based on the
isInternalOrderparameter is correctly implemented and behaves as expected, we need to review the implementation ofHandleMsgPlaceOrderand its usage in tests.
To ensure the conditional logic based on the
isInternalOrderparameter is correctly tested, we need to extract the test cases forHandleMsgPlaceOrder.
The implementation of
HandleMsgPlaceOrderincludes conditional logic based on theisInternalOrderparameter, and the test cases for this function are present inprotocol/x/clob/keeper/msg_server_place_order_test.go. The tests cover scenarios with bothtrueandfalsevalues forisInternalOrder, ensuring that the conditional logic is exercised.
protocol/x/clob/keeper/msg_server_place_order_test.go:func TestHandleMsgPlaceOrder(t *testing.T)The test cases confirm that the conditional logic for
isInternalOrderis correctly implemented and tested.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditional logic of `isInternalOrder` in order replacements and event emissions. # Test: Search for the method usage with different parameter values. Expect: Correct conditional logic. rg --type go $'HandleMsgPlaceOrder'Length of output: 1133
Script:
#!/bin/bash # Description: Verify the implementation and test coverage of `HandleMsgPlaceOrder` with `isInternalOrder` parameter. # Extract the implementation of `HandleMsgPlaceOrder` ast-grep --lang go --pattern $'func (k Keeper) HandleMsgPlaceOrder($_, $_, $_) $_' # Extract test cases for `HandleMsgPlaceOrder` rg --type go 'HandleMsgPlaceOrder(' --context 5Length of output: 10451
Script:
#!/bin/bash # Description: Verify the test coverage of `HandleMsgPlaceOrder` with `isInternalOrder` parameter. # Extract test cases for `HandleMsgPlaceOrder` rg --type go 'HandleMsgPlaceOrder\(' --context 5Length of output: 6078
indexer/services/vulcan/src/handlers/order-place-handler.ts (1)
171-231: Addition ofupdatePriceLevelmethod enhances dynamic price level adjustments based on order placements.Verification successful
The
updatePriceLevelmethod is well-integrated and tested across multiple files, ensuring its functionality and behavior are validated.
indexer/services/vulcan/src/handlers/order-place-handler.tsindexer/services/vulcan/src/handlers/order-update-handler.tsindexer/services/vulcan/src/handlers/order-remove-handler.ts- Various test files in
indexer/services/vulcan/__tests__andindexer/packages/redis/__tests__Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration and behavior of `updatePriceLevel` method. # Test: Search for the method usage. Expect: Correct integration and behavior. rg --type ts $'updatePriceLevel'Length of output: 11743
protocol/x/vault/keeper/orders.go (3)
10-11: LGTM! Good use of logging for error handling and conditions to skip unnecessary operations.Also applies to: 31-31
375-377: LGTM! The function is well-documented and handles errors appropriately.
87-89: Verify the use ofisInternalOrderflag across the system.Also applies to: 93-97, 110-111, 121-122, 123-124, 125-126, 127-128, 129-130, 131-132, 133-134, 135-136, 137-138, 139-140, 141-142, 143-144, 145-146, 147-148
Verification successful
The
isInternalOrderflag is used consistently across multiple files in the system. Here are the relevant locations:
protocol/x/vault/types/expected_keepers.goprotocol/x/clob/types/clob_keeper.goprotocol/x/clob/keeper/msg_server_place_order_test.goprotocol/x/clob/keeper/msg_server_cancel_orders.goprotocol/x/clob/keeper/msg_server_place_order.goprotocol/x/clob/keeper/orders.goprotocol/mocks/ClobKeeper.goThe usage appears to be consistent and correctly implemented across these files.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the use of `isInternalOrder` flag across the system. # Test: Search for the usage of `isInternalOrder` flag. Expect: Consistent usage across all relevant files. rg --type go $'isInternalOrder'Length of output: 3199
protocol/x/clob/keeper/msg_server_place_order_test.go (1)
438-438: LGTM! Comprehensive test coverage for various scenarios.protocol/x/vault/keeper/orders_test.go (4)
Line range hint
1-1: LGTM! The test scenarios inTestRefreshAllVaultOrdersare comprehensive and well-structured.
Line range hint
1-1: LGTM! The test scenarios inTestRefreshVaultClobOrderseffectively cover both success and error cases.
Line range hint
1-1: LGTM! The test scenarios inTestGetVaultClobOrdersare comprehensive and well-structured.
688-716: LGTM! The test scenarios inTestGetVaultClobOrderClientIdcorrectly validate the generation of client IDs using bitwise operations.protocol/app/app.go (1)
1091-1091: Ensure proper initialization and usage ofIndexerEventManager.Verification successful
The
IndexerEventManageris properly initialized and used throughout the application. The initialization is done usingindexer_manager.NewIndexerEventManager, and it is utilized in various places within theprotocol/app/app.gofile and other related files.
- Initialization in
protocol/app/app.go:
app.IndexerEventManager = indexer_manager.NewIndexerEventManager(...)- Usage in
protocol/app/app.go:
app.IndexerEventManager.ProduceBlock(ctx)app.IndexerEventManager.SendOnchainData(block)app.IndexerEventManager.ClearEvents(ctx)The initialization and usage of
IndexerEventManagerappear to be correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `IndexerEventManager` is properly initialized and used throughout the application. # Test: Search for initialization and usage of `IndexerEventManager`. rg --type go 'IndexerEventManager'Length of output: 40911
| // After replacing the order the quantums at the price level of the old order should be: | ||
| // initial quantums - (old order quantums - old order total filled) | ||
| const expectedPriceLevelQuantums: number = ( | ||
| oldPriceLevelInitialQuantums - (Number(initialOrderToPlace.quantums) - oldOrderTotalFilled) | ||
| ); | ||
| const expectedPriceLevel: PriceLevel = { | ||
| humanPrice: expectedRedisOrder.price, | ||
| quantums: oldPriceLevelInitialQuantums.toString(), | ||
| quantums: expectedPriceLevelQuantums.toString(), |
There was a problem hiding this comment.
Refactor to simplify the calculation of expectedPriceLevelQuantums.
The calculation of expectedPriceLevelQuantums can be simplified for better readability. Consider using a helper function to encapsulate the logic, which can also be reused in other tests if needed.
- const expectedPriceLevelQuantums: number = (
- oldPriceLevelInitialQuantums - (Number(initialOrderToPlace.quantums) - oldOrderTotalFilled)
- );
+ const expectedPriceLevelQuantums: number = calculateNetQuantums(oldPriceLevelInitialQuantums, initialOrderToPlace.quantums, oldOrderTotalFilled);
+ function calculateNetQuantums(initialQuantums, orderQuantums, totalFilled) {
+ return initialQuantums - (Number(orderQuantums) - totalFilled);
+ }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.
| // After replacing the order the quantums at the price level of the old order should be: | |
| // initial quantums - (old order quantums - old order total filled) | |
| const expectedPriceLevelQuantums: number = ( | |
| oldPriceLevelInitialQuantums - (Number(initialOrderToPlace.quantums) - oldOrderTotalFilled) | |
| ); | |
| const expectedPriceLevel: PriceLevel = { | |
| humanPrice: expectedRedisOrder.price, | |
| quantums: oldPriceLevelInitialQuantums.toString(), | |
| quantums: expectedPriceLevelQuantums.toString(), | |
| // After replacing the order the quantums at the price level of the old order should be: | |
| // initial quantums - (old order quantums - old order total filled) | |
| const expectedPriceLevelQuantums: number = calculateNetQuantums(oldPriceLevelInitialQuantums, initialOrderToPlace.quantums, oldOrderTotalFilled); | |
| function calculateNetQuantums(initialQuantums, orderQuantums, totalFilled) { | |
| return initialQuantums - (Number(orderQuantums) - totalFilled); | |
| } | |
| const expectedPriceLevel: PriceLevel = { | |
| humanPrice: expectedRedisOrder.price, | |
| quantums: expectedPriceLevelQuantums.toString(), |
| func (_m *ClobKeeper) HandleMsgCancelOrder(ctx types.Context, msg *clobtypes.MsgCancelOrder, isInternalOrder bool) error { | ||
| ret := _m.Called(ctx, msg, isInternalOrder) | ||
|
|
||
| if len(ret) == 0 { | ||
| panic("no return value specified for HandleMsgCancelOrder") | ||
| } | ||
|
|
||
| var r0 error | ||
| if rf, ok := ret.Get(0).(func(types.Context, *clobtypes.MsgCancelOrder) error); ok { | ||
| r0 = rf(ctx, msg) | ||
| if rf, ok := ret.Get(0).(func(types.Context, *clobtypes.MsgCancelOrder, bool) error); ok { | ||
| r0 = rf(ctx, msg, isInternalOrder) |
There was a problem hiding this comment.
Tip
Codebase Verification
The new isInternalOrder parameter is not consistently integrated across all usages of HandleMsgCancelOrder. Please update the following:
protocol/x/vault/types/expected_keepers.go: Update the function signature to include theisInternalOrderparameter.protocol/x/clob/types/clob_keeper.go: Update the function signature to include theisInternalOrderparameter.protocol/x/vault/keeper/orders.go: Update the call toHandleMsgCancelOrderto include theisInternalOrderparameter.
Analysis chain
Ensure the new isInternalOrder parameter is integrated correctly across all usages of HandleMsgCancelOrder.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new parameter `isInternalOrder` is used correctly in all calls to `HandleMsgCancelOrder`.
# Test: Search for the function usage. Expect: All occurrences should pass a boolean argument.
rg --type go $'HandleMsgCancelOrder'
Length of output: 945
Script:
#!/bin/bash
# Description: Verify the function signature and calls for HandleMsgCancelOrder in the identified files.
# Check function signature in expected_keepers.go
echo "Checking expected_keepers.go..."
rg --type go 'HandleMsgCancelOrder' protocol/x/vault/types/expected_keepers.go
# Check function signature and calls in orders.go
echo "Checking orders.go..."
rg --type go 'HandleMsgCancelOrder' protocol/x/vault/keeper/orders.go
# Check function signature in clob_keeper.go
echo "Checking clob_keeper.go..."
rg --type go 'HandleMsgCancelOrder' protocol/x/clob/types/clob_keeper.go
# Check function signature and calls in msg_server_cancel_orders.go
echo "Checking msg_server_cancel_orders.go..."
rg --type go 'HandleMsgCancelOrder' protocol/x/clob/keeper/msg_server_cancel_orders.go
# Check function signature and calls in ClobKeeper.go
echo "Checking ClobKeeper.go..."
rg --type go 'HandleMsgCancelOrder' protocol/mocks/ClobKeeper.go
Length of output: 1268
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.Summary by CodeRabbit
New Features
Bug Fixes
Tests