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

Begin conversion from BigInteger to OmniValue and Coin #101

Merged
merged 8 commits into from Sep 14, 2015

Conversation

Projects
None yet
2 participants
@msgilligan
Member

msgilligan commented Sep 10, 2015

This PR is partial conversion, but @dexX7 if you think it's OK to merge it to master, I'd like to do so. There are a lot of changes and I'd rather not have to keep merging them.

regTests are currently passing.

/**
* Short cut to confirm a balance.
*/
void assertBalance(Address address, CurrencyID currency, def expectedAvailable, def expectedReserved) {
void assertBalance(Address address, CurrencyID currency, BigDecimal expectedAvailable, BigDecimal expectedReserved) {

This comment has been minimized.

@msgilligan

msgilligan Sep 10, 2015

Member

I think this was the change that found the Groovy bug. (Going from def to BigDecimal)

@msgilligan

msgilligan Sep 10, 2015

Member

I think this was the change that found the Groovy bug. (Going from def to BigDecimal)

@msgilligan msgilligan changed the title from Being conversion from BigInteger to OmniValue and Coin to Begin conversion from BigInteger to OmniValue and Coin Sep 10, 2015

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 10, 2015

Member

Hmmm.. there are a few parts where I'm split, or tend to believe it's a step backwards.

A few examples:

-    final static BigDecimal startBTC = 1.0
-    final static BigDecimal startMSC = 50.0
+    final static Coin startBTC = 1.btc
+    final static OmniDivisibleValue startMSC = OmniDivisibleValue.of(50)

This isn't consistent, and "more work". A divisible amount, whether it's BTC or Omni, expressed as 1.0 or 50.0 is pretty clear and intuitive. The construction via OmniDivisibleValue.of(50) is long and seems unnecesary. 1.btc is fancy, but might be difficult to grasp, because it's very different to other coding styles. While something like 15.msc would be obvious in this context, we often have "amounts of unspecified tokens", so I'm also not sure, how this would be handled.

         def offerTxid = createDexSellOffer(
-                fundedAddress, currencyOffered, amountOffered, desiredBTC, stdBlockSpan, stdCommitFee, actionNew)
+                fundedAddress, currencyOffered, amountOffered, desiredBTC, stdBlockSpan, stdCommitFee.decimalBtc, actionNew)

Somewhat similar: I think it would be better to reduce code size, not add more. In this case for example I'm wondering: why does the stdCommitFee need .decimalBtc, while desiredBTC doesn't?

-        offerAmount == Math.min(amountAvailableAtStart, amountOffered)
+        offerAmount == amountOffered.min(amountAvailableAtStart)

This doesn't make sense to me. :)

I'm very open to the general direction though, and maybe I'm just a bit reserved, because it's so different to what I usually see.

Member

dexX7 commented Sep 10, 2015

Hmmm.. there are a few parts where I'm split, or tend to believe it's a step backwards.

A few examples:

-    final static BigDecimal startBTC = 1.0
-    final static BigDecimal startMSC = 50.0
+    final static Coin startBTC = 1.btc
+    final static OmniDivisibleValue startMSC = OmniDivisibleValue.of(50)

This isn't consistent, and "more work". A divisible amount, whether it's BTC or Omni, expressed as 1.0 or 50.0 is pretty clear and intuitive. The construction via OmniDivisibleValue.of(50) is long and seems unnecesary. 1.btc is fancy, but might be difficult to grasp, because it's very different to other coding styles. While something like 15.msc would be obvious in this context, we often have "amounts of unspecified tokens", so I'm also not sure, how this would be handled.

         def offerTxid = createDexSellOffer(
-                fundedAddress, currencyOffered, amountOffered, desiredBTC, stdBlockSpan, stdCommitFee, actionNew)
+                fundedAddress, currencyOffered, amountOffered, desiredBTC, stdBlockSpan, stdCommitFee.decimalBtc, actionNew)

Somewhat similar: I think it would be better to reduce code size, not add more. In this case for example I'm wondering: why does the stdCommitFee need .decimalBtc, while desiredBTC doesn't?

-        offerAmount == Math.min(amountAvailableAtStart, amountOffered)
+        offerAmount == amountOffered.min(amountAvailableAtStart)

This doesn't make sense to me. :)

