-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
sendTransaction
and sendSignedTransaction
Error Refactors
#5854
Changes from 5 commits
43d93de
db9cb50
8eb82ba
e4a2891
36c9b0d
2378e6a
044fea2
3a12870
63554f2
8c33881
853d0b2
bd4ff70
75a99f2
d075a86
2b59dc6
7e830d2
91a4e6f
ff142e0
8948973
c39aab2
3cb0a27
c951bb7
b2c21b2
af78858
77d6fa0
e5c59af
de1fe5e
f334a9e
71e8f1b
de2f1cf
4205577
a66aaf0
284f4ad
19aadcc
5376bc8
662a390
81be9d8
0f655a5
a1d1a56
d7ce12f
dc03c22
32add6d
66b7377
b998f8f
35293d6
68efd93
d6fc658
3d3e7bc
64a3e1f
fedc918
c54d2b9
683515c
547f9be
9e3c637
f4f43f8
d72206c
2b816bf
c4180b6
306b650
76ee3da
bcffc9f
2828b41
5838adf
e79e59f
a2175ff
5cbb9bf
e267663
2662eaf
c216751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,11 @@ import { | |
import { Web3Context, Web3PromiEvent } from 'web3-core'; | ||
import { ETH_DATA_FORMAT, FormatType, DataFormat, DEFAULT_RETURN_FORMAT, format } from 'web3-utils'; | ||
import { isBlockTag, isBytes, isNullish, isString } from 'web3-validator'; | ||
import { SignatureError, TransactionError, ContractExecutionError } from 'web3-errors'; | ||
import { | ||
InvalidResponseError, | ||
SignatureError, | ||
TransactionError, | ||
} from 'web3-errors'; | ||
import { ethRpcMethods } from 'web3-rpc-methods'; | ||
import { decodeSignedTransaction } from './utils/decode_signed_transaction'; | ||
import { | ||
|
@@ -77,8 +81,7 @@ import { trySendTransaction } from './utils/try_send_transaction'; | |
import { waitForTransactionReceipt } from './utils/wait_for_transaction_receipt'; | ||
import { watchTransactionForConfirmations } from './utils/watch_transaction_for_confirmations'; | ||
import { NUMBER_DATA_FORMAT } from './constants'; | ||
// eslint-disable-next-line import/no-cycle | ||
import { getRevertReason } from './utils/get_revert_reason'; | ||
import { getTransactionError } from './utils/get_transaction_error'; | ||
|
||
/** | ||
* | ||
|
@@ -1078,14 +1081,6 @@ export function sendTransaction< | |
ETH_DATA_FORMAT, | ||
); | ||
|
||
if (web3Context.handleRevert) { | ||
// eslint-disable-next-line no-use-before-define | ||
await getRevertReason( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whole idea of doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jdevcs function revertSend(string memory revertString) public {
require(keccak256(abi.encodePacked(revertString)) != keccak256(abi.encodePacked("revert")), "This is a send revert");
} so relying on getting the revert reason without sending the transaction isn't a complete solution. Instead, I would suggest checking for a revert reason before sending the transaction only if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There also appears to be a discrepancy between providers with returning revert errors when using Sending the transaction: {
from: tempAcc.address,
to: '0x0000000000000000000000000000000000000000',
value: BigInt('999999999999999999999999999999999999999999999999999999999'),
} yields There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed by this commit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not secure users 100% of times but where applicable, if it can save user in few scenarios even in that case it should not be skipped before sending tx, and its more good to check it based on defined flag. Thanks for making this change. LGTM. |
||
web3Context, | ||
transactionFormatted as TransactionCall, | ||
); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of addressing #5839, the revert reason is retrieved after the transaction has been processed by the network and when |
||
if ( | ||
!options?.ignoreGasPricing && | ||
isNullish(transactionFormatted.gasPrice) && | ||
|
@@ -1177,17 +1172,17 @@ export function sendTransaction< | |
) as unknown as ResolveType, | ||
); | ||
} else if (transactionReceipt.status === BigInt(0)) { | ||
const error = await getTransactionError<ReturnFormat>( | ||
web3Context, | ||
transactionFormatted as TransactionCall, | ||
transactionReceiptFormatted, | ||
); | ||
|
||
if (promiEvent.listenerCount('error') > 0) { | ||
promiEvent.emit( | ||
'error', | ||
new TransactionError( | ||
'Transaction failed', | ||
transactionReceiptFormatted, | ||
), | ||
); | ||
Comment on lines
-1181
to
-1187
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the cause of #5837 |
||
promiEvent.emit('error', error); | ||
} | ||
reject(transactionReceiptFormatted as unknown as ResolveType); | ||
return; | ||
|
||
reject(error); | ||
jdevcs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
resolve(transactionReceiptFormatted as unknown as ResolveType); | ||
} | ||
|
@@ -1206,14 +1201,10 @@ export function sendTransaction< | |
); | ||
} | ||
} catch (error) { | ||
if (error instanceof ContractExecutionError) { | ||
promiEvent.emit('contractExecutionError', error); | ||
} | ||
Comment on lines
-1209
to
-1211
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see now that it's being using in web3-eth-contract There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (promiEvent.listenerCount('error') > 0) { | ||
promiEvent.emit( | ||
'error', | ||
new TransactionError((error as Error).message), | ||
); | ||
if (error instanceof InvalidResponseError && | ||
promiEvent.listenerCount('error') > 0 | ||
) { | ||
promiEvent.emit('error', error); | ||
} | ||
|
||
reject(error); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,45 @@ import { DataFormat, DEFAULT_RETURN_FORMAT } from 'web3-utils'; | |
import { call } from '../rpc_method_wrappers'; | ||
import { RevertReason, RevertReasonWithCustomError } from '../types'; | ||
|
||
export const parseTransactionError = (error: unknown, contractAbi?: ContractAbi) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to it's own function so it could make |
||
if ( | ||
error instanceof ContractExecutionError && | ||
error.innerError instanceof Eip838ExecutionError | ||
) { | ||
if (contractAbi !== undefined) { | ||
const errorsAbi = contractAbi.filter(abi => | ||
isAbiErrorFragment(abi), | ||
) as unknown as AbiErrorFragment[]; | ||
decodeContractErrorData(errorsAbi, error.innerError); | ||
|
||
return { | ||
reason: error.innerError.message, | ||
signature: error.innerError.data?.slice(0, 10), | ||
data: error.innerError.data?.substring(10), | ||
customErrorName: error.innerError.errorName, | ||
customErrorDecodedSignature: error.innerError.errorSignature, | ||
customErrorArguments: error.innerError.errorArgs, | ||
} as RevertReasonWithCustomError; | ||
} | ||
|
||
return { | ||
reason: error.innerError.message, | ||
signature: error.innerError.data?.slice(0, 10), | ||
data: error.innerError.data?.substring(10), | ||
} as RevertReason; | ||
} | ||
|
||
if ( | ||
error instanceof InvalidResponseError && | ||
!Array.isArray(error.innerError) && | ||
error.innerError !== undefined | ||
) { | ||
return error.innerError.message; | ||
} | ||
|
||
throw error; | ||
}; | ||
|
||
/** | ||
* Returns the revert reason generated by the EVM if the transaction were to be executed. | ||
* | ||
|
@@ -44,41 +83,6 @@ export async function getRevertReason< | |
await call(web3Context, transaction, web3Context.defaultBlock, returnFormat); | ||
return undefined; | ||
} catch (error) { | ||
if ( | ||
error instanceof ContractExecutionError && | ||
error.innerError instanceof Eip838ExecutionError | ||
) { | ||
if (contractAbi !== undefined) { | ||
const errorsAbi = contractAbi.filter(abi => | ||
isAbiErrorFragment(abi), | ||
) as unknown as AbiErrorFragment[]; | ||
decodeContractErrorData(errorsAbi, error.innerError); | ||
|
||
return { | ||
reason: error.innerError.message, | ||
signature: error.innerError.data?.slice(0, 10), | ||
data: error.innerError.data?.substring(10), | ||
customErrorName: error.innerError.errorName, | ||
customErrorDecodedSignature: error.innerError.errorSignature, | ||
customErrorArguments: error.innerError.errorArgs, | ||
} as RevertReasonWithCustomError; | ||
} | ||
|
||
return { | ||
reason: error.innerError.message, | ||
signature: error.innerError.data?.slice(0, 10), | ||
data: error.innerError.data?.substring(10), | ||
} as RevertReason; | ||
} | ||
|
||
if ( | ||
error instanceof InvalidResponseError && | ||
!Array.isArray(error.innerError) && | ||
error.innerError !== undefined | ||
) { | ||
return error.innerError.message; | ||
} | ||
|
||
throw error; | ||
return parseTransactionError(error, contractAbi); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added request to the error object as I wanted to add sent transaction details to failed transaction errors to give the user more context (e.g. the sending account when the error
Unknown account
orInsufficient funds
is thrown) on what exactly was sent to provider