Skip to content
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

refactor(core,server): make commands not rely on server context #20422

Merged
merged 39 commits into from
May 28, 2024

Conversation

tac0turtle
Copy link
Member

@tac0turtle tac0turtle commented May 17, 2024

Description

Ref: #20288 (comment)


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...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

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.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • Refactor

    • Updated context handling to use Viper and Logger for better configuration management.
    • Deprecated ServerContext in favor of Viper-based configuration.
  • Bug Fixes

    • Improved context key usage to avoid conflicts and enhance logging.
  • Chores

    • Reorganized imports and removed deprecated dependencies.
    • Updated test setups to align with new context handling methods.

Copy link
Contributor

coderabbitai bot commented May 17, 2024

Walkthrough

Walkthrough

The changes primarily involve replacing the usage of the server.Context with viper.Viper and log.Logger for configuration and logging management. This refactoring affects various files, including test utilities, network setup, and CLI commands, ensuring a consistent context handling approach using Viper and Logger.

Changes

File Path Change Summary
server/util_test.go Updated context creation to use viper.Viper and log.Logger; replaced serverCtx with config.
testutil/network/interface.go Modified ValidatorI interface to use GetViper() and GetLogger() instead of GetCtx().
testutil/network/network.go Replaced server.NewDefaultContext() with viper.New() and client.GetConfigFromViper(viper).
testutil/network/util.go Updated startInProcess and collectGenFiles to use GetLogger() and GetConfigFromViper().
testutil/network/validator.go Changed Validator struct to include viper *viper.Viper and logger log.Logger.
x/genutil/client/cli/export_test.go Replaced server.Context with Viper *viper.Viper and Logger log.Logger in ExportSystem struct.
x/genutil/client/cli/genaccount_test.go Replaced server.NewContext with writeAndTrackDefaultConfig.
x/genutil/client/cli/init_test.go Introduced writeAndTrackDefaultConfig and updated context keys to use corectx.ViperContextKey.
x/genutil/client/testutil/helpers.go Added functions for writing and tracking configurations using Viper.
x/genutil/collect_test.go Commented out server.NewDefaultContext() usage.
CHANGELOG.md Documented deprecation of ServerContext and changes in related modules and functions.
core/context/server_context.go Introduced LoggerContextKey and ViperContextKey for context management.
simapp/simd/cmd/testnet_test.go Updated context initialization and configuration handling in Test_TestnetCmd.
tests/e2e/genutil/export_test.go Modified context handling and configuration setup using Viper in test functions.
server/util.go Deprecated Context struct and adjusted functions to work with viper.Viper and corectx.
simapp/test_helpers.go Updated logger and configuration access in NewTestNetworkFixture.

Sequence Diagram(s) (Beta)

sequenceDiagram
    participant User
    participant CLI
    participant Viper
    participant Logger
    participant Config

    User ->> CLI: Execute Command
    CLI ->> Viper: Initialize Viper
    CLI ->> Logger: Initialize Logger
    CLI ->> Config: Get Configuration
    Config -->> CLI: Return Configuration
    CLI ->> User: Display Output

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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to full the review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added C:CLI C:x/genutil genutil module issues labels May 17, 2024
x/genutil/client/testutil/helpers.go Fixed Show fixed Hide fixed
testutil/network/network.go Fixed Show fixed Hide fixed
@tac0turtle tac0turtle marked this pull request as ready for review May 20, 2024 09:12
@tac0turtle tac0turtle requested a review from a team as a code owner May 20, 2024 09:12
testutil/network/util.go Fixed Show fixed Hide fixed
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Outside diff range and nitpick comments (1)
server/util.go (1)

Line range hint 231-316: Review error handling and configuration management in interceptConfigs.

The function interceptConfigs handles various configuration-related tasks but lacks comprehensive error handling and logging. Consider adding more detailed error messages and logging to help diagnose issues.

- return fmt.Errorf("failed to read in %s: %w", cmtCfgFile, err)
+ log.Error("failed to read configuration file", "file", cmtCfgFile, "error", err)
+ return fmt.Errorf("failed to read in configuration file %s: %w", cmtCfgFile, err)
Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 6ab5b56 and 7c4e602.
Files selected for processing (4)
  • client/cmd.go (2 hunks)
  • core/context/server_context.go (1 hunks)
  • server/start.go (22 hunks)
  • server/util.go (10 hunks)
Files skipped from review as they are similar to previous changes (1)
  • core/context/server_context.go
Additional Context Used
Path-based Instructions (3)
client/cmd.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

server/util.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

server/start.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (20)
client/cmd.go (1)

408-416: Optimize error handling in GetConfigFromViper.

The function GetConfigFromViper handles errors by returning a default configuration with the root directory set. However, it might be more appropriate to also log the error or throw it back to the caller to handle it, as swallowing the error might hide underlying issues.

- return cmtcfg.DefaultConfig().SetRoot(rootDir)
+ return nil, fmt.Errorf("failed to unmarshal configuration: %w", err)
server/util.go (1)

Line range hint 211-221: Use custom types for context keys to avoid potential collisions.

Using built-in types like string as keys for context values can lead to collisions. Define custom types for LoggerContextKey and ViperContextKey to avoid this issue.

- cmd.SetContext(context.WithValue(cmdCtx, corectx.LoggerContextKey, logger))
- cmd.SetContext(context.WithValue(cmdCtx, corectx.ViperContextKey, viper))
+ type loggerKey struct{}
+ type viperKey struct{}
+ cmd.SetContext(context.WithValue(cmdCtx, loggerKey{}, logger))
+ cmd.SetContext(context.WithValue(cmdCtx, viperKey{}, viper))
server/start.go (18)

35-35: Added import for viper, log, and pruningtypes from cosmossdk.io.

This change aligns with the PR's objective to refactor the configuration management by using viper and enhancing logging with the log package.


117-121: Refactored Context struct to use viper.Viper and log.Logger instead of server.Context.

This change is consistent with the PR's goal to remove server.Context and use more modular components like viper for configuration and log for logging.


130-130: Updated PostSetup, PostSetupStandalone, and StartCommandHandler function signatures in StartCmdOptions to use viper and logger.

These changes are necessary to pass the new viper and logger instances through the application's setup and runtime, replacing the old context system.

Also applies to: 132-132, 136-136


187-189: Introduced local variables viper and logger by extracting them from the command context.

This is a direct application of the new configuration and logging approach, ensuring that each command handler can access these settings directly.


205-206: Wrapped the StartCommandHandler call within wrapCPUProfile to manage CPU profiling based on the viper configuration.

This encapsulation ensures that CPU profiling is handled correctly and only when specified, improving the performance and configurability of the application.


209-214: Added structured logging for the shutdown process, providing clear start and completion messages.

Enhanced logging provides better visibility into the application's state during shutdown, which is crucial for debugging and monitoring.


Line range hint 225-247: Refactored start, startStandAlone, and startInProcess functions to use the new viper and logger.

These changes are essential for integrating the new configuration and logging mechanisms throughout the application's startup processes.


Line range hint 250-291: Updated startStandAlone and startAPIServer functions to utilize viper for configuration and logger for logging.

The use of viper and logger in these functions aligns with the overall architectural shift towards better modularity and separation of concerns.


297-304: Enhanced error handling and logging in the standalone post-setup process.

This improvement ensures that errors during the post-setup phase are logged appropriately, aiding in troubleshooting and maintaining system stability.


Line range hint 318-364: Refactored startInProcess to integrate viper and logger, and handle gRPC-only mode based on configuration.

This refactoring supports conditional startup modes (like gRPC-only) and integrates the new configuration system, enhancing flexibility and maintainability.


Line range hint 379-396: Updated startCmtNode to use viper and logger, aligning with the new configuration and logging strategy.

This update is crucial for ensuring that node startup is managed with the latest configuration and logging practices, providing better control and observability.


415-416: Refactored getAndValidateConfig to use viper, streamlining configuration management.

This change simplifies configuration handling by centralizing it in viper, making the code cleaner and more maintainable.


Line range hint 492-529: Enhanced gRPC server startup to use viper and logger, improving configuration and logging during server initialization.

This enhancement ensures that the gRPC server is started with the correct settings and that its activity is logged appropriately, which is vital for operational monitoring and debugging.


Line range hint 549-561: Updated startAPIServer to use viper for configuration and enhanced logging for API server operations.

These updates are consistent with the PR's objectives to improve configuration management and logging, ensuring that the API server operates with the latest settings and provides detailed operational logs.


583-601: Refined CPU profiling setup and teardown processes to be managed via viper and include detailed logging.

This refinement ensures that CPU profiling is configurable and that its lifecycle is well-documented through logs, aiding in performance tuning and issue resolution.


628-664: Updated getCtx and startApp to use viper and logger, aligning with the new configuration and logging framework.

These updates are crucial for ensuring that the application context and startup processes are managed with the latest practices, enhancing clarity and control.


Line range hint 707-753: Introduced changes in the InPlaceTestnetCreator function to utilize viper and logger for configuration and logging.

This integration ensures that the testnet creation process is configurable and well-logged, aligning with the new architectural direction of using viper and log.


Line range hint 776-834: Refactored testnetify function to use viper for configuration management and logger for logging, enhancing the testnet setup process.

This refactoring improves the configurability and observability of the testnet setup process, making it more robust and easier to manage.

