getTransaction should return pending transactions

[danfinlay]

From @ricmoo:

newer versions of MM aren’t responding to getTransaction until it is mined... :s

This could also lead into a bigger discussion of how to provide general tx data to dapp developers, but there is an initial bug that is much smaller in scope here.

[ricmoo]

I’m also more specifically interested that it returns pending transactions from an account managed by the user’s MetaMask, since it could be stored locally and the getTransaction can check this cache before hitting INFURA at all. I understand that in general responding to getTransaction for a given pending hash by an arbitrary addresses may be infeasible.

Thanks Dan! :)

[danfinlay]

Yeah, that's what I imagined, but thanks for making it explicit for anyone who jumps on this issue.

I actually thought we never returned a tx until it was mined before. Are you sure this isn't a new feature request? I'm pretty sure some dapps use this getTransactionByHash method to check for mining status.

Could this be a candidate for a new method, something like getTransactionMetadata? We could use it to include information outside the scope of the json-rpc tx definition, like submission status.

[ricmoo]

I’m not certain, but I “feel” like I used to get transaction data before it was mined. It could have also been an artifact of how the fallback provider works in ethers. If MM is one back end, and returns null, maybe when it fell back onto the next provider, like Etherscan, it was able to get the pending transaction.

It should still be safe to return a transaction, since the blockNumber and blockHas will be null, so that can be used to determine mined-ness, but I could imagine if people were relying on getTransaction returning null until it is mined. I usually recommend using receipts for that though... I may have an old computer with an antique MetaMask on it that I can try...

[danfinlay]

We have releases going back quite a way over here.

My primary concern is that if dapp devs are relying on a returned tx to represent mined-status today, adding it could give users a false sense of security, with dapps displaying confirmations when things are not confirmed.

So it seems like a kind of "softly" breaking API: The code keeps working, but the expectations change.

I agree that getting the receipt or checking the blockHash is fine for someone who knows about this, but for "already live" code, this could introduce insecurity.

[ricmoo]

Oh, completely agree on that. We need to see if old versions returned pending transactions first. If they didn’t, this becomes a whole new bit of functionality with the possible failures you described.

If they did return pending transactions, it’s a bit more “softly”... Since any developer that built their dapp since this changed, may fall in the same boat of that expectation...

I could also have the JsonRpcSigner create its own connection directly to INFURA to look for pending transactions... Since I do have the transaction hash, there is no way for a backend to lie about the contents of the transaction, since I can verify the raw transaction hash matches.

[danfinlay]

Sounds good. Removing bug for now, since we don't know if this is a regression or a feature request yet. If it's a new feature, I'll just note getTransactionMetadata would probably be an easy one for us to provide, since we already store something internally that we call a txMeta object.

[deacix]

Hey everyone! We have the same problem at 1inch.exchange! Metamask was giving one or two weeks before the tx hash after sending it and now metamask gives the tx if its really mined.

[danfinlay]

Metamask was giving one or two weeks before the tx hash after sending it

Could you clarify this? I'm not sure what you mean here.

As far as I can tell, we have always only returned a transaction once it was mined.

[ricmoo]

Actually, I have just confirmed myself, that at least in my version (6.6.1), the call to eth_getTransactionByHash returns nearly immediately (not requiring mining) the Transaction, with a blockHash and blockNumber of null, which is correct and the behaviour I want.

So, I am not seeing either the bug people are having (on a fairly recent version), nor am I experiencing what @danfinlay says has always been the behaviour. :p

@deacix What version of MetaMask are you using?

Is there maybe some legacy configuration setting I may have enabled?

[danfinlay]

Oh I'm sorry, it seems I mistook this with getTransactionReceipt, which only returns once mined.

I don't understand what problem @deacix is having.

[ricmoo]

Oh! Haha. Yes, that makes sense. The getTransactionReceipt should not return until it is mined (or null if not mined).

The problem, which I have not been able to reproduce myself, but have had many people come to me about, is that when they send a transaction (using eth_sendTransaction), which returns the transaction hash, when they then immediately perform an eth_getTransactionByHash, using that hash, they get null back, until the transaction is mined.

Internally, this is how ethers deals with the fact that the JSON-RPC API only returns the transaction hash, where as in ethers the response to sendTransaction is a complete transaction; for example, if gas price was omitted, and MM populated it, or updated it through the user’s interaction with the UI, the gasPrice used is returned, along with nonce and anything else the signing entity (like MM) populated.

All other signers implicitly provide this data, so I’m trying to rally support to fix this in the eth2.0 JSON-RPC API (the correct thing for them to have done would be to return the signed transaction, but the calls we aimed to mimic the bitcoind API). This also enables additional levels of security and reliability a library can perform. :)

