Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Absorb go-libp2p abstractions and core types into this module #1

Merged
merged 31 commits into from
May 22, 2019

Conversation

raulk
Copy link
Member

@raulk raulk commented Apr 16, 2019

This PR is part of a batch across go-libp2p repos to consolidate abstractions, interfaces and core types into this repo.

Work is being performed in the consolidate-skeleton branch across repos.


Master plan:


Merge strategy: squash and rebase.

Copy link

@lanzafame lanzafame left a comment

Choose a reason for hiding this comment

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

Other then the specific things mentioned, I would like to see each package have a package level comment that explains its purpose and also that every exported interface has a comment, I don't care so much for the types and functions at this point.

Thanks for bringing this all together @raulk, this is awesome 👍.

discovery/discovery.go Outdated Show resolved Hide resolved
host/host.go Show resolved Hide resolved
metrics/interface.go Outdated Show resolved Hide resolved
network/network.go Show resolved Hide resolved
// Global noop notifiee. Do not change.
var GlobalNoopNotifiee = &NoopNotifiee{}

type NoopNotifiee struct{}

Choose a reason for hiding this comment

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

Should this be named NullNotifiee to be consistent with the NullConnMgr and also in a separate file, again for consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of that, but there is a nuisance, and it's totally subjective.

IMO Noop makes a strong concrete assertion that this will literally do nothing when called. Null is less strict: it conveys that functionally speaking this is equivalent to having no connection manager. Underneath it may not noop, or it may actually do something, but its behaviour as externally observed is akin to a null object.

Granted, this is one man's opinion. And the benefit in DX of unifying naming may trump pragmatism. No strong opinions, but if we are to unify, I'd pick Noop.

Choose a reason for hiding this comment

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

Yeah, I am fine with Noop, just want to take this chance to unify on something.

protocol/switch.go Show resolved Hide resolved
routing/notifications/query.go Outdated Show resolved Hide resolved
routing/routing.go Outdated Show resolved Hide resolved
routing/routing.go Outdated Show resolved Hide resolved
transport/transport.go Outdated Show resolved Hide resolved
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Can we call the network directory/package just net?

peer/info.go Outdated

// PeerInfo is a small struct used to pass around a peer with
// a set of addresses (and later, keys?).
type Info struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't rename the type, it should still be called PeerInfo.

Choose a reason for hiding this comment

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

Except it now stutters with the package name or always did

Copy link
Member Author

@raulk raulk Apr 17, 2019

Choose a reason for hiding this comment

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

peer.PeerInfo is not idiomatic.

This type used to live in the peerstore, i.e. pstore.PeerInfo. That's why the Peer qualifier was required. This type is now in the place where it belongs, and the qualifier becomes redundant.

However, peer.Info is vague and unhelpful. Possible alternatives:

  • peer.DialInfo
  • peer.AddrInfo
  • peer.AddressSlip

Copy link
Contributor

Choose a reason for hiding this comment

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

peer.AddrInfo seems the most pertinent; it is addr information about a peer.

@raulk
Copy link
Member Author

raulk commented Apr 17, 2019

@vyzo

Can we call the network directory/package just net?

I had initially called it net but it overshadows stdlib net, and it gets disconcerting.

* flatten out slim packages in routing.
* simplify naming in discovery.
* break up big monolithic files into constituents (network, pnet).
* renames for consistency.
transport/test/utils.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

We should also add something to the root of this repo so godoc picks it up.

@@ -0,0 +1,56 @@
package helpers
Copy link
Member

Choose a reason for hiding this comment

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

OT: these should really go away. We need to fix the stream interfaces and:

  1. Make Close() close.
  2. Add CloseWrite(), CloseRead().

Copy link
Member Author

Choose a reason for hiding this comment

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

Since both funcs take a stream as their first arg, I wonder if we can preserve backwards compatibility by hoisting the instance method to a package var. Will try it out, otherwise I suggest punting it to iteration 3 (backwards-incompatible ergonomics changes).

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might call the FullyClose as Shutdown.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll open an issue for this.

metrics/meter.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member Author

raulk commented Apr 19, 2019

@Warchant @kamilsa – this is the PR I'd like you to poke about. Disregard the godocs for now (they'll improve dramatically in iteration 2, see description). But it'd be super helpful to have fresh pairs of eyes go through the type tree and see if it's easily intelligible and intuitive, or if there's room to rename entities or shuffle things around for better clarity.

mux/mux.go Show resolved Hide resolved
mux/mux.go Show resolved Hide resolved
mux/test/muxer.go Outdated Show resolved Hide resolved
network/conn.go Show resolved Hide resolved
sec/test/ttest.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member Author

raulk commented Apr 22, 2019

How would we feel about moving all conformance tests and test helpers to a go-libp2p-testing module?

@raulk
Copy link
Member Author

raulk commented May 16, 2019

@yusefnapora thanks for owning the godocs review and update; great job! I've merged it onto this branch, so the repo will be inited with iteration 1 and iteration 2 completed. This is awesome news!

vyzo
vyzo previously requested changes May 16, 2019
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

This is missing recent additions to the ConnManager interface and openssl support for crypto.

Also note the ProtocolBook suggestion for Peerstore.

connmgr/connmgr.go Show resolved Hide resolved
crypto/rsa.go Outdated Show resolved Hide resolved
helpers/stream.go Show resolved Hide resolved
peerstore/peerstore.go Outdated Show resolved Hide resolved
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

As @vyzo said, we need to audit all the related libp2p commits since we started this process to pull in any new changes. Other than that, LGTM.

helpers/stream.go Show resolved Hide resolved
@vyzo
Copy link
Contributor

vyzo commented May 18, 2019

the peerstore interface has changed too -- there is a split of the protocol methods in the ProtoBook interface.

@raulk raulk dismissed vyzo’s stale review May 22, 2019 17:13

peerstore interface changes synced

@raulk
Copy link
Member Author

raulk commented May 22, 2019

I'm not going to address the global var situation as there is no clean way of solving it. We can choose to drop the vars in the donor packages, but that'll break consumers. It's probably worth it, though, much better than a silent mess.

@raulk raulk requested a review from vyzo May 22, 2019 17:14
@vyzo
Copy link
Contributor

vyzo commented May 22, 2019

Yeah, let's drop them from the source packages, better break the build than a bug.

@raulk raulk mentioned this pull request May 22, 2019
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

LGTM. Just an observation about the size element going unused in peer sets.

peer/set.go Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants