update(dot/rpc): insertKey and hasKey takes keystore type (ChainSafe#…

…1961)

This commit changes `author_insertKey`, `core.InsertKey` and
`core.HasKey` to accept keystore type as an argument.

- So far, these operations were using `account` keystore, but now they
will use the keystore associated with the type passed in the argument.
-  Error when key does not match keystore type. Earlier, we returned without inserting and without erroring, when keystore did not support the key type of passed key.

dot/core/service.go

@@ -445,15 +445,24 @@ func (s *Service) maintainTransactionPool(block *types.Block) {
 }
 
 // InsertKey inserts keypair into the account keystore
-// TODO: define which keystores need to be updated and create separate insert funcs for each (#1850)
-func (s *Service) InsertKey(kp crypto.Keypair) {
-	s.keys.Acco.Insert(kp)
+func (s *Service) InsertKey(kp crypto.Keypair, keystoreType string) error {
+	ks, err := s.keys.GetKeystore([]byte(keystoreType))
+	if err != nil {
+		return err
+	}
+
+	return ks.Insert(kp)
 }
 
 // HasKey returns true if given hex encoded public key string is found in keystore, false otherwise, error if there
-//  are issues decoding string
-func (s *Service) HasKey(pubKeyStr, keyType string) (bool, error) {
-	return keystore.HasKey(pubKeyStr, keyType, s.keys.Acco)
+// are issues decoding string
+func (s *Service) HasKey(pubKeyStr, keystoreType string) (bool, error) {
+	ks, err := s.keys.GetKeystore([]byte(keystoreType))
+	if err != nil {
+		return false, err
+	}
+
+	return keystore.HasKey(pubKeyStr, keystoreType, ks)
 }
 
 // DecodeSessionKeys executes the runtime DecodeSessionKeys and return the scale encoded keys

dot/core/service_test.go

@@ -18,6 +18,7 @@ package core
 
 import (
 	"errors"
+	"fmt"
 	"io/ioutil"
 	"math/big"
 	"os"
@@ -117,6 +118,59 @@ func TestAnnounceBlock(t *testing.T) {
 	net.AssertCalled(t, "GossipMessage", expected)
 }
 
+func TestService_InsertKey(t *testing.T) {
+	ks := keystore.NewGlobalKeystore()
+
+	cfg := &Config{
+		Keystore: ks,
+	}
+	s := NewTestService(t, cfg)
+
+	kr, err := keystore.NewSr25519Keyring()
+	require.NoError(t, err)
+
+	testCases := []struct {
+		description  string
+		keystoreType string
+		err          error
+	}{
+		{
+			description:  "Test that insertKey fails when keystore type is invalid ",
+			keystoreType: "some-invalid-type",
+			err:          keystore.ErrInvalidKeystoreName,
+		},
+		{
+			description:  "Test that insertKey fails when keystore type is valid but inappropriate",
+			keystoreType: "gran",
+			err:          fmt.Errorf("%v, passed key type: sr25519, acceptable key type: ed25519", keystore.ErrKeyTypeNotSupported),
+		},
+		{
+			description:  "Test that insertKey succeeds when keystore type is valid and appropriate ",
+			keystoreType: "acco",
+			err:          nil,
+		},
+	}
+
+	for _, c := range testCases {
+		c := c
+		t.Run(c.description, func(t *testing.T) {
+			t.Parallel()
+
+			err := s.InsertKey(kr.Alice(), c.keystoreType)
+
+			if c.err == nil {
+				require.Nil(t, err)
+				res, err := s.HasKey(kr.Alice().Public().Hex(), c.keystoreType)
+				require.Nil(t, err)
+				require.True(t, res)
+			} else {
+				require.NotNil(t, err)
+				require.Equal(t, err.Error(), c.err.Error())
+			}
+		})
+	}
+}
+
 func TestService_HasKey(t *testing.T) {
 	ks := keystore.NewGlobalKeystore()
 	kr, err := keystore.NewSr25519Keyring()
@@ -128,9 +182,17 @@ func TestService_HasKey(t *testing.T) {
 	}
 	s := NewTestService(t, cfg)
 
-	res, err := s.HasKey(kr.Alice().Public().Hex(), "babe")
+	res, err := s.HasKey(kr.Alice().Public().Hex(), "acco")
 	require.NoError(t, err)
 	require.True(t, res)
+
+	res, err = s.HasKey(kr.Alice().Public().Hex(), "babe")
+	require.NoError(t, err)
+	require.False(t, res)
+
+	res, err = s.HasKey(kr.Alice().Public().Hex(), "gran")
+	require.NoError(t, err)
+	require.False(t, res)
 }
 
 func TestService_HasKey_UnknownType(t *testing.T) {
@@ -145,7 +207,7 @@ func TestService_HasKey_UnknownType(t *testing.T) {
 	s := NewTestService(t, cfg)
 
 	res, err := s.HasKey(kr.Alice().Public().Hex(), "xxxx")
-	require.EqualError(t, err, "unknown key type: xxxx")
+	require.EqualError(t, err, "invalid keystore name")
 	require.False(t, res)
 }
 

dot/core/test_helpers.go

@@ -51,7 +51,8 @@ func NewTestService(t *testing.T, cfg *Config) *Service {
 		if err != nil {
 			t.Fatal(err)
 		}
-		cfg.Keystore.Acco.Insert(kp)
+		err = cfg.Keystore.Acco.Insert(kp)
+		require.NoError(t, err)
 	}
 
 	cfg.LogLvl = 3

dot/rpc/modules/api.go

@@ -94,7 +94,7 @@ type TransactionStateAPI interface {
 
 // CoreAPI is the interface for the core methods
 type CoreAPI interface {
-	InsertKey(kp crypto.Keypair)
+	InsertKey(kp crypto.Keypair, keystoreType string) error
 	HasKey(pubKeyStr string, keyType string) (bool, error)
 	GetRuntimeVersion(bhash *common.Hash) (runtime.Version, error)
 	HandleSubmittedExtrinsic(types.Extrinsic) error

dot/rpc/modules/api_mocks.go

@@ -56,7 +56,7 @@ func NewMockTransactionStateAPI() *modulesmocks.TransactionStateAPI {
 // NewMockCoreAPI creates and return an rpc CoreAPI interface mock
 func NewMockCoreAPI() *modulesmocks.CoreAPI {
 	m := new(modulesmocks.CoreAPI)
-	m.On("InsertKey", mock.AnythingOfType("crypto.Keypair"))
+	m.On("InsertKey", mock.AnythingOfType("crypto.Keypair"), mock.AnythingOfType("string")).Return(nil)
 	m.On("HasKey", mock.AnythingOfType("string"), mock.AnythingOfType("string")).Return(false, nil)
 	m.On("GetRuntimeVersion", mock.AnythingOfType("*common.Hash")).Return(NewMockVersion(), nil)
 	m.On("IsBlockProducer").Return(false)

dot/rpc/modules/author.go

@@ -164,7 +164,6 @@ func (am *AuthorModule) InsertKey(r *http.Request, req *KeyInsertRequest, res *K
 		return err
 	}
 
-	// TODO: correctly use keystore type (#1850)
 	keyPair, err := keystore.DecodeKeyPairFromHex(keyBytes, keystore.DetermineKeyType(keyReq.Type))
 	if err != nil {
 		return err
@@ -175,7 +174,11 @@ func (am *AuthorModule) InsertKey(r *http.Request, req *KeyInsertRequest, res *K
 		return errors.New("generated public key does not equal provide public key")
 	}
 
-	am.coreAPI.InsertKey(keyPair)
+	err = am.coreAPI.InsertKey(keyPair, keyReq.Type)
+	if err != nil {
+		return err
+	}
+
 	am.logger.Info("inserted key " + keyPair.Public().Hex() + " into keystore")
 	return nil
 }

dot/rpc/modules/author_test.go

@@ -107,7 +107,7 @@ func TestAuthorModule_HasSessionKey(t *testing.T) {
 	globalStore := keystore.NewGlobalKeystore()
 
 	coremockapi := new(apimocks.CoreAPI)
-	mockInsertKey := coremockapi.On("InsertKey", mock.AnythingOfType("*sr25519.Keypair"))
+	mockInsertKey := coremockapi.On("InsertKey", mock.AnythingOfType("*sr25519.Keypair"), mock.AnythingOfType("string")).Return(nil)
 	mockInsertKey.Run(func(args mock.Arguments) {
 		kp := args.Get(0).(*sr25519.Keypair)
 		globalStore.Acco.Insert(kp)
@@ -146,7 +146,7 @@ func TestAuthorModule_HasSessionKey(t *testing.T) {
 		Seed:      "0xfec0f475b818470af5caf1f3c1b1558729961161946d581d2755f9fb566534f8",
 		PublicKey: "0x34309a9d2a24213896ff06895db16aade8b6502f3a71cf56374cc38520426026",
 	}, nil)
-	coremockapi.AssertCalled(t, "InsertKey", mock.AnythingOfType("*sr25519.Keypair"))
+	coremockapi.AssertCalled(t, "InsertKey", mock.AnythingOfType("*sr25519.Keypair"), mock.AnythingOfType("string"))
 	require.NoError(t, err)
 	require.Equal(t, 1, globalStore.Acco.Size())
 
@@ -324,7 +324,7 @@ func TestAuthorModule_PendingExtrinsics(t *testing.T) {
 
 func TestAuthorModule_InsertKey(t *testing.T) {
 	mockCoreAPI := &apimocks.CoreAPI{}
-	mockCoreAPI.On("InsertKey", mock.Anything).Return(nil)
+	mockCoreAPI.On("InsertKey", mock.Anything, mock.AnythingOfType("string")).Return(nil)
 
 	type fields struct {
 		logger     log.LeveledLogger

lib/keystore/basic_keystore.go

@@ -18,12 +18,18 @@ package keystore
 
 import (
 	"bytes"
+	"errors"
+	"fmt"
 	"sync"
 
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/crypto"
 )
 
+var (
+	ErrKeyTypeNotSupported = errors.New("given key type is not supported by this keystore")
+)
+
 // BasicKeystore holds keys of a certain type
 type BasicKeystore struct {
 	name Name
@@ -57,17 +63,18 @@ func (ks *BasicKeystore) Size() int {
 }
 
 // Insert adds a keypair to the keystore
-func (ks *BasicKeystore) Insert(kp crypto.Keypair) {
+func (ks *BasicKeystore) Insert(kp crypto.Keypair) error {
 	ks.lock.Lock()
 	defer ks.lock.Unlock()
 
 	if kp.Type() != ks.typ {
-		return
+		return fmt.Errorf("%v, passed key type: %s, acceptable key type: %s", ErrKeyTypeNotSupported, kp.Type(), ks.typ)
 	}
 
 	pub := kp.Public()
 	addr := crypto.PublicKeyToAddress(pub)
 	ks.keys[addr] = kp
+	return nil
 }
 
 // GetKeypair returns a keypair corresponding to the given public key, or nil if it doesn't exist

lib/keystore/generic_keystore.go

@@ -58,13 +58,14 @@ func (ks *GenericKeystore) Size() int {
 }
 
 // Insert adds a keypair to the keystore
-func (ks *GenericKeystore) Insert(kp crypto.Keypair) {
+func (ks *GenericKeystore) Insert(kp crypto.Keypair) error {
 	ks.lock.Lock()
 	defer ks.lock.Unlock()
 
 	pub := kp.Public()
 	addr := crypto.PublicKeyToAddress(pub)
 	ks.keys[addr] = kp
+	return nil
 }
 
 // GetKeypair returns a keypair corresponding to the given public key, or nil if it doesn't exist

lib/keystore/helpers.go

@@ -156,24 +156,27 @@ func LoadKeystore(key string, ks Keystore) error {
 		}
 
 		switch strings.ToLower(key) {
+		// Insert can error only if kestore type do not match with key
+		// type do not match. Since we have created keyring based on ks.Type(),
+		// Insert would never error here. Thus, ignoring those errors.
 		case "alice":
-			ks.Insert(kr.Alice())
+			_ = ks.Insert(kr.Alice())
 		case "bob":
-			ks.Insert(kr.Bob())
+			_ = ks.Insert(kr.Bob())
 		case "charlie":
-			ks.Insert(kr.Charlie())
+			_ = ks.Insert(kr.Charlie())
 		case "dave":
-			ks.Insert(kr.Dave())
+			_ = ks.Insert(kr.Dave())
 		case "eve":
-			ks.Insert(kr.Eve())
+			_ = ks.Insert(kr.Eve())
 		case "ferdie":
-			ks.Insert(kr.Ferdie())
+			_ = ks.Insert(kr.Ferdie())
 		case "george":
-			ks.Insert(kr.George())
+			_ = ks.Insert(kr.George())
 		case "heather":
-			ks.Insert(kr.Heather())
+			_ = ks.Insert(kr.Heather())
 		case "ian":
-			ks.Insert(kr.Ian())
+			_ = ks.Insert(kr.Ian())
 		default:
 			return fmt.Errorf("invalid test key provided")
 		}
@@ -300,7 +303,9 @@ func UnlockKeys(ks Keystore, dir, unlock, password string) error {
 			return fmt.Errorf("failed to create keypair from private key %d: %s", idx, err)
 		}
 
-		ks.Insert(kp)
+		if err = ks.Insert(kp); err != nil {
+			return fmt.Errorf("failed to insert key in keystore: %v", err)
+		}
 	}
 
 	return nil
@@ -329,7 +334,7 @@ func DetermineKeyType(t string) crypto.KeyType {
 }
 
 // HasKey returns true if given hex encoded public key string is found in keystore, false otherwise, error if there
-//  are issues decoding string
+// are issues decoding string
 func HasKey(pubKeyStr, keyType string, keystore Keystore) (bool, error) {
 	keyBytes, err := common.HexToBytes(pubKeyStr)
 	if err != nil {

lib/keystore/keystore.go

@@ -23,6 +23,10 @@ import (
 	"github.com/ChainSafe/gossamer/lib/crypto"
 )
 
+var (
+	ErrInvalidKeystoreName = errors.New("invalid keystore name")
+)
+
 // Name represents a defined keystore name
 type Name string
 
@@ -41,7 +45,7 @@ var (
 type Keystore interface {
 	Name() Name
 	Type() crypto.KeyType
-	Insert(kp crypto.Keypair)
+	Insert(kp crypto.Keypair) error
 	GetKeypairFromAddress(pub common.Address) crypto.Keypair
 	GetKeypair(pub crypto.PublicKey) crypto.Keypair
 	PublicKeys() []crypto.PublicKey
@@ -92,6 +96,6 @@ func (k *GlobalKeystore) GetKeystore(name []byte) (Keystore, error) {
 	case DumyName:
 		return k.Dumy, nil
 	default:
-		return nil, errors.New("invalid keystore name")
+		return nil, ErrInvalidKeystoreName
 	}
 }

lib/runtime/life/resolver.go

@@ -844,7 +844,11 @@ func ext_crypto_ed25519_generate_version_1(vm *exec.VirtualMachine) int64 {
 		return 0
 	}
 
-	ks.Insert(kp)
+	err = ks.Insert(kp)
+	if err != nil {
+		logger.Warnf("[ext_crypto_ed25519_generate_version_1] failed to insert key: %s", err)
+		return 0
+	}
 
 	ret, err := toWasmMemorySized(memory, kp.Public().Encode(), 32)
 	if err != nil {
@@ -1027,7 +1031,12 @@ func ext_crypto_sr25519_generate_version_1(vm *exec.VirtualMachine) int64 {
 		return 0
 	}
 
-	ks.Insert(kp)
+	err = ks.Insert(kp)
+	if err != nil {
+		logger.Warnf("[ext_crypto_sr25519_generate_version_1] failed to insert key: %s", err)
+		return 0
+	}
+
 	ret, err := toWasmMemorySized(memory, kp.Public().Encode(), 32)
 	if err != nil {
 		logger.Errorf("[ext_crypto_sr25519_generate_version_1] failed to allocate memory: %s", err)