WriteConfigFile incorrectly marshals the IndexEvents field.

[dwedul-figure]

Summary of Bug

The github.com/cosmos/cosmos-sdk/server/config WriteConfigFile(configFilePath, config) function improperly marshalls the Config.IndexEvents field. The result resembles the output of fmt.Printf("%v", []string{...}).

This then causes the config file to be unreadable using viper.ReadInConfig. Error: While parsing config: (61, 17): no value can start with k. In this case, 'k' is the first character in the first key I had set.

What it writes:
index-events = [key1 key2]
What it should write
index-events = ["key1", "key2"]

Manually updating the file to the corrected format allows the file to be readable again.

Version

v0.43.0

Steps to Reproduce

1. Have a Config object (e.g. the result of DefaultConfig()).
2. Set its IndexEvents to []string{"key1", "key2"}.
3. Use WriteConfigFile(configFilePath, config) to save the config to a file.
4. Ask viper to read the config file, e.g. viper.ReadInConfig().

Expected result: The config file is read and parsed by viper.
Actual result: Viper returns an error: While parsing config: (61, 17): no value can start with k

Here's a unit test:

package config

import (
	"path/filepath"
	"testing"

	"github.com/spf13/viper"
	"github.com/stretchr/testify/suite"

	serverconfig "github.com/cosmos/cosmos-sdk/server/config"
)

type IndexEventsTestSuite struct {
	suite.Suite
}

func TestIndexEventsTestSuite(t *testing.T) {
	suite.Run(t, new(IndexEventsTestSuite))
}

func (s *IndexEventsTestSuite) TestConfigIndexEventsWriteRead() {
	// Create config with two IndexEvents entries, and write it to a file.
	confFile := filepath.Join(s.T().TempDir(), "app.toml")
	conf := serverconfig.DefaultConfig()
	conf.IndexEvents = []string{"key1", "key2"}
	serverconfig.WriteConfigFile(confFile, conf)

	// Read that file into viper.
	vpr := viper.New()
	vpr.SetConfigFile(confFile)
	err := vpr.ReadInConfig()
	s.Require().NoError(err, "reading config file into viper")
	vprIndexEvents := vpr.GetStringSlice("index-events")
	s.Require().Equal(conf.IndexEvents, vprIndexEvents, "viper's index events")
}

Test result:

=== RUN   TestIndexEventsTestSuite/TestConfigIndexEventsWriteRead
    indexevents_test.go:32: 
        	Error Trace:	indexevents_test.go:32
        	Error:      	Received unexpected error:
        	            	While parsing config: (61, 17): no value can start with k
        	Test:       	TestIndexEventsTestSuite/TestConfigIndexEventsWriteRead
        	Messages:   	reading config file into viper
    --- FAIL: TestIndexEventsTestSuite/TestConfigIndexEventsWriteRead (0.00s)

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

Thanks for the bug report @dwedul-figure. Since the default is [] for IndexEvents, it makes sense why we haven't issue before.

Just to clarify, you're stating this issue exists when the default IndexEvents is non-empty, it breaks? I ask because you can set IndexEvents after the file exists and restart the node just fine.

[dwedul-figure]

Just to clarify, you're stating this issue exists when the default IndexEvents is non-empty, it breaks? I ask because you can set IndexEvents after the file exists and restart the node just fine.

The problem comes up any time a Config struct has a non-empty IndexEvents, and WriteConfigFile is used to write that config to a file. The next time that an attempt is made to load that config file, it'll fail to parse the index-events entry.

[dwedul-figure]

Based off some comparisons with Tendermint's stuff in toml.go, I think the change would be something like this:

Change this line in the template:

index-events = {{ .BaseConfig.IndexEvents }}

to

index-events = [{{ range .BaseConfig.IndexEvents }}{{ printf "%q, " . }}{{end}}]

I think you might need to update the init too:
From this:

tmpl := template.New("appConfigFileTemplate")

to this:

tmpl := template.New("appConfigFileTemplate").Funcs(template.FuncMap{
	"StringsJoin": strings.Join,
})

Keep an eye on telemetry.global-labels though. I don't think that change would mess it up, but I'm not certain.

[alexanderbez]

@dwedul-figure would you be open to making a PR?

[dwedul-figure]

Sure thing! Coming up!

[dwedul-figure]

The PR has been submitted: #10067. I haven't checked off the confirmed all CI checks have passed checkbox yet, though, because it looks like a maintainer needs provide some sort of authorization in order for the CI stuff to run.

https://github.com/cosmos/cosmos-sdk/actions/runs/1195217037

This workflow is awaiting approval from a maintainer in #10067

Other than that, though, I think it's good to go.