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.

Sign up for GitHub

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

fix: Don't error on startup if min-gas-price is empty #9621

Merged
merged 5 commits into from
Jul 2, 2021
Merged

fix: Don't error on startup if min-gas-price is empty #9621

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26026c7
fix: Don't error on startup if min-gas-price is empty
amaury1093 Jul 2, 2021
7a3185c
remove test
amaury1093 Jul 2, 2021
26fa272
Update server/start.go
amaury1093 Jul 2, 2021
259f663
Update server/start.go
amaury1093 Jul 2, 2021
b9ca9fc
Switch to error
amaury1093 Jul 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#8628](https://github.com/cosmos/cosmos-sdk/issues/8628) Commands no longer print outputs using `stderr` by default
* [\#9134](https://github.com/cosmos/cosmos-sdk/pull/9134) Renamed the CLI flag `--memo` to `--note`.
* [\#9291](https://github.com/cosmos/cosmos-sdk/pull/9291) Migration scripts prior to v0.38 have been removed from the CLI `migrate` command. The oldest supported migration is v0.39->v0.42.
* [\#9371](https://github.com/cosmos/cosmos-sdk/pull/9371) Non-zero default fees/Server will error if there's an empty value for min-gas-price in app.toml
Copy link
Contributor Author

@amaury1093 amaury1093 Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'll add this line to the "Unreleased" section in #9592

edit: done



### Improvements
Expand Down
4 changes: 3 additions & 1 deletion server/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,9 @@ func startInProcess(ctx *Context, clientCtx client.Context, appCreator types.App

config := config.GetConfig(ctx.Viper)
if err := config.ValidateBasic(); err != nil {
return err
ctx.Logger.Info("The minimum-gas-prices config in app.toml is set to the empty string. "+
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can we do Logger.Error or will that cause a panic?

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ctx.Logger.Info("The minimum-gas-prices config in app.toml is set to the empty string. "+
ctx.Logger.Info("WARNING: The minimum-gas-prices config in app.toml is set to the empty string. "+

Copy link
Contributor Author

@amaury1093 amaury1093 Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Switched to error, it doesn't panic:

Screenshot 2021-07-02 at 17 17 20

Too bad the logger doesn't have a .Warn that's yellow, that would be the most accurate

"This defaults to 0 in the current version, but will error in the next version "+
"(SDK v0.44). Please explicitly put the desired minimum-gas-prices in your app.toml.")
}

app := appCreator(ctx.Logger, db, traceWriter, ctx.Viper)
Expand Down
37 changes: 0 additions & 37 deletions server/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,9 @@ import (
"testing"

"github.com/spf13/cobra"
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/server"
"github.com/cosmos/cosmos-sdk/server/config"
"github.com/cosmos/cosmos-sdk/simapp"
genutilcli "github.com/cosmos/cosmos-sdk/x/genutil/client/cli"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

var cancelledInPreRun = errors.New("Cancelled in prerun")
Expand Down Expand Up @@ -406,34 +400,3 @@ func TestInterceptConfigsWithBadPermissions(t *testing.T) {
t.Fatalf("Failed to catch permissions error, got: [%T] %v", err, err)
}
}

func TestEmptyMinGasPrices(t *testing.T) {
tempDir := t.TempDir()
err := os.Mkdir(filepath.Join(tempDir, "config"), os.ModePerm)
require.NoError(t, err)
encCfg := simapp.MakeTestEncodingConfig()

// Run InitCmd to create necessary config files.
clientCtx := client.Context{}.WithHomeDir(tempDir).WithJSONCodec(encCfg.Marshaler)
serverCtx := server.NewDefaultContext()
ctx := context.WithValue(context.Background(), server.ServerContextKey, serverCtx)
ctx = context.WithValue(ctx, client.ClientContextKey, &clientCtx)
cmd := genutilcli.InitCmd(simapp.ModuleBasics, tempDir)
cmd.SetArgs([]string{"appnode-test"})
err = cmd.ExecuteContext(ctx)
require.NoError(t, err)

// Modify app.toml.
appCfgTempFilePath := filepath.Join(tempDir, "config", "app.toml")
appConf := config.DefaultConfig()
appConf.BaseConfig.MinGasPrices = ""
config.WriteConfigFile(appCfgTempFilePath, appConf)

// Run StartCmd.
cmd = server.StartCmd(nil, tempDir)
cmd.PreRunE = func(cmd *cobra.Command, _ []string) error {
return server.InterceptConfigsPreRunHandler(cmd, "", nil)
}
err = cmd.ExecuteContext(ctx)
require.Errorf(t, err, sdkerrors.ErrAppConfig.Error())
}