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

Refactor for latest Metamask changes #201

Merged
merged 10 commits into from
Feb 20, 2021
Merged

Conversation

xaler5
Copy link
Collaborator

@xaler5 xaler5 commented Feb 18, 2021

Resolves #198
Resolves #200
and partially #188

src/index.js Outdated Show resolved Hide resolved
src/utils/ethutil.js Outdated Show resolved Hide resolved
@wbnns
Copy link

wbnns commented Feb 18, 2021

There are a couple of MetaMask deprecation notices in the console; not sure if it is in scope for this PR, so just mentioning in case helpful:

inpage.js:1  MetaMask: The event 'close' is deprecated and may be removed in the future. Please use 'disconnect' instead.
For more information, see: https://eips.ethereum.org/EIPS/eip-1193#disconnect
inpage.js:1 MetaMask: The event 'data' is deprecated and will be removed in the future. Use 'message' instead.
For more information, see: https://eips.ethereum.org/EIPS/eip-1193#message

Related: #188

@wbnns
Copy link

wbnns commented Feb 19, 2021

@xaler5

Maybe add a note that this "Resolves #198" which will cause GitHub to automatically close it once this is merged.

Edit: Can also mention that this "Resolves #200" since web3.js gets upgraded in this PR.

@xaler5
Copy link
Collaborator Author

xaler5 commented Feb 19, 2021

There are a couple of MetaMask deprecation notices in the console; not sure if it is in scope for this PR, so just mentioning in case helpful:

inpage.js:1  MetaMask: The event 'close' is deprecated and may be removed in the future. Please use 'disconnect' instead.
For more information, see: https://eips.ethereum.org/EIPS/eip-1193#disconnect
inpage.js:1 MetaMask: The event 'data' is deprecated and will be removed in the future. Use 'message' instead.
For more information, see: https://eips.ethereum.org/EIPS/eip-1193#message

Related: #188

Yes, apparently there's no way right now to get rid of them. They are given by the Web3. Apparently, because of backward compatibility, they decided to not remove them for the time being. You can read more about it here and here. You can still see them being used

@wbnns
Copy link

wbnns commented Feb 19, 2021

@xaler5

I deleted my last comment - apologies for the false notification - I switched off Rinkeby in the middle of testing and forgot to switch back.

Copy link
Contributor

@ylv-io ylv-io left a comment

Choose a reason for hiding this comment

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

Hey @xaler5 , thanks for fixing this so fast. Hope it gets pushed to prod fast and we all can enjoy Ethernaut again!

@@ -47,14 +47,12 @@ window.addEventListener('load', async() => {
if (window.ethereum) {
window.web3 = new constants.Web3(window.ethereum)
try {
await window.ethereum.enable()
await window.ethereum.request({method: `eth_requestAccounts`})
Copy link
Contributor

Choose a reason for hiding this comment

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

Metamask docs:

You should never initiate a connection request on page load.

Though, it should be address as a separate issue. Let's bring it back ASAP.

@frangio
Copy link
Contributor

frangio commented Feb 20, 2021

Thank you all for working on this and reviewing it!

@frangio frangio merged commit f48c7f4 into master Feb 20, 2021
@frangio frangio deleted the upgrade-to-latest-metamask branch February 20, 2021 20:33
CeliktepeMurat pushed a commit to CeliktepeMurat/ethernaut that referenced this pull request Jan 9, 2023
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.

Upgrade web3.js to latest minor Requesting new level instance does not populate contract var
4 participants