I'm very open to the general direction though, and maybe I'm just a bit reserved, because it's so different to what I usually see.

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 10, 2015

Member

it's a step backwards.

It is a step backwards in many places. But sometimes you have to take 1 step backwards to take 2 steps forward.

This isn't consistent,

Agreed.

OmniDivisibleValue.of(50) is long and seems unnecessary

Agreed. That's why I want to move towards 50.divisible

1.btc [is] different to other coding styles.

Agree, but it is common in Groovy and Ruby DSLs. If we document it and use it consistently it should be fine. I'm imagining the people who will read and write these tests are a small group and the time spent learning some new coding styles will be more than repaid by tests that are shorter and more readable.

While something like 15.msc would be obvious in this context

I'm thinking 15.msc will eventually map to creation of the OmniAmount type that holds a currencyID and a value.

we often have "amounts of unspecified tokens"

Right, thus 10.5.divisible and 10.indivisible (not sure if we'll need 1000000000.willets to create an OmniRawValue that could be either divisible or indivisible)

stdCommitFee.decimalBtc

Yes, a step backwards, but when the method being called takes the Coin class for that parameter the .decimalBtc will be removed.

why does the stdCommitFee need .decimalBtc, while desiredBTC doesn't?

Because the current code is incomplete and inconsistent.

amountOffered.min(amountAvailableAtStart)

This change was unrelated to refactoring, but I made it to fix a warning in the IDE. Note that Math.min does not take BigDecimal so conversions were occurring. BigDecimal.min() is an instance method on any BigDecimal instance and does no conversions.

maybe I'm just a bit reserved, because it's so different to what I usually see

I should have made it clear how incomplete and inconsistent this is. Sorry.

I'm very open to the general direction though

Yay! Thanks for your patience and support during re-construction. :)

Member

msgilligan commented Sep 10, 2015

it's a step backwards.

It is a step backwards in many places. But sometimes you have to take 1 step backwards to take 2 steps forward.

This isn't consistent,

Agreed.

OmniDivisibleValue.of(50) is long and seems unnecessary

Agreed. That's why I want to move towards 50.divisible

1.btc [is] different to other coding styles.

Agree, but it is common in Groovy and Ruby DSLs. If we document it and use it consistently it should be fine. I'm imagining the people who will read and write these tests are a small group and the time spent learning some new coding styles will be more than repaid by tests that are shorter and more readable.

While something like 15.msc would be obvious in this context

I'm thinking 15.msc will eventually map to creation of the OmniAmount type that holds a currencyID and a value.

we often have "amounts of unspecified tokens"

Right, thus 10.5.divisible and 10.indivisible (not sure if we'll need 1000000000.willets to create an OmniRawValue that could be either divisible or indivisible)

stdCommitFee.decimalBtc

Yes, a step backwards, but when the method being called takes the Coin class for that parameter the .decimalBtc will be removed.

why does the stdCommitFee need .decimalBtc, while desiredBTC doesn't?

Because the current code is incomplete and inconsistent.

amountOffered.min(amountAvailableAtStart)

This change was unrelated to refactoring, but I made it to fix a warning in the IDE. Note that Math.min does not take BigDecimal so conversions were occurring. BigDecimal.min() is an instance method on any BigDecimal instance and does no conversions.

maybe I'm just a bit reserved, because it's so different to what I usually see

I should have made it clear how incomplete and inconsistent this is. Sorry.

I'm very open to the general direction though

Yay! Thanks for your patience and support during re-construction. :)

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 10, 2015

Member

Sorry, I hope I didn't sound demotivational! :)

