Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

feat: Utilize interface-data-exchange to build a completly new webrtc-star #148

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mkg20001
Copy link
Member

@ghost ghost assigned mkg20001 Jun 15, 2018
@ghost ghost added the status/in-progress In progress label Jun 15, 2018
@mkg20001 mkg20001 force-pushed the feat/use-interface-data-exchange branch from 775c2a3 to ec5e614 Compare June 15, 2018 11:51
@mkg20001
Copy link
Member Author

mkg20001 commented Jun 17, 2018

Blocked:

Todos:

  • Better integration of interface-data-exchange into libp2p
  • Update docs

Note: Tests on CI are failing as the current setup requires some npm-linking until all prs are merged and deps get updated.

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

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

interface-data-exchange is not a libp2p interface and I'm not seeing the proposal/reasoning for it to exist. What's the benefits here?

@mkg20001
Copy link
Member Author

mkg20001 commented Aug 27, 2018

@diasdavid Currently webrtc-star utilizes a centralized socket.io server as an exchange mechanism for exchanging the webrtc signalling data.
This interface generallizes this behaviour into an interface and introduces two modules as well as removing the need for a specific centralized server as any node can now act as an exchange.

  • Rendezvous exchange: Utilize a server/common node with specfic protocol enabled to exchange data between two not yet connected nodes (a good idea would be to make some/all non-browser nodes rendezvous exchanges, with certain limitations applied so it can't be abused easily)
  • Direct exchange: If a node is connected to another node over p2p-circuit it makes more sense to directly send the data over this connection. Direct exchange exchanges the data directly.

For something to be a valid implementation it must:

  • Expose the documented api (might need some more docs)
  • Ensure that only the parties that want to exchange data can decrypt it and can be assured that data really came from a specific origin id (basically: prevent mitms using signatures, encrypt data using secio - where applicable - or peer-ids)

There are some slides (which are also linked above). Specifically this part (and the slides that follow it) should elaborate a bit more specifically how this works, previous slides should give an idea about why this is necesarry.

@mkg20001
Copy link
Member Author

@diasdavid Any follow-up feedback?

@cryptoquick
Copy link

This is very cool stuff! I wish @mkg20001 's contributions could get some more follow-up, and I think efforts like this are incredibly valuable towards moving WebRTC transports and peer exchange along for IPFS, which is becoming a critical requirement for more and more developers.

@mkg20001
Copy link
Member Author

Can I get some feedback? I mean it's been almost year. Considering the effort I invested into this I think it's only fair if this gets at least looked at and reviewed.

@jacobheun
Copy link
Contributor

@mkg20001 sorry this fell off the radar. @vasco-santos and I will coordinate to make sure we get you feedback for this this week.

@jacobheun
Copy link
Contributor

@mkg20001 I did a run through of the modules and added some feedback/questions below.

Interface Data Exchange

  • More documentation around what the actual interface methods should handle would be helpful. https://github.com/mkg20001/interface-data-exchange/tree/v0.0.2#api. While the slides help with understanding the overall goal, it's not immediately clear what the interface methods are for and what new implementations should ensure/not do with them.
  • The interface should really be standardizing things like
    offer = JSON.parse(String(request))
    . I'd expect the interface to return the objects we need instead of needing to convert from a string every time. Is there a valid reason to have stringified objects come back from the interface?
  • What's the value in this being an interface as opposed to existing inside of webrtc-star? Are there other scenarios / transports that you see benefiting from leveraging the interface?

General

  • More comments and documentation in general across the code would help us in reducing review times. It can take a significant amount of time to go through the code to get a clearer picture so we can provide meaningful, timely feedback.
  • Examples or a demo of this in action would be great to see.
  • I think dropping socket.io and the need for a single signaling server is a great improvement.

Questions

  • Right now it looks like webrtc-star will use a single exchange, direct or rendezvous. Is there a reason to not use both? If not, would that require adding two configurations of webrtc-star to libp2p? What are the libp2p environments that each would be best suited for if they're to be used individually?

@mkg20001
Copy link
Member Author

mkg20001 commented Apr 5, 2019

. I'd expect the interface to return the objects we need instead of needing to convert from a string every time. Is there a valid reason to have stringified objects come back from the interface?

It uses the most generic thing, bytes, to support as many protocols as possible.
It's just getting stringified because this particular transport uses JSON, but it could use protobuf or whatever instead.

While the slides help with understanding the overall goal, it's not immediately clear what the interface methods are for and what new implementations should ensure/not do with them.

Will add more docs. For now that is:

  • ensure data is encrypted at all times
  • allow a single request/response cycle with up to 4kb of data (such as signalling data for webrtc star)

What's the value in this being an interface as opposed to existing inside of webrtc-star? Are there other scenarios / transports that you see benefiting from leveraging the interface?

We aren't stuck with a single transport such as socket.io, but instead can use a variety of methods such as direct exchange, exchange via neighbours (rendezvous) or really anything (ex: Physical beacons using ham radio, for example to do webrtc or to agree on a frequency range to use for further communications via one fixed frequency)

More comments and documentation in general across the code would help us in reducing review times. It can take a significant amount of time to go through the code to get a clearer picture so we can provide meaningful, timely feedback.

I didn't invest much effort into this in the beginning because I wasn't sure of it's inclusion into the libp2p project as a standardized interface.

Examples or a demo of this in action would be great to see.

Will add this onto my pile of stuff for this month, maybe at the end of it something should be ready. But not so sure if I get the time to demo it at the all-hands (I remember doing a demo where I just let the tests run)

I think dropping socket.io and the need for a single signaling server is a great improvement.

:thumbs_up:

Right now it looks like webrtc-star will use a single exchange, direct or rendezvous. Is there a reason to not use both? If not, would that require adding two configurations of webrtc-star to libp2p? What are the libp2p environments that each would be best suited for if they're to be used individually?

https://docs.google.com/presentation/d/1yfxI_4wY-5ydFxcIr2NBsg8E0wyJJStekRfectZjcf0/edit#slide=id.g3bff07d16e_0_0

WIP ™️

@jacobheun
Copy link
Contributor

It uses the most generic thing, bytes, to support as many protocols as possible.
It's just getting stringified because this particular transport uses JSON, but it could use protobuf or whatever instead.

I think protobuf would be good as we're already leveraging it in a lot of places. I also think it helps remove some of the potential ambiguity of what's being passed.

Will add this onto my pile of stuff for this month, maybe at the end of it something should be ready. But not so sure if I get the time to demo it at the all-hands (I remember doing a demo where I just let the tests run)

A repo of it working I could pull down and run would be fine too, assuming that's easier than the demo. (Maybe not depending on what needs to be linked for the patches you've previously linked to).

https://docs.google.com/presentation/d/1yfxI_4wY-5ydFxcIr2NBsg8E0wyJJStekRfectZjcf0/edit#slide=id.g3bff07d16e_0_0

👍 on that approach.

Overall I like the approach and think it's valuable for a more reliable network, as well as for providing a mechanism of transport "upgrading". I appreciate the work you put into it. Feel free to ping @vasco-santos or I if something isn't getting looked at and we'll do our best to get back to you quickly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants