Create a new type for peer id #2159
Conversation
| @@ -45,21 +45,21 @@ func (bwc *BandwidthCounter) LogRecvMessage(size int64) { | |||
| // Bandwidth is associated with the given protocol.ID and peer.ID. | |||
| func (bwc *BandwidthCounter) LogSentMessageStream(size int64, proto protocol.ID, p peer.ID) { | |||
| bwc.protocolOut.Get(string(proto)).Mark(uint64(size)) | |||
| bwc.peerOut.Get(string(p)).Mark(uint64(size)) | |||
| bwc.peerOut.Get(string(p.MustMarshalBinary())).Mark(uint64(size)) | |||
There was a problem hiding this comment.
The old code used the bytes. This PR should not change anything semantically. If we used p.String() we would be changing semantics.
There was a problem hiding this comment.
Agreed. Let’s keep this change minimal. We can do a follow up PR if we want to change this interface.
| @@ -382,7 +382,7 @@ func (as *AmbientAutoNAT) probe(pi *peer.AddrInfo) { | |||
| func (as *AmbientAutoNAT) getPeerToProbe() peer.ID { | |||
| peers := as.host.Network().Peers() | |||
| if len(peers) == 0 { | |||
| return "" | |||
| return peer.EmptyID | |||
There was a problem hiding this comment.
Probably more idiomatic:
| return peer.EmptyID | |
| return peer.ID{} |
|
Oh boy, this is going to wreak havoc in many places... I am not convinced at all that is a good idea. |
I suggest trying to bubble it up into Kubo before merging this PR. Then we'll see how bad it is. Design-wise, it's definitely the right thing to do. |
I think on principle it's a good idea. But practically, I'm not sure if the benefit is worth the effort. It'll be a breaking change that will affect most downstream consumers without much benefit to them. If we were designing this from scratch today I would say yes, it's worth it. At this point however, I'm not sure the benefits outweigh the cost. |
|
I am not arguing design wise, type safety not a bad thing. But this is something that should have been done from the beginning. Also, do not underestimate the convenience. Even if the wrapper type looks desirable design wise, it is still very convenient as it is. But regardless, my point is this change can cause major pain at this stage. |
|
You're probably right. Kubo seems pretty unhappy when I try to build with PR. Haven't looked at it in detail yet though. |
|
Closing per discussion in triage. context for not doing this in above comments |
Protects against fiddling with the peer bytes directly, which is probably wrong. Also fixes the root issues like libp2p/go-libp2p-resource-manager#67 (comment) and #2155.
This change should only be a translation of existing uses and doesn't try to change the semantics. If things needed bytes we update the code to return the bytes.