-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.HaltHeight
andc.cfg.AppTomlConfig.HaltTime
is incomplete. The following references toHaltHeight
andHaltTime
still 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 = 100
server/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.HaltHeight
andc.cfg.AppTomlConfig.HaltTime
improves clarity and organization. Ensure that all references toHaltHeight
andHaltTime
in 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
Name
andInterfaceRegistry
methods enhances the interface's expressiveness.server/v2/cometbft/options.go (2)
21-22
: LGTM!The addition of
AddrPeerFilter
andIdPeerFilter
fields enhances the configurability of the server options.
35-36
: LGTM!The default initialization of
AddrPeerFilter
andIdPeerFilter
tonil
is 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
AddQueryFlagsToCmd
correctly 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
AppTomlConfig
andcmtcfg.Config
in theConfig
type enhances modularity and separation of concerns.
17-28
: LGTM! Function implementation is correct.The
DefaultAppTomlConfig
function correctly returns a default configuration forAppTomlConfig
.
30-39
: LGTM! Type definition is correct.The
AppTomlConfig
type correctly encapsulates various application-specific configuration parameters.
41-43
: LGTM! Type definition is correct.The
CfgOption
type correctly allows overwriting the default server configuration.
44-56
: LGTM! Function implementations are correct.The
OverwriteDefaultConfigTomlConfig
andOverwriteDefaultAppTomlConfig
functions correctly allow overwriting the default configurations.
69-75
: LGTM! Function implementation is correct.The
GetAppTomlFromViper
function correctly retrieves the application configuration from a Viper instance.runtime/v2/app.go (1)
54-57
: LGTM! Method implementation is correct.The
Name
method correctly retrieves and returns the application name from theconfig
field of theApp
instance.server/v2/cometbft/query.go (2)
23-30
: LGTM! The switch statement improves readability and maintainability.The refactoring from nested
if
statements to aswitch
statement enhances the clarity and reduces the indentation level.
Line range hint
65-79
:
LGTM! The changes simplify state management.The updates to reference
c.indexedEvents
andc.version
instead ofc.cfg.IndexEvents
andc.cfg.Version
improve the design by directly accessing instance variables.server/v2/server.go (1)
212-213
: LGTM! The change to useWriteCustomConfigAt
improves configurability.The modification from
WriteDefaultConfigAt
toWriteCustomConfigAt
allows for more flexible configuration management.However, ensure that all components implementing this interface are updated accordingly.
Verification successful
LGTM! The change to use
WriteCustomConfigAt
improves configurability.The modification from
WriteDefaultConfigAt
toWriteCustomConfigAt
allows 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
cmd
and 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 parameterT
fromgenesisCommand
.Ensure that this change does not affect the functionality of the
genesisCommand
and that it aligns with the new command construction requirements.
77-77
: Verify the impact of removing the type parameterT
fromcometbft.New
.Ensure that this change does not affect the instantiation of the
cometbft
server and that it aligns with the new server instantiation requirements.
184-185
: Reorganization oftemporaryTxDecoder
type.The reintroduction of the
temporaryTxDecoder
type appears to be a reorganization rather than a functional change.
186-222
: Reorganization oftemporaryTxDecoder
methods.The methods
Decode
andDecodeJSON
have 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 inCometBFTServer
struct.The fields
options
andcmtConfigOptions
have been renamed toserverOptions
andcfgOptions
, respectively. This change enhances clarity regarding the purpose of these fields.
49-53
: Update toNew
function parameters.The parameters of the
New
function have been updated to reflect the renaming of fields in theCometBFTServer
struct. This change aligns the function parameters with the new field names.
58-99
: Update toInit
method.The
Init
method has been updated to use the newAppTomlConfig
structure and to initialize theConsensus
object with the newserverOptions
field. This change improves the modularity and clarity of the configuration retrieval process and the consensus logic.
109-113
: Update toStart
method.The
Start
method has been updated to access theAppTomlConfig
directly for standalone operation checks. This change enhances readability and maintainability.
224-237
: Introduction ofConfig
method.The
Config
method 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 ofWriteCustomConfigAt
method.The
WriteCustomConfigAt
method 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/auth
to an indirect dependency.Ensure that this change does not affect the overall functionality and that the
auth
package 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.Standalone
improves clarity and organization. Ensure that all references toStandalone
in the codebase are updated accordingly.Verification successful
All references to
Standalone
are consistent with the new configuration structure.The updates to use
s.config.AppTomlConfig.Standalone
are correctly applied in all relevant contexts.
server/v2/cometbft/commands.go
server/v2/cometbft/config.go
server/v2/cometbft/server.go
The references in
flags.go
andstart.go
are 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
, andFlagType
are defined inserver/v2/cometbft/flags.go
and used appropriately inserver/v2/cometbft/commands.go
.
- Definitions found in:
server/v2/cometbft/flags.go
- Usage verified in:
server/v2/cometbft/commands.go
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 `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.MinRetainBlocks
improves clarity and organization. Ensure that all references toMinRetainBlocks
in the codebase are updated accordingly.Also applies to: 371-371
Verification successful
Verification Successful: All references to
MinRetainBlocks
have been updated to the new structure.The transition to using
c.cfg.AppTomlConfig.MinRetainBlocks
has been completed successfully. No instances of the old structurec.cfg.MinRetainBlocks
were found.
- Verified references in:
server/start.go
server/util.go
server/v2/cometbft/utils.go
server/v2/cometbft/config.go
server/config/config.go
baseapp/baseapp.go
baseapp/options.go
baseapp/abci_test.go
Scripts 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
, andTypeHash
are properly defined in the codebase, ensuring standardized flag usage and improved modularity.
FlagType
is defined inserver/v2/cometbft/flags.go
and used in multiple places.TypeHeight
is defined inserver/v2/cometbft/flags.go
and used in multiple places.TypeHash
is defined inserver/v2/cometbft/flags.go
and 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.initialHeight
instead ofc.cfg.InitialHeight
simplifies 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.MinRetainBlocks
improves clarity and organization. Ensure that all references toMinRetainBlocks
in 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.HaltHeight
andc.cfg.AppTomlConfig.HaltTime
improves clarity and organization. Ensure that all references toHaltHeight
andHaltTime
in 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
flagMinGasPrices
to replacecometbft.FlagMinGasPrices
improves clarity and consistency within the code.
74-74
: LGTM! Ensures unified references to the minimum gas prices flag.The update to use
flagMinGasPrices
instead ofcometbft.FlagMinGasPrices
ensures 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.OverwriteDefaultConfigTomlConfig
instead ofcometbft.OverwriteDefaultCometConfig
indicates 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
OverwriteDefaultConfigTomlConfig
method is implemented to overwrite theConfigTomlConfig
field, and it is used correctly in theinitTestnetFiles
method 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
, andinitialHeight
improves the configurability and clarity of theConsensus
struct.
108-113
: LGTM! Renaming and error handling improve clarity and robustness.The renaming of
RegisterExtensions
toRegisterSnapshotExtensions
and the addition of error handling enhance the clarity and robustness of the function.
133-133
: LGTM! Usage ofindexedEvents
is correct.The
indexedEvents
field is correctly used in theCheckTx
function to filter events.
164-164
: LGTM! Usage ofappName
is correct.The
appName
field is correctly used in theInfo
function to provide the application name.
229-233
: LGTM! Logic for settinginitialHeight
is correct.The logic for setting
initialHeight
to 1 if it is provided as 0 is correctly implemented.
421-421
: LGTM! Usage ofinitialHeight
is correct.The
initialHeight
field is correctly used to handle the genesis block in theFinalizeBlock
function.
505-505
: No changes to review in theCommit
function.There are no changes in the
Commit
function to review.
505-505
: No changes to review in theVerifyVoteExtension
function.There are no changes in the
VerifyVoteExtension
function to review.
505-505
: No changes to review in theExtendVote
function.There are no changes in the
ExtendVote
function 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
NewConsensus
match 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
NewConsensus
match 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
server/v2/cometbft/abci.go (1)
46-50
: Consider initializinginitialHeight
with a meaningful default value.Setting
initialHeight
to 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
initialHeight
to 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
appName
field. Verify thatappName
is correctly initialized and used.Verification successful
Verified proper handling of
appName
.The
appName
field is correctly initialized and used within theabci.go
file, and its usage in other parts of the codebase appears consistent.
server/v2/cometbft/abci.go
server/v2/cometbft/client/grpc/cmtservice/query.pb.go
Scripts 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
, andindexedEvents
have 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
, andindexedEvents
are correctly initialized in the constructor and used throughout the codebase.
appName
is used in various files such asserver/v2/cometbft/abci.go
,testutil/integration/router.go
, and others.consensusAuthority
is used inserver/v2/cometbft/abci.go
.indexedEvents
is used inserver/v2/cometbft/abci.go
andserver/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
indexedEvents
field. Verify thatindexedEvents
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved via c28574f but we might still POC koanf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
viper
instance 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofviper
for configuration management.The
ReadConfig
function usesviper
to read and merge configuration files and watch for changes. Ensure thatviper
is 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
mapstructure
with custom decoding options to decode sub-configurations from theviper
instance. 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.UnmarshalSubConfig
for configuration unmarshalling.The
Init
method now usesserverv2.UnmarshalSubConfig
for 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.UnmarshalSubConfig
in the test.The
TestReadConfig
function now usesserverv2.UnmarshalSubConfig
for 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.UnmarshalSubConfig
function is correctly implemented and handles all necessary edge cases.Verification successful
Verified: The implementation of
serverv2.UnmarshalSubConfig
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
enable
setting 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
TestReadConfig
function tests reading the configuration from a specified path and verifies the gRPC address. Ensure that the test data in thetestdata
directory is up-to-date and accurately reflects the expected configuration.
24-37
: Good test coverage for unmarshalling sub-configuration.The
TestUnmarshalSubConfig
function tests unmarshalling a sub-configuration for gRPC settings and verifies theenable
setting. This ensures that the configuration changes are correctly reflected in the application.server/v2/server_test.go (8)
42-42
: Good use ofrequire.NoError
for error handling.The use of
require.NoError
improves the readability and maintainability of the test code by simplifying error handling.
52-53
: Good use ofrequire.NoError
for gRPC server initialization.The use of
require.NoError
ensures that any errors during the gRPC server initialization are properly handled.
64-65
: Good use ofrequire.Equal
for configuration assertions.The use of
require.Equal
improves the readability and maintainability of the test code by simplifying configuration assertions.
68-69
: Good use ofrequire.NoError
for writing configuration.The use of
require.NoError
ensures that any errors during the configuration writing process are properly handled.
72-72
: Good use ofrequire.NoError
for reading configuration.The use of
require.NoError
ensures that any errors during the configuration reading process are properly handled.
74-74
: Good use ofrequire.Equal
for configuration assertions.The use of
require.Equal
improves the readability and maintainability of the test code by simplifying configuration assertions.
83-84
: Good use ofrequire.NoError
for stopping the server.The use of
require.NoError
ensures that any errors during the server stopping process are properly handled.
87-87
: Good use ofrequire.NoError
for starting the server.The use of
require.NoError
ensures that any errors during the server starting process are properly handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
ReadConfig
function 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
ReadConfig
function is well-structured and includes necessary error handling.
- The configuration path in
server/v2/commands.go
is 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.0
is 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.0
is approved.The usage of the
mapstructure
package 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
Reviewers 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