Support multiple versions of node.js

[shuse2]

Description

Currently, lisk-js only supports fixed node.js version of v6.12.3.
It would be great if it supports all major v6, and even v6 or higher.

Affected versions

1.0.0

Changes

• Change all Buffer.from to use crypto function.
• Buffer.from should ignore invalid hex strings.

[AndrewBarba]

This is wrong, currently v0.5.1 works on Node v6, v8 and v9. It is the main dependency in Lisk Nano, running on Electron, which uses Node v8

[AndrewBarba]

[willclarktech]

@AndrewBarba We're talking about different things here. This issue concerns the v1.0.0 branch of LiskJS, not the currently published v0.5.1.

[AndrewBarba]

Okay so newer version of the same sdk will actually downgrade which systems it runs on, got it. Would love to see a test on 1.0.0 that fails on Node 8 and higher, currently they all pass (I tested on both master and 1.0.0 branches). Kind of hard to claim it doesn’t work without a test case

[willclarktech]

Running tests on 1.0.0 branch with Node.js v8.9.1 I get the following output:

  643 passing (13s)
  4 failing

  1) #castVotes transaction when the cast vote transaction is created with one plus prepended public key should throw an error:
     AssertionError: expected Function { name: 'bound ' } to throw exception with a message matching 'Invalid hex string', but got 'Public key must be a valid hex string.'
      at Assertion.fail (node_modules/should/cjs/should.js:258:17)
      at Assertion.value (node_modules/should/cjs/should.js:335:19)
      at Context.<anonymous> (test/transactions/3_castVotes.js:241:11)

  2) #registerMultisignatureAccount transaction when the register multisignature account transaction is created with one plus prepended public key should throw an error:
     AssertionError: expected Function { name: 'bound ' } to throw exception with a message matching 'Invalid hex string', but got 'Public key must be a valid hex string.'
      at Assertion.fail (node_modules/should/cjs/should.js:258:17)
      at Assertion.value (node_modules/should/cjs/should.js:335:19)
      at Context.<anonymous> (test/transactions/4_registerMultisignatureAccount.js:230:11)

  3) public key validation #validatePublicKey Given a hex string with odd length should throw an error:
     AssertionError: expected Function { name: 'bound validatePublicKey' } to throw exception with a message matching 'Invalid hex string', but got 'Public key must be a valid hex string.'
      at Assertion.fail (node_modules/should/cjs/should.js:258:17)
      at Assertion.value (node_modules/should/cjs/should.js:335:19)
      at Context.<anonymous> (test/transactions/utils/validation.js:29:12)

  4) public key validation #validatePublicKeys Given an array of public keys with one invalid public key should throw an error:
     AssertionError: expected Function { name: 'bound validatePublicKeys' } to throw exception with a message matching 'Invalid hex string', but got 'Public key must be a valid hex string.'
      at Assertion.fail (node_modules/should/cjs/should.js:258:17)
      at Assertion.value (node_modules/should/cjs/should.js:335:19)
      at Context.<anonymous> (test/transactions/utils/validation.js:95:12)

[willclarktech]

Initial thought regarding v8 functionality is to enrich the hexToBuffer function in src/cryptography/convert.js and use it more consistently throughout the application but this will require more investigation.

[AndrewBarba]

All 4 of those errors are a discrepancy in the returned error message string, they tested the correct functionality. Just update the expected error string to the error that your code actually throws:

[willclarktech]

@AndrewBarba Sorry, I don't understand your point here. These tests behave differently on Node.js version 6 and version 8. We can't just change the tests because we want consistent behaviour across versions.

[AndrewBarba]

@willclarktech The failed test expected the error string: "Invalid hex string". Search the entire 1.0.0 branch for that string and you will only find it in your tests. So you guys are testing for an error that you are not catching in your code, thats a problem. The check occurs here: https://github.com/LiskHQ/lisk-js/blob/1.0.0/src/transactions/utils/validation.js#L20. So this means that on Node v6 the naclInstance.to_hex(buffer) is throwing the error and you guys are not catching it. On Node v8 it does not throw an error and your check is successful in that the two values are not equal. I would actually say the Node v8 system is MORE correct than Node v6.

The bottom line here is that code needs to be rethought whether you want to admit that everything works on Node v8 or not

[willclarktech]

Current thinking is to update hexToBuffer to match Node.js v8 functionality, use it consistently throughout the code base, and update tests accordingly. Then invalid hex strings will all throw the same error (ours).

We still need to confirm that no other changes between 6 and 8 are relevant (or indeed between other versions of Node 6) in order to fully address this issue.

[AndrewBarba]

Thank you I appreciate it. I understand I am being a pain in the ass, but trust me this is important to the community to support this range of versions.

[willclarktech]

@AndrewBarba Thanks for engaging us on this! We definitely consider this an important issue and it will be sorted out. So that you know where the original approach was coming from: our instinct is always to err on the conservative side with supported version ranges, for security reasons and because the stakes are high. But LiskJS is intended to be used in other applications, so clearly there needs to be some accommodation for other versions of Node.js besides the one used for development in this project.