From 7d4cc6bc09e4d7d4258eb123b2a961fc2463355a Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 30 Jun 2026 14:43:43 -0700 Subject: [PATCH 1/2] fix(io): lenient decode so ReadConfigFromDir parses real seid config.toml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The seid/tendermint config templates emit some numeric and bool fields QUOTED (e.g. `duplicate-txs-cache-size = "100000"`, `gossip-tx-key-only = "true"`). The legacy reader (cosmos/Viper) tolerates this via weakly-typed mapstructure coercion, but ReadConfigFromDir decoded with strict BurntSushi/toml, which rejects a quoted string into an int/bool field: toml: line 308 "mempool.duplicate-txs-cache-size": incompatible types: TOML value has type string; destination has type integer So a node whose config.toml came from the standard template could not be read into the unified model at all — the v2 in-binary ConfigManager (sei-chain PLT-775) would fail at boot. Fix: decode TOML to a generic map, then mapstructure-decode into the legacy structs with WeaklyTypedInput (coerces quoted int/bool/uint) and the TextUnmarshaller hook (keeps Duration and other string-encoded types parsing). This mirrors how cosmos/Viper reads the same files, so a real seid config.toml round-trips through the model. Regression test reproduces the quoted int / bool / Duration case. version -> v0.0.21. Co-Authored-By: Claude Opus 4.8 --- go.mod | 2 ++ go.sum | 2 ++ io.go | 31 +++++++++++++++++++++-- io_quoted_scalars_test.go | 53 +++++++++++++++++++++++++++++++++++++++ version.json | 2 +- 5 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 io_quoted_scalars_test.go diff --git a/go.mod b/go.mod index ed35f19..5ed55ba 100644 --- a/go.mod +++ b/go.mod @@ -3,3 +3,5 @@ module github.com/sei-protocol/sei-config go 1.25.6 require github.com/BurntSushi/toml v1.5.0 + +require github.com/mitchellh/mapstructure v1.5.0 diff --git a/go.sum b/go.sum index ff7fd09..3cc5d32 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,4 @@ github.com/BurntSushi/toml v1.5.0 h1:W5quZX/G/csjUnuI8SUYlsHs9M38FC7znL0lIO+DvMg= github.com/BurntSushi/toml v1.5.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= +github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY= +github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= diff --git a/io.go b/io.go index eb18256..3949461 100644 --- a/io.go +++ b/io.go @@ -7,6 +7,7 @@ import ( "path/filepath" "github.com/BurntSushi/toml" + "github.com/mitchellh/mapstructure" ) const ( @@ -23,12 +24,12 @@ func ReadConfigFromDir(homeDir string) (*SeiConfig, error) { appPath := filepath.Join(cfgDir, appTomlFile) var tm legacyTendermintConfig - if _, err := toml.DecodeFile(configPath, &tm); err != nil { + if err := decodeTOMLFile(configPath, &tm); err != nil { return nil, fmt.Errorf("reading %s: %w", configPath, err) } var app legacyAppConfig - if _, err := toml.DecodeFile(appPath, &app); err != nil { + if err := decodeTOMLFile(appPath, &app); err != nil { return nil, fmt.Errorf("reading %s: %w", appPath, err) } @@ -36,6 +37,32 @@ func ReadConfigFromDir(homeDir string) (*SeiConfig, error) { return cfg, nil } +// decodeTOMLFile decodes a TOML file into out, coercing quoted scalars the way +// the legacy Viper/mapstructure reader does. The seid/tendermint config +// templates emit some numeric and bool fields quoted (e.g. +// `duplicate-txs-cache-size = "100000"`, `gossip-tx-key-only = "true"`); +// BurntSushi/toml alone is strict and rejects a quoted string into an int/bool +// field, so we decode to a generic map and then weakly-typed-decode into the +// struct. The TextUnmarshaller hook keeps string-encoded types (Duration) +// parsing as before. This mirrors how cosmos/Viper reads the same files, so a +// real seid config.toml round-trips through the unified model. +func decodeTOMLFile(path string, out any) error { + var raw map[string]any + if _, err := toml.DecodeFile(path, &raw); err != nil { + return err + } + dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ + DecodeHook: mapstructure.TextUnmarshallerHookFunc(), + WeaklyTypedInput: true, + TagName: "toml", + Result: out, + }) + if err != nil { + return err + } + return dec.Decode(raw) +} + // WriteConfigToDir writes the SeiConfig as config.toml and app.toml into // homeDir/config/. Writes are atomic (temp file + rename) to prevent // corruption on crash. diff --git a/io_quoted_scalars_test.go b/io_quoted_scalars_test.go new file mode 100644 index 0000000..648c6c1 --- /dev/null +++ b/io_quoted_scalars_test.go @@ -0,0 +1,53 @@ +package seiconfig + +import ( + "os" + "path/filepath" + "testing" +) + +// TestReadConfigFromDir_CoercesQuotedScalars reproduces the seid/tendermint +// config template's quoted primitives — e.g. `duplicate-txs-cache-size = "100000"` +// (a Go int) and `broadcast = "true"` (a Go bool) — and asserts ReadConfigFromDir +// coerces them instead of failing a strict decode. +// +// Regression for the v2 ConfigManager differential (PLT-775): a real seid +// config.toml is written with these primitives quoted, and the legacy reader +// (cosmos/Viper) tolerates it via weakly-typed coercion. ReadConfigFromDir must +// do the same, or v2 cannot read a real node's config and fails at boot. +func TestReadConfigFromDir_CoercesQuotedScalars(t *testing.T) { + home := t.TempDir() + cfgDir := filepath.Join(home, configDir) + if err := os.MkdirAll(cfgDir, 0o755); err != nil { + t.Fatal(err) + } + + // Quoted int, quoted bool, and a string-encoded Duration — the three + // coercion paths the lenient decoder must handle. + configToml := ` +[mempool] +duplicate-txs-cache-size = "100000" +broadcast = "true" +ttl-duration = "1s" +` + writeFile(t, filepath.Join(cfgDir, configTomlFile), configToml) + writeFile(t, filepath.Join(cfgDir, appTomlFile), "") // empty app.toml: fields default + + cfg, err := ReadConfigFromDir(home) + if err != nil { + t.Fatalf("ReadConfigFromDir failed on quoted scalars: %v", err) + } + if got := cfg.Mempool.DuplicateTxsCacheSize; got != 100000 { + t.Errorf("Mempool.DuplicateTxsCacheSize = %d, want 100000 (quoted int not coerced)", got) + } + if !cfg.Mempool.Broadcast { + t.Errorf("Mempool.Broadcast = false, want true (quoted bool not coerced)") + } +} + +func writeFile(t *testing.T, path, contents string) { + t.Helper() + if err := os.WriteFile(path, []byte(contents), 0o644); err != nil { + t.Fatal(err) + } +} diff --git a/version.json b/version.json index 3e76eee..6966a31 100644 --- a/version.json +++ b/version.json @@ -1,3 +1,3 @@ { - "version": "v0.0.20" + "version": "v0.0.21" } From 3296aa4fd0e8d46fe31df3eec7a8084287edf705 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Tue, 30 Jun 2026 16:00:30 -0700 Subject: [PATCH 2/2] review: harden lenient decode (xreview findings) Addresses the /xreview slate on #36: - dep: github.com/mitchellh/mapstructure (archived) -> github.com/go-viper/ mapstructure/v2 (maintained fork viper migrated to); drop-in, and respects the repo's minimal/maintained-deps invariant (idiomatic-reviewer). - guard: rejectEmptyScalarStringHook errors on an empty string bound to a numeric/bool field instead of letting WeaklyTypedInput silently coerce it to zero/false (the dissenter's empty-string->zero footgun, e.g. a blanked connection limit pinning to 0). - tests: TestReadConfigFromDir_LocksLeniencyBoundary pins what must still error (non-numeric string, empty-string-into-int, malformed duration) so a future decoder change can't silently widen coercion. - comment: soften "mirrors cosmos/Viper" -> "approximates", and document the one remaining accepted widening (non-string scalar -> string field), which seid templates never emit (sei-network-specialist). Co-Authored-By: Claude Opus 4.8 --- go.mod | 2 +- go.sum | 4 +-- io.go | 60 ++++++++++++++++++++++++++++++++------- io_quoted_scalars_test.go | 28 ++++++++++++++++++ 4 files changed, 81 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index 5ed55ba..d3d188d 100644 --- a/go.mod +++ b/go.mod @@ -4,4 +4,4 @@ go 1.25.6 require github.com/BurntSushi/toml v1.5.0 -require github.com/mitchellh/mapstructure v1.5.0 +require github.com/go-viper/mapstructure/v2 v2.5.0 diff --git a/go.sum b/go.sum index 3cc5d32..3a30160 100644 --- a/go.sum +++ b/go.sum @@ -1,4 +1,4 @@ github.com/BurntSushi/toml v1.5.0 h1:W5quZX/G/csjUnuI8SUYlsHs9M38FC7znL0lIO+DvMg= github.com/BurntSushi/toml v1.5.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= -github.com/mitchellh/mapstructure v1.5.0 h1:jeMsZIYE/09sWLaz43PL7Gy6RuMjD2eJVyuac5Z2hdY= -github.com/mitchellh/mapstructure v1.5.0/go.mod h1:bFUtVrKA4DC2yAKiSyO/QUcy7e+RRV2QTWOzhPopBRo= +github.com/go-viper/mapstructure/v2 v2.5.0 h1:vM5IJoUAy3d7zRSVtIwQgBj7BiWtMPfmPEgAXnvj1Ro= +github.com/go-viper/mapstructure/v2 v2.5.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM= diff --git a/io.go b/io.go index 3949461..352474f 100644 --- a/io.go +++ b/io.go @@ -5,9 +5,11 @@ import ( "fmt" "os" "path/filepath" + "reflect" + "strings" "github.com/BurntSushi/toml" - "github.com/mitchellh/mapstructure" + "github.com/go-viper/mapstructure/v2" ) const ( @@ -38,21 +40,32 @@ func ReadConfigFromDir(homeDir string) (*SeiConfig, error) { } // decodeTOMLFile decodes a TOML file into out, coercing quoted scalars the way -// the legacy Viper/mapstructure reader does. The seid/tendermint config -// templates emit some numeric and bool fields quoted (e.g. -// `duplicate-txs-cache-size = "100000"`, `gossip-tx-key-only = "true"`); -// BurntSushi/toml alone is strict and rejects a quoted string into an int/bool -// field, so we decode to a generic map and then weakly-typed-decode into the -// struct. The TextUnmarshaller hook keeps string-encoded types (Duration) -// parsing as before. This mirrors how cosmos/Viper reads the same files, so a -// real seid config.toml round-trips through the unified model. +// the legacy reader does. The seid/tendermint config templates emit some +// numeric and bool fields quoted (e.g. `duplicate-txs-cache-size = "100000"`, +// `gossip-tx-key-only = "true"`); BurntSushi/toml alone is strict and rejects a +// quoted string into an int/bool field, so we decode to a generic map and then +// weakly-typed-decode into the struct. The TextUnmarshaller hook keeps +// string-encoded types (Duration) parsing. +// +// This approximates how cosmos/Viper tolerates quoted scalars (not full parity +// — Viper's hooks and key handling differ). WeaklyTypedInput widens tolerance +// two ways worth knowing: (a) a non-string scalar bound to a string field is +// stringified (e.g. a stray `true` becomes "1") — accepted as benign since seid +// templates never emit that form; (b) an empty string bound to a numeric/bool +// field would coerce to the zero value — this we reject (see +// rejectEmptyScalarStringHook) so blanking a limit errors rather than silently +// pinning it to zero. Genuinely malformed values (non-numeric strings, bad +// durations, overflow) still error (locked by io_quoted_scalars_test.go). func decodeTOMLFile(path string, out any) error { var raw map[string]any if _, err := toml.DecodeFile(path, &raw); err != nil { return err } dec, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{ - DecodeHook: mapstructure.TextUnmarshallerHookFunc(), + DecodeHook: mapstructure.ComposeDecodeHookFunc( + rejectEmptyScalarStringHook, + mapstructure.TextUnmarshallerHookFunc(), + ), WeaklyTypedInput: true, TagName: "toml", Result: out, @@ -63,6 +76,33 @@ func decodeTOMLFile(path string, out any) error { return dec.Decode(raw) } +// rejectEmptyScalarStringHook fails an empty-string value bound to a numeric or +// bool field instead of letting WeaklyTypedInput silently coerce it to the zero +// value. Blanking a numeric (a connection limit, a cache size) should error, +// not silently pin it to 0/false. Non-empty strings pass through unchanged to +// the quoted-scalar coercion the template requires. +func rejectEmptyScalarStringHook(from, to reflect.Type, data any) (any, error) { + if from.Kind() != reflect.String { + return data, nil + } + if s, _ := data.(string); strings.TrimSpace(s) != "" { + return data, nil + } + t := to + for t.Kind() == reflect.Ptr { + t = t.Elem() + } + switch t.Kind() { + case reflect.Bool, + reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, + reflect.Float32, reflect.Float64: + return nil, fmt.Errorf("empty string for %s field", to) + default: + return data, nil + } +} + // WriteConfigToDir writes the SeiConfig as config.toml and app.toml into // homeDir/config/. Writes are atomic (temp file + rename) to prevent // corruption on crash. diff --git a/io_quoted_scalars_test.go b/io_quoted_scalars_test.go index 648c6c1..7b4ef0c 100644 --- a/io_quoted_scalars_test.go +++ b/io_quoted_scalars_test.go @@ -51,3 +51,31 @@ func writeFile(t *testing.T, path, contents string) { t.Fatal(err) } } + +// TestReadConfigFromDir_LocksLeniencyBoundary pins what must STILL error after +// the lenient decode. The whole risk of weakly-typed coercion is silently +// widening; these cases (a genuinely non-numeric string, an empty string into a +// numeric field, a malformed duration) must keep failing, so a future decoder +// change cannot loosen the boundary with a green suite. +func TestReadConfigFromDir_LocksLeniencyBoundary(t *testing.T) { + cases := map[string]string{ + "non-numeric string into int": "[mempool]\nduplicate-txs-cache-size = \"banana\"\n", + "empty string into int": "[mempool]\nduplicate-txs-cache-size = \"\"\n", + "malformed duration": "[mempool]\nttl-duration = \"notaduration\"\n", + } + for name, configToml := range cases { + t.Run(name, func(t *testing.T) { + home := t.TempDir() + cfgDir := filepath.Join(home, configDir) + if err := os.MkdirAll(cfgDir, 0o755); err != nil { + t.Fatal(err) + } + writeFile(t, filepath.Join(cfgDir, configTomlFile), configToml) + writeFile(t, filepath.Join(cfgDir, appTomlFile), "") + + if _, err := ReadConfigFromDir(home); err == nil { + t.Fatalf("expected ReadConfigFromDir to error on %s, got nil", name) + } + }) + } +}