feat: reject market-updates that modify enabled field in ante-handler #1962
Conversation
WalkthroughThis update significantly enhances market management in the application by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant AnteHandler
participant MarketMapKeeper
User->>App: Initiate transaction
App->>AnteHandler: Build AnteHandler with MarketMapKeeper
AnteHandler->>MarketMapKeeper: Validate market updates
MarketMapKeeper-->>AnteHandler: Return market data
AnteHandler->>App: Process transaction with validation
App-->>User: Transaction result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/market_update.go (4 hunks)
- protocol/app/ante/market_update_test.go (8 hunks)
- protocol/app/app.go (1 hunks)
Additional comments not posted (13)
protocol/app/ante/market_update.go (5)
22-24: Interface definition looks good.The
MarketMapKeeperinterface is well-defined with a single methodGetAllMarkets.
43-49: Constructor changes look good.The
NewValidateMarketUpdateDecoratorconstructor correctly initializes theMarketMapKeeperparameter.
75-97: Integration of validation logic looks good.The
AnteHandlemethod correctly integrates the call todoMarketsUpdateEnabledValuesfor validating market updates.
131-174: Validation logic and error handling look good.The
doMarketsUpdateEnabledValuesmethod correctly validates market updates and handles errors appropriately.
176-196: Helper function logic and error handling look good.The
marketParamsSliceToMapfunction correctly converts a slice of market parameters into a map and handles errors appropriately.protocol/app/ante/market_update_test.go (5)
Line range hint
4-26: Import changes look good.The new imports for
slinkyliband other dependencies are necessary and correctly added.
Line range hint
28-181: Test cases look good.The
TestIsMarketUpdateTxfunction includes comprehensive test cases that cover all necessary scenarios.
182-195: Test variables look good.The new test variables
testMarketWithEnabledandtestMarketParamsare correctly defined and used.
226-230: Struct changes look good.The
argsstruct is correctly updated with new fieldsmarketMapMarketsandmarketParams.
Line range hint
387-510: Test cases look good.The
TestValidateMarketUpdateDecorator_AnteHandlefunction includes comprehensive test cases that cover all necessary scenarios.protocol/app/ante.go (2)
35-35: Struct changes look good.The
HandlerOptionsstruct is correctly updated with a new fieldMarketMapKeeper.
122-124: Function changes look good.The
NewAnteHandlerfunction correctly integrates theMarketMapKeeperparameter.protocol/app/app.go (1)
1911-1911: Verify the integration ofMarketMapKeeper.Ensure that the
MarketMapKeeperis correctly integrated and utilized within theAnteHandlerto enforce the new validation rules for market updates.Verification successful
Verified the integration of
MarketMapKeeper.The
MarketMapKeeperis correctly integrated and utilized within theAnteHandlerlogic to enforce validation rules for market updates.
protocol/app/ante.go:MarketMapKeeperis part of theAnteHandleroptions.protocol/app/ante/market_update.go:MarketMapKeeperis used inValidateMarketUpdateDecoratorto validate market updates.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `MarketMapKeeper` in the `AnteHandler`. # Test: Search for the `MarketMapKeeper` usage in the `AnteHandler`. Expect: Proper utilization in validation logic. rg --type go -A 10 'MarketMapKeeper' | grep -B 10 -A 10 'AnteHandler'Length of output: 2691
Script:
#!/bin/bash # Description: Verify the actual usage of `MarketMapKeeper` within the `AnteHandler` logic. # Search for the `AnteHandler` implementation and look for `MarketMapKeeper` usage within it. rg --type go -A 20 'AnteHandler' | grep -A 20 'MarketMapKeeper'Length of output: 3115
Script:
#!/bin/bash # Description: Verify the actual usage of `MarketMapKeeper` within the `AnteHandler` implementation. # Search for the `MarketMapKeeper` usage in the context of `AnteHandler` implementation. rg --type go 'MarketMapKeeper' -A 20Length of output: 49910
| } | ||
|
|
||
| // setup initial market-map markets | ||
| for _, market := range tt.args.marketMapMarkets { |
There was a problem hiding this comment.
Can you move this to before creating the market in x/prices? I'm adding a dependency when creating the market in x/prices that the market needs to exist in market map
protocol/app/ante/market_update.go
Outdated
| if !exists { | ||
| // if market does not exist in x/prices, it should be disabled | ||
| if market.Ticker.Enabled { | ||
| return errors.New("newly added market should be disabled") |
There was a problem hiding this comment.
nit: Can we add errors registered to the x/prices module for these?
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
protocol/app/ante/market_update_test.go (1)
387-387: Add test description for clarity.Consider adding a comment to describe the purpose of this test case.
// Test case for accepting a single message with no cross markets (only isolated).
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/app/ante/market_update.go (4 hunks)
- protocol/app/ante/market_update_test.go (8 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/app/ante/market_update.go
Additional comments not posted (10)
protocol/app/ante/market_update_test.go (10)
226-230: Ensure new fields inargsstruct are utilized properly.The
argsstruct now includesmarketMapMarketsandmarketParams. Ensure these fields are correctly utilized in the test cases.
446-461: New test case: Reject a single message with a new market that's enabled.This test case ensures that a new market that is enabled is rejected if it already exists in the market map as disabled.
463-477: New test case: Reject a single update-markets message with a new market that's enabled.This test case ensures that an update-markets message with a new market that's enabled is rejected if it already exists in the market map as disabled.
479-493: New test case: Reject a single message with a new market that's enabled - simulate.This test case ensures that a new market that is enabled is rejected in simulation mode if it already exists in the market map as disabled.
495-508: New test case: Reject a single message with a new market that's enabled, but doesn't exist in x/marketmap.This test case ensures that a new market that is enabled is rejected if it doesn't exist in the market map.
510-523: New test case: Reject a single update-markets message with a new market that's enabled, but doesn't exist in x/marketmap.This test case ensures that an update-markets message with a new market that's enabled is rejected if it doesn't exist in the market map.
525-539: New test case: Reject a single message with a new market that's enabled, but doesn't exist in x/marketmap - simulate.This test case ensures that a new market that is enabled is rejected in simulation mode if it doesn't exist in the market map.
541-556: New test case: Reject single message disabling a market.This test case ensures that a message disabling a market is rejected if the market is enabled in the market map.
558-573: New test case: Reject single update markets message disabling a market.This test case ensures that an update markets message disabling a market is rejected if the market is enabled in the market map.
575-590: New test case: Reject single message disabling a market - simulate.This test case ensures that a message disabling a market is rejected in simulation mode if the market is enabled in the market map.
nivasan1
force-pushed
the
nv/ante-handler-upsert-rejections
branch
from
36fcae4 to
9ec5a76
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/market_update.go (4 hunks)
- protocol/app/ante/market_update_test.go (8 hunks)
- protocol/app/app.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/ante.go
- protocol/app/app.go
Additional comments not posted (13)
protocol/app/ante/market_update.go (7)
22-24: InterfaceMarketMapKeeperlooks good.The
MarketMapKeeperinterface is well-defined and aligns with the PR objectives.
43-49: Constructor update looks good.The
NewValidateMarketUpdateDecoratorconstructor update aligns with the introduction of theMarketMapKeeperinterface.
75-97: Enhanced validation logic inAnteHandlelooks good.The additional validation logic ensures that market updates adhere to specific rules, enhancing the robustness of market state management.
131-174: New methoddoMarketsUpdateEnabledValueslooks good.The
doMarketsUpdateEnabledValuesmethod encapsulates the validation logic, making the code more modular and maintainable.
176-196: New functionmarketParamsSliceToMaplooks good.The
marketParamsSliceToMapfunction includes validation to prevent duplicate entries, ensuring the integrity of market data.
Line range hint
4-4:
Test cases inTestIsMarketUpdateTxlook good.The test cases cover various scenarios, ensuring comprehensive validation of market update transactions.
182-196: Test cases inTestValidateMarketUpdateDecorator_AnteHandlelook good.The new test cases ensure that the enhanced validation logic in the
AnteHandlemethod is thoroughly tested.protocol/app/ante/market_update_test.go (6)
195-195: New test variabletestMarketWithEnabledlooks good.The new test variable is essential for testing various scenarios involving market updates.
226-230: Updatedargsstruct looks good.The updated struct enhances the test's ability to simulate different market states and validate the logic of the
ValidateMarketUpdateDecorator.
387-387: New test cases inTestValidateMarketUpdateDecorator_AnteHandlelook good.The new test cases ensure comprehensive coverage of various states and transitions of market updates.
446-461: New test case for rejecting enabled market updates looks good.The test case ensures that the validation logic correctly rejects unauthorized market updates.
463-477: New test case for rejecting update-markets messages with enabled market looks good.The test case ensures that the validation logic correctly rejects unauthorized market updates.
479-539: New test case for rejecting enabled market updates in simulation looks good.The test case ensures that the validation logic correctly rejects unauthorized market updates in simulation scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/market_update.go (4 hunks)
- protocol/app/ante/market_update_test.go (8 hunks)
- protocol/app/app.go (1 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/ante.go
- protocol/app/app.go
Additional comments not posted (11)
protocol/x/prices/types/errors.go (4)
51-55: LGTM!The error constant
ErrUnsafeMarketUpdateis clear and follows the existing pattern.
57-61: LGTM!The error constant
ErrMarketUpdateChangesMarketMapEnabledValueis clear and follows the existing pattern.
63-67: LGTM!The error constant
ErrMarketDoesNotExistInMarketMapis clear and follows the existing pattern.
69-73: LGTM!The error constant
ErrAdditionOfEnabledMarketis clear and follows the existing pattern.protocol/app/ante/market_update.go (5)
22-24: LGTM!The interface
MarketMapKeeperis clear and follows the existing pattern.
43-49: LGTM!The constructor
NewValidateMarketUpdateDecoratoris clear and follows the existing pattern.
75-97: LGTM!The method
AnteHandleis clear and follows the existing pattern.
131-173: LGTM!The method
doMarketsUpdateEnabledValuesis clear and follows the existing pattern.
175-195: LGTM!The method
marketParamsSliceToMapis clear and follows the existing pattern.protocol/app/ante/market_update_test.go (2)
Line range hint
28-178: LGTM!The test case
TestIsMarketUpdateTxis clear and follows the existing pattern.
Line range hint
233-680: LGTM!The test case
TestValidateMarketUpdateDecorator_AnteHandleis clear and follows the existing pattern.
nivasan1
force-pushed
the
nv/ante-handler-upsert-rejections
branch
from
da537c5 to
e2e7f5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/app/ante.go (2 hunks)
- protocol/app/ante/market_update.go (4 hunks)
- protocol/app/ante/market_update_test.go (8 hunks)
- protocol/app/app.go (1 hunks)
- protocol/x/prices/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/app/ante.go
- protocol/app/ante/market_update.go
- protocol/app/app.go
- protocol/x/prices/types/errors.go
Additional comments not posted (4)
protocol/app/ante/market_update_test.go (4)
4-5: LGTM! New imports are necessary for the added functionality.The new imports are appropriate for the added functionality.
182-193: LGTM! New test cases cover various scenarios.The new test cases for
TestIsMarketUpdateTxadequately cover different scenarios for identifying market update transactions.
264-268: LGTM! Modifications to theargsstruct are necessary for the new test cases.The modifications to the
argsstruct are appropriate for the new test cases.
Line range hint
425-429:
LGTM! New test cases cover various scenarios.The new test cases for
TestValidateMarketUpdateDecorator_AnteHandleadequately cover different scenarios for validating market update transactions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/go.sumis excluded by!**/*.sum
Files selected for processing (2)
- protocol/app/ante/market_update_test.go (9 hunks)
- protocol/go.mod (1 hunks)
Additional comments not posted (18)
protocol/app/ante/market_update_test.go (17)
264-268: Good addition of new fields in theargsstruct.The new fields
marketMapMarketsandmarketParamsenhance the test's ability to simulate different market states.
425-425: Ensure comprehensive test coverage.The test case "accept a single message with no cross markets (only isolated)" is well-defined. Ensure that all edge cases are covered.
485-499: Good test case for rejecting enabled markets.The test case "reject a single message with a new market that's enabled" is crucial for validating the rejection logic.
501-515: Good test case for rejecting update-markets with enabled markets.The test case "reject a single update-markets message with a new market that's enabled" ensures that the update logic is correctly validated.
517-531: Good test case for rejecting enabled markets in simulation mode.The test case "reject a single message with a new market that's enabled - simulate" covers the simulation scenario effectively.
533-546: Good test case for rejecting enabled markets not in market map.The test case "reject a single message with a new market that's enabled, but doesn't exist in x/marketmap" ensures that only existing markets can be enabled.
548-561: Good test case for rejecting update-markets with enabled markets not in market map.The test case "reject a single update-markets message with a new market that's enabled, but doesn't exist in x/marketmap" validates the update logic for non-existent markets.
563-577: Good test case for rejecting enabled markets not in market map in simulation mode.The test case "reject a single message with a new market that's enabled, but doesn't exist in x/marketmap - simulate" covers the simulation scenario for non-existent markets.
579-594: Good test case for rejecting disabling markets.The test case "reject single message disabling a market" ensures that enabled markets cannot be disabled through updates.
596-611: Good test case for rejecting update-markets disabling markets.The test case "reject single update markets message disabling a market" validates the update logic for disabling markets.
613-628: Good test case for rejecting disabling markets in simulation mode.The test case "reject single message disabling a market - simulate" covers the simulation scenario for disabling markets.
630-645: Good test case for adding providers to enabled markets.The test case "adding an additional provider, while market is enabled" ensures that providers can be added to enabled markets.
647-662: Good test case for adding providers to enabled markets in simulation mode.The test case "adding an additional provider, market is enabled - simulate" covers the simulation scenario for adding providers to enabled markets.
664-679: Good test case for adding providers via update-markets to enabled markets.The test case "adding an additional provider via update-markets, while market is enabled" validates the update logic for adding providers to enabled markets.
681-695: Good test case for adding providers to disabled markets.The test case "adding an additional provider, while market is disabled" ensures that providers can be added to disabled markets.
697-711: Good test case for adding providers to disabled markets in simulation mode.The test case "adding an additional provider, while market is disabled - simulate" covers the simulation scenario for adding providers to disabled markets.
713-727: Good test case for adding providers via update-markets to disabled markets.The test case "adding an additional provider via update-markets, while market is disabled" validates the update logic for adding providers to disabled markets.
protocol/go.mod (1)
70-70: Dependency Update: Verify compatibility with the new version ofslinky.The
slinkydependency has been updated to a newer version. Ensure that the new version is compatible and does not introduce any breaking changes.
In This PR
ValidateMarketUpdateDecorator'sAnteHandlerto rejectMarketMapmarket updates that mutate markets as followsPricesKeeperDisabled, any update that results in a market-map violating this will be rejected.PricesKeeperSummary by CodeRabbit
New Features
MarketMapKeeperto enhance market data management in the application.Bug Fixes
Tests