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

add sender id to message #207

Closed
wants to merge 2 commits into from
Closed

Conversation

adam-hanna
Copy link

Unless I'm mistaken, there's no way to get the sender ID from a message.

msg.GetFrom() returns the originator's id, not the most recent sender. There could be use cases where knowing who sent this message is valuable. For instance, I'm building a small benchmark tool and I need to calculate last delivery hop. I do so by logging recipient host id and sender id. My analyzer then parses the logs and calculates each message path.

I don't know if my simple implementation accomplishes this goal correctly or if there is a better way but would love to hear your thoughts.

Thanks!

@adam-hanna
Copy link
Author

broken, one sec...

@adam-hanna adam-hanna closed this Oct 9, 2019
@adam-hanna
Copy link
Author

ok, I believe this is a more correct implementation.

This isn't ready to be merged, it was mostly opened to show a possible implementation. Also, I did not write any tests. After discussion, I'm happy to create a PR that could be ready for merge.

@adam-hanna adam-hanna reopened this Oct 9, 2019
@@ -11,3 +11,5 @@ require (
github.com/multiformats/go-multistream v0.1.0
github.com/whyrusleeping/timecache v0.0.0-20160911033111-cfcb2f1abfee
)

go 1.13
Copy link
Author

Choose a reason for hiding this comment

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

I didn't do this. It must've been added via go?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's done automatically be go; please remove this line.

Copy link
Collaborator

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Not sure if we want that, see notes on exposure.

@@ -135,12 +135,17 @@ type PubSubRouter interface {

type Message struct {
*pb.Message
senderID peer.ID
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is available in the RPC struct and will be exposed to routers in the refactoring in #199.
It is also visible to validators as it is passed in the validation function.

@@ -11,3 +11,5 @@ require (
github.com/multiformats/go-multistream v0.1.0
github.com/whyrusleeping/timecache v0.0.0-20160911033111-cfcb2f1abfee
)

go 1.13
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's done automatically be go; please remove this line.

@vyzo vyzo requested a review from Stebalien October 9, 2019 19:17
@vyzo
Copy link
Collaborator

vyzo commented Oct 9, 2019

To clarify, I don't think this is relevant (the direct source of the message) to subscribers, as it is entirely non deterministic.
Routers and validators, who need it, have access to it.

@adam-hanna
Copy link
Author

Ah yes, I agree!

I didn't realize it was already made available on the validator. That worked! Thanks!

Closing...

@adam-hanna adam-hanna closed this Oct 10, 2019
@vyzo
Copy link
Collaborator

vyzo commented Oct 18, 2019

Note that we did end up adding it in #218, turns out it is needed for filecoin.

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.

2 participants