Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use peerInfo and multiaddresses instead of peerID #1269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

richard-ramos
Copy link
Member

@richard-ramos richard-ramos commented Dec 19, 2024

Required by status-im/status-go#5946

The interaction with libwaku is done thru multiaddresses instead of peerID, because the peerstore is not exposed like we do in go-waku. So, to make it possible to use both go-waku and nwaku in status-go (depending on a build flag), changes are done both in go-waku and the api package to use multiaddresses and/or peer.AddrInfo where needed.

It's worth mentioning that the cases in which a peer.AddrInfo are those in which a node can have more than one multiaddress (like the storenodes), and in these cases the peer.AddrInfo will be translated into a comma separated string containing all the multiaddresses like it is done here: https://github.com/status-im/status-go/blob/1348510acfdfd93dbe5fc5b0a23e9caf532272a3/wakuv2/nwaku.go#L2904-L2910

This PR should not be merged until status-im/status-go#5946 is ready to be merged

cc: @Ivansete-status @gabrielmer

@status-im-auto
Copy link

status-im-auto commented Dec 19, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1ed0036 #1 2024-12-19 18:50:05 ~2 min unknown 📄log
✔️ 498022a #2 2024-12-19 20:18:15 ~2 min unknown 📄log

@richard-ramos richard-ramos changed the title refactor: use peerInfo instead of peerID refactor: use peerInfo and multiaddresses instead of peerID Jan 22, 2025
@richard-ramos richard-ramos marked this pull request as ready for review January 22, 2025 18:35
Copy link

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for it! 💯
Just one nitpick comment

@@ -352,7 +352,7 @@ func AddPeer(instance *WakuInstance, address string, protocolID string) (string,
return "", err
}

peerID, err := instance.node.AddPeer(ma, peerstore.Static, instance.relayTopics, libp2pProtocol.ID(protocolID))
peerID, err := instance.node.AddPeer([]multiaddr.Multiaddr{ma}, peerstore.Static, instance.relayTopics, libp2pProtocol.ID(protocolID))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to avoid that "cast" , []multiaddr.Multiaddr{...}? Maybe we can make it once within the AddPeer func.

Copy link
Collaborator

@chaitanyaprem chaitanyaprem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants