[CT-856] Add order replacement to fix vault causing orderbook flickering #1602
Conversation
WalkthroughThe recent updates introduce modifications and enhancements related to event indexing and order handling across various files. Key changes include the addition of parameters to existing functions for better event management, integration of indexer event manager functionalities, and test adaptations to support these new features. These updates aim to streamline event handling, improve test coverage, and enhance overall code maintainability and observability. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant IndexerEventManager
participant VaultKeeper
participant ClobKeeper
User->>App: Initiate New App
App->>IndexerEventManager: Initialize IndexerEventManager
App->>VaultKeeper: Initialize VaultKeeper
App->>ClobKeeper: Initialize ClobKeeper
User->>ClobKeeper: Cancel Order
ClobKeeper->>ClobKeeper: HandleMsgCancelOrder(isInternalOrder)
ClobKeeper-->>IndexerEventManager: Conditionally Emit Event
User->>ClobKeeper: Place Order
ClobKeeper->>ClobKeeper: HandleMsgPlaceOrder(validate internal/external)
ClobKeeper-->>IndexerEventManager: Conditionally Emit Event based on Order Type
VaultKeeper->>ClobKeeper: RefreshVaultClobOrders
ClobKeeper->>ClobKeeper: HandleMsgCancelOrder(true)
VaultKeeper-->>IndexerEventManager: Emit Indexer Messages based on Order Differences
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: 7
Outside diff range and nitpick comments (1)
protocol/app/app.go (1)
Line range hint
744-744: Secure the gRPC server by adding SSL credentials.- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress) + creds, err := credentials.NewServerTLSFromFile("cert.pem", "cert.key") + if err != nil { + panic(fmt.Sprintf("Failed to generate gRPC credentials: %v", err)) + } + app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(creds)), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- protocol/app/app.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/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 (4 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Additional Context Used
Path-based Instructions (8)
protocol/x/vault/types/expected_keepers.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/keeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/types/clob_keeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/msg_server_cancel_orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/msg_server_place_order.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/app/app.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (14)
protocol/x/vault/types/expected_keepers.go (1)
26-26: AddedisInternalOrderparameter toHandleMsgCancelOrdermethod to differentiate internal and external orders.protocol/x/vault/keeper/keeper.go (3)
24-24: AddedindexerEventManagerto theKeeperstruct to manage indexer events.
37-37: TheNewKeeperfunction now requires anindexerEventManagerparameter, aligning with the structural changes in theKeeperstruct.
53-55: AddedGetIndexerEventManagermethod to retrieve theindexerEventManagerinstance.protocol/x/clob/types/clob_keeper.go (1)
49-49: TheisInternalOrderboolean parameter has been added toHandleMsgCancelOrderto handle internal orders differently.protocol/x/clob/keeper/msg_server_cancel_orders.go (2)
40-40: TheisInternalOrderparameter is now included inHandleMsgCancelOrderto support conditional logic based on the order type.
115-127: Conditional logic added to emit indexer events only for non-internal orders, ensuring internal operations remain unlogged.protocol/x/clob/keeper/msg_server_place_order.go (2)
91-99: Added logic to allow replacement of cancelled orders if they are not internal, enhancing the flexibility of order management.
120-131: Indexer events for new order placements are conditionally added based on whether the order is internal, maintaining the integrity of external data visibility.Also applies to: 137-148
protocol/x/vault/keeper/orders.go (2)
93-93: TheisInternalOrderparameter is used to mark the cancellation of orders as internal, aligning with the new parameter requirements.
121-158: Complex logic for sending indexer messages based on order changes. Ensure that the conditions for sending these messages are thoroughly tested to prevent incorrect data in the indexer.protocol/x/vault/keeper/orders_test.go (2)
705-705: The test case "Buy, Block Height Even, Layer 1" also appears to be missing the block height condition in the test setup.
700-700: The test case "Buy, Block Height Odd, Layer 1" seems to be missing the block height condition in the test setup.protocol/app/app.go (1)
1095-1095: Ensure the newIndexerEventManagerparameter is properly integrated and utilized.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/testutil/keeper/vault.go (1 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (1 hunks)
Additional Context Used
Path-based Instructions (3)
protocol/testutil/keeper/vault.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/msg_server_place_order_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/mocks/ClobKeeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (3)
protocol/testutil/keeper/vault.go (1)
61-61: The integration ofIndexerEventManagerincreateVaultKeeperaligns with the PR's objectives to enhance event indexing capabilities.protocol/x/clob/keeper/msg_server_place_order_test.go (1)
438-438: The new test scenario for handling already cancelled internal orders is correctly implemented and aligns with the PR's objectives to manage order cancellations more robustly.protocol/mocks/ClobKeeper.go (1)
629-638: The updatedHandleMsgCancelOrdermethod correctly includes theisInternalOrderparameter, aligning with changes in the actualClobKeeperinterface.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/protocol-build-and-push.yml (1 hunks)
- protocol/x/vault/keeper/orders.go (7 hunks)
- protocol/x/vault/keeper/orders_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/orders_test.go
Additional Context Used
Path-based Instructions (1)
protocol/x/vault/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments not posted (4)
.github/workflows/protocol-build-and-push.yml (1)
9-9: Added a new branch trigger for CI/CD processes.Verification successful
The branch 'chenyao/order-replacement-messages-for-vault' exists on the remote and is correctly configured in the GitHub Actions workflow.
.github/workflows/protocol-build-and-push.yml: Line 9Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the branch 'chenyao/order-replacement-messages-for-vault' is correctly set up for CI/CD. # Test: Check if the branch exists and has the necessary configuration. git branch --list 'chenyao/order-replacement-messages-for-vault'Length of output: 64
Script:
#!/bin/bash # Description: Verify that the branch 'chenyao/order-replacement-messages-for-vault' exists on the remote and check CI/CD configuration. # Test: Check if the branch exists on the remote. git ls-remote --heads origin 'chenyao/order-replacement-messages-for-vault' # Test: Check the GitHub Actions workflow configuration for the branch. grep -r 'chenyao/order-replacement-messages-for-vault' .github/workflows/Length of output: 347
protocol/x/vault/keeper/orders.go (3)
9-11: Added new imports for event indexing and management.
91-91: TheisInternalOrderparameter is now used to specify whether the order cancellation is internal.
119-156: Enhanced event indexing logic to handle order replacements more effectively.
chenyaoy
changed the title
chenyaoy
changed the title
chenyaoy
changed the title
| @@ -6,6 +6,7 @@ on: # yamllint disable-line rule:truthy | |||
| - main | |||
| - 'release/protocol/v[0-9]+.[0-9]+.x' # e.g. release/protocol/v0.1.x | |||
| - 'release/protocol/v[0-9]+.x' # e.g. release/protocol/v1.x | |||
| - 'chenyao/order-replacement-messages-for-vault' | |||
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/app/app.go (1 hunks)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/vault/keeper/orders_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/app.go
- protocol/mocks/ClobKeeper.go
- protocol/x/clob/types/clob_keeper.go
- protocol/x/vault/keeper/orders_test.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/x/vault/keeper/orders.go (7 hunks)
- protocol/x/vault/types/vault_id.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/orders.go
Additional context used
golangci-lint
protocol/x/vault/types/vault_id.go
14-14: undefined: VaultId (typecheck)
19-19: undefined: VaultId (typecheck)
24-24: undefined: VaultId (typecheck)
39-39: undefined: VaultType_value (typecheck)
51-51: undefined: VaultType (typecheck)
Additional comments not posted (4)
protocol/x/vault/types/vault_id.go (3)
73-80: LGTM! TheIncrCountermethod correctly increments a metric counter using vault labels.
113-115: LGTM! TheaddLabelsmethod correctly appends vault-specific labels to the provided labels.
Line range hint
117-127: LGTM! ThegetVaultLabelsmethod correctly constructs a slice of labels for the vault.protocol/lib/metrics/constants.go (1)
248-256: LGTM! The new constants for vault operations (VaultPlaceOrderDifferentPrice,VaultPlaceOrderSamePrice) are clearly named and serve to differentiate between different scenarios of vault order placements.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/orders.go (2)
9-9: Enhance logging for better operational traceability.Consider adding more detailed logging at key operation points within the
RefreshAllVaultOrdersfunction to aid in debugging and operational monitoring.Also applies to: 30-30
179-179: Improve code documentation for complex calculations.The
GetVaultClobOrdersfunction involves complex calculations. Adding more detailed comments explaining each step would improve code readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders.go (7 hunks)
Additional context used
golangci-lint
protocol/x/vault/keeper/orders.go
22-22: undefined: Keeper (typecheck)
75-75: undefined: Keeper (typecheck)
193-193: undefined: Keeper (typecheck)
Additional comments not posted (2)
protocol/x/vault/keeper/orders.go (2)
412-412: LGTM! Correct implementation of client ID generation.The
GetVaultClobOrderClientIdfunction correctly implements the logic for generating unique client IDs for CLOB orders based on the order side and layer.
414-414: LGTM! Efficient internal order placement.The
PlaceVaultClobOrderfunction efficiently handles the internal placement of vault CLOB orders, aligning with the PR's objectives to streamline order management.
This reverts commit 83cc892.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
protocol/x/vault/keeper/orders.go (1)
Line range hint
91-164: LGTM! The functionRefreshVaultClobOrderseffectively manages the cancellation and placement of orders. Consider optimizing the loop that sends indexer messages to reduce redundancy.- for i, order := range ordersToPlace { + for i := range ordersToPlace { + order := ordersToPlace[i]
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders.go (7 hunks)
Additional context used
golangci-lint
protocol/x/vault/keeper/orders.go
22-22: undefined: Keeper (typecheck)
75-75: undefined: Keeper (typecheck)
178-178: undefined: Keeper (typecheck)
Additional comments not posted (4)
protocol/x/vault/keeper/orders.go (4)
Line range hint
9-42: LGTM! The functionRefreshAllVaultOrdersis well-structured and handles errors and logging appropriately.
Line range hint
164-397: LGTM! The functionGetVaultClobOrdersis complex but well-handled with appropriate error checks and context-rich error messages.
397-399: LGTM! The functionGetVaultClobOrderClientIdcorrectly calculates the client ID for CLOB orders.
399-399: LGTM! The functionPlaceVaultClobOrdereffectively delegates the order placement to the appropriate handler.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
protocol/x/vault/keeper/orders.go (2)
Line range hint
86-152: Refactor to reduce complexity and improve readability.The
RefreshVaultClobOrdersfunction has grown significantly in complexity due to the new logic for handling order replacements. Consider breaking down this function into smaller, more manageable functions. For example, the logic for cancelling and placing orders could be separated into their own functions. This would not only improve readability but also make the code easier to maintain and test.
Line range hint
86-152: Optimize data structures used in order processing.The use of slices for
replacedOrdersand iterating over them can be optimized. Since the size ofreplacedOrdersis known in advance (it matches the size ofordersToCancel), consider pre-allocating the slice with the correct capacity to avoid potential reallocations during the append operations.- replacedOrders := make([]*clobtypes.Order, len(ordersToCancel)) + replacedOrders := make([]*clobtypes.Order, 0, len(ordersToCancel))
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders.go (6 hunks)
Additional context used
golangci-lint
protocol/x/vault/keeper/orders.go
22-22: undefined: Keeper (typecheck)
75-75: undefined: Keeper (typecheck)
166-166: undefined: Keeper (typecheck)
Additional comments not posted (2)
protocol/x/vault/keeper/orders.go (2)
123-151: Ensure consistent event handling for order updates.The logic for sending indexer messages based on the price comparison of old and new orders is crucial for the intended fix of orderbook flickering. It's important to verify that this logic correctly handles all edge cases, especially when orders are not replaced but simply updated. Consider adding more comprehensive tests around this logic to ensure its robustness.
123-151: Validate the logic for incrementing metrics based on order changes.The increment of metrics based on the success of order operations (lines 126-128) is critical for monitoring. Ensure that this metric accurately reflects the operations' outcomes and consider adding automated tests to verify that the metrics are incremented correctly under all expected conditions.
Verification successful
The
RefreshVaultClobOrdersfunction inprotocol/x/vault/keeper/orders.goincludes logic for incrementing metrics usingIncrCounterandIncrCounterWithLabels. The metrics are incremented based on the success of order cancellations and placements, as well as when there are differences in order subticks.
- The function increments the
VaultCancelOrdermetric with success labels when orders are canceled.- It increments the
VaultPlaceOrdermetric with success labels when new orders are placed.- It increments the
VaultPlaceOrderDifferentPricemetric when there are differences in order subticks.These increments appear to reflect the outcomes of the operations accurately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct increment of metrics in `RefreshVaultClobOrders`. # Test: Search for metric increments. Expect: Correct reflection of operation outcomes. rg --type go $'IncrCounter'Length of output: 12713
Script:
#!/bin/bash # Locate the implementation of the `RefreshVaultClobOrders` function. ast-grep --lang go --pattern 'func RefreshVaultClobOrders($_, $_) $_ { $$$ }'Length of output: 79
Script:
#!/bin/bash # List all function definitions in the `protocol/x/vault/keeper/orders.go` file. ast-grep --lang go --pattern 'func $_($_, $_) $_ { $$$ }' protocol/x/vault/keeper/orders.goLength of output: 93
Script:
#!/bin/bash # List all function definitions in the `protocol/x/vault/keeper/orders.go` file without assuming specific parameters. ast-grep --lang go --pattern 'func $_($$$) $_ { $$$ }' protocol/x/vault/keeper/orders.goLength of output: 90
Script:
#!/bin/bash # Search for the `func` keyword in the `protocol/x/vault/keeper/orders.go` file to list all function definitions. rg 'func ' protocol/x/vault/keeper/orders.goLength of output: 312
Script:
#!/bin/bash # Extract the implementation of the `RefreshVaultClobOrders` function. ast-grep --lang go --pattern 'func (k Keeper) RefreshVaultClobOrders(ctx sdk.Context, vaultId types.VaultId) (err error) { $$$ }' protocol/x/vault/keeper/orders.goLength of output: 5912
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (7)
protocol/x/clob/keeper/msg_server_cancel_orders.go (1)
40-40: Ensure to document the purpose of theisInternalOrderparameter to clarify its role in event handling.protocol/app/app.go (6)
Line range hint
383-383: Resolve the undefined functionGetEncodingConfig.- encodingConfig := GetEncodingConfig() + encodingConfig := app.GetEncodingConfig() // Assuming GetEncodingConfig is a method of the App structThe function
GetEncodingConfigis called but not defined in the provided code or standard libraries. If it is intended to be a method of theAppstruct, it should be called accordingly.
Line range hint
498-498: DefinemaccPermsor ensure it is imported correctly.- maccPerms, + app.maccPerms, // Assuming maccPerms is a field or method of the App structThe variable
maccPermsis used but not defined in the provided scope. It needs to be either defined or correctly referenced if it's part of another structure or package.
Line range hint
507-507: Clarify or define the functionBlockedAddresses.- BlockedAddresses(), + app.BlockedAddresses(), // Assuming BlockedAddresses is a method of the App structThe function
BlockedAddressesis called but not defined. If it belongs to theAppstruct or another package, it should be referenced properly.
Line range hint
381-381: Address the undefined functioninitDatadogProfiler.- initDatadogProfiler(logger, appFlags.DdAgentHost, appFlags.DdTraceAgentPort) + app.initDatadogProfiler(logger, appFlags.DdAgentHost, appFlags.DdTraceAgentPort) // Assuming initDatadogProfiler is a method of the App structThe function
initDatadogProfileris not defined in the provided code. If it is a method of theAppstruct, it should be called on an instance ofApp.
Line range hint
652-652: Correct the undefined methodMsgServiceRouter.- app.MsgServiceRouter(), + app.BaseApp.MsgServiceRouter(), // Assuming MsgServiceRouter is a method of the BaseApp embedded structThe method
MsgServiceRouteris called on theAppstruct but appears to be undefined. If it is part of the embeddedBaseAppstruct, it should be accessed accordingly.
Line range hint
774-774: Ensure proper reference toLoggerandGRPCQueryRouter.- app.Logger + app.BaseApp.Logger() // Assuming Logger is a method of the BaseApp embedded struct - app.GRPCQueryRouter + app.BaseApp.GRPCQueryRouter() // Assuming GRPCQueryRouter is a method of the BaseApp embedded structThe methods
LoggerandGRPCQueryRouterare referenced as if they are directly part of theAppstruct, but they are likely part of the embeddedBaseAppstruct. They should be accessed throughBaseApp.Also applies to: 1380-1380, 1386-1386
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- indexer/services/vulcan/tests/handlers/order-replace-handler.test.ts (4 hunks)
- indexer/services/vulcan/src/handlers/order-replace-handler.ts (1 hunks)
- protocol/app/app.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/vault/keeper/orders.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/vulcan/tests/handlers/order-replace-handler.test.ts
- indexer/services/vulcan/src/handlers/order-replace-handler.ts
Additional context used
golangci-lint
protocol/x/clob/keeper/msg_server_cancel_orders.go
20-20: undefined: msgServer (typecheck)
37-37: undefined: Keeper (typecheck)
protocol/x/clob/keeper/msg_server_place_order.go
22-22: undefined: msgServer (typecheck)
40-40: undefined: Keeper (typecheck)
protocol/x/vault/keeper/orders.go
22-22: undefined: Keeper (typecheck)
75-75: undefined: Keeper (typecheck)
173-173: undefined: Keeper (typecheck)
protocol/app/app.go
381-381: undefined: initDatadogProfiler (typecheck)
383-383: undefined: GetEncodingConfig (typecheck)
498-498: undefined: maccPerms (typecheck)
507-507: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter) (typecheck)
515-515: undefined: BlockedAddresses (typecheck)
607-607: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter) (typecheck)
652-652: app.MsgServiceRouter undefined (type *App has no field or method MsgServiceRouter) (typecheck)
774-774: app.Logger undefined (type *App has no field or method Logger) (typecheck)
1380-1380: app.GRPCQueryRouter undefined (type *App has no field or method GRPCQueryRouter) (typecheck)
1386-1386: app.GRPCQueryRouter undefined (type *App has no field or method GRPCQueryRouter) (typecheck)
Additional comments not posted (5)
protocol/x/clob/keeper/msg_server_cancel_orders.go (3)
26-26: LGTM! The function correctly wraps the internal handler with appropriate error handling.
Line range hint
20-20: ThemsgServerandKeepertypes are reported as undefined. This is likely an issue with the static analysis setup and not with your code. Please verify the environment and dependencies used by the static analysis tool.Also applies to: 37-37
114-126: Ensure that the conditional logic for event emission based onisInternalOrderis correct and intentional, as it affects how order cancellations are indexed.Verification successful
The conditional logic for event emission based on
isInternalOrderis correctly implemented and intentional. This ensures that only non-internal orders trigger event emissions.
protocol/x/clob/keeper/msg_server_cancel_orders.goprotocol/x/clob/keeper/msg_server_place_order.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the conditional logic for event emission. # Test: Search for usage of `isInternalOrder` in event emission logic. rg --type python $'isInternalOrder'Length of output: 65
Script:
#!/bin/bash # Description: Verify the conditional logic for event emission. # Test: Search for usage of `isInternalOrder` in event emission logic. rg --type go 'isInternalOrder'Length of output: 3149
protocol/x/clob/keeper/msg_server_place_order.go (1)
Line range hint
22-22: ThemsgServerandKeepertypes are reported as undefined. This is likely an issue with the static analysis setup and not with your code. Please verify the environment and dependencies used by the static analysis tool.Also applies to: 40-40
protocol/x/vault/keeper/orders.go (1)
Line range hint
22-22: TheKeepertype is reported as undefined. This is likely an issue with the static analysis setup and not with your code. Please verify the environment and dependencies used by the static analysis tool.Also applies to: 75-75, 173-173
| @@ -1097,6 +1097,7 @@ func New( | |||
| app.PricesKeeper, | |||
| app.SendingKeeper, | |||
| app.SubaccountsKeeper, | |||
| app.IndexerEventManager, | |||
There was a problem hiding this comment.
Add documentation for IndexerEventManager.
The addition of IndexerEventManager to the App struct is significant, especially given its role in event management. It would be beneficial for maintainability and clarity to add a comment explaining its purpose and usage in the context of the application.
| @@ -95,6 +95,7 @@ func (k Keeper) HandleMsgPlaceOrder( | |||
| order, | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The logic for handling internal and external orders is well-implemented. Consider adding more detailed logging for the steps involved, especially around the conditions that lead to early returns.
Also applies to: 118-146
There was a problem hiding this comment.
@dydxwill, the new line you mentioned has been noted for removal. If there's anything else you need, feel free to let me know!
| @@ -85,7 +88,7 @@ func (k Keeper) RefreshVaultClobOrders(ctx sdk.Context, vaultId types.VaultId) ( | |||
| err := k.clobKeeper.HandleMsgCancelOrder(ctx, clobtypes.NewMsgCancelOrderStateful( | |||
| order.OrderId, | |||
| uint32(ctx.BlockTime().Unix())+orderExpirationSeconds, | |||
| )) | |||
| ), true) | |||
There was a problem hiding this comment.
The logic for refreshing CLOB orders is complex but correctly handles the cancellation and placement of orders. Consider adding more detailed logging for the steps involved, especially around the conditions that lead to early returns.
Also applies to: 107-159
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders.go (4 hunks)
Additional context used
golangci-lint
protocol/x/vault/keeper/orders.go
22-22: undefined: Keeper (typecheck)
75-75: undefined: Keeper (typecheck)
172-172: undefined: Keeper (typecheck)
Additional comments not posted (5)
protocol/x/vault/keeper/orders.go (5)
10-10: Imports for indexer-related packages added as part of the PR to handle new order replacement logic.Also applies to: 11-11
30-30: Error handling is correctly implemented to continue the loop if a vault ID cannot be retrieved.
91-91: The functionHandleMsgCancelOrderis correctly called with the new boolean parameterisInternalOrderset totrue. This aligns with the PR's intention to handle internal order operations differently.
119-119: The logic for sending indexer events has been updated to handle order replacements. Ensure that all conditions for sending these events are met, especially concerning the order indices and the presence of replaced orders.Also applies to: 120-120, 121-121, 122-122, 123-123, 124-124, 125-125, 126-126, 127-127, 128-128, 129-129, 130-130, 131-131, 132-132, 133-133, 134-134, 135-135, 136-136, 137-137, 138-138, 139-139, 140-140, 141-141, 142-142, 143-143, 144-144, 145-145, 146-146, 147-147, 148-148, 149-149, 150-150, 151-151, 152-152, 153-153, 154-154, 155-155, 156-156
Verification successful
The extracted lines from
protocol/x/vault/keeper/orders.goshow that theAddTxnEventmethod is called within specific conditions related to order replacements. The conditions check whether the index is within the bounds ofordersToCanceland whetherreplacedOrderisnil.These conditions appear to be correctly implemented to ensure that indexer events are sent appropriately for order replacements. Therefore, the concern mentioned in the review comment seems to be addressed adequately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the conditions for sending indexer events are correctly implemented. # Test: Search for the usage of `AddTxnEvent` related to order replacement. rg --type go $'AddTxnEvent'Length of output: 8584
Script:
#!/bin/bash # Description: Extract relevant lines from `protocol/x/vault/keeper/orders.go` to verify the conditions for sending indexer events. # Extract lines around the usage of `AddTxnEvent` in `protocol/x/vault/keeper/orders.go` rg --context 5 'AddTxnEvent' protocol/x/vault/keeper/orders.goLength of output: 1013
Line range hint
172-172: Ensure that theKeepertype is correctly defined in the context of this file, as indicated by static analysis.
| indexerevents.NewStatefulOrderRemovalEvent( | ||
| msg.OrderId, | ||
| indexershared.OrderRemovalReason_ORDER_REMOVAL_REASON_USER_CANCELED, | ||
| if !isInternalOrder { |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- indexer/services/vulcan/src/handlers/order-replace-handler.ts (1 hunks)
- protocol/x/clob/keeper/msg_server_cancel_orders.go (3 hunks)
- protocol/x/vault/keeper/orders.go (4 hunks)
- protocol/x/vault/keeper/orders_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- indexer/services/vulcan/src/handlers/order-replace-handler.ts
- protocol/x/clob/keeper/msg_server_cancel_orders.go
Additional comments not posted (12)
protocol/x/vault/keeper/orders.go (7)
10-11: Added imports for indexer-related packages.This change supports the new functionality for emitting indexer events as described in the PR.
91-91: Refactored cancellation of CLOB orders to include indexer event emission.This change aligns with the PR's goal to improve order management by integrating with the indexer system.
108-108: Added a loop for placing new CLOB orders and handling errors.This ensures that each order placement is attempted even if previous ones fail, improving the robustness of order handling.
119-146: Enhanced the logic for sending indexer messages. This includes handling both new order placements and replacements.The logic now correctly handles the scenario where
ordersToCancelandordersToPlaceare expected to have the same length, and it properly emits the appropriate events for new placements or replacements.
30-30: Ensure proper error handling when fetching vault IDs.Proper logging and continuation in the loop prevent the function from failing due to errors in fetching vault IDs.
Line range hint
91-146: The refactoring to include indexer events in the order handling process is well-implemented.The changes are in line with the PR's objectives to enhance the system's responsiveness and reliability by better managing the order lifecycle.
30-30: The logic for constructing orders is complex but appears to be well-structured and error-handled.The detailed calculations and checks ensure that orders are constructed correctly, even under various market conditions.
protocol/x/vault/keeper/orders_test.go (5)
10-13: Imports related to the indexer have been added as expected.
114-118: The test setup now includes the indexer event manager, aligning with the PR's objective to integrate with an indexer event system.
Line range hint
172-184: Added logic to simulate previous orders based on vault shares and asset quantums. This setup is crucial for testing the new order replacement functionality.
197-223: The test case now checks for the generation of expected indexer events, which is a direct response to the changes made in the handling of orders. The detailed setup here ensures that the events are correctly generated when orders are refreshed.
233-238: The final assertion checks that the indexer events emitted are as expected. This is a good practice to ensure that the system's external behaviors (like event generation) align with internal state changes.
| @@ -25,6 +27,7 @@ func (k Keeper) RefreshAllVaultOrders(ctx sdk.Context) { | |||
| defer totalSharesIterator.Close() | |||
| for ; totalSharesIterator.Valid(); totalSharesIterator.Next() { | |||
| vaultId, err := types.GetVaultIdFromStateKey(totalSharesIterator.Key()) | |||
|
|
|||
There was a problem hiding this comment.
Consider adding metrics or more detailed logging for vaults that are skipped.
This could provide better visibility into the operation of this function, especially when diagnosing issues in production environments.
| @@ -95,6 +95,7 @@ func (k Keeper) HandleMsgPlaceOrder( | |||
| order, | |||
| ) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/msg_server_place_order.go (1 hunks)
- protocol/x/vault/keeper/orders_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/keeper/msg_server_place_order.go
- protocol/x/vault/keeper/orders_test.go
Changelist
Vault currently always send an order cancel and order place message. This change instead sends an order replacement message if there is an existing order to cancel, otherwise just send an order place message
Test Plan
I deployed these changes to a dev environment and verified that vault orders do not cause flickering and that the old order is correctly removed when the price changes.
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
IndexerEventManagerin the app configuration.Bug Fixes
Tests
Logging