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

[TRA-108] Add market type to indexer events. #1201

Merged
merged 5 commits into from
Mar 19, 2024

Conversation

vincentwschau
Copy link
Contributor

Changelist

Add perpetual market type to perpetual market creation event sent from protocol to indexer.

Test Plan

Unit tests.

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.

Copy link

linear bot commented Mar 19, 2024

@github-actions github-actions bot added the proto label Mar 19, 2024
Copy link
Contributor

coderabbitai bot commented Mar 19, 2024

Walkthrough

The changes primarily focus on integrating a new marketType field across various components of the dydxprotocol indexer, affecting files related to events, protocols, and the CLOB (Central Limit Order Book) keeper. This addition necessitates updates in import paths, the introduction of new enums and functions for market type definitions, and modifications in function calls to include the marketType parameter. These adjustments ensure the system accurately handles different perpetual market types, enhancing the protocol's functionality and flexibility.

Changes

Files Change Summary
.../v4-protos/src/codegen/dydxprotocol/bundle.ts
.../gogoproto/bundle.ts
.../google/bundle.ts
Updated import paths and reordering. Adjustments in the structure of namespaces.
.../v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts
proto/dydxprotocol/indexer/events/events.proto
Added support for marketType in PerpetualMarketCreateEvent interfaces and messages.
.../v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/perpetual.ts
proto/dydxprotocol/indexer/protocol/v1/perpetual.proto
Introduced enums and functions for perpetual market types.
protocol/indexer/events/perpetual_market_create.go
protocol/indexer/events/perpetual_market_create_test.go
protocol/indexer/protocol/v1/v1_mappers.go
protocol/indexer/protocol/v1/v1_mappers_test.go
protocol/testutil/keeper/clob.go
protocol/x/clob/keeper/... (all files in clob keeper and tests)
Renamed imports, added new imports for perptypes, included marketType in function calls and test cases. Added and updated tests to accommodate the new marketType parameter.

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-tests 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 tests 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 tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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 a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @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.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

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.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between be4ba54 and bace278.
Files ignored due to path filters (2)
  • protocol/indexer/events/events.pb.go is excluded by: !**/*.pb.go
  • protocol/indexer/protocol/v1/types/perpetual.pb.go is excluded by: !**/*.pb.go
Files selected for processing (21)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (7 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/perpetual.ts (1 hunks)
  • indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
  • indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
  • proto/dydxprotocol/indexer/events/events.proto (2 hunks)
  • proto/dydxprotocol/indexer/protocol/v1/perpetual.proto (1 hunks)
  • protocol/indexer/events/perpetual_market_create.go (3 hunks)
  • protocol/indexer/events/perpetual_market_create_test.go (3 hunks)
  • protocol/indexer/protocol/v1/v1_mappers.go (2 hunks)
  • protocol/indexer/protocol/v1/v1_mappers_test.go (2 hunks)
  • protocol/testutil/keeper/clob.go (1 hunks)
  • protocol/x/clob/keeper/clob_pair.go (2 hunks)
  • protocol/x/clob/keeper/clob_pair_test.go (8 hunks)
  • protocol/x/clob/keeper/deleveraging_test.go (2 hunks)
  • protocol/x/clob/keeper/get_price_premium_test.go (1 hunks)
  • protocol/x/clob/keeper/liquidations_test.go (8 hunks)
  • protocol/x/clob/keeper/msg_server_create_clob_pair_test.go (2 hunks)
  • protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
  • protocol/x/clob/keeper/orders_test.go (5 hunks)
  • protocol/x/clob/keeper/process_operations_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
  • indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts
Additional comments: 37
indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
  • 1-14: The import statements and namespace exports in bundle.ts are correctly structured and follow best practices for organizing and exporting namespaces in TypeScript. This change enhances the modularity and readability of the code by clearly separating concerns and providing a structured way to access api and protobuf related functionalities under the google namespace.
proto/dydxprotocol/indexer/protocol/v1/perpetual.proto (1)
  • 8-17: The PerpetualMarketType enum in perpetual.proto is well-defined, with clear and descriptive names for each market type. This addition will allow for more precise representation and handling of different perpetual market types in the system, enhancing the protocol's flexibility and functionality.
protocol/indexer/events/perpetual_market_create_test.go (2)
  • 8-8: Adding the perptypes import is necessary for accessing the PerpetualMarketType enum values in the test. This change correctly aligns with the modifications made to the perpetual market creation event to include the marketType attribute.
  • 25-25: Updating the MarketType field assignment to use v1types.PerpetualMarketType_PERPETUAL_MARKET_TYPE_CROSS from the perptypes package in the test function is correct. This ensures that the test reflects the new functionality introduced by adding the marketType attribute to perpetual market creation events.
protocol/indexer/events/perpetual_market_create.go (2)
  • 5-6: Renaming the imported package to clobtypes and adding the perptypes import are appropriate changes that improve code clarity and enable the use of the PerpetualMarketType enum in the NewPerpetualMarketCreateEvent function.
  • 16-22: The addition of the marketType parameter of type PerpetualMarketType from the perptypes package to the NewPerpetualMarketCreateEvent function is a necessary change to support the new functionality of distinguishing between different types of perpetual markets. This change is correctly implemented and enhances the protocol's capabilities.
indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/perpetual.ts (1)
  • 1-67: The TypeScript enums PerpetualMarketType and PerpetualMarketTypeSDKType, along with the conversion functions perpetualMarketTypeFromJSON and perpetualMarketTypeToJSON, are correctly defined and implemented. These changes facilitate the handling of perpetual market types in the TypeScript codebase, ensuring consistency and ease of use when working with these values.
protocol/x/clob/keeper/msg_server_create_clob_pair_test.go (2)
  • 6-6: The addition of the lib import is necessary for the test setup, ensuring that the test has access to required functionalities from the lib package. This change is appropriate and supports the test's requirements.
  • 66-66: Including testPerp1.Params.MarketType in the function call within the TestCreateClobPair function is a correct update that reflects the new functionality of handling different perpetual market types. This ensures that the test accurately tests the system's behavior with the addition of the marketType attribute.
protocol/indexer/protocol/v1/v1_mappers.go (1)
  • 193-207: The addition of the ConvertToPerpetualMarketType function in v1_mappers.go is a necessary and well-implemented change that facilitates the conversion of perpetual market types between different versions. This function enhances the system's flexibility and interoperability by allowing for seamless conversion of market types, ensuring compatibility across different parts of the system.
protocol/x/clob/keeper/get_price_premium_test.go (1)
  • 186-186: Adding perpetual.Params.MarketType as a parameter in the function call within the TestGetPricePremiumForPerpetual function is a correct and necessary update. This change ensures that the test accurately reflects the system's behavior with the introduction of the marketType attribute, testing the system's ability to handle different perpetual market types.
protocol/testutil/keeper/clob.go (1)
  • 291-291: The addition of the MarketType parameter to the NewPerpetualMarketCreateEvent function call is correctly implemented. This change aligns with the PR's objective to enhance market creation events with more detailed information, specifically distinguishing between different types of perpetual markets. It's important to ensure that all downstream functions and event handlers that consume this event are updated accordingly to handle the new MarketType attribute.
indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
  • 49-175: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [52-326]

The reordering of imports and updating of paths in bundle.ts improves code organization and readability. It's important to verify that these changes do not introduce any breaking changes or issues with dependencies within the project. Ensuring that all references to these modules are updated accordingly will help maintain the integrity of the codebase.

Verification successful

The reordering of imports and updating of paths in bundle.ts have been successfully verified. The references to the updated modules by their aliases (_56 and _57) are correctly implemented within the bundle.ts file, and there's no indication of issues or misreferences based on the executed scripts. It's still recommended to ensure comprehensive testing to verify the overall integrity of the project.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that updated import paths are correctly referenced throughout the project.
rg --type ts "./indexer/protocol/v1/perpetual" "./indexer/protocol/v1/subaccount"

Length of output: 152



Script:

#!/bin/bash
# Adjusting the search strategy to find references to the updated modules by their aliases.
rg --type ts "_56" --files-with-matches
rg --type ts "_57" --files-with-matches

Length of output: 203

protocol/x/clob/keeper/msg_server_place_order_test.go (2)
  • 179-179: The addition of the MarketType parameter to the NewPerpetualMarketCreateEvent function call within the TestPlaceOrder_Error test setup is correctly implemented. This change ensures that the test reflects the updated event structure and aligns with the PR's objective to enhance market creation events. It's important to verify that the test logic correctly handles the new MarketType attribute and that all related tests are updated accordingly.
  • 324-324: The inclusion of the MarketType parameter in the NewPerpetualMarketCreateEvent function call within the TestPlaceOrder_Success test setup is correctly implemented. This ensures consistency with the updated event structure and the PR's objectives. As with other tests, it's crucial to ensure that the MarketType attribute is correctly handled in the test logic and that any related tests are also updated to reflect these changes.
protocol/indexer/protocol/v1/v1_mappers_test.go (2)
  • 12-12: The addition of the perptypes package import is necessary for the new test function TestConvertToPerpetualMarketType to function correctly.
  • 432-475: The new test function TestConvertToPerpetualMarketType correctly tests the conversion of PerpetualMarketType to v1.PerpetualMarketType. It includes a comprehensive set of test cases, covering all defined PerpetualMarketType values, and correctly handles the PERPETUAL_MARKET_TYPE_UNSPECIFIED case by expecting a panic. This thorough approach ensures that the conversion function behaves as expected across all valid inputs and fails gracefully on invalid inputs.
proto/dydxprotocol/indexer/events/events.proto (2)
  • 8-8: The addition of the import statement for dydxprotocol/indexer/protocol/v1/perpetual.proto is necessary to use the PerpetualMarketType enum in the PerpetualMarketCreateEventV1 message.
  • 356-358: The addition of the marketType field to the PerpetualMarketCreateEventV1 message is correctly implemented. It uses the PerpetualMarketType enum from the newly imported perpetual.proto, allowing for the distinction between different types of perpetual markets. This change aligns with the PR objectives to enhance perpetual market creation events with more detailed information. It's also correctly positioned at the end of the message to maintain backward compatibility.
protocol/x/clob/keeper/clob_pair.go (1)
  • 108-108: The addition of perpetual.Params.MarketType to the NewPerpetualMarketCreateEvent function call within CreatePerpetualClobPair effectively integrates the new marketType attribute into the perpetual market creation events. This change aligns with the PR's objective to enhance event data sent to the indexer by distinguishing between different types of perpetual markets.

Ensure that the marketType attribute is correctly documented in the protobuf definitions and that all downstream systems consuming these events are updated to handle the new attribute appropriately.

protocol/x/clob/keeper/clob_pair_test.go (8)
  • 69-69: The addition of MarketType to the NewPerpetualMarketCreateEvent function call aligns with the PR objectives to enhance perpetual market creation events. This change ensures that the MarketType information is included in the event data, which is crucial for distinguishing between different types of perpetual markets.
  • 191-191: Similar to the previous comment, the inclusion of MarketType in the NewPerpetualMarketCreateEvent function call here is consistent with the PR's goal of enhancing the detail provided in perpetual market creation events. This ensures that the indexer receives comprehensive information about the market being created.
  • 283-283: The addition of MarketType in this context is crucial for ensuring that the test accurately reflects the changes made to the perpetual market creation event structure. By including MarketType, the test validates that the new attribute is correctly processed and mapped within the system.
  • 436-436: Including MarketType in the NewPerpetualMarketCreateEvent function call within this test case is essential for verifying that the system correctly handles the new attribute in various scenarios. This change is a direct reflection of the PR's objectives to enhance the detail and accuracy of perpetual market creation events.
  • 628-628: The inclusion of MarketType in the NewPerpetualMarketCreateEvent function call here is necessary for ensuring that the final settlement process for a CLOB pair correctly includes the MarketType information. This aligns with the PR's goal of enhancing the detail provided in perpetual market creation events.
  • 745-745: The addition of MarketType in this test case is important for validating the system's ability to handle valid status transitions for CLOB pairs, including the new MarketType attribute. This ensures that the system correctly processes and maps the MarketType information during CLOB pair status updates.
  • 803-803: Including MarketType in the NewPerpetualMarketCreateEvent function call within this test case is crucial for testing the system's handling of unsupported status transitions for CLOB pairs. This addition ensures that the MarketType attribute is considered in various status transition scenarios.
  • 841-841: The inclusion of MarketType in the NewPerpetualMarketCreateEvent function call here is necessary for testing the system's response to unsupported status transitions to unsupported statuses. This ensures that the MarketType attribute is correctly handled in all status transition scenarios.
protocol/x/clob/keeper/deleveraging_test.go (2)
  • 416-416: The addition of the MarketType parameter in the NewPerpetualMarketCreateEvent function call ensures that the market type information is correctly passed and utilized. This change aligns with the PR's objective to enhance perpetual market creation events with the addition of the MarketType attribute.
  • 790-790: Similar to the previous comment, the addition of the MarketType parameter in the NewPerpetualMarketCreateEvent function call within the TestOffsetSubaccountPerpetualPosition test function is consistent with the PR's objective. This ensures that the market type information is correctly incorporated into the event creation process.
indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2)
  • 849-851: The addition of the marketType attribute to the PerpetualMarketCreateEventV1 interface is correctly implemented and aligns with the PR objectives to enhance perpetual market creation events. The use of the PerpetualMarketType enum for this attribute is appropriate and consistent with the rest of the file.
  • 922-924: The addition of the marketType attribute to the PerpetualMarketCreateEventV1SDKType interface is correctly implemented and aligns with the PR objectives to enhance perpetual market creation events. The use of the PerpetualMarketTypeSDKType enum for this attribute is appropriate and consistent with the rest of the file.
protocol/x/clob/keeper/orders_test.go (5)
  • 1044-1050: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [14-1047]

The test function TestPlaceShortTermOrder covers a wide range of scenarios for placing short-term orders. It's well-structured and uses table-driven tests to ensure various conditions are tested. However, there are a few areas where improvements can be made:

  1. Error Handling: Ensure that all error paths are tested. For example, tests for invalid inputs that could lead to runtime errors should be included if not already covered.
  2. Code Duplication: There's a noticeable pattern of setup code that is repeated across sub-tests. Consider refactoring this setup code into helper functions to reduce duplication and improve maintainability.
  3. Test Coverage: Verify that all critical paths and edge cases are covered. This includes testing for boundary conditions, potential race conditions, and any special cases that might affect the order placement logic.

Overall, the test function appears to be comprehensive, but ensure that the above points are addressed to improve the quality and maintainability of the tests.

  • 1099-1105: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1048-1102]

The test TestPlaceShortTermOrder_PanicsOnStatefulOrder correctly verifies that the system panics when a stateful order is passed to a function intended for short-term orders. This is a good practice to ensure that the system behaves as expected in error conditions. However, consider adding a comment explaining why this behavior is expected and the importance of ensuring that stateful orders are not mistakenly treated as short-term orders. This will help future maintainers understand the rationale behind this test case.

  • 1854-1860: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1103-1857]

The test function TestAddPreexistingStatefulOrder thoroughly tests the addition of preexisting stateful orders, including various scenarios and edge cases. A few suggestions for improvement:

  1. Clarity and Documentation: Some of the test cases are complex and involve multiple steps and conditions. Adding comments or documentation within the test cases to explain the setup and expected outcomes can greatly improve readability and maintainability.
  2. Error Handling and Edge Cases: Ensure comprehensive coverage of error handling paths and edge cases. This includes testing for invalid order states, incorrect order types, and any other conditions that could lead to unexpected behavior.
  3. Refactoring for Reusability: If there's common setup code or assertions that are repeated across test cases, consider refactoring them into helper functions. This can reduce duplication and make the tests easier to update in the future.

Overall, the test function is well-structured and covers a broad range of scenarios, but the above refinements can enhance its clarity and effectiveness.

  • 2068-2074: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1858-2071]

The test TestPlaceOrder_SendOffchainMessages effectively verifies that offchain messages are sent as expected during order placement. It's a good practice to mock external dependencies, like the indexer event manager, to isolate the system under test. However, consider the following improvements:

  1. Mock Verification: Ensure that the mock expectations are verified at the end of the test. This includes checking that all expected calls to the mocked indexer event manager were made with the correct parameters.
  2. Error Scenarios: If applicable, add tests for scenarios where sending offchain messages might fail or behave unexpectedly. This can help ensure robust error handling in the system.
  3. Documentation: Adding comments to explain the purpose of the test and the significance of sending offchain messages in this context can help future maintainers understand the test's intent.

By addressing these points, the test can be made more comprehensive and maintainable.

  • 2200-2206: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2072-2203]

The test TestPerformStatefulOrderValidation_PreExistingStatefulOrder checks the validation logic for preexisting stateful orders. It's crucial for ensuring the integrity of the system's state. A few points to consider:

  1. Comprehensive Coverage: Make sure that all relevant scenarios are tested, including cases where orders might be partially filled, canceled, or otherwise modified before validation.
  2. Error Handling: Test how the system handles errors or inconsistencies in the state of preexisting orders. This includes situations where the order data might be corrupted or incomplete.
  3. Documentation: Given the importance of stateful order validation, adding detailed comments or documentation to explain the test setup, expected behavior, and significance of each test case can be very helpful for future maintainers.

Enhancing the test with these considerations can improve its effectiveness in verifying the system's behavior.

@@ -2261,6 +2261,7 @@ func setupProcessProposerOperationsTestCase(
clobPair.SubticksPerTick,
clobPair.StepBaseQuantums,
tc.perpetuals[perpetualId].Params.LiquidityTier,
tc.perpetuals[perpetualId].Params.MarketType,
Copy link
Contributor

Choose a reason for hiding this comment

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

The line seems to be a stray line number without corresponding code. This could be a typographical error or misplaced comment. Please review and remove or correct it as necessary.


The line seems to be a stray line number without corresponding code. This could be a typographical error or misplaced comment. Please review and remove or correct it as necessary.


The line seems to be a stray line number without corresponding code. This could be a typographical error or misplaced comment. Please review and remove or correct it as necessary.

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.

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between bace278 and 2bbf4df.
Files selected for processing (2)
  • indexer/services/ender/tests/helpers/constants.ts (2 hunks)
  • protocol/x/clob/abci_test.go (2 hunks)
Additional comments: 5
indexer/services/ender/__tests__/helpers/constants.ts (2)
  • 27-27: The import of PerpetualMarketType is correctly added to support the new marketType attribute in perpetual market creation events. Good job on maintaining the import order and consistency.
  • 142-142: Initialization of marketType with PerpetualMarketType.PERPETUAL_MARKET_TYPE_CROSS in defaultPerpetualMarketCreateEvent is correctly implemented. Ensure that this default value aligns with the expected behavior in tests and does not inadvertently affect test outcomes.
protocol/x/clob/abci_test.go (3)
  • 712-718: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1085-1085]

The test TestPrepareCheckState_WithProcessProposerMatchesEventsWithBadBlockHeight correctly verifies the panic behavior when the block height does not match, ensuring robust error handling.

  • 712-718: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1185-1185]

The test TestBeginBlocker_Success effectively verifies the BeginBlocker function's behavior in initializing ProcessProposerMatchesEvents for the next block, ensuring correct state management.

  • 712-718: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1235-1235]

The test TestPrepareCheckState provides comprehensive coverage of the PrepareCheckState function under various scenarios, including a regression case, ensuring robust functionality and state management.

@@ -712,6 +712,7 @@ func TestEndBlocker_Success(t *testing.T) {
constants.ClobPair_Btc.SubticksPerTick,
constants.ClobPair_Btc.StepBaseQuantums,
constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.LiquidityTier,
constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,
Copy link
Contributor

Choose a reason for hiding this comment

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

The market type for the ETH perpetual market creation event is incorrectly set to the BTC market type.

- constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,
+ constants.EthUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,

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.

Suggested change
constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,
constants.EthUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,

Comment on lines 712 to 718
constants.ClobPair_Btc.SubticksPerTick,
constants.ClobPair_Btc.StepBaseQuantums,
constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.LiquidityTier,
constants.BtcUsd_20PercentInitial_10PercentMaintenance.Params.MarketType,
),
),
).Once().Return()
Copy link
Contributor

Choose a reason for hiding this comment

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

📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1035-1035]

The test TestLiquidateSubaccounts contains commented-out assertions related to the liquidation process, indicating incomplete test coverage.

Please complete the test coverage to ensure the liquidation process is correctly verified.


📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1135-1135]

The test TestCommitBlocker_WithProcessProposerMatchesEventsWithBadBlockHeight mistakenly calls PrepareCheckState instead of CommitBlocker, leading to a duplicate test of PrepareCheckState.

Please correct the function call to accurately test the intended CommitBlocker behavior.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2bbf4df and 2851c55.
Files ignored due to path filters (1)
  • protocol/indexer/events/events.pb.go is excluded by: !**/*.pb.go
Files selected for processing (3)
  • proto/dydxprotocol/indexer/events/events.proto (2 hunks)
  • protocol/x/clob/genesis_test.go (2 hunks)
  • protocol/x/clob/module_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • proto/dydxprotocol/indexer/events/events.proto
Additional comments: 3
protocol/x/clob/genesis_test.go (2)
  • 6-7: The addition of cosmossdk.io/errors import is appropriate for enhanced error handling in the test cases.
  • 460-460: The addition of the perpetual.Params.MarketType parameter in the function call is crucial for testing the new marketType attribute functionality. It's important to ensure that tests are updated to cover new functionalities introduced in the codebase.
protocol/x/clob/module_test.go (1)
  • 309-309: The addition of the MarketType parameter in the TestAppModule_InitExportGenesis function is essential for ensuring comprehensive test coverage for the new marketType attribute functionality. It's important to maintain thorough tests for new features introduced in the codebase.

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2851c55 and 1190d07.
Files selected for processing (1)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (7 hunks)
Files skipped from review as they are similar to previous changes (1)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts

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.

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1190d07 and 9404c5b.
Files ignored due to path filters (1)
  • protocol/indexer/protocol/v1/types/perpetual.pb.go is excluded by: !**/*.pb.go
Files selected for processing (1)
  • proto/dydxprotocol/indexer/protocol/v1/perpetual.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
  • proto/dydxprotocol/indexer/protocol/v1/perpetual.proto

@vincentwschau vincentwschau merged commit 9888453 into main Mar 19, 2024
33 checks passed
@vincentwschau vincentwschau deleted the vincentc/tra-108-add-market-type-indexer-events branch March 19, 2024 21:41
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