Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST-80 Finished IdentityManager class (generate identity, list identities) #48

Merged
merged 24 commits into from Nov 28, 2017

Conversation

Projects
None yet
4 participants
@interro
Copy link
Contributor

commented Nov 21, 2017

No description provided.

@interro interro requested review from Waldz and cirka Nov 21, 2017

@Waldz Waldz requested review from ignasbernotas and tadovas Nov 21, 2017

"crypto/elliptic"
"crypto/rand"
"crypto/x509"
base64 "encoding/base64"

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

No point of alias

@Waldz

This comment has been minimized.

Copy link
Member

commented Nov 21, 2017

Lets user branch name format: MYST-84-identity-keys -> feature/MYST-84-identity-keys

"math/big"
)

func GenerateKeys() *ecdsa.PrivateKey {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

GenerateKeys -> GenerateKey
or
GenerateKeys -> CreateKey

return crypto.Keccak256([]byte(msg))
}

func SignMessage(path string, identity string, message string) ([]byte, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

string -> []byte

//
// This gives context to the signed message and prevents signing of transactions.
func signHash(data []byte) []byte {
msg := fmt.Sprintf("\x19Ethereum Signed Message:\n%d%s", len(data), data)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Maybe it's possible to append by []byte variable, instead of converting []byte -> string -> []byte

}

func EncodePublicKey(publicKey ecdsa.PublicKey) string {
x509Encoded, _ := x509.MarshalPKIXPublicKey(&publicKey)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Dont disable errors

}

func Sign(privateKey *ecdsa.PrivateKey, message string) (r *big.Int, s *big.Int, err error) {
hashed := []byte(message)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Sign(message []byte)
SignString(message string)

func DecodePublicKey(encodedPublicKey string) *ecdsa.PublicKey {
x509Encoded, _ := base64.StdEncoding.DecodeString(encodedPublicKey)
genericPublicKey, _ := x509.ParsePKIXPublicKey(x509Encoded)
publicKey := genericPublicKey.(*ecdsa.PublicKey)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Can casting fail? Can key be another type?

assert.True(t, verified)
}

func Test_EthNewAccount(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

This test fails


func CreateNewIdentity(path string) (string, error) {
keystoreManager := keystore.NewKeyStore(path, keystore.StandardScryptN, keystore.StandardScryptP)
account, err := keystoreManager.NewAccount("")

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Make passphrase as package constant or variable

}

func Test_SignMessage(t *testing.T) {
ids := GetIdentities("testdata")

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

Add testdata to .gitignore

}

func GetIdentities(path string) []string {
keystoreManager := keystore.NewKeyStore(path, keystore.StandardScryptN, keystore.StandardScryptP)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 21, 2017

Member

keystoreManager is constructed several times, make it as factory NewKeystore() or package variable

return ids
}

func (idm *IdentityManager) IdentityExists() (bool) {

This comment has been minimized.

Copy link
@interro

interro Nov 23, 2017

Author Contributor

IdentityExists method sounds like it should lookup for an Account in keystore by string.
it now returns true if at least one id was registered.

@interro interro changed the title Myst 84 identity keys MYST-80 Finished IdentityManager class (generate identity, list identities) Nov 23, 2017

@Waldz Waldz requested a review from shroomist Nov 23, 2017

@interro interro assigned interro and unassigned interro Nov 23, 2017

manager := NewIdentityManager("testdata")
ids := manager.GetIdentities()
for _, id := range ids {
fmt.Println(id)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

This test has no assertions


func Test_GetIdentity(t *testing.T) {
manager := NewIdentityManager("testdata")
ids := manager.GetIdentities()

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

Test should test only one function GetIdentity but not GetIdentities

This comment has been minimized.

Copy link
@interro

interro Nov 23, 2017

Author Contributor

Test asserts results only from GetIdentity function

for _, id := range ids {
signature, err := manager.SignMessage(id, "message to sign")
assert.NoError(t, err)
assert.Equal(t, len(signature), 65)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

assert.Len(...)

ids := manager.GetIdentities()
for _, id := range ids {
signature, err := manager.SignMessage(id, "message to sign")
assert.NoError(t, err)

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

I am missing exact string assertion

This comment has been minimized.

Copy link
@interro

interro Nov 28, 2017

Author Contributor

removed signing from this branch


func Test_SignMessage(t *testing.T) {
manager := NewIdentityManager("testdata")
ids := manager.GetIdentities()

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

This test should test only one function, and should not depend on other functions

@Waldz
Copy link
Member

left a comment

Nice lib, but we need start writing real unit tests

func (idm *IdentityManager) CreateNewIdentity() (dto.Identity, error) {
account, err := idm.keystoreManager.NewAccount(PASSPHRASE)
if err != nil {
return "", err

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

return identity, err not string

This comment has been minimized.

Copy link
@interro

interro Nov 27, 2017

Author Contributor

fixed

}

func (idm *IdentityManager) GetIdentities() []dto.Identity {
var ids []dto.Identity

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member

Instead of appending to list, which makes memory reallocation each time, suggest all memory in the begining:

ids = make([]dto.Identity, len(accounts))

This comment has been minimized.

Copy link
@interro

interro Nov 27, 2017

Author Contributor

memory optimized

func (idm *IdentityManager) GetIdentity(identityString string) *dto.Identity {
identityString = strings.ToLower(identityString)
for _, id := range idm.GetIdentities() {
if strings.ToLower(string(id)) == identityString {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 23, 2017

Member
  1. make accountToIdentity to lowercase, so You dont need to lower in many places
  2. do we need to change format at all, we can break consistency with Ethereum

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 23, 2017

Contributor

we don't need to do .ToLower() here at all.
user input validation should be handled elsewhere, and GetIdentities() returns valid data.
I'd rather do id.ToString() to compare here.
also how about using map[string]*dto.Identity in the keystore? or is it outside the scope of this ticket?

@@ -0,0 +1,93 @@
package identity

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 25, 2017

Contributor

Needs a brief description what this manager does. ex "maps ethereum network account to dto.Identity. currently creates a new eth account with PASSPHRASE password on CreateNewIdentity(). Used by Discovery Service."

This comment has been minimized.

Copy link
@interro

interro Nov 28, 2017

Author Contributor

added

"strings"
)

const PASSPHRASE = ""

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 25, 2017

Contributor

needs a TODO or FIXME tag comment here so that constant empty password doesn't reach production

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 28, 2017

Member

Passphrase now can be configurable argument for factory, as keydir

}
}
func Test_SignVerifyMessage(t *testing.T) {

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 25, 2017

Contributor

IdentityManager knows how to sign message for an Identity, probably he should also encapsilate the VerifyMesasgeSignature, thought @Waldz?

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 28, 2017

Member

I would not commit sign/verify methods at all, because we will continue working with them later

This comment has been minimized.

Copy link
@interro

interro Nov 28, 2017

Author Contributor

removed signing from this branch

@Waldz

Waldz approved these changes Nov 28, 2017

}

func (idm *identityManager) CreateNewIdentity() (*dto.Identity, error) {
account, err := idm.keystoreManager.NewAccount(PASSPHRASE)

This comment has been minimized.

Copy link
@shroomist

shroomist Nov 28, 2017

Contributor

PASSPHRASE -> should now be a parameter to CreateNewIdentity() or reuse keydir from NewIdentityManager factory

ErrorMock error
}

func (self *keyStoreFake) Accounts() []accounts.Account {

This comment has been minimized.

Copy link
@tadovas

tadovas Nov 28, 2017

Member

Lets be consistent and name receivers with abbrevations instead of "self".

@Waldz Waldz force-pushed the MYST-84-identity-keys branch from ba34f9c to d784822 Nov 28, 2017

@Waldz Waldz merged commit 581904b into master Nov 28, 2017

@Waldz Waldz deleted the MYST-84-identity-keys branch Nov 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.