[cloudonshore]

I made this codesandbox that you can reproduce the issue with (works on mainnet or rinkeby, all that is needed is a small ETH balance to test)
https://codesandbox.io/embed/ethers-metamask-react-ott4i

I'm using metamask version 6.6.2. This issue started occurring a couple weeks ago, so that might have been with release 6.6.0.

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 step transaction mined listed

Observed 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 and transaction mined display simultaneously.

[cloudonshore]

So I went and sequentially installed all the versions between 6.6.2 and 6.0.0 and none of them fixed the issue. I also tried using older versions of ethers.js. I then thought what the other variable in the system was and I decided to try switching out the default geth node and voila! That fixed it!

Instead of the default rinkeby provider in metamask I used the custom RPC https://geth-rinkeby.airswap-api.com which is our rinkeby geth node at AirSwap (running geth version 1.8.27). That fixes the issue. So diffing the responses between those two nodes should help you narrow down on what the issue is.

[deacix]

Hi everyone! I have 6.6.2 and ethers.js 4.0.30.
Normally i get the transaction after sending it with ethers.js and have a possibility to wait until it is ined by a special function. Currently i don't get transaction object back until it is mined..

[deacix]

Our application is https://1inch.exchange

[ricmoo]

@deacix Which network are you using?

[deacix]

Mainnet of course :)

[deacix]

Feel free test it on 1inch. Do a token swap and see that the loading spinner is showing until the transaction is mined.

[deacix]

Here is my code:

---

const tx = await this.aggregatedTokenSwapService.tokenSwap(
this.fromToken,
this.toToken,
fromTokenAmount,
ordersTokensAmount,
uniswapTokensAmount,
bancorTokensAmount,
kyberTokensAmount,
oasisTokensAmount,
minTokensAmount,
referrer,
this.bidsOrders,
this.asksOrders
);

        this.done = true;

        this.transactionHash = tx.hash;

---

Here is the return of the service call:

---

return this.contract.aggregate(
fromTokenAddress,
toTokenAddress,
fromTokenAmount,
ordersTargets,
callDataConcat,
starts,
ordersValues,
0.5e9,
minTokensAmount,
referrer,
{
value: value,
gasPrice: bancorTokensAmount.eq(0) ?
this.configurationService.fastGasPrice :
await this.bestPricePathContract.getBancorGasPrice(),
// gasLimit: 1000000
gasLimit: gasLimit.mul(140).div(100)
}
);

[cloudonshore]

@ricmoo @deacix it seems to me to be an issue with metamask's geth provider, which I believe is Infura. To verify this I tried pasting in a mainnet infura url (https://mainnet.infura.io/8LNJeV3XEJUtC5YzpkF6) as a custom RPC endpoint in metamask , and tried to reproduce the issue @deacix described on https://1inch.exchange . I was able to reproduce it, it has the same problem as the default mainnet metamask provider.

If you use a non-infura mainnet node as a custom RPC in metamask it fixes the issue. I tried https://geth-cluster.airswap-api.com which is the mainnet node we use at airswap, and https://eth-mainnet.alchemyapi.io/jsonrpc/V1GjKybGLx6rzSu517KSWpSrTSIIXmV7, which the the alchemy endpoint that kyber uses. Both fix the issue.

I'm guessing the issue is that Infura is doing something in their JSON RPC response that other mainnet nodes aren't, and there is logic in ethers.js that depends on something in the data structure and so a promise isn't getting called, etc. The issue doesn't occur in Web3 as far as I can tell, just ethers.

[cloudonshore]

I was able to narrow down the issue, I don't think it is metamask related at all, it just involves Infura and ethers.js. This issue can probably be closed out. Write up is here: ethers-io/ethers.js#544 (comment)

[ricmoo]

@deacix Can you try out ethers 4.0.31, and see if that solves your issue? It uses a dynamic id, which may solve the issue if it is related to the findings from @cloudonshore.

[danfinlay]

In Summary

getTransaction and getTransactionByHash should check our local tx store first, before passing the request out to the network. This should be very straightforward to implement, it would involve exposing a new json-rpc-engine middleware from the tx controller and included in our provider middleware stack.

[danfinlay]

I'd like @frankiebee's opinion since I know she's doing some work on the tx-controller and how it connects to the network provider.

[frankiebee]

I believe we already have in are middleware stack for getTransactionCount but not by has. This would not be difficult to do

[frankiebee]

https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/controllers/network/createMetamaskMiddleware.js#L37L44

[frankiebee]

This would be pretty easy to do I can pick up this issue