Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CT-875] Simplify indexer messages for short term order replacement #1593

Merged
merged 18 commits into from
Jun 3, 2024

Conversation

chenyaoy
Copy link
Contributor

@chenyaoy chenyaoy commented May 28, 2024

Changelist

For short term order replacements, we don't actually need to send an order replacement message (and doing so would complicate the logic). In the case where the new order's price is different, we want to keep the existing flow of sending the order cancel before the order place. This does not affect flickering because price changes will always require 2 visual updates (2 websocket messages) to the orderbook. Otherwise, send just the order place message. Indexer's order place handler already handles new orders and replacement orders.

Also changed Indexer's order place handler to not send websocket message if order is replaced because we expect the order update handler to send the correct size right after. This allows us to send a single websocket message for order replacements instead of 2

Test Plan

[Describe how this PR was tested (if applicable)]

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • New Features

    • Enhanced order placement and replacement handling to better manage price changes.
  • Bug Fixes

    • Improved logic for creating order removal messages before placing new orders.
  • Tests

    • Updated test cases to include new parameters for tracking price changes in orders.
    • Adjusted assertions to accommodate changes in off-chain message handling.
  • Chores

    • Added new workflow triggers for specific branches to improve CI/CD pipeline coverage.

Copy link
Contributor

coderabbitai bot commented May 28, 2024

Walkthrough

The recent changes enhance the MemClobPriceTimePriority order placement logic by adding checks for existing orders with different prices to generate order removal messages before placing new ones. Additionally, the CI workflows now trigger on a new branch, and test cases have been updated to reflect the new order handling logic. Code related to updatedQuantums in the OrderPlaceHandler has been removed.

Changes

File Path Change Summary
protocol/x/clob/memclob/memclob.go Updated order placement logic to handle existing orders with different prices, enhancing order replacement handling.
.github/workflows/indexer-build-and-push-dev-staging.yml, .github/workflows/protocol-build-and-push.yml Added new branch trigger chenyao/short-term-order-replacement-indexer-messages for workflow execution.
protocol/x/clob/memclob/memclob_place_order_reduce_only_test.go Added false as an argument in two function calls within specific tests.
protocol/x/clob/memclob/memclob_place_order_test.go Added expectedReplacementOrderPriceChanged variable and modified test cases to include this parameter.
protocol/x/clob/memclob/memclob_test_util.go Modified assertPlaceOrderOffchainMessages function to handle replacement order price changes and update off-chain messages assertion.
indexer/services/vulcan/src/handlers/order-place-handler.ts Removed code related to sending an orderbook message based on updatedQuantums.
indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts Removed conditional block and associated logic for creating an OrderbookMessage based on expectOrderBookUpdate.

Poem

In the land of code where orders dance,
A rabbit tweaks with careful glance.
Prices checked, messages sent,
Workflows triggered, none are bent.
Tests refined, logic clear,
Bugs and glitches disappear.
Oh, the joy of code's embrace,
As changes bring a smoother pace. 🐇✨


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 50919ec and 1719980.
Files selected for processing (1)
  • protocol/x/clob/memclob/memclob.go (1 hunks)
Additional Context Used
Path-based Instructions (1)
protocol/x/clob/memclob/memclob.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.

Learnings (1)
protocol/x/clob/memclob/memclob.go (1)
User: jonfung-dydx
PR: dydxprotocol/v4-chain#1009
File: protocol/x/clob/memclob/memclob_place_order_test.go:4108-4108
Timestamp: 2024-02-05T20:01:55.204Z
Learning: The `assertPlaceOrderOffchainMessages` function in `memclob_test_util.go` indeed requires `ctx` as a parameter for its operations, contrary to the initial comment made.
Additional comments not posted (1)
protocol/x/clob/memclob/memclob.go (1)

