Improve navigation structure #6135

ripcurlx · 2022-04-06T16:06:14Z

Based on #6092 this is the PR that implements all required changes.
I hope I have covered everything, but it is a lot of testing.

Anyone who wants to give it a shot and provide feedback is more than welcome.
This PR also includes improvements in other areas as well (e.g. only Payment methods are shown in filter for selected currency) and many more.

ghost

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 :)

ghost · 2022-04-07T10:14:47Z

core/src/main/java/bisq/core/locale/CurrencyUtil.java

@@ -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

Yes, I agree that it would make more sense to move this information closer to the Domain. I didn't spend too much time yet on bigger refactorings to prevent too many unexpected side effects by changing too much. But yes, I think moving it the PaymentMethod would make sense. Although it is a 1:1 relationship all the time AFAIK, but I think it would be a better fit for the PaymentMethod, which is used by the Payment Account.

After looking at the code, I revert my opinion and also think adding it to the concrete account implementation is the better way to do this.

ghost · 2022-04-07T11:06:44Z

core/src/main/java/bisq/core/payment/PaymentAccountUtil.java

@@ -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. Refactoring steps suggestion:

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

use created classes in constructors of every PaymentAccount, f.e.:

public SepaAccount() { super(new SepaPaymentMethod()); }

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

Add List<TradeCurrency> getSupportedCurrencies() into PaymentMethod

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.

I agree in general with your comment, but this will be a hell of a refactoring and testing. I'd keep it for a separate effort to not completely blow this PR out of proportion.

…ifferent assets Still missing correct create and take offer handling

if currency does not support previous selected payment method in a different currency.

and fix lots of minor issues

ripcurlx · 2022-04-08T16:05:09Z

take-offer-to-sell-bsq.mov

create-offer-to-buy-xmr.mov

basic-navigation-walk-through.mov

ghost · 2022-04-08T21:01:24Z

Checked it out, looks good! (Some niggles need ironing out).

Creating BSQ swap offer it somehow is mixing BSQ Swap with Altcoin, because the offer that gets created has a security deposit Tx and gets displayed like an altcoin offer even though it says "BSQ Swap" as offer type. When that offer gets taken the trade fails with "hash of payment account is invalid". This is a clean build from your current branch, run on a fresh regtest env with no altcoin accounts.
Clicking on an offer in Market View does not always take me to the market containing that offer. e.g. There is only one offer in the market a Buy BSQ offer. In Market View click on the offer. It takes me to Sell BTC for Fiat, and has BSQ selected in the currency drop down. No offer is shown.

ripcurlx · 2022-04-11T07:28:54Z

Checked it out, looks good! (Some niggles need ironing out).

Creating BSQ swap offer it somehow is mixing BSQ Swap with Altcoin, because the offer that gets created has a security deposit Tx and gets displayed like an altcoin offer even though it says "BSQ Swap" as offer type. When that offer gets taken the trade fails with "hash of payment account is invalid". This is a clean build from your current branch, run on a fresh regtest env with no altcoin accounts.

Clicking on an offer in Market View does not always take me to the market containing that offer. e.g. There is only one offer in the market a Buy BSQ offer. In Market View click on the offer. It takes me to Sell BTC for Fiat, and has BSQ selected in the currency drop down. No offer is shown.

Thanks for testing - I'll look at the BSQ Swap issue you experienced in more detail. It did work for me, so it is kind of weird. I'll post an update today as soon as I'm through with my testing.

@Market View: It can be that there is an issue. I only fixed the behavior on the buy/sell buttons, but not on the triggers in each row. Quite a bit of changes this change caused in the end 😅

ripcurlx · 2022-04-11T08:37:00Z

@jmacxx I just tested it again to create a Buy BSQ and a Sell BSQ offer using BSQ Swaps and it worked as expected for me. Is there a specific flow you used to generate this issue?

The only case where this could happen IMO is that BSQ Swap didn't switch to the BsqSwapCreateOfferView in your case using the regular CreateOfferView.

ripcurlx · 2022-04-11T08:39:50Z

I'll try it with a fresh account without any Altcoin accounts next. Maybe that causes this weird behavior.

Otherwise it would display the old 'Buy BTC' and 'Sell BTC' navigation

ripcurlx · 2022-04-11T08:48:19Z

Ok - now I'm able to reproduce the issue. It is as I expected caused with a fresh account with only one BSQ account.

ripcurlx · 2022-04-11T09:06:01Z

@jmacxx Just fixed your issue with 1712f74

ripcurlx · 2022-04-11T10:18:08Z

