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

feature: opcode enum type implemented in a compatible way with current version #2028

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

Conversation

ChrisCho-H
Copy link
Contributor

@ChrisCho-H ChrisCho-H commented Jan 5, 2024

  • Better types for opcodes #1966 mentions that current opcode is type-unsafe. Type safe version is considered as tricky and breaking change as it might function in a bit different way from it does so far(in any versions) and have reverse opcode, too.
  • However, as TS numeric Enum compiles with reverse mapping(https://www.typescriptlang.org/docs/handbook/enums.html#reverse-mappings), we can just change OPS to enum, and export OPS and OPS as REVERSE_OPS for reverse opcode.
  • IMO It could be good change as it provides type-safe ops and reverse_ops, without changing the way to use this module(no breaking change)
  • Also, I deleted OP_PUBKEY and OP_PUBKEYHASH, which are not used in bitcoin anymore

@@ -173,7 +173,8 @@ export function fromASM(asm: string): Buffer {
return compile(
asm.split(' ').map(chunkStr => {
// opcode?
if (OPS[chunkStr] !== undefined) return OPS[chunkStr];
if (OPS[chunkStr as keyof typeof OPS] !== undefined)
Copy link
Member

Choose a reason for hiding this comment

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

I think this kind of proves the "compatible way" part not being 100% accurate.

If we look at the JS, then yeah. But TypeScript people will have breakage.

Copy link
Member

Choose a reason for hiding this comment

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

This is a great PR though.

I'm thinking of how to push this through.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure how many people are writing script parsers from scratch just pulling in the OPS though...

The use case of "get opcode from arbitrary string" is probably very niche. But still it's a possible breakage.

Comment on lines -132 to -133
OP_PUBKEYHASH: 253,
OP_PUBKEY: 254,
Copy link
Member

Choose a reason for hiding this comment

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

These two are missing. This is not compatible.

Copy link
Member

@junderw junderw left a comment

Choose a reason for hiding this comment

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

I'll approve and set it for the next major version milestone due to breakages.

@junderw junderw added this to the 7.0.0 milestone Jan 6, 2024
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