feat: Slinky full integration PR #1141
Conversation
WalkthroughThese updates introduce significant enhancements and restructuring across the protocol, notably the integration of a new client for the Slinky sidecar and oracle functionalities. Changes revolve around refining the proposal preparation and processing mechanisms, specifically through the introduction of new interfaces and error handling strategies for market price updates. Additionally, the modifications include the setup of an oracle Prometheus server, adjustments in daemon flags for enhanced control, and updates in testing configurations to accommodate the new features. 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: 30
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
protocol/docker-compose.ymlis excluded by:!**/*.ymlprotocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sum
Files selected for processing (53)
- protocol/Dockerfile (3 hunks)
- protocol/app/app.go (11 hunks)
- protocol/app/prepare/expected_keepers.go (2 hunks)
- protocol/app/prepare/full_node_prepare_proposal.go (1 hunks)
- protocol/app/prepare/prepare_proposal.go (9 hunks)
- protocol/app/prepare/prepare_proposal_test.go (17 hunks)
- protocol/app/prepare/prices/default_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/errors.go (1 hunks)
- protocol/app/prepare/prices/price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator.go (1 hunks)
- protocol/app/prepare/prices/slinky_price_update_generator_test.go (1 hunks)
- protocol/app/process/default_market_price_decoder.go (1 hunks)
- protocol/app/process/errors.go (1 hunks)
- protocol/app/process/full_node_process_proposal.go (2 hunks)
- protocol/app/process/full_node_process_proposal_test.go (1 hunks)
- protocol/app/process/market_price_decoder.go (1 hunks)
- protocol/app/process/market_prices_test.go (4 hunks)
- protocol/app/process/process_proposal.go (2 hunks)
- protocol/app/process/process_proposal_test.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
- protocol/app/process/transactions.go (4 hunks)
- protocol/app/process/transactions_test.go (4 hunks)
- protocol/app/vote_extensions/expected_keepers.go (1 hunks)
- protocol/app/vote_extensions/extend_vote_handler.go (1 hunks)
- protocol/app/vote_extensions/extend_vote_handler_test.go (1 hunks)
- protocol/app/vote_extensions/oracle_client.go (1 hunks)
- protocol/app/vote_extensions/oracle_client_test.go (1 hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (5 hunks)
- protocol/daemons/flags/flags.go (5 hunks)
- protocol/daemons/slinky/client/client.go (1 hunks)
- protocol/daemons/slinky/client/client_test.go (1 hunks)
- protocol/daemons/slinky/client/constants.go (1 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher.go (1 hunks)
- protocol/daemons/slinky/client/market_pair_fetcher_test.go (1 hunks)
- protocol/daemons/slinky/client/price_fetcher.go (1 hunks)
- protocol/daemons/slinky/client/price_fetcher_test.go (1 hunks)
- protocol/lib/slinky/utils.go (1 hunks)
- protocol/lib/slinky/utils_test.go (1 hunks)
- protocol/mocks/ExtendVoteHandler.go (1 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/mocks/MarketPairFetcher.go (1 hunks)
- protocol/mocks/OracleClient.go (1 hunks)
- protocol/mocks/PriceFetcher.go (1 hunks)
- protocol/mocks/PriceUpdateGenerator.go (1 hunks)
- protocol/mocks/PricesKeeper.go (15 hunks)
- protocol/mocks/UpdateMarketPriceTxDecoder.go (1 hunks)
- protocol/testing/genesis.sh (70 hunks)
- protocol/testing/testnet-local/local.sh (3 hunks)
- protocol/testutil/keeper/prices.go (1 hunks)
- protocol/x/prices/keeper/slinky_adapter.go (1 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (1 hunks)
- protocol/x/prices/types/types.go (2 hunks)
Files not summarized due to errors (1)
- protocol/testing/genesis.sh: Error: Message exceeds token limit
Files skipped from review due to trivial changes (5)
- protocol/daemons/slinky/client/constants.go
- protocol/mocks/ExtendVoteHandler.go
- protocol/mocks/OracleClient.go
- protocol/mocks/PriceFetcher.go
- protocol/mocks/UpdateMarketPriceTxDecoder.go
Additional comments: 119
protocol/app/prepare/prices/price_update_generator.go (1)
- 1-12: The introduction of the
PriceUpdateGeneratorinterface with its methodGetValidMarketPriceUpdatesis well-defined and aligns with the protocol's requirements for abstracting the logic of retrieving market price updates. The use ofsdk.Contextand the method's return types are appropriate, indicating a thoughtful integration process.protocol/app/prepare/prices/errors.go (1)
- 1-17: The
InvalidPriceErrorcustom error type is well-implemented, following Go's idiomatic error handling practices. The error message provided by theErrormethod is clear and informative, facilitating easier debugging and error handling. This addition enhances the protocol's robustness in handling price-related errors.protocol/app/vote_extensions/expected_keepers.go (1)
- 1-18: The
PricesKeeperinterface is well-defined, with methods that clearly outline the responsibilities related to currency pair retrieval and market price updates. The integration with Slinky through the use oforacletypes.CurrencyPairand the methods provided align well with the protocol's requirements for handling price data and vote extensions. This indicates a thoughtful approach to integrating Slinky's functionalities into the protocol.protocol/lib/slinky/utils.go (1)
- 1-29: The utility function
MarketPairToCurrencyPairis well-implemented, correctly handling the conversion between the protocol's market price format and Slinky's oracle currency pair format. The validation and error handling are appropriately done, ensuring that the input format is correct and providing informative error messages. This utility function is a key component in integrating Slinky's functionalities into the protocol.protocol/app/prepare/expected_keepers.go (1)
- 7-12: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-28]
The interfaces
PrepareClobKeeper,PreparePerpetualsKeeper, andPrepareBridgeKeeperare well-defined, each focusing on specific functionalities required for thePrepareProposalprocess. The removal ofPreparePricesKeepersuggests a significant restructuring in handling market price updates, likely due to the integration of Slinky. The remaining interfaces are correctly defined and align with the protocol's requirements.protocol/app/prepare/full_node_prepare_proposal.go (1)
- 20-25: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-23]
The
FullNodePrepareProposalHandlerfunction is well-implemented, correctly handling the scenario where a full node attempts to runPrepareProposal. The function ensures that proposals fail gracefully in full-node mode by returning an empty transaction array and logging an informative error message. This change enhances the protocol's robustness and operational integrity.protocol/lib/slinky/utils_test.go (1)
- 1-34: The test cases for the
MarketPairToCurrencyPairutility function are well-structured and comprehensive, covering both correctly and incorrectly formatted market pairs. The use of table-driven tests and informative error messages aligns with best practices in Go testing. These tests effectively validate the utility function's behavior, ensuring its reliability within the protocol.protocol/app/process/market_price_decoder.go (1)
- 1-28: The
UpdateMarketPriceTxDecoderinterface and its methods are well-defined, clearly outlining the responsibilities for decoding market price transactions and handling injected transactions. The constructor functionNewUpdateMarketPricesTxis correctly implemented, facilitating the creation ofUpdateMarketPricesTxinstances with the necessary context and data. These changes align well with the protocol's requirements and best practices.protocol/app/prepare/prices/default_price_update_generator.go (1)
- 1-36: The
DefaultPriceUpdateGeneratorstruct and its methodGetValidMarketPriceUpdatesare well-implemented, providing a default mechanism for retrieving market price updates from thePricesKeeper. The error handling and use ofsdk.Contextalign with best practices, ensuring the robustness and reliability of the implementation. This addition effectively supports the protocol's requirements for handling market price updates.protocol/app/process/errors.go (1)
- 10-49: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-49]
The new error types and functions introduced in this file are well-defined, covering a range of scenarios related to proposed price validation in the
processmodule. The error messages are descriptive and informative, facilitating easier debugging and error handling. This enhancement significantly improves the module's robustness and reliability by ensuring consistent and informative error handling.protocol/app/process/default_market_price_decoder.go (1)
- 7-16: The introduction of
DefaultUpdateMarketPriceTxDecoderstruct is a significant change. It's essential to ensure that thepkandtxDecoderfields are used appropriately throughout the codebase, especially in the context of decoding market price transactions.protocol/x/prices/types/types.go (1)
- 48-58: The addition of
slinkyandoraclerelated methods to thePricesKeeperinterface is a significant change. It's important to ensure that these methods are implemented across all concrete types that satisfy thePricesKeeperinterface. This change integratesslinkyandoraclemodules more deeply into the protocol, which could have implications for data handling and performance.protocol/mocks/PriceUpdateGenerator.go (1)
- 13-46: The mock implementation of
PriceUpdateGeneratoris correctly set up usingtestify/mock. It's important to ensure that the methodGetValidMarketPriceUpdatesaccurately reflects the interface it's mocking, including the parameters and return types. This mock will be crucial for unit testing components that depend onPriceUpdateGenerator.protocol/app/process/full_node_process_proposal.go (1)
- 17-18: The replacement of
pricesKeeperwithpricesTxDecoderin theFullNodeProcessProposalHandlerfunction signature is a significant change. It's important to ensure that all references and usages ofpricesKeeperare correctly updated topricesTxDecoder. This change likely reflects a shift towards a more modular and decoupled architecture for handling market price updates.protocol/Dockerfile (1)
- 3-3: Updating the Golang Alpine base image version to
1.22is a significant change. Ensure that this version is compatible with the rest of the project dependencies and does not introduce any breaking changes or security vulnerabilities. It's also good practice to periodically review and update base images to benefit from security patches and performance improvements.protocol/mocks/MarketPairFetcher.go (1)
- 16-90: The mock implementation of
MarketPairFetcheris correctly set up usingtestify/mock. Ensure that the methodsFetchIdMappings,GetIDForPair,Start, andStopaccurately reflect the interface they're mocking, including the parameters and return types. This mock will be crucial for unit testing components that depend onMarketPairFetcher.protocol/app/vote_extensions/oracle_client_test.go (4)
- 17-24: The test
TestStartStopNoopcorrectly verifies that theStartandStopmethods ofOracleClientdo not produce errors. This is a good practice to ensure that these lifecycle methods behave as expected.- 47-54: The test
TestNonSdkContextFailscorrectly verifies that thePricesmethod returns an error when a non-SDK context is passed. This is important for ensuring that the method behaves as expected in different contexts.- 56-68: The test
TestEmptyUpdatesPassesverifies that thePricesmethod does not produce an error when thePricesKeeperreturns an empty list of market price updates. This is a good practice to ensure that the method can handle empty updates gracefully.- 70-80: The test
TestNilUpdatesPassesverifies that thePricesmethod does not produce an error when thePricesKeeperreturnsnilfor market price updates. This is important for ensuring that the method can handlenilupdates gracefully.protocol/daemons/slinky/client/market_pair_fetcher_test.go (2)
- 32-51: The test case "caches and returns valid pairs" correctly mocks the
AllMarketParamscall, simulates fetching ID mappings, and asserts the expected behavior. However, it's important to ensure that theOnce()call on the mock ensures that the method is called exactly once, which is good practice for precise test behavior validation.- 81-89: The test case "fails on prices query error" correctly simulates a scenario where the
AllMarketParamsquery fails. This test ensures that theFetchIdMappingsmethod correctly handles and propagates errors from the prices query client. Good coverage for error handling.protocol/app/process/full_node_process_proposal_test.go (1)
- 91-91: The replacement of
pricesKeeperwithprocess.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestFullNodeProcessProposalHandlerfunction is a significant change. It aligns with the PR's objective to integrate Slinky by enhancing the mechanism for handling market price updates. This change ensures that the new decoder is utilized for processing proposals, which is crucial for the integration. Ensure that theNewDefaultUpdateMarketPriceTxDecoderfunction is thoroughly tested to guarantee its reliability.protocol/app/prepare/prices/slinky_price_update_generator.go (1)
- 47-104: The implementation of
SlinkyPriceUpdateGenerator.GetValidMarketPriceUpdatesmethod is comprehensive, covering the process from checking vote extensions' enablement to generating aMsgUpdateMarketPricesmessage with sorted market price updates. A few points to consider:
- The use of
ve.VoteExtensionsEnabled(ctx)to conditionally proceed with generating price updates is a good practice, ensuring that the functionality is only active when required.- Error handling is robust, with clear error messages and checks at each step of the process.
- The sorting of market price updates before validation (
msg.ValidateBasic()) ensures consistency in the message structure, which is crucial for deterministic behavior.- The method's complexity is justified by its responsibility, but ensure that each component (
agg,extCommitCodec,veCodec,currencyPairStrategy) is well-tested to maintain the reliability of price updates.Overall, the method aligns with the PR's objectives by integrating Slinky's vote extensions into the market price update process, enhancing the protocol's capability to utilize real-time price data.
protocol/daemons/slinky/client/market_pair_fetcher.go (2)
- 48-64: The implementation of the
Startmethod inMarketPairFetcherImplcorrectly establishes a gRPC connection and initializes thePricesQueryClient. This setup is crucial for the fetcher's operation, allowing it to query market parameters from the prices module. It's good to see error handling in place for connection failures, ensuring that the fetcher does not proceed without a valid connection. Ensure that the gRPC connection is securely configured, especially if it communicates over public networks.- 87-107: The
FetchIdMappingsmethod efficiently refreshes the cache of market pair IDs by querying all market parameters and mapping them to Slinky'sCurrencyPairtype. This method is critical for ensuring that the fetcher has up-to-date information. The use of a local variablecompatMappingsfor constructing the new mappings before atomically replacing the existing cache is a good practice for thread safety. Additionally, logging the mapping process aids in monitoring and debugging. Ensure that error handling for theMarketPairToCurrencyPairconversion is comprehensive, as malformed market pairs could lead to runtime errors.protocol/app/process/process_proposal.go (2)
- 40-40: The addition of the
pricesTxDecoderparameter to theProcessProposalHandlerfunction is a significant change that aligns with the PR's objectives. This new parameter allows for a more flexible and modular approach to decoding market price updates, which is essential for integrating Slinky's functionalities. Ensure that theUpdateMarketPriceTxDecoderinterface is well-defined and that its implementations are thoroughly tested to maintain the integrity of the proposal processing mechanism.- 76-76: The use of
pricesTxDecoderin theDecodeProcessProposalTxscall withinProcessProposalHandlerdemonstrates the shift towards a more modular decoding mechanism for market price updates. This change is crucial for accommodating Slinky's price update mechanism. It's important to ensure that all existing functionalities are preserved and that the new decoder accurately processes market price updates according to the protocol's requirements.protocol/daemons/slinky/client/client_test.go (1)
- 97-123: The test case "services are all started and call their deps" effectively simulates the Slinky client's operation, including starting services and performing health checks. The use of mocks for the OracleClient and the PricesQueryServer allows for controlled testing of the client's interactions with its dependencies. However, ensure that the health check assertion after stopping the client (
c.Require().NoError(cli.HealthCheck())) accurately reflects the expected behavior, as it might be more appropriate to expect an error or a specific health check status indicating that the client has stopped.protocol/x/prices/keeper/slinky_adapter_test.go (1)
- 107-129: The
TestMarketPairToCurrencyPairfunction tests the conversion from market pair strings to currency pair structs. It includes tests for both valid and invalid inputs. This test is comprehensive as it covers different formats of market pairs. No further action is required here.protocol/cmd/dydxprotocold/cmd/config.go (5)
- 10-10: The import of
oracleconfigfrom thegithub.com/skip-mev/slinky/oracle/configpackage is correctly added to support the new Slinky oracle configuration. This change is necessary for the integration and appears to be correctly implemented.- 29-29: The addition of the
Oraclefield of typeoracleconfig.AppConfigto theDydxAppConfigstruct is a crucial change for integrating the Slinky oracle. This change is correctly implemented, following Go's struct declaration conventions.- 57-63: The initialization of the
Oraclefield within theinitAppConfigfunction is correctly done, setting up default values for the Slinky oracle configuration. It's important to ensure these default values are sensible and align with the expected operational parameters of the oracle.Please verify that the default values for the Slinky oracle configuration (
OracleAddress,ClientTimeout, etc.) are appropriate and align with the operational requirements.
- 77-77: The update to the
appTemplateto includeoracleconfig.DefaultConfigTemplateis necessary for incorporating the Slinky oracle configuration into the app's configuration template. This change is correctly implemented, ensuring that the oracle configuration is part of the default configuration template.- 77-77: The removal of
RPC.MaxOpenConnectionsandRPC.GRPCMaxOpenConnectionssettings in theinitTendermintConfigfunction is not directly shown in the provided code snippet. However, if these settings were removed as part of the changes, it's important to ensure that their removal does not negatively impact the network's performance or security.Please verify the impact of removing
RPC.MaxOpenConnectionsandRPC.GRPCMaxOpenConnectionssettings on the network's performance and security to ensure that these changes do not introduce any unintended side effects.protocol/app/process/slinky_market_price_decoder.go (3)
- 12-28: The
SlinkyMarketPriceDecoderstruct and its constructor functionNewSlinkyMarketPriceDecoderare correctly implemented. This setup is essential for integrating the Slinky oracle with the existing market price decoding functionality. The struct fields and constructor parameters are appropriately defined.- 80-88: The
GetTxOffsetmethod provides a mechanism to adjust the expected indices of transactions based on whether vote-extensions are enabled. This method is correctly implemented and plays a critical role in handling the dynamic nature of transaction offsets due to the injection of vote-extensions.- 90-119: The
checkEqualityOfMarketPriceUpdatefunction is a utility method for validating the equality of market price updates. It correctly performs basic validation and compares the expected and actual market price updates. This method ensures the integrity of market price updates by verifying their consistency.protocol/app/process/transactions.go (4)
- 4-5: The addition of the
slicesimport from the Go standard library is correctly used in the initialization logic to ensure the consistency of transaction indices and offsets. This usage of theslicespackage is appropriate for the operations performed in the initialization block.- 65-65: The addition of the
UpdateMarketPricesTxfield to theProcessProposalTxsstruct is a significant change that abstracts over market price updates. This addition is correctly implemented and is crucial for integrating the Slinky oracle's market price updates into the proposal processing logic.- 77-99: The modifications in the
DecodeProcessProposalTxsfunction to incorporate theUpdateMarketPriceTxDecoderand handle injected vote-extensions are correctly implemented. These changes are essential for processing proposal transactions in the presence of vote-extensions and ensuring the correct decoding of market price updates.However, it's important to ensure that the handling of offsets and injected transactions is thoroughly tested to prevent any potential issues during proposal processing.
Please verify that the handling of offsets and injected transactions in the
DecodeProcessProposalTxsfunction is thoroughly tested to ensure robustness and correctness.
- 126-128: The logic for decoding other transactions (
OtherMsgsTx) considering the offset introduced by injected vote-extensions is correctly implemented. This adjustment ensures that all transactions are correctly processed and decoded in the presence of vote-extensions.protocol/daemons/slinky/client/client.go (3)
- 22-31: The
Clientstruct is well-defined, encapsulating all necessary fields for the daemon's operation, including context handling, logging, and fetchers for market pairs and prices. This struct follows good practices in struct organization and field encapsulation.- 47-74: The
startmethod correctly initializes theMarketPairFetcherandPriceFetcherand starts them in separate goroutines. This approach is effective for concurrent operations but ensure proper error handling within these goroutines to prevent unhandled errors from causing unexpected behavior.- 103-108: The
Stopmethod correctly cancels the context and stops both thePriceFetcherandMarketPairFetcher, ensuring that all goroutines are properly terminated before returning. This is a good practice for clean shutdown procedures.protocol/daemons/slinky/client/price_fetcher.go (3)
- 19-24: The
PriceFetcherinterface is well-defined, clearly outlining the responsibilities of a price fetcher. This abstraction allows for easy testing and potential future implementations that fetch prices from different sources.- 26-49: The
PriceFetcherImplstruct and theNewPriceFetcherconstructor function are well-implemented. They encapsulate the necessary dependencies for fetching prices, including the market pair fetcher, GRPC client, and the Slinky oracle client. This setup promotes modularity and testability.- 66-71: The
Stopmethod properly closes the GRPC connection and stops the Slinky client. This clean-up is crucial for preventing resource leaks. Good implementation of resource management.protocol/app/process/market_prices_test.go (4)
- 7-8: The import of
errorsmod "cosmossdk.io/errors"is a good practice for distinguishing between standard library errors and Cosmos SDK-specific errors. This helps in writing clearer, more maintainable test code.- 69-70: The introduction of
NewDefaultUpdateMarketPriceTxDecoderin the tests is a positive change, as it encapsulates the decoding logic within a dedicated method. This makes the tests cleaner and more focused on their specific testing logic.- 132-135: The use of
NewDefaultUpdateMarketPriceTxDecoderin theTestUpdateMarketPricesTx_Validatetest function further demonstrates the utility of this new method in simplifying test setup. This consistency across test functions is beneficial for maintainability.- 171-175: The test
TestUpdateMarketPricesTx_GetMsgeffectively verifies theGetMsgmethod's behavior under different conditions. The use ofNewDefaultUpdateMarketPriceTxDecoderhere again highlights the method's value in streamlining test code.protocol/app/vote_extensions/extend_vote_handler_test.go (3)
- 37-56: The test
TestExtendVoteHandlerUpdatePricesTxValidateFailuresimulates a validation failure in the update prices transaction. This test is well-structured and effectively tests the error handling path. No issues found.- 83-107: The test
TestExtendVoteHandlerSlinkyFailureeffectively simulates a failure in the Slinky execution path. This test is crucial for ensuring robust error handling in the integration with the Slinky protocol. Well done on covering this scenario.- 109-133:
TestExtendVoteHandlerSlinkySuccesssuccessfully tests the happy path where all components work as expected, and the Slinky execution completes without errors. This test is essential for validating the correct integration and operation of theExtendVoteHandlerwith the Slinky protocol.protocol/mocks/Makefile (1)
- 58-63: The addition of mock generation commands for
PriceFetcher,MarketPairFetcher,OracleClient,PriceUpdateGenerator,ExtendVoteHandler, andUpdateMarketPriceTxDecoderis a good practice for ensuring that the new functionalities introduced by the Slinky integration can be thoroughly tested. It's important to verify that the directories specified in these commands exist and are correct, and that theSLINKY_VERSIONvariable is defined elsewhere in the Makefile or in the environment to avoid potential build issues.protocol/testing/testnet-local/local.sh (1)
- 157-164: The
use_slinkyfunction correctly configures theapp.tomlfile to disable theprice-daemon-enabledsetting and enableslinky-daemon-enabled, pointing the oracle address toslinky0:8080. Ensure that theslinky0:8080address is correct and reachable from the context where this script is executed. Additionally, verify that disabling the pricefeed daemon and enabling the Slinky daemon does not conflict with other parts of the system.protocol/app/prepare/prices/slinky_price_update_generator_test.go (4)
- 52-66: The test
TestWithVoteExtensionsNotEnabledcorrectly checks that theSlinkyPriceUpdateGeneratorreturns an empty update when vote extensions are not enabled. This test ensures that the system behaves as expected under this condition. It's well-structured and follows best practices for unit testing.- 68-111: The test
TestVoteExtensionAggregationFailsproperly simulates a failure in aggregating oracle votes and asserts that an error is returned. This test is crucial for ensuring robust error handling in the system. However, ensure that the error messages used in assertions are consistent with those implemented in the system to avoid false negatives in the tests.- 114-163: The test
TestCurrencyPairConversionFailseffectively simulates a scenario where currency pair conversion fails, leading to an error. This test is important for validating the system's behavior in handling conversion errors. Similar to previous comments, ensure that error messages are consistent with the actual implementation.- 166-230: The test
TestValidMarketPriceUpdatesuccessfully simulates a scenario where valid market price updates are generated and asserts that the resulting message contains the expected updates. This test is key for verifying the correct functionality of theSlinkyPriceUpdateGenerator. It's well-structured and covers an important aspect of the system's behavior.protocol/daemons/slinky/client/price_fetcher_test.go (10)
- 28-30: The test suite initialization using
suite.Runis correctly implemented.- 41-72: The
SetupTestmethod properly initializes the test environment, including daemon and gRPC servers. However, it's important to ensure that the chosenSocketAddressandGrpcAddressdo not conflict with any real services during testing.Please verify that the addresses used for testing (
SocketAddressandGrpcAddress) are isolated from production or development environments to avoid unintended side effects.
- 75-78: The
TearDownTestmethod correctly stops the servers and waits for the goroutines to finish, ensuring a clean test environment.- 88-108: This test case, "fetches prices on valid inputs", correctly mocks the necessary components and verifies the behavior of the
PriceFetcherunder expected conditions.- 111-125: The test case "errors on slinky.Prices failure" properly simulates a failure in fetching prices and checks for the expected error. This ensures robust error handling in the
PriceFetcher.- 128-146: The test case "errors on slinky.Prices returning invalid currency pairs" effectively tests the error handling for incorrectly formatted currency pairs, which is crucial for data integrity.
- 149-169: The "no-ops on marketPairFetcher currency pair not found" test case correctly handles the scenario where a currency pair is not found, ensuring the system's resilience.
- 172-192: The "continues on non-parsable price data" test case is important for ensuring the system can handle unexpected data formats without crashing.
- 195-212: The "no-ops on empty price response" test case ensures that the system behaves correctly when no prices are returned, which is a valid scenario that needs to be handled gracefully.
- 215-235: The "fails on closed connections" test case simulates a scenario where the connection is closed unexpectedly. This is crucial for ensuring the system can handle network issues gracefully.
protocol/daemons/flags/flags.go (6)
- 27-27: The introduction of the
FlagSlinkyDaemonEnabledconstant is correctly implemented and follows the naming convention of other daemon flags.- 68-71: The
SlinkyFlagsstruct is correctly defined with anEnabledfield to toggle the Slinky daemon. This aligns with the structure of other daemon flag structs.- 79-79: Including
SlinkyFlagsin theDaemonFlagsstruct is necessary for the comprehensive management of daemon configurations. This change is correctly implemented.- 107-109: Setting the default value of
SlinkyFlags.EnabledtofalseinGetDefaultDaemonFlagsis a safe approach to ensure that the Slinky daemon is opt-in.- 188-193: The handling of the
FlagSlinkyDaemonEnabledflag inAddDaemonFlagsToCmdis correctly implemented, allowing for the Slinky daemon to be enabled via command-line arguments.- 266-271: The inclusion of the
FlagSlinkyDaemonEnabledinGetDaemonFlagValuesFromOptionsensures that the Slinky daemon can be configured through application options, aligning with the configuration approach of other daemons.protocol/testutil/keeper/prices.go (1)
- 126-126: The change to the
Pairfield in theCreateNMarketsfunction to include the index twice separated by a hyphen (fmt.Sprintf("%v-%v", i, i)) is a logical update for generating unique and identifiable market pairs for testing. This ensures that each test market has a distinct pair identifier, which is important for tests that rely on specific market pair formatting or uniqueness.protocol/app/process/process_proposal_test.go (1)
- 288-288: The addition of
process.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())as a new argument to theProcessProposalHandlerfunction call within the testTestProcessProposalHandler_Erroris a significant change. This modification aligns with the PR's objective to integrate Slinky and its related functionalities, specifically handling market price updates more effectively. Ensure that all relevant test cases have been updated or added to cover the new functionalities introduced by this change.protocol/app/prepare/prepare_proposal.go (3)
- 13-16: The reorganization of imports, including the addition of
github.com/dydxprotocol/v4-chain/protocol/app/prepare/pricesandgithub.com/skip-mev/slinky/abci/types, is appropriate and improves the clarity of dependencies. This change supports the integration of Slinky and the new price update functionalities.- 55-55: Replacing the
PreparePricesKeeperwith thePriceUpdateGeneratorinterface in thePrepareProposalHandlerfunction signature is a positive change. It enhances modularity and flexibility in handling market price updates, aligning with the PR's objectives to integrate Slinky effectively.- 87-97: Refactoring
GetUpdateMarketPricesTxintoEncodeMarketPriceUpdatesand updating its usage withinPrepareProposalHandleris a significant improvement. This change makes the code more readable and maintainable by clearly separating the encoding of market price updates from other functionalities. Ensure comprehensive testing of this refactored functionality to verify its correctness and performance.protocol/app/process/slinky_market_price_decoder_test.go (7)
- 16-24: The setup for the test suite
SlinkyMarketPriceDecoderSuiteis well-structured, initializing mocks forPriceUpdateGenerator,UpdateMarketPriceTxDecoder, and thesdk.Context. This setup ensures that each test runs in a controlled environment with predictable behavior from dependencies.- 41-68: The test case
TestVoteExtensionsNotEnabledcorrectly verifies that when vote extensions are not enabled, the decoder should not allow non-empty proposed market-price updates. This test ensures that the system behaves as expected under the condition where vote extensions are disabled, aligning with the protocol's requirements for feature gating.- 70-86: The second part of
TestVoteExtensionsNotEnabledfurther validates the requirement that proposed prices must be empty when vote extensions are not enabled. This test complements the previous one by checking the positive scenario where the proposed update is correctly empty, ensuring comprehensive coverage of this feature's behavior.- 90-121: The
TestVoteExtensionsEnabledtest cases cover scenarios where vote extensions are enabled but either the extended commit is missing or the price-update generator fails. These tests are crucial for ensuring the system's robustness in handling errors or incomplete data when vote extensions are active. It's important to verify that the system gracefully handles these scenarios without causing unexpected behavior.- 124-253: The series of tests under
TestMarketPriceUpdateValidation_WithVoteExtensionsEnabledare well-designed to cover various failure scenarios related to market price update validation when vote extensions are enabled. These include failures due to underlying decoder errors, conflicting updates, and messages that don't passValidateBasic. These tests ensure that the system correctly identifies and handles invalid or conflicting data, maintaining the integrity of market price updates.- 256-295: The
TestHappyPath_VoteExtensionsEnabledtest case effectively demonstrates the happy path scenario where vote extensions are enabled, and the system successfully processes a valid market price update. This test is essential for verifying that the system functions correctly under ideal conditions, providing confidence in the integration's overall stability and correctness.- 297-319: The
TestGetTxOffsetmethod tests the logic for determining the transaction offset based on whether vote extensions are enabled. These tests ensure that the system correctly calculates the offset, which is critical for processing transactions in the correct order and context. The distinction between scenarios with and without vote extensions enabled is well captured, ensuring comprehensive coverage of this functionality.protocol/mocks/PricesKeeper.go (6)
- 1-1: The file correctly indicates that it is autogenerated by mockery v2.42.0, ensuring that readers are aware that manual edits should not be made to this file. This is a good practice for maintaining clarity and avoiding accidental modifications to autogenerated code.
- 26-28: The panic statements added for missing return values in mock methods, such as
CreateMarket, ensure that tests fail loudly and clearly when the mock's expectations are not properly set up. This approach helps catch issues early in the testing process by making it evident when a mock is used without defining its expected behavior.- 120-146: The addition of the
GetCurrencyPairFromIDmock method is a good practice, as it allows for testing functionalities that depend on currency pair identification. This method, along with its return value specification and panic for missing return values, ensures that tests can simulate various scenarios involving currency pairs, contributing to more robust and comprehensive test coverage.- 252-278: The new mock function
GetPriceForCurrencyPairis crucial for testing functionalities that involve fetching price information for specific currency pairs. This addition enhances the mock's utility by allowing for the simulation of scenarios where price data is retrieved, which is essential for testing components that rely on this functionality.- 280-298: The
GetValidMarketPriceUpdatesmock function is an important addition for testing the system's ability to fetch valid market price updates. This function's presence in the mock ensures that tests can accurately simulate the behavior of the PricesKeeper in scenarios where market price updates are processed, contributing to the thoroughness of the testing strategy.- 420-425: The updated
NewPricesKeeperfunction signature, which now includes a testing interface and a cleanup function to assert the mock's expectations, is a significant improvement. This change ensures that the mock's usage aligns with best practices for test cleanliness and reliability, making it easier to manage mock expectations and cleanup in test suites.protocol/app/process/transactions_test.go (4)
- 119-119: The replacement of
pricesKeeperwithprocess.NewDefaultUpdateMarketPriceTxDecoder(pricesKeeper, constants.TestEncodingCfg.TxConfig.TxDecoder())in theTestDecodeProcessProposalTxs_Errorfunction is a significant change. This update aligns with the PR's objective to integrate Slinky by modifying how market price updates are decoded. It's crucial to ensure that this new decoder is thoroughly tested, especially since it directly impacts how price updates are processed in the protocol.Ensure comprehensive testing of
NewDefaultUpdateMarketPriceTxDecoderto validate its integration and functionality within the protocol.
- 200-200: Similar to the previous comment, the use of
process.NewDefaultUpdateMarketPriceTxDecoderin theTestDecodeProcessProposalTxs_Validfunction is consistent with the PR's goal of enhancing the protocol's price update mechanism. This change introduces a more specialized decoder for market price updates, which is expected to offer improved flexibility and efficiency.It's important to verify that the new decoder handles all expected scenarios correctly, including various types of valid and invalid transactions, to maintain the integrity of the protocol's price update process.
- 348-348: In the
TestProcessProposalTxs_Validate_Errorfunction, replacingpricesKeeperwith theprocess.NewDefaultUpdateMarketPriceTxDecoderis part of the broader effort to refine the protocol's handling of market price updates. This change suggests a move towards a more modular and maintainable codebase, which is beneficial for long-term development and scalability.Given the critical role of price updates in the protocol, thorough validation of the new decoder's error handling capabilities is essential. This includes testing for various error conditions to ensure robustness and reliability.
- 455-455: The integration of
process.NewDefaultUpdateMarketPriceTxDecoderin theTestProcessProposalTxs_Validate_Validfunction further demonstrates the PR's commitment to enhancing the protocol's market price update mechanism. This change is expected to streamline the decoding process, making it more efficient and adaptable to future requirements.Continued testing and validation of the new decoder's performance in handling valid transactions are crucial. This will help ensure that the decoder accurately processes market price updates without introducing any regressions or unintended behaviors.
protocol/app/prepare/prepare_proposal_test.go (12)
- 19-52: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [8-31]
The addition of new imports, including
cosmossdk.io/log,github.com/skip-mev/slinky, and related mocks, is appropriate for the expanded functionality related to Slinky integration and improved logging. Ensure that these packages are used effectively throughout the tests and that there are no unused imports.
- 35-49: The definition of various
TxEncoderfunctions (failingTxEncoder,emptyTxEncoder,passingTxEncoderOne,passingTxEncoderTwo,passingTxEncoderFour) is a good practice for testing different scenarios of transaction encoding. This approach allows for comprehensive testing of the transaction preparation logic under various conditions.- 366-373: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [339-370]
The creation of
mockTxConfigusingcreateMockTxConfigand the setup of mock keepers (mockPricesKeeper,mockPerpKeeper,mockBridgeKeeper,mockClobKeeper) are well-implemented for setting up the test environment. However, ensure that the mock responses and behaviors closely mimic the real-world scenarios to make the tests as realistic as possible.
- 370-370: The use of
prices.NewDefaultPriceUpdateGeneratorto create a price update generator for thePrepareProposalHandleris crucial for testing the integration with Slinky. It's important to verify that this generator behaves as expected, especially in scenarios where Slinky's data is used for price updates.- 465-509: The test case "ves not enabled" correctly simulates a scenario where vote extensions are not enabled, and thus, a default empty
UpdateMarketPricestransaction is expected. This test is important for ensuring that the system behaves correctly under different configuration states. However, it's crucial to also verify that the logic for determining whether vote extensions are enabled is accurate and robust.- 511-641: The test case "ves enabled" is comprehensive, simulating a scenario where vote extensions are enabled and valid
ExtendedCommitInfois present. The mocking of dependencies and the use ofNewSlinkyPriceUpdateGeneratorwith mocked currency pair strategy and vote aggregator are well-executed. Ensure that the logic for aggregating oracle votes and generating market price updates is thoroughly tested, especially considering the potential complexity of handling various currency pairs and vote data.- 686-698: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [643-715]
The function
getMarketPriceUpdatesand its test cases provide a focused examination of generating market price updates. It's good to see that different scenarios, including nil messages and encoding failures, are tested. This function is critical for ensuring that market price updates are correctly prepared for inclusion in proposals. Ensure that the error handling and edge cases are adequately covered in these tests.
- 704-723: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [716-762]
The tests for
GetAcknowledgeBridgesTxcover various scenarios, including empty lists of messages and encoding failures. This thorough testing is essential for ensuring that bridge acknowledgments are correctly processed and encoded into transactions. It's important to ensure that the bridge acknowledgment logic aligns with the protocol's requirements and correctly handles all possible cases.
- 779-785: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [782-824]
The tests for
GetAddPremiumVotesTxare well-structured, covering scenarios such as nil messages, empty messages, and encoding failures. These tests are crucial for ensuring that premium votes are correctly processed and encoded. It's important to verify that the logic for handling premium votes is consistent with the protocol's governance mechanisms and accurately reflects the intended behavior.
- 841-847: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [844-889]
The tests for
GetProposedOperationsTxeffectively cover various scenarios, including nil messages and encoding failures. This thorough testing ensures that proposed operations are correctly processed and encoded into transactions. It's important to verify that the logic for handling proposed operations aligns with the protocol's trading mechanisms and accurately processes all types of operations.
- 904-910: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [907-929]
The tests for
EncodeMsgsIntoTxBytescover critical scenarios, including failures in setting messages and encoding transactions. This function is essential for the overall transaction preparation process, and its robust testing ensures that messages are correctly encoded into transactions. Ensure that the transaction encoding logic is thoroughly tested and aligns with the protocol's requirements for transaction formats.
- 943-943: The
createMockTxConfigfunction is a useful utility for setting up mock transaction configurations for testing. It's important to ensure that this function accurately simulates the behavior of the real transaction configuration, especially in terms of encoding and decoding transactions. This will help ensure that the tests are as realistic and reliable as possible.protocol/app/app.go (4)
- 95-95: The import of
go.uber.org/zapfor logging is a good choice for structured logging. Ensure that logging does not expose sensitive information.- 334-334: Initialization of
SlinkyClientis correctly placed within theAppstruct. Ensure that the client is properly managed and closed upon application shutdown to release resources.- 808-818: The initialization of the
SlinkyClientwithin a conditional block checking fordaemonFlags.Slinky.Enabledis a good practice, ensuring that the client is only initialized if the Slinky daemon is enabled. However, ensure that error handling is in place for the initialization process to gracefully handle any failures.- 1370-1370: The setup for
priceUpdateGeneratorwithNewSlinkyPriceUpdateGeneratorand related components appears to be well-structured. However, consider evaluating the performance impact of these operations, especially in high-throughput scenarios, to ensure that the system remains scalable.protocol/testing/genesis.sh (4)
- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-23]
The initial comments and variable definitions are clear and well-documented.
- 77-85: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [25-82]
Ensure that external JSON files referenced in the script (e.g., exchange_config_json_dir and delay_msg_json_dir) are correctly formatted and present as expected. These files are critical for the correct configuration of the genesis state.
- 77-85: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [84-100]
The
add_subaccountfunction correctly updates the genesis.json file with subaccount configurations. Ensure that input parameters are validated, especially if this script is used in contexts where data might come from untrusted sources.
- 477-483: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [102-990]
The script correctly applies a wide range of configurations to the genesis.json file using
dasel. Verify the correctness and presence of external JSON files used for configurations. Consider adding error handling for file operations to improve the script's robustness in different execution environments.
| var extCommitBzTx []byte | ||
| if len(req.Txs) >= slinkyabci.NumInjectedTxs { | ||
| extCommitBzTx = req.Txs[slinkyabci.OracleInfoIndex] | ||
| } | ||
|
|
||
| // get the update market prices tx | ||
| msg, err := priceUpdateGenerator.GetValidMarketPriceUpdates(ctx, extCommitBzTx) | ||
| if err != nil { | ||
| ctx.Logger().Error(fmt.Sprintf("GetValidMarketPriceUpdates error: %v", err)) | ||
| recordErrorMetricsWithLabel(metrics.PricesTx) | ||
| return &abci.ResponsePrepareProposal{Txs: [][]byte{}}, nil |
There was a problem hiding this comment.
The addition of logic to handle the Slinky-specific extCommitBzTx within the PrepareProposalHandler function is crucial for integrating Slinky's functionalities. However, ensure that there is adequate error handling and logging for scenarios where extCommitBzTx processing fails, to maintain robustness and ease of debugging.
Consider adding more detailed error logging or recovery mechanisms for scenarios where processing extCommitBzTx encounters issues.
| } | ||
| } | ||
|
|
||
| // GetUpdateMarketPricesTx returns a tx containing `MsgUpdateMarketPrices`. | ||
| func GetUpdateMarketPricesTx( | ||
| ctx sdk.Context, | ||
| // EncodeMarketPriceUpdates returns a tx containing `MsgUpdateMarketPrices`. | ||
| func EncodeMarketPriceUpdates( | ||
| txConfig client.TxConfig, | ||
| pricesKeeper PreparePricesKeeper, | ||
| msg *pricetypes.MsgUpdateMarketPrices, | ||
| ) (PricesTxResponse, error) { | ||
| // Get prices to update. | ||
| msgUpdateMarketPrices := pricesKeeper.GetValidMarketPriceUpdates(ctx) | ||
| if msgUpdateMarketPrices == nil { | ||
| return PricesTxResponse{}, fmt.Errorf("MsgUpdateMarketPrices cannot be nil") | ||
| } | ||
|
|
||
| tx, err := EncodeMsgsIntoTxBytes(txConfig, msgUpdateMarketPrices) | ||
| tx, err := EncodeMsgsIntoTxBytes(txConfig, msg) | ||
| if err != nil { | ||
| return PricesTxResponse{}, err | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [195-210]
The implementation of EncodeMarketPriceUpdates is well-structured and aligns with the objective of encoding market price updates into transactions. However, ensure that the error handling is robust, especially in scenarios where encoding fails or results in an invalid transaction.
Consider enhancing error messages to provide more context about the failure, which could aid in debugging and maintenance.
protocol/app/app.go
Outdated
| func (app *App) initOracleMetrics(appOpts servertypes.AppOptions) servicemetrics.Metrics { | ||
| cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| oracleMetrics, err := servicemetrics.NewMetricsFromConfig(cfg, app.ChainID()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| // run prometheus metrics | ||
| if cfg.MetricsEnabled { | ||
| promLogger, err := zap.NewProduction() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| app.oraclePrometheusServer, err = promserver.NewPrometheusServer(cfg.PrometheusServerAddress, promLogger) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| // start the prometheus server | ||
| go app.oraclePrometheusServer.Start() | ||
| } | ||
| return oracleMetrics | ||
| } |
There was a problem hiding this comment.
The method initOracleMetrics correctly initializes oracle metrics and handles potential errors by panicking. While panicking ensures that the application does not run in an improperly configured state, consider logging the error for better visibility before panicking.
if err != nil {
app.Logger().Error("Failed to initialize oracle metrics", "error", err)
panic(err)
}| func newClient(ctx context.Context, logger log.Logger) *Client { | ||
| logger = logger.With(log.ModuleKey, SlinkyClientDaemonModuleName) | ||
| client := &Client{ | ||
| HealthCheckable: daemontypes.NewTimeBoundedHealthCheckable( | ||
| SlinkyClientDaemonModuleName, | ||
| &libtime.TimeProviderImpl{}, | ||
| logger, | ||
| ), | ||
| logger: logger, | ||
| } | ||
| client.ctx, client.cf = context.WithCancel(ctx) | ||
| return client |
There was a problem hiding this comment.
The newClient function is private, which is appropriate since it's only meant to be used within the package. However, consider documenting the function to explain its purpose and usage within the package, enhancing maintainability and readability.
// newClient initializes a new Client with the provided context and logger.
// It sets up a cancellable context and initializes the HealthCheckable interface.| // RunPriceFetcher periodically calls the priceFetcher to grab prices from the slinky sidecar and | ||
| // push them to the pricefeed server. | ||
| func (c *Client) RunPriceFetcher(ctx context.Context) { | ||
| err := c.priceFetcher.Start(ctx) | ||
| if err != nil { | ||
| c.logger.Error("Error initializing PriceFetcher in slinky daemon: %w", err) | ||
| panic(err) | ||
| } | ||
| ticker := time.NewTicker(SlinkyPriceFetchDelay) | ||
| defer ticker.Stop() | ||
| for { | ||
| select { | ||
| case <-ticker.C: | ||
| err := c.priceFetcher.FetchPrices(ctx) | ||
| if err != nil { | ||
| c.logger.Error("Failed to run fetch prices for slinky daemon", "error", err) | ||
| c.ReportFailure(errors.Wrap(err, "failed to run PriceFetcher for slinky daemon")) | ||
| } else { | ||
| c.ReportSuccess() | ||
| } | ||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
In the RunPriceFetcher method, the use of panic (line 83) upon encountering an error during the initialization of PriceFetcher is concerning. Crashing the entire daemon due to a single component's failure is generally not advisable. Consider implementing a more graceful error handling mechanism, such as retrying with backoff or reporting the error to a monitoring system.
- panic(err)
+ c.logger.Error("Failed to initialize PriceFetcher, retrying...", "error", err)
// Implement retry logic hereCommittable 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.
| // RunPriceFetcher periodically calls the priceFetcher to grab prices from the slinky sidecar and | |
| // push them to the pricefeed server. | |
| func (c *Client) RunPriceFetcher(ctx context.Context) { | |
| err := c.priceFetcher.Start(ctx) | |
| if err != nil { | |
| c.logger.Error("Error initializing PriceFetcher in slinky daemon: %w", err) | |
| panic(err) | |
| } | |
| ticker := time.NewTicker(SlinkyPriceFetchDelay) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ticker.C: | |
| err := c.priceFetcher.FetchPrices(ctx) | |
| if err != nil { | |
| c.logger.Error("Failed to run fetch prices for slinky daemon", "error", err) | |
| c.ReportFailure(errors.Wrap(err, "failed to run PriceFetcher for slinky daemon")) | |
| } else { | |
| c.ReportSuccess() | |
| } | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| // RunPriceFetcher periodically calls the priceFetcher to grab prices from the slinky sidecar and | |
| // push them to the pricefeed server. | |
| func (c *Client) RunPriceFetcher(ctx context.Context) { | |
| err := c.priceFetcher.Start(ctx) | |
| if err != nil { | |
| c.logger.Error("Error initializing PriceFetcher in slinky daemon: %w", err) | |
| c.logger.Error("Failed to initialize PriceFetcher, retrying...", "error", err) | |
| // Implement retry logic here | |
| } | |
| ticker := time.NewTicker(SlinkyPriceFetchDelay) | |
| defer ticker.Stop() | |
| for { | |
| select { | |
| case <-ticker.C: | |
| err := c.priceFetcher.FetchPrices(ctx) | |
| if err != nil { | |
| c.logger.Error("Failed to run fetch prices for slinky daemon", "error", err) | |
| c.ReportFailure(errors.Wrap(err, "failed to run PriceFetcher for slinky daemon")) | |
| } else { | |
| c.ReportSuccess() | |
| } | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } |
| func (k Keeper) GetCurrencyPairFromID(ctx sdk.Context, id uint64) (cp slinkytypes.CurrencyPair, found bool) { | ||
| mp, found := k.GetMarketParam(ctx, uint32(id)) | ||
| if !found { | ||
| return cp, false | ||
| } | ||
| cp, err := MarketPairToCurrencyPair(mp.Pair) | ||
| if err != nil { | ||
| k.Logger(ctx).Error("CurrencyPairFromString", "error", err) | ||
| return cp, false | ||
| } | ||
| return cp, true |
There was a problem hiding this comment.
The method GetCurrencyPairFromID correctly fetches and converts a market parameter to a Slinky currency pair. However, it's important to ensure that error logging provides enough context for debugging. Consider including the id in the log message when an error occurs during conversion.
if err != nil {
- k.Logger(ctx).Error("CurrencyPairFromString", "error", err)
+ k.Logger(ctx).Error("CurrencyPairFromString", "id", id, "error", err)
return cp, false
}This change improves the debuggability of the system by providing more context in the error log.
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 (k Keeper) GetCurrencyPairFromID(ctx sdk.Context, id uint64) (cp slinkytypes.CurrencyPair, found bool) { | |
| mp, found := k.GetMarketParam(ctx, uint32(id)) | |
| if !found { | |
| return cp, false | |
| } | |
| cp, err := MarketPairToCurrencyPair(mp.Pair) | |
| if err != nil { | |
| k.Logger(ctx).Error("CurrencyPairFromString", "error", err) | |
| return cp, false | |
| } | |
| return cp, true | |
| func (k Keeper) GetCurrencyPairFromID(ctx sdk.Context, id uint64) (cp slinkytypes.CurrencyPair, found bool) { | |
| mp, found := k.GetMarketParam(ctx, uint32(id)) | |
| if !found { | |
| return cp, false | |
| } | |
| cp, err := MarketPairToCurrencyPair(mp.Pair) | |
| if err != nil { | |
| k.Logger(ctx).Error("CurrencyPairFromString", "id", id, "error", err) | |
| return cp, false | |
| } | |
| return cp, true |
| func (k Keeper) GetPriceForCurrencyPair(ctx sdk.Context, cp slinkytypes.CurrencyPair) (oracletypes.QuotePrice, error) { | ||
| id, found := k.GetIDForCurrencyPair(ctx, cp) | ||
| if !found { | ||
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String()) | ||
| } | ||
| mp, err := k.GetMarketPrice(ctx, uint32(id)) | ||
| if err != nil { | ||
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String()) | ||
| } | ||
| return oracletypes.QuotePrice{ | ||
| Price: math.NewIntFromUint64(mp.Price), | ||
| }, nil |
There was a problem hiding this comment.
The method GetPriceForCurrencyPair correctly fetches the price for a given currency pair. However, the error message for a not found currency pair could be more descriptive by including the currency pair identifier.
if !found {
- return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String())
+ return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found for id %d", cp.String(), id)
}This change ensures that the error message provides more context, improving the system's debuggability.
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 (k Keeper) GetPriceForCurrencyPair(ctx sdk.Context, cp slinkytypes.CurrencyPair) (oracletypes.QuotePrice, error) { | |
| id, found := k.GetIDForCurrencyPair(ctx, cp) | |
| if !found { | |
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String()) | |
| } | |
| mp, err := k.GetMarketPrice(ctx, uint32(id)) | |
| if err != nil { | |
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String()) | |
| } | |
| return oracletypes.QuotePrice{ | |
| Price: math.NewIntFromUint64(mp.Price), | |
| }, nil | |
| func (k Keeper) GetPriceForCurrencyPair(ctx sdk.Context, cp slinkytypes.CurrencyPair) (oracletypes.QuotePrice, error) { | |
| id, found := k.GetIDForCurrencyPair(ctx, cp) | |
| if !found { | |
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found for id %d", cp.String(), id) | |
| } | |
| mp, err := k.GetMarketPrice(ctx, uint32(id)) | |
| if err != nil { | |
| return oracletypes.QuotePrice{}, fmt.Errorf("currency pair %s not found", cp.String()) | |
| } | |
| return oracletypes.QuotePrice{ | |
| Price: math.NewIntFromUint64(mp.Price), | |
| }, nil |
| // ExtendVoteHandler is a wrapper around the Slinky ExtendVoteHandler. This wrapper is responsible for | ||
| // applying the newest MarketPriceUpdates in a block so that the prices to be submitted in a vote extension are | ||
| // determined on the latest available information. | ||
| type ExtendVoteHandler struct { | ||
| SlinkyExtendVoteHandler sdk.ExtendVoteHandler | ||
| PricesTxDecoder process.UpdateMarketPriceTxDecoder | ||
| PricesKeeper PricesKeeper | ||
| } | ||
|
|
||
| // ExtendVoteHandler returns a sdk.ExtendVoteHandler, responsible for the following: | ||
| // 1. Decoding the x/prices MsgUpdateMarketPrices in the current block - fail on errors | ||
| // 2. Validating the proposed MsgUpdateMarketPrices in accordance with the ProcessProposal check | ||
| // 3. Updating the market prices in the PricesKeeper so that the GetValidMarketPriceUpdates function returns the | ||
| // latest available market prices | ||
| // 4. Calling the Slinky ExtendVoteHandler to handle the rest of ExtendVote | ||
| func (e *ExtendVoteHandler) ExtendVoteHandler() sdk.ExtendVoteHandler { | ||
| return func(ctx sdk.Context, req *cometabci.RequestExtendVote) (resp *cometabci.ResponseExtendVote, err error) { | ||
| // Decode the x/prices txn in the current block | ||
| updatePricesTx, err := e.PricesTxDecoder.DecodeUpdateMarketPricesTx(ctx, req.Txs) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("DecodeMarketPricesTx failure %w", err) | ||
| } | ||
|
|
||
| // ensure that the proposed MsgUpdateMarketPrices is valid in accordance w/ stateful information | ||
| // this check is equivalent to the check in ProcessProposal (indexPriceCache has not been updated) | ||
| err = updatePricesTx.Validate() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("updatePricesTx.Validate failure %w", err) | ||
| } | ||
|
|
||
| // Update the market prices in the PricesKeeper, so that the GetValidMarketPriceUpdates | ||
| // function returns the latest available market prices. | ||
| updateMarketPricesMsg, ok := updatePricesTx.GetMsg().(*prices.MsgUpdateMarketPrices) | ||
| if !ok { | ||
| return nil, fmt.Errorf("expected %s, got %T", "MsgUpdateMarketPrices", updateMarketPricesMsg) | ||
| } | ||
|
|
||
| // Update the market prices in the PricesKeeper | ||
| err = e.PricesKeeper.UpdateMarketPrices(ctx, updateMarketPricesMsg.MarketPriceUpdates) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to update market prices in extend vote handler pre-slinky invocation %w", err) | ||
| } | ||
|
|
||
| // Call the Slinky ExtendVoteHandler | ||
| return e.SlinkyExtendVoteHandler(ctx, req) | ||
| } |
There was a problem hiding this comment.
The ExtendVoteHandler struct and its method for extending votes are correctly implemented. However, ensure that error handling is robust, especially when decoding market prices transactions and updating market prices in the PricesKeeper. Any errors in these operations should be logged or handled appropriately to avoid silent failures.
if err != nil {
- return nil, fmt.Errorf("DecodeMarketPricesTx failure %w", err)
+ sdkCtx.Logger().Error("DecodeMarketPricesTx failure", "error", err)
+ return nil, err
}
...
if err != nil {
- return nil, fmt.Errorf("failed to update market prices in extend vote handler pre-slinky invocation %w", err)
+ sdkCtx.Logger().Error("Failed to update market prices in extend vote handler pre-slinky invocation", "error", err)
+ return nil, err
}This change ensures that errors are not only returned but also logged, improving the system's debuggability and reliability.
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.
| // ExtendVoteHandler is a wrapper around the Slinky ExtendVoteHandler. This wrapper is responsible for | |
| // applying the newest MarketPriceUpdates in a block so that the prices to be submitted in a vote extension are | |
| // determined on the latest available information. | |
| type ExtendVoteHandler struct { | |
| SlinkyExtendVoteHandler sdk.ExtendVoteHandler | |
| PricesTxDecoder process.UpdateMarketPriceTxDecoder | |
| PricesKeeper PricesKeeper | |
| } | |
| // ExtendVoteHandler returns a sdk.ExtendVoteHandler, responsible for the following: | |
| // 1. Decoding the x/prices MsgUpdateMarketPrices in the current block - fail on errors | |
| // 2. Validating the proposed MsgUpdateMarketPrices in accordance with the ProcessProposal check | |
| // 3. Updating the market prices in the PricesKeeper so that the GetValidMarketPriceUpdates function returns the | |
| // latest available market prices | |
| // 4. Calling the Slinky ExtendVoteHandler to handle the rest of ExtendVote | |
| func (e *ExtendVoteHandler) ExtendVoteHandler() sdk.ExtendVoteHandler { | |
| return func(ctx sdk.Context, req *cometabci.RequestExtendVote) (resp *cometabci.ResponseExtendVote, err error) { | |
| // Decode the x/prices txn in the current block | |
| updatePricesTx, err := e.PricesTxDecoder.DecodeUpdateMarketPricesTx(ctx, req.Txs) | |
| if err != nil { | |
| return nil, fmt.Errorf("DecodeMarketPricesTx failure %w", err) | |
| } | |
| // ensure that the proposed MsgUpdateMarketPrices is valid in accordance w/ stateful information | |
| // this check is equivalent to the check in ProcessProposal (indexPriceCache has not been updated) | |
| err = updatePricesTx.Validate() | |
| if err != nil { | |
| return nil, fmt.Errorf("updatePricesTx.Validate failure %w", err) | |
| } | |
| // Update the market prices in the PricesKeeper, so that the GetValidMarketPriceUpdates | |
| // function returns the latest available market prices. | |
| updateMarketPricesMsg, ok := updatePricesTx.GetMsg().(*prices.MsgUpdateMarketPrices) | |
| if !ok { | |
| return nil, fmt.Errorf("expected %s, got %T", "MsgUpdateMarketPrices", updateMarketPricesMsg) | |
| } | |
| // Update the market prices in the PricesKeeper | |
| err = e.PricesKeeper.UpdateMarketPrices(ctx, updateMarketPricesMsg.MarketPriceUpdates) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to update market prices in extend vote handler pre-slinky invocation %w", err) | |
| } | |
| // Call the Slinky ExtendVoteHandler | |
| return e.SlinkyExtendVoteHandler(ctx, req) | |
| } | |
| if err != nil { | |
| sdkCtx.Logger().Error("DecodeMarketPricesTx failure", "error", err) | |
| return nil, err | |
| } | |
| if err != nil { | |
| sdkCtx.Logger().Error("Failed to update market prices in extend vote handler pre-slinky invocation", "error", err) | |
| return nil, err | |
| } |
| // OracleClient is a wrapper around the default Slinky OracleClient interface. This object is responsible for requesting | ||
| // prices from the x/prices module (originally sent via the sidecar to the price-feed service), and | ||
| // injecting those prices into the vote-extension. | ||
| type OracleClient struct { | ||
| PricesKeeper PricesKeeper | ||
| } | ||
|
|
||
| // NewOracleClient returns a new OracleClient object. | ||
| func NewOracleClient(pricesKeeper PricesKeeper) *OracleClient { | ||
| return &OracleClient{ | ||
| PricesKeeper: pricesKeeper, | ||
| } | ||
| } | ||
|
|
||
| // Start starts the OracleClient. | ||
| func (o *OracleClient) Start(_ context.Context) error { | ||
| return nil | ||
| } | ||
|
|
||
| // Stop stops the OracleClient. | ||
| func (o *OracleClient) Stop() error { | ||
| return nil | ||
| } | ||
|
|
||
| // Prices is a wrapper around the Slinky OracleClient's Prices method. | ||
| // This method is responsible for doing the following: | ||
| // 1. Get the latest prices from the x/prices module's indexPriceCache via GetValidMarketPriceUpdates | ||
| // 2. Translate the response from x/prices into a QueryPricesResponse, and return it. | ||
| // | ||
| // This method fails if: | ||
| // - The passed in context is not an sdk.Context | ||
| func (o *OracleClient) Prices(ctx context.Context, | ||
| _ *oracleservicetypes.QueryPricesRequest, | ||
| opts ...grpc.CallOption) (*oracleservicetypes.QueryPricesResponse, error) { | ||
| sdkCtx, ok := ctx.(sdk.Context) | ||
| if !ok { | ||
| return nil, fmt.Errorf("oracle client was passed on non-sdk context object") | ||
| } | ||
|
|
||
| // get the final prices to include in the vote-extension from the x/prices module | ||
| validUpdates := o.PricesKeeper.GetValidMarketPriceUpdates(sdkCtx) | ||
| if validUpdates == nil { | ||
| sdkCtx.Logger().Info("prices keeper returned no valid market price updates") | ||
| return nil, nil | ||
| } | ||
| sdkCtx.Logger().Info("prices keeper returned valid updates", "length", len(validUpdates.MarketPriceUpdates)) | ||
|
|
||
| // translate price updates into oracle response | ||
| var outputResponse = &oracleservicetypes.QueryPricesResponse{ | ||
| Prices: make(map[string]string), | ||
| Timestamp: time.Now(), | ||
| } | ||
| for _, update := range validUpdates.MarketPriceUpdates { | ||
| mappedPair, found := o.PricesKeeper.GetCurrencyPairFromID(sdkCtx, uint64(update.GetMarketId())) | ||
| if found { | ||
| sdkCtx.Logger().Info("added currency pair", "pair", mappedPair.String()) | ||
| outputResponse.Prices[mappedPair.String()] = strconv.FormatUint(update.Price, 10) | ||
| } else { | ||
| sdkCtx.Logger().Info("failed to add currency pair", "pair", mappedPair.String()) | ||
| } | ||
| } | ||
| return outputResponse, nil | ||
| } |
There was a problem hiding this comment.
The OracleClient struct and its Prices method are correctly implemented. However, ensure that error handling is robust, especially when fetching the latest prices and translating them into a QueryPricesResponse. Any errors in these operations should be logged or handled appropriately to avoid silent failures or incorrect data being injected into the vote-extension.
if validUpdates == nil {
- sdkCtx.Logger().Info("prices keeper returned no valid market price updates")
+ sdkCtx.Logger().Error("Prices keeper returned no valid market price updates")
return nil, fmt.Errorf("no valid market price updates available")
}
...
if !found {
- sdkCtx.Logger().Info("failed to add currency pair", "pair", mappedPair.String())
+ sdkCtx.Logger().Error("Failed to add currency pair", "pair", mappedPair.String())
}This change ensures that errors and unexpected conditions are not only logged but also handled appropriately, improving the system's reliability and debuggability.
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.
| // OracleClient is a wrapper around the default Slinky OracleClient interface. This object is responsible for requesting | |
| // prices from the x/prices module (originally sent via the sidecar to the price-feed service), and | |
| // injecting those prices into the vote-extension. | |
| type OracleClient struct { | |
| PricesKeeper PricesKeeper | |
| } | |
| // NewOracleClient returns a new OracleClient object. | |
| func NewOracleClient(pricesKeeper PricesKeeper) *OracleClient { | |
| return &OracleClient{ | |
| PricesKeeper: pricesKeeper, | |
| } | |
| } | |
| // Start starts the OracleClient. | |
| func (o *OracleClient) Start(_ context.Context) error { | |
| return nil | |
| } | |
| // Stop stops the OracleClient. | |
| func (o *OracleClient) Stop() error { | |
| return nil | |
| } | |
| // Prices is a wrapper around the Slinky OracleClient's Prices method. | |
| // This method is responsible for doing the following: | |
| // 1. Get the latest prices from the x/prices module's indexPriceCache via GetValidMarketPriceUpdates | |
| // 2. Translate the response from x/prices into a QueryPricesResponse, and return it. | |
| // | |
| // This method fails if: | |
| // - The passed in context is not an sdk.Context | |
| func (o *OracleClient) Prices(ctx context.Context, | |
| _ *oracleservicetypes.QueryPricesRequest, | |
| opts ...grpc.CallOption) (*oracleservicetypes.QueryPricesResponse, error) { | |
| sdkCtx, ok := ctx.(sdk.Context) | |
| if !ok { | |
| return nil, fmt.Errorf("oracle client was passed on non-sdk context object") | |
| } | |
| // get the final prices to include in the vote-extension from the x/prices module | |
| validUpdates := o.PricesKeeper.GetValidMarketPriceUpdates(sdkCtx) | |
| if validUpdates == nil { | |
| sdkCtx.Logger().Info("prices keeper returned no valid market price updates") | |
| return nil, nil | |
| } | |
| sdkCtx.Logger().Info("prices keeper returned valid updates", "length", len(validUpdates.MarketPriceUpdates)) | |
| // translate price updates into oracle response | |
| var outputResponse = &oracleservicetypes.QueryPricesResponse{ | |
| Prices: make(map[string]string), | |
| Timestamp: time.Now(), | |
| } | |
| for _, update := range validUpdates.MarketPriceUpdates { | |
| mappedPair, found := o.PricesKeeper.GetCurrencyPairFromID(sdkCtx, uint64(update.GetMarketId())) | |
| if found { | |
| sdkCtx.Logger().Info("added currency pair", "pair", mappedPair.String()) | |
| outputResponse.Prices[mappedPair.String()] = strconv.FormatUint(update.Price, 10) | |
| } else { | |
| sdkCtx.Logger().Info("failed to add currency pair", "pair", mappedPair.String()) | |
| } | |
| } | |
| return outputResponse, nil | |
| } | |
| // OracleClient is a wrapper around the default Slinky OracleClient interface. This object is responsible for requesting | |
| // prices from the x/prices module (originally sent via the sidecar to the price-feed service), and | |
| // injecting those prices into the vote-extension. | |
| type OracleClient struct { | |
| PricesKeeper PricesKeeper | |
| } | |
| // NewOracleClient returns a new OracleClient object. | |
| func NewOracleClient(pricesKeeper PricesKeeper) *OracleClient { | |
| return &OracleClient{ | |
| PricesKeeper: pricesKeeper, | |
| } | |
| } | |
| // Start starts the OracleClient. | |
| func (o *OracleClient) Start(_ context.Context) error { | |
| return nil | |
| } | |
| // Stop stops the OracleClient. | |
| func (o *OracleClient) Stop() error { | |
| return nil | |
| } | |
| // Prices is a wrapper around the Slinky OracleClient's Prices method. | |
| // This method is responsible for doing the following: | |
| // 1. Get the latest prices from the x/prices module's indexPriceCache via GetValidMarketPriceUpdates | |
| // 2. Translate the response from x/prices into a QueryPricesResponse, and return it. | |
| // | |
| // This method fails if: | |
| // - The passed in context is not an sdk.Context | |
| func (o *OracleClient) Prices(ctx context.Context, | |
| _ *oracleservicetypes.QueryPricesRequest, | |
| opts ...grpc.CallOption) (*oracleservicetypes.QueryPricesResponse, error) { | |
| sdkCtx, ok := ctx.(sdk.Context) | |
| if !ok { | |
| return nil, fmt.Errorf("oracle client was passed on non-sdk context object") | |
| } | |
| // get the final prices to include in the vote-extension from the x/prices module | |
| validUpdates := o.PricesKeeper.GetValidMarketPriceUpdates(sdkCtx) | |
| if validUpdates == nil { | |
| sdkCtx.Logger().Error("Prices keeper returned no valid market price updates") | |
| return nil, fmt.Errorf("no valid market price updates available") | |
| } | |
| sdkCtx.Logger().Info("prices keeper returned valid updates", "length", len(validUpdates.MarketPriceUpdates)) | |
| // translate price updates into oracle response | |
| var outputResponse = &oracleservicetypes.QueryPricesResponse{ | |
| Prices: make(map[string]string), | |
| Timestamp: time.Now(), | |
| } | |
| for _, update := range validUpdates.MarketPriceUpdates { | |
| mappedPair, found := o.PricesKeeper.GetCurrencyPairFromID(sdkCtx, uint64(update.GetMarketId())) | |
| if found { | |
| sdkCtx.Logger().Info("added currency pair", "pair", mappedPair.String()) | |
| outputResponse.Prices[mappedPair.String()] = strconv.FormatUint(update.Price, 10) | |
| } else { | |
| sdkCtx.Logger().Error("Failed to add currency pair", "pair", mappedPair.String()) | |
| } | |
| } | |
| return outputResponse, nil | |
| } |
| func TestValidPriceResponse(t *testing.T) { | ||
| pk := mocks.NewPricesKeeper(t) | ||
| cli := NewOracleClient(pk) | ||
| pk.On("GetValidMarketPriceUpdates", mock.Anything). | ||
| Return(&types.MsgUpdateMarketPrices{ | ||
| MarketPriceUpdates: []*types.MsgUpdateMarketPrices_MarketPrice{ | ||
| {MarketId: 2, Price: 1}, | ||
| }, | ||
| }).Once() | ||
| pk.On("GetCurrencyPairFromID", mock.Anything, mock.Anything). | ||
| Return( | ||
| oracletypes.CurrencyPair{Base: "FOO", Quote: "BAR"}, | ||
| true, | ||
| ).Once() | ||
|
|
||
| _, err := cli.Prices(sdk.Context{}.WithLogger(log.NewNopLogger()), nil) | ||
|
|
||
| require.NoError(t, err) | ||
| pk.AssertExpectations(t) | ||
| } |
There was a problem hiding this comment.
The test TestValidPriceResponse correctly verifies that the Prices method returns a valid response when the PricesKeeper returns valid market price updates. However, ensure that the test also verifies the contents of the response to ensure that the translation from PricesKeeper updates to the QueryPricesResponse is accurate.
_, err := cli.Prices(sdk.Context{}.WithLogger(log.NewNopLogger()), nil)
+ require.NoError(t, err)
+ // Add assertions to verify the contents of the responseThis change ensures that the test not only checks for the absence of errors but also verifies the accuracy of the response contents.
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 TestValidPriceResponse(t *testing.T) { | |
| pk := mocks.NewPricesKeeper(t) | |
| cli := NewOracleClient(pk) | |
| pk.On("GetValidMarketPriceUpdates", mock.Anything). | |
| Return(&types.MsgUpdateMarketPrices{ | |
| MarketPriceUpdates: []*types.MsgUpdateMarketPrices_MarketPrice{ | |
| {MarketId: 2, Price: 1}, | |
| }, | |
| }).Once() | |
| pk.On("GetCurrencyPairFromID", mock.Anything, mock.Anything). | |
| Return( | |
| oracletypes.CurrencyPair{Base: "FOO", Quote: "BAR"}, | |
| true, | |
| ).Once() | |
| _, err := cli.Prices(sdk.Context{}.WithLogger(log.NewNopLogger()), nil) | |
| require.NoError(t, err) | |
| pk.AssertExpectations(t) | |
| } | |
| func TestValidPriceResponse(t *testing.T) { | |
| pk := mocks.NewPricesKeeper(t) | |
| cli := NewOracleClient(pk) | |
| pk.On("GetValidMarketPriceUpdates", mock.Anything). | |
| Return(&types.MsgUpdateMarketPrices{ | |
| MarketPriceUpdates: []*types.MsgUpdateMarketPrices_MarketPrice{ | |
| {MarketId: 2, Price: 1}, | |
| }, | |
| }).Once() | |
| pk.On("GetCurrencyPairFromID", mock.Anything, mock.Anything). | |
| Return( | |
| oracletypes.CurrencyPair{Base: "FOO", Quote: "BAR"}, | |
| true, | |
| ).Once() | |
| _, err := cli.Prices(sdk.Context{}.WithLogger(log.NewNopLogger()), nil) | |
| require.NoError(t, err) | |
| // Add assertions to verify the contents of the response | |
| require.NoError(t, err) | |
| pk.AssertExpectations(t) | |
| } |
protocol/app/app.go
Outdated
| ) | ||
| app.RegisterDaemonWithHealthMonitor(app.PriceFeedClient, maxDaemonUnhealthyDuration) | ||
| } | ||
| if daemonFlags.Slinky.Enabled { |
There was a problem hiding this comment.
if both price daemon and slinky are enabled, we will run both?
| } | ||
|
|
||
| // NewDefaultPriceUpdateGenerator returns a new DefaultPriceUpdateGenerator. | ||
| func NewDefaultPriceUpdateGenerator(keeper PricesKeeper) *DefaultPriceUpdateGenerator { |
There was a problem hiding this comment.
when is this ever used? Shouldn't this replace SlinkyPriceUpdateGenerator if we set a flag? I dont see this being called anywhere
There was a problem hiding this comment.
Updated--current version can run either VE-based prepare/process or existing proposer based oracle logic. And we can switch the sidecar/pricefeed daemon on and off as well.
commit 96d900d Author: Eric <eric.warehime@gmail.com> Date: Tue Mar 5 08:51:29 2024 -0800 Fix non-initialized return values commit 9ec7992 Author: Eric <eric.warehime@gmail.com> Date: Tue Mar 5 08:42:48 2024 -0800 Upstream compat commit 47809c7 Merge: 7607b9e f6b594c Author: Eric <eric.warehime@gmail.com> Date: Tue Mar 5 08:40:34 2024 -0800 Merge remote-tracking branch 'upstream' into eric/protocol-feature-flags commit 7607b9e Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 14:58:06 2024 -0800 Fix accidental deletes commit 297b592 Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 14:22:06 2024 -0800 Merge commit 6c32623 Merge: 5811502 daad125 Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 13:50:26 2024 -0800 Merge remote-tracking branch 'origin/eric/ve-logic' into eric/protocol-feature-flags commit 5811502 Merge: 6adf756 7feaddb Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 13:49:23 2024 -0800 Merge remote-tracking branch 'origin/eric/proposal-logic' into eric/protocol-feature-flags commit 6adf756 Merge: c84eb82 88675f5 Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 13:44:46 2024 -0800 Merge remote-tracking branch 'origin/eric/sidecar-integration' into eric/protocol-feature-flags commit daad125 Author: Eric <eric.warehime@gmail.com> Date: Mon Mar 4 10:51:23 2024 -0800 VoteExtension slinky logic commit 7feaddb Author: Eric <eric.warehime@gmail.com> Date: Sun Mar 3 21:28:32 2024 -0800 Fix import order commit 382d45c Author: Eric <eric.warehime@gmail.com> Date: Sun Mar 3 21:24:41 2024 -0800 Prepare and Process logic for Slinky commit 88675f5 Author: Eric <eric.warehime@gmail.com> Date: Tue Feb 27 12:46:33 2024 -0800 Register daemon with health monitor commit 8438d54 Author: Eric <eric.warehime@gmail.com> Date: Tue Feb 27 12:32:36 2024 -0800 PR updates commit 3046d69 Author: Eric <eric.warehime@gmail.com> Date: Mon Feb 26 15:05:52 2024 -0800 Make unflaky commit 7799d29 Author: Eric <eric.warehime@gmail.com> Date: Mon Feb 26 15:01:18 2024 -0800 Lints, more tests commit e0a8c37 Author: Eric <eric.warehime@gmail.com> Date: Mon Feb 26 14:35:11 2024 -0800 Add tests commit d8e5e90 Author: Eric <eric.warehime@gmail.com> Date: Sat Feb 24 21:01:35 2024 -0800 Add interfaces, generate mocks commit 3a8ebec Author: Eric <eric.warehime@gmail.com> Date: Sat Feb 24 10:18:33 2024 -0800 Lint commit 5cdd35f Author: Eric <eric.warehime@gmail.com> Date: Sat Feb 24 10:09:56 2024 -0800 Initial commit of sidecar integration
Eric-Warehime
force-pushed
the
eric/protocol-feature-flags
branch
from
96d900d
to
935c393
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/go.modis excluded by:!**/*.modprotocol/go.sumis excluded by:!**/*.sum
Files selected for processing (14)
- protocol/Dockerfile (1 hunks)
- protocol/app/app.go (9 hunks)
- protocol/app/app_test.go (1 hunks)
- protocol/app/flags/flags.go (5 hunks)
- protocol/app/prepare/full_node_prepare_proposal.go (1 hunks)
- protocol/app/prepare/prepare_proposal.go (7 hunks)
- protocol/app/process/slinky_market_price_decoder.go (1 hunks)
- protocol/app/process/slinky_market_price_decoder_test.go (1 hunks)
- protocol/cmd/dydxprotocold/cmd/config.go (2 hunks)
- protocol/daemons/flags/flags.go (2 hunks)
- protocol/mocks/Makefile (1 hunks)
- protocol/testing/genesis.sh (70 hunks)
- protocol/testing/testnet-local/local.sh (1 hunks)
- protocol/x/prices/keeper/slinky_adapter_test.go (2 hunks)
Files not summarized due to errors (1)
- protocol/testing/genesis.sh: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- protocol/app/process/slinky_market_price_decoder.go
Additional comments: 19
protocol/app/prepare/full_node_prepare_proposal.go (1)
- 23-23: The change to return an empty array of bytes instead of an empty response object seems appropriate for the described behavior. However, it would be beneficial for maintainability to add a comment explaining why an empty array of bytes is specifically required in this context.
protocol/Dockerfile (1)
- 60-61: The modifications to the Dockerfile for copying oracle and market configuration files are clear and align with the integration requirements for the slinky daemon. Please ensure that the configuration files exist at the specified paths to avoid runtime errors during container startup.
protocol/x/prices/keeper/slinky_adapter_test.go (1)
- 2-7: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the
slinkypackage import and theTestMarketPairToCurrencyPairfunction suggests a change in the testing strategy. Please ensure that the functionality previously tested byTestMarketPairToCurrencyPairis either covered elsewhere in the test suite or has become obsolete.protocol/cmd/dydxprotocold/cmd/config.go (1)
- 57-63: The addition of the
Oracleconfiguration is a necessary update for integrating the slinky daemon and is well-defined. However, please verify the impact of removingMaxOpenConnectionsandGRPCMaxOpenConnectionssettings on the system's performance and stability, ensuring that connection management is still optimized.protocol/app/flags/flags.go (1)
- 25-25: The addition of the
VEOracleEnabledflag for Slinky Vote Extensions is clear and necessary for the feature's integration. It would be beneficial to add documentation or comments explaining the purpose and usage of this flag for future maintainability.protocol/mocks/Makefile (1)
- 61-61: The reorganization of mock generation commands, including the movement of the
PriceUpdateGeneratorcommand, appears to be a maintenance improvement. Please verify that all necessary mocks are generated as expected and that the reorganization does not omit any required mock generation commands.protocol/app/app_test.go (1)
- 109-109: The addition of
oraclePrometheusServerto the list of expected uninitialized fields inTestAppIsFullyInitializedaligns with the PR's objectives of integrating Slinky and its components into the protocol. This change suggests thatoraclePrometheusServeris a new component related to the Slinky integration, which should not be initialized by default under certain test conditions. Ensure that this behavior is consistent with the intended design and operational requirements of theoraclePrometheusServerwithin the protocol.protocol/daemons/flags/flags.go (2)
- 77-78: Adding the
Enabledfield to theSlinkyFlagsstruct is a straightforward and effective way to toggle the Slinky daemon's operation. This change aligns with the PR's objective of introducing feature flagging for the Slinky integration. It's crucial to ensure that all parts of the codebase that interact with the Slinky daemon respect this flag to prevent unintended behavior when the daemon is disabled.- 122-122: Setting the default value of the
Enabledfield tofalsein theGetDefaultDaemonFlagsfunction is a cautious approach, ensuring that the Slinky daemon is off by default. This decision minimizes the risk of inadvertently enabling the daemon without explicit configuration, aligning with the principle of secure defaults. It's important to document this behavior clearly so that operators are aware they need to enable the daemon explicitly if they wish to use its functionality.protocol/app/process/slinky_market_price_decoder_test.go (1)
- 8-10: The addition of
github.com/skip-mev/slinky/abci/testutilsandgithub.com/stretchr/testify/suiteimports is appropriate for the context of this test file. Thetestutilspackage is likely used for creating test contexts and other utility functions specific to testing Slinky-related functionality, whiletestify/suiteprovides a structured way to organize tests into suites, enhancing readability and maintainability. It's important to ensure that these packages are used effectively throughout the test suite to justify their inclusion.protocol/app/prepare/prepare_proposal.go (2)
- 13-13: The addition of the
github.com/dydxprotocol/v4-chain/protocol/app/constantsimport is necessary for accessing constants used within thePrepareProposalHandlerfunction, such asconstants.OracleVEInjectedTxsandconstants.OracleInfoIndex. This change supports the logic that handles vote extensions and oracle data, contributing to the PR's objective of integrating Slinky with governance mechanisms. Ensuring that these constants are used correctly throughout the function is crucial for maintaining the integrity of the proposal preparation process.- 94-134: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [97-181]
The modifications to error handling in the
PrepareProposalHandlerfunction, where specificabci.ResponsePrepareProposalinstances with empty transaction slices are returned instead of a genericEmptyResponse, improve the clarity and specificity of the error responses. This change enhances the maintainability of the code by making it easier to understand the flow of error handling and the conditions under which different types of errors occur. It's important to ensure that all error paths are tested to verify that the correct responses are returned under various failure conditions.protocol/testing/genesis.sh (7)
- 80-82: The consensus parameters are being set with hardcoded values. Ensure these values (
max_bytes,max_gas, andvote_extensions_enable_height) are aligned with the network's performance and security requirements.- 1001-1001: The minimum price change ppm (parts per million) for the DYDX-USD market is set to
2500(0.25%). Ensure this threshold aligns with the market's volatility and liquidity to avoid unnecessary price update transactions that could congest the network.- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1025-1035]
The function
update_genesis_use_test_exchangehardcodes the use of a "TestExchange" for BTC, ETH, and LINK markets. This is appropriate for testing environments but should be clearly documented or flagged to avoid accidental use in production environments.
- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1039-1050]
Adding a test volatile market (TEST-USD) with specific liquidity tiers and perpetuals configurations is a good practice for testing environments. Ensure that the configurations for this test market, such as leverage and margin requirements, are aligned with the intended testing scenarios.
- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1054-1062]
The function
update_all_markets_with_fixed_price_exchangesets all markets to use a "TestFixedPriceExchange". This approach is suitable for testing but should be clearly documented to prevent its use in production environments. Additionally, verify that the "TestFixedPriceExchange" is configured correctly in the system to provide the expected fixed prices.
- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1066-1066]
Reducing the complete bridge delay for testing purposes is reasonable. However, ensure that this configuration is not accidentally applied to production environments, as it could affect the security and reliability of cross-chain operations.
- 77-85: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1070-1085]
The
set_denom_metadatafunction correctly sets human-readable metadata for token denominations. This enhances the user experience by providing clear and understandable information about the token. Ensure that the metadata values such asbase_denom,whole_coin_denom, andcoin_nameare accurate and meaningful.
protocol/testing/genesis.sh
Outdated
| @@ -520,7 +488,7 @@ function edit_genesis() { | |||
| dasel put -t string -f "$GENESIS" '.app_state.prices.market_params.[0].pair' -v 'BTC-USD' | |||
| dasel put -t int -f "$GENESIS" '.app_state.prices.market_params.[0].id' -v '0' | |||
| dasel put -t int -f "$GENESIS" '.app_state.prices.market_params.[0].exponent' -v '-5' | |||
| dasel put -t int -f "$GENESIS" '.app_state.prices.market_params.[0].min_exchanges' -v '3' | |||
| dasel put -t int -f "$GENESIS" '.app_state.prices.market_params.[0].min_exchanges' -v '1' | |||
There was a problem hiding this comment.
The minimum number of exchanges required for price updates is set to 1. This could potentially introduce price manipulation risks if only one exchange is used for price data. Consider increasing this number to ensure price data robustness.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/genesis.sh (35 hunks)
Additional comments: 6
protocol/testing/genesis.sh (6)
- 80-82: The consensus parameters configuration, including the enabling of ABCI vote extensions, appears correctly set.
- 82-82: The assets module configuration, including the setup for the USDC asset, is properly defined.
- 82-82: The perpetuals module configuration, including liquidity tiers and perpetual market setups, is thoroughly defined and appears correct.
- 82-82: The prices module configuration, including market prices and exchange configurations for various assets, is well-structured and comprehensive.
- 82-82: The clob module configuration, including setups for clob pairs, liquidations, block rate limits, and equity tier limits, is meticulously defined and appears correct.
- 82-82: The additional configurations and functions, including denom metadata setting, genesis updates, and ICA params updates, are appropriately defined and serve important purposes for network setup and operations.
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/go.modis excluded by:!**/*.mod
Files selected for processing (1)
- protocol/app/app.go (9 hunks)
Additional comments: 9
protocol/app/app.go (9)
- 207-222: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [95-219]
The addition of imports related to
slinkyandskip-mev/slinkyaligns with the PR's objective to integrate Slinky into the protocol. It's crucial to ensure that these libraries are well-maintained and secure, given their critical role in the application.
- 210-219: The imports from
github.com/skip-mev/slinkyintroduce various components necessary for Slinky's integration, such as proposals, strategies, and metrics. It's important to review these components for security and performance implications, especially since they involve external data processing and might impact the protocol's governance mechanisms.- 344-345: The introduction of
oraclePrometheusServerandoracleMetricsfor monitoring and metrics collection is a positive step towards enhancing observability. However, ensure that the Prometheus server is securely configured to prevent unauthorized access to sensitive metrics data.- 1371-1373: Refactoring the
Newfunction to create proposal handlers (prepareProposalHandler,processProposalHandler) is a significant change. It's essential to ensure that these handlers are correctly implemented and tested, especially since they play a crucial role in the proposal processing workflow.- 1429-1598: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1423-1446]
The
initOracleMetricsfunction introduces a new way to initialize metrics for the oracle component. While the addition of metrics is beneficial for monitoring, it's crucial to ensure that the metrics do not inadvertently expose sensitive information and that the Prometheus server is securely configured.
- 1448-1460: The
initSlinkySidecarClientfunction is crucial for setting up the Slinky client. It's important to ensure that the client is securely configured, especially in terms of network communication and data handling, to prevent potential security vulnerabilities.- 1468-1569: The changes related to creating proposal handlers (
createProposalHandlers) introduce significant logic for handling proposals with Slinky integration. It's essential to thoroughly review this logic for correctness, security implications, and adherence to best practices, especially given its impact on governance mechanisms.- 1571-1594: The introduction of the
initOraclefunction and related vote extension handling logic is a critical part of the Slinky integration. It's important to ensure that the vote extension logic is secure and correctly implemented, as it directly impacts the integrity of voting and governance processes.- 1870-1872: Closing the
oraclePrometheusServerin theClosefunction is a necessary cleanup step. Ensure that all resources associated with the Prometheus server are properly released to prevent resource leaks.
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/go.modis excluded by:!**/*.modprotocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (2)
- protocol/cmd/dydxprotocold/cmd/config.go (1 hunks)
- protocol/testing/genesis.sh (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/cmd/dydxprotocold/cmd/config.go
- protocol/testing/genesis.sh
protocol/app/flags/flags.go
Outdated
| @@ -47,6 +51,7 @@ const ( | |||
| DefaultDdErrorTrackingFormat = false | |||
|
|
|||
| DefaultGrpcStreamingEnabled = false | |||
| DefaultVEOracleEnabled = false | |||
There was a problem hiding this comment.
don't we want this to be enable to true for the next release?
protocol/app/app.go
Outdated
| app.StakingKeeper, | ||
| app.PerpetualsKeeper, | ||
| priceUpdateDecoder, | ||
| ) |
There was a problem hiding this comment.
if NonValidatingFullNode and VEOracleEnabled, right now you are returning the VE process proposal handler instead of the full node process proposal handler. Shouldn't we return dydxPrepareProposalHandler, dydxProcessProposalHandler here?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/app/app.go (9 hunks)
- protocol/app/flags/flags.go (5 hunks)
- protocol/daemons/flags/flags.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/flags/flags.go
- protocol/daemons/flags/flags.go
Additional comments: 1
protocol/app/app.go (1)
- 1429-1594: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1423-1446]
The
initOracleMetricsfunction reads configuration and initializes metrics, but it panics on any error. While panicking ensures that the application does not run in an improperly configured state, it might be more user-friendly to handle these errors gracefully, allowing for a more controlled shutdown or retry mechanism. Consider logging the error and returning it to the caller for a decision on how to proceed.
protocol/app/app.go
Outdated
| func (app *App) initSlinkySidecarClient(appOpts servertypes.AppOptions) oracleclient.OracleClient { | ||
| // Create the oracle service. | ||
| if app.oracleMetrics == nil { | ||
| app.oracleMetrics = app.initOracleMetrics(appOpts) | ||
| } | ||
| cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| slinkyClient, err := oracleclient.NewClientFromConfig( | ||
| cfg, | ||
| app.Logger().With("client", "oracle"), | ||
| oracleMetrics, | ||
| app.oracleMetrics, | ||
| ) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return slinkyClient | ||
| } |
There was a problem hiding this comment.
Similar to initOracleMetrics, the initSlinkySidecarClient function panics on error when reading configuration or creating a new client. This abrupt termination might not be the most user-friendly approach. It's recommended to handle errors more gracefully by logging them and returning them to the caller, allowing for a more controlled response to configuration issues or failures in client creation.
protocol/app/app.go
Outdated
| func (app *App) createProposalHandlers( | ||
| appFlags flags.Flags, | ||
| txConfig client.TxConfig, | ||
| appOpts servertypes.AppOptions, | ||
| ) (sdk.PrepareProposalHandler, sdk.ProcessProposalHandler) { | ||
| var priceUpdateDecoder process.UpdateMarketPriceTxDecoder = process.NewDefaultUpdateMarketPriceTxDecoder( | ||
| app.PricesKeeper, app.txConfig.TxDecoder()) | ||
| // If the node is a NonValidatingFullNode, we don't need to run any oracle code | ||
| // Note: If the command-line flag `--non-validating-full-node` is enabled, this node will use | ||
| // an implementation of `ProcessProposal` which always returns `abci.ResponseProcessProposal_ACCEPT`. | ||
| // Full-nodes do not participate in consensus, and therefore should not participate in voting / `ProcessProposal`. | ||
| if appFlags.NonValidatingFullNode { | ||
| if app.oracleMetrics == nil { | ||
| app.oracleMetrics = servicemetrics.NewNopMetrics() | ||
| } | ||
| return prepare.FullNodePrepareProposalHandler(), process.FullNodeProcessProposalHandler( | ||
| txConfig, | ||
| app.BridgeKeeper, | ||
| app.ClobKeeper, | ||
| app.StakingKeeper, | ||
| app.PerpetualsKeeper, | ||
| priceUpdateDecoder, | ||
| ) | ||
| } | ||
| strategy := currencypair.NewDefaultCurrencyPairStrategy(app.PricesKeeper) | ||
| var priceUpdateGenerator prices.PriceUpdateGenerator = prices.NewDefaultPriceUpdateGenerator(app.PricesKeeper) | ||
|
|
||
| veCodec := compression.NewCompressionVoteExtensionCodec( | ||
| compression.NewDefaultVoteExtensionCodec(), | ||
| compression.NewZLibCompressor(), | ||
| ) | ||
| extCommitCodec := compression.NewCompressionExtendedCommitCodec( | ||
| compression.NewDefaultExtendedCommitCodec(), | ||
| compression.NewZLibCompressor(), | ||
| ) | ||
|
|
||
| // Set Price Update Generators/Decoders for Slinky | ||
| if appFlags.VEOracleEnabled { | ||
| priceUpdateGenerator = prices.NewSlinkyPriceUpdateGenerator( | ||
| aggregator.NewDefaultVoteAggregator( | ||
| app.Logger(), | ||
| voteweighted.MedianFromContext( | ||
| app.Logger(), | ||
| app.StakingKeeper, | ||
| voteweighted.DefaultPowerThreshold, | ||
| ), | ||
| strategy, | ||
| ), | ||
| extCommitCodec, | ||
| veCodec, | ||
| strategy, | ||
| ) | ||
| priceUpdateDecoder = process.NewSlinkyMarketPriceDecoder( | ||
| priceUpdateDecoder, | ||
| priceUpdateGenerator, | ||
| ) | ||
| } | ||
| // Generate the dydx handlers | ||
| dydxPrepareProposalHandler := prepare.PrepareProposalHandler( | ||
| txConfig, | ||
| app.BridgeKeeper, | ||
| app.ClobKeeper, | ||
| app.PerpetualsKeeper, | ||
| priceUpdateGenerator, | ||
| ) | ||
|
|
||
| // ProcessProposal setup. | ||
| var dydxProcessProposalHandler = process.ProcessProposalHandler( | ||
| txConfig, | ||
| app.BridgeKeeper, | ||
| app.ClobKeeper, | ||
| app.StakingKeeper, | ||
| app.PerpetualsKeeper, | ||
| app.PricesKeeper, | ||
| priceUpdateDecoder, | ||
| ) | ||
|
|
||
| // Wrap dydx handlers with slinky handlers | ||
| if appFlags.VEOracleEnabled { | ||
| if app.oracleMetrics == nil { | ||
| app.oracleMetrics = app.initOracleMetrics(appOpts) | ||
| } | ||
| app.initOracle(priceUpdateDecoder) | ||
| proposalHandler := slinkyproposals.NewProposalHandler( | ||
| app.Logger(), | ||
| dydxPrepareProposalHandler, | ||
| dydxProcessProposalHandler, | ||
| ve.NewDefaultValidateVoteExtensionsFn(app.ChainID(), app.StakingKeeper), | ||
| veCodec, | ||
| extCommitCodec, | ||
| strategy, | ||
| app.oracleMetrics, | ||
| slinkyproposals.RetainOracleDataInWrappedProposalHandler(), | ||
| ) | ||
| return proposalHandler.PrepareProposalHandler(), proposalHandler.ProcessProposalHandler() | ||
| } | ||
| return dydxPrepareProposalHandler, dydxProcessProposalHandler | ||
| } |
There was a problem hiding this comment.
The createProposalHandlers function configures proposal handlers based on application flags and configurations. It's well-structured and modular, but it directly panics inside conditional blocks when setting up metrics or initializing oracle-related functionalities. As mentioned earlier, consider adopting a more graceful error handling strategy that logs errors and returns them, rather than using panic. This approach would improve the robustness and maintainability of the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/daemons/slinky/config/market.jsonis excluded by:!**/*.jsonprotocol/daemons/slinky/config/oracle.jsonis excluded by:!**/*.json
Files selected for processing (6)
- protocol/app/app.go (9 hunks)
- protocol/app/flags/flags.go (6 hunks)
- protocol/daemons/flags/flags.go (1 hunks)
- protocol/testing/testnet-local/local.sh (2 hunks)
- protocol/testutil/app/app.go (2 hunks)
- protocol/testutil/appoptions/app_options.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/app/flags/flags.go
Additional comments: 10
protocol/testutil/appoptions/app_options.go (1)
- 52-57: The addition of flags to disable the Slinky Daemon and Slinky Metrics by default for end-to-end and integration tests is a good practice. It ensures that tests run in a controlled environment without external dependencies unless explicitly enabled. This change aligns with the principle of having predictable and isolated test conditions.
protocol/daemons/flags/flags.go (1)
- 118-119: Adding
MetricsEnabledandPrometheusServerAddressfields to theSlinkyFlagsstruct allows for fine-grained control over metrics reporting and the configuration of the Prometheus server address. This is a positive change that enhances observability and monitoring capabilities for the Slinky daemon.protocol/testutil/app/app.go (1)
- 1290-1291: The changes from lines 1290 to 1291 involve adding flags
--oracle.enabled=falseand--oracle.metrics_enabled=falseto the command execution in thelaunchValidatorInDirfunction. This is consistent with the PR's goal to disable Oracle features and metrics. It's crucial to verify that these flags are correctly recognized by the application and that their inclusion does not conflict with other command-line arguments or configurations. Additionally, ensure that the documentation or help text for the command-line interface is updated to reflect these new flags.protocol/app/app.go (7)
- 207-222: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [95-219]
The changes from lines 95 to 219 introduce a significant number of new imports, including those related to the Slinky integration and other functionalities. It's crucial to ensure that these imports are necessary and correctly utilized within the file to avoid unnecessary dependencies and potential conflicts.
- 339-345: The introduction of
SlinkyClient,oraclePrometheusServer, andoracleMetricswithin theAppstruct is a key part of integrating Slinky into the application. It's important to ensure that these fields are properly initialized and used throughout the application to support the new functionalities.- 1425-1433: The
initSlinkySidecarClientfunction correctly initializes the Slinky oracle client using configuration options. It's essential to handle the potential error fromoracleconfig.ReadConfigFromAppOptsgracefully to prevent the application from panicking in scenarios where the configuration might be missing or malformed.- 1441-1535: The
createProposalHandlersfunction introduces handlers for preparing and processing proposals, including specific logic for handling price updates and Slinky-related functionalities. It's important to ensure that the logic within this function correctly implements the intended behaviors and interacts properly with other parts of the application, especially considering the conditional logic based on flags likeNonValidatingFullNodeandVEOracleEnabled.- 1537-1560: The
initOraclefunction sets up vote extension handling for the Slinky integration, including the creation of a vote extension handler and extending the vote handler. It's crucial to ensure that this setup is correctly configured to support the new oracle functionalities without introducing unintended side effects or vulnerabilities.- 1562-1585: The
initOracleMetricsfunction initializes metrics for the oracle, including setting up a Prometheus server if metrics are enabled. It's important to ensure that the metrics are correctly configured and that the Prometheus server is securely set up to prevent unauthorized access to metrics data.- 1861-1863: The
Closefunction now includes logic to close theoraclePrometheusServerif it's not nil. It's important to ensure that all resources are correctly released and that the application shuts down gracefully to prevent resource leaks or other shutdown-related issues.
| @@ -1,5 +1,5 @@ | |||
| #!/bin/bash | |||
| set -eo pipefail | |||
| set -exo pipefail | |||
There was a problem hiding this comment.
The addition of the -x option to the set command enables script debugging, which is beneficial for troubleshooting. However, consider making this behavior optional to avoid cluttering the output during regular use. An environment variable could control the inclusion of the -x flag, allowing users to enable or disable debugging as needed.
- set -exo pipefail
+ set -eo pipefail # Optionally add `-x` based on an environment variable for debugging.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.
| set -exo pipefail | |
| set -eo pipefail # Optionally add `-x` based on an environment variable for debugging. |
| @@ -1259,6 +1259,7 @@ func launchValidatorInDir( | |||
| appConfig.API.Enable = false | |||
| // We disable telemetry since multiple instances of the application fail to register causing a panic. | |||
| appConfig.Telemetry.Enabled = false | |||
| appConfig.Oracle.MetricsEnabled = false | |||
There was a problem hiding this comment.
The changes from lines 1262 to 1262 involve setting MetricsEnabled to false for the Oracle configuration in appConfig. This aligns with the PR's objective to disable Oracle features and metrics as part of the integration process. However, it's important to ensure that this change does not inadvertently affect other functionalities that might rely on Oracle metrics being enabled. Consider adding a comment explaining the reason for this change to aid future maintainability.
+ // Disabling Oracle metrics as part of the Slinky integration process
appConfig.Oracle.MetricsEnabled = falseCommittable 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.
| appConfig.Oracle.MetricsEnabled = false | |
| // Disabling Oracle metrics as part of the Slinky integration process | |
| appConfig.Oracle.MetricsEnabled = false |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/scripts/genesis/sample_pregenesis.jsonis excluded by:!**/*.json
Files selected for processing (2)
- protocol/app/app.go (9 hunks)
- protocol/testing/genesis.sh (1 hunks)
| "github.com/cosmos/ibc-go/modules/capability" | ||
| capabilitykeeper "github.com/cosmos/ibc-go/modules/capability/keeper" | ||
| capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" | ||
| "github.com/dydxprotocol/v4-chain/protocol/daemons/configs" | ||
| "github.com/gorilla/mux" | ||
| "github.com/rakyll/statik/fs" | ||
| "github.com/spf13/cast" | ||
| "go.uber.org/zap" | ||
| "google.golang.org/grpc" | ||
|
|
||
| // App |
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]
Given the static analysis hint and the context of the PR, it's crucial to ensure secure communication for gRPC servers. The setup for the gRPC server lacks encryption, which could expose sensitive data or allow unauthorized access. Consider using SSL/TLS credentials for a secure connection.
- app.Server = daemonserver.NewServer(logger, grpc.NewServer(), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)
+ app.Server = daemonserver.NewServer(logger, grpc.NewServer(grpc.Creds(credentials.NewServerTLSFromFile("cert.pem", "cert.key"))), &daemontypes.FileHandlerImpl{}, daemonFlags.Shared.SocketAddress)| // Create the oracle service. | ||
| cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| oracleMetrics, err := servicemetrics.NewMetricsFromConfig(cfg, app.ChainID()) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| // Create the oracle service. | ||
| slinkyClient, err := oracleclient.NewClientFromConfig( | ||
| cfg, | ||
| app.Logger().With("client", "oracle"), | ||
| oracleMetrics, | ||
| app.oracleMetrics, |
There was a problem hiding this comment.
The creation of the oracleClient within initSlinkySidecarClient function does not handle potential errors gracefully. It's important to ensure that the application can recover or exit cleanly in case of configuration errors.
- slinkyClient, err := oracleclient.NewClientFromConfig(cfg, app.Logger().With("client", "oracle"), app.oracleMetrics)
- if err != nil {
- panic(err)
- }
+ slinkyClient, err := oracleclient.NewClientFromConfig(cfg, app.Logger().With("client", "oracle"), app.oracleMetrics)
+ if err != nil {
+ app.Logger().Error("Failed to create oracle client from config", "error", err)
+ return nil // Or handle the error as appropriate for your application's error handling strategy
+ }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.
| // Create the oracle service. | |
| cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts) | |
| if err != nil { | |
| panic(err) | |
| } | |
| oracleMetrics, err := servicemetrics.NewMetricsFromConfig(cfg, app.ChainID()) | |
| if err != nil { | |
| panic(err) | |
| } | |
| // Create the oracle service. | |
| slinkyClient, err := oracleclient.NewClientFromConfig( | |
| cfg, | |
| app.Logger().With("client", "oracle"), | |
| oracleMetrics, | |
| app.oracleMetrics, | |
| // Create the oracle service. | |
| cfg, err := oracleconfig.ReadConfigFromAppOpts(appOpts) | |
| if err != nil { | |
| panic(err) | |
| } | |
| slinkyClient, err := oracleclient.NewClientFromConfig( | |
| cfg, | |
| app.Logger().With("client", "oracle"), | |
| app.oracleMetrics, | |
| if err != nil { | |
| app.Logger().Error("Failed to create oracle client from config", "error", err) | |
| return nil // Or handle the error as appropriate for your application's error handling strategy | |
| } |
| if app.oraclePrometheusServer != nil { | ||
| app.oraclePrometheusServer.Close() | ||
| } |
There was a problem hiding this comment.
Closing the oraclePrometheusServer without checking if an error occurred during the shutdown can lead to unnoticed issues. It's a good practice to log or handle errors even during shutdown processes.
- app.oraclePrometheusServer.Close()
+ if err := app.oraclePrometheusServer.Close(); err != nil {
+ app.Logger().Error("Failed to close oracle Prometheus server", "error", err)
+ }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.
| if app.oraclePrometheusServer != nil { | |
| app.oraclePrometheusServer.Close() | |
| } | |
| if app.oraclePrometheusServer != nil { | |
| if err := app.oraclePrometheusServer.Close(); err != nil { | |
| app.Logger().Error("Failed to close oracle Prometheus server", "error", err) | |
| } | |
| } |
| dasel put -t string -f "$GENESIS" '.genesis_time' -v "$GENESIS_TIME" | ||
|
|
||
| # Consensus params | ||
| dasel put -t string -f "$GENESIS" '.consensus_params.block.max_bytes' -v '4194304' | ||
| dasel put -t string -f "$GENESIS" '.consensus_params.block.max_gas' -v '-1' | ||
| dasel put -t string -f "$GENESIS" '.consensus.params.block.max_bytes' -v '4194304' | ||
| dasel put -t string -f "$GENESIS" '.consensus.params.block.max_gas' -v '-1' | ||
| dasel put -t string -f "$GENESIS" '.consensus.params.abci.vote_extensions_enable_height' -v '1' | ||
|
|
||
| # Update crisis module. | ||
| dasel put -t string -f "$GENESIS" '.app_state.crisis.constant_fee.denom' -v "$NATIVE_TOKEN" |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [29-30]
Variables TESTNET_VALIDATOR_SELF_DELEGATE_AMOUNT and FAUCET_NATIVE_TOKEN_BALANCE appear unused in the script. If these are intended for external use, consider exporting them or documenting their purpose to avoid confusion.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [531-996]
Several instances of cat commands are used in a manner that could be simplified. For example, instead of using cat "$EXCHANGE_CONFIG_JSON_DIR/btc_exchange_config.json" | jq -c '.', you can directly pass the file to jq like so: jq -c '.' "$EXCHANGE_CONFIG_JSON_DIR/btc_exchange_config.json". This applies to all similar uses of cat throughout the script.
- btc_exchange_config_json=$(cat "$EXCHANGE_CONFIG_JSON_DIR/btc_exchange_config.json" | jq -c '.')
+ btc_exchange_config_json=$(jq -c '.' "$EXCHANGE_CONFIG_JSON_DIR/btc_exchange_config.json")Repeat this pattern for all similar instances.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1051-1060]
The use of $/${} for arithmetic variables is unnecessary. You can directly use the variable name without them for arithmetic operations. This applies to all similar instances in the script.
- total_accounts_quote_balance=$(($total_accounts_quote_balance + $DEFAULT_SUBACCOUNT_QUOTE_BALANCE))
+ total_accounts_quote_balance=$((total_accounts_quote_balance + DEFAULT_SUBACCOUNT_QUOTE_BALANCE))📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1075-1075]
Consider quoting this variable expansion to prevent word splitting, which can lead to unexpected behavior in scripts.
- bridge_module_account_balance=$(echo "$bridge_module_account_balance - $TESTNET_VALIDATOR_NATIVE_TOKEN_BALANCE" | bc)
+ bridge_module_account_balance=$(echo "$bridge_module_account_balance - \"$TESTNET_VALIDATOR_NATIVE_TOKEN_BALANCE\"" | bc)📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1583-1619]
When using variables in commands that might be affected by word splitting or globbing, it's a good practice to quote them. This ensures that the variables are treated as single arguments even if they contain spaces or glob characters.
- dasel put -t string -f "$GENESIS" '.app_state.prices.market_params.[0].exchange_config_json' -v "$btc_exchange_config_json"
+ dasel put -t string -f "$GENESIS" '.app_state.prices.market_params.[0].exchange_config_json' -v "$btc_exchange_config_json"Apply this change to all similar instances where variables are used in commands.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2e/trading_rewards/trading_rewards_test.go (2 hunks)
Additional comments: 2
protocol/testing/e2e/trading_rewards/trading_rewards_test.go (2)
- 6-6: The addition of the
flagspackage import is necessary for accessing theVEOracleEnabledflag. This change is straightforward and correctly implemented.- 709-709: Setting
flags.VEOracleEnabledtofalsewithin theappOptsmap directly impacts the behavior of the test suite by disabling the Vote Extension Oracle functionality. This change is crucial for testing scenarios where the Oracle is not supposed to influence the outcome. Ensure that this configuration aligns with the intended test scenarios and that any tests relying on the Oracle being enabled are appropriately adjusted or flagged.Consider verifying that all test cases that require the Oracle to be enabled have been reviewed and adjusted accordingly. This ensures that the test suite accurately reflects the intended functionality and scenarios.
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/cancel_order_cli_test.go (1 hunks)
- protocol/x/clob/client/cli/liquidations_cli_test.go (1 hunks)
- protocol/x/clob/client/cli/place_order_cli_test.go (1 hunks)
Additional comments: 3
protocol/x/clob/client/cli/place_order_cli_test.go (1)
- 71-71: The addition of
appOptions.Set(daemonflags.FlagOracleEnabled, false)effectively disables the Oracle daemon for integration tests. This is a crucial change for ensuring that tests run in a controlled environment without external dependencies on the Oracle daemon. It's important to verify that this change does not inadvertently affect other tests or functionalities that might rely on the Oracle daemon being active.protocol/x/clob/client/cli/liquidations_cli_test.go (1)
- 77-77: The addition of
appOptions.Set(daemonflags.FlagOracleEnabled, false)in the integration tests setup for liquidations is consistent with the approach taken in other test suites to ensure a controlled test environment. This change is necessary for tests that should not depend on external Oracle daemon behavior. It's good practice to isolate tests from external dependencies unless they are explicitly testing interactions with those external systems.protocol/x/clob/client/cli/cancel_order_cli_test.go (1)
- 76-76: The inclusion of
appOptions.Set(daemonflags.FlagOracleEnabled, false)in the setup for integration tests related to canceling orders is a prudent measure to ensure that the tests are not influenced by the Oracle daemon's state or behavior. This aligns with the best practices of creating predictable and isolated test environments, especially for integration tests that could be affected by external services.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/prices/client/cli/prices_cli_test.go (1 hunks)
Additional comments: 1
protocol/x/prices/client/cli/prices_cli_test.go (1)
- 98-98: The addition of
appOptions.Set(daemonflags.FlagOracleEnabled, false)disables the Oracle daemon for these integration tests. This change seems aligned with the PR's objectives and the need to control test conditions.However, it's important to ensure that disabling the Oracle daemon does not inadvertently skip testing critical integration points, especially given the comprehensive nature of the Slinky integration. Consider documenting the rationale behind this decision and verifying that all necessary functionalities are adequately tested.
Changelist
Fully integrates Slinky in the protocol.
Test Plan
All tests included. Perf tests results can be included here or surfaced out of band.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.