[bisq-network/bisq-core] Improve text (#158)

[chirhonul]

chirhonul commented on this pull request.

I left some initial comments.

> @@ -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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq-core/pull/158#pullrequestreview-147604269
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20180820/f536843d/attachment-0001.html>