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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST-153 / Refactored dto.Identity string to identity.Identity struct #60

Merged

Conversation

Projects
None yet
4 participants
@ignasbernotas
Copy link
Member

commented Dec 18, 2017

Have fun 馃槀

@ignasbernotas ignasbernotas changed the title Refactored dto.Identity string to identity.Identity struct MYST-153 / Refactored dto.Identity string to identity.Identity struct Dec 18, 2017

@tadovas
Copy link
Member

left a comment

That id package alias is a bit confusing

@@ -2,7 +2,7 @@ package client_connection

import (
"errors"
"github.com/mysterium/node/service_discovery/dto"
id "github.com/mysterium/node/identity"

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 19, 2017

Member

confusing naming. Identity from "id" package :/ why we need alias here? (and in other imports)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

I just wanted to shorten it, plus there are quite a few variables called "identity" so it kind of collides with the package name. Didn't want to refactor too much.

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 19, 2017

Contributor

I did actually use id for variable names to avoid this clash in my code xD
I don't like this alias

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

:D so should I change it? Already got a commit for it.

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Yeah, looks strange for me also

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

Cool, this will be pushed in a bit.

Id string
}

func NewIdentity(id string) Identity {

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 19, 2017

Member

Are we really making "new" identity here or just creating some kind of Identity from string? What about identity.FromString(id string)?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

Hmm, not sure. The "New" convention is what we usually go for, so maybe worth leaving this as is? Though I do get what you're saying. @tadovas @Waldz

This comment has been minimized.

Copy link
@shroomist

This comment has been minimized.

Copy link
@shroomist

shroomist Dec 19, 2017

Contributor

to be more specific, I think it goes like identity.FromEthAddress(address)

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

Maybe use FromAddress(), with a change of Identity struct Id property to Address?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

Just need advice on this last bit. Problem with Id -> Address rename is that a lot of outside structures (DTOs) use IdentityId naming.

This comment has been minimized.

Copy link
@tadovas

tadovas Dec 19, 2017

Member

It would make sense if it signature would be FromAddress(ethAddress Address) but now we are "creating" "identity" from string, looks like a simple converter to me, and not to identity but to some kind of Identifier which can be used with identityManager to load real identity

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Looks like good idea identity.address. But lets not hurry with that, we might have separate topup address.
NewIdentity() - usual factory pattern looks good for me
FromId() - looks good also

)

func TestRequestSerialize(t *testing.T) {
var identity = dto_discovery.Identity("123")
var identity = identity.NewIdentity("123")

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need identity struct, could be just string

@@ -48,14 +48,14 @@ func TestRequestUnserialize(t *testing.T) {
"identity_id": "123"
}`,
dialogCreateRequest{
IdentityId: dto_discovery.Identity("123"),
IdentityId: identity.NewIdentity("123").Id,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need identity struct, could be just string

"io/ioutil"
"encoding/json"
"os"
"path/filepath"
)

type cacheData struct {
Identity dto.Identity `json:"identity"`
Identity Identity `json:"identity"`

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

should not be string?

This comment has been minimized.

Copy link
@ignasbernotas

ignasbernotas Dec 19, 2017

Author Member

Doesn't matter. We can store the full structure in the cache file.

@@ -39,7 +40,7 @@ func NewServiceProposalWithLocation(
Price: money.NewMoney(0.125, money.CURRENCY_MYST),
Duration: 1 * time.Hour,
},
ProviderId: dto_discovery.Identity(providerId),
ProviderId: id.Identity(providerId).Id,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Those casting looks strange. Should not it be just providerId.Id?

@@ -38,7 +39,7 @@ func TestServiceProposalUnserialize(t *testing.T) {
ServiceDefinition: dto_openvpn.ServiceDefinition{},
PaymentMethodType: "PER_TIME",
PaymentMethod: dto_openvpn.PaymentMethodPerTime{},
ProviderId: dto_discovery.Identity("node"),
ProviderId: id.NewIdentity("node").Id,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

just string "node"

@@ -119,7 +120,7 @@ func TestServiceProposalSerialize(t *testing.T) {
ServiceDefinition: dto_openvpn.ServiceDefinition{},
PaymentMethodType: "PER_TIME",
PaymentMethod: dto_openvpn.PaymentMethodPerTime{},
ProviderId: dto_discovery.Identity("node"),
ProviderId: id.NewIdentity("node").Id,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Just string "node"

@@ -27,7 +28,7 @@ func TestServiceProposalSerialize(t *testing.T) {
ServiceDefinition: TestServiceDefinition{},
PaymentMethodType: "PER_TIME",
PaymentMethod: TestPaymentMethod{},
ProviderId: Identity("node"),
ProviderId: identity.NewIdentity("node").Id,

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Just string "node"

@@ -26,7 +26,7 @@ func (endpoint *identitiesApi) List(writer http.ResponseWriter, request *http.Re
idsSerializable := make([]identityDto, len(idArry))
for i, id := range idArry {
idsSerializable[i] = identityDto{
Id: string(id),
Id: string(id.Id),

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need to cast string -> string

},
})
if err != nil || response.(*dialogCreateResponse).Reason != 200 {
return nil, fmt.Errorf("Dialog creation rejected: %s", err)
}

dialogAddress := nats_discovery.NewAddressNested(contactAddress, string(establisher.myIdentity))
dialogAddress := nats_discovery.NewAddressNested(contactAddress, string(establisher.myIdentity.Id))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need to cast string -> string

func NewAddressForIdentity(identity dto_discovery.Identity) *NatsAddress {
return NewAddress("127.0.0.1", 4222, string(identity))
func NewAddressForIdentity(identity id.Identity) *NatsAddress {
return NewAddress("127.0.0.1", 4222, string(identity.Id))

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need to cast string -> string

identityString = strings.ToLower(identityString)
for _, identity := range idm.GetIdentities() {
if strings.ToLower(string(identity)) == identityString {
if strings.ToLower(string(identity.Id)) == identityString {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need to cast string -> string

identity, err = ih.cache.GetIdentity()

if err != nil || !ih.manager.HasIdentity(string(identity)) {
if err != nil || !ih.manager.HasIdentity(string(identity.Id)) {

This comment has been minimized.

Copy link
@Waldz

Waldz Dec 19, 2017

Member

Dont need to cast string -> string

@tadovas
Copy link
Member

left a comment

LGTM

@ignasbernotas ignasbernotas merged commit 0448b26 into master Dec 19, 2017

@ignasbernotas ignasbernotas deleted the feature/MYST-153-separate-identity-dto-from-discovery branch Dec 19, 2017

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