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

Need support for integer division in Vyper #716

Closed
yzhang90 opened this issue Mar 22, 2018 · 11 comments
Closed

Need support for integer division in Vyper #716

yzhang90 opened this issue Mar 22, 2018 · 11 comments
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting

Comments

@yzhang90
Copy link

What's your issue about?

I noticed that a recent change Vyper where / returns decimal instead of int128. However, if a developer do want to use integer division (int128 // int128), he/she has to use floor to do the conversion. However, this conversion can potentially waste gas because '/' will first multiply DECIMAL_DIVISOR and floor will divide DECIMAL_DIVISOR. On the other hand, EVM opcode DIV performs the integer division.

How can it be fixed?

Introduce operator // for integer division and it can be directly translated to DIV opcode in EVM.

Cute Animal Picture

image

@fubuloubu
Copy link
Member

Potential solution: add optimization that int128 = floor(int128/int128) should use DIV opcode directly instead of more complex math, e.g. catch and remove clamp in that limited circumstance in our optimization module

@jacqueswww
Copy link
Contributor

@fubuloubu yes, was thinking the same, the only problem comes when it's a nested expression.

@fubuloubu
Copy link
Member

That's why optimization is the right solution. If it's not simple to figure out, don't do the optimization! Positive confirmation.

@yzhang90
Copy link
Author

@fubuloubu why not use // to replace floor(int128/int128) instead of optimizing floor function?

@fubuloubu
Copy link
Member

fubuloubu commented Mar 24, 2018

Because from an auditability perspective, it may be easy to miss that extra / and misunderstand what the command is doing. That was our concern with //.

Vyper is ease of audit, not ease of writability.

@jacqueswww jacqueswww added the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Mar 26, 2018
@dani-corie
Copy link

dani-corie commented Mar 31, 2018

I think decimal numbers in a smart contract are a luxury. Ethereum is integer-based, and 99% of all use cases ever call for integers, not decimals.

I think the best solution would be to disallow integers and decimals to co-mingle.

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

Alternatively, we could still have separate operators for integer and decimal division!

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

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

decimal / integer = COMPILE TIME ERROR
integer / decimal = COMPILE TIME ERROR

decimal // integer = COMPILE TIME ERROR
integer // decimal = COMPILE TIME ERROR

In fact, I'd remove ALL implicit conversions. Operators should only ever operate on operands of the exact same type, and return the same type. If you need to add an int128 to a uint256, cast explicitly. If you need to divide a decimal with an integer, cast explicitly.

@fubuloubu
Copy link
Member

You know, this is probably a stronger and simpler approach. It makes auditability the main goal, and it would be simpler to describe. No implict conversions at all.

What difficulties exist with this approach?

@dani-corie
Copy link

I don't see any issue beyond code becoming more verbose (which, for Vyper, is more a requirement than a problem). Existing code not compiling after the change should not be an issue since Vyper is still in alpha.

@jacqueswww
Copy link
Contributor

jacqueswww commented Apr 3, 2018

As mentioned in #717 (comment), I think we should put the above suggestion (#716 (comment)) into a VIP.

This means we drop support for integer / integer = decimal, floor and ceil remain decimal operators and any cross-type division is not supported forcing explicit conversion.

@fubuloubu
Copy link
Member

Sounds good to me!

@jacqueswww
Copy link
Contributor

jacqueswww commented Apr 9, 2018

Merge into #737 as a VIP, thanks ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting
Projects
None yet
Development

No branches or pull requests

4 participants