[bisq-network/bisq] Move Payment account creation and removal from UI to core (#3586)

[chimp1984]

chimp1984 commented on this pull request.



> +
+            }
+            accountAgeWitnessService.publishMyAccountAgeWitness(paymentAccount.getPaymentAccountPayload());
+        }
+        return paymentAccount;
+    }
+
+    public void removePaymentAccount(String id) {
+        PaymentAccount paymentAccount = user.getPaymentAccount(id);
+        if (paymentAccount == null) {
+            throw new NotFoundException(format("Payment account %s not found", id));
+        }
+        boolean isPaymentAccountUsed = openOfferManager.getObservableList().stream()
+                .anyMatch(openOffer -> id.equals(openOffer.getOffer().getMakerPaymentAccountId()));
+        if (isPaymentAccountUsed) {
+            throw new PaymentAccountInUseException(format("Payment account %s is used for open offer", id));

I disagree. I think the method removePaymentAccount should do what its name suggests and not throw an unchecked exception if the valid case happens that the user tries to remove an account but has an offer open using it. 
Bisq is using exceptions for exceptional cases not for flow control. I think we should stick with that patter and not introduce tons of exceptions for all kind of checks.

>      }
 
     public boolean onDeleteAccount(PaymentAccount paymentAccount) {
-        boolean isPaymentAccountUsed = openOfferManager.getObservableList().stream()
-                .filter(o -> o.getOffer().getMakerPaymentAccountId().equals(paymentAccount.getId()))
-                .findAny()
-                .isPresent();
-        isPaymentAccountUsed = isPaymentAccountUsed || tradeManager.getTradableList().stream()
-                .filter(t -> t.getOffer().getMakerPaymentAccountId().equals(paymentAccount.getId()) ||
-                        paymentAccount.getId().equals(t.getTakerPaymentAccountId()))
-                .findAny()
-                .isPresent();
-        if (!isPaymentAccountUsed)
-            user.removePaymentAccount(paymentAccount);
-        return isPaymentAccountUsed;
+        try {
+            paymentAccountManager.removePaymentAccount(paymentAccount.getId());

I prefer to have then 2 methods if you need that or just do the lookup in your client code.

-- 
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/3586#discussion_r347070743
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191115/2b6a6699/attachment.html>