Conversation
|
The test results of CircleCI do not seem to be related to this commit. |
| var opcodenum = opcode.toNumber(); | ||
|
|
||
| if (opcodenum == null) { | ||
| if (_.isUndefined(opcodenum)) { |
There was a problem hiding this comment.
For consistency and for linting, would you mind just copying the code from the bitcore-lib-cash script.js? Feel free to clean it up a little (don't need to re-declare opcodenum, for example), but your code here has some inconsistent formatting/linting with the rest of the codebase.
| }); | ||
| }); | ||
|
|
||
| describe('#fromASM PUSHDATA', function() { |
There was a problem hiding this comment.
there is already a #fromASM describe in this file, and bitcore-lib-cash already has some pushdata tests. Would you please copy those tests to the existing #fromASM describe?
|
In the cash code, a constant is calculated every time , which is unnecessary. In my code, the value of the constant is explicitly stated. Additionally, cash does not provide test cases for this part of the code. |
…throw an exception. Signed-off-by: Long Li <[email protected]>
Constants are fine - I agree that your constants are better than the computed values in BCH lib. I was more noticing the linting. You have inconsistent spacing around the braces and we're trying to move away from using lodash (_), so There are 3 PUSHDATA tests in the BCH lib that are relevant to this codeblock, but your additional test is welcome. Lastly, all of your commits need to be signed, so you'll need to squash all your commits into a new one and force push. |
Script.fromASM should support PUSHDATA, otherwise it will lead to incorrect chunks settings, which in turn makes the result of toBuffer incorrect.
The bitcore-lib-cash code was previously fixed. LTC and DOGE are also incorrect, if corrections are needed, submit a patch separately.