Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Issue 189 | Add WebsocketProvider #227

Closed

Conversation

ryan-rowland
Copy link

@ryan-rowland ryan-rowland commented Mar 23, 2018

#189

Happy path implementation. Specific errors and error codes haven't been observed or considered. A cheat sheet on which errors to expect and how to handle them would be helpful.

Works against test networks. Mainnet seems to want to disconnect the websocket almost immediately on each connection, causing some data loss. We should hold off on merging until we figure out why this is.

Test by running node example.js, which runs both FetchProvider and WebsocketProvider side by side to compare results.

@ryan-rowland
Copy link
Author

@danfinlay Looks like there may be an issue with wss://mainnet.infura.io/ws. All of the other endpoints, including wss://mainnet.infura.io/_ws, work fine. Could I get a review based on the assumption the main endpoint is broken and will be updated?

PS: looks like circleCI is currently broken for this repo, causing a few approved PRs to not be merged.

return extend({
// defaults
id: getRandomId(),
jsonrpc: '2.0',
Copy link
Member

Choose a reason for hiding this comment

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

does the ws endpoint not accept this key?

Copy link
Author

Choose a reason for hiding this comment

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

It does, but looking at the RPC over WebSocket spec I saw this was missing from requests and figured it better to be exact.

@kumavis
Copy link
Member

kumavis commented Apr 10, 2018

Added the websocket provider from this pull request, thank you

@kumavis kumavis closed this Apr 10, 2018
@ghost
Copy link

ghost commented Apr 10, 2018

@kumavis , I tried to spot the relevant commit, but failed. Which is it?

@ryan-rowland
Copy link
Author

@lazaridiscom Looks like it's here 5861341

And there are a few follow-up commits making some adjustments.

Thanks for the update @kumavis

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

Successfully merging this pull request may close these issues.

2 participants