fix rejected tx error reporting, add tests, and clean up sundry other…

… things which were in the way

fixes #471

lib/provider.js

@@ -9,6 +9,7 @@ var GethApiDouble = require('./subproviders/geth_api_double');
 var BlockTracker = require('./block_tracker');
 
 var RuntimeError = require("./utils/runtimeerror");
+var TXRejectedError = require("./utils/txrejectederror");
 var EventEmitter = require('events');
 
 var clone = require('clone')
@@ -141,6 +142,13 @@ Provider.prototype._processRequestQueue = function() {
 }
 
 Provider.prototype.cleanUpErrorObject = function(err, response) {
+  // Our response should already have an error field at this point, if it
+  // doesn't, this was likely intentional. If not, this is the wrong place to
+  // fix that problem.
+  if (!err || !response.error) {
+    return clone(response)
+  }
+
   var errorObject = {
     error: {}
   }
@@ -168,47 +176,39 @@ var transactionMethods = [
   'personal_sendTransaction'
 ];
 
-// helper list of RPC methods which execute code regardless of what their response object looks like
-var executingMethods = [
-  'eth_sendTransaction',
-  'eth_sendRawTransaction',
-  'eth_call',
-  'eth_estimateGas',
-  'debug_traceTransaction'
-];
-
 Provider.prototype._isTransactionRequest = function(request) {
   return transactionMethods.indexOf(request.method) != -1;
 }
 
-Provider.prototype._isExecutingRequest = function(request) {
-  return executingMethods.indexOf(request.method) != -1;
-}
-
 Provider.prototype.reportErrorInResponse = function(request, err, response) {
   const self = this;
   var newResponse = clone(response);
 
   if (!err) return newResponse;
 
-  // make sure we always return the transaction hash on failed transactions so
-  // the caller can get their tx receipt
-  if (self._isTransactionRequest(request) && err.hashes) {
-    newResponse.result = err.hashes[0];
+  // TODO: for next major release: move reporting of tx hash on error to error
+  // field to prevent poorly-written clients which assume that the existence of
+  // the "result" field implies no errors from breaking.
+  if (self._isTransactionRequest(request)) {
+    if (err instanceof RuntimeError) {
+      // Make sure we always return the transaction hash on failed transactions so
+      // the caller can get their tx receipt. This breaks JSONRPC 2.0, but it's how
+      // we've always done it.
+      newResponse.result = err.hashes[0]
+
+      // If we executed code as part of this request, check whether we're supposed
+      // to cause web3 and friends to throw by including the error field in our
+      // response. If we don't report an error, the caller must fetch inspect the
+      // `status` field of the receipt to detect an error, and check the ganache
+      // logging output to see the error message. This is how all major clients
+      // work today, so this is the default option.
+      if (!self.options.vmErrorsOnRPCResponse) {
+        delete newResponse.error
+      }
+    }
   }
 
-  // If we executed code as part of this request, check whether we're supposed
-  // to cause web3 and friends to throw by including the error field in our
-  // response. If we don't report an error, the caller must fetch inspect the
-  // `status` field of the receipt to detect an error, and check the ganache
-  // logging output to see the error message. This is how all major clients
-  // work today, so this is the default option.
-  if (self._isExecutingRequest(request) && !self.options.vmErrorsOnRPCResponse) {
-    delete newResponse.error
-    return newResponse
-  } else {
-    return self.cleanUpErrorObject(err, newResponse)
-  }
+  return self.cleanUpErrorObject(err, newResponse)
 }
 
 module.exports = Provider;

lib/statemanager.js

@@ -18,6 +18,7 @@ var util = require("util");
 var to = require('./utils/to');
 var random = require('./utils/random');
 var txhelper = require('./utils/txhelper');
+var TXRejectedError = require('./utils/txrejectederror');
 
 StateManager = function(options) {
   var self = this;
@@ -100,14 +101,15 @@ StateManager.prototype.initialize = function(options, callback) {
 
   accounts.forEach(function(data) {
     self.accounts[data.address] = data;
+    self.personal_accounts[data.address.toLowerCase()] = true;
   });
 
   // Turn array into object, mostly for speed purposes.
   // No need for caller to specify private keys.
   this.unlocked_accounts = (options.unlocked_accounts || []).reduce(function(obj, address) {
     // If it doesn't have a hex prefix, must be a number (either a string or number type).
     if ((address + "").indexOf("0x") != 0) {
-      address = accounts[parseInt(address)].address;
+      address = accounts[parseInt(address)].address.toLowerCase();
     }
 
     obj[address.toLowerCase()] = true; // can be any value
@@ -116,7 +118,7 @@ StateManager.prototype.initialize = function(options, callback) {
 
   if (!this.secure) {
     accounts.forEach(function(data) {
-      self.unlocked_accounts[data.address] = data;
+      self.unlocked_accounts[data.address.toLowerCase()] = data;
     });
   }
 
@@ -187,7 +189,7 @@ StateManager.prototype.createAccount = function(opts, i) {
   var data = {
     secretKey: secretKey,
     publicKey: publicKey,
-    address: to.hex(address),
+    address: to.hex(address).toLowerCase(),
     account: account
   };
 
@@ -261,15 +263,15 @@ StateManager.prototype.queueStorage = function(address, position, block, callbac
 
 StateManager.prototype.queueTransaction = function(method, tx_params, callback) {
   if (tx_params.from == null) {
-    callback(new Error("from not found; is required"));
+    callback(new TXRejectedError("from not found; is required"));
     return;
   }
 
   // use toLowerCase() to properly handle from addresses meant to be validated.
   tx_params.from = utils.addHexPrefix(tx_params.from).toLowerCase();
 
   if (method == "eth_sendTransaction" && this.unlocked_accounts[tx_params.from] == null) {
-    return callback(new Error("could not unlock signer account"));
+    return callback(new TXRejectedError("could not unlock signer account"));
   }
 
   var rawTx = {
@@ -309,12 +311,12 @@ StateManager.prototype.queueTransaction = function(method, tx_params, callback)
 
   // Error checks
   if (rawTx.to && typeof rawTx.to != "string") {
-    return callback(new Error("Invalid to address"));
+    return callback(new TXRejectedError("Invalid to address"));
   }
 
   // If the transaction has a higher gas limit than the block gas limit, error.
   if (to.number(rawTx.gasLimit) > to.number(this.blockchain.blockGasLimit)) {
-    return callback(new Error("Exceeds block gas limit"));
+    return callback(new TXRejectedError("Exceeds block gas limit"));
   }
 
   // Get the nonce for this address, taking account any transactions already queued.
@@ -406,7 +408,7 @@ StateManager.prototype.processNextAction = function(override) {
 };
 
 StateManager.prototype.sign = function(address, dataToSign) {
-  var account = this.accounts[to.hex(address)];
+  var account = this.accounts[to.hex(address).toLowerCase()];
 
   if (!account) {
     throw new Error("cannot sign data; no private key");
@@ -758,7 +760,7 @@ StateManager.prototype.isUnlocked = function(address) {
 
 StateManager.prototype.createFakeTransactionWithCorrectNonce = function(rawTx, from, callback) {
   const self = this;
-  self.blockchain.getQueuedNonce(from, (err, nonce) => {
+  self.blockchain.getQueuedNonce(from, (err, expectedNonce) => {
     if (err) return callback(err);
 
     var tx = new FakeTransaction(rawTx);
@@ -767,7 +769,13 @@ StateManager.prototype.createFakeTransactionWithCorrectNonce = function(rawTx, f
     // If the user specified a nonce, use that instead.
     if (rawTx.nonce == null) {
       // account for transactions waiting in the tx queue
-      tx.nonce = to.hex(nonce);
+      tx.nonce = to.hex(expectedNonce);
+    } else {
+      if (to.number(rawTx.nonce) !== to.number(expectedNonce)) {
+        return callback(new TXRejectedError(`the tx doesn't have the correct nonce. ` +
+        `account has nonce of: ${to.number(expectedNonce)} ` +
+        `tx has nonce of: ${to.number(tx.nonce)}`))
+      }
     }
     callback(null, tx);
   });

lib/subproviders/geth_api_double.js

@@ -407,26 +407,26 @@ GethApiDouble.prototype.personal_newAccount = function(password, callback) {
 
 GethApiDouble.prototype.personal_importRawKey = function(rawKey, password, callback) {
   var account = this.state.createAccount({ secretKey: rawKey });
-  this.state.accounts[account.address] = account;
-  this.state.personal_accounts[account.address] = true;
-  this.state.account_passwords[account.address] = password;
+  this.state.accounts[account.address.toLowerCase()] = account;
+  this.state.personal_accounts[account.address.toLowerCase()] = true;
+  this.state.account_passwords[account.address.toLowerCase()] = password;
   callback(null, account.address);
 };
 
 GethApiDouble.prototype.personal_lockAccount = function(address, callback) {
   var account = this.state.personal_accounts[address.toLowerCase()];
   if (account !== true) {
-    return callback("Account not found")
+    return callback("Account not found");
   }
-  this.state.unlocked_accounts[address.toLowerCase()] = false;
+  delete this.state.unlocked_accounts[address.toLowerCase()];
   callback(null, true);
 };
 
 GethApiDouble.prototype.personal_unlockAccount = function(address, password, duration, callback) {
   // FIXME handle duration
   var account = this.state.personal_accounts[address.toLowerCase()];
   if (account !== true) {
-    return callback("Account not found")
+    return callback("Account not found");
   }
 
   var storedPassword = this.state.account_passwords[address.toLowerCase()];

lib/utils/runtimeerror.js

@@ -6,9 +6,16 @@ inherits(RuntimeError, Error);
 // Note: ethereumjs-vm will return an object that has a "results" and "receipts" keys.
 // You should pass in the whole object.
 function RuntimeError(transactions, vm_output) {
-  Error.call(this);
+
+  // Why not just Error.apply(this, [message])? See
+  // https://gist.github.com/justmoon/15511f92e5216fa2624b#anti-patterns
+  Error.captureStackTrace(this, this.constructor)
+  this.name = this.constructor.name;
+
   this.results = {};
   this.hashes = [];
+
+  // handles creating this.message
   this.combine(transactions, vm_output);
 };
 

lib/utils/txrejectederror.js

@@ -0,0 +1,15 @@
+var inherits = require("util").inherits;
+
+// raised when the transaction is rejected prior to running it in the EVM.
+function TXRejectedError(message) {
+
+  // Why not just Error.apply(this, [message)? See
+  // https://gist.github.com/justmoon/15511f92e5216fa2624b#anti-patterns
+  Error.captureStackTrace(this, this.constructor)
+  this.name = this.constructor.name;
+  this.message = message;
+};
+
+inherits(TXRejectedError, Error);
+
+module.exports = TXRejectedError;

test/accounts.js

@@ -35,7 +35,10 @@ describe("Accounts", function() {
       gasLimit: 90000
     }, function(err, tx) {
       if (!err) return done(new Error("We expected the account to be locked, which should throw an error when sending a transaction"));
-      assert(err.message.toLowerCase().indexOf("could not unlock signer account") >= 0);
+      assert(err.message.toLowerCase().indexOf("could not unlock signer account") >= 0,
+        'Expected error message containing "could not unlock signer account" ' +
+        '(case insensitive check). Received the following error message, instead. ' +
+        `"${err.message}"`)
       done();
     });
   });

test/hex.js

@@ -30,15 +30,24 @@ describe("to.rpcQuantityHexString", function() {
   });
 });
 
-function noLeadingZeros(result) {
+function noLeadingZeros(method, result, path) {
+  if (!path) {
+    path = 'result'
+  }
+
   if (typeof result === "string") {
     if (/^0x/.test(result)) {
-      assert.equal(result, to.rpcQuantityHexString(result));
+      assert.equal(result, to.rpcQuantityHexString(result), 'Field ${path} in ${method} response has leading zeroes.');
     }
   } else if (typeof result === "object") {
     for (var key in result) {
       if (result.hasOwnProperty(key)) {
-        noLeadingZeros(result[key]);
+        if (Array.isArray(result)) {
+          path += [key]
+        } else {
+          path += '.' + key
+        }
+        noLeadingZeros(method, result[key], path + (path ? '.' : '') + key);
       }
     }
   }
@@ -58,7 +67,7 @@ describe("JSON-RPC Response", function() {
     });
   });
 
-  it("should not have leading zeros in hex strings", function(done) {
+  it("should not have leading zeros in rpc quantity hex strings", function(done) {
     var request = {
       "jsonrpc": "2.0",
       "method": "eth_getTransactionCount",
@@ -70,7 +79,7 @@ describe("JSON-RPC Response", function() {
     };
 
     provider.sendAsync(request, function(err, result) {
-      noLeadingZeros(result);
+      noLeadingZeros('eth_getTransactionCount', result);
 
       request = {
         "jsonrpc": "2.0",
@@ -86,7 +95,7 @@ describe("JSON-RPC Response", function() {
       };
 
       provider.sendAsync(request, function(err, result) {
-        noLeadingZeros(result);
+        noLeadingZeros('eth_sendTransaction', result);
 
         request = {
           "jsonrpc": "2.0",
@@ -99,7 +108,7 @@ describe("JSON-RPC Response", function() {
         };
 
         provider.sendAsync(request, function(err, result) {
-          noLeadingZeros(result);
+          noLeadingZeros('eth_getTransactionCount', result);
           done();
         });
       });

test/requests.js

@@ -398,7 +398,7 @@ var tests = function(web3) {
       transaction.sign(secretKeyBuffer)
 
       web3.eth.sendSignedTransaction(transaction.serialize(), function(err, result) {
-        assert(err.message.indexOf("the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: 0") >= 0);
+        assert(err.message.indexOf("the tx doesn't have the correct nonce. account has nonce of: 1 tx has nonce of: 0") >= 0, `Incorrect error message: ${err.message}`);
         done()
       })
 
@@ -1066,7 +1066,7 @@ var tests = function(web3) {
     it("should return more than 0 accounts", function(done) {
       web3.eth.personal.getAccounts(function(err, result) {
         if (err) return done(err);
-        assert.equal(result.length, 3);
+        assert.equal(result.length, 13);
         done();
       });
     });