Skip to content

Implement minimal EIP 1102 support#37

Merged
czxtm merged 1 commit into
czxtm:masterfrom
danfinlay:Minimal-1102
Nov 15, 2018
Merged

Implement minimal EIP 1102 support#37
czxtm merged 1 commit into
czxtm:masterfrom
danfinlay:Minimal-1102

Conversation

@danfinlay
Copy link
Copy Markdown
Contributor

@danfinlay danfinlay commented Nov 15, 2018

For some reason some tests have broken, and I can't tell why. Opening a PR so someone else could take a look maybe. I'll try to chip away at this as time allows.

This minimal version of EIP 1102 support simply requests user login when the component loads.

Another, more nuanced approach would be to show a login widget that allows users to deliberately login if they want, or to remain anonymous and continue using the page.

I have a basic implementation of that on another branch.

Fixes #35

For some reason some tests have broken, and I can't tell why.
@czxtm
Copy link
Copy Markdown
Owner

czxtm commented Nov 15, 2018

Awesome, I'll look into the test cases.

On another note, reading the EIP, I noticed that Provider#enable is already deprecated in favor of eth_requestAccounts RPC call - should this also be implemented? See https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1102.md#providerenable-deprecated

@czxtm
Copy link
Copy Markdown
Owner

czxtm commented Nov 15, 2018

Hey @danfinlay ,

Maybe why the test broke is because the provider relies on both a synchronous and asynchronous method for getting the current accounts. It tries to populate in a synchronous manner and falls back to asynchronous.

My understanding from the EIP is that accounts will no longer be available by default synchronously, which is fine. My question is, after the user has authorized a dApp, does this spec specify if the account is stored in an accessible place in memory afterwards.

For example, in web3 v1, once the user has signed in I can access web3.eth.accounts.wallet[0]. Will this still be the case in metamask with currentProvider.enable()?

@czxtm
Copy link
Copy Markdown
Owner

czxtm commented Nov 15, 2018

Actually nevermind. I think it's just weird async issues in enzyme

@czxtm czxtm merged commit 35bbbfc into czxtm:master Nov 15, 2018
@czxtm
Copy link
Copy Markdown
Owner

czxtm commented Nov 15, 2018

Fixed the tests. thank you

@czxtm
Copy link
Copy Markdown
Owner

czxtm commented Nov 15, 2018

Published on npm: react-web3@1.2.0

@danfinlay danfinlay deleted the Minimal-1102 branch December 17, 2018 19:52
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.

2 participants