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

Add Web3 1.0 support #2350

Closed
danfinlay opened this issue Oct 12, 2017 · 98 comments

Comments

@danfinlay
Copy link
Contributor

@danfinlay danfinlay commented Oct 12, 2017

Appears to include:

@kumavis

This comment has been minimized.

Copy link
Member

@kumavis kumavis commented Oct 12, 2017

or just polyfill subscriptions
also: we're already no longer using provider-engine! : )

@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Oct 16, 2017

Is it normal if I cannot find MetaMask provider with Web3 1.0 ?
When I check on Web3.givenProvider || Web3.currentProvider, I always got "null", even after several seconds...

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Oct 16, 2017

Not capitalized. Just web3.currentProvider

@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Oct 16, 2017

Ok, I tried, it works, thanks. It tooks 500ms to find.
Just for you to know, here is what we can read on Web3 1.0 documentation :
"After that you need to create a web3 instance and set a provider. Ethereum supported Browsers like Mist or MetaMask will have a ethereumProvider or web3.currentProvider available, web3.js is setting this one to Web3.givenProvider. If this property is null you need to connect to a remote/local node."

Which is wrong because I can find web3.currentProvider, while Web3.givenProvider is null.

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Oct 16, 2017

Since the Web3 class is the one you're importing from web3 1.0, I think they're instructing you how to assign the provider to their object. They're not telling you what to check. If this was confusing to you, you should open an issue on their documentation.

@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Oct 16, 2017

Hmmm, indeed it makes sense. Maybe my English is guilty here, but if I misunderstood, maybe other will. Thanks anyway, it makes my life much more simple.

@Zanibas Zanibas added the labeled label Oct 18, 2017
@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Nov 14, 2017

Any idea when "web3.eth.subscribe" will be available with MetaMask ?

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Dec 20, 2017

This issue now has a funding of 0.3 ETH (241.43 USD) attached to it.

  • If you would like to work on this issue you can claim it here.
  • If you've completed this issue and want to claim the bounty you can do so here
  • Questions? Get help on the Gitcoin Slack
  • $11752.94 more Funded OSS Work Available at: https://gitcoin.co/explorer
@edkek

This comment has been minimized.

Copy link

@edkek edkek commented Dec 20, 2017

If no one is currently working on this issue, I'd like to tackle it.

What's the best way to start this? Is there a gitter or something similar to discuss details?

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Dec 21, 2017

@edkek you can use the github issue page :) or https://gitter.im/MetaMask/Lobby

@kumavis

This comment has been minimized.

Copy link
Member

@kumavis kumavis commented Dec 21, 2017

here is my roadmap for this feature:

i'd like to separate the notions of websocket transport and subscription support. I think it should be easy enough to provide subscription support on top of http rpc (which will Infura's life easier) with the help of eth-block-tracker.

@awgneo

This comment has been minimized.

Copy link

@awgneo awgneo commented Dec 21, 2017

Will Infura be content with the many polling calls introduced by eth-block-tracker?

@kumavis

This comment has been minimized.

Copy link
Member

@kumavis kumavis commented Dec 21, 2017

@awgneo we already use the block-tracker, it effectively reduces the number of requests made by a dapp via per-block client side caching

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Dec 26, 2017

The funding of 0.3 ETH (221.33 USD) attached has been claimed by @muhammaddava1321.

@muhammaddava1321, please leave a comment to let the funder (@owocki) and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Dec 27, 2017

@muhammaddava1321 please introduce yourself

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Dec 27, 2017

@muhammaddava1321 if i dont hear from you soon, im going to reject your claim and let someone else take this. nothing personal, just want to make sure someone who's making coordinating with everyone a priority has the claim. let us know!

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Dec 29, 2017

i am rejecting this claim due to radio silence from the claimee

@choochootrain

This comment has been minimized.

Copy link

@choochootrain choochootrain commented Dec 29, 2017

@owocki i'd like to claim this! i'll start poking around and get a preliminary changeset out

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Dec 29, 2017

