Refactor ArtifactSignature to provide signature as hex string (Consen…

…sys#1036) - Refactor ArtifactSignature to use asHex method to return signature - Remove SignatureFormatter functional interface - Update SignerForIdentifie - Use generics with signAndGetArtifactSignature
usmansaleem · Nov 21, 2024 · d058255 · d058255
1 parent 1e0a1e2
commit d058255
Show file tree

Hide file tree

Showing 25 changed files with 136 additions and 174 deletions.
diff --git a/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java b/core/src/main/java/tech/pegasys/web3signer/core/Eth2Runner.java
@@ -235,13 +235,12 @@ private MappedResults<ArtifactSigner> bulkLoadSigners(
 
     if (keystoresParameters.isEnabled()) {
       LOG.info("Bulk loading keys from local keystores ... ");
-      final BlsKeystoreBulkLoader blsKeystoreBulkLoader = new BlsKeystoreBulkLoader();
       final MappedResults<ArtifactSigner> keystoreSignersResult =
           keystoresParameters.hasKeystoresPasswordsPath()
-              ? blsKeystoreBulkLoader.loadKeystoresUsingPasswordDir(
+              ? BlsKeystoreBulkLoader.loadKeystoresUsingPasswordDir(
                   keystoresParameters.getKeystoresPath(),
                   keystoresParameters.getKeystoresPasswordsPath())
-              : blsKeystoreBulkLoader.loadKeystoresUsingPasswordFile(
+              : BlsKeystoreBulkLoader.loadKeystoresUsingPasswordFile(
                   keystoresParameters.getKeystoresPath(),
                   keystoresParameters.getKeystoresPasswordFile());
       LOG.info(

diff --git a/core/src/main/java/tech/pegasys/web3signer/core/routes/eth1/Eth1SignRoute.java b/core/src/main/java/tech/pegasys/web3signer/core/routes/eth1/Eth1SignRoute.java
@@ -20,7 +20,6 @@
 import tech.pegasys.web3signer.core.service.http.handlers.signing.SignerForIdentifier;
 import tech.pegasys.web3signer.core.service.http.metrics.HttpApiMetrics;
 import tech.pegasys.web3signer.signing.ArtifactSignerProvider;
-import tech.pegasys.web3signer.signing.SecpArtifactSignature;
 import tech.pegasys.web3signer.signing.config.DefaultArtifactSignerProvider;
 
 import java.util.Optional;
@@ -33,7 +32,7 @@ public class Eth1SignRoute implements Web3SignerRoute {
 
   private final Context context;
   private final ArtifactSignerProvider signerProvider;
-  private final SignerForIdentifier<SecpArtifactSignature> secpSigner;
+  private final SignerForIdentifier secpSigner;
 
   public Eth1SignRoute(final Context context) {
     this.context = context;
@@ -46,9 +45,7 @@ public Eth1SignRoute(final Context context) {
 
     if (first.isPresent()) {
       signerProvider = first.get();
-      secpSigner =
-          new SignerForIdentifier<>(
-              signerProvider, sig -> SecpArtifactSignature.toBytes(sig).toHexString(), SECP256K1);
+      secpSigner = new SignerForIdentifier(signerProvider);
     } else {
       throw new IllegalStateException(
           "No DefaultArtifactSignerProvider found in Context for eth1 mode");

diff --git a/core/src/main/java/tech/pegasys/web3signer/core/routes/eth1/JsonRpcRoute.java b/core/src/main/java/tech/pegasys/web3signer/core/routes/eth1/JsonRpcRoute.java
@@ -12,8 +12,6 @@
  */
 package tech.pegasys.web3signer.core.routes.eth1;
 
-import static tech.pegasys.web3signer.signing.KeyType.SECP256K1;
-
 import tech.pegasys.web3signer.core.Context;
 import tech.pegasys.web3signer.core.Runner;
 import tech.pegasys.web3signer.core.WebClientOptionsFactory;
@@ -38,11 +36,8 @@
 import tech.pegasys.web3signer.core.service.jsonrpc.handlers.sendtransaction.SendTransactionHandler;
 import tech.pegasys.web3signer.core.service.jsonrpc.handlers.sendtransaction.transaction.TransactionFactory;
 import tech.pegasys.web3signer.signing.ArtifactSignerProvider;
-import tech.pegasys.web3signer.signing.SecpArtifactSignature;
 import tech.pegasys.web3signer.signing.config.SecpArtifactSignerProviderAdapter;
 
-import java.util.Optional;
-
 import io.vertx.core.Vertx;
 import io.vertx.core.http.HttpClient;
 import io.vertx.core.http.HttpMethod;
@@ -62,18 +57,17 @@ public class JsonRpcRoute implements Web3SignerRoute {
   public JsonRpcRoute(final Context context, final Eth1Config eth1Config) {
     this.context = context;
 
-    // we need signerProvider which is an instance of SecpArtifactSignerProviderAdapter
-    final Optional<ArtifactSignerProvider> first =
+    // we need signerProvider which is an instance of SecpArtifactSignerProviderAdapter which uses
+    // eth1 address as identifier
+    final ArtifactSignerProvider signerProvider =
         context.getArtifactSignerProviders().stream()
             .filter(provider -> provider instanceof SecpArtifactSignerProviderAdapter)
-            .findFirst();
-    final ArtifactSignerProvider signerProvider;
-    if (first.isPresent()) {
-      signerProvider = first.get();
-    } else {
-      throw new IllegalStateException(
-          "No SecpArtifactSignerProviderAdapter found in Context for eth1 mode");
-    }
+            .findFirst()
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        "No SecpArtifactSignerProviderAdapter found in Context for eth1 mode"));
+
     // use same instance of downstreamHttpClient and path calculator for all requests
     final HttpClient downstreamHttpClient =
         createDownstreamHttpClient(eth1Config, context.getVertx());
@@ -126,11 +120,8 @@ private static RequestMapper createRequestMapper(
       final long chainId) {
     final PassThroughHandler defaultHandler =
         new PassThroughHandler(transmitterFactory, JSON_DECODER);
-    final SignerForIdentifier<SecpArtifactSignature> secpSigner =
-        new SignerForIdentifier<>(
-            signerProviderMappedToEth1Address,
-            sig -> SecpArtifactSignature.toBytes(sig).toHexString(),
-            SECP256K1);
+    final SignerForIdentifier secpSigner =
+        new SignerForIdentifier(signerProviderMappedToEth1Address);
     final TransactionFactory transactionFactory =
         new TransactionFactory(chainId, JSON_DECODER, transmitterFactory);
     final SendTransactionHandler sendTransactionHandler =

diff --git a/core/src/main/java/tech/pegasys/web3signer/core/routes/eth2/Eth2SignExtensionRoute.java b/core/src/main/java/tech/pegasys/web3signer/core/routes/eth2/Eth2SignExtensionRoute.java
@@ -12,14 +12,11 @@
  */
 package tech.pegasys.web3signer.core.routes.eth2;
 
-import static tech.pegasys.web3signer.signing.KeyType.BLS;
-
 import tech.pegasys.web3signer.core.Context;
 import tech.pegasys.web3signer.core.routes.Web3SignerRoute;
 import tech.pegasys.web3signer.core.service.http.handlers.signing.SignerForIdentifier;
 import tech.pegasys.web3signer.core.service.http.handlers.signing.SigningExtensionHandler;
 import tech.pegasys.web3signer.signing.ArtifactSignerProvider;
-import tech.pegasys.web3signer.signing.BlsArtifactSignature;
 
 import io.vertx.core.http.HttpMethod;
 import io.vertx.core.json.JsonObject;
@@ -28,19 +25,22 @@ public class Eth2SignExtensionRoute implements Web3SignerRoute {
   public static final String SIGN_EXT_PATH = "/api/v1/eth2/ext/sign/:identifier";
 
   private final Context context;
-  private final SignerForIdentifier<BlsArtifactSignature> blsSigner;
+  private final SignerForIdentifier blsSigner;
 
   public Eth2SignExtensionRoute(final Context context) {
     this.context = context;
 
     // there should be only one ArtifactSignerProvider in eth2 mode at the moment which is of BLS
     // types.
     final ArtifactSignerProvider artifactSignerProvider =
-        context.getArtifactSignerProviders().stream().findFirst().orElseThrow();
+        context.getArtifactSignerProviders().stream()
+            .findFirst()
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        "No ArtifactSignerProvider found in Context for eth2 mode"));
 
-    blsSigner =
-        new SignerForIdentifier<>(
-            artifactSignerProvider, sig -> sig.getSignatureData().toString(), BLS);
+    blsSigner = new SignerForIdentifier(artifactSignerProvider);
   }
 
   @Override

diff --git a/core/src/main/java/tech/pegasys/web3signer/core/routes/eth2/Eth2SignRoute.java b/core/src/main/java/tech/pegasys/web3signer/core/routes/eth2/Eth2SignRoute.java
@@ -23,7 +23,6 @@
 import tech.pegasys.web3signer.core.service.http.handlers.signing.eth2.Eth2SignForIdentifierHandler;
 import tech.pegasys.web3signer.core.service.http.metrics.HttpApiMetrics;
 import tech.pegasys.web3signer.signing.ArtifactSignerProvider;
-import tech.pegasys.web3signer.signing.BlsArtifactSignature;
 import tech.pegasys.web3signer.slashingprotection.SlashingProtection;
 import tech.pegasys.web3signer.slashingprotection.SlashingProtectionContext;
 
@@ -36,7 +35,7 @@
 public class Eth2SignRoute implements Web3SignerRoute {
   private static final String SIGN_PATH = "/api/v1/eth2/sign/:identifier";
   private final Context context;
-  private final SignerForIdentifier<BlsArtifactSignature> blsSigner;
+  private final SignerForIdentifier blsSigner;
   private final ObjectMapper objectMapper = SigningObjectMapperFactory.createObjectMapper();
   private final Spec eth2Spec;
   private final Optional<SlashingProtection> slashingProtection;
@@ -52,11 +51,14 @@ public Eth2SignRoute(
     // there should be only one ArtifactSignerProvider in eth2 mode at the moment which is of BLS
     // types.
     final ArtifactSignerProvider artifactSignerProvider =
-        context.getArtifactSignerProviders().stream().findFirst().orElseThrow();
+        context.getArtifactSignerProviders().stream()
+            .findFirst()
+            .orElseThrow(
+                () ->
+                    new IllegalStateException(
+                        "No ArtifactSignerProvider found in Context for eth2 mode"));
 
-    blsSigner =
-        new SignerForIdentifier<>(
-            artifactSignerProvider, sig -> sig.getSignatureData().toString(), BLS);
+    blsSigner = new SignerForIdentifier(artifactSignerProvider);
   }
 
   @Override

diff --git a/...h/pegasys/web3signer/core/service/http/handlers/signing/Eth1SignForIdentifierHandler.java b/...h/pegasys/web3signer/core/service/http/handlers/signing/Eth1SignForIdentifierHandler.java
@@ -14,7 +14,6 @@
 
 import static io.vertx.core.http.HttpHeaders.CONTENT_TYPE;
 import static tech.pegasys.web3signer.core.service.http.handlers.ContentTypes.TEXT_PLAIN_UTF_8;
-import static tech.pegasys.web3signer.core.service.http.handlers.signing.SignerForIdentifier.toBytes;
 import static tech.pegasys.web3signer.signing.util.IdentifierUtils.normaliseIdentifier;
 
 import tech.pegasys.web3signer.core.service.http.metrics.HttpApiMetrics;
@@ -23,6 +22,7 @@
 import io.vertx.core.json.JsonObject;
 import io.vertx.ext.web.RequestBody;
 import io.vertx.ext.web.RoutingContext;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes;
@@ -31,11 +31,11 @@
 public class Eth1SignForIdentifierHandler implements Handler<RoutingContext> {
 
   private static final Logger LOG = LogManager.getLogger();
-  private final SignerForIdentifier<?> signerForIdentifier;
+  private final SignerForIdentifier signerForIdentifier;
   private final HttpApiMetrics metrics;
 
   public Eth1SignForIdentifierHandler(
-      final SignerForIdentifier<?> signerForIdentifier, final HttpApiMetrics metrics) {
+      final SignerForIdentifier signerForIdentifier, final HttpApiMetrics metrics) {
     this.signerForIdentifier = signerForIdentifier;
     this.metrics = metrics;
   }
@@ -47,7 +47,7 @@ public void handle(final RoutingContext routingContext) {
       final Bytes data;
       try {
         data = getDataToSign(routingContext.body());
-      } catch (final IllegalArgumentException e) {
+      } catch (final RuntimeException e) {
         metrics.getMalformedRequestCounter().inc();
         LOG.debug("Invalid signing request", e);
         routingContext.fail(400);
@@ -72,6 +72,14 @@ private void respondWithSignature(final RoutingContext routingContext, final Str
 
   private Bytes getDataToSign(final RequestBody requestBody) {
     final JsonObject jsonObject = requestBody.asJsonObject();
-    return toBytes(jsonObject.getString("data"));
+
+    if (!jsonObject.containsKey("data")) {
+      throw new IllegalArgumentException("Request must contain a 'data' field");
+    }
+    if (StringUtils.isBlank(jsonObject.getString("data"))) {
+      throw new IllegalArgumentException("Data field must not be empty");
+    }
+
+    return Bytes.fromHexString(jsonObject.getString("data"));
   }
 }
diff --git a/...n/java/tech/pegasys/web3signer/core/service/http/handlers/signing/SignatureFormatter.java b/...n/java/tech/pegasys/web3signer/core/service/http/handlers/signing/SignatureFormatter.java
diff --git a/.../java/tech/pegasys/web3signer/core/service/http/handlers/signing/SignerForIdentifier.java b/.../java/tech/pegasys/web3signer/core/service/http/handlers/signing/SignerForIdentifier.java
@@ -14,28 +14,20 @@
 
 import tech.pegasys.web3signer.signing.ArtifactSignature;
 import tech.pegasys.web3signer.signing.ArtifactSignerProvider;
-import tech.pegasys.web3signer.signing.KeyType;
 
 import java.util.Optional;
 
-import org.apache.commons.lang3.StringUtils;
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.apache.tuweni.bytes.Bytes;
 
-public class SignerForIdentifier<T extends ArtifactSignature> {
-  private static final Logger LOG = LogManager.getLogger();
+/**
+ * This class wraps the {@link ArtifactSignerProvider} and provides a way to check if a signer is
+ * available for a given identifier and to sign a message.
+ */
+public class SignerForIdentifier {
   private final ArtifactSignerProvider signerProvider;
-  private final SignatureFormatter<T> signatureFormatter;
-  private final KeyType type;
 
-  public SignerForIdentifier(
-      final ArtifactSignerProvider signerProvider,
-      final SignatureFormatter<T> signatureFormatter,
-      final KeyType type) {
+  public SignerForIdentifier(final ArtifactSignerProvider signerProvider) {
     this.signerProvider = signerProvider;
-    this.signatureFormatter = signatureFormatter;
-    this.type = type;
   }
 
   /**
@@ -48,43 +40,21 @@ public SignerForIdentifier(
    * @throws IllegalArgumentException if data is invalid i.e. not a valid hex string, null or empty.
    */
   public Optional<String> sign(final String identifier, final Bytes data) {
-    return signerProvider.getSigner(identifier).map(signer -> formatSignature(signer.sign(data)));
-  }
-
-  @SuppressWarnings("unchecked")
-  public Optional<T> signAndGetArtifactSignature(final String identifier, final Bytes data) {
-    return signerProvider.getSigner(identifier).map(signer -> (T) signer.sign(data));
+    return signerProvider.getSigner(identifier).map(signer -> signer.sign(data).asHex());
   }
 
   /**
-   * Converts hex string to bytes
+   * Sign data for given identifier and return ArtifactSignature. Useful for SECP signing usages.
    *
-   * @param data hex string
-   * @return Bytes
-   * @throws IllegalArgumentException if data is invalid i.e. not a valid hex string, null or empty
+   * @param <T> The specific type of ArtifactSignature
+   * @param identifier The identifier for which to sign data.
+   * @param data Bytes which is signed
+   * @return Optional ArtifactSignature of type T. Empty if no signer available for given identifier
    */
-  public static Bytes toBytes(final String data) {
-    final Bytes dataToSign;
-    try {
-      if (StringUtils.isBlank(data)) {
-        throw new IllegalArgumentException("Blank data");
-      }
-      dataToSign = Bytes.fromHexString(data);
-    } catch (final IllegalArgumentException e) {
-      LOG.debug("Invalid hex string {}", data, e);
-      throw e;
-    }
-    return dataToSign;
-  }
-
   @SuppressWarnings("unchecked")
-  private String formatSignature(final ArtifactSignature signature) {
-    if (signature.getType() == type) {
-      final T artifactSignature = (T) signature;
-      return signatureFormatter.format(artifactSignature);
-    } else {
-      throw new IllegalStateException("Invalid signature type");
-    }
+  public <T extends ArtifactSignature<?>> Optional<T> signAndGetArtifactSignature(
+      final String identifier, final Bytes data) {
+    return signerProvider.getSigner(identifier).map(signer -> (T) signer.sign(data));
   }
 
   /**

diff --git a/...a/tech/pegasys/web3signer/core/service/http/handlers/signing/SigningExtensionHandler.java b/...a/tech/pegasys/web3signer/core/service/http/handlers/signing/SigningExtensionHandler.java
@@ -36,9 +36,9 @@ public class SigningExtensionHandler implements Handler<RoutingContext> {
           .copy()
           .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
 
-  private final SignerForIdentifier<?> signerForIdentifier;
+  private final SignerForIdentifier signerForIdentifier;
 
-  public SigningExtensionHandler(final SignerForIdentifier<?> signerForIdentifier) {
+  public SigningExtensionHandler(final SignerForIdentifier signerForIdentifier) {
     this.signerForIdentifier = signerForIdentifier;
   }
 

diff --git a/...asys/web3signer/core/service/http/handlers/signing/eth2/Eth2SignForIdentifierHandler.java b/...asys/web3signer/core/service/http/handlers/signing/eth2/Eth2SignForIdentifierHandler.java
@@ -56,7 +56,7 @@
 public class Eth2SignForIdentifierHandler implements Handler<RoutingContext> {
 
   private static final Logger LOG = LogManager.getLogger();
-  private final SignerForIdentifier<?> signerForIdentifier;
+  private final SignerForIdentifier signerForIdentifier;
   private final HttpApiMetrics httpMetrics;
   private final SlashingProtectionMetrics slashingMetrics;
   private final Optional<SlashingProtection> slashingProtection;
@@ -69,7 +69,7 @@ public class Eth2SignForIdentifierHandler implements Handler<RoutingContext> {
   public static final int SLASHING_PROTECTION_ENFORCED = 412;
 
   public Eth2SignForIdentifierHandler(
-      final SignerForIdentifier<?> signerForIdentifier,
+      final SignerForIdentifier signerForIdentifier,
       final HttpApiMetrics httpMetrics,
       final SlashingProtectionMetrics slashingMetrics,
       final Optional<SlashingProtection> slashingProtection,