Address pull request comments

* Simplify scrypt constants with const block
* Add key store constructors and make their types private
* Simplify key store and file namings to be less Java Enterprise™
* Change test error logging to use t.Error(err)
* Reduce number of naked returns (just like my ex-gf)
* Simplify file reading path code

4 files changed, 91 insertions, 84 deletions

diff --git a/crypto/key.go b/crypto/key.go
index d4845ee22..1a8b770e0 100644
--- a/crypto/key.go
+++ b/crypto/key.go
@@ -49,7 +49,7 @@ type Key struct {
 	PrivateKey *ecdsa.PrivateKey
 }
 
-type KeyPlainJSON struct {
+type PlainKeyJSON struct {
 	Id         string
 	Flags      string
 	PrivateKey string
@@ -61,7 +61,7 @@ type CipherJSON struct {
 	CipherText string
 }
 
-type KeyProtectedJSON struct {
+type EncryptedKeyJSON struct {
 	Id     string
 	Flags  string
 	Crypto CipherJSON
@@ -73,44 +73,44 @@ func (k *Key) Address() []byte {
 }
 
 func (k *Key) MarshalJSON() (j []byte, err error) {
-	stringStruct := KeyPlainJSON{
+	stringStruct := PlainKeyJSON{
 		k.Id.String(),
 		hex.EncodeToString(k.Flags[:]),
 		hex.EncodeToString(FromECDSA(k.PrivateKey)),
 	}
-	j, _ = json.Marshal(stringStruct)
-	return
+	j, err = json.Marshal(stringStruct)
+	return j, err
 }
 
 func (k *Key) UnmarshalJSON(j []byte) (err error) {
-	keyJSON := new(KeyPlainJSON)
+	keyJSON := new(PlainKeyJSON)
 	err = json.Unmarshal(j, &keyJSON)
 	if err != nil {
-		return
+		return err
 	}
 
 	u := new(uuid.UUID)
 	*u = uuid.Parse(keyJSON.Id)
 	if *u == nil {
 		err = errors.New("UUID parsing failed")
-		return
+		return err
 	}
 	k.Id = u
 
 	flagsBytes, err := hex.DecodeString(keyJSON.Flags)
 	if err != nil {
-		return
+		return err
 	}
 
 	PrivateKeyBytes, err := hex.DecodeString(keyJSON.PrivateKey)
 	if err != nil {
-		return
+		return err
 	}
 
 	copy(k.Flags[:], flagsBytes[0:4])
 	k.PrivateKey = ToECDSA(PrivateKeyBytes)
 
-	return
+	return err
 }
 
 func NewKey() *Key {
diff --git a/crypto/key_store_passphrase_encryped.go b/crypto/key_store_passphrase.go
index b17536c3f..eaf73422f 100644
--- a/crypto/key_store_passphrase_encryped.go
+++ b/crypto/key_store_passphrase.go
@@ -76,35 +76,46 @@ import (
 	"path"
 )
 
-const scryptN int = 262144 // 2^18
-const scryptr int = 8
-const scryptp int = 1
-const scryptdkLen int = 32
+const (
+	// 2^18 / 8 / 1 uses 256MB memory and approx 1s CPU time on a modern CPU.
+	scryptN     = 1 << 18
+	scryptr     = 8
+	scryptp     = 1
+	scryptdkLen = 32
+)
 
-type KeyStorePassphrase struct {
+type keyStorePassphrase struct {
 	keysDirPath string
 }
 
-func (ks KeyStorePassphrase) GenerateNewKey(auth string) (key *Key, err error) {
-	key, err = GenerateNewKeyDefault(ks, auth)
-	return
+func NewKeyStorePassphrase(path string) KeyStore2 {
+	ks := new(keyStorePassphrase)
+	ks.keysDirPath = path
+	return ks
+}
+
+func (ks keyStorePassphrase) GenerateNewKey(auth string) (key *Key, err error) {
+	return GenerateNewKeyDefault(ks, auth)
 }
 
-func (ks KeyStorePassphrase) GetKey(keyId *uuid.UUID, auth string) (key *Key, err error) {
+func (ks keyStorePassphrase) GetKey(keyId *uuid.UUID, auth string) (key *Key, err error) {
 	keyBytes, flags, err := DecryptKey(ks, keyId, auth)
+	if err != nil {
+		return nil, err
+	}
 	key = new(Key)
 	key.Id = keyId
 	copy(key.Flags[:], flags[0:4])
 	key.PrivateKey = ToECDSA(keyBytes)
-	return
+	return key, err
 }
 
-func (ks KeyStorePassphrase) StoreKey(key *Key, auth string) (err error) {
+func (ks keyStorePassphrase) StoreKey(key *Key, auth string) (err error) {
 	authArray := []byte(auth)
 	salt := GetEntropyCSPRNG(32)
 	derivedKey, err := scrypt.Key(authArray, salt, scryptN, scryptr, scryptp, scryptdkLen)
 	if err != nil {
-		return
+		return err
 	}
 
 	keyBytes := FromECDSA(key.PrivateKey)
@@ -113,7 +124,7 @@ func (ks KeyStorePassphrase) StoreKey(key *Key, auth string) (err error) {
 
 	AES256Block, err := aes.NewCipher(derivedKey)
 	if err != nil {
-		return
+		return err
 	}
 
 	iv := GetEntropyCSPRNG(aes.BlockSize) // 16
@@ -126,70 +137,68 @@ func (ks KeyStorePassphrase) StoreKey(key *Key, auth string) (err error) {
 		hex.EncodeToString(iv),
 		hex.EncodeToString(cipherText),
 	}
-	keyStruct := KeyProtectedJSON{
+	keyStruct := EncryptedKeyJSON{
 		key.Id.String(),
 		hex.EncodeToString(key.Flags[:]),
 		cipherStruct,
 	}
 	keyJSON, err := json.Marshal(keyStruct)
 	if err != nil {
-		return
+		return err
 	}
 
-	err = WriteKeyFile(key.Id.String(), ks.keysDirPath, keyJSON)
-	return
+	return WriteKeyFile(key.Id.String(), ks.keysDirPath, keyJSON)
 }
 
-func (ks KeyStorePassphrase) DeleteKey(keyId *uuid.UUID, auth string) (err error) {
+func (ks keyStorePassphrase) DeleteKey(keyId *uuid.UUID, auth string) (err error) {
 	// only delete if correct passphrase is given
 	_, _, err = DecryptKey(ks, keyId, auth)
 	if err != nil {
-		return
+		return err
 	}
 
 	keyDirPath := path.Join(ks.keysDirPath, keyId.String())
-	err = os.RemoveAll(keyDirPath)
-	return
+	return os.RemoveAll(keyDirPath)
 }
 
-func DecryptKey(ks KeyStorePassphrase, keyId *uuid.UUID, auth string) (keyBytes []byte, flags []byte, err error) {
+func DecryptKey(ks keyStorePassphrase, keyId *uuid.UUID, auth string) (keyBytes []byte, flags []byte, err error) {
 	fileContent, err := GetKeyFile(ks.keysDirPath, keyId)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
-	keyProtected := new(KeyProtectedJSON)
+	keyProtected := new(EncryptedKeyJSON)
 	err = json.Unmarshal(fileContent, keyProtected)
 
 	flags, err = hex.DecodeString(keyProtected.Flags)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	salt, err := hex.DecodeString(keyProtected.Crypto.Salt)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	iv, err := hex.DecodeString(keyProtected.Crypto.IV)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	cipherText, err := hex.DecodeString(keyProtected.Crypto.CipherText)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	authArray := []byte(auth)
 	derivedKey, err := scrypt.Key(authArray, salt, scryptN, scryptr, scryptp, scryptdkLen)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	AES256Block, err := aes.NewCipher(derivedKey)
 	if err != nil {
-		return
+		return nil, nil, err
 	}
 
 	AES256CBCDecrypter := cipher.NewCBCDecrypter(AES256Block, iv)
@@ -199,14 +208,14 @@ func DecryptKey(ks KeyStorePassphrase, keyId *uuid.UUID, auth string) (keyBytes
 	plainText := PKCS7Unpad(paddedPlainText)
 	if plainText == nil {
 		err = errors.New("Decryption failed: PKCS7Unpad failed after decryption")
-		return
+		return nil, nil, err
 	}
 
 	keyBytes = plainText[:len(plainText)-32]
 	keyBytesHash := plainText[len(plainText)-32:]
 	if !bytes.Equal(Sha3(keyBytes), keyBytesHash) {
 		err = errors.New("Decryption failed: checksum mismatch")
-		return
+		return nil, nil, err
 	}
 	return keyBytes, flags, err
 }
diff --git a/crypto/key_store_plaintext_file.go b/crypto/key_store_plain.go
index 7ca5e5679..00d9767b6 100644
--- a/crypto/key_store_plaintext_file.go
+++ b/crypto/key_store_plain.go
@@ -41,7 +41,7 @@ type KeyStore2 interface {
 	DeleteKey(*uuid.UUID, string) error      // delete key by id and auth string
 }
 
-type KeyStorePlaintext struct {
+type keyStorePlain struct {
 	keysDirPath string
 }
 
@@ -51,9 +51,14 @@ func DefaultDataDir() string {
 	return path.Join(usr.HomeDir, ".ethereum")
 }
 
-func (ks KeyStorePlaintext) GenerateNewKey(auth string) (key *Key, err error) {
-	key, err = GenerateNewKeyDefault(ks, auth)
-	return
+func NewKeyStorePlain(path string) KeyStore2 {
+	ks := new(keyStorePlain)
+	ks.keysDirPath = path
+	return ks
+}
+
+func (ks keyStorePlain) GenerateNewKey(auth string) (key *Key, err error) {
+	return GenerateNewKeyDefault(ks, auth)
 }
 
 func GenerateNewKeyDefault(ks KeyStore2, auth string) (key *Key, err error) {
@@ -64,50 +69,46 @@ func GenerateNewKeyDefault(ks KeyStore2, auth string) (key *Key, err error) {
 	}()
 	key = NewKey()
 	err = ks.StoreKey(key, auth)
-	return
+	return key, err
 }
 
-func (ks KeyStorePlaintext) GetKey(keyId *uuid.UUID, auth string) (key *Key, err error) {
+func (ks keyStorePlain) GetKey(keyId *uuid.UUID, auth string) (key *Key, err error) {
 	fileContent, err := GetKeyFile(ks.keysDirPath, keyId)
 	if err != nil {
-		return
+		return nil, err
 	}
 
 	key = new(Key)
 	err = json.Unmarshal(fileContent, key)
-	return
+	return key, err
 }
 
-func (ks KeyStorePlaintext) StoreKey(key *Key, auth string) (err error) {
+func (ks keyStorePlain) StoreKey(key *Key, auth string) (err error) {
 	keyJSON, err := json.Marshal(key)
 	if err != nil {
-		return
+		return err
 	}
 	err = WriteKeyFile(key.Id.String(), ks.keysDirPath, keyJSON)
-	return
+	return err
 }
 
-func (ks KeyStorePlaintext) DeleteKey(keyId *uuid.UUID, auth string) (err error) {
+func (ks keyStorePlain) DeleteKey(keyId *uuid.UUID, auth string) (err error) {
 	keyDirPath := path.Join(ks.keysDirPath, keyId.String())
 	err = os.RemoveAll(keyDirPath)
-	return
+	return err
 }
 
 func GetKeyFile(keysDirPath string, keyId *uuid.UUID) (fileContent []byte, err error) {
-	idString := keyId.String()
-	keyDirPath := path.Join(keysDirPath, idString)
-	keyFilePath := path.Join(keyDirPath, idString)
-	fileContent, err = ioutil.ReadFile(keyFilePath)
-	return
+	id := keyId.String()
+	return ioutil.ReadFile(path.Join(keysDirPath, id, id))
 }
 
-func WriteKeyFile(idString string, keysDirPath string, content []byte) (err error) {
-	keyDirPath := path.Join(keysDirPath, idString)
-	keyFilePath := path.Join(keyDirPath, idString)
+func WriteKeyFile(id string, keysDirPath string, content []byte) (err error) {
+	keyDirPath := path.Join(keysDirPath, id)
+	keyFilePath := path.Join(keyDirPath, id)
 	err = os.MkdirAll(keyDirPath, 0700) // read, write and dir search for user
 	if err != nil {
-		return
+		return err
 	}
-	err = ioutil.WriteFile(keyFilePath, content, 0600) // read, write for user
-	return
+	return ioutil.WriteFile(keyFilePath, content, 0600) // read, write for user
 }
diff --git a/crypto/key_store_test.go b/crypto/key_store_test.go
index 412735444..8469d2369 100644
--- a/crypto/key_store_test.go
+++ b/crypto/key_store_test.go
@@ -6,20 +6,19 @@ import (
 	"testing"
 )
 
-func TestKeyStorePlaintext(t *testing.T) {
-	ks := new(KeyStorePlaintext)
-	ks.keysDirPath = DefaultDataDir()
+func TestKeyStorePlain(t *testing.T) {
+	ks := NewKeyStorePlain(DefaultDataDir())
 	pass := "" // not used but required by API
 	k1, err := ks.GenerateNewKey(pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
 	k2 := new(Key)
 	k2, err = ks.GetKey(k1.Id, pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
@@ -40,25 +39,24 @@ func TestKeyStorePlaintext(t *testing.T) {
 
 	err = ks.DeleteKey(k2.Id, pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 }
 
 func TestKeyStorePassphrase(t *testing.T) {
-	ks := new(KeyStorePassphrase)
-	ks.keysDirPath = DefaultDataDir()
+	ks := NewKeyStorePassphrase(DefaultDataDir())
 	pass := "foo"
 	k1, err := ks.GenerateNewKey(pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
 	k2 := new(Key)
 	k2, err = ks.GetKey(k1.Id, pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
@@ -79,36 +77,35 @@ func TestKeyStorePassphrase(t *testing.T) {
 
 	err = ks.DeleteKey(k2.Id, pass) // also to clean up created files
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 }
 
 func TestKeyStorePassphraseDecryptionFail(t *testing.T) {
-	ks := new(KeyStorePassphrase)
-	ks.keysDirPath = DefaultDataDir()
+	ks := NewKeyStorePassphrase(DefaultDataDir())
 	pass := "foo"
 	k1, err := ks.GenerateNewKey(pass)
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
 	_, err = ks.GetKey(k1.Id, "bar") // wrong passphrase
-	// fmt.Println(err.Error())
+	// t.Error(err)
 	if err == nil {
 		t.FailNow()
 	}
 
 	err = ks.DeleteKey(k1.Id, "bar") // wrong passphrase
 	if err == nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 
 	err = ks.DeleteKey(k1.Id, pass) // to clean up
 	if err != nil {
-		fmt.Println(err.Error())
+		t.Error(err)
 		t.FailNow()
 	}
 }