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

perfs: optimize script.decomplie return type #2097

Merged
merged 5 commits into from
May 17, 2024

Conversation

jasonandjay
Copy link
Collaborator

Optimize script.decomplie return type to Stack

  1. avoid type error. for example: null.map toASM throws TypeError: Cannot read properties of null (reading 'map') #2096
  2. avoid unnecessary type assertion
  3. simplify the judgment of returned results

@junderw
Copy link
Member

junderw commented May 10, 2024

There is a semantic difference between [] and null

null indicates "the decompilation failed / there is no valid decompilation that exists for this Buffer" without need for throwing an exception.

[] means "we decompiled it and it was an empty script."

By collapsing this, [] can now mean two things.

This is a breaking change.

I agree that the bug in the other issue should be fixed, but this is not the way we should fix it.

Thanks for the PR though.

@jasonandjay
Copy link
Collaborator Author

There is a semantic difference between [] and null

null indicates "the decompilation failed / there is no valid decompilation that exists for this Buffer" without need for throwing an exception.

[] means "we decompiled it and it was an empty script."

By collapsing this, [] can now mean two things.

This is a breaking change.

I agree that the bug in the other issue should be fixed, but this is not the way we should fix it.

Thanks for the PR though.

I agree

How about throw error replace return null ?

@junderw
Copy link
Member

junderw commented May 10, 2024

I'd rather not have to think about the knock on effects.

A simple bug fix is easier to approve.

@jasonandjay
Copy link
Collaborator Author

@junderw how about this?
just throw an error from toASM when decompile failed
Minimal impact on existing logic

src/script.js Outdated Show resolved Hide resolved
@jasonandjay
Copy link
Collaborator Author

Please help reRun actions
it`s failed for netowrk issue, reRun failed works will repair

@junderw junderw merged commit eb12504 into bitcoinjs:master May 17, 2024
12 checks passed
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.

None yet

2 participants