feat: peering implementation #260
Conversation
|
@alvin-reyes why not use the go-ipfs peering package at https://github.com/ipfs/go-ipfs/tree/master/peering |
alvin-reyes
requested review from
gmelodie,
whyrusleeping and
en0ma
and removed request for
gmelodie and
whyrusleeping
handlers.go
Outdated
|
|
||
| peerParamId, _ := peer.Decode(peerParam.ID) | ||
| // add the peer(s) | ||
| s.Node.Peering.AddPeer( |
There was a problem hiding this comment.
why not build/verify the multiaddr before adding it for peering? if it fails halfway, it means partial adding. Except this it your intention
There was a problem hiding this comment.
@en0ma Not sure what you mean. multiaddr.NewMultiaddr parses and verifies the multiaddr. So we only add a peer if all their multiaddrs are correctly parsed
There was a problem hiding this comment.
I meant the complete peer sent for the request - if there are multiple peers, 1 fails and 1 pass - it adds 1, yet returns an error for the request. So my question is, is that intended?
There was a problem hiding this comment.
Yeah maybe we should add all of them together. If one fails we don't add anything
There was a problem hiding this comment.
if one of the address is invalid, then the entire request should be invalid. It would be best for us to impose an absolute correctness of the addresses and inform the consumer if one of them is invalid.
There was a problem hiding this comment.
@alvin-reyes on line 479, it is not all or nothing - which is what I am pointing at
There was a problem hiding this comment.
Yes. That's why we return a json response immediately if one of the addrs is invalid.
Line 466 - 475
for _, addr := range peerParam.Addrs {
a, err := multiaddr.NewMultiaddr(addr)
if err != nil {
log.Errorf("handlePeeringPeersAdd error: %s", err)
return c.JSON(http.StatusBadRequest,
util.PeeringPeerAddMessage{
"Adding Peer(s) on Peering failed, the addr is invalid" + addr, params})
}
multiAddrs = append(multiAddrs, a)
}
If one of them is invalid, the entire request to add will be cancelled with StatusBadRequest response. It'll also show the message and list of invalid addrs.
There was a problem hiding this comment.
you are looking at per addr level. I am talking about per peer level. If 2 peers are sent - if 1 is added and the other fails
There was a problem hiding this comment.
If one of the peers are invalid, then we reject the entire request and raise the error similar to how we handle peer addrs level.
It make sense to reject the entire request if one of the peers or peer.addrs are invalid.
handlers.go
Outdated
|
|
||
| peerParamId, _ := peer.Decode(peerParam.ID) | ||
| // add the peer(s) | ||
| s.Node.Peering.AddPeer( |
There was a problem hiding this comment.
@en0ma Not sure what you mean. multiaddr.NewMultiaddr parses and verifies the multiaddr. So we only add a peer if all their multiaddrs are correctly parsed
handlers.go
Outdated
| type PeeringPeerRemoveMessage struct { | ||
| Message string `json: Message` | ||
| PeersRemove []peer.ID `json: Peers` | ||
| } |
There was a problem hiding this comment.
nit: put this on util/peering.go
There was a problem hiding this comment.
done. I think this should be in a model package rather than util. We can revisit once we start refactoring the codebase.
node/modules/peering/peering.go
Outdated
| func (ps *EstuaryPeeringService) Stop() error { | ||
| return ps.PeeringService.Stop() | ||
| } | ||
|
|
||
| func (ps *EstuaryPeeringService) ListPeers() []peer.AddrInfo { | ||
| return ps.PeeringService.ListPeers() | ||
| } | ||
|
|
||
| func (ps *EstuaryPeeringService) AddPeer(info peer.AddrInfo) { | ||
| ps.PeeringService.AddPeer(info) | ||
| } | ||
|
|
||
| func (ps *EstuaryPeeringService) RemovePeer(peerId peer.ID) { | ||
| ps.PeeringService.RemovePeer(peerId) | ||
| } |
There was a problem hiding this comment.
I'm not sure why we need to wrap everything up under the name of EstuaryPeeringService. Can't we simply use ps.PeeringService altogether?
There was a problem hiding this comment.
We want to make sure that we are loosely couple from the implementation so that we can extend the functionality. I wrapped it so that can add more features and decorate this later if needed.
Changes
Implemented Peering based on IPFS peering
Related Issue
#259
How to use
Add peering.peers via estuary cli
Add peering.peers via estuary-shuttle cli
Peering endpoints
Add peers
Remove
List
Start
Stop
Verification
Add peers
Remove peers
List all peers
Start peering
Stop peering