Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Script.fromASM support PUSHDATA #3807

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

baryon
Copy link

@baryon baryon commented Oct 9, 2024

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.

@baryon
Copy link
Author

baryon commented Oct 9, 2024

The test results of CircleCI do not seem to be related to this commit.

 1 failing

  1) Coin Model
       should appropriately mark coins related to transactions that are in mempool, but no longer valid:

      AssertionError: expected -1 to equal -3
      + expected - actual

      --1
      +-3
      
      at Context.<anonymous> (test/integration/models/coin.spec.ts:123:38)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

Copy link
Collaborator

@kajoseph kajoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @baryon ! I left some feedback on making it a little cleaner and consistent with the rest of the codebase.

Also, I won't be able to merge this in unless your commits are signed

@@ -157,12 +157,23 @@ Script.fromASM = function(str) {
var opcode = Opcode(token);
var opcodenum = opcode.toNumber();

if (opcodenum == null) {
if (_.isUndefined(opcodenum)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -205,6 +205,25 @@ describe('Script', function() {
});
});

describe('#fromASM PUSHDATA', function() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@baryon
Copy link
Author

baryon commented Oct 10, 2024

In the cash code, a constant is calculated every time

} else if (len < Math.pow(2, 8)) {
, 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.

@kajoseph
Copy link
Collaborator

In the cash code, a constant is calculated every time

} else if (len < Math.pow(2, 8)) {

, 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.

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 opcodenum == null or (opcodenum === undefined if null is a valid value) is more preferable over _.isUndefined(opcodenum)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants