-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
size-limit report 📦
|
93b27f0
to
6009af7
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.
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", |
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.
nit: we could add all endpoint strings part of an object/enum/constants
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.
a bit late but left some comments. mainly renamings.
import pRetry from "p-retry"; | ||
import portfinder from "portfinder"; | ||
|
||
import { | ||
Args, | ||
KeyPair, | ||
LogLevel, | ||
MessageRpcQuery, |
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.
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[]>( |
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.
same MessageRpcResponse
? rename to rest?
Problem
nwaku is deprecating the JSON RPC api for communicating with a node. js-waku uses the api when running e2e tests.
Solution
/private
Notes