[COMP-1165] fixed precision decimal implementation (wip) #11

waynenilsen · 2020-11-20T17:09:47Z

As discussed bigint + u8 for decimal representation also avoiding the
use of generics for the most part. For now the only imaginable way
to have errors in my mind is decimal mismatch.

Lots to fill in here and some open questions

Do we want this in a separate crate? I imagine it is possible the
eth-rpc crate may ultimately depend on this functionality.

I just figured out that u128 is natively available in rust on all
targets as LLVM has support for it rust simply exposes that. See
this pr from late
2016. This has been stablized in 1.26.
The representable range here even with a token that is using 18
decimals of precision is 19 9s. If you have 10^19 of something
I think it is alright to say that you need to do two transactions.

I believe that u128 represents the best combination of speed
flexibility and consistency. It is very unlikely to me that
any token balance will natively overflow this representation.

linear · 2020-11-20T17:09:49Z

COMP-1165 Compound Interest [Compound Chain]

We need to calculate continuously compounding interest for CASH on the Compound Chain. This calculation is easy mathematically e^rt, but is actually difficult to compute and store with a bounded error, esp. as the rate changes over time. This ticket is about choosing a strong method for computing and storing the continuously compounding interest for CASH for both the total CASH supply and the yield to accounts and the spread to miners. The primary goals should be, in order:

The Golden Equality (sum of accounts equals total)
Bounded error
Computational efficiency
Interoperability (e.g. we can compute on Ethereum using the same technique)

Feel free to use libraries and take wisdom from other projects for this work. Additionally, pay attention to the decisions made by COMP-1143 which is continuous interest on the Ethereum chain.

pallets/cash/src/fixed_precision.rs

waynenilsen · 2020-11-24T19:24:19Z

Summarizing a call that we just had

Amount is essentially BigUint everywhere (for now)
Decimals are inherent in various contexts and passing around number of decimals should be avoided wherever possible
Implementation should be set up so that we can swap out storage (bigint for something else) in the future without having to really pay any cost
Implementation should provide that generic swap out in a "zero-cost abstraction"

not pursuing for now

some kind of rational number representation for some quantities
more than one amount type (stick to one amount type for now to keep it simple)

those things may change over time as the implementation details become clearer

jflatow · 2020-12-02T19:00:54Z

Great start, though I sort of think we should try this math out in real situations (implementing some core fns using it) to make sure its ergonomic. Alternatively we merge early and iterate on it later

coburncoburn · 2020-12-02T21:47:17Z

pallets/cash/src/amount.rs

+/// 12345.6789. The decimals are stored separately.
+#[derive(Clone, PartialEq, Debug)]
+pub struct Amount {
+    mantissa: MantissaType,


don't we want a decimals field here too? like 1 eth = Amount{ mantissa: 1e18, decimals: 18 } ?

Shouldn't Amount be just type Amount = BigUint actually?

i have type Amount = u32 as a stub for the moment, I figure we probably want this thing to map pretty closely to the Decimal type in Elixir tbh, seemed to me like the discussion was pretty much there, but maybe I misunderstand

I thought we landed on Amounts are infinite precision unsigned integers? I don't think we really want to store decimals on chain for every single amount and its kind of nice to deal with decimals explicitly when we need them for display (or importing prices)

At the end of the day, we probably should start writing functions before musing on this too much further, since it should clear up our path

Not really sure how this is a blocker if we just use type Amount = BigUint or even uint128 for writing functions now

don't we want a decimals field here too? like 1 eth = Amount{ mantissa: 1e18, decimals: 18 } ?

I had this in a previous commit and took it out because there really isn't any need to carry around the decimals everywhere. It does provide some nice runtime guarantees but they aren't worth the storage cost as per standup discussion a while back

Shouldn't Amount be just type Amount = BigUint actually?

The biggest difference between this type wrapper style and the type alias style is that the type wrapper style allows us to implement traits defined outside the current crate. For more discussion see https://doc.rust-lang.org/stable/error-index.html#E0117

This is necessary to, for example, implement serialization and deserialization for parity's binary encoding (name escapes me now) traits.

github-actions · 2020-12-03T00:26:10Z

🫖 View Test Results

github-actions · 2020-12-04T01:03:30Z

🫖 View Test Results

jflatow · 2020-12-04T02:19:39Z

pallets/cash/src/amount.rs

+                rhs.decimals
+            );
+        }
+
        // note - this cannot fail with BigUint but that will change based on underlying storage


What will change based on storage? 🤔

