fix(client,server): consistently set env prefix between client/server

[julienrbrt]

Description

Closes: #18332

The client and server both have a viper instance, but they did not use the same prefix.
Server was always setting the binary name as env prefix, while for the client it was left to the user.
This brings consistency and happens to resolve the issue.

I will only backport this to v0.50 but not patch releases, just in case chains rely on the buggy behavior.

---

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
• added ! to 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
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• run make lint and make test
• 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 ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

Summary by CodeRabbit

• Bug Fixes
  • Fixed inconsistency in setting viper prefix in client and server.
  • Removed hardcoded gRPC address to localhost in the server.
  • Fixed builder function for missed blocks key in slashing module.
  • Fixed issue causing failure of inactive and active proposals in governance module if their messages cannot be decoded.

[coderabbitai]

Walkthrough

The changes primarily focus on improving the configuration and environment variable handling in the client and server. The Viper prefix is now consistently set to the binary name in both client and server, and hardcoded gRPC address to localhost in the server has been removed. Fixes were also made to the slashing and governance modules.

Changes

File(s)	Summary
client/context.go, client/tx/factory.go, simapp/simd/cmd/root.go, simapp/simd/cmd/root_v2.go	The Viper prefix is now set to the binary name in the client. The WithViper function has been updated to set the environment prefix based on the executable name if no prefix is provided.
server/start.go	The hardcoded gRPC address to localhost in the server has been removed. The PreRunE function has been removed and its functionality has been merged into the RunE function.
client/config/config_test.go	The nodeEnv constant has been updated and the cleanup steps in the test functions have been modified to unset the nodeEnv environment variable.
CHANGELOG.md	Documented the bug fixes and changes made to the Viper prefix, gRPC address, and the slashing and governance modules.

🐇 "Hopping through the code, making changes with glee,
🍂 As the leaves fall, we improve for all to see.
🌐 No more hardcoded gRPC, a victory indeed,
🎉 Celebrate the changes, for we've planted a good seed.
🌕 Under the November moon, we code without any fear,
🥕 With each bug fix, the vision becomes clear!" 🎈

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

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 a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.
• @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• 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/coderabbit-overrides.json

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 32c8ab9 and 53b6188.

Files selected for processing (4)

• CHANGELOG.md (1 hunks)
• client/context.go (1 hunks)
• client/tx/factory.go (2 hunks)
• server/start.go (1 hunks)

Files skipped from review due to trivial changes (2)

• client/context.go
• client/tx/factory.go

Additional comments: 4

CHANGELOG.md (1)

• 75-78: The changes in the CHANGELOG.md accurately reflect the changes made in the pull request. The links to the pull requests provide a good reference for further details.

server/start.go (3)

• 147-159: The PreRunE function has been removed and its logic has been moved to the RunE function. This change affects the execution flow of the code. Ensure that the removal of PreRunE does not affect other parts of the code that might be relying on it. Also, verify that the error handling and return statements in the RunE function are correctly implemented after the change.

• 151-155: The GetPruningOptionsFromFlags function is now called directly within the RunE function. Ensure that the function is correctly handling the returned error.

• 157-159: The GetClientQueryContext function is also called within the RunE function. Ensure that the function is correctly handling the returned error.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 53b6188 and f90d7e1.

Files selected for processing (2)

• client/context.go (2 hunks)
• client/tx/factory.go (2 hunks)

Files skipped from review due to trivial changes (1)

• client/context.go

Additional comments: 2

client/tx/factory.go (2)

• 8-13: The removal of the import statement for "github.com/spf13/viper" is not reflected in this hunk. Please verify if this is the correct hunk.

• 49-55: The change from clientCtx.Viper to clientCtx.WithViper("") is a good practice as it ensures that clientCtx has a non-nil Viper instance. This prevents potential nil pointer dereference errors.

[odeke-em]

LGTM, thank you @julienrbrt, just one question.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f90d7e1 and 396fc2b.

Files selected for processing (3)

