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-101 client. list identities teqilapi #55

Merged
merged 11 commits into from Dec 14, 2017

Conversation

Projects
None yet
5 participants
@shroomist
Copy link
Contributor

commented Dec 7, 2017

I needed IdentityManager and KeystoreManager to be public

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

@Waldz

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

  1. Use PR format, Jira relates it to ticket
    StoryID Human readable explanation what was changed
    e.g. MYST-101 Client. API method "list identities"

  2. Dont need to mention that PR contains tests, because tests is standard anyway

@@ -3,16 +3,19 @@ package command_run
import (
"encoding/json"
"errors"
"io"

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

We should agree on code formatting guidelines.
Otherwise it will start to reformat it forward&backward.

@shroomist shroomist changed the title list identities endpoint register and testing MYST-101 client. list identities teqila endpoint Dec 7, 2017

@@ -71,7 +74,9 @@ func (cmd *CommandRun) Run(options CommandOptions) (err error) {
return err
}

apiEndpoints := tequilapi.NewApiEndpoints()
idm := identity.NewIdentityManager("TEST")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

This password should be input for identity create method, e.g. it's optional for listing identities

apiEndpoints := tequilapi.NewApiEndpoints()
idm := identity.NewIdentityManager("TEST")
router := tequilapi.NewApiEndpoints()
router.GET("/list-identities", endpoints.ListIdentitiesEndpointFactory(idm).ListIdentities)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Lets reconsider this, as main controller will start to be fat

apiEndpoints := tequilapi.NewApiEndpoints()
idm := identity.NewIdentityManager("TEST")
router := tequilapi.NewApiEndpoints()
router.GET("/list-identities", endpoints.ListIdentitiesEndpointFactory(idm).ListIdentities)

This comment has been minimized.

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

changed to '/identities'

@@ -26,23 +26,23 @@ func accountToIdentity(account accounts.Account) *dto.Identity {
return &identity
}

func identityToAccount(identityString string) accounts.Account {
func IdentityToAccount(identityString string) accounts.Account {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

This method was intentionally private

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

ok, made it private again

type identityManager struct {
keystoreManager keystoreInterface
type IdentityManager struct {
KeystoreManager keystoreInterface

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

This attribute was intentionally private.

  • lets force usage of factory NewIdentityManager()
  • lets ramake to NewIdentityManager(keydir string) -> NewIdentityManager(keystore keystoreManager)

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

done

}

type listIdentitiesEndpoint struct {
listIdentities func() []dto.Identity

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

I would inject IdentityManager as dependency

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

now injected on IdentityHandlers construction.
or are you suggesting to inject it for every api call separately?
we now only have list but there might be others..


func (lsid *listIdentitiesEndpoint) ListIdentities(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
list := identityList{
Identities: lsid.listIdentities(),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

I miss DTO layer which is mapping internal structures -> to API structures

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

please check service_discovery/dto/identity.go if it's what you meant to say

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 11, 2017

Member

REST DTOs should be nest to REST service. See example of Healthcheck endpoint

return &identity.IdentityManager{
KeystoreManager: &identity.KeyStoreFake{
AccountsMock: []accounts.Account{
identity.IdentityToAccount(accountValue),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Dont call internal function of package

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

fixed


assert.JSONEq(
t,
`{

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Looks like misformated

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

fixed

assert.JSONEq(
t,
`{
"identities":["0x000000000000000000000000000000000000000A"]

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Lets have identity as structure, because it will have more fields later:

{
    "id": "0x000000000000000000000000000000000000000A"
}

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

done

resp := httptest.NewRecorder()

mockIdm := newManager("0x000000000000000000000000000000000000000A")
handlerFunc := ListIdentitiesEndpointFactory(mockIdm).ListIdentities

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

Factory could return Handler callback

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

factory now returns an object with Get handler.
I think we might have a pattern where we'd have a single object for each endpoint with Get Put Delete handlers
let me know if you still want a factory that returns single callbacks for each endpoint method


mockIdm := newManager("0x000000000000000000000000000000000000000A")
handlerFunc := ListIdentitiesEndpointFactory(mockIdm).ListIdentities
handlerFunc(resp, req, httprouter.Params{})

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 7, 2017

Member

I think 3rd parameter could be optional. Should try

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 9, 2017

Author Contributor

it's not optional in this router library

1. moved accounts/keystore to main command scope (client & server)
2. changed signature of NewIdentityManager (dir string)->(keystore
identity.keystoreInterface)
3. hiding private members exposed by last commit
4. broken directory link at identity/controller
"net/http"
)

type CollectionTequilaInterface interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 11, 2017

Member

What is das?


func IdentityHandlers(idm *identity.IdentityManager) *IdentitiesApi {
return &IdentitiesApi{getIdentityArray: idm.GetIdentities}
}

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 11, 2017

Member

Why do we need intermediate getIdentityArray func if we depend on IdentityManager anyway? just store the reference in the struct and call GetIdentities when needed.
Another way would be get rid of IdentityManager dependecy and pass getIdentityArray to IdentityHandlers. But current situation is overabstracted and doesnt add anything

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 12, 2017

Author Contributor

fixed

cache *identityCache
}

func SelectIdentity(dir string, nodeKey string) (id *dto.Identity, err error) {
handler := NewIdentityHandler(dir)
func SelectIdentity(keystore keystoreInterface, nodeKey string) (id *dto.Identity, err error) {

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 11, 2017

Member

passing key store to select identity looks bad, shouldn't this function be part of identitymanager? I hope it's in the middle of refactoring

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 12, 2017

Author Contributor

@ignasbernotas is taking care of this from here afaik.

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

I would like to merge this.
Test_CreateNewIdentity is failing, but some voodoomagic happening there. when merged will create a ticket to fix this test, or submit your fix. @interro
@tadovas your comments addressed.
@Waldz waiting for your approval and merging tonight.

@tadovas

This comment has been minimized.

Copy link
Member

commented Dec 12, 2017

Failing tests == failing builds. I would like to have all tests fixed/refactored before accepting pull request

@shroomist

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

tests fixed

@@ -19,7 +20,8 @@ func (hfts *handleFunctionTestStruct) httprouterHandle(resp http.ResponseWriter,
func TestHttpRouterHandlesRequests(t *testing.T) {
ts := handleFunctionTestStruct{false}

router := NewApiEndpoints()
idmFake := identity.NewIdentityManagerFake()
router := NewApiEndpoints(idmFake)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member

You dont need to boo all API routes, while testing only one Router.
Construct Router{} structure without factory

"github.com/stretchr/testify/assert"
)

func TestListIdentities(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member

What about one more test then zero identities exist?

"github.com/mysterium/node/tequilapi/utils"
)

type IdentityData struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member
  • IdentityData -> IdentityDto
  • Identities -> IdentityListDto
  • tequilapi/endpoints/identities.go -> tequilapi/endpoints/identities/endpoint.go
  • DTO object could be described in extra file dto.go
  • DTO structures does not need to be public
@@ -5,42 +5,40 @@ import (
"github.com/ethereum/go-ethereum/common"
)

type keyStoreFake struct {
type KeyStoreFake struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member
  • KeyStoreFake does not need to be public, it's interface is public
func (idm *identityManager) CreateNewIdentity(passphrase string) (*dto.Identity, error) {
account, err := idm.keystoreManager.NewAccount(passphrase)
func (idm *identityManager) CreateNewIdentity(addrToHex string) (*dto.Identity, error) {
account, err := idm.keystoreManager.NewAccount(addrToHex)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member

Should be passphrase, doublecheck

}

func newManager() identity.IdentityManagerInterface {
keystoreFake := &identity.KeyStoreFake{}

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member

Use your fake identity manager

idm identity.IdentityManagerInterface
}

func NewIdentitiesEndpoint(idm identity.IdentityManagerInterface) *IdentitiesApi {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 13, 2017

Member

IdentitiesApi does not need to be public

apiEndpoints := tequilapi.NewApiEndpoints()
//TODO additional endpoint registration can go here i.e apiEndpoints.GET("/path", httprouter.Handle function)
// options.keystoreDir still to be implemented. represents keystore directory/file
keystore := keystore.NewKeyStore(options.DirectoryKeystore, keystore.StandardScryptN, keystore.StandardScryptP)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 13, 2017

Member

Variable 'keystore' collides with imported package name

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 13, 2017

Author Contributor

nice catch

@Waldz Waldz changed the title MYST-101 client. list identities teqila endpoint MYST-101 client. list identities teqilapi Dec 14, 2017

assert.Equal(t, *identity, dto.Identity("0x000000000000000000000000000000000000bEEF"))
assert.Len(t, manager.keystoreManager.Accounts(), 2)
assert.Equal(t, dto.Identity("0x000000000000000000000000000000000000bEEf"), *identity)
assert.Len(t, manager.ksmFake.Accounts(), 1)

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Member

Dont adopt test by failing result

}

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

func (self *keyStoreFake) NewAccount(passphrase string) (accounts.Account, error) {
func (self *keyStoreFake) NewAccount(address string) (accounts.Account, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Member

It's passphrase, not address

if self.ErrorMock != nil {
return accounts.Account{}, self.ErrorMock
}

accountNew := accounts.Account{
Address: common.HexToAddress("0x000000000000000000000000000000000000bEEF"),
Address: common.HexToAddress(address),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Member

All new accounts was create with ending "...bEEF"


func NewIdentityManagerFake() *idmFake {
return &idmFake{
ksmFake:NewKeystoreFake(),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 14, 2017

Member

Dont depend on keystore, then You dont need it

@Waldz

Waldz approved these changes Dec 14, 2017

Ignas is fixing those issues

@Waldz Waldz merged commit 3844da1 into master Dec 14, 2017

@Waldz Waldz deleted the feature/MYST-101-list-identities branch Dec 14, 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.