[bisq-network/bisq] Refactor desktop's BsqSendView 'sendbsq', share with api (#4801)

[sqrrm]

@sqrrm commented on this pull request.



> @@ -331,6 +342,25 @@ private void verifyEncryptedWalletIsUnlocked() {
             throw new IllegalStateException("wallet is locked");
     }
 
+    // Returns a LegacyAddress for the string, or a RuntimeException if invalid.
+    private LegacyAddress getValidBsqLegacyAddress(String address) {
+        try {
+            return bsqFormatter.getAddressFromBsqAddress(address);
+        } catch (Throwable t) {
+            log.error("", t);
+            throw new IllegalStateException(format("%s is not a valid bsq address", address));
+        }
+    }
+
+    // Returns a Coin for the double amount, or a RuntimeException if invalid.
+    private Coin getValidBsqTransferAmount(double amount) {

Depending on what you consider a valid amount in this context, you might want to consider any dust amount as invalid since it's too small to send. The naming of this method indicates dust amounts should be invalid.

It's possible to burn less than dust since the burnt satoshis are added to the mining fee and the BSQ change lowered by the burnt amount. That wouldn't be considered a transfer though.

> +                            Transaction signedTx) {
+        this.receiverAddress = receiverAddress;
+        this.receiverAmount = receiverAmount;
+        this.preparedSendTx = preparedSendTx;
+        this.txWithBtcFee = txWithBtcFee;
+        this.signedTx = signedTx;
+        this.miningFee = signedTx.getFee();
+        this.txSize = signedTx.bitcoinSerialize().length;
+        this.txType = TxType.TRANSFER_BSQ;
+    }
+
+    public String getReceiverAddressAsString() {
+        return receiverAddress.toString();
+    }
+
+    public double getMiningFeeInSatoshisPerByte() {

Why use double for satoshis? Long seems better.

> @@ -354,7 +352,7 @@ private LegacyAddress getValidBsqLegacyAddress(String address) {
 
     // Returns a Coin for the double amount, or a RuntimeException if invalid.
     private Coin getValidBsqTransferAmount(double amount) {
-        Coin amountAsCoin = parseToCoin(new BigDecimal(amount).toString(), bsqFormatter);
+        Coin amountAsCoin = parseToCoin(Double.toString(amount), bsqFormatter);

What is the reason to use `double` for any monetary values? In general I have never seen it done and for anything that's dealing with bitcoins I would use `long` and count satoshis.

-- 
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/pull/4801#pullrequestreview-540912524
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201201/4a42bb6c/attachment.htm>