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: Allow declaring custom units #752

Closed
ben-kaufman opened this issue Apr 3, 2018 · 13 comments
Closed

VIP: Allow declaring custom units #752

ben-kaufman opened this issue Apr 3, 2018 · 13 comments
Labels
VIP: Approved VIP Approved

Comments

@ben-kaufman
Copy link
Contributor

ben-kaufman commented Apr 3, 2018

Preamble

VIP: 752
Title: Allow declaring custom units
Author: Ben Kaufman [email protected]
Type: Standard Track
Status: Draft
Created: 2018-04-03

Simple Summary

Add a new keyword unit to allow declaring custom units like so:

Abstract

Adding a way to create custom units can make the code more readable and improve security (type-safety).

Motivation

The motivation for this came as I saw that the Casper contract uses labels incorrectly. they use the m which stands for meter where they clearly didn't meant to represent meters. This make the code very confusing and might be a bigger issue in the future.
Currently, there are a few units Vyper support, but there are much more measurements Vyper doesn't take in count. For example, in the company.v.py the stocks are labeled as currency while it actually should be a stock.

Specification

The proposed unit declaration is as below:

stock: unit
stocks_count: int128(stock)
wei_per_stock: int128(wei / stock)

However, it may be declared in a different way such as with a dedicated function.

Backwards Compatibility

This change is backward compatible

Copyright

Copyright and related rights waived via CC0

Cute Animal Picture

image

@jacqueswww
Copy link
Contributor

I think this a decent idea. Perhaps we should consider sometype of predefined unit table ?
Something like (don't worry about the syntax for now, but just the idea):

units = (
    ('m', 'meter'),
    ('cm', 'centimeter')
)

This would force a description on each unit used in the contract.

@ben-kaufman
Copy link
Contributor Author

Sounds good, I think it will be better. I will update the VIP in accordance with a syntax for that. I will also add it to the list of subjects for the next meeting.

Sent with GitHawk

@fubuloubu
Copy link
Member

I wamted the unit labels to be double quoted so it's obvious that those are labels and not types or variables

@fubuloubu
Copy link
Member

Example:

stocks_count: int128("stock")
wei_per_stock: int128("wei" / "stock")

I don't think we need an extra syntax to declare a unit, if you made a spelling mistake it will tell you when you compile, because that's the whole point of units.

@ben-kaufman
Copy link
Contributor Author

I think a syntax like @jacqueswww proposed would be better both because it gives a dedicated section with specifications (which makes it more readable IMO) and to prevent spelling mistakes (the compiler can't always recognize the spelling mistakes and even if it does it can't point out where is the mistake and it can make it harder for debugging).
I think we will still need a few base units like wei because this should be consistent for all contracts.

@fubuloubu
Copy link
Member

Hmm, a hybrid of the two?

units = (
    ("stock", "one ownership share"),
    # "wei" comes for free (built-in)
)
stocks_count: int128("stock")
wei_per_stock: int128("wei" / "stock")

@fubuloubu
Copy link
Member

If there's ever a misspelling it could find it at the front because it won't exist in the table

@jacqueswww
Copy link
Contributor

jacqueswww commented Apr 3, 2018

On that note, we definitely need the built-in units to be listed in the documentation under 'Types/Unit Types'.
We could also consider using : instead of assign = as it does seem to be a deceleration more than an assignment ;)

units: (
   ("stock", "one share")
)
units: {
   "stock": "one share",
   "m": "meter",
   "int128": "hahaha" # this should fail obviously.
}

@fubuloubu
Copy link
Member

Idea is that each file extends the base units for itself

@ben-kaufman
Copy link
Contributor Author

The second syntax looks good IMO.
Just wondering, maybe the unit should be vyper.units or __units__ to make it more notable. What do you think?

Sent with GitHawk

@fubuloubu
Copy link
Member

I'm not opposed to __units__...

@jacqueswww jacqueswww added VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting VIP: Approved VIP Approved labels Apr 9, 2018
@jacqueswww
Copy link
Contributor

Approved, we should only allow basically one layer deep operator (divide/multiply/exponential).

units: {
     stock: "one share",
     m: "meter",
 }

@jacqueswww jacqueswww removed the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Apr 13, 2018
@ben-kaufman
Copy link
Contributor Author

Closing as it's implemented. Great work!

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

3 participants