[bisq-network/bisq] [WIP] Improve navigation structure (PR #6135)

[xyzmaker123]

@xyzmaker123 commented on this pull request.

I reviewed only small part of this pull request for now. Let me know what do you think about proposed changes. I'm okay if you disagree with them, however I will be very pleased if they will be introduced :)

> @@ -502,6 +506,114 @@ public static void setBaseCurrencyCode(String baseCurrencyCode) {
         return currencies;
     }
 
+    public static List<TradeCurrency> getAllInteracETransferIdCurrencies() {

What about creating static field with supported currencies in `InteracETransferAccount` instead:
```
//...
public final class InteracETransferAccount extends PaymentAccount {
    public static final List<TradeCurrency> SUPPORTED_CURRENCIES = List.of(new FiatCurrency("CAD"))
//...
``` 
also for other account types (Strike, TikkieId etc.). 

As a alternative add `SUPPORTED_CURRENCIES` into `PaymentMethod` instead of `PaymentAccount`

> @@ -106,6 +113,114 @@ public static boolean isPaymentAccountValidForOffer(Offer offer, PaymentAccount
         return acceptedCountryCodes;
     }
 
+    public static List<TradeCurrency> getTradeCurrencies(PaymentMethod paymentMethod) {

I think this method breaks [open-closed principle](https://medium.com/@alexandre.malavasi/why-is-the-open-closed-principle-so-important-bed2f2a0d4c7). Refactoring steps suggestion:

1) make `PaymentMethod` abstract class, and create dedicated classes, f.e.:
```
public class SepaPaymentMethod extends PaymentMethod {
    public SepaPaymentMethod () {
        super("SEPA", 6 * DAY, DEFAULT_TRADE_LIMIT_HIGH_RISK);
    }
}
``` 


2) use created classes in constructors of every `PaymentAccount`, f.e.:
```
public SepaAccount() {
    super(new SepaPaymentMethod());
}
```

3. To generate list of supported payment methods use similar approach like in `AssetRegistry`:
```
static {
    for (PaymentAccount account : ServiceLoader.load(PaymentAccount.class)) {
        registeredAccounts.add(account);
    }
}
    
public Stream<PaymentMethod> getAvailablePaymentMethods() {
    return registeredAccounts.stream().map(account -> account.getPaymentMethod());
}
```

* maybe instead loading `PaymentAccount` instances we should load `PaymentMethod` instances - see point 5

4) Add `List<TradeCurrency> getSupportedCurrencies()` into `PaymentMethod`

5) `PaymentMethod` <-> `PaymentAccount` seems to be 1:1 relationship. I think it could be used to additional refactoring. f.e. to replace `PaymentAccountFactory` (which also breaks open-closed principle) with `paymentMethod.createAccountInstance()` 



WDYT ? It could be big refactoring, I didn't analyzed how many changes in codebase it could induce.




-- 
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/6135#pullrequestreview-934824416
You are receiving this because you are subscribed to this thread.

Message ID: <bisq-network/bisq/pull/6135/review/934824416 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20220407/25bbfc0c/attachment.htm>