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

[Łukasz Usarz]

lusarz requested changes on this pull request.



> + *
+ * 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;
+
+/**
+ * Copy of ValidationException from javax.validation:validation-api
+ */
+public class ValidationException extends RuntimeException {
+    public ValidationException(String message) {
+        super(message);
+    }
+
+    public ValidationException() {

Only first one constructor with `String message` as param is used anywhere in application. 

> + *
+ * Bisq is free software: you can redistribute it and/or modify it
+ * 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.payment;

I wonder if it's appropriate place for this Exception class. What about `bisq.core.payment.exceptions` ? 

Although I don't see consistency in project codebase - some exceptions are [inside](https://github.com/blabno/exchange/blob/2ad86abe9fc3ea802c95d7c25d1195eacb587d60/core/src/main/java/bisq/core/dao/exceptions/DaoDisabledException.java) `exceptions` package, others [no](https://github.com/blabno/exchange/blob/d55114e019fda469266dab50f63916289b2e71cb/core/src/main/java/bisq/core/support/dispute/DisputeAlreadyOpenException.java).

> +                                 TradeManager tradeManager,
+                                 User user) {
+        this.accountAgeWitnessService = accountAgeWitnessService;
+        this.altCoinAddressValidator = altCoinAddressValidator;
+        this.openOfferManager = openOfferManager;
+        this.preferences = preferences;
+        this.tradeManager = tradeManager;
+        this.user = user;
+    }
+
+    public PaymentAccount addPaymentAccount(PaymentAccount paymentAccount) {
+        if (paymentAccount instanceof CryptoCurrencyAccount) {
+            CryptoCurrencyAccount cryptoCurrencyAccount = (CryptoCurrencyAccount) paymentAccount;
+            TradeCurrency tradeCurrency = cryptoCurrencyAccount.getSingleTradeCurrency();
+            if (tradeCurrency == null) {
+                throw new ValidationException("CryptoCurrency account must have exactly one trade currency");

Shouldn't `cryptoCurrencyAccount.getSingleTradeCurrency()` throws exception instead returning `null` ?

I'm just curious your opinion - such a change would require changes in many places, and it's out of scope of current 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 see this exception is handled [here](https://github.com/blabno/exchange/blob/a98cf1b9e84ceca658692547a92b0ec45653460b/desktop/src/main/java/bisq/desktop/main/account/content/fiataccounts/FiatAccountsDataModel.java#L100-L107). Maybe `removePaymentAccount` method can return boolean directly ?

> +    }
+
+    public PaymentAccount addPaymentAccount(PaymentAccount paymentAccount) {
+        if (paymentAccount instanceof CryptoCurrencyAccount) {
+            CryptoCurrencyAccount cryptoCurrencyAccount = (CryptoCurrencyAccount) paymentAccount;
+            TradeCurrency tradeCurrency = cryptoCurrencyAccount.getSingleTradeCurrency();
+            if (tradeCurrency == null) {
+                throw new ValidationException("CryptoCurrency account must have exactly one trade currency");
+            }
+            altCoinAddressValidator.setCurrencyCode(tradeCurrency.getCode());
+            InputValidator.ValidationResult validationResult = altCoinAddressValidator.validate(cryptoCurrencyAccount.getAddress());
+            if (!validationResult.isValid) {
+                throw new ValidationException(validationResult.errorMessage);
+            }
+        }
+//        TODO we should validate payment account here as well

Is it out of scope of current pull request ?

>      final ObservableList<PaymentAccount> paymentAccounts = FXCollections.observableArrayList();
     private final SetChangeListener<PaymentAccount> setChangeListener;
     private final String accountsFileName = "FiatPaymentAccounts";
     private final PersistenceProtoResolver persistenceProtoResolver;
     private final CorruptedDatabaseFilesHandler corruptedDatabaseFilesHandler;
 
     @Inject
-    public FiatAccountsDataModel(User user,
+    public FiatAccountsDataModel(PaymentAccountManager paymentAccountManager, User user,

Seems like `FiatAccountsDataModel` and `AltCoinAccountsDataModel` are very similar, especially after changes in this pull request. Shouldn't we introduce some common abstraction for them ?

-- 
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-318185686
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191118/fbad7861/attachment-0001.html>