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

Introduce PeerId interface #955

Closed
Gozala opened this issue Jun 24, 2021 · 10 comments
Closed

Introduce PeerId interface #955

Gozala opened this issue Jun 24, 2021 · 10 comments

Comments

@Gozala
Copy link
Contributor

Gozala commented Jun 24, 2021

Idea here is to introduce PeerId as an interface so that all APIs can produce / consume as opposed to specific implementation in form of https://github.com/libp2p/js-peer-id

@nazarhussain
Copy link
Contributor

nazarhussain commented Jun 25, 2021

@vasco-santos Can you explain the issue a bit further. Does interface for PeerId requires to have just a type definition like interface-record or with implementation like interface-connection ?

@Gozala
Copy link
Contributor Author

Gozala commented Jun 25, 2021

@nazarhussain I'm still working out all the details but here is my current thinking about this subject:

  1. We will have PeerId interface with a most minimal interface needed defined as a TS interface, something along the lines of my current draft
     export interface PeerId {
       /**
        * Binary representation of the peer id.
        */
       readonly id: Uint8Array
    
       readonly privKey: PrivateKey | null
       readonly pubKey: PublicKey
    
       marshal: (excludePriv?: boolean) => Uint8Array
       toPrint: () => string
       toJSON: () => JSONPeerId
    
       /**
        * Return raw id bytes.
        */
       toBytes: () => Uint8Array
    
       toString: () => string
       toB58String: () => string
    
       equals: (other: PeerId) => boolean
     }
    I would like to refine it further, but it's easier to do as string all the deps together where TS can identify all the call sites and methods that are used.
  2. Update libp2p / ipfs and related libs so that argument / return type PeerId refers to the interface everywhere it currently refers to a PeerId class instance.
  3. Turn peer-id lib into a one possible implementation of PeerId interface. Updating all the factory function return types to PeerId interface as opposed to class instance.

In my current working draft this already reduced significant number of dependencies, limiting it only to cases where:

  1. String or binary encoded PeerId is parsed/decoded to value.
  2. Where do runtime type checks with PeerId.isPeerId

We can eliminate 2nd case we are get type checks at compile time with TS. It does imply that some JS users may not get good errors if they pass garbage in, but I think that is reasonable for a low level library especially since users will get intellisence through vscode (and other LSP compatible editors).

@Gozala Gozala transferred this issue from libp2p/js-libp2p-interfaces Jun 25, 2021
@Gozala
Copy link
Contributor Author

Gozala commented Jun 25, 2021

I have initially started by adding interface defs to libp2p-interface repo, but I'm starting to recognize several problems in comparison to approach taken by https://github.com/ipfs/js-ipfs/tree/master/packages/ipfs-core-types:

  1. libp2p-interface comes with large number of dependencies which does not help with reducing number of dependencies we'd like to get out of this.
  2. We really do not need to bundle any JS code with interfaces as it will (unintentionally) tie things to a specific implementation.

I think main problem here is that libp2p-interface serves double duty:

  • 💚 Provides tests to verify implementation compatibility
  • 💔 Provides common building blocks that can be reused across concrete implementations.

I'd like to propose following:

  1. Remove all (non dev) dependencies from libp2p-interface repo.
  2. Change things up so that we provides interface defs and tests.
    • That way concrete implementations can use @implements directive to ensure compatibility through TS.
    • Use tests to verify that implementation upholds invariants that can be expressed at type level.
  3. Migrate common building blocks into different repo / package e.g. lib2p-primitives.

This would accomplish following:

  1. All the consumers of core interfaces (that code that takes implementations of some interface as opposed to create them) can depend on dependency free libp2p-interfaces package containing only interface defs. So get type checks at compile time with no overhead at runtime.
  2. All the producers of core interfaces can will import concrete library providing implementation e.g. peer-id. These should be rare just places that take user input or data over wire. They don't need to depend on libp2p-interfaces because factory functions will be typed to return interface.
  3. Implementations of the core interfaces will import libp2p-interfaces and ensure type level compatibility via @implements directive. Where it makes sense they will also import libp2p-primitives to make use of common building blocks.

@Gozala
Copy link
Contributor Author

Gozala commented Jun 26, 2021

Is anything actually uses Connection class ? I can't seem to find a single place where it's utilized

@vasco-santos
Copy link
Member

@Gozala I am glad you raise these points. When we added connection and topology into the interfaces repo, it felt odd to me, as they are Types/Primitives, same for PeerId. We did this in the time that we were going to the direction of less repos is better, so let's not create a new one for now.

I think we should distinct between the interfaces that target libp2p modules, like transport, encryption, peerDiscovery, ... and the primitives as you suggest. I have been working on making interfaces a monorepo libp2p/js-libp2p-interfaces#97 separating the compliance tests from the interfaces. I think we can probably reuse the same repo and add a package for the primitives.

There is a bunch of work happening in parallel in this context, which is also not a priority, so we need to coordinate well on how we can advance in this direction. I would suggest just adding the PeerId interface in the interfaces, then get the monorepo and then create the primitives repo.

Is anything actually uses Connection class ? I can't seem to find a single place where it's utilized

Yes, Connection class is used in the Upgrader https://github.com/libp2p/js-libp2p/blob/302bb9005891aa06b70a5f354bfac6b2d5a3c3b8/src/upgrader.js

A raw connection from a transport will be transformed into a libp2p Connection in the upgrade process

@Gozala
Copy link
Contributor Author

Gozala commented Jun 28, 2021

Yes, Connection class is used in the Upgrader https://github.com/libp2p/js-libp2p/blob/302bb9005891aa06b70a5f354bfac6b2d5a3c3b8/src/upgrader.js

If that is only place maybe implementation should move to js-lipbp2 instead and we can just keep interface and tests in the libp2p-interfaces ?

@lidel
Copy link
Member

lidel commented Jul 2, 2021

Triage notes:

  • avoid using js-libp2p-interfaces
    • risk: introducing dependency that gets pulled automatically by everything that uses it
  • alternative approach:
    • make peerid an interface
    • make ipld depend on interface and pull implementation

@Gozala
Copy link
Contributor Author

Gozala commented Jul 2, 2021

avoid using js-libp2p-interfaces
risk: introducing dependency that gets pulled automatically by everything that uses it

@lidel I’m not sure I understand what is the problem though. Sure many libs will depend on libp2p-interfaces, but if there’s no runtime code pulled what’s the harm ?

Most libs already already depend e.g on aegir but that doesn’t seem to be a problem.

@Gozala
Copy link
Contributor Author

Gozala commented Jul 2, 2021

make ipld depend on interface and pull implementation

did you mean js-libp2p instead of ipld here ?

achingbrain added a commit to libp2p/js-libp2p-interfaces that referenced this issue Nov 3, 2021
Creates `PeerId` interface and its compliance tests. The discussion was initiated from libp2p/js-libp2p#955 and libp2p/js-peer-id#150 (review)

Co-authored-by: achingbrain <alex@achingbrain.net>
@BigLep
Copy link
Contributor

BigLep commented Nov 12, 2021

Closing because libp2p/js-libp2p-interfaces#107 was merged.

@BigLep BigLep closed this as completed Nov 12, 2021
@tinytb tinytb moved this to Done in js-libp2p Oct 11, 2022
@tinytb tinytb added this to js-libp2p Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants