fix(WAKU2-RPC): encode waku message payloads in base64 #572

LNSD · 2023-02-13T09:57:03Z

While the plan is to supersede the JSON-PRC API with a Rest API implementation, some conversations are pending about the Waku API design that should happen before resuming the Rest API initiative.

Meanwhile, some stakeholders are using the JSON-RPC API (e.g., the Logos Data Science Team), and as a consequence, some issues are being reported in the nwaku repository:

bug: get_waku_v2_relay_v1_messages return an array has payload instead of expected hex string waku-org/nwaku#1139
bug: Duplicated peers in get_waku_v2_admin_v1_peers waku-org/nwaku#1549

This PR aims to replace the WakuMessage payload hex encoding with the more efficient and ergonomic (e.g., for the browsers) base64 encoding.

Changed the WakuMessage payload encoding from hex to base64.
As the "Private API" is related to and has many similarities with the Waku Relay API, both APIs have been collocated.
Change some API response types to use the common WakuMessage type.

I aim to merge these JSON-RPC implementation changes as part of this week's nwaku release (v0.15.0).

richard-ramos

Nice! base64 for byte arrays feel much more 'natural'.

Btw, TIL that private api was exclusive for relay. In go-waku, the methods to retrieve messages return them regardless of the protocol on which they were received (filter or relay), since filter does not have a way to decode messages, only get_waku_v2_filter_v1_messages; and neither relay did before this PR

jm-clius

I certainly agree with the change to base64, but think we need to decide on two things:

Using WakuMessage as input for post methods now require applications to provide fields for which they may not have the info to fill it out. I.e. to use version 1 they must use the private api methods. This may be OK, but it's worth considering the impact.
I'm OK with introducing backwards incompatibilities and announcing it clearly in release notes, but we should consider using the built in versioning in method signatures to maintain the old post method (at least for a release cycle or so, to allow people to have a time where they can change their API usage before it breaks).

jm-clius · 2023-02-13T13:32:30Z

content/docs/rfcs/16/README.md

-`WakuRelayMessage` is an `Object` containing the following fields:
-
-| Field | Type | Inclusion | Description |
-| ----: | :---: | :---: | ----------- |
-| `payload` | `String` | mandatory | The payload being relayed as a hex encoded data string |
-| `contentTopic` | `String` | optional | Message content topic for optional content-based filtering |
-| `timestamp` | `Number` | optional | The time at which the message is generated by its sender. This field holds the Unix epoch time in nanoseconds as a 64-bits integer value.  |
-| `ephemeral` | `Boolean` | optional | This flag indicates the transient nature of the message. |


This API input Object was constructed to separate the fields that a user/app may want to provide to the API from what the API should provide itself. This required separating the WakuMessage wire format from the object provided to post calls. Currently the only field which is determined by the API is version, but wouldn't it make sense to maintain separation between what is published and the message constructed by the node/API?

jm-clius · 2023-02-13T13:36:40Z

content/docs/rfcs/16/README.md

-| `ephemeral` | `Boolean` | optional | This flag indicates the transient nature of the message. |
-
- > **_NOTE:_** `WakuRelayMessage` maps directly to a [`WakuMessage`](#wakumessage), except that the latter contains an explicit message `version`. For `WakuRelay` purposes, the versioning is handled by the API.
-
 ### `post_waku_v2_relay_v1_message`


This is a breaking change for this API method (also the Private API methods, but these are presumable very seldomly used). Since the Relay API is used by several applications/platforms, wouldn't it make sense to maintain backwards compatibility with this API version (at least in the implementation) and introduce post_waku_v2_relay_v2_message (note v2 indicates new API version). The versioning as part of the method signature was introduced to prevent backwards incompatibilities.
We could also deprecate this function, but it may be a good idea to maintain backwards compatibility for one release cycle (i.e. until nwaku v0.16.0).

LNSD · 2023-02-13T15:10:07Z

In go-waku, the methods to retrieve messages return them regardless of the protocol on which they were received (filter or relay), since filter does not have a way to decode messages, only get_waku_v2_filter_v1_messages; and neither relay did before this PR

I didn't know about that at all 🤯 But for me makes sense to have separate APIs for different protocols (separation of concerns and single responsibility principles).

jm-clius

Changing to approval as it seems:

all major stakeholders are agreed on API change, including 3rd party platforms
the existing API definition is inconsistent

fryorcraken · 2023-02-13T22:38:40Z

content/docs/rfcs/16/README.md

+## Relay Private API
+
+The Private API provides functionality to encrypt/decrypt `WakuMessage` payloads using either symmetric or asymmetric cryptography. This allows backwards compatibility with [Waku v1 nodes](https://github.com/vacp2p/specs/blob/master/specs/waku/v1/waku-1.md).
+It is the API client's responsibility to keep track of the keys used for encrypted communication. Since keys must be cached by the client and provided to the node to encrypt/decrypt payloads, a Private API SHOULD NOT be exposed on non-local or untrusted nodes.


a Private API SHOULD NOT be exposed on [non-local] or untrusted nodes.

I find it to be an odd phrasing. How can one expose an API on an untrusted node? Would be good to rephrase.
I guess there are two warnings:

Do not expose your private API to untrusted actors (but if the nwaku node does not hold the key then is the encryption/decryption really the reason not to do that?)

Do not use the relay private API on a node you don't trust (they could be caching your keys)

fryorcraken · 2023-02-13T22:39:49Z

content/docs/rfcs/16/README.md

+| ----: | :---: | :---: |----------- |
+| `topic` | `String` | mandatory | The [PubSub `topic`](https://github.com/libp2p/specs/blob/master/pubsub/README.md#the-topic-descriptor) being published on |
+| `message` | [`WakuMessage`](#wakumessage) | mandatory | The (unencrypted) `message` being relayed |
+| `symkey` | `String` | mandatory | The hex encoded symmetric key to use for payload encryption. This field MUST be included if symmetric key cryptography is selected |


symkey

Maybe best to stick to camel case across the whole RFC?

LNSD · 2023-02-14T10:00:59Z

@fryorcraken I agree with you. But it was not my intention to rewrite the RFC. I just collocated the Private API after the Relay API. The diff messes it up 😅

We'll also update the phrasing if further improvements to the JSON-RPC RFC are necessary. Thanks for your comments :)

fix(WAKU2-RPC): encode waku message payloads in base64

fef583c

LNSD added the track:nwaku-productionization nwaku productionization track (Waku Product) label Feb 13, 2023

LNSD requested review from richard-ramos, jm-clius and fryorcraken February 13, 2023 09:57

LNSD self-assigned this Feb 13, 2023

richard-ramos approved these changes Feb 13, 2023

View reviewed changes

richard-ramos mentioned this pull request Feb 13, 2023

fix: encode waku message payloads in base64 waku-org/go-waku#448

Merged

jm-clius reviewed Feb 13, 2023

View reviewed changes

jm-clius approved these changes Feb 13, 2023

View reviewed changes

LNSD merged commit 1e9e985 into master Feb 13, 2023

LNSD deleted the fix-16-rpc-base64 branch February 13, 2023 16:52

LNSD mentioned this pull request Feb 13, 2023

fix(jsonrpc): encode waku messages payload in base64 (#1139) waku-org/nwaku#1555

Merged

6 tasks

fryorcraken reviewed Feb 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(WAKU2-RPC): encode waku message payloads in base64 #572

fix(WAKU2-RPC): encode waku message payloads in base64 #572

LNSD commented Feb 13, 2023 •

edited

Loading

richard-ramos left a comment

jm-clius left a comment

jm-clius Feb 13, 2023

jm-clius Feb 13, 2023

LNSD commented Feb 13, 2023

jm-clius left a comment

fryorcraken Feb 13, 2023

fryorcraken Feb 13, 2023

LNSD commented Feb 14, 2023

fix(WAKU2-RPC): encode waku message payloads in base64 #572

fix(WAKU2-RPC): encode waku message payloads in base64 #572

Conversation

LNSD commented Feb 13, 2023 • edited Loading

richard-ramos left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Feb 13, 2023

Choose a reason for hiding this comment

jm-clius Feb 13, 2023

Choose a reason for hiding this comment

LNSD commented Feb 13, 2023

jm-clius left a comment

Choose a reason for hiding this comment

fryorcraken Feb 13, 2023

Choose a reason for hiding this comment

fryorcraken Feb 13, 2023

Choose a reason for hiding this comment

LNSD commented Feb 14, 2023

LNSD commented Feb 13, 2023 •

edited

Loading