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-159 Node. Call API method on creating new identity #54

Merged
merged 14 commits into from Dec 18, 2017

Conversation

Projects
None yet
3 participants
@ignasbernotas
Copy link
Member

commented Dec 7, 2017

Node registers identity via indentity API

@ignasbernotas ignasbernotas requested review from shroomist, tadovas, interro and Waldz Dec 7, 2017


if err == nil {
defer response.Body.Close()
log.Info(MYSTERIUM_API_LOG_PREFIX, "Identity created: ", *identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Identity registered..

@@ -31,6 +31,19 @@ type clientRest struct {
httpClient http.Client
}

func (client *clientRest) RegisterIdentity(identity *dto_discovery.Identity) (err error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

We should reconsider our pointer convention.
Because now You can easily pass nil which unwanted situation

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 8, 2017

Member
  1. Lets change signature of IdentityManager.GetIdentity() *Identity -> IdentityManager.GetIdentity() Identity, error
  2. Same with all other IdentityManager methods, which returns pointer *Identity
  3. Lets stop passing *Identity in all project which can easily become nil

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 11, 2017

Contributor

Identities should be immutable, indestructible and persisted on myst network.

@@ -31,6 +31,19 @@ type clientRest struct {
httpClient http.Client
}

func (client *clientRest) RegisterIdentity(identity *dto_discovery.Identity) (err error) {
response, err := client.doRequest("POST", "identities", dto.CreateIdentityRequest{

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

I miss test, which freezes request JSON structure. I know we did not have tests for this, but we should start having

@@ -25,6 +25,12 @@ func (client *clientFake) NodeRegister(proposal dto_discovery.ServiceProposal) (
return nil
}

func (client *clientFake) RegisterIdentity(identity *dto_discovery.Identity) (err error) {
log.Info(MYSTERIUM_API_LOG_PREFIX, "Identity created: ", *identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Fake identity registered

- package: github.com/ethereum/go-ethereum
version: ^1.7.2
subpackages:
- accounts
- package: github.com/julienschmidt/httprouter
version: ^1.1.0
- package: github.com/mitchellh/go-homedir

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Should not version be freezed?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 15, 2017

Author Member

Fixed.

}

func CreateIdentity(dir string) (id *dto.Identity, err error) {
handler := NewIdentityHandler(dir)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Use DI pattern. All dependencies should be injected for top layer, avoid constructing them in the middle of functions

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Same in SelectIdentity(), and we have situation then handler is factored 2 times

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 11, 2017

Contributor

The is connected to certain keystore after NewIdentityHandler
I'm not sure what is the use case for dir string, it represents some filesystem structure.
Handler should be constructed for keystore+dirand perhaps we should add methods to change current keystore and directory

@@ -38,6 +38,18 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

// if no identity can be selected, lets create a new one
if providerId == nil {
providerId, err = identity.CreateIdentity(options.DirectoryKeystore)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

What we decided on json metadata file about all local identities?
Could be like:

[
    {
        "identity": "asdad12425",
        "default": false,
        "registered": true
    },
    {
        "identity": "asdad15346",
        "default": true,
        "registered": false
    }
]

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 8, 2017

Author Member

I thought that was just an idea. Is it confirmed now?

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 8, 2017

Member
  1. Most important is to start having internal structure:
type Identity struct {
    Name string
    IsDefault boolean
    IsRegistered boolean
}
  1. Make dto_discovery.Identity private OR it's not needed anymore
  2. (Optional) Metadata storage it's nice to have (can be JSON, SQL lite, etc.) we can create anotjer ticket for that

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 11, 2017

Contributor

I might be doing (1) in Myst-101
I don't think IsDefault belongs to this struct
also having doubts about IsRegistered

@@ -38,6 +38,18 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

// if no identity can be selected, lets create a new one
if providerId == nil {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

This logic is very tight with selection process, and could easily fit in identityHandler aswel

@Waldz
Copy link
Member

left a comment

I miss mostly

  • Dependency Injection
  • cleaning our technical depth on tests
@Waldz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

Reformated PR from:
MYST-159-node-api-identity
To
MYST-159 Node. Call API method on creating new identity

@Waldz Waldz changed the title MYST-159-node-api-identity MYST-159 Node. Call API method on creating new identity Dec 7, 2017

account, err := idm.keystoreManager.NewAccount(passphrase)
if err != nil {
return nil, err
return "", err

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

Ideally should be "" -> dto.Identity(""), now your string is casted

@@ -22,22 +22,22 @@ func NewIdentityCache(dir string, jsonFile string) *identityCache {
}
}

func (ic *identityCache) GetIdentity() *dto.Identity {
func (ic *identityCache) GetIdentity() (identity dto.Identity) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

GetIdentity() (dto.Identity, error) to recognise error about that result is empty

}

return ids
}

func (idm *identityManager) GetIdentity(identityString string) *dto.Identity {
func (idm *identityManager) GetIdentity(identityString string) dto.Identity {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

GetIdentity() (dto.Identity, error) to recognise error about that result is empty

}

func (idm *identityManager) HasIdentity(identityString string) bool {
return idm.GetIdentity(identityString) != nil
return len(idm.GetIdentity(identityString)) != 0

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

Here You could be using error from GetIdentity

@@ -25,6 +25,12 @@ func (client *clientFake) NodeRegister(proposal dto_discovery.ServiceProposal) (
return nil
}

func (client *clientFake) RegisterIdentity(identity dto_discovery.Identity) (err error) {
log.Info(MYSTERIUM_API_LOG_PREFIX, "Identity registered: ", identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

Mention something in logs, that fake registration was used e.g. Fake identity registered


if err == nil {
defer response.Body.Close()
log.Info(MYSTERIUM_API_LOG_PREFIX, "Identity created: ", identity)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

Registered, not created :)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 18, 2017

Author Member

Weird, I thought I fixed it somewhere:)

@Waldz

Waldz approved these changes Dec 18, 2017

return ih.manager.GetIdentity(id), nil
}

return dto.Identity(""), errors.New("identity not found")

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 18, 2017

Contributor

empty string identity is still a valid identity, shouldn't this be
return nil, error()

return id, nil
}

return identity, errors.New("identity not found in cache")

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 18, 2017

Contributor

is this a common practice to return nil like this?

account, err := idm.keystoreManager.NewAccount(passphrase)
if err != nil {
return nil, err
return "", err

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 18, 2017

Contributor

should return dto.Identity, err not string

}
}

return nil
return dto.Identity("")

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 18, 2017

Contributor

dto.Identity("") is a valid identity, should probably return nil, error

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 18, 2017

Member

Yeah that's normal to return empty struct on error, if you return is by-value, not by-reference

func NewNodeIdentityHandler(keystore keystoreInterface, cacheDir string) *identityHandler {
return &identityHandler{
manager: NewIdentityManager(keystore),
cache: NewIdentityCache(cacheDir, "remember.json"),

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 18, 2017

Contributor

no error handling on file read/creation?

@Waldz

Waldz approved these changes Dec 18, 2017

@Waldz Waldz merged commit df5cc6c into master Dec 18, 2017

@Waldz Waldz deleted the feature/MYST-159-node-api-identity branch Dec 18, 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.