495-515: The changes from lines 495 to 515 introduce logic to handle order replacements with different prices by sending an order removal message before the new order placement. This aligns with the PR objectives to manage order replacements more effectively in the indexer. However, ensure that the CreateOrderRemoveMessageWithReason and CreateOrderPlaceMessage functions handle errors appropriately and that their failure does not impact the critical path of order processing.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review Details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits Files that changed from the base of the PR and between 9eff317 and 1c131a9.
Files selected for processing (4)
  • protocol/x/clob/memclob/memclob.go (1 hunks)
  • protocol/x/clob/memclob/memclob_place_order_reduce_only_test.go (2 hunks)
  • protocol/x/clob/memclob/memclob_place_order_test.go (8 hunks)
  • protocol/x/clob/memclob/memclob_test_util.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_place_order_reduce_only_test.go
  • protocol/x/clob/memclob/memclob_place_order_test.go
  • protocol/x/clob/memclob/memclob_test_util.go

@chenyaoy chenyaoy requested a review from dydxwill May 29, 2024 13:40
// If this is a replacement order, then ensure we send the appropriate replacement message.
// If existing order ID is found and the price of the existing order is different from the new order,
// create an order removal message and then an order place message.
// If the price of the existing order is the same as the new order, don't send any messages here because
Copy link
Contributor

@dydxwill dydxwill May 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow why no message needs to be sent if price is the same.
What if the replaced order isn't matched? Will an update not be sent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will get handled by the Order Update message sent at the end of this function

@chenyaoy chenyaoy requested a review from jayy04 May 29, 2024 20:49
@chenyaoy chenyaoy changed the title Simplify indexer messages for short term order replacement [CT-875] Simplify indexer messages for short term order replacement May 29, 2024
Copy link

linear bot commented May 29, 2024

headers,
};
sendMessageWrapper(orderbookMessage, KafkaTopics.TO_WEBSOCKETS_ORDERBOOKS);
}
Copy link
Contributor Author

@chenyaoy chenyaoy May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing because this is sending a websocket message to remove the unfilled amount of the original order from the orderbook price level. This is not expected to be called currently (before my changes) because we expect order removal to always happen before order place so the updatedQuantums should always be undefined (because an existing order in the Orders Cache should have already been removed at this point)

On order replacement there are 2 cases:

  1. If the price is different, we expect an order removal message first so the original order shouldn't exist at this point, so we don't need this here
  2. When the price is the same, we shouldn't remove the unfilled amount of the original order from the orderbook and then re-add the replacement order size to the orderbook via the Order Update message because this is 2 websocket messages and will cause flickering. We should just wait for the update message we expect after this order place to update the orderbook price-level size

I am concerned that the data sent through socks will not match the data in OpenOrdersCache and OrderbookLevelsCache for the short amount of time between this handler and when the Order Update Handler finishes running, I'm not sure if this is okay or not.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
indexer/services/vulcan/src/handlers/order-place-handler.ts (1)

Line range hint 63-63: Avoid using non-null assertions.

Non-null assertions have been used extensively throughout the code. While TypeScript allows non-null assertions, they can lead to runtime errors if not handled properly. Consider adding appropriate checks or handling potential null values gracefully to ensure robustness and maintainability of the code.

Also applies to: 64-64, 65-65, 69-69, 91-91, 113-113, 136-136, 137-137, 181-181, 214-214, 215-215, 232-232, 234-234, 275-275

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 1c131a9 and 45f7950.

Files selected for processing (2)
  • indexer/services/vulcan/src/handlers/order-place-handler.ts (1 hunks)
  • protocol/x/clob/memclob/memclob.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/memclob/memclob.go
