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

[chimp1984]

chimp1984 commented on this pull request.



> + * under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or (at
+ * your option) any later version.
+ *
+ * Bisq is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public
+ * License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with Bisq. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package bisq.core.exceptions;
+
+public class NotFoundException extends RuntimeException {

I think the class name should be more specific. What is not found?

>  @Slf4j
-public final class AltCoinAddressValidator extends InputValidator {
+public class AltCoinAddressValidator extends InputValidator {

If that was removed for testing consider to use the mockito-extensions like in: https://github.com/bisq-network/bisq/pull/3587/commits/6d983dec8d37a038a6e9f1cc024f941050632140#diff-1bf51dc85c38e8dd66e85a3d5286d1e0 

> @@ -78,7 +78,7 @@
 
 @Slf4j
 @Singleton
-public final class Preferences implements PersistedDataHost, BridgeAddressProvider {
+public class Preferences implements PersistedDataHost, BridgeAddressProvider {

If 'final' was removed for testing consider to use the mockito-extensions like in: https://github.com/bisq-network/bisq/pull/3587/commits/6d983dec8d37a038a6e9f1cc024f941050632140#diff-1bf51dc85c38e8dd66e85a3d5286d1e0 

> +
+            }
+            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 prefer to use exceptions only in exceptional cases not in valid use case (user tries to remove account but app does not allow it as its used in trades or offers). Maybe best to refactor the check out to 'canRemove()' method and only call the method if it is allowed to remove.  

>      }
 
     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());

Why not pass the paymentAccount and avoid the extra lookup and null check in paymentAccountManager.removePaymentAccount? 

> -            if (singleTradeCurrency instanceof FiatCurrency)
-                preferences.addFiatCurrency((FiatCurrency) singleTradeCurrency);
-            else
-                preferences.addCryptoCurrency((CryptoCurrency) singleTradeCurrency);
-        } else if (tradeCurrencies != null && !tradeCurrencies.isEmpty()) {
-            tradeCurrencies.stream().forEach(tradeCurrency -> {
-                if (tradeCurrency instanceof FiatCurrency)
-                    preferences.addFiatCurrency((FiatCurrency) tradeCurrency);
-                else
-                    preferences.addCryptoCurrency((CryptoCurrency) tradeCurrency);
-            });
-        }
-
-        if (!(paymentAccount instanceof AssetAccount))
-            accountAgeWitnessService.publishMyAccountAgeWitness(paymentAccount.getPaymentAccountPayload());
+        paymentAccountManager.addPaymentAccount(paymentAccount);
     }
 
     public boolean onDeleteAccount(PaymentAccount paymentAccount) {

I think we should change the API here to not return a boolean but to add a 'canRemove()' method at the caller and only call remove if that returns true. 

-- 
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#pullrequestreview-315276027
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191111/2401daf7/attachment.html>