-
-
Notifications
You must be signed in to change notification settings - Fork 126
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
Make compatible with EIP 1193, EIP 1102, and EIP 2255 (i.e. LoginPerSite) #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few things you might want to tighten up or discuss, but I don’t see anything worth blocking. Very clean, pleasant to read!
It looks like this needs to be rebased; there are conflicts. |
use yarn update ethereum.send and related methods for EIP 1102 and 1193 compatibility add metadata retrieval refactor utils; use eth-json-rpc-errors implement EIP 1193 events receive accountsChanged notification from background add _metamask.sendBatch, eslint 4.0.0
feb2cca
to
8760cf5
Compare
@Gudahtt fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the first part of my review - I'm still only part-way through.
ea0c434
to
8a9b2cb
Compare
@Gudahtt, just pushed stuff, mostly related to things I marked resolved. Only significant changes worth looking over ought to be the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew 😌 , I think that's everything! This looks good to me. Both of the problems we encountered at the hackathon are fixed now (emitting accountsChanged
upon unlock and accepting a callback to .send
).
The `data` event was emitted from v3.0.0 of the inpage provider whenever a `notification` event was received from MetaMask. This event was removed in #11 because it was thought it was only used internally. Apparently `web3` was relying upon it though. It has been restored exactly as it was in v3.0.0, except now a deprecation warning is shown in the console the first time a listener is added for this event.
The `data` event was emitted from v3.0.0 of the inpage provider whenever a `notification` event was received from MetaMask. This event was removed in #11 because it was thought it was only used internally. Apparently `web3` was relying upon it though. It has been restored exactly as it was in v3.0.0, except now a deprecation warning is shown in the console the first time a listener is added for this event.
An alias has been added to restore the `publicCOnfigStore` property that was renamed in #11. A warning is printed to the console upon first use, explaining that the property is deprecated and will be removed in the future.
An alias has been added to restore the `publicConfigStore` property that was renamed in #11. A warning is printed to the console upon first use, explaining that the property is deprecated and will be removed in the future.
This makes the inpage provider compatible with EIP 1193, EIP 1102, and EIP 2255 (i.e., this branch of the extension). It also sets us up for the December 16 API changes (basically, delete everything marked with
TODO:deprecate:2019-12-16
).Some noteworthy changes:
yarn
ethereum._metamask
proxy moved from extension to hereethereum.send
fully implemented per EIP 11934.0.0
We probably shouldn't merge this and publish the package until the extension's
LoginPerSite
PR is approved, but we definitely review it now.