Additional context used
Path-based instructions (1)
indexer/services/vulcan/src/handlers/order-place-handler.ts (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.

Biome
indexer/services/vulcan/src/handlers/order-place-handler.ts

[error] 63-63: Forbidden non-null assertion.


[error] 64-64: Forbidden non-null assertion.


[error] 65-65: Forbidden non-null assertion.


[error] 69-69: Forbidden non-null assertion.


[error] 91-91: Forbidden non-null assertion.


[error] 113-113: Forbidden non-null assertion.


[error] 136-136: Forbidden non-null assertion.


[error] 136-136: Forbidden non-null assertion.


[error] 137-137: Forbidden non-null assertion.


[error] 137-137: Forbidden non-null assertion.


[error] 181-181: Forbidden non-null assertion.


[error] 181-181: Forbidden non-null assertion.


[error] 214-214: Forbidden non-null assertion.


[error] 214-214: Forbidden non-null assertion.


[error] 215-215: Forbidden non-null assertion.


[error] 232-232: Forbidden non-null assertion.


[error] 232-232: Forbidden non-null assertion.


[error] 234-234: Forbidden non-null assertion.


[error] 275-275: Forbidden non-null assertion.


[error] 275-275: Forbidden non-null assertion.

Additional comments not posted (4)
indexer/services/vulcan/src/handlers/order-place-handler.ts (4)

Line range hint 63-275: The changes in the handle method align with the PR objectives to simplify the handling of indexer messages.

However, consider improving the handling of potential null values as mentioned in the previous comment.


Line range hint 181-275: The updatePriceLevel method correctly handles price level updates based on the order placement result.


Line range hint 214-275: The validateOrderPlace method performs essential validations for order placement.

Consider improving the handling of potential null values as mentioned in previous comments.


Line range hint 232-275: The shouldSendSubaccountMessage method correctly determines when to send subaccount websocket messages.

@chenyaoy chenyaoy requested a review from dydxwill May 30, 2024 17:11
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (5)
indexer/services/vulcan/src/handlers/order-place-handler.ts (1)

Line range hint 63-63: Avoid using non-null assertions unless absolutely necessary.

Non-null assertions have been used extensively throughout the code. This practice can lead to runtime errors if the asserted values are actually null or undefined. Consider adding proper null checks or handling potential null values gracefully.

Also applies to: 64-64, 65-65, 69-69, 91-91, 113-113, 136-136, 137-137, 181-181, 214-214, 215-215, 232-232, 234-234, 275-275

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (4)

Line range hint 204-208: Consider using for...of instead of forEach for better performance and readability.

Using for...of allows for better handling of asynchronous operations within loops and can improve the readability of the code.


Line range hint 582-582: Avoid using non-null assertions unless absolutely necessary.

Similar to the previous file, non-null assertions are used extensively. It's safer to handle potential null values explicitly to avoid runtime errors.

Also applies to: 717-717, 803-803, 920-920, 1194-1194, 1196-1196, 1201-1201, 1204-1204, 1206-1206


Line range hint 1012-1012: Specify a more explicit type instead of any.

Using any type defeats the purpose of TypeScript's static typing. It's better to specify a more explicit type to benefit from type checking during compile time.


Line range hint 1140-1140: Remove unnecessary type annotations.

These type annotations are trivially inferred from their initialization and can be safely removed to clean up the code.

Also applies to: 1169-1169, 1181-1181

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 45f7950 and 0f3185a.

Files selected for processing (5)
  • indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (1 hunks)
  • indexer/services/vulcan/src/handlers/order-place-handler.ts (2 hunks)
  • protocol/x/clob/memclob/memclob.go (1 hunks)
  • protocol/x/clob/memclob/memclob_place_order_test.go (8 hunks)
  • protocol/x/clob/memclob/memclob_test_util.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
  • protocol/x/clob/memclob/memclob.go
  • protocol/x/clob/memclob/memclob_place_order_test.go
  • protocol/x/clob/memclob/memclob_test_util.go
Additional context used
Path-based instructions (2)
indexer/services/vulcan/src/handlers/order-place-handler.ts (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.

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (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.

Biome
indexer/services/vulcan/src/handlers/order-place-handler.ts

[error] 63-63: Forbidden non-null assertion.


[error] 64-64: Forbidden non-null assertion.


[error] 65-65: Forbidden non-null assertion.


[error] 69-69: Forbidden non-null assertion.


[error] 91-91: Forbidden non-null assertion.


[error] 113-113: Forbidden non-null assertion.


[error] 136-136: Forbidden non-null assertion.


[error] 136-136: Forbidden non-null assertion.


[error] 137-137: Forbidden non-null assertion.


[error] 137-137: Forbidden non-null assertion.


[error] 181-181: Forbidden non-null assertion.


[error] 181-181: Forbidden non-null assertion.


[error] 214-214: Forbidden non-null assertion.


[error] 214-214: Forbidden non-null assertion.


[error] 215-215: Forbidden non-null assertion.


[error] 232-232: Forbidden non-null assertion.


[error] 232-232: Forbidden non-null assertion.


[error] 234-234: Forbidden non-null assertion.


[error] 275-275: Forbidden non-null assertion.


[error] 275-275: Forbidden non-null assertion.

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts

[error] 204-208: Prefer for...of instead of forEach.


[error] 582-582: Forbidden non-null assertion.


[error] 717-717: Forbidden non-null assertion.


[error] 803-803: Forbidden non-null assertion.


[error] 920-920: Forbidden non-null assertion.


[error] 920-920: Forbidden non-null assertion.


[error] 1012-1012: Unexpected any. Specify a different type.


[error] 1140-1140: This type annotation is trivially inferred from its initialization.


[error] 1169-1169: This type annotation is trivially inferred from its initialization.


[error] 1181-1181: This type annotation is trivially inferred from its initialization.


[error] 1194-1194: Forbidden non-null assertion.


[error] 1196-1196: Forbidden non-null assertion.


[error] 1196-1196: Forbidden non-null assertion.


[error] 1201-1201: Forbidden non-null assertion.


[error] 1201-1201: Forbidden non-null assertion.


[error] 1204-1204: Forbidden non-null assertion.


[error] 1204-1204: Forbidden non-null assertion.


[error] 1204-1204: Forbidden non-null assertion.


[error] 1206-1206: Forbidden non-null assertion.


[error] 1206-1206: Forbidden non-null assertion.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (4)
indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (4)

Line range hint 201-205: Prefer using for...of instead of forEach for better readability and performance in modern JavaScript.

- [replacementMessage, replacementMessageGoodTilBlockTime, replacementMessageConditional,
-  replacementMessageFok, replacementMessageIoc].forEach((message) => {
-  // eslint-disable-next-line no-param-reassign
-  message.headers = defaultKafkaHeaders;
- });
+ for (const message of [replacementMessage, replacementMessageGoodTilBlockTime, replacementMessageConditional,
+  replacementMessageFok, replacementMessageIoc]) {
+  // eslint-disable-next-line no-param-reassign
+  message.headers = defaultKafkaHeaders;
+ }

Line range hint 575-575: Avoid using non-null assertions. They bypass TypeScript's strict null checks and can lead to runtime errors if assumptions about non-nullability prove incorrect.

Consider adding null checks or ensuring that the variable cannot be null before usage.

Also applies to: 704-704, 790-790, 907-907, 1181-1181, 1183-1183, 1188-1188, 1191-1191, 1193-1193


Line range hint 999-999: Specify a more precise type instead of using any.

Using any disables TypeScript's type checking, which can lead to runtime errors. Define a more specific type for better type safety.


Line range hint 1127-1127: This type annotation is trivially inferred from its initialization and can be omitted for cleaner code.

- const producerSendSpy: jest.SpyInstance = jest.spyOn(producer, 'send').mockReturnThis();
+ const producerSendSpy = jest.spyOn(producer, 'send').mockReturnThis();

Also applies to: 1156-1156, 1168-1168

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 0f3185a and e6aef53.

Files selected for processing (1)
  • indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (5 hunks)
Additional context used
Path-based instructions (1)
indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (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.

Biome
indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts

[error] 201-205: Prefer for...of instead of forEach.


[error] 575-575: Forbidden non-null assertion.


[error] 704-704: Forbidden non-null assertion.


[error] 790-790: Forbidden non-null assertion.


[error] 907-907: Forbidden non-null assertion.


[error] 907-907: Forbidden non-null assertion.


[error] 999-999: Unexpected any. Specify a different type.


[error] 1127-1127: This type annotation is trivially inferred from its initialization.


[error] 1156-1156: This type annotation is trivially inferred from its initialization.


[error] 1168-1168: This type annotation is trivially inferred from its initialization.


[error] 1181-1181: Forbidden non-null assertion.


[error] 1183-1183: Forbidden non-null assertion.


[error] 1183-1183: Forbidden non-null assertion.


[error] 1188-1188: Forbidden non-null assertion.


[error] 1188-1188: Forbidden non-null assertion.


[error] 1191-1191: Forbidden non-null assertion.


[error] 1191-1191: Forbidden non-null assertion.


[error] 1191-1191: Forbidden non-null assertion.


[error] 1193-1193: Forbidden non-null assertion.


[error] 1193-1193: Forbidden non-null assertion.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (4)
indexer/services/vulcan/src/handlers/order-place-handler.ts (2)

Line range hint 62-62: Consider handling potential null values explicitly instead of using non-null assertions.

Using non-null assertions (!) forces TypeScript to ignore null and undefined checks, which can lead to runtime errors if the assumptions are incorrect. It's safer to handle these cases explicitly, either by checking for null or undefined values or by using optional chaining (?.) where appropriate.

Also applies to: 63-63, 64-64, 68-68, 90-90, 116-116, 117-117, 161-161, 194-194, 195-195, 212-212, 255-255, 261-261


Line range hint 161-161: Track or resolve the TODO comment regarding the removal of logic.

There's a TODO comment about removing logic once best-effort-open is not sent for stateful orders. Consider creating a task or issue in your project management tool to track this, ensuring it doesn't get overlooked.

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (2)

Line range hint 569-569: Consider handling potential null values explicitly in tests instead of using non-null assertions.

The tests use non-null assertions extensively, which might lead to false positives if the assumptions about non-null values are incorrect. Consider handling these cases explicitly in your tests to ensure they are robust and reliable.

Also applies to: 686-686, 772-772, 889-889, 981-981, 1104-1104, 1133-1133, 1145-1145, 1158-1158, 1160-1160, 1165-1165, 1168-1168, 1170-1170


Line range hint 200-204: Replace forEach with for...of for better handling of asynchronous operations.

The use of forEach can lead to issues when dealing with asynchronous operations within the loop. Consider using for...of instead, which handles await more intuitively and is generally easier to debug.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e6aef53 and b8df623.

Files selected for processing (3)
  • indexer/services/vulcan/tests/handlers/order-place-handler.test.ts (7 hunks)
  • indexer/services/vulcan/src/handlers/order-place-handler.ts (2 hunks)
  • protocol/x/clob/memclob/memclob.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • protocol/x/clob/memclob/memclob.go
Additional context used
Path-based instructions (2)
indexer/services/vulcan/src/handlers/order-place-handler.ts (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.

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts (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.

Biome
indexer/services/vulcan/src/handlers/order-place-handler.ts

[error] 62-62: Forbidden non-null assertion.


[error] 63-63: Forbidden non-null assertion.


[error] 64-64: Forbidden non-null assertion.


[error] 68-68: Forbidden non-null assertion.


[error] 90-90: Forbidden non-null assertion.


[error] 116-116: Forbidden non-null assertion.


[error] 116-116: Forbidden non-null assertion.


[error] 117-117: Forbidden non-null assertion.


[error] 117-117: Forbidden non-null assertion.


[error] 161-161: Forbidden non-null assertion.


[error] 161-161: Forbidden non-null assertion.


[error] 194-194: Forbidden non-null assertion.


[error] 194-194: Forbidden non-null assertion.


[error] 195-195: Forbidden non-null assertion.


[error] 212-212: Forbidden non-null assertion.


[error] 212-212: Forbidden non-null assertion.


[error] 214-214: Forbidden non-null assertion.


[error] 255-255: Forbidden non-null assertion.


[error] 255-255: Forbidden non-null assertion.


[error] 261-261: Forbidden non-null assertion.

indexer/services/vulcan/__tests__/handlers/order-place-handler.test.ts

[error] 200-204: Prefer for...of instead of forEach.


[error] 569-569: Forbidden non-null assertion.


[error] 686-686: Forbidden non-null assertion.


[error] 772-772: Forbidden non-null assertion.


[error] 889-889: Forbidden non-null assertion.


[error] 889-889: Forbidden non-null assertion.


[error] 981-981: Unexpected any. Specify a different type.


[error] 1104-1104: This type annotation is trivially inferred from its initialization.


[error] 1133-1133: This type annotation is trivially inferred from its initialization.


[error] 1145-1145: This type annotation is trivially inferred from its initialization.


[error] 1158-1158: Forbidden non-null assertion.


[error] 1160-1160: Forbidden non-null assertion.


[error] 1160-1160: Forbidden non-null assertion.


[error] 1165-1165: Forbidden non-null assertion.


[error] 1165-1165: Forbidden non-null assertion.


[error] 1168-1168: Forbidden non-null assertion.


[error] 1168-1168: Forbidden non-null assertion.


[error] 1168-1168: Forbidden non-null assertion.


[error] 1170-1170: Forbidden non-null assertion.


[error] 1170-1170: Forbidden non-null assertion.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (2)
indexer/services/vulcan/src/handlers/order-place-handler.ts (2)

Line range hint 58-58: Remove non-null assertions to ensure robust error handling.

- const orderPlace: OrderPlaceV1 = update.orderPlace!;
+ const orderPlace: OrderPlaceV1 = update.orderPlace;
+ if (!orderPlace) {
+   logger.error('OrderPlace is undefined');
+   return;
+ }

Apply similar changes to other non-null assertions throughout the file.

Also applies to: 59-59, 60-60, 64-64, 86-86, 112-112, 113-113, 174-174, 180-180, 240-240


Line range hint 2-9: Optimize import statements by using type-only imports where applicable.

- import { OrderFromDatabase, OrderTable, PerpetualMarketFromDatabase, perpetualMarketRefresher } from '@dydxprotocol-indexer/postgres';
+ import type { OrderFromDatabase, OrderTable, PerpetualMarketFromDatabase, perpetualMarketRefresher } from '@dydxprotocol-indexer/postgres';

Apply similar changes to other relevant imports.

Also applies to: 9-16, 22-30, 30-31

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between b8df623 and 0d900eb.

Files selected for processing (1)
  • indexer/services/vulcan/src/handlers/order-place-handler.ts (5 hunks)
Additional context used
Biome
indexer/services/vulcan/src/handlers/order-place-handler.ts

[error] 58-58: Forbidden non-null assertion.


[error] 59-59: Forbidden non-null assertion.


[error] 60-60: Forbidden non-null assertion.


[error] 64-64: Forbidden non-null assertion.


[error] 86-86: Forbidden non-null assertion.


[error] 112-112: Forbidden non-null assertion.


[error] 112-112: Forbidden non-null assertion.


[error] 113-113: Forbidden non-null assertion.


[error] 113-113: Forbidden non-null assertion.


[error] 174-174: Forbidden non-null assertion.


[error] 174-174: Forbidden non-null assertion.


[error] 180-180: Forbidden non-null assertion.


[error] 180-180: Forbidden non-null assertion.


[error] 240-240: Forbidden non-null assertion.


[error] 2-9: Some named imports are only used as types.


[error] 9-16: Some named imports are only used as types.


[error] 22-30: Some named imports are only used as types.


[error] 30-31: All these imports are only used as types.

Additional comments not posted (1)
indexer/services/vulcan/src/handlers/order-place-handler.ts (1)

Line range hint 161-161: The changes align with the PR objectives to simplify the websocket message flow. However, monitor for potential data mismatches as noted in your comment.

@chenyaoy chenyaoy merged commit 86eb47d into main Jun 3, 2024
27 checks passed
@chenyaoy chenyaoy deleted the chenyao/short-term-order-replacement-indexer-messages branch June 3, 2024 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants