[CT-515] Combine place, cancel, batch cancel rate limiters #1165
Conversation
WalkthroughThe recent modifications enhance the rate limiting mechanism within the protocol by merging the rate limiting functionalities for both order placements and cancellations. This consolidation is reflected across various components, including test files, rate limiter implementations, and configuration structures. The updates streamline the approach to rate limiting, making it more efficient by treating order placements and cancellations uniformly and extending support for batch cancel operations. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| lib.AssertCheckTxMode(ctx) | ||
|
|
||
| // calcualate how mcuh each batch cancel should be weighted. Use 2 for now. | ||
| weight := uint32(2) |
There was a problem hiding this comment.
TODO: check the ratio of how much work it takes to gossip/deserialize/sigverify messages compared to actually processing messages and then set this to be X + YperId so like ceil(1+ 0.1 per ID)
| dasel put -t int -f "$GENESIS" '.app_state.clob.block_rate_limit_config.max_short_term_orders_per_n_blocks.[0].limit' -v '200' | ||
| dasel put -t int -f "$GENESIS" '.app_state.clob.block_rate_limit_config.max_short_term_orders_per_n_blocks.[0].num_blocks' -v '1' | ||
| # Max 50 short term order cancellations per block | ||
| dasel put -t json -f "$GENESIS" '.app_state.clob.block_rate_limit_config.max_short_term_order_cancellations_per_n_blocks.[]' -v "{}" |
There was a problem hiding this comment.
don't know why the comment says 50 per block when it's 200 per block. Set to be 400 per block for the combined version for now.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/clob/types/block_rate_limit_config.pb.gois excluded by:!**/*.pb.go
Files selected for processing (24)
- proto/dydxprotocol/clob/block_rate_limit_config.proto (2 hunks)
- protocol/app/app.go (1 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/testing/genesis.sh (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/x/clob/ante/rate_limit.go (2 hunks)
- protocol/x/clob/e2e/rate_limit_test.go (7 hunks)
- protocol/x/clob/genesis_test.go (4 hunks)
- protocol/x/clob/keeper/block_rate_limit_config.go (2 hunks)
- protocol/x/clob/keeper/grpc_query_block_rate_limit_configuration_test.go (3 hunks)
- protocol/x/clob/keeper/keeper.go (3 hunks)
- protocol/x/clob/keeper/msg_server_update_block_rate_limit_config_test.go (4 hunks)
- protocol/x/clob/keeper/rate_limit.go (2 hunks)
- protocol/x/clob/module_test.go (2 hunks)
- protocol/x/clob/rate_limit/multi_block_rate_limiter.go (4 hunks)
- protocol/x/clob/rate_limit/noop_rate_limiter.go (1 hunks)
- protocol/x/clob/rate_limit/order_rate_limiter.go (6 hunks)
- protocol/x/clob/rate_limit/panic_rate_limiter.go (1 hunks)
- protocol/x/clob/rate_limit/rate_limit.go (1 hunks)
- protocol/x/clob/rate_limit/single_block_rate_limiter.go (2 hunks)
- protocol/x/clob/types/block_rate_limit_config.go (2 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
- protocol/x/clob/types/genesis_test.go (6 hunks)
Additional comments: 34
protocol/x/clob/rate_limit/rate_limit.go (1)
- 11-13: The addition of
RateLimitIncrBymethod to theRateLimiterinterface is a logical extension to support the unified rate limiting approach. This method allows for more flexible rate limiting operations, such as incrementing the rate limit counter by a specific amount, which is particularly useful for batch operations. Ensure that all implementations of theRateLimiterinterface are updated to include this new method.protocol/x/clob/rate_limit/noop_rate_limiter.go (1)
- 22-24: The implementation of
RateLimitIncrByin thenoOpRateLimiterstruct correctly adheres to the expected behavior of a no-operation rate limiter by immediately returningnil, indicating no error. This ensures that the method is compatible with the interface while maintaining the no-op behavior.protocol/x/clob/rate_limit/panic_rate_limiter.go (1)
- 22-24: The implementation of
RateLimitIncrByin thepanicRateLimiterstruct correctly panics, consistent with the intended behavior of this rate limiter variant. This ensures that any attempt to use this rate limiter in a production environment will be immediately noticeable during development or testing phases, aiding in the identification of misconfigurations.proto/dydxprotocol/clob/block_rate_limit_config.proto (1)
- 10-16: The update to the
BlockRateLimitConfigurationmessage, specifically the renaming ofmax_short_term_orders_per_n_blockstomax_short_term_orders_and_cancels_per_n_blocks, effectively communicates the unified approach to rate limiting for both order placements and cancellations. This change simplifies the configuration and is consistent with the PR's objectives. Ensure that all references to this configuration in the codebase are updated accordingly.protocol/x/clob/ante/rate_limit.go (1)
- 11-19: The update to include
MsgBatchCancelin theClobRateLimitDecoratoris a significant improvement, aligning with the PR's objective to unify rate limiting across different types of order transactions. This change ensures that batch cancel requests are also subject to rate limiting, enhancing the system's overall robustness. Ensure that the implementation ofRateLimitBatchCancelin theclobKeeperis thoroughly tested to handle various scenarios.protocol/x/clob/rate_limit/single_block_rate_limiter.go (1)
- 37-41: Refactoring the
RateLimitmethod to delegate toRateLimitIncrBywith an increment value of 1 is a clean and efficient way to streamline the rate limiting logic. This change not only reduces code duplication but also enhances the flexibility of the rate limiting mechanism. Ensure that this refactoring does not introduce any regressions by thoroughly testing both methods under various scenarios.protocol/x/clob/keeper/block_rate_limit_config.go (1)
- 40-40: Introducing the
placeCancelOrderRateLimiterto handle both placing and canceling orders under a single rate limiter is a key part of achieving the PR's objective of unifying rate limiting mechanisms. This change simplifies the rate limiting logic and should improve maintainability. Ensure that theNewPlaceCancelOrderRateLimiterfunction is implemented correctly and that it accurately reflects the unified rate limiting strategy.protocol/x/clob/keeper/msg_server_update_block_rate_limit_config_test.go (1)
- 35-47: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [26-44]
The test updates, including the renaming of
MaxShortTermOrdersPerNBlockstoMaxShortTermOrdersAndCancelsPerNBlocksand the adjustment of test data, correctly reflect the changes made to theBlockRateLimitConfiguration. These updates are crucial for ensuring that the tests accurately assess the new unified rate limiting strategy. It's important to verify that all edge cases are covered in the tests to ensure the robustness of the rate limiting logic.protocol/x/clob/types/block_rate_limit_config.go (1)
- 8-27: The updates to the
BlockRateLimitConfigurationstruct, including the introduction of new constants for maximum limits and the updated validation logic, are well-aligned with the PR's objective of unifying rate limiting mechanisms. These changes ensure that the configuration adheres to the new unified strategy while maintaining robust validation to prevent misconfigurations. It's crucial to ensure that these new limits are communicated clearly to the users of the system to avoid confusion.protocol/x/clob/types/clob_keeper.go (1)
- 126-126: The addition of
RateLimitBatchCancelmethod to theClobKeeperinterface aligns with the PR's objective to unify rate limiting mechanisms. This method specifically addresses the rate limiting for batch cancel operations, which is a crucial part of the consolidation effort.protocol/x/clob/keeper/rate_limit.go (4)
- 34-34: The update to use
placeCancelOrderRateLimiterinstead ofcancelOrderRateLimiterinRateLimitCancelOrderfunction is consistent with the PR's goal of consolidating rate limiters. This change ensures that both order placements and cancellations are handled by a single rate limiter, simplifying the logic and potentially improving performance.- 62-62: Similarly, the update in
RateLimitPlaceOrderto useplaceCancelOrderRateLimiteraligns with the consolidation effort. By using a unified rate limiter for both placing and canceling orders, the system's rate limiting mechanism becomes more streamlined and easier to manage.- 65-90: The addition of
RateLimitBatchCancelfunction is a key part of the PR's objective to unify rate limiting mechanisms. This function specifically addresses rate limiting for batch cancel operations, ensuring that these operations are also subject to the consolidated rate limiting framework. The implementation checks for valid clob pairs and ensures the GTB (Good Til Block) is valid before rate limiting, which is crucial for preventing replay attacks. This thorough approach enhances the security and efficiency of the rate limiting process.- 94-94: Updating
PruneRateLimitsto operate onplaceCancelOrderRateLimiterinstead ofplaceOrderRateLimiteris a logical continuation of the consolidation effort. This ensures that the pruning process is aligned with the unified rate limiting strategy, maintaining the system's efficiency and consistency.protocol/x/clob/rate_limit/multi_block_rate_limiter.go (2)
- 70-71: Modifying the
RateLimitmethod to callRateLimitIncrBywith an increment of 1 is a smart refactor that enhances the flexibility of the rate limiting mechanism. This change allows for more granular control over rate limiting, which can be particularly useful in the context of the unified rate limiting framework introduced by this PR.- 88-94: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [73-101]
The addition of
RateLimitIncrBymethod is a significant improvement to themultiBlockRateLimiter's capabilities. This method allows for incrementing rate limits by a specified amount, which is essential for handling operations like batch cancels that may need to be weighted differently than single operations. The implementation ensures that rate limits are accurately tracked and enforced, contributing to the overall goal of a more cohesive and scalable rate limiting strategy.protocol/x/clob/rate_limit/order_rate_limiter.go (2)
- 12-29: Renaming the
placeOrderRateLimiterstruct toplaceAndCancelOrderRateLimiterand expanding its functionality to include rate limiting for order cancellation messages is a clear reflection of the PR's consolidation objectives. This change not only simplifies the naming convention but also accurately represents the broader scope of operations the rate limiter now handles, enhancing clarity and maintainability.- 82-108: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [85-165]
The introduction of the
RateLimitBatchCancelOrdermethod within theplaceAndCancelOrderRateLimiterstruct is a crucial addition that supports the unified rate limiting approach for batch cancel operations. This method's implementation, particularly the decision to assign a weight to batch cancel operations, demonstrates a thoughtful approach to handling different types of transactions within the unified framework. It's important to ensure that the chosen weight accurately reflects the impact of batch cancel operations on the system to maintain fairness and efficiency in rate limiting.protocol/x/clob/keeper/keeper.go (1)
- 59-59: Merging the
placeOrderRateLimiterandcancelOrderRateLimiterinto a singleplaceCancelOrderRateLimiterin theKeeperstruct is a significant change that directly supports the PR's goal of unifying rate limiting mechanisms. This consolidation simplifies the rate limiting logic and potentially improves the system's performance by handling all order-related rate limiting through a single, cohesive framework.protocol/testutil/keeper/clob.go (1)
- 221-221: Updating the
createClobKeeperfunction to use a unified rate limiter forsdk.Msgin the test utilities is a necessary change to align the testing environment with the new consolidated rate limiting framework introduced in the PR. This ensures that tests accurately reflect the system's behavior under the updated rate limiting strategy, contributing to more reliable and relevant test outcomes.protocol/x/clob/genesis_test.go (2)
- 38-38: The renaming of
MaxShortTermOrdersPerNBlockstoMaxShortTermOrdersAndCancelsPerNBlocksin theBlockRateLimitConfigstruct is consistent with the PR's objective to combine rate limiters for order placements and cancellations. This change correctly reflects the updated field name in the test case setup.- 372-378: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [375-392]
The test case "Genesis state is invalid when BlockRateLimitConfiguration is invalid" has been correctly updated to use the new field name
MaxShortTermOrdersAndCancelsPerNBlocks. The test logic remains valid and accurately tests the scenario whereNumBlocksis set to an invalid value (0), ensuring the system correctly identifies and handles invalid configurations.protocol/x/clob/module_test.go (1)
- 340-340: The renaming of
MaxShortTermOrdersPerNBlockstoMaxShortTermOrdersAndCancelsPerNBlocksin theBlockRateLimitConfigurationstruct within the test setup is consistent with the PR's objective and the changes made in other parts of the codebase. This change correctly reflects the updated field name and aligns with the unified rate limiting strategy.protocol/x/clob/types/genesis_test.go (6)
- 28-35: The renaming of
MaxShortTermOrdersPerNBlocksandMaxShortTermOrderCancellationsPerNBlockstoMaxShortTermOrdersAndCancelsPerNBlocksaligns with the PR's objective of consolidating rate limiters. This change simplifies the configuration by treating order placements and cancellations under a single framework. Ensure that all references to these variables throughout the codebase have been updated accordingly.- 262-270: The test case "max num blocks for short term order rate limit is zero" correctly checks for an invalid configuration where
NumBlocksis set to 0 forMaxShortTermOrdersAndCancelsPerNBlocks. This validation is crucial for maintaining the integrity of the rate limiting configuration and preventing misconfigurations that could lead to unbounded rate limiting.- 275-283: The test case "max limit for short term order rate limit is zero" properly validates that a limit of 0 for
MaxShortTermOrdersAndCancelsPerNBlocksis considered invalid. This check ensures that there is a meaningful limit set for the rate of orders and cancellations, supporting the system's stability and fairness.- 314-323: The test case "max num blocks for short term order rate limit is greater than max" introduces a validation for an upper bound on
NumBlocksforMaxShortTermOrdersAndCancelsPerNBlocks. This is a good practice to ensure that the configuration does not exceed expected limits, potentially leading to unintended behavior in rate limiting.- 328-337: The test case "max limit for short term order rate limit is greater than max" checks for an upper limit on the
Limitvalue forMaxShortTermOrdersAndCancelsPerNBlocks. This validation is important for preventing overly permissive rate limits that could compromise the system's performance or fairness.- 364-373: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [370-379]
The test case "duplicate short term order rate limit NumBlocks not allowed" ensures that duplicate
NumBlocksconfigurations forMaxShortTermOrdersAndCancelsPerNBlocksare not permitted. This check is essential for maintaining a clear and unambiguous rate limiting configuration, preventing potential conflicts or inconsistencies in the rate limiting behavior.protocol/x/clob/e2e/rate_limit_test.go (2)
- 28-28: The renaming of rate limit configuration variables to
MaxShortTermOrdersAndCancelsPerNBlocksreflects the PR's objective to unify rate limiting for order placements and cancellations. This change is consistent across multiple test scenarios, indicating a thorough update to align with the new unified rate limiting strategy. However, ensure that the corresponding logic in the application that interprets these configurations has been updated accordingly to handle the new unified rate limit settings.Also applies to: 40-40, 76-76, 88-88
- 491-491: The introduction of tests for guarding against replay attacks for both short-term order placements and cancellations under the new unified rate limiting configuration is a positive addition. These tests ensure that the system correctly handles scenarios where orders might be replayed with different
GoodTilBlockvalues. It's crucial to verify that the system's logic for handling these cases aligns with the expected behavior outlined in these tests, particularly in rejecting orders that do not meet the criteria set by the unified rate limiting configuration.Also applies to: 505-505
protocol/lib/metrics/constants.go (1)
- 95-95: The addition of the
BatchCancelconstant is consistent with the PR's objective to streamline rate limiting mechanisms. It's well-placed within theCLOBsection of constants, which suggests its relevance to the Central Limit Order Book operations.Ensure that the integration and usage of
BatchCancelacross the application are verified, especially in areas related to rate limiting and metrics tracking.Verification successful
The search results confirm the integration and usage of
BatchCancelacross the application, including in rate limiting mechanisms, metrics tracking/logging, and core business logic related to the Central Limit Order Book (CLOB) operations. This aligns with the PR's objective and demonstrates a thorough implementation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of BatchCancel across the application rg "BatchCancel" --type goLength of output: 20062
protocol/mocks/ClobKeeper.go (1)
- 847-859: The addition of the
RateLimitBatchCancelmock function aligns well with the PR's objective to unify rate limiting mechanisms. The function signature and implementation are correctly structured for testing purposes, using standard mocking practices. This change supports the new unified rate limiting approach for batch cancel operations, enhancing the testability of the related logic.protocol/testing/genesis.sh (1)
- 1407-1410: The changes from lines 1407 to 1410 update the block rate limit configuration to allow a maximum of 400 short-term orders/cancels per block instead of the previous limit. This adjustment is in line with the PR's objective to streamline and unify the rate limiting mechanisms for order placements, cancellations, and batch cancellations. By increasing the limit, the system can handle a higher volume of transactions, which could improve the overall efficiency and responsiveness of the trading protocol. It's important to ensure that this new limit has been thoroughly tested under various load scenarios to confirm that it does not adversely affect the system's stability or performance.
| @@ -19,7 +20,7 @@ func TestGetBlockRateLimitConfiguration(t *testing.T) { | |||
| req: &types.QueryBlockRateLimitConfigurationRequest{}, | |||
| res: &types.QueryBlockRateLimitConfigurationResponse{ | |||
| BlockRateLimitConfig: types.BlockRateLimitConfiguration{ | |||
| MaxShortTermOrdersPerNBlocks: []types.MaxPerNBlocksRateLimit{ | |||
| MaxShortTermOrdersAndCancelsPerNBlocks: []types.MaxPerNBlocksRateLimit{ | |||
There was a problem hiding this comment.
The test data update to use MaxShortTermOrdersAndCancelsPerNBlocks instead of MaxShortTermOrdersPerNBlocks correctly reflects the changes made to the BlockRateLimitConfiguration message. This ensures that the tests remain relevant and accurate after the configuration changes. However, there seems to be a typo in the Limit value for MaxStatefulOrdersPerNBlocks (Limit: 02). Please verify if this is intentional or a mistake.
- Limit: 02,
+ Limit: 2,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.
| MaxShortTermOrdersAndCancelsPerNBlocks: []types.MaxPerNBlocksRateLimit{ |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/x/clob/types/block_rate_limit_config.pb.gois excluded by:!**/*.pb.go
Files selected for processing (6)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (6 hunks)
- proto/dydxprotocol/clob/block_rate_limit_config.proto (2 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/clob/module_test.go (5 hunks)
- protocol/x/clob/rate_limit/order_rate_limiter.go (6 hunks)
- protocol/x/clob/rate_limit/panic_rate_limiter.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- proto/dydxprotocol/clob/block_rate_limit_config.proto
- protocol/x/clob/module_test.go
- protocol/x/clob/rate_limit/order_rate_limiter.go
- protocol/x/clob/rate_limit/panic_rate_limiter.go
Additional comments: 6
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (5)
- 6-6: The renaming of
maxShortTermOrdersPerNBlockstomaxShortTermOrdersAndCancelsPerNBlocksclarifies the scope of the rate limit, aligning well with the PR's objective to consolidate rate limiting mechanisms. Good job on making the configuration more intuitive.- 21-21: Renaming
max_short_term_orders_per_n_blockstomax_short_term_orders_and_cancels_per_n_blocksin theBlockRateLimitConfigurationSDKTypeinterface maintains consistency with the non-SDK type interface and enhances clarity. This is a positive change.- 66-67: Initialization of
maxShortTermOrdersAndCancelsPerNBlocksandmaxStatefulOrdersPerNBlocksas empty arrays in thecreateBaseBlockRateLimitConfigurationfunction correctly reflects the updated interface structure. This is standard practice for base configuration creation.- 63-83: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [73-94]
The updates to the encoding and decoding logic for
maxShortTermOrdersAndCancelsPerNBlocksin theBlockRateLimitConfigurationobject ensure correct handling of the renamed configuration during protobuf serialization and deserialization. These changes are necessary and correctly implemented.
- 112-112: The update to the
fromPartialmethod in theBlockRateLimitConfigurationobject to handlemaxShortTermOrdersAndCancelsPerNBlockscorrectly reflects the updated interface. This method is essential for creating instances from partial objects, and the change is correctly implemented.protocol/testutil/constants/genesis.go (1)
- 221-224: The change from
max_short_term_orders_per_n_blockstomax_short_term_orders_and_cancels_per_n_blocksand the adjustment of the limit value from 200 to 400 aligns with the PR's objective to streamline rate limiting mechanisms. However, it's crucial to assess the broader impact of this change on system performance and the effectiveness of rate limiting, especially considering the doubling of the limit value.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.jsonprotocol/testing/containertest/preupgrade_genesis.jsonis excluded by:!**/*.json
Files selected for processing (2)
- protocol/testutil/constants/genesis.go (2 hunks)
- protocol/x/clob/module_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/testutil/constants/genesis.go
- protocol/x/clob/module_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (2)
- protocol/x/clob/keeper/grpc_query_block_rate_limit_configuration_test.go (3 hunks)
- protocol/x/clob/module_test.go (5 hunks)
Check Runs (18)
install-runsim completed (5)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (1)
v4-proto-py-gen completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (1)
v4-proto-js-gen completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (1)
protocol-gen completed (1)
indexer-gen completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (1)
check-bc-breaking completed (2)
check-build-bazooka completed (1)
validate-yaml completed (1)
check-sample-pregenesis-up-to-date completed (1)
lint completed (1)
golangci-lint completed (5)
build completed (1)
format completed (1)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/keeper/grpc_query_block_rate_limit_configuration_test.go
- protocol/x/clob/module_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/clob/types/block_rate_limit_config.pb.gois excluded by:!**/*.pb.go
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (8 hunks)
- proto/dydxprotocol/clob/block_rate_limit_config.proto (2 hunks)
- protocol/x/clob/rate_limit/order_rate_limiter.go (6 hunks)
- protocol/x/clob/types/block_rate_limit_config.go (2 hunks)
Check Runs (18)
install-runsim completed (5)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (1)
v4-proto-py-gen completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (1)
v4-proto-js-gen completed (1)
protocol-gen completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (1)
lint completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (1)
indexer-gen completed (1)
check-build-bazooka completed (1)
validate-yaml completed (1)
format completed (1)
check-bc-breaking completed (1)
build completed (1)
golangci-lint completed (1)
check-sample-pregenesis-up-to-date completed (2)
Files skipped from review as they are similar to previous changes (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts
- proto/dydxprotocol/clob/block_rate_limit_config.proto
- protocol/x/clob/types/block_rate_limit_config.go
Additional comments: 3
protocol/x/clob/rate_limit/order_rate_limiter.go (3)
- 11-18: The renaming of
placeOrderRateLimitertoplaceAndCancelOrderRateLimiterand the expansion of its functionality to include rate limiting for order cancellations and batch cancellations is a positive change. It aligns with the PR's objective of unifying rate limiting mechanisms. This change enhances maintainability and simplifies the logic by treating order placements, cancellations, and batch cancellations under a single framework.- 8-31: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [25-39]
The
NewPlaceCancelOrderRateLimiterfunction's renaming and its updated documentation clearly reflect the broader scope of rate limiting operations, including order placements, cancellations, and batch cancellations. This is a good practice as it improves code readability and maintainability. However, ensure that all references to the oldNewPlaceOrderRateLimiterfunction throughout the codebase are updated toNewPlaceCancelOrderRateLimiterto avoid any potential issues.Verification successful
The search for the old function name
NewPlaceOrderRateLimiterdid not yield any results, indicating that it has likely been fully replaced in the codebase with the new nameNewPlaceCancelOrderRateLimiter. This supports the initial review comment about ensuring all references to the old function name are updated.* 84-95: The `RateLimit` method's switch statement correctly handles different message types (`MsgCancelOrder`, `MsgPlaceOrder`, `MsgBatchCancel`) by delegating to the appropriate rate limiting method. This modular approach facilitates easy extension or modification of rate limiting logic for different message types in the future. Good use of polymorphism and clean separation of concerns.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for old function name usage to ensure it's fully replaced. rg --type go 'NewPlaceOrderRateLimiter'Length of output: 39
| func (r *placeAndCancelOrderRateLimiter) RateLimitIncrBy(ctx sdk.Context, msg sdk.Msg, incrBy uint32) (err error) { | ||
| panic("PlaceAndCancelOrderRateLimiter is a top-level rate limiter. It should not use IncrBy.") | ||
| } |
There was a problem hiding this comment.
The panic in RateLimitIncrBy method serves as a safeguard against incorrect usage of the PlaceAndCancelOrderRateLimiter as a top-level rate limiter. This explicit error handling strategy prevents subtle bugs and enforces correct usage patterns. However, consider documenting this behavior in the method's comments to inform users of the intended usage constraints.
+ // RateLimitIncrBy panics because PlaceAndCancelOrderRateLimiter is a top-level rate limiter and should not use IncrBy.
func (r *placeAndCancelOrderRateLimiter) RateLimitIncrBy(ctx sdk.Context, msg sdk.Msg, incrBy uint32) (err error) {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.
| func (r *placeAndCancelOrderRateLimiter) RateLimitIncrBy(ctx sdk.Context, msg sdk.Msg, incrBy uint32) (err error) { | |
| panic("PlaceAndCancelOrderRateLimiter is a top-level rate limiter. It should not use IncrBy.") | |
| } | |
| // RateLimitIncrBy panics because PlaceAndCancelOrderRateLimiter is a top-level rate limiter and should not use IncrBy. | |
| func (r *placeAndCancelOrderRateLimiter) RateLimitIncrBy(ctx sdk.Context, msg sdk.Msg, incrBy uint32) (err error) { | |
| panic("PlaceAndCancelOrderRateLimiter is a top-level rate limiter. It should not use IncrBy.") | |
| } |
| lib.AssertCheckTxMode(ctx) | ||
|
|
||
| // calcualate how mcuh each batch cancel should be weighted. Use 2 for now. | ||
| weight := uint32(2) |
There was a problem hiding this comment.
Given the previous comment by jonfung-dydx regarding the need to adjust the weight for batch cancel operations based on the work required for gossip/deserialize/sigverify messages, it's crucial to revisit this calculation. The hardcoded value of 2 for weight may not accurately reflect the actual cost of processing batch cancel orders. Consider conducting a performance analysis to determine a more accurate weighting strategy.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.json
Files selected for processing (1)
- protocol/x/clob/module_test.go (5 hunks)
Check Runs (18)
install-runsim completed (5)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (1)
v4-proto-py-gen completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (1)
protocol-gen completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (1)
v4-proto-js-gen completed (1)
lint completed (1)
check-build-bazooka completed (1)
indexer-gen completed (1)
build completed (1)
validate-yaml completed (1)
format completed (1)
golangci-lint completed (2)
check-bc-breaking completed (1)
check-sample-pregenesis-up-to-date completed (2)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/module_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/e2e/app_test.go (1 hunks)
- protocol/x/clob/e2e/rate_limit_test.go (13 hunks)
- protocol/x/clob/module_test.go (5 hunks)
Check Runs (24)
install-runsim completed (2)
build completed (1)
unit-end-to-end-and-integration completed (2)
test-race completed (2)
liveness-test completed (2)
test-coverage-upload completed (2)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (2)
v4-proto-py-gen completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (2)
v4-proto-js-gen completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (2)
protocol-gen completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (2)
lint completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (2)
test / run_command completed (2)
indexer-gen completed (1)
check-build-bazooka completed (2)
format completed (1)
validate-yaml completed (1)
container-tests completed (2)
check-sample-pregenesis-up-to-date completed (2)
check-bc-breaking completed (1)
golangci-lint completed (2)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/e2e/rate_limit_test.go
- protocol/x/clob/module_test.go
Additional comments: 1
protocol/x/clob/e2e/app_test.go (1)
- 227-266: The addition of multiple
MsgBatchCancelinstances with differentGTB(Good Till Block) values is a significant change. It's crucial to ensure that these batch cancel operations are correctly tested across various scenarios to validate the unified rate limiting system's behavior. Consider adding more comprehensive tests that specifically target the rate limiting logic, especially since the PR's objective is to consolidate rate limiters. This would help ensure that the unified rate limiter behaves as expected under different load conditions and order/cancellation patterns.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go (1 hunks)
- protocol/x/clob/e2e/app_test.go (3 hunks)
- protocol/x/clob/e2e/rate_limit_test.go (14 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/clob/e2e/app_test.go
- protocol/x/clob/e2e/rate_limit_test.go
Additional comments: 2
protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go (2)
- 21-21: The addition of
MaxShortTermOrdersAndCancelsPerNBlockstoemptyConfigaligns with the PR's objective of unifying rate limiters. Ensure this change is consistently reflected in related configurations and tests across the entire codebase.Verification successful
The usage of
MaxShortTermOrdersAndCancelsPerNBlocksacross various parts of the codebase, including configuration files, rate limiting logic, and multiple test cases, confirms its consistent integration in line with the PR's objectives. This comprehensive update suggests that the addition and modifications related to this field have been thoroughly implemented and tested.* 4-24: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [24-39]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the new field is consistently used in configurations and tests rg "MaxShortTermOrdersAndCancelsPerNBlocks" --type goLength of output: 7909
The test
TestCmdGetBlockRateLimitConfigurationappears to be correctly implemented, ensuring the CLI command's functionality is as expected. Consider adding more detailed tests to cover scenarios specific to the new unified rate limiting logic, if not already covered elsewhere.Would you like assistance in drafting additional test cases to cover the new unified rate limiting logic?
| repeated MaxPerNBlocksRateLimit max_short_term_orders_per_n_blocks = 1 | ||
| [ (gogoproto.nullable) = false ]; | ||
| [ (gogoproto.nullable) = false, deprecated = true ]; |
There was a problem hiding this comment.
should we just remove this? and mark 1 and 3 as reserved?
There was a problem hiding this comment.
we haven't removed any fields in production so I don't know if this breaks things. but I feel like this is probably the way to do it going forwards
There was a problem hiding this comment.
hmm reserved makes it non-backwards compatible. That is why I chose deprecated. 🤔
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/testing/containertest/preupgrade_genesis.jsonis excluded by:!**/*.json
Files selected for processing (1)
- protocol/x/clob/module_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/module_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/upgrades/v5.0.0/upgrade.go (3 hunks)
Additional comments: 2
protocol/app/upgrades/v5.0.0/upgrade.go (2)
- 34-62: The
blockRateLimitConfigUpdatefunction correctly initializes the block rate limit configuration. However, it would be beneficial to add comments explaining the source or rationale behind the hardcoded rate limit values. This would improve maintainability and clarity for future developers or maintainers of the code.- 77-79: The modifications to
CreateUpgradeHandlerto include theblockRateLimitConfigUpdatefunction as part of the upgrade process are correctly implemented and align with the objectives of the PR.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/app/upgrades.go (1 hunks)
Additional comments: 1
protocol/app/upgrades.go (1)
- 34-34: The addition of
app.ClobKeeperto the arguments ofv5_0_0.CreateUpgradeHandleris noted. This change implies an expansion of the upgrade handler's functionality to potentially include operations related to theClobKeeper. Ensure that this integration is thoroughly tested and consistent with the objectives of the PR, specifically the consolidation of rate limiters and any related upgrade processes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/clob/types/block_rate_limit_config.pb.gois excluded by:!**/*.pb.go
Files selected for processing (1)
- proto/dydxprotocol/clob/block_rate_limit_config.proto (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/dydxprotocol/clob/block_rate_limit_config.proto
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/app/testdata/default_genesis_state.jsonis excluded by:!**/*.jsonprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (4)
- protocol/app/upgrades/v5.0.0/upgrade.go (3 hunks)
- protocol/mocks/ClobKeeper.go (2 hunks)
- protocol/x/clob/module_test.go (5 hunks)
- protocol/x/clob/types/clob_keeper.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/upgrades/v5.0.0/upgrade.go
- protocol/mocks/ClobKeeper.go
- protocol/x/clob/module_test.go
- protocol/x/clob/types/clob_keeper.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/clob/types/block_rate_limit_config.pb.gois excluded by:!**/*.pb.go
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts (8 hunks)
- proto/dydxprotocol/clob/block_rate_limit_config.proto (2 hunks)
- protocol/app/upgrades/v5.0.0/upgrade.go (3 hunks)
- protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/block_rate_limit_config.ts
- proto/dydxprotocol/clob/block_rate_limit_config.proto
- protocol/app/upgrades/v5.0.0/upgrade.go
- protocol/x/clob/client/cli/query_block_rate_limit_configuration_test.go
No description provided.