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

EIP-1102: Opt-in provider access #4703

Merged
merged 32 commits into from
Nov 5, 2018
Merged

EIP-1102: Opt-in provider access #4703

merged 32 commits into from
Nov 5, 2018

Conversation

bitpshr
Copy link
Contributor

@bitpshr bitpshr commented Jul 2, 2018

This pull request includes a fully-updated EIP-1102 implementation:

  • Provider is exposed at window.ethereum
  • Provider is populated with no accounts by default
  • Provider exposes Promise-returning enable method
  • Calling enable shows user approval UI
  • User approval resolves Promise and populates provider with accounts
  • User reject rejects Promise with error message
  • Users have the option to disable privacy mode

Note: This pull request depends on MetaMask/eth-json-rpc-middleware#5.

Required next steps

Example detection code

https://gist.github.com/bitpshr/076b164843f0414077164fe7fe3278d9

Blog post

https://medium.com/metamask/https-medium-com-metamask-breaking-change-injecting-web3-7722797916a8

Example

Cached domain approval

preview

Resolves #3930
Refs #714

@bitpshr bitpshr force-pushed the eip-1102 branch 2 times, most recently from a05b070 to ef27fe9 Compare July 3, 2018 18:07
@danfinlay
Copy link
Contributor

For sites that currently rely on web3:

  • Should we provide a method for users to force-inject web3?
  • Should we issue a blog post about this breaking change? (almost definitely, needs author)
  • Should we give developers a period of time to read the blog post, and add this line of code to their sites?

Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

This looks great. I added a few comments, none very blocking (although restoring e2e tests is important).

The biggest thing for me that we need to do is decisively agree on when to cut over to this. I suspect we need some of the strongest dapp-dev communication we've ever employed for this, because it will instantly break every dapp out there until they add at least one line of JS to their code.

The good news is this one line is so easy. We should tweet it out. We should get people adding it to their dapps ASAP.

"message": "Please review this web3 API request."
},
"web3RequestInfo": {
"message": "The domain listed below is attempting to request access to the web3 API so it can interact with the Ethereum blockchain. Always double check that you're on the correct site before approving web3 access."
Copy link
Contributor

Choose a reason for hiding this comment

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

I bet we could punch this up a bit, we might want to workshop it.

I'm fine with the current draft for now. Might be worth discussing in the design meeting. @cjeria?

One pass:

The domain listed below is requesting access to the web3 API so it can interact with the Ethereum blockchain and view your current account. Always double check that you're on the correct site before approving web3 access.

*/
handleWeb3Request (requestedOrigin) {
const { openPopup } = this.opts
this.memStore.updateState({ pendingWeb3Requests: [requestedOrigin] })
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth moving into its own controller long-term. Non-blocking, this works for MVP.

assert.equal(await tokenBalance.getText(), '100 TST')
})
})
// describe('Token Factory', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer to include the injection request as part of the e2e tests. Maybe @tmashuang could take a pass at this?

@danfinlay
Copy link
Contributor

For consistency:

I notice EIP 1102 names its message 'ETHEREUM_PROVIDER_REQUEST', and here we have 'WEB3_API_REQUEST'.

I think we should maintain consistency between the two.

@danfinlay
Copy link
Contributor

It might also make sense to add our usual titlebar to this window, with the logo and the name "MetaMask", just to help clarify to the user who is asking.

@bitpshr
Copy link
Contributor Author

bitpshr commented Jul 4, 2018

@danfinlay thanks for the review. I took care of all feedback except for adding the titlebar to the approval popup window. Since the popup isn't network-specific, the main function of the titlebar would only be to hold a logo. I agree that a logo would help inform users, but I feel that we should stay consistent with how we also brand transaction approval popups (at least for this PR.)

Updates to this PR:

  • Conforms to the latest updates to EIP-1102
  • Approval logic extracted into standalone controller
  • Approval verbiage updated accordingly

What this PR means to dapps:

  • A provider is injected globally as ETHEREUM_PROVIDER.
  • Dapp reload on network change is no longer implemented.
  • We no longer set defaultAddress, only accounts.

Release comments:

  • Agreed, we should author a blog post including relevant code.
  • Agreed, a (1 month?) grace period seems sufficient and necessary.
  • I think we should also be very proactive with other browser teams.

This is going to be a jarring change for dapp developers. MetaMask and other Ethereum-enabled DOM environments are unique in that it's difficult to version changes: extensions (and desktop applications) are usually evergreen, meaning that once a feature is released, downstream users can't selectively pick up that feature as they could if this were, say, an NPM module that could be versioned as downstream projects see fit. Still, it's an extremely necessary change for security purposes. I feel that a sufficient explanation, hand-holding via example code, a lengthy grace period, and buy-in from other dapp browsers will go a long way with our active developer base. I'm happy to author the relevant blog post(s) and to spread awareness throughout the community.

Edit: Ah, forgot e2e tests. Need to trigger postMessage calls using webdriver so we can re-enable all disabled tests. We need to use executeScript but I need to check the correct syntax and API. Will update shortly.

