-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feature: opcode enum type implemented in a compatible way with current version #2028
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
base: master
Are you sure you want to change the base?
Conversation
@@ -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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
OP_PUBKEYHASH: 253, | ||
OP_PUBKEY: 254, |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
Uh oh!
There was an error while loading. Please reload this page.