[SKI-1] Remove smoothed prices #1215
Conversation
WalkthroughThe recent modifications across various components of the protocol primarily involve the removal of the smoothed prices update mechanism and its related logic. This includes eliminating the initialization and update processes of smoothed prices, adjusting test setups and variable assignments, and refining the price update logic to focus more on immediate market conditions without relying on smoothed prices. These changes streamline the price management process and simplify the codebase by removing unused variables and functions. 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: 2
Configuration used: CodeRabbit UI
Files selected for processing (36)
- protocol/app/app.go (1 hunks)
- protocol/app/prepare/prepare_proposal_test.go (2 hunks)
- protocol/app/process/expected_keepers.go (1 hunks)
- protocol/app/process/full_node_process_proposal_test.go (1 hunks)
- protocol/app/process/market_prices_test.go (3 hunks)
- protocol/app/process/process_proposal.go (2 hunks)
- protocol/app/process/process_proposal_test.go (2 hunks)
- protocol/app/process/transactions_test.go (4 hunks)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/mocks/PricesKeeper.go (1 hunks)
- protocol/testutil/keeper/assets.go (1 hunks)
- protocol/testutil/keeper/clob.go (1 hunks)
- protocol/testutil/keeper/perpetuals.go (1 hunks)
- protocol/testutil/keeper/prices.go (6 hunks)
- protocol/testutil/keeper/rewards.go (1 hunks)
- protocol/testutil/keeper/sending.go (1 hunks)
- protocol/testutil/keeper/subaccounts.go (1 hunks)
- protocol/x/prices/genesis_test.go (4 hunks)
- protocol/x/prices/keeper/grpc_query_market_test.go (4 hunks)
- protocol/x/prices/keeper/keeper.go (2 hunks)
- protocol/x/prices/keeper/keeper_test.go (1 hunks)
- protocol/x/prices/keeper/market_param_test.go (4 hunks)
- protocol/x/prices/keeper/market_price_test.go (5 hunks)
- protocol/x/prices/keeper/market_test.go (4 hunks)
- protocol/x/prices/keeper/msg_server_create_oracle_market_test.go (1 hunks)
- protocol/x/prices/keeper/msg_server_update_market_param_test.go (1 hunks)
- protocol/x/prices/keeper/msg_server_update_market_prices_test.go (4 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (4 hunks)
- protocol/x/prices/keeper/update_price.go (6 hunks)
- protocol/x/prices/keeper/update_price_test.go (4 hunks)
- protocol/x/prices/keeper/update_price_whitebox_test.go (3 hunks)
- protocol/x/prices/keeper/util.go (1 hunks)
- protocol/x/prices/keeper/util_test.go (1 hunks)
- protocol/x/prices/keeper/validate_market_price_updates_test.go (4 hunks)
- protocol/x/prices/module_test.go (1 hunks)
- protocol/x/prices/types/types.go (1 hunks)
Files skipped from review due to trivial changes (5)
- protocol/x/prices/genesis_test.go
- protocol/x/prices/keeper/market_param_test.go
- protocol/x/prices/keeper/msg_server_create_oracle_market_test.go
- protocol/x/prices/keeper/msg_server_update_market_param_test.go
- protocol/x/prices/keeper/validate_market_price_updates_test.go
Additional comments: 44
protocol/x/prices/keeper/keeper_test.go (1)
- 11-11: The change in variable assignment correctly reflects the removal of smoothed prices, as the removed variable was likely related to this feature. Ensure that the
keepertest.PricesKeepers(t)function's updated signature is consistently applied across all test functions that use it.protocol/x/prices/types/types.go (1)
- 40-45: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-48]
The removal of the
UpdateSmoothedPricesmethod from thePricesKeeperinterface aligns with the PR's objective. Ensure that all implementations of this interface and any code that called this method have been appropriately updated or refactored to reflect this change.protocol/x/prices/keeper/update_price_whitebox_test.go (2)
- 12-15: The simplification of constants used in price calculations is a good practice, making the tests more readable and maintainable. Ensure that these constants still provide sufficient coverage for the scenarios being tested.
- 27-42: The refinement of test cases to focus on conditions relevant to proposing prices without smoothed prices is appropriate. Verify that all necessary scenarios are covered, especially those that might have been affected by the removal of smoothed prices.
protocol/x/prices/keeper/keeper.go (1)
- 20-26: The removal of the
marketToSmoothedPricesfield from theKeeperstruct and the adjustment of theauthoritiesparameter to a map in theNewKeeperfunction are consistent with the PR's objectives. Ensure that all references to the removed field have been cleaned up and that the newauthoritiesparameter type is correctly handled throughout the codebase.protocol/app/process/expected_keepers.go (1)
- 19-24: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-48]
The removal of the
UpdateSmoothedPricesmethod from theProcessPricesKeeperinterface aligns with the PR's objective. Ensure that all implementations of this interface and any code that called this method have been appropriately updated or refactored to reflect this change.protocol/app/process/full_node_process_proposal_test.go (1)
- 76-76: The modification in the variable assignment during the test setup correctly reflects the removal of smoothed prices, as the removed variable was likely related to this feature. Ensure that the
keepertest.PricesKeepers(t)function's updated signature is consistently applied across all test functions that use it.protocol/x/prices/keeper/slinky_adapter_test.go (4)
- 16-16: The reassignment of variables within the test functions for getting currency pair information and mocking time provider behavior is appropriate. Ensure that these changes do not impact the tests' ability to accurately reflect the application's behavior without smoothed prices.
- 33-33: Similar to the previous comment, ensure that the adjustments in the test setup for market data IDs do not affect the integrity of the tests and accurately reflect the application's behavior without smoothed prices.
- 57-57: The changes in the test setup for prices for currency pairs are consistent with the PR's objectives. Verify that the tests still provide sufficient coverage for the scenarios being tested, especially in the context of the removal of smoothed prices.
- 81-81: The adjustments in the test setup for bad market data are appropriate. Ensure that these changes accurately reflect the application's behavior and do not introduce any unintended consequences.
protocol/app/process/process_proposal.go (1)
- 3-8: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-76]
The removal of the update of smoothed prices logic within the
ProcessProposalHandlerfunction aligns with the PR's objective. Ensure that this change does not impact the overall functionality of theProcessProposalHandlerand that any dependencies on the removed logic have been appropriately addressed.protocol/testutil/keeper/assets.go (1)
- 59-59: The removal of the unused return value in the
createPricesKeeperfunction call is a good practice for cleaner and more maintainable code.protocol/testutil/keeper/rewards.go (1)
- 45-45: Removing the unused variable from the
createPricesKeeperfunction call enhances code clarity and maintainability.protocol/testutil/keeper/sending.go (1)
- 60-60: Adjusting the
createPricesKeeperfunction call to remove an unused return value is a positive change for code maintainability.protocol/x/prices/keeper/update_price_test.go (1)
- 90-102: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [71-99]
The adjustments made to the test cases, including the removal of smoothed prices and the restructuring to focus on index prices, align with the PR's objective of removing historical price smoothing. It's important to ensure that all related test cases are updated accordingly to reflect this significant change in the application's logic.
protocol/testutil/keeper/subaccounts.go (1)
- 55-55: The modification to remove an unused return value from the
createPricesKeeperfunction call in theSubaccountsKeepersfunction is consistent with best practices for clean and maintainable code.protocol/x/prices/keeper/market_price_test.go (1)
- 25-31: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [28-63]
The corrections made to the test functions, including fixing duplicated variable assignments and ensuring consistency in test setups, contribute to the reliability and clarity of the tests. It's crucial to maintain such standards to ensure the test suite accurately reflects the application's behavior.
protocol/app/process/market_prices_test.go (1)
- 120-126: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-165]
Removing unused variable assignments in the test functions improves code clarity and maintainability. These adjustments are beneficial for keeping the test suite clean and focused.
protocol/x/prices/keeper/util.go (6)
- 9-13: LGTM! This function remains unchanged and continues to serve its purpose effectively.
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [16-32]
The logic for calculating the minimum price change amount is sound and uses big integers appropriately. The panic condition acts as a necessary safeguard.
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [34-39]
The
PriceTuplestruct is a clear and effective way to encapsulate related price values for computations.
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-47]
This function effectively checks if the new price is towards the index price using a clear and straightforward logic.
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [49-54]
This function correctly checks if the index price is crossing between the current and new prices using a clear logic.
- 9-14: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [56-118]
The remaining functions related to price calculations and conditions for price updates are well-designed and use precise arithmetic operations effectively. These changes contribute to the PR's objective of simplifying price update logic.
protocol/x/prices/keeper/update_price.go (1)
- 107-112: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [101-123]
The changes from lines 101 to 123 in
update_price.goremove the consideration of smoothed prices in the decision-making process for proposing price updates. This aligns with the PR's objective to eliminate historical price smoothing. However, ensure that all dependent logic and tests have been updated accordingly to reflect this change. Additionally, consider adding comments to clarify the new logic for future maintainability.protocol/x/prices/keeper/grpc_query_market_test.go (1)
- 18-18: The changes from lines 18, 69, 129, and 180 in
grpc_query_market_test.goinvolve the removal of redundantmockTimeProviderassignments in various test functions. This cleanup enhances the readability and maintainability of the test code. Ensure that the removal of these assignments does not impact the intended test outcomes.protocol/testutil/keeper/prices.go (1)
- 44-50: The changes from lines 44 to 50 in
prices.gosimplify thePricesKeepersfunction by removing themarketToSmoothedPricesparameter. This adjustment is consistent with the PR's objective to eliminate smoothed prices. Ensure that all tests using this utility function have been updated to reflect these changes. Additionally, consider verifying that the removal of this parameter does not inadvertently affect any test scenarios that relied on smoothed prices for their logic.protocol/x/prices/keeper/market_test.go (1)
- 17-17: The changes from lines 17, 91, 190, and 239 in
market_test.gocorrect the assignment of variables related tomockTimeProviderin various test functions. This adjustment ensures that the tests accurately reflect the current state of the application without smoothed prices. It's important to verify that these changes do not impact the intended outcomes of the tests and that all tests remain reliable and accurate.protocol/x/prices/module_test.go (1)
- 52-52: The change in the
createAppModuleWithKeeperfunction reflects an update to thekeeper.PricesKeepersfunction call, removing an unused return value. This simplification improves code clarity and maintainability.protocol/testutil/keeper/perpetuals.go (1)
- 66-66: Correcting the assignment of
pc.MockTimeProviderin thePerpetualsKeepersWithClobHelpersfunction ensures accurate and reliable testing of time-dependent logic. This is a crucial fix for maintaining the integrity of tests.protocol/testutil/keeper/clob.go (1)
- 82-82: Removing an unused return value from the
GenesisInitializerslice initialization in theNewClobKeepersTestContextWithUninitializedMemStorefunction simplifies the test setup process and improves code clarity.protocol/app/process/process_proposal_test.go (1)
- 261-261: Removing the assertion related to
marketToSmoothedPricescomparison in theTestProcessProposalHandler_Errorfunction aligns the test with the updated application logic, following the removal of smoothed prices.protocol/app/process/transactions_test.go (3)
- 111-111: The changes in
TestDecodeProcessProposalTxs_Errorstreamline the test setup by removing an unused variable assignment. This adjustment does not compromise test coverage or introduce new issues.- 191-191: The adjustments in
TestDecodeProcessProposalTxs_Validremove unnecessary variable assignments, streamlining the test setup without affecting its effectiveness.- 321-321: The changes in
TestProcessProposalTxs_Validate_ErrorandTestProcessProposalTxs_Validate_Validadjust the parameter lists and remove unnecessary variable assignments. These modifications are consistent with the PR's objectives and do not compromise the tests' ability to validate proposal transactions accurately.protocol/x/prices/keeper/msg_server_update_market_prices_test.go (4)
- 189-189: The removal of
mockTimeProviderfromkeepertest.PricesKeepersfunction calls simplifies the test setup by eliminating unnecessary mock time provider setup. This change aligns with the PR's objective of removing smoothed prices, as the time provider might have been primarily used for handling time-sensitive smoothed price logic.- 315-315: Similar to the previous comment, the removal of
mockTimeProviderin this section also streamlines the test setup. It's good to see consistency in test setup modifications across different test cases.- 379-379: Again, the removal of
mockTimeProviderin the error handling test cases maintains consistency in the approach to simplifying test setups across the file.- 402-402: The removal of
mockTimeProviderin the panic test case further demonstrates a consistent approach to simplifying test setups. This change is appropriate given the context of the PR and the removal of smoothed prices.protocol/x/prices/keeper/util_test.go (1)
- 15-20: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
TestGetProposalPricefunction and its test cases, as mentioned in the AI-generated summary, is not directly visible in the provided code. However, the presence ofTestGetMinPriceChangeAmountForMarketand other test functions related to price updates and conditions indicates a focus on refining and simplifying the testing of price-related utilities. This aligns with the PR's objective of removing smoothed prices and potentially related functionalities.protocol/lib/metrics/constants.go (1)
- 258-273: The removal of constants related to smoothed prices aligns with the PR's objective. However, it's crucial to ensure these constants are not referenced elsewhere in the codebase.
Verification successful
The executed script did not find any references to the removed constants in the codebase, confirming that the removal aligns with the PR's objective without causing reference errors. This supports the correctness of the changes made.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for removed constants in the codebase to ensure they are not used. rg --type go 'ProposedPriceCrossesOraclePrice|RecentSmoothedPriceDoesNotMeetMinPriceChange|RecentSmoothedPriceCrossesOraclePrice|UpdateSmoothedPrices'Length of output: 152
protocol/app/prepare/prepare_proposal_test.go (2)
- 363-363: The changes from lines 363 to 363 involve the initialization of test keepers. While this change does not directly relate to the removal of smoothed prices, it's essential to ensure that the test setup accurately reflects the current state of the application's architecture and dependencies.
- 443-443: The changes from lines 443 to 443 also involve the initialization of test keepers for a different test case. Similar to the previous comment, this change is crucial for maintaining the integrity and relevance of the test suite in light of architectural modifications.
| appCodec, | ||
| keys[pricesmoduletypes.StoreKey], | ||
| indexPriceCache, | ||
| pricesmoduletypes.NewMarketToSmoothedPrices(pricesmoduletypes.SmoothedPriceTrackingBlockHistoryLength), | ||
| timeProvider, | ||
| app.IndexerEventManager, | ||
| // set the governance and delaymsg module accounts as the authority for conducting upgrades |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [738-738]
The gRPC server is initialized without specifying credentials, which could potentially expose the server to unencrypted connections. It's recommended to secure the gRPC server by using SSL/TLS credentials. You can create credentials using credentials.NewServerTLSFromFile("cert.pem", "cert.key") and pass them to the grpc.NewServer function using the grpc.Creds() option.
- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)
+ creds, err := credentials.NewServerTLSFromFile("cert.pem", "cert.key")
+ if err != nil {
+ panic(err)
+ }
+ app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(creds)), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)| MissingPriceUpdates = "missing_price_updates" | ||
| NumMarketPricesToUpdate = "num_market_prices_to_update" | ||
| PriceChangeRate = "price_change_rate" | ||
| ProposedPriceChangesPriceUpdateDecision = "proposed_price_changes_price_update_decision" |
There was a problem hiding this comment.
Tip
Codebase Verification
The search results confirm that the old constant name ProposedPriceDoesNotMeetMinPriceChange is still being used in the codebase, indicating that not all references to it have been updated as expected. This supports the initial review comment.
Analysis chain
The renaming of ProposedPriceDoesNotMeetMinPriceChange to ProposedPriceChangesPriceUpdateDecision is noted. Ensure all references to the old constant name have been updated throughout the codebase.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the old constant name to ensure it has been updated everywhere.
rg --type go 'ProposedPriceDoesNotMeetMinPriceChange'
Length of output: 510
Changelist
Removes historical price smoothing.
Test Plan
Unit tests updated.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.