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

Fix issue with boolean typed receipt status #2548

Closed
wants to merge 1 commit into from

Conversation

angus-hamill
Copy link

@angus-hamill angus-hamill commented Mar 21, 2019

Description

The status field of the transaction receipt is sometimes returned as a boolean, not hex. This was causing web3 to think the transaction reverted when it had actually succeeded.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Enhancement

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 warnings.
  • I have updated or added types for all modules I've changed
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test in the root folder with success and extended the tests if necessary.
  • I ran npm run build in the root folder and tested it in the browser and with node.
  • I ran npm run dtslint in the root folder and tested that all my types are correct
  • I have tested my code on an ethereum test network.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 98.653% when pulling 2f8e92f on angus-hamill:fix-ganache-revert-error into 1a7c600 on ethereum:1.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 98.653% when pulling 2f8e92f on angus-hamill:fix-ganache-revert-error into 1a7c600 on ethereum:1.0.

@chris-shyft
Copy link

awesome, some of my team was having this issue in certain system configurations

@nivida
Copy link
Contributor

nivida commented Mar 22, 2019

This isn't an issue of Web3.js. Ganache has to be aligned with the JSON-RPC spec.

@nivida nivida closed this Mar 22, 2019
@angus-hamill angus-hamill deleted the fix-ganache-revert-error branch March 22, 2019 10:42
@angus-hamill
Copy link
Author

angus-hamill commented Mar 22, 2019

@nivida
I was mistaken in thinking this was a ganache only issue. While ganache does emit that status field as a boolean sometimes (which should be fixed) there is also a web3js issue.

As you see here the observer is calling the GetTransactionReceiptMethod which calls a formatter as part of its execution here.

This casts the status field to a boolean here.

I guess my fix is still too hacky for this though, what do you propose? Should the transaction receipt formatter still be casting to a bool like this?

@dileepfrog
Copy link

I wouldn't consider this hacky, but rather robustifying the receipt status check to handle varying upstream formatting with consistent semantics. Just my $0.02

@SwissArmyBud
Copy link

SwissArmyBud commented Mar 26, 2019

@nivida This error is thrown when using Web3 against Geth 1.8 - no Ganache anywhere in the ecosystem. Here's my receipt from a Geth IPC connection in Web3:

{ blockHash:
   '0x7edf444c9920e6bcdbbc6597d8089d0f454a162870e4e204f1e20db5b8fcc0d1',
  blockNumber: 4,
  contractAddress: '0x88EbAB3EFe955D427D817D50a1C9444aC9DdB7ab',
  cumulativeGasUsed: 569523,
  from: '0xf6f63d9596ed1d10cf6191fc448ad4af980e0222',
  gasUsed: 569523,
  logs: [],
  logsBloom:
   '0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000',
  status: true,
  to: null,
  transactionHash:
   '0x7081793c182edd6100bc67b4f51941c7847cdeb26bf6cc53a1b57a7fc0305c0f',
  transactionIndex: 0 }

And a Node.js REPL can demonstrate the error:

> var rc = new Object({status: true})
{ status: true }
> rc.status
true
> parseInt(rc.status)
NaN
> Boolean(parseInt(rc.status))
false

The parsing scheme is located in two places in Web3.js and prevents legitimately completed transactions being parsed without a large error handling block:

let receipt = await web3Contract.deploy({ data: contract.bytecode })
                      .send( contract.options )
                      .on('transactionHash', (tx) => {
                        console.log("[DEPLOY] -> Tx Hash: " + tx)
                      }).catch(e => {
                        if( e.toString().indexOf("reverted by the EVM:") == -1 ){
                          return undefined;
                        } else {
                          var er = e.toString();
                          try{
                            var jsonIndex = {
                              start: er.indexOf( "{", er.indexOf("EVM:")),
                              end: er.indexOf( "}", er.indexOf("logsBloom")) + 1
                            }
                            er = JSON.parse( er.substring( jsonIndex.start, jsonIndex.end ) );
                            return er;
                          } catch (error) {
                            console.log("[DEPLOY] -> WARNING - WEB3 REPORTS EVM ROLLBACK, AND BAD RECEIPT PARSE...");
                            console.log(er);
                            console.log(error);
                            // Our error parsing has failed - no guaranteed state, return undefined
                            return undefined;
                          }
                        }
                      });

@angus-hamill
Copy link
Author

angus-hamill commented Mar 27, 2019

@SwissArmyBud
The status field was always (or at least for a long time) returned as a boolean when you queried it directly to the node with web3js.
The problem is it's now using this get transaction receipt method with the formatter internally as of #2466 (in the place I highlighted in my comment above)

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

Successfully merging this pull request may close these issues.

6 participants