Skip to content
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

Possible incorrect parsing of receipt status #2183

Closed
gabmontes opened this issue Jan 11, 2019 · 3 comments · Fixed by #2429
Closed

Possible incorrect parsing of receipt status #2183

gabmontes opened this issue Jan 11, 2019 · 3 comments · Fixed by #2429
Labels
Bug Addressing a bug

Comments

@gabmontes
Copy link
Contributor

The Byzantinum fork, IIRC, enabled adding the status to the transaction receipts. But for chains that did not activate it, the status is still null instead of 0x0 or 0x1. Following is the code that parses it to boolean:

https://github.com/ethereum/web3.js/blob/1bbfc5a36f4422a79bb0abfdd4b30aba6b3dc340/packages/web3-core-helpers/src/formatters.js#L219-L221

When testing with some transactions in a chain that did not fork, we saw the status field was incorrectly parsed as false for receipts with status coming as null, which might not be correct as status is actually not provided. This is not enough to say the transaction failed or not.

The condition might be changed to something like the following to properly account for null, 0x0 and 0x1 values:

    if(typeof receipt.status === 'string') {
        receipt.status = Boolean(parseInt(receipt.status));
    }

@frozeman & team, can you please provide your thoughts?

@nivida
Copy link
Contributor

nivida commented Jan 18, 2019

Thanks for submitting this issue. That's true will change it as soon as possible.

@gabmontes
Copy link
Contributor Author

@alcuadrado @cgewecke this issue was patched in 1ac78d1 @ #2429 on the 2.x branch but that PR cannot be backported to 1.x. Actually, the only line that needs to be backported, strictly speaking, is this:

--- a/packages/web3-core-helpers/src/formatters.js
+++ b/packages/web3-core-helpers/src/formatters.js
@@ -216,7 +216,7 @@ var outputTransactionReceiptFormatter = function (receipt){
         receipt.contractAddress = utils.toChecksumAddress(receipt.contractAddress);
     }
 
-    if(typeof receipt.status !== 'undefined') {
+    if(typeof receipt.status !== 'undefined' && receipt.status !== null) {
         receipt.status = Boolean(parseInt(receipt.status));
     }

Issue #3070 says "change causes many 1.x test failures" but it looks like applying this patch on top of v1.2.1 does not break any test.

  2495 passing (25s)

Of course some tests should be added to cover the case of a receipt being returned as null.

@cgewecke
Copy link
Collaborator

cgewecke commented Oct 7, 2019

@gabmontes Ok excellent. Will open a PR fixing rn Will update #3090 to include this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Addressing a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants