Improve text #158

ManfredKarrer · 2018-08-16T17:45:36Z

No description provided.

sqrrm

Please fix getAdjustedAmount function, either with explanation on what it does if current behavior is expected or rounding of return data for all cases to match expectation.

sqrrm · 2018-08-18T20:26:39Z

src/main/java/bisq/core/offer/OfferUtil.java

-            rounded = Math.min(maxTradeLimit, rounded - amountByVolumeRoundedToFactor.value);
+            long reduced = rounded - amountByVolumeRoundedToFactor.value;
+            if (reduced > Restrictions.getMinTradeAmount().value)
+                rounded = Math.min(maxTradeLimit, reduced);


If the amount is more than factor more than maxTradeLimit the returned value will be maxTradeLimit which is not rounded. This is later rounded down to factor when this function is called again from MutableOfferDataModel:702 so everything works in when running the code.

This is somewhat confusing while reading the code though, I expect getAdjustedAmount to return a rounded amount for every input value.

sqrrm · 2018-08-18T21:10:39Z

src/main/java/bisq/core/offer/OfferUtil.java

@@ -167,20 +168,28 @@ public static Coin getAdjustedAmountForHalCash(Coin amount, Price price, long ma
    private static Coin getAdjustedAmount(Coin amount, Price price, long maxTradeLimit, String currencyCode, int factor) {
        // Amount must result in a volume of min factor units of the fiat currency, e.g. 1 EUR or 10 EUR in case of Halcash
        Volume volumeRoundedToFactor = Volume.parse(String.valueOf(factor), currencyCode);
+        if (volumeRoundedToFactor.getValue() <= 0)
+            return Coin.ZERO;
+
        Coin amountByVolumeRoundedToFactor = price.getAmountByVolume(volumeRoundedToFactor);
        // We set min amount so it has a volume of 10 EUR
        if (amount.compareTo(amountByVolumeRoundedToFactor) < 0)
            amount = amountByVolumeRoundedToFactor;

        // We adjust the amount so that the volume is a multiple of 10 EUR


Refers to a multiple of factor

sqrrm · 2018-08-18T21:11:03Z

src/main/java/bisq/core/offer/OfferUtil.java

@@ -167,20 +168,28 @@ public static Coin getAdjustedAmountForHalCash(Coin amount, Price price, long ma
    private static Coin getAdjustedAmount(Coin amount, Price price, long maxTradeLimit, String currencyCode, int factor) {
        // Amount must result in a volume of min factor units of the fiat currency, e.g. 1 EUR or 10 EUR in case of Halcash
        Volume volumeRoundedToFactor = Volume.parse(String.valueOf(factor), currencyCode);
+        if (volumeRoundedToFactor.getValue() <= 0)
+            return Coin.ZERO;
+
        Coin amountByVolumeRoundedToFactor = price.getAmountByVolume(volumeRoundedToFactor);
        // We set min amount so it has a volume of 10 EUR


These comments refer to a multiple of factor

ripcurlx

utACK

ripcurlx · 2018-08-20T08:22:08Z

@ManfredKarrer There is no need to make any changes in translation files except English as it will be updated anyways from Transifex when I sync the translations for a release.

- Move getTxFeeForWithdrawalPerByte to another section - Update other callers of getNumTxOutputsForAddress(address) with checks to 0

chirhonul

I left some initial comments.

chirhonul · 2018-08-20T11:31:55Z

src/main/java/bisq/core/btc/wallet/BsqWalletService.java

@@ -634,6 +634,9 @@ public Transaction getPreparedUnlockTx(TxOutput lockedTxOutput) throws AddressFo
    }

    public Address getUnusedAddress() {
-        return wallet.freshReceiveAddress();
+        return wallet.getIssuedReceiveAddresses().stream()
+                .filter(address -> getNumTxOutputsForAddress(address) == 0)


Based on our conversation, I understand that before the commit 4aaad1f, the getUnusedAddress() method generated a fresh set of addresses on each call, whereas it after the commit only generates fresh addresses if necessary.

(Optional) I feel like the commit ideally should mention this (currently it says only Fix getUnusedAddress method), and that ideally there should be some tests that cover the behavior before and after the change.

Apart from that, I think that the filtering on getNumTxOutputsForAddress(address) == 0 predicate could be more clear in its semantics if e.g a method for Address like .hasBeenUsed() returned true if there were any tx outputs for that Address. Alternatively, we could document the semantics of what we're doing with a comment here.

Edit: Ah, I see now that Address is a bitcoinj class, and it doesn't know about the tx outputs.. adding a utility method in BsqWalletService is probably best.

chirhonul

utACK 8010863.

I left some comments with optional suggestions that you may want to review when you have the chance, but I think this is safe to go in with its current state.

chirhonul · 2018-08-20T11:54:49Z

src/main/java/bisq/core/offer/OfferUtil.java

+     * @return The adjusted amount
+     */
+    @VisibleForTesting
+    static Coin getAdjustedAmount(Coin amount, Price price, long maxTradeLimit, int factor) {


This method is fairly complex! It would be great if there were tests for it..

I have been working on that (and tests for some of the methods in Coin) which I will send out for review after this change goes in. I think I understand the behavior of the method, and that it's going to do what you want under normal conditions in terms of making user-provided values from the UI match the expected constraints.

For future reference, those tests I mentioned above were merged in #160.

chirhonul · 2018-08-20T11:55:41Z

src/main/resources/i18n/displayStrings.properties

@@ -2155,7 +2156,7 @@ payment.f2f.info='Face to Face' trades have different rules and come with differ
  happened at the meeting. In such cases the BTC funds might get locked up forever or until the trading peers come to \
  an agreement.\n\n\
  To be sure you fully understand the differences with 'Face to Face' trades please read the instructions and \
-  recommendations at: 'https://docs.bisq.network/#f2f'
+  recommendations at: 'https://docs.bisq.network/trading-rules.html#f2f-trading'


(Optional) Maybe the URL could be removed from the property, since it's not being translated? Then we could append it to the translated property string in code.

ManfredKarrer added 3 commits August 16, 2018 18:17

Merge

642ff9e

Merged

77852a7

Improve text

413fabf

ManfredKarrer requested review from ripcurlx and sqrrm August 16, 2018 17:45

ManfredKarrer added 2 commits August 17, 2018 11:22

Merged master

24f2c43

Add checks to not have zero or negative volume and amount

9bbe5a6

ManfredKarrer mentioned this pull request Aug 17, 2018

Testing: Stack trace from trying to create order bisq-network/bisq#1632

Closed

ManfredKarrer added 3 commits August 17, 2018 13:07

Fix getUnusedAddress method

4aaad1f

Fix proposal list management

f6667c2

Change link to F2F docs page

25c0f7b

sqrrm suggested changes Aug 18, 2018

View reviewed changes

ManfredKarrer added 2 commits August 19, 2018 12:30

Update getAdjustedFiatVolume and getAdjustedAmount methods

7c4d333

Update getAdjustedFiatVolume and getAdjustedAmount methods

ec6f335

ripcurlx approved these changes Aug 20, 2018

View reviewed changes

Adds isAddressUnused method to WalletService

8010863

- Move getTxFeeForWithdrawalPerByte to another section - Update other callers of getNumTxOutputsForAddress(address) with checks to 0

chirhonul reviewed Aug 20, 2018

View reviewed changes

chirhonul approved these changes Aug 20, 2018

View reviewed changes

ManfredKarrer merged commit b468197 into bisq-network:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve text #158

Improve text #158

ManfredKarrer commented Aug 16, 2018

sqrrm left a comment

sqrrm Aug 18, 2018

sqrrm Aug 18, 2018

sqrrm Aug 18, 2018

ripcurlx left a comment

ripcurlx commented Aug 20, 2018

chirhonul left a comment

chirhonul Aug 20, 2018

chirhonul left a comment •

edited

chirhonul Aug 20, 2018

chirhonul Aug 21, 2018

chirhonul Aug 20, 2018

Improve text #158

Improve text #158

Conversation

ManfredKarrer commented Aug 16, 2018

sqrrm left a comment

Choose a reason for hiding this comment

sqrrm Aug 18, 2018

Choose a reason for hiding this comment

sqrrm Aug 18, 2018

Choose a reason for hiding this comment

sqrrm Aug 18, 2018

Choose a reason for hiding this comment

ripcurlx left a comment

Choose a reason for hiding this comment

ripcurlx commented Aug 20, 2018

chirhonul left a comment

Choose a reason for hiding this comment

chirhonul Aug 20, 2018

Choose a reason for hiding this comment

chirhonul left a comment • edited

Choose a reason for hiding this comment

chirhonul Aug 20, 2018

Choose a reason for hiding this comment

chirhonul Aug 21, 2018

Choose a reason for hiding this comment

chirhonul Aug 20, 2018

Choose a reason for hiding this comment

chirhonul left a comment •

edited