-
Notifications
You must be signed in to change notification settings - Fork 54
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
feat: supporting meta field in WakuMessage #2384
feat: supporting meta field in WakuMessage #2384
Conversation
You can find the image built from this PR at
Built from ebbf3f5 |
0401a7f
to
02cb4a7
Compare
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.
It looks great so far. Good work!
I just added a minor comment
a822948
to
10d2bf8
Compare
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.
LGTM!
What happens with the messages in DB do they have meta
already but nwaku was discarding the field?
Great question! So it seems that nwaku/waku/waku_core/message/message.nim Lines 20 to 38 in 849d76d
And this is the one being used in the db nwaku/waku/waku_store/common.nim Lines 61 to 63 in 849d76d
But apparently the field wasn't getting filled when sending neither relay, filter nor store messages |
Good to know! but... Let me rephrase my question. Are messages stored in Postgres as blobs? If not, are the messages previously created by nwaku show no value in the DB for that field? Lastly, is this a problem or not? |
Ooh, I think there's actually an issue with that. I think it's not getting saved in the DB at all 😶 nwaku/waku/waku_archive/driver/postgres_driver/postgres_driver.nim Lines 30 to 41 in 849d76d
What I don't understand is it working on the Store test then. Don't we use any DB in those tests? @Ivansete-status |
Yup! We use a database in Postgres tests. |
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.
LGTM! Thanks!
I think we can merge this PR, stating clearly that is missing the proper storage of this field, and submit another PR in the future where we start storing the meta
field in the database.
So in https://github.com/waku-org/nwaku/blob/0dac9f9dd49e2047f3988f4cf0332712028efd0e/tests/wakunode_rest/test_rest_store.nim, are store messages saved in memory? If so, is it done different in a way only for testing? There's something I don't quite understand - I thought store messages were always retrieved from a db |
Yes, this particular test module stores messages in memory.
That represents a test gap that should be filled with a These are the test modules that interact with a local Postgres database: nwaku/tests/all_tests_waku.nim Lines 26 to 28 in 0dac9f9
|
Super, sounds good! Will merge it and implement in a different PR support for db storage of the |
Description
The "meta" field is late addition to the Waku Message specification. It is currently supported in go-waku and not in nwaku. We're implementing its support in this PR.
Changes
meta
field to Relay, Filter and Store Waku MessagesIssue
closes #2214
Note
We also wanted to avoid responding with an error when an extra field is sent in a POST request.
There seems to be an issue in the
nim-json-serialization
dependency that throws an exception when this happens. Will report and track the issue to make sure it gets fixed