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.

Sign up for GitHub

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

Prefer receipt status when checking for OOG #3189

Closed
wants to merge 4 commits into from
Closed

Prefer receipt status when checking for OOG #3189

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e6c8fa
Defer to receipt status when checking OOG
cgewecke Nov 7, 2019
6673659
Update changelog
cgewecke Nov 7, 2019
b24dfd5
Missing issue #
cgewecke Nov 7, 2019
bafe3e9
Remove 0x1 status check, incorrect test
cgewecke Nov 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,5 +89,6 @@ Released with 1.0.0-beta.37 code base.

### Fixed

- Fix incorrectly errors on tx success when using perfect gas estimates (#3175)
- Fix TS types for eth.subscribe syncing, newBlockHeaders, pendingTransactions (#3159)
- Improve web3-eth-abi decodeParameters error message (#3134)
6 changes: 5 additions & 1 deletion packages/web3-core-method/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,12 @@ Method.prototype._confirmTransaction = function(defer, result, payload) {
// CHECK for normal tx check for receipt only
.then(function(receipt) {
if (!isContractDeployment && !promiseResolved) {

var hasStatus = receipt.status === false || receipt.status === true;
var isOOG = !hasStatus && gasProvided === utils.numberToHex(receipt.gasUsed);

if (!receipt.outOfGas &&
(!gasProvided || gasProvided !== utils.numberToHex(receipt.gasUsed)) &&
(!gasProvided || !isOOG) &&
(receipt.status === true || receipt.status === '0x1' || typeof receipt.status === 'undefined')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • What exactly are the thoughts behind hasStatus? (Because the status field does always exists in post Byzantium hard fork chains)
  • The if conditions who check the status based on the hex value e.g.: receipt.status === '0x1' aren't required because the receipt.status property does get type casted to a boolean value type in the outputTransactionReceiptFormatter of the used getTransactionReceipt method.
    • If the status property is of type undefined or null is no type cast applied.
  • Shouldn't isOOG be like this: isOOG = !receipt.status && gasProvided === utils.numberToHex(receipt.gasUsed); ?
  • Shouldn't we add here the isOOG condition as well?
    • I would propose those conditions: 
        if(receipt.status === true || receipt.status === null || typeof receipt.status === 'undefined' ) {
    // resolve with receipt
  } else {
    if (gasProvided === utils.numberToHex(receipt.gasUsed)) {
      // reject with out of gas
    } else {
      // reject with reverted by EVM
    }
  }

Copy link
Collaborator Author

@cgewecke cgewecke Nov 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly are the thoughts behind hasStatus

My thought is that the reason for the original correction in #3123 is to support non-byzantium chains where gasUsed === gasSent is (almost) logically equivalent to status === false. What is your view of this?

'0x1' aren't required

👍 Will remove

Shouldn't isOOG be like this: isOOG = !receipt.status...

That was my initial thought as well. But it's a problem when people combine estimateGas with tx's that actually revert. In that case receipt status is false and exact gas is consumed. See this added test. My theory is that preferring the status / ignoring the gas is closest to what the 1.2.1 behavior is.

If you look at what GabMontes suggests in #3175, there are other receipt fields they're looking at to disambiguate real OOG from revert-like transaction failure. I'm ambivalent about adding those without being able to add a test against an ETC client here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I just realized the test has a copy-pasta mistake in it. Let me investigate this further...

defer.eventEmitter.emit('receipt', receipt);
defer.resolve(receipt);
Expand Down
37 changes: 37 additions & 0 deletions test/e2e.method.send.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,20 @@ describe('method.send [ @E2E ]', function() {
assert(web3.utils.isHexStrict(receipt.transactionHash));
});

it('succeeds when using a perfect estimate', async function(){
var estimate = await instance
.methods
.setValue('1')
.estimateGas();

var receipt = await instance
.methods
.setValue('1')
.send({from: accounts[0], gas: estimate});

assert(receipt.status === true);
});

it('errors on OOG', async function(){
try {
await instance
Expand Down Expand Up @@ -65,6 +79,29 @@ describe('method.send [ @E2E ]', function() {
assert(receipt.status === false);
}
});

it('errors on revert using a perfect estimate', async function(){
var estimate = await instance
.methods
.setValue('1')
.estimateGas();

try {
await instance
.methods
.reverts()
.send({from: accounts[0], gas: estimate});

assert.fail();

} catch(err){
var receipt = utils.extractReceipt(err.message);

assert(err.message.includes('revert'))
assert(receipt.status === false);
}

});
});

describe('ws', function() {
Expand Down