Number of coins class

[dexX7]

There are a few situations where one needs to be careful when using BigDecimal or rather number conversion.

Here is an example:

This can become an issue when using external data, such as a test plan, with indivisible amounts like 9007199254740993 in this example.

A "native" NumberOfCoins class would be helpful, with BigDecimal as underlying value type, but proper constructors, string representation and so on.

We have divisible and indivisible amounts, with different ranges each, and furthermore different use-cases, for example send_MP which consumes the number as string ("1,0", "0.00000001", "1"), as well as the creation methods, which use the number represented in units, where "1.0" is actually "100000000".

[msgilligan]

I'm thinking we might actually want to use long as the underlying type for a number of coins class. We definitely want to look at the bitcionj Coin class and also at the new Java Money API. I don't feel like I fully understand all the issues, but appreciate your thoughtful and test-based methodology.

[dexX7]

Sure, long would be fine, too, whatever fits. As a starting point I created https://github.com/dexX7/bitcoin-spock/commit/6bbd6d026d5cf577aecc83939e60bde64a69250f, but it was more about "testing the water", so to speak. :)

BitcoinJ's Coin class looks great, might make sense to use this one and extend it to fit our needs.

I think it's all not a very huge deal at all, and only nice to have, as everything we have done so far works pretty well. The biggest "issue" I see is that there are two types of amounts: indivisible amounts and divisible amounts, and it would be nice, if a NumberOfCoins class could deal with both properly.

[msgilligan]

Make sure to take a look at the Bibliography and Links in the JSR 354 Java Money and Currency Specification. There's many years of experience dealing with currency issue to be found there.

Edit: Note the "EVALUATION LICENCE" section of the specification. Wow.

[msgilligan]

Note that org.bitcoinj.core.Coin is a final class and can't be extended. I'm also surprised that it doesn't extend java.lang.Number, but there could be good reasons for that (perhaps related to some of the casting issues we've seen.) We could implement org.bitcoinj.core.Monetary, though.

The Java Money stuff looks like it does a lot of what we need. Downsides are that it may be too complicated for our needs, might require newer versions of Java that preclude Android, and that it isn't final yet.

[msgilligan]

I've been researching JavaMoney further and despite possible issues with Android support, I think we should go ahead and use it for an initial implementation of Coin and Currency classes. There are multiple strategies we can use to support Android if or when needed, but implementing JavaMoney (and requiring Java 8 for the Spock tests) seems like the best way to start.

[dexX7]

I followed your research a bit and I'm impressed by the JavaMoney specification document, but haven't found the time to look into it, yet.

A few questions:

1. Is there any significant overhead vs. BigDecimal?
2. What happens with the data driven tests? Per default 0.0 is transformed into BigDecimal, 0 into Integer, and Long, if it's a larger number. I assume there is no way around this, but we should be fine, if our Money class can interact with those types, right?

Edit: regarding Java 8: if there is no significant downside, then I'd welcome the upgrade. Especially Java Lambdas could be handy.

[msgilligan]

I've been writing some test code and it is going well. I should have something to post in a few days.

1. The implementation of money amounts is flexible and can use BigDecimal or even Long or long so there's no significant overhead vs. BigDecimal
2. No matter how we implement our "coin" class, we'll need a way to convert from BigDecimal, Integer, and Long. We'll just have to figure out the safest and cleanest way to do the conversion.

OK, then @dexX7, Java 8 it is.

[msgilligan]

@dexX7 Here's some exploration of JavaMoney + Groovy: https://github.com/JavaMoney/javamoney-shelter/pull/5

[dexX7]

Very interesting

As far as I can see we'd need to add special methods to handle arithmetic operations, and probably also to compare values. Since Money.of(10, "USD") + Money.of(1, "EUR") appears to throw an exception, I assume this must be done more than once, to cover all combinations such as OmniMoney x BigDecimal, OmniMoney x Long, ...?

We'd probably need two Money classes - for indivisible amounts with range [0, 9223372036854775807] and a step of 1, and another for divisible amounts with range of [0, 92233720368.54775807] with step = 0.00000001? It would be great, if they were convertible from one to the other.

[msgilligan]

I think the arithmetic methods can be added to a base class or possibly even an interface. I also think we can probably get support for that from the JavaMoney and Groovy communities, so hopefully it won't be something that we maintain as Omni-only. Although you may be right about the type combinations issue.

The correct (default?) behavior for plus() / add() or minus() / subtract() should be to throw an exception. It just doesn't make sense to add two different units together. Perhaps there is a special case where you're using an exchange rate, but it seems the use of an exchange rate should probably be explicit.

Note that the "Implementation Recommendations" in the JSR say:

