Skip to content
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

Merged
merged 4 commits into from Jul 3, 2019

Conversation

@jakerockland
Copy link
Contributor

jakerockland commented Jun 26, 2019

What I did

Added support for conversion to bytes32 from decimal. Minor part of #1093

How 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 from decimal.

Cute Animal Picture

image


hooVal = c.hoo()
hoonarVal = c.hoonar()
assert hooVal[0:11] == (b"\xff" * 11)

This comment has been minimized.

Copy link
@fubuloubu

fubuloubu Jun 26, 2019

Member

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?

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jun 26, 2019

Author Contributor

@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 👍

This comment has been minimized.

Copy link
@fubuloubu

fubuloubu Jun 26, 2019

Member

You the man!

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jun 27, 2019

Author Contributor

@fubuloubu updated! 😄

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

jakerockland commented Jun 27, 2019

Just updated the VIP #1093 and pending docs PR #1501 with additional clarification as well @fubuloubu 😄

kar: decimal
@public
def foo() -> bytes32:

This comment has been minimized.

Copy link
@davesque

davesque Jul 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jul 2, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@davesque

davesque Jul 2, 2019

Contributor

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.

This comment has been minimized.

Copy link
@jakerockland

jakerockland Jul 2, 2019

Author Contributor

@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)

This comment has been minimized.

Copy link
@davesque

davesque Jul 2, 2019

Contributor

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.

@davesque

This comment has been minimized.

Copy link
Contributor

davesque commented Jul 2, 2019

Thanks for the work, @jakerockland !

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

jakerockland commented Jul 3, 2019

@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 foobar thing is definitely present in other tests 😅I've written some of those foos but definitely not all of them 😛

@davesque

This comment has been minimized.

Copy link
Contributor

davesque commented Jul 3, 2019

@jakerockland Nice, thanks for the heads up. I wasn't aware of that.

Copy link
Contributor

davesque left a comment

LGTM. @fubuloubu Think this is ready to merge?

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

jakerockland commented Jul 3, 2019

@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 😅

@fubuloubu fubuloubu merged commit dc3845a into ethereum:master Jul 3, 2019
3 checks passed
3 checks passed
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.