fix: add checks for duplicate timelock refs or conflicting regs with mcms

Author: ecPablo

mcms/changesets/deploy-custom-topology/changeset.go

@@ -11,6 +11,8 @@ import (
 	cldf "github.com/smartcontractkit/chainlink-deployments-framework/deployment"
 	"github.com/smartcontractkit/chainlink-deployments-framework/operations"
 	mcmstypes "github.com/smartcontractkit/mcms/types"
+
+	"github.com/smartcontractkit/cld-changesets/internal/maputil"
 )
 
 var _ cldf.ChangeSetV2[Input] = Changeset{}
@@ -40,7 +42,7 @@ func (Changeset) Apply(e cldf.Environment, input Input) (cldf.ChangesetOutput, e
 	var agg ChainOutput
 	var reports []operations.Report[any, any]
 
-	for _, chainSelector := range sortedChainSelectors(input.Cfg.ChainConfigs) {
+	for _, chainSelector := range maputil.SortedMapKeys(input.Cfg.ChainConfigs) {
 		seq, seqErr := SequenceForChainSelector(chainSelector)
 		if seqErr != nil {
 			return buildOutput(e, input.MCMS, agg, reports, fmt.Errorf("chain selector %d: %w", chainSelector, seqErr))
@@ -140,7 +142,7 @@ func validateConfig(e cldf.Environment, cfg Config, mcms *cldf.MCMSTimelockPropo
 	}
 
 	byFamily := make(map[string][]ChainInput)
-	for _, chainSelector := range sortedChainSelectors(cfg.ChainConfigs) {
+	for _, chainSelector := range maputil.SortedMapKeys(cfg.ChainConfigs) {
 		chainCfg := cfg.ChainConfigs[chainSelector]
 		family, err := chain_selectors.GetSelectorFamily(chainSelector)
 		if err != nil {
@@ -173,29 +175,39 @@ func validateConfig(e cldf.Environment, cfg Config, mcms *cldf.MCMSTimelockPropo
 	return nil
 }
 
-// validateChainConfig checks a single chain's topology: unique MCM refs, every
-// referenced MCMRef is declared, every RoleHolder is well-formed, and ownership
-// transfers require an MCMS input.
+// validateChainConfig checks a single chain's topology: unique refs within MCMs
+// and timelocks (with no overlap between the two), every referenced MCMRef is
+// declared, every RoleHolder is well-formed, and ownership transfers require an
+// MCMS input.
 func validateChainConfig(chainSelector uint64, cfg ChainTopologyConfig, mcms *cldf.MCMSTimelockProposalInput) error {
 	if len(cfg.MCMs) == 0 && len(cfg.Timelocks) == 0 {
 		return fmt.Errorf("chain %d: no MCMs or timelocks to deploy", chainSelector)
 	}
 
-	declared := make(map[string]struct{}, len(cfg.MCMs))
+	mcmRefs := make(map[string]struct{}, len(cfg.MCMs))
 	for i, m := range cfg.MCMs {
 		if m.Ref == "" {
 			return fmt.Errorf("chain %d: mcms[%d]: ref is required", chainSelector, i)
 		}
-		if _, dup := declared[m.Ref]; dup {
+		if _, dup := mcmRefs[m.Ref]; dup {
 			return fmt.Errorf("chain %d: duplicate MCM ref %q", chainSelector, m.Ref)
 		}
-		declared[m.Ref] = struct{}{}
+		mcmRefs[m.Ref] = struct{}{}
 	}
 
+	timelockRefs := make(map[string]struct{}, len(cfg.Timelocks))
 	for i, t := range cfg.Timelocks {
 		if t.Ref == "" {
 			return fmt.Errorf("chain %d: timelocks[%d]: ref is required", chainSelector, i)
 		}
+		if _, dup := timelockRefs[t.Ref]; dup {
+			return fmt.Errorf("chain %d: duplicate timelock ref %q", chainSelector, t.Ref)
+		}
+		if _, collides := mcmRefs[t.Ref]; collides {
+			return fmt.Errorf("chain %d: timelock ref %q conflicts with an MCM ref", chainSelector, t.Ref)
+		}
+		timelockRefs[t.Ref] = struct{}{}
+
 		if t.Qualifier == "" {
 			return fmt.Errorf("chain %d: timelock %q: qualifier is required", chainSelector, t.Ref)
 		}
@@ -207,11 +219,11 @@ func validateChainConfig(chainSelector uint64, cfg ChainTopologyConfig, mcms *cl
 			"admins":     t.Roles.Admins,
 		}
 		for name, holders := range roleSets {
-			if err := validateRoleHolders(chainSelector, t.Ref, name, holders, declared); err != nil {
+			if err := validateRoleHolders(chainSelector, t.Ref, name, holders, mcmRefs); err != nil {
 				return err
 			}
 		}
-		if err := validateRoleHolders(chainSelector, t.Ref, "transferOwnership", t.TransferOwnership, declared); err != nil {
+		if err := validateRoleHolders(chainSelector, t.Ref, "transferOwnership", t.TransferOwnership, mcmRefs); err != nil {
 			return err
 		}
 		if len(t.TransferOwnership) > 0 && mcms == nil {
@@ -239,16 +251,6 @@ func validateRoleHolders(chainSelector uint64, timelockRef, field string, holder
 	return nil
 }
 
-func sortedChainSelectors(byChain map[uint64]ChainTopologyConfig) []uint64 {
-	selectors := make([]uint64, 0, len(byChain))
-	for chainSelector := range byChain {
-		selectors = append(selectors, chainSelector)
-	}
-	slices.Sort(selectors)
-
-	return selectors
-}
-
 // mergeChainOutputs concatenates metadata and proposal groups from one chain's
 // sequence output into the running aggregate.
 func mergeChainOutputs(agg, out ChainOutput) ChainOutput {

mcms/changesets/deploy-custom-topology/changeset_test.go

@@ -113,6 +113,16 @@ func TestChangeset_VerifyPreconditions(t *testing.T) {
 	dupMCM := validChainConfig()
 	dupMCM.MCMs = append(dupMCM.MCMs, MCMSpec{Ref: "proposer", Qualifier: "CCIP"})
 
+	dupTimelock := validChainConfig()
+	dupTimelock.Timelocks = append(dupTimelock.Timelocks, TimelockSpec{
+		Ref:       "timelock",
+		MinDelay:  big.NewInt(0),
+		Qualifier: "RMN",
+	})
+
+	mcmTimelockRefCollision := validChainConfig()
+	mcmTimelockRefCollision.Timelocks[0].Ref = "proposer"
+
 	undeclaredRef := validChainConfig()
 	undeclaredRef.Timelocks[0].Roles.Proposers = []RoleHolder{{MCMRef: "missing"}}
 
@@ -150,6 +160,16 @@ func TestChangeset_VerifyPreconditions(t *testing.T) {
 			input:   Input{Cfg: Config{ChainConfigs: map[uint64]ChainTopologyConfig{fakeEVMFamilySelector: dupMCM}}},
 			wantErr: "duplicate MCM ref",
 		},
+		{
+			name:    "duplicate timelock ref",
+			input:   Input{Cfg: Config{ChainConfigs: map[uint64]ChainTopologyConfig{fakeEVMFamilySelector: dupTimelock}}},
+			wantErr: "duplicate timelock ref",
+		},
+		{
+			name:    "timelock ref conflicts with MCM ref",
+			input:   Input{Cfg: Config{ChainConfigs: map[uint64]ChainTopologyConfig{fakeEVMFamilySelector: mcmTimelockRefCollision}}},
+			wantErr: "conflicts with an MCM ref",
+		},
 		{
 			name:    "undeclared mcmRef",
 			input:   Input{Cfg: Config{ChainConfigs: map[uint64]ChainTopologyConfig{fakeEVMFamilySelector: undeclaredRef}}},
@@ -256,13 +276,6 @@ func TestChangeset_Apply_unregisteredFamilyErrors(t *testing.T) {
 	require.ErrorContains(t, err, "chain selector 1")
 }
 
-func TestSortedChainSelectors(t *testing.T) {
-	t.Parallel()
-
-	got := sortedChainSelectors(map[uint64]ChainTopologyConfig{3: {}, 1: {}, 2: {}})
-	require.Equal(t, []uint64{1, 2, 3}, got)
-}
-
 func TestMergeChainOutputs(t *testing.T) {
 	t.Parallel()
 

mcms/changesets/deploy-custom-topology/types.go

@@ -59,6 +59,8 @@ type MCMSpec struct {
 // TimelockSpec is one timelock (RBACTimelock on EVM) to deploy, with its role
 // wiring and optional ownership transfers.
 type TimelockSpec struct {
+	// Ref is a logical handle, unique within the chain and distinct from every
+	// MCMSpec.Ref on the same chain.
 	Ref string `json:"ref"`
 
 	MinDelay  *big.Int `json:"minDelay"`

mcms/evm/deploy-custom-topology/sequence.go

@@ -82,7 +82,6 @@ func runEVMDeployTopology(
 		if err != nil {
 			return out, err
 		}
-
 	}
 
 	return out, nil
@@ -166,6 +165,7 @@ func deployCallProxy(
 		return common.Address{}, fmt.Errorf("deploy call proxy for timelock %q on chain %d: %w", timelockSpec.Ref, chainSelector, err)
 	}
 	callProxyAddr := common.HexToAddress(cpReport.Output.Address)
+
 	return callProxyAddr, nil
 }
 
@@ -191,15 +191,15 @@ func deployTimelockAndSetRoles(
 	}
 	bypassers, err := resolveHolders(tl.Roles.Bypassers, refToAddr)
 	if err != nil {
-		fmt.Errorf("timelock %q bypassers: %w", tl.Ref, err)
+		return fmt.Errorf("timelock %q bypassers: %w", tl.Ref, err)
 	}
 	executors, err := resolveHolders(tl.Roles.Executors, refToAddr)
 	if err != nil {
-		fmt.Errorf("timelock %q executors: %w", tl.Ref, err)
+		return fmt.Errorf("timelock %q executors: %w", tl.Ref, err)
 	}
 	admins, err := resolveHolders(tl.Roles.Admins, refToAddr)
 	if err != nil {
-		fmt.Errorf("timelock %q admins: %w", tl.Ref, err)
+		return fmt.Errorf("timelock %q admins: %w", tl.Ref, err)
 	}
 
 	// Deploy the timelock with the deployer as initial admin. Executors are left
@@ -225,7 +225,7 @@ func deployTimelockAndSetRoles(
 		chainIdempotencyKey[opscontract.DeployInput[timelockops.ConstructorArgs], cldf_evm.Chain](chain),
 	)
 	if err != nil {
-		fmt.Errorf("deploy timelock %q on chain %d: %w", tl.Ref, chainSelector, err)
+		return fmt.Errorf("deploy timelock %q on chain %d: %w", tl.Ref, chainSelector, err)
 	}
 	timelockAddr := common.HexToAddress(tlReport.Output.Address)
 	refToAddr[tl.Ref] = timelockAddr
@@ -348,6 +348,7 @@ func grantRoles(
 				"Role", g.Name,
 				"Error", err,
 			)
+
 			return err
 		}
 
@@ -375,6 +376,7 @@ func grantRoles(
 					"Role Name", g.Name,
 					"Address", addr.Hex(),
 				)
+
 				return err
 			}
 			b.Logger.Infow("Role granted",