feat: Enforce MarketParam.Pair uniqueness constraint #1193
Conversation
WalkthroughThe recent updates bring enhanced validation mechanisms to prevent market pair duplication in market creation and parameter modification processes. A new function ensures specific market events are absent in Indexer blocks, while checks in creation and modification ensure market pair uniqueness. Test setups and error handling are refined to align with these changes, including a specific error for duplicate market parameter pairs. 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 (
|
![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: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/testutil/keeper/prices.go (1 hunks)
- protocol/x/prices/keeper/market.go (1 hunks)
- protocol/x/prices/keeper/market_param.go (1 hunks)
- protocol/x/prices/keeper/market_param_test.go (1 hunks)
- protocol/x/prices/keeper/market_price_test.go (1 hunks)
- protocol/x/prices/keeper/market_test.go (3 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
Additional comments: 7
protocol/x/prices/types/errors.go (1)
- 27-27: The addition of
ErrMarketParamPairAlreadyExistswith code 205 is correctly implemented and consistent with the established pattern for market-related errors in this file.protocol/x/prices/keeper/market_param.go (1)
- 47-52: The added validation for base/quote uniqueness in
ModifyMarketParamis correctly implemented and follows the error handling pattern of the codebase. However, consider more efficient data structures or indexing mechanisms for uniqueness checks if the number of market params is expected to grow significantly.protocol/x/prices/keeper/market.go (1)
- 39-47: The stateful validation added to
CreateMarketfor ensuring market pair uniqueness is correctly implemented. As withModifyMarketParam, consider more efficient data structures or indexing mechanisms for uniqueness checks if the number of markets is expected to grow significantly.protocol/x/prices/keeper/market_price_test.go (1)
- 138-154: The addition of explicit pair definitions for market prices in the test setup is correctly implemented and necessary for testing the new market pair uniqueness validation logic.
protocol/x/prices/keeper/market_param_test.go (1)
- 112-122: The new test case "Duplicate pair fails" in
TestModifyMarketParam_Errorscorrectly tests the added validation logic for base/quote pair uniqueness and follows best practices for error handling.protocol/x/prices/keeper/market_test.go (1)
- 170-198: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [161-184]
The updates to error messages, adjustments to market param and price IDs, and the introduction of market creation logic with associated checks in
TestCreateMarket_Errorsare correctly implemented. These changes ensure that the new functionality is accurately tested and follow best practices for error handling.protocol/testutil/keeper/prices.go (1)
- 174-183: The addition of
AssertNMarketEventsNotInIndexerBlockis a valuable tool for verifying the correct number of market events in the Indexer block message, especially in the context of testing the enforcement of market pair uniqueness constraints. It's recommended to add a brief comment explaining the function's purpose and usage to aid future maintainability.
| ctx = ctx.WithTxBytes(constants.TestTxBytes) | ||
|
|
||
| mockTimeKeeper.On("Now").Return(constants.TimeT) |
There was a problem hiding this comment.
Why do we need a timekeeper now?
There was a problem hiding this comment.
The other test cases never succesfully create a market param. They all fail before hitting this line
The additional test case relies on a market param already existing so it has to be able to succeed once.
| @@ -44,6 +44,13 @@ func (k Keeper) ModifyMarketParam( | |||
| errorsmod.Wrapf(types.ErrMarketExponentCannotBeUpdated, lib.UintToString(updatedMarketParam.Id)) | |||
| } | |||
|
|
|||
| // Validate base/quote uniqueness | |||
There was a problem hiding this comment.
This doesn't seem necessary? Isn't there already a check that the updatedMarket must already exist in state + we've introduced a uniqueness constraint across all market-pairs in state?
There was a problem hiding this comment.
Looks like this check is actually necessary, as it's possible for a MarketParam update to change the marketParam.Pair, and the updated Pair may conflict with one in state
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/testing/e2e/gov/perpetuals_test.go (2 hunks)
- protocol/testutil/keeper/perpetuals.go (1 hunks)
- protocol/x/perpetuals/keeper/msg_server_update_perpetual_params_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (2 hunks)
Additional comments: 6
protocol/x/perpetuals/keeper/msg_server_update_perpetual_params_test.go (1)
- 27-28: The introduction of specific pairs for test markets (
testMarket1andtestMarket4) is a positive change. It ensures that the tests are more deterministic and closely aligned with the PR's objective to enforce uniqueness constraints on market pairs. This change enhances the robustness of the test suite by explicitly testing the behavior with distinct market pairs.protocol/testutil/keeper/perpetuals.go (1)
- 240-240: Moving the
CreateNMarketsfunction call outside the loop is a significant performance optimization. Previously, ifCreateNMarketswas called inside the loop, it would unnecessarily recreate markets for each iteration, leading to redundant operations and increased test execution time. This change ensures that markets are created once before the loop, which is more efficient and aligns with best practices for setting up test data.protocol/testing/e2e/gov/perpetuals_test.go (2)
- 4-4: The addition of the
fmtimport is necessary for the changes made in the test cases, specifically for generating market parameters with dynamic pairs. This is a good practice as it ensures that the tests cover scenarios with varying market pairs, aligning with the PR's objective to enforce uniqueness constraints.- 280-280: Modifying the generation of market parameters to include a pair based on the index (
pricestest.WithPair(fmt.Sprintf("%d-%d", i, i))) is a thoughtful change. It ensures that each market parameter has a unique pair, which is crucial for testing the enforcement of uniqueness constraints on market pairs. This change enhances the test's ability to validate the new logic introduced in the PR effectively.protocol/x/subaccounts/keeper/subaccount_test.go (2)
- 2022-2026: The test case "2 updates, 1 update involves not-updatable perp" uses a
MarketParamPricewith aPriceValueof 0, which might be intended to simulate a scenario where a perpetual cannot be updated due to its market parameters. However, it's crucial to ensure that the logic for determining whether a perpetual is updatable or not is correctly implemented and tested. This test case seems to focus on the behavior when an update involves a perpetual that cannot be updated, but it's important to verify that the system's behavior aligns with the expected business rules regarding perpetual updates.Please verify that the logic for determining updatable perpetuals correctly handles cases where the market parameters might indicate a non-updatable state. Additionally, ensure that the test case accurately reflects the intended scenarios and that there are corresponding checks in the implementation to prevent updates to non-updatable perpetuals.
- 3609-3613: The test case "2 updates, 1 update involves not-updatable perp" defines market parameter prices for perpetuals with IDs 100 and 101. It's important to ensure that these IDs correspond to perpetuals that are correctly set up in the test environment and that the test accurately simulates the scenario it's designed to test. Specifically, the
PriceValueof 0 for the perpetual with ID 101 might be used to simulate a scenario where the perpetual cannot be updated due to its market parameters.Please verify that the perpetual IDs used in the test case correspond to perpetuals that are correctly set up and that the test's setup accurately reflects the scenario it's intended to test. Additionally, ensure that the logic for handling updates to perpetuals correctly takes into account the market parameters and prevents updates to perpetuals that should not be updated based on their current state or market conditions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/x/prices/keeper/market_param.go (1 hunks)
- protocol/x/prices/keeper/market_param_test.go (3 hunks)
- protocol/x/prices/keeper/msg_server_update_market_param_test.go (1 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/prices/keeper/market_param.go
- protocol/x/prices/keeper/market_param_test.go
- protocol/x/prices/types/errors.go
Additional comments: 1
protocol/x/prices/keeper/msg_server_update_market_param_test.go (1)
- 33-33: The change from a hardcoded
"PIKACHU-XXX"value totestMarketParam.Pairin thePairfield assignment enhances test flexibility and maintainability by using the setup provided byGenerateMarketParamPrice. This is a good practice for writing dynamic tests.Ensure that all relevant test scenarios, especially edge cases, are still adequately covered after this change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/testing/e2e/gov/prices_test.go (1 hunks)
- protocol/testutil/keeper/perpetuals.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/keeper/perpetuals.go
Additional comments: 1
protocol/testing/e2e/gov/prices_test.go (1)
- 31-31: The change to use
GENESIS_MARKET_PARAM.PairforMODIFIED_MARKET_PARAM.Pairis a good practice for ensuring consistency in test setups. This is particularly important for accurately testing the enforcement of uniqueness constraints onMarketParamsduring update operations. Well done on maintaining test accuracy and relevance to the PR objectives.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/prices/keeper/market_param.go (1 hunks)
- protocol/x/prices/keeper/market_param_test.go (3 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/prices/keeper/market_param.go
- protocol/x/prices/keeper/market_param_test.go
- protocol/x/prices/types/errors.go
Changelist
Enforces a uniqueness constraint on Create/Update operations for MarketParams.
This is required for Slinky since it uses Base/Quote as a compound primary key to reason about pairs being posted to chain as well as prices retrieved from the sidecar.
Test Plan
Unit tests included.
Mainnet data also shows no existing pairs with duplicates, so this should just prevent things in addition to avoiding this in governance votes in the interim.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.