Simplify tests with OmniValue

[dexX7]

Many tests are a lot less readable due to value conversions such as:

omniGetTransaction(txid).type == "MetaDEx trade"
omniGetTransaction(txid).propertyidforsale == CurrencyID.TMSC_VALUE // no!
omniGetTransaction(txid).propertyidforsaleisdivisible
omniGetTransaction(txid).amountforsale as BigDecimal == amountForSale // no!
omniGetTransaction(txid).amountremaining as BigDecimal == amountForSale // no!
omniGetTransaction(txid).propertyiddesired == nonManagedID.longValue() // no!

Since I have some work in progress, which uses a lot similar code like this (see #82 (comment)), I'm wondering, how it could be improved.

I see basically two options:

1. All RPC results are strong typed, and amounts in RPC results are no longer string, property identifiers no longer long etc., but instead all fields of the results are parsed early, and converted into OmniValue, CurrencyID, ...

2. Or there could be comparison (and basic math) operators to compare string with OmniValue, long with CurrencyID, ...

Are there alternatives? What would be the way to go?

[msgilligan]

I like option (1) and have created a few strongly-typed response objects for some of the other RPCs.

However, there is also room for various utilities and convenience methods as there are always corner cases where those solutions come in handy.

For Bitcoin transactions the bitcoinj Transaction is used, of course. It would definitely be nice to have an OmniTransaction object. I started one (and decided to at least try it as an extension of a bitcoinj Transaction) here: https://github.com/OmniLayer/OmniJ/blob/master/omnij-core/src/main/java/foundation/omni/tx/OmniTransaction.java

[msgilligan]

Note that we should be able to use OmniAmount to simplify some things even further by combining the propertyID and value in one variable.

[dexX7]

I like option (1) and have created a few strongly-typed response objects for some of the other RPCs.

Agreed, this would be great. I think this would be pretty straight forward for many calls, though I'm not really sure how it would look like for the different transaction types.

https://github.com/OmniLayer/OmniJ/blob/master/omnij-core/src/main/java/foundation/omni/tx/OmniTransaction.java

We have some base properties, such as the sender, receiver, a payload etc., but there are about 16 different transaction types, each with different properties.

[msgilligan]

There's a pull request for what we could (generously) call "Phase 1" of this work: #101

[msgilligan]

Next steps in rough order:

1. Convert all RPC calls in bitcoinj-addons to use the Coin class for parameters and return values. See Convert from BigDecimal to Coin ConsensusJ/consensusj#3. -- Done!
2. Update OmniJ to use the new BitcoinClient. -- Done!
3. Continue converting to 1.btc, 1.divisible, and 1.indivisble where possible.
4. Convert the Omni RPC calls to use OmniValue instead of BigDecimal
5. Covert remainder of the test code.

[dexX7]

@msgilligan: I like the plan!

I thought a bit more about the initial issue, and with #98 I noticed something interesting: .getValue() is already much cleaner in my opinion:

- omniGetTransaction(txid).propertyidforsale == CurrencyID.TMSC_VALUE // no!
- omniGetTransaction(txid).propertyiddesired == nonManagedID.longValue() // no!
+ omniGetTransaction(txid).propertyidforsale == CurrencyID.TMSC.getValue()
+ omniGetTransaction(txid).propertyiddesired == nonManagedID.getValue()

Would it be unreasonable to actually do the conversion on the right hand side, instead of the left? Such as:

- omniGetTransaction(txid).amountforsale as BigDecimal == amountForSale // no!
- omniGetTransaction(txid).amountremaining as BigDecimal == amountForSale // no!
+ omniGetTransaction(txid).amountforsale == amountForSale.getValue()
+ omniGetTransaction(txid).amountremaining == amountForSale.getValue()

?

Since you mentioned strong typed responses and Jackson, I'd like to know, if you may have a quick introduction or could point in the direction where I get more information? This sounds as if the manual mapping with a plain data object might no longer be the way to go?

Last but no least: I often call a RPC over and over again, mostly for convenience, instead of fetching the result once:

omniGetTransaction(txid).valid
omniGetTransaction(txid).amount as BigDecimal != orderAmount
omniGetTransaction(txid).amount as BigDecimal == balanceAtStart // less than offered!
// ...

Which style do you prefer?

[msgilligan]

.getValue() is already much cleaner

Well, in Groovy, property access can be shortened to .value which is even shorter.

Would it be unreasonable to actually do the conversion on the right hand side, instead of the left?

Not unreasonable. When we're done with this transition, in most cases conversion will not be needed. When conversion is needed, I think I prefer it on the right hand side.

Jackson … if you may have a quick introduction or could point in the direction where I get more information?

The wiki seems to be a good place to get started, try JacksonInFiveMinutes. You could also take a look at the serializer/module work I've started. I'm just learning this stuff, too. But it's high on my priority list so I should know more soon.

I often call a RPC over and over again, mostly for convenience, instead of fetching the result once

I would try to avoid doing that for performance reasons. In general I prefer readability to performance in tests, but I do try to avoid extra network I/O. I also generally try to put RPC calls in a when: clause and the testing of the results in a then: clause. So something like:

    when:
    def omniTx = omniGetTransaction(txid)

    then:
    omniTx.valid
    omniTx.amount != orderAmount
    omniTx.amount == balanceAtStart // less than offered!

(and hopefully the types/units will match, so I've shown it with no conversion required.)

[msgilligan]

#1 and #2 from the plan above are now complete. I'm going to release and publish to BinTray version 0.0.7 of bitcoinj-addons. Once that is done, I'd like to merge PR #101.

What do you think, @dexX7 ?

[dexX7]

What do you think, @dexX7 ?

Oh sorry, I just stumbled upon your question. I assume that's already clear via the discussion in #101? I support the plan. :)

I'm currently wondering about step 4 and 5: convert RPCs to use OmniValue and use it.

As intermediate step, would it make sense to add comparison methods to OmniValue, to support native comparisons such as the following?

- crowdsaleInfo.tokensperunit as BigDecimal == 3400.indivisible.bigDecimalValue()
- balanceEntry.balance == 123.45.divisible.bigDecimalValue()
+ crowdsaleInfo.tokensperunit as BigDecimal == 3400.indivisible
+ balanceEntry.balance == 123.45.divisible

On the other hand, since the plan is to move completely to OmniValue, we may just skip this.

[dexX7]

Ah, another question: I'm wondering, whether we should support the construction of OmniValue by String.

Might be nice to:

- someTx.amount as BigDecimal == 1234.56.divisible.bigDecimalValue()
- someTx.amount as BigDecimal == 555.indivisible.bigDecimalValue()
+ someTx.amount as OmniDivisibleValue == 1234.56.divisible
+ someTx.amount as OmniIndivisibleValue == 555.indivisible

Likewise, I'm curious why OmniDivisibleValue .of(bigDecimal) is used instead of new OmniDivisibleValue(bigDecimal)? x.of(y) is no new invention, and there is also BigDecimal.valueOf() and others, so I'm sure there is a reason.

Alternatively we may explicitly add custom conversions via Groovy's asType() (see: 3.5. Custom type coercion or 8.7. Coercion operator).

Then again, you mentioned in response to how we should deal with the comparison of untyped RPC results and OmniValue:

Not unreasonable. When we're done with this transition, in most cases conversion will not be needed. When conversion is needed, I think I prefer it on the right hand side. [this is what I'm interested in]

... we should probably instead use something like:

- someTx.amount as BigDecimal == 1234.56.divisible.bigDecimalValue()
+ someTx.amount == 1234.56.divisible.RpcValue() // amount: String with a specific format

?

[msgilligan]

I assume that's already clear via the discussion in #101?

Yes.

On the other hand, since the plan is to move completely to OmniValue, we may just skip this.

I'm thinking the move is coming pretty soon. Though there will be (already are, I think) comparison methods on OmniValue.

I'm wondering, whether we should support the construction of OmniValue by String

I don't think it is necessary. Once we have everything else implemented, I don't think it will be needed. But, if you make a case for it, we can add it. My plan is that we soul be directly able to say:

someTx.amount  == 555.indivisible

why OmniDivisibleValue .of(bigDecimal) is used instead of new OmniDivisibleValue(bigDecimal)

I took the .of() from JavaMoney. I like it because it's short. I also prefer using a static creator to the constructor because there are cases where the class returned is not the same as the static class (e.g. OmniValue.of(value, propertyType)

how we should deal with the comparison of untyped RPC results and OmniValue

I'm hoping we'll be able to make all the RPC results typed. (Even if this means creating an OmniValue subclass for unknown property types)

[msgilligan]

@dexX7 Is there anything else we should do for this issue before closing it?

(I'm open to creating new issues if there's something related that we can postpone haha)

[dexX7]

Is there anything else we should do for this issue before closing it?

No. :)