feat/v0.1.0 #9

daviddias · 2017-03-27T18:22:14Z

dependent PRs:

Relevant issues:

Tasks:

Tests:

Dialer
- should dial over relay if no common transport exists
- should dial over specific relay
  - should dial over specific peer routed relay
  - should dial over specific relay on the provided transport
  - should dial over chained relay address
Listener
- Should receive relayed connections
Circuit relay
- Should listen on /libp2p/circuit/relay/1.0.0/hop and forward connections to dst

chore: adding default readme feat: reworking as a transport feat: getting peers communicating over relay (wip) feat: address in swarm [wip] feat: adding onion dial feat: adding onion dialing and tests feat: make circuit a full fledged transport refactor: split transport dialer and circuit logic test: adding dial tests feat: adding passive/active dialing test test: adding relay tests fix: several isues feat: consolidate and cleanup dialing feat: handle listenning circuit addresses correctly feat: make utils a factory feat: adding StreamHandler to aid with pull-stream read/write refactor: clean up and refactor relay and listener tests: adding more relay and listener tests tests: moving long multiaddr to a fixture feat: adding _writeErr to handle returning errors in relay.js fix: cleanup, moving setup code outside of dialer

daviddias

Had to make my review as comments as I was the one creating this PR.

@dryajov lot's of good work here! I see some deadcode and I feel it can get a good cleanup. What do you think?

daviddias · 2017-05-10T20:49:58Z

README.md

@@ -1 +1,38 @@
-# js-libp2p-circuit
+# <topic>


daviddias · 2017-05-10T20:50:15Z

README.md

+
+## License
+
+[MIT](LICENSE) © 2016 Protocol Labs Inc.


The whole README needs to be done.

daviddias · 2017-05-10T20:50:28Z

LICENSE

@@ -0,0 +1,21 @@
+MIT License
+
+Copyright (c) 2017 libp2p


Protocol Labs Inc

daviddias · 2017-05-10T20:51:16Z

package.json

+ "rules": {
+ "strict": "off"
+ }
+ },


Please don't monkey patch our codestyle, we want to use the same for every single module in the same way.

daviddias · 2017-05-10T20:51:33Z

package.json

+ "pull-stream": "^3.5.0",
+ "safe-buffer": "^5.0.1",
+ "setimmediate": "^1.0.5"
+ }


Deps need to be updated

daviddias · 2017-05-10T20:55:33Z

src/circuit/onion-dialer.js

+'use strict'
+
+require('setimmediate')
+require('safe-buffer')


Same as above (safe-buffer)

daviddias · 2017-05-10T20:56:03Z

src/circuit/onion-dialer.js

+ }
+}
+
+module.exports = OnionDialer


What is this OnionDialer feature? First time reading about it.

I believe the term was brought up either by @lgierth or @whyrusleeping during one of our IRC convos and it describes one of two dialing modes, onion and telescope.

Onion refers to the client dialing all the relays in a multihop address

Telescope would dial the first hop and and send the reminder of the multihop address as the destination address to that relay for further dialing, effectively having each hop in the multihop address dial the next hop in the chain.

Yeah, that's accurate. We should add these two modes to the spec as well, and make sure they're reasonably good names. I can see how onion might not be the best names (although it's accurate).

I actually like onion 😄, but I can see how its not the most descriptive name. Maybe something that implies that the client is the one dialing all the relays - Client and Telescope?

+1 adding to the spec

+1 for picking a better name than onion

Have we settled on a name, should we vote? 🚀

daviddias · 2017-05-10T20:57:37Z

src/listener.js

+ }
+
+ return listener
+}


Is this file still used? Relay is not a transport.

Circuit currently relies on the transport interface to plug in with swarm, hence the Dialer/Listener parts.

See #9 (comment)

@diasdavid I think this has been resolved as part of our previous discussions?

daviddias · 2017-05-10T20:59:12Z

src/index.js

+ Dialer: require('./dialer'),
+ multicodec: require('./multicodec'),
+ tag: 'Circuit'
+}


I believe there should be here a file for

Dialer - what you currently have

Hop - the handler for the protocol /libp2p/relay/hop

Stop - the handler for the protocol /libp2p/relay/stop

Same reasons as above - circuit pretends to be a transport to allow pluging in with swarm/libp2p hence the weird similarities with other transport implementations.

daviddias · 2017-05-10T20:59:28Z

src/index.js

+ Relay: require('./circuit/relay'),
+ Dialer: require('./dialer'),
+ multicodec: require('./multicodec'),
+ tag: 'Circuit'


Where is this tag used?

