Address code review comments

- Avoid modifying the message or changing error handling in the
  watcher's message publication flow when transfer verifier is disabled
- Remove continue statements when verifyAndPublish returns an error in
  the reobservation flow
- Modify code comments around VerificationState
- Use UintSlice when parsing CLI flags
- Change new SDK constants to be strings instead of common.Address to be
  consistent with other SDK values

node/cmd/guardiand/node.go

@@ -4,14 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"math"
 	"net"
 	_ "net/http/pprof" // #nosec G108 we are using a custom router (`router := mux.NewRouter()`) and thus not automatically expose pprof.
 	"os"
 	"os/signal"
 	"path"
 	"runtime"
 	"slices"
-	"strconv"
 	"strings"
 	"syscall"
 	"time"
@@ -282,7 +282,7 @@ var (
 	subscribeToVAAs *bool
 
 	// A list of chain IDs that should enable the Transfer Verifier. If empty, Transfer Verifier will not be enabled.
-	transferVerifierEnabledChains *string
+	transferVerifierEnabledChains *[]uint
 	// Global variable used to store enabled Chain IDs for Transfer Verification. Contents are parsed from
 	// transferVerifierEnabledChains.
 	txVerifierChains []vaa.ChainID
@@ -511,7 +511,7 @@ func init() {
 
 	subscribeToVAAs = NodeCmd.Flags().Bool("subscribeToVAAs", false, "Guardiand should subscribe to incoming signed VAAs, set to true if running a public RPC node")
 
-	transferVerifierEnabledChains = NodeCmd.Flags().String("transferVerifierEnabledChains", "", "Transfer Verifier will be enabled for these chain IDs (comma-separated)")
+	transferVerifierEnabledChains = NodeCmd.Flags().UintSlice("transferVerifierEnabledChains", make([]uint, 2), "Transfer Verifier will be enabled for these chain IDs (comma-separated)")
 }
 
 var (
@@ -930,13 +930,15 @@ func runNode(cmd *cobra.Command, args []string) {
 		logger.Fatal("If coinGeckoApiKey is set, then chainGovernorEnabled must be set")
 	}
 
+	// NOTE: If this flag isn't set, or the list is empty, Transfer Verifier should not be enabled.
 	if cmd.Flags().Changed("transferVerifierEnabledChains") {
 		var parseErr error
 		// NOTE: avoid shadowing txVerifierChains here. It should refer to the global variable.
-		txVerifierChains, parseErr = parseTxVerifierChains(*transferVerifierEnabledChains)
-		logger.Debug("parsed txVerifierChains", zap.Any("chains", txVerifierChains))
+		txVerifierChains, parseErr = validateTxVerifierChains(*transferVerifierEnabledChains)
+
+		logger.Debug("validated txVerifierChains", zap.Any("chains", txVerifierChains))
 		if parseErr != nil {
-			logger.Fatal("Could not parse transferVerifierEnabledChains", zap.Error(parseErr))
+			logger.Fatal("transferVerifierEnabledChains input is invalid", zap.Error(parseErr))
 		}
 	}
 
@@ -1910,28 +1912,24 @@ func argsConsistent(args []string) bool {
 	return true
 }
 
-// parseTxVerifierChains parses a list of chainIds from a comma separated string and validate that they have a Transfer Verifier implementation.
-// If the input is empty, Transfer Verifier will not be enabled.
-func parseTxVerifierChains(
+// validateTxVerifierChains validates that a slice of uints correspond to chain IDs with a Transfer Verifier implementation.
+func validateTxVerifierChains(
 	// A comma-separated list of Wormhole ChainIDs.
-	input string,
+	input []uint,
 ) ([]vaa.ChainID, error) {
 	if len(input) == 0 {
-		return nil, errors.New("could not parse input to parseTxVerifierChains: input empty")
+		return nil, errors.New("no chain IDs provided for transfer verification")
 	}
-	parsed := strings.Split(input, ",")
 	knownChains := vaa.GetAllNetworkIDs()
 	supportedChains := txverifier.SupportedChains()
 
 	// NOTE: Using a known capacity and counter here avoids unnecessary reallocations compared to using `append()`.
-	enabled := make([]vaa.ChainID, len(parsed))
+	enabled := make([]vaa.ChainID, len(input))
 	i := uint8(0)
-	for _, chainStr := range parsed {
-		chain, parseErr := strconv.Atoi(chainStr)
-		if parseErr != nil {
-			return nil, fmt.Errorf("could not parse chainId from string %s: %w", chainStr, parseErr)
+	for _, chain := range input {
+		if chain > uint(math.MaxUint16) {
+			return nil, fmt.Errorf("invalid chain ID %d: exceeds MaxUint16", chain)
 		}
-
 		chainId := vaa.ChainID(chain)
 
 		if !slices.Contains(knownChains, chainId) {

node/pkg/common/chainlock.go

@@ -18,25 +18,41 @@ import (
 const HashLength = 32
 const AddressLength = 32
 
-// This type represents whether a message has been verified for some definition of verification. This may mean different things
-// on a per-application or per-chain basis.
-// NOTE: This status is currently used only by the Transfer Verifier for supported chains.
+// The `VerificationState` is the result of applying transfer verification to the transaction associated with the `MessagePublication`.
+// While this could likely be extended to additional security controls in the future, it is only used for `txverifier` at present.
+// Consequently, its status should be set to `NotVerified` or `NotApplicable` for all messages that aren't token transfers.
 type VerificationState uint8
 
 const (
 	// The default state for a message. This can be used before verification occurs. If no verification is required, `NotApplicable` should be used instead.
-	// NOTE: this value is used as the default, zero-value for the type, so this field should not be re-ordered among other variants.
 	NotVerified VerificationState = iota
 	// Represents a "known bad" status where a Message has been validated and the result indicates an erroneous or invalid message. The message should be discarded.
 	Rejected
 	// Represents an unusual state after validation, neither confirmed to be good or bad.
 	Anomalous
-	// Represents a "known good" status where a Message has been validated and the result is good. The message should be process normally.
+	// Represents a "known good" status where a Message has been validated and the result is good. The message should be processed normally.
 	Valid
 	// Indicates that no verification is necessary.
 	NotApplicable
 )
 
+func (v VerificationState) String() string {
+	switch v {
+	case NotVerified:
+		return "NotVerified"
+	case Valid:
+		return "Valid"
+	case Anomalous:
+		return "Anomalous"
+	case Rejected:
+		return "Rejected"
+	case NotApplicable:
+		return "NotApplicable"
+	default:
+		return ""
+	}
+}
+
 type MessagePublication struct {
 	TxID      []byte
 	Timestamp time.Time
@@ -52,8 +68,10 @@ type MessagePublication struct {
 	// Unreliable indicates if this message can be reobserved. If a message is considered unreliable it cannot be
 	// reobserved.
 	Unreliable bool
-	// This type represents whether a message has been verified for some definition of verification. This may mean different things
-	// on a per-application or per-chain basis.
+
+	// The `VerificationState` is the result of applying transfer verification to the transaction associated with the `MessagePublication`.
+	// While this could likely be extended to additional security controls in the future, it is only used for `txverifier` at present.
+	// Consequently, its status should be set to `NotVerified` or `NotApplicable` for all messages that aren't token transfers.
 	verificationState VerificationState
 }
 
@@ -298,20 +316,3 @@ func (msg *MessagePublication) ZapFields(fields ...zap.Field) []zap.Field {
 		zap.String("verificationState", msg.verificationState.String()),
 	)
 }
-
-func (v VerificationState) String() string {
-	switch v {
-	case NotVerified:
-		return "NotVerified"
-	case Valid:
-		return "Valid"
-	case Anomalous:
-		return "Anomalous"
-	case Rejected:
-		return "Rejected"
-	case NotApplicable:
-		return "NotApplicable"
-	default:
-		return ""
-	}
-}

node/pkg/watchers/evm/reobserve.go

@@ -52,7 +52,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch
 
 			if pubErr != nil {
 				w.logger.Error("Error when publishing message", zap.Error(err))
-				continue
 			}
 
 			numObservations++
@@ -80,7 +79,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch
 
 				if pubErr != nil {
 					w.logger.Error("Error when publishing message", zap.Error(err))
-					continue
 				}
 
 				numObservations++
@@ -125,7 +123,6 @@ func (w *Watcher) handleReobservationRequest(ctx context.Context, chainId vaa.Ch
 
 			if pubErr != nil {
 				w.logger.Error("Error when publishing message", zap.Error(err))
-				continue
 			}
 
 			numObservations++

node/pkg/watchers/evm/watcher.go

@@ -273,24 +273,25 @@ func (w *Watcher) Run(parentCtx context.Context) error {
 				return errors.New("watcher attempted to create Transfer Verifier but this chainId is not supported")
 			}
 
-			var tbridge eth_common.Address
-			var weth eth_common.Address
+			// Fetch the constants for the Token Bridge and the WETH address from the SDK.
+			var tbridge []byte
+			var weth string
 			switch w.env {
 			case common.UnsafeDevNet:
-				tbridge = eth_common.BytesToAddress(sdk.KnownDevnetTokenbridgeEmitters[w.chainID])
+				tbridge = sdk.KnownDevnetTokenbridgeEmitters[w.chainID]
 				weth = sdk.KnownDevnetWrappedNativeAddresses[w.chainID]
 			case common.TestNet:
-				tbridge = eth_common.BytesToAddress(sdk.KnownTestnetTokenbridgeEmitters[w.chainID])
+				tbridge = sdk.KnownTestnetTokenbridgeEmitters[w.chainID]
 				weth = sdk.KnownTestnetWrappedNativeAddresses[w.chainID]
 			case common.MainNet:
-				tbridge = eth_common.Address(sdk.KnownTokenbridgeEmitters[w.chainID])
+				tbridge = sdk.KnownTokenbridgeEmitters[w.chainID]
 				// https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2
 				weth = sdk.KnownWrappedNativeAddress[w.chainID]
 			}
 			addrs := txverifier.TVAddresses{
 				CoreBridgeAddr:    w.contract,
-				TokenBridgeAddr:   tbridge,
-				WrappedNativeAddr: weth,
+				TokenBridgeAddr:   eth_common.BytesToAddress(tbridge[:]),
+				WrappedNativeAddr: eth_common.HexToAddress(weth),
 			}
 
 			// The block height difference between the latest block and the oldest block to keep in memory.
@@ -861,15 +862,17 @@ func (w *Watcher) verifyAndPublish(
 		return errors.New("verifyAndPublish: message publication already has a verification status")
 	}
 
-	// Verification is only relevant when Transfer Verification is enabled.
-	verificationState := common.NotApplicable
-
 	if w.txVerifierEnabled {
 		// This should have already been initialized by the constructor.
 		if w.txVerifier == nil {
 			return errors.New("verifyAndPublish: transfer verifier should be instantiated but is nil")
 		}
 
+		// Setting a custom verification state is only required when
+		// Transfer Verification is enabled. If disabled, the message
+		// will be emitted with the default value `NotVerified`.
+		var verificationState common.VerificationState
+
 		// Verify the transfer by analyzing the transaction receipt. This is a defense-in-depth mechanism
 		// to protect against fraudulent message emissions.
 		valid := w.txVerifier.ProcessEvent(ctx, txHash, receipt)
@@ -880,13 +883,14 @@ func (w *Watcher) verifyAndPublish(
 			w.logger.Debug("verifyAndPublish: transfer verification successful", zap.String("txHash", txHash.String()))
 			verificationState = common.Valid
 		}
-	}
 
-	updateErr := msg.SetVerificationState(verificationState)
-	if updateErr != nil {
-		errMsg := fmt.Sprintf("verifyAndPublish: could not set verification state for message with txID %s", msg.TxIDString())
-		w.logger.Error(errMsg, zap.Error(updateErr))
-		return fmt.Errorf("%s %w", errMsg, updateErr)
+		// Update the state of the message depending on the results of transfer verification.
+		updateErr := msg.SetVerificationState(verificationState)
+		if updateErr != nil {
+			errMsg := fmt.Sprintf("verifyAndPublish: could not set verification state for message with txID %s", msg.TxIDString())
+			w.logger.Error(errMsg, zap.Error(updateErr))
+			return fmt.Errorf("%s %w", errMsg, updateErr)
+		}
 	}
 
 	w.msgC <- msg

node/pkg/watchers/evm/watcher_test.go

@@ -69,12 +69,12 @@ func TestVerifyAndPublish(t *testing.T) {
 
 	// Check preconditions
 	require.Equal(t, 0, len(w.msgC))
-	require.Equal(t, common.NotVerified, msg.VerificationState())
+	require.Equal(t, common.NotVerified.String(), msg.VerificationState().String())
 
 	// Check nil message
 	err := w.verifyAndPublish(nil, ctx, eth_common.Hash{}, &types.Receipt{})
 	require.ErrorContains(t, err, "message publication cannot be nil")
-	require.Equal(t, common.NotVerified, msg.VerificationState())
+	require.Equal(t, common.NotVerified.String(), msg.VerificationState().String())
 
 	// Check transfer verifier not enabled case. The message should be published normally
 	msg = common.MessagePublication{}
@@ -84,7 +84,7 @@ func TestVerifyAndPublish(t *testing.T) {
 	publishedMsg := <-msgC
 	require.NotNil(t, publishedMsg)
 	require.Equal(t, 0, len(msgC))
-	require.Equal(t, common.NotApplicable, publishedMsg.VerificationState())
+	require.Equal(t, common.NotVerified.String(), publishedMsg.VerificationState().String())
 
 	// Check scenario where transfer verifier is enabled but isn't initialized.
 	msg = common.MessagePublication{}
@@ -93,7 +93,7 @@ func TestVerifyAndPublish(t *testing.T) {
 	err = w.verifyAndPublish(&msg, ctx, eth_common.Hash{}, &types.Receipt{})
 	require.ErrorContains(t, err, "transfer verifier should be instantiated but is nil")
 	require.Equal(t, 0, len(msgC))
-	require.Equal(t, common.NotVerified, publishedMsg.VerificationState())
+	require.Equal(t, common.NotVerified.String(), publishedMsg.VerificationState().String())
 
 	// Check scenario where the message already has a verification status.
 	msg = common.MessagePublication{}
@@ -103,7 +103,7 @@ func TestVerifyAndPublish(t *testing.T) {
 	err = w.verifyAndPublish(&msg, ctx, eth_common.Hash{}, &types.Receipt{})
 	require.ErrorContains(t, err, "message publication already has a verification status")
 	require.Equal(t, 0, len(msgC))
-	require.Equal(t, common.Anomalous, msg.VerificationState())
+	require.Equal(t, common.Anomalous.String(), msg.VerificationState().String())
 
 	// Check case where Transfer Verifier finds a dangerous transaction. Note that this case does
 	// not return an error, but the published message should be marked as Rejected.
@@ -117,7 +117,7 @@ func TestVerifyAndPublish(t *testing.T) {
 	publishedMsg = <-msgC
 	require.NotNil(t, publishedMsg)
 	require.Equal(t, 0, len(msgC))
-	require.Equal(t, common.Rejected, publishedMsg.VerificationState())
+	require.Equal(t, common.Rejected.String(), publishedMsg.VerificationState().String())
 
 	// Check happy path where txverifier is enabled and initialized
 	msg = common.MessagePublication{}
@@ -130,7 +130,7 @@ func TestVerifyAndPublish(t *testing.T) {
 	publishedMsg = <-msgC
 	require.NotNil(t, publishedMsg)
 	require.Equal(t, 0, len(msgC))
-	require.Equal(t, common.Valid, publishedMsg.VerificationState())
+	require.Equal(t, common.Valid.String(), publishedMsg.VerificationState().String())
 }
 
 // Helper function to set up a test Ethereum Watcher

sdk/devnet_consts.go

@@ -1,7 +1,6 @@
 package sdk
 
 import (
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/wormhole-foundation/wormhole/sdk/vaa"
 )
 
@@ -44,7 +43,7 @@ var KnownDevnetAutomaticRelayerEmitters = []struct {
 }
 
 // KnownDevnetWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum
-var KnownDevnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{
+var KnownDevnetWrappedNativeAddresses = map[vaa.ChainID]string{
 	// WETH deployed by the Tilt devnet configuration.
-	vaa.ChainIDEthereum: common.HexToAddress("0xDDb64fE46a91D46ee29420539FC25FD07c5FEa3E"),
+	vaa.ChainIDEthereum: "0xDDb64fE46a91D46ee29420539FC25FD07c5FEa3E",
 }

sdk/mainnet_consts.go

@@ -4,7 +4,6 @@ import (
 	"encoding/hex"
 	"fmt"
 
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/wormhole-foundation/wormhole/sdk/vaa"
 )
 
@@ -197,7 +196,7 @@ var KnownAutomaticRelayerEmitters = []struct {
 }
 
 // KnownWrappedNativeAddress is a map of wrapped native addresses by chain ID, e.g. WETH for Ethereum
-var KnownWrappedNativeAddress = map[vaa.ChainID]common.Address{
+var KnownWrappedNativeAddress = map[vaa.ChainID]string{
 	// WETH
-	vaa.ChainIDEthereum: common.HexToAddress("0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8"),
+	vaa.ChainIDEthereum: "0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8",
 }

sdk/testnet_consts.go

@@ -1,7 +1,6 @@
 package sdk
 
 import (
-	"github.com/ethereum/go-ethereum/common"
 	"github.com/wormhole-foundation/wormhole/sdk/vaa"
 )
 
@@ -107,9 +106,9 @@ var KnownTestnetAutomaticRelayerEmitters = []struct {
 }
 
 // KnownTestnetWrappedNativeAddresses is a list of addresses for deployments of wrapped native asssets (e.g. WETH) on various testnets.
-var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]common.Address{
+var KnownTestnetWrappedNativeAddresses = map[vaa.ChainID]string{
 	// WETH
-	vaa.ChainIDSepolia: common.HexToAddress("0x7b79995e5f793a07bc00c21412e50ecae098e7f9"),
+	vaa.ChainIDSepolia: "0x7b79995e5f793a07bc00c21412e50ecae098e7f9",
 	// WETH
-	vaa.ChainIDHolesky: common.HexToAddress("0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8"),
+	vaa.ChainIDHolesky: "0xc8f93d9738e7Ad5f3aF8c548DB2f6B7F8082B5e8",
 }