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

VIP: Forbid implicit conversion in arithmetic and comparison #775

Closed
yzhang90 opened this issue Apr 11, 2018 · 6 comments
Closed

VIP: Forbid implicit conversion in arithmetic and comparison #775

yzhang90 opened this issue Apr 11, 2018 · 6 comments
Labels
VIP: Approved VIP Approved

Comments

@yzhang90
Copy link

yzhang90 commented Apr 11, 2018

Preamble

VIP: <to be assigned>
Title: Forbid implicit conversion in arithmetic and comparison
Author: yzhang90
Type: <Standard Track>
Status: Draft
Created: 2018-04-11

Simple Summary

We should forbid the implicit conversion in arithmetic and comparison and require the developers to explicitly do the conversion by themselves.

Abstract

Implicit conversion between integer types (int128, uint256, decimal) in arithmetic and comparison can be confusing and harm the auditability of smart contracts. We should forbid the implicit conversion in arithmetic and comparison.

Motivation

Implicit conversion between integer types (int128, uint256, decimal) in arithmetic and comparison can cause harm the auditability of smart contracts, increase the complexity of the compiler and make it very hard to optimize gas cost ( See #issue716, #issue717 and #issue737). Thus, we should forbid the implicit conversion in arithmetic and comparison and require the developers to explicitly do the conversion.

Specification

General principle is that the parameters of arithmetic op (+, -, *, /) and comparison (<, <=, >, >=, !=) should be of the same type. Arithmetic will return the same type and comparison will return bool.

Specifically, for arithmetic:

decimal [op] decimal = decimal (decimal division)
int128 [op] int128 = int128 (signed integer division)
uint256 [op] uint256 = uint256 (unsigned integer division)
decimal [op] integer = COMPILE TIME ERROR
integer [op] decimal = COMPILE TIME ERROR

The developers should use convert or floor to do the conversion by themselves.

Backwards Compatibility

This change is NOT backward compatible.

Copyright

Copyright and related rights waived via CC0

@jacqueswww jacqueswww added the VIP: Approved VIP Approved label Apr 11, 2018
@daejunpark
Copy link
Contributor

@jacqueswww Just for a note about the gas cost that may require a (good-to-have) optimization.

Let d be a decimal, i be an integer, and [d] be the decimal representation (i.e., [d] = d * 10^10).
d * convert(i) => [d] * (i * 10^10) / 10^10 => [d] * i
d / convert(i) => [d] * 10^10 / (i * 10^10) => [d] / i

Essentially, if we have a decimal being multiplied/divided by an integer (with the explicit conversion to a decimal), then we can save the gas by optimizing (dropping the redundant) the multiplication of 10^10 followed by the division of 10^10.

@fubuloubu
Copy link
Member

@daejunpark I'm not sure that is fully correct, it would be only two of these scenarios because decimal assignment would not perform the division by 10^10:

d = d * convert(i) => d = [d] * (i * 10^10)
d = d / convert(i) => d = [d] / (i * 10^10)
i = convert(d * convert(i)) => i = ([d] * (i * 10^10) ) / 10^10 => i = ([d] * i)
i = convert(d / convert(i)) => i = ([d] * 10^10) / (i * 10^10) => i = ([d] / i)

@daejunpark
Copy link
Contributor

daejunpark commented Apr 23, 2018

@fubuloubu Let me clarify.

Let a and b be decimals, and [a] and [b] be their integer-scaled representations (i.e., [a] = a * 10^10 and [b] = b * 10^10).

Then the decimal multiplication is defined as follows:
[a * b] = [a] * [b] / 10^10

For example,

[1.0 * 1.0] = [1.0] * [1.0] / 10^10
=>
10^10       = 10^10 * 10^10 / 10^10

Similarly, the decimal division is:
[a / b] = [a] * 10^10 / [b]

@fubuloubu
Copy link
Member

Ah yes, I see it now. Thanks!

@jakerockland
Copy link
Contributor

jakerockland commented Nov 27, 2018

@yzhang90 Is there still something outstanding on the implementation of this VIP? It looks to me to be resolved (using version 0.1.0b4).

@jacqueswww
Copy link
Contributor

jacqueswww commented Nov 27, 2018

Yes I believe these got implemented, issue was just never closed. Closing now.

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

No branches or pull requests

5 participants