search

status-im / specs

Specifications for Status clients.
https://specs.status.im/
MIT License
14 stars 14 forks source link

Update/waku replace #114

Closed Samyoul closed 4 years ago

Samyoul commented 4 years ago

Resolving issue https://github.com/status-im/specs/issues/61

Samyoul commented 4 years ago

@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.

Samyoul commented 4 years ago

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?

Samyoul commented 4 years ago

@decanus removing all personal pronouns from these specs will change the whole tone of the writing. Is this something we want to introduce with this draft?

Samyoul commented 4 years ago

Do we want to update the 8-eips.md with the Vac p2p specs for Waku?

Would a document dedicated to BIPs and EIPs be the correct place for the Vac specs?

If yes, should we rename this document from 8-eips.md to 8-supported-specs.md?

oskarth commented 4 years ago

@Samyoul please mark this PR as 'ready to review' once it is ready to be reviewed.

Do we want to update the 8-eips.md with the Vac p2p specs for Waku?

Would a document dedicated to BIPs and EIPs be the correct place for the Vac specs?

If yes, should we rename this document from 8-eips.md to 8-supported-specs.md?

This seems unrelated to the issue this PR should solve.

Samyoul commented 4 years ago

@oskarth no problem I will, but I've still got a few items to address and then I'll have some questions that need answering.

This seems unrelated to the issue this PR should solve.

The issues says:

Status Client spec reference Waku spec

I've included references to the Waku specs in every document that requires it, with the exception of 8-eips.md. From my understanding 8-eips.md should also have references to Waku specs, and if 8-eips.md has references to Waku specs the document name should change (In fact 8-eips.md already should change its name as it also contains BIPs).

To me this change is entirely in the scope of the issue's requirements. If you would be happier with this particular change in a separate PR I will open one later today.

decanus commented 4 years ago

@decanus removing all personal pronouns from these specs will change the whole tone of the writing. Is this something we want to introduce with this draft?

I'd be fine with doing this later on.

oskarth commented 4 years ago

The intent of https://github.com/status-im/specs/blob/master/docs/stable/8-eips.md was more to indicate various EIPs/(BIPs) not captured elsewhere in detail. Waku isn't an EIP, but I suppose we could add it there as opposed to only having it in-line. Up to you. In that case, it'd probably be more accurate to extend title of EIP to something more general (cc @3esmit). Just beware of scope creep :)

3esmit commented 4 years ago

Notice that Whisper is a EIP, see https://eips.ethereum.org/EIPS/eip-627 and Waku seems to fall into the same preposition.

Samyoul commented 4 years ago

I've split this issue into https://github.com/status-im/specs/issues/116

Samyoul commented 4 years ago

Questions

@oskarth, @decanus, @kdeme, @cammellos

https://github.com/status-im/specs/blob/87cab77f203efa7d53a19ce47bf7624255804eb4/docs/stable/1-client.md#L168-L170

https://github.com/status-im/specs/blob/2c7d086e0973667a929cc3ce5a18408ab5959bfe/docs/stable/9-waku-usage.md#L292-L297

https://github.com/status-im/specs/blob/2c7d086e0973667a929cc3ce5a18408ab5959bfe/docs/stable/9-waku-usage.md#L328-L330

kdeme commented 4 years ago

@Samyoul I'll reply with what I know. But others might want to chime in.

* Is there a `Waku` analogous term of `whispermail`?

I don't know this. I'm not even sure if we still use the (old) discovery v5 & its topics registration in combination with Waku.

* Should this reference to [the theoretical scaling model](https://github.com/vacp2p/research/tree/dcc71f4779be832d3b5ece9c4e11f1f7ec24aac2/whisper_scalability) be retained with the introduction of Waku?

That part of the scaling problem has been tackled with topic-interest in Waku. However, there are other scalability issues (mailserver and the general gossip/flooding nature of the protocol)

* If so should this section be rewritten from a post-Whisper perspective?

Perhaps yes, based on the above.

* Has research been done and/or introduced into Waku?

Not really no.

* ~Should I add a table for the message Status Options? See the items updated here~ [~Clarify packet types and fix status-options fields sections~](https://github.com/vacp2p/specs/pull/119)

Yeah, I'm not sure why we need to repeat that part to begin with.

* Is there a Waku analogous version of `shh_generateSymKeyFromPassword`?

I'd guess there is such JSON-RPC call. But be aware that RPC calls are currently not specified at all in Waku. So if it is there it is probably copied from the Whisper v6 JSON-RPC calls.

* Does Waku use extensions. Are the extensions now part of the Waku spec?

Not part of the spec. Not sure what is used.

Samyoul commented 4 years ago

I've opened this PR for review. Please note that I've still got 3 outstanding points that I feel need clarification https://github.com/status-im/specs/pull/114#issuecomment-629272782

oskarth commented 4 years ago

If we resolve outstanding issues and fix conflicting files it lgtm

decanus commented 4 years ago

@Samyoul this PR seems to have gotten rather large, I'd be happy if we merge it soon.

Samyoul commented 4 years ago

@oskarth @decanus I believe that this monster is now ready for the open seas 🐳

Samyoul commented 4 years ago

Hey @decanus would you be able to unblock this merge? Thanks 😄