Stronger address book typing (#1326)

## Motivation
Want to be maximally defensive for the address book to prevent
corruption.

## Solution
- Type contract and version
- Let products compose type and version at deploy/state gen time to
avoid a combinatorial number of strings

integration-tests/deployment/address_book.go

@@ -1,25 +1,112 @@
 package deployment
 
-import "fmt"
+import (
+	"fmt"
+	"strings"
+
+	"github.com/Masterminds/semver/v3"
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/pkg/errors"
+	chainsel "github.com/smartcontractkit/chain-selectors"
+)
+
+var (
+	ErrInvalidChainSelector = fmt.Errorf("invalid chain selector")
+	ErrInvalidAddress       = fmt.Errorf("invalid address")
+)
+
+// ContractType is a simple string type for identifying contract types.
+type ContractType string
+
+var (
+	Version1_0_0     = *semver.MustParse("1.0.0")
+	Version1_1_0     = *semver.MustParse("1.1.0")
+	Version1_2_0     = *semver.MustParse("1.2.0")
+	Version1_5_0     = *semver.MustParse("1.5.0")
+	Version1_6_0_dev = *semver.MustParse("1.6.0-dev")
+)
+
+type TypeAndVersion struct {
+	Type    ContractType
+	Version semver.Version
+}
+
+func (tv TypeAndVersion) String() string {
+	return fmt.Sprintf("%s %s", tv.Type, tv.Version.String())
+}
+
+func (tv TypeAndVersion) Equal(other TypeAndVersion) bool {
+	return tv.String() == other.String()
+}
+
+func MustTypeAndVersionFromString(s string) TypeAndVersion {
+	tv, err := TypeAndVersionFromString(s)
+	if err != nil {
+		panic(err)
+	}
+	return tv
+}
+
+// Note this will become useful for validation. When we want
+// to assert an onchain call to typeAndVersion yields whats expected.
+func TypeAndVersionFromString(s string) (TypeAndVersion, error) {
+	parts := strings.Split(s, " ")
+	if len(parts) != 2 {
+		return TypeAndVersion{}, fmt.Errorf("invalid type and version string: %s", s)
+	}
+	v, err := semver.NewVersion(parts[1])
+	if err != nil {
+		return TypeAndVersion{}, err
+	}
+	return TypeAndVersion{
+		Type:    ContractType(parts[0]),
+		Version: *v,
+	}, nil
+}
+
+func NewTypeAndVersion(t ContractType, v semver.Version) TypeAndVersion {
+	return TypeAndVersion{
+		Type:    t,
+		Version: v,
+	}
+}
 
 // AddressBook is a simple interface for storing and retrieving contract addresses across
-// chains. It is family agnostic.
+// chains. It is family agnostic as the keys are chain selectors.
+// We store rather than derive typeAndVersion as some contracts do not support it.
+// For ethereum addresses are always stored in EIP55 format.
 type AddressBook interface {
-	Save(chainSelector uint64, address string, typeAndVersion string) error
-	Addresses() (map[uint64]map[string]string, error)
-	AddressesForChain(chain uint64) (map[string]string, error)
+	Save(chainSelector uint64, address string, tv TypeAndVersion) error
+	Addresses() (map[uint64]map[string]TypeAndVersion, error)
+	AddressesForChain(chain uint64) (map[string]TypeAndVersion, error)
 	// Allows for merging address books (e.g. new deployments with existing ones)
 	Merge(other AddressBook) error
 }
 
 type AddressBookMap struct {
-	AddressesByChain map[uint64]map[string]string
+	AddressesByChain map[uint64]map[string]TypeAndVersion
 }
 
-func (m *AddressBookMap) Save(chainSelector uint64, address string, typeAndVersion string) error {
+func (m *AddressBookMap) Save(chainSelector uint64, address string, typeAndVersion TypeAndVersion) error {
+	_, exists := chainsel.ChainBySelector(chainSelector)
+	if !exists {
+		return errors.Wrapf(ErrInvalidChainSelector, "chain selector %d not found", chainSelector)
+	}
+	if address == "" || address == common.HexToAddress("0x0").Hex() {
+		return errors.Wrap(ErrInvalidAddress, "address cannot be empty")
+	}
+	if common.IsHexAddress(address) {
+		// IMPORTANT: WE ALWAYS STANDARDIZE ETHEREUM ADDRESS STRINGS TO EIP55
+		address = common.HexToAddress(address).Hex()
+	} else {
+		return errors.Wrapf(ErrInvalidAddress, "address %s is not a valid Ethereum address, only Ethereum addresses supported", address)
+	}
+	if typeAndVersion.Type == "" {
+		return fmt.Errorf("type cannot be empty")
+	}
 	if _, exists := m.AddressesByChain[chainSelector]; !exists {
 		// First time chain add, create map
-		m.AddressesByChain[chainSelector] = make(map[string]string)
+		m.AddressesByChain[chainSelector] = make(map[string]TypeAndVersion)
 	}
 	if _, exists := m.AddressesByChain[chainSelector][address]; exists {
 		return fmt.Errorf("address %s already exists for chain %d", address, chainSelector)
@@ -28,11 +115,11 @@ func (m *AddressBookMap) Save(chainSelector uint64, address string, typeAndVersi
 	return nil
 }
 
-func (m *AddressBookMap) Addresses() (map[uint64]map[string]string, error) {
+func (m *AddressBookMap) Addresses() (map[uint64]map[string]TypeAndVersion, error) {
 	return m.AddressesByChain, nil
 }
 
-func (m *AddressBookMap) AddressesForChain(chain uint64) (map[string]string, error) {
+func (m *AddressBookMap) AddressesForChain(chain uint64) (map[string]TypeAndVersion, error) {
 	if _, exists := m.AddressesByChain[chain]; !exists {
 		return nil, fmt.Errorf("chain %d not found", chain)
 	}
@@ -55,8 +142,11 @@ func (m *AddressBookMap) Merge(ab AddressBook) error {
 	return nil
 }
 
+// TODO: Maybe could add an environment argument
+// which would ensure only mainnet/testnet chain selectors are used
+// for further safety?
 func NewMemoryAddressBook() *AddressBookMap {
 	return &AddressBookMap{
-		AddressesByChain: make(map[uint64]map[string]string),
+		AddressesByChain: make(map[uint64]map[string]TypeAndVersion),
 	}
 }

integration-tests/deployment/address_book_test.go

@@ -1,71 +1,93 @@
 package deployment
 
 import (
+	"errors"
 	"testing"
 
+	"github.com/ethereum/go-ethereum/common"
+	chainsel "github.com/smartcontractkit/chain-selectors"
 	"github.com/stretchr/testify/require"
 	"gotest.tools/v3/assert"
 )
 
-func TestAddressBook(t *testing.T) {
+func TestAddressBook_Save(t *testing.T) {
 	ab := NewMemoryAddressBook()
-	err := ab.Save(1, "0x1", "OnRamp 1.0.0")
+	onRamp100 := NewTypeAndVersion("OnRamp", Version1_0_0)
+	onRamp110 := NewTypeAndVersion("OnRamp", Version1_1_0)
+	offRamp100 := NewTypeAndVersion("OffRamp", Version1_0_0)
+	addr1 := common.HexToAddress("0x1").String()
+	addr2 := common.HexToAddress("0x2").String()
+	addr3 := common.HexToAddress("0x3").String()
+
+	err := ab.Save(chainsel.TEST_90000001.Selector, addr1, onRamp100)
 	require.NoError(t, err)
-	// Duplicate address will error
-	err = ab.Save(1, "0x1", "OnRamp 1.0.0")
+
+	// Check input validation
+	err = ab.Save(chainsel.TEST_90000001.Selector, "asdlfkj", onRamp100)
+	require.Error(t, err)
+	assert.Equal(t, errors.Is(err, ErrInvalidAddress), true, "err %s", err)
+	err = ab.Save(0, addr1, onRamp100)
 	require.Error(t, err)
+	assert.Equal(t, errors.Is(err, ErrInvalidChainSelector), true)
+	// Duplicate
+	err = ab.Save(chainsel.TEST_90000001.Selector, addr1, onRamp100)
+	require.Error(t, err)
+	// Zero address
+	err = ab.Save(chainsel.TEST_90000001.Selector, common.HexToAddress("0x0").Hex(), onRamp100)
+	require.Error(t, err)
+
 	// Distinct address same TV will not
-	err = ab.Save(1, "0x2", "OnRamp 1.0.0")
+	err = ab.Save(chainsel.TEST_90000001.Selector, addr2, onRamp100)
 	require.NoError(t, err)
 	// Same address different chain will not error
-	err = ab.Save(2, "0x1", "OnRamp 1.0.0")
+	err = ab.Save(chainsel.TEST_90000002.Selector, addr1, onRamp100)
 	require.NoError(t, err)
 	// We can save different versions of the same contract
-	err = ab.Save(2, "0x2", "OnRamp 1.2.0")
+	err = ab.Save(chainsel.TEST_90000002.Selector, addr2, onRamp110)
 	require.NoError(t, err)
 
 	addresses, err := ab.Addresses()
 	require.NoError(t, err)
-	assert.DeepEqual(t, addresses, map[uint64]map[string]string{
-		1: {
-			"0x1": "OnRamp 1.0.0",
-			"0x2": "OnRamp 1.0.0",
+	assert.DeepEqual(t, addresses, map[uint64]map[string]TypeAndVersion{
+		chainsel.TEST_90000001.Selector: {
+			addr1: onRamp100,
+			addr2: onRamp100,
 		},
-		2: {
-			"0x1": "OnRamp 1.0.0",
-			"0x2": "OnRamp 1.2.0",
+		chainsel.TEST_90000002.Selector: {
+			addr1: onRamp100,
+			addr2: onRamp110,
 		},
 	})
 
 	// Test merge
 	ab2 := NewMemoryAddressBook()
-	require.NoError(t, ab2.Save(3, "0x3", "OnRamp 1.0.0"))
+	require.NoError(t, ab2.Save(chainsel.TEST_90000003.Selector, addr3, onRamp100))
 	require.NoError(t, ab.Merge(ab2))
 	// Other address book should remain unchanged.
 	addresses, err = ab2.Addresses()
 	require.NoError(t, err)
-	assert.DeepEqual(t, addresses, map[uint64]map[string]string{
-		3: {
-			"0x3": "OnRamp 1.0.0",
+	assert.DeepEqual(t, addresses, map[uint64]map[string]TypeAndVersion{
+		chainsel.TEST_90000003.Selector: {
+			addr3: onRamp100,
 		},
 	})
 	// Existing addressbook should contain the new elements.
 	addresses, err = ab.Addresses()
 	require.NoError(t, err)
-	assert.DeepEqual(t, addresses, map[uint64]map[string]string{
-		1: {
-			"0x1": "OnRamp 1.0.0",
-			"0x2": "OnRamp 1.0.0",
+	assert.DeepEqual(t, addresses, map[uint64]map[string]TypeAndVersion{
+		chainsel.TEST_90000001.Selector: {
+			addr1: onRamp100,
+			addr2: onRamp100,
 		},
-		2: {
-			"0x1": "OnRamp 1.0.0",
-			"0x2": "OnRamp 1.2.0",
+		chainsel.TEST_90000002.Selector: {
+			addr1: onRamp100,
+			addr2: onRamp110,
 		},
-		3: {
-			"0x3": "OnRamp 1.0.0",
+		chainsel.TEST_90000003.Selector: {
+			addr3: onRamp100,
 		},
 	})
 
 	// Merge to an existing chain.
-	require.NoError(t, ab2.Save(2, "0x3", "OffRamp 1.0.0"))
+	require.NoError(t, ab2.Save(chainsel.TEST_90000002.Selector, addr3, offRamp100))
 }