CCIP-4573 Changesets ccip 1.5 #15588
Conversation
AER Report: CI Coreaer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , Flakeguard Root Project / Get Tests To Run , test-scripts , Flakeguard Deployment Project / Get Tests To Run , GolangCI Lint (integration-tests) , GolangCI Lint (deployment) , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_fuzz) , Core Tests (go_core_race_tests) , Flakeguard Root Project / Run Tests , Flakeguard Root Project / Report , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/ccip/changeset, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/ccip/changeset/v1_5, ubuntu-latest) , Flakeguard Deployment Project / Run Tests (github.com/smartcontractkit/chainlink/deployment/common/changeset/internal, ubuntu-lat... , lint , SonarQube Scan , Flakey Test Detection , Flakeguard Deployment Project / Report 1. Consider pre-allocating slices:
Source of Error:var prereqCfg []DeployPrerequisiteConfigPerChain
var prereqCfgs []DeployPrerequisiteConfigPerChain
var schedule []int
var oracles []confighelper.OracleIdentityExtra
var signersAddresses []common.Address
var transmittersAddresses []common.Address
var jobSpecs []JobSpecInput
var addLanesCfg []DeployLaneConfig
var commitOCR2Configs []CommitOCR2ConfigParams
var execOCR2Configs []ExecuteOCR2ConfigParamsWhy: The linter suggests pre-allocating slices to improve performance by reducing the number of allocations. Suggested fix: Pre-allocate the slices with an appropriate capacity using 2. Variable naming issues:
Source of Error:func WithChainIds(chainIDs []uint64) TestOps {
package v1_5
PriceGetterConfigJson string
destEVMChainIdStr, err := chain_selectors.GetChainIDFromSelector(dest)
destEVMChainId, err := strconv.ParseUint(destEVMChainIdStr, 10, 64)Why: The linter suggests using consistent and idiomatic naming conventions, such as using Suggested fix: Rename variables and package names to follow Go naming conventions, e.g., 3. Performance improvements:
Source of Error:return fmt.Sprintf("%d", c.Selector)
name = fmt.Sprintf("%d", chainSelector)
return fmt.Errorf("TokenPricesUSDPipeline or PriceGetterConfigJson is required")
return fmt.Errorf("USDCAttestationAPI is required")Why: The linter suggests using more efficient functions like Suggested fix: Replace 4. Avoid using
|
| Chains map[uint64]CCIPChainState | ||
| } | ||
|
|
||
| // CCIPChainState holds a Go binding for all the currently deployed CCIP contracts |
There was a problem hiding this comment.
I think it'll be simpler if we have a single monster CCIPChainState, but embed the legacy one like
type CCIPChainState1_5 struct {
// Only 15 contracts which will be fully deprecated
EVM2EVMOnRamp map[uint64]*evm_2_evm_onramp.EVM2EVMOnRamp
...
}
// Existing thing
type CCIPChainState struct {
commoncs.MCMSWithTimelockState
commoncs.LinkTokenState
commoncs.StaticLinkTokenState
CCIPChainState1_5
OnRamp onramp.OnRamp
...
}
Since ultimately they'll be in the same address book. Then we can still just LoadCCIPChainState in a changeset, and take actions on the old contracts and the new ones if needed for migration
| "github.com/smartcontractkit/chainlink/v2/core/gethwrappers/ccip/generated/rmn_contract" | ||
| ) | ||
|
|
||
| // CCIPChainStateLegacy holds 1.5 state for CCIP chains |
There was a problem hiding this comment.
lets clarify it holds it for a single chain
|
|
||
| // CCIPChainStateLegacy holds 1.5 state for CCIP chains | ||
| type CCIPChainStateLegacy struct { | ||
| EVM2EVMOnRamp map[uint64]*evm_2_evm_onramp.EVM2EVMOnRamp |
There was a problem hiding this comment.
need to document what the keys are here
| var _ deployment.ChangeSet[DeployChainContractsConfig] = DeployChainContracts | ||
|
|
||
| type DeployChainContractsConfig struct { | ||
| configs []DeployChainContractsConfigPerChain |
There was a problem hiding this comment.
will be unusable if private
| return nil | ||
| } | ||
|
|
||
| func deployChainContractsForChains(env deployment.Environment, ab deployment.AddressBook, cfg DeployChainContractsConfig) error { |
There was a problem hiding this comment.
could probably just inline this
| // │ Deploy RMN │ | ||
| // ================================================================ | ||
| var rmnAddr common.Address | ||
| if cfg.RMNConfig == nil { |
There was a problem hiding this comment.
might be simpler to just add the price reg + real RMN to prereqs?
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
|
I see you updated files related to
|
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) | ||
| defer cancel() | ||
| receipt, err := bind.WaitMined(ctx, backend, tx) |
There was a problem hiding this comment.
The linter normally complains about defers from for loops since they can be error prone. An alternative form is to wrap it up in a func:
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) | |
| defer cancel() | |
| receipt, err := bind.WaitMined(ctx, backend, tx) | |
| receipt, err := func() (receipt, error) { | |
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) | |
| defer cancel() | |
| return bind.WaitMined(ctx, backend, tx) | |
| } |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
Flakeguard SummaryRan new or updated tests between View Flaky Detector Details | Compare Changes Found Flaky Tests ❌
ArtifactsFor detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json. |
| @@ -62,6 +70,7 @@ func (o *OCR2TaskJobSpec) String() (string, error) { | |||
| }{ | |||
| Name: o.Name, | |||
| JobType: o.JobType, | |||
| ExternalJobID: externalID, | |||
There was a problem hiding this comment.
This is required for JD to propose the job, without ExternalJobID it's throwing error
| @@ -28,6 +30,7 @@ type OCR2TaskJobSpec struct { | |||
| ForwardingAllowed bool `toml:"forwardingAllowed"` | |||
| OCR2OracleSpec job.OCR2OracleSpec | |||
| ObservationSource string `toml:"observationSource"` // List of commands for the Chainlink node | |||
| ExternalJobID string `toml:"externalJobID"` | |||
There was a problem hiding this comment.
is this exclusively for tests / JD? So no need to port it to the fork and it won't break the fork if it does get ported?
There was a problem hiding this comment.
it won't break it. Exclusively for JD. without externaljobId it's throwing error that it's missing, which does not happen through node api job creation
deployment/ccip/changeset/state.go
Outdated
| for _, commitStore := range c.CommitStore { | ||
| commitStoreView, err := v1_5.GenerateCommitStoreView(commitStore) | ||
| if err != nil { | ||
| return chainView, errors.Wrapf(err, "failed to generate commit store view for commit store %s", commitStore.Address().String()) |
There was a problem hiding this comment.
can include the source like the offramp
| @@ -312,7 +364,11 @@ func (s CCIPOnChainState) View(chains []uint64) (map[string]view.ChainView, erro | |||
| if err != nil { | |||
| return m, err | |||
| } | |||
| m[chainInfo.ChainName] = chainView | |||
| name := chainInfo.ChainName | |||
| if chainInfo.ChainName == "" { | |||
There was a problem hiding this comment.
should never happen right?
There was a problem hiding this comment.
happens for memory env, all the 900.... ids have blank name
| destStartBlock, err := destChain.Client.HeaderByNumber(context.Background(), nil) | ||
| require.NoError(t, err) | ||
| WaitForCommit(t, srcChain, destChain, state.Chains[dest].CommitStore[src], sentEvent.Message.SequenceNumber) | ||
| WaitForExecute(t, srcChain, destChain, state.Chains[dest].EVM2EVMOffRamp[src], []uint64{sentEvent.Message.SequenceNumber}, destStartBlock.Number.Uint64()) |
| *memory.JobClient | ||
| } | ||
|
|
||
| // ProposeJob is an overridden implementation of the jobclient.ProposeJob method which allows offchainreporting2 job type |
There was a problem hiding this comment.
wait the JD doesn't support this?
There was a problem hiding this comment.
the jobclient currently only supports ccip - https://github.com/smartcontractkit/chainlink/blob/develop/deployment/environment/memory/job_client.go#L296
| }}, nil | ||
| } | ||
|
|
||
| type MemoryEnvironment struct { |
There was a problem hiding this comment.
I think the overriding will be confusing for the migration test involving 1.5 and 1.6 - can we not have a constructor/flag/config in memory.NewEnvironment which ensures there's a 1337 chain available?
|
Requires
Supports