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-88/MYST-89 Service proposal serialisation #39

Merged
merged 4 commits into from Oct 25, 2017

Conversation

Projects
None yet
2 participants
@ignasbernotas
Copy link
Member

commented Oct 20, 2017

No description provided.

@ignasbernotas

This comment has been minimized.

Copy link
Member Author

commented on cmd/mysterium_client/command_run/command.go in 84bc47c Oct 20, 2017

Need a proper way to load the openvpn/dto package init()

@ignasbernotas ignasbernotas requested review from interro and Waldz Oct 20, 2017

"github.com/mysterium/node/server"
dto_discovery "github.com/mysterium/node/service_discovery/dto"
"io"
"time"
"github.com/mysterium/node/openvpn/service_discovery/dto"

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

dto_openvpn


cmd.communicationClient = cmd.CommunicationClientFactory(consumerId)
_, _, err = cmd.communicationClient.CreateDialog(serviceProposal.ProviderContacts[0])
dto.Initialize()

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

Could it be more breath openvpn.Initialize() maybe?

_, _, err = cmd.communicationClient.CreateDialog(serviceProposal.ProviderContacts[0])
dto.Initialize()

// rename this

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

What's da point of useless comment?

if err != nil {
return err
}

vpnSession, err := cmd.MysteriumClient.SessionCreate(options.NodeKey)
cmd.communicationClient = cmd.CommunicationClientFactory(consumerId)

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

Looks like same code and no need for newline

if err != nil {
return err
}

vpnConfig, err := openvpn.NewClientConfigFromString(
vpnSession.ConnectionConfig,
vpnSession.ServiceProposal.ConnectionConfig,

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

proposal used in several places, I would put it to separate variable

"payment_method_type": "PER_TIME",
"payment_method": {},
"provider_contacts": [
{

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

NATS contact should be tested else-there

err := json.Unmarshal(jsonData, &actual)

assert.Nil(t, err)
assert.Equal(t, "nats.ContactNATSV1", reflect.TypeOf(actual.ProviderContacts[0].Definition).String())

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

Too strange assertion

}

length := len(contacts)
if length == 0 {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

I dont think special case for 0 case is needed. Empty list will traverse 0 times

@@ -88,7 +143,7 @@ func (genericProposal *ServiceProposal) UnmarshalJSON(data []byte) (err error) {
genericProposal.ServiceType = jsonData.ServiceType
genericProposal.ProviderId = Identity(jsonData.ProviderId)
genericProposal.PaymentMethodType = jsonData.PaymentMethodType
genericProposal.ProviderContacts = jsonData.ProviderContacts
//genericProposal.ProviderContacts = jsonData.ProviderContacts

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

Cleanup

@@ -111,5 +166,13 @@ func (genericProposal *ServiceProposal) UnmarshalJSON(data []byte) (err error) {
return
}

if jsonData.ProviderContacts != nil {

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 20, 2017

Member

It should no be nilable atrribute, it should be:

[]struct {
    Type       string           `json:"type"`
    Definition *json.RawMessage `json:"definition"`
 }
@Waldz

Waldz approved these changes Oct 25, 2017

@@ -76,7 +128,7 @@ func (genericProposal *ServiceProposal) UnmarshalJSON(data []byte) (err error) {
PaymentMethodType string `json:"payment_method_type"`
ServiceDefinition *json.RawMessage `json:"service_definition"`
PaymentMethod *json.RawMessage `json:"payment_method"`
ProviderContacts []Contact `json:"provider_contacts"`
ProviderContacts *json.RawMessage `json:"provider_contacts"`

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 25, 2017

Member

Here You could have type []contactJson, why:

  • avoid parsing root json string twice
  • gives You already parsed array of untyped contacts

@Waldz Waldz merged commit 6f69d39 into master Oct 25, 2017

@Waldz Waldz deleted the feature/MYST-38-proposal-register branch Oct 26, 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.