• When adding or subtracting amounts, best practice recommends to use parameters that are instances of MonetaryAmount, hereby ensuring that both amounts have the same currency.
• When multiplying or dividing amount, best practice recommends parameters that are simple numeric values.

(I'm hearing the voice of Fr. Capitolo, my high-school physics teacher saying "Don't forget your D.A.")

The JSR does have complex and full-featured support for conversions and exchange rates.

[dexX7]

I've been thinking more about it, especially regarding the two units: the underlying data type is a 64 bit wide signed integer for both, and "divisibility", implied by the property type, comes into play in an UI or when interacting with the RPC interface of Master/Omni Core. Strictly seen one could say we're not combining different currencies, but have to handle different representations of one.

Let's take a look at the context:

• RPC input for Master/Omni Core is string: "##########0.########", "##################0"
• RPC output of Master/Omni Core is string: "##########0.00000000", "##################0"
• RPC input of Bitcoin Core is decimal: #######0.########
• RPC output of Bitcoin Core is decimal: #######0.00000000

Basic operations:

• add, subtract, divide and multiply values, including mixed types (e.g Integer * OmniMoney)
• compare values, including mixed types (e.g. OmniMoney < BigDecimal, OmniMoney == Long)

We need to:

• handle input from test plans as well as hard coded numbers, which natively translate to BigDecimal, Long and Integer
• process RPC results of strings and decimals, and be able to handle it as Object
• before transmitting via RPC: convert Money back to strings or decimals with correct representation
• convert OmniMoney (and other fields) to hex-encoded strings, when constructing raw transactions, with correct representation (e.g. 1.0 MSC => 100000000 base units => "0000000005f5e100")

Usually "divisible amounts" convert to 100000000x "base units", and "indivisible amounts" convert to 1x "base units".

There is an exception related to crowdsales, see: https://github.com/mastercoin-MSC/mastercore/issues/234#issuecomment-71788525

[msgilligan]

This looks like a complete specification for what we need. You've looked at the spec carefully enough to see the use of NumberValue vs MonetaryAmount -- one is a numeric value only the other is essentially a type of (value, currency)

I'm still undecided on whether we should try to require JavaMoney framework for the basic classes or whether we should design classes/interfaces that don't depend on classes/interfaces in JavaMoney, but are compatible with it. (The lack of multiple inheritance or traits in Java 6 may be an issue here)

[dexX7]

I'm still undecided on whether we should try to require JavaMoney framework for the basic classes or whether we should design classes/interfaces that don't depend on classes/interfaces in JavaMoney, but are compatible with it.

Sorry, I'm not sure, if I understand: are you wondering, if we should work with JavaMoney or use BigDecimal, Long, ... as underlying data type instead and mirror JavaMoney? Exposing and using interfaces should probably done either way.

This could actually expand into a larger scope: "money" or "number" is one aspect, and another one is "being a transaction field", which Ecosystem, CurrencyID and others have in common. The raw transaction creation for sendrawtx_MP, currently done by createPropertyHex, createDexSellOfferHex ..., is a significant part, which a new or different design approach should factor in.

At the moment objects are "passively processed": String.format("%016x", numberOfCoins.longValue()), but it's also thinkable to move the serialization and handle it somewhere else, for example by exposing a method such as numberOfCoins.serialize(), or maybe something entirely different..?

[msgilligan]

Yes, @dexX7 that's basically what I'm wondering. Unfortunately the type that would be most useful to us is NumberValue which is an abstract class not an interface, so that would give us a dependency on JavaMoney. Since the main purpose of this project (at this point, at least) is testing Omni Core, we probably shouldn't worry so much about Android and just implement things with the JavaMoney dependency and then figure out how to support Android later. Although we could define some common interfaces and use those to define parameters to the RPC methods. (I doubt Android apps will ever be using the RPCs directly, anyway)

I don't understand the raw transaction details as well as I should. That said, it seems we could use some kind of a builder approach to create them from our various types. The raw transaction support is something that we'll definitely want on Android, I think.

    RawOmniTx tx = new OmniTxBuildier()
        .addNumberofCoins(var1)
        .addOtherParam(var2)
        .build()

This would free us from having to implement Omni-specific serialization in the other classes.

[msgilligan]

The JavaMoney API Backport project when final today and supports Android. This should be what we need: https://github.com/JavaMoney/jsr354-api-bp/releases

@dexX7

[msgilligan]

OK, I added the JavaMoney API Backport JAR to build.gradle file and created Skeleton OmniValue and OmniAmount classes. Not my top priority now, but will be soon.

[msgilligan]

@dexX7: can we close this? Do OmniValue, OmniDivisbleValue, and OmniIndivisibleValue do the job?

[dexX7]

Yes! :)