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

feat: complete identify protocol and improved tests #506

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

acul71
Copy link
Contributor

@acul71 acul71 commented Feb 19, 2025

NOTE: this is a draft pull request
The returned info agent version was a generic py-libp2p/unknown
I've added a function in libp2p/utils.py that return the agent version so that I can return it in the agentVersion field of identify protocol modifying libp2p/identity/identify/protocol.py

I've improved the test tests/core/identity/identify/test_protocol.py adding tests so that every field is verified.

I'm planning to complete #358 adding missing observed address from stream and then identify/push .
Specs are here https://github.com/libp2p/specs/blob/master/identify/README.md

🕷️

@pacrob
Copy link
Member

pacrob commented Feb 21, 2025

@acul71, I'm unclear if this is ready for review. It will need a rebase and a newsfragment, otherwise looking good so far.

@acul71
Copy link
Contributor Author

acul71 commented Feb 21, 2025

@pacrob

@acul71, I'm unclear if this is ready for review. It will need a rebase and a newsfragment, otherwise looking good so far.

This code is ready for review.
But does not include all features requested by identify protocol.
Anyway Manusheel suggested me to commit PR of my work on identify little by little, so that I can get feedback, instead of a "big" commit at the end of the work
PS: doing make lint, it did lint the README.md file (not sure why) so I commit also this, even if it's unrelated

@seetadev
Copy link

@acul71 : Hi Luca. Kindly add a note that this a draft pull request. I will keep reviewing side by side.

For final review by Paul, I will leave a message tagged to him.

@pacrob : Wish to share that I recommended Luca and other PLDG cohort members to share the pull request with unit and integration tests. Also, step by step so that we could discuss their work in progress during the maintainer's call every Thursday.

@pacrob

@acul71, I'm unclear if this is ready for review. It will need a rebase and a newsfragment, otherwise looking good so far.

This code is ready for review. But does not include all features requested by identify protocol. Anyway Manusheel suggested me to commit PR of my work on identify little by little, so that I can get feedback, instead of a "big" commit at the end of the work PS: doing make lint, it did lint the README.md file (not sure why) so I commit also this, even if it's unrelated

@acul71 acul71 changed the title feat: add agent version to identify protocol and improved tests feat: complete identify protocol and improved tests Feb 25, 2025
@acul71
Copy link
Contributor Author

acul71 commented Feb 25, 2025

@pacrob @seetadev
With this commit f24c6c8 the identify protocol is complete including all the requested fields.
In particular I've added the missing observed_addr to identify protocol
I saw that get_peerstore method was removed from IHost interface. Not sure why but I added it back (It's implemented in class BasicHost(IHost) )
I've added a test so now it should be able to identify another peer
Planning to add this other tests:

  • it('should register for identify protocol on start')
  • it('should time out during identify')
        # get observed address from ``stream``
        # class Swarm(Service, INetworkService):
        # TODO: Connection and `peer_id` are 1-1 mapping in our implementation,
        # whereas in Go one `peer_id` may point to multiple connections.
        # connections: dict[ID, INetConn]
        # Luca: So I'm assuming that the connection is 1-1 mapping for now
        peer_id = stream.muxed_conn.peer_id  # remote peer_id
        peer_store = host.get_peerstore()  # get the peer store from the host
        remote_peer_multiaddrs = peer_store.addrs(
            peer_id
        )  # get the Multiaddrs for the remote peer_id
        logger.debug("multiaddrs of remote peer is %s", remote_peer_multiaddrs)
        logger.debug("received a request for %s from %s", ID, peer_id)

        # Select the first address if available, else None
        observed_multiaddr = (
            remote_peer_multiaddrs[0] if remote_peer_multiaddrs else None
        )
        protobuf = _mk_identify_protobuf(host, observed_multiaddr)

🕷️

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

Successfully merging this pull request may close these issues.

3 participants