Fix Error object creation resulting in failing blockchain tests #197
Conversation
| @@ -177,7 +177,7 @@ module.exports = function (opts, cb) { | |||
| error: err | |||
There was a problem hiding this comment.
Is this field expected to be an Error object or text?
There was a problem hiding this comment.
I just stayed in the semantics of the change with the new Error object introduced before.
Tested in CL, this works if an Error object is provided, but it uses just the error description from the first error and not the string. Is this acceptable? I'm not so deep into error handling...
| @@ -177,7 +177,7 @@ module.exports = function (opts, cb) { | |||
| error: err | |||
| } | |||
|
|
|||
| afterBlock(cb.bind(this, new Error(err), result)) | |||
| afterBlock(cb.bind(this, err, result)) | |||
There was a problem hiding this comment.
Is the input err to this method (parseBlockResults) always an Error instance?
There was a problem hiding this comment.
I think this was changed in the last weeks on many places to always use an Error object and not a plain string, wasn't it? Not sure though if this was a concerted effort to change this on all places.
It's probably nevertheless not a good solution to start with checks like
if (typeof(err) === 'string') {
err = new Error(err)
}
at this place.
There was a problem hiding this comment.
The Error/string types seem compatible in node and the browser, so I guess this is fine.
There was a problem hiding this comment.
@holgerd77 I meant that this function receives err and only sets it to an Error in certain places, but there are no safeguards in place that the incoming err is an instance of Error.
|
The invalid block error was being swallowed, so I added a line to log it. Any reason not to log it to the console? |
Since this is a library it is not supposed to log to the console :) |
toBuffer now throws if strings aren't 0x-prefixed hex values
No description provided.