feat(server/v2/cometbft): config #20989
Conversation
WalkthroughWalkthroughThe recent changes enhance the modularity, clarity, and configurability of the CometBFT server and related components. Key updates include the introduction of new configuration structures, reorganization of command handling, and improvements in the consensus logic. These enhancements aim to streamline interactions within the codebase, improve error handling, and facilitate better management of application-specific settings, ultimately leading to a more maintainable and flexible architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommandParser
participant CometBFTServer
participant Consensus
User->>CommandParser: Start command
CommandParser->>CometBFTServer: Initialize with options
CometBFTServer->>Consensus: Create consensus instance
Consensus->>CometBFTServer: Register extensions and handle queries
CometBFTServer->>User: Command executed successfully
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range, codebase verification and nitpick comments (1)
server/v2/cometbft/commands.go (1)
Incomplete transition to new configuration structure
The transition to using
c.cfg.AppTomlConfig.HaltHeightandc.cfg.AppTomlConfig.HaltTimeis incomplete. The following references toHaltHeightandHaltTimestill use the old structure and should be updated:
server/util.go:baseapp.SetHaltHeight(cast.ToUint64(appOpts.Get(FlagHaltHeight))),server/v2/cometbft/config.go:HaltHeight: 0,server/config/config.go:HaltHeight uint64 \mapstructure:"halt-height"``client/grpc/node/service.go:HaltHeight: s.cfg.HaltHeight,client/grpc/node/service_test.go:defaultCfg.HaltHeight = 100server/util.go:baseapp.SetHaltTime(cast.ToUint64(appOpts.Get(FlagHaltTime))),server/config/config.go:HaltTime uint64 \mapstructure:"halt-time"``Please ensure all references are updated to the new configuration structure.
Analysis chain
Line range hint
386-386:
LGTM! But verify the usage of the new configuration structure.The change to use
c.cfg.AppTomlConfig.HaltHeightandc.cfg.AppTomlConfig.HaltTimeimproves clarity and organization. Ensure that all references toHaltHeightandHaltTimein the codebase are updated accordingly.Also applies to: 389-389, 394-394
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `HaltHeight` and `HaltTime` in the codebase. # Test: Search for the usage of `HaltHeight` and `HaltTime`. Expect: Only occurances of the new structure. rg --type go 'HaltHeight' && rg --type go 'HaltTime'Length of output: 5838
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (17)
- runtime/v2/app.go (1 hunks)
- runtime/v2/builder.go (1 hunks)
- server/v2/cometbft/abci.go (12 hunks)
- server/v2/cometbft/commands.go (7 hunks)
- server/v2/cometbft/config.go (2 hunks)
- server/v2/cometbft/flags.go (3 hunks)
- server/v2/cometbft/go.mod (2 hunks)
- server/v2/cometbft/options.go (3 hunks)
- server/v2/cometbft/query.go (3 hunks)
- server/v2/cometbft/server.go (4 hunks)
- server/v2/cometbft/utils.go (4 hunks)
- server/v2/cometbft/version.go (1 hunks)
- server/v2/commands.go (2 hunks)
- server/v2/server.go (1 hunks)
- server/v2/types.go (1 hunks)
- simapp/v2/simdv2/cmd/commands.go (3 hunks)
- simapp/v2/simdv2/cmd/testnet.go (4 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/builder.go
Additional context used
Path-based instructions (15)
server/v2/cometbft/version.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/options.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/flags.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/commands.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/utils.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/v2/simdv2/cmd/testnet.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (48)
server/v2/types.go (1)
16-17: LGTM!The addition of
NameandInterfaceRegistrymethods enhances the interface's expressiveness.server/v2/cometbft/options.go (2)
21-22: LGTM!The addition of
AddrPeerFilterandIdPeerFilterfields enhances the configurability of the server options.
35-36: LGTM!The default initialization of
AddrPeerFilterandIdPeerFiltertonilis appropriate.server/v2/cometbft/flags.go (3)
Line range hint
5-19: LGTM! Constants are well organized.The reorganization of query flags enhances readability and maintainability.
Line range hint
20-39: LGTM! Function implementation is correct.The function
AddQueryFlagsToCmdcorrectly adds query flags to a Cobra command.
41-49: LGTM! Constants are well organized.The reorganization of server flags enhances readability and maintainability.
server/v2/cometbft/config.go (6)
11-15: LGTM! Enhanced modularity.The inclusion of pointers to
AppTomlConfigandcmtcfg.Configin theConfigtype enhances modularity and separation of concerns.
17-28: LGTM! Function implementation is correct.The
DefaultAppTomlConfigfunction correctly returns a default configuration forAppTomlConfig.
30-39: LGTM! Type definition is correct.The
AppTomlConfigtype correctly encapsulates various application-specific configuration parameters.
41-43: LGTM! Type definition is correct.The
CfgOptiontype correctly allows overwriting the default server configuration.
44-56: LGTM! Function implementations are correct.The
OverwriteDefaultConfigTomlConfigandOverwriteDefaultAppTomlConfigfunctions correctly allow overwriting the default configurations.
69-75: LGTM! Function implementation is correct.The
GetAppTomlFromViperfunction correctly retrieves the application configuration from a Viper instance.runtime/v2/app.go (1)
54-57: LGTM! Method implementation is correct.The
Namemethod correctly retrieves and returns the application name from theconfigfield of theAppinstance.server/v2/cometbft/query.go (2)
23-30: LGTM! The switch statement improves readability and maintainability.The refactoring from nested
ifstatements to aswitchstatement enhances the clarity and reduces the indentation level.
Line range hint
65-79:
LGTM! The changes simplify state management.The updates to reference
c.indexedEventsandc.versioninstead ofc.cfg.IndexEventsandc.cfg.Versionimprove the design by directly accessing instance variables.server/v2/server.go (1)
212-213: LGTM! The change to useWriteCustomConfigAtimproves configurability.The modification from
WriteDefaultConfigAttoWriteCustomConfigAtallows for more flexible configuration management.However, ensure that all components implementing this interface are updated accordingly.
Verification successful
LGTM! The change to use
WriteCustomConfigAtimproves configurability.The modification from
WriteDefaultConfigAttoWriteCustomConfigAtallows for more flexible configuration management. No other functions related to writing configurations were found, indicating that the change has been fully implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `WriteCustomConfigAt` in the codebase. # Test: Search for the interface implementation. Expect: Implementations of `WriteCustomConfigAt`. rg --type go 'WriteCustomConfigAt\('Length of output: 326
Script:
#!/bin/bash # Description: Verify if there are any other components that need to implement `WriteCustomConfigAt`. # Test: Search for the previous usage of `WriteDefaultConfigAt` in the codebase. rg --type go 'WriteDefaultConfigAt\('Length of output: 39
Script:
#!/bin/bash # Description: Verify the previous existence of `WriteDefaultConfigAt` by searching for its implementation. # Test: Search for the implementation of `WriteDefaultConfigAt` in the codebase. rg --type go 'func WriteDefaultConfigAt\('Length of output: 44
Script:
#!/bin/bash # Description: Verify the existence of any function definitions related to writing configurations. # Test: Search for function definitions related to writing configurations in the codebase. rg --type go 'func .*ConfigAt\('Length of output: 138
server/v2/commands.go (1)
Line range hint
105-153:
LGTM! The changes enhance readability and configurability.The introduction of an intermediate variable
cmdand the addition of start flags to the command improve the command setup process.simapp/v2/simdv2/cmd/commands.go (4)
65-65: Verify the impact of removing the type parameterTfromgenesisCommand.Ensure that this change does not affect the functionality of the
genesisCommandand that it aligns with the new command construction requirements.
77-77: Verify the impact of removing the type parameterTfromcometbft.New.Ensure that this change does not affect the instantiation of the
cometbftserver and that it aligns with the new server instantiation requirements.
184-185: Reorganization oftemporaryTxDecodertype.The reintroduction of the
temporaryTxDecodertype appears to be a reorganization rather than a functional change.
186-222: Reorganization oftemporaryTxDecodermethods.The methods
DecodeandDecodeJSONhave been reintroduced without functional changes. Ensure that the methods are correctly implemented and adhere to best practices.server/v2/cometbft/server.go (6)
42-46: Renaming of fields inCometBFTServerstruct.The fields
optionsandcmtConfigOptionshave been renamed toserverOptionsandcfgOptions, respectively. This change enhances clarity regarding the purpose of these fields.
49-53: Update toNewfunction parameters.The parameters of the
Newfunction have been updated to reflect the renaming of fields in theCometBFTServerstruct. This change aligns the function parameters with the new field names.
58-99: Update toInitmethod.The
Initmethod has been updated to use the newAppTomlConfigstructure and to initialize theConsensusobject with the newserverOptionsfield. This change improves the modularity and clarity of the configuration retrieval process and the consensus logic.
109-113: Update toStartmethod.The
Startmethod has been updated to access theAppTomlConfigdirectly for standalone operation checks. This change enhances readability and maintainability.
224-237: Introduction ofConfigmethod.The
Configmethod has been introduced to retrieve the current application configuration. This change improves configuration management by providing a clear way to retrieve the current application configuration.
239-246: Introduction ofWriteCustomConfigAtmethod.The
WriteCustomConfigAtmethod has been introduced to handle the writing of the default CometBFT configuration to a specified path. This change enhances configuration management by providing a clear way to write the default configuration.server/v2/cometbft/go.mod (1)
58-58: Verify the impact of changingcosmossdk.io/x/authto an indirect dependency.Ensure that this change does not affect the overall functionality and that the
authpackage is still correctly utilized by other dependencies.server/v2/cometbft/commands.go (4)
31-31: LGTM! But verify the usage of the new configuration structure.The change to use
s.config.AppTomlConfig.Standaloneimproves clarity and organization. Ensure that all references toStandalonein the codebase are updated accordingly.Verification successful
All references to
Standaloneare consistent with the new configuration structure.The updates to use
s.config.AppTomlConfig.Standaloneare correctly applied in all relevant contexts.
server/v2/cometbft/commands.goserver/v2/cometbft/config.goserver/v2/cometbft/server.goThe references in
flags.goandstart.goare appropriately used and do not require changes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `Standalone` in the codebase. # Test: Search for the usage of `Standalone`. Expect: Only occurances of the new structure. rg --type go 'Standalone'Length of output: 871
Script:
#!/bin/bash # Description: Verify the context of each occurrence of `Standalone` to ensure consistency with the new structure. # Search for the usage of `Standalone` and include context lines for better understanding. rg --type go 'Standalone' -A 3 -B 3Length of output: 3945
203-203: LGTM! But verify the definition of the new constants.The change to use new constants for command-line flags standardizes the flag usage and improves modularity. Ensure that the new constants (
FlagQuery,FlagOrderBy,FlagType) are defined correctly.Also applies to: 206-206, 225-227
Verification successful
Verified: Constants are correctly defined and used.
The constants
FlagQuery,FlagOrderBy, andFlagTypeare defined inserver/v2/cometbft/flags.goand used appropriately inserver/v2/cometbft/commands.go.
- Definitions found in:
server/v2/cometbft/flags.go- Usage verified in:
server/v2/cometbft/commands.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of the new constants. # Test: Search for the definition of the new constants. Expect: Definitions of `FlagQuery`, `FlagOrderBy`, `FlagType`. rg --type go 'const FlagQuery' && rg --type go 'const FlagOrderBy' && rg --type go 'const FlagType'Length of output: 32
Script:
#!/bin/bash # Description: Verify the definition of the new constants. # Search for the definition of the constants `FlagQuery`, `FlagOrderBy`, and `FlagType` across the entire codebase. rg 'FlagQuery' && rg 'FlagOrderBy' && rg 'FlagType'Length of output: 6296
Line range hint
330-330:
LGTM! But verify the usage of the new configuration structure.The change to use
c.cfg.AppTomlConfig.MinRetainBlocksimproves clarity and organization. Ensure that all references toMinRetainBlocksin the codebase are updated accordingly.Also applies to: 371-371
Verification successful
Verification Successful: All references to
MinRetainBlockshave been updated to the new structure.The transition to using
c.cfg.AppTomlConfig.MinRetainBlockshas been completed successfully. No instances of the old structurec.cfg.MinRetainBlockswere found.
- Verified references in:
server/start.goserver/util.goserver/v2/cometbft/utils.goserver/v2/cometbft/config.goserver/config/config.gobaseapp/baseapp.gobaseapp/options.gobaseapp/abci_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to `MinRetainBlocks` in the codebase. # Test: Search for the usage of `MinRetainBlocks`. Expect: Only occurances of the new structure. rg --type go 'MinRetainBlocks'Length of output: 1710
Script:
#!/bin/bash # Description: Verify if the old structure `c.cfg.MinRetainBlocks` is still in use. # Test: Search for the usage of `c.cfg.MinRetainBlocks`. Expect: No occurrences of the old structure. rg --type go 'c\.cfg\.MinRetainBlocks'Length of output: 40
242-243: LGTM! But verify the definition of the new constants.The change to use new constants for command-line flags standardizes the flag usage and improves modularity. Ensure that the new constants (
FlagType,TypeHeight,TypeHash) are defined correctly.Also applies to: 246-246, 254-254, 284-284, 308-308, 314-314
Verification successful
LGTM! The new constants are correctly defined and used.
The constants
FlagType,TypeHeight, andTypeHashare properly defined in the codebase, ensuring standardized flag usage and improved modularity.
FlagTypeis defined inserver/v2/cometbft/flags.goand used in multiple places.TypeHeightis defined inserver/v2/cometbft/flags.goand used in multiple places.TypeHashis defined inserver/v2/cometbft/flags.goand used in multiple places.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of the new constants. # Test: Search for the definition of the new constants. Expect: Definitions of `FlagType`, `TypeHeight`, `TypeHash`. rg --type go 'const FlagType' && rg --type go 'const TypeHeight' && rg --type go 'const TypeHash'Length of output: 31
Script:
#!/bin/bash # Description: Verify the definition of the new constants. # Test: Search for the definition of the new constants. Expect: Definitions of `FlagType`, `TypeHeight`, `TypeHash`. rg --type go 'FlagType' && rg --type go 'TypeHeight' && rg --type go 'TypeHash'Length of output: 6134
server/v2/cometbft/utils.go (3)
265-268: LGTM! Simplifies the code by using the instance variable directly.The change to use
c.initialHeightinstead ofc.cfg.InitialHeightsimplifies the code and maintains the logic.
330-330: LGTM! But verify the usage of the new configuration structure.The change to use
c.cfg.AppTomlConfig.MinRetainBlocksimproves clarity and organization. Ensure that all references toMinRetainBlocksin the codebase are updated accordingly.Also applies to: 371-371
386-386: LGTM! But verify the usage of the new configuration structure.The change to use
c.cfg.AppTomlConfig.HaltHeightandc.cfg.AppTomlConfig.HaltTimeimproves clarity and organization. Ensure that all references toHaltHeightandHaltTimein the codebase are updated accordingly.Also applies to: 389-389, 394-394
simapp/v2/simdv2/cmd/testnet.go (3)
43-43: LGTM! Enhances clarity and consistency.The declaration of
flagMinGasPricesto replacecometbft.FlagMinGasPricesimproves clarity and consistency within the code.
74-74: LGTM! Ensures unified references to the minimum gas prices flag.The update to use
flagMinGasPricesinstead ofcometbft.FlagMinGasPricesensures that all references to this flag are unified under the new name.
131-131: LGTM! But verify the usage of the new configuration method.The update to use
cometbft.OverwriteDefaultConfigTomlConfiginstead ofcometbft.OverwriteDefaultCometConfigindicates a potential shift in the configuration management approach. Ensure that the new method is used correctly throughout the codebase.Also applies to: 340-344
Verification successful
Verified: Correct usage of the new configuration method.
The
OverwriteDefaultConfigTomlConfigmethod is implemented to overwrite theConfigTomlConfigfield, and it is used correctly in theinitTestnetFilesmethod to configure thecometServer.
server/v2/cometbft/config.go: Implementation ofOverwriteDefaultConfigTomlConfig.simapp/v2/simdv2/cmd/testnet.go: Correct usage ofOverwriteDefaultConfigTomlConfig.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new configuration method. # Test: Search for the usage of `OverwriteDefaultConfigTomlConfig`. Expect: Correct usage of the new method. rg --type go 'OverwriteDefaultConfigTomlConfig'Length of output: 361
Script:
#!/bin/bash # Extract the implementation of OverwriteDefaultConfigTomlConfig from config.go ast-grep --lang go --pattern 'func OverwriteDefaultConfigTomlConfig($_) $_ { $$$ }' --json # Extract the context around the usage of OverwriteDefaultConfigTomlConfig in testnet.go rg --type go -A 10 -B 10 'OverwriteDefaultConfigTomlConfig' simapp/v2/simdv2/cmd/testnet.goLength of output: 1714
server/v2/cometbft/abci.go (10)
35-50: LGTM! New fields enhance configurability and clarity.The addition of new fields like
appName,consensusAuthority,indexedEvents, andinitialHeightimproves the configurability and clarity of theConsensusstruct.
108-113: LGTM! Renaming and error handling improve clarity and robustness.The renaming of
RegisterExtensionstoRegisterSnapshotExtensionsand the addition of error handling enhance the clarity and robustness of the function.
133-133: LGTM! Usage ofindexedEventsis correct.The
indexedEventsfield is correctly used in theCheckTxfunction to filter events.
164-164: LGTM! Usage ofappNameis correct.The
appNamefield is correctly used in theInfofunction to provide the application name.
229-233: LGTM! Logic for settinginitialHeightis correct.The logic for setting
initialHeightto 1 if it is provided as 0 is correctly implemented.
421-421: LGTM! Usage ofinitialHeightis correct.The
initialHeightfield is correctly used to handle the genesis block in theFinalizeBlockfunction.
505-505: No changes to review in theCommitfunction.There are no changes in the
Commitfunction to review.
505-505: No changes to review in theVerifyVoteExtensionfunction.There are no changes in the
VerifyVoteExtensionfunction to review.
505-505: No changes to review in theExtendVotefunction.There are no changes in the
ExtendVotefunction to review.
Line range hint
66-97:
LGTM! But verify the function usage in the codebase.The changes in the constructor correctly initialize the new fields. Ensure that all function calls to
NewConsensusmatch the new signature.Verification successful
Verify the function usage in the codebase.
The changes in the constructor correctly initialize the new fields. Ensure that all function calls to
NewConsensusmatch the new signature.
server/v2/cometbft/server.go: NewConsensus(s.logger, appI.Name(), appI.GetConsensusAuthority(), appI.GetAppManager(), s.serverOptions.Mempool, ... )The function call matches the new signature.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewConsensus` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewConsensus'Length of output: 2074
|
@julienrbrt your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
server/v2/cometbft/abci.go (1)
46-50: Consider initializinginitialHeightwith a meaningful default value.Setting
initialHeightto 0 might lead to unexpected behavior if not handled correctly in the code.- initialHeight uint64 + initialHeight uint64 = 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- server/v2/cometbft/abci.go (13 hunks)
- server/v2/cometbft/config.go (2 hunks)
- server/v2/commands.go (2 hunks)
- simapp/v2/simdv2/cmd/commands.go (3 hunks)
- simapp/v2/simdv2/cmd/testnet.go (4 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/v2/cometbft/config.go
- server/v2/commands.go
- simapp/v2/simdv2/cmd/commands.go
- simapp/v2/simdv2/cmd/testnet.go
Additional context used
Path-based instructions (1)
server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
server/v2/cometbft/abci.go (6)
108-113: LGTM! Improved error handling.The function now returns an error instead of panicking, which improves robustness.
229-232: LGTM! Improved handling ofinitialHeight.The function now sets
initialHeightto 1 if it is provided as 0, which enhances the handling of chain initialization.
420-420: Ensure proper handling of initial block commit.The function now handles the initial block commit. Verify that the initial block commit is correctly handled.
Verification successful
Ensure proper handling of initial block commit.
The initial block commit is checked correctly with
if req.Height == int64(c.initialHeight). The code commits the changeset and updates the last committed height. This confirms that the initial block commit is being handled as expected.
- Line 420:
if req.Height == int64(c.initialHeight) {ensures that the initial block commit is processed correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper handling of initial block commit in the `FinalizeBlock` function. # Test: Search for the handling of initial block commit in the `FinalizeBlock` function. Expect: Proper handling. rg --type go -A 5 'initialHeight' server/v2/cometbft/abci.goLength of output: 1241
164-166: Ensure proper handling ofappName.The function now uses the
appNamefield. Verify thatappNameis correctly initialized and used.Verification successful
Verified proper handling of
appName.The
appNamefield is correctly initialized and used within theabci.gofile, and its usage in other parts of the codebase appears consistent.
server/v2/cometbft/abci.goserver/v2/cometbft/client/grpc/cmtservice/query.pb.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper handling of `appName` in the `Info` function. # Test: Search for the usage of `appName` in the `Info` function. Expect: Proper handling. rg --type go -A 5 'appName' server/v2/cometbft/abci.goLength of output: 1038
Script:
#!/bin/bash # Description: Verify proper handling of `appName` throughout the codebase. # Test: Search for all instances of `appName` usage. Expect: Proper handling. rg --type go 'appName' server/v2/cometbft/Length of output: 427
Line range hint
66-97:
Ensure proper initialization of new fields.The new fields
appName,consensusAuthority, andindexedEventshave been added to the constructor. Ensure that these fields are correctly initialized and used throughout the code.Verification successful
Initialization and usage of new fields are properly handled.
The fields
appName,consensusAuthority, andindexedEventsare correctly initialized in the constructor and used throughout the codebase.
appNameis used in various files such asserver/v2/cometbft/abci.go,testutil/integration/router.go, and others.consensusAuthorityis used inserver/v2/cometbft/abci.go.indexedEventsis used inserver/v2/cometbft/abci.goandserver/v2/cometbft/query.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper initialization and usage of new fields in the constructor. # Test: Search for the usage of new fields. Expect: Proper initialization and usage. rg --type go -A 5 'appName\|consensusAuthority\|indexedEvents'Length of output: 64
Script:
#!/bin/bash # Description: Verify proper initialization and usage of new fields in the constructor. # Test: Search for the usage of new fields. Expect: Proper initialization and usage. rg --type go 'appName' -A 10 rg --type go 'consensusAuthority' -A 10 rg --type go 'indexedEvents' -A 10Length of output: 41476
133-133: Ensure proper handling ofindexedEvents.The function now uses the
indexedEventsfield. Verify thatindexedEventsis correctly initialized and used.
| @@ -38,6 +38,9 @@ type HasConfig interface { | |||
|
|
|||
| // HasStartFlags is a server module that has start flags. | |||
| type HasStartFlags interface { | |||
| // StartCmdFlags returns server start flags. | |||
| // Those flags should be prefixed with the server name. | |||
| // They are then merged with the server config in one viper instance. | |||
There was a problem hiding this comment.
So we cannot use v.Sub as it will miss the flags (even if prefixed).
v.Sub seemed to most logical to use, so I am going to check if we can extend viper for supporting this.
However, viper has not had any external contributor PR merged in months: https://github.com/spf13/viper, while many PRs are solving valid bugs.
I might POC a switch to https://github.com/knadh/koanf for server/v2
There was a problem hiding this comment.
Solved via c28574f but we might still POC koanf
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- server/v2/cometbft/config.go (1 hunks)
- server/v2/cometbft/server.go (6 hunks)
- server/v2/commands.go (3 hunks)
- server/v2/server.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- server/v2/cometbft/config.go
- server/v2/cometbft/server.go
- server/v2/commands.go
- server/v2/server.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- server/v2/cometbft/config.go (1 hunks)
- server/v2/cometbft/server.go (6 hunks)
- server/v2/config.go (1 hunks)
- server/v2/server.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- server/v2/cometbft/config.go
- server/v2/cometbft/server.go
- server/v2/server.go
Additional context used
Path-based instructions (1)
server/v2/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (6)
server/v2/config.go (6)
1-7: LGTM!The package declaration and imports are appropriate and necessary for the functionality provided.
9-12: LGTM!The function signature and initialization of the
viperinstance are correct.
13-14: LGTM!The configuration name and path setting are appropriate.
15-17: LGTM!The error handling for reading the configuration file is correct and provides informative error messages.
19-21: LGTM!The merging of additional configuration and error handling are correct and follow best practices.
24-26: LGTM!The configuration watching and return statement are correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (5)
- server/v2/api/grpc/server.go (1 hunks)
- server/v2/api/grpcgateway/server.go (1 hunks)
- server/v2/cometbft/server.go (6 hunks)
- server/v2/config.go (1 hunks)
- server/v2/server_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/server.go
Additional context used
Path-based instructions (4)
server/v2/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/api/grpcgateway/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
server/v2/config.go (2)
11-28: Verify the use ofviperfor configuration management.The
ReadConfigfunction usesviperto read and merge configuration files and watch for changes. Ensure thatviperis the best choice for this use case and that it meets all requirements for configuration management.
31-63: Verify the decoding options and error handling inUnmarshalSubConfig.The function uses
mapstructurewith custom decoding options to decode sub-configurations from theviperinstance. Ensure that the decoding options are appropriate for the expected configuration data and that the error handling is robust.server/v2/api/grpc/server.go (1)
37-37: Verify the use ofserverv2.UnmarshalSubConfigfor configuration unmarshalling.The
Initmethod now usesserverv2.UnmarshalSubConfigfor configuration unmarshalling. Ensure that this method is used correctly and that it improves the configuration handling process.server/v2/server_test.go (1)
132-132: Verify the use ofserverv2.UnmarshalSubConfigin the test.The
TestReadConfigfunction now usesserverv2.UnmarshalSubConfigfor configuration unmarshalling. Ensure that this method is used correctly and that it improves the test coverage.server/v2/api/grpcgateway/server.go (1)
87-88: LGTM! Verify the implementation ofserverv2.UnmarshalSubConfig.The change centralizes the unmarshalling logic, which can improve maintainability and error handling.
However, ensure that the
serverv2.UnmarshalSubConfigfunction is correctly implemented and handles all necessary edge cases.Verification successful
Verified: The implementation of
serverv2.UnmarshalSubConfigis correct and comprehensive.The function iterates through settings, filters based on a prefix, and uses a custom decoder to handle the unmarshalling process. This ensures proper configuration handling and improves maintainability.
server/v2/config.go: Implementation ofUnmarshalSubConfig.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `serverv2.UnmarshalSubConfig`. # Test: Search for the function implementation. Expect: Correct and comprehensive implementation. ast-grep --lang go --pattern $'func UnmarshalSubConfig(v *viper.Viper, name string, cfg interface{}) error { $$$ }'Length of output: 118
Script:
#!/bin/bash # Description: Verify the implementation of `serverv2.UnmarshalSubConfig`. # Test: Search for the function implementation using a broader pattern. rg 'func UnmarshalSubConfig' -A 10 # Additionally, search for the function definition in the `serverv2` package. fd 'serverv2' --exec rg 'UnmarshalSubConfig' -A 10Length of output: 665
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- server/v2/config_test.go (1 hunks)
- server/v2/server_test.go (4 hunks)
- server/v2/testdata/app.toml (1 hunks)
Additional context used
Path-based instructions (2)
server/v2/config_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"server/v2/server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (11)
server/v2/testdata/app.toml (1)
3-3: Disabling gRPC server functionality.The
enablesetting for the gRPC server has been changed tofalse, effectively disabling the gRPC server. Ensure that this change is intentional and that it won't negatively impact any dependent components or services.server/v2/config_test.go (2)
13-22: Good test coverage for reading configuration.The
TestReadConfigfunction tests reading the configuration from a specified path and verifies the gRPC address. Ensure that the test data in thetestdatadirectory is up-to-date and accurately reflects the expected configuration.
24-37: Good test coverage for unmarshalling sub-configuration.The
TestUnmarshalSubConfigfunction tests unmarshalling a sub-configuration for gRPC settings and verifies theenablesetting. This ensures that the configuration changes are correctly reflected in the application.server/v2/server_test.go (8)
42-42: Good use ofrequire.NoErrorfor error handling.The use of
require.NoErrorimproves the readability and maintainability of the test code by simplifying error handling.
52-53: Good use ofrequire.NoErrorfor gRPC server initialization.The use of
require.NoErrorensures that any errors during the gRPC server initialization are properly handled.
64-65: Good use ofrequire.Equalfor configuration assertions.The use of
require.Equalimproves the readability and maintainability of the test code by simplifying configuration assertions.
68-69: Good use ofrequire.NoErrorfor writing configuration.The use of
require.NoErrorensures that any errors during the configuration writing process are properly handled.
72-72: Good use ofrequire.NoErrorfor reading configuration.The use of
require.NoErrorensures that any errors during the configuration reading process are properly handled.
74-74: Good use ofrequire.Equalfor configuration assertions.The use of
require.Equalimproves the readability and maintainability of the test code by simplifying configuration assertions.
83-84: Good use ofrequire.NoErrorfor stopping the server.The use of
require.NoErrorensures that any errors during the server stopping process are properly handled.
87-87: Good use ofrequire.NoErrorfor starting the server.The use of
require.NoErrorensures that any errors during the server starting process are properly handled.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for k, val := range v.AllSettings() { | ||
| if strings.HasPrefix(k, subName) { | ||
| sub = val | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- server/v2/cometbft/config.go (1 hunks)
- server/v2/config.go (1 hunks)
- server/v2/config_test.go (1 hunks)
- server/v2/go.mod (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/v2/cometbft/config.go
- server/v2/config_test.go
Additional context used
Path-based instructions (1)
server/v2/config.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (2)
server/v2/config.go (1)
11-28: LGTM! But verify the configuration paths.The
ReadConfigfunction is well-structured and includes necessary error handling.However, ensure that the configuration paths (
configPath) are correctly set and verified in the calling code.Verification successful
LGTM! But verify the configuration paths.
The
ReadConfigfunction is well-structured and includes necessary error handling.
- The configuration path in
server/v2/commands.gois correctly set usingfilepath.Join(home, "config").- In
tools/confix/upgrade.go, the configuration is read from a byte slice, so there's no direct file path to verify.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the configuration paths used in ReadConfig function. # Test: Search for the function usage. Expect: Correct configuration paths being passed. rg --type go -A 5 $'ReadConfig'Length of output: 4474
Script:
#!/bin/bash # Description: Verify the configuration paths used in non-test files for ReadConfig function. # Test: Search for the function usage in non-test files. Expect: Correct configuration paths being passed. rg --type go -g '!*_test.go' -A 5 $'ReadConfig'Length of output: 1012
Script:
#!/bin/bash # Description: Verify the definitions and usages of configuration paths in non-test files. # Test: Search for the definition and usage of configDir in server/v2/commands.go rg --type go -A 5 'configDir' server/v2/commands.go # Test: Search for the definition and usage of the relevant path in tools/confix/upgrade.go rg --type go -A 5 'ReadConfig' tools/confix/upgrade.goLength of output: 680
Script:
#!/bin/bash # Description: Verify the surrounding code in tools/confix/upgrade.go to understand the context of the path used in ReadConfig. # Test: Search for the surrounding code in tools/confix/upgrade.go rg --type go -A 10 -B 10 'ReadConfig' tools/confix/upgrade.goLength of output: 666
server/v2/go.mod (1)
31-31: LGTM! But verify the usage of the new dependency.The addition of a direct dependency on
github.com/mitchellh/mapstructure v1.5.0is approved.However, ensure that the dependency is necessary and correctly used in the codebase.
Verification successful
LGTM! But verify the usage of the new dependency.
The addition of a direct dependency on
github.com/mitchellh/mapstructure v1.5.0is approved.The usage of the
mapstructurepackage is widespread and consistent with its intended purpose, confirming the necessity and correctness of the dependency.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new dependency. # Test: Search for the usage of mapstructure package. Expect: Correct and necessary usage. rg --type go -A 5 $'mapstructure'Length of output: 30107
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- server/v2/cometbft/config.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/cometbft/config.go
* main: (48 commits) build(deps): add missing replaces and remove unnecessary ones (#21033) build(deps): Bump bufbuild/buf-setup-action from 1.34.0 to 1.35.0 (#21028) chore: fix some comments for struct field (#21027) chore(x): replace `fmt.Errorf` without parameters with `errors.New` (#21032) chore: fix errors reported by running `make lint` (#21015) ci: skip spelling check in go.mod/go.sum (#21021) chore(all)!: use gogoproto/any instead of codec/types/any (#21013) chore(server/v2/cometbft): ensure consistent dash-case in app.toml (#21018) docs(server): wrong function comments (#21017) refactor(storev2): update snapshot manager and migration manager tests (#20441) feat(server/v2/cometbft): config (#20989) refactor: set `help` as default target of Makefile (#21011) fix(simapp): duplicated import (#21014) chore(docs): fix functions and struct comments (#21010) fix(simapp/v2): panic with testnet init-files command (#21012) fix: make help won't work (#21005) fix: NewIntegrationApp does not write default genesis to state (#21006) chore(x/staking,x/upgrade): replace `fmt.Errorf` without parameters with `errors.New` (#21004) chore: prepare depinject 1.0.0 (#21001) docs: Fix typos in RFC Creation Process (#20998) ...
Description
Properly wires the cometbft config
CometBFT server now supports app.toml and config.toml as presently.
Fix as well a bunch of bugs (start flags missing, viper sub not working with flags, comet config was full for not "config" things)
Closes: #20097
ref: #19674
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style