Ensures all transaction properties that are strings are 0x prefixed

[danjm]

fixes #90

This PR ensures that when tx properties are passed to the constructor in either an array or object, the properties that are strings are '0x' prefixed. Without this, the following two sets of inputs would create different transactions (i.e. with different hashes and serializations):

Problem being solved.

var rawTxWithout0x = {
      nonce: 0,
      gasPrice: '4a817c800',
      gasLimit: 21000,
      to: '3535353535353535353535353535353535353535',
      value: 'de0b6b3a7640000',
      data: ''
    }

    var rawTxWith0x = {
      nonce: '0x0',
      gasPrice: '0x4a817c800',
      gasLimit: '0x5208',
      to: '0x3535353535353535353535353535353535353535',
      value: '0xde0b6b3a7640000',
      data: '0x'
    }

These properties are converted to buffers during instantiation by ethUtil.definedProperties. When strings are prefixed by '0x', they will be converted by Buffer.from(exports.padToEven(exports.stripHexPrefix(v)), 'hex');, otherwise they will be converted by Buffer.from(v). This will convert the string with different encoding https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_string_encoding

This could cause unexpected discrepencies in transactions when '0x' prefixes are forgotten, as indicated in the issue motivating this PR: #90

[coveralls]

Coverage increased (+0.03%) to 99.0% when pulling f0cfb2e on prepend-0x-to-tx-data into 002172c on master.

[holgerd77]

@danjm can you also fix conflicts here? Eventually also drop a note on your preferred review/merge order.

[danjm]

@chikeichan awaiting your final approval

[s1na]

I just saw this. I have mixed feelings about the goal tbh for two reasons:

• '0x' is specifying the base, without it there's no way to tell if by '20' the user intended 20 or 32 (0x20).
• The other is a more general argument in favor of restricting the API at the core level to decrease complexity (and hopefully increase safety), and then adding convenience on top (e.g. by enabling easy conversions between types).

@holgerd77 what do you think?

[holgerd77]

TBH I also had mixed feelings but didn't say anything. Yes, as @s1na mentioned, the preferred way would be to restrict the API on the base layer. We had all sorts of problems in the past with becoming too loose including serious bugs and the current thinking - also influenced by experienced devs like @axic with longer experience on the libraries - is to go to more disambiguous API definitions.

[alcuadrado]

I just saw this. I have mixed feelings about the goal tbh for two reasons:

• '0x' is specifying the base, without it there's no way to tell if by '20' the user intended 20 or 32 (0x20).

This is a very good point. Especially in javascript, where sometimes '20' would act as 20 (e.g. '20' * 2 === 40, 10 * 2 == '20'). Treating every string as hex encoded can be unexpected.

This PR introduces a breaking change. So given that we are doing that, why not just throw when there's no 0x prefix? It's not clear what to do in those cases anyway.

I believe every use of unprefixed strings here is an error, and users will appreciate this change.

Currently, if strings don't have the 0x prefix, they will end up as arguments to Buffer.from(string[, encoding]) via

    at Object.exports.toBuffer (/private/tmp/test/node_modules/ethereumjs-tx/node_modules/ethereumjs-util/dist/index.js:171:21)
    at /private/tmp/test/node_modules/ethereumjs-tx/node_modules/ethereumjs-util/dist/index.js:704:41
    at Array.forEach (<anonymous>)
    at Object.exports.defineProperties (/private/tmp/test/node_modules/ethereumjs-tx/node_modules/ethereumjs-util/dist/index.js:703:12)
    at new Transaction (/private/tmp/test/node_modules/ethereumjs-tx/es5/index.js:112:13)

The default encoding of Buffer.from is utf8, so I'd be surprised if someone is actually using it on purpose. For example, Buffer.from("20").toString('hex') === "3230".

Also, if we are going to throw in this case, I'd do it in ethereumjs-util, as the reasoning is the same for anything calling toBuffer.

[holgerd77]

Yep, also think we should throw in such cases.

[danjm]

@holgerd77 @s1na @alcuadrado So it sounds like there is consensus that we should close this in favour of a PR in ethereumjs-util to ensure an error is thrown when dealing with such inputs?

[alcuadrado]

I started working on a PR in ethereumjs-util. To my surprise, that behavior is used internally. I wouldn't close this PR until we are confident that solving it in ethereumjs-util makes sense.

[alcuadrado]

The PR I mentioned in my previous comment has already been merged, so I'll close this one.