Right, thus 10.5.divisible and 10.indivisible (not sure if we'll need 1000000000.willets to create an OmniRawValue that could be either divisible or indivisible)

Ah, this is awesome!

Hmm.. 1000000000.willets would be the same as 1000000000.indivisible, so I'm not sure if it's needed either.

Member

dexX7 commented Sep 10, 2015

Sorry, I hope I didn't sound demotivational! :)

Right, thus 10.5.divisible and 10.indivisible (not sure if we'll need 1000000000.willets to create an OmniRawValue that could be either divisible or indivisible)

Ah, this is awesome!

Hmm.. 1000000000.willets would be the same as 1000000000.indivisible, so I'm not sure if it's needed either.

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 10, 2015

Member

1000000000.willets would be the same as 1000000000.indivisible, so I'm not sure if it's needed either.

There may be cases (return data from some of the RPCs) where we are getting token values and we don't know whether they are divisible or not. In that case we may need to create a type that implements OmniValue, but is neither OmniDivisibleValue or OmniIndivisibleValue. I'm not sure if we'll need it -- I'm hoping we won't.

Note that currently there is a .getPropertyType() method that can be called on OmniValue that seems to be useful.

(Personally I wish there was no distinction between divisible and indivisible tokens in the Spec or the low-level implementation.)

Member

msgilligan commented Sep 10, 2015

1000000000.willets would be the same as 1000000000.indivisible, so I'm not sure if it's needed either.

There may be cases (return data from some of the RPCs) where we are getting token values and we don't know whether they are divisible or not. In that case we may need to create a type that implements OmniValue, but is neither OmniDivisibleValue or OmniIndivisibleValue. I'm not sure if we'll need it -- I'm hoping we won't.

Note that currently there is a .getPropertyType() method that can be called on OmniValue that seems to be useful.

(Personally I wish there was no distinction between divisible and indivisible tokens in the Spec or the low-level implementation.)

@@ -60,18 +62,18 @@ class DexSpec extends BaseRegTestSpec {
offerTx.propertyid == currencyOffered.getValue()
where: "the currency identifier is either MSC or TMSC"
currencyOffered << [CurrencyID.MSC, CurrencyID.TMSC]
currencyOffered << [MSC, TMSC]

This comment has been minimized.

@dexX7

dexX7 Sep 11, 2015

Member

This might create a conflict with Ecosystem.MSC, Ecosystem.TMSC.

@dexX7

dexX7 Sep 11, 2015

Member

This might create a conflict with Ecosystem.MSC, Ecosystem.TMSC.

This comment has been minimized.

@msgilligan

msgilligan Sep 11, 2015

Member

Good point.

When we do the token rebrand to Omni and Test Omni, we'll also need to rename the ecosystems. Hopefully at that point the names will allow for static import without conflict.

@msgilligan

msgilligan Sep 11, 2015

Member

Good point.

When we do the token rebrand to Omni and Test Omni, we'll also need to rename the ecosystems. Hopefully at that point the names will allow for static import without conflict.

This comment has been minimized.

@dexX7

dexX7 Sep 11, 2015

Member

The Omni Core API currently uses "main" and "test" as description for the ecosystem.

Maybe the extra Ecosystem. and CurrencyID. isn't a bad thing. It's very expressive, and if we adopt MAIN and TEST, then MAIN on it's own is probably a bit lonely.

@dexX7

dexX7 Sep 11, 2015

Member

The Omni Core API currently uses "main" and "test" as description for the ecosystem.

Maybe the extra Ecosystem. and CurrencyID. isn't a bad thing. It's very expressive, and if we adopt MAIN and TEST, then MAIN on it's own is probably a bit lonely.

msgilligan added some commits Sep 11, 2015

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 12, 2015

Member

OK, @dexX7 with d3f8025 I'd like to merge this into master. Can you run this branch locally and see if it works for you?

Member

msgilligan commented Sep 12, 2015

OK, @dexX7 with d3f8025 I'd like to merge this into master. Can you run this branch locally and see if it works for you?

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 12, 2015

Member

Can you run this branch locally and see if it works for you?

Yes, tests are currently running. Will report back in a few minutes.

Member

dexX7 commented Sep 12, 2015

Can you run this branch locally and see if it works for you?

Yes, tests are currently running. Will report back in a few minutes.

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 12, 2015

Member

3 tests in foundation.omni.test.rpc.reorgs.PropertyCreationReorgSpec failed so far. More information once all tests are through.

Imho all the extra log output should be debug, not info.

Member

dexX7 commented Sep 12, 2015

3 tests in foundation.omni.test.rpc.reorgs.PropertyCreationReorgSpec failed so far. More information once all tests are through.

Imho all the extra log output should be debug, not info.

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 12, 2015

Member

edit: nevermind ... had an issue running the tests, but it was resolved after rebuilding everything.

Member

dexX7 commented Sep 12, 2015

edit: nevermind ... had an issue running the tests, but it was resolved after rebuilding everything.

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 12, 2015

Member

Imho all the extra log output should be debug, not info.

Fixed.

Member

msgilligan commented Sep 12, 2015

Imho all the extra log output should be debug, not info.

Fixed.

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 12, 2015

Member

I'm about to call it a day, but will look in to the above test errors tomorrow. I'm wondering if you can run that same test command against the master branch.

Note, @dexX7 that I also added this TODO

Member

msgilligan commented Sep 12, 2015

I'm about to call it a day, but will look in to the above test errors tomorrow. I'm wondering if you can run that same test command against the master branch.

Note, @dexX7 that I also added this TODO

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 12, 2015

Member

The issue was that all values specified by numberOfCoins in PropertyCreationReorgSpec are "base units". With the PR it can actually be simplified:

diff --git a/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy b/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
index 35c9e37..16b0d04 100644
--- a/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
+++ b/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
@@ -46,7 +46,7 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         def actorAddress = createFundedAddress(startBTC, startMSC)

         when: "broadcasting and confirming a property creation transaction"
-        def value = OmniValue.of(numberOfTokens, propertyType)
+        def value = OmniValue.of(amount, propertyType)
         def txid = createProperty(actorAddress, ecosystem, value)
         def blockHashOfCreation = generateAndGetBlockHash()

@@ -65,7 +65,7 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         currencyID == expectedCurrencyID

         and: "the creator was credited with the correct amount of created tokens"
-        omniGetBalance(actorAddress, currencyID).balance == expectedBalance
+        omniGetBalance(actorAddress, currencyID).balance == amount

         when: "invalidating the block and property creation transaction"
         invalidateBlock(blockHashOfCreation)
@@ -91,11 +91,11 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         thrown(JsonRPCStatusException)

         where:
-        ecosystem      | propertyType             | numberOfTokens        | expectedBalance               | expectedCurrencyID
-        Ecosystem.MSC  | PropertyType.INDIVISIBLE | new Long("150")       | new BigDecimal("150")         | nextMainPropertyID
-        Ecosystem.MSC  | PropertyType.DIVISIBLE   | new Long("100000000") | new BigDecimal("1.00000000")  | nextMainPropertyID
-        Ecosystem.TMSC | PropertyType.INDIVISIBLE | new Long("350")       | new BigDecimal("350")         | nextTestPropertyID
-        Ecosystem.TMSC | PropertyType.DIVISIBLE   | new Long("250000000") | new BigDecimal("2.50000000")  | nextTestPropertyID
+        ecosystem      | propertyType             | amount                       | expectedCurrencyID
+        Ecosystem.MSC  | PropertyType.INDIVISIBLE | new BigDecimal("150")        | nextMainPropertyID
+        Ecosystem.MSC  | PropertyType.DIVISIBLE   | new BigDecimal("1.00000000") | nextMainPropertyID
+        Ecosystem.TMSC | PropertyType.INDIVISIBLE | new BigDecimal("350")        | nextTestPropertyID
+        Ecosystem.TMSC | PropertyType.DIVISIBLE   | new BigDecimal("2.50000000") | nextTestPropertyID
     }

 }

This patch should fix the issue.

Member

dexX7 commented Sep 12, 2015

The issue was that all values specified by numberOfCoins in PropertyCreationReorgSpec are "base units". With the PR it can actually be simplified:

diff --git a/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy b/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
index 35c9e37..16b0d04 100644
--- a/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
+++ b/omnij-rpc/src/integ/groovy/foundation/omni/test/rpc/reorgs/PropertyCreationReorgSpec.groovy
@@ -46,7 +46,7 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         def actorAddress = createFundedAddress(startBTC, startMSC)

         when: "broadcasting and confirming a property creation transaction"
-        def value = OmniValue.of(numberOfTokens, propertyType)
+        def value = OmniValue.of(amount, propertyType)
         def txid = createProperty(actorAddress, ecosystem, value)
         def blockHashOfCreation = generateAndGetBlockHash()

@@ -65,7 +65,7 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         currencyID == expectedCurrencyID

         and: "the creator was credited with the correct amount of created tokens"
-        omniGetBalance(actorAddress, currencyID).balance == expectedBalance
+        omniGetBalance(actorAddress, currencyID).balance == amount

         when: "invalidating the block and property creation transaction"
         invalidateBlock(blockHashOfCreation)
@@ -91,11 +91,11 @@ class PropertyCreationReorgSpec extends BaseReorgSpec {
         thrown(JsonRPCStatusException)

         where:
-        ecosystem      | propertyType             | numberOfTokens        | expectedBalance               | expectedCurrencyID
-        Ecosystem.MSC  | PropertyType.INDIVISIBLE | new Long("150")       | new BigDecimal("150")         | nextMainPropertyID
-        Ecosystem.MSC  | PropertyType.DIVISIBLE   | new Long("100000000") | new BigDecimal("1.00000000")  | nextMainPropertyID
-        Ecosystem.TMSC | PropertyType.INDIVISIBLE | new Long("350")       | new BigDecimal("350")         | nextTestPropertyID
-        Ecosystem.TMSC | PropertyType.DIVISIBLE   | new Long("250000000") | new BigDecimal("2.50000000")  | nextTestPropertyID
+        ecosystem      | propertyType             | amount                       | expectedCurrencyID
+        Ecosystem.MSC  | PropertyType.INDIVISIBLE | new BigDecimal("150")        | nextMainPropertyID
+        Ecosystem.MSC  | PropertyType.DIVISIBLE   | new BigDecimal("1.00000000") | nextMainPropertyID
+        Ecosystem.TMSC | PropertyType.INDIVISIBLE | new BigDecimal("350")        | nextTestPropertyID
+        Ecosystem.TMSC | PropertyType.DIVISIBLE   | new BigDecimal("2.50000000") | nextTestPropertyID
     }

 }

