Use multiaddrs instead of UNIX socket paths for daemon sockets #41

mvid · 2018-12-06T22:19:33Z

No description provided.

bigs

some minor bits but generally seems good

ROADMAP.md

pb/p2pd.proto

specs/CONTROL.md

vyzo

Do we really want this change? It complicates a tad.

package.json

vyzo · 2018-12-07T08:49:58Z

I would like to see implementation go together with the spec for this, otherwise we'll have a spec that's incompatible with the implementation.

mvid · 2018-12-11T00:21:01Z

@vyzo @bigs This is ready for review

vyzo

You have removed the compiled protobuf; please regenerate with protoc --gogofast_out=. p2pd.proto.

conn.go

daemon.go

dht.go

p2pclient/p2pclient.go

p2pclient/streams.go

p2pd/main.go

stream.go

vyzo · 2018-12-11T17:48:50Z

please refrain from changing code you are not actively working on, especially little things like that :)

the convention is to have 3 groups of imports 1. system imports (unnamed), 2. same package imports (usually named) and 3. gx imports (named).

mvid · 2018-12-11T19:38:24Z

please refrain from changing code you are not actively working on, especially little things like that :)

the convention is to have 3 groups of imports 1. system imports (unnamed), 2. same package imports (usually named) and 3. gx imports (named).

I disabled the inspection, so the import auto-optimization should no longer happen. I have replaced the redundant names. If you would like me to commit my JetBrains configuration so that anyone else who develops with it won't cause automatic import optimization, please let me know.

p2pclient/streams.go

p2pd/main.go

test/utils.go

dht.go

p2pd/main.go

vyzo · 2018-12-12T07:56:37Z

conn.go

