search

libp2p / go-libp2p-pubsub

The PubSub implementation for go-libp2p
https://github.com/libp2p/specs/tree/master/pubsub
Other
313 stars 179 forks source link

feat: msgIdGenerator #468

Closed Wondertan closed 2 years ago

Wondertan commented 2 years ago

Closes #464

Personally, I still prefer the solution, where we compute and set Message.ID once in the pipeline and then assume it is set everywhere, instead of calling generator everywhere, but I might be missing something

vyzo commented 2 years ago

Basically the one small thing that we can and should improve on is provide a raw ID method in idGen, so that we don't have to allocate a wrapper message to compute it in the tracer.

This method could simply be refactored out of ID, and used by it.

vyzo commented 2 years ago

hey @Wondertan, when will you have some time to finish this? I would like to merge it soon.

It is very close, just needs to fix the allocation in the tracer by refactoring rawMsgInd. It also needs a rebase.

Wondertan commented 2 years ago

Hey @vyzo, planned for tomorrow.

Wondertan commented 2 years ago

Rebased on master

Wondertan commented 2 years ago

One last thing I am concerned about is naming for options. WithMessageIdFn is global options WithMsgIdFunction is for topic. Would be cool to unify them somehow or just append Topic in the end.

I will also add a test for the option later today.

vyzo commented 2 years ago

yeah, lets call it WithTopicMessageId or something similar.

Wondertan commented 2 years ago

Ready.

Btw, why do you use real Swarm and not mocknet in tests? What does this give for testing?

vyzo commented 2 years ago

Btw, why do you use real Swarm and not mocknet in tests? What does this give for testing?

Well, we want some integration testing happening and observing effects of the actual network in use; mocknet would deny us that.