chore(cre-deployment): refactor register nodes; add better test #16212

krehermann · 2025-02-04T21:27:13Z

Add the AddNodes changeset
refactor RegisterNodes to seperate out reuseable logic
add lots of tests

Requires

Supports

krehermann · 2025-02-04T21:28:03Z

deployment/keystone/changeset/internal/deploy.go

@@ -615,19 +604,15 @@ func RegisterNodes(lggr logger.Logger, req *RegisterNodesRequest) (*RegisterNode
 			params, ok := nodeIDToParams[n.NodeID]

 			if !ok {
-				evmCC, exists := n.SelToOCRConfig[registryChainDetails]


this was the only call site of registryChainDetails; mv'd to helper

krehermann · 2025-02-04T21:48:20Z

deployment/keystone/changeset/internal/deploy.go

 				params = capabilities_registry.CapabilitiesRegistryNodeParams{
 					NodeOperatorId:      nop.NodeOperatorId,
 					Signer:              signer,
 					P2pId:               n.PeerID,
-					EncryptionPublicKey: csakey,


csakey was a misleading var name

krehermann · 2025-02-04T21:50:30Z

deployment/keystone/changeset/internal/deploy.go

 		}, nil
 	}

-	tx, err := registry.AddNodes(registryChain.DeployerKey, nodes2Add)
+	tx, err := registry.AddNodes(req.RegistryChain.DeployerKey, nodes2Add)
 	if err != nil {
 		err = deployment.DecodeErr(capabilities_registry.CapabilitiesRegistryABI, err)
 		// no typed errors in the abi, so we have to do string matching


this was unneeded b/c getNodesToRegistry already dedups

github-actions · 2025-02-04T21:57:10Z

AER Report: CI Core ran successfully ✅

aer_workflow , commit

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

github-actions · 2025-02-05T20:00:14Z

Flakeguard Summary

Ran new or updated tests between develop and a977493 (cre/refactor-register-nodes).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestAddNodes	0.00%	false	false	false	3	0	3	0	github.com/smartcontractkit/chainlink/deployment/keystone/changeset	false	15.466666666s	@smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation
TestAddNodes/with_mcms	0.00%	false	false	false	3	0	3	0	github.com/smartcontractkit/chainlink/deployment/keystone/changeset	false	7.76s	@smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation

Artifacts

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

krehermann · 2025-02-05T20:32:39Z

deployment/keystone/changeset/add_nodes.go

+
+var _ deployment.ChangeSet[*AddNodesRequest] = AddNodes
+
+func AddNodes(env deployment.Environment, req *AddNodesRequest) (deployment.ChangesetOutput, error) {


this is the new changeset; follow up will be to use in CLD instead of this https://github.com/smartcontractkit/chainlink-deployments/blob/badb8425d61c3079a0df23e59b78fbfd44a95fff/keystone/shared/changeset_registry.go#L108

krehermann · 2025-02-05T20:33:48Z

deployment/keystone/changeset/add_nodes_test.go

+		checkErr func(t *testing.T, useMCMS bool, err error)
+	}
+	//run the same tests for both mcms and non-mcms
+	var mcmsConfigs = []*changeset.MCMSConfig{nil, {MinDuration: 0}}


this is a nice evolution of the test convention; run all the tests for the both mcms & non mcms and ensure they do the same thing, up to much poorer mcms error handling

krehermann · 2025-02-05T20:34:14Z

deployment/keystone/changeset/add_nodes_test.go

+					checkErr: func(t *testing.T, useMCMS bool, err error) {
+						require.NotNil(t, err)
+						if useMCMS {
+							assert.ErrorContains(t, err, "underlying transaction reverted")


notice the loss of info when using mcms wrt contract errors

out of scope for this PR, but just out of curiosity: is there a way to bubble a more specific error up?

krehermann · 2025-02-05T20:36:28Z

deployment/keystone/changeset/internal/deploy.go

 		}
 	}
+	addResp, err := AddNodes(lggr, &AddNodesRequest{


this is the refactored, shareable code that is leveraged in the new changeset

krehermann · 2025-02-05T20:36:57Z

deployment/keystone/changeset/test/registry.go

 		t:        t,
 		nameToId: make(map[string][32]byte),
 	}
+	caps, err := registry.GetCapabilities(nil)
+	require.NoError(t, err)
+	for _, capb := range caps {


populate the cache with onchain state

github-actions · 2025-02-05T20:56:54Z

Flakeguard Summary

Ran new or updated tests between develop and bb63ef9 (cre/refactor-register-nodes).

View Flaky Detector Details | Compare Changes

Found Flaky Tests ❌

2 Results

Name	Pass Ratio	Panicked?	Timed Out?	Race?	Runs	Successes	Failures	Skips	Package	Package Panicked?	Avg Duration	Code Owners
TestAddNodes	66.67%	false	false	false	3	2	1	0	github.com/smartcontractkit/chainlink/deployment/keystone/changeset/internal	false	346.666666ms	@smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation
TestAddNodes/add_two_nodes	66.67%	false	false	false	3	2	1	0	github.com/smartcontractkit/chainlink/deployment/keystone/changeset/internal	false	40ms	@smartcontractkit/keystone, @smartcontractkit/core, @smartcontractkit/deployment-automation

Artifacts

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

cl-sonarqube-production · 2025-02-06T02:59:03Z

Quality Gate passed

Issues
2 New issues
1 Fixed issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

patrickhuie19 · 2025-02-06T04:12:42Z

deployment/keystone/changeset/add_nodes.go

+// NOPIdentity is a node operator identity
+//
+// either by operator or registration ID must be non empty
+// it is an error to have both set


/nit both set is an error: [why]

patrickhuie19 · 2025-02-06T04:13:30Z

deployment/keystone/changeset/add_nodes.go

+}
+
+func (i NOPIdentity) Validate() error {
+	dflt := kcr.CapabilitiesRegistryNodeOperator{}


/nit dflt stands for what?

patrickhuie19 · 2025-02-06T04:24:14Z

deployment/keystone/changeset/add_nodes.go

+	MCMSConfig *MCMSConfig
+}
+
+func (r *AddNodesRequest) Validate() error {


Should this also run Validate on each CreateNodeRequest in CreateNodeRequests?

patrickhuie19 · 2025-02-06T04:27:17Z

deployment/keystone/changeset/add_nodes.go

+		if err != nil {
+			return deployment.ChangesetOutput{}, fmt.Errorf("failed to resolve node params for node %s: %w", nodeName, err)
+		}
+		p2p := string(p.P2pId[:])


/nit, could the array to slice conversion happen in Resolve?

patrickhuie19 · 2025-02-06T04:30:53Z

deployment/keystone/changeset/add_nodes_test.go

+					checkErr: func(t *testing.T, useMCMS bool, err error) {
+						require.NotNil(t, err)
+						if useMCMS {
+							assert.ErrorContains(t, err, "underlying transaction reverted")


out of scope for this PR, but just out of curiosity: is there a way to bubble a more specific error up?

patrickhuie19 · 2025-02-06T04:40:09Z

deployment/keystone/changeset/add_nodes_test.go

+								NOPIdentity: changeset.NOPIdentity{
+									RegistrationID: te.Nops()[0].NodeOperatorId,
+								},
+								Signer:              test32byte(t, "dupe signer"),


Signer, EncryptionPublicKey, and P2PID are all duplicated. Would it be valuable to test each duplicate individually?

krehermann commented Feb 4, 2025

View reviewed changes

krehermann force-pushed the cre/refactor-register-nodes branch from 42bf97a to b7282ba Compare February 4, 2025 21:34

krehermann commented Feb 4, 2025

View reviewed changes

krehermann marked this pull request as ready for review February 5, 2025 20:29

krehermann requested review from a team, AnieeG and kylesmartin as code owners February 5, 2025 20:29

krehermann requested review from winder, asoliman92, makramkd, dimkouv, mateusz-sekara, 0xAustinWang and 0xnogo February 5, 2025 20:29

krehermann changed the title ~~refactor register nodes; add better test~~ chore(cre-deployment): refactor register nodes; add better test Feb 5, 2025

krehermann commented Feb 5, 2025

View reviewed changes

krehermann force-pushed the cre/refactor-register-nodes branch from 6b8c4de to bb63ef9 Compare February 5, 2025 20:37

krehermann requested review from patrickhuie19, george-dorin and Tofel February 5, 2025 20:47

chore(cre-deployment): CRE-236 AddNodes CS

6b16dad

krehermann force-pushed the cre/refactor-register-nodes branch from c7733ff to 6b16dad Compare February 6, 2025 00:05

krehermann added 3 commits February 5, 2025 17:05

Merge branch 'develop' into cre/refactor-register-nodes

27ceba1

more linter

978c856

this linter is killing me

a24908e

krehermann requested a review from vyzaldysanchez February 6, 2025 02:26

AnieeG previously approved these changes Feb 6, 2025

View reviewed changes

rename

64a83b3

krehermann dismissed AnieeG’s stale review via 64a83b3 February 6, 2025 02:37

patrickhuie19 reviewed Feb 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cre-deployment): refactor register nodes; add better test #16212

chore(cre-deployment): refactor register nodes; add better test #16212

krehermann commented Feb 4, 2025 •

edited by jira bot

Loading

krehermann Feb 4, 2025

krehermann Feb 4, 2025

krehermann Feb 4, 2025

github-actions bot commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 5, 2025

krehermann Feb 5, 2025

krehermann Feb 5, 2025

krehermann Feb 5, 2025

patrickhuie19 Feb 6, 2025

krehermann Feb 5, 2025

krehermann Feb 5, 2025

github-actions bot commented Feb 5, 2025

cl-sonarqube-production bot commented Feb 6, 2025

patrickhuie19 Feb 6, 2025

patrickhuie19 Feb 6, 2025

patrickhuie19 Feb 6, 2025

patrickhuie19 Feb 6, 2025

patrickhuie19 Feb 6, 2025

patrickhuie19 Feb 6, 2025


		var _ deployment.ChangeSet[*AddNodesRequest] = AddNodes

		func AddNodes(env deployment.Environment, req *AddNodesRequest) (deployment.ChangesetOutput, error) {

chore(cre-deployment): refactor register nodes; add better test #16212

Are you sure you want to change the base?

chore(cre-deployment): refactor register nodes; add better test #16212

Conversation

krehermann commented Feb 4, 2025 • edited by jira bot Loading

Requires

Supports

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2025 • edited Loading

AER Report: CI Core ran successfully ✅

AER Report: Operator UI CI ran successfully ✅

github-actions bot commented Feb 5, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

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

github-actions bot commented Feb 5, 2025

Flakeguard Summary

Found Flaky Tests ❌

Artifacts

cl-sonarqube-production bot commented Feb 6, 2025

Quality Gate passed

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

krehermann commented Feb 4, 2025 •

edited by jira bot

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading