Implement SAR opcode #1134
Implement SAR opcode #1134
Conversation
cburgdorf
force-pushed
the
christoph/feat/sar-opcode
branch
4 times, most recently
from
d31c552
to
ff118c2
Compare
eth/vm/logic/arithmetic.py
Outdated
| value = unsigned_to_signed(value) | ||
|
|
||
| if shift_length >= 256: | ||
| result = 0 if value >= 0 else signed_to_unsigned(-1) |
There was a problem hiding this comment.
Can we constantify signed_to_unsigned(-1)
There was a problem hiding this comment.
Makes sense! However, I'm not sure if I got the naming right because my bits and bytes fu is pretty bad.
I added the following constant to eth.constants
UINT_255_NEGATIVE_ONE = -1 + UINT_256_CEILINGMy understanding is that this is representing the number -1 according to the rules of the two's complement which uses the most significant bit to represent the sign and leaving us with 255 other bits to represent the number.
Btw, the constant basically inlines what signed_to_unsigned does because I can't import it here (circular import)
There was a problem hiding this comment.
@pipermerriam interpreted 👍 as indicator that the constant name seems reasonable and merged it
| """ | ||
| Arithmetic bitwise right shift | ||
| """ | ||
| shift_length, value = computation.stack_pop(num_items=2, type_hint=constants.UINT256) |
There was a problem hiding this comment.
Side note, I just realized that we use keyword style calling for all stack operations, and a while back @davesque uncovered that calling things with positional arguments is significantly faster than keyword style calling. We should look into updating all of these call sites to use positional arguments.
cburgdorf
force-pushed
the
christoph/feat/sar-opcode
branch
from
ff118c2
to
c7a1baf
Compare
What was wrong?
This is a follow up to #1112 that also includes the
SARopcode. I branched this off because I feel less confident about this one compared to the other ones. That said, it passes all test cases that are specified in the EIP.How was it fixed?
Implemented new opcode.
Cute Animal Picture