Merge pull request #2387 from ethereum/issues/2381

Validation of the gas usage fixed in TransactionReceiptValidator
web3 · Feb 19, 2019 · ff3038e · ff3038e
2 parents 1f98597 + 15f7e3b
commit ff3038e
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 56 deletions.
diff --git a/packages/web3-core-method/src/validators/TransactionReceiptValidator.js b/packages/web3-core-method/src/validators/TransactionReceiptValidator.js
@@ -20,23 +20,21 @@
  * @date 2018
  */
 
-import isObject from 'lodash/isObject';
-
 export default class TransactionReceiptValidator {
     /**
      * Validates the receipt
      *
      * @method validate
      *
      * @param {Object} receipt
-     * @param {Array} methodParameters
+     * @param {AbstractMethod} method
      *
      * @returns {Error|Boolean}
      */
-    validate(receipt, methodParameters) {
+    validate(receipt, method) {
         const receiptJSON = JSON.stringify(receipt, null, 2);
 
-        if (!this.isValidGasUsage(receipt, methodParameters)) {
+        if (!this.isValidGasUsage(receipt, method)) {
             return new Error(`Transaction ran out of gas. Please provide more gas:\n${receiptJSON}`);
         }
 
@@ -66,17 +64,11 @@ export default class TransactionReceiptValidator {
      * @method isValidGasUsage
      *
      * @param {Object} receipt
-     * @param {Array} methodParameters
+     * @param {AbstractMethod} method
      *
      * @returns {Boolean}
      */
-    isValidGasUsage(receipt, methodParameters) {
-        let gasProvided = null;
-
-        if (isObject(methodParameters[0]) && methodParameters[0].gas) {
-            gasProvided = methodParameters[0].gas;
-        }
-
-        return !receipt.outOfGas && (!gasProvided || gasProvided !== receipt.gasUsed);
+    isValidGasUsage(receipt, method) {
+        return !receipt.outOfGas && method.utils.hexToNumber(method.parameters[0].gas) !== receipt.gasUsed;
     }
 }
diff --git a/packages/web3-core-method/src/workflows/TransactionConfirmationWorkflow.js b/packages/web3-core-method/src/workflows/TransactionConfirmationWorkflow.js
@@ -55,7 +55,7 @@ export default class TransactionConfirmationWorkflow {
 
         this.getTransactionReceiptMethod.execute(moduleInstance).then((receipt) => {
             if (receipt && receipt.blockHash) {
-                const validationResult = this.transactionReceiptValidator.validate(receipt, method.parameters);
+                const validationResult = this.transactionReceiptValidator.validate(receipt, method);
                 if (validationResult === true) {
                     this.handleSuccessState(receipt, method, promiEvent);
 
@@ -72,10 +72,7 @@ export default class TransactionConfirmationWorkflow {
                 if (!this.isTimeoutTimeExceeded(moduleInstance, this.newHeadsWatcher.isPolling)) {
                     this.getTransactionReceiptMethod.execute(moduleInstance).then((receipt) => {
                         if (receipt && receipt.blockHash) {
-                            const validationResult = this.transactionReceiptValidator.validate(
-                                receipt,
-                                method.parameters
-                            );
+                            const validationResult = this.transactionReceiptValidator.validate(receipt, method);
 
                             if (validationResult === true) {
                                 this.confirmationsCounter++;

diff --git a/packages/web3-core-method/tests/src/validators/TransactionReceiptValidatorTest.js b/packages/web3-core-method/tests/src/validators/TransactionReceiptValidatorTest.js
@@ -1,10 +1,14 @@
+import * as Utils from 'web3-utils';
 import TransactionReceiptValidator from '../../../src/validators/TransactionReceiptValidator';
 
+// Mocks
+jest.mock('Utils');
+
 /**
  * TransactionReceiptValidator test
  */
 describe('TransactionReceiptValidatorTest', () => {
-    let transactionReceiptValidator, receipt;
+    let transactionReceiptValidator, receipt, method;
 
     beforeEach(() => {
         receipt = {
@@ -13,52 +17,63 @@ describe('TransactionReceiptValidatorTest', () => {
             gasUsed: 100
         };
 
+        method = {};
+        method.utils = Utils;
+
         transactionReceiptValidator = new TransactionReceiptValidator();
     });
 
     it('calls validate and returns true', () => {
-        expect(
-            transactionReceiptValidator.validate(receipt, [
-                {
-                    gas: 110
-                }
-            ])
-        ).toEqual(true);
+        Utils.hexToNumber.mockReturnValueOnce(110);
+
+        method.parameters = [
+            {
+                gas: 110
+            }
+        ];
+
+        expect(transactionReceiptValidator.validate(receipt, method)).toEqual(true);
+
+        expect(Utils.hexToNumber).toHaveBeenCalledWith(110);
     });
 
-    it(
-        'calls validate and returns error because of invalid gasUsage',
-        () => {
-            const error = transactionReceiptValidator.validate(receipt, [
-                {
-                    gas: 100
-                }
-            ]);
-
-            expect(error).toBeInstanceOf(Error);
-
-            expect(error.message).toEqual(
-                `Transaction ran out of gas. Please provide more gas:\n${JSON.stringify(receipt, null, 2)}`
-            );
-        },
-        [
+    it('calls validate and returns error because of invalid gasUsage', () => {
+        Utils.hexToNumber.mockReturnValueOnce(100);
+
+        method.parameters = [
             {
-                gas: 100
+                gas: 110
             }
-        ]
-    );
+        ];
+
+        const error = transactionReceiptValidator.validate(receipt, method);
+
+        expect(error).toBeInstanceOf(Error);
+
+        expect(error.message).toEqual(
+            `Transaction ran out of gas. Please provide more gas:\n${JSON.stringify(receipt, null, 2)}`
+        );
+
+        expect(Utils.hexToNumber).toHaveBeenCalledWith(110);
+    });
 
     it('calls validate and returns error because the EVM has reverted it', () => {
-        receipt.status = false;
+        Utils.hexToNumber.mockReturnValueOnce(110);
 
-        const error = transactionReceiptValidator.validate(receipt, [
+        method.parameters = [
             {
                 gas: 101
             }
-        ]);
+        ];
+
+        receipt.status = false;
+
+        const error = transactionReceiptValidator.validate(receipt, method);
 
         expect(error).toBeInstanceOf(Error);
 
         expect(error.message).toEqual(`Transaction has been reverted by the EVM:\n${JSON.stringify(receipt, null, 2)}`);
+
+        expect(Utils.hexToNumber).toHaveBeenCalledWith(101);
     });
 });
diff --git a/packages/web3-core-method/tests/src/workflows/TransactionConfirmationWorkflowTest.js b/packages/web3-core-method/tests/src/workflows/TransactionConfirmationWorkflowTest.js
@@ -82,7 +82,7 @@ describe('TransactionConfirmationWorkflowTest', () => {
 
             expect(getTransactionReceiptMethodMock.execute).toHaveBeenCalledWith(moduleInstanceMock);
 
-            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: true}, []);
+            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: true}, methodMock);
 
             expect(transactionConfirmationWorkflow.timeoutCounter).toEqual(0);
 
@@ -158,7 +158,10 @@ describe('TransactionConfirmationWorkflowTest', () => {
 
             expect(contractDeployMethodMock.afterExecution).toHaveBeenCalledWith({blockHash: '0x0'});
 
-            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: '0x0'});
+            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith(
+                {blockHash: '0x0'},
+                contractDeployMethodMock
+            );
         });
     });
 
@@ -216,7 +219,7 @@ describe('TransactionConfirmationWorkflowTest', () => {
 
             expect(methodMock.afterExecution).toHaveBeenCalledWith({blockHash: '0x0'});
 
-            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: '0x0'});
+            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: '0x0'}, methodMock);
 
             done();
         });
@@ -260,10 +263,7 @@ describe('TransactionConfirmationWorkflowTest', () => {
 
             expect(newHeadsWatcherMock.stop).toHaveBeenCalled();
 
-            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith(
-                {blockHash: '0x0'},
-                methodMock.parameters
-            );
+            expect(transactionReceiptValidatorMock.validate).toHaveBeenCalledWith({blockHash: '0x0'}, methodMock);
 
             expect(getTransactionReceiptMethodMock.execute).toHaveBeenNthCalledWith(2, moduleInstanceMock);