Since circuit pretends to be a transport, its used by swarm/libp2p when loading all transports. It's essentially used as a transport key.

daviddias · 2017-05-12T21:00:54Z

src/index.js

+ Dialer: require('./dialer'),
+ multicodec: require('./multicodec'),
+ tag: 'Circuit'
+}


The listener never gets exported though //cc @dryajov

Thats the same pattern the other transports use, the listener then gets instantiated with createListener.

Could you show me where you are using that?

Yep, this is handled by the swarm using the standard transport init flow - https:/libp2p/js-libp2p-swarm/blob/master/src/transport.js#L82.

BTW, when I mean that circuit implements the transport interface I mean that it literally implements this interface - https:/libp2p/interface-transport

I also think that transport-interface is possibly a misnomer, perhaps we can rename it to something like connection handler/supplier, once you break away from the assumption that it's a transport any possible impedance mismatches start going away.

@diasdavid re failing tests for transport interface. Those are on my list to fix, but I believe that when I looked at them initially it required some adjustments to make it work with circuit. I'll take another look and come back with my findings, in any case it should work, because circuit is not doing anything different from the rest of the transport at the interface level.

re .getAddr right now the only thing that circuit does a little different from other transports is that during instantiation it will check if there is /p2p-circuit/<maddr> address provided and if there isn't it will add a default /p2p-circuit, the reason for doing this is to enable circuit dialing/listening by default without having to repeat /p2p-circuit in the swarm config over and over. I think this is OK to do, since /p2p-circuit is a perfectly valid circuit address that means that this peer is reachable over any circuit.

Also, one more thing about .getAddr - we do have a clear use case for it since circuit does listen on specific /p2p-circuit addresses. This IMO, satisfies the interface intent of returning the list of available addresses the transport is listening on.

I definitely share @diasdavid initial reaction to doing it this way (using interface-transport) . The weird feeling was the hardest thing for me to overcome and drove me to try different approaches but every time realizing that I've just reinvented the wheel again and other transports are already doing exactly the same thing, which is why I stuck with interface-transport.

The good thing is that the actual circuit logic is not tied to interface-transport in any way, and could even be exposed as a stand alone module, we could have libp2p-circuit and libp2p-circuit-transport, but not sure if doing that makes any sense, since there would be no way of consuming it in swarm/libp2p without also pulling in the transport part.

Ok, let's stick with it. I just have one request. This needs to be all documented! :D

dryajov · 2017-05-16T09:14:49Z

Do you mean documenting the decision behind using transport interface? If that's the case, what's the best place for that, not sure if the spec is appropriate since it's still a low level implementation detail.

daviddias · 2017-05-16T09:37:58Z

@dryajov It has to be clear that is how relay attaches in swarm, a diagram in the repo README and some comments in the code should do fine.

dryajov · 2017-05-16T14:54:28Z

Ah, makes sense. I'll add that.

dryajov · 2017-06-10T01:15:29Z

@diasdavid @lgierth @dignifiedquire I've added some initial documentation to the readme and a diagram. Please let me know what you think about it - I'm not sure if it communicates the intent correctly, so feedback is appreciated :)

dryajov · 2017-06-13T17:29:18Z

@diasdavid @lgierth @dignifiedquire what do you guys need from me to get this moving? Anything i'm missing? Anything that needs to be done before you continue with the review?

daviddias · 2017-06-19T00:42:25Z

As discussed in IRC. Let's finalize the spec first.

dryajov · 2017-06-19T02:43:20Z

@diasdavid @lgierth

I've added a comment here, proposing a timeboxed decision, libp2p/specs#18.

dryajov · 2017-08-03T19:59:23Z

@diasdavid I've removed onion dialing since we're not supporting multihop for this iteration, this also removed the dependency on modifying swarm, this has reduced the number of outstanding PRs drastically and should make it a lot easier to review and accept.

daviddias · 2017-08-09T22:50:52Z

Thank you for pushing the updates on this PR, @dryajov. I'll review it as soon as possible.

As a side request, please do not use libp2p/javascript-team to ask for reviews, you essentially notified everyone that has ever contributed to the js parts of the project to review a PR. We should only use that when we require everyone's attention.

dryajov · 2017-08-10T02:19:30Z

@daviddias upps didn't know that! Thanks, looking forward for the review!

dryajov · 2017-08-17T19:41:06Z

closed in favor of #14

dryajov requested a review from dignifiedquire April 2, 2017 01:33

dryajov self-assigned this Apr 2, 2017

dryajov removed the request for review from dignifiedquire April 2, 2017 02:07

daviddias mentioned this pull request Apr 2, 2017

