<p><b>@chimp1984</b> commented on this pull request.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345002797">core/src/main/java/bisq/core/exceptions/NotFoundException.java</a>:</p>
<pre style='color:#555'>> + * 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 {
</pre>
<p>I think the class name should be more specific. What is not found?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345003008">core/src/main/java/bisq/core/payment/validation/AltCoinAddressValidator.java</a>:</p>
<pre style='color:#555'>> @Slf4j
-public final class AltCoinAddressValidator extends InputValidator {
+public class AltCoinAddressValidator extends InputValidator {
</pre>
<p>If that was removed for testing consider to use the mockito-extensions like in: <a class="commit-link" href="https://github.com/bisq-network/bisq/commit/6d983dec8d37a038a6e9f1cc024f941050632140#diff-1bf51dc85c38e8dd66e85a3d5286d1e0"><tt>6d983de</tt>#diff-1bf51dc85c38e8dd66e85a3d5286d1e0</a></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345003049">core/src/main/java/bisq/core/user/Preferences.java</a>:</p>
<pre style='color:#555'>> @@ -78,7 +78,7 @@
@Slf4j
@Singleton
-public final class Preferences implements PersistedDataHost, BridgeAddressProvider {
+public class Preferences implements PersistedDataHost, BridgeAddressProvider {
</pre>
<p>If 'final' was removed for testing consider to use the mockito-extensions like in: <a class="commit-link" href="https://github.com/bisq-network/bisq/commit/6d983dec8d37a038a6e9f1cc024f941050632140#diff-1bf51dc85c38e8dd66e85a3d5286d1e0"><tt>6d983de</tt>#diff-1bf51dc85c38e8dd66e85a3d5286d1e0</a></p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345006129">core/src/main/java/bisq/core/payment/PaymentAccountManager.java</a>:</p>
<pre style='color:#555'>> +
+ }
+ 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));
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345006734">desktop/src/main/java/bisq/desktop/main/account/content/altcoinaccounts/AltCoinAccountsDataModel.java</a>:</p>
<pre style='color:#555'>> }
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());
</pre>
<p>Why not pass the paymentAccount and avoid the extra lookup and null check in paymentAccountManager.removePaymentAccount?</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/3586#discussion_r345007219">desktop/src/main/java/bisq/desktop/main/account/content/altcoinaccounts/AltCoinAccountsDataModel.java</a>:</p>
<pre style='color:#555'>> - 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) {
</pre>
<p>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.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/3586?email_source=notifications&email_token=AJFFTNR4URUCALKUGCNNK2TQTIR47A5CNFSM4JLIFYS2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLFLV6Y#pullrequestreview-315276027">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNUYJPCKQCTLFM54FWLQTIR47ANCNFSM4JLIFYSQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNSZNDJBED5PJFMOOFTQTIR47A5CNFSM4JLIFYS2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLFLV6Y.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/3586?email_source=notifications\u0026email_token=AJFFTNR4URUCALKUGCNNK2TQTIR47A5CNFSM4JLIFYS2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLFLV6Y#pullrequestreview-315276027",
"url": "https://github.com/bisq-network/bisq/pull/3586?email_source=notifications\u0026email_token=AJFFTNR4URUCALKUGCNNK2TQTIR47A5CNFSM4JLIFYS2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCLFLV6Y#pullrequestreview-315276027",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>