feat: pointer-based TOML config with proper default merging#385
Open
feat: pointer-based TOML config with proper default merging#385
Conversation
Contributor
|
👋 Fletch153, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
Fletch153
force-pushed
the
NONEVM-810/merge-toml-defaults
branch
from
7b93dbd to
b360ac9
Compare
Fletch153
force-pushed
the
NONEVM-810/merge-toml-defaults
branch
6 times, most recently
from
b0a5abf to
61659e9
Compare
Fletch153
changed the title
Merge the dual-struct pattern (value-based Config + pointer-based TOMLConfig) into a single Config struct per package with pointer fields and TOML tags. This eliminates type duplication while preserving correct nil-vs-zero semantics for TOML deserialization. Each Config has a Resolve() method that fills nil fields from defaults, creating new pointer copies to prevent aliasing with global DefaultConfigSet. - txm: 11 fields, all pointer-based - logpoller: 6 fields (3 Duration, 2 uint64, 1 bool) - write_target: 3 fields (1 string, 2 Duration) - monitor: 1 field (Duration) - config: delete rawTOMLConfig, decode directly into TOMLConfig, add applyDefaults() that calls Resolve() on each section
- Trim verbose boilerplate comments on Config structs and Resolve methods - Remove TEST-NN labels and restating comments from config_test.go - Remove duplicate default-checking tests (EdgeCases_ZeroDefaults, FullySpecified BalanceMonitor, defaults halves of TOML round-trip tests) - Simplify config_test.go to spot-check orchestration (one field per section) - Move exhaustive per-field Resolve assertions into sub-package test files: txm/config_test.go, logpoller/config_test.go, write_target/config_test.go, monitor/balance_config_test.go
config_test.go now only tests the TOML pipeline (decode, applyDefaults, validate). Per-package Resolve() behavior (all defaults, partial override, explicit zero) is tested in each sub-package's config_test.go.
Fletch153
force-pushed
the
NONEVM-810/merge-toml-defaults
branch
from
59cd4e5 to
ff91d58
Compare
- Dereference *TxExpirationSecs in aptos_service.go duration conversion - Remove erroneous dereference of MustNewDuration (already returns pointer) in balance test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Problem
When a user specifies an Aptos.TransactionManager section with only BroadcastChanSize = 50, the TOML decoder creates a non-nil config. The old SetDefaults only checked for nil section pointers, so it skipped the section entirely -- leaving all other fields at Go zero values. With value-typed fields, there was no way to distinguish a field the user did not set from one the user explicitly set to zero.
Solution
Pointer-based fields are nil when absent from TOML, and non-nil when explicitly set to zero. Each packages Resolve checks if a field is nil -- correctly distinguishing absent fields from explicit zero values. This aligns with Chainlink cores config pattern.
Test plan
config/config_test.go -- TOML pipeline integration:
Sub-package Resolve unit tests for txm, logpoller, write_target, monitor: