Refactor `getRevertReason` and add tests #5844

spacesailor24 · 2023-02-16T01:36:55Z

`web3-eth-abi`

Added

decodeErrorData from web3-eth-contract is now exported from this package and was renamed to decodeContractErrorData (Refactor getRevertReason and add tests #5844)

`web3-eth-contract`

Removed

decodeErrorData is no longer exported (method was moved to web3-eth-abi and renamed decodeContractErrorData) (Refactor getRevertReason and add tests #5844)

`web3-eth`

Removed

getRevertReason is no longer exported (Refactor getRevertReason and add tests #5844)

`web3-utils`

Changed

SocketProvider abstract class now resolves JSON RPC response errors instead of rejecting them (Refactor getRevertReason and add tests #5844)

closes #5841 #5838

github-actions · 2023-02-16T02:53:19Z

Bundle Stats

Hey there, this message comes from a github action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Asset	Old size	New size	Diff	Diff %
Total	1.18 MB	1.06 MB	-119 KB	-9.91%

View detailed bundle breakdown

Added

Asset	New size	Diff	Diff %
../lib/eth.exports.d.ts.map	358 bytes	358 bytes	-
../lib/eth.exports.d.ts	321 bytes	321 bytes	-
../lib/providers.exports.d.ts.map	292 bytes	292 bytes	-
../lib/providers.exports.d.ts	238 bytes	238 bytes	-

Removed

No assets were removed

Bigger

Asset	Old size	New size	Diff	Diff %
../lib/index.d.ts	77 bytes	899 bytes	822 bytes	1067.53%
../lib/index.d.ts.map	155 bytes	919 bytes	764 bytes	492.90%

Smaller

Asset	Old size	New size	Diff	Diff %
web3.min.js	1.17 MB	1.05 MB	-122 KB	-10.19%
../lib/types.d.ts	1.92 KB	1.68 KB	-253 bytes	-12.84%
../lib/types.d.ts.map	1.75 KB	1.51 KB	-241 bytes	-13.46%

Unchanged

Asset	Old size	New size	Diff	Diff %
../lib/accounts.d.ts	3.42 KB	3.42 KB	0	0.00%
../lib/abi.d.ts	1020 bytes	999 bytes	-18 bytes	-1.77%
../lib/web3.d.ts	842 bytes	842 bytes	0	0.00%
../lib/web3.d.ts.map	693 bytes	694 bytes	1 bytes	0.14%
../lib/accounts.d.ts.map	528 bytes	528 bytes	0	0.00%
../lib/version.d.ts.map	140 bytes	140 bytes	0	0.00%
../lib/abi.d.ts.map	124 bytes	124 bytes	0	0.00%
../lib/version.d.ts	97 bytes	97 bytes	0	0.00%

cloudflare-workers-and-pages · 2023-02-16T02:56:11Z

Deploying with Cloudflare Pages

Latest commit:	`28a3d84`
Status:	✅ Deploy successful!
Preview URL:	https://f43fd401.web3-js-docs.pages.dev
Branch Preview URL:	https://wyatt-4-x-5841-return-revert.web3-js-docs.pages.dev

View logs

spacesailor24 · 2023-02-17T06:18:04Z

packages/web3-eth-abi/src/decode_contract_error_data.ts

+import { decodeParameters } from './api/parameters_api';
+import { jsonInterfaceMethodToString } from './utils';
+
+export const decodeContractErrorData = (


This method was moved from web3-eth-contract package to web3-eth-abi to avoid a circular dependency issue between web3-eth-contract and web3-eth since it's now being used inside web3-eth's getRevertReason util method. It was also renamed from decodeErrorData

packages/web3-eth/src/rpc_method_wrappers.ts

spacesailor24 · 2023-02-17T06:23:24Z

packages/web3-utils/src/socket_provider.ts

-			} else {
-				this._eventEmitter.emit('message', response, undefined);
-				requestItem?.deferredPromise.reject(new ResponseError(response));


I removed this code and the tests from packages/web3-providers-ws/test/unit/web_socket_provider.test.ts because I couldn't get an error to actually trigger this code because any error resulting from a failed network request was emitted by _onError and any RPC response with an error is now being resolved. As the only tests written for this code were for RPC response errors, I don't think this code is needed anymore

This change also fixes #5838

spacesailor24 · 2023-02-17T06:36:32Z

packages/web3/test/integration/handle_revert.test.ts

@@ -28,7 +28,7 @@ import { BasicAbi, BasicBytecode } from '../shared_fixtures/build/Basic';

 Error.stackTraceLimit = Infinity;

-describe('eth', () => {
+describe.skip('eth', () => {


In order for these tests to pass with the expected error, #5839 and #5840 need to be addressed. This is because previously when sendTransaction was called getRevertReason we were throwing the reason making the sendTransaction promise to reject with the revert reason. Now that we're returning the reason, sendTransaction rejects at the requestManager (#5840) giving sendTransaction no opportunity to throw an error with the retrieved revert reason - we will need to re-enable and refactor these tests after #5840 and #5839

For context, the following is the result of running these tests with this PR's changes:

$ jest --config=./test/integration/jest.config.js --forceExit test/integration/handle_revert.test.ts FAIL test/integration/handle_revert.test.ts ● eth › handleRevert › should get revert reason expect(received).rejects.toThrow(expected) - Expected message - 2 + Received message + 1 - Transaction has been reverted by the EVM: - undefined + Error happened while trying to execute a function inside a smart contract 440 | // However, more processing will happen at a higher level to decode the error data, 441 | // according to the Error ABI, if it was available as of EIP-838. > 442 | if (error?.message.includes('revert')) throw new ContractExecutionError(error); | ^ 443 | 444 | return false; 445 | } at Function._isReverted (../web3-core/src/web3_request_manager.ts:442:48) at Web3RequestManager._processJsonRpcResponse (../web3-core/src/web3_request_manager.ts:371:35) at Web3RequestManager.<anonymous> (../web3-core/src/web3_request_manager.ts:216:16) at fulfilled (../web3-core/lib/web3_request_manager.js:5:58) 89 | it('should get revert reason', async () => { 90 | contract.handleRevert = true; > 91 | await expect(contract.methods.reverts().send({ from: accounts[0] })).rejects.toThrow( | ^ 92 | new TransactionRevertError( 93 | 'Returned error: execution reverted: REVERTED WITH REVERT', 94 | ), at Object.toThrow (../../node_modules/expect/build/index.js:241:22) at test/integration/handle_revert.test.ts:91:81 at test/integration/handle_revert.test.ts:8:71 at Object.<anonymous>.__awaiter (test/integration/handle_revert.test.ts:4:12) at Object.<anonymous> (test/integration/handle_revert.test.ts:89:45) ● eth › handleRevert › should get revert reason for eth tx expect(received).rejects.toThrow(expected) - Expected message - 2 + Received message + 1 - Transaction has been reverted by the EVM: - undefined + Returned error: invalid argument 0: json: cannot unmarshal invalid hex string into Go struct field TransactionArgs.data of type hexutil.Bytes 370 | } 371 | } else if (!Web3RequestManager._isReverted(response)) { > 372 | throw new InvalidResponseError<ErrorType>(response); | ^ 373 | } 374 | } 375 | at Web3RequestManager._processJsonRpcResponse (../web3-core/src/web3_request_manager.ts:372:11) at Web3RequestManager.<anonymous> (../web3-core/src/web3_request_manager.ts:216:16) at fulfilled (../web3-core/lib/web3_request_manager.js:5:58) 112 | s: '0x39f77e0b68d5524826e4385ad4e1f01e748f32c177840184ae65d9592fdfe5c', 113 | }), > 114 | ).rejects.toThrow( | ^ 115 | new TransactionRevertError( 116 | 'Returned error: invalid argument 0: json: cannot unmarshal invalid hex string into Go struct field TransactionArgs.data of type hexutil.Bytes', 117 | ), at Object.toThrow (../../node_modules/expect/build/index.js:241:22) at test/integration/handle_revert.test.ts:114:14 at test/integration/handle_revert.test.ts:8:71 at Object.<anonymous>.__awaiter (test/integration/handle_revert.test.ts:4:12) at Object.<anonymous> (test/integration/handle_revert.test.ts:98:56) Test Suites: 1 failed, 1 total Tests: 2 failed, 1 passed, 3 total Snapshots: 0 total Time: 1.873 s Ran all test suites matching /test\/integration\/handle_revert.test.ts/i. Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished? error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

It should be part of RC.0 @mconnelly8

jdevcs · 2023-02-17T18:05:27Z

packages/web3-eth/src/utils/get_revert_reason.ts

+			return error.innerError.message;
+		}
+
+		return error instanceof Error ? error.message : error;


In case of generic error ( that is not due to failed tx reverted ) , we should throw it and remove string from return typePromise<undefined | RevertReason | RevertReasonWithCustomError | string | unknown>.

Because for getRevertReason function call with eth_call rpc in case of generic errors:

neither tx eth_call was reverted with reason.

nor it succeeded with out any revert reason,
but request for looking any of above answers failed, that user should catch. It will also make return types homogeneous.

Updated via this commit, however string is still a possible return type because of

if ( error instanceof InvalidResponseError && !Array.isArray(error.innerError) && error.innerError !== undefined ) { return error.innerError.message; }

if InvalidResponseError is also not categorizable under 1 and 2, shouldn't it also be thrown?

@jdevcs The code snippet above is for errors such as:

InvalidResponseError: Returned error: err: intrinsic gas too low: have 0, want 21544 (supplied gas 0) at Web3RequestManager._processJsonRpcResponse (/home/anon/Public/code/ChainSafe/git-repos/web3.js/packages/web3-core/src/web3_request_manager.ts:372:11) at Web3RequestManager.<anonymous> (/home/anon/Public/code/ChainSafe/git-repos/web3.js/packages/web3-core/src/web3_request_manager.ts:216:16) at Generator.next (<anonymous>) at fulfilled (/home/anon/Public/code/ChainSafe/git-repos/web3.js/packages/web3-core/lib/web3_request_manager.js:5:58) at processTicksAndRejections (node:internal/process/task_queues:95:5) { innerError: { code: -32000, message: 'err: intrinsic gas too low: have 0, want 21544 (supplied gas 0)' }, code: 101, data: undefined }

keep in mind that this method is now an internal method to be used by devs to further process why transactions failed before returning an error to the end user with more context

also it can be generic error like :

web3.js/packages/web3-utils/src/chunk_response_parser.ts

Line 67 in b948ea4

throw new InvalidResponseError({

. ( I think in that case at least , it should be modified in chunkresponseparser to be some different error type and code ) and we can keep returning InvalidResponseError as it is here.

@jdevcs That's a good point. I think the error in the chunk parser should remain InvalidResponseError, but errors like the gas too low from above should be renamed to something like RpcError. The gas too low isn't an InvalidResponse it's just an error returned from the EVM because the transaction was invalid

If you agree, I can create an issue for it, merge this PR, then make the error name change?

sure. Good work @spacesailor24

jdevcs

Overall looks good, Thanks Wyatt. I posted a change for getRevertReason return behavior.

avkos

Great Job, Thx!

Refactor getRevertReason and init tests

f9f50e7

spacesailor24 added the 4.x 4.0 related label Feb 16, 2023

spacesailor24 self-assigned this Feb 16, 2023

Correct decodeContractErrorData import in web3-eth-contract

11c5a52

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 16, 2023 02:55 Inactive

Temporarily disable hande_revert tests

f217ee3

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 16, 2023 04:38 Inactive

Remove ResponseError from socker_provider onMessage

9581a77

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 16, 2023 04:58 Inactive

spacesailor24 added 3 commits February 16, 2023 16:18

Update socker_provider to no reject if RPC error

e6a380c

Remove no longer used ResponseError from socket_provider

4c5c17d

Merge branch '4.x' into wyatt/4.x/5841-return-revert-reason

ba26d13

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 17, 2023 02:36 Inactive

Remove unused imports

a54d637

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 17, 2023 02:50 Inactive

spacesailor24 commented Feb 17, 2023

View reviewed changes

packages/web3-eth/src/rpc_method_wrappers.ts Show resolved Hide resolved

spacesailor24 commented Feb 17, 2023

View reviewed changes

Update CHANGELOGs

dc77fb0

spacesailor24 marked this pull request as ready for review February 17, 2023 07:04

spacesailor24 requested review from Muhammad-Altabba, nikoulai, jdevcs, avkos and luu-alex February 17, 2023 07:05

github-actions bot temporarily deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 17, 2023 07:11 Inactive

nikoulai approved these changes Feb 17, 2023

View reviewed changes

jdevcs requested changes Feb 17, 2023

View reviewed changes

jdevcs reviewed Feb 17, 2023

View reviewed changes

getRevertReason now throws an error if it's unknown

28a3d84

github-actions bot deployed to Preview: (wyatt/4.x/5841-return-revert-reason) February 17, 2023 19:31 View deployment

jdevcs approved these changes Feb 17, 2023

View reviewed changes

avkos approved these changes Feb 18, 2023

View reviewed changes

spacesailor24 merged commit a5aa322 into 4.x Feb 18, 2023

spacesailor24 deleted the wyatt/4.x/5841-return-revert-reason branch February 18, 2023 01:58

spacesailor24 mentioned this pull request Feb 18, 2023

JSON RPC Response Errors Shouldn't be Returned as InvalidResponseErrors #5848

Closed

jdevcs mentioned this pull request Mar 10, 2023

Release/4.0.1 rc.0 #5912

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `getRevertReason` and add tests #5844

Refactor `getRevertReason` and add tests #5844

spacesailor24 commented Feb 16, 2023 •

edited

Loading

github-actions bot commented Feb 16, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 16, 2023 •

edited

Loading

spacesailor24 Feb 17, 2023

spacesailor24 Feb 17, 2023

spacesailor24 Feb 17, 2023 •

edited

Loading

spacesailor24 Feb 17, 2023

jdevcs Feb 17, 2023

jdevcs Feb 17, 2023

spacesailor24 Feb 17, 2023

jdevcs Feb 17, 2023

spacesailor24 Feb 17, 2023 •

edited

Loading

jdevcs Feb 17, 2023

spacesailor24 Feb 17, 2023 •

edited

Loading

jdevcs Feb 17, 2023

jdevcs left a comment

avkos left a comment

Refactor getRevertReason and add tests #5844

Refactor getRevertReason and add tests #5844

Conversation

spacesailor24 commented Feb 16, 2023 • edited Loading

web3-eth-abi

Added

web3-eth-contract

Removed

web3-eth

Removed

web3-utils

Changed

github-actions bot commented Feb 16, 2023 • edited Loading

Bundle Stats

cloudflare-workers-and-pages bot commented Feb 16, 2023 • edited Loading

Deploying with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacesailor24 Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacesailor24 Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacesailor24 Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdevcs left a comment

Choose a reason for hiding this comment

avkos left a comment

Choose a reason for hiding this comment

Refactor `getRevertReason` and add tests #5844

Refactor `getRevertReason` and add tests #5844

spacesailor24 commented Feb 16, 2023 •

edited

Loading

`web3-eth-abi`

`web3-eth-contract`

`web3-eth`

`web3-utils`

github-actions bot commented Feb 16, 2023 •

edited

Loading

cloudflare-workers-and-pages bot commented Feb 16, 2023 •

edited

Loading

spacesailor24 Feb 17, 2023 •

edited

Loading

spacesailor24 Feb 17, 2023 •

edited

Loading

spacesailor24 Feb 17, 2023 •

edited

Loading