New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert BalancyEntry from BigDecimal to OmniValue #117

Merged
merged 11 commits into from Nov 24, 2015

Conversation

Projects
None yet
2 participants
@msgilligan
Member

msgilligan commented Oct 10, 2015

@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

This comment has been minimized.

Member

msgilligan commented Oct 10, 2015

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 msgilligan changed the title from Convert BalancyEntry from BigDecimal to OmniValue (Work-in-progress) to Convert BalancyEntry from BigDecimal to OmniValue Oct 10, 2015

@msgilligan

This comment has been minimized.

Member

msgilligan commented Oct 10, 2015

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

@@ -116,8 +116,8 @@ class ConsensusCLI extends CliCommand {
void output(ConsensusSnapshot snap, PrintWriter writer, boolean tsv) {
snap.entries.each { Address address, BalanceEntry entry ->
String balance = entry.balance.toPlainString()
String reserved = entry.reserved.toPlainString()
String balance = entry.balance.toString()

This comment has been minimized.

@dexX7

dexX7 Oct 10, 2015

Member

I haven't really tested the toString() behavior of OmniValue, but if I recall correctly, then toPlainString() was used here and there to avoid scientific notations.

This comment has been minimized.

@msgilligan

msgilligan Oct 11, 2015

Member

Yeah, I probably need to update the toString() method or provide an alternative. Is this something you use regularly?

This comment has been minimized.

@dexX7

dexX7 Oct 11, 2015

Member

It was necessary to use toPlainString() for the parameter lists for the old send(), but now it's purely cosmetic. It's used in the test reports.

@dexX7

This comment has been minimized.

Member

dexX7 commented Oct 10, 2015

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

This comment has been minimized.

Member

msgilligan commented Oct 11, 2015

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.

Update for BalanceEntry containing OmniValue
Required extra call for each property to find PropertyType.
@dexX7

This comment has been minimized.

Member

dexX7 commented Oct 11, 2015

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()

msgilligan added some commits Oct 12, 2015

Updates to Chest and Omniwallet consensus tools for OmniValue
Note: There is a “hack” in the Chest tool that will only work for current consensus tests
(e.g. only MSC, TMSC, MaidSafe, and TetherUSD)

msgilligan added a commit that referenced this pull request Nov 24, 2015

Merge pull request #117 from OmniLayer/msgilligan-omnivalue-balances
Convert BalancyEntry from BigDecimal to OmniValue

@msgilligan msgilligan merged commit 2fd3b19 into master Nov 24, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment