Adds revert instruction handling

[nivida]

Description

Fixes #176 #1903 #1707 #2197

This PR does implement revert instruction handling in the web3.js lib.

If the handleRevert options property is set to true you will get revert reasons on calling of Contract object methods, sendTransaction, and call of the eth module. Those revert reasons will get returned to the user over the error listener (call doesn't have a error event) or the catch handler if you are using the native Promise object.

Error Interface

interface RevertInstructionError extends Error {
    reason: string;
    signature: string;
}

Type of change

• New feature (non-breaking change which adds functionality)

Checklist:

• I have selected the correct base branch.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success and extended the tests if necessary.
• I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
• I have updated the CHANGELOG.md file in the root folder.
• I have tested my code on the live network.

[nivida]

length = (data.length - 2); // exclude prefix
length = length / 2; // Transforming length to bytes length
argsCount = ((length % 32) === 4 ) // We have 4 arguments: signature, offset, length, data
isErrorSignature = data.substring(0, 10) === '0x08c379a0'; // The first 4 bytes are the Error signature

isRevertReasonString = (argsCount && isErrorSignature)

[cgewecke]

Hi, just leaving a quick note. This looks really cool!

The are couple things that seem like breaking changes

• Throwing on eth_call
• Throwing with a different error message on eth_send

Additionally, I'm a little worried about additional server calls to fetch the reason since many wrappers or libs around Web3 are already doing that. In a contract testing context with lots error cases being run it could slow things down to duplicate this logic.

What do you think about making reason fetching available as an option that is toggled off by default and on via a new contract option?

Super interested in how eth_call works as well...do you think it might be possible to spoof a 'revert' message for a call? Like if data can be set by a contract's users, they could set it so that on 'read', web3 would decode it as an error message and throw?

[nivida]

@cgewecke

This looks really cool!

Thanks!:)

What do you think about making reason fetching available as an option that is toggled off by default and on via a new contract option?

Sounds good to me but we have to be careful to not have too many configuration options because it could have a bad impact on the DX in the long run. I would also not have this only as a Contractoption instead would I prefer to have this as a "global" web3-eth module option with the possibility to disable and enable the revert instruction handling for single Contract objects. This because of eth.sendTransaction, eth.sendSignedTransaction, and eth.call could also be used to call Contract functions with the help of the eth.abi module.

Super interested in how eth_call works as well...do you think it might be possible to spoof a 'revert' message for a call? Like if data can be set by a contract's users, they could set it so that on 'read', web3 would decode it as an error message and throw?

I'm not sure if I understood you correctly. But you would like to have the revert instruction handling also for the bare eth JSON-RPC methods as eth.call(...)?
This would already be implemented for eth.sendTransaction, eth.sendSignedTransaction, and eth.call.

[coveralls]

Coverage increased (+0.3%) to 84.757% when pulling b0b7f3a on feature/revert-reason-string into efc0a9b on 1.x.

[nivida]

@cgewecke I have added all the missing test cases and improved the solution I had with the handleRevert module option which does default to false. I'm not sure if this module option name is the most explicit one we can have, so feel free to propose a better name! ;)

During testing of the revert instruction handling did I notice that only module options which are defined before the Contract instance got created do get applied and all others not. This behavior is existing since the beginning of 1.0 and is actually not that easy to fix as I thought first.
That’s why I will check that closer later and will open a related issue with a possible solution after.

Example:

web3.eth.handleRevert = true;
contract = new web3.eth.Contract(..);
contract.methods.method(...).send(...);  // works as expected

-----

contract = new web3.eth.Contract(..); 
contract.handleRevert = true;
contract.methods.method(...).send(...);  // doesn't work as expected

Edit:

I've added all new properties to the contract options with commit 9db572c.

Example:

contract = new web3.eth.Contract([], '0x...', {handleRevert: true, ...}); 
contract.methods.method(...).send(...);  // does work as expected

ToDo:

• Extending of Contract options tests

[cgewecke]

Apart from the fight about the error message, this LGTM.

WDYT about including test cases for?

web3.eth.call()
web3.eth.sendTransaction()

[nivida]

WDYT about including test cases for?

The test cases would be the same and would also test the same code. But I definitely can create a generic test helper for this and extend the test cases for eth.call and eth.sendTransaction. 👍

[cgewecke]

In summary, this review ends in approval from me, per the PR review guidelines proposed in #3224. AFAIK it doesn't violate any of the requirements.

(There's are a few dozen unnecessary lints that go against the idea of making the smallest number of changes required, but the core changes seem well-restricted in scope).

[cgewecke]

@nivida Final note! This is really cool, Web3 might be the first lib to collect the revert string from view functions and will be very useful.

Looked pretty safe to me as well - the length of the reason message is hard to replicate using conventional datatypes.

[knaderi]

Hi,
I upgraded from web3 1.2.4 to 1.2.7.
I have this call in my code:
const transactionReceipt:TransactionReceipt = await web3.eth.sendSignedTransaction(answer.rawTransaction)

error TS2322: Type 'TransactionRevertInstructionError | TransactionReceipt' is not assignable to type 'TransactionReceipt'.
Type 'TransactionRevertInstructionError' is missing the following properties from type 'TransactionReceipt': status, transactionHash, transactionIndex, blockHash, and 7 more.

57 const transactionReceipt:TransactionReceipt = await web3.eth.sendSignedTransaction(answer.rawTransaction)

[cgewecke]

@knaderi

It looks like many of the promise types in web3-eth have mis-defined optional error types. Am opening an issue to correct this right now.

If you don't specify the type of your variable (e.g. just let TS infer it) does everything work ok?

 const transactionReceipt = await web3.eth.sendSignedTransaction(answer.rawTransaction)