[TRA-487] Move currency pair id cache to store #2072
Conversation
WalkthroughThe recent updates across various files involve significant refactoring within the Changes
Possibly related PRs
Suggested labels
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (2)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I'll add upgrade handler to add CurrencyPairID for existing markets in a separate PR |
![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 (62)
- protocol/mocks/AnteDecorator.go (1 hunks)
- protocol/mocks/AppOptions.go (1 hunks)
- protocol/mocks/BankKeeper.go (1 hunks)
- protocol/mocks/BridgeKeeper.go (1 hunks)
- protocol/mocks/BridgeQueryClient.go (1 hunks)
- protocol/mocks/BridgeServiceClient.go (1 hunks)
- protocol/mocks/CacheMultiStore.go (1 hunks)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/mocks/Configurator.go (1 hunks)
- protocol/mocks/DelayMsgKeeper.go (1 hunks)
- protocol/mocks/EthClient.go (1 hunks)
- protocol/mocks/ExchangeConfigUpdater.go (1 hunks)
- protocol/mocks/ExchangeQueryHandler.go (1 hunks)
- protocol/mocks/ExchangeToMarketPrices.go (1 hunks)
- protocol/mocks/ExtendVoteHandler.go (1 hunks)
- protocol/mocks/FileHandler.go (1 hunks)
- protocol/mocks/GrpcClient.go (1 hunks)
- protocol/mocks/GrpcServer.go (1 hunks)
- protocol/mocks/HealthCheckable.go (1 hunks)
- protocol/mocks/IndexerEventManager.go (1 hunks)
- protocol/mocks/IndexerMessageSender.go (1 hunks)
- protocol/mocks/Logger.go (1 hunks)
- protocol/mocks/MarketPairFetcher.go (1 hunks)
- protocol/mocks/MemClob.go (1 hunks)
- protocol/mocks/MemClobKeeper.go (1 hunks)
- protocol/mocks/MsgRouter.go (1 hunks)
- protocol/mocks/MultiStore.go (1 hunks)
- protocol/mocks/OracleClient.go (1 hunks)
- protocol/mocks/PerpetualsClobKeeper.go (1 hunks)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/mocks/PrepareBridgeKeeper.go (1 hunks)
- protocol/mocks/PrepareClobKeeper.go (1 hunks)
- protocol/mocks/PreparePerpetualsKeeper.go (1 hunks)
- protocol/mocks/PriceFeedMutableMarketConfigs.go (1 hunks)
- protocol/mocks/PriceFetcher.go (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (2 hunks)
- protocol/mocks/ProcessBridgeKeeper.go (1 hunks)
- protocol/mocks/ProcessClobKeeper.go (1 hunks)
- protocol/mocks/ProcessPerpetualKeeper.go (1 hunks)
- protocol/mocks/ProcessStakingKeeper.go (1 hunks)
- protocol/mocks/QueryClient.go (1 hunks)
- protocol/mocks/QueryServer.go (1 hunks)
- protocol/mocks/RequestHandler.go (1 hunks)
- protocol/mocks/SendingKeeper.go (1 hunks)
- protocol/mocks/Server.go (1 hunks)
- protocol/mocks/SubaccountsKeeper.go (1 hunks)
- protocol/mocks/TimeProvider.go (1 hunks)
- protocol/mocks/TxBuilder.go (1 hunks)
- protocol/mocks/TxConfig.go (1 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
- protocol/mocks/VaultKeeper.go (1 hunks)
- protocol/x/prices/keeper/keeper.go (4 hunks)
- protocol/x/prices/keeper/market.go (1 hunks)
- protocol/x/prices/keeper/market_param.go (2 hunks)
- protocol/x/prices/keeper/market_test.go (2 hunks)
- protocol/x/prices/keeper/slinky_adapter.go (4 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (2 hunks)
- protocol/x/prices/module.go (3 hunks)
- protocol/x/prices/types/keys.go (1 hunks)
- protocol/x/prices/types/keys_test.go (1 hunks)
- protocol/x/prices/types/types.go (1 hunks)
Files skipped from review due to trivial changes (51)
- protocol/mocks/AnteDecorator.go
- protocol/mocks/AppOptions.go
- protocol/mocks/BankKeeper.go
- protocol/mocks/BridgeKeeper.go
- protocol/mocks/BridgeQueryClient.go
- protocol/mocks/BridgeServiceClient.go
- protocol/mocks/CacheMultiStore.go
- protocol/mocks/ClobKeeper.go
- protocol/mocks/Configurator.go
- protocol/mocks/DelayMsgKeeper.go
- protocol/mocks/EthClient.go
- protocol/mocks/ExchangeConfigUpdater.go
- protocol/mocks/ExchangeQueryHandler.go
- protocol/mocks/ExchangeToMarketPrices.go
- protocol/mocks/ExtendVoteHandler.go
- protocol/mocks/FileHandler.go
- protocol/mocks/GrpcClient.go
- protocol/mocks/GrpcServer.go
- protocol/mocks/HealthCheckable.go
- protocol/mocks/IndexerEventManager.go
- protocol/mocks/IndexerMessageSender.go
- protocol/mocks/Logger.go
- protocol/mocks/MarketPairFetcher.go
- protocol/mocks/MemClob.go
- protocol/mocks/MemClobKeeper.go
- protocol/mocks/MsgRouter.go
- protocol/mocks/MultiStore.go
- protocol/mocks/OracleClient.go
- protocol/mocks/PerpetualsClobKeeper.go
- protocol/mocks/PerpetualsKeeper.go
- protocol/mocks/PrepareBridgeKeeper.go
- protocol/mocks/PrepareClobKeeper.go
- protocol/mocks/PreparePerpetualsKeeper.go
- protocol/mocks/PriceFeedMutableMarketConfigs.go
- protocol/mocks/PriceFetcher.go
- protocol/mocks/PriceUpdateGenerator.go
- protocol/mocks/ProcessBridgeKeeper.go
- protocol/mocks/ProcessClobKeeper.go
- protocol/mocks/ProcessPerpetualKeeper.go
- protocol/mocks/ProcessStakingKeeper.go
- protocol/mocks/QueryClient.go
- protocol/mocks/QueryServer.go
- protocol/mocks/RequestHandler.go
- protocol/mocks/SendingKeeper.go
- protocol/mocks/Server.go
- protocol/mocks/SubaccountsKeeper.go
- protocol/mocks/TimeProvider.go
- protocol/mocks/TxBuilder.go
- protocol/mocks/TxConfig.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
- protocol/mocks/VaultKeeper.go
Additional comments not posted (18)
protocol/x/prices/types/keys_test.go (1)
18-19: Good addition to test coverage.The new assertion in
TestStateKeysensures that theCurrencyPairIDPrefixis correctly defined, enhancing test coverage.protocol/x/prices/types/keys.go (1)
20-22: Addition ofCurrencyPairIDPrefixenhances key management.The introduction of
CurrencyPairIDPrefixprovides a clear and explicit key prefix for currency pair IDs, improving semantic clarity in key management.protocol/x/prices/types/types.go (1)
Line range hint
13-61:
Simplification ofPricesKeeperinterface.The removal of
InitializeCurrencyPairIdCachesimplifies thePricesKeeperinterface. Ensure that the initialization logic is correctly handled elsewhere if needed.Verification successful
Removal of
InitializeCurrencyPairIdCacheis Appropriately HandledThe cache management logic, specifically the use of
AddCurrencyPairIDToCache, ensures that currency pair IDs are added to the cache as needed within the market operations. This indicates that the removal ofInitializeCurrencyPairIdCachefrom thePricesKeeperinterface does not adversely affect the codebase. The initialization and management of the cache are effectively integrated into the existing logic.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the removal of `InitializeCurrencyPairIdCache` has any impact. # Test: Search for any usage of `InitializeCurrencyPairIdCache`. Expect: No occurrences. rg --type go 'InitializeCurrencyPairIdCache'Length of output: 44
Script:
#!/bin/bash # Search for any comments or methods related to currency pair ID initialization. rg --type go 'currency pair id|initialization|cache'Length of output: 18733
Script:
#!/bin/bash # Extract relevant sections from market.go to verify cache handling logic. rg --type go 'currency-pair-id cache' -A 5 -B 5 protocol/x/prices/keeper/market.goLength of output: 455
protocol/x/prices/keeper/keeper.go (2)
43-50: Refactor: SimplifiedNewKeeperfunction.The
NewKeeperfunction no longer initializes the removed cache fields. Ensure that this simplification aligns with the new caching strategy and that theKeeperinitialization remains complete and correct.
19-26: Refactor: Removed cache fields fromKeeperstruct.The
currencyPairIDCacheandcurrencyPairIdCacheInitializedfields have been removed from theKeeperstruct. This change likely indicates a refactoring of the caching mechanism. Ensure that the new caching strategy is well-documented and integrated into the system.Verification successful
No references found for removed cache fields.
The fields
currencyPairIDCacheandcurrencyPairIdCacheInitializedwere removed from theKeeperstruct and are not referenced elsewhere in the codebase. This suggests that their removal is part of a refactoring effort and does not impact other parts of the system. Ensure that any new caching strategy is well-documented and integrated.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if the removed fields `currencyPairIDCache` and `currencyPairIdCacheInitialized` are used elsewhere in the codebase. # Test: Search for references to the removed fields. rg --type go 'currencyPairIDCache|currencyPairIdCacheInitialized'Length of output: 67
protocol/x/prices/keeper/slinky_adapter_test.go (2)
29-29: Improvement: Userequire.Falsefor clarity.The assertion
require.False(t, found)improves readability and clearly communicates the test's intent. This change ensures the test accurately reflects the expected behavior.
53-53: Improvement: Userequire.Falsefor clarity.The assertion
require.False(t, found)enhances readability and aligns with the test's purpose, ensuring the expected behavior is clearly communicated.protocol/x/prices/keeper/market_param.go (2)
60-65: Improvement: Enhanced error handling when removing old currency pair.The code now logs an error if removing the old currency pair from the cache fails. This improvement provides better visibility into potential issues during cache updates.
68-72: Improvement: Enhanced error handling when adding new currency pair.The code now logs an error if adding the new currency pair to the cache fails. This change improves error visibility and aids in troubleshooting.
protocol/x/prices/keeper/slinky_adapter.go (5)
26-29: LGTM!The
getCurrencyPairIDStoremethod correctly initializes a prefix store for currency pair IDs.
87-99: LGTM!The methods
AddCurrencyPairIDToCacheandRemoveCurrencyPairFromCachecorrectly manage cache entries.
Line range hint
48-70: LGTM! But verify cache update logic.The method efficiently retrieves a market ID using cache and iterates over market parameters if necessary.
Ensure that the cache update logic in
AddCurrencyPairIDToCacheis functioning correctly.Verification successful
Cache Update Logic Verified
The
AddCurrencyPairIDToCachefunction correctly updates the cache by storing the market ID associated with a currency pair. The cache update logic is functioning as expected.
- The function converts the currency pair to a string and uses it as a key.
- It marshals the market ID and stores it in the cache.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify cache update logic in `AddCurrencyPairIDToCache`. # Test: Search for the implementation of `AddCurrencyPairIDToCache`. Expect: Correct cache update. ast-grep --lang go --pattern $'func (k Keeper) AddCurrencyPairIDToCache(ctx sdk.Context, marketId uint32, cp slinkytypes.CurrencyPair) { $$$ }'Length of output: 810
74-85: LGTM! But verify unmarshalling process.The method correctly retrieves and unmarshals a market ID from the cache.
Ensure that the unmarshalling process in
MustUnmarshalis error-free.
Line range hint
31-44: LGTM! But verify error handling inGetMarketParam.The method correctly retrieves and converts market parameters to a currency pair.
Ensure that
GetMarketParamhandles errors appropriately and returns valid data.protocol/x/prices/keeper/market.go (1)
78-78: LGTM!The refactoring in
CreateMarketcentralizes cache management, enhancing maintainability.protocol/x/prices/keeper/market_test.go (2)
54-56: LGTM! Verify cache initialization.The added assertions correctly verify the absence of the currency pair in the cache before market creation.
81-84: LGTM! Verify cache update.The assertions confirm the presence of the currency pair in the cache after market creation, ensuring the cache is updated correctly.
protocol/mocks/PricesKeeper.go (1)
1-1: Verify impact of removed method on tests.The
InitializeCurrencyPairIdCachemethod has been removed from the mock. Ensure that any tests relying on this mock method are updated to reflect the new caching mechanism.Verification successful
No impact from removed method
InitializeCurrencyPairIdCache.The
InitializeCurrencyPairIdCachemethod is not referenced anywhere in the codebase, indicating that its removal does not affect any tests or functionality. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if tests are affected by the removal of `InitializeCurrencyPairIdCache`. # Test: Search for occurrences of `InitializeCurrencyPairIdCache` in test files. rg --type go 'InitializeCurrencyPairIdCache'Length of output: 44
Script:
#!/bin/bash # Description: Search for any references to `InitializeCurrencyPairIdCache` in the entire codebase. rg 'InitializeCurrencyPairIdCache'Length of output: 34
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/x/prices/keeper/market.go (1 hunks)
- protocol/x/prices/keeper/market_param.go (2 hunks)
- protocol/x/prices/keeper/slinky_adapter.go (4 hunks)
- protocol/x/prices/types/keys_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/prices/keeper/market.go
- protocol/x/prices/keeper/market_param.go
- protocol/x/prices/types/keys_test.go
Additional comments not posted (5)
protocol/x/prices/keeper/slinky_adapter.go (5)
26-29: LGTM!The
getCurrencyPairIDStoremethod is correctly implemented to return a prefix store for market IDs.
Line range hint
31-44: Efficient data retrieval!The
GetCurrencyPairFromIDmethod now efficiently retrieves market parameters usingGetMarketParam. The error handling is appropriate.
Line range hint
48-71: Good fallback mechanism!The
GetIDForCurrencyPairmethod efficiently retrieves the market ID from the store and provides a fallback to iterate through market parameters. Adding the currency pair to the store after finding it is a good practice.
74-85: Correct store retrieval!The
GetCurrencyPairIDFromStoremethod correctly retrieves the market ID from the store using the currency pair string. The error handling is appropriate.
87-99: Effective store management!The
AddCurrencyPairIDToStoreandRemoveCurrencyPairFromStoremethods effectively manage the addition and removal of currency pair IDs in the store.
chenyaoy
changed the title
| if found { | ||
| return id, true | ||
| return uint64(marketId), true | ||
| } | ||
|
|
||
| // if not found, iterate through all market params and find the id |
There was a problem hiding this comment.
For ease of reasoning, think we should maintain the invariant that the cache is always well-populated, instead of relying on a lazy approach which made more sense when the cache was transient.
This means:
- Explicitly setting up the cache for all markets in v7 upgrade handler.
- No need to maintain the "if not found" case
Let me know if this makes sense
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/app/upgrades.go (1 hunks)
- protocol/app/upgrades/v7.0.0/upgrade.go (1 hunks)
- protocol/app/upgrades/v7.0.0/upgrade_container_test.go (1 hunks)
Additional comments not posted (4)
protocol/app/upgrades/v7.0.0/upgrade.go (2)
15-24: LGTM!The new
initCurrencyPairIDCachefunction is well-implemented and follows best practices. It correctly initializes the currency pair ID cache based on existing market parameters and handles errors appropriately.
26-40: LGTM!The modifications to the
CreateUpgradeHandlerfunction are well-implemented and follow best practices. The addition of thepricesKeeperparameter and the call toinitCurrencyPairIDCacheensure that the currency pair ID cache is properly initialized before proceeding with the migration process.protocol/app/upgrades/v7.0.0/upgrade_container_test.go (1)
47-48: LGTM!The code changes are approved. The addition of the
postUpgradeCurrencyPairIDCacheStatefunction call withinpostUpgradeChecksimproves the organization of the test code and enhances maintainability by separating concerns related to different aspects of the upgrade process.protocol/app/upgrades.go (1)
33-33: LGTM!The addition of
app.PricesKeeperas a parameter to theCreateUpgradeHandlerfunction call is approved. This change allows the upgrade handler to utilize pricing data or functionalities during the upgrade process, potentially enhancing how upgrades are managed and executed within the application.
| if err == nil { | ||
| k.currencyPairIDCache.AddCurrencyPair(uint64(updatedMarketParam.Id), cp.String()) | ||
| k.AddCurrencyPairIDToStore(ctx, updatedMarketParam.Id, newCurrencyPair) | ||
| } else { | ||
| k.Logger(ctx).Error("failed to add currency pair to cache", "pair", updatedMarketParam.Pair) |
There was a problem hiding this comment.
Why only log the error here? Since this is main point of the state change, seems like we should return error here (thus failing the transaction)
There was a problem hiding this comment.
makes sense, good catch
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit