Review feedback

Author: alpe

docs/guides/operations/aws-kms-signer.md

@@ -59,7 +59,7 @@ signer:
       key_id: "arn:aws:kms:us-east-1:123456789012:key/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx"
       region: "us-east-1"    # optional but recommended
       profile: "prod"        # optional; omit when using IAM role/env creds
-      timeout: "10s"         # must be > 0
+      timeout: "1s"         # must be > 0
       max_retries: 3         # must be >= 0
 ```
 

pkg/config/config.go

@@ -312,7 +312,7 @@ type P2PConfig struct {
 
 // SignerConfig contains all signer configuration parameters
 type SignerConfig struct {
-	SignerType string          `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, grpc, kms)"`
+	SignerType string          `mapstructure:"signer_type" yaml:"signer_type" comment:"Type of remote signer to use (file, kms)"`
 	SignerPath string          `mapstructure:"signer_path" yaml:"signer_path" comment:"Path to the signer file or address"`
 	KMS        SignerKMSConfig `mapstructure:"kms" yaml:"kms"`
 }

pkg/signer/aws/signer.go

@@ -158,6 +158,9 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
 	maxAttempts := maxRetries + 1
 
 	for attempt := 0; attempt < maxAttempts; attempt++ {
+		if err := ctx.Err(); err != nil {
+			return nil, err
+		}
 		if attempt > 0 {
 			// Exponential backoff: 100ms, 200ms, 400ms, ...
 			backoff := time.Duration(100<<uint(attempt-1)) * time.Millisecond
@@ -180,15 +183,15 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
 		if err != nil {
 			lastErr = err
 			if !isRetryableKMSError(err) {
-				return nil, fmt.Errorf("KMS Sign failed with non-retryable error: %w", err)
+				return nil, fmt.Errorf("AWS KMS sign failed with non-retryable error: %w", err)
 			}
 			continue
 		}
 
 		return out.Signature, nil
 	}
 
-	return nil, fmt.Errorf("KMS Sign failed after %d attempts: %w", maxAttempts, lastErr)
+	return nil, fmt.Errorf("AWS KMS sign failed after %d attempts: %w", maxAttempts, lastErr)
 }
 
 // GetPublic returns the cached public key.
@@ -212,8 +215,9 @@ func (s *KmsSigner) GetAddress() ([]byte, error) {
 	if s.address == nil {
 		return nil, fmt.Errorf("address not loaded")
 	}
-
-	return s.address, nil
+	r := make([]byte, len(s.address))
+	copy(r, s.address)
+	return r, nil
 }
 
 func isRetryableKMSError(err error) bool {

pkg/signer/aws/signer_test.go

@@ -131,7 +131,7 @@ func TestSign_KMSFailure(t *testing.T) {
 
 	_, err = s.Sign(context.Background(), []byte("hello world"))
 	require.Error(t, err)
-	assert.Contains(t, err.Error(), "KMS Sign failed")
+	assert.Contains(t, err.Error(), "AWS KMS sign failed")
 	assert.Equal(t, int32(4), atomic.LoadInt32(&calls), "default retries should make 4 attempts")
 }
 

pkg/signer/factory.go

@@ -30,8 +30,10 @@ func newSigner(ctx context.Context, config *rollconf.Config, passphrase string,
 		if passphrase == "" {
 			return nil, fmt.Errorf("passphrase is required when using local file signer")
 		}
+		if config.Signer.SignerPath == "" {
+			return nil, fmt.Errorf("signer path is required when using local file signer")
+		}
 
-		// Resolve signer path; allow absolute, relative to node root, or relative to CWD if resolution fails
 		signerPath, err := filepath.Abs(strings.TrimSuffix(config.Signer.SignerPath, "signer.json"))
 		if err != nil {
 			return nil, err

pkg/signer/gcp/signer.go

@@ -105,7 +105,7 @@ func kmsSignerFromClient(ctx context.Context, client KMSClient, keyName string,
 		return nil, fmt.Errorf("gcp kms client is required")
 	}
 
-	o := Options{Timeout: 10 * time.Second, MaxRetries: 3}
+	o := Options{Timeout: 1 * time.Second, MaxRetries: 3}
 	if opts != nil {
 		if opts.Timeout > 0 {
 			o.Timeout = opts.Timeout
@@ -181,6 +181,9 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
 	maxAttempts := maxRetries + 1
 
 	for attempt := 0; attempt < maxAttempts; attempt++ {
+		if err := ctx.Err(); err != nil {
+			return nil, err
+		}
 		if attempt > 0 {
 			// Exponential backoff with cap: 100ms, 200ms, 400ms, ... up to 5s.
 			backoff := retryBackoff(attempt)
@@ -203,7 +206,7 @@ func (s *KmsSigner) Sign(ctx context.Context, message []byte) ([]byte, error) {
 		if err != nil {
 			lastErr = err
 			if !isRetryableKMSError(err) {
-				return nil, fmt.Errorf("KMS Sign failed with non-retryable error: %w", err)
+				return nil, fmt.Errorf("GCP KMS sign failed with non-retryable error: %w", err)
 			}
 			continue
 		}
@@ -279,8 +282,9 @@ func (s *KmsSigner) GetAddress() ([]byte, error) {
 	if s.address == nil {
 		return nil, fmt.Errorf("address not loaded")
 	}
-
-	return s.address, nil
+	r := make([]byte, len(s.address))
+	copy(r, s.address)
+	return r, nil
 }
 
 func isRetryableKMSError(err error) bool {

test/e2e/evm_kms_e2e_test.go

@@ -4,7 +4,6 @@ package e2e
 
 import (
 	"context"
-	"flag"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -29,8 +28,6 @@ func TestEvmSequencerWithAWSKMSSignerE2E(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip e2e in short mode")
 	}
-	flag.Parse()
-
 	kmsKeyID := os.Getenv("EVNODE_E2E_AWS_KMS_KEY_ID")
 	if kmsKeyID == "" {
 		t.Skip("set EVNODE_E2E_AWS_KMS_KEY_ID to run AWS KMS EVM e2e test")
@@ -69,7 +66,6 @@ func TestEvmSequencerWithGCPKMSSignerE2E(t *testing.T) {
 	if testing.Short() {
 		t.Skip("skip e2e in short mode")
 	}
-	flag.Parse()
 
 	kmsKeyName := os.Getenv("EVNODE_E2E_GCP_KMS_KEY_NAME")
 	if kmsKeyName == "" {

types/utils.go

@@ -195,7 +195,7 @@ func GetRandomNextSignedHeader(ctx context.Context, signedHeader *SignedHeader,
 		Signer: signedHeader.Signer,
 	}
 
-	signature, err := GetSignature(ctx, signedHeader.Header, signer)
+	signature, err := GetSignature(ctx, newSignedHeader.Header, signer)
 	if err != nil {
 		return nil, err
 	}

types/utils_test.go

@@ -1,7 +1,6 @@
 package types_test
 
 import (
-	"context"
 	// Import bytes package
 	"testing"
 	"time" // Used for time.Time comparison
@@ -79,7 +78,7 @@ func TestGetFirstSignedHeader(t *testing.T) {
 			noopSigner, err := noop.NewNoopSigner(privKey)
 			assert.NoError(t, err)
 
-			firstSignedHeader, err := types.GetFirstSignedHeader(context.Background(), noopSigner, tc.chainID)
+			firstSignedHeader, err := types.GetFirstSignedHeader(t.Context(), noopSigner, tc.chainID)
 			assert.NoError(t, err)
 			assert.NotNil(t, firstSignedHeader)
 			assert.Equal(t, uint64(1), firstSignedHeader.Height())