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

feat: migrates e2e tests to use rest instead of rpc #1856

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

adklempner
Copy link
Member

@adklempner adklempner commented Feb 17, 2024

Problem

nwaku is deprecating the JSON RPC api for communicating with a node. js-waku uses the api when running e2e tests.

Solution

  • Replace all use of JSON RPC API with REST API
  • Update docker settings to run nwaku in tests with REST enabled
  • remove already deprecated endpoints from /private

Notes

@adklempner adklempner requested a review from a team as a code owner February 17, 2024 02:43
Copy link

github-actions bot commented Feb 17, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 30.86 KB (0%) 618 ms (0%) 816 ms (+18.59% 🔺) 1.5 s
Waku Simple Light Node 276.36 KB (0%) 5.6 s (0%) 4.2 s (+9.83% 🔺) 9.7 s
ECIES encryption 27.55 KB (0%) 552 ms (0%) 891 ms (+21.34% 🔺) 1.5 s
Symmetric encryption 26.87 KB (0%) 538 ms (0%) 858 ms (+48.38% 🔺) 1.4 s
DNS discovery 92.51 KB (0%) 1.9 s (0%) 2.5 s (+67.39% 🔺) 4.4 s
Privacy preserving protocols 135.71 KB (0%) 2.8 s (0%) 2.2 s (+12.46% 🔺) 4.9 s
Light protocols 29.27 KB (0%) 586 ms (0%) 737 ms (-35.64% 🔽) 1.4 s
History retrieval protocols 27.64 KB (0%) 553 ms (0%) 744 ms (-39.7% 🔽) 1.3 s
Deterministic Message Hashing 6 KB (0%) 120 ms (0%) 288 ms (-28.85% 🔽) 408 ms

Copy link
Collaborator

@danisharora099 danisharora099 left a comment

Choose a reason for hiding this comment

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

p cool!

@@ -210,13 +208,26 @@ export class ServiceNode {
async peers(): Promise<string[]> {
this.checkProcess();

return this.rpcCall<string[]>("get_waku_v2_admin_v1_peers", []);
return this.restCall<string[]>(
"/admin/v1/peers",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we could add all endpoint strings part of an object/enum/constants

@adklempner adklempner merged commit 26eb7dd into master Feb 19, 2024
11 checks passed
@adklempner adklempner deleted the feat/rpc-to-rest branch February 19, 2024 15:39
Copy link

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

a bit late but left some comments. mainly renamings.

import pRetry from "p-retry";
import portfinder from "portfinder";

import {
Args,
KeyPair,
LogLevel,
MessageRpcQuery,

Choose a reason for hiding this comment

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

Can see a bunch of "rpc" references here and there. Perhaps rename to rest? eg MessageRpcQuery?

@@ -233,9 +244,8 @@ export class ServiceNode {
async messages(
pubsubTopic: string = DefaultPubsubTopic
): Promise<MessageRpcResponse[]> {
pubsubTopic = encodeURIComponent(pubsubTopic);
return this.restCall<MessageRpcResponse[]>(

Choose a reason for hiding this comment

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

same MessageRpcResponse? rename to rest?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: start using REST in e2e tests
3 participants