integration: source reader instantiation via accessors #598
Conversation
cmd/verifier/common.go
Outdated
| reg.Register(chainsel.FamilyEVM, sourcereader.NewEVMFactory(lggr, helper, config.OnRampAddresses, config.RMNRemoteAddresses)) | ||
| reg.Register(chainsel.FamilyCanton, canton.NewFactory(lggr, helper, config.CantonConfigs)) |
There was a problem hiding this comment.
Nice, this becomes the only place where non-evms hook their implementation into the service.
It could be a little better with a driver pattern where the config/selector are one big JSON string.
That would let you skip calling reg.Register, but you'd have to pass the config in like reg.GetSourceReader(ctx, srConfig[selector])
There was a problem hiding this comment.
I debated internally the driver pattern vs. this explicit registration but I found the explicit to be better due to the dep injection. In general I think I'm preferring an explicit vs. implicit approach, the big JSON string reminds me too much of ChainReader 😛
There was a problem hiding this comment.
The benefit is when you need to setup the registry for tests. With this approach you'll need this block of code in several places. I think you still have the same properties regarding dep injection, it's also still explicit because you still need _ integrations/canton to trigger the driver to register itself.
Hopefully accessor configs don't become a full on meta programming exercise, but I'm sort of imagining a directory full of config files. From there tests / apps can select the right directory and initialize their accessors dynamically (or explicitly if they want to define the config inline).
| reader, err := reg.GetSourceReader(ctx, selector) | ||
| if err != nil { | ||
| lggr.Errorw("Failed to create EVM source reader", "selector", selector, "error", err) | ||
| lggr.Errorw("❌ Failed to create source reader", "chainSelector", selector, "error", err) |
There was a problem hiding this comment.
| lggr.Errorw("❌ Failed to create source reader", "chainSelector", selector, "error", err) | |
| lggr.Errorw("Failed to create source reader", "chainSelector", selector, "error", err) |
There was a problem hiding this comment.
I actually added these emojis, found it easier to follow along in the logs, but can remove them if thats preferred / consistency.
There was a problem hiding this comment.
I wonder if emoji's are going to be the new "I think you forgot to remove a printf"
There was a problem hiding this comment.
I actually added these emojis, found it easier to follow along in the logs, but can remove them if thats preferred / consistency.
My personal preferences is that I like having them as they make debugging much easier. One thing that was mentioned before is that not all NOPs allow emojis so it'll show as unicode which is even worse. Probably we can ask NOPs to allow it though.
makramkd
changed the title
|
|
||
| cmd.StartPyroscope(lggr, config.PyroscopeURL, "verifier") | ||
| blockchainHelper, chainClients := cmd.LoadBlockchainInfo(ctx, lggr, blockchainInfos) | ||
| blockchainHelper := cmd.LoadBlockchainInfo(ctx, lggr, blockchainInfos) |
There was a problem hiding this comment.
I know I named the original struct `BlockChainHelpere but I don't like that name 😂
| func RegisterEVM(ctx context.Context, registry *accessors.Registry, lggr logger.Logger, helper *blockchain.Helper, onRampAddresses, rmnRemoteAddresses map[string]string) { | ||
| // Create the chain clients then the head trackers | ||
| chainClients := make(map[protocol.ChainSelector]client.Client) | ||
| for _, selector := range helper.GetAllChainSelectors() { |
There was a problem hiding this comment.
shouldn't' this check for EVM family selectors only?
| return nil, fmt.Errorf("RMN Remote address is not set for chain %d", chainSelector) | ||
| } | ||
|
|
||
| // Create chain client |
| return nil, fmt.Errorf("chain client is not set for chain %d", chainSelector) | ||
| } | ||
|
|
||
| // Create head tracker wrapper |
|
Code coverage report:
WARNING: go tool cover failed for coverage_target.out |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
Summary
This PR refactors the SourceReader creation logic to be chain-agnostic using a factory pattern, enabling support for non-EVM chains like Canton.
Changes
BlockchainInfoout ofprotocoland intointegration/pkg/blockchainAccessorinterface: IntroducedAccessorinterface in pkg/chainaccess. The idea behind this is to serve as a one-stop shop for all the blockchain-specific reader(s) that we have defined across the codebase. For now it only creates thechainaccess.SourceReaderobject, but should be easily extensible in the future to support new types, likeexecutor.DestinationReader.AccessorFactory Pattern: IntroducedAccessorFactoryinterface andRegistrytypes to decouple accessor instantiation from the main application logic.Next Steps
NewVerificationCoordinatorto takemap[protocol.ChainSelector]chainaccess.SourceReaderinstead ofmap[protocol.ChainSelector]legacyevm.Chain, assuming this approach will also be used in the Chainlink node to construct the source readers.