toBuffer now throws if strings aren't 0x-prefixed hex values

[alcuadrado]

I made this in response to the conversation we had in this other PR.

As we agreed three, toBuffer had a surprising and error-prone behavior when a string other than 0x-prefixed hex one was given. This PR removes that behavior, throwing a clear error instead.

There were a few tests using this older behavior (i.e. passing utf-8 string), but that was not necessary and they were harder to read, so I replaced with equivalent 0x-prefixed hex strings.

keccak was also using and exposing this behavior, which makes sense as people sometimes hash strings. I implemented it inside keccack and documented it.

Finally, hashPersonalMessage was using it internally, but not exposing it, so I just updated it to an equivalent lower-level implementation.

Technically, this PR introduces a breaking change in toBuffer, but I think virtually all of the uses of toBuffer with non-hex strings were errors, as its behavior was unexpected and undocumented.

[coveralls]

Coverage increased (+0.007%) to 99.333% when pulling f0df622 on toBuffer-unrecognized-strings into 323e600 on master.

[alcuadrado]

Travis is failing with node 6 because the autoformat sometimes adds commas after the last argument in a function call. This is not supported in node 6.

This is an example of that pattern:

assert.equal(
      ethUtils.MAX_INTEGER.toString('hex'),
      'ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff',
    )

Are we still supporting node 6? It's not maintained anymore.

[holgerd77]

Thanks for the PR, had a first look but need some more time to answer.

One first thing: can you please generally don't reformat code and remove the commit with the format changes in the tests file?

Some broader first note: maybe we want to take this occasion and generally do the API overhaul discussed here #172, eventually you already want to have a look and give this your own thoughts. This goes a bit in the direction of this PR, things have to be re-read a bit since we won't use a special HexString class for now which I proposed there.

@s1na do we eventually do this API overhaul a two-step process and first start with updates similar to the one being done in this PR and do integrations of your new types like the Address type #186 as a second step on top?

[s1na]

I personally agree with this approach (and probably would go further and limit the APIs to only Buffer and throw an error otherwise 😄 ).

@s1na do we eventually do this API overhaul a two-step process and first start with updates similar to the one being done in this PR and do integrations of your new types like the Address type #186 as a second step on top?

Yeah, I think that makes sense. They can even be possibly worked on in parallel as there's no conflict in between.

[alcuadrado]

One first thing: can you please generally don't reformat code and remove the commit with the format changes in the tests file?

Sure! I thought the idea was to always reformat everything with ethereumjs-config-format-fix and ethereumjs-config-lint-fix. Should tests be excluded from this? Or how is this handled?

I updated it.

Some broader first note: maybe we want to take this occasion and generally do the API overhaul discussed here #172, eventually you already want to have a look and give this your own thoughts. This goes a bit in the direction of this PR, things have to be re-read a bit since we won't use a special HexString class for now which I proposed there.

I'll go through that issue. Thanks for the link.

I personally agree with this approach (and probably would go further and limit the APIs to only Buffer and throw an error otherwise 😄 ).

I'd love this, but wouldn't that mean breaking changes in all of ethereumjs?

[holgerd77]

@alcuadrado With EthereumJS we are generally at the point where we can't have substantial progress to a greater extend any more if we don't do bigger steps and more significant changes on various libraries. We shouldn't do breaking stuff just for the change/breaking 😋, but if there is significant reasoning to do so we should absolutely do so to prepare some more solid ground for the future.

The ethereumjs-util library e.g. is definitely a candidate for some bigger changes especially on the API front. With dependency updates this is not too much of a problem. There is no need to do everything at once and we can just update "on occasion", we are already doing this regularly with things like that and that is the normal work progress work-flow to slowly but steadily evolve on all fronts.

[holgerd77]

@alcuadrado can you please rebase your branch for a review?

[alcuadrado]

@alcuadrado With EthereumJS we are generally at the point where we can't have substantial progress to a greater extend any more if we don't do bigger steps and more significant changes on various libraries. We shouldn't do breaking stuff just for the change/breaking 😋, but if there is significant reasoning to do so we should absolutely do so to prepare some more solid ground for the future.

The ethereumjs-util library e.g. is definitely a candidate for some bigger changes especially on the API front. With dependency updates this is not too much of a problem. There is no need to do everything at once and we can just update "on occasion", we are already doing this regularly with things like that and that is the normal work progress work-flow to slowly but steadily evolve on all fronts.

Then, I agree with using Buffer everywhere. That'd also make much easier to type most of the things that now have any. Let's continue this discussion in #172 :)

@alcuadrado can you please rebase your branch for a review?

Done

[holgerd77]

Looks good, thanks! 😄