-
Notifications
You must be signed in to change notification settings - Fork 59
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
Implement OP_WITHIN opcode #28
Conversation
} | ||
|
||
fn opcode_within(ref engine: Engine) { | ||
let a = engine.dstack.pop_int(); |
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.
Would be prefered to make the variable naming here more explicit, like min_value, max_value, ...
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.
Will rename the variables now
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.
Hi I just pushed an update to rename the variables to be more explicit
src/opcodes/opcodes.cairo
Outdated
}); | ||
} | ||
|
||
fn opcode_within(ref engine: Engine) { |
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.
Please refer to this implementation to see the proper ordering of variables on the stack : https://github.com/btcsuite/btcd/blob/b161cd6a199b4e35acec66afc5aad221f05fe1e3/txscript/opcode.go#L1844
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.
Working on this
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.
Hi, opcode_within is number 165 in the order, please could you tell me what you mean? I have pushed an update where I renamed and reordered the variables though
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.
So we have to make sure the values poped off the stack are in the same order as the bitcoin spec, so here max_value
should be popped first, then min_value
, then selected_value
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.
Hi @b-j-roberts I have rearranged the variables based on the order max before min before value
I implemented the code for op_within