"message": "Одобрить"
},
"reject": {
"message": "отклонять"
Copy link
Contributor

Choose a reason for hiding this comment

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

Отклонить

"message": "Просмотрите этот запрос API web3."
},
"providerRequestInfo": {
"message": "Домен, указанный ниже, пытается запросить доступ к API-интерфейсу web3, чтобы он мог взаимодействовать с блок-цепочкой Ethereum. Всегда проверяйте, что вы находитесь на правильном сайте, прежде чем одобрять доступ к веб-сайту."
Copy link
Contributor

Choose a reason for hiding this comment

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

Домен, указанный ниже, пытается запросить доступ к API-интерфейсу web3, чтобы он мог взаимодействовать с блокчейном Ethereum. Всегда проверяйте, что вы находитесь на правильном сайте, прежде чем одобрять доступ к веб-сайту.

@bdresser
Copy link
Contributor

bdresser commented Jul 9, 2018

Should we provide a method for users to force-inject web3?

Seems like a good idea - do we need design for this?

@cjeria
Copy link
Contributor

cjeria commented Jul 9, 2018

@bdresser @bitpshr I definitely think this needs design. It shouldn't block progress, but this will ultimately need design.

@bdresser
Copy link
Contributor

bdresser commented Jul 9, 2018

@cjeria opened #4759 to discuss ^

@bitpshr bitpshr force-pushed the eip-1102 branch 9 times, most recently from 43e8583 to fa1192d Compare July 11, 2018 16:25
@bitpshr bitpshr changed the title Initial opt-in web3 access (EIP-1102) EIP-1102: Opt-in provider access Jul 15, 2018
@wbt
Copy link
Contributor

wbt commented Nov 5, 2018

@Grsmto It would also be nice to see the blog post linked to from the console warning updated. The current released version doesn't even have the privacy mode option to ease testing, and the public statements still remain that instead of a phased roll-out it'll be an all-of-a-sudden change applying to everyone by default, to take effect on a now-passed date.

I've already heard some devs say that their dapp is continuing to function fine well after that announced change date, so they must be done and not have anything more to worry about (moving on to the next order of business now...) when the dapp is definitely NOT ready. Updating the blog post with a revised timeline or note that it's on indefinite hold, and noting that once the privacy mode setting is released it'll be off by default for a while to permit dapp changes and testing (hoping that's true!) would be very helpful. Also, devs who did integrate the needed changes early are getting burned by last-minute breaking changes in the breaking change, underscoring the incentive to wait until MetaMask has decided and implemented how this new behavior will work.

@derrickpelletier
Copy link

Bug in latest build:

Metamask state: Locked, Privacy mode OFF

  • run window.ethereum.enable().
  • MM shows locked popup
  • enter password
  • MM shows "connect" screen.

My understanding is that this screen should not be seen if the user has not opted in.
If MM is unlocked, and I run .enable() it does not show the connect screen.

@metamaskbot
Copy link
Collaborator

Builds ready [1bc8ec7]: mascara, chrome, firefox, edge, opera

@bdresser
Copy link
Contributor

bdresser commented Nov 5, 2018

@Grsmto @wbt this feature will be released in 4.18 today or tomorrow morning. Ironing out some last kinks, thanks for your patience!

@derrickpelletier see #5679

@metamaskbot
Copy link
Collaborator

Builds ready [a3ad59f]: mascara, chrome, firefox, edge, opera

@danfinlay
Copy link
Contributor

Thanks for the report, @derrickpelletier, I actually just found this too, discussing/fixing here: #5679

@wbt
Copy link
Contributor

wbt commented Nov 5, 2018

@bdresser When Privacy Mode is added as an option, is it going to be on by default (per description in the public blog post) or off by default (per responses to @hayesgm above) to allow developers to adapt their dapps based on what the actual implementation is, as the adaptation actually needed has apparently been changing quickly within the past few days?

I would highly recommend updating the blog post about what's happening, without waiting for the PR to be merged!

Thanks for supporting better privacy.

@bdresser
Copy link
Contributor

bdresser commented Nov 5, 2018

@wbt the blog post states

On November 2nd, 2018, MetaMask plans to release these changes with “privacy mode” disabled by default. This option will eventually be enabled by default in a future release.

The wording in the subtitle could have been misinterpreted; I've updated it to clarify 😄

@filips123
Copy link
Contributor

Could you make a new blog post when this will be final?

@wbt
Copy link
Contributor

wbt commented Nov 5, 2018

@bdresser As far as what I'm seeing, the subtitle says, emphasis added:

On November 2nd, MetaMask and other dapp browsers will stop universally exposing user accounts. Instead, dapps must request access using a new provider method: provider.enable().

How about:

On or after November 6th, MetaMask will be updated with an off-by-default "privacy mode" that requires an extra step to ask user permissions before accounts are exposed to dapps. Dapp developers should update their code to support this privacy-protecting feature, which will be enabled by default in the future.

@filips123
Copy link
Contributor

@bitpshr What is website doesn't request or user rejects request? Will website have access to network (for example watching blocks, getting other addresses, interacting with contracts) but without user accounts?

@bitpshr
Copy link
Contributor Author

bitpshr commented Nov 25, 2018

Hi @filips123. Yes, your understanding is correct: dapps can only initiate RPC calls that don't require user accounts until account access is approved.

@whymarrh whymarrh mentioned this pull request Aug 14, 2019
4 tasks
@MetaMask MetaMask locked as resolved and limited conversation to collaborators Nov 26, 2019
@whymarrh
Copy link
Contributor

Please open up new issues for related topics and questions

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.

Implement minimal log-in-per-domain on new branch