-
Notifications
You must be signed in to change notification settings - Fork 57
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
refactor: draft state machine #2656
Conversation
You can find the image built from this PR at
Built from f569321 |
You can find the image built from this PR at
Built from f569321 |
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.
Code definitely looks cleaner.
But some of the tests seem to be failing.
We still have to wrap the protocol then send a "end" payload from the client to have a clean stop on both side. |
The clean stop does not happen when running tests because the test end abruptly. |
About the closing of the connection, it seams like no other protocol do so, so I removed it. |
That is interesting, doesn't it cause the stream to stay open? Or when the object is GC'd the stream gets closed. Maybe worth investigating to confirm streams are not left dangling. |
147f6e3
to
d41f23b
Compare
Co-authored-by: Ivan FB <[email protected]> Co-authored-by: Prem Chaitanya Prathi <[email protected]>
d41f23b
to
01c2f10
Compare
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.
LGTM! Thanks!
I just added some minor comments/questions that I hope you find useful
Cheers
periodicSyncFut: Future[void] | ||
type WakuSync* = ref object of LPProtocol | ||
storage: Storage | ||
peerManager: PeerManager |
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.
Should we only keep a list of Store
peers?
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.
That would be the responsibility of PeerManager
no? Or do you meant instead of PeerManager
?
* feat: Waku Sync Protocol * feat: state machine (#2656) * feat: pruning storage mehcanism (#2673) * feat: message transfer mechanism & tests (#2688) * update docker files * added ENR filed for sync & misc. fixes * adding new sync range param & fixes --------- Co-authored-by: Ivan FB <[email protected]> Co-authored-by: Prem Chaitanya Prathi <[email protected]>
First draft of the Waku sync type state machine. Much more difficult to do in Nim than Rust but it works.
@chaitanyaprem WDYT?