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

bug: get_waku_v2_relay_v1_messages return an array has payload instead of expected hex string #1139

Closed
fryorcraken opened this issue Sep 10, 2022 · 10 comments · Fixed by #1555
Assignees
Labels
bug Something isn't working

Comments

@fryorcraken
Copy link
Collaborator

Problem

get_waku_v2_relay_v1_messages returns an array of decimal number in payload instead of the expected hex string defined in https://rfc.vac.dev/spec/16/#get_waku_v2_relay_v1_messages

Impact

Minor defect, just unexpected as an implementer.

To reproduce

  1. Ensure the node receives some messages over relay

  2. JSON RPC call:

 get_waku_v2_relay_v1_messages [ '/waku/2/default-waku/proto' ]
```

JSON RPC Response:

```
{
  "jsonrpc": "2.0",
  "id": 1,
  "result": [
    {
      "payload": [
        76,
        105,
        103,
        104,
        116,
        32,
        80,
        117,
        115,
        104,
        32,
        119,
        111,
        114,
        107,
        115,
        33
      ],
      "contentTopic": "/test/1/waku-light-push/utf8",
      "version": 0,
      "timestamp": 1662821422291000000,
      "proof": {
        ...
      }
    }
  ]
}
```

### Expected behavior

`payload` is a hex string.

### nwaku version/commit hash

`v0.11`
@fryorcraken fryorcraken added bug Something isn't working track:maintenance labels Sep 10, 2022
@fryorcraken
Copy link
Collaborator Author

fryorcraken commented Sep 10, 2022

Odd, the crypto API returns hex:

RPC Query:  get_waku_v2_private_v1_asymmetric_messages [
  '/waku/2/default-waku/proto',
  '0xf604e5439d1df44b2f07153d14cc2d2f960997fa3672733309cb6ecbb33eea34'
] 
 {"jsonrpc":"2.0","id":1,"result":[{"payload":"0x546869732069732061206d657373616765204920616d20676f696e6720746f20656e6372797074","contentTopic":"/test/1/waku-message/utf8","timestamp":1662825744753000000}]} +4ms

fryorcraken added a commit to waku-org/js-waku that referenced this issue Sep 10, 2022
@fryorcraken
Copy link
Collaborator Author

@richard-ramos I have witnessed the same behaviour on go-waku

@richard-ramos
Copy link
Member

@fryorcraken in go-waku I added this behavior to match nwaku's. Otherwise Go would have encoded the byte array as base64 (which is what it's used in REST API)

@rymnc rymnc self-assigned this Sep 12, 2022
@rymnc
Copy link
Contributor

rymnc commented Sep 13, 2022

Upon some investigation,

@jm-clius
Copy link
Contributor

@fryorcraken @richard-ramos is it worth getting a fix in nwaku (and parity in the other clients)? Although the JSON-RPC API is going to be deprecated, we have to maintain it while still being used.

@fryorcraken
Copy link
Collaborator Author

IMHO, it'd be cleaner but now that I deal with it, I am not fussed:

https://github.com/waku-org/js-waku/blob/5bab85f93f22f853f0d2d07d134b8c5d0d4a93f1/src/test_utils/nwaku.ts#L316

@dao any thoughts on the matter as you use the JSON RPC API?

@dao
Copy link

dao commented Sep 26, 2022

IMHO, it'd be cleaner but now that I deal with it, I am not fussed:

https://github.com/waku-org/js-waku/blob/5bab85f93f22f853f0d2d07d134b8c5d0d4a93f1/src/test_utils/nwaku.ts#L316

@dao any thoughts on the matter as you use the JSON RPC API?

I vote for consistency - easy enough for us to make this change once

@jm-clius
Copy link
Contributor

@rymnc this is not priority. Given your involvement with RLN, should we unassign you here and move this item to a backlog?

@jm-clius jm-clius added this to the Release 0.14.0 milestone Nov 21, 2022
@rymnc rymnc removed their assignment Feb 10, 2023
@rymnc
Copy link
Contributor

rymnc commented Feb 10, 2023

Unassigned, although I would suggest to leave it in Todo since there are quite a few consumers of the json-rpc api, as @LNSD suggested. Will leave it up to you to decide though!

@LNSD LNSD self-assigned this Feb 10, 2023
@LNSD
Copy link
Contributor

LNSD commented Feb 10, 2023

As @richard-ramos commented above, the efficient way to serialize the messages' payload is to use base64 encoding.

I am assigning this issue to me, as I think it is feasible to fix this as part of v0.15.0.

@LNSD LNSD modified the milestones: Release 0.16.0, Release 0.15.0 Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants