Closed danfinlay closed 6 years ago
Relevant notes here: https://github.com/MetaMask/metamask-extension/issues/2350#issuecomment-353437246
id like to differentiate between:
@danfinlay & @kumavis I'm working on websockets support for ganache right now, which included updating to the latest provider engine. I'd be all kinds of happy to package up the subscription subprovider I'm writing as a PR.
I have it mostly written already, but I need to do some cleanup/refactoring. Do you have any strong preferences for the way it should be done? At the moment it holds a reference to the FilterSubprovider
but I'm likely going to nix that and extend that subprovider instead.
At present it works by exposing an EventEmitter
interface, which emits subscription updates out on the data
event. This will work fine for a websocket provider, but our server in ganache will need to handle routing the subscriptions accordingly based on their ID, cleaning them up on disconnect, etc.
I’d defer to @kumavis, but the end subproviders should just pass through requests, allowing server-side filter management, unless a middleware like filter subprovider were before it.
Isn't this already tackled by #207?
@pablasso I don't think so. While #207 adds a websocket-based block-subscription subprovider, it only uses websockets for block subscription.
This issue would be for a full websocket subprovider, fully replacing all of the roles currently performed by the fetch
subprovider.
I'm working on this.
Looking closer at this, it seems there's more to do than just replace FetchProvider with a WebsocketProvider. The provider stack seems to be built around a polling mechanism (using eth_getBlockByNumber
). To correctly provide websocket support, the information we're receiving from the server should be expected via subscription rather than polling. This means writing a new subscriptions
subprovider at least. I believe filters and other subproviders may also be built around this assumption of polling.
@ryan-rowland You are correct, sorry for not including this in the original post (will update it):
Since provider-engine
is reliant on block-polling, there has been an effort to re-write provider-engine
without these ethereum-opinionated architectures.
Enter: json-rpc-engine. Basically the same as provider-engine, but without the ethereum-opinionated portions like block-polling.
It would be more correct to write the websocket subprovider for that, and then move MetaMask over to it from provider-engine
.
I'm hoping @kumavis can come in and shed additional light on this, since he's the one who's been re-writing provider-engine
as json-rpc-engine
.
@ryan-rowland you added WebsocketSubprovider (thanks!) in https://github.com/MetaMask/provider-engine/pull/227 but did not actually setup forwarding subscription responses (server-sent json rpc 'notifications') on the provider-engine. I did the final steps, and I'm releasing in 14.0.0
Fixed in 14.0.0
Thanks for following up @kumavis ! Glad to see this issue moving forward.
@lazaridiscom This adds the logic to talk to the websocket gateways, so it's a step forward. The issue I ran into at this point was getting disconnected from the gateway because the provider was still using polling logic rather than subscription. It seems like @kumavis may have updated the logic to use subscriptions, in which case I'd say this is a big step toward web3 1.0 support.
I'll let @kumavis speak to it in more detail as he's been following up on it.
@lazaridiscom @ryan-rowland
https://github.com/matthewlilley/metamask-extension/commit/6827fdf66581c0ecfa57fab9572f135f5231a681
This is the simplest solution I came up with for providing Web3 1.0+ subscription support to MetaMask, and it's inpage provider. It works. Needs review. @danfinlay @kumavis
@matthewlilley I left comments on your commit. Open a PR next time please. 😄
@lazaridiscom Sorry I won't be a position to test any time this week.
@ryan-rowland Thank you Ryan.
Pull request https://github.com/MetaMask/metamask-extension/pull/4001/commits/65d907f85339e3154e8e0aae01cfdeb5ce94a09d
@matthewlilley You tricked me again! That's a commit, not a PR.
@ryan-rowland https://github.com/MetaMask/metamask-extension/pull/4047
So... does Metamask now support connecting to custom WS-RPC? If I select Custom RPC and enter New RPC URL that starts with ws://, I get error "URIs require the appropriate HTTP/HTTPS prefix."
@jtakalai no, seems to not yet seem the case.
I guess the issue you need to track is https://github.com/MetaMask/metamask-extension/issues/1645
Worth noting that MetaMask has converted to a different module we wrote, json-rpc-engine, which can accomplish the same goals in combination with eth-json-rpc-middleware.
It is less coupled to the ethereum RPC, and allows a more modular composition of features.
Yes, a subprovider was added to provider-engine, but it was never added to MetaMask for a few reasons. Moving off provider-engine was more important to allowing performance than websockets (allowed better block-tracker pausing and cache busting by decoupling them from the main engine).
That work could potentially be ported to json-rpc-engine
, but I'm not sure what else might be needed to get that to work. At the very least, this file would need to be moved from eth-json-rpc-infura
to a websocket equivalent, but I think there would also be additional work to allow our current filter-middleware
(which polyfills subscription behavior with polling) to be deactivated when connected to a websocket source.
There might be other implications, I would ask @kumavis to chime in.
This would allow push updates, and compatibility with Web3 1.0.
Updated March 22, 2018 to help potential new contributors make sense of this issue:
Provider engine is a system for composing middleware objects (which we call
subproviders
) into a potentially complex system for managing the response to a given request object.ProviderEngine itself is also an Ethereum Provider, as in web3.currentProvider, so once composed with middleware, it exposes the standard
sendAsync()
method for Ethereum developers to make requests of the JSON RPC API.When calling
providerEngine.sendAsync(yourOpts, callbackFunction)
, those options you pass in get passed from one middleware object to the next. You can see how here.As you can see, each provider is passed the same options object, potentially mutating it, and with a pair of callback functions to either end the response immediately, or to pass the options on to the next provider.
To operate as a
subprovider
, an object only needs to expose afunction handleRequest (options, next, end)
, as you can see here in the fetch subprovider.In that function, the
subprovider
can mutate theoptions
freely, and then either call thenext()
orend()
functions.next()
is only used by true "middleware" subproviders, to pass the options to the next subprovider. That function will not be needed for this feature.The
end()
function represents the result that will be returned to theProviderEngine
consumer, and should follow the JavaScript API specification, including its JSON-RPC style error format.The Fetch Subprovider is how MetaMask currently talks to an Ethereum node. It uses the
fetch
API, which is pure HTTP, to make requests of whatever RPC it is pointed at.This issue is to create a similar subprovider, but one that uses Websockets instead of HTTP, and uses the Geth Websocket API instead of the usual HTTP-based JSON RPC API. The request/response format should otherwise be basically identical.
Further Goals
There are other goals that are often associated with this one, which can make it seem more complicated, but are actually separate deliverables.
Websocket Block Tracker
Right now the way MetaMask keeps track of the current block is also via HTTP, via the eth-block-tracker module. We could definitely also improve performance by moving that over to websockets, or making a websocket version of it.
Websocket Based Subscriptions
While #207 adds a subprovider for providing the
providerEngine.subscribe()
API, it does this with polling under the hood. This API would be much more performant if its functionality were moved into the websocket subprovider.