@choochootrain can you please submit a claim on https://gitcoin.co/funding/details?url=https://github.com/MetaMask/metamask-extension/issues/2350 ? thats the source of truth for who has it claimed :)

@gitcoinbot

This comment has been minimized.

Copy link

@gitcoinbot gitcoinbot commented Dec 29, 2017

The funding of 0.3 ETH (221.28 USD) attached has been claimed by @choochootrain.

@choochootrain, please leave a comment to let the funder (@owocki) and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.

@choochootrain

This comment has been minimized.

Copy link

@choochootrain choochootrain commented Dec 31, 2017

here's my mental model of this so far: i have to read up on the new web3 surface area and work backwards to see what changes need to be made to the metamask provider (this is json-rpc-engine with the eth-json middleware?), which uses eth-block-tracker under the hood. @kumavis is this accurate?

i'll dig into this further on monday - happy new year!

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Jan 2, 2018

@choochootrain ill let the metamask folks comment on the scope... (but they might be busy catching up from the holidays)

looking forward to seeing this done :)

@alexvandesande

This comment has been minimized.

Copy link

@alexvandesande alexvandesande commented Mar 16, 2018

@lazaridiscom @danfinlay it just means that we won't be injecting it by default, but using a web provider to allow the apps themselves choose their own library, given that more than one exists. It's still our library of choice and we use it ourselves, just giving users more options.

@chapati23

This comment has been minimized.

Copy link

@chapati23 chapati23 commented Mar 18, 2018

@danfinlay @lazaridiscom Would raising the bounty for this issue help move it forward quicker? If yes, we at Brickblock would be happy to throw some ETH into the hat.

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Mar 18, 2018

@chapati23

If the core team completes this feature, we’ll reject all bounties for it, since we’re already salaried, but a bounty could encourage an external community member to try to implement this sooner.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 19, 2018

@alexvandesande , thank yo for the info. You should possibly clarify this in your documentation, because the announcement reads like "Deprecated". Possibly the by default in won't inject web3 object by default implies that devs can still opt to inject/use it. But this has quite a reduced visibility (at least it had for me).

@chapati23 raising the bounty should help, but i've the feeling that providing a concise task-description will help even more!

@danfinlay , @kumavis - I really think its time to create a task description, with the minimum estimated steps needed to fulfill this.

Would possibly be enough to

Could(!) look like this (to invite people to take a look at this issue, to possibly do one subtask)

Task

MetaMask should be compatible with web3 1.0. This quick overview provides the estimated necessary steps:

Solution Path A (by kumvaris)

source: #2350 (comment)

Issue: Add websocket subprovider - MetaMask/web3-provider-engine/issues/189

Comments by benjamincburns: #2350 (comment)

Solution Path B (by benjamincburns)

info from end of: #2350 (comment)
(easier to implement, but without the full benefits of websockets. Would still enable use of web3 1.0)

Base work:

Solution Path C

This would be my process/path:

  • Ignore everything within this thread
  • Provide standard test-cases #3618
  • Start development
  • Once your stuck, review the solution-paths / contents of this issue to get hints

Solution Path D (by yourself)

  • Take your own path.
@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Mar 19, 2018

two notes:

  1. it might make sense to move the issue scope over to a new, clean, issue.
  2. though i broadly defer to the metamask team on their specific policy, i disagree that it's on the metamask team to think through the solution path for the bounty hunters. if they are going to think through the implementation for you, then they might as well just do it themselves. you could generate a pretty plausible implementation path by just looking at whats changed in web3.js 1.0 and grepping the metamask codebase for what needs to be updated & reworked.
@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Mar 19, 2018

@owocki I think we are beyond this kind of consideration. This is a core feature for many Dapps that rely on web3 1.0.0 and the bounty is so small (0.3Eth) that nobody will take the time needed to solve this issue.
I think that the real problem is that few people know in depth web3-provider-engine and among them fewer have the time to work on it. A not-so-good dapp developer can earn 0.3Eth per hour of work on freelance mission, why would anyone spend some time to solve this problem except if he is facing it in his job ? Therefore if someone is going to solve this issue, it is absolutely not for the bounty !
I, for example, would be glad to work on that, but my knowledge of this project is too low for that. So I think that if the metamask team hasn't the time to work on that issue, we should at least know what to do to solve it ourselves.

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Mar 19, 2018