This patch should fix the issue.

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 12, 2015

Member

Actually, in this case we might just merge propertyType and amount in the "data providing" block completely and use OmniValue.of(amount, propertyType) directly, right? :)

Member

dexX7 commented Sep 12, 2015

Actually, in this case we might just merge propertyType and amount in the "data providing" block completely and use OmniValue.of(amount, propertyType) directly, right? :)

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 12, 2015

Member

Yes. Or maybe N.N.divisible or N.indivisible, etc.

Member

msgilligan commented Sep 12, 2015

Yes. Or maybe N.N.divisible or N.indivisible, etc.

Use OmniValue and DSL to simplify
As suggested by @dexX7 in a comment on PR #101.
@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 14, 2015

Member

@dexX7 Here it is: 02dbea0

Note that omniGetBalance(address, currency).balance will be changed to be an OmniValue, so:

omniGetBalance(actorAddress, currencyID).balance == value.bigDecimalValue()

will change to:

omniGetBalance(actorAddress, currencyID).balance == value
Member

msgilligan commented Sep 14, 2015

@dexX7 Here it is: 02dbea0

Note that omniGetBalance(address, currency).balance will be changed to be an OmniValue, so:

omniGetBalance(actorAddress, currencyID).balance == value.bigDecimalValue()

will change to:

