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

Feature/myst 586 endpoint with identity registration status #310

Merged

Conversation

Projects
None yet
6 participants
@tadovas
Copy link
Member

commented Aug 1, 2018

  1. New tequila api endpoint /identities//registration provides registration status and required data for identity registration if needed (smart contract call)
  2. Localnet now contains private blockchain with the same state on startup
  3. E2e tests enriched with identity registration greenpath, also no ports are published/bound on host - e2e tests are fully isolated inside docker

@tadovas tadovas requested review from donce and Waldz as code owners Aug 1, 2018

@tadovas tadovas requested a review from zolia Aug 1, 2018

@donce

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2018

tumblr_n1pyp1fibf1ts0osto2_400

}

// TestnetDefinition defines parameters for test network (currently default network)
var TestnetDefinition = NetworkDefinition{
"https://testnet-api.mysterium.network/v1",
"testnet-broker.mysterium.network",
"https://ropsten.infura.io",

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

without user key? Will be request limited?

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 3, 2018

Author Member

Currently there is no limitation of request to infura, with or without key. In this case if such problem arrises - user can always specify infura link with api key embedded.

@@ -106,11 +106,11 @@ func NewCommand(options CommandOptions) *Command {
tequilapi_endpoints.AddRoutesForProposals(router, mysteriumClient)
tequilapi_endpoints.AddRouteForStop(router, node_cmd.SoftKiller(command.Kill))

client, err := blockchain.NewClient("https://ropsten.infura.io")
client, err := blockchain.NewClient(networkDefinition.EtherClientRPC)
if err != nil {
fmt.Println("Error: ", err.Error())

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

We might use log.Error here..

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 6, 2018

Author Member

This is outdated version

VOLUME "/ethereum/geth-runtime"

RUN curl https://gethstore.blob.core.windows.net/builds/geth-linux-amd64-1.8.12-37685930.tar.gz > downloaded.tar.gz
RUN tar --strip 1 -xvf downloaded.tar.gz && rm -rf downloaded.tar.gz

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

We could merge these two commands via pipe, no need for extra file:

curl http://somefile.tar.gz | tar -zxf -

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 3, 2018

Author Member

And it worked. 💯 karma points!

@@ -0,0 +1 @@
{"address":"a754f0d31411d88e46aed455fa79b9fced122497","crypto":{"cipher":"aes-128-ctr","ciphertext":"56c3a9b187b9acfe64d9d9ab2f0236f990f533f99ddb05cc0d5be9340f7fb755","cipherparams":{"iv":"fa76b9c835859593db14cd4820078b88"},"kdf":"scrypt","kdfparams":{"dklen":32,"n":262144,"p":1,"r":8,"salt":"3dfdd74c4e7eb4d6ef3b449b836438f3082a34bd0c6dc82a85bde4f30421e1b9"},"mac":"97c1323a22cddb1dac9652a1b3b6b07706193a12cf056256dee963647686cde7"},"id":"965998e2-2a62-4aae-9b0d-b3a723b8d521","version":3}

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

private key in public repo?

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

ok, its localnet 👍

func (c *Command) registration(argsString string) {
status, err := c.tequilapi.RegistrationStatus(argsString)
if err != nil {
warn("Something went wrong: ", err)

This comment has been minimized.

Copy link
@zolia

zolia Aug 3, 2018

Member

we could implement new method here, like error(items ...interface{}), since its error, not warning.

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 3, 2018

Author Member

This sounds like another PR to improve CLI "ux". Let's register task for that

@tadovas tadovas force-pushed the feature/MYST-586-endpoint-with-identity-registration-status branch from 8524cee to 1779b4d Aug 3, 2018

@@ -0,0 +1,14 @@
FROM frolvlad/alpine-glibc

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 3, 2018

Member

I see this as separate repo

This comment has been minimized.

Copy link
@tadovas
)

func main() {
flag.Parse()

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 3, 2018

Member

There are flags You are parsing?

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 6, 2018

Author Member

Scattered in helpers which are imported from payments lib :( it was not a good idea to register params as "globals". Will improve payments lib and come back here

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 7, 2018

Author Member

Extracted as list of args before flag parse.

"fmt"
"github.com/MysteriumNetwork/payments/cli/helpers"
"github.com/MysteriumNetwork/payments/mysttoken/generated"
generated2 "github.com/MysteriumNetwork/payments/promises/generated"

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 3, 2018

Member

Why not generated69? Lets add meaningful package name

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 6, 2018

Author Member

aliased as promises and mysttoken accordingly. This is a good idea to improve payments lib

"flag"
"fmt"
"github.com/MysteriumNetwork/payments/cli/helpers"
"github.com/MysteriumNetwork/payments/mysttoken/generated"

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 3, 2018

Member

same here

// Returns true if identity is registered in payments smart contract
Registered bool

PublicKey *PublicKeyPartsDTO `json:"PublicKey,omitempty"`

This comment has been minimized.

Copy link
@mipo47

mipo47 Aug 6, 2018

To keep same styling, let's start JSON values names from lower case:
PublicKey -> publicKey
Signature -> signature

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 6, 2018

Author Member

👍

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 6, 2018

Author Member

Improved

@tadovas tadovas force-pushed the feature/MYST-586-endpoint-with-identity-registration-status branch from 4487f7a to 5c6d99a Aug 7, 2018

@tadovas

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

@Waldz all comments addresed. Please review

tadovas added some commits Jul 18, 2018

tadovas and others added some commits Aug 1, 2018

@tadovas tadovas force-pushed the feature/MYST-586-endpoint-with-identity-registration-status branch from e269e57 to 2ccf5a4 Aug 8, 2018

@@ -82,3 +82,23 @@ type BuildInfoDTO struct {
Branch string `json:"branch"`
BuildNumber string `json:"buildNumber"`
}

// RegistrationStatusDTO holds input data required to register new myst identity on blockchain smart contract
type RegistrationStatusDTO struct {

This comment has been minimized.

Copy link
@Waldz

Waldz Aug 8, 2018

Member

Your tequitelapi client-side DTOs is not compatible with Tequilapi service-side DTOs, see RegistrationDataDTO
Suggest to add JSON annotations

This comment has been minimized.

Copy link
@tadovas

tadovas Aug 8, 2018

Author Member

E2e test rely heavily on tequilapi client - in case of incomaptible DTO (response parsing), it would fail with fireworks. Anyway - added json annotations for the sake of clarity.

@Waldz

Waldz approved these changes Aug 9, 2018

@interro

interro approved these changes Aug 9, 2018

@tadovas tadovas merged commit b48aa38 into master Aug 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tadovas tadovas deleted the feature/MYST-586-endpoint-with-identity-registration-status branch Aug 9, 2018

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.