• client/config/config_test.go (7 hunks)
• client/context.go (2 hunks)
• server/util.go (1 hunks)

Files skipped from review due to trivial changes (2)

• client/context.go
• server/util.go

Additional comments: 4

client/config/config_test.go (4)

• 18-24: The nodeEnv constant has been updated to "CONFIG.TEST_NODE". Ensure that this change doesn't break any other parts of the code that might be using the old value "NODE".

• 44-55: The initClientContextWithTemplate function now unsets the nodeEnv environment variable in the cleanup function. This is a good practice as it ensures that the environment variable doesn't affect other tests.

• 124-129: The TestCustomTemplateAndConfig, TestConfigCmdEnvFlag test functions have been modified to remove the call to os.Unsetenv(nodeEnv) in the cleanup function. This is because the cleanup is now handled in the initClientContextWithTemplate function.

• 164-167: The TestConfigCmdEnvFlag test function now uses the initClientContext function to set up the client context. This is a good change as it simplifies the test setup.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 396fc2b and 0dadebe.

Files selected for processing (2)

• simapp/simd/cmd/root.go (1 hunks)
• simapp/simd/cmd/root_v2.go (1 hunks)

Files skipped from review due to trivial changes (2)

• simapp/simd/cmd/root.go
• simapp/simd/cmd/root_v2.go

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 0dadebe and 00bec62.

Files selected for processing (2)

• client/config/config_test.go (7 hunks)
• client/context.go (2 hunks)

Files skipped from review due to trivial changes (2)

• client/config/config_test.go
• client/context.go

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between c664daa and 57ee71a.

Files selected for processing (7)

• CHANGELOG.md (1 hunks)
• client/config/config_test.go (7 hunks)
• client/context.go (2 hunks)
• client/tx/factory.go (2 hunks)
• server/start.go (1 hunks)
• simapp/simd/cmd/root.go (1 hunks)
• simapp/simd/cmd/root_v2.go (1 hunks)

Files skipped from review due to trivial changes (4)

• client/config/config_test.go
• client/context.go
• simapp/simd/cmd/root.go
• simapp/simd/cmd/root_v2.go

Additional comments: 4

CHANGELOG.md (1)

• 75-78: The changes in the changelog are well documented and provide clear information about the bug fixes. No issues found.

client/tx/factory.go (1)

• 49-55: The change to the NewFactoryCLI function is correct and improves the initialization of the clientCtx object. The use of clientCtx.WithViper("") ensures that the clientCtx object has a Viper instance even if none was provided, which is a good practice for avoiding null pointer exceptions. The error handling for clientCtx.Viper.BindPFlags(flagSet) is also correctly implemented.

server/start.go (2)

• 147-159: The GetPruningOptionsFromFlags function is called but the result is not used anywhere. If the function has side effects, it's fine. Otherwise, it's unnecessary to call it. Also, error handling is good here.

- _, err := GetPruningOptionsFromFlags(serverCtx.Viper)
+ pruningOptions, err := GetPruningOptionsFromFlags(serverCtx.Viper)

• 150-159: The GetClientQueryContext function is called and the result is used. Error handling is also done correctly.

[julienrbrt]

Now that v0.50 is released, I am not sure we can backport this as it will be a behavior change (even though a correct one).
Leaving this for v0.51.

[fmorency]

Now that v0.50 is released, I am not sure we can backport this as it will be a behavior change (even though a correct one). Leaving this for v0.51.

@julienrbrt Setting the Viper prefix doesn't work properly right now on 0.50.x. I cannot set --home through an environment variable. Will this get backported?

[julienrbrt]

Now that v0.50 is released, I am not sure we can backport this as it will be a behavior change (even though a correct one). Leaving this for v0.51.

@julienrbrt Setting the Viper prefix doesn't work properly right now on 0.50.x. I cannot set --home through an environment variable. Will this get backported?

Yeah that's unfortunate, you indeed cannot use a prefix consistently in that version. I still stand to my first point that we unfortunately cannot backport this.