fix: make getPeerId return the target peer id from relay addrs #325
Conversation
If a p2p-circuit address doesn't contain the `/p2p/QmFoo` tuple for the target peer, `getPeerId` returns the peer id of the relay which is not what you'd expect. The fix here is for relay addresses, to return the peer id of the target and never the relay. Fixes #319
| if (code === names.p2p.code) { | ||
| tuples.push([code, name]) | ||
| } | ||
|
|
||
| // if this is a p2p-circuit address, return the target peer id if present | ||
| // not the peer id of the relay | ||
| if (code === names['p2p-circuit'].code) { | ||
| tuples = [] | ||
| } |
There was a problem hiding this comment.
i find it confusing that we push to tuples and then empty tuples if p2p-circuit is found.
Could this result in (eventually longer and currently unknown) multiaddr tuples being removed that shouldn't? Instead, should we just not call tuples.push if it's the p2p-circuit case for the current code?
There was a problem hiding this comment.
I don't think so, because circuit relay addresses are not supposed to be relayed themselves so whatever is after p2p-circuit should be what we are interested in.
We reset the tuples because whatever was before the p2p-circuit tuple is the relay address and we're trying to extract the PeerId of the target, not the relay.
It's trying to handle the case of:
/ip4/foo/p2p/QmRelay/p2p-circuit -> should return null not QmRelay
| it('does not extract a peer Id from a circuit relay multiaddr where only the relay peer id is present', () => { | ||
| expect( | ||
| multiaddr('/ip4/127.0.0.1/tcp/123/p2p/bafzbeigweq4zr4x4ky2dvv7nanbkw6egutvrrvzw6g3h2rftp7gidyhtt4/p2p-circuit').getPeerId() | ||
| ).to.be.null() | ||
| }) |
There was a problem hiding this comment.
the positive test case (peerId of relayed peer exists) already exists in the tests from what I can see. However, do we need to check a case where we have something like
/ip4/127.0.0.1/tcp/123/p2p/bafzbeigweq4zr4x4ky2dvv7nanbkw6egutvrrvzw6g3h2rftp7gidyhtt4/p2p-circuit/p2p/bafzbeidt255unskpefjmqb2rc27vjuyxopkxgaylxij6pw35hhys4vnyp4
to make sure the correct peer ID is returned when there are two?
There was a problem hiding this comment.
I think we have this in the 'extracts the correct peer Id from a circuit multiaddr' test?
achingbrain
changed the title
|
🎉 This PR is included in version 12.1.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
If a p2p-circuit address doesn't contain the
/p2p/QmFootuple for the target peer,getPeerIdreturns the peer id of the relay which is not what you'd expect.The fix here is for relay addresses, to return the peer id of the target and never the relay.
Fixes #319