Conversation
| "github.com/smartcontractkit/chainlink-ccv/bootstrap" | ||
| "github.com/smartcontractkit/chainlink-ccv/integration/pkg/accessors/evm" | ||
| "github.com/smartcontractkit/chainlink-ccv/pkg/chainaccess" | ||
| _ "github.com/smartcontractkit/chainlink-ccv/integration/pkg/accessors/evm" // evm accessor driver |
There was a problem hiding this comment.
EVM factory is now loaded like a driver.
|
|
||
| "github.com/smartcontractkit/chainlink-ccv/bootstrap" | ||
| "github.com/smartcontractkit/chainlink-ccv/integration/pkg/accessors/evm" | ||
| _ "github.com/smartcontractkit/chainlink-ccv/integration/pkg/accessors/evm" // evm accessor driver |
There was a problem hiding this comment.
EVM factory is now loaded like a driver.
| type factory[T any] struct { | ||
| type factory struct { |
There was a problem hiding this comment.
Generics no longer required because accessor creation is handled by the registry + chain family driver.
| type tokenVerifierFactory[T any] struct { | ||
| type tokenVerifierFactory struct { |
There was a problem hiding this comment.
Same as committee service factory. Generics no longer needed because it's handle by the registry + chain family driver.
| family, err := chainsel.GetSelectorFamily(uint64(selector)) | ||
| if err != nil { | ||
| lggr.Errorw("Failed to get selector family", "error", err, "selector", selector) | ||
| return fmt.Errorf("failed to get selector family: %w", err) | ||
| } | ||
| if family != f.chainFamily { | ||
| lggr.Warnw("Skipping chain in provided config, doesn't match expected chain family", | ||
| "selector", selector, | ||
| "family", family, | ||
| "expectedFamily", f.chainFamily, | ||
| ) | ||
| continue | ||
| } |
There was a problem hiding this comment.
This is no longer a limitation, so the check is removed. If you want multiple families, you would load multiple drivers. But I don't think we want multiple families in a single binary, so we would only load/configure one family at a time.
b636937 to
404d674
Compare
Pass registry in via bootstrap dependency. Remove unused generics. No more missing key tests. More logs Justfile stuff... Fix tests.
ef376ac to
307f896
Compare
| // GetAccessor is called, it will delegate to the AccessorFactory corresponding to the chain | ||
| // family of the given chain selector. | ||
| type AccessorFactoryConstructor func(lggr logger.Logger, cfg string) (AccessorFactory, error) | ||
| type AccessorFactoryConstructor func(lggr logger.Logger, cfg GenericConfig) (AccessorFactory, error) |
There was a problem hiding this comment.
Changed the constructor to use a stronger type.
|
|
||
| // NewRegistry creates a new Registry with some configuration. | ||
| func NewRegistry(lggr logger.Logger, config string) (AccessorFactory, error) { | ||
| func NewRegistry(lggr logger.Logger, config string) (*Registry, error) { |
There was a problem hiding this comment.
I don't think we need AccessorFactory anymore. That may be removed in a followup.
There was a problem hiding this comment.
Pull request overview
This PR continues the accessor-registry refactor by initializing a chainaccess.Registry in the bootstrap layer and passing it into bootstrap applications via bootstrap.ServiceDeps, removing the need for app-specific accessor factory wiring and generics.
Changes:
- Initialize
chainaccess.Registryinside the bootstrap runner/bootstrapper and expose it to apps throughServiceDeps.Registry. - Remove generic
Infos[T]plumbing in verifier config loading in favor ofInfos[any]and use the injected registry to obtain accessors. - Update the EVM accessor driver constructor to accept pre-parsed
chainaccess.GenericConfigand adjust EVM factory address maps to useprotocol.ChainSelectorkeys.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| verifier/pkg/token/config.go | Drops generic config wrapper and switches blockchain infos to Infos[any]. |
| verifier/pkg/commit/load.go | Updates committee config loader to return Infos[any] (but changes strict-decoding behavior). |
| verifier/pkg/commit/load_test.go | Updates tests to match the new Infos[any] flow and GenericConfig usage. |
| verifier/cmd/token/main.go | Token verifier now relies on deps.Registry and blank-imports the EVM driver. |
| verifier/cmd/servicefactory.go | Committee verifier factory now uses deps.Registry and removes family-specific wiring. |
| verifier/cmd/committee/main.go | Removes deprecated accessor wiring and uses the non-generic service factory. |
| pkg/chainaccess/registry.go | Registry constructors now take GenericConfig; NewRegistry returns *Registry. |
| pkg/chainaccess/test_test.go | Adds a minimal test package file for chainaccess. |
| integration/pkg/accessors/evm/factory_constructor.go | EVM driver constructor takes GenericConfig and logs loaded chain info. |
| integration/pkg/accessors/evm/factory.go | Switches on-ramp/RMN address maps to ChainSelector keys and improves init error reporting. |
| bootstrap/bootstrap.go | Creates a registry per job start and injects it into ServiceDeps. |
| bootstrap/bootstrap_test.go | Updates runner tests to include a test logger and use t.Context(). |
| build/devenv/services/tokenVerifier.go | Adapts devenv token verifier config generation to Infos[any]. |
| build/devenv/fakes/Justfile | Adds --network=host to docker build commands. |
| indexer/Justfile | Adds --network=host to docker build commands and modifies replay build recipe. |
| changelog/2025-04-14_accessor_registry.md | Adds a detailed migration/adoption guide for the registry changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _, err := toml.Decode(cfg, &decodeTarget) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to decode committee verifier config: %w", err) | ||
| } |
There was a problem hiding this comment.
LoadConfigWithBlockchainInfos no longer performs strict TOML decoding (Undecoded field check was removed), but the function comment still claims unknown keys (including under blockchain_infos) cause an error. This change will silently ignore config typos and can lead to misconfigured services; reintroduce strict decoding (e.g., keep the toml.Metadata from toml.Decode and fail if md.Undecoded() is non-empty, or reuse bootstrap.parseTOMLStrict if appropriate).
| _, err := toml.Decode(cfg, &decodeTarget) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to decode committee verifier config: %w", err) | |
| } | |
| md, err := toml.Decode(cfg, &decodeTarget) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("failed to decode committee verifier config: %w", err) | |
| } | |
| if undecoded := md.Undecoded(); len(undecoded) > 0 { | |
| return nil, nil, fmt.Errorf("failed to decode committee verifier config: unknown keys %v", undecoded) | |
| } |
| @@ -47,36 +50,19 @@ UniqueChainName = "chain-1" | |||
| info, ok := infos["1"] | |||
| require.True(t, ok, "blockchain_infos should contain key \"1\"") | |||
| require.NotNil(t, info) | |||
| assert.Equal(t, "1", info.ChainID) | |||
| assert.Equal(t, "evm", info.Type) | |||
| assert.Equal(t, "evm", info.Family) | |||
| assert.Equal(t, "chain-1", info.UniqueChainName) | |||
| } | |||
|
|
|||
| func TestLoadConfigWithBlockchainInfos_UnknownTopLevelKey_ReturnsError(t *testing.T) { | |||
| tomlConfig := ` | |||
| typo_key = "should fail" | |||
| ` | |||
| _, _, err := LoadConfigWithBlockchainInfos[testChainInfo](tomlConfig) | |||
| require.Error(t, err) | |||
| assert.Contains(t, err.Error(), "unknown fields") | |||
| assert.Contains(t, err.Error(), "typo_key") | |||
| } | |||
|
|
|||
| func TestLoadConfigWithBlockchainInfos_UnknownKeyUnderBlockchainInfos_ReturnsError(t *testing.T) { | |||
| tomlConfig := ` | |||
| [blockchain_infos."1"] | |||
| UnknownField = "should fail" | |||
| ` | |||
| _, _, err := LoadConfigWithBlockchainInfos[testChainInfo](tomlConfig) | |||
| require.Error(t, err) | |||
| assert.Contains(t, err.Error(), "unknown fields") | |||
| assert.Contains(t, err.Error(), "blockchain_infos") | |||
| gcfg := chainaccess.GenericConfig{ChainConfig: infos} | |||
| var tinfo testChainInfo | |||
| require.NoError(t, gcfg.GetConcreteConfig(protocol.ChainSelector(1), &tinfo)) | |||
| assert.Equal(t, "1", tinfo.ChainID) | |||
| assert.Equal(t, "evm", tinfo.Type) | |||
| assert.Equal(t, "evm", tinfo.Family) | |||
| assert.Equal(t, "chain-1", tinfo.UniqueChainName) | |||
There was a problem hiding this comment.
The tests for LoadConfigWithBlockchainInfos no longer validate the documented “strict decode” behavior (unknown top-level keys / unknown keys under blockchain_infos). Without these assertions, config typos can slip through unnoticed; consider restoring explicit tests that unknown fields produce an error once strict decoding is re-enabled.
| } | ||
| if len(md.Undecoded()) > 0 { | ||
| return nil, nil, fmt.Errorf("unknown fields in committee verifier config: %v", md.Undecoded()) | ||
| } |
There was a problem hiding this comment.
Any reason we rm'd the strict decode? It has caused errors in the past where we silently unmarshal into a struct that ends up having empty fields.
There was a problem hiding this comment.
I removed it because the config now has any fields for the blockchain config. I think all of these load functions can go away in a followup because the app no longer needs the family specific RPC config.
There was a problem hiding this comment.
I can work on this as a followup today.
Co-authored-by: Makram <makramkd@users.noreply.github.com>
|
Code coverage report:
|
| @@ -59,49 +58,32 @@ func chainSelectorsFromMap[T any](m chainaccess.Infos[T]) []protocol.ChainSelect | |||
| // NOTE: this factory supports only a single chain family at a time. | |||
There was a problem hiding this comment.
we can update the comment now that it's no longer generic
| } | ||
|
|
||
| // NewCommitteeVerifierServiceFactory creates a new ServiceFactory for the committee verifier service. | ||
| // T is the chain config type for this family (e.g. blockchain.Info for EVM). |
Followup to #1002. Use the new initialization path for the registry and pass it in to apps via the bootstrapper.
Includes a changelog to explain the major changes in #1002, #1007, #1005 and #1022.