Removing chain dependencies from NewCommitServices construct #1361
Conversation
This file contains 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
…ed out ContractReader config
…rds using ChainReader
dimkouv
reviewed
Sep 24, 2024
This reverts commit b323a8b.
This PR tests https://github.com/smartcontractkit/ccip/pull/1361/files without changes made to integration-tests/docker/test_env/cl_node.go --------- Co-authored-by: Patrick <[email protected]>
asoliman92
reviewed
Sep 26, 2024
asoliman92
approved these changes
Sep 26, 2024
winder
reviewed
Sep 26, 2024
Comment on lines
+1619
to
+1687
| var priceGetter ccip.AllTokensPriceGetter | ||
| withPipeline := strings.Trim(pluginJobSpecConfig.TokenPricesUSDPipeline, "\n\t ") != "" | ||
| if withPipeline { | ||
| priceGetter, err = ccip.NewPipelineGetter(pluginJobSpecConfig.TokenPricesUSDPipeline, d.pipelineRunner, jb.ID, jb.ExternalJobID, jb.Name.ValueOrZero(), lggr) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating pipeline price getter: %w", err) | ||
| } | ||
| } else { | ||
| // Use dynamic price getter. | ||
| if pluginJobSpecConfig.PriceGetterConfig == nil { | ||
| return nil, fmt.Errorf("priceGetterConfig is nil") | ||
| } | ||
|
|
||
| // Configure contract readers for all chains specified in the aggregator configurations. | ||
| // Some lanes (e.g. Wemix/Kroma) requires other clients than source and destination, since they use feeds from other chains. | ||
| aggregatorChainsToContracts := make(map[uint64][]common.Address) | ||
| for _, aggCfg := range pluginJobSpecConfig.PriceGetterConfig.AggregatorPrices { | ||
| if _, ok := aggregatorChainsToContracts[aggCfg.ChainID]; !ok { | ||
| aggregatorChainsToContracts[aggCfg.ChainID] = make([]common.Address, 0) | ||
| } | ||
|
|
||
| aggregatorChainsToContracts[aggCfg.ChainID] = append(aggregatorChainsToContracts[aggCfg.ChainID], aggCfg.AggregatorContractAddress) | ||
| } | ||
|
|
||
| contractReaders := map[uint64]types.ContractReader{} | ||
|
|
||
| for chainID, aggregatorContracts := range aggregatorChainsToContracts { | ||
| relayID := types.RelayID{Network: spec.Relay, ChainID: strconv.FormatUint(chainID, 10)} | ||
| relay, rerr := d.RelayGetter.Get(relayID) | ||
| if rerr != nil { | ||
| return nil, fmt.Errorf("get relay by id=%v: %w", relayID, err) | ||
| } | ||
|
|
||
| contractsConfig := make(map[string]evmrelaytypes.ChainContractReader, len(aggregatorContracts)) | ||
| for i := range aggregatorContracts { | ||
| contractsConfig[fmt.Sprintf("%v_%v", ccip.OFFCHAIN_AGGREGATOR, i)] = evmrelaytypes.ChainContractReader{ | ||
| ContractABI: ccip.OffChainAggregatorABI, | ||
| Configs: map[string]*evmrelaytypes.ChainReaderDefinition{ | ||
| "decimals": { // CR consumers choose an alias | ||
| ChainSpecificName: "decimals", | ||
| }, | ||
| "latestRoundData": { | ||
| ChainSpecificName: "latestRoundData", | ||
| }, | ||
| }, | ||
| } | ||
| } | ||
| contractReaderConfig := evmrelaytypes.ChainReaderConfig{ | ||
| Contracts: contractsConfig, | ||
| } | ||
|
|
||
| contractReaderConfigJsonBytes, jerr := json.Marshal(contractReaderConfig) | ||
| if jerr != nil { | ||
| return nil, fmt.Errorf("marshal contract reader config: %w", jerr) | ||
| } | ||
|
|
||
| contractReader, cerr := relay.NewContractReader(ctx, contractReaderConfigJsonBytes) | ||
| if cerr != nil { | ||
| return nil, fmt.Errorf("new ccip commit contract reader %w", cerr) | ||
| } | ||
|
|
||
| contractReaders[chainID] = contractReader | ||
| } | ||
|
|
||
| priceGetter, err = ccip.NewDynamicPriceGetter(*pluginJobSpecConfig.PriceGetterConfig, contractReaders) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("creating dynamic price getter: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be nice to wrap this in a function similar to d.ccipCommitGetDstProvider and d.ccipCommitGetSrcProvider above.
There was a problem hiding this comment.
winder
reviewed
Sep 26, 2024
Comment on lines
+23
to
+25
| const OFFCHAIN_AGGREGATOR = "OffchainAggregator" | ||
| const DECIMALS_METHOD_NAME = "decimals" | ||
| const LATEST_ROUND_DATA_METHOD_NAME = "latestRoundData" |
There was a problem hiding this comment.
All caps is not idiomatic (stdlib uses UpperCamelCase, i.e. math.MaxUint64). They were also private before. So two requests:
- Can it be private?
- Please rename to
DecimalsMethodNameordecimalsMethodName
There was a problem hiding this comment.
These specific vars are not exported anywhere else, since they live in a package that is defined behind in a leaf of aninternal path. Will rename to UpperCamelCase, thanks for the stdlib callout.
There was a problem hiding this comment.
winder
reviewed
Sep 26, 2024
Comment on lines
+51
to
+53
| .PHONY: install-chainlink-delve | ||
| install-chainlink-delve: operator-ui ## Install the chainlink binary. | ||
| go install $(GOFLAGS) -gcflags "all=-N -l" . |
There was a problem hiding this comment.
What is this? It doesn't seem to have anything to do with delve
There was a problem hiding this comment.
The -gcflags "all=-N -l" portion of this compiles the binary in a way that allows a debugger to step through.
winder
approved these changes
Sep 26, 2024
skudasov
approved these changes
Sep 27, 2024
|
valerii-kabisov-cll
pushed a commit
that referenced
this pull request
Oct 1, 2024
## Motivation LOOP-ify ## Solution --------- Co-authored-by: Bartek Tofel <[email protected]> Co-authored-by: lukaszcl <[email protected]>
patrickhuie19
added a commit
that referenced
this pull request
Oct 2, 2024
## Motivation Quick follow ups to #1361
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
LOOP-ify
Solution