apply pr reviews

node/cmd/guardiand/node.go

@@ -658,7 +658,7 @@ func runNode(cmd *cobra.Command, args []string) {
 	// In devnet mode, we generate a deterministic guardian key and write it to disk.
 	if env == common.UnsafeDevNet {
 		// Only if the signer is file-based should we generate the deterministic key and write it to disk
-		if st, _ := guardiansigner.ParseSignerUri(*guardianSignerUri); st == guardiansigner.FileSignerType {
+		if st, _, _ := guardiansigner.ParseSignerUri(*guardianSignerUri); st == guardiansigner.FileSignerType {
 			err := devnet.GenerateAndStoreDevnetGuardianKey(*guardianKeyPath)
 			if err != nil {
 				logger.Fatal("failed to generate devnet guardian key", zap.Error(err))

node/pkg/guardiansigner/generatedsigner.go

@@ -9,6 +9,9 @@ import (
 	ethcrypto "github.com/ethereum/go-ethereum/crypto"
 )
 
+// The GeneratedSigner is a signer that is intended for use in tests. It uses the private
+// key supplied to GenerateSignerWithPrivatekeyUnsafe, or defaults to generating a random
+// private key if no private key is supplied.
 type GeneratedSigner struct {
 	privateKey *ecdsa.PrivateKey
 }
@@ -51,3 +54,10 @@ func (gs *GeneratedSigner) Verify(sig []byte, hash []byte) (valid bool, err erro
 
 	return recoveredPubKey.Equal(fsPubkey), nil
 }
+
+// This function is meant to be a helper function that returns a guardian signer for tests
+// that simply require a private key. The caller can specify a private key to be used, or
+// pass nil to have `NewGeneratedSigner` generate a random private key.
+func GenerateSignerWithPrivatekeyUnsafe(key *ecdsa.PrivateKey) (GuardianSigner, error) {
+	return NewGeneratedSigner(key)
+}

node/pkg/guardiansigner/guardiansigner.go

@@ -30,23 +30,27 @@ type GuardianSigner interface {
 func NewGuardianSignerFromUri(signerUri string, unsafeDevMode bool) (GuardianSigner, error) {
 
 	// Get the signer type
-	signerType, signerKeyConfig := ParseSignerUri(signerUri)
+	signerType, signerKeyConfig, err := ParseSignerUri(signerUri)
+
+	if err != nil {
+		return nil, err
+	}
 
 	switch signerType {
 	case FileSignerType:
 		return NewFileSigner(unsafeDevMode, signerKeyConfig)
 	default:
-		return nil, fmt.Errorf("unsupported guardian signer type")
+		return nil, fmt.Errorf("Unsupported guardian signer type")
 	}
 }
 
-func ParseSignerUri(signerUri string) (signerType SignerType, signerKeyConfig string) {
+func ParseSignerUri(signerUri string) (signerType SignerType, signerKeyConfig string, err error) {
 	// Split the URI using the standard "://" scheme separator
 	signerUriSplit := strings.Split(signerUri, "://")
 
 	// This check is purely for ensuring that there is actually a path separator.
 	if len(signerUriSplit) < 2 {
-		return InvalidSignerType, ""
+		return InvalidSignerType, "", fmt.Errorf("No path separator in guardian signer URI")
 	}
 
 	typeStr := signerUriSplit[0]
@@ -56,18 +60,8 @@ func ParseSignerUri(signerUri string) (signerType SignerType, signerKeyConfig st
 
 	switch typeStr {
 	case "file":
-		return FileSignerType, keyConfig
+		return FileSignerType, keyConfig, nil
 	default:
-		return InvalidSignerType, ""
+		return InvalidSignerType, "", fmt.Errorf("Unsupported guardian signer type: %s", typeStr)
 	}
 }
-
-// WARNING: DO NOT USE THIS SIGNER OUTSIDE OF TESTS
-//
-// This function is meant to be a helper function that returns a guardian signer for tests
-// that simply require a private key.
-// The caller can specify a private key to be used, or pass nil to have `NewGeneratedSigner`
-// generate a random private key.
-func GenerateSignerWithPrivatekeyUnsafe(key *ecdsa.PrivateKey) (GuardianSigner, error) {
-	return NewGeneratedSigner(key)
-}

node/pkg/guardiansigner/guardiansigner_test.go

@@ -25,9 +25,17 @@ func TestParseSignerUri(t *testing.T) {
 
 	for _, testcase := range tests {
 		t.Run(testcase.label, func(t *testing.T) {
-			signerType, _ := ParseSignerUri(testcase.path)
+			signerType, _, err := ParseSignerUri(testcase.path)
 
 			assert.Equal(t, signerType, testcase.expectedType)
+
+			// If the signer type is Invalid, then an error should have been returned.
+			if testcase.expectedType == InvalidSignerType {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+
 		})
 	}
 }
@@ -40,7 +48,7 @@ func TestFileSignerNonExistentFile(t *testing.T) {
 	assert.Error(t, err)
 
 	// Attempt to generate signer using NewFileSigner
-	_, keyPath := ParseSignerUri(nonexistentFileUri)
+	_, keyPath, _ := ParseSignerUri(nonexistentFileUri)
 	fileSigner, err := NewFileSigner(true, keyPath)
 	assert.Nil(t, fileSigner)
 	assert.Error(t, err)
@@ -63,8 +71,9 @@ func TestFileSigner(t *testing.T) {
 	assert.Equal(t, ethcrypto.PubkeyToAddress(fileSigner1.PublicKey()).Hex(), expectedEthAddress)
 
 	// Attempt to generate signer using NewFileSigner
-	signerType, keyPath := ParseSignerUri(fileUri)
+	signerType, keyPath, err := ParseSignerUri(fileUri)
 	assert.Equal(t, signerType, FileSignerType)
+	assert.NoError(t, err)
 
 	fileSigner2, err := NewFileSigner(true, keyPath)
 	assert.NoError(t, err)