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

Replace json format with protobuf Struct #1192

Merged
merged 21 commits into from
Jul 26, 2019
Merged

Conversation

krhubert
Copy link
Contributor

@krhubert krhubert commented Jul 22, 2019

fix #1176

@antho1404
Copy link
Member

antho1404 commented Jul 23, 2019

I don't understand why you don't directly use import "google/protobuf/struct.proto";
Now because you use your own struct this is not recognized by proto and it will not do any processing on the message but if you use the one from google you should have pretty much nothing to do.

If I compile with the official struct I have in my generated file a new type imported _struct "github.com/golang/protobuf/ptypes/struct"

Also, please description, labels, issue solved, project related on the pull requests ;)

@krhubert
Copy link
Contributor Author

I don't understand why you don't directly use import "google/protobuf/struct.proto";

google/protobuf/struct.proto: File not found.
protobuf/types/event.proto:3:1: Import "google/protobuf/struct.proto" was not found or had errors.

We have to pass path to struct when building (flag --proto_path) and we don't keep protobuf definitions in repo. We have to copy it somewhere (so I did it with just few modifications)

Now because you use your own struct this is not recognized by proto and it will not do any processing on the message but if you use the one from google you should have pretty much nothing to do.

What do you mean "is not recognized by proto"/"will not do any processing on the message" - any more info.

If I compile with the official struct I have in my generated file a new type imported _struct "github.com/golang/protobuf/ptypes/struct"

Yes, the import path is different (if this is what you mean) because the go_package in the officaial file is different. But they are pretty much the same after generation.

https://github.com/golang/protobuf/blob/master/ptypes/struct/struct.pb.go

Also, please description, labels, issue solved, project related on the pull requests ;)

Sure :)

@krhubert krhubert added the wip label Jul 23, 2019
@krhubert
Copy link
Contributor Author

fix #1176

wip: need to fix ethwallet as well.

@@ -8,7 +8,9 @@ require (
github.com/deckarep/golang-set v1.7.1 // indirect
github.com/ethereum/go-ethereum v1.8.27
github.com/go-stack/stack v1.8.0 // indirect
github.com/mesg-foundation/engine v0.5.1-0.20190702155243-a2826b3949a7
github.com/mesg-foundation/core v0.10.1 // indirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it needed?

@krhubert
Copy link
Contributor Author

Ok I think I'm done here, only tests left but for tests I need cli updated @NicolasMahe @antho1404 can you prepare an update of cli so I can test eth wallet and marketplace?

@antho1404
Copy link
Member

can't this be tested just with the api? Also is this ready to review? there is still the wip label? Please also add details on this PR as discussed

@krhubert krhubert added this to the next milestone Jul 25, 2019
@krhubert
Copy link
Contributor Author

krhubert commented Jul 25, 2019

can't this be tested just with the api?

I don't know because it might be hard to pass protobuf.Struct as an option via grpcurl tool.
So I need to check if this can be tested with just api but mesg-cli needs an update anyway, so why to test it twice, if this could be done with mesg-cli.

Also is this ready to review? there is still the wip label?

It's ready for review, but it's not tested this is why I left wip label.

@antho1404
Copy link
Member

antho1404 commented Jul 25, 2019

This would actually make grpcurl easier because we don't have to json stringify the inputs and just pass them in a map directly.

example:

grpcurl -plaintext -d "{\"instanceHash\":\"6LfmaGAKcpGT93z6fMp1PVqraCbXstWwaSwM7HdvfgPp\", \"taskKey\":\"taskX\", \"inputs\":{\"foo\": \"hello\", \"bar\":\"world\"}}" localhost:50052 api.Execution/Create

instead of something like

grpcurl -plaintext -d "{\"instanceHash\":\"6LfmaGAKcpGT93z6fMp1PVqraCbXstWwaSwM7HdvfgPp\", \"taskKey\":\"taskX\", \"inputs\":\"{\\\"foo\\\": \\\"hello\\\", \\\"bar\\\":\\\"world\\\"}\"}" localhost:50052 api.Execution/Create

Also yes cli and lib needs to be updated when this development is finished and tested


You did update the ethwallet service so you could do the tests with this service and execute tasks/events to see if everything works fine.

From my tests, it doesn't work

grpcurl -plaintext -d "{\"instanceHash\":\"Hsy6y2i7ZxrTMHFb39sm1bLrSB2QenjXEcGKcNjb7kbw\", \"taskKey\":\"list\", \"inputs\":{}}" localhost:50052 api.Execution/Create

My execution stays in progress

@antho1404
Copy link
Member

I have a lot of issues testing with the ethwallet service (that is the only one updated with the new api)

Example:

Invalid inputs for the list task

grpcurl -plaintext -d "{\"instanceHash\":\"Hsy6y2i7ZxrTMHFb39sm1bLrSB2QenjXEcGKcNjb7kbw\", \"taskKey\":\"list\", \"inputs\":{\"passphrase\":\"1\"}}" localhost:50052 api.Execution/Create

No errors in the engine, execution with the passphrase as input and still in pending.

Generate a new address

grpcurl -plaintext -d "{\"instanceHash\":\"Hsy6y2i7ZxrTMHFb39sm1bLrSB2QenjXEcGKcNjb7kbw\", \"taskKey\":\"create\", \"inputs\":{\"passphrase\":\"1\"}}" localhost:50052 api.Execution/Create

Execution working but with an error invalid character '\\x11' looking for beginning of value

@NicolasMahe
Copy link
Member

Invalid inputs for the list task

grpcurl -plaintext -d "{\"instanceHash\":\"Hsy6y2i7ZxrTMHFb39sm1bLrSB2QenjXEcGKcNjb7kbw\", \"taskKey\":\"list\", \"inputs\":{\"passphrase\":\"1\"}}" localhost:50052 api.Execution/Create

No errors in the engine, execution with the passphrase as input and still in pending.

This is not related to this PR. The Engine only check for required data and their type, not for data that are not declared by the task.

@antho1404
Copy link
Member

sure but the execution should be completed and it is not but I agree, the parameter is irrelevant

Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a big breaking change but I'm ok to release it in this week release though :) It's really nicer.

I'm excited to see the simplification on the js lib :)

@krhubert krhubert removed the wip label Jul 25, 2019
@antho1404 antho1404 merged commit 41d0482 into dev Jul 26, 2019
@antho1404 antho1404 deleted the refactor/proto-drop-json branch July 26, 2019 03:26
@mesg-bot
Copy link
Member

mesg-bot commented Aug 8, 2019

This pull request has been mentioned on MESG Community. There might be relevant details there:

https://forum.mesg.com/t/release-notes-of-engine-v0-12-cli-v1-2-and-js-library-v4-2/367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the json encoded in the API
4 participants