Not seeing any connected peers when using js-ipfs in browser ipfs/js-ipfs#800

Closed

dryajov force-pushed the feat/v0.1.0 branch 2 times, most recently from 8ba4249 to d0a3d59 Compare April 8, 2017 17:51

initial commit

b2bd6c9

dryajov force-pushed the feat/v0.1.0 branch from 70ef4a6 to 5cde2aa Compare April 11, 2017 18:26

dryajov mentioned this pull request Apr 16, 2017

WIP - Awesome Relay Endeavour ipfs/js-ipfs#833

Closed

9 tasks

dryajov force-pushed the feat/v0.1.0 branch 4 times, most recently from 1548348 to 01f58e1 Compare April 18, 2017 04:39

dryajov requested a review from dignifiedquire April 18, 2017 18:11

dryajov force-pushed the feat/v0.1.0 branch from 920b25e to befc025 Compare April 21, 2017 23:47

daviddias commented May 10, 2017

View reviewed changes

dryajov added 8 commits May 10, 2017 17:20

refactor: renaming Relay to Hop

4f33101

chore: fixing license

f195e12

chore: removing custom eslint config

732ccda

refactor: extract listening functionality into Stop handler

2f977a9

fix: removing unused priority functionality

011e7be

fix: correct import of safe-buffer

a5af868

lint: remove unused import

1dcab27

refactor: use async/setImmediate

f199867

dryajov force-pushed the feat/v0.1.0 branch from 69ea48a to f199867 Compare May 11, 2017 00:27

fix: remove unused constant

7f51231

dryajov mentioned this pull request May 11, 2017

feat: add maxLength to allow controlling max buffer length during decode dignifiedquire/pull-length-prefixed#9

Merged

refactor: move active/passive check out of _readDstAddr

70fcc87

daviddias commented May 12, 2017

View reviewed changes

feat: initial readme write up

1d4a4ce

dryajov force-pushed the feat/v0.1.0 branch from 14d3df1 to 1d4a4ce Compare June 5, 2017 05:03

dryajov added 4 commits July 2, 2017 14:25

fix: output correct circuit addresses

11222e4

feat: initial reimplementation of /hop /stop as a protobufs

0110a71

feat: reworking with protobufs

bef3f9e

feat: moving SUCCESS response to HOP from STOP

b6ce189

daviddias added the in progress label Jul 27, 2017

feat: removing multihop (onion) dialing

be12489

dryajov force-pushed the feat/v0.1.0 branch from 1c7dd20 to be12489 Compare August 3, 2017 19:20

0.0.3

f71892e

dryajov requested a review from a team August 3, 2017 22:01

fix: go-js integration test changes

24bd2fd

dryajov removed the request for review from a team August 10, 2017 02:31

dryajov closed this Aug 16, 2017

dryajov removed the in progress label Aug 16, 2017

dryajov mentioned this pull request Sep 1, 2017

Awesome Endeavour: Circuit Relay ipfs/js-ipfs#830

Merged

9 tasks

feat/v0.1.0 #9

feat/v0.1.0 #9

Conversation

daviddias commented Mar 27, 2017 • edited by dryajov Loading

daviddias 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

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

dryajov May 10, 2017 • edited Loading

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

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

dryajov May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov May 10, 2017 • edited Loading

Choose a reason for hiding this comment

daviddias May 12, 2017 • edited Loading

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

dryajov May 15, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov May 15, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov May 15, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov May 15, 2017 • edited Loading

Choose a reason for hiding this comment

dryajov May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dryajov commented May 16, 2017

daviddias commented May 16, 2017

dryajov commented May 16, 2017

dryajov commented Jun 10, 2017

dryajov commented Jun 13, 2017

daviddias commented Jun 19, 2017

dryajov commented Jun 19, 2017 • edited Loading

dryajov commented Aug 3, 2017

daviddias commented Aug 9, 2017

dryajov commented Aug 10, 2017

dryajov commented Aug 17, 2017

daviddias commented Mar 27, 2017 •

edited by dryajov

Loading

dryajov May 10, 2017 •

edited

Loading

dryajov May 10, 2017 •

edited

Loading

dryajov May 10, 2017 •

edited

Loading

daviddias May 12, 2017 •

edited

Loading

dryajov May 15, 2017 •

edited

Loading

dryajov May 15, 2017 •

edited

Loading

dryajov May 15, 2017 •

edited

Loading

dryajov May 15, 2017 •

edited

Loading

dryajov May 15, 2017 •

edited

Loading

dryajov commented Jun 19, 2017 •

edited

Loading