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

raulk · 2019-04-16T19:11:11Z

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:

Iteration 1: focused on copying over the abstractions with verbatim godoc and making no breaking changes.
Iteration 2: revising godoc to make it top-class // proposed owner: @yusefnapora with @raulk collaborating.
Iteration 3: minor, aesthetic backwards-incompatible changes, e.g. Ttl => TTL. Manifest (keep adding):

Merge strategy: squash and rebase.

connmgr/null.go

lanzafame

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

host/host.go

metrics/interface.go

network/network.go

lanzafame · 2019-04-17T01:41:37Z

network/notifiees.go

+// Global noop notifiee. Do not change.
+var GlobalNoopNotifiee = &NoopNotifiee{}
+
+type NoopNotifiee struct{}


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

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.

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

protocol/switch.go

routing/notifications/query.go

routing/routing.go

transport/transport.go

vyzo

Can we call the network directory/package just net?

vyzo · 2019-04-17T07:19:12Z

peer/info.go

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


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

Except it now stutters with the package name or always did

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

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

Merging libp2p/go-libp2p-peerstore#67.

raulk · 2019-04-17T11:35:09Z

@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

Stebalien

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

Stebalien · 2019-04-18T05:12:58Z

helpers/stream.go

@@ -0,0 +1,56 @@
+package helpers


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

Make Close() close.

Add CloseWrite(), CloseRead().

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).

We might call the FullyClose as Shutdown.

I'll open an issue for this.

metrics/meter.go

raulk · 2019-04-19T13:31:23Z

@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

mux/test/muxer.go

network/conn.go

sec/test/ttest.go

raulk · 2019-04-22T10:48:58Z

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

raulk · 2019-05-16T11:38:36Z

@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

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

Also note the ProtocolBook suggestion for Peerstore.

connmgr/connmgr.go

crypto/rsa.go

helpers/stream.go

peerstore/peerstore.go

Stebalien

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

vyzo · 2019-05-18T16:28:18Z

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

add Close to ConnManager interface

protocol/switch.go

peerstore interface changes synced

raulk · 2019-05-22T17:13:53Z

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.

vyzo · 2019-05-22T17:18:06Z

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

vyzo

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

peer/set.go

raulk added 4 commits April 16, 2019 14:00

genesis of go-libp2p-core.

1e60799

reorg null connmgr and OOTB network notifiees.

12d7eef

add routing interfaces.

54a1b70

downgrade to go 1.11

557fb9e

raulk mentioned this pull request Apr 16, 2019

Consolidate libp2p abstractions and core types libp2p/go-libp2p#602

Closed

7 tasks

raulk added 2 commits April 16, 2019 22:22

remove go 1.11 directive from go.mod.

bdf6ef0

mv protocol/protocol.go => protocol/switch.go

95ab6c0

lanzafame reviewed Apr 17, 2019

View reviewed changes

connmgr/null.go Show resolved Hide resolved

lanzafame previously requested changes Apr 17, 2019

View reviewed changes

vyzo reviewed Apr 17, 2019

View reviewed changes

update peerstore.TempAddrTTL to 2 min.

5163265

Merging libp2p/go-libp2p-peerstore#67.

raulk added 6 commits April 17, 2019 12:43

address minor review comments.

40efb03

absorb transport tests.

c9a30b9

reorg network context options.

2e11bca

absorb discovery opts.

4e1f238

reorg code; see below.

47396eb

* flatten out slim packages in routing. * simplify naming in discovery. * break up big monolithic files into constituents (network, pnet). * renames for consistency.

add mux test suite.

f41b5dd

raulk force-pushed the consolidate-skeleton branch from 74fe43b to f41b5dd Compare April 17, 2019 16:21

lanzafame mentioned this pull request Apr 18, 2019

[docs] list of doc improvements #2

Open

lanzafame reviewed Apr 18, 2019

View reviewed changes

transport/test/utils.go Outdated Show resolved Hide resolved

Stebalien reviewed Apr 18, 2019

View reviewed changes

Warchant reviewed Apr 20, 2019

View reviewed changes

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 added 2 commits April 22, 2019 11:24

sec/test/ttest.go -> sec/test/transport.go.

978f710

add top-level aliases for central primitives of go-libp2p.

ec2a527

absorb go-libp2p-crypto.

3a54eb6

raulk force-pushed the consolidate-skeleton branch from 3e8fb28 to 3a54eb6 Compare April 22, 2019 12:59

rename peer.Info to peer.AddrInfo.

c30d4b4

raulk requested review from vyzo, Stebalien, yusefnapora and lanzafame May 16, 2019 11:38

vyzo previously requested changes May 16, 2019

View reviewed changes

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

Stebalien approved these changes May 16, 2019

View reviewed changes

helpers/stream.go Show resolved Hide resolved

raulk added 3 commits May 18, 2019 18:16

sync w/ go-libp2p-crypto master: rsa openssl.

09ae22d

connmgr sync with master: UpsertTag().

9a572d1

go fmt.

9b47104

raulk and others added 3 commits May 18, 2019 19:38

remove WIP notice from README.md.

d017d06

add Close to interface, implement in nullconnmgr

a69990c

Merge pull request #6 from libp2p/update-connmgr-interface

3953b18

add Close to ConnManager interface

Warchant reviewed May 20, 2019

View reviewed changes

protocol/switch.go Show resolved Hide resolved

Steven Allen and others added 2 commits May 22, 2019 17:49

metrics: allow getting metrics by peer/protocol (#5)

0b2b19c

sync peerstore interface changes.

f5a63f2

raulk requested a review from vyzo May 22, 2019 17:14

raulk mentioned this pull request May 22, 2019

Review the helpers #7

Closed

vyzo approved these changes May 22, 2019

View reviewed changes

peer/set.go Show resolved Hide resolved

raulk merged commit 6e566d1 into master May 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raulk commented Apr 16, 2019 •

edited

Loading

lanzafame left a comment

lanzafame Apr 17, 2019

raulk Apr 17, 2019

lanzafame Apr 18, 2019

vyzo left a comment

vyzo Apr 17, 2019

lanzafame Apr 17, 2019

raulk Apr 17, 2019 •

edited

Loading

vyzo Apr 17, 2019

raulk commented Apr 17, 2019

Stebalien left a comment

Stebalien Apr 18, 2019

raulk Apr 18, 2019

Stebalien Apr 18, 2019

vyzo Apr 19, 2019

raulk May 22, 2019

raulk commented Apr 19, 2019 •

edited

Loading

raulk commented Apr 22, 2019 •

edited

Loading

raulk commented May 16, 2019

vyzo left a comment

Stebalien left a comment

vyzo commented May 18, 2019

raulk commented May 22, 2019

vyzo commented May 22, 2019

vyzo left a comment

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

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

Conversation

raulk commented Apr 16, 2019 • edited Loading

lanzafame 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

vyzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk Apr 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk commented Apr 17, 2019

Stebalien 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

raulk commented Apr 19, 2019 • edited Loading

raulk commented Apr 22, 2019 • edited Loading

raulk commented May 16, 2019

vyzo left a comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

vyzo commented May 18, 2019

raulk commented May 22, 2019

vyzo commented May 22, 2019

vyzo left a comment

Choose a reason for hiding this comment

raulk commented Apr 16, 2019 •

edited

Loading

raulk Apr 17, 2019 •

edited

Loading

raulk commented Apr 19, 2019 •

edited

Loading

raulk commented Apr 22, 2019 •

edited

Loading