feature: opcode enum type implemented in a compatible way with current version #2028
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
ChrisCho-H
commented
Jan 5, 2024
•
ChrisCho-H
commented
- 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
junderw
reviewed
Jan 6, 2024
| @@ -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) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is a great PR though.
I'm thinking of how to push this through.
There was a problem hiding this comment.
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.
junderw
reviewed
Jan 6, 2024
Comment on lines
-132
to
-133
| OP_PUBKEYHASH: 253, | ||
| OP_PUBKEY: 254, |
There was a problem hiding this comment.
These two are missing. This is not compatible.
junderw
approved these changes
Jan 6, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.