chore: Infer key type from public key and private key #535

olimpias · 2020-02-02T21:58:52Z

Solves #403

golangcibot · 2020-02-02T22:01:24Z

crypto/multikey/kind.go

+		if err != nil {
+			continue
+		}
+		matches = append(matches, cPrivateKey)


append only allowed to cuddle with appended value (from wsl)

golangcibot · 2020-02-02T22:01:24Z

crypto/multikey/kind.go

+// Supported private key types are defined in possibleKeyKinds variable.
+func GetKeyKindFromBytes(publicKey []byte, privateKey []byte) (crypto.PrivateKey, error) {
+	matches := make([]crypto.PrivateKey, 0, 1)
+	for _, keyKind := range possibleKeyKinds {


ranges should only be cuddled with assignments used in the iteration (from wsl)

golangcibot · 2020-02-02T22:01:24Z

crypto/multikey/kind.go

+		}
+		matches = append(matches, cPrivateKey)
+	}
+	switch len(matches) {


only one cuddle assignment allowed before switch statement (from wsl)

golangcibot · 2020-02-02T22:01:25Z

crypto/multikey/kind.go

+		if err != nil {
+			return nil, err
+		}
+		if bytes.Equal(privateKey.PublicKey().Bytes(), publicKey) {


if statements should only be cuddled with assignments (from wsl)

golangcibot · 2020-02-02T22:01:25Z

crypto/multikey/kind.go

+		if err != nil {
+			return nil, err
+		}
+		if bytes.Equal(privateKey.PublicKey().Bytes(), publicKey) {


if statements should only be cuddled with assignments (from wsl)

golangcibot · 2020-02-02T22:01:26Z

crypto/multikey/kind.go

+	default:
+		return nil, ErrKindNotFound
+	}
+	return nil, errPrivateAndPublicKeyNotMatched


return statements should not be cuddled if block has more than two lines (from wsl)

golangcibot · 2020-02-02T22:01:26Z

crypto/multikey/kind.go

+	}
+}
+
+func verifyPrivateAndPublicKey(publicKey []byte, privateKey []byte, kind string) (crypto.PrivateKey, error) {


cyclomatic complexity 11 of func verifyPrivateAndPublicKey is high (> 10) (from gocyclo)

golangcibot · 2020-02-02T22:01:26Z

crypto/multikey/kind.go

+	ErrKindNotFound = errors.New("key kind is not found")
+
+	// Possible key kinds for now
+	possibleKeyKinds = []string{crypto.KindED25519, crypto.KindSECP256K1, crypto.KindSR25519}


possibleKeyKinds is a global variable (from gochecknoglobals)

golangcibot · 2020-02-02T22:01:26Z

crypto/multikey/kind.go

@@ -56,6 +70,59 @@ func KeyKindFromSignature(pubKey, message, sig []byte, keyKinds []string) (crypt
 	}
 }

