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: add peerstore spec and protobuf #91

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

Conversation

jacobheun
Copy link
Contributor

This adds a subset of commands to interact with the Peerstore.

  • ADD_PROTOCOLS, GET_PROTOCOLS, GET_PEER_INFO

Resolves #89

@ghost ghost assigned jacobheun Mar 20, 2019
@ghost ghost added the in progress label Mar 20, 2019
@vyzo
Copy link
Collaborator

vyzo commented Mar 20, 2019

There are more functions in the Peerstore interface. Also, don't expose SET_PROTOCOLS, this is used internally by identify I think.

@jacobheun
Copy link
Contributor Author

The only other two methods I can see being useful to expose would be

  • GET_PEER_INFOS, get PeerInfo's for a list of peer id's and,
  • GET_PEERS, get a list of peer id's in the Peerstore.

Probably also the majority of the AddrBook.

SupportsProtocols seems unnecessary since we're exposing GetProtocols. Close and SetProtocols (as you mentioned) probably shouldn't be exposed.

@vyzo
Copy link
Collaborator

vyzo commented Mar 20, 2019

There are more interfaces it implements.
And again, we don't want to expose the SET_PROTOCOLS method, as it is pointless to set it unless the peer advertises a protocol -- expose SUPPORTS_PROTOCOLS instead.

@jacobheun
Copy link
Contributor Author

And again, we don't want to expose the SET_PROTOCOLS method

It's not implemented and I don't intend to add it, are you referring to ADD_PROTOCOLS?

Is SUPPORTS_PROTOCOLS even necessary though? It's just a convenience check on top of GET_PROTOCOLS the client could easily perform themselves.

@vyzo
Copy link
Collaborator

vyzo commented Mar 20, 2019

It's not implemented and I don't intend to add it, are you referring to ADD_PROTOCOLS?

Yes, both are not to be used willy nilly.

Is SUPPORTS_PROTOCOLS even necessary though? It's just a convenience check on top of GET_PROTOCOLS the client could easily perform themselves.

fair enough.

@jacobheun
Copy link
Contributor Author

I've added some more types based on the available interfaces. If those look good I will add accompanying specs for those requests. I didn't include the PeerMetadata interface for now as I'm not quite clear what that would look like at the moment.

@vyzo
Copy link
Collaborator

vyzo commented Mar 20, 2019

Now this is probably too much; we need to be a little more selective for utility here :)

@jacobheun
Copy link
Contributor Author

Here's an updated list. I removed metrics as that's probably unnecessary at this point. I think this gives us a good base for interacting with the Peerstore.

Peer operations
GET_PROTOCOLS so we can retrieve all known protocols for a peer
GET_PEER_INFOS so we can get PeerInfo for any number of PeerIds
GET_PEERS get all peers currently in the Peerstore. LIST_PEERS currently only returns connected peers.

Address operations
These should account for basic address operation needs
ADD_ADDRS
SET_ADDRS
GET_ADDRS

Key Operations
I think this may be the only thing we may want to expose from the Keybook
PUB_KEY

@vyzo
Copy link
Collaborator

vyzo commented Mar 21, 2019

That looks better. @bigs opinions?

@vyzo
Copy link
Collaborator

vyzo commented Mar 21, 2019

cc @raulk

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

Successfully merging this pull request may close these issues.

2 participants