Skip to content
This repository has been archived by the owner on Mar 24, 2023. It is now read-only.

Define non-integer type #124

Merged
merged 4 commits into from
Feb 16, 2021
Merged

Define non-integer type #124

merged 4 commits into from
Feb 16, 2021

Conversation

adlerjohn
Copy link
Member

Fixes #120.

Defines the Decimal non-integer type as a rational number.

@adlerjohn adlerjohn added the enhancement New feature or request label Feb 10, 2021
@adlerjohn adlerjohn self-assigned this Feb 10, 2021
@adlerjohn
Copy link
Member Author

adlerjohn commented Feb 10, 2021

@liamsi is there anything to worry about w.r.t. overflowing on the multiplication? I assume that a simple "if it overflows, the tx is invalid" check is sufficient?

Edit: I think it should be fine as-is for now without explicitly dealing with width of intermediate results and overflows. We can think about that in #108 (though admittedly python uses unlimited-width integers, so...)

@adlerjohn adlerjohn marked this pull request as ready for review February 16, 2021 18:46
| `numerator` | uint64 | Rational numerator. |
| `denominator` | uint64 | Rational denominator. |

Represents a (potentially) non-integer number.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a relatively big change compared to the SDK. I guess switching from the current implementation to this rational type is really low priority, right?

Copy link
Member Author

@adlerjohn adlerjohn Feb 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, it doesn't affect the devnet at all. We can argue that with proper abstractions it doesn't even need to be done for first testnet. Both Decimal as it currently exists in the SDK and Rational (i.e. our Decimal type) would produce similar numbers, so it wouldn't even change the state transitions in anything but in some cases that required lots of precision.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notes under "optional to implement until we there is nothing left to crunch on" :-D (tagging @evan-forbes s.t. he is also aware of this.

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@adlerjohn adlerjohn merged commit e714f9d into master Feb 16, 2021
@adlerjohn adlerjohn deleted the adlerjohn-decimal_type branch February 16, 2021 23:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define decimal type
2 participants