Adapted the amount and volume description so it matches the fiat case.

Fiat

Altcoin

ripcurlx · 2022-04-20T09:32:09Z

I'm probably already quite blind by all these changes, but I haven't found anything obvious anymore comparing it with v1.8.4 on Mainnet. I'll proceed with the one open refactoring and might do some performance optimizations and clean-ups (and create some new bugs of course 🤪 )

ghost · 2022-04-20T10:11:57Z

Found one "not obvious" kind of issue - preferred currency set to crypto has impact to fiat logic:

go to Settings -> Preferences
switch preferred currency to ETH, XMR, BSQ
go to Buy->Bitcoin or Sell->Bitcoin tab
see strange behavior (crypto offers visible on list, not possible to create offer)

Sorry for being pain 🥺

ripcurlx · 2022-04-20T19:07:36Z

Found one "not obvious" kind of issue - preferred currency set to crypto has impact to fiat logic:

go to Settings -> Preferences

switch preferred currency to ETH, XMR, BSQ

go to Buy->Bitcoin or Sell->Bitcoin tab

see strange behavior (crypto offers visible on list, not possible to create offer)

Sorry for being pain 🥺

No worries! Better you find the bug than users. Should be fixed now.

ripcurlx · 2022-04-20T19:42:56Z

After doing some Mainnet tests I found some more edge cases with the preferred currency, which needs some minor improvements - will do it tomorrow.

…older versions

ripcurlx · 2022-04-21T08:24:58Z

I think it is now in a state ready for a final review testing @jmacxx @xyzmaker123

There is one UX problem IMO opinion that needs to get solved which we have already for a long time and I'm not 100% sure how we want to solve it best.

Let's assume you want to buy XMR.

But all those offers doesn't fit your need and you want to create an own offer to buy XMR.

So what would you expect now? ATM for a new user we close the popup show a notification that you can manage your offers in portfolio. If you close this notification and/or are an existing user it will bring you back to the list of XMR offers that can be bought. But of course there is not your offer visible as it is under Sell > XMR as you created an offer to buy XMR.

Shall we navigate the user instead to the buy XMR offers under Sell > XMR instead?

@UX-P What is your take on this?

UX-P · 2022-04-21T12:27:54Z

@ripcurlx looks good!
The user should land on the Portfolio screen 'My open offers'. This provides them with immediate access to review and undo their action.

Also, in regards to the create offer screen, instead of 'choose trading account' could the header text read ' Create offer' so that it is clear to the user they are now creating an offer.

ghost · 2022-04-21T12:32:53Z

ACK - I created a few offers on regtest, took them etc. and everything worked okay. Also tested scenarios that has issues before, and now everything is okay. Good job!

ripcurlx · 2022-04-21T12:33:03Z

Also, in regards to the create offer screen, instead of 'choose trading account' could the header text read ' Create offer' so that it is clear to the user they are now creating an offer.

Sure - I can do the same for "Take offer" as well. I'll adapt the navigational behavior as well.

ripcurlx · 2022-04-22T08:03:10Z

@UX-P It will now look like this:

If it is a crypto currency and not a fiat currency

ripcurlx · 2022-04-22T08:44:19Z

The user can now adapt the forth tab by setting a crypto currency as his preferred currency. If no crypto currency is set it will default to XMR.

Example with ETH

XMR as default Altcoin

bisq-github-admin-3

ACK - based on #6135 (comment)

ghost reviewed Apr 7, 2022

View reviewed changes

ripcurlx added 7 commits April 7, 2022 20:48

Adapt navigational structure and offerbook handling to buy and sell d…

98f355d

…ifferent assets Still missing correct create and take offer handling

Adapt offer book chart view to new navigational structure

875e5f4

Add payment account filtering for remaining payment methods

5781e3b

Add missing payment account filtering

810a29b

Show all payment methods

e3ceb80

if currency does not support previous selected payment method in a different currency.

Adapt take offer process to new navigation structure

924e92a

Adapt create offer process to new navigation structure

a8189d7

ripcurlx force-pushed the improve-navigation-structure branch from eb010e7 to a8189d7 Compare April 7, 2022 18:49

ripcurlx added 3 commits April 7, 2022 21:13

Fix rebase errors with master

497158a

Adapt navigation labels back to currency code

33725c2

and fix lots of minor issues

Fix BSQ swap labels

b70d102

ripcurlx added 2 commits April 11, 2022 10:29

Move OfferViewUtil

98285da

Fix navigation to specific offer book view

b734420

Properly rename main navigation

5b67766

Otherwise it would display the old 'Buy BTC' and 'Sell BTC' navigation

Use BSQ Swap as default when BSQ offer is created

1712f74

ripcurlx added 2 commits April 11, 2022 11:32

Adapt amount and volume description for take offer

492ff35

Adapt amount and volume description for create offer

63ea054

ripcurlx closed this Apr 11, 2022

ripcurlx reopened this Apr 11, 2022

ripcurlx added 2 commits April 11, 2022 12:31

Use Offer.isFiat helper method

9a3ed33

Remove close button duplication in DetailsWindows

542cc4d

ripcurlx added 2 commits April 20, 2022 17:57

Move supported currencies into payment accounts

108255b

Fix handling of preferred currency

e43a8ae

ripcurlx added 2 commits April 20, 2022 21:57

Add safeguard for already stored crypto currencies in preferences by …

0894037

…older versions

Improve help icon

89926d1

ripcurlx force-pushed the improve-navigation-structure branch from de9be4c to 89926d1 Compare April 20, 2022 20:34

Unify tab handling

3ce374e

Fix Codacy complaints

2233f73

ripcurlx mentioned this pull request Apr 21, 2022

Edit BSQ swap offer #6116

Closed

ripcurlx added 2 commits April 22, 2022 09:37

Add create and take offer title

4ce861a

Switch to open offers after creating a new offer

4447435

ripcurlx force-pushed the improve-navigation-structure branch from 389aeb9 to 4447435 Compare April 22, 2022 08:01

Set preferred currency as top altcoin

7ad2dd0

If it is a crypto currency and not a fiat currency

bisq-github-admin-3 approved these changes Apr 22, 2022

View reviewed changes

ripcurlx added this to the v1.9.0 milestone Apr 22, 2022

ripcurlx merged commit 0466f41 into bisq-network:master Apr 22, 2022

ghost mentioned this pull request Apr 24, 2022

When creating an offer it does not recognize the account currency #6157

Closed

ripcurlx mentioned this pull request Apr 26, 2022

For Cycle 35 bisq-network/compensation#1060

Closed

ghost mentioned this pull request May 11, 2022

For Cycle 36 bisq-network/compensation#1071

Closed

This was referenced Jun 26, 2022

For Cycle 37 bisq-network/compensation#1095

Closed

Correct wording of altcoin payto address label. #6266

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve navigation structure #6135

Improve navigation structure #6135

ripcurlx commented Apr 6, 2022

ghost left a comment

ghost Apr 7, 2022

ripcurlx Apr 7, 2022

ripcurlx Apr 7, 2022

ghost Apr 7, 2022

ripcurlx Apr 7, 2022

ripcurlx commented Apr 8, 2022

ghost commented Apr 8, 2022

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022 •

edited

Loading

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022 •

edited

Loading

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 20, 2022

ghost commented Apr 20, 2022

ripcurlx commented Apr 20, 2022

ripcurlx commented Apr 20, 2022

ripcurlx commented Apr 21, 2022

UX-P commented Apr 21, 2022

ghost commented Apr 21, 2022

ripcurlx commented Apr 21, 2022

ripcurlx commented Apr 22, 2022

ripcurlx commented Apr 22, 2022

bisq-github-admin-3 left a comment

Improve navigation structure #6135

Improve navigation structure #6135

Conversation

ripcurlx commented Apr 6, 2022

ghost left a comment

Choose a reason for hiding this comment

ghost Apr 7, 2022

Choose a reason for hiding this comment

ripcurlx Apr 7, 2022

Choose a reason for hiding this comment

ripcurlx Apr 7, 2022

Choose a reason for hiding this comment

ghost Apr 7, 2022

Choose a reason for hiding this comment

ripcurlx Apr 7, 2022

Choose a reason for hiding this comment

ripcurlx commented Apr 8, 2022

ghost commented Apr 8, 2022

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022 • edited Loading

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 11, 2022 • edited Loading

ripcurlx commented Apr 11, 2022

ripcurlx commented Apr 20, 2022

ghost commented Apr 20, 2022

ripcurlx commented Apr 20, 2022

ripcurlx commented Apr 20, 2022

ripcurlx commented Apr 21, 2022

UX-P commented Apr 21, 2022

ghost commented Apr 21, 2022

ripcurlx commented Apr 21, 2022

ripcurlx commented Apr 22, 2022

ripcurlx commented Apr 22, 2022

bisq-github-admin-3 left a comment

Choose a reason for hiding this comment

ripcurlx commented Apr 11, 2022 •

edited

Loading

ripcurlx commented Apr 11, 2022 •

edited

Loading