OmniLayer / OmniJ

OmniLayer for Java, JVM, and Android
Apache License 2.0
133 stars 89 forks source link

Convert BalancyEntry from BigDecimal to OmniValue #117

Closed msgilligan closed 8 years ago

msgilligan commented 9 years ago

@dexX7 I started on this conversion. So far so good, but using .equals() instead of == is not very nice, but I don't see any real alternative.

msgilligan commented 9 years ago

I expected tests to fail on the first checkin, but I expected more failures than I got. So a couple more commits and this is passing tests and should be ready to merge.

Many of the other RPCs are still using default Jackson conversion (to Map types) so they will require conversion to POJOs containing OmniValue.

msgilligan commented 9 years ago

Oh, right. I haven't run the consensus tests.

dexX7 commented 9 years ago

The tests fail due to the itemToEntry methods:

    private BalanceEntry itemToEntry(Object item) {
        BigDecimal balance = jsonToBigDecimal(item.balance)
        BigDecimal reserved = jsonToBigDecimal(item.reserved_balance)
        return new BalanceEntry(balance, reserved)
    }

Since OmniValue.of() doesn't exist, I tried OmniDivisibleValue.of(), however this causes several errors:


Rounding necessary
java.lang.ArithmeticException: Rounding necessary
    at java.math.BigDecimal.longValueExact(BigDecimal.java:2988)
    at java.math.BigDecimal.intValueExact(BigDecimal.java:3047)
    at foundation.omni.OmniDivisibleValue.intValueExact(OmniDivisibleValue.java:84)
    at foundation.omni.OmniDivisibleValue.intValue(OmniDivisibleValue.java:124)
    at foundation.omni.consensus.ChestConsensusTool.getConsensusForCurrency_closure1(ChestConsensusTool.groovy:54)
    at groovy.lang.Closure.call(Closure.java:426)
    at groovy.lang.Closure.call(Closure.java:442)
    at foundation.omni.consensus.ChestConsensusTool.getConsensusForCurrency(ChestConsensusTool.groovy:49)
    at foundation.omni.consensus.ChestConsensusTool.getConsensusSnapshot(ChestConsensusTool.groovy:133)
    at foundation.omni.test.consensus.ChestServerSpec.Can get Chest consensus data(ChestServerSpec.groovy:33)

And in the tests:

Condition not satisfied:

entry1 == entry2
|      |  |
|      |  [balance: 9518.00000000, reserved: 0E-8 ]
|      false
[balance: 9518, reserved: 0 ]

I wasn't sure how to use the OmniValueDeserializer.

msgilligan commented 9 years ago

OmniValue.of(number) can't exist because we'd have to know whether it was intended to be a divisible or indivisible property. We could add a string constructor that works with the strings that are returned from Omni Core, but I don't know if Omniwallet or Chest are returning strings in the same format (i.e. where we can assume that the property is divisible if-and-only-if the string value has a decimal point.

OmniValueDeserializer (or any serializer or deserializer) isn't really designed for use independent of Jackson. I should export that functionality to a (static?) method that is reusable. Perhaps it could be a string constructor or factory method of OmniValue.

dexX7 commented 9 years ago

We could add a string constructor that works with the strings that are returned from Omni Core,

As long as not all RPC results are strong typed, this would probably simplify quite a few tests actually, like:

tx.amount as BigDecimal == expectedBalance.numberValue()