[CORE-813] - Add pricefeed config tomls back into the repository #863
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent changes involve the dynamic configuration of exchange query settings for a protocol. Instead of using a static configuration, the system now reads exchange query configurations from a file. Additionally, there's a new functionality to write a default configuration file if it doesn't exist. This is set up during the initialization phase of the application. The changes also include tests to ensure the new configuration functions work correctly and the necessary imports and constants to support these 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 X ? TipsChat with CodeRabbit Bot (
|
![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: 10
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- protocol/daemons/configs/test_data/broken_test.toml
- protocol/daemons/configs/test_data/missingvals_test.toml
- protocol/daemons/configs/test_data/valid_test.toml
- protocol/daemons/configs/test_data/wrongvaltype_test.toml
- protocol/go.mod
- protocol/go.sum
Files selected for processing (7)
- protocol/app/app.go (2 hunks)
- protocol/cmd/dydxprotocold/cmd/init.go (1 hunks)
- protocol/cmd/dydxprotocold/main.go (1 hunks)
- protocol/daemons/configs/default_pricefeed_exchange_config.go (1 hunks)
- protocol/daemons/configs/default_pricefeed_exchange_config_test.go (1 hunks)
- protocol/daemons/constants/pricefeed.go (1 hunks)
- protocol/testing/containertest/Dockerfile (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/daemons/constants/pricefeed.go
- protocol/testing/containertest/Dockerfile
Additional comments: 3
protocol/app/app.go (1)
- 630-634: The changes from lines 630 to 634 correctly implement the logic to read the exchange query configuration from a file, which aligns with the PR's objective to reintroduce pricefeed configuration files and ensure they are properly integrated and tested.
protocol/cmd/dydxprotocold/cmd/init.go (1)
- 25-25: The
WriteDefaultPricefeedExchangeTomlfunction is called without checking if the file already exists. According to the PR's objective and summary, the file should only be written if it does not exist. Ensure that the implementation ofWriteDefaultPricefeedExchangeTomlincludes a check for the file's existence or add such a check before calling the function.protocol/daemons/configs/default_pricefeed_exchange_config_test.go (1)
- 112-119: The test
TestGenerateDefaultExchangeTomlStringcorrectly asserts that the generated TOML string matches the expected value. This ensures that the default configuration string is generated as intended.
Comment on lines
+133
to
+146
| func TestWriteDefaultPricefeedExchangeToml_FileExists(t *testing.T) { | ||
| helloWorld := "Hello World" | ||
|
|
||
| err := os.Mkdir("config", 0700) | ||
| require.NoError(t, err) | ||
|
|
||
| tmos.MustWriteFile(filePath, bytes.NewBuffer([]byte(helloWorld)).Bytes(), 0644) | ||
| configs.WriteDefaultPricefeedExchangeToml("") | ||
|
|
||
| buffer, err := os.ReadFile(filePath) | ||
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, helloWorld, string(buffer[:])) | ||
| os.RemoveAll("config") |
There was a problem hiding this comment.
The test TestWriteDefaultPricefeedExchangeToml_FileExists checks the behavior when the file already exists. It's good to see that it verifies the file is not overwritten. However, the cleanup process is repeated across tests and could be refactored into a teardown function to adhere to the DRY principle.
+ func teardown() {
+ os.RemoveAll(configDir)
+ }
- os.RemoveAll("config")
+ teardown()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.
Suggested change
| func TestWriteDefaultPricefeedExchangeToml_FileExists(t *testing.T) { | |
| helloWorld := "Hello World" | |
| err := os.Mkdir("config", 0700) | |
| require.NoError(t, err) | |
| tmos.MustWriteFile(filePath, bytes.NewBuffer([]byte(helloWorld)).Bytes(), 0644) | |
| configs.WriteDefaultPricefeedExchangeToml("") | |
| buffer, err := os.ReadFile(filePath) | |
| require.NoError(t, err) | |
| require.Equal(t, helloWorld, string(buffer[:])) | |
| os.RemoveAll("config") | |
| func TestWriteDefaultPricefeedExchangeToml_FileExists(t *testing.T) { | |
| helloWorld := "Hello World" | |
| err := os.Mkdir("config", 0700) | |
| require.NoError(t, err) | |
| tmos.MustWriteFile(filePath, bytes.NewBuffer([]byte(helloWorld)).Bytes(), 0644) | |
| configs.WriteDefaultPricefeedExchangeToml("") | |
| buffer, err := os.ReadFile(filePath) | |
| require.NoError(t, err) | |
| require.Equal(t, helloWorld, string(buffer[:])) | |
| teardown() |
</details>
<!-- suggestion_end -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+149
to
+242
| func TestReadExchangeStartupConfigFile(t *testing.T) { | ||
| pwd, _ := os.Getwd() | ||
|
|
||
| tests := map[string]struct { | ||
| // parameters | ||
| exchangeConfigSourcePath string | ||
| doNotWriteFile bool | ||
|
|
||
| // expectations | ||
| expectedExchangeId types.ExchangeId | ||
| expectedIntervalMsExchange uint32 | ||
| expectedTimeoutMsExchange uint32 | ||
| expectedMaxQueries uint32 | ||
| expectedPanic error | ||
| }{ | ||
| "valid": { | ||
| exchangeConfigSourcePath: "test_data/valid_test.toml", | ||
| expectedExchangeId: binanceId, | ||
| expectedIntervalMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].IntervalMs, | ||
| expectedTimeoutMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].TimeoutMs, | ||
| expectedMaxQueries: pfconstants.StaticExchangeQueryConfig[binanceId].MaxQueries, | ||
| }, | ||
| "config file cannot be found": { | ||
| exchangeConfigSourcePath: "test_data/notexisting_test.toml", | ||
| doNotWriteFile: true, | ||
| expectedPanic: fmt.Errorf( | ||
| "open %s%s: no such file or directory", | ||
| pwd+"/config/", | ||
| constants.PricefeedExchangeConfigFileName, | ||
| ), | ||
| }, | ||
| "config file cannot be unmarshalled": { | ||
| exchangeConfigSourcePath: "test_data/broken_test.toml", | ||
| expectedPanic: errors.New("(1, 12): was expecting token [[, but got unclosed table array key instead"), | ||
| }, | ||
| "config file has malformed values": { | ||
| exchangeConfigSourcePath: "test_data/missingvals_test.toml", | ||
| expectedPanic: errors.New( | ||
| "One or more query config values are unset or are set to zero for exchange with id: 'BinanceUS'", | ||
| ), | ||
| }, | ||
| "config file has incorrect values": { | ||
| exchangeConfigSourcePath: "test_data/wrongvaltype_test.toml", | ||
| expectedPanic: errors.New( | ||
| "(3, 1): Can't convert a(string) to uint32", | ||
| ), | ||
| }, | ||
| } | ||
|
|
||
| for name, tc := range tests { | ||
| t.Run(name, func(t *testing.T) { | ||
| if !tc.doNotWriteFile { | ||
| err := os.Mkdir("config", 0700) | ||
| require.NoError(t, err) | ||
|
|
||
| file, err := os.Open(tc.exchangeConfigSourcePath) | ||
| require.NoError(t, err) | ||
|
|
||
| config, err := os.Create(filepath.Join("config", constants.PricefeedExchangeConfigFileName)) | ||
| require.NoError(t, err) | ||
| _, err = config.ReadFrom(file) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| if tc.expectedPanic != nil { | ||
| require.PanicsWithError( | ||
| t, | ||
| tc.expectedPanic.Error(), | ||
| func() { configs.ReadExchangeQueryConfigFile(pwd) }, | ||
| ) | ||
|
|
||
| os.RemoveAll("config") | ||
| return | ||
| } | ||
|
|
||
| exchangeStartupConfigMap := configs.ReadExchangeQueryConfigFile(pwd) | ||
|
|
||
| require.Equal( | ||
| t, | ||
| &types.ExchangeQueryConfig{ | ||
| ExchangeId: tc.expectedExchangeId, | ||
| IntervalMs: tc.expectedIntervalMsExchange, | ||
| TimeoutMs: tc.expectedTimeoutMsExchange, | ||
| MaxQueries: tc.expectedMaxQueries, | ||
| }, | ||
| exchangeStartupConfigMap[tc.expectedExchangeId], | ||
| ) | ||
|
|
||
| os.RemoveAll("config") | ||
| }) | ||
| } | ||
|
|
||
| // In case tests fail and the path was never removed. | ||
| os.RemoveAll("config") |
There was a problem hiding this comment.
The test TestReadExchangeStartupConfigFile uses the current working directory to locate the configuration file, which may not be reliable in all environments. Consider using a more robust method to determine the project root or configuration path. Additionally, for better test isolation and to avoid side effects on the actual filesystem, consider using a mock filesystem or ioutil.TempDir to create temporary directories for testing.
- pwd, _ := os.Getwd()
+ tempDir, err := ioutil.TempDir("", "test")
+ require.NoError(t, err)
+ defer os.RemoveAll(tempDir) // cleanup after testCommittable 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.
Suggested change
| func TestReadExchangeStartupConfigFile(t *testing.T) { | |
| pwd, _ := os.Getwd() | |
| tests := map[string]struct { | |
| // parameters | |
| exchangeConfigSourcePath string | |
| doNotWriteFile bool | |
| // expectations | |
| expectedExchangeId types.ExchangeId | |
| expectedIntervalMsExchange uint32 | |
| expectedTimeoutMsExchange uint32 | |
| expectedMaxQueries uint32 | |
| expectedPanic error | |
| }{ | |
| "valid": { | |
| exchangeConfigSourcePath: "test_data/valid_test.toml", | |
| expectedExchangeId: binanceId, | |
| expectedIntervalMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].IntervalMs, | |
| expectedTimeoutMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].TimeoutMs, | |
| expectedMaxQueries: pfconstants.StaticExchangeQueryConfig[binanceId].MaxQueries, | |
| }, | |
| "config file cannot be found": { | |
| exchangeConfigSourcePath: "test_data/notexisting_test.toml", | |
| doNotWriteFile: true, | |
| expectedPanic: fmt.Errorf( | |
| "open %s%s: no such file or directory", | |
| pwd+"/config/", | |
| constants.PricefeedExchangeConfigFileName, | |
| ), | |
| }, | |
| "config file cannot be unmarshalled": { | |
| exchangeConfigSourcePath: "test_data/broken_test.toml", | |
| expectedPanic: errors.New("(1, 12): was expecting token [[, but got unclosed table array key instead"), | |
| }, | |
| "config file has malformed values": { | |
| exchangeConfigSourcePath: "test_data/missingvals_test.toml", | |
| expectedPanic: errors.New( | |
| "One or more query config values are unset or are set to zero for exchange with id: 'BinanceUS'", | |
| ), | |
| }, | |
| "config file has incorrect values": { | |
| exchangeConfigSourcePath: "test_data/wrongvaltype_test.toml", | |
| expectedPanic: errors.New( | |
| "(3, 1): Can't convert a(string) to uint32", | |
| ), | |
| }, | |
| } | |
| for name, tc := range tests { | |
| t.Run(name, func(t *testing.T) { | |
| if !tc.doNotWriteFile { | |
| err := os.Mkdir("config", 0700) | |
| require.NoError(t, err) | |
| file, err := os.Open(tc.exchangeConfigSourcePath) | |
| require.NoError(t, err) | |
| config, err := os.Create(filepath.Join("config", constants.PricefeedExchangeConfigFileName)) | |
| require.NoError(t, err) | |
| _, err = config.ReadFrom(file) | |
| require.NoError(t, err) | |
| } | |
| if tc.expectedPanic != nil { | |
| require.PanicsWithError( | |
| t, | |
| tc.expectedPanic.Error(), | |
| func() { configs.ReadExchangeQueryConfigFile(pwd) }, | |
| ) | |
| os.RemoveAll("config") | |
| return | |
| } | |
| exchangeStartupConfigMap := configs.ReadExchangeQueryConfigFile(pwd) | |
| require.Equal( | |
| t, | |
| &types.ExchangeQueryConfig{ | |
| ExchangeId: tc.expectedExchangeId, | |
| IntervalMs: tc.expectedIntervalMsExchange, | |
| TimeoutMs: tc.expectedTimeoutMsExchange, | |
| MaxQueries: tc.expectedMaxQueries, | |
| }, | |
| exchangeStartupConfigMap[tc.expectedExchangeId], | |
| ) | |
| os.RemoveAll("config") | |
| }) | |
| } | |
| // In case tests fail and the path was never removed. | |
| os.RemoveAll("config") | |
| func TestReadExchangeStartupConfigFile(t *testing.T) { | |
| tempDir, err := ioutil.TempDir("", "test") | |
| require.NoError(t, err) | |
| defer os.RemoveAll(tempDir) // cleanup after test | |
| tests := map[string]struct { | |
| // parameters | |
| exchangeConfigSourcePath string | |
| doNotWriteFile bool | |
| // expectations | |
| expectedExchangeId types.ExchangeId | |
| expectedIntervalMsExchange uint32 | |
| expectedTimeoutMsExchange uint32 | |
| expectedMaxQueries uint32 | |
| expectedPanic error | |
| }{ | |
| "valid": { | |
| exchangeConfigSourcePath: "test_data/valid_test.toml", | |
| expectedExchangeId: binanceId, | |
| expectedIntervalMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].IntervalMs, | |
| expectedTimeoutMsExchange: pfconstants.StaticExchangeQueryConfig[binanceId].TimeoutMs, | |
| expectedMaxQueries: pfconstants.StaticExchangeQueryConfig[binanceId].MaxQueries, | |
| }, | |
| "config file cannot be found": { | |
| exchangeConfigSourcePath: "test_data/notexisting_test.toml", | |
| doNotWriteFile: true, | |
| expectedPanic: fmt.Errorf( | |
| "open %s%s: no such file or directory", | |
| tempDir+"/config/", | |
| constants.PricefeedExchangeConfigFileName, | |
| ), | |
| }, | |
| "config file cannot be unmarshalled": { | |
| exchangeConfigSourcePath: "test_data/broken_test.toml", | |
| expectedPanic: errors.New("(1, 12): was expecting token [[, but got unclosed table array key instead"), | |
| }, | |
| "config file has malformed values": { | |
| exchangeConfigSourcePath: "test_data/missingvals_test.toml", | |
| expectedPanic: errors.New( | |
| "One or more query config values are unset or are set to zero for exchange with id: 'BinanceUS'", | |
| ), | |
| }, | |
| "config file has incorrect values": { | |
| exchangeConfigSourcePath: "test_data/wrongvaltype_test.toml", | |
| expectedPanic: errors.New( | |
| "(3, 1): Can't convert a(string) to uint32", | |
| ), | |
| }, | |
| } | |
| for name, tc := range tests { | |
| t.Run(name, func(t *testing.T) { | |
| if !tc.doNotWriteFile { | |
| err := os.Mkdir(tempDir+"/config", 0700) | |
| require.NoError(t, err) | |
| file, err := os.Open(tc.exchangeConfigSourcePath) | |
| require.NoError(t, err) | |
| config, err := os.Create(filepath.Join(tempDir+"/config", constants.PricefeedExchangeConfigFileName)) | |
| require.NoError(t, err) | |
| _, err = config.ReadFrom(file) | |
| require.NoError(t, err) | |
| } | |
| if tc.expectedPanic != nil { | |
| require.PanicsWithError( | |
| t, | |
| tc.expectedPanic.Error(), | |
| func() { configs.ReadExchangeQueryConfigFile(tempDir) }, | |
| ) | |
| os.RemoveAll(tempDir + "/config") | |
| return | |
| } | |
| exchangeStartupConfigMap := configs.ReadExchangeQueryConfigFile(tempDir) | |
| require.Equal( | |
| t, | |
| &types.ExchangeQueryConfig{ | |
| ExchangeId: tc.expectedExchangeId, | |
| IntervalMs: tc.expectedIntervalMsExchange, | |
| TimeoutMs: tc.expectedTimeoutMsExchange, | |
| MaxQueries: tc.expectedMaxQueries, | |
| }, | |
| exchangeStartupConfigMap[tc.expectedExchangeId], | |
| ) | |
| os.RemoveAll(tempDir + "/config") | |
| }) | |
| } | |
| // In case tests fail and the path was never removed. | |
| os.RemoveAll(tempDir) | |
| } |
Comment on lines
+71
to
+114
| // ReadExchangeQueryConfigFile gets a mapping of `exchangeIds` to `ExchangeQueryConfigs` | ||
| // where `ExchangeQueryConfig` for querying exchanges for market prices comes from parsing a TOML | ||
| // file in the config directory. | ||
| // NOTE: if the config file is not found for the price-daemon, return the static exchange query | ||
| // config. | ||
| func ReadExchangeQueryConfigFile(homeDir string) map[types.ExchangeId]*types.ExchangeQueryConfig { | ||
| // Read file for exchange query configurations. | ||
| tomlFile, err := os.ReadFile(getConfigFilePath(homeDir)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Unmarshal `tomlFile` into `exchanges` for `exchangeStartupConfigMap`. | ||
| exchanges := map[string][]types.ExchangeQueryConfig{} | ||
| if err = toml.Unmarshal(tomlFile, &exchanges); err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| // Populate configs for exchanges. | ||
| exchangeStartupConfigMap := make(map[types.ExchangeId]*types.ExchangeQueryConfig, len(exchanges)) | ||
| for _, exchange := range exchanges["exchanges"] { | ||
| // Zero is an invalid configuration value for all parameters. This could also point to the | ||
| // configuration file being setup wrong with one or more exchange parameters unset. | ||
| if exchange.IntervalMs == 0 || | ||
| exchange.TimeoutMs == 0 || | ||
| exchange.MaxQueries == 0 { | ||
| panic( | ||
| fmt.Errorf( | ||
| "One or more query config values are unset or are set to zero for exchange with id: '%v'", | ||
| exchange.ExchangeId, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| // Insert Key-Value pair into `exchangeStartupConfigMap`. | ||
| exchangeStartupConfigMap[exchange.ExchangeId] = &types.ExchangeQueryConfig{ | ||
| ExchangeId: exchange.ExchangeId, | ||
| IntervalMs: exchange.IntervalMs, | ||
| TimeoutMs: exchange.TimeoutMs, | ||
| MaxQueries: exchange.MaxQueries, | ||
| } | ||
| } | ||
|
|
||
| return exchangeStartupConfigMap |
There was a problem hiding this comment.
The ReadExchangeQueryConfigFile function should not panic when encountering an error. Instead, it should return an error and allow the caller to handle it. This would improve error handling and make the code more robust and maintainable.
- func ReadExchangeQueryConfigFile(homeDir string) map[types.ExchangeId]*types.ExchangeQueryConfig {
+ func ReadExchangeQueryConfigFile(homeDir string) (map[types.ExchangeId]*types.ExchangeQueryConfig, error) {
...
- panic(err)
+ return nil, err
...
- panic(err)
+ return nil, err
...
- panic(
+ return nil, fmt.Errorf(
...
- )
+ )
...
- return exchangeStartupConfigMap
+ return exchangeStartupConfigMap, nil
}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.
Suggested change
| // ReadExchangeQueryConfigFile gets a mapping of `exchangeIds` to `ExchangeQueryConfigs` | |
| // where `ExchangeQueryConfig` for querying exchanges for market prices comes from parsing a TOML | |
| // file in the config directory. | |
| // NOTE: if the config file is not found for the price-daemon, return the static exchange query | |
| // config. | |
| func ReadExchangeQueryConfigFile(homeDir string) map[types.ExchangeId]*types.ExchangeQueryConfig { | |
| // Read file for exchange query configurations. | |
| tomlFile, err := os.ReadFile(getConfigFilePath(homeDir)) | |
| if err != nil { | |
| panic(err) | |
| } | |
| // Unmarshal `tomlFile` into `exchanges` for `exchangeStartupConfigMap`. | |
| exchanges := map[string][]types.ExchangeQueryConfig{} | |
| if err = toml.Unmarshal(tomlFile, &exchanges); err != nil { | |
| panic(err) | |
| } | |
| // Populate configs for exchanges. | |
| exchangeStartupConfigMap := make(map[types.ExchangeId]*types.ExchangeQueryConfig, len(exchanges)) | |
| for _, exchange := range exchanges["exchanges"] { | |
| // Zero is an invalid configuration value for all parameters. This could also point to the | |
| // configuration file being setup wrong with one or more exchange parameters unset. | |
| if exchange.IntervalMs == 0 || | |
| exchange.TimeoutMs == 0 || | |
| exchange.MaxQueries == 0 { | |
| panic( | |
| fmt.Errorf( | |
| "One or more query config values are unset or are set to zero for exchange with id: '%v'", | |
| exchange.ExchangeId, | |
| ), | |
| ) | |
| } | |
| // Insert Key-Value pair into `exchangeStartupConfigMap`. | |
| exchangeStartupConfigMap[exchange.ExchangeId] = &types.ExchangeQueryConfig{ | |
| ExchangeId: exchange.ExchangeId, | |
| IntervalMs: exchange.IntervalMs, | |
| TimeoutMs: exchange.TimeoutMs, | |
| MaxQueries: exchange.MaxQueries, | |
| } | |
| } | |
| return exchangeStartupConfigMap | |
| // ReadExchangeQueryConfigFile gets a mapping of `exchangeIds` to `ExchangeQueryConfigs` | |
| // where `ExchangeQueryConfig` for querying exchanges for market prices comes from parsing a TOML | |
| // file in the config directory. | |
| // NOTE: if the config file is not found for the price-daemon, return the static exchange query | |
| // config. | |
| func ReadExchangeQueryConfigFile(homeDir string) (map[types.ExchangeId]*types.ExchangeQueryConfig, error) { | |
| // Read file for exchange query configurations. | |
| tomlFile, err := os.ReadFile(getConfigFilePath(homeDir)) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Unmarshal `tomlFile` into `exchanges` for `exchangeStartupConfigMap`. | |
| exchanges := map[string][]types.ExchangeQueryConfig{} | |
| if err = toml.Unmarshal(tomlFile, &exchanges); err != nil { | |
| return nil, err | |
| } | |
| // Populate configs for exchanges. | |
| exchangeStartupConfigMap := make(map[types.ExchangeId]*types.ExchangeQueryConfig, len(exchanges)) | |
| for _, exchange := range exchanges["exchanges"] { | |
| // Zero is an invalid configuration value for all parameters. This could also point to the | |
| // configuration file being setup wrong with one or more exchange parameters unset. | |
| if exchange.IntervalMs == 0 || | |
| exchange.TimeoutMs == 0 || | |
| exchange.MaxQueries == 0 { | |
| return nil, fmt.Errorf( | |
| "One or more query config values are unset or are set to zero for exchange with id: '%v'", | |
| exchange.ExchangeId, | |
| ) | |
| } | |
| // Insert Key-Value pair into `exchangeStartupConfigMap`. | |
| exchangeStartupConfigMap[exchange.ExchangeId] = &types.ExchangeQueryConfig{ | |
| ExchangeId: exchange.ExchangeId, | |
| IntervalMs: exchange.IntervalMs, | |
| TimeoutMs: exchange.TimeoutMs, | |
| MaxQueries: exchange.MaxQueries, | |
| } | |
| } | |
| return exchangeStartupConfigMap, nil | |
| } |
clemire
commented
Dec 8, 2023
There was a problem hiding this comment.
The following two files were previously removed, and were added back in wholesale with the following caveat: the old ExchangeStartupConfig type was renamed to ExchangeQueryConfig, so I replaced Startup with Query everywhere in this file and the corresponding test file.
tqin7
approved these changes
Dec 8, 2023
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/testing/containertest/config/pricefeed_exchange_config.toml
Files selected for processing (1)
- protocol/x/prices/client/cli/prices_cli_test.go (2 hunks)
Additional comments: 1
protocol/x/prices/client/cli/prices_cli_test.go (1)
- 8-9: The addition of imports for
configsandfilepathaligns with the PR's objective to handle pricefeed configuration files.
Comment on lines
+100
to
+101
| homeDir := filepath.Join(testval.Dir, "simd") | ||
| configs.WriteDefaultPricefeedExchangeToml(homeDir) // must manually create config file. |
There was a problem hiding this comment.
The call to configs.WriteDefaultPricefeedExchangeToml(homeDir) is added to set up the default TOML configuration. However, it appears that the return value, which likely includes an error, is not being checked. It is important to handle potential errors when writing files to ensure that the test setup does not proceed with invalid state.
+ homeDir := filepath.Join(testval.Dir, "simd")
+ err := configs.WriteDefaultPricefeedExchangeToml(homeDir) // must manually create config file.
+ s.Require().NoError(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.
Suggested change
| homeDir := filepath.Join(testval.Dir, "simd") | |
| configs.WriteDefaultPricefeedExchangeToml(homeDir) // must manually create config file. | |
| homeDir := filepath.Join(testval.Dir, "simd") | |
| err := configs.WriteDefaultPricefeedExchangeToml(homeDir) // must manually create config file. | |
| s.Require().NoError(err) |
|
|
||
| var ( | ||
| binanceId = exchange_common.EXCHANGE_ID_BINANCE | ||
| filePath = fmt.Sprintf("config/%v", constants.PricefeedExchangeConfigFileName) |
There was a problem hiding this comment.
The filePath variable is still hardcoded, which could be refactored to use constants or a setup function for better maintainability. This is a follow-up on a previous comment that remains valid.
- filePath = fmt.Sprintf("config/%v", constants.PricefeedExchangeConfigFileName)
+ const configDir = "config"
+ filePath = fmt.Sprintf("%v/%v", configDir, constants.PricefeedExchangeConfigFileName)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.
Suggested change
| filePath = fmt.Sprintf("config/%v", constants.PricefeedExchangeConfigFileName) | |
| const configDir = "config" | |
| filePath = fmt.Sprintf("%v/%v", configDir, constants.PricefeedExchangeConfigFileName) |
| require.NoError(t, err) | ||
|
|
||
| require.Equal(t, tomlString, string(buffer[:])) | ||
| os.RemoveAll("config") |
There was a problem hiding this comment.
The cleanup process is repeated across tests and could be refactored into a teardown function to adhere to the DRY principle. This is a follow-up on a previous comment that remains valid.
+ func teardown() {
+ os.RemoveAll(configDir)
+ }
- os.RemoveAll("config")
+ teardown()Also applies to: 151-151, 242-242, 247-247
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.
Suggested change
| os.RemoveAll("config") | |
| func teardown() { | |
| os.RemoveAll(configDir) | |
| } | |
| teardown() |
| } | ||
|
|
||
| func TestReadExchangeStartupConfigFile(t *testing.T) { | ||
| pwd, _ := os.Getwd() |
There was a problem hiding this comment.
The test TestReadExchangeStartupConfigFile uses the current working directory to locate the configuration file, which may not be reliable in all environments. Consider using a more robust method to determine the project root or configuration path. Additionally, for better test isolation and to avoid side effects on the actual filesystem, consider using a mock filesystem or ioutil.TempDir to create temporary directories for testing. This is a follow-up on a previous comment that remains valid.
- pwd, _ := os.Getwd()
+ tempDir, err := ioutil.TempDir("", "test")
+ require.NoError(t, err)
+ defer os.RemoveAll(tempDir) // cleanup after testCommittable 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.
Suggested change
| pwd, _ := os.Getwd() | |
| tempDir, err := ioutil.TempDir("", "test") | |
| require.NoError(t, err) | |
| defer os.RemoveAll(tempDir) // cleanup after test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
Since the project work for adding a daemon config file will not be ready by the next release, add the pricefeed configuration toml back into the protocol for the next release so that the pricefeed daemon does not lose configurability.
This PR is a manually constructed reversion of this PR.
Test Plan
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.