Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deployment/ccip/changeset: mcms optional ccip home cses #15658

Merged
merged 6 commits into from
Dec 13, 2024

Conversation

makramkd
Copy link
Contributor

Add MCMS optionality for the rest of the CCIPHome changesets, and organize things a little bit better by moving the AddDON changeset into cs_ccip_home.go out of cs_add_chain.go

Requires

Supports

Add MCMS optionality for the rest of the CCIPHome changesets, and
organize things a little bit better by moving the AddDON changeset into
cs_ccip_home.go out of cs_add_chain.go
Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and 4d372b4 (mk/part2/ccip-4524).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestAddChainInbound 0.00% false false false 3 0 3 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset false 47.55s @smartcontractkit/ccip, @smartcontractkit/core

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

Copy link
Contributor

github-actions bot commented Dec 12, 2024

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@makramkd makramkd marked this pull request as ready for review December 12, 2024 15:19
@makramkd makramkd requested review from a team as code owners December 12, 2024 15:19
Copy link
Contributor

Flakeguard Summary

Ran new or updated tests between develop and 9492c9b (mk/part2/ccip-4524).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

Name Pass Ratio Panicked? Timed Out? Race? Runs Successes Failures Skips Package Package Panicked? Avg Duration Code Owners
TestInitialAddChainAppliedTwice 66.67% false false false 3 2 1 0 github.com/smartcontractkit/chainlink/deployment/ccip/changeset false 1m59.44s @smartcontractkit/ccip, @smartcontractkit/core

Artifacts

For detailed logs of the failed tests, please refer to the artifact failed-test-results-with-logs.json.

deployment/ccip/changeset/cs_ccip_home.go Show resolved Hide resolved
deployment/ccip/changeset/cs_ccip_home.go Outdated Show resolved Hide resolved

PluginType types.PluginType
NodeIDs []string
CCIPOCRParams CCIPOCRParams
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little awkward where pluginType will determine which field in CCIPOCRParams is used. Probably won't be super easy to refactor but lets add a clear comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought when I was writing this, was considering just passing in the full OCR3Config instead of constructing it within the CS. WDYT?

deployment/ccip/changeset/cs_ccip_home.go Show resolved Hide resolved
return nil, fmt.Errorf("get node info: %w", err)
}

// TODO: validate token config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also gas config

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be easier to track if we keep mentioning tickets for todos

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will create tickets

return deployment.ChangesetOutput{}, fmt.Errorf("missing commit plugin in ocr3Configs")
}

expectedDonID := latestDon.Id + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to find a way to prevent running a AddDonAndSetCandidateChangeset until the previous one is fully executed otherwise you can have 2 with the same expectedDonID. For now lets add a super loud comment on the CS definition

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we include a parameter for expected donID ? we can include the internal.LatestCCIPDON in view to get the value externally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we'll need to find a way to prevent running a AddDonAndSetCandidateChangeset until the previous one is fully executed otherwise you can have 2 with the same expectedDonID

Yeah, had a similar thought, not sure of the best way to make this bullet proof.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets address in a follow up

deployment/ccip/changeset/cs_ccip_home.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AnieeG AnieeG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generic question for all proposal driven changesets -
If there is any dependent precursor proposal, would be great to have a check whether that's executed first. If there is no way, we can update godoc

return deployment.ChangesetOutput{}, fmt.Errorf("missing commit plugin in ocr3Configs")
}

expectedDonID := latestDon.Id + 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we include a parameter for expected donID ? we can include the internal.LatestCCIPDON in view to get the value externally

@makramkd makramkd enabled auto-merge December 12, 2024 23:49
@makramkd makramkd added this pull request to the merge queue Dec 12, 2024
Merged via the queue into develop with commit 7a5e54c Dec 13, 2024
190 checks passed
@makramkd makramkd deleted the mk/part2/ccip-4524 branch December 13, 2024 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants