op-chain-ops: use generic CheckNoZeroAddresses instead of struct-specific methods (#15785)

Author: bitwiseguy

op-chain-ops/addresses/roles.go

@@ -1,10 +1,6 @@
 package addresses
 
 import (
-	"errors"
-	"fmt"
-	"reflect"
-
 	"github.com/ethereum/go-ethereum/common"
 )
 
@@ -36,21 +32,3 @@ type OpChainFaultProofsRoles struct {
 	Proposer   common.Address
 	Challenger common.Address
 }
-
-var ErrSuperchainRoleZeroAddress = errors.New("SuperchainRole is set to zero address")
-
-func (s *SuperchainRoles) CheckNoZeroAddresses() error {
-	val := reflect.ValueOf(*s)
-	typ := reflect.TypeOf(*s)
-
-	// Iterate through all the fields
-	for i := 0; i < val.NumField(); i++ {
-		fieldValue := val.Field(i)
-		fieldName := typ.Field(i).Name
-
-		if fieldValue.Interface() == (common.Address{}) {
-			return fmt.Errorf("%w: %s", ErrSuperchainRoleZeroAddress, fieldName)
-		}
-	}
-	return nil
-}

op-chain-ops/addresses/roles_test.go

@@ -0,0 +1,53 @@
+package addresses
+
+import (
+	"errors"
+	"fmt"
+	"reflect"
+
+	"github.com/ethereum/go-ethereum/common"
+)
+
+var ErrZeroAddress = errors.New("found zero address")
+var ErrNilPointer = errors.New("nil pointer provided")
+var ErrNotAddressType = errors.New("field is not of type common.Address")
+
+// CheckNoZeroAddresses checks that all fields in a struct are of type common.Address
+// and that none of them are zero addresses. Works with both struct values and pointers to structs.
+// Returns an error if any field is not a common.Address or if any common.Address field is zero.
+func CheckNoZeroAddresses(s interface{}) error {
+	val := reflect.ValueOf(s)
+
+	// If we have a pointer, dereference it to get the struct
+	if val.Kind() == reflect.Ptr {
+		if val.IsNil() {
+			return ErrNilPointer
+		}
+		val = val.Elem()
+	}
+
+	// Ensure we're working with a struct
+	if val.Kind() != reflect.Struct {
+		return fmt.Errorf("can only check structs, but got %s", val.Kind())
+	}
+
+	typ := val.Type()
+	addressType := reflect.TypeOf(common.Address{})
+
+	// Iterate through all the fields
+	for i := 0; i < val.NumField(); i++ {
+		fieldValue := val.Field(i)
+		fieldName := typ.Field(i).Name
+
+		// Verify field is a common.Address
+		if fieldValue.Type() != addressType {
+			return fmt.Errorf("%w: %s (type: %s)", ErrNotAddressType, fieldName, fieldValue.Type())
+		}
+
+		// Check if the address is zero
+		if fieldValue.Interface() == (common.Address{}) {
+			return fmt.Errorf("%w: %s", ErrZeroAddress, fieldName)
+		}
+	}
+	return nil
+}

op-chain-ops/addresses/utils.go

@@ -0,0 +1,70 @@
+package addresses
+
+import (
+	"testing"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/stretchr/testify/require"
+)
+
+func TestCheckNoZeroAddresses(t *testing.T) {
+	t.Run("no zero addresses", func(t *testing.T) {
+		roles := SuperchainRoles{
+			SuperchainProxyAdminOwner: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			SuperchainGuardian:        common.HexToAddress("0x2222222222222222222222222222222222222222"),
+			ProtocolVersionsOwner:     common.HexToAddress("0x3333333333333333333333333333333333333333"),
+		}
+
+		err := CheckNoZeroAddresses(roles)
+		require.NoError(t, err)
+	})
+
+	t.Run("detects zero address", func(t *testing.T) {
+		roles := SuperchainRoles{
+			SuperchainProxyAdminOwner: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			ProtocolVersionsOwner:     common.HexToAddress("0x3333333333333333333333333333333333333333"),
+		}
+
+		require.Equal(t, roles.SuperchainGuardian, common.HexToAddress("0x0000000000000000000000000000000000000000"))
+		err := CheckNoZeroAddresses(roles)
+		require.Error(t, err)
+		require.ErrorIs(t, err, ErrZeroAddress)
+		require.Contains(t, err.Error(), "SuperchainGuardian")
+	})
+
+	t.Run("error for non-address fields", func(t *testing.T) {
+		roles := struct {
+			SuperchainProxyAdminOwner common.Address
+			ProtocolVersionsOwner     common.Address
+			chainId                   uint64
+		}{
+			SuperchainProxyAdminOwner: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			ProtocolVersionsOwner:     common.HexToAddress("0x3333333333333333333333333333333333333333"),
+			chainId:                   1,
+		}
+
+		err := CheckNoZeroAddresses(roles)
+		require.Error(t, err)
+		require.ErrorIs(t, err, ErrNotAddressType)
+		require.Contains(t, err.Error(), "chainId")
+	})
+
+	t.Run("struct pointer works", func(t *testing.T) {
+		roles := SuperchainRoles{
+			SuperchainProxyAdminOwner: common.HexToAddress("0x1111111111111111111111111111111111111111"),
+			SuperchainGuardian:        common.HexToAddress("0x2222222222222222222222222222222222222222"),
+			ProtocolVersionsOwner:     common.HexToAddress("0x3333333333333333333333333333333333333333"),
+		}
+
+		err := CheckNoZeroAddresses(&roles)
+		require.NoError(t, err)
+	})
+
+	t.Run("nil struct pointer fails", func(t *testing.T) {
+		var roles *SuperchainRoles = nil
+
+		err := CheckNoZeroAddresses(roles)
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "nil pointer provided")
+	})
+}

