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

julienrbrt · 2023-11-02T17:19:40Z

Description

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

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 · 2023-11-02T17:19:46Z

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

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 53b6188ac0f5d0f8c703185830c35216ac7a7b23 and f90d7e145594fef96e8092d2dcf4b625b12ebf8b.

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.

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 f90d7e145594fef96e8092d2dcf4b625b12ebf8b and 396fc2b97bdfc07f4f5dbbddfc123866d370451d.

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 396fc2b97bdfc07f4f5dbbddfc123866d370451d and 0dadebedea3154c1a5c25a52adb9575fd0ce36b2.

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 0dadebedea3154c1a5c25a52adb9575fd0ce36b2 and 00bec626efa6b733e751c5994f44d3a9674330d2.

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.

…#18345) (cherry picked from commit 9e91c7b) # Conflicts: # client/config/config_test.go # client/tx/factory.go

julienrbrt · 2023-11-10T10:40:03Z

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 · 2024-05-16T13:41:35Z

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 · 2024-05-16T14:08:19Z

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.

github-actions bot added the C:CLI label Nov 2, 2023

julienrbrt changed the title ~~refactor: viper investigation~~ fix(client/server): consistently set env prefix between client/server Nov 2, 2023

julienrbrt added the backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release label Nov 2, 2023

julienrbrt changed the title ~~fix(client/server): consistently set env prefix between client/server~~ fix(client,server): consistently set env prefix between client/server Nov 2, 2023

julienrbrt marked this pull request as ready for review November 2, 2023 20:12

julienrbrt requested a review from a team as a code owner November 2, 2023 20:12

coderabbitai bot reviewed Nov 2, 2023

View reviewed changes

odeke-em approved these changes Nov 2, 2023

View reviewed changes

client/context.go Outdated Show resolved Hide resolved

client/context.go Show resolved Hide resolved

coderabbitai bot reviewed Nov 3, 2023

View reviewed changes

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

828e689

julienrbrt force-pushed the julien/viper-investigation branch from 00bec62 to 828e689 Compare November 3, 2023 09:42

Merge branch 'main' into julien/viper-investigation

57ee71a

julienrbrt enabled auto-merge November 3, 2023 09:42

coderabbitai bot reviewed Nov 3, 2023

View reviewed changes

julienrbrt added this pull request to the merge queue Nov 3, 2023

Merged via the queue into main with commit 9e91c7b Nov 3, 2023
60 of 61 checks passed

julienrbrt deleted the julien/viper-investigation branch November 3, 2023 09:59

mergify bot pushed a commit that referenced this pull request Nov 3, 2023

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

8518cb4

…#18345) (cherry picked from commit 9e91c7b) # Conflicts: # client/config/config_test.go # client/tx/factory.go

mergify bot mentioned this pull request Nov 3, 2023

fix(client,server): consistently set env prefix between client/server (backport #18345) #18353

Closed

julienrbrt added TODO:backport-v0.50.1 and removed backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Nov 3, 2023

julienrbrt removed the TODO:backport-v0.50.1 label Nov 10, 2023

fmorency mentioned this pull request May 16, 2024

Add Viper prefix for env var liftedinit/manifest-ledger#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

julienrbrt commented Nov 2, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot left a comment

odeke-em left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

coderabbitai bot left a comment

julienrbrt commented Nov 10, 2023

fmorency commented May 16, 2024

julienrbrt commented May 16, 2024

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

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

Conversation

julienrbrt commented Nov 2, 2023 • edited by coderabbitai bot Loading

Description

Author Checklist

Reviewers Checklist

Summary by CodeRabbit

coderabbitai bot commented Nov 2, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

odeke-em left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

julienrbrt commented Nov 10, 2023

fmorency commented May 16, 2024

julienrbrt commented May 16, 2024

julienrbrt commented Nov 2, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 2, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)