Konradstaniec/add remote signer module #33

KonradStaniec · 2024-11-20T09:45:08Z

Add remote signer to covenant-emulator repo
For now signer only have cosmos keyring from which it can retrieve private key
Signer should only accept signing request from covenant-emulator, as it does not do any validations

TODO: Wiring signer in covenant-emulator will be done in separate pr

Lazar955

Great job! Overall lgtm, some nits bellow. 🙌

Lazar955 · 2024-11-20T09:57:25Z

covenant-signer/Dockerfile

@@ -0,0 +1,37 @@
+FROM golang:1.22.3-alpine as builder


nit: let's use go 1.23

Lazar955 · 2024-11-20T10:03:32Z

covenant-signer/cmd/signerCmd.go

+
+		m.Start(metricsAddress, metrics.Registry)
+
+		// TODO: Add signal handling and gracefull shutdown


Let's start using context instead of chan and signal interrupts (see vigilante or benchmark repo). Can be done in separate PR

agree on that though I would to in separate pr 👍

Lazar955 · 2024-11-20T10:04:19Z

covenant-signer/config/config.go

+	serverConfig, err := cfg.Server.Parse()
+
+	if err != nil {


Suggested change

serverConfig, err := cfg.Server.Parse()

if err != nil {

serverConfig, err := cfg.Server.Parse()

if err != nil {

nit

Lazar955 · 2024-11-20T10:06:52Z

covenant-signer/config/metrics.go

+	Port int
+}
+
+func (cfg *MetricsConfig) Parse() (*ParsedMetricsConfig, error) {


why the need for both MetricsConfig and ParsedMetricsConfig?

We can just validate MatricsConfig

Ah I am fan of using as many types as possible i.e when you do MetricsConfig.Validate() the information about the validation is lost to all of the consumers of MetricsConfig i.e whatever code receives MetricsConfig it does not know whether config was already validated or not. This information lives solely in the head of the developer.

With parsing, any code that receives ParsedMetricsConfig knows that config was already validated i.e information about validation is encoded into the type system.

Lazar955 · 2024-11-20T10:09:40Z

covenant-signer/config/server.go

+	MaxContentLength uint32
+}
+
+func (c *ServerConfig) Parse() (*ParsedServerConfig, error) {


Same question for this one, why the need for 2 same structs

Lazar955 · 2024-11-20T10:23:13Z

covenant-signer/observability/tracing/tracing.go

@@ -0,0 +1,29 @@
+package tracing


Lazar955 · 2024-11-20T10:24:06Z

covenant-signer/signerapp/hardcoded_priv_key_retriever.go

@@ -0,0 +1,29 @@
+package signerapp


Inconsistency in filenames, should be: hardcodedprivkeyretriever.go

Lazar955 · 2024-11-20T10:24:20Z

covenant-signer/signerapp/hardcoded_priv_key_retriever.go

+	// return copy of the private key
+	bytes := r.privKey.Serialize()
+
+	newPrivKey, _ := btcec.PrivKeyFromBytes(bytes)


as above, btcec.PrivKeyFromBytes actually returns privateKey, publicKey tuple, no errors

Lazar955 · 2024-11-20T10:25:58Z

covenant-signer/signerservice/handlers/sign_transactions.go

@@ -0,0 +1,42 @@
+package handlers


file name style

Lazar955 · 2024-11-20T10:26:50Z

covenant-signer/signerservice/middlewares/content_length.go

+	"net/http"
+)
+
+func ContentLengthMiddleware(maxBytes int64) func(http.Handler) http.Handler {


we can have middleware for setting content type to json as well

what do you mean ?

gitferry

LGTM! Some future improvement could be reduce duplicate with covenant emulator

gitferry · 2024-11-20T13:03:52Z

covenant-signer/cmd/dumpDefaultCfgCmd.go

+
+var dumpCfgCmd = &cobra.Command{
+	Use:   "dump-cfg",
+	Short: "dumps default confiiguration file",


Suggested change

Short: "dumps default confiiguration file",

Short: "dumps default configuration file",

gitferry · 2024-11-20T13:05:30Z

covenant-signer/cmd/root.go

+	//   C:\Users\<username>\AppData\Local\tools on Windows
+	//   ~/.tools on Linux
+	//   ~/Library/Application Support/tools on MacOS


changed to signer (it was copy paste mistake 😅 )

gitferry · 2024-11-20T13:11:31Z

covenant-signer/keystore/cosmos/keyring.go

seems duplicate to covenant-emulator/keyring/keyring.go

gitferry · 2024-11-20T13:12:03Z

covenant-signer/keystore/cosmos/keyringcontroller.go

seems duplicate to covenant-emulator/keyring/keyringcontroller.go

gitferry · 2024-11-20T13:13:55Z

covenant-signer/utils/btc.go

this could be repo level utils

RafilxTenfen · 2024-11-21T00:17:00Z

covenant-signer/Dockerfile

+# Version to build. Default is the Git HEAD.
+ARG VERSION="HEAD"


version is not being used

RafilxTenfen · 2024-11-21T00:20:32Z

Mixed feelings about having this as a submodule, during the discussion I misunderstood and thought it would be like just a separate package folder '-'

In my opinion, since the covenant-emulator is a public repo, this remote-signer could go into a new repo...
Besides that all the comments were already made, nice work 💯

KonradStaniec · 2024-11-21T07:28:15Z

Mixed feelings about having this as a submodule, during the discussion I misunderstood and thought it would be like just a separate package folder '-'

In my opinion, since the covenant-emulator is a public repo, this remote-signer could go into a new repo...
Besides that all the comments were already made, nice work 💯

tbh I am also a bit unsure whether this is best approach, but I really do not want to have yet another repo, especially for such small program (one endpoint and key store) . Also this makes it super clear that phase-2 covenant signer must work together with covenant-emulator.

Eitherway, if after some time we will feel this setup is slowing us down, we will just copy paste it to separate repo 😅

KonradStaniec added 4 commits November 19, 2024 12:35

Add remote signer module

8066bf3

run tests through root make file

5ceac16

Adapt signer to phase-2

ed6bb0b

add cosmos keyring

a86f8d8

KonradStaniec requested review from gitferry, Lazar955 and RafilxTenfen November 20, 2024 09:45

add change log

3d4c63c

KonradStaniec requested review from filippos47 and maiquanghiep November 20, 2024 09:49

Lazar955 approved these changes Nov 20, 2024

View reviewed changes

KonradStaniec mentioned this pull request Nov 20, 2024

Use context.Context to control lifecycle of covenant signer #34

Open

pr comments

ba5b366

gitferry approved these changes Nov 20, 2024

View reviewed changes

Make gosec work

6cafc58

RafilxTenfen reviewed Nov 21, 2024

View reviewed changes

minor fixes

e8624d5

KonradStaniec merged commit 1717509 into main Nov 21, 2024
13 checks passed

KonradStaniec deleted the konradstaniec/add-remote-signer-module branch November 21, 2024 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Konradstaniec/add remote signer module #33

Konradstaniec/add remote signer module #33

KonradStaniec commented Nov 20, 2024 •

edited

Loading

Lazar955 left a comment

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

KonradStaniec Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

KonradStaniec Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

KonradStaniec Nov 20, 2024

Lazar955 Nov 20, 2024

Lazar955 Nov 20, 2024

KonradStaniec Nov 20, 2024

gitferry left a comment

gitferry Nov 20, 2024

gitferry Nov 20, 2024

KonradStaniec Nov 21, 2024

gitferry Nov 20, 2024

gitferry Nov 20, 2024

gitferry Nov 20, 2024

RafilxTenfen Nov 21, 2024

RafilxTenfen commented Nov 21, 2024

KonradStaniec commented Nov 21, 2024


		m.Start(metricsAddress, metrics.Registry)

		// TODO: Add signal handling and gracefull shutdown

	Short: "dumps default confiiguration file",
	Short: "dumps default configuration file",

		# Version to build. Default is the Git HEAD.
		ARG VERSION="HEAD"

Konradstaniec/add remote signer module #33

Konradstaniec/add remote signer module #33

Conversation

KonradStaniec commented Nov 20, 2024 • edited Loading

Lazar955 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RafilxTenfen commented Nov 21, 2024

KonradStaniec commented Nov 21, 2024

KonradStaniec commented Nov 20, 2024 •

edited

Loading