Ccip-4158 - Initial Add Chain changeset #15349

AnieeG · 2024-11-20T22:59:06Z

It addresses the InitialAddChain part in ticket CCIP-4158
- Creates a changeset to AddChainConfig + AddDON (candidate->primary promotion i.e. init) on the home chain, SetOCR3Config on the remote chain
Moves USDC contract deployment to DeployPrerequisites as these contracts are separate from ccip.
Creates ChangesetApplication.Apply so that multiple changesets can be applied from test as it would be followed in real deployment.
Moves SaveExistingChangeset to common/changeset so that it can be used by all
Makes all contract deployment across different chains parallel.
Creates separate go routine for funding of nodes in different chains
Updates few function to return error instead of relying on require.NoError

…develop

…ccip-4308-optimize-docker-tests

…t/chainlink into ccip-4158-add-chain

…ccip-4158-add-chain

makramkd · 2024-11-21T09:26:44Z

deployment/ccip/changeset/deploy.go

 }

 // DeployPrerequisiteContracts deploys the contracts that can be ported from previous CCIP version to the new one.
 // This is only required for staging and test environments where the contracts are not already deployed.
-func DeployPrerequisiteContracts(e deployment.Environment, ab deployment.AddressBook, state CCIPOnChainState, chain deployment.Chain) error {
+func DeployPrerequisiteContracts(e deployment.Environment, ab deployment.AddressBook, state CCIPOnChainState, chain deployment.Chain, usdcEnabled bool) error {


Can we make this function take a variadic or something at the end? I assume I can do something similar to this in the multicall3 PR

type Options struct { USDCEnabled bool Multicall3Enabled bool } type DeployOpt func(o *Options) func WithUSDC(enabled bool) DeployOpt { return func(o *Options) { o.USDCEnabled = enabled } } func WithMulticall3(enabled bool) DeployOpt { return func(o *Options) { o.Multicall3Enabled = enabled } } func DeployPrerequisiteContracts(e deployment.Environment, ab deployment.AddressBook, state CCIPOnChainState, chain deployment.Chain, deployOpts ...DeployOpt) { options := &Options{} for _, deployOpt := range deployOpts { deployOpt(options) } // use options.USDCEnabled }

makramkd · 2024-11-21T09:29:51Z

deployment/ccip/changeset/test_helpers.go

@@ -242,15 +244,10 @@ func mockAttestationResponse() *httptest.Server {
 	return server
 }

-func NewMemoryEnvironmentWithJobsAndContracts(t *testing.T, lggr logger.Logger, numChains int, numNodes int) DeployedEnv {
+func NewMemoryEnvironmentWithJobsAndContracts(t *testing.T, lggr logger.Logger, numChains int, numNodes int, isUSDC bool) DeployedEnv {


I think this would benefit greatly from using the options pattern I mentioned above, everything after lggr logger.Logger can be an option

in this case maybe just a TestConfig struct would be easier? Its more like regular configuration so option funcs might be overkill

deployment/common/changeset/apply.go

makramkd · 2024-11-21T09:35:25Z

deployment/common/changeset/apply.go

+				signed, err := SignProposal(e, &prop)
+				if err != nil {
+					return deployment.Environment{}, fmt.Errorf("failed to sign proposal: %w", err)
+				}
+				for _, sel := range chains.ToSlice() {
+					timelock, ok := timelocksPerChain[sel]
+					if !ok || timelock == nil {
+						return deployment.Environment{}, fmt.Errorf("timelock not found for chain %d", sel)
+					}
+					err := ExecuteProposal(e, signed, timelock, sel)


Is this func expected to be used only in tests? Signs + executes the proposal seems to suggest that, but not 100% sure

yeah it is. Lets name the file test_helpers.go to be clear

makramkd · 2024-11-21T09:36:29Z

integration-tests/ccip-tests/testsetups/test_helpers.go

-	t *testing.T,
-	lggr logger.Logger) (changeset.DeployedEnv, *test_env.CLClusterTestEnv, testconfig.TestConfig) {
-	return NewLocalDevEnvironment(t, lggr, changeset.MockLinkPrice, changeset.MockWethPrice)
+func NewLocalDevEnvironmentWithDefaultPrice(t *testing.T, lggr logger.Logger, isUSDC bool) (changeset.DeployedEnv, *test_env.CLClusterTestEnv, tc.TestConfig) {


Ditto here and below re: options

makramkd · 2024-11-21T09:37:13Z

integration-tests/ccip-tests/testsetups/test_helpers.go

+			},
+		},
+	}
+	ctx = testcontext.Get(t)


This seems unnecessary

makramkd · 2024-11-21T09:37:29Z

integration-tests/ccip-tests/testsetups/test_helpers.go

-
-	state, err := changeset.LoadOnchainState(*e)
+	// Need to deploy prerequisites first so that we can for the CCIP contracts.
+	cs01 := []commonchangeset.ChangesetApplication{


Suggested change

cs01 := []commonchangeset.ChangesetApplication{

prereqChangesets := []commonchangeset.ChangesetApplication{

makramkd · 2024-11-21T09:37:46Z

integration-tests/ccip-tests/testsetups/test_helpers.go

-				API:         endpoint,
-				APITimeout:  commonconfig.MustNewDuration(time.Second),
-				APIInterval: commonconfig.MustNewDuration(500 * time.Millisecond),
+	USDCCCTPConfig := make(map[cciptypes.ChainSelector]pluginconfig.USDCCCTPTokenConfig)


Suggested change

USDCCCTPConfig := make(map[cciptypes.ChainSelector]pluginconfig.USDCCCTPTokenConfig)

usdcCCTPConfig := make(map[cciptypes.ChainSelector]pluginconfig.USDCCCTPTokenConfig)

makramkd · 2024-11-21T09:38:40Z

integration-tests/ccip-tests/testsetups/test_helpers.go

+	}
+
+	// Deploy second set of changesets to deploy and configure the CCIP contracts.
+	cs02 := []commonchangeset.ChangesetApplication{


Suggested change

cs02 := []commonchangeset.ChangesetApplication{

deployCCIPChangesets := []commonchangeset.ChangesetApplication{

connorwstein · 2024-11-21T14:52:54Z

deployment/common/changeset/apply.go

+				signed, err := SignProposal(e, &prop)
+				if err != nil {
+					return deployment.Environment{}, fmt.Errorf("failed to sign proposal: %w", err)
+				}
+				for _, sel := range chains.ToSlice() {
+					timelock, ok := timelocksPerChain[sel]
+					if !ok || timelock == nil {
+						return deployment.Environment{}, fmt.Errorf("timelock not found for chain %d", sel)
+					}
+					err := ExecuteProposal(e, signed, timelock, sel)


yeah it is. Lets name the file test_helpers.go to be clear

connorwstein · 2024-11-21T14:53:46Z

deployment/common/changeset/mcms_test_helpers.go

@@ -42,62 +43,81 @@ func SingleGroupMCMS(t *testing.T) config.Config {
 	return *c
 }

-func SignProposal(t *testing.T, env deployment.Environment, proposal *timelock.MCMSWithTimelockProposal) *mcms.Executor {


why the change here? its kind of nice that the testing arg makes it clear its for test only

connorwstein · 2024-11-21T14:54:50Z

deployment/common/changeset/save_existing.go

@@ -42,6 +42,8 @@ func (cfg ExistingContractsConfig) Validate() error {
 	return nil
 }

+// SaveExistingContracts saves the existing contracts to the address book.
+// Caller should update the environment's address book with the returned addresses.
 func SaveExistingContracts(env deployment.Environment, cfg ExistingContractsConfig) (deployment.ChangesetOutput, error) {


can we move the test here too and modify it to be abstract? thats more like a unit test which should be as close as possible to the code its testing

I added a test here but kept the ccip specific test too

connorwstein · 2024-11-21T14:57:28Z

deployment/ccip/changeset/test_helpers.go

@@ -242,15 +244,10 @@ func mockAttestationResponse() *httptest.Server {
 	return server
 }

-func NewMemoryEnvironmentWithJobsAndContracts(t *testing.T, lggr logger.Logger, numChains int, numNodes int) DeployedEnv {
+func NewMemoryEnvironmentWithJobsAndContracts(t *testing.T, lggr logger.Logger, numChains int, numNodes int, isUSDC bool) DeployedEnv {


in this case maybe just a TestConfig struct would be easier? Its more like regular configuration so option funcs might be overkill

connorwstein · 2024-11-21T15:16:50Z

deployment/common/changeset/apply.go

+)
+
+type ChangesetApplication struct {
+	Changeset func(e deployment.Environment, config any) (deployment.ChangesetOutput, error)


you can do deployment.ChangeSet[any] here no need to redefine

connorwstein · 2024-11-21T15:21:11Z

deployment/ccip/changeset/deploy.go

+	APIInterval *commonconfig.Duration
+}
+
+type DeployCCIPContractConfig struct {


since this is top level changeset input config, lets put it in the changeset file itself

connorwstein · 2024-11-21T15:24:33Z

deployment/ccip/changeset/initial_add_chain.go

+// - AddChainConfig + AddDON (candidate->primary promotion i.e. init) on the home chain
+// - SetOCR3Config on the remote chain
+// ConfigureChain assumes that the home chain is already enabled and all CCIP contracts are already deployed.
+func InitialAddChain(env deployment.Environment, c DeployCCIPContractConfig) (deployment.ChangesetOutput, error) {


should be plural as its adding multiple? Maybe just ConfigureNewChains

connorwstein · 2024-11-21T15:26:02Z

deployment/ccip/changeset/initial_add_chain.go

+	"github.com/smartcontractkit/chainlink/deployment"
+)
+
+var _ deployment.ChangeSet[DeployCCIPContractConfig] = InitialAddChain


definitely should rename this config struct - NewChainsConfig? Also lets keep the definition of the config here

connorwstein · 2024-11-21T15:27:46Z

deployment/ccip/changeset/deploy.go

+	HomeChainSel   uint64
+	FeedChainSel   uint64
+	ChainsToDeploy []uint64
+	TokenConfig    TokenConfig


hmm doesn't the token config vary by chain @makramkd ?

connorwstein · 2024-11-21T15:29:16Z

deployment/ccip/changeset/deploy.go

+	USDCConfig     USDCConfig
+	// For setting OCR configuration
+	OCRSecrets deployment.OCRSecrets
+}


we need to parameterize the chain configuration values as well those are just hard coded right now

chainlink/deployment/ccip/changeset/deploy_home_chain.go

Line 294 in b375b3c

GasPriceDeviationPPB: ccipocr3.NewBigIntFromInt64(1000),

also I think the OCR timing parameters need to be parameterized and could vary by chain

added ticket ccip-4320

connorwstein · 2024-11-21T19:08:27Z

deployment/ccip/changeset/deploy.go

+
+type PrerequisiteOpt func(o *DeployPrerequisiteContractsOpts)
+
+func WithUSDCChains(chains []uint64) PrerequisiteOpt {


these still don't look very useful to me, whats wrong with just using the struct directly?

It makes the DeployPrerequisite signature constant and helps adding more options. Otherwise for multicall there needs to be another bool addition to deployChainContracts

connorwstein · 2024-11-21T19:08:54Z

deployment/ccip/changeset/deploy.go

+// It then deploys the rest of the CCIP chain contracts to the selected chains
+// registers the nodes with the capability registry and creates a DON for
+// each new chain.
+func DeployCCIPContracts(


can be private

connorwstein · 2024-11-21T19:09:01Z

deployment/ccip/changeset/deploy.go

+
+	return nil
+}
+
 func DeployChainContractsForChains(


can be private

connorwstein · 2024-11-21T19:10:24Z

deployment/ccip/changeset/deploy.go

-		}
+		deployGrp.Go(
+			func() error {
+				err := deployChainContracts(e, chain, ab, rmnHome)


I'm not certain that rmnHome etc are threadsafe - maybe we come back to this idea in a follow up?

This does not deal with any home contract write operation. Just deploys chain contracts in different chains

connorwstein · 2024-11-21T19:11:05Z

deployment/ccip/changeset/deploy_chain.go

+// DeployChainContracts deploys all new CCIP v1.6 or later contracts for the given chains.
+// It returns the new addresses for the contracts.
+// If there is an error, it will return the successfully deployed addresses and the error so that the caller can call the
+// changeset again with the same input to retry the failed deployment.


ah so it is already idempotent now? Lets explicitly mention that

connorwstein · 2024-11-21T19:11:49Z

deployment/ccip/changeset/initial_deploy_test.go

@@ -21,61 +15,11 @@ import (

 func TestInitialDeploy(t *testing.T) {


I see ok, we can come back to it then

connorwstein · 2024-11-21T19:18:12Z

deployment/environment.go

@@ -76,6 +76,17 @@ type Environment struct {
 	Offchain          OffchainClient
 }

+func (e Environment) Copy() (Environment, error) {


I think this is confusing because its only does a deep copy of one field. Its more clear to just use the NewEnvironment constructor in ApplyChangesets and remove this

connorwstein · 2024-11-21T19:20:14Z

deployment/common/changeset/test_helpers.go

+			return e, fmt.Errorf("failed to apply changeset at index %d: %w", i, err)
+		}
+		if out.AddressBook != nil {
+			err := currentEnv.ExistingAddresses.Merge(out.AddressBook)


we're planning on making ExistingAddresses read-only as per CCIP-4247. So I'd invert this:
newAddresses = out.AddressBook.Merge(currentEnv.ExistingAddresses)

There is an issue with this. The consecutive changesets will load the state and try to look for previously deployed dependent contracts. that will fail if we do not update the existing addresses.

Discussed offline and addressed

connorwstein and others added 20 commits November 20, 2024 09:39

Move files

e6653a3

Move more files

95fd35a

Move the rest

654f326

Move some files to internal

d8fc0dc

Get building

edb4eb9

Move a bunch of home chain stuff to internal

b375b3c

Update README

17ca535

Fix integration tests

77f1199

Fix merge conflicts

74eb6ca

Merge branch 'develop' of github.com:smartcontractkit/chainlink into …

b408fbd

…develop

More merge conflicts

8e9de17

More merge conflicts

7d2bd9c

updates

ef60262

Merge branch 'develop' of github.com:smartcontractkit/chainlink into …

86b516f

…ccip-4308-optimize-docker-tests

Merge branch 'ccip-deploy-internal-pkg' of github.com:smartcontractki…

52dbb43

…t/chainlink into ccip-4158-add-chain

changes

aa9e9c9

changes

374c0d5

Merge branch 'develop' of github.com:smartcontractkit/chainlink into …

3612d8e

…ccip-4158-add-chain

changes

7fba91d

more changes

b53d58a

AnieeG requested review from a team as code owners November 20, 2024 22:59

AnieeG requested review from winder, rstout, asoliman92, makramkd and dimkouv November 20, 2024 22:59

AnieeG added 4 commits November 20, 2024 15:45

fix tests

a662514

more optimize

060c7fe

more fix

55f9099

remove unnecessary methods

b2128b4

makramkd reviewed Nov 21, 2024

View reviewed changes

connorwstein requested changes Nov 21, 2024

View reviewed changes

AnieeG added 3 commits November 21, 2024 09:27

fix review comments

ea1bc14

fix conflict

baa87c0

fixes

49dcad3

AnieeG requested review from connorwstein and makramkd November 21, 2024 18:22

AnieeG added 2 commits November 21, 2024 10:35

add a test

7cb2a62

more fix

8d4a5bb

makramkd previously approved these changes Nov 21, 2024

View reviewed changes

connorwstein reviewed Nov 21, 2024

View reviewed changes

more review comments

9ca4940

AnieeG dismissed makramkd’s stale review via 9ca4940 November 21, 2024 19:39

fix ApplyChangesets

321539e

connorwstein approved these changes Nov 21, 2024

View reviewed changes

pramodhs2 self-requested a review November 21, 2024 21:16

pramodhs2 approved these changes Nov 21, 2024

View reviewed changes

AnieeG added this pull request to the merge queue Nov 21, 2024

winder approved these changes Nov 21, 2024

View reviewed changes

kalverra approved these changes Nov 21, 2024

View reviewed changes

b-gopalswami approved these changes Nov 21, 2024

View reviewed changes

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 21, 2024

AnieeG added this pull request to the merge queue Nov 21, 2024

Merged via the queue into develop with commit d2817c5 Nov 21, 2024
167 of 168 checks passed

AnieeG deleted the ccip-4158-add-chain branch November 21, 2024 21:58

	cs01 := []commonchangeset.ChangesetApplication{
	prereqChangesets := []commonchangeset.ChangesetApplication{

	USDCCCTPConfig := make(map[cciptypes.ChainSelector]pluginconfig.USDCCCTPTokenConfig)
	usdcCCTPConfig := make(map[cciptypes.ChainSelector]pluginconfig.USDCCCTPTokenConfig)

	cs02 := []commonchangeset.ChangesetApplication{
	deployCCIPChangesets := []commonchangeset.ChangesetApplication{


		type PrerequisiteOpt func(o *DeployPrerequisiteContractsOpts)

		func WithUSDCChains(chains []uint64) PrerequisiteOpt {

		@@ -21,61 +15,11 @@ import (

		func TestInitialDeploy(t *testing.T) {

Ccip-4158 - Initial Add Chain changeset #15349

Ccip-4158 - Initial Add Chain changeset #15349

Conversation

AnieeG commented Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnieeG Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnieeG Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnieeG commented Nov 20, 2024 •

edited

Loading

AnieeG Nov 21, 2024 •

edited

Loading

AnieeG Nov 21, 2024 •

edited

Loading