the add function may fail due to overflow if the storage type for the mantissa is overflowable

in order for us to be "able to switch out what the mantissa is stored as easily" we need to be able to fail during the add function here

As discussed bigint + u8 for decimal representation also avoiding the use of generics for the most part. For now the only imaginable way to have errors in my mind is decimal mismatch. Lots to fill in here and some open questions Do we want this in a separate crate? I imagine it is possible the eth-rpc crate may ultimately depend on this functionality. I just figured out that u128 is natively available in rust on all targets as LLVM has support for it rust simply exposes that. See this pr from late 2016. This has been stablized in 1.26. The representable range here even with a token that is using 18 decimals of precision is 19 9s. If you have 10^19 of something I think it is alright to say that you need to do two transactions. I believe that u128 represents the best combination of speed flexibility and consistency. It is very unlikely to me that any token balance will natively overflow this representation.

github-actions · 2020-12-04T03:21:24Z

🫖 View Test Results

github-actions · 2020-12-04T04:04:16Z

🫖 View Test Results

github-actions · 2020-12-04T04:04:24Z

Unit Test Results

  1 files ±0 11 suites ±0 0s ⏱️ ±0s
16 tests +3 16 ✔️ +3 0 💤 ±0 0 ❌ ±0
21 runs +3 21 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit f8f7f50. ± Comparison against base commit bfceb1c.

hayesgm · 2020-12-04T04:30:24Z

I guess we should just start writing those and then decide

On Thu, Dec 3, 2020 at 8:01 PM Jared Flatow ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pallets/cash/src/amount.rs <#11 (comment)> : > +use num_bigint::BigUint; + +/// The type of the decimal field. +pub type DecimalType = u8; + +/// The type of the mantissa field +pub type MantissaType = BigUint; + +/// Represents a decimal number in base 10 with fixed precision. The number of decimals depends +/// on the amount being represented and is not stored alongside the amount. +/// +/// For example, if the mantissa is 123456789 and decimals is 4 the number that is represented is +/// 12345.6789. The decimals are stored separately. +#[derive(Clone, PartialEq, Debug)] +pub struct Amount { + mantissa: MantissaType, Not really sure how this is a blocker if we just use type Amount = BigUint or even uint128 for writing functions now — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCFMAHFOPNCWMAZYPHVP3STBNJXANCNFSM4T5BAPGA> . --

geoffrey hayes geoff@compound.finance 415.735.8262

waynenilsen · 2020-12-07T17:30:57Z

I'm going to close this since it has been integrated into #26

waynenilsen mentioned this pull request Nov 20, 2020

[COMP-1165] add fixed precision math library (wip) #8

Closed

jflatow reviewed Nov 23, 2020

View reviewed changes

pallets/cash/src/fixed_precision.rs Outdated Show resolved Hide resolved

coburncoburn reviewed Dec 2, 2020

View reviewed changes

hayesgm force-pushed the wayne/fixed-precision-3 branch from 11779a6 to ad74ba7 Compare December 3, 2020 00:03

This comment has been minimized.

Sign in to view

jflatow reviewed Dec 4, 2020

View reviewed changes

hayesgm added 2 commits December 3, 2020 19:08

Add decimals to amount type

2be9974

hayesgm force-pushed the wayne/fixed-precision-3 branch from 3da74db to 2be9974 Compare December 4, 2020 03:08

This comment has been minimized.

Sign in to view

Explore adding error handling

f8f7f50

waynenilsen closed this Dec 7, 2020

jflatow deleted the wayne/fixed-precision-3 branch December 28, 2020 18:29

[COMP-1165] fixed precision decimal implementation (wip) #11

[COMP-1165] fixed precision decimal implementation (wip) #11

Uh oh!

Conversation

waynenilsen commented Nov 20, 2020

Uh oh!

linear bot commented Nov 20, 2020

Uh oh!

Uh oh!

waynenilsen commented Nov 24, 2020

Uh oh!

jflatow commented Dec 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 3, 2020

Uh oh!

This comment has been minimized.

github-actions bot commented Dec 4, 2020

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

waynenilsen Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 4, 2020

Uh oh!

This comment has been minimized.

github-actions bot commented Dec 4, 2020

Uh oh!

github-actions bot commented Dec 4, 2020

Unit Test Results

Uh oh!

hayesgm commented Dec 4, 2020 via email

Uh oh!

waynenilsen commented Dec 7, 2020

Uh oh!

Uh oh!

waynenilsen Dec 4, 2020 •

edited

Loading