apply pr reviews

wormhole-foundation · Oct 16, 2024 · 5c19304 · 5c19304
1 parent c453a1e
commit 5c19304
Show file tree

Hide file tree

Showing 6 changed files with 29 additions and 27 deletions.
diff --git a/node/cmd/guardiand/node.go b/node/cmd/guardiand/node.go
@@ -626,7 +626,7 @@ func runNode(cmd *cobra.Command, args []string) {
 	}
 	if *guardianKeyPath == "" {
 		// This if-statement is nested, since checking if both are empty at once will always result in the else-branch
-		// being executed if at least one is specified. For example, in the case where the singer URI is specified and
+		// being executed if at least one is specified. For example, in the case where the signer URI is specified and
 		// the guardianKeyPath not, then the else-statement will create an empty `file://` URI.
 		if *guardianSignerUri == "" {
 			logger.Fatal("Please specify --guardianKey or --guardianSignerUri")

diff --git a/node/pkg/adminrpc/adminserver.go b/node/pkg/adminrpc/adminserver.go
@@ -42,6 +42,7 @@ import (
 )
 
 const maxResetReleaseTimerDays = 7
+const ecdsaSignatureLength = 65
 
 var (
 	vaaInjectionsTotal = promauto.NewCounter(
@@ -1167,12 +1168,12 @@ func (s *nodePrivilegedService) SignExistingVAA(ctx context.Context, req *nodev1
 		panic(err)
 	}
 
-	sigData := [65]byte{}
-	copy(sigData[:], sig)
+	signature := [ecdsaSignatureLength]byte{}
+	copy(signature[:], sig)
 
 	newVAA.Signatures = append(v.Signatures, &vaa.Signature{
 		Index:     uint8(localGuardianIndex),
-		Signature: sigData,
+		Signature: signature,
 	})
 
 	// Sort VAA signatures by guardian ID

diff --git a/node/pkg/adminrpc/adminserver_test.go b/node/pkg/adminrpc/adminserver_test.go
@@ -107,7 +107,8 @@ func addrsToHexStrings(addrs []common.Address) (out []string) {
 	return
 }
 
-func generateMockVAA(gsIndex uint32, signers []guardiansigner.GuardianSigner) []byte {
+func generateMockVAA(gsIndex uint32, signers []guardiansigner.GuardianSigner, t *testing.T) []byte {
+	t.Helper()
 	v := &vaa.VAA{
 		Version:          1,
 		GuardianSetIndex: gsIndex,
@@ -123,15 +124,15 @@ func generateMockVAA(gsIndex uint32, signers []guardiansigner.GuardianSigner) []
 	for i, signer := range signers {
 		sig, err := signer.Sign(v.SigningDigest().Bytes())
 		if err != nil {
-			panic(err)
+			require.NoError(t, err)
 		}
 
-		sigData := [65]byte{}
-		copy(sigData[:], sig)
+		signature := [ecdsaSignatureLength]byte{}
+		copy(signature[:], sig)
 
 		v.Signatures = append(v.Signatures, &vaa.Signature{
 			Index:     uint8(i),
-			Signature: sigData,
+			Signature: signature,
 		})
 
 	}
@@ -182,7 +183,7 @@ func TestSignExistingVAA_NotGuardian(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers)
+	v := generateMockVAA(0, signers, t)
 
 	_, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
 		Vaa:                 v,
@@ -196,7 +197,7 @@ func TestSignExistingVAA_InvalidVAA(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers[:2])
+	v := generateMockVAA(0, signers[:2], t)
 
 	gsAddrs = append(gsAddrs, s.guardianAddress)
 	_, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
@@ -211,7 +212,7 @@ func TestSignExistingVAA_DuplicateGuardian(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers)
+	v := generateMockVAA(0, signers, t)
 
 	gsAddrs = append(gsAddrs, s.guardianAddress)
 	gsAddrs = append(gsAddrs, s.guardianAddress)
@@ -231,7 +232,7 @@ func TestSignExistingVAA_AlreadyGuardian(t *testing.T) {
 		guardianSetIndex: 0,
 	}
 
-	v := generateMockVAA(0, append(signers, s.guardianSigner))
+	v := generateMockVAA(0, append(signers, s.guardianSigner), t)
 
 	gsAddrs = append(gsAddrs, s.guardianAddress)
 	_, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
@@ -246,7 +247,7 @@ func TestSignExistingVAA_NotAFutureGuardian(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers)
+	v := generateMockVAA(0, signers, t)
 
 	_, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
 		Vaa:                 v,
@@ -260,7 +261,7 @@ func TestSignExistingVAA_CantReachQuorum(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers)
+	v := generateMockVAA(0, signers, t)
 
 	gsAddrs = append(gsAddrs, s.guardianAddress)
 	_, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
@@ -275,7 +276,7 @@ func TestSignExistingVAA_Valid(t *testing.T) {
 	signers, gsAddrs := generateGuardianSigners(5)
 	s := setupAdminServerForVAASigning(0, gsAddrs)
 
-	v := generateMockVAA(0, signers)
+	v := generateMockVAA(0, signers, t)
 
 	gsAddrs = append(gsAddrs, s.guardianAddress)
 	res, err := s.SignExistingVAA(context.Background(), &nodev1.SignExistingVAARequest{
@@ -285,7 +286,7 @@ func TestSignExistingVAA_Valid(t *testing.T) {
 	})
 
 	require.NoError(t, err)
-	v2 := generateMockVAA(1, append(signers, s.guardianSigner))
+	v2 := generateMockVAA(1, append(signers, s.guardianSigner), t)
 	require.Equal(t, v2, res.Vaa)
 }
 

diff --git a/node/pkg/guardiansigner/generatedsigner.go b/node/pkg/guardiansigner/generatedsigner.go
@@ -5,7 +5,6 @@ import (
 	"crypto/rand"
 	"fmt"
 
-	"github.com/ethereum/go-ethereum/crypto"
 	ethcrypto "github.com/ethereum/go-ethereum/crypto"
 )
 
@@ -18,7 +17,7 @@ type GeneratedSigner struct {
 
 func NewGeneratedSigner(key *ecdsa.PrivateKey) (*GeneratedSigner, error) {
 	if key == nil {
-		privateKey, err := ecdsa.GenerateKey(crypto.S256(), rand.Reader)
+		privateKey, err := ecdsa.GenerateKey(ethcrypto.S256(), rand.Reader)
 		return &GeneratedSigner{privateKey: privateKey}, err
 	} else {
 		return &GeneratedSigner{privateKey: key}, nil
@@ -28,10 +27,10 @@ func NewGeneratedSigner(key *ecdsa.PrivateKey) (*GeneratedSigner, error) {
 
 func (gs *GeneratedSigner) Sign(hash []byte) (sig []byte, err error) {
 	// Sign the hash
-	sig, err = crypto.Sign(hash, gs.privateKey)
+	sig, err = ethcrypto.Sign(hash, gs.privateKey)
 
 	if err != nil {
-		return nil, fmt.Errorf("failed to sign wormchain address: %w", err)
+		return nil, fmt.Errorf("failed to sign: %w", err)
 	}
 
 	return sig, nil

diff --git a/node/pkg/guardiansigner/guardiansigner.go b/node/pkg/guardiansigner/guardiansigner.go
@@ -41,7 +41,7 @@ func NewGuardianSignerFromUri(signerUri string, unsafeDevMode bool) (GuardianSig
 	case FileSignerType:
 		return NewFileSigner(unsafeDevMode, signerKeyConfig)
 	default:
-		return nil, errors.New("Unsupported guardian signer type")
+		return nil, errors.New("unsupported guardian signer type")
 	}
 }
 
@@ -51,7 +51,7 @@ func ParseSignerUri(signerUri string) (signerType SignerType, signerKeyConfig st
 
 	// This check is purely for ensuring that there is actually a path separator.
 	if len(signerUriSplit) < 2 {
-		return InvalidSignerType, "", errors.New("No path separator in guardian signer URI")
+		return InvalidSignerType, "", errors.New("no path separator in guardian signer URI")
 	}
 
 	typeStr := signerUriSplit[0]
@@ -63,6 +63,6 @@ func ParseSignerUri(signerUri string) (signerType SignerType, signerKeyConfig st
 	case "file":
 		return FileSignerType, keyConfig, nil
 	default:
-		return InvalidSignerType, "", fmt.Errorf("Unsupported guardian signer type: %s", typeStr)
+		return InvalidSignerType, "", fmt.Errorf("unsupported guardian signer type: %s", typeStr)
 	}
 }
diff --git a/node/pkg/guardiansigner/guardiansigner_test.go b/node/pkg/guardiansigner/guardiansigner_test.go
@@ -6,6 +6,7 @@ import (
 	"github.com/ethereum/go-ethereum/crypto"
 	ethcrypto "github.com/ethereum/go-ethereum/crypto"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestParseSignerUri(t *testing.T) {
@@ -66,17 +67,17 @@ func TestFileSigner(t *testing.T) {
 
 	// Attempt to generate signer using top-level generator
 	fileSigner1, err := NewGuardianSignerFromUri(fileUri, true)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.NotNil(t, fileSigner1)
 	assert.Equal(t, ethcrypto.PubkeyToAddress(fileSigner1.PublicKey()).Hex(), expectedEthAddress)
 
 	// Attempt to generate signer using NewFileSigner
 	signerType, keyPath, err := ParseSignerUri(fileUri)
 	assert.Equal(t, signerType, FileSignerType)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	fileSigner2, err := NewFileSigner(true, keyPath)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.NotNil(t, fileSigner2)
 	assert.Equal(t, ethcrypto.PubkeyToAddress(fileSigner2.PublicKey()).Hex(), expectedEthAddress)