Skip to content
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

14/WAKU2-MESSAGE: Update #655

Closed
wants to merge 18 commits into from
Closed

14/WAKU2-MESSAGE: Update #655

wants to merge 18 commits into from

Conversation

jimstir
Copy link
Contributor

@jimstir jimstir commented Jan 7, 2024

Updating spec fixing links, sembr and language. Also included new message validator section.

@jimstir jimstir requested a review from kaiserd January 7, 2024 02:36

An example proto file following this specification can be found [here (vacp2p/waku)](https://github.com/vacp2p/waku/blob/main/waku/message/v1/message.proto).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link is not necessary as the message.proto is displayed in the specification.

@@ -82,11 +94,17 @@ message WakuMessage {
optional bool ephemeral = 31;
}
```
## Waku Message Vaildator
Copy link
Contributor Author

@jimstir jimstir Jan 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specification does not explicitly mention message validation. As detailed in libp2p spec and pubsub spec there is a message validator. Also in Waku node implements a validator here, message_validator.ts and wakuGossipSub.

@jimstir jimstir requested a review from jm-clius January 26, 2024 15:41
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. Added a couple of comments.

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved
If, instead, the attribute is omitted or set to `false`, the message SHOULD be interpreted as non-ephemeral.
* The `ephemeral` attribute, if present,
signifies the transient nature of the message.
For example, an ephemeral message SHOULD not be persisted by the [64/WAKU2-NETWORK](/spec/64).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a bit weird to refer to a specific network deployment (RFC64) here. Perhaps the more correct phrasing is more generic:

SHOULD not be persisted by any Waku nodes

syntax = "proto3";

message WakuMessage {
bytes payload = 1;
string content_topic = 2;
string contentTopic = 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protobuf field names should use snake_case.

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved
```

An example proto file following this specification can be found [here (vacp2p/waku)](https://github.com/vacp2p/waku/blob/main/waku/message/v1/message.proto).
### Waku Message Vaildation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this section belongs in this RFC. Wouldn't validation rather be a function of the relay protocol (RFC17, which already includes RLN validation)? We do a bunch of validations on Waku Messages, but this is covered in the network spec in RFC64. According to me the scope of this RFC should just be the minimal definition of a Waku Message - Relayer behaviour and how the message is interpreted otherwise belong on "higher layer" specs. Happy to hear other opinions here though.

Copy link
Contributor Author

@jimstir jimstir Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jm-clius Should this spec maybe reference 64/Waku2-Network for message interpretation instead? I suggested this section so the implementer can be aware why the two attributes content_topic and payload MUST be included in a message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with @jm-clius . Validation should be in 17. This doc just specs the message format.

jimstir and others added 6 commits February 6, 2024 23:31
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
Co-authored-by: Hanno Cornelius <68783915+jm-clius@users.noreply.github.com>
@jimstir
Copy link
Contributor Author

jimstir commented Mar 5, 2024

Continue discussion: vacp2p/rfc-index#20. The RFC process has been changed separating raw specs and the draft/stable specs into different repositories.

@jimstir jimstir closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants