From 2f51ced5c0abc390261b0a7a804d7b0a0885df4c Mon Sep 17 00:00:00 2001 From: catbref Date: Fri, 12 Apr 2019 08:44:13 +0100 Subject: [PATCH] Fix incorrectly applied price improvement refund. Also fix broken tests which contributed to this bug slipping by. --- src/main/java/org/qora/asset/Order.java | 4 +- .../org/qora/test/assets/NewTradingTests.java | 76 +++++++++++++++---- .../org/qora/test/assets/OldTradingTests.java | 8 +- .../java/org/qora/test/common/AssetUtils.java | 8 +- 4 files changed, 70 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/qora/asset/Order.java b/src/main/java/org/qora/asset/Order.java index 9efc3917..0c8dfa8e 100644 --- a/src/main/java/org/qora/asset/Order.java +++ b/src/main/java/org/qora/asset/Order.java @@ -413,8 +413,8 @@ public class Order { BigDecimal tradedWantAmount = (isOurOrderNewPricing && haveAssetId > wantAssetId) ? returnAmountTraded : matchedAmount; BigDecimal tradedHaveAmount = (isOurOrderNewPricing && haveAssetId > wantAssetId) ? matchedAmount : returnAmountTraded; - // We also need to know how much have-asset to refund based on price improvement ('new' pricing only) - BigDecimal haveAssetRefund = !isOurOrderNewPricing ? BigDecimal.ZERO : ourPrice.subtract(theirPrice).abs().multiply(tradedWantAmount).setScale(8, RoundingMode.DOWN); + // We also need to know how much have-asset to refund based on price improvement ('new' pricing only and only one direction applies) + BigDecimal haveAssetRefund = isOurOrderNewPricing && haveAssetId < wantAssetId ? ourPrice.subtract(theirPrice).abs().multiply(matchedAmount).setScale(8, RoundingMode.DOWN) : BigDecimal.ZERO; LOGGER.trace(String.format("We traded %s %s (have-asset) for %s %s (want-asset), saving %s %s (have-asset)", tradedHaveAmount.toPlainString(), haveAssetData.getName(), diff --git a/src/test/java/org/qora/test/assets/NewTradingTests.java b/src/test/java/org/qora/test/assets/NewTradingTests.java index 07b1d563..a1bb4a2a 100644 --- a/src/test/java/org/qora/test/assets/NewTradingTests.java +++ b/src/test/java/org/qora/test/assets/NewTradingTests.java @@ -55,7 +55,7 @@ public class NewTradingTests extends Common { // Alice should be -testAmount, +qoraAmount // Bob should be -qoraAmount, +testAmount - AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } @Test @@ -90,7 +90,7 @@ public class NewTradingTests extends Common { // Alice should be -testAmount, +otherAmount // Bob should be -otherAmount, +testAmount - AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -145,7 +145,7 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = qoraAmount; final BigDecimal bobReturn = otherAmount; - AssetUtils.genericTradeTest(otherAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(otherAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -190,7 +190,7 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = bobAmount; // riches final BigDecimal bobReturn = bobAmount.multiply(alicePrice).setScale(8); // rags - AssetUtils.genericTradeTest(ragsAssetId, richesAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(ragsAssetId, richesAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -257,7 +257,7 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = aliceAmount; // riches final BigDecimal bobReturn = aliceAmount.multiply(alicePrice).setScale(8); - AssetUtils.genericTradeTest(ragsAssetId, richesAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(ragsAssetId, richesAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -292,7 +292,57 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = new BigDecimal("24.00000000").setScale(8); // OTHER final BigDecimal bobReturn = new BigDecimal("1.99999992").setScale(8); // TEST - AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); + } + + /** + * Check matching of orders with price improvement. + */ + @Test + public void testSimplePriceImprovement() throws DataException { + long otherAssetId; + try (Repository repository = RepositoryManager.getRepository()) { + otherAssetId = AssetUtils.issueAsset(repository, "bob", "OTHER", 5000L, true); + } + + // Alice is buying OTHER + final BigDecimal aliceAmount = new BigDecimal("100").setScale(8); // OTHER + final BigDecimal alicePrice = new BigDecimal("0.3").setScale(8); // TEST/OTHER + final BigDecimal aliceCommitment = new BigDecimal("30").setScale(8); // 100 * 0.3 = 30 TEST + + // Bob is selling OTHER + final BigDecimal bobAmount = new BigDecimal("100").setScale(8); // OTHER + final BigDecimal bobPrice = new BigDecimal("0.2").setScale(8); // TEST/OTHER + final BigDecimal bobCommitment = new BigDecimal("100").setScale(8); // OTHER + + // Expected traded amounts + final BigDecimal aliceReturn = new BigDecimal("100").setScale(8); // OTHER + final BigDecimal bobReturn = new BigDecimal("30").setScale(8); // TEST + + AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); + } + + /** + * Check matching of orders with price improvement. + */ + @Test + public void testSimplePriceImprovementInverted() throws DataException { + // Alice is seller TEST + final BigDecimal aliceAmount = new BigDecimal("100").setScale(8); // TEST + final BigDecimal alicePrice = new BigDecimal("2").setScale(8); // QORA/TEST + final BigDecimal aliceCommitment = new BigDecimal("100").setScale(8); // TEST + + // Bob is buying TEST + final BigDecimal bobAmount = new BigDecimal("50").setScale(8); // TEST + final BigDecimal bobPrice = new BigDecimal("3").setScale(8); // QORA/TEST + final BigDecimal bobCommitment = new BigDecimal("150").setScale(8); // 50 * 3 = 150 QORA + + // Expected traded amounts + final BigDecimal aliceReturn = new BigDecimal("100").setScale(8); // 50 * 2 = 100 QORA + final BigDecimal bobReturn = new BigDecimal("50").setScale(8); // 50 TEST + final BigDecimal bobSaving = new BigDecimal("50").setScale(8); // 50 * (3 - 2) = 50 QORA + + AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, bobSaving); } /** @@ -333,12 +383,12 @@ public class NewTradingTests extends Common { // We're expecting Alice's order to match with Chloe's order (as Bob's and Dilberts's orders have worse prices) BigDecimal matchedQoraAmount = matchingTestAssetAmount.multiply(bestPrice).setScale(8, RoundingMode.DOWN); BigDecimal tradedTestAssetAmount = matchingTestAssetAmount; - // Due to price improvement, Alice should get back some of her TEST - BigDecimal testRefund = minimalPrice.subtract(bestPrice).abs().multiply(matchedQoraAmount).setScale(8, RoundingMode.DOWN); + // NO refund due to price improvement - Alice receives more QORA back than she was expecting + BigDecimal aliceSaving = BigDecimal.ZERO; // Alice TEST BigDecimal aliceCommitment = matchingTestAssetAmount; - expectedBalance = initialBalances.get("alice").get(AssetUtils.testAssetId).subtract(aliceCommitment).add(testRefund); + expectedBalance = initialBalances.get("alice").get(AssetUtils.testAssetId).subtract(aliceCommitment).add(aliceSaving); AccountUtils.assertBalance(repository, "alice", AssetUtils.testAssetId, expectedBalance); // Alice QORA @@ -437,11 +487,11 @@ public class NewTradingTests extends Common { BigDecimal matchedOtherAmount = aliceOtherAmount; BigDecimal tradedTestAmount = aliceOtherAmount.multiply(bestPrice).setScale(8, RoundingMode.DOWN); // Due to price improvement, Alice should get back some of her TEST - BigDecimal testRefund = maximalPrice.subtract(bestPrice).abs().multiply(matchedOtherAmount).setScale(8, RoundingMode.DOWN); + BigDecimal aliceSaving = maximalPrice.subtract(bestPrice).abs().multiply(matchedOtherAmount).setScale(8, RoundingMode.DOWN); // Alice TEST BigDecimal aliceCommitment = aliceOtherAmount.multiply(maximalPrice).setScale(8, RoundingMode.DOWN); - expectedBalance = initialBalances.get("alice").get(AssetUtils.testAssetId).subtract(aliceCommitment).add(testRefund); + expectedBalance = initialBalances.get("alice").get(AssetUtils.testAssetId).subtract(aliceCommitment).add(aliceSaving); AccountUtils.assertBalance(repository, "alice", AssetUtils.testAssetId, expectedBalance); // Alice OTHER @@ -517,7 +567,7 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = BigDecimal.ZERO; final BigDecimal bobReturn = BigDecimal.ZERO; - AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -550,7 +600,7 @@ public class NewTradingTests extends Common { final BigDecimal aliceReturn = BigDecimal.ZERO; final BigDecimal bobReturn = BigDecimal.ZERO; - AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, otherAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } } \ No newline at end of file diff --git a/src/test/java/org/qora/test/assets/OldTradingTests.java b/src/test/java/org/qora/test/assets/OldTradingTests.java index a2449d60..a6fc8111 100644 --- a/src/test/java/org/qora/test/assets/OldTradingTests.java +++ b/src/test/java/org/qora/test/assets/OldTradingTests.java @@ -66,7 +66,7 @@ public class OldTradingTests extends Common { final BigDecimal asset112Matched = new BigDecimal("1000").setScale(8); final BigDecimal asset113Matched = new BigDecimal("1000").setScale(8); - AssetUtils.genericTradeTest(asset113Id, asset112Id, asset113Amount, asset112Price, asset112Amount, asset113Price, asset113Amount, asset112Amount, asset112Matched, asset113Matched); + AssetUtils.genericTradeTest(asset113Id, asset112Id, asset113Amount, asset112Price, asset112Amount, asset113Price, asset113Amount, asset112Amount, asset112Matched, asset113Matched, BigDecimal.ZERO); // Further trade final BigDecimal asset113Amount2 = new BigDecimal("986").setScale(8); @@ -128,7 +128,7 @@ public class OldTradingTests extends Common { final BigDecimal aliceReturn = new BigDecimal("1.99999992").setScale(8); final BigDecimal bobReturn = new BigDecimal("24.00000000").setScale(8); - AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -168,7 +168,7 @@ public class OldTradingTests extends Common { final BigDecimal aliceReturn = new BigDecimal("1.99999985").setScale(8); final BigDecimal bobReturn = new BigDecimal("1.17647050").setScale(8); - AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(AssetUtils.testAssetId, Asset.QORA, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } /** @@ -209,7 +209,7 @@ public class OldTradingTests extends Common { final BigDecimal aliceReturn = new BigDecimal("73250.99992674").setScale(8); final BigDecimal bobReturn = new BigDecimal("81389.99991860").setScale(8); - AssetUtils.genericTradeTest(Asset.QORA, AssetUtils.testAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn); + AssetUtils.genericTradeTest(Asset.QORA, AssetUtils.testAssetId, aliceAmount, alicePrice, bobAmount, bobPrice, aliceCommitment, bobCommitment, aliceReturn, bobReturn, BigDecimal.ZERO); } } \ No newline at end of file diff --git a/src/test/java/org/qora/test/common/AssetUtils.java b/src/test/java/org/qora/test/common/AssetUtils.java index a6f99a02..c16234af 100644 --- a/src/test/java/org/qora/test/common/AssetUtils.java +++ b/src/test/java/org/qora/test/common/AssetUtils.java @@ -3,7 +3,6 @@ package org.qora.test.common; import static org.junit.Assert.assertNotNull; import java.math.BigDecimal; -import java.math.RoundingMode; import java.util.Map; import org.qora.account.PrivateKeyAccount; @@ -66,7 +65,7 @@ public class AssetUtils { BigDecimal aliceAmount, BigDecimal alicePrice, BigDecimal bobAmount, BigDecimal bobPrice, BigDecimal aliceCommitment, BigDecimal bobCommitment, - BigDecimal aliceReturn, BigDecimal bobReturn) throws DataException { + BigDecimal aliceReturn, BigDecimal bobReturn, BigDecimal bobSaving) throws DataException { try (Repository repository = RepositoryManager.getRepository()) { Map> initialBalances = AccountUtils.getBalances(repository, haveAssetId, wantAssetId); @@ -92,11 +91,6 @@ public class AssetUtils { AccountUtils.assertBalance(repository, "alice", wantAssetId, expectedBalance); // Bob selling want asset - // If bobReturn is non-zero then we expect trade to go through - // so we can calculate potential saving to Bob due to price improvement ('new' pricing only) - BigDecimal bobSaving = BigDecimal.ZERO; - if (isNewPricing && bobReturn.compareTo(BigDecimal.ZERO) > 0) - bobSaving = alicePrice.subtract(bobPrice).abs().multiply(bobReturn).setScale(8, RoundingMode.DOWN); expectedBalance = initialBalances.get("bob").get(wantAssetId).subtract(bobCommitment).add(bobSaving); AccountUtils.assertBalance(repository, "bob", wantAssetId, expectedBalance);