Add changelog directory and a document about the registry changes.#1021
Add changelog directory and a document about the registry changes.#1021
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new changelog document describing how to adopt the accessor registry pattern (chainaccess.NewRegistry + blank-import registration) introduced in PRs #1002, #1005, and #1007.
Changes:
- Introduces
changelog/documentation capturing the accessor registry architecture and migration steps. - Documents
chainaccess.Registerauto-registration,bootstrap.JobSpecenvelope, andchainaccess.NewRegistrydispatch behavior. - Provides before/after examples and guidance for adding new chain families and running without JD.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (f *myFactory) Start(ctx context.Context, spec bootstrap.JobSpec, deps bootstrap.ServiceDeps) error { | ||
| lggr := logger.Sugared(logger.Named(deps.Logger, "MyService")) | ||
|
|
||
| // 1. Parse the application's own config (strict decode). | ||
| // blockchain_infos must still be present in spec.AppConfig. | ||
| config, _, err := commit.LoadConfigWithBlockchainInfos[any](spec.AppConfig) |
There was a problem hiding this comment.
The comment says blockchain_infos must still be present, but commit.LoadConfigWithBlockchainInfos does not require it to be present (it succeeds with an empty config). The actual constraint is that if your config does include blockchain_infos and you use strict decoding, your decode target must include that field so it isn’t flagged as unknown.
| func (f *myFactory) Start(ctx context.Context, spec bootstrap.JobSpec, deps bootstrap.ServiceDeps) error { | |
| lggr := logger.Sugared(logger.Named(deps.Logger, "MyService")) | |
| // 1. Parse the application's own config (strict decode). | |
| // blockchain_infos must still be present in spec.AppConfig. | |
| config, _, err := commit.LoadConfigWithBlockchainInfos[any](spec.AppConfig) | |
| type myConfig struct { | |
| OnRampAddresses map[string]string `toml:"on_ramp_addresses"` | |
| // Include blockchain_infos in the strict-decode target so configs that | |
| // contain it are accepted instead of being rejected as unknown fields. | |
| BlockchainInfos any `toml:"blockchain_infos"` | |
| } | |
| func (f *myFactory) Start(ctx context.Context, spec bootstrap.JobSpec, deps bootstrap.ServiceDeps) error { | |
| lggr := logger.Sugared(logger.Named(deps.Logger, "MyService")) | |
| // 1. Parse the application's own config (strict decode). | |
| // If spec.AppConfig contains blockchain_infos, your decode target must | |
| // include that field so strict decoding does not reject it as unknown. | |
| config, _, err := commit.LoadConfigWithBlockchainInfos[myConfig](spec.AppConfig) |
| // 3. Get an Accessor for each chain you want to read. | ||
| sourceReaders := make(map[protocol.ChainSelector]chainaccess.SourceReader) | ||
| for selector := range config.OnRampAddresses { | ||
| sel, _ := strconv.ParseUint(selector, 10, 64) |
There was a problem hiding this comment.
The example ignores the error from strconv.ParseUint. If a config contains a malformed selector key, this will silently produce 0 and potentially fetch the wrong accessor; since this is likely to be copy/pasted, it should either handle the error (log + continue) or explicitly mention that selector keys are guaranteed numeric.
| sel, _ := strconv.ParseUint(selector, 10, 64) | |
| sel, err := strconv.ParseUint(selector, 10, 64) | |
| if err != nil { | |
| lggr.Errorw("Invalid chain selector in config", "selector", selector, "error", err) | |
| continue | |
| } |
|
|
||
| sourceReaders := make(map[protocol.ChainSelector]chainaccess.SourceReader) | ||
| for strSel := range config.OnRampAddresses { | ||
| sel, _ := strconv.ParseUint(strSel, 10, 64) |
There was a problem hiding this comment.
Same issue as above: ignoring strconv.ParseUint errors in this example can lead to selector 0 and misleading behavior when copy/pasted. Recommend handling the parse error (warn + continue) or stating the invariants that make it safe to ignore.
| sel, _ := strconv.ParseUint(strSel, 10, 64) | |
| sel, err := strconv.ParseUint(strSel, 10, 64) | |
| if err != nil { | |
| lggr.Warnw("Skipping chain — invalid selector", "selector", strSel, "error", err) | |
| continue | |
| } |
| # Adopting `chainaccess.Register` in Bootstrap Applications | ||
|
|
||
| > **Scope of this guide**: PRs #1002 (`chainaccess.Register` registry), #1005 (registry integrated into bootstrap), and #1007 (`bootstrap.JobSpec` as standard config envelope). |
There was a problem hiding this comment.
The doc frames the migration as adopting chainaccess.Register, but application code typically doesn’t call Register directly; it blank-imports family packages (which call Register in init()) and then uses chainaccess.NewRegistry / chainaccess.Registry. Renaming this section (and the scope line) to refer to the registry pattern (chainaccess.Registry / chainaccess.NewRegistry) would better match the actual API surface consumers interact with.
| # Adopting `chainaccess.Register` in Bootstrap Applications | |
| > **Scope of this guide**: PRs #1002 (`chainaccess.Register` registry), #1005 (registry integrated into bootstrap), and #1007 (`bootstrap.JobSpec` as standard config envelope). | |
| # Adopting `chainaccess.Registry` / `chainaccess.NewRegistry` in Bootstrap Applications | |
| > **Scope of this guide**: PRs #1002 (`chainaccess.Registry` / `chainaccess.NewRegistry` pattern), #1005 (registry integrated into bootstrap), and #1007 (`bootstrap.JobSpec` as standard config envelope). |
| Family constructors decode only the keys they care about (using a non-strict TOML decode), so your application-level keys (`verifier_id`, `aggregator_address`, etc.) are silently ignored by the registry. | ||
| Conversely, `blockchain_infos` and the shared addresses are ignored by your application-level strict decode. | ||
|
|
||
| > **Important:** when using `commit.LoadConfigWithBlockchainInfos` (which does a **strict** decode), pass `spec.AppConfig` as the string and keep `blockchain_infos` present — the strict decoder must see it or it will error on unknown keys. Use `LoadConfigWithBlockchainInfos[any]` if you no longer need a typed `T` on the infos. |
There was a problem hiding this comment.
This guidance about strict decoding is backwards: commit.LoadConfigWithBlockchainInfos will not error if blockchain_infos is absent (see verifier/pkg/commit/load_test.go), and the real requirement for strict decoding is that the decode target includes blockchain_infos so it isn’t treated as an unknown field when present. Consider rewording to: keep blockchain_infos in the config if your app config includes it, and when strict-decoding app config ensure your decode target accounts for blockchain_infos (or use LoadConfigWithBlockchainInfos[any] and ignore the returned infos).
| > **Important:** when using `commit.LoadConfigWithBlockchainInfos` (which does a **strict** decode), pass `spec.AppConfig` as the string and keep `blockchain_infos` present — the strict decoder must see it or it will error on unknown keys. Use `LoadConfigWithBlockchainInfos[any]` if you no longer need a typed `T` on the infos. | |
| > **Important:** when using `commit.LoadConfigWithBlockchainInfos` (which does a **strict** decode), pass `spec.AppConfig` as the string. Keep `blockchain_infos` in the config if your app config includes it, and make sure your strict-decode target accounts for `blockchain_infos` so it is not treated as an unknown field when present. Use `LoadConfigWithBlockchainInfos[any]` if you no longer need a typed `T` on the infos and just want to ignore the returned infos. |
Co-authored-by: Makram <makramkd@users.noreply.github.com>
Co-authored-by: Makram <makramkd@users.noreply.github.com>
|
Code coverage report:
|
|
moved to #1005 |
Add some documentation for the accessor registry pattern added in
#1002
#1007
#1005