op-chain-ops/addresses/utils_test.go

@@ -2,12 +2,12 @@ package state
 
 import (
 	"fmt"
-	"reflect"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
 
 	"github.com/ethereum-optimism/optimism/cannon/mipsevm/versions"
+	"github.com/ethereum-optimism/optimism/op-chain-ops/addresses"
 	"github.com/ethereum-optimism/optimism/op-chain-ops/genesis"
 )
 
@@ -86,7 +86,6 @@ type ChainRoles struct {
 	Challenger        common.Address `json:"challenger" toml:"challenger"`
 }
 
-var ErrChainRoleZeroAddress = fmt.Errorf("ChainRole is set to zero address")
 var ErrFeeVaultZeroAddress = fmt.Errorf("chain has a fee vault set to zero address")
 var ErrNonStandardValue = fmt.Errorf("chain contains non-standard config value")
 var ErrEip1559ZeroValue = fmt.Errorf("eip1559 param is set to zero value")
@@ -97,7 +96,7 @@ func (c *ChainIntent) Check() error {
 		return fmt.Errorf("id must be set")
 	}
 
-	if err := c.Roles.CheckNoZeroAddresses(); err != nil {
+	if err := addresses.CheckNoZeroAddresses(c.Roles); err != nil {
 		return err
 	}
 
@@ -118,21 +117,3 @@ func (c *ChainIntent) Check() error {
 
 	return nil
 }
-
-// Returns an error if any fields in ChainRoles is set to common.Address{}
-func (cr *ChainRoles) CheckNoZeroAddresses() error {
-	val := reflect.ValueOf(*cr)
-	typ := reflect.TypeOf(*cr)
-
-	// Iterate through all the fields
-	for i := 0; i < val.NumField(); i++ {
-		fieldValue := val.Field(i)
-		fieldName := typ.Field(i).Name
-
-		if fieldValue.Interface() == (common.Address{}) {
-			return fmt.Errorf("%w: %s", ErrChainRoleZeroAddress, fieldName)
-		}
-	}
-
-	return nil
-}

op-deployer/pkg/deployer/state/chain_intent.go

@@ -99,7 +99,7 @@ func (c *Intent) validateCustomConfig() error {
 		if c.SuperchainRoles == nil {
 			return fmt.Errorf("%w: must set superchain roles if OPCM address is nil", ErrIncompatibleValue)
 		}
-		if err := c.SuperchainRoles.CheckNoZeroAddresses(); err != nil {
+		if err := addresses.CheckNoZeroAddresses(c.SuperchainRoles); err != nil {
 			return err
 		}
 	} else {

op-deployer/pkg/deployer/state/intent.go

@@ -14,7 +14,7 @@ func TestValidateStandardValues(t *testing.T) {
 
 	err = intent.Check()
 	require.Error(t, err)
-	require.ErrorIs(t, err, ErrChainRoleZeroAddress)
+	require.ErrorIs(t, err, addresses.ErrZeroAddress)
 
 	setChainRoles(&intent)
 	err = intent.Check()
@@ -110,12 +110,12 @@ func TestValidateCustomValues(t *testing.T) {
 
 	err = intent.Check()
 	require.Error(t, err)
-	require.ErrorIs(t, err, addresses.ErrSuperchainRoleZeroAddress)
+	require.ErrorIs(t, err, addresses.ErrZeroAddress)
 
 	setSuperchainRoles(&intent)
 	err = intent.Check()
 	require.Error(t, err)
-	require.ErrorIs(t, err, ErrChainRoleZeroAddress)
+	require.ErrorIs(t, err, addresses.ErrZeroAddress)
 
 	setChainRoles(&intent)
 	err = intent.Check()