-	path := *req.StreamHandler.Path
+	maddr, err := ma.NewMultiaddrBytes(req.StreamHandler.Addr)
+	if err != nil {
+		return errorResponseString(err.Error())


return errorResponse(err)

i believe the error itself implements the error interface

stream.go

bigs · 2018-12-13T20:52:09Z

pb/p2pd.pb.go

@@ -3,11 +3,11 @@

 package p2pd_pb

-import proto "github.com/gogo/protobuf/proto"
+import proto "github.com/golang/protobuf/proto"


should be generating with gogo

Regenerated with protoc --gogofast_out=. p2pd.proto, also committed that line to a .sh

bigs · 2018-12-18T19:22:55Z

after that small simplification re: error, let's merge

vyzo · 2018-12-18T19:24:04Z

fix pb generation with gogo -- that's pretty important. Also, a conflict has developed in package.json.

bigs · 2018-12-18T19:24:38Z

gah @vyzo my diff is rendering so oddly. good catch, specifically the gogo bit. i'll look into the gx deps.

vyzo

Documentation changes, the code looks fine.

vyzo · 2018-12-21T09:26:21Z

ROADMAP.md

-those endpoints are converted to writes and reads to/from the stream, allowing
-any application to interact with a libp2p network through simple, local IO.
+negotiation, and protocol and stream multiplexing. Streams are mapped 1:1 to 
+connections over streaming transports supported by go-multiaddr-net. Writes and 


over streaming transports supported by go-multiaddr-net

That's not good enough, this is an irrelevant implementation detail.
Please specify UNIX domain sockets and tcp.

vyzo · 2018-12-21T09:29:07Z

specs/CONTROL.md

@@ -21,8 +21,7 @@ There are two pieces to the libp2p daemon:

 ### Technical Details

-The libp2p daemon and client will communicate with each other over a simple Unix
-socket based protocol built with [protobuf](https://developers.google.com/protocol-buffers/).
+The libp2p daemon and client will communicate with each other over any multiaddr supported protocol with [protobuf](https://developers.google.com/protocol-buffers/).


again here, developer don't care about the multiaddr detail...
"they communicate over a simple socket based protocol" is sufficient.

vyzo · 2018-12-21T09:29:40Z

specs/CONTROL.md

-careful not to read excess bytes from the socket when parsing the daemon
-response, otherwise they risk reading into the stream output.
+After writing the response message to the address, the daemon begins piping the
+newly created stream to the client over the multi-address supported transport. 


just socket; wtf is a multiaddress supported transport anyway?

vyzo · 2018-12-27T09:06:38Z

I took it upon myself to fix the documentation issues.

vyzo · 2018-12-27T09:08:33Z

~~@mvid can you pull and then rebase/squash this to a few commits without delete/undelete flip-flops etc?~~

nvm, we can merge as is.

vyzo · 2018-12-27T09:11:27Z

Also, it currently doesn't build because of gx dupes issues.

vyzo · 2018-12-27T09:13:44Z

There is an avalanche of errors related to gx deps issues; we'll have to bubble up some changes it seems.

vyzo · 2018-12-27T10:39:10Z

Bubbled up gx updates and resolved the dep dupes.

TBD: testing with gerbil-libp2p.

vyzo · 2018-12-27T11:15:32Z

tested with gerbil-libp2p, everything appears to be in order.

vyzo · 2018-12-27T11:17:08Z

p2pd/main.go

@@ -110,7 +116,7 @@ func main() {
 	}

 	if !*quiet {
-		fmt.Printf("Control socket: %s\n", *sock)
+		fmt.Printf("Control multiaddr: %s\n", maddr.String())


that's still a socket...

vyzo

this is ready for merge

mvid requested a review from bigs December 6, 2018 22:19

ghost assigned mvid Dec 6, 2018

ghost added the in progress label Dec 6, 2018

bigs suggested changes Dec 6, 2018

View reviewed changes

ROADMAP.md Outdated Show resolved Hide resolved

ROADMAP.md Outdated Show resolved Hide resolved

pb/p2pd.proto Outdated Show resolved Hide resolved

specs/CONTROL.md Outdated Show resolved Hide resolved

specs/CONTROL.md Outdated Show resolved Hide resolved

vyzo reviewed Dec 7, 2018

View reviewed changes

package.json Show resolved Hide resolved

mvid force-pushed the feat/multiaddr-spec branch 2 times, most recently from 5e3c761 to 9bdda06 Compare December 11, 2018 00:18

mvid changed the title ~~Update documentation to refer to multi-addrs instead of unix sockets~~ Update documentation/implementation to use multi-addrs instead of unix sockets Dec 11, 2018

vyzo requested changes Dec 11, 2018

View reviewed changes

bigs suggested changes Dec 11, 2018

View reviewed changes

p2pclient/streams.go Show resolved Hide resolved

p2pd/main.go Show resolved Hide resolved

test/utils.go Show resolved Hide resolved

bigs approved these changes Dec 11, 2018

View reviewed changes

vyzo reviewed Dec 12, 2018

View reviewed changes

vyzo mentioned this pull request Dec 12, 2018

Windows IPC support #18

Closed

3 tasks

bigs approved these changes Dec 18, 2018

View reviewed changes

mvid force-pushed the feat/multiaddr-spec branch from 812d852 to ccd3a40 Compare December 18, 2018 19:57

vyzo changed the title ~~Update documentation/implementation to use multi-addrs instead of unix sockets~~ Use multiaddrs instead of UNIX socket paths for daemon sockets Dec 21, 2018

vyzo requested changes Dec 21, 2018

View reviewed changes

ghost assigned vyzo Dec 27, 2018

vyzo force-pushed the feat/multiaddr-spec branch from fbdbe90 to 25b3a7c Compare December 27, 2018 10:38

mvid and others added 13 commits December 27, 2018 12:38

update to documentation relating to sockets

07bb326

delete

763eb96

undelete

518ea09

gofast generated

88d35ae

first pass notes

e0e0420

named imports

05673e8

update create functions

961df89

socket naming convention

0d72350

review notes

cdf8cea

gogo fast

117fd65

commit regenerationg script for pb

1874c44

fix documentation issues

900c9b8

gx update deps

25b3a7c

vyzo mentioned this pull request Dec 27, 2018

Control paths are multiaddrs vyzo/gerbil-libp2p#9

Merged

vyzo reviewed Dec 27, 2018

View reviewed changes

vyzo added 3 commits December 27, 2018 13:19

fix nit

678e75b

fix nonsensical wording

5975489

fix more nonsensical wording

41b859c

vyzo approved these changes Dec 27, 2018

View reviewed changes

vyzo merged commit 1d7d0a2 into master Dec 27, 2018

vyzo deleted the feat/multiaddr-spec branch December 27, 2018 11:29

ghost removed the in progress label Dec 27, 2018

This was referenced Feb 13, 2019

Use multiaddrs instead of UNIX socket paths for daemon sockets libp2p/js-libp2p-daemon#9

Closed

Use multiaddrs instead of UNIX socket paths for daemon sockets libp2p/js-libp2p-daemon-client#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use multiaddrs instead of UNIX socket paths for daemon sockets #41

Use multiaddrs instead of UNIX socket paths for daemon sockets #41

mvid commented Dec 6, 2018

bigs left a comment

vyzo left a comment

vyzo commented Dec 7, 2018

mvid commented Dec 11, 2018

vyzo left a comment

vyzo commented Dec 11, 2018

mvid commented Dec 11, 2018

vyzo Dec 12, 2018

mvid Dec 12, 2018

bigs Dec 18, 2018

bigs Dec 13, 2018

mvid Dec 18, 2018

bigs commented Dec 18, 2018

vyzo commented Dec 18, 2018

bigs commented Dec 18, 2018

vyzo left a comment

vyzo Dec 21, 2018

vyzo Dec 21, 2018

vyzo Dec 21, 2018

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018 •

edited

Loading

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

vyzo Dec 27, 2018

vyzo left a comment

Use multiaddrs instead of UNIX socket paths for daemon sockets #41

Use multiaddrs instead of UNIX socket paths for daemon sockets #41

Conversation

mvid commented Dec 6, 2018

bigs left a comment

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

vyzo commented Dec 7, 2018

mvid commented Dec 11, 2018

vyzo left a comment

Choose a reason for hiding this comment

vyzo commented Dec 11, 2018

mvid commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigs commented Dec 18, 2018

vyzo commented Dec 18, 2018

bigs commented Dec 18, 2018

vyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018 • edited Loading

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

vyzo commented Dec 27, 2018

Choose a reason for hiding this comment

vyzo left a comment

Choose a reason for hiding this comment

vyzo commented Dec 27, 2018 •

edited

Loading