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

WebSockets #1251

Open
wants to merge 31 commits into
base: develop
Choose a base branch
from
Open

WebSockets #1251

wants to merge 31 commits into from

Conversation

tabaktoni
Copy link
Collaborator

@tabaktoni tabaktoni commented Oct 28, 2024

Motivation and Resolution

  • Implement WebSockets channel
  • Implement WebSocketChannel basic tests

Blocked by:

  • Devnet WS version for CI
  • Final RPC 0.8 Spec for types
  • Final Cleanup

TBD

  • Implement WebSocketChannel advanced tests
  • Implement WebSocketProvider (1:1 with RPC Provider)
  • Add WebSocketProvider to fixtures (so that all test can run over RpcProvider or WsProvider) Devnet

RPC version (if applicable)

0.8

Usage related changes

  • NEW WebSocketChannel

Development related changes

  • NEW WebSocketChannel

Checklist:

  • Performed a self-review of the code
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Linked the issues which this PR resolves
  • Documented the changes in code (API docs will be generated automatically)
  • Updated the tests
  • All tests are passing

@tabaktoni tabaktoni changed the title WIP WebSockerts WebSockerts Oct 30, 2024
@ivpavici ivpavici changed the title WebSockerts WebSockets Oct 30, 2024

// websocket
const webSocketChannel = new WebSocketChannel({
nodeUrl: 'wss://toni.spaceshard.io/rpc/v0_8',
Copy link
Collaborator

Choose a reason for hiding this comment

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

flagging this to not forget to change later :D

__tests__/WebSocketChannel.test.ts Outdated Show resolved Hide resolved
__tests__/WebSocketChannel.test.ts Show resolved Hide resolved

describe('websocket regular endpoints - pathfinder test', () => {
const webSocketChannel = new WebSocketChannel({
nodeUrl: 'wss://toni.spaceshard.io/rpc/v0_8',
Copy link
Collaborator

Choose a reason for hiding this comment

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

flagging here as well for later, also maybe to extract into constant

src/channel/ws_0_8.ts Outdated Show resolved Hide resolved
reject(Error(`error on ${method}, ${message.error}`));
}
}
// console.log(`data from ${method} response`, data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftover?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still need it for testing as this PR can't be 100% complete before RPC 0.8 final release, Pathfinder WS Release and Devnet WS Release.
This is more like PrePR all working and can be checked but will be updated later.

* ```
*/
public async waitForUnsubscription(forSubscriptionId?: number) {
// unsubscribe
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this comment be removed?

}

/**
* subscribe to pending transactions (mempool)
Copy link
Collaborator

Choose a reason for hiding this comment

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

todo add params on this and rest that are missing

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it is auto add on doccs ? @penovicp

src/channel/ws_0_8.ts Show resolved Hide resolved

private onMessageProxy(event: MessageEvent<any>) {
const message: WebSocketEvent = JSON.parse(event.data);
// console.log('onMessage:', data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

leftover?

Copy link
Collaborator

@penovicp penovicp left a comment

Choose a reason for hiding this comment

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

Does the ws dependency get used? It looks like only isows does.

The approach looks good to me.

@tabaktoni
Copy link
Collaborator Author

tabaktoni commented Oct 30, 2024

isows

isows use ws, at least it should use installed one.

@tabaktoni tabaktoni marked this pull request as ready for review November 25, 2024 16:09
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.

3 participants