CCIP-4420 Merging back CCIP codebase (#15890) #15894
Conversation
|
I see you updated files related to
|
| @@ -145,4 +148,14 @@ func (rf *CommitReportingPluginFactory) NewReportingPluginFn(ctx context.Context | |||
|
|
|||
| return reportingPluginAndInfo{plugin, pluginInfo}, nil | |||
| } | |||
|
|
|||
| return func() (reportingPluginAndInfo, error) { | |||
There was a problem hiding this comment.
this looks odd to me - can you sanity check this @mateusz-sekara
There was a problem hiding this comment.
Just a wrapper to publish prometheus metrics, it was added to the CCIP repo some time ago by @rstout . Looks legit imo
There was a problem hiding this comment.
Understood. I'd suggest refactoring, but not in scope for this
| @@ -159,4 +162,14 @@ func (rf *ExecutionReportingPluginFactory) NewReportingPluginFn(ctx context.Cont | |||
|
|
|||
| return reportingPluginAndInfo{plugin, pluginInfo}, nil | |||
| } | |||
|
|
|||
| return func() (reportingPluginAndInfo, error) { | |||
There was a problem hiding this comment.
same comment here @mateusz-sekara can you sanity check this flow? does this somehow make testing easier?
There was a problem hiding this comment.
we used that for tracking number of failures during boot, it's only a tiny wrapper layer, should be fine
| @@ -468,21 +468,36 @@ func (r *ExecutionReportingPlugin) buildReport(ctx context.Context, lggr logger. | |||
| return encodedReport, nil | |||
| } | |||
|
|
|||
| // Returns required number of observations to reach consensus | |||
| func (r *ExecutionReportingPlugin) getConsensusThreshold() int { | |||
There was a problem hiding this comment.
should this be exported so that unit tests of the reporting plugin can override the consensus thresholds?
There was a problem hiding this comment.
Let's keep it as it is. It was introduced here and copy pasted right now during the merge. I'm assuming code is correct
| @@ -63,7 +64,13 @@ func TypeAndVersion(addr common.Address, client bind.ContractBackend) (ContractT | |||
| return ContractType(contractType), *v, nil | |||
| } | |||
|
|
|||
| // default version to use when TypeAndVersion is missing. | |||
| const defaultVersion = "1.0.0" | |||
There was a problem hiding this comment.
There's a few CCIP contracts in v1.0.0 that don't have a typeAndVersion. This fixes it by assuming they are 1.0.0
There was a problem hiding this comment.
For CCIP case it is 1.0.0. I think this code is dead, because all of the contracts are 1.5 right now. I think it was some sort of sanity check added by EngOps a long time ago. We can safely remove that, but it also doesn't harm
AER Report: CI Core ran successfully ✅ |
| @@ -353,7 +355,8 @@ func (c *CommitStore) GetAcceptedCommitReportsGteTimestamp(ctx context.Context, | |||
| return nil, err | |||
| } | |||
|
|
|||
| reportsQuery, err := logpoller.Where( | |||
| reportsQuery, err := query.Where( | |||
| c.address.String(), | |||
There was a problem hiding this comment.
/nit but should c.address not be common.address b/c we are doing chain specific stuff here anyways?
| func (o *OnRamp) SourcePriceRegistryAddress(ctx context.Context) (cciptypes.Address, error) { | ||
| c, err := o.GetDynamicConfig(ctx) | ||
| if err != nil { | ||
| return "", err |
| offRampV150: offRamp, | ||
| feeEstimatorConfig: feeEstimatorConfig, | ||
| OffRamp: v120, | ||
| offRampV150: offRamp, |
There was a problem hiding this comment.
/nit naming - hard for me to sanity check that everything is wired properly
| @@ -20,6 +20,10 @@ var ( | |||
| Name: "ccip_sequence_number_counter", | |||
| Help: "Sequence number of the last message processed by the plugin", | |||
| }, []string{"plugin", "source", "dest", "ocrPhase"}) | |||
| newReportingPluginErrorCounter = promauto.NewGaugeVec(prometheus.GaugeOpts{ | |||
| Name: "ccip_new_reporting_plugin_error_counter", | |||
There was a problem hiding this comment.
/nit does prefixing metrics with ccip add value in a metrics dashboard where you are separating out ccip specific nodes?
There was a problem hiding this comment.
I think it's easier to grep available metrics on our Prometheus engine so IMO it does add value (on prom you see all metrics reported by all nodes)
| s.usdcReader, | ||
| attestationURI, | ||
| //nolint:gosec // integer overflow | ||
| int(s.usdcConfig.AttestationAPITimeoutSeconds), |
There was a problem hiding this comment.
/nit do we perform some type of sensible validation upstream? either when the job is received or in the delegate?
|
Please see this branch for details #15890
I needed to make it two step in order to squash everything before adding to the merge q