Skip to content

Conversation

@nolash
Copy link
Contributor

@nolash nolash commented Dec 11, 2019

No description provided.

@nolash nolash self-assigned this Dec 11, 2019
@nolash nolash added the improvement enhancement of an existing protocol/strategy/convention label Dec 11, 2019
@nagydani nagydani self-requested a review December 13, 2019 16:29
Copy link
Collaborator

@nagydani nagydani left a comment

Choose a reason for hiding this comment

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

Very well written. Approved.

@diegomasini diegomasini self-requested a review December 16, 2019 15:40
Copy link
Contributor

@diegomasini diegomasini left a comment

Choose a reason for hiding this comment

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

Great SWIP! Please assign number 35 to the document so we can merge it.


type SessionRPCInterface interface {
NewForwardPeerSession(ctx context.Context) (context.Context, error)
Subscribe(ctx context.Context, "kademlia", ch chan PeerEvent, "forward") (sub rpc.Subscription, err error)
Copy link

Choose a reason for hiding this comment

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

can you explain why there are hardcoded values in the signature?

NewForwardPeerSession(ctx context.Context) (context.Context, error)
Subscribe(ctx context.Context, "kademlia", ch chan PeerEvent, "forward") (sub rpc.Subscription, err error)
GetForwardPeer(ctx context.Context, numberOfPeers int) (peers []ForwardPeer, err error)
CloseForwardPeerSession(ctx context.Context)
Copy link

Choose a reason for hiding this comment

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

maybe just CloseSession?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it not more intiutive if it matches the constructor name?

SessionId string
}

type SessionRPCInterface interface {
Copy link

Choose a reason for hiding this comment

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

interface type name should not end with Interface. I'd also vote to reshuffle this name altogether in favor of something more meaningful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ForwardSession ForwardSessionAPI @acud ?


## Rationale

The component shall provide a service where given a specific Swarm Overlay Address, connected peers will be returned in the order of closest to farthest.
Copy link

Choose a reason for hiding this comment

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

you mention a given Swarm Overlay Address, however a pivot address is not mentioned in any of the function signatures you've defined. In layman terms: you need to know which peer to forward to given a content addressed hash and the peers to forward to would be ordered according to that reference, but that reference parameter is not mentioned in the signatures (or am i getting this all wrong?)

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

Labels

improvement enhancement of an existing protocol/strategy/convention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants