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

remove peer.IDFromString #274

Merged
merged 1 commit into from
Jul 9, 2022
Merged

Conversation

marten-seemann
Copy link
Contributor

This method is almost always used incorrectly. Contrary to what the name indicates, the following does not work:

var id peer.ID
id2 := peer.IDFromString()
// id is not == id2 now!

The reason is that String() encodes the peer ID, so you need to call Decode instead. The existence of this function has cost me hours and hours of debugging, and we continue to use it in places where we shouldn't. We'd be better off if this function didn't exist.

Here's where we got this wrong in go-libp2p: libp2p/go-libp2p#1644
And I'm pretty sure that lotus also got it wrong here: https://github.com/filecoin-project/lotus/blob/1de56d5e470d743e8b1542c889411360c84db431/node/modules/lp2p/libp2p.go#L70-L78 (I haven't followed the entire code path where these peer IDs originate from, but assuming they come from a config file, they're most likely encoded peer IDs)

@github-actions
Copy link

github-actions bot commented Jul 7, 2022

gocompat says:

branch 'master' set up to track 'origin/master'.
"github.com/libp2p/go-libp2p-core/peer".IDFromString TopLevelDeclarationDeleted

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.

not a fan either, but maybe there are (correct) users out there.

@Stebalien ?

@marten-seemann
Copy link
Contributor Author

Just want to point out, we still have the IDFromBytes method, so those use cases can easily cast their string into a byte slice.

@Stebalien
Copy link
Member

We should add a IDFromByteString. Normally, I'd say deprecate. But I'm seeing 99% incorrect uses so yeah.... let's kill it.

We have:

  1. One use in go-ipns
  2. One use in go-ipfs
  3. One incorrect use in the resource manager.
  4. One correct use in the routed host (libp2p).
  5. Some incorrect uses in lotus.

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.

3 participants