-
-
Notifications
You must be signed in to change notification settings - Fork 788
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
Added bytes32
as target type for decimal
conversion
#1500
Added bytes32
as target type for decimal
conversion
#1500
Conversation
|
||
hooVal = c.hoo() | ||
hoonarVal = c.hoonar() | ||
assert hooVal[0:11] == (b"\xff" * 11) |
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.
Definitely need some comments here and similar other lines in this test to describe why certain bit patters are set.
Will docs be added to describe what the conversion will look like?
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.
@fubuloubu great point, I'll add some comments noting the decimal precision and why these tests are like this + also will add more tests
I'll also add to the VIP at #1093 and to PR #1501 to describe in a bit more detail what this specific conversion looks like, as the decimal precision part is a bit unintuitive 😅
Will take care of this tonight 👍
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.
You the man!
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.
@fubuloubu updated! 😄
Just updated the VIP #1093 and pending docs PR #1501 with additional clarification as well @fubuloubu 😄 |
kar: decimal | ||
|
||
@public | ||
def foo() -> bytes32: |
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.
Can we rename this method to something more meaningful like convert_literal_zero
? Let's also do that for the storage variables and other methods. I think it will make it a bit easier for future readers to follow.
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.
Sure thing @davesque will take care of this and the suggested refactoring below this evening.
fooVal = c.foo() | ||
foobarVal = c.foobar() | ||
assert fooVal == (b"\x00" * 32) | ||
assert len(fooVal) == 32 |
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.
The assertions about the lengths of fooVal
and foobarVal
as well as the equality check between fooVal
and foobarVal
aren't needed if the two assertions comparing those values to b"\x00" * 32
are satisfied.
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.
@davesque good point, these is a bit of left-over bad testing from the original PR before I updated the tests—I'll clean this up 👍
|
||
hooVal = c.hoo() | ||
hoonarVal = c.hoonar() | ||
_hoo = ((-2**127) * decimal_divisor).to_bytes(32, byteorder="big", signed=True) |
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.
Maybe we should factor out these repeated .to_bytes(...)
operations into a helper function at the top of this testing module. Also, you can import DECIMAL_DIVISOR
from vyper.utils
.
Thanks for the work, @jakerockland ! |
@davesque just pushed a commit that incorporates all of your suggested changes above, may be worth making a code quality ticket to address auditing test methods across the testing suite and renaming needed functions to be more meaningful/readable, as the |
@jakerockland Nice, thanks for the heads up. I wasn't aware of that. |
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.
LGTM. @fubuloubu Think this is ready to merge?
@davesque sure thing! Definitely would be a nice improvement in code-quality, but catching it new additions to the codebase like here is a good first step 😅 |
What I did
Added support for conversion to
bytes32
fromdecimal
. Minor part of #1093How I did it
Minor change to
convert.py
.How to verify it
Run the tests:
make test
Or more specifically:
pytest -k test_convert_to_bytes32
Description for the changelog
Added support for conversion to
bytes32
fromdecimal
.Cute Animal Picture