libp2p / js-peer-info

libp2p Peer abstraction Node.js implementation
https://libp2p.io
MIT License
37 stars 28 forks source link

considering merging peer-info and peer-id into `peer` #55

Closed daviddias closed 6 years ago

daviddias commented 7 years ago

Despite being an adept of modularity, I also sometimes feel that having modules coalesced together could save a lot of dev time and maintenance, especially when two modules are used together almost every single time. I believe this to be the case with peer-info and peer-id.

We also have across the board this pattern:

  waterfall([
    (cb) => PeerId.create({ bits: 1024 }, cb),
    (peerId, cb) => PeerInfo.create(peerId, cb),
    (peerInfo, cb) => {
      multiaddrs.map((ma) => peerInfo.multiaddrs.add(ma))
      cb(null, peerInfo)
    },
    (peerInfo, cb) => {
      const node = new Node(peerInfo, undefined, options)
      cb(null, node)
    }
  ], callback)

Which could be simplified to:

Peer.create({ bits: 1024, multiaddrs: [ma]}, (err, peer) => {})
dignifiedquire commented 6 years ago

Not convinced this is the right way to go, I would rather see a better definition of peerinfo and peerid that is language agnostic, such that go and js can be more aligned, as well as the peerstore and all the new things coming with that from the rust-libp2p work.

dryajov commented 6 years ago

What's the benefit of having them aligned? Do we ever use the objects across implementations (i.e. serialize/deserialize to send over the wire), if not, then it's probably overkill forcing them to conform across languages.

mkg20001 commented 6 years ago

I don't like the idea of merging both as the Id as a standalone module is also good for doing crypto stuff that does not necessarily require a peer-info. Instead, the Peer.create command could be added to peer-info while still having the peer-id as a separate module.

daviddias commented 6 years ago

Let's drop this proposal then.