p2p: make NodeID and NetAddress public

[tychoish]

This is a big PR, but I think it has to be. It's split up into two
commits whith the substantive changes in the first commit and more
rote changes in the second that just update references.

This change moves a couple of types out of the p2p layer and into
the public types directory. I don't want this to be a referenda on
types, which I think is a problem we should address later, but
before 1.0.

[cmwaters]

And the main reason for moving NodeID to the types folder is because we need it exposed because it is a response in the RPC?

[alexanderbez]

I know we don't want to have a lengthy discussion on this, but can we please just move the necessary types to pkg/p2p please? pkg being our external package used by external processes and clients. There is nothing stopping us from doing this now. We shouldn't be moving stuff into types.

ref: https://github.com/golang-standards/project-layout#pkg

[tychoish]

And the main reason for moving NodeID to the types folder is because we need it exposed because it is a response in the RPC?

Yes, and also because some of these functions are useful for people attempting to construct config objects. This request grew out of my conversation with @ValarDragon and seemed quite reasonable to me.

pkg being our external package used by external processes and clients. There is nothing stopping us from doing this now. We shouldn't be moving stuff into types.

I don't really think this is the right form to have a conversation about this, but:

• pkg doesn't exist now, and I don't think this is the right place to be creating new paradigms.
• I think it creates a more fragmented experience to have public types spread throughout the legacy types location and new locations, and think that we should avoid having a release where public types end up both in types and in the pkg directory.

I think it makes sense to break types into pkg packages at some point, and if we have plans to do that before 0.35, that'd be cool, but I think we should handle these changes distinctly. If we're going to break apart types, we should do that, and then apply this change. If not, we should just merge this, and break apart types in 0.36.

[alexanderbez]

I do not think we should proceed with this PR by just moving these the types package -- I'm strongly opposed to this.

[tychoish]

Let me think about this a bit more and see if there's a better path forward that preserves the general release timeline without sacrificing the coherence of the API.

[tac0turtle]

This was one of my fears with not thinking the internal work through thoroughly. Types is already a mess, we have had this discussion countless times, adding more things into and passing the work on to a later date is something I'm not a fan of. pkg/p2p is a step in the right direction and I would wish to avoid breaking the API twice and aim for once, not already plan for two breaking changes.

Also CHANGELOG!!

[codecov]

Codecov Report

Merging #6583 (8323765) into master (007eeb9) will decrease coverage by 4.82%.
The diff coverage is 17.31%.

@@            Coverage Diff             @@
##           master    #6583      +/-   ##
==========================================
- Coverage   65.99%   61.16%   -4.83%     
==========================================
  Files         234      297      +63     
  Lines       20269    27974    +7705     
==========================================
+ Hits        13376    17111    +3735     
- Misses       5825     9141    +3316     
- Partials     1068     1722     +654     

Impacted Files	Coverage Δ
abci/client/grpc_client.go	0.00% <0.00%> (ø)
abci/client/local_client.go	0.00% <0.00%> (ø)
abci/types/application.go	0.00% <0.00%> (ø)
abci/types/pubkey.go	0.00% <0.00%> (ø)
abci/types/result.go	23.07% <0.00%> (ø)
abci/types/util.go	0.00% <0.00%> (ø)
cmd/tendermint/commands/gen_node_key.go	0.00% <ø> (ø)
cmd/tendermint/commands/gen_validator.go	18.18% <ø> (+18.18%)	⬆️
cmd/tendermint/commands/init.go	3.27% <ø> (+3.27%)	⬆️
cmd/tendermint/commands/light.go	24.27% <ø> (ø)
... and 429 more

[tychoish]

This was one of my fears with not thinking the internal work through thoroughly. Types is already a mess, we have had this discussion countless times, adding more things into and passing the work on to a later date is something I'm not a fan of. pkg/p2p is a step in the right direction and I would wish to avoid breaking the API twice and aim for once, not already plan for two breaking changes.

Just to be fair, I think I did think about this pretty thoroughly, and while I mentioned this implication (e.g. putting some things in types for a while) I think you're right that we/I didn't call the implication of this out very clearly, and I should have done that.

I think the open question is not "should we keep types but rather "when do we get rid of it". I think there are a few assumptions, and practicalities on my mind:

• The Go API is definitely going to break between 0.35 and 0.36.
• There's going to be a hard upgrade (e.g. protocol breaking) change before 1.0 to accommodate proposer-based timestamps, which will require more intervention than a simple API break
• Reorganizing code (e.g. moving types to pkg packages) produces very clear (and easy to resolve) compiler errors.
• We shouldn't do a release that has some things in pkg and some things in types because it significantly hurts discovery and usability, and leaves the repo in an "aspirational state"

I'm also aware that ADR-60 doesn't include anything about breaking up types, which might be an oversight. I think the core issue/questions here are:

• When should we break apart types relative to other milestones in the release schedule?
• Should the scope of related work (e.g. ADR-60 or the proposed ADR-69) expand to cover the types directory?

Regardless, I think PRs like this are extremely the wrong place to have conversations about this level of scoping and scheduling.

[alexanderbez]

We shouldn't do a release that has some things in pkg and some things in types because it significantly hurts discovery and usability, and leaves the repo in an "aspirational state"

Why? We can move things into pkg piecemeal until we hit 1.0, in which types should be completely removed and pkg is stabilized.

[tac0turtle]

approving to unblock upgrade to sdk. Still think we should move to pkg directory

#6583