Promote value arithmetic to i128

[ghost]

This does all value arithmetic in i128, which allows u64::MAX value amounts in transactions, and also slightly lessens the risk of overflow problems.

[murisi]

Hi! I looked through the changes and just wanted to contextualize myself a little bit:

• What value range are we targetting for MASP amounts?
• In the Namada crate we need a 256-bit multitoken Amount type. Is there a way of making this i128 change somehow generic so that consumers can supply a type parameter to get i256 Amounts?
• Have you considered tackling overflow problems by instead adding more validation to the accessors/mutators of Amount? And also by preventing direct access to its underlying BTreeMap?
  • Something like using checked_add, checked_sub, checked_mul, and checked_div in Amount's methods to ensure that no multitoken arithmetic operations ever overflow?
• Could space usage be an issue? I'm thinking if the the size of an Amount doubles, then so will AllowedConversion and the data structures holding the up to 200,000 conversions at each epoch on the ledger.

[ghost]

* What value range are we targetting for MASP amounts?

Notes can be a minimum of 0 value and maximum of u64::MAX value, which is a hard limit.
I think everything else is designed around this constraint.

* In the Namada crate we need a 256-bit multitoken Amount type. Is there a way of making this `i128` change somehow generic so that consumers can supply a type parameter to get i256 `Amount`s?

I don't think it will help to make this an i256, or generic. Because each note value is limited to u64::MAX, it would take ~2^63 notes to reach the limits of i128. To handle 256-bit values, there has to be logic that encodes 256-bit values into 64 bits. The purpose of using i128 is only to avoid overflow issues.

* Have you considered tackling overflow problems by instead adding more validation to the accessors/mutators of Amount? And also by preventing direct access to its underlying `BTreeMap`?

Should probably do both, in addition. Since it's not mutually exclusive.

  * Something like using checked_add, checked_sub, checked_mul, and checked_div in `Amount`'s methods to ensure that no multitoken arithmetic operations ever overflow?

Since Amounts can be negative, if we tried to do arithmetic in i64 and use checked_* for verification, then (for example) it would not be possible to Spend a u64::MAX value note and Output a u64::MAX value note, because neither could be safely converted to i64. Whereas promoting a u64 to i128 always succeeds and further should never overflow (although maybe we should use checked_* just in case)

* Could space usage be an issue? I'm thinking if the the size of an `Amount` doubles, then so will `AllowedConversion` and the data structures holding the up to 200,000 conversions at each epoch on the ledger.

Yes, this could be a problem. Since these Amount represent ratios and not actual values, they could be represented with a smaller bit-width. In fact, 32-bits might be enough, if the AllowedConversion serialization is already too large.

[murisi]

@joebebel Thanks for the answers! Understood: the i128s are used in order to avoid overflows of u64s.

It seems that conceptually we are using Amount<i128>s to accumulate value balances, Amount<i64>s when serializing/summarizing value balances, and we may want Amount<i32> or Amount<i64>s for serializing/backing AllowedConversions. Furthermore, we may want Amount<Address, i256> or Amount<AssetType, i256>s in order to provide an API for the Namada SDK that abstracts away how we split up amounts into u64s.

In this light, generics could simultaneously achieve these goals in a way that is consistent with your i128 value balance accumulator approach. Maybe some ideas could be used from here: #52 ?

[ghost]

It seems that conceptually we are using Amount<i128>s to accumulate value balances, Amount<i64>s when serializing/summarizing value balances, and we may want Amount<i32> or Amount<i64>s for serializing/backing AllowedConversions.

This might be good for saving storage costs, although I'm concerned about using i64 when the actual range is -u64::MAX..u64::MAX. But also AllowedConversion should be fairly small, less than i32::MAX so that makes sense there.

Furthermore, we may want Amount<Address, i256> or Amount<AssetType, i256>s in order to provide an API for the Namada SDK that abstracts away how we split up amounts into u64s.

It's possible, but I'm not 100% sure this is the correct place for this - since the API here would have to handle the splitting of one AssetType into 4 others. But it's possible. There's also the thought that we could rename this Amount to ValueSum and have Amount represent an Amount of a single asset only (more in line with librustzcash)

In this light, generics could simultaneously achieve these goals in a way that is consistent with your i128 value balance accumulator approach. Maybe some ideas could be used from here: #52 ?

Yes, I think #52 is the way to do this.

[murisi]

@joebebel Understood. So I've made the following changes in #52 :

• Widened the value balance serializations to be Amount<i128>
• Contracted the AllowedConversion backing to be Amount<i32>
• Renamed Amount to be called ValueSum
• Made transparent input/output values u64 to allow the same range as their shielded equivalents
• Verified that Namada will continue working with this branch integrated

Let me know if anything else remains to be done. Thanks!