[ConfigManager] Register Sections 1/4 - #3974
Conversation
Four sections, each registered by the package that owns its struct, so the struct, the values and the keys come from one place and cannot drift apart. The keys derive from the mapstructure tags, which is what makes the registry's spelling and each reader's own constants the same strings, and each package's test holds the two against each other rather than against a written-out list. admin_server 2 keys giga_executor 2 keys receipt-store 6 keys wasm 3 keys Three register their own struct. wasm needs a schema, because the upstream type carries no mapstructure tags at all, so keys derived from it would be field names rather than the ones the module reads. Its simulation gas limit is text because the field it stands for is an optional number and absent is a meaning of its own: unset means the consensus block gas limit applies, which no number can say. Two of that type's settings declare nothing, one having no key any reader resolves and the other written into app.toml by the template and read by nothing. Registering the receipt store needed a distinction the registry did not draw. mapstructure reads a tag of "-" as skip this field, and a configuration struct uses it for a field something else assigns: KeepRecent comes from the global min-retain-blocks flag at the app layer, ExternalPruning from whatever constructs the garbage collector. The registry read that as a missing name and refused the whole section. Such a field now declares no key, which is narrower than declaring one that resolves to a default and safer for the same reason: a declared key is written at override precedence, so a default would land on top of the value that code assigned, and a node with min-retain-blocks set would silently keep nothing. A field with no tag at all stays a defect, because that is the opposite intent, a key nothing names reaching no field. The skip lives in tagOf, which both walks already share, so the declared keys and the rendered defaults describe the same fields. Reverting either walk's skip on its own fails a test. The recorded configuration surface does not move: nothing consumes the registry yet, and no golden changed. 100% of statements in config/registry, race clean. Three mutations each fail a named test: refusing a dash again, skipping an untagged field, and letting the two walks disagree.
PR SummaryLow Risk Overview The registry now treats Wasm registers via a small
Reviewed by Cursor Bugbot for commit 4e382a0. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3974 +/- ##
==========================================
- Coverage 59.08% 58.00% -1.09%
==========================================
Files 2305 2210 -95
Lines 197017 185478 -11539
==========================================
- Hits 116414 107582 -8832
+ Misses 69842 68073 -1769
+ Partials 10761 9823 -938
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-scoped first batch of registry sections: the - tag is now skipped symmetrically in both the type walk (walk) and the value walk (walkValues) through the shared tagOf, so declared keys and rendered defaults stay in agreement, and the four sections' derived keys match what each reader actually resolves. No correctness problems found; the notes below are test-coverage gaps that matter more as the remaining thirteen sections land.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No test links more than one registering package, so cross-section registration failures are invisible.
RegisterSectionrecords a duplicate section name or anenvNamesAreDistinctcollision as aDefectand silently drops the section; each package's ownLookuptest only links that package, so the collision never occurs in any test binary. With four sections today and thirteen more planned (receipt-storevs. a futurereceipt_store, for example, collide onSEID_RECEIPT_STORE_*), consider an aggregate test — e.g. underappor a dedicatedconfig/registry/sectionstest package — that imports every registering package and assertsregistry.Defects()is empty. - [suggestion]
admin/register_test.gois the one section whose stated property is not actually enforced:admin.ReadConfiginlines"admin_server.admin_enabled"/"admin_server.admin_address"as literals, so the test's written-out list guards the mapstructure tags but not the reader. Extracting those two keys into constants inadmin/config.goand using them in both the reader and the test would give this section the same rename-safety the other three have. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| t.Fatalf("%s is not registered, so nothing resolves its keys", ReceiptStoreSectionName) | ||
| } | ||
|
|
||
| want := []string{ |
There was a problem hiding this comment.
[suggestion] This test is in package config, so the reader's own key constants are in scope — flagRSAsyncWriteBuffer, flagRSDBDirectory, flagRSReadWriteMetrics, flagRSLogFilterParallelism, flagRSPruneIntervalSeconds, flagRSBackend (receipt_config.go:17-23). Using them here instead of the written-out strings is what makes the PR's stated property hold: a rename of a flag constant would then move the assertion or fail, whereas today renaming flagRSBackend's value silently drifts the reader away from the derived key while this test still passes. Same for the excluded list on line 33.
There was a problem hiding this comment.
Addressed in f7dc6bb, which landed after the commit this was written against.
The list is now built from the reader's own constants — flagRSAsyncWriteBuffer, flagRSDBDirectory, flagRSReadWriteMetrics, flagRSLogFilterParallelism, flagRSPruneIntervalSeconds, flagRSBackend — and the separate excluded list is gone, folded into the doc comment as the reason those two fields declare nothing.
I checked the property you named rather than assuming it: renaming flagRSBackend's value to receipt-store.rs-backend-renamed now fails the suite. Before the change it would not have.
The rule was right and the reason recorded for it was not, which matters because it is the reason the remaining sections will cite. It said a declared key would be written at override precedence and land on top of the value that code assigned. That cannot happen for either field it named: the app layer assigns the receipt store's retention after the reader has returned, so the assignment is last and wins. It also said such a node would silently keep nothing, and the field's own comment says the opposite, that keeping zero versions means keeping everything. An operator handed that sentence during an incident looks for missing receipts and finds a full disk. The reproducible reason is the one every other refusal here rests on. A key for such a field is one an operator can write that the assignment then discards, so it reaches no field. A field with no tag stays a defect for the same reason read from the other end, because it would declare a key derived from a field name and no operator writes that. The two look alike in a diff and mean opposite things, which is why the package's own contract now states the distinction rather than leaving it in a comment beside one branch. Two of the four sections held their declared keys against a written-out list of the same strings. That is a second statement of the key set, which is what a section exists to remove: a tag and the list move together and the reader keeps asking for the old spelling. Both now hold against the constants their reader passes to Get. The admin server's reader was spelling its two keys inline, so it has constants for them now, and its registration no longer recites the derived keys in prose that drifts the moment a tag moves. One assertion is gone because it could not fail, the exact key set having been compared three lines above it. Two declared values are also stated elsewhere in the binary, and each now says so where it is declared. The wasm query gas limit resolves to ten times what the template writes into a generated file, so a node provisioned by the binary runs the smaller number and a node whose file predates the section runs the declared one; whoever renders declared values into a file has to decide which survives, and that limit bounds the work one smart query can ask of a node serving queries to anyone. The receipt store's database directory resolves to an empty string, and the emptiness carries the meaning: the app layer fills it from the host, keeping the former path for a node that already holds the store there. A path written into a file is one host's answer and names an empty directory on another. One comment said the contract debug switch has no key any reader resolves. It is read from the node-wide trace flag, so its key belongs to the root of the file rather than to that section, which also means the section's three keys do not determine the configuration the module ends up with. A new test asks the whole set at once. Two refusals depend on what else has registered, neither is visible from inside either section, and the section that loses is dropped whole with every key it declared. Registering a section whose key collides with the receipt store's leaves all four section suites green and fails only this one.
The failure messages for an excluded field still claimed the mechanism the comment above them no longer does: that a declared key would land over whatever assigns the field. They now say what is true, that such a key is one an operator can write and the assignment discards. The half of that test covering an untagged field asserted only that some refusal was recorded, so it passed on a refusal raised for any other reason. It now requires the message to name the field, and changing the name it looks for fails it. The registry's contract adds the one thing its new rule leaves unsaid: the exclusion tag is meaningful only on an exported field, because an unexported one carrying any tag is refused before the tag is read. The giga executor's default says why this section does not vary by mode without stating it as a rule for every section, since another section in this work does vary and the binary is what decides which. The fixture in the registry's own spec no longer describes that package as one that would register, because it now does. The wasm section's defaults function takes a name that does not collide with a local of the same name elsewhere in that package, and its test asks every mode rather than one.
…de runs The section declared this module's own default and the command writes a tenth of it, so the two disagreed by a factor of ten on the only bound on the work one smart query can ask of a node that serves queries to anyone. Declaring the larger one meant a caller rendering a file from these values would loosen that bound, and a test named for the module's default invited exactly that reading. The number now lives once, beside the section, and the command reads it. So a declared value is what reaches an operator's file, and the generated file is unchanged, which the command's own characterization suite confirms. Raising it to the module's default fails a test rather than silently widening a public surface. The module's default is still what a node whose file carries no wasm section resolves. That is a different question, and it is held as one. Three other sections say what their declared values are rather than what a node with nothing written resolves.
tagOf carried its rationale inline, which meant the steps were never named. Each condition that needed explaining is now a predicate whose name says why it is there, and the rationale moved to that predicate's doc comment. The tag excluding a field from configuration and the spelling no written key can match are the two that carried comments. Splitting the tag into a name and the squash option is a step of its own, so the body reads as a sequence rather than parsing in place. Behaviour is unchanged and no test moved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
The registry declares configuration keys and resolves a value for each, and until now nothing declared anything. These are the first four sections: each is registered by the package that owns its struct, so the struct, the values and the keys come from one place and cannot drift apart.
Start with
giga/executor/config/register.go— the shape three of the four use, and the smallest.Properties
seid initdoes write some keys per mode, so where a section has one, the declared default is what an absent key resolves to and the mode-written value arrives as a written value.-as skip this field, and a configuration struct uses it for a field something else assigns: the receipt store'sKeepRecentcomes from the globalmin-retain-blocksflag at the app layer, and itsExternalPruningfrom whatever constructs the garbage collector. Declaring a key that resolved to a default would be worse than refusing the section, because a declared key is written at override precedence, so the default would land over the value that code assigned and a node withmin-retain-blocksset would silently keep nothing. A field with no tag at all stays a defect: that is the opposite intent, a key nothing names reaching no field.wasmneeds one because the upstream type carries no mapstructure tags, so derived keys would be field names rather than the three the module reads. A schema restates a key set, and a second statement is only safe while something holds it against the first, so its test compares against the module's own flag constants and its values against the live default.The sections
admin_serveradmingiga_executorgiga/executor/configreceipt-storesei-db/configwasmsei-wasmd/x/wasmScope
Four of seventeen. The remaining thirteen follow in three more changes, grouped by owning package. Nothing consumes the registry yet, so declaring a section changes no read: the recorded configuration surface does not move and no golden changed.
config/cosmosbaseis deliberately last. It needs two registry capabilitiesmaindoes not carry, one for a section whose keys sit at the root with no prefix and one for a key no environment variable can supply.The tag conditions are named rather than commented
Review asked for the conditions a field tag has to pass to be predicates with names saying why
they are there, rather than conditions behind comments.
tagOfnow reads as a sequence of namedsteps, and the rationale moved onto three of them:
assignedOutsideConfiguration— the dash tag. This absorbed an eight-line inline comment,trimmed to four on its own doc.
neverMatchesAWrittenKey— the lower-case rule, whose reason is that a source enumerateslower-cased.
parseTag— splitting the tag into a name and the squash option, so the body reads as stepsrather than parsing in place.
Comment volume inside
tagOfgoes from eight lines to none. Two conditions stayed as plainexpressions: a predicate around
name == ""adds a hop and no meaning, and the squash pair istwo branches of one decision that reads worse split apart. No test moved, which is the proof
the behaviour did not.
Verified
build,vet,gofmt -s,goimportsclean.config/registryat 100% of statements, race clean. The characterization suite intestutil/configtestpasses unchanged.Three mutations each fail a named test: refusing a
-tag again, skipping an untagged field, and letting the type walk and the value walk disagree about which fields exist.Two review suggestions on the tests were already closed by later commits here, and I checked the property each asked for rather than assuming. The receipt-store test builds its key list from the reader's own constants, so renaming
flagRSBackend's value now fails the suite; before the change it would not have. The wasm gas-limit assertion is symmetric, so a non-nil live limit is compared against its digits rather than skipped.Two declared values the binary also states elsewhere
wasm.query_gas_limitresolves to ten times what the template writes into a generatedapp.toml. Both are correct about different nodes: a node the binary provisioned runs thesmaller number as a written value, and a node whose file predates the section resolves the
declared one. Whoever renders declared values into a file has to decide which survives, and that
limit bounds the work one smart query can ask of a node that serves queries to anyone. Named at
the declaration so the decision cannot be made silently.
receipt-store.db-directoryresolves to an empty string, and the emptiness is the meaning. Theapp layer fills it from the host, keeping the former path for a node that already holds the store
there. A path written into a file is one host's answer, and on another host it names an empty
directory.