-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: Add ability for P2P to wait for pushlog by peer #1098
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1098 +/- ##
===========================================
- Coverage 67.43% 67.39% -0.04%
===========================================
Files 178 178
Lines 16618 16635 +17
===========================================
+ Hits 11206 11211 +5
- Misses 4465 4476 +11
- Partials 947 948 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just my preliminary read, flagging some typos :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and good job figuring this one out :)
@@ -270,6 +270,7 @@ func (p *Peer) RegisterNewDocument( | |||
DocKey: &pb.ProtoDocKey{DocKey: dockey}, | |||
Cid: &pb.ProtoCid{Cid: c}, | |||
SchemaID: []byte(schemaID), | |||
Creator: p.host.ID().String(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I do worry a bit that we'll end up running into some kind of privacy/security issue by either broadcasting or relying-on fixed host ids like this, a problem for another day (if ever) though I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID was already used in the P2P exchange so I'm not sure if adding it here make a huge difference? You think it does?
John was also saying we'll need the creator ID at some point in the future anyways for state-proofs if I recall correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ID was already used in the P2P exchange so I'm not sure if adding it here make a huge difference? You think it does?
I really don't know. Just feels odd to see a trackable, fixed, actor identifier in a (privacy focused?) decentralised system. I also no idea how avoidable such a thing is. But it might be good to avoid spreading its use where we can do so easily (not saying it is easy here, I have no idea).
Relevant issue(s) Resolves #1097 Description This PR splits the WaitForPushLogEvent function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.
…1098) Relevant issue(s) Resolves sourcenetwork#1097 Description This PR splits the WaitForPushLogEvent function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.
Relevant issue(s)
Resolves #1097
Description
This PR splits the
WaitForPushLogEvent
function into two distinct versions. We can now wait for a log create by a specific peer or sent by an intermediary peer.Tasks
integration test
(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.
Specify the platform(s) on which this was tested: