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.

Sign up for GitHub

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

Jump to bottom

feat: lightpush rest api #2052

Merged
merged 8 commits into from
Sep 22, 2023
Merged

feat: lightpush rest api #2052

merged 8 commits into from
Sep 22, 2023

Conversation

NagyZoltanPeter
Copy link
Contributor

Description

As part of epic #1076 RestApi endpoint for lightpush service is provided here

Changes

New endpoint POST:/lightpush/v1/message is added for wakunode.

How to test

  1. nim c -r ./tests/wakunode_rest/test_rest_lightpush.nim

Issue

#2040

@NagyZoltanPeter
Openapi definition for lightpush rest api
cae9a60 
RestApi Lightpush endpoint implemented
@NagyZoltanPeter NagyZoltanPeter self-assigned this Sep 19, 2023
@NagyZoltanPeter NagyZoltanPeter linked an issue Sep 19, 2023 that may be closed by this pull request
feat: HTTP REST API: lightpush #2040
Closed
@github-actions
Copy link

github-actions bot commented Sep 19, 2023 •
edited

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2052

Built from f3ff2ea

SionoiS
SionoiS approved these changes Sep 19, 2023
View reviewed changes
Copy link
Contributor

@SionoiS SionoiS left a comment

Choose a reason for hiding this comment

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

LGTM

See comments for nitpicks

apps/wakunode2/app.nim Outdated Show resolved Hide resolved
waku/node/rest/lightpush/types.nim
#### Types

type PushRequest* = object
pubsubTopic*: Option[PubSubTopic]
Copy link
Contributor

Choose a reason for hiding this comment

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

Per the RFC the topic is not optional but I guess we are going to update the RFC soon ™️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it was not obvious and I followed node interface lightpushPublish which has as optional due to sharding. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah and one more. I'm not pretty sure if it is ok to push a message with empty pubsubTopic and empty contentTopic, that seems doable now. I'm actually testing it further meanwhile.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok maybe I was confused is PushRequest used for client -> server or is it between rest -> node?

client -> server; should follow the RFC
rest -> node; should allow optional pubsub with mandatory content topic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly the latter. You ask your node (presumably a light node) to make a push request toward a service provider relay node. So as in this case sharding will take place in case no pubsubTopic provided.

waku/node/rest/lightpush/handlers.nim Outdated Show resolved Hide resolved
@NagyZoltanPeter @SionoiS
Update waku/node/rest/lightpush/handlers.nim
cde78d9 
Co-authored-by: Simon-Pierre Vivier <simvivier@status.im>
@NagyZoltanPeter NagyZoltanPeter added the E:REST API service node See https://github.com/waku-org/pm/issues/82 for details label Sep 20, 2023
Ivansete-status
Ivansete-status approved these changes Sep 20, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks! Just a nitpick to enhance the test coverage :)

tests/wakunode_rest/test_rest_lightpush.nim

# Then
check:
response.status == 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

May we have any "400" or "500" test cases as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on those. Just not added yet.

waku/node/rest/lightpush/client.nim Show resolved Hide resolved
waku/node/rest/lightpush/client.nim Show resolved Hide resolved
jm-clius
jm-clius approved these changes Sep 20, 2023
View reviewed changes
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.

So much less controversial than filter. :) LGTM.

@NagyZoltanPeter
Copy link
Contributor Author

So much less controversial than filter. :) LGTM.

Agree. But still while testing negative cases found some validation issue. I will fix it. Probably effects relay rest api validations too, as I was reusing code from there. So it worth testing for.

@NagyZoltanPeter
Copy link
Contributor Author

@Ivansete-status
If you have time to take a look at the last commits that add negative cases and also fix some related issues.

Ivansete-status
Ivansete-status approved these changes Sep 22, 2023
View reviewed changes
Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

LGTM! Beautiful PR 💯 !

Just added a couple of nitpick comments.
Aside from that, before merging we need to understand the underlying issue with the js-waku tests that are failing.

tests/wakunode_rest/test_rest_lightpush.nim Outdated Show resolved Hide resolved
tests/wakunode_rest/test_rest_lightpush.nim Show resolved Hide resolved
@NagyZoltanPeter NagyZoltanPeter merged commit 02a814b into master Sep 22, 2023
9 of 10 checks passed
@NagyZoltanPeter NagyZoltanPeter deleted the feat-1851-http-rest-api-lightpush branch September 22, 2023 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SionoiS SionoiS SionoiS approved these changes

@jm-clius jm-clius jm-clius approved these changes

@Ivansete-status Ivansete-status Ivansete-status approved these changes

@s-tikhomirov s-tikhomirov Awaiting requested review from s-tikhomirov

@gabrielmer gabrielmer Awaiting requested review from gabrielmer

Assignees

@NagyZoltanPeter NagyZoltanPeter

Labels
E:REST API service node See https://github.com/waku-org/pm/issues/82 for details
Projects
Waku
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: HTTP REST API: lightpush
4 participants
@NagyZoltanPeter @SionoiS @jm-clius @Ivansete-status