-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update/waku replace #114
Update/waku replace #114
Conversation
There may be broken links after this change
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.
Good start! I think the main thing to keep in mind is that this spec for Status clients still supports Whisper. That is, we can't just replace Waku with Whisper, since that would indicate clients aren't backwards compatible. Instead, we have to find a way to communicate intent that Whisper MAY be used and Waku SHOULD be used, and that the latter is strongly recommended. Then find a way to refer to that consistently from other parts of the spec.
@oskarth @kdeme Should these specs include a page or at least a section about the Whisper / Waku bridge implementation? Currently we have the functionality but it isn't documented. Whisper / Waku bridging is mentioned in the specs, but there is no mention (at least that I've seen) of bridging being required. I believe that we need clear language that details that a Status network simultaneously running Whisper and Waku MUST have a Whisper / Waku bridge, to allow messages to propagate both networks. Otherwise supporting both Whisper and Waku becomes redundant. |
To properly update this spec we will need at least 2 additional pages.
This will allow for detailing the Whisper usage concurrently with detailing the Waku usage. Would the below file names match the current convention?
|
Co-authored-by: Dean Eigenmann <7621705+decanus@users.noreply.github.com>
Addressed in GH-120
If we resolve outstanding issues and fix conflicting files it lgtm |
@Samyoul this PR seems to have gotten rather large, I'd be happy if we merge it soon. |
Added my name where appropriate and addressed some minor issues
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.
Nice!
Co-authored-by: Dean Eigenmann <7621705+decanus@users.noreply.github.com>
Co-authored-by: Dean Eigenmann <7621705+decanus@users.noreply.github.com>
Hey @decanus would you be able to unblock this merge? Thanks 😄 |
Resolving issue #61