omniGetBalance(actorAddress, currencyID).balance == value
@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 14, 2015

Member

Do I get a thumbs-up on this merge, now? (Even though there is still work to do?)

Member

msgilligan commented Sep 14, 2015

Do I get a thumbs-up on this merge, now? (Even though there is still work to do?)

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 14, 2015

Member

Yes, looks good to me!

Member

dexX7 commented Sep 14, 2015

Yes, looks good to me!

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 14, 2015

Member

Thanks!

Member

msgilligan commented Sep 14, 2015

Thanks!

msgilligan added a commit that referenced this pull request Sep 14, 2015

Merge pull request #101 from OmniLayer/msgilligan-omni-types
Begin conversion from BigInteger to OmniValue and Coin

@msgilligan msgilligan merged commit 1ba919d into master Sep 14, 2015

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Sep 14, 2015

Member

Since I was a bit negative at the beginning, I'd like to underline: I really like where this is going, and 02dbea0 is a great example. :)

Member

dexX7 commented Sep 14, 2015

Since I was a bit negative at the beginning, I'd like to underline: I really like where this is going, and 02dbea0 is a great example. :)

@msgilligan

This comment has been minimized.

Show comment
Hide comment
@msgilligan

msgilligan Sep 14, 2015

Member

Thanks, @dexX7 !

Member

msgilligan commented Sep 14, 2015

Thanks, @dexX7 !

@msgilligan msgilligan deleted the msgilligan-omni-types branch Sep 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment