chore: Lightpush minor refactor #3538
Conversation
Ivansete-status
left a comment
Ivansete-status
left a comment
There was a problem hiding this comment.
Wonderful PR! Thanks so much for it! 💯
NagyZoltanPeter
left a comment
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the effort! Added some considerations though.
|
You can find the image built from this PR at Built from 5602781 |
s-tikhomirov
force-pushed
the
chore/lightpush-minor-refactor
branch
4 times, most recently
from
79f1f08 to
297fa16
Compare
|
Did some more refactoring unifying publishing procedures to avoid code duplication. No more substantial changes planned, re-requesting review. |
s-tikhomirov
requested review from
Ivansete-status,
NagyZoltanPeter,
darshankabariya and
fryorcraken
Ivansete-status
force-pushed
the
master
branch
from
5118fc3 to
08d14fb
Compare
waku/waku_lightpush/protocol.nim
Outdated
| wl: WakuLightPush, peerId: PeerId, pushRequest: LightpushRequest | ||
| ): Future[WakuLightPushResult] {.async.} = | ||
| let pubsubTopic = pushRequest.pubSubTopic.valueOr: | ||
| proc extractPubsubTopic( |
There was a problem hiding this comment.
nit: I am wondering if this logic should be handled by the pushHandler instead.
There was a problem hiding this comment.
Could you elaborate? I'm not sure I understand how this relates to your other suggestion.
There was a problem hiding this comment.
Well, the question is where should the pubsubtopic/sharding logic be applied? Within handleRequest ? or by the caller of handleRequest?
I am not sure which one is a best choice. My comment was here to raise awareness so you can think of it. Maybe @NagyZoltanPeter has an opinion as he touched light push recently.
There was a problem hiding this comment.
Well, the question is where should the pubsubtopic/sharding logic be applied? Within
handleRequest? or by the caller ofhandleRequest?I am not sure which one is a best choice. My comment was here to raise awareness so you can think of it. Maybe @NagyZoltanPeter has an opinion as he touched light push recently.
Hm, good question.
I'm not against this change, meanwhile I think pubsub-topic or better shard handling shall be refactored separately from this. Due we have this shard derivative spread in many places of the code (waku interface, rest api, filter and lightpush). It would be good to concentrate into fewer places. Ideally from send perspective it would be good to place it into behind relay's publish.
One naming comment from my side for @s-tikhomirov, is I would name extractPubsubTopic rather extractShard.
WDYT?
s-tikhomirov
force-pushed
the
chore/lightpush-minor-refactor
branch
from
746570f to
2d22dcb
Compare
Co-authored-by: NagyZoltanPeter <[email protected]>
Co-authored-by: Ivan FB <[email protected]>
Co-authored-by: Ivan FB <[email protected]>
s-tikhomirov
force-pushed
the
chore/lightpush-minor-refactor
branch
from
2d22dcb to
d778bb0
Compare
s-tikhomirov
force-pushed
the
chore/lightpush-minor-refactor
branch
from
3a8f126 to
da4394c
Compare
Description
In the spirit of "make change easy", this PR is a minor refactor of Lightpush that would hopefully make the protocol easier to reason about.
Changes
Issue