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

chore: remove json rpc #2416

Merged
merged 2 commits into from
Feb 29, 2024
Merged

chore: remove json rpc #2416

merged 2 commits into from
Feb 29, 2024

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Feb 9, 2024

Description

⚠️ Completely removes the json-rpc api, removing also the following cli flags:

  • rpc
  • rpcAddress
  • rpcPort
  • rpcAdmin
  • rpcPrivate

From now on, to interact with nwaku refer to the REST API.
Closes #2053

Copy link

github-actions bot commented Feb 9, 2024

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

Copy link

github-actions bot commented Feb 9, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2416

Built from 7d77d2d

@Ivansete-status
Copy link
Collaborator

Thanks for this!

However, IMO, I think we first need to have a mature REST and I'm afraid we are not in that stage yet.
On the other hand, we will need to coordinate and inform infra about that deprecation.

IMO, the best strategy would be:

  1. Complete REST and make sure the documentation and all endpoints work perfectly.
  2. Inform infra about our intention of deprecating json-rpc
  3. Merge this PR

@alrevuelta
Copy link
Contributor Author

Complete REST and make sure the documentation and all endpoints work perfectly.

@Ivansete-status mind being more actionable? whats missing in the rest API?

Inform infra about our intention of deprecating json-rpc

done

@Ivansete-status
Copy link
Collaborator

@Ivansete-status mind being more actionable? whats missing in the rest API?

Sure! These are the most relevant outstanding points, without considering bug issues.

Private Rest API - #2227
Reorganize RestApi specs for live documentation - #2120
Reorganize rest-api types - #2121
Allow to configure RLN via the REST API - #1985

As a reminder, REST tasks were deprioritized in favor of others.

@chair28980 - this is something that we could comment on in our next pm meeting and give more priority to that matter.

@alrevuelta
Copy link
Contributor Author

alrevuelta commented Feb 12, 2024

Private Rest API - #2227
Reorganize RestApi specs for live documentation - #2120
Reorganize rest-api types - #2121
Allow to configure RLN via the REST API - #1985

In order:

  • Lets move the conversations of this to feat: HTTP REST API: Private API #2227
  • Don't see how that's a blocker, isn't enough with this?
  • Seems like a refactor to me, why blocker?
  • No RLN endpoints are present neither in the json rpc, why would that be a blocker?

@alrevuelta
Copy link
Contributor Author

Relevant
waku-org/pm#125 (comment)

@Ivansete-status
Copy link
Collaborator

  • Lets move the conversations of this to feat: HTTP REST API: Private API #2227
  • Don't see how that's a blocker, isn't enough with this?
  • Seems like a refactor to me, why blocker?
  • No RLN endpoints are present neither in the json rpc, why would that be a blocker?

in order:

1.- Ok!

2.- 3.- 4.-
I agree that these points seem not to be blockers at all but I'd like someone (me or another nwaku engineer) to invest some time in ensuring:

  • All endpoints work as expected and are aligned with the current JSON-RPC.
  • The OpenAPI docs are accurate.

On the other hand, we cannot deprecate JSON-RPC abruptly without proper notification to counterparties. i.e. we need to inform that we are starting to deprecate the JSON-RPC and keep this notification for at least two consecutive releases. In other words, we need to start informing other teams/projects to start using REST and once we make sure (assuming 2 releases is enough time) they can work well with REST, then it is fine to decommission JSON-RPC.

And btw, thanks again @alrevuelta for this initiative! I think is very interesting to have it completed!


@chair28980 @gabrielmer @NagyZoltanPeter - I've added a point for tomorrow's PM session to discuss it

@NagyZoltanPeter
Copy link
Contributor

NagyZoltanPeter commented Feb 13, 2024

Private Rest API - #2227

This one is missing although it was not part of json-rpc so nothing outstanding.

Reorganize RestApi specs for live documentation - #2120

With new targets of go-waku, it will be easier, due to former msalignment between the two implementations.
Reorg is done in a separate repo: https://github.com/waku-org/waku-rest-api
We need to remove specs from nwaku and go-waku to let live them in one place.

Reorganize rest-api types - #2121

Yes, this is aiming a small refactoring.

Allow to configure RLN via the REST API - #1985

This one also new stuff.

In order:

  • Lets move the conversations of this to feat: HTTP REST API: Private API #2227
  • Don't see how that's a blocker, isn't enough with this?
  • Seems like a refactor to me, why blocker?
  • No RLN endpoints are present neither in the json rpc, why would that be a blocker?

I see nothing blocking here in order to remove json-rpc.

@NagyZoltanPeter
Copy link
Contributor

Btw:
rpcPrivate
flag is not used anywhere.

@alrevuelta
Copy link
Contributor Author

As agreed, we will announce the deprecation in the current release 0.25.0 and completely remove support in 0.26.0. This PR is blocked till then.

@alrevuelta alrevuelta added the blocked This issue is blocked by some other work label Feb 13, 2024
Copy link
Contributor

@jakubgs jakubgs left a comment

Choose a reason for hiding this comment

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

We will have to remove the rpc.sh script from the Ansible role:
https://github.com/status-im/infra-role-nim-waku/blob/master/templates/rpc.sh.j2

Possibly replace it with something similar that would allow us to get some common types of info via REST API, like version or number of peers, but that's trivial.

Also, the docs for REST API are very sparse, for example I see no info about /admin/v1/peers endpoint:
https://github.com/waku-org/nwaku/blob/master/docs/api/rest-api.md

I see something here:
https://github.com/waku-org/nwaku/blob/master/waku/waku_api/rest/admin/openapi.yaml

But I see no info about endpoint that returns node version.

@NagyZoltanPeter
Copy link
Contributor

NagyZoltanPeter commented Feb 13, 2024

@jakubgs please note that API docs had moved and living live here: https://waku-org.github.io/waku-rest-api/#get-/debug/v1/info

Version info:

curl -X GET "http://127.0.0.1:8645/debug/v1/version" -H "accept: text/plain"

@jakubgs
Copy link
Contributor

jakubgs commented Feb 13, 2024

I see, didn't know that. But if the docs exist there shouldn't the obsolete/outdated one in docs/api be removed?

Also, shouldn't we publish https://waku-org.github.io/waku-rest-api/ under https://rest-api.waku.org/ or https://api.waku.org/?

jakubgs added a commit to status-im/infra-role-nim-waku that referenced this pull request Feb 13, 2024
Related to removal of RPC API support in:
waku-org/nwaku#2416

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@jakubgs
Copy link
Contributor

jakubgs commented Feb 13, 2024

jakubgs added a commit to status-im/infra-role-nim-waku that referenced this pull request Feb 13, 2024
Related to removal of RPC API support in:
waku-org/nwaku#2416

Signed-off-by: Jakub Sokołowski <jakub@status.im>
jakubgs added a commit to status-im/infra-role-nim-waku that referenced this pull request Feb 13, 2024
Related to removal of RPC API support in:
waku-org/nwaku#2416

Signed-off-by: Jakub Sokołowski <jakub@status.im>
jakubgs added a commit to status-im/infra-role-nim-waku that referenced this pull request Feb 13, 2024
Related to removal of RPC API support in:
waku-org/nwaku#2416

Signed-off-by: Jakub Sokołowski <jakub@status.im>
@NagyZoltanPeter
Copy link
Contributor

I see, didn't know that. But if the docs exist there shouldn't the obsolete/outdated one in docs/api be removed?

Also, shouldn't we publish https://waku-org.github.io/waku-rest-api/ under https://rest-api.waku.org/ or https://api.waku.org/?

Yes, I agree we will need to find a more straightforward place for it instead of a gh pages hosted one.
But notice REST API works got de-prio in favor of other task.
For the doc reference page we will need to cross check any difference in order to remove specs from nwaku / go-waku repos.

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

LGTM from purely technical point of view!
@gabrielmer to watch out his conflicting work on refactoring wakunode setup.

Also to mention we need to update docs.waku.org as soon as it is released!
cc: @LordGhostX

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.

Finally we're removing it! LGTM

@LordGhostX
Copy link
Contributor

@NagyZoltanPeter, thanks for the ping 🫡

I removed references to the JSON-RPC API a long time ago in preference for the REST API. I'll take down the config section once it's released: https://docs.waku.org/guides/nwaku/config-options#json-rpc-config

@alrevuelta
Copy link
Contributor Author

With status-im/infra-role-nim-waku#25 merged and js-waku-node CI job ✅ , merging. js-waku-node-optional is a known flaky one.

@alrevuelta alrevuelta merged commit c994ee0 into master Feb 29, 2024
9 of 10 checks passed
@alrevuelta alrevuelta deleted the remove-rpc branch February 29, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue is blocked by some other work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: Remove json-rpc
6 participants