the bounty is so small (0.3Eth) that nobody will take the time needed to solve this issue.

this issue was funded back when ETH was 2x the price it is now.

nonetheless, your point about the amount of funding is still valid. i am happy to increase the funding for the issue if @danfinlay and @kumavis want to give it another go (perhaps on another ticket)

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 19, 2018

we should at least know what to do to solve it ourselves.

@GrandSchtroumpf , the solution-paths are already given, mostly in comments within this issue. I've update my comment above and collected quickly the relevant info of kumvaris path (and added a personal one) : #2350 (comment)

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 19, 2018

For the case anyone wants to make a tiny step: #3618

@guix77

This comment has been minimized.

Copy link

@guix77 guix77 commented Mar 19, 2018

I totally agree that the bounty is very small. We're talking about a major door into the Ethereum world, here, not a super fancy awesome useless feature.

Furthermore, maybe no one cares because it's clear that the injection of Metamask is deprecated, because changing its default version would break most dApps and because we can always use currentProvider and replace the whole injected Web3.JS

window.web3 = new Web3(window.web3.currentProvider);

@GrandSchtroumpf

This comment has been minimized.

Copy link

@GrandSchtroumpf GrandSchtroumpf commented Mar 19, 2018

@lazaridiscom web3 uses this version of websocket when websocket is not available on the window or if we are on a server:
https://github.com/frozeman/WebSocket-Node
Is it ok to use this module in the providerFromEngine to handle server-push and send ?

@ryan-rowland

This comment has been minimized.

Copy link
Contributor

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

The wheels have been spinning on this one for months... It seems like the common pattern is:

  • Ooh money! I'll give it a shot.
  • Wow that's a lot of work.
  • I give up / I'm too busy / etc...
  • Progress (and time) is wasted due to all-or-nothing bounty
  • Repeat

We have a general idea of the steps that need to be taken to get this task knocked out. Can we get a rough consensus on the steps required, and then split those sub-tasks out into their own tasks so this can be tackled in a more gradual and granular fashion?

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 20, 2018

@GrandSchtroumpf - In this issue here, I just apply the standard-procedure in an abstract manner. With the quite low amount of domain-knowledge I have, I would answer you: yes. But @kumavis and others should know better.

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Mar 20, 2018

Sorry for the low bounty, and the low response rate. The core team has been very busy with other issues at the moment, and we've always planned to do this eventually, and we might just do it ourselves soon. The community doesn't need to feel responsible for completing this.

That said, since this issue clearly needs a re-focusing, I will now write a brief summary of what this issue involves.

The Current State of MetaMask's Provider

MetaMask's most popular open source component to date has by far been web3-provider-engine. This express inspired middleware architecture allowed @kumavis to compose MetaMask's custom provider logic into a stack of "subproviders" that handle different requests, or portions of requests, and then either pass the mutated request down the stack, or reply directly.

This engine was designed for the original Ethereum JSON RPC spec. Since then, there has also emerged a websocket API that includes subscriptions called the RPC Pub Sub API. This new API allowed push subscriptions, and Web3 1.0 to exist.

Currently, MetaMask still uses polling & HTTP to get all of its data, and so the most obvious way to add Web3 1.0 support is to allow us to move onto a websocket based infrastructure.

A Problem With That

Over time, it has become increasingly clear that some of the ethereum-opinionated aspects of provider-engine were actually getting in the way of creating a good provider, and things were being hacked on top of it.

For example, our cache-subprovider needs block awareness to know when to clear its cache, and so block tracking was built into provider-engine, and bit by bit, these pieces of opinionated architecture has made provider-engine a bit harder to work with.

Furthermore, since provider-engine was never designed for handling subscriptions, since it's a middleware for responding to requests, it has no natural way of representing a websocket connection.

The Websocket Solution

Since much of the difficulty of adding a new connection type is that provider-engine encompasses all the middleware, and makes them hard to interact with, we're moving MetaMask over to json-rpc-engine. You'll see us use it a few places in MetaMask instead of provider-engine. This is basically the same general product as provider-engine, except with all the ethereum-opinionated aspects torn out, so they can more easily be accessed by external consumers, and so some types of features (like subscriptions) can more easily be added.

By removing the specialized logic from the engine itself, that kind of wiring can be left to the subproviders themselves. For example, the new arrangement might look a little more manual, but it allows much more diversity in subproviders (pseudocode below):

var engine = new JsonRpcEngine()

var blockTracker = new BlockTrackerSubprovider()
var cache = new CacheSubprovider({ blockTracker })

engine.add(blockTracker)
engine.add(cache)

engine.start()

Moving to json-rpc-engine means every subprovider in the zero subprovider needs to be re-written or modified for json-rpc-engine, so its functionality can be swapped in place. Many of these subproviders have already been written here:
https://github.com/MetaMask/eth-json-rpc-middleware

In particular, the final subprovider that will need to be written for this issue is the websocket subprovider. Once we have this all ready, we'll probably need to compose the zero provider which is currently only imported, because only by composing the subproviders will we have access to the block-tracker's events. Or we come up with a way of accessing specific subproviders from the parent controller.

Anyways, this proposal is hardly trivial, as provider-engine is basically the heart of MetaMask, and so this is something like MetaMask heart surgery. We've already seen some regressions related to moving provider-engine over to json-rpc-engine, and so this needs to be done slowly and carefully, and json-rpc-engine needs to get really solid really fast.

An MVP Solution

While everything above has been accurate and true, there is a way we can provide web3 1.0 support a bit faster, but without the full performance benefits of websockets.

This is to polyfill the subscription API, the way Ben Burns did here already.

If that subprovider got a little polish and QA, it's possible it would be ready to go very soon, and so someone looking for the shortest path to letting their dapp use Web3 1.0 might just take that on, and I think it would still win the bounty for this issue, since this issue is really about web3 1.0 support, not websockets themselves.

The subscription data event would still need to be exposed in some way, and maybe a new module could be used to wrap provider-enigne to expand its API to include the full PUB SUB API. Figuring out those details would still be part of this "MVP".

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Mar 20, 2018

@danfinlay thanks for the thoughtful re-focusing comment. if its okay to you ill move your latest comment over to a new github issue and issue a new bounty for the MVP scope.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 20, 2018

Updated Summary: #2350 (comment)

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Mar 20, 2018

@danfinlay here is a new issue: #3639

i move we

  1. close down this issue
  2. put a bounty of 1 ETH on that issue.

does that sound good to the group?

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Mar 21, 2018

Either that one or this one, @owocki.

I will do more work to clean up this story tomorrow. I might close this thread in favor of these new, cleaner issues with links to important comments.

@danfinlay

This comment has been minimized.

Copy link
Contributor Author

@danfinlay danfinlay commented Mar 22, 2018

To help clarify what would be involved in adding websocket support, I've enhanced the issue description here:

MetaMask/web3-provider-engine#189

@owocki

This comment has been minimized.

Copy link

@owocki owocki commented Mar 22, 2018

just bountied #3642 with 1 ETH

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Mar 23, 2018

I might close this thread

@danfinlay , yeah, before it reaches 100 comments!

@danfinlay danfinlay closed this Mar 24, 2018
@kyriediculous

This comment has been minimized.

Copy link

@kyriediculous kyriediculous commented Apr 8, 2018

I'm sad that the core team doesn't see this as a major issue :(

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Apr 9, 2018

Well, @kyriediculous, don't be sad. This has quite a priority and there is ongoing work, see e.g. #3642

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Apr 9, 2018

@danfinlay , as this issue has follow-up's set, you could lock it.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented Apr 11, 2018

@ all

It looks that there is a first working draft of web3 1.0 support for testing available, see:

#3642 (comment)

@MetaMask MetaMask locked as off topic and limited conversation to collaborators Apr 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
You can’t perform that action at this time.