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

uint256_div produce a uint256 (expected decimal) #717

Closed
ben-kaufman opened this issue Mar 23, 2018 · 7 comments
Closed

uint256_div produce a uint256 (expected decimal) #717

ben-kaufman opened this issue Mar 23, 2018 · 7 comments

Comments

@ben-kaufman
Copy link
Contributor

When dividing ints in Vyper it should give a decimal.
When dividing a uint256 using uint256_div(x, y) it produces a uint256 instead of a decimal.
I think this should be changed as it is not the expected behaviour.

Cute Animal Picture

image

@jacqueswww
Copy link
Contributor

Agreed.

@dani-corie
Copy link

dani-corie commented Apr 3, 2018

See #716 also.

Types co-mingling is the root of all chaos. Also, on Ethereum, you usually only ever use integers. Ever. Having decimal as an option is cool, but it's a 1% vs 99% thing.

I think dividing ints should give an int. For decimal division, divide decimals.

In fact, Vyper should have absolutely no implicit conversions at all (especially not between signed and unsigned ints!) Operations should be allowed between variables of the exact same type, and the result should be the same type as the operands.

decimal / decimal = decimal (decimal division)
integer / integer = integer (integer division)
decimal / integer = COMPILE TIME ERROR
integer / decimal = COMPILE TIME ERROR

@jacqueswww
Copy link
Contributor

@daniel-jozsef the main reason we currently do integer / integer = decimal derision is make the rounding specific by using ceil and floor, making it clear what rounding is being used.

@dani-corie
Copy link

dani-corie commented Apr 3, 2018

Integer division is grade school stuff. It's NOT ROUNDING. How many times can you fit two in five? Two times, with a remainder of one.

I think it's a horrible antipattern to mix types like this. If we prioritize readability, this is not intuitive at all. Anyone with any experience in programming will expect an operation on two variables of the same type have the exact same type as the two inputs.

(Also, using anything other than integers on the blockchain makes me cringe.)

If someone wants decimal division between two integers, they should cast them both to decimal.

@jacqueswww
Copy link
Contributor

jacqueswww commented Apr 3, 2018

@daniel-jozsef @yzhang90 @fubuloubu

I agree with your suggestion/assesment and I think we should try and wrap the whole division issue into one clean VIP. I was just giving some background on the the thought process at the time.

These are all the related issues I could track:

#717 (this one)
#737 (VIP: Disallow implicit conversion)
#716 (Need support for integer division in Vyper)
ethereum/casper#67

@fubuloubu
Copy link
Member

@jacqueswww on board!

@jacqueswww
Copy link
Contributor

I am closing this as a decision has already been made on #737 and #716.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants