Feature: Warn when attempting to send tx with data to non-contract

[HowardBraham]

Fixes #1789
Fixes #4525

I think this is technically an issue between Infura and the ethjs library. When you call getCode() on an address that does not have a contract, Ganache will return 0x0 and Infura will return 0x. The ethjs library correctly parses the 0x0, but does not know how to deal with the 0x. But absent fixes from Infura and ethjs, it is now fixed in MetaMask.

I have also improved the error message, which used to say

ALERT: Transaction Error. Exception thrown in contract code.

And now says the clearer

ALERT: Trying to call a contract function at an address that is not a contract address.

(This does have to be translated to additional languages.)

I have done regression tests against the following 12 test cases in the table below:

	Infura	Ganache
call function on nonexistent contract	4.9.3 will let you, PR ??? will stop you (ALERT was improved)	will stop you (ALERT was improved)
call function on real contract	works	works
call function on ERC-20 contract	works and shows as a TRANSFER	works and shows as a TRANSFER
send ETH to personal address	works and shows as CONFIRM	works and shows as CONFIRM
send ETH to contract without payable	will stop you (ALERT should be improved)	will stop you (ALERT should be improved)
send ETH to contract with payable	works (but should possibly show a warning in the future)	works (but should possibly show a warning in the future)

Two opportunities for additional improvement (as mentioned in the table above) are the alert when you send ETH to a contract without payable, and a possible warning when you send ETH to a contract with payable (as this is a special case with pitfalls the user should be aware of).

Please let me know if I overlooked any test cases.

[frankiebee]

Infura dosent manipulate responses for getCode so most likely is coming directly from the client code i just want to verify this behavior and will come back to this PR after that.

[frankiebee]

This is not a bug with metamask/infura but a bug with ganache and ethjs 0x is nil and 0x0 or 0x00 as it should be is 0 my recommendation is to fix the bug directly in ganache and have ethjs handle 0x as null

[HowardBraham]

What's the standard? Returning 0x, or returning 0x0?

[frankiebee]

0x is standard

[HowardBraham]

Ahah, okay. I previously thought 0x0 was the standard. So it's not an Infura bug. As you said, it's a bug between Ganache and ethjs. Is it possible to make that error message clearer from inside ethjs?

[HowardBraham]

I found this issue mentioned at Ganache at trufflesuite/ganache#51, and interestingly, they claimed to have closed the issue on April 12.

I submitted a new pull request to Ganache at trufflesuite/ganache#171 that I believe should actually close that issue.

Does anyone happen to know which package in ethjs contains the relevant getCode? I'm having a little trouble locating it.

[frankiebee]

@HowardBraham your looking at https://github.com/ethjs ?

[HowardBraham]

@frankiebee Yes, exactly, MetaMask has a dependency on ethjs, ethjs-contract, ethjs-ens, and ethjs-query. Do we know where this problem might be located?

[HowardBraham]

@frankiebee Actually, I have a feeling it might be in https://github.com/ethjs/ethjs-format. There are probably hundreds of projects that depend on this code, so fixing it is probably not that simple. I'm going to open an issue over there.

[HowardBraham]

@frankiebee After further investigation, I'm not so sure anymore that there's a problem with ethjs. I think the 0x0 thing was actually a bit of a red herring. I think the problem is this:

If you POST an eth_estimateGas request to Ganache, and the contract does not exist, Ganache will return
{"id":8379431946159,"jsonrpc":"2.0","error":{"message":"Attempting to run transaction which calls a contract function, but recipient address 0x000000000000000000000000000000000000dead is not a contract address","code":-32000,...

This will come back as an error in MetaMask.

On the other hand, when you try eth_estimateGas with Infura, it does not return an error. Instead, it returns:
{"jsonrpc":"2.0","id":8379431946155,"result":"0x53d8"}

So we can't always trust the remote node to give an accurate eth_estimateGas answer.

Instead, we must:

• Proactively call eth_getCode
• If the txParams have data AND the code is either falsy, 0x, or 0x0
  • Then we're trying to call a contract function on a contract that does not exist, and we should warn the user.
  • Don't even bother trying eth_estimateGas.

My pull request already does this, and thus fixes the issues.

[HowardBraham]

@frankiebee: ganache-core merged my PR trufflesuite/ganache#171

So now future versions of Ganache will no longer return the non-standard 0x0.

But I think we should keep the functionality in MetaMask to parse 0x0 for maximum compatibility with all RPCs. Plus, the rest of my changes are necessary for connecting to Infura, which will not give an accurate eth_estimateGas answer for no contract.

[HowardBraham]

UPDATE: This was blocked by #5458, but fixed by #5475. I have now rebased this PR to the head of develop.

[kumavis]

I'm not familiar with this style of instantiating errors - is this correct? How does the error message get set?

is it appropriate to throw errors in this estimateTxGas function?

[HowardBraham]

The errorKey is converted to 'transactionErrorNoContract' in ui\app\constants\error-keys.js, and then "Trying to call a contract function at an address that is not a contract address." in app\_locales\en\messages.json.

[kumavis]

This generally looks good.
Mostly concerned about the error instantiation and whether we actually get this custom error key on the UI side.

This PR does include any tests that guarantee the new behavior

[kumavis]

@HowardBraham can you expand on why you think sending eth to a payable contract requires a warning?

[kumavis]

Perhaps we can improve the language here to something less technical users can understand:

"Trying to call a contract function at an address that is not a contract address."

[HowardBraham]

@kumavis
A proposal for less technical language:

ALERT: You're trying to call a function that does not exist. This will fail and waste your gas fee.

When sending ETH to a payable contract, I think there should at least be an indication that it's a contract and not an individual. Just a little reminder for the user to think twice and make sure they're doing what they expect. Currently, there's no indication whatsoever.

ganache's behavior was changed

[HowardBraham]

I don't know if you were able to test this, but the correct error message does show up in the UI.

Would a unit test for this actually connect to Infura or geth, or just simulate the connection? Simply connecting to the internal Ganache might not be enough, because Ganache did not exhibit this bug. Can someone show me an example of a similar unit test?

[HowardBraham]

@kumavis, can you help me figure out how to finish this PR and get it merged? I answered your questions and asked one above (about unit test examples). Thanks!

[kumavis]

@HowardBraham i guess my primary concern at this point is the creation of an error without a message (even though the message gets set via the localization). @bitpshr @whymarrh do we do this anywhere else?

[HowardBraham]

@kumavis @bitpshr @whymarrh I based it on this code, which I know is not exactly the same, because it's not throwing an Error object:

metamask-extension/ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js

Lines 124 to 129 in 600f755

	if (insufficientBalance) {
	return {
	valid: false,
	errorKey: INSUFFICIENT_FUNDS_ERROR_KEY,
	}
	}

If there's a better pattern to follow, in order to throw an error with a localized message, I'm happy to change it.

[kumavis]

Thanks again @HowardBraham
made some changes and published a new PR
#5283

this file+method have evolved, tx-gas-util and estimateGas will later need to be renamed to more correctly reflect their role