+// GetKeyKindFromBytes extracts the private key type from the publicKey and privateKey.
+// Supported private key types are defined in possibleKeyKinds variable.
+func GetKeyKindFromBytes(publicKey []byte, privateKey []byte) (crypto.PrivateKey, error) {


paramTypeCombine: func(publicKey []byte, privateKey []byte) (crypto.PrivateKey, error) could be replaced with func(publicKey, privateKey []byte) (crypto.PrivateKey, error) (from gocritic)

golangcibot · 2020-02-02T22:01:27Z

crypto/multikey/kind.go

+	switch len(matches) {
+	case 0:
+		return nil, ErrNoMatch
+	case 1:


mnd: Magic number: 1, in detected (from gomnd)

golangcibot · 2020-02-02T22:26:42Z

crypto/multikey/kind.go

+	ErrKindNotFound = errors.New("key kind is not found")
+
+	// PossibleKeyKinds current possible key kinds
+	PossibleKeyKinds = []string{crypto.KindED25519, crypto.KindSECP256K1, crypto.KindSR25519}


PossibleKeyKinds is a global variable (from gochecknoglobals)

golangcibot · 2020-02-02T22:26:43Z

crypto/multikey/kind.go

+	switch len(matches) {
+	case 0:
+		return nil, ErrNoMatch
+	case 1: //nolint:gomnd


whyNoLint: include an explanation for nolint directive (from gocritic)

robdefeo

Thanks @olimpias for the PR it looks great 😄 I have requested just a couple of small changes around this tests. Then it will be ready to merge.

Awesome with the depth of the tests too 💯

robdefeo · 2020-02-03T20:06:25Z

crypto/multikey/kind_test.go

 	"github.com/mailchain/mailchain/crypto"
 	"github.com/mailchain/mailchain/crypto/ed25519/ed25519test"
 	"github.com/mailchain/mailchain/crypto/secp256k1/secp256k1test"
 	"github.com/stretchr/testify/assert"
 )

 func TestKeyKindFromSignature(t *testing.T) {
-	assert := assert.New(t)
+	assertion := assert.New(t)


I have come to realise that assert := assert.New(t) is not a good approach. For two reasons I can see now, the first is what you have addressed; changing what assert does part the want through a function. Second is that it gives less information about the test case that failed. Can you remove this line and instead add t to the assert calls.

robdefeo · 2020-02-03T20:07:19Z

crypto/multikey/kind_test.go

@@ -156,15 +158,108 @@ func TestKeyKindFromSignature(t *testing.T) {
 				t.Errorf("KeyKindFromSignature() err = %v, want %v", err, tt.wantErr)
 			}

-			if !assert.Equal(key, tt.wantKey) {
+			if !assertion.Equal(key, tt.wantKey) {


If you change to if !assert.Equal(t, key, tt.wantKey) { this will give more information about what test case failed.

robdefeo · 2020-02-03T20:08:44Z

crypto/multikey/kind_test.go

+			name: "success-ed25519-sofia-key",
+			args: args{
+				publicKey:  ed25519test.SofiaPublicKey.Bytes(),
+				privateKey: ed25519test.SofiaPrivateKey.Bytes(),
+			},
+			wantKey: ed25519test.SofiaPrivateKey,
+		},
+		{
+			name: "success-ed25519-charlotte-key",
+			args: args{
+				publicKey:  ed25519test.CharlottePublicKey.Bytes(),
+				privateKey: ed25519test.CharlottePrivateKey.Bytes(),
+			},
+			wantKey: ed25519test.CharlottePrivateKey,
+		},
+		{
+			name: "success-secp256k1-sofia-key",
+			args: args{
+				publicKey:  secp256k1test.SofiaPublicKey.Bytes(),
+				privateKey: secp256k1test.SofiaPrivateKey.Bytes(),
+			},
+			wantKey: secp256k1test.SofiaPrivateKey,
+		},
+		{
+			name: "success-secp256k1-charlotte-key",
+			args: args{
+				publicKey:  secp256k1test.CharlottePublicKey.Bytes(),
+				privateKey: secp256k1test.CharlottePrivateKey.Bytes(),
+			},
+			wantKey: secp256k1test.CharlottePrivateKey,
+		},
+		{
+			name: "success-sr25519-sofia-key",
+			args: args{
+				publicKey:  sr25519test.SofiaPublicKey.Bytes(),
+				privateKey: sr25519test.SofiaPrivateKey.Bytes(),
+			},
+			wantKey: sr25519test.SofiaPrivateKey,
+		},
+		{
+			name: "success-sr25519-charlotte-key",
+			args: args{
+				publicKey:  sr25519test.CharlottePublicKey.Bytes(),
+				privateKey: sr25519test.CharlottePrivateKey.Bytes(),
+			},
+			wantKey: sr25519test.CharlottePrivateKey,
+		},
+		{
+			name: "err-invalid-public-key-bytes",
+			args: args{
+				publicKey:  []byte{0x1},
+				privateKey: sr25519test.CharlottePrivateKey.Bytes(),
+			},
+			wantErr: ErrNoMatch,
+		},
+		{
+			name: "err-no-key-kinds",
+			args: args{
+				publicKey:  []byte{0x1},
+				privateKey: []byte{0x2},
+			},
+			wantErr: ErrNoMatch,
+		},
+	}


This is great 😄 very through 💯

robdefeo · 2020-02-03T20:09:02Z

crypto/multikey/kind_test.go

@@ -193,7 +288,7 @@ func TestRemoveDuplicates(t *testing.T) {
 	for _, tt := range tests {
 		t.Run("remove-duplicates", func(t *testing.T) {
 			got := removeDuplicates(tt.in)
-			if !assert.Equal(tt.want, got) {
+			if !assertion.Equal(tt.want, got) {


If you change to if !assert.Equal(t, tt.want, got) { this will give more information about what test case failed.

robdefeo · 2020-02-03T20:09:35Z

crypto/multikey/kind_test.go

 func TestRemoveDuplicates(t *testing.T) {
-	assert := assert.New(t)
+	assertion := assert.New(t)


I have come to realise that assert := assert.New(t) is not a good approach. For two reasons I can see now, the first is what you have addressed; changing what assert does part the want through a function. Second is that it gives less information about the test case that failed. Can you remove this line and instead add t to the assert calls.

robdefeo

Thanks @olimpias for the PR it looks great 😄 I have requested just a couple of small changes around this tests. There is one issue from https://golangci.com/r/github.com/mailchain/mailchain/pulls/535. Then it will be ready to merge.

Awesome with the depth of the tests too 💯

robdefeo

Thanks @olimpias for the update to the PR 😄 this is good to merge 👍

olimpias · 2020-02-03T21:23:20Z

Thank you for your review :).

Infer key type from public key and private key

daa05b6

olimpias requested a review from robdefeo February 2, 2020 21:58

golangcibot reviewed Feb 2, 2020

View reviewed changes

olimpias added 2 commits February 2, 2020 23:09

Golint is fixed.

6a92041

verifyPrivateAndPublicKey method is simplified.

0e644a1

golangcibot reviewed Feb 2, 2020

View reviewed changes

Global variable is removed

6f02df8

robdefeo requested changes Feb 3, 2020

View reviewed changes

olimpias added 2 commits February 3, 2020 21:29

assert new is removed.

55d2555

Constant values is added.

4faf668

robdefeo approved these changes Feb 3, 2020

View reviewed changes

robdefeo merged commit 5809404 into mailchain:master Feb 3, 2020

olimpias deleted the infer-key branch February 3, 2020 21:23

robdefeo mentioned this pull request Feb 3, 2020

Infer key type from public key and private key bytes #403

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Infer key type from public key and private key #535

chore: Infer key type from public key and private key #535

olimpias commented Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

golangcibot Feb 2, 2020

robdefeo left a comment

robdefeo Feb 3, 2020

olimpias Feb 3, 2020

robdefeo Feb 3, 2020

robdefeo Feb 3, 2020

robdefeo Feb 3, 2020

robdefeo Feb 3, 2020

robdefeo left a comment •

edited

robdefeo left a comment

olimpias commented Feb 3, 2020

chore: Infer key type from public key and private key #535

chore: Infer key type from public key and private key #535

Conversation

olimpias commented Feb 2, 2020

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

robdefeo 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

robdefeo left a comment • edited

Choose a reason for hiding this comment

robdefeo left a comment

Choose a reason for hiding this comment

olimpias commented Feb 3, 2020

robdefeo left a comment •

edited