client/cmd.go Outdated
Comment on lines 381 to 388
func GetViperFromCmd(cmd *cobra.Command) *viper.Viper {
value := cmd.Context().Value(corectx.ViperContextKey)
v, ok := value.(*viper.Viper)
if !ok {
return viper.New()
}
return v
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling when retrieving Viper instance from command context.

The function GetViperFromCmd returns a new Viper instance if it cannot find one in the command context. This might lead to unexpected behavior if the context was supposed to already contain a specific configuration. Consider throwing an error or logging a warning when the Viper instance is not found.

- if !ok {
-     return viper.New()
- }
+ if !ok {
+     return nil, fmt.Errorf("viper instance not found in command context")
+ }

Committable suggestion was skipped due low confidence.

client/cmd.go Outdated
Comment on lines 399 to 406
func GetLoggerFromCmd(cmd *cobra.Command) log.Logger {
v := cmd.Context().Value(corectx.LoggerContextKey)
logger, ok := v.(log.Logger)
if !ok {
return log.NewLogger(cmd.OutOrStdout())
}
return logger
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure consistent error handling in GetLoggerFromCmd.

Similar to GetViperFromCmd, the function GetLoggerFromCmd creates a new logger if it cannot find one in the command context. This could lead to inconsistent logging behavior. Consider returning an error if the logger is not found.

- if !ok {
-     return log.NewLogger(cmd.OutOrStdout())
- }
+ if !ok {
+     return nil, fmt.Errorf("logger not found in command context")
+ }

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 GetLoggerFromCmd(cmd *cobra.Command) log.Logger {
v := cmd.Context().Value(corectx.LoggerContextKey)
logger, ok := v.(log.Logger)
if !ok {
return log.NewLogger(cmd.OutOrStdout())
}
return logger
}
func GetLoggerFromCmd(cmd *cobra.Command) (log.Logger, error) {
v := cmd.Context().Value(corectx.LoggerContextKey)
logger, ok := v.(log.Logger)
if !ok {
return nil, fmt.Errorf("logger not found in command context")
}
return logger, nil
}

client/cmd.go Outdated
Comment on lines 390 to 397
func GetConfigFromCmd(cmd *cobra.Command) *cmtcfg.Config {
v := cmd.Context().Value(corectx.ViperContextKey)
viper, ok := v.(*viper.Viper)
if !ok {
return cmtcfg.DefaultConfig()
}
return GetConfigFromViper(viper)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review error handling strategy in GetConfigFromCmd.

The function GetConfigFromCmd returns a default configuration when it fails to retrieve the Viper instance from the command context. This could mask configuration errors. It might be more appropriate to return an error to the caller.

- if !ok {
-     return cmtcfg.DefaultConfig()
- }
+ if !ok {
+     return nil, fmt.Errorf("failed to retrieve viper instance from command context")
+ }

Committable suggestion was skipped due low confidence.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 7c4e602 and baae5d4.
Files selected for processing (11)
  • client/cmd.go (2 hunks)
  • core/context/server_context.go (1 hunks)
  • server/cmd/execute.go (2 hunks)
  • server/util.go (10 hunks)
  • server/util_test.go (15 hunks)
  • simapp/simd/cmd/testnet_test.go (4 hunks)
  • tests/e2e/genutil/export_test.go (5 hunks)
  • x/genutil/client/cli/export_test.go (3 hunks)
  • x/genutil/client/cli/genaccount_test.go (4 hunks)
  • x/genutil/client/cli/init_test.go (16 hunks)
  • x/genutil/client/testutil/helpers.go (2 hunks)
Files skipped from review as they are similar to previous changes (10)
  • client/cmd.go
  • core/context/server_context.go
  • server/cmd/execute.go
  • server/util.go
  • simapp/simd/cmd/testnet_test.go
  • tests/e2e/genutil/export_test.go
  • x/genutil/client/cli/export_test.go
  • x/genutil/client/cli/genaccount_test.go
  • x/genutil/client/cli/init_test.go
  • x/genutil/client/testutil/helpers.go
Additional Context Used
Path-based Instructions (1)
server/util_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 (12)
server/util_test.go (12)

19-21: Imports updated to use cosmossdk.io/core/context and cosmossdk.io/log instead of the server package.

This change aligns with the PR's objective to refactor context handling by removing dependencies on the server package and using more specific packages for context and logging.


147-148: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


187-188: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


217-218: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


255-256: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


364-365: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


382-383: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


400-401: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


418-419: Repeated issue: Use of empty struct keys in context.WithValue.

Refer to the previous comment on lines 65-66 for the suggested fix.


449-451: Use of log.NewNopLogger() in test setup.

This is a good practice in tests where logging output is not necessary, as it prevents cluttering the test output.


465-468: Proper use of context values in test setup.

This segment correctly sets multiple context values which are essential for the test's configuration and logging. Good use of context to pass necessary data.


486-491: Ensure error handling is robust in configuration interception.

It's crucial to have tests that cover error paths to ensure that the application behaves correctly under failure conditions.

Verification successful

The tests in server/util_test.go do cover various scenarios for InterceptConfigsPreRunHandler, which calls InterceptConfigsAndCreateContext. Additionally, the TestInterceptConfigsWithBadPermissions function specifically tests for permission errors, ensuring that such errors are correctly handled.

  • TestInterceptConfigsPreRunHandlerCreatesConfigFilesWhenMissing
  • TestInterceptConfigsPreRunHandlerReadsConfigToml
  • TestInterceptConfigsPreRunHandlerReadsAppToml
  • TestInterceptConfigsPreRunHandlerReadsFlags
  • TestInterceptConfigsPreRunHandlerReadsEnvVars
  • TestInterceptConfigsPreRunHandlerPrecedenceFlag
  • TestInterceptConfigsPreRunHandlerPrecedenceEnvVar
  • TestInterceptConfigsPreRunHandlerPrecedenceConfigFile
  • TestInterceptConfigsPreRunHandlerPrecedenceConfigDefault
  • TestInterceptConfigsWithBadPermissions

These tests collectively ensure that error handling in the configuration interception process is robust.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that error handling paths in configuration interception are covered by tests.

# Test: Search for tests covering error paths in configuration interception. Expect: Tests that simulate errors and check responses.
rg --type go 'server.InterceptConfigsAndCreateContext'

Length of output: 168



Script:

#!/bin/bash
# Description: Verify that error handling paths in configuration interception are covered by tests.

# Test: Search for tests covering error paths in configuration interception. Expect: Tests that simulate errors and check responses.
rg --type go 'InterceptConfigsAndCreateContext.*err'

Length of output: 238



Script:

#!/bin/bash
# Description: Verify if error handling paths in configuration interception are covered indirectly by other tests.

# Test: Search for test functions that call InterceptConfigsAndCreateContext and check their error handling.
rg --type go 'InterceptConfigsAndCreateContext' -A 5

Length of output: 2037



Script:

#!/bin/bash
# Description: Extract the full test function from server/util_test.go to review its coverage of error handling paths.

# Extract: Get the full test function that calls InterceptConfigsAndCreateContext.
rg --type go -A 20 'func Test.*InterceptConfigsAndCreateContext' server/util_test.go

Length of output: 86



Script:

#!/bin/bash
# Description: List all test functions in server/util_test.go to identify potential indirect tests for error handling in InterceptConfigsAndCreateContext.

# List: Get all test functions in server/util_test.go.
rg --type go '^func Test' server/util_test.go

Length of output: 837



Script:

#!/bin/bash
# Description: Extract the TestInterceptConfigsPreRunHandler* and TestInterceptConfigsWithBadPermissions functions to review their coverage of error handling paths.

# Extract: Get the full TestInterceptConfigsPreRunHandler* and TestInterceptConfigsWithBadPermissions functions.
rg --type go -A 20 'func TestInterceptConfigsPreRunHandler|func TestInterceptConfigsWithBadPermissions' server/util_test.go

Length of output: 7766

Comment on lines 65 to 66
ctx := context.WithValue(context.Background(), corectx.LoggerContextKey{}, log.NewLogger(os.Stdout))
ctx = context.WithValue(ctx, corectx.ViperContextKey{}, viper.New())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use of empty struct keys in context.WithValue is unconventional and potentially error-prone.

- ctx := context.WithValue(context.Background(), corectx.LoggerContextKey{}, log.NewLogger(os.Stdout))
- ctx = context.WithValue(ctx, corectx.ViperContextKey{}, viper.New())
+ ctx := context.WithValue(context.Background(), corectx.LoggerContextKey, log.NewLogger(os.Stdout))
+ ctx = context.WithValue(ctx, corectx.ViperContextKey, viper.New())

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
ctx := context.WithValue(context.Background(), corectx.LoggerContextKey{}, log.NewLogger(os.Stdout))
ctx = context.WithValue(ctx, corectx.ViperContextKey{}, viper.New())
ctx := context.WithValue(context.Background(), corectx.LoggerContextKey, log.NewLogger(os.Stdout))
ctx = context.WithValue(ctx, corectx.ViperContextKey, viper.New())

Comment on lines 109 to 112
v := ctx.Value(corectx.ViperContextKey{})
viper, _ := v.(*viper.Viper)

if viper == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

Ensure nil checks for context values to prevent potential nil pointer dereferences.

The following instances of ctx.Value usage need attention to ensure proper nil checks:

  • server/v2/server.go:

    serverCfg := ctx.Value(ServerContextKey).(Config)
  • runtime/v2/services/transaction.go:

    return ctx.Value(corecontext.ExecModeKey).(transaction.ExecMode)
  • runtime/v2/services/comet.go:

    ci := ctx.Value(corecontext.CometInfoKey).(comet.Info)
  • collections/colltest/store.go:

    return ctx.Value(contextStoreKey{}).(store.KVStore)
  • types/context.go:

    return ctx.Value(SdkContextKey).(Context)
  • x/accounts/internal/implementation/context.go:

    return ctx.Value(contextKey{}).(contextValue)

Please ensure that these instances include proper nil checks before using the retrieved values to prevent runtime panics.

Analysis chain

Check for potential nil pointer dereference when asserting type from context value.

Ensure that the viper instance retrieved from the context is checked for nil before usage to prevent runtime panics.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that there are no instances where the viper instance could be nil before being used.

# Test: Search for viper usage after being retrieved from context. Expect: Proper checks before usage.
rg --type go 'viper, _ := ctx.Value(corectx.ViperContextKey{}).(*viper.Viper)'

Length of output: 272



Script:

#!/bin/bash
# Description: Verify that there are no instances where the viper instance could be nil before being used.

# Test: Search for viper usage after being retrieved from context. Expect: Proper checks before usage.
rg --type go 'viper, _ := ctx\.Value\(corectx\.ViperContextKey\{\}\)\.\(\*viper\.Viper\)'

Length of output: 91



Script:

#!/bin/bash
# Description: Search for all instances where ctx.Value is used and inspect the handling of the retrieved values.

# Test: Search for all instances of ctx.Value usage in Go files.
rg --type go 'ctx\.Value'

Length of output: 961



Script:

#!/bin/bash
# Description: Extract and display the relevant lines of code around each instance of ctx.Value usage for manual inspection.

# Test: Display lines around each ctx.Value usage to check for nil handling.
rg --type go --context 5 'ctx\.Value'

Length of output: 7335

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a mention of all the public api break in the changelog.md?

server/start.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Outside diff range and nitpick comments (1)
server/start.go (1)

577-595: Review the implementation of CPU profiling.

Consider encapsulating the CPU profiling setup and cleanup logic into a separate function for better code organization and reusability. This would make the wrapCPUProfile function cleaner and more focused on its primary responsibility.

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between baae5d4 and 5de4f6c.
Files selected for processing (2)
  • server/start.go (23 hunks)
  • server/util.go (10 hunks)
Files skipped from review as they are similar to previous changes (1)
  • server/util.go
Additional Context Used
Path-based Instructions (1)
server/start.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (9)
server/start.go (9)

124-124: Refactor StartCmdOptions to use viper.Viper and log.Logger instead of server.Context.

The changes align with the PR's objective to remove server.Context and use more modular components like viper for configuration and log for logging. This should enhance the maintainability and flexibility of the configuration management.

Also applies to: 126-126, 130-130


Line range hint 219-241: Refactor start function to use viper.Viper and log.Logger.

The refactoring of the start function to use viper and logger instead of server.Context is consistent with the overall goal of the PR. This change should make the function more straightforward and aligned with modern Go practices by decoupling it from a custom context implementation.


244-255: Refactor startStandAlone to handle configuration and logging with viper and log.

The changes in startStandAlone function reflect the shift from using a custom server context to using viper for configuration and log for logging. This should improve the clarity and maintainability of the code.

Also applies to: 258-266, 280-291, 298-305


Line range hint 312-358: Refactor startInProcess to integrate viper and log.

The startInProcess function has been successfully refactored to utilize viper and log instead of the previous server.Context. This change supports the PR's goal of enhancing configuration management and logging capabilities within the server's operational context.


Line range hint 373-390: Update startCmtNode to use viper and log.

The startCmtNode function now uses viper for configuration and log for logging, replacing the old context-based approach. This update is in line with the PR's objectives and should help in maintaining a cleaner and more modular codebase.


409-410: Ensure configuration and API server start-up are correctly refactored.

The refactoring of configuration retrieval and validation, as well as the API server start-up, to use viper and log is well-executed. These changes are crucial for ensuring that the server's configuration management is both flexible and robust.

Also applies to: 486-523, 543-555


622-626: Refactor context and error group setup.

The refactoring of the context and error group setup to be more modular and testable is a positive change. It aligns with modern Go practices and enhances the readability and maintainability of the code.


630-658: Ensure startApp function correctly handles new configurations.

The startApp function has been updated to handle configurations using viper. It's important to verify that these changes are well-covered by unit tests, especially given the significant refactoring involved.


181-183: Ensure proper error handling and logging during server start-up.

The refactored code properly initializes viper and logger and uses them in the start-up process. However, it's crucial to ensure that all error paths are adequately covered by tests, especially since the configuration handling has been significantly changed.

Also applies to: 199-199, 203-208

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think given the internals are now changed to work smoothly with no server context, we should only remove the usage of server context in modules commands (already done here), but not in the server/ package. The notion of server context can still live in a server v0 world imho. But now with those changes, we can have better APIs for server v2 and do no rely on it.

server/util.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Outside diff range and nitpick comments (2)
server/util.go (2)

49-49: Mark Context struct as deprecated in the documentation.

Consider adding a @deprecated tag in the GoDoc comment for the Context struct to clearly indicate its deprecation status in the documentation.


246-246: Clarify the purpose of the debug statement.

The debug statement fmt.Println("Init root", rootDir) might be useful during development, but consider removing or replacing it with a more structured logging approach before merging to production.

Review Details

Configuration used: .coderabbit.yml
Review profile: CHILL

Commits Files that changed from the base of the PR and between 5de4f6c and 731751b.
Files selected for processing (1)
  • server/util.go (4 hunks)
Additional Context Used
Path-based Instructions (1)
server/util.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Additional comments not posted (1)
server/util.go (1)

228-233: Use custom types for context keys to avoid potential collisions.

This comment is still valid as per the previous review. Using built-in types like string as keys for context values can lead to collisions. Define custom types for LoggerContextKey and ViperContextKey to avoid this issue.

- cmdCtx = context.WithValue(cmdCtx, corectx.ViperContextKey{}, serverCtx.Viper)
- cmdCtx = context.WithValue(cmdCtx, corectx.LoggerContextKey{}, serverCtx.Logger)
+ type loggerKey struct{}
+ type viperKey struct{}
+ cmdCtx = context.WithValue(cmdCtx, loggerKey{}, serverCtx.Logger)
+ cmdCtx = context.WithValue(cmdCtx, viperKey{}, serverCtx.Viper)

client/cmd.go Show resolved Hide resolved
server/util.go Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
server/util.go Outdated Show resolved Hide resolved
x/genutil/client/cli/export_test.go Outdated Show resolved Hide resolved
x/genutil/client/testutil/helpers.go Outdated Show resolved Hide resolved
x/genutil/client/testutil/helpers.go Outdated Show resolved Hide resolved
x/genutil/collect_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 731751b and e3c0a03.
Files selected for processing (9)
  • CHANGELOG.md (1 hunks)
  • core/context/server_context.go (1 hunks)
  • server/util.go (3 hunks)
  • simapp/simd/cmd/testnet_test.go (4 hunks)
  • tests/e2e/genutil/export_test.go (5 hunks)
  • testutil/network/util.go (3 hunks)
  • x/genutil/client/cli/init_test.go (16 hunks)
  • x/genutil/client/testutil/helpers.go (2 hunks)
  • x/genutil/collect_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (8)
  • core/context/server_context.go
  • server/util.go
  • simapp/simd/cmd/testnet_test.go
  • tests/e2e/genutil/export_test.go
  • testutil/network/util.go
  • x/genutil/client/cli/init_test.go
  • x/genutil/client/testutil/helpers.go
  • x/genutil/collect_test.go
Additional Context Used
Markdownlint (384)
CHANGELOG.md (384)

70: Expected: 2; Actual: 4
Unordered list indentation


71: Expected: 2; Actual: 4
Unordered list indentation


75: Expected: 2; Actual: 4
Unordered list indentation


76: Expected: 2; Actual: 4
Unordered list indentation


77: Expected: 2; Actual: 4
Unordered list indentation


78: Expected: 2; Actual: 4
Unordered list indentation


83: Expected: 2; Actual: 4
Unordered list indentation


127: Expected: 2; Actual: 4
Unordered list indentation


128: Expected: 2; Actual: 4
Unordered list indentation


129: Expected: 2; Actual: 4
Unordered list indentation


133: Expected: 2; Actual: 4
Unordered list indentation


136: Expected: 2; Actual: 4
Unordered list indentation


137: Expected: 2; Actual: 4
Unordered list indentation


138: Expected: 2; Actual: 4
Unordered list indentation


145: Expected: 2; Actual: 4
Unordered list indentation


155: Expected: 2; Actual: 4
Unordered list indentation


157: Expected: 2; Actual: 4
Unordered list indentation


160: Expected: 2; Actual: 4
Unordered list indentation


179: Expected: 2; Actual: 4
Unordered list indentation


180: Expected: 2; Actual: 4
Unordered list indentation


182: Expected: 2; Actual: 4
Unordered list indentation


183: Expected: 2; Actual: 4
Unordered list indentation


215: Expected: 2; Actual: 4
Unordered list indentation


216: Expected: 2; Actual: 4
Unordered list indentation


217: Expected: 2; Actual: 4
Unordered list indentation


381: Expected: 2; Actual: 4
Unordered list indentation


384: Expected: 2; Actual: 4
Unordered list indentation


406: Expected: 2; Actual: 4
Unordered list indentation


407: Expected: 2; Actual: 4
Unordered list indentation


420: Expected: 2; Actual: 4
Unordered list indentation


452: Expected: 2; Actual: 4
Unordered list indentation


453: Expected: 2; Actual: 4
Unordered list indentation


454: Expected: 2; Actual: 4
Unordered list indentation


455: Expected: 2; Actual: 4
Unordered list indentation


457: Expected: 2; Actual: 4
Unordered list indentation


458: Expected: 2; Actual: 4
Unordered list indentation


459: Expected: 2; Actual: 4
Unordered list indentation


460: Expected: 2; Actual: 4
Unordered list indentation


474: Expected: 2; Actual: 4
Unordered list indentation


476: Expected: 2; Actual: 4
Unordered list indentation


478: Expected: 2; Actual: 4
Unordered list indentation


480: Expected: 2; Actual: 4
Unordered list indentation


483: Expected: 2; Actual: 4
Unordered list indentation


484: Expected: 2; Actual: 4
Unordered list indentation


485: Expected: 2; Actual: 4
Unordered list indentation


493: Expected: 2; Actual: 4
Unordered list indentation


494: Expected: 2; Actual: 4
Unordered list indentation


496: Expected: 2; Actual: 4
Unordered list indentation


497: Expected: 2; Actual: 4
Unordered list indentation


499: Expected: 2; Actual: 4
Unordered list indentation


500: Expected: 2; Actual: 4
Unordered list indentation


501: Expected: 2; Actual: 4
Unordered list indentation


503: Expected: 2; Actual: 4
Unordered list indentation


504: Expected: 2; Actual: 4
Unordered list indentation


512: Expected: 2; Actual: 4
Unordered list indentation


523: Expected: 2; Actual: 4
Unordered list indentation


524: Expected: 2; Actual: 4
Unordered list indentation


525: Expected: 2; Actual: 4
Unordered list indentation


531: Expected: 2; Actual: 4
Unordered list indentation


532: Expected: 2; Actual: 4
Unordered list indentation


533: Expected: 2; Actual: 4
Unordered list indentation


539: Expected: 2; Actual: 4
Unordered list indentation


555: Expected: 2; Actual: 4
Unordered list indentation


556: Expected: 2; Actual: 4
Unordered list indentation


557: Expected: 2; Actual: 4
Unordered list indentation


558: Expected: 2; Actual: 4
Unordered list indentation


559: Expected: 2; Actual: 4
Unordered list indentation


560: Expected: 2; Actual: 4
Unordered list indentation


565: Expected: 2; Actual: 4
Unordered list indentation


566: Expected: 2; Actual: 4
Unordered list indentation


567: Expected: 2; Actual: 4
Unordered list indentation


568: Expected: 2; Actual: 4
Unordered list indentation


575: Expected: 2; Actual: 4
Unordered list indentation


576: Expected: 2; Actual: 4
Unordered list indentation


577: Expected: 2; Actual: 4
Unordered list indentation


611: Expected: 2; Actual: 4
Unordered list indentation


612: Expected: 2; Actual: 4
Unordered list indentation


613: Expected: 2; Actual: 4
Unordered list indentation


614: Expected: 2; Actual: 4
Unordered list indentation


619: Expected: 2; Actual: 4
Unordered list indentation


620: Expected: 2; Actual: 4
Unordered list indentation


757: Expected: 2; Actual: 4
Unordered list indentation


900: Expected: 2; Actual: 4
Unordered list indentation


921: Expected: 2; Actual: 4
Unordered list indentation


924: Expected: 2; Actual: 4
Unordered list indentation


1006: Expected: 2; Actual: 4
Unordered list indentation


1007: Expected: 2; Actual: 4
Unordered list indentation


1008: Expected: 2; Actual: 4
Unordered list indentation


1009: Expected: 2; Actual: 4
Unordered list indentation


1010: Expected: 2; Actual: 4
Unordered list indentation


1011: Expected: 2; Actual: 4
Unordered list indentation


1108: Expected: 2; Actual: 4
Unordered list indentation


1194: Expected: 2; Actual: 4
Unordered list indentation


1240: Expected: 2; Actual: 4
Unordered list indentation


1246: Expected: 2; Actual: 4
Unordered list indentation


1247: Expected: 2; Actual: 4
Unordered list indentation


1248: Expected: 2; Actual: 4
Unordered list indentation


1249: Expected: 2; Actual: 4
Unordered list indentation


1250: Expected: 2; Actual: 4
Unordered list indentation


1251: Expected: 2; Actual: 4
Unordered list indentation


1351: Expected: 2; Actual: 4
Unordered list indentation


1476: Expected: 2; Actual: 4
Unordered list indentation


1477: Expected: 4; Actual: 8
Unordered list indentation


1478: Expected: 4; Actual: 8
Unordered list indentation


1479: Expected: 2; Actual: 4
Unordered list indentation


1480: Expected: 4; Actual: 8
Unordered list indentation


1481: Expected: 4; Actual: 8
Unordered list indentation


1482: Expected: 4; Actual: 8
Unordered list indentation


1483: Expected: 2; Actual: 4
Unordered list indentation


1486: Expected: 2; Actual: 4
Unordered list indentation


1487: Expected: 4; Actual: 8
Unordered list indentation


1488: Expected: 2; Actual: 4
Unordered list indentation


1489: Expected: 4; Actual: 8
Unordered list indentation


1490: Expected: 4; Actual: 8
Unordered list indentation


1491: Expected: 4; Actual: 8
Unordered list indentation


1740: Expected: 2; Actual: 4
Unordered list indentation


1741: Expected: 2; Actual: 4
Unordered list indentation


1742: Expected: 2; Actual: 4
Unordered list indentation


1743: Expected: 2; Actual: 4
Unordered list indentation


1744: Expected: 2; Actual: 4
Unordered list indentation


1745: Expected: 2; Actual: 4
Unordered list indentation


1855: Expected: 2; Actual: 4
Unordered list indentation


2192: Expected: 2; Actual: 4
Unordered list indentation


2193: Expected: 2; Actual: 4
Unordered list indentation


2194: Expected: 2; Actual: 4
Unordered list indentation


2197: Expected: 2; Actual: 4
Unordered list indentation


2198: Expected: 2; Actual: 4
Unordered list indentation


2199: Expected: 2; Actual: 4
Unordered list indentation


2221: Expected: 2; Actual: 4
Unordered list indentation


2222: Expected: 2; Actual: 4
Unordered list indentation


2223: Expected: 2; Actual: 4
Unordered list indentation


2224: Expected: 2; Actual: 4
Unordered list indentation


2225: Expected: 2; Actual: 4
Unordered list indentation


2233: Expected: 2; Actual: 4
Unordered list indentation


2234: Expected: 2; Actual: 4
Unordered list indentation


2235: Expected: 2; Actual: 4
Unordered list indentation


2236: Expected: 2; Actual: 4
Unordered list indentation


2237: Expected: 2; Actual: 4
Unordered list indentation


2239: Expected: 2; Actual: 4
Unordered list indentation


2240: Expected: 2; Actual: 4
Unordered list indentation


2241: Expected: 2; Actual: 4
Unordered list indentation


2568: Expected: 2; Actual: 4
Unordered list indentation


2569: Expected: 2; Actual: 4
Unordered list indentation


2570: Expected: 2; Actual: 4
Unordered list indentation


2571: Expected: 2; Actual: 4
Unordered list indentation


2572: Expected: 2; Actual: 4
Unordered list indentation


2574: Expected: 2; Actual: 4
Unordered list indentation


2576: Expected: 2; Actual: 4
Unordered list indentation


2577: Expected: 2; Actual: 4
Unordered list indentation


2578: Expected: 2; Actual: 4
Unordered list indentation


2579: Expected: 2; Actual: 4
Unordered list indentation


2580: Expected: 2; Actual: 4
Unordered list indentation


2581: Expected: 2; Actual: 4
Unordered list indentation


2583: Expected: 2; Actual: 4
Unordered list indentation


2584: Expected: 2; Actual: 4
Unordered list indentation


2585: Expected: 2; Actual: 4
Unordered list indentation


2588: Expected: 2; Actual: 4
Unordered list indentation


2589: Expected: 2; Actual: 4
Unordered list indentation


2590: Expected: 2; Actual: 4
Unordered list indentation


2591: Expected: 2; Actual: 4
Unordered list indentation


2592: Expected: 2; Actual: 4
Unordered list indentation


2595: Expected: 2; Actual: 4
Unordered list indentation


2598: Expected: 2; Actual: 4
Unordered list indentation


2601: Expected: 2; Actual: 4
Unordered list indentation


2602: Expected: 2; Actual: 4
Unordered list indentation


2605: Expected: 2; Actual: 4
Unordered list indentation


2612: Expected: 2; Actual: 4
Unordered list indentation


2613: Expected: 2; Actual: 4
Unordered list indentation


2614: Expected: 2; Actual: 4
Unordered list indentation


2615: Expected: 2; Actual: 4
Unordered list indentation


2616: Expected: 2; Actual: 4
Unordered list indentation


2618: Expected: 2; Actual: 4
Unordered list indentation


2619: Expected: 2; Actual: 4
Unordered list indentation


2620: Expected: 2; Actual: 4
Unordered list indentation


2621: Expected: 2; Actual: 4
Unordered list indentation


2622: Expected: 2; Actual: 4
Unordered list indentation


2623: Expected: 2; Actual: 4
Unordered list indentation


2624: Expected: 2; Actual: 4
Unordered list indentation


2625: Expected: 2; Actual: 4
Unordered list indentation


2626: Expected: 4; Actual: 8
Unordered list indentation


2629: Expected: 2; Actual: 4
Unordered list indentation


2630: Expected: 2; Actual: 4
Unordered list indentation


2631: Expected: 2; Actual: 4
Unordered list indentation


2632: Expected: 2; Actual: 4
Unordered list indentation


2633: Expected: 2; Actual: 4
Unordered list indentation


2634: Expected: 2; Actual: 4
Unordered list indentation


2641: Expected: 2; Actual: 4
Unordered list indentation


2642: Expected: 2; Actual: 4
Unordered list indentation


2643: Expected: 2; Actual: 4
Unordered list indentation


2644: Expected: 2; Actual: 4
Unordered list indentation


2651: Expected: 2; Actual: 4
Unordered list indentation


2653: Expected: 2; Actual: 4
Unordered list indentation


2655: Expected: 2; Actual: 4
Unordered list indentation


2656: Expected: 2; Actual: 4
Unordered list indentation


2657: Expected: 2; Actual: 4
Unordered list indentation


2658: Expected: 2; Actual: 4
Unordered list indentation


2659: Expected: 2; Actual: 4
Unordered list indentation


2660: Expected: 2; Actual: 4
Unordered list indentation


2661: Expected: 2; Actual: 4
Unordered list indentation


2662: Expected: 2; Actual: 4
Unordered list indentation


2663: Expected: 2; Actual: 4
Unordered list indentation


2664: Expected: 2; Actual: 4
Unordered list indentation


2665: Expected: 2; Actual: 4
Unordered list indentation


2666: Expected: 2; Actual: 4
Unordered list indentation


2667: Expected: 2; Actual: 4
Unordered list indentation


2668: Expected: 4; Actual: 8
Unordered list indentation


2669: Expected: 4; Actual: 8
Unordered list indentation


2670: Expected: 4; Actual: 8
Unordered list indentation


2671: Expected: 2; Actual: 4
Unordered list indentation


2672: Expected: 2; Actual: 4
Unordered list indentation


2673: Expected: 2; Actual: 4
Unordered list indentation


2674: Expected: 2; Actual: 4
Unordered list indentation


2675: Expected: 2; Actual: 4
Unordered list indentation


2676: Expected: 2; Actual: 4
Unordered list indentation


2677: Expected: 2; Actual: 4
Unordered list indentation


2678: Expected: 4; Actual: 8
Unordered list indentation


2679: Expected: 4; Actual: 8
Unordered list indentation


2680: Expected: 4; Actual: 8
Unordered list indentation


2681: Expected: 2; Actual: 4
Unordered list indentation


2683: Expected: 4; Actual: 8
Unordered list indentation


2684: Expected: 4; Actual: 8
Unordered list indentation


2686: Expected: 2; Actual: 4
Unordered list indentation


2687: Expected: 2; Actual: 4
Unordered list indentation


2688: Expected: 4; Actual: 8
Unordered list indentation


2689: Expected: 4; Actual: 8
Unordered list indentation


2690: Expected: 2; Actual: 4
Unordered list indentation


2691: Expected: 2; Actual: 4
Unordered list indentation


2692: Expected: 2; Actual: 4
Unordered list indentation


2695: Expected: 2; Actual: 4
Unordered list indentation


2696: Expected: 2; Actual: 4
Unordered list indentation


2698: Expected: 2; Actual: 4
Unordered list indentation


2699: Expected: 2; Actual: 4
Unordered list indentation


2702: Expected: 2; Actual: 4
Unordered list indentation


2703: Expected: 4; Actual: 8
Unordered list indentation


2704: Expected: 4; Actual: 8
Unordered list indentation


2705: Expected: 4; Actual: 8
Unordered list indentation


2706: Expected: 4; Actual: 8
Unordered list indentation


2707: Expected: 2; Actual: 4
Unordered list indentation


2708: Expected: 2; Actual: 4
Unordered list indentation


2709: Expected: 2; Actual: 4
Unordered list indentation


2711: Expected: 2; Actual: 4
Unordered list indentation


2712: Expected: 2; Actual: 4
Unordered list indentation


2713: Expected: 2; Actual: 4
Unordered list indentation


2719: Expected: 2; Actual: 4
Unordered list indentation


2722: Expected: 2; Actual: 4
Unordered list indentation


2728: Expected: 2; Actual: 4
Unordered list indentation


2736: Expected: 2; Actual: 4
Unordered list indentation


2737: Expected: 4; Actual: 8
Unordered list indentation


2738: Expected: 4; Actual: 8
Unordered list indentation


2739: Expected: 2; Actual: 4
Unordered list indentation


2747: Expected: 2; Actual: 4
Unordered list indentation


2754: Expected: 2; Actual: 4
Unordered list indentation


2755: Expected: 2; Actual: 4
Unordered list indentation


2762: Expected: 2; Actual: 4
Unordered list indentation


2764: Expected: 2; Actual: 4
Unordered list indentation


2768: Expected: 2; Actual: 4
Unordered list indentation


2769: Expected: 2; Actual: 4
Unordered list indentation


2771: Expected: 2; Actual: 4
Unordered list indentation


2779: Expected: 2; Actual: 4
Unordered list indentation


2781: Expected: 2; Actual: 4
Unordered list indentation


2782: Expected: 2; Actual: 4
Unordered list indentation


2788: Expected: 2; Actual: 4
Unordered list indentation


2796: Expected: 2; Actual: 4
Unordered list indentation


2797: Expected: 2; Actual: 4
Unordered list indentation


2798: Expected: 2; Actual: 4
Unordered list indentation


2799: Expected: 2; Actual: 4
Unordered list indentation


2800: Expected: 2; Actual: 4
Unordered list indentation


2801: Expected: 2; Actual: 4
Unordered list indentation


2802: Expected: 2; Actual: 4
Unordered list indentation


2803: Expected: 2; Actual: 4
Unordered list indentation


2804: Expected: 2; Actual: 4
Unordered list indentation


2805: Expected: 2; Actual: 4
Unordered list indentation


2806: Expected: 2; Actual: 4
Unordered list indentation


2807: Expected: 2; Actual: 4
Unordered list indentation


2808: Expected: 2; Actual: 4
Unordered list indentation


2809: Expected: 2; Actual: 4
Unordered list indentation


2811: Expected: 2; Actual: 4
Unordered list indentation


2812: Expected: 2; Actual: 4
Unordered list indentation


2814: Expected: 2; Actual: 4
Unordered list indentation


2815: Expected: 2; Actual: 4
Unordered list indentation


2816: Expected: 4; Actual: 8
Unordered list indentation


2817: Expected: 4; Actual: 8
Unordered list indentation


2818: Expected: 2; Actual: 4
Unordered list indentation


2819: Expected: 2; Actual: 4
Unordered list indentation


2820: Expected: 2; Actual: 4
Unordered list indentation


2821: Expected: 2; Actual: 4
Unordered list indentation


2822: Expected: 4; Actual: 8
Unordered list indentation


2823: Expected: 4; Actual: 8
Unordered list indentation


2825: Expected: 2; Actual: 4
Unordered list indentation


2826: Expected: 2; Actual: 4
Unordered list indentation


2829: Expected: 2; Actual: 4
Unordered list indentation


2830: Expected: 2; Actual: 4
Unordered list indentation


2831: Expected: 2; Actual: 4
Unordered list indentation


2832: Expected: 2; Actual: 4
Unordered list indentation


2833: Expected: 4; Actual: 8
Unordered list indentation


2834: Expected: 4; Actual: 8
Unordered list indentation


2835: Expected: 4; Actual: 8
Unordered list indentation


2836: Expected: 4; Actual: 8
Unordered list indentation


2837: Expected: 4; Actual: 8
Unordered list indentation


2838: Expected: 4; Actual: 8
Unordered list indentation


2839: Expected: 4; Actual: 8
Unordered list indentation


2840: Expected: 4; Actual: 8
Unordered list indentation


2841: Expected: 4; Actual: 8
Unordered list indentation


2842: Expected: 4; Actual: 8
Unordered list indentation


2843: Expected: 2; Actual: 4
Unordered list indentation


2844: Expected: 2; Actual: 4
Unordered list indentation


2849: Expected: 2; Actual: 4
Unordered list indentation


2850: Expected: 2; Actual: 4
Unordered list indentation


2851: Expected: 2; Actual: 4
Unordered list indentation


2852: Expected: 2; Actual: 4
Unordered list indentation


2853: Expected: 2; Actual: 4
Unordered list indentation


2854: Expected: 2; Actual: 4
Unordered list indentation


2856: Expected: 2; Actual: 4
Unordered list indentation


2858: Expected: 2; Actual: 4
Unordered list indentation


2872: Expected: 2; Actual: 4
Unordered list indentation


2873: Expected: 2; Actual: 4
Unordered list indentation


2874: Expected: 2; Actual: 4
Unordered list indentation


2879: Expected: 2; Actual: 4
Unordered list indentation


2880: Expected: 2; Actual: 4
Unordered list indentation


2881: Expected: 2; Actual: 4
Unordered list indentation


2885: Expected: 2; Actual: 4
Unordered list indentation


2886: Expected: 2; Actual: 4
Unordered list indentation


2887: Expected: 2; Actual: 4
Unordered list indentation


2888: Expected: 2; Actual: 4
Unordered list indentation


2889: Expected: 2; Actual: 4
Unordered list indentation


2890: Expected: 2; Actual: 4
Unordered list indentation


2893: Expected: 2; Actual: 4
Unordered list indentation


2894: Expected: 2; Actual: 4
Unordered list indentation


2895: Expected: 2; Actual: 4
Unordered list indentation


2896: Expected: 2; Actual: 4
Unordered list indentation


2897: Expected: 2; Actual: 4
Unordered list indentation


2898: Expected: 2; Actual: 4
Unordered list indentation


2899: Expected: 2; Actual: 4
Unordered list indentation


2900: Expected: 2; Actual: 4
Unordered list indentation


2901: Expected: 2; Actual: 4
Unordered list indentation


2903: Expected: 2; Actual: 4
Unordered list indentation


2905: Expected: 2; Actual: 4
Unordered list indentation


2907: Expected: 2; Actual: 4
Unordered list indentation


2912: Expected: 2; Actual: 4
Unordered list indentation


2913: Expected: 2; Actual: 4
Unordered list indentation


2914: Expected: 2; Actual: 4
Unordered list indentation


2915: Expected: 2; Actual: 4
Unordered list indentation


2916: Expected: 2; Actual: 4
Unordered list indentation


2917: Expected: 4; Actual: 8
Unordered list indentation


2918: Expected: 4; Actual: 8
Unordered list indentation


2919: Expected: 2; Actual: 4
Unordered list indentation


2920: Expected: 4; Actual: 8
Unordered list indentation


2921: Expected: 4; Actual: 8
Unordered list indentation


2922: Expected: 4; Actual: 8
Unordered list indentation


2923: Expected: 2; Actual: 4
Unordered list indentation


2924: Expected: 2; Actual: 4
Unordered list indentation


2925: Expected: 2; Actual: 4
Unordered list indentation


2926: Expected: 4; Actual: 8
Unordered list indentation


2927: Expected: 4; Actual: 8
Unordered list indentation


2928: Expected: 2; Actual: 4
Unordered list indentation


2929: Expected: 2; Actual: 4
Unordered list indentation


2930: Expected: 2; Actual: 4
Unordered list indentation


2931: Expected: 2; Actual: 4
Unordered list indentation


2932: Expected: 2; Actual: 4
Unordered list indentation


189: Expected: 0 or 2; Actual: 1
Trailing spaces


212: Expected: 0 or 2; Actual: 1
Trailing spaces


1658: null
Bare URL used


1688: null
Bare URL used


2635: null
Spaces inside emphasis markers


2723: null
Spaces inside emphasis markers


2725: null
Spaces inside emphasis markers


2730: null
Spaces inside emphasis markers


2732: null
Spaces inside emphasis markers


2742: null
Spaces inside emphasis markers


2744: null
Spaces inside emphasis markers


2749: null
Spaces inside emphasis markers


2757: null
Spaces inside emphasis markers


2774: null
Spaces inside emphasis markers


2776: null
Spaces inside emphasis markers


2784: null
Spaces inside emphasis markers


2790: null
Spaces inside emphasis markers


2860: null
Spaces inside emphasis markers


2863: null
Spaces inside emphasis markers


2866: null
Spaces inside emphasis markers


2868: null
Spaces inside emphasis markers


2908: null
Spaces inside emphasis markers


1029: null
Spaces inside code span elements


2676: null
Spaces inside code span elements


2676: null
Spaces inside code span elements

Path-based Instructions (1)
CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

Additional comments not posted (4)
CHANGELOG.md (4)

122-122: The description of the deprecation of ServerContext and the introduction of client.GetCometConfigFromCmd(cmd) is clear and informative.


122-122: The update on the signature change of HasAminoCodec is clearly stated.


122-122: The modification in the arguments for SimulateFromSeed is accurately documented.


122-122: The change in the argument type for NewMsgVerifyInvariant is correctly documented.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between e3c0a03 and 7896c75.
Files selected for processing (1)
  • x/genutil/client/cli/export_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
  • x/genutil/client/cli/export_test.go

@julienrbrt julienrbrt changed the title refactor: remove CometBFT Config from ServerContext refactor(core,server): make commands not rely on server context May 23, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits Files that changed from the base of the PR and between 7896c75 and 8eb2ebf.
Files ignored due to path filters (2)
  • core/go.mod is excluded by !**/*.mod
  • core/go.sum is excluded by !**/*.sum
Files selected for processing (2)
  • server/util.go (3 hunks)
  • simapp/test_helpers.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • server/util.go
  • simapp/test_helpers.go

@hieuvubk hieuvubk added this pull request to the merge queue May 28, 2024
Merged via the queue into main with commit e6e1a85 May 28, 2024
68 checks passed
@hieuvubk hieuvubk deleted the hieu/simplify_server_ctx branch May 28, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:x/genutil genutil module issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants