-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Metamask updates seem to have broken some functionality in the Contract API in ethers #544
Comments
I've been chatting with MetaMask, as I think this is a change on their end. I haven't had a chance to verify, but could you possibly try installing an old version of MM and see if (with the new code, either v4 or v5) this still happens? If it is a MM problem, it is something they said they would be happy to fix. I just haven't had time to check. If you are busy though, I will be checking later this week. Thanks! |
This is happening to more and more people, I’ll be looking more into it tomorrow, but if people who have reproduced this could try out various versions of MetaMask and help isolate which versions are and are not affected, it would be a huge help. Here are the MetaMask releases to try: https://github.com/MetaMask/metamask-extension/releases And here is the ticket on MetaMask’s side of the investigation: MetaMask/metamask-extension#6704 Thanks! :) |
@ricmoo So I went and sequentially installed all the versions between Instead of the default rinkeby provider in metamask I used the custom RPC |
@ricmoo AIRSWAP MAINNET NODE (https://geth-cluster.airswap-api.com)
subsequent
INFURA MAINNET NODE (https://mainnet.infura.io/8LNJeV3XEJUtC5YzpkF6)
subsequent
The Also btw I generated the above behavior with just ethers.js and a ledger nano, so no metamask involved. |
Interesting... I’ve removed the hard-coded 42 from JsonRpcProvider, but missed Web3Provider. I will make that change tonight regardless. Maybe INFURA added some sort of new caching? Because that hasn’t changed in the Web3Provider. It also doesn’t explain why I can’t reproduce it. :p I will reach out to INFURA to see if this makes sense to them. |
It might be notable that geth recently made a change to how they report errors for some kinds of invalid requests, it just occurred to me that it might remotely be related: |
I've updated the Web3Provider to use a dynamic id. In v5, I'll be adding a random salt to the ID to mitigate response injection, which a lot of popular wallets seem to be susceptible to (MetaMask seems fine, but Trust Wallet is certainly vulnerable). But in v4, this is fine since it was already hard-coded previously. @cloudonshore Can you try out |
Hi ricmoo, is still not work with 4.0.31 |
@deacix Thanks. It doesn't have anything to do with the JSON-RPC id then. I think I have a new theory. Can I get some help from people experiencing this? Follow these instructions for "Background Logs". Basically, right click on the extension and enable "developer tools" for the background script. Then just load MM as usual, looking at the logs, and note the URL it is using. My MetaMask (which works fine) is using the INFURA v1 URL. My theory is that new installs (which would use the new v3), may be designed to not return transactions in the pending TX pool. Still a theory, but lets find out. :) |
It's also worth keeping in mind that Infura is a load-balanced service, and not every node necessarily knows about the full tx pool of every other Infura node. If you want a fast response from the provider API, it probably is necessary that MetaMask locally cache these values and return them without hitting the network. |
Absolutely, which is why ethers polls, (with exponential backoff) until it gets a response. :) I do think it makes sense for MetaMask to locally cache transactions it signs and sends, but that may be a non-trivial change... |
The basic implementation would actually be very trivial. We already cache all of this (and have to, for the sake of our own submission-with-exponential-backoff). It's just a matter of checking our local store as a middleware cache before hitting the network. Hopefully this comment here can serve as an actionable summary when I return here, so we don't have to re-read the whole thread to get oriented ;) |
Confirmed this issue on Ethereum mainnet, but on Ropsten it is fine. |
Hey, @cloudonshore, you have encountered a pretty strange MetaMask bug. That value should have been migrated away like 2 years ago. Is it possible you've been operating on the same installation for over 2 years? Would you mind emailing me your state logs? This thread is probably not the best place for debugging that particular issue, I don't think it's related to ethers.js To help us diagnose your issue, you can download your "state logs" by following this guide, and then sending them to support@metamask.io . |
Also if your install is very old, it's possible the easiest workaround is to :
Sorry that this will mean settings like token list and account nicknames would be lost (we're adding config backup soon!). |
@ricmoo @danfinlay the api endpoint it is hitting inside the background logs is So the issue seems to be caused by infura not returning a transaction when queried about a transactionHash that was returned by a
and the response is
ethers continues polling until after the block for that transaction is mined, and then that same call returns a transaction
When I change my RPC provider to the custom mainnet provider (https://geth-cluster.airswap-api.com), immediately after I submit the transaction, I see the ethers query for it show up in metamask, but instead of a GET request it's a POST request to the RPC url
and receive a response immediately:
This is definitely the root behavior that is causing the issue for me. |
@danfinlay I've forwarded the state logs to that support email |
@cloudonshore Thanks! Yes, I've already sent a quick note regarding this to the MetaMask peeps last week, to see what they think. :) I suspect this is not an issue with MetaMask at all, but something INFURA likely recently added (possibly as an anti-DDoS metric). Regardless, it is something MetaMask has tentatively agreed to address, since it is somewhat simpler (and efficient) for them to implement anyways. |
Seems like progress on it has kind of stalled, I was noticing that there is potentially another workaround from the ethers side.
If, instead of returning a "poll" for the transaction, this function returned the transactionHash immediately, then I could know that the transaction had been successfully submitted, and could poll for it myself. The way it is now, no response is given until it is mined. . |
If you do not need the transaction (only the hash), you can use the UncheckedJsonRpcSigner. Keep in mind that when deploying contracts, you will need to wait until the transaction is mined before you can get the Contract Address, and things like that. In v5 this class is built-in. To get an UncheckedSigner, use The |
Any updates or workarounds regarding this issue? I'm using v4.0.37. |
One of my recently released projects also suffers from this issue. I’ll be pinging the MM team again this week. :) |
@ricmoo same! Some users have been complaining about it. Did the MM team get back to you? |
I'm checking in on our (MetaMask) side, looks like it got dropped somehow, sorry about that. |
@danfinlay I have another related ask that could probably get wrapped into the same changes; when calling eth_getTransactionCount for an account in MetaMask with “pending”, can the response take into account the same pool of inflight transactions? :) |
I believe this has long since been taken care of? I'm going to close it now, but if anyone continues to have a problem, please re-open or start another issue. Thanks! :) |
I've created this codesandbox to reproduce (works with metamask on mainnet or rinkeby, all that is needed is a small ETH balance to test) https://ott4i.codesandbox.io/
The bug happens with all Contract interactions, but in the above test, the user tries to approve the null address to transfer 5 wei of WETH.
Expected behavior:
After submitting the transaction, you should see the step
signature successful, awaiting mined transaction
listed, and after mining, you should see the steptransaction mined
listedObserved behavior:
After submitting the transaction, the promise for the successful signature doesn't resolve. However, when the transaction is mined both promises resolve at once and
signature successful, awaiting mined transaction
andtransaction mined
display simultaneouslyYou can change the ethers.js version in package.json, it seems to happen with all recent versions (including the new v5 beta). This started happening in one of our codebases that had no changes in the past couple weeks, so I believe it's because of updates that metamask has rolled out.
Please let me know if you have any other questions.
Thanks,
Sam